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

Expose frame rate of streams #35

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

niklaskorz
Copy link

Getting the frame rate of a stream works essentially the same way as getting the time base.
I'm unsure about whether it makes sense to introduce a new type, as TimeBase basically already resembles the AVRational type.
If anything, it might make sense to rename TimeBase to a shared Rational type and make TimeBase an alias of Rational.

@operutka
Copy link
Member

Hi @niklaskorz! And thank you for the pull request.

Getting the frame rate of a stream works essentially the same way as getting the time base.

I think it won't be that easy. I hope you are familiar with the FFmpeg documentation for the r_frame_rate field. The thing is that the field might be a bit misleading and the values can be quite unexpected sometimes. Especially in cases when intervals between frames are not regular (this happens quite a lot in live streaming). The official FFmpeg documentation says:

Real base framerate of the stream.

This is the lowest framerate with which all timestamps can be represented accurately
(it is the least common multiple of all framerates in the stream). Note, this value is just
a guess! For example, if the time base is 1/90000 and all frames have either approximately
3600 or 1800 timer ticks, then r_frame_rate will be 50/1. 

There is also another field named avg_frame_rate which may be a better alternative in many cases. However, this field doesn't always have to be filled by the demuxer. I think that if we're going to expose r_frame_rate, we should also expose avg_frame_rate and explain clearly the difference between these two in the documentation.

I'm unsure about whether it makes sense to introduce a new type, as TimeBase basically already resembles the AVRational type.

Using TimeBase for representing frame rates is a bit clunky. A type called Rational actually makes more sense here. However, simple renaming of TimeBase to Rational also isn't the best solution because the associated constant MICROSECONDS would make no sense anymore. I'd prefer these changes:

  1. Rename TimeBase to Rational and remove the associated constant named MICROSECONDS.
  2. Change the numerator and denominator types to i32 (rational numbers can be also negative).
  3. Create a wrapper struct called TimeBase containing a single field (a rational number).
  4. Create the associated constant MICROSECONDS for the new TimeBase type.
  5. Implement AsRef<Rational>, Borrow<Rational>, Deref<Target = Rational> and From<Rational> for the new TimeBase type.

@operutka operutka self-assigned this Oct 13, 2021
@operutka operutka added the enhancement New feature or request label Oct 13, 2021
@operutka operutka self-requested a review October 19, 2021 11:03
Copy link
Member

@operutka operutka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job! Sorry for the delay. There's a few minor things that need to be fixed. Otherwise, I'm happy with the PR.

src/format/stream.rs Outdated Show resolved Hide resolved
src/format/stream.c Outdated Show resolved Hide resolved
src/format/stream.rs Outdated Show resolved Hide resolved
src/format/stream.rs Outdated Show resolved Hide resolved
src/time.rs Outdated
Comment on lines 30 to 33
pub const fn new(num: u32, den: u32) -> Self {
Self { num, den }
Self {
rational: Rational::new(num as i32, den as i32),
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Casting the numerator and denominator types from u32 to i32 can lead to unexpected results. I think it's better to simply change the method to:

pub const fn new(num: i32, den: i32) -> Self {
    ...
}

and allow negative time bases. I guess it's less evil than allowing unexpected results. FFmpeg allows negative time bases anyway.

src/time.rs Outdated Show resolved Hide resolved
@niklaskorz
Copy link
Author

niklaskorz commented Oct 27, 2021

Thanks for the input @operutka! I'll try to add the required changes tomorrow.

@niklaskorz
Copy link
Author

@operutka As suggested, timebases now use i32 and thus allow negative values. However, sample rates still use u32 and are cast into time bases at some points. I have resorted to try_into for these cases without panicking, i.e. propagating the try into error instead. Is this acceptable or do you have a better idea?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants