Skip to content

Fast seek implementation #3179

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 23 commits into from
Nov 3, 2021

Conversation

bjuncek
Copy link
Contributor

@bjuncek bjuncek commented Dec 16, 2020

Implementation of the fast seek; rebase of the #3015
@fmassa can you please pull it in fbcode to check if ok?

rebased on the latest master

cc @pmeier

@bjuncek
Copy link
Contributor Author

bjuncek commented Dec 16, 2020

For some reason it's not allowing me to push it to fbsync, I've tried twice and it always sends me back to master?

@codecov
Copy link

codecov bot commented Dec 16, 2020

Codecov Report

Merging #3179 (ef6e129) into master (1a300d8) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3179   +/-   ##
=======================================
  Coverage   73.48%   73.48%           
=======================================
  Files          99       99           
  Lines        9230     9230           
  Branches     1476     1476           
=======================================
  Hits         6783     6783           
  Misses       1991     1991           
  Partials      456      456           
Impacted Files Coverage Δ
torchvision/io/__init__.py 89.65% <100.00%> (ø)

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 1a300d8...ef6e129. Read the comment docs.

Copy link
Contributor

@prabhat00155 prabhat00155 left a comment

Choose a reason for hiding this comment

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

Thanks @bjuncek, could you also resolve the merge conflicts?


self.assertTrue(len(av_keyframes) == len(vr_keyframes))
# NOTE: this video gets different keyframe with different
# loaders (0.333 pyav, 0.666 for us)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know the reason for this?

vr_keyframes = []
if av_reader.streams.video:

# get all keyframes using pyav. Then, seek randomly into video reader
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment should go before the if statement as it describes the entire chunk of code from line 417 onwards, not just the if block.

@prabhat00155
Copy link
Contributor

I can't import this PR internally to run tests due to merge conflicts. Could you please fix them? Thanks!

@datumbox
Copy link
Contributor

@bjuncek Just wanted to clarify why the PR contains [fbsync] in the title?

As far as I understand, please correct me if I'm wrong, this is a brand new PR you wrote (it doesn't come from the fbsync branch) which you want to have it tested on FBcode. If this is indeed the case, you need to remove the prefix because this will cause the exact opposite result of what you want. PRs tagged with [fbsync] are skipped from being synced to FBcode. This is because we reserve the tag for those PRs that sync FBCode=>Github.

Copy link
Contributor

@prabhat00155 prabhat00155 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 @bjuncek! One minor comment.

@bjuncek
Copy link
Contributor Author

bjuncek commented Oct 13, 2021

@datumbox sorry, my bad - Francisco told me that adding fbsync tag would import it into fbcode; removed now.

@bjuncek bjuncek changed the title [fbsync] Fast seek implementation Fast seek implementation Oct 13, 2021
@prabhat00155 prabhat00155 merged commit 4ccef06 into pytorch:main Nov 3, 2021
@github-actions
Copy link

github-actions bot commented Nov 3, 2021

Hey @prabhat00155!

You merged this PR, but no labels were added. The list of valid labels is available at https://github.com/pytorch/vision/blob/main/.github/process_commit.py

facebook-github-bot pushed a commit that referenced this pull request Nov 8, 2021
Summary:
* modify processPacket to support fast seek

* add fastSeek to ProcessPacket decoder definition

* add fastseek flag to DecoderParametersStruct

* add fastseek flag to the process packet call

* no default params in C++ implementation

* enable flag in C++ implementation

* make order of parameters more normal

* register new seek with python api

* [somewhat broken] test suite for keyframes using pyav

* revert " changes

* add type annotations to init

* Adding tests

* linter

* Flake doesn't show up :|

* Change from unitest to pytest syntax

* add return type

Reviewed By: kazhang

Differential Revision: D32216689

fbshipit-source-id: 695975c2930cb663ea82c83e4bc924a09e124a7d

Co-authored-by: Prabhat Roy <[email protected]>
cyyever pushed a commit to cyyever/vision that referenced this pull request Nov 16, 2021
* modify processPacket to support fast seek

* add fastSeek to ProcessPacket decoder definition

* add fastseek flag to DecoderParametersStruct

* add fastseek flag to the process packet call

* no default params in C++ implementation

* enable flag in C++ implementation

* make order of parameters more normal

* register new seek with python api

* [somewhat broken] test suite for keyframes using pyav

* revert " changes

* add type annotations to init

* Adding tests

* linter

* Flake doesn't show up :|

* Change from unitest to pytest syntax

* add return type

Co-authored-by: Prabhat Roy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants