[MSE] Removing samples when presentation order does not match decode order can cause...
authorjer.noble@apple.com <jer.noble@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 14 Jul 2017 20:08:05 +0000 (20:08 +0000)
committerjer.noble@apple.com <jer.noble@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 14 Jul 2017 20:08:05 +0000 (20:08 +0000)
https://bugs.webkit.org/show_bug.cgi?id=174514

Reviewed by Sam Weinig.

Source/WebCore:

Test: media/media-source/media-source-remove-decodeorder-crash.html

Fix the algorithm in removeCodedFrames() so that it's not possible to have a removePresentationStart >
removePresentationEnd (and also removeDecodeStart > removeDecodeEnd).

* Modules/mediasource/SampleMap.cpp:
(WebCore::PresentationOrderSampleMap::findSampleContainingOrAfterPresentationTime):
(WebCore::PresentationOrderSampleMap::findSampleStartingAfterPresentationTime):
* Modules/mediasource/SampleMap.h:
* Modules/mediasource/SourceBuffer.cpp:
(WebCore::SourceBuffer::removeCodedFrames):

Tools:

* TestWebKitAPI/Tests/WebCore/SampleMap.cpp:
(TestWebKitAPI::TEST_F):

LayoutTests:

* media/media-source/media-source-remove-decodeorder-crash-expected.txt: Added.
* media/media-source/media-source-remove-decodeorder-crash.html: Added.

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@219519 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/media/media-source/media-source-remove-decodeorder-crash-expected.txt [new file with mode: 0644]
LayoutTests/media/media-source/media-source-remove-decodeorder-crash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/Modules/mediasource/SampleMap.cpp
Source/WebCore/Modules/mediasource/SampleMap.h
Source/WebCore/Modules/mediasource/SourceBuffer.cpp
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebCore/SampleMap.cpp

index 312eab5..7eb7dbc 100644 (file)
@@ -1,3 +1,13 @@
+2017-07-14  Jer Noble  <jer.noble@apple.com>
+
+        [MSE] Removing samples when presentation order does not match decode order can cause bad behavior.
+        https://bugs.webkit.org/show_bug.cgi?id=174514
+
+        Reviewed by Sam Weinig.
+
+        * media/media-source/media-source-remove-decodeorder-crash-expected.txt: Added.
+        * media/media-source/media-source-remove-decodeorder-crash.html: Added.
+
 2017-07-14  Matt Lewis  <jlewis3@apple.com>
 
         Correcting test expectations after mac-expectation changes.
 2017-07-14  Matt Lewis  <jlewis3@apple.com>
 
         Correcting test expectations after mac-expectation changes.
diff --git a/LayoutTests/media/media-source/media-source-remove-decodeorder-crash-expected.txt b/LayoutTests/media/media-source/media-source-remove-decodeorder-crash-expected.txt
new file mode 100644 (file)
index 0000000..8d3cccb
--- /dev/null
@@ -0,0 +1,16 @@
+This tests the SourceBuffer.remove() API. The test removes samples where the decode and presentation orders differ. Should not crash.
+
+RUN(video.src = URL.createObjectURL(source))
+EVENT(sourceopen)
+RUN(sourceBuffer = source.addSourceBuffer("video/mock; codecs=mock"))
+RUN(sourceBuffer.appendBuffer(initSegment))
+EVENT(updateend)
+RUN(sourceBuffer.appendBuffer(samples))
+EVENT(updateend)
+RUN(sourceBuffer.remove(1.9, 2))
+EVENT(updateend)
+EXPECTED (sourceBuffer.buffered.length == '1') OK
+EXPECTED (sourceBuffer.buffered.start(0) == '0') OK
+EXPECTED (sourceBuffer.buffered.end(0) == '1') OK
+END OF TEST
+
diff --git a/LayoutTests/media/media-source/media-source-remove-decodeorder-crash.html b/LayoutTests/media/media-source/media-source-remove-decodeorder-crash.html
new file mode 100644 (file)
index 0000000..eaebb82
--- /dev/null
@@ -0,0 +1,58 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <title>mock-media-source</title>
+    <script src="mock-media-source.js"></script>
+    <script src="../video-test.js"></script>
+    <script>
+    var source;
+    var sourceBuffer;
+    var initSegment;
+
+    if (window.internals)
+        internals.initializeMockMediaSource();
+
+    function runTest() {
+        findMediaElement();
+
+        source = new MediaSource();
+        waitForEventOn(source, 'sourceopen', sourceOpen);
+        run('video.src = URL.createObjectURL(source)');
+    }
+
+    function sourceOpen() {
+        run('sourceBuffer = source.addSourceBuffer("video/mock; codecs=mock")');
+        waitForEventOn(sourceBuffer, 'updateend', loadSamples, false, true);
+        initSegment = makeAInit(8, [makeATrack(1, 'mock', TRACK_KIND.VIDEO)]);
+        run('sourceBuffer.appendBuffer(initSegment)');
+    }
+
+    function loadSamples() {
+        samples = concatenateSamples([
+            makeASample(0, 0, 1, 1, SAMPLE_FLAG.SYNC),
+            makeASample(2, 1, 1, 1, SAMPLE_FLAG.NONE),
+            makeASample(1, 2, 1, 1, SAMPLE_FLAG.SYNC),
+        ]);
+        waitForEventOn(sourceBuffer, 'updateend', remove, false, true);
+        run('sourceBuffer.appendBuffer(samples)');
+    }
+
+    function remove() {
+        waitForEventOn(sourceBuffer, 'updateend', checkRemoved, false, true);
+        run('sourceBuffer.remove(1.9, 2)');
+    }
+
+    function checkRemoved() {
+        testExpected('sourceBuffer.buffered.length', 1);
+        testExpected('sourceBuffer.buffered.start(0)', 0);
+        testExpected('sourceBuffer.buffered.end(0)', 1);
+        endTest();
+    }
+
+    </script>
+</head>
+<body onload="runTest()">
+    <div>This tests the SourceBuffer.remove() API. The test removes samples where the decode and presentation orders differ. Should not crash.</div>
+    <video></video>
+</body>
+</html>
index 0e9b0ef..fd1e5ed 100644 (file)
@@ -1,3 +1,22 @@
+2017-07-14  Jer Noble  <jer.noble@apple.com>
+
+        [MSE] Removing samples when presentation order does not match decode order can cause bad behavior.
+        https://bugs.webkit.org/show_bug.cgi?id=174514
+
+        Reviewed by Sam Weinig.
+
+        Test: media/media-source/media-source-remove-decodeorder-crash.html
+
+        Fix the algorithm in removeCodedFrames() so that it's not possible to have a removePresentationStart >
+        removePresentationEnd (and also removeDecodeStart > removeDecodeEnd).
+
+        * Modules/mediasource/SampleMap.cpp:
+        (WebCore::PresentationOrderSampleMap::findSampleContainingOrAfterPresentationTime):
+        (WebCore::PresentationOrderSampleMap::findSampleStartingAfterPresentationTime):
+        * Modules/mediasource/SampleMap.h:
+        * Modules/mediasource/SourceBuffer.cpp:
+        (WebCore::SourceBuffer::removeCodedFrames):
+
 2017-07-14  Youenn Fablet  <youenn@apple.com>
 
         Increase CoreAudio render audio buffer sizes for WebRTC
 2017-07-14  Youenn Fablet  <youenn@apple.com>
 
         Increase CoreAudio render audio buffer sizes for WebRTC
index 42157c5..af5f4e4 100644 (file)
@@ -156,11 +156,35 @@ PresentationOrderSampleMap::iterator PresentationOrderSampleMap::findSampleConta
     return end();
 }
 
     return end();
 }
 
+PresentationOrderSampleMap::iterator PresentationOrderSampleMap::findSampleContainingOrAfterPresentationTime(const MediaTime& time)
+{
+    if (m_samples.empty())
+        return end();
+
+    // upper_bound will return the first sample whose presentation start time is greater than the search time.
+    // If this is the first sample, that means no sample in the map contains the requested time.
+    auto iter = m_samples.upper_bound(time);
+    if (iter == begin())
+        return iter;
+
+    // Look at the previous sample; does it contain the requested time?
+    --iter;
+    MediaSample& sample = *iter->second;
+    if (sample.presentationTime() + sample.duration() > time)
+        return iter;
+    return ++iter;
+}
+
 PresentationOrderSampleMap::iterator PresentationOrderSampleMap::findSampleStartingOnOrAfterPresentationTime(const MediaTime& time)
 {
     return m_samples.lower_bound(time);
 }
 
 PresentationOrderSampleMap::iterator PresentationOrderSampleMap::findSampleStartingOnOrAfterPresentationTime(const MediaTime& time)
 {
     return m_samples.lower_bound(time);
 }
 
+PresentationOrderSampleMap::iterator PresentationOrderSampleMap::findSampleStartingAfterPresentationTime(const MediaTime& time)
+{
+    return m_samples.upper_bound(time);
+}
+
 DecodeOrderSampleMap::iterator DecodeOrderSampleMap::findSampleWithDecodeKey(const KeyType& key)
 {
     return m_samples.find(key);
 DecodeOrderSampleMap::iterator DecodeOrderSampleMap::findSampleWithDecodeKey(const KeyType& key)
 {
     return m_samples.find(key);
index d7ce03f..d6285bf 100644 (file)
@@ -57,7 +57,9 @@ public:
 
     WEBCORE_EXPORT iterator findSampleWithPresentationTime(const MediaTime&);
     WEBCORE_EXPORT iterator findSampleContainingPresentationTime(const MediaTime&);
 
     WEBCORE_EXPORT iterator findSampleWithPresentationTime(const MediaTime&);
     WEBCORE_EXPORT iterator findSampleContainingPresentationTime(const MediaTime&);
+    WEBCORE_EXPORT iterator findSampleContainingOrAfterPresentationTime(const MediaTime&);
     WEBCORE_EXPORT iterator findSampleStartingOnOrAfterPresentationTime(const MediaTime&);
     WEBCORE_EXPORT iterator findSampleStartingOnOrAfterPresentationTime(const MediaTime&);
+    WEBCORE_EXPORT iterator findSampleStartingAfterPresentationTime(const MediaTime&);
     WEBCORE_EXPORT reverse_iterator reverseFindSampleContainingPresentationTime(const MediaTime&);
     WEBCORE_EXPORT reverse_iterator reverseFindSampleBeforePresentationTime(const MediaTime&);
     WEBCORE_EXPORT iterator_range findSamplesBetweenPresentationTimes(const MediaTime&, const MediaTime&);
     WEBCORE_EXPORT reverse_iterator reverseFindSampleContainingPresentationTime(const MediaTime&);
     WEBCORE_EXPORT reverse_iterator reverseFindSampleBeforePresentationTime(const MediaTime&);
     WEBCORE_EXPORT iterator_range findSamplesBetweenPresentationTimes(const MediaTime&, const MediaTime&);
index 4ef3256..b563d77 100644 (file)
@@ -742,6 +742,8 @@ void SourceBuffer::removeCodedFrames(const MediaTime& start, const MediaTime& en
         // 3.1. Let remove end timestamp be the current value of duration
         // 3.2 If this track buffer has a random access point timestamp that is greater than or equal to end, then update
         // remove end timestamp to that random access point timestamp.
         // 3.1. Let remove end timestamp be the current value of duration
         // 3.2 If this track buffer has a random access point timestamp that is greater than or equal to end, then update
         // remove end timestamp to that random access point timestamp.
+        // NOTE: Step 3.2 will be incorrect for any random access point timestamp whose decode time is later than the sample at end,
+        // but whose presentation time is less than the sample at end. Skip this step until step 3.3 below.
 
         // NOTE: To handle MediaSamples which may be an amalgamation of multiple shorter samples, find samples whose presentation
         // interval straddles the start and end times, and divide them if possible:
 
         // NOTE: To handle MediaSamples which may be an amalgamation of multiple shorter samples, find samples whose presentation
         // interval straddles the start and end times, and divide them if possible:
@@ -766,17 +768,9 @@ void SourceBuffer::removeCodedFrames(const MediaTime& start, const MediaTime& en
         divideSampleIfPossibleAtPresentationTime(start);
         divideSampleIfPossibleAtPresentationTime(end);
 
         divideSampleIfPossibleAtPresentationTime(start);
         divideSampleIfPossibleAtPresentationTime(end);
 
-        // NOTE: findSyncSampleAfterPresentationTime will return the next sync sample on or after the presentation time
-        // or decodeOrder().end() if no sync sample exists after that presentation time.
-        DecodeOrderSampleMap::iterator removeDecodeEnd = trackBuffer.samples.decodeOrder().findSyncSampleAfterPresentationTime(end);
-        PresentationOrderSampleMap::iterator removePresentationEnd;
-        if (removeDecodeEnd == trackBuffer.samples.decodeOrder().end())
-            removePresentationEnd = trackBuffer.samples.presentationOrder().end();
-        else
-            removePresentationEnd = trackBuffer.samples.presentationOrder().findSampleWithPresentationTime(removeDecodeEnd->second->presentationTime());
-
-        PresentationOrderSampleMap::iterator removePresentationStart = trackBuffer.samples.presentationOrder().findSampleStartingOnOrAfterPresentationTime(start);
-        if (removePresentationStart == removePresentationEnd)
+        auto removePresentationStart = trackBuffer.samples.presentationOrder().findSampleContainingOrAfterPresentationTime(start);
+        auto removePresentationEnd = trackBuffer.samples.presentationOrder().findSampleStartingAfterPresentationTime(end);
+        if (start == end)
             continue;
 
         // 3.3 Remove all media data, from this track buffer, that contain starting timestamps greater than or equal to
             continue;
 
         // 3.3 Remove all media data, from this track buffer, that contain starting timestamps greater than or equal to
@@ -784,9 +778,12 @@ void SourceBuffer::removeCodedFrames(const MediaTime& start, const MediaTime& en
         // NOTE: frames must be removed in decode order, so that all dependant frames between the frame to be removed
         // and the next sync sample frame are removed. But we must start from the first sample in decode order, not
         // presentation order.
         // NOTE: frames must be removed in decode order, so that all dependant frames between the frame to be removed
         // and the next sync sample frame are removed. But we must start from the first sample in decode order, not
         // presentation order.
-        PresentationOrderSampleMap::iterator minDecodeTimeIter = std::min_element(removePresentationStart, removePresentationEnd, decodeTimeComparator);
-        DecodeOrderSampleMap::KeyType decodeKey(minDecodeTimeIter->second->decodeTime(), minDecodeTimeIter->second->presentationTime());
-        DecodeOrderSampleMap::iterator removeDecodeStart = trackBuffer.samples.decodeOrder().findSampleWithDecodeKey(decodeKey);
+        auto minmaxDecodeTimeIterPair = std::minmax_element(removePresentationStart, removePresentationEnd, decodeTimeComparator);
+        auto& firstSample = *minmaxDecodeTimeIterPair.first->second;
+        auto& lastSample = *minmaxDecodeTimeIterPair.second->second;
+        auto removeDecodeStart = trackBuffer.samples.decodeOrder().findSampleWithDecodeKey({firstSample.decodeTime(), firstSample.presentationTime()});
+        auto removeDecodeLast = trackBuffer.samples.decodeOrder().findSampleWithDecodeKey({lastSample.decodeTime(), lastSample.presentationTime()});
+        auto removeDecodeEnd = trackBuffer.samples.decodeOrder().findSyncSampleAfterDecodeIterator(removeDecodeLast);
 
         DecodeOrderSampleMap::MapType erasedSamples(removeDecodeStart, removeDecodeEnd);
         PlatformTimeRanges erasedRanges = removeSamplesFromTrackBuffer(erasedSamples, trackBuffer, this, "removeCodedFrames");
 
         DecodeOrderSampleMap::MapType erasedSamples(removeDecodeStart, removeDecodeEnd);
         PlatformTimeRanges erasedRanges = removeSamplesFromTrackBuffer(erasedSamples, trackBuffer, this, "removeCodedFrames");
index 1ee963a..caa9f4d 100644 (file)
@@ -1,3 +1,13 @@
+2017-07-14  Jer Noble  <jer.noble@apple.com>
+
+        [MSE] Removing samples when presentation order does not match decode order can cause bad behavior.
+        https://bugs.webkit.org/show_bug.cgi?id=174514
+
+        Reviewed by Sam Weinig.
+
+        * TestWebKitAPI/Tests/WebCore/SampleMap.cpp:
+        (TestWebKitAPI::TEST_F):
+
 2017-07-14  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         [WTF] Newly added AtomicStringImpl should use BufferInternal static string if StringImpl is static
 2017-07-14  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         [WTF] Newly added AtomicStringImpl should use BufferInternal static string if StringImpl is static
index f7e91c6..3363a6b 100644 (file)
@@ -153,6 +153,27 @@ TEST_F(SampleMapTest, findSampleStartingOnOrAfterPresentationTime)
     EXPECT_TRUE(presentationMap.end() == presentationMap.findSampleContainingPresentationTime(MediaTime(20, 1)));
 }
 
     EXPECT_TRUE(presentationMap.end() == presentationMap.findSampleContainingPresentationTime(MediaTime(20, 1)));
 }
 
+TEST_F(SampleMapTest, findSampleContainingOrAfterPresentationTime)
+{
+    auto& presentationMap = map.presentationOrder();
+    EXPECT_EQ(MediaTime(0, 1), presentationMap.findSampleContainingOrAfterPresentationTime(MediaTime(0, 1))->second->presentationTime());
+    EXPECT_EQ(MediaTime(19, 1), presentationMap.findSampleContainingOrAfterPresentationTime(MediaTime(19, 1))->second->presentationTime());
+    EXPECT_EQ(MediaTime(0, 1), presentationMap.findSampleContainingOrAfterPresentationTime(MediaTime(1, 2))->second->presentationTime());
+    EXPECT_EQ(MediaTime(0, 1), presentationMap.findSampleContainingOrAfterPresentationTime(MediaTime(-1, 1))->second->presentationTime());
+    EXPECT_EQ(MediaTime(11, 1), presentationMap.findSampleContainingOrAfterPresentationTime(MediaTime(10, 1))->second->presentationTime());
+    EXPECT_TRUE(presentationMap.end() == presentationMap.findSampleContainingOrAfterPresentationTime(MediaTime(20, 1)));
+}
+
+TEST_F(SampleMapTest, findSampleStartingAfterPresentationTime)
+{
+    auto& presentationMap = map.presentationOrder();
+    EXPECT_EQ(MediaTime(1, 1), presentationMap.findSampleStartingAfterPresentationTime(MediaTime(0, 1))->second->presentationTime());
+    EXPECT_TRUE(presentationMap.end() == presentationMap.findSampleStartingAfterPresentationTime(MediaTime(19, 1)));
+    EXPECT_EQ(MediaTime(1, 1), presentationMap.findSampleStartingAfterPresentationTime(MediaTime(1, 2))->second->presentationTime());
+    EXPECT_EQ(MediaTime(0, 1), presentationMap.findSampleStartingAfterPresentationTime(MediaTime(-1, 1))->second->presentationTime());
+    EXPECT_EQ(MediaTime(11, 1), presentationMap.findSampleStartingAfterPresentationTime(MediaTime(10, 1))->second->presentationTime());
+    EXPECT_TRUE(presentationMap.end() == presentationMap.findSampleStartingAfterPresentationTime(MediaTime(20, 1)));
+}
 TEST_F(SampleMapTest, findSamplesBetweenPresentationTimes)
 {
     auto& presentationMap = map.presentationOrder();
 TEST_F(SampleMapTest, findSamplesBetweenPresentationTimes)
 {
     auto& presentationMap = map.presentationOrder();