ManimCommunity / manim

A community-maintained Python framework for creating mathematical animations.
https://www.manim.community
MIT License
26.81k stars 1.84k forks source link

Set AAC codec for audio in mp4 files, add transcoding utility #3956

Closed achille-fouilleul closed 1 month ago

achille-fouilleul commented 1 month ago

When the output format is mp4, pyav seems to reject wav-format audio. Convert to AAC.

Also, certain versions of pyav reject frame rates specified as float. Ensure frame_rate has type Fraction.

behackl commented 1 month ago

I am somewhat sure that I've tried aac-transcoding to fix the documentation build issue at some point yesterday in the context of #3953. But even in case it still isn't fixed, this is probably a good idea; I am in favor of this change. 👍

(Crossing fingers for the docbuild issue too ...)

achille-fouilleul commented 1 month ago

I think I understand your earlier comments a bit better.

TIL .mp4 (MPEG-4 Part 14) was standardized from .mov (QuickTime File Format). While .mp4 and .mov may overlap to a large extent, this is not true with other formats like .webm or .gif.

That both the output file suffix and config.format (and also config.transparent) influence the output is confusing. The test suite may be passing but the logic is not correct. A bit more work is needed to clean this up.

behackl commented 1 month ago

TIL .mp4 (MPEG-4 Part 14) was standardized from .mov (QuickTime File Format). While .mp4 and .mov may overlap to a large extent, this is not true with other formats like .webm or .gif.

The key difference is that .mp4 containers do not support transparency, which is why at some point we introduced this "automatic" format switch if transparency is requested.

That both the output file suffix and config.format (and also config.transparent) influence the output is confusing. The test suite may be passing but the logic is not correct. A bit more work is needed to clean this up.

I think this deserves to be cleaned up; following the Python idiom of "explicit is better than implicit", I think requesting transparency with an incompatible container format should just raise an error. (We have started working on making the config less confusing and more inherently consistent in the context of #3466.)

For this PR it would be sufficient to check the currently constructed format based on config.movie_file_extension instead of out_suffix / config.format.

achille-fouilleul commented 1 month ago

OK. Still, I think using config.format directly is wrong. For example:

        if config.format == "webm":
            partial_movie_file_codec = "libvpx-vp9"

Suppose config.format is None but the output file suffix is .webm. The "then" branch should be taken. I will add a simple conversion function and do a consistency check in it.

behackl commented 1 month ago

Looks like we are back in the luxurious world where we have a working docbuild process. Thanks again for your efforts @achille-fouilleul, much appreciated!

achille-fouilleul commented 1 month ago

I'm glad it helped.