Diff
Modified: trunk/LayoutTests/ChangeLog (219518 => 219519)
--- trunk/LayoutTests/ChangeLog 2017-07-14 20:04:20 UTC (rev 219518)
+++ trunk/LayoutTests/ChangeLog 2017-07-14 20:08:05 UTC (rev 219519)
@@ -1,3 +1,13 @@
+2017-07-14 Jer Noble <jer.no...@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 <jlew...@apple.com>
Correcting test expectations after mac-expectation changes.
Added: trunk/LayoutTests/media/media-source/media-source-remove-decodeorder-crash-expected.txt (0 => 219519)
--- trunk/LayoutTests/media/media-source/media-source-remove-decodeorder-crash-expected.txt (rev 0)
+++ trunk/LayoutTests/media/media-source/media-source-remove-decodeorder-crash-expected.txt 2017-07-14 20:08:05 UTC (rev 219519)
@@ -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 = ""
+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
+
Added: trunk/LayoutTests/media/media-source/media-source-remove-decodeorder-crash.html (0 => 219519)
--- trunk/LayoutTests/media/media-source/media-source-remove-decodeorder-crash.html (rev 0)
+++ trunk/LayoutTests/media/media-source/media-source-remove-decodeorder-crash.html 2017-07-14 20:08:05 UTC (rev 219519)
@@ -0,0 +1,58 @@
+<!DOCTYPE html>
+<html>
+<head>
+ <title>mock-media-source</title>
+ <script src=""
+ <script src=""
+ <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 = ""
+ }
+
+ 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>
Modified: trunk/Source/WebCore/ChangeLog (219518 => 219519)
--- trunk/Source/WebCore/ChangeLog 2017-07-14 20:04:20 UTC (rev 219518)
+++ trunk/Source/WebCore/ChangeLog 2017-07-14 20:08:05 UTC (rev 219519)
@@ -1,3 +1,22 @@
+2017-07-14 Jer Noble <jer.no...@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 <you...@apple.com>
Increase CoreAudio render audio buffer sizes for WebRTC
Modified: trunk/Source/WebCore/Modules/mediasource/SampleMap.cpp (219518 => 219519)
--- trunk/Source/WebCore/Modules/mediasource/SampleMap.cpp 2017-07-14 20:04:20 UTC (rev 219518)
+++ trunk/Source/WebCore/Modules/mediasource/SampleMap.cpp 2017-07-14 20:08:05 UTC (rev 219519)
@@ -156,11 +156,35 @@
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::findSampleStartingAfterPresentationTime(const MediaTime& time)
+{
+ return m_samples.upper_bound(time);
+}
+
DecodeOrderSampleMap::iterator DecodeOrderSampleMap::findSampleWithDecodeKey(const KeyType& key)
{
return m_samples.find(key);
Modified: trunk/Source/WebCore/Modules/mediasource/SampleMap.h (219518 => 219519)
--- trunk/Source/WebCore/Modules/mediasource/SampleMap.h 2017-07-14 20:04:20 UTC (rev 219518)
+++ trunk/Source/WebCore/Modules/mediasource/SampleMap.h 2017-07-14 20:08:05 UTC (rev 219519)
@@ -57,7 +57,9 @@
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 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&);
Modified: trunk/Source/WebCore/Modules/mediasource/SourceBuffer.cpp (219518 => 219519)
--- trunk/Source/WebCore/Modules/mediasource/SourceBuffer.cpp 2017-07-14 20:04:20 UTC (rev 219518)
+++ trunk/Source/WebCore/Modules/mediasource/SourceBuffer.cpp 2017-07-14 20:08:05 UTC (rev 219519)
@@ -742,6 +742,8 @@
// 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:
@@ -766,17 +768,9 @@
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
@@ -784,9 +778,12 @@
// 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");
Modified: trunk/Tools/ChangeLog (219518 => 219519)
--- trunk/Tools/ChangeLog 2017-07-14 20:04:20 UTC (rev 219518)
+++ trunk/Tools/ChangeLog 2017-07-14 20:08:05 UTC (rev 219519)
@@ -1,3 +1,13 @@
+2017-07-14 Jer Noble <jer.no...@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....@gmail.com>
[WTF] Newly added AtomicStringImpl should use BufferInternal static string if StringImpl is static
Modified: trunk/Tools/TestWebKitAPI/Tests/WebCore/SampleMap.cpp (219518 => 219519)
--- trunk/Tools/TestWebKitAPI/Tests/WebCore/SampleMap.cpp 2017-07-14 20:04:20 UTC (rev 219518)
+++ trunk/Tools/TestWebKitAPI/Tests/WebCore/SampleMap.cpp 2017-07-14 20:08:05 UTC (rev 219519)
@@ -153,6 +153,27 @@
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();
