- Notifications
You must be signed in to change notification settings - Fork72
Add frame rate to VideoEncoder API#1054
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Add frame rate to VideoEncoder API#1054
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| pixel_format:Optional[str]=None, | ||
| crf:Optional[Union[int,float]]=None, | ||
| preset:Optional[Union[str,int]]=None, | ||
| frame_rate:Optional[int]=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm happy to bikeshed on parameter order.sample_rate is the last param inAudioEncoder, but that constructor is much smaller. Here, having it last might makeframe_rate less discoverable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
My views:
- Optional parameters should always be keyword-only.
codecshould be the first keyword-only parameter, as it's likely to be the most used.extra_optionsshould be the last keyword-only parameter, as "catch-all" parameters tend to be last.
I don't have a preference for the order of the rest. 🚲 🛖 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I didn't realizeextra_options wasn't keyword-only, sorry for not catching that earlier. Let's make it keyword-only and keep it as the last parameter (it's probably going to be the least used param).frame_rate can go anywhere betweencodec andextra_options
NicolasHug left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for the PR@Dan-Flores !
I think there are different ways to adjust the frame rate: in this PR we're keeping the frames exactly as they are and we adjust their duration. Effectively if the input video is 1 min long and we ask for twice the frame rate, then we just halved the video duration to 30s, so it's running at twice the speed.
I'm not entirely sure this is what we want - I think generally users would expect the same output duration and some frames to be duplicated, dropped, or potentially interpolated in their temporal dimension.
Adding outputsample_rate support to the audio encoder was pretty hard, it involved internal FIFOs etc. I suspect adding outputframe_rate support will be similarly difficult, at the very least we'll have to figure out what the FFmpeg CLI does and reproduce that.
I'd suggest to hold off for now. I think the features we have in the encoder are already fairly complete, we can leave that as future improvement!
This PR adds output
frame_rateto the API.Previously, we always encoded videos using the same
frame_rateas the inputframes.Now, we can set the output
frame_ratewhen callingto_file/to_tensor/to_filelike(as inAudioEncoder), to adjusts the encoded video's frame durations. If no outputframe_rateis specified, fallback to reusing the inputframe_rate.Testing:
To test, I added
test_frame_rate_parameterwhich slows and speeds up a test video and ensures the video metadata is updated correctly. I also observed the video plays back slower or faster.API after changes:
The complete API now looks as follows: