Improve FrameInfo handling
#8099
Conversation
emilk
added
🧑💻 dev experience
| @@ -93,6 +93,11 @@ impl From<Error> for crate::decode::Error { | |||
| /// ffmpeg does not tell us the timestamp/duration of a given frame, so we need to remember it. | |||
| #[derive(Clone)] | |||
| struct FfmpegFrameInfo { | |||
| /// The start of a new [`crate::demux::GroupOfPictures`]? | |||
| /// | |||
| /// If true, an entire frame can be decoded from this one sample, | |||
There was a problem hiding this comment.
I thought for very long that this was true. But now I'm not so sure anymore that we are guaranteed to get a frame back from a decoder when we enqueue such a frame.
I would have expected PTS==DTS on those, but that part I know for sure now is not the case.
One subtle problem with this statement that I'm a bit more sure about is that while we have a sample -> frame, it may still be that a frame needs meta information from other samples even though this is supposed to be an IDR frame.
For instance in AV1, the information we append right now in our annex_b stream in the ffmpeg decoder is dispersed over the samples of the video and I have no ideas if there's any guarantees at all.
With that in mind I'd be in favor for leaving this out of the doc comment & tooltip (affects a bunch of places where this is copy pasted) until we understand better what we're dealing with.
There was a problem hiding this comment.
FWIW, this is what Claude 3.5 believes:
…but I'm sure it's mostly trained on common misconceptions
There was a problem hiding this comment.
had mixed success with LLMs in this area. And I've got too often burned by now believing the simple answers :/
There was a problem hiding this comment.
not a strong conviction here as described.
What bugs me about all this is that the is_sync is derived in quite strange way in the mp4 crate which I don't follow and I'd rather have per codec defined things that are easier to look up :(
There was a problem hiding this comment.
we obviously depend on it meaning something though as it governs our enqueuing strategy
There was a problem hiding this comment.
maybe just slap a We generally assume in front of it 🤷
| "{} {}:{} {}", | ||
| record.level(), |
There was a problem hiding this comment.
what is this? I'm lacking context
There was a problem hiding this comment.
I meant
- why does it make it better - before/after
- what has this to do with anything in this pr
There was a problem hiding this comment.
I like to flip that switch so I can see where a log message comes from.
Before I couldn't see the log level when I did so. Now I can.
There was a problem hiding this comment.
It was completely unrelated to the PR, except I wanted to debug "what the hell is wring that annoying log message"
What
Added
is_syncand make sureFrameInfois either valid orNone.Checklist