search

WebPlatformForEmbedded / WPEWebKit

WPE WebKit port (downstream)
213 stars 136 forks source link

Fix removal of samples being replaced #1117

Closed emutavchi closed 11 months ago

emutavchi commented 1 year ago

During replacement of buffered range, SourceBuffer may fail to remove some samples if its PTS falls in "contiguousFrameTolerance" gap introduced in https://bugs.webkit.org/show_bug.cgi?id=190085.

The problem can be seen with this test https://emutavchi.github.io/tests/mse/mse_sample_replacement_test.html and instrumentation on WebKit side that logs the append number for each sample: https://github.com/emutavchi/WebKitForWayland/commit/8d272af173a4ba23a0922949a02d0c7f79339158

In problematic case you should see several "old" samples left in SourceBuffer after replacement of buffered range completed.

This change tries to preserve the behavior introduced in https://bugs.webkit.org/show_bug.cgi?id=190085, and still allow sample removal for non-sync samples.

eocanha commented 1 year ago

This patch has been submitted for review as https://bugs.webkit.org/show_bug.cgi?id=259397 / https://github.com/WebKit/WebKit/pull/15986, but two layout test regression have been detected:

I've been analyzing the first test. It creates 3 generations (0, 1, 2) of sample batches ([0, 2], [1, 3], [1.5, 3.5]). The third generation should overwrite most of the samples from the second one, but there's a weird sample surviving, one that is "nested" between two of the 3rd generation samples. It can be more easily understood if we examine the buffered ranges after each append:

Buffered after first append:
{PTS({0/6 = 0.000000}), DTS({0/6 = 0.000000}), duration({1/6 = 0.166667}), flags(1), generation(0)}
{PTS({1/6 = 0.166667}), DTS({1/6 = 0.166667}), duration({1/6 = 0.166667}), flags(0), generation(0)}
{PTS({2/6 = 0.333333}), DTS({2/6 = 0.333333}), duration({1/6 = 0.166667}), flags(1), generation(0)}
{PTS({3/6 = 0.500000}), DTS({3/6 = 0.500000}), duration({1/6 = 0.166667}), flags(0), generation(0)}
{PTS({4/6 = 0.666667}), DTS({4/6 = 0.666667}), duration({1/6 = 0.166667}), flags(1), generation(0)}
{PTS({5/6 = 0.833333}), DTS({5/6 = 0.833333}), duration({1/6 = 0.166667}), flags(0), generation(0)}
{PTS({6/6 = 1.000000}), DTS({6/6 = 1.000000}), duration({1/6 = 0.166667}), flags(1), generation(0)}
{PTS({7/6 = 1.166667}), DTS({7/6 = 1.166667}), duration({1/6 = 0.166667}), flags(0), generation(0)}
{PTS({8/6 = 1.333333}), DTS({8/6 = 1.333333}), duration({1/6 = 0.166667}), flags(1), generation(0)}
{PTS({9/6 = 1.500000}), DTS({9/6 = 1.500000}), duration({1/6 = 0.166667}), flags(0), generation(0)}
{PTS({10/6 = 1.666667}), DTS({10/6 = 1.666667}), duration({1/6 = 0.166667}), flags(1), generation(0)}
{PTS({11/6 = 1.833333}), DTS({11/6 = 1.833333}), duration({1/6 = 0.166667}), flags(0), generation(0)}

Buffered after second append:
{PTS({0/6 = 0.000000}), DTS({0/6 = 0.000000}), duration({1/6 = 0.166667}), flags(1), generation(0)}
{PTS({1/6 = 0.166667}), DTS({1/6 = 0.166667}), duration({1/6 = 0.166667}), flags(0), generation(0)}
{PTS({2/6 = 0.333333}), DTS({2/6 = 0.333333}), duration({1/6 = 0.166667}), flags(1), generation(0)}
{PTS({3/6 = 0.500000}), DTS({3/6 = 0.500000}), duration({1/6 = 0.166667}), flags(0), generation(0)}
{PTS({4/6 = 0.666667}), DTS({4/6 = 0.666667}), duration({1/6 = 0.166667}), flags(1), generation(0)}
{PTS({5/6 = 0.833333}), DTS({5/6 = 0.833333}), duration({1/6 = 0.166667}), flags(0), generation(0)}
{PTS({6/6 = 1.000000}), DTS({6/6 = 1.000000}), duration({1/6 = 0.166667}), flags(1), generation(1)}
{PTS({7/6 = 1.166667}), DTS({7/6 = 1.166667}), duration({1/6 = 0.166667}), flags(0), generation(1)}
{PTS({8/6 = 1.333333}), DTS({8/6 = 1.333333}), duration({1/6 = 0.166667}), flags(1), generation(1)}
{PTS({9/6 = 1.500000}), DTS({9/6 = 1.500000}), duration({1/6 = 0.166667}), flags(0), generation(1)}
{PTS({10/6 = 1.666667}), DTS({10/6 = 1.666667}), duration({1/6 = 0.166667}), flags(1), generation(1)}
{PTS({11/6 = 1.833333}), DTS({11/6 = 1.833333}), duration({1/6 = 0.166667}), flags(0), generation(1)}
{PTS({12/6 = 2.000000}), DTS({12/6 = 2.000000}), duration({1/6 = 0.166667}), flags(1), generation(1)}
{PTS({13/6 = 2.166667}), DTS({13/6 = 2.166667}), duration({1/6 = 0.166667}), flags(0), generation(1)}
{PTS({14/6 = 2.333333}), DTS({14/6 = 2.333333}), duration({1/6 = 0.166667}), flags(1), generation(1)}
{PTS({15/6 = 2.500000}), DTS({15/6 = 2.500000}), duration({1/6 = 0.166667}), flags(0), generation(1)}
{PTS({16/6 = 2.666667}), DTS({16/6 = 2.666667}), duration({1/6 = 0.166667}), flags(1), generation(1)}
{PTS({17/6 = 2.833333}), DTS({17/6 = 2.833333}), duration({1/6 = 0.166667}), flags(0), generation(1)}

Buffered after third append:
{PTS({0/6 = 0.000000}), DTS({0/6 = 0.000000}), duration({1/6 = 0.166667}), flags(1), generation(0)}
{PTS({1/6 = 0.166667}), DTS({1/6 = 0.166667}), duration({1/6 = 0.166667}), flags(0), generation(0)}
{PTS({2/6 = 0.333333}), DTS({2/6 = 0.333333}), duration({1/6 = 0.166667}), flags(1), generation(0)}
{PTS({3/6 = 0.500000}), DTS({3/6 = 0.500000}), duration({1/6 = 0.166667}), flags(0), generation(0)}
{PTS({4/6 = 0.666667}), DTS({4/6 = 0.666667}), duration({1/6 = 0.166667}), flags(1), generation(0)}
{PTS({5/6 = 0.833333}), DTS({5/6 = 0.833333}), duration({1/6 = 0.166667}), flags(0), generation(0)}
{PTS({6/6 = 1.000000}), DTS({6/6 = 1.000000}), duration({1/6 = 0.166667}), flags(1), generation(1)}
{PTS({7/6 = 1.166667}), DTS({7/6 = 1.166667}), duration({1/6 = 0.166667}), flags(0), generation(1)}
{PTS({8/6 = 1.333333}), DTS({8/6 = 1.333333}), duration({1/6 = 0.166667}), flags(1), generation(1)}
{PTS({15/10 = 1.500000}), DTS({15/10 = 1.500000}), duration({1/5 = 0.200000}), flags(1), generation(2)}
{PTS({17/10 = 1.700000}), DTS({17/10 = 1.700000}), duration({1/5 = 0.200000}), flags(0), generation(2)}
{PTS({19/10 = 1.900000}), DTS({19/10 = 1.900000}), duration({1/5 = 0.200000}), flags(1), generation(2)}
{PTS({21/10 = 2.100000}), DTS({21/10 = 2.100000}), duration({1/5 = 0.200000}), flags(0), generation(2)}
{PTS({23/10 = 2.300000}), DTS({23/10 = 2.300000}), duration({1/5 = 0.200000}), flags(1), generation(2)}
{PTS({25/10 = 2.500000}), DTS({25/10 = 2.500000}), duration({1/5 = 0.200000}), flags(0), generation(2)}
{PTS({16/6 = 2.666667}), DTS({16/6 = 2.666667}), duration({1/6 = 0.166667}), flags(1), generation(1)}
^^^ This sample shouldn't be here, it's "nested" between the [2.5, 2.7] and the [2.7, 2.9] samples from two lines above this one and the line below
{PTS({27/10 = 2.700000}), DTS({27/10 = 2.700000}), duration({1/5 = 0.200000}), flags(1), generation(2)}
{PTS({29/10 = 2.900000}), DTS({29/10 = 2.900000}), duration({1/5 = 0.200000}), flags(0), generation(2)}
{PTS({31/10 = 3.100000}), DTS({31/10 = 3.100000}), duration({1/5 = 0.200000}), flags(1), generation(2)}
{PTS({33/10 = 3.300000}), DTS({33/10 = 3.300000}), duration({1/5 = 0.200000}), flags(0), generation(2)}

I'll try to debug what's happening in more detail to try to understand why that sample remains.

calvaris commented 1 year ago

@emutavchi I really wonder how you run this tests you mention in the first comment, it requires the internals that are only available in the WebKit Test Runner.

eocanha commented 1 year ago

Some more insight. In this case, when the new generation 2 [2.5, 2.7] sample that should overwrite the generation 1 [2.666667, 2.833334] arrives, eraseEndTimeMinusTolerance kicks in and avoids the erasure of the old sample, as shown in these extra logs (upstream):

### void WebCore::SourceBufferPrivate::processMediaSample(Ref<WebCore::MediaSample> &&): ==============================
### void WebCore::SourceBufferPrivate::processMediaSample(Ref<WebCore::MediaSample> &&): sample: {PTS({25/10 = 2.500000}), DTS({25/10 = 2.500000}), duration({1/5 = 0.200000}), flags(0), generation(2)}
### void WebCore::SourceBufferPrivate::processMediaSample(Ref<WebCore::MediaSample> &&) -------------
### void WebCore::SourceBufferPrivate::processMediaSample(Ref<WebCore::MediaSample> &&): highestBufferedTime: {18/6 = 3}
### void WebCore::SourceBufferPrivate::processMediaSample(Ref<WebCore::MediaSample> &&): eraseBeginTime: {25/10 = 2.5}
### void WebCore::SourceBufferPrivate::processMediaSample(Ref<WebCore::MediaSample> &&): eraseEndTime: {27/10 = 2.7}
### void WebCore::SourceBufferPrivate::processMediaSample(Ref<WebCore::MediaSample> &&): highestBufferedTime - trackBuffer.highestPresentationTimestamp() < trackBuffer.lastFrameDuration() = {18/6 = 3} - {25/10 = 2.5} < {1/5 = 0.2} = {15/30 = 0.5} < {1/5 = 0.2} = false
### void WebCore::SourceBufferPrivate::processMediaSample(Ref<WebCore::MediaSample> &&): range = trackBuffer.samples().presentationOrder().findSamplesBetweenPresentationTimes({25/10 = 2.5}, {27/10 = 2.7})
### void WebCore::SourceBufferPrivate::processMediaSample(Ref<WebCore::MediaSample> &&): eraseEndTimeMinusTolerance: {2699/1000 = 2.699}
### void WebCore::SourceBufferPrivate::processMediaSample(Ref<WebCore::MediaSample> &&):     Evaluating sample {17/6 = 2.8333333333333335}
### void WebCore::SourceBufferPrivate::processMediaSample(Ref<WebCore::MediaSample> &&): erasedSamples.addRange({16/6 = 2.6666666666666665}, {23/10 = 2.3})
### void WebCore::SourceBufferPrivate::processMediaSample(Ref<WebCore::MediaSample> &&): ==============================

The second parameter to erasedSamples.addRange() comes from range.second--. range.second is an iterator that might point to the end element, so I need to decrement a copy of it to get the a "real" element (a (MediaSample)(range.second--.second)) to print.

emutavchi commented 1 year ago

@calvaris I did test with webkit test runner. I've pushed the build fix I was using here: https://github.com/emutavchi/WebKitForWayland/commit/5e3af79064ea42ad10d9559ff120a1086f272add

eocanha commented 11 months ago

@emutavchi I've made a little improvement to move this task forward. I'm bailing out of the loop that discards extra samples when the current sample being processed by sourceBufferPrivateDidReceiveSample() and the oldSample being discarded have a different duration. I foresee that the typical videos in production won't have different sample durations (framerates), so this tweak would allow you to still have your change working while keeping the Layout Tests happy (I hope). Would that fix work for you?

eocanha commented 11 months ago

Still, even with the changes I commented before, there's still the failure in the media/media-source/mock-managedmse-bufferedchange.html layout test. I analyzed the failure cause and what happens is that, at some point, the ranges [3, 4), [6, 8) are present (with sync samples at 3 and 6, the rest are dependant), the [5, 7) range is added, the [7, 8) range should be auto-removed (because it's dependant of the old deleted sync sample at 6), but it isn't.

I've been trying to analyze and understand the algorithm and how your proposed change works. I'd like to have been actually able to come up with some creative solution to make the test pass but, after several days trying to find one with no success, I have no other choice but reject this Pull Request.