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

Add max_b_frames property to codec context #1119

Merged
merged 1 commit into from
Nov 13, 2023

Conversation

davidplowman
Copy link
Contributor

Allows an application to control the maximum number of B frames, irrespective of what other settings or preset is used.

@davidplowman
Copy link
Contributor Author

Hi, thanks for all your work on PyAV!

I found myself using it recently but wanted to be able to use different codec presets but without B frames (which the presets normally allowed). This change seemed to be all that was necessary to resolve it, but please let me know if there's anything I should be doing differently. Thanks!

@davidplowman
Copy link
Contributor Author

Hi everyone, just wondering if there was any feedback on this suggestion? Thanks!

@davidplowman
Copy link
Contributor Author

Hi again, just wondering if there is anything else I need to do to get this PR considered? Obviously it would be really nice for us if we can just tell our users to install the standard pyav package. Thanks!

@jlaine
Copy link
Member

jlaine commented Oct 31, 2023

We have a policy of requiring unit tests for new code. Depending on whether this flag makes sense for encoding or decoding you'll most likely want to add some test code in test_encode.py or test_decode.py.

@davidplowman
Copy link
Contributor Author

Hi, thanks for your reply. I'm a bit busy with other things at the moment but will get back to this just as soon as I can. Would you prefer the test code in a 2nd commit in this PR, or all merged together in one commit?

We're using PyAV as the interface from Python to libav on the new Raspberry Pi 5 now, so I'm very keen to try and un-fork the version that we're currently distributing. Thanks!

Allows an application to control the maximum number of B frames,
irrespective of what other settings or preset is used. A test is
also added, to check that it works as intended.
@davidplowman
Copy link
Contributor Author

The latest update adds a docstring and a test for the functionality. Please let me know if it needs re-formatting or changing. Thanks!

@davidplowman
Copy link
Contributor Author

Thanks for running the tests. Obviously I'm not experienced at understanding these logs, but looking at the failure, I'm suspicious that this is just a timeout, and that nothing is actually "wrong". What do you think? Am I right in thinking the ubuntu aarch64 platform is much slower than the others - do you happen to know what kind of machine it is? Thanks!

@WyattBlue
Copy link
Member

The one failing test is just aach64 timing out (not the fault of this PR). Everything looks good so I'm going to merge this.

@WyattBlue WyattBlue merged commit 8bfb5f6 into PyAV-Org:main Nov 13, 2023
16 of 17 checks passed
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.

3 participants