Skip to content

Conversation

scotts
Copy link
Contributor

@scotts scotts commented Sep 4, 2025

This check was added in PR #503. We're keeping setting all inactive streams to AVDISCARD_ALL, but removing the check in scanFileAndUpdateMetadataAndIndex(). The reasoning behind the check was, I believe: let's make sure we're scanning the packets for all streams. And since we're only setting AVDISCARD_ALL in addVideoStreamDecoder(), then if we encounter it when scanning, something out-of-order must have happened.

However, I don't think we're the only ones setting the discard value. I've encountered a video that ffprobe reports has an unsupported codec for stream 1. Stream 0 seems fine. I suspect that FFmpeg is then setting AVDISCARD_ALL for us on stream 1 in avformat_open_input() or avformat_find_stream_info(). We should be able to decode stream 0 just fine.

@scotts scotts requested a review from NicolasHug September 4, 2025 20:04
@meta-cla meta-cla bot added the CLA Signed This label is managed by the Meta Open Source bot. label Sep 4, 2025
@scotts scotts marked this pull request as ready for review September 4, 2025 20:05
@Dan-Flores
Copy link
Contributor

What is the codec_id for this stream? Perhaps there is a different field we can check to ignore streams with invalid codecs.

@scotts
Copy link
Contributor Author

scotts commented Sep 4, 2025

@Dan-Flores, I think that FFmpeg is already doing that for us. That is, we open the file (avformat_open_input()) and try to get better quality metadata from it (avformat_find_stream_info()). I think FFmpeg detects that we don't support the codec for a stream, and it sets the discard field appropriately. We don't have the codec, so we should discard those packets.

The error on our part, I believe, is that we assumed we were the only ones setting that field. If that were the case, seeing it set to AVDISCARD_ALL would mean that the member functions on the C++ SingleStreamDecoder were called in the wrong order. But we're not the only ones setting that field, so our check is, I think, invalid.

And yes, the behavior with the file in question is that we hit this error upon initialization.

@scotts scotts merged commit da3edda into meta-pytorch:main Sep 4, 2025
42 checks passed
@scotts scotts deleted the discard_all branch September 4, 2025 20:55
@NicolasHug
Copy link
Contributor

The reasoning behind the check was, I believe: let's make sure we're scanning the packets for all streams. And since we're only setting AVDISCARD_ALL in addVideoStreamDecoder(), then if we encounter it when scanning, something out-of-order must have happened

Yes, that's correct. With the check now removed, we're exposed to the small risk that someone would call addStream before calling scanFileAndUpdateMetadataAndIndex, which would result in scanFileAndUpdateMetadataAndIndex only scanning the target stream, instead of all streams. But that's probably not a big deal and that's never an issue when using the public APIs anyway.
Thanks for the fix!

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