-
Notifications
You must be signed in to change notification settings - Fork 7.1k
Unify video metadata in VideoClips #1527
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
Unify video metadata in VideoClips #1527
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1527 +/- ##
=========================================
+ Coverage 64.56% 64.6% +0.04%
=========================================
Files 87 87
Lines 6694 6742 +48
Branches 1033 1034 +1
=========================================
+ Hits 4322 4356 +34
- Misses 2071 2085 +14
Partials 301 301
Continue to review full report at Codecov.
|
@@ -383,7 +383,7 @@ def get_pts(time_base): | |||
audio_timebase = info['audio_timebase'] | |||
audio_pts_range = get_pts(audio_timebase) | |||
|
|||
return _read_video_from_file( | |||
vframes, aframes, info = _read_video_from_file( |
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.
It seems a few arguments of _read_video_from_file
, such as video_width
, video_height
, video_min_dimension
, are not used here, and can not be specified in API _read_video
.
Video frame resizing is fast if done inside of video reader. I would like to keep those argument exposed.
However, pyav
backends does not support such resizing. For now, you already raise ValueError when they are not zero. In the long-run, we can consider applying video resizing transform to keep the same behavior between pyav and video_reader backend.
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 thinking is that I want to first properly benchmark the benefits of having the rescaling happening in ffmpeg to have an idea of how much improvements it will bring us.
Once we have the numbers, we should analyze them and then decide on how to expose the resizing functionality in a clean way to the users. I think this should be done after December though
Note that power users can still use the private API for specifying the video_width
/ etc, so this functionality is available but not part of the public API
* Unify video metadata in VideoClips * Bugfix * Make tests a bit more robust * Fix merge conflicts for cherry-pick for 0.4.2
This PR unifies the metadata information in
VideoClips
, relying on the new unified backend functionality from #1514It still keeps the call to the internal
_read_video_from_file
, because we still need to pass_video_height
/ etc for efficiency (but maybe that can be removed in the future?)The downside of the current approach is that we now need to call
_probe_video_from_file
at every call toget_clip
, which should be fine given that_probe_video_from_file
is very cheap.cc @stephenyan1231 , as this will probably break old checkpoints from internal runs that relied on the old metadata.