Skip to content

added stream specification option to read_video#1222

Open
CCareaga wants to merge 6 commits intopytorch:mainfrom
CCareaga:optional-av-streams
Open

added stream specification option to read_video#1222
CCareaga wants to merge 6 commits intopytorch:mainfrom
CCareaga:optional-av-streams

Conversation

@CCareaga
Copy link

@CCareaga CCareaga commented Aug 9, 2019

Added functionality described in issue #1220

@codecov-io
Copy link

codecov-io commented Aug 9, 2019

Codecov Report

Merging #1222 into master will decrease coverage by 0.01%.
The diff coverage is 87.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1222      +/-   ##
==========================================
- Coverage   65.64%   65.63%   -0.02%     
==========================================
  Files          74       74              
  Lines        5784     5788       +4     
  Branches      884      886       +2     
==========================================
+ Hits         3797     3799       +2     
- Misses       1723     1725       +2     
  Partials      264      264
Impacted Files Coverage Δ
torchvision/io/video.py 72.93% <87.5%> (+1.61%) ⬆️
torchvision/transforms/transforms.py 80.94% <0%> (-0.59%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8635be9...f3354a0. Read the comment docs.

Copy link
Contributor

@bjuncek bjuncek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me - neat fix

Copy link
Member

@fmassa fmassa left a 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!

I think this is a good feature to have, but I'm not yet sure about how it should be handled.

I wonder if we can have multiple video / audio streams in a video? In which case, the proposed solution wouldn't be enough.

@CCareaga
Copy link
Author

@fmassa Interesting, do you have a scenario in mind where a video might have multiple streams of a certain modality? Also, would the av package be able to handle that scenario?
I think that in the mean time, the stream selection option is useful (I originally proposed the feature for a project I'm working on).

@fmassa
Copy link
Member

fmassa commented Aug 29, 2019

@CCareaga some video formats supports an arbitrary number of streams per type, see e.g., https://video.stackexchange.com/questions/20933/can-video-have-multiple-streams-like-it-has-2-audio-streams

PyAV handles this case (at least in its API), because we can query streams by it index.

One possible scenario I'd imagine would be having stereo video (for example for augmented reality with headsets).

Still, the current API of torchvision does not support that, as we return a single audio and video elements. We would need anyway to do a backwards-incompatible change to be able to support this case.

In this case, I'd say it would be ok for now to assume that we can specify it in the way you proposed.

Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, thanks a lot!

I have only one more comment, which relates to the default empty type and shapes.

@CCareaga
Copy link
Author

@fmassa Oh okay, that totally seems like a valuable feature. With the current read video function it would probably require a decent overhaul.

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

Successfully merging this pull request may close these issues.

4 participants