Various fixes to seeking and decoding #437
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
arch1t3cht
force-pushed
the
seeking_fixes
branch
from
c3ae10e
to
bedbffe
Compare
|
@arch1t3cht Thanks! This is a lot, so I'll start digging into it later today - might take a day or three. (I'll run it through $dayjob's internal tests as well, in the future.) |
|
Pushed another few commits (sorry for the added work...), to try and fix most of the remaining mpegts files. Seek test results (capped to 500 frames this time, except for capping (Of course, if you have other files that regress with one of these commits, I can see if I can fix that). With the last two commits here it gets a bit more ugly since it's starting to become a game of whack-a-mole to work around certain files while not breaking others (e.g. most mpegts files here seem to behave better when |
|
Starting to go through them. A few commits look like I can pull them out and merge immeiately, some I think require some more explanation (like under what circumstances you have seen keyframes with PTS != DTS, or how using PTS in DecodePacket() can work on e.g. MPEG-TS with timestamp wraparound (non-monotonic)). I have dropped you a PM on IRC, as you seem to be active there (?), since real time communication, at least initially, may be easier. (I wish GitHub was less bad at per-commit review...) |
arch1t3cht
force-pushed
the
seeking_fixes
branch
from
96cb429
to
c3e931d
Compare
kodawah
reviewed
Apr 11, 2024
src/core/videosource.cpp
Outdated
| // Seeking too close to the end of the stream can result in a different decoder delay since | ||
| // frames are returned as soon as draining starts, so avoid this to keep the delay predictable. | ||
| // Is the +1 necessary here? Not sure, but let's keep it to be safe. | ||
| int EndOfStreamDist = CodecContext->has_b_frames + 1; |
There was a problem hiding this comment.
where does the +1 even come from?
arch1t3cht
force-pushed
the
seeking_fixes
branch
from
c3e931d
to
afe3645
Compare
Commit 1/2 to fix interlaced files. After this commit, seeking is correct on interlaced h264 with threads=1, but not yet with more. Remove the PAFF adjustment hacks, which don't work for actual mixed interlaced/progressive content. The second field of each frame is already being marked as hidden during indexing, but previously that would not actually be used during decoding since DecodePacket checks the AV_PKG_FLAG_DISCARD flag rather than ffms2's own Hidden flag. Hence, this commit makes DecodePacket check the Hidden flag instead. It also simplifies the DelayCounter logic a bit by simply increasing the DelayCounter every time a non-hidden packet is sent and decreasing it every time a frame is received. As far as I can see, the added AllowHidden arguments for FrameFromPos and FrameFromPTS are not actually necessary, and can just always be true, but this way less existing behavior is changed. This can be revisited when a sample comes up where this would actually make a difference (i.e. the first frame after seeking being hidden, or ending up somewhere unknown after a seek in a file with hidden frames).
An example file where this is necessary is ffms2_seeking_issue.mp4 in the doom9 collection of seek test sample files. This solution is quite ugly but it's the best I could find. The only alternative I can think of is seeking to the same point twice on every seek, once to compute CurrentFrame and a second time to actually start the decoding loop. This might make the GetFrame loop a lot cleaner but it'd be a bigger refactor and make seeking slower again.
Commit 2/2 to fix interlaced files. Seeking is now correct on interlaced h264 while not breaking progressive files with hidden frames. Previously, the second field packets of complementary field pairs were marked as hidden during indexing, but that does not fully work because decoder delay behaves differently on such fields compared to frame packets marked as invisible. Thus, these are now split into two separate flags MarkedHidden and SecondField in the index. Furthermore, decoder delay is now split into thread delay and reorder delay, since these two behave differently with respect do such second fields: While hidden frames don't contribute to delay at all, second fields contribute to thread delay but not to reorder delay. For codecs like AV1, where the delay is an opaque CodecContext->delay variable, we cannot split delay into thread delay and reorder delay, but here that's not a problem because those codecs don't have interlaced video. If this does cause issues, it has to be dealt with case by case either way. I'm not sure if the logic in MaybeHideFrames is still needed but I have not seen it triggered for any file yet so I'll keep it around for now.
The previous commit updated the index format, so this can now be stored in the index too.
The Seek(0) in the FFMS_VideoSource constructor can break the invariants assumed during the GetFrame loop later on when the first frame in output order is not the first frame in decoding order. An example for this is 3d-parrot.MTS from the doom9 collection. We could try to ensure that this initial seek behaves just like the future seeks in the GetFrame loop do, but it's simpler to just force a seek at the start to go into decoding with a clean slate. Similarly, don't output the frame we decoded in the constructor, just to be sure.
For (non-MBAFF) interlaced h264 in mpegts containers, the second fields all have FilePos equal to -1 or to the previous packet's FilePos. Previously, the fields with FilePos=-1 would abort the loop to find CurrentFrame prematurely.
Presentation timestamps seem to work fine for most files in these containers (and replacing PTS with DTS can break predictions) so wait until we encounter a packet with no PTS (which does happen sometimes) before reverting to UseDTS. On the other hand, seeking by PTS is unreliable in mpegts (since it's just lavf's generic internal seek function), so default to SeekByPos there. I don't have an mpegtsraw test file on hand but as far as I can see that should get the same treatment. For nuv I don't really know, so I'll leave it as it is.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This consists of various bigger refactors which were necessary to fix certain parts of seeking, but it can still mostly be read commit for commit. Some of the later commits do make some of the earlier commits partially obsolete, so let me know if you'd like any of this squashed.
I sent a patch to ffmpeg to fix 70a7c1b but didn't get a response yet.
Running seek tests with this on the doom9 sample file collection shows no regressions:
Before (the format being
filename,number of errors):After: (Don't pay too close attention to the actual numbers, for this second seek test I had to clip all videos to 200 frames to make the tests faster. What's important is the presence or absence of errors. If necessary I can also run a full seek test, though.)
Some background on some specific files affected by this PR:
3d-parrot.mkvis an interlaced open-gop h264 fileinterlaced_h264.mkvis an interlaced open-gop h264 file that uses MBAFF and hence still has one packet per frame.ffms2_seeking_issue.mp4is a progressive open-gop h264 file that has been cut at a non-IDR recovery point (and hence doesn't start with an IDR frame), where frames 1 and 2 (starting at 0) in decoding order rely on missing references, are marked as hidden by the demuxer, and are skipped by the decoder.So after this PR all mkv and mp4 files have consistent seeking, except for:
MPEGSolution_stuart.mp4, which seems to just be a broken file that also corrupts in players like mpv.