imageio / imageio-ffmpeg

FFMPEG wrapper for Python
BSD 2-Clause "Simplified" License
221 stars 50 forks source link

Automatically find available h264 encoders and choose the best one according to our heuristics #67

Closed hmaarrfk closed 2 years ago

hmaarrfk commented 2 years ago

I tried to leave the defaults the same. But if x264 isn't available (for example, maybe a user doesn't want GPL), then it will gracefully fallback onto other encoders.

hmaarrfk commented 2 years ago

The other thing that this allows us is to move to newer encoding format relatively easily.

hmaarrfk commented 2 years ago

I could fix this in my code, for my library alone, however, many people call imageio assuming that the video writer will work with the default arguments.

However, the underlying installation of ffmpeg may not have libx264, then the default call will fail.

This helps us build more graceful fallbacks. All the above of x264 codecs so they should be equivalent for most users.

I think ubuntu even has more of them.

hmaarrfk commented 2 years ago

I couldn't find a good way to "find encoders that used a specific codec" i resorted to string parsing, which I'm not too happy about. I would also agree that hardcoding encoders is likely a good option for the few that we know of with string parsing as a fallback.

almarklein commented 2 years ago

I would also agree that hardcoding encoders is likely a good option for the few that we know of with string parsing as a fallback.

Not sure what you mean. Do you mean that in the result of the ffmpeg invokation we search for a set of hardcoded codecs instead of doing the parsing? Sounds good, if that makes the code a bit more elegant/reliable.

hmaarrfk commented 2 years ago

@almarklein I'm glad this is well received.

I'm really scare of string parsing command line programs that aren't documented to behave a certain way. I've included a hybrid approach that I believe will be more robust.

It still assumes that the encoder name comes at a specific place in the header. However, this assumption was made in both cases.

I'm not sure if there are any additional tests to make. It is somewhat hard to install other software in different configurations in the CIs.

hmaarrfk commented 2 years ago

One issue that I noticed is that this lists the codecs that are available, but for hardware accelerated codecs this doesn't always mean that they will succeed.... I have other utility functions I wrote I can work to open source those, but I just wanted to explain one potential challenge with the current implementation.

hmaarrfk commented 2 years ago

Yeah... even the methods i have, mostly just calling

ffmpeg -hide_banner -hwaccels

only list the hardware accelerators that were compiled with ffmpeg at compile time, but not the presently available hardware. I'm not sure if there is a way to do this with the C-API.

Though this file is about using the command line interface.

hmaarrfk commented 2 years ago

Well through some gymnastics I think i got something that at least won't crash the user's system. I'm really confused with vaapi doesn't work on my computer. Myabe the null test is broken for that.

hmaarrfk commented 2 years ago

I think we need some kind of cross platform manual verification before this moves in.

hmaarrfk commented 2 years ago

Well, I think the vaapi might be more involved. I recall I actually wrote some documentation for it a while back.

https://imageio.readthedocs.io/en/stable/examples.html?highlight=vaapi#writing-videos-with-ffmpeg-and-vaapi

Without all those flags, I remember things failed.

I'm not sure I really have the bandwidth to debug the encoder (considering the fact that I am mostly using the nvidia encoder).

I believe the current commit works with libopenh264 which at least gives one fallback.

Nothing would preclude users for specifying their own codecs and flags. This mostly just "helps" the default be more correct in the case of errors.

almarklein commented 2 years ago

This needs black . for the CI linter to pass.

I tested this locally (on MacOS):

>>> imageio_ffmpeg._io.get_compiled_h264_encoders()
('libx264', 'libx264rgb', 'h264_videotoolbox')

Maybe worth adding a test for ffmpeg_test_encoder.

hmaarrfk commented 2 years ago

I'm waiting till the implementation review goes through, this is somewhat copied from an internal implementation where we aren't as strict as black.

Black wants to split the flag + arguments into two separate lines.

Maybe worth adding a test for ffmpeg_test_encoder.

The only test i can think of is one where I test that things don't crash. But one can never be sure of what version is installed underneath.

hmaarrfk commented 2 years ago

OK, I've added a test that I believe should be "good enough". It tests the interface of the suggested code.

I have not tested this on windows. I'm mostly concerned about the useage of the "null" stream.

almarklein commented 2 years ago

These tests look good, thanks! They are run on Windows on CI.

This is great, thanks!