ffmpeg-light icon indicating copy to clipboard operation
ffmpeg-light copied to clipboard

Introduce new Bytes InputSource

Open jcberentsen opened this issue 4 years ago • 4 comments

Lazy ByteString deliver works. Cleanup still missing

This is working, but incomplete. I would appreciate some feedback on the direction, especially how to handle cleanup. I'm thinking of changing the API of openInput by also returning a cleanupInput action and combining with the existing cleanup in frame*Reader functions.

Also, any other feedback on what would make this mergeable would be nice.

Issue #54 (I added this on top of the audio branch, as I presumed this would be the most likely point of merge in the future, let me know if I should bring it over to master instead)

jcberentsen avatar Apr 27 '20 08:04 jcberentsen

I took a stab at implementing cleanup for openByteStringReader

jcberentsen avatar Apr 27 '20 10:04 jcberentsen

I was hoping the audio PR would land sooner, so I put off looking closely at this. I'm sorry for how that's delayed things.

All in all, this looks like a great addition. I have a couple questions:

  1. Could we refactor parts of this into a separate module? The parts you added look cohesive to me, and I don't love the idea of always adding things to Decode. I'm happy to hear arguments against having a separate module, even if it's just that you'd rather we do that as another change later. In that case, I'd still like to hear your thoughts.

  2. Could we avoid the explicit cleanup action part, perhaps with a Finalizer? This is just in the name of API simplicity. I can imagine that you might have a use-case where you really do want prompt cleanup of the input, but I'd like to keep simple uses of any input as simple as possible. AVFormatContext is a Ptr; would we have to make it a ForeignPtr to do this? How much pain would that cause?

acowley avatar May 05 '20 18:05 acowley

I was hoping the audio PR would land sooner, so I put off looking closely at this. I'm sorry for how that's delayed things.

No problem!

All in all, this looks like a great addition. I have a couple questions:

  1. Could we refactor parts of this into a separate module? The parts you added look cohesive to me, and I don't love the idea of always adding things to Decode. I'm happy to hear arguments against having a separate module, even if it's just that you'd rather we do that as another change later. In that case, I'd still like to hear your thoughts.

Sure, would it make sense to put this in a Decode.Bytes module? Or some other name?

  1. Could we avoid the explicit cleanup action part, perhaps with a Finalizer? This is just in the name of API simplicity. I can imagine that you might have a use-case where you really do want prompt cleanup of the input, but I'd like to keep simple uses of any input as simple as possible. AVFormatContext is a Ptr; would we have to make it a ForeignPtr to do this? How much pain would that cause?

Agree that a solution taking advantage of garbage collection would be good. I'm not too familiar with FFI stuff, so I'll investigate.

jcberentsen avatar May 07 '20 07:05 jcberentsen

I was hoping the audio PR would land sooner, so I put off looking closely at this. I'm sorry for how that's delayed things.

Sorry about that. Let me fire it up and try to get it polished again.

DiegoNolan avatar Mar 21 '21 22:03 DiegoNolan