Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce new Bytes InputSource #55

Open
wants to merge 5 commits into
base: audio
Choose a base branch
from

Conversation

jcberentsen
Copy link

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)

Lazy ByteString deliver works. Cleanup still missing
This will allow the user of openInput to clean up used resources.
Currently all inputCleanups do nothing.
Only `openByteStringReader` needs this at the moment.
@jcberentsen
Copy link
Author

I took a stab at implementing cleanup for openByteStringReader

@acowley
Copy link
Owner

acowley commented May 5, 2020

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
Copy link
Author

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
Copy link
Contributor

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.

fmt = -1 will cause an assert (eventual abort)
We avoid that by using fail
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants