search

acowley / ffmpeg-light

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

[RFC] Audio Support #53

Closed DiegoNolan closed 3 years ago

DiegoNolan commented 4 years ago

Need some comments on this as it is a very large and backwards incompatible change.

Most importantly the frameWriter function. I have to return the AVCodecContext because as far as I can tell it is needed for resampling audio. Open to feedback. Still have some planned changes.

To test to audio demo

./audio some-video.mp4 audio.mp3

To extract the audio in the video to mp3

acowley commented 4 years ago

First, thank you! I really appreciate the obvious care you've taken over a wide expanse of code.

I am going to make another read through, but the only things that jumped out at me thus far were the potential benefit of documenting walkPtrs (we should use this for video formats, and attached cameras), and the remaining TODO in Transcode.hs.

As for breaking backwards compatibility, can you document those breakages for inclusion in the CHANGELOG?

DiegoNolan commented 4 years ago

Cool, i'll make those changes. I have another demo i want to add with sinusoidal waves, working on it now.

I've been thinking about a higher level API similar to the JuicyPixels one but I think we'll have to land on a solid design before I implement it.

DiegoNolan commented 4 years ago

Okay, I think it is a good. Spot the the muxing example seems to be working with both audio and video. I'm not sure I really like the API I've provided. I'll comment inline on the code so you can see what I mean.

acowley commented 4 years ago

Everything builds and tests for me, and, again, thank you for working so clean with such a big change. The notes I added are just minor questions, and I can address them after merging if you'd prefer. Let me know what you want, and we'll get this merged.

DiegoNolan commented 4 years ago

Cool, I'll let you merge. Before you merge the audio branch on to master I'll think some more about the resampler. I think we can make the API as clean as the image rescaler.

One thing I'll note when I was testing, VLC does not like anything but floating point samples as far as I can tell. I thought I had a bug for like 4 hours because it was messing up playing my mp3. When I used ffplay it sounded fine.

acowley commented 4 years ago

You'd like me to hold off on merging, then? I fixed up webcam support on Linux, and added the double backflip contortions necessary to support mjpeg to fix #52, but I'd like to land this PR first since it's so involved.

ETA: I made my changes atop your branch, so there are no conflicts. I can PR that commit to your fork's audio branch if that would be helpful, or you can merge it into your fork yourself (it's on the audio branch of this repo), or we can wait until you're happy with the audio API.

DiegoNolan commented 4 years ago

You can merge but I'm still not super sure of the API provided by frameWriter if you saw my concerns about it above. I'm not sure we should publish to hackage until we figure that out.

acowley commented 4 years ago

I guess we should have frameWriter return a record with meaningful field names as the tuple was already unwieldy. As for the floating point samples issue, I don't know anything about this, but if floating point samples are more broadly compatible, then can we default to that? If that's a hassle, then we document the limitation.

The only other thing that seems reasonable is to split off a frameWriterAudioVideo to simplify video-only usage (and perhaps a frameWriterAudio and a frameWriterVideo where we make frameWriter alias to the latter for the sake of compatibility). We can either return separate, simple record types from frameWriter and frameWriterAudioVideo, or stick a type family in there so we have a FrameWriter Video, FrameWriter Audio, and FrameWriter AV, and the type family maps the relevant fields to Void or the payload type depending on the type index. I'm fine with either approach.

DiegoNolan commented 4 years ago

If you want send my fork your webcam patch with my changes merged I'll try to make the final changes this weekend.

DiegoNolan commented 3 years ago

K, I think I made it backwards compatible. I added some new types AVEncodingParams, AEncodingParams, and AVWriterContext. I think AVWriterContext should probably use the type families like you said but I'm not that familiar with them. I may need an example to implement it.

acowley commented 3 years ago

It's all in, and I included the PR I had opened on your branch last year!