Skip to content

add _backend argument to __init__() of class VideoClips #1363

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

Merged
merged 5 commits into from
Sep 24, 2019

Conversation

stephenyan1231
Copy link
Contributor

Changes

  • To enable the use of recently added video reader [video reader] inception commit #1303 by class VideoClips, add an argument _backend to switch between pyav and video_reader backend.
  • In torchvision/__init__.py , add a global variable to control the video backend
  • Update unit tests test/test_io.py and test/test_datasets_video_utils.py to retrieve global video backend config

Unit test

  • test/test_io.py
  • test/test_datasets_video_utils.py

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.

Looks good to me, thanks!

I'm approving it, but I'd like to know if using get_video_backend in VideoClips would be enough, instead of passing a _backend argument.

Is this because the metadata changes depending on the backend?

@@ -49,9 +67,11 @@ class VideoClips(object):
on the resampled video
"""
def __init__(self, video_paths, clip_length_in_frames=16, frames_between_clips=1,
frame_rate=None, _precomputed_metadata=None, num_workers=1):
frame_rate=None, _precomputed_metadata=None, num_workers=1,
_backend="pyav"):
Copy link
Member

Choose a reason for hiding this comment

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

Instead of passing a _backend argument, what about just using the get_video_backend to get this? Would this be enough for your use-case?

Copy link
Member

Choose a reason for hiding this comment

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

I'm sending a commit changing backend to _backend in the self, so no need to do anything here.

@codecov-io
Copy link

Codecov Report

Merging #1363 into master will decrease coverage by 0.35%.
The diff coverage is 34.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1363      +/-   ##
==========================================
- Coverage   64.76%   64.41%   -0.36%     
==========================================
  Files          76       76              
  Lines        5975     6028      +53     
  Branches      915      929      +14     
==========================================
+ Hits         3870     3883      +13     
- Misses       1833     1869      +36     
- Partials      272      276       +4
Impacted Files Coverage Δ
torchvision/io/__init__.py 100% <ø> (ø) ⬆️
torchvision/datasets/video_utils.py 63.75% <31.74%> (-20.47%) ⬇️
torchvision/__init__.py 71.42% <57.14%> (-4.77%) ⬇️
torchvision/transforms/transforms.py 81.56% <0%> (+0.58%) ⬆️

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 02a8c0a...d40913d. Read the comment docs.

@fmassa fmassa merged commit 7874374 into pytorch:master Sep 24, 2019
facebook-github-bot pushed a commit to facebookresearch/ClassyVision that referenced this pull request Oct 16, 2019
Summary:
Pull Request resolved: #62

Current dependency torchvision 0.4.0 was released in August.
It missed quite a few PRs that are merged after that, and that are needed for video classification, such as

- pytorch/vision#1437
- pytorch/vision#1431
- pytorch/vision#1423
- pytorch/vision#1418
- pytorch/vision#1408
- pytorch/vision#1376
- pytorch/vision#1363
- pytorch/vision#1353
- pytorch/vision#1303

This will fail the CI test when a diff uses changes made in those PRs.
Before a new official version of TorchVision is released, we can temporarily use the nightly torchvision to get all the recent PRs, and unblock the PR merging.
We plan to use a fixed version of TorchVision later.

Reviewed By: vreis

Differential Revision: D17944239

fbshipit-source-id: 86ff540e3fc4f08ef767e84ef103525db5158201
@fmassa fmassa mentioned this pull request Oct 31, 2019
fmassa pushed a commit that referenced this pull request Oct 31, 2019
* add _backend argument to __init__() of class VideoClips

* minor fix

* minor fix

* Make backend private in VideoClips

* Fix lint

* Fix conflict due to cherry-pick 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