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

Refactor VideoDecoder C++ initialization #435

Merged
merged 1 commit into from
Dec 16, 2024
Merged

Conversation

scotts
Copy link
Contributor

@scotts scotts commented Dec 16, 2024

In the C++ layer, in VideoDeocder::initializeDecoder(), we were calling std::vector::resize(), adding 1 each time, and then calling std::vector::back() to get a reference to the last element and fill it out. That is not how vectors are designed to be used. Instead, in the loop, we just create a struct on the stack, fill it with values, and call std::vector::push_back().

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Meta Open Source bot. label Dec 16, 2024
@scotts scotts marked this pull request as ready for review December 16, 2024 16:48
@scotts scotts merged commit 9653969 into pytorch:main Dec 16, 2024
36 of 37 checks passed
@scotts scotts deleted the refactor_init branch December 16, 2024 16:48
Comment on lines -237 to -240
// Some formats don't store enough info in the header so we read/decode a few
// frames to grab that. This is needed for the filter graph. Note: If this
// takes a long time, consider initializing the filter graph after the first
// frame decode.
Copy link
Member

Choose a reason for hiding this comment

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

Curious if we should keep that comment, or what was the reason to remove it? It might be helpful to keep (especially the "Note" part), particularly so that the reader knows that some frame decoding does happen within avformat_find_stream_info()?

I'm finding this from the FFmpeg docs

Some formats do not have a header or do not store enough information there, so it is recommended that you call the avformat_find_stream_info() function which tries to read and decode a few frames to find missing information.

Copy link
Contributor Author

@scotts scotts Jan 9, 2025

Choose a reason for hiding this comment

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

Good catch. I removed the comment because of the latter part about lazy initialization - we're doing that now. But the other stuff is both relevant and informative. I'll submit a PR which revives the relevant stuff.

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