modified code of io.read_video and io.read_video_timestamps to intepret pts values in seconds #1331
Conversation
…ret pts values in seconds
|
@fmassa @bjuncek @stephenyan1231 please check this PR. |
torchvision/io/video.py
Outdated
| video_end_pts = end_pts | ||
| video_stream = container.streams.video[0] | ||
| if pts_unit == 'sec': | ||
| video_start_pts = math.floor(start_pts * (1 / video_stream.time_base)) |
There was a problem hiding this comment.
will math.floor cast it to int?
There was a problem hiding this comment.
No, for python2 it is returning answer as float and for python3 it is returning int.
There was a problem hiding this comment.
Hi, I have converted ouput of math.floor to int for handling the python2 problem.
torchvision/io/video.py
Outdated
| container.close() | ||
| if pts_unit == 'sec': | ||
| return [float(x.pts * video_time_base) for x in video_frames], video_fps |
There was a problem hiding this comment.
is there a reason we're casting it to floats rather than keeping it as fractions (just asking).
There was a problem hiding this comment.
For making it more understandable and uniform, i.e. for read_video method users will pass values of start_pts and end_pts in floats. Therefore, I think it is good to return the values in same format it would make it more understandable.
There was a problem hiding this comment.
I agree with you that returning floats might be more natural, but I'm worries that we might be introducing some unnecessary precision loss due to this cast.
I think we should return the values as Fraction, and handle both Fraction and float in read_video (which should be already the case or almost).
Thoughts?
There was a problem hiding this comment.
I have changed code to return Fraction from read_video_timestamps. The read_video was already handling both Fractions and floats, added that info to the doc string to make it more clear.
There was a problem hiding this comment.
Thanks a lot for the PR!
Can you also add a test comparing that there is no difference in results while reading using seconds or pts? We now have a few test videos in the repo, so you can use those as well, instead of just using the randomly generated videos.
torchvision/io/video.py
Outdated
| container.close() | ||
| if pts_unit == 'sec': | ||
| return [float(x.pts * video_time_base) for x in video_frames], video_fps |
There was a problem hiding this comment.
I agree with you that returning floats might be more natural, but I'm worries that we might be introducing some unnecessary precision loss due to this cast.
I think we should return the values as Fraction, and handle both Fraction and float in read_video (which should be already the case or almost).
Thoughts?
… test cases for pts_unit sec
I have added three test cases using pts_unit as 'sec'. |
torchvision/io/video.py
Outdated
| video_start_pts = start_pts | ||
| video_end_pts = end_pts | ||
| video_stream = container.streams.video[0] | ||
| if pts_unit == 'sec': |
There was a problem hiding this comment.
I'm wondering if we can't move this conversion logic to _read_from_stream? It could avoid duplicating the implementation for both audio and video.
There was a problem hiding this comment.
Hi, I have moved the unit conversion logic to _read_from_stream method.
Codecov Report
@@ Coverage Diff @@
## master #1331 +/- ##
=========================================
- Coverage 65.89% 65.4% -0.49%
=========================================
Files 75 75
Lines 5785 5851 +66
Branches 885 900 +15
=========================================
+ Hits 3812 3827 +15
- Misses 1708 1752 +44
- Partials 265 272 +7
Continue to review full report at Codecov.
|
Hi, I have made the changes and moved the unit conversion logic to _read_from_stream method for reducing the duplication. The build is failing on macos but it is not caused by my changes. Let me know if some changes are required in the code. |
|
Looks good to me! |
…et pts values in seconds (#1331) * modified code of io.read_video and io.read_video_timestamps to interpret pts values in seconds * changed default value for pts_unit to pts, corrected formatting * hanndliing both fractions and floats for start_pts and end_pts, added test cases for pts_unit sec * moved unit conversion logic to _read_from_stream method
The start_pts and end_pts attributes of io.read_video method were not having any time unit. It was not clear what start_pts and end_pts values signifies (this values depends on stream.time_base time unit).
Modified the code of io.read_video and io.read_video_timestamps to intepret pts values in seconds, stream.time_base is used for conversion of seconds and pts values.