Skip to content

torchvision.io.video.read_video pts units for video only #1931

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

Closed
tmabraham opened this issue Mar 2, 2020 · 4 comments
Closed

torchvision.io.video.read_video pts units for video only #1931

tmabraham opened this issue Mar 2, 2020 · 4 comments

Comments

@tmabraham
Copy link

If the pts_unit='pts' for the function torchvision.io.video.read_video we get a warning:

The pts_unit 'pts' gives wrong results and will be removed in a follow-up version. 
Please use pts_unit 'sec'.

Looking through an earlier issue regarding this, the usage of pts is only wrong if accurate AV sync is needed. So does that mean if I only care about the video content and not the audio (which is the case for many problems like video classification), then will it be fine to use pts? I ask because often we are concerned about only taking a certain number of frames, and using pts allows us to do that without loading in the whole video first.

If the usage of pts is only wrong if AV sync is desired, then should that be made clear in the documentation and in the warnings? Also, shouldn't the pts option still be available in future versions, while sec could be the new default?

@bjuncek
Copy link
Contributor

bjuncek commented Mar 5, 2020

@tmabraham yeah - all the models were trained using pts and in principle there is nothing wrong with it if you don't care about seeking into a particular parts of the video and accurate AV sync (and if you do, you're probably using read_video function which has pts_unit option).

We trained all benchmark models using the pts and the results are comparable to the benchmarks.

I believe @fmassa was planning to make sec the default at some point for 0.5 release but it got lost amongst other tasks.

@tmabraham
Copy link
Author

@bjuncek Thanks for the clarification. To confirm, the 'pts' option will not be removed in a future release?

@bjuncek
Copy link
Contributor

bjuncek commented Mar 10, 2020

My understanding is that it will remain as is, the only thing to change will be the default option.

@tmabraham
Copy link
Author

@bjuncek Sounds good. Thanks!

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

No branches or pull requests

2 participants