Skip to content

Conversation

@fmassa
Copy link
Member

@fmassa fmassa commented Oct 22, 2019

This PR makes set_video_backend to actually do something inside read_video and read_video_timestamps.
It unifies the interface of both backends, and also add separate unittest for each backend.

I had to move the video reader initialization because it was causing reference cycles with the code.

cc @stephenyan1231 for review

@codecov-io
Copy link

codecov-io commented Oct 22, 2019

Codecov Report

Merging #1514 into master will decrease coverage by 0.76%.
The diff coverage is 87.71%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1514      +/-   ##
=========================================
- Coverage   64.66%   63.9%   -0.77%     
=========================================
  Files          83      83              
  Lines        6461    6507      +46     
  Branches      992    1004      +12     
=========================================
- Hits         4178    4158      -20     
- Misses       1984    2056      +72     
+ Partials      299     293       -6
Impacted Files Coverage Δ
torchvision/io/__init__.py 100% <100%> (ø) ⬆️
torchvision/io/video.py 28.32% <76.47%> (-46.68%) ⬇️
torchvision/io/_video_opt.py 65.85% <92.3%> (+26.49%) ⬆️
torchvision/datasets/video_utils.py 65.08% <0%> (+1.77%) ⬆️
torchvision/__init__.py 70.96% <0%> (+3.22%) ⬆️

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 d409c11...857ea74. Read the comment docs.

@stephenyan1231
Copy link
Contributor

stephenyan1231 commented Oct 23, 2019

@fmassa this PR looks good to me. Thanks for unifying the API from pyav and video_reader backends.

Copy link
Contributor

@stephenyan1231 stephenyan1231 left a comment

Choose a reason for hiding this comment

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

LGTM

@fmassa fmassa merged commit 97b53f9 into pytorch:master Oct 23, 2019
@fmassa fmassa deleted the unify-video-backend branch October 23, 2019 18:03
fmassa added a commit that referenced this pull request Oct 31, 2019
* Unify video backend interfaces

* Remove reference cycle

* Make functions private and enable tests on OSX

* Disable test if video_reader backend not available

* Lint

* Fix import after refactoring

* Fix lint

* Fix merge conflict after cherry-picking for 0.4.2
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.

3 participants