search

acowley / ffmpeg-light

Minimal Haskell bindings to the FFmpeg library
BSD 3-Clause "New" or "Revised" License
67 stars 29 forks source link

Introduce new Bytes InputSource #55

Open jcberentsen opened 4 years ago

jcberentsen commented 4 years ago

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 commented 4 years ago

I took a stab at implementing cleanup for openByteStringReader

acowley commented 4 years ago

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?

jcberentsen commented 4 years ago

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.

DiegoNolan commented 3 years ago

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.