Skip to content
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

Add stream_index as an option to VideoDecoder #254

Merged
merged 3 commits into from
Oct 14, 2024

Conversation

scotts
Copy link
Contributor

@scotts scotts commented Oct 9, 2024

Adds stream_index as an option to VideoDecoder. Example usage:

decoder = VideoDecoder("myfile.mp4", stream_index=2)

While this is a modest change in the interface, it required:

  1. Adding the concept of streams to our test file generation.
  2. Adding new test files for the new stream.
  3. Adding the concept of streams to our testing utils.
  4. Enhancing some existing tests to parameterize on the stream index.

I suggest reviewing the changes to VideoDecoder first. Everything else is a consequence of that.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Meta Open Source bot. label Oct 9, 2024
@scotts scotts force-pushed the stream_index branch 2 times, most recently from 44aedb0 to 38a85bb Compare October 9, 2024 21:23
@scotts scotts marked this pull request as ready for review October 9, 2024 21:30
Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

Thanks @scotts , LGTM.

As an aside, I find that our testing framework is becoming more and more... frameworky. I fear that writing even simple tests is going to get increasingly difficult, as one has to familiarize themselves with the testing framework, which itself grows in complexity over time.

Comment on lines 289 to 294
best_stream_index = video_metadata.best_video_stream_index
if best_stream_index is None:
if best_stream_index is None and stream_index is None:
raise ValueError(
"The best video stream is unknown. " + _ERROR_REPORTING_INSTRUCTIONS
"The best video stream is unknown and there is no specified stream. "
+ _ERROR_REPORTING_INSTRUCTIONS
)
Copy link
Member

Choose a reason for hiding this comment

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

Nit
We have 2 instances of if stream_index is None, and we're retrieving best_stream_index unconditionally while it should only be needed when streaming_index is None. I'd suggest to move this block within a single if stream_index is None below, starting at line 296

@scotts
Copy link
Contributor Author

scotts commented Oct 14, 2024

@NicolasHug, point taken about the testing framework being frameworky. My goal here is to make something that models the parts of video formats that we care about so that we can declaratively state such things when defining a new kind of test video. Basically, our testing logic is going to be a very simple model of the underlying FFmpeg file formats.

In general, adding a new fundamental capability in our decoder will probably require changing some testing logic. For example, I anticipate having to make some more changes when we add audio decoding. This tendency will level-out as our public capabilities match the underlying capabilities in the C++ layer. This current change is the most disruptive change I can think of, as having to support multiple streams is far-reaching. Most future changes should be less involved.

In general, when adding a new media file for testing, we will probably not require changing testing logic. We'll probably be able to just add a new declaration. I'm anticipating much more of adding new test files in the future, and much less of new fundamental capabilities. That is, I want it to be easy for us to add a new test file and state the metadata we need for tests in a declarative manner.

@scotts scotts merged commit d841909 into pytorch:main Oct 14, 2024
22 checks passed
@scotts scotts deleted the stream_index branch October 14, 2024 02:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Meta Open Source bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants