Bug 177884 – SourceBuffer remove throws out way more content than requested

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Joey Parrish]

SourceBuffer remove in Safari 11 removes way more content than requested.  In this particular case, we're asking to remove time 0 through 10s, and Safari is removing times 0 through 15s.

Fails in Safari 11.0 (11604.1.38.1.7) and WebKit nightly (r222776).

Passes in Chrome 61 (61.0.3163.100) and Safari 10.0.3 (12602.4.8).

Simple repro page: http://storage.googleapis.com/shaka-demo-assets/_bugs/safari-11-mse-remove/index.html

[Joey Parrish]

It appears that the issue is removal of a range with an end time corresponding exactly to the PTS of a key frame.  Safari 11 seems to treat this as including the key frame, which then causes it to remove up to the next key frame.

As far as I can tell, the end of the removal range should be exclusive, not inclusive.

[Jer Noble]

Joey, that's exactly what's happening.  Here's a simple solution:

@@ -767,7 +767,7 @@ void SourceBuffer::removeCodedFrames(const MediaTime& start, const MediaTime& en
         divideSampleIfPossibleAtPresentationTime(end);
 
         auto removePresentationStart = trackBuffer.samples.presentationOrder().findSampleContainingOrAfterPresentationTime(start);
-        auto removePresentationEnd = trackBuffer.samples.presentationOrder().findSampleStartingAfterPresentationTime(end);
+        auto removePresentationEnd = trackBuffer.samples.presentationOrder().findSampleStartingOnOrAfterPresentationTime(end);
         if (removePresentationStart == removePresentationEnd)
             continue;

[Radar WebKit Bug Importer]

<rdar://problem/34817104>

[Jer Noble]

But please note, because you're comparing floating point numbers to rational integer numbers, there will always be the potential for rounding errors. While "double 10.0" can accurately represent "122880/12288", the same isn't necessarily true of "10.041667" and "123392/12288".  So in general, you should probably target the middle of the last to-be-removed frame, not the exact start of the first to-remain frame.

[Joey Parrish]

Perhaps ideally, applications would never target specific frames.

Shaka Player's integration tests target exact segment start times, and fairly arbitrarily, which is how we tripped over this.

In a real application, Shaka Player would be choosing removal times based on a fixed offset behind video.currentTime.

In any case, at our level, we don't have knowledge of frame times to begin with, so not only could we not target them deliberately, we also could not avoid them.

[Jer Noble]

Created attachment 322854 [details]
Patch

[Build Bot]

Comment on attachment 322854 [details]
Patch

Attachment 322854 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4769628

New failing tests:
media/media-source/media-source-remove-decodeorder-crash.html

[Build Bot]

Created attachment 322868 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Jer Noble]

Created attachment 322869 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 322869 [details]
Patch

Clearing flags on attachment: 322869

Committed r223029: <http://trac.webkit.org/changeset/223029>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.