Skip to content

Commit 158900c

Browse files
cadubentzencalvaris
cadubentzen
authored and
calvaris
committed
[MSE] Decoding errors when appending segment with B-frames and previous segment ends with an I-frame
https://bugs.webkit.org/show_bug.cgi?id=272521 Reviewed by Xabier Rodriguez-Calvar. When a segment containing B-frames is appended, WebKit has logic to erase frames from the previous segment and avoid decoding glitches (219507@main). However, the current logic falls short in one edge case: if the previous segment ends with an I-frame in the overlap region to be erased. If the demuxer honors ISOBMFF edit lists, then the first I-frame from an incoming segment with B-frames can be placed earlier in decoding order than the last (I-)frame from the previous segment (potentially with different resolution), and that last I-frame doesn't get erased, and will be pushed for decoding. This confuses the decoder and the following P/B frames will fail to decode or decode incorrectly with artifacts due to missing/incorrect reference frame. This patch fixes this edge case by allowing I-frames to be erased from the track buffer only if they are presented earlier than the incoming I-frame. We remove frames from the track buffer until we find an I-frame that is presented later than the incoming I-frame. This handles the case when we get multiple I-frames in the overlapping area, as exercised in the layout test. Credit to Vivek Arumugam <vivek_arumugam@comcast.com> for initially finding the bug and investigating this. This patch builds on top of a patch he proposed. See #1301. * LayoutTests/media/media-source/media-source-samples-out-of-order-erase-sync-frames-expected.txt: Added. * LayoutTests/media/media-source/media-source-samples-out-of-order-erase-sync-frames.html: Added. * Source/WebCore/platform/graphics/SourceBufferPrivate.cpp: (WebCore::SourceBufferPrivate::processMediaSample): Canonical link: https://commits.webkit.org/277532@main
1 parent 52ff406 commit 158900c

3 files changed

Lines changed: 88 additions & 4 deletions

File tree

‎LayoutTests/media/media-source/media-source-samples-out-of-order-erase-sync-frames-expected.txt‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
2+
RUN(video.src = URL.createObjectURL(source))
3+
EVENT(sourceopen)
4+
RUN(sourceBuffer = source.addSourceBuffer("video/mock; codecs=mock"))
5+
First segment has normal, monotonically increasing samples, and ends with two key frames.
6+
RUN(sourceBuffer.appendBuffer(mediaSegment))
7+
EVENT(updateend)
8+
EXPECTED (bufferedSamples.length == '4') OK
9+
{PTS({0/1 = 0.000000}), DTS({0/1 = 0.000000}), duration({2/1 = 2.000000}), flags(1), generation(0)}
10+
{PTS({2/1 = 2.000000}), DTS({2/1 = 2.000000}), duration({2/1 = 2.000000}), flags(0), generation(0)}
11+
{PTS({4/1 = 4.000000}), DTS({4/1 = 4.000000}), duration({2/1 = 2.000000}), flags(1), generation(0)}
12+
{PTS({6/1 = 6.000000}), DTS({6/1 = 6.000000}), duration({2/1 = 2.000000}), flags(1), generation(0)}
13+
Second, overlapping segment has out-of-display-order samples. This append should replace the last two samples from the previous append.
14+
RUN(sourceBuffer.appendBuffer(mediaSegment))
15+
EVENT(updateend)
16+
EXPECTED (bufferedSamples.length == '4') OK
17+
{PTS({0/1 = 0.000000}), DTS({0/1 = 0.000000}), duration({2/1 = 2.000000}), flags(1), generation(0)}
18+
{PTS({2/1 = 2.000000}), DTS({2/1 = 2.000000}), duration({2/1 = 2.000000}), flags(0), generation(0)}
19+
{PTS({8/1 = 8.000000}), DTS({3/1 = 3.000000}), duration({2/1 = 2.000000}), flags(1), generation(1)}
20+
{PTS({10/1 = 10.000000}), DTS({5/1 = 5.000000}), duration({2/1 = 2.000000}), flags(0), generation(1)}
21+
END OF TEST
22+

‎LayoutTests/media/media-source/media-source-samples-out-of-order-erase-sync-frames.html‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
<!DOCTYPE html>
2+
<html>
3+
<head>
4+
<title>media-source-samples-out-of-order</title>
5+
<script src="mock-media-source.js"></script>
6+
<script src="../video-test.js"></script>
7+
<script>
8+
var source;
9+
var sourceBuffer;
10+
var initSegment;
11+
var mediaSegment;
12+
13+
if (window.internals)
14+
internals.initializeMockMediaSource();
15+
16+
window.addEventListener('load', async event => {
17+
findMediaElement();
18+
19+
source = new MediaSource();
20+
run('video.src = URL.createObjectURL(source)');
21+
await waitFor(source, 'sourceopen');
22+
23+
run('sourceBuffer = source.addSourceBuffer("video/mock; codecs=mock")');
24+
consoleWrite('First segment has normal, monotonically increasing samples, and ends with two key frames.');
25+
mediaSegment = concatenateSamples([
26+
makeAInit(4, [makeATrack(0, 'mock', TRACK_KIND.VIDEO)]),
27+
makeASample(0, 0, 2, 1, 0, SAMPLE_FLAG.SYNC, 0),
28+
makeASample(2, 2, 2, 1, 0, SAMPLE_FLAG.NONE, 0),
29+
makeASample(4, 4, 2, 1, 0, SAMPLE_FLAG.SYNC, 0),
30+
makeASample(6, 6, 2, 1, 0, SAMPLE_FLAG.SYNC, 0),
31+
]);
32+
run('sourceBuffer.appendBuffer(mediaSegment)');
33+
await waitFor(sourceBuffer, 'updateend');
34+
35+
bufferedSamples = await internals.bufferedSamplesForTrackId(sourceBuffer, 0);
36+
testExpected("bufferedSamples.length", 4);
37+
bufferedSamples.forEach(consoleWrite);
38+
39+
consoleWrite('Second, overlapping segment has out-of-display-order samples. This append should replace the last two samples from the previous append.');
40+
mediaSegment = concatenateSamples([
41+
makeAInit(3, [makeATrack(0, 'mock', TRACK_KIND.VIDEO)]),
42+
makeASample(8, 3, 2, 1, 0, SAMPLE_FLAG.SYNC, 1),
43+
makeASample(10, 5, 2, 1, 0, SAMPLE_FLAG.NONE, 1),
44+
]);
45+
run('sourceBuffer.appendBuffer(mediaSegment)');
46+
await waitFor(sourceBuffer, 'updateend');
47+
48+
bufferedSamples = await internals.bufferedSamplesForTrackId(sourceBuffer, 0);
49+
testExpected("bufferedSamples.length", 4);
50+
bufferedSamples.forEach(consoleWrite);
51+
52+
endTest();
53+
});
54+
</script>
55+
</head>
56+
<body>
57+
<video></video>
58+
</body>
59+
</html>

‎Source/WebCore/platform/graphics/SourceBufferPrivate.cpp‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -956,11 +956,11 @@ bool SourceBufferPrivate::processMediaSample(SourceBufferPrivateClient& client,
956956
}
957957

958958
// When appending media containing B-frames (media whose samples' presentation timestamps
959-
// do not increase monotonically, the prior erase steps could leave a sample in the trackBuffer
959+
// do not increase monotonically, the prior erase steps could leave samples in the trackBuffer
960960
// which will be disconnected from its previous I-frame. If the incoming frame is an I-frame,
961961
// remove all samples in decode order between the incoming I-frame's decode timestamp and the
962-
// next I-frame. See <https://github.com/w3c/media-source/issues/187> for a discussion of what
963-
// the how the MSE specification should handlie this secnario.
962+
// next I-frame that is presented after the incoming I-frame. See <https://github.com/w3c/media-source/issues/187>
963+
// for a discussion of how the MSE specification should handle this scenario.
964964
do {
965965
if (!sample->isSync())
966966
break;
@@ -970,10 +970,13 @@ bool SourceBufferPrivate::processMediaSample(SourceBufferPrivateClient& client,
970970
if (nextSampleInDecodeOrder == trackBuffer.samples().decodeOrder().end())
971971
break;
972972

973-
if (nextSampleInDecodeOrder->second->isSync())
973+
if (nextSampleInDecodeOrder->second->isSync() && nextSampleInDecodeOrder->second->presentationTime() > sample->presentationTime())
974974
break;
975975

976976
auto nextSyncSample = trackBuffer.samples().decodeOrder().findSyncSampleAfterDecodeIterator(nextSampleInDecodeOrder);
977+
while (nextSyncSample != trackBuffer.samples().decodeOrder().end() && nextSyncSample->second->presentationTime() <= sample->presentationTime())
978+
nextSyncSample = trackBuffer.samples().decodeOrder().findSyncSampleAfterDecodeIterator(nextSyncSample);
979+
977980
INFO_LOG(LOGIDENTIFIER, "Discovered out-of-order frames, from: ", nextSampleInDecodeOrder->second.get(), " to: ", (nextSyncSample == trackBuffer.samples().decodeOrder().end() ? "[end]"_s : toString(nextSyncSample->second.get())));
978981
erasedSamples.addRange(nextSampleInDecodeOrder, nextSyncSample);
979982
} while (false);

0 commit comments

Comments
 (0)