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 rate and framerate properties to VideoStream (fixes: #1005) #1007

Closed
wants to merge 2 commits into from

Conversation

uvjustin
Copy link
Contributor

@uvjustin uvjustin commented Jul 7, 2022

Fixes the regression introduced by #910

This PR fixes the issue by adding properties that point directly to the underlying AVStream's avg_frame_rate. It doesn't set the rate on the new codec_context, but that doesn't appear to be necessary.

tests/test_encode.py Outdated Show resolved Hide resolved
@jlaine
Copy link
Member

jlaine commented Jul 18, 2022

I'm having another look at this, and I'm really confused. We already have Stream.average_rate, which does the exact same thing but with a more explicit name. Why do we need two additional accessors?

If the only reason is due to the historic confusion between stream and codec_context properties I'd actually favor going in the opposite direction and make both Stream.__getattr__ and Stream.__setattr__ raise an AttributeError for rate and framerate.

PyAV 10.0 is going to have some breaking changes, might as well start cleaning up..

@uvjustin
Copy link
Contributor Author

I see your point and that makes sense. We should just probably remove these accessors, but we should be explicit that it is a breaking change.

@uvjustin
Copy link
Contributor Author

I guess a follow on question is what to do with AudioStream.
The actual rate field for an audio stream is stored in sample_rate of the AVCodecParameters. We should probably get rid of the rate passthrough and enforce sample_rate since it is a more explicit name as well.

@jlaine
Copy link
Member

jlaine commented Oct 17, 2022

Closing in favour of #1036

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.

None yet

2 participants