modified code of io.read_video to interpret start_pts and end_pts in seconds

[cskanani]

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 so now user can provide start_pts and end_pts values in seconds, the stream.time_base is then used to convert the values of start_pts and end_pts for av.

[fmassa]

Hi,

Thanks for the PR! I do think we should use seconds as a global mesure, instead of the pts from each stream.

This PR seems to keep backwards-compatibility with the previous code, and also fixes the issues that have been discussed in #1248 I believe, is that right?

also, cc @bjuncek and @stephenyan1231

[cskanani]

Hi,

Thanks for the PR! I do think we should use seconds as a global mesure, instead of the pts from each stream.

This PR seems to keep backwards-compatibility with the previous code, and also fixes the issues that have been discussed in #1248 I believe, is that right?

also, cc @bjuncek and @stephenyan1231

Yes this PR solves the issues discussed in #1248. As far as I know to keep this change backward compaitible an attribute can be added to the io.read_video method for selecting the time unit, and if we set it default to 'pts' then code will be backward compaitible.

cc @bjuncek and @stephenyan1231

[bjuncek]

This seems much cleaner than #1248, the only question/concern I have is the fact that we are still storing the pts in a stream-specific way, which means that it is not consistent along the pipeline (iiuc, read_timestamps will still return video- based pts when generating video-clips object).

Thoughts?

[cskanani]

This seems much cleaner than #1248, the only question/concern I have is the fact that we are still storing the pts in a stream-specific way, which means that it is not consistent along the pipeline (iiuc, read_timestamps will still return video- based pts when generating video-clips object).

Thoughts?

It can be handled by changing return statement of io.read_video_timestamps from return [x.pts for x in video_frames], video_fps to return [x.pts*video_stream.time_base for x in video_frames], video_fps

cc @bjuncek and @stephenyan1231

[bjuncek]

Yeah for sure (that's what I tried to do in #1248)

@fmassa, should we maybe cherrypick between the two PRs?

[fmassa]

I think we should find the best of both worlds.

It is good that we have started the implementation with PyAV first, because it was simple and enabled us to iterate faster on a few things.
We will be soon moving the backend to C++ with @stephenyan1231 PR #1303 (which is actually faster than the current implementation based on PyAV), so we need to decide on which approach to follow.

If it wasn't for backwards-compatibility reasons, I'd prefer an approach that uses absolute metrics everywhere, for example seconds.

Can we find a way of keeping BC given this change?

[cskanani]

I think we should find the best of both worlds.

It is good that we have started the implementation with PyAV first, because it was simple and enabled us to iterate faster on a few things.
We will be soon moving the backend to C++ with @stephenyan1231 PR #1303 (which is actually faster than the current implementation based on PyAV), so we need to decide on which approach to follow.

If it wasn't for backwards-compatibility reasons, I'd prefer an approach that uses absolute metrics everywhere, for example seconds.

Can we find a way of keeping BC given this change?

To keep it BC the function definitions of functions using pts can be changed i.e. def read_video(filename, start_pts=0, end_pts=None): can be changed to def read_video(filename, start_pts=0, end_pts=None, pts_unit='pts'): here pts_unit can have two values i.e 'pts' and 'sec'. For handling this only addition of an if-else statement will be required.

cc @bjuncek and @stephenyan1231

[fmassa]

I think I agree with your proposal.
We can also make pts_unit raise a warning saying that pts gives wrong results and will be removed in a follow-up version.

I also think we should add the same option to read_video_timestamps to be consistent.

Can you implement those changes?

[cskanani]

I think I agree with your proposal.
We can also make pts_unit raise a warning saying that pts gives wrong results and will be removed in a follow-up version.

I also think we should add the same option to read_video_timestamps to be consistent.

Can you implement those changes?

Yes, I will implement it and will generate a new PR for it.

cc @bjuncek and @stephenyan1231

[cskanani]

I think I agree with your proposal.
We can also make pts_unit raise a warning saying that pts gives wrong results and will be removed in a follow-up version.

I also think we should add the same option to read_video_timestamps to be consistent.

Can you implement those changes?

Hi, I have made the required changes and created a new PR #1331 .

cc @bjuncek and @stephenyan1231

[cskanani]

Hi, @fmassa is there any update on this PR.

[fmassa]

Hi @cskanani

Test failures seem to be related to this PR

hold_placehold_placehold_placehold_placehold_placehold_placehold_placeh/lib/python2.7/site-packages/torchvision/io/video.py", line 193
    warnings.warn("The pts_unit 'pts' produces wrong results and will be removed
import: 'torchvision'

Also, FYI we are working on with @stephenyan1231 to make the video reading part be entirely on C++, so this part of the codebase will be deprecated soon.

[cskanani]

Hi @cskanani

Test failures seem to be related to this PR

hold_placehold_placehold_placehold_placehold_placehold_placehold_placeh/lib/python2.7/site-packages/torchvision/io/video.py", line 193
    warnings.warn("The pts_unit 'pts' produces wrong results and will be removed
import: 'torchvision'

Also, FYI we are working on with @stephenyan1231 to make the video reading part be entirely on C++, so this part of the codebase will be deprecated soon.

Ok. Then I am not working further on this PR.

[fmassa]

Hi,

Actually, for now the PR that I mentioned before will live together with the current implementation based on PyAV, and will be in experimental state.

So the changes that you made here are actually very relevant and will be merged in torchvision.
Sorry for the confusion and thanks for the PR!

[cskanani]

Hi,

Actually, for now the PR that I mentioned before will live together with the current implementation based on PyAV, and will be in experimental state.

So the changes that you made here are actually very relevant and will be merged in torchvision.
Sorry for the confusion and thanks for the PR!

Hi, Please check this PR #1331 I have made all the required changes, there are no merge conflicts and it is also passing all the test cases.

cc @bjuncek and @stephenyan1231

[cskanani]

Hi, @fmassa please check PR #1331 I have made all the changes that were discussed and it is passing all the test cases.

[bjuncek]

Looks good to me - can we close this PR then and continue onwards there?

[cskanani]

Looks good to me - can we close this PR then and continue onwards there?

Yes, please.