Merge r236566 - [MSE] Fix unwanted sample erase from the decode queue

https://bugs.webkit.org/show_bug.cgi?id=180643 Reviewed by Jer Noble. Source/WebCore: Test: media/media-source/media-source-append-acb-no-frame-lost.html This bug reproduced when unordered appends were made. For instance, if the application appended [0, 10) and then [20, 30), the frame at 20 would be wrongly discarded from the decode queue. Later the application could append [10, 20) and the gap at [20, 21) would persist in the decode queue, even if the frame remained in the track buffer table. Thanks to Daniel Zhang for reporting the issue. * Modules/mediasource/SourceBuffer.cpp: (WebCore::SourceBuffer::provideMediaData): LayoutTests: Added a test case for the fixed bug. * media/media-source/media-source-append-acb-no-frame-lost.html: Added.
WebKit · Oct 1, 2018 · bbed884 · bbed884
1 parent 69c7153
commit bbed884
Show file tree

Hide file tree

Showing 5 changed files with 153 additions and 1 deletion.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,14 @@
+2018-09-27  Alicia Boya García  <aboya@igalia.com>
+
+        [MSE] Fix unwanted sample erase from the decode queue
+        https://bugs.webkit.org/show_bug.cgi?id=180643
+
+        Reviewed by Jer Noble.
+
+        Added a test case for the fixed bug.
+
+        * media/media-source/media-source-append-acb-no-frame-lost.html: Added.
+
 2018-09-15  Rob Buis  <rbuis@igalia.com>
 
         XMLHttpRequest::createResponseBlob() should create a Blob with type for empty response

diff --git a/LayoutTests/media/media-source/media-source-append-acb-no-frame-lost-expected.txt b/LayoutTests/media/media-source/media-source-append-acb-no-frame-lost-expected.txt
@@ -0,0 +1,46 @@
+
+EXPECTED (source.readyState == 'closed') OK
+EVENT(sourceopen)
+RUN(sourceBuffer = source.addSourceBuffer("video/mock; codecs=mock"))
+RUN(sourceBuffer.appendBuffer(initSegment))
+EVENT(updateend)
+RUN(sourceBuffer.appendBuffer(syncSampleRun(0, 10)))
+EVENT(updateend)
+RUN(sourceBuffer.appendBuffer(syncSampleRun(20, 30)))
+EVENT(updateend)
+RUN(sourceBuffer.appendBuffer(syncSampleRun(10, 20)))
+EVENT(updateend)
+EXPECTED (bufferedSamples.length == '30') OK
+EXPECTED (enqueuedSamples.length == '30') OK
+{PTS({0/1000 = 0.000000}), DTS({0/1000 = 0.000000}), duration({1000/1000 = 1.000000}), flags(1), generation(0)}
+{PTS({1000/1000 = 1.000000}), DTS({1000/1000 = 1.000000}), duration({1000/1000 = 1.000000}), flags(0), generation(0)}
+{PTS({2000/1000 = 2.000000}), DTS({2000/1000 = 2.000000}), duration({1000/1000 = 1.000000}), flags(0), generation(0)}
+{PTS({3000/1000 = 3.000000}), DTS({3000/1000 = 3.000000}), duration({1000/1000 = 1.000000}), flags(0), generation(0)}
+{PTS({4000/1000 = 4.000000}), DTS({4000/1000 = 4.000000}), duration({1000/1000 = 1.000000}), flags(0), generation(0)}
+{PTS({5000/1000 = 5.000000}), DTS({5000/1000 = 5.000000}), duration({1000/1000 = 1.000000}), flags(0), generation(0)}
+{PTS({6000/1000 = 6.000000}), DTS({6000/1000 = 6.000000}), duration({1000/1000 = 1.000000}), flags(0), generation(0)}
+{PTS({7000/1000 = 7.000000}), DTS({7000/1000 = 7.000000}), duration({1000/1000 = 1.000000}), flags(0), generation(0)}
+{PTS({8000/1000 = 8.000000}), DTS({8000/1000 = 8.000000}), duration({1000/1000 = 1.000000}), flags(0), generation(0)}
+{PTS({9000/1000 = 9.000000}), DTS({9000/1000 = 9.000000}), duration({1000/1000 = 1.000000}), flags(0), generation(0)}
+{PTS({10000/1000 = 10.000000}), DTS({10000/1000 = 10.000000}), duration({1000/1000 = 1.000000}), flags(1), generation(0)}
+{PTS({11000/1000 = 11.000000}), DTS({11000/1000 = 11.000000}), duration({1000/1000 = 1.000000}), flags(0), generation(0)}
+{PTS({12000/1000 = 12.000000}), DTS({12000/1000 = 12.000000}), duration({1000/1000 = 1.000000}), flags(0), generation(0)}
+{PTS({13000/1000 = 13.000000}), DTS({13000/1000 = 13.000000}), duration({1000/1000 = 1.000000}), flags(0), generation(0)}
+{PTS({14000/1000 = 14.000000}), DTS({14000/1000 = 14.000000}), duration({1000/1000 = 1.000000}), flags(0), generation(0)}
+{PTS({15000/1000 = 15.000000}), DTS({15000/1000 = 15.000000}), duration({1000/1000 = 1.000000}), flags(0), generation(0)}
+{PTS({16000/1000 = 16.000000}), DTS({16000/1000 = 16.000000}), duration({1000/1000 = 1.000000}), flags(0), generation(0)}
+{PTS({17000/1000 = 17.000000}), DTS({17000/1000 = 17.000000}), duration({1000/1000 = 1.000000}), flags(0), generation(0)}
+{PTS({18000/1000 = 18.000000}), DTS({18000/1000 = 18.000000}), duration({1000/1000 = 1.000000}), flags(0), generation(0)}
+{PTS({19000/1000 = 19.000000}), DTS({19000/1000 = 19.000000}), duration({1000/1000 = 1.000000}), flags(0), generation(0)}
+{PTS({20000/1000 = 20.000000}), DTS({20000/1000 = 20.000000}), duration({1000/1000 = 1.000000}), flags(1), generation(0)}
+{PTS({21000/1000 = 21.000000}), DTS({21000/1000 = 21.000000}), duration({1000/1000 = 1.000000}), flags(0), generation(0)}
+{PTS({22000/1000 = 22.000000}), DTS({22000/1000 = 22.000000}), duration({1000/1000 = 1.000000}), flags(0), generation(0)}
+{PTS({23000/1000 = 23.000000}), DTS({23000/1000 = 23.000000}), duration({1000/1000 = 1.000000}), flags(0), generation(0)}
+{PTS({24000/1000 = 24.000000}), DTS({24000/1000 = 24.000000}), duration({1000/1000 = 1.000000}), flags(0), generation(0)}
+{PTS({25000/1000 = 25.000000}), DTS({25000/1000 = 25.000000}), duration({1000/1000 = 1.000000}), flags(0), generation(0)}
+{PTS({26000/1000 = 26.000000}), DTS({26000/1000 = 26.000000}), duration({1000/1000 = 1.000000}), flags(0), generation(0)}
+{PTS({27000/1000 = 27.000000}), DTS({27000/1000 = 27.000000}), duration({1000/1000 = 1.000000}), flags(0), generation(0)}
+{PTS({28000/1000 = 28.000000}), DTS({28000/1000 = 28.000000}), duration({1000/1000 = 1.000000}), flags(0), generation(0)}
+{PTS({29000/1000 = 29.000000}), DTS({29000/1000 = 29.000000}), duration({1000/1000 = 1.000000}), flags(0), generation(0)}
+END OF TEST
+
diff --git a/LayoutTests/media/media-source/media-source-append-acb-no-frame-lost.html b/LayoutTests/media/media-source/media-source-append-acb-no-frame-lost.html
@@ -0,0 +1,71 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <title>media-source-append-acb-no-frame-lost</title>
+    <script src="mock-media-source.js"></script>
+    <script src="../video-test.js"></script>
+    <script>
+    var source;
+    var sourceBuffer;
+    var initSegment;
+    var bufferedSamples;
+    var enqueuedSamples;
+
+    if (window.internals)
+        internals.initializeMockMediaSource();
+
+    function syncSampleRun(start, end) {
+        const samples = [];
+        for (let time = start; time < end; time++)
+            samples.push(makeASample(time, time, 1, 1, time === start ? SAMPLE_FLAG.SYNC : SAMPLE_FLAG.NONE));
+        return concatenateSamples(samples);
+    }
+
+    window.addEventListener('load', async () => {
+        findMediaElement();
+        source = new MediaSource();
+        testExpected('source.readyState', 'closed');
+        const sourceOpened = waitFor(source, 'sourceopen');
+
+        const videoSource = document.createElement('source');
+        videoSource.type = 'video/mock; codecs=mock';
+        videoSource.src = URL.createObjectURL(source);
+        video.appendChild(videoSource);
+
+        await sourceOpened;
+        run('sourceBuffer = source.addSourceBuffer("video/mock; codecs=mock")');
+
+        initSegment = makeAInit(30, [makeATrack(1, 'mock', TRACK_KIND.VIDEO)]);
+
+        run('sourceBuffer.appendBuffer(initSegment)');
+        await waitFor(sourceBuffer, 'updateend');
+
+        // Segment A
+        run('sourceBuffer.appendBuffer(syncSampleRun(0, 10))');
+        await waitFor(sourceBuffer, 'updateend');
+
+        // Segment C
+        run('sourceBuffer.appendBuffer(syncSampleRun(20, 30))');
+        await waitFor(sourceBuffer, 'updateend');
+
+        // Segment B
+        run('sourceBuffer.appendBuffer(syncSampleRun(10, 20))');
+        await waitFor(sourceBuffer, 'updateend');
+
+        bufferedSamples = internals.bufferedSamplesForTrackID(sourceBuffer, 1);
+        enqueuedSamples = internals.enqueuedSamplesForTrackID(sourceBuffer, 1);
+
+        // 30 samples were appended in total. All of them should have ended up in buffered samples and in the decode queue.
+        testExpected('bufferedSamples.length', 30);
+        testExpected('enqueuedSamples.length', 30);
+
+        enqueuedSamples.forEach(consoleWrite);
+
+        endTest();
+    });
+    </script>
+</head>
+<body>
+    <video controls></video>
+</body>
+</html>
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,25 @@
+2018-09-27  Alicia Boya García  <aboya@igalia.com>
+
+        [MSE] Fix unwanted sample erase from the decode queue
+        https://bugs.webkit.org/show_bug.cgi?id=180643
+
+        Reviewed by Jer Noble.
+
+        Test: media/media-source/media-source-append-acb-no-frame-lost.html
+
+        This bug reproduced when unordered appends were made. For instance, if
+        the application appended [0, 10) and then [20, 30), the frame at 20
+        would be wrongly discarded from the decode queue.
+
+        Later the application could append [10, 20) and the gap at [20, 21)
+        would persist in the decode queue, even if the frame remained in the
+        track buffer table.
+
+        Thanks to Daniel Zhang for reporting the issue.
+
+        * Modules/mediasource/SourceBuffer.cpp:
+        (WebCore::SourceBuffer::provideMediaData):
+
 2018-09-24  Alicia Boya García  <aboya@igalia.com>
 
         [MSE][GStreamer] Pull demuxed samples in batches

diff --git a/Source/WebCore/Modules/mediasource/SourceBuffer.cpp b/Source/WebCore/Modules/mediasource/SourceBuffer.cpp
@@ -1861,7 +1861,6 @@ void SourceBuffer::provideMediaData(TrackBuffer& trackBuffer, const AtomicString
         // against re-entrancy introduces a small inefficency when removing appended samples from the decode queue one at a time
         // rather than when all samples have been enqueued.
         auto sample = trackBuffer.decodeQueue.begin()->second;
-        trackBuffer.decodeQueue.erase(trackBuffer.decodeQueue.begin());
 
         // Do not enqueue samples spanning a significant unbuffered gap.
         // NOTE: one second is somewhat arbitrary. MediaSource::monitorSourceBuffers() is run
@@ -1874,6 +1873,9 @@ void SourceBuffer::provideMediaData(TrackBuffer& trackBuffer, const AtomicString
         if (trackBuffer.lastEnqueuedDecodeEndTime.isValid() && sample->decodeTime() - trackBuffer.lastEnqueuedDecodeEndTime > oneSecond)
             break;
 
+        // Remove the sample from the decode queue now.
+        trackBuffer.decodeQueue.erase(trackBuffer.decodeQueue.begin());
+
         trackBuffer.lastEnqueuedPresentationTime = sample->presentationTime();
         trackBuffer.lastEnqueuedDecodeEndTime = sample->decodeTime() + sample->duration();
         m_private->enqueueSample(sample.releaseNonNull(), trackID);