Bug 227559 – SourceBuffer.abort() doesn't go back to state WAITING_FOR_SEGMENT properly

• Component: Media
• Status: RESOLVED FIXED

[Jean-Yves Avenard [:jya]]

Consider the following test:

            consoleWrite('Append the two partial media segments in reverse order. Separated with an abort');
            run('sourceBuffer.appendBuffer(loader.mediaSegment(1).slice(0, 65677))');
            await waitFor(sourceBuffer, 'update');
            run('sourceBuffer.abort()');

            testExpected('sourceBuffer.buffered.length', '1');
            testExpected('sourceBuffer.buffered.end(0)', '82.981', '==');

            run('sourceBuffer.appendBuffer(loader.mediaSegment(0).slice(0, 65677))');
            await waitFor(sourceBuffer, 'update');
            testExpected('sourceBuffer.buffered.length', '2');
            testExpected('sourceBuffer.buffered.end(0)', '6.041', '==');
            testExpected('sourceBuffer.buffered.end(1)', '82.981', '==');

While we should get:
Append the two partial media segments in reverse order. Separated with an abort
RUN(sourceBuffer.appendBuffer(loader.mediaSegment(1).slice(0, 65677)))
EVENT(update)
RUN(sourceBuffer.abort())
EXPECTED (sourceBuffer.buffered.length == '1') OK
EXPECTED (sourceBuffer.buffered.end(0) == '82.981') OK
RUN(sourceBuffer.appendBuffer(loader.mediaSegment(0).slice(0, 65677)))
EVENT(update)
EXPECTED (sourceBuffer.buffered.length == '2') OK
EXPECTED (sourceBuffer.buffered.end(0) == '6.041') OK
EXPECTED (sourceBuffer.buffered.end(1) == '82.981') OK
END OF TEST

We get:
Append the two partial media segments in reverse order. Separated with an abort
RUN(sourceBuffer.appendBuffer(loader.mediaSegment(1).slice(0, 65677)))
EVENT(update)
RUN(sourceBuffer.abort())
EXPECTED (sourceBuffer.buffered.length == '1') OK
EXPECTED (sourceBuffer.buffered.end(0) == '82.981') OK
RUN(sourceBuffer.appendBuffer(loader.mediaSegment(0).slice(0, 65677)))
EVENT(update)
EXPECTED (sourceBuffer.buffered.length == '2'), OBSERVED '1' FAIL
EXPECTED (sourceBuffer.buffered.end(0) == '6.041'), OBSERVED '82.981' FAIL
IndexSizeError: The index is not in the allowed range.
END OF TEST

The reason for this is that when calling SourceBuffer.abort() we call into:
void SourceBufferPrivateAVFObjC::abort()
(https://webkit-search.igalia.com/webkit/rev/a76db2def990ef700e79d59e175d0c6c5eaf8472/Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm#659)

which does:
m_initializationSegmentIsHandled = false;

for webm we call into:
void SourceBufferParserWebM::resetParserState()
https://webkit-search.igalia.com/webkit/rev/a76db2def990ef700e79d59e175d0c6c5eaf8472/Source/WebCore/platform/graphics/cocoa/SourceBufferParserWebM.cpp#679

Which will call into:
    m_tracks.clear();
    m_initializationSegment = nullptr;
    m_initializationSegmentEncountered = false;

So we effectively move back to SourceBuffer into append state = WAITING_FOR_SEGMENT and "first initialization segment received flag is false" (https://w3c.github.io/media-source/#first-init-segment-received-flag)

This prevents appending any more data into the source buffer until a new init segment is received.

This is an incorrect behaviour.

I'm not seeing YouTube being impacted much when constantly seeking in the video ; even though the above JS is a pattern they use. I'm guessing they work around this bug by always sending a new init segment after calling abort, when they don't have it.

Firefox had a similar bug a while back, and this used to cause stalls.

[Radar WebKit Bug Importer]

<rdar://problem/79996056>

[Jean-Yves Avenard [:jya]]

*** Bug 222495 has been marked as a duplicate of this bug. ***

[Jean-Yves Avenard [:jya]]

Created attachment 432766 [details]
Patch

[Jean-Yves Avenard [:jya]]

Created attachment 432767 [details]
Patch

Fix compilation on machine with no VP9

[Jean-Yves Avenard [:jya]]

Created attachment 432770 [details]
Patch

Handle case where sourceBuffer.remove is called immediately after abort

[Eric Carlson]

Comment on attachment 432770 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=432770&action=review

> Source/WebCore/ChangeLog:34
> +        - The SourceBufferParser handle two type of parser: SourceBufferParser and the

s/two type/two types/

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:417
> +        callOnMainThread([this, weakThis = WTFMove(weakThis)] {

I believe we are trying to get away from using `callOnMainThread` as a way to queue tasks in favor of using the event queue. SourceBuffer is an ActiveDOMObject, so you could add a method that calls `queueCancellableTaskKeepingObjectAlive` and use it instead. This would presumably allow you to get rid of the WeakPtr, because SourceBuffer would cancel the pending task when it clears the client pointer.

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:575
> +    callOnMainThread([weakThis = makeWeakPtr(*this), data = WTFMove(data), this]() mutable {
> +        if (!weakThis)
> +            return;

Ditto

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:588
> +            callOnMainThread([weakThis = WTFMove(weakThis), trackID, respondedSemaphore]() {

Ditto.

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:591
> +                respondedSemaphore->signal();

Is it safe to signaling the semaphore if `weakThis` is null (the object has been destroyed)?

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:608
> +            callOnMainThread([weakThis = WTFMove(weakThis), initData = WTFMove(initData), trackID, hasSessionSemaphore]() mutable {

Ditto

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:629
> +                callOnMainThread([weakThis = WTFMove(weakThis)] {

Ditto

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:672
> +    callOnMainThread([weakThis = makeWeakPtr(*this), start, end, currentMediaTime, isEnded, completionHandler = WTFMove(completionHandler)]() mutable {

Ditto

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:696
> +    callOnMainThread([weakThis = makeWeakPtr(*this), this]() {

Ditto

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:727
> +    callOnMainThread([weakThis = makeWeakPtr(*this), parser = m_parser, this]() {

Ditto.

> Source/WebCore/platform/graphics/cocoa/SourceBufferParserWebM.cpp:812
> +    // partial init segment is encountered after a call to sourceBuffer.abort().

s/segment is encountered/segment be encountered/

[Jean-Yves Avenard [:jya]]

Comment on attachment 432770 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=432770&action=review

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:417
>> +        callOnMainThread([this, weakThis = WTFMove(weakThis)] {
> 
> I believe we are trying to get away from using `callOnMainThread` as a way to queue tasks in favor of using the event queue. SourceBuffer is an ActiveDOMObject, so you could add a method that calls `queueCancellableTaskKeepingObjectAlive` and use it instead. This would presumably allow you to get rid of the WeakPtr, because SourceBuffer would cancel the pending task when it clears the client pointer.

As discussed on slack, as callOnMainThread is used thoroughly in this file and related once, I will keep this as-is so we don't have to worry on the impact in the use (or no-longer used) semaphores.

[Jean-Yves Avenard [:jya]]

Created attachment 432843 [details]
Patch

Apply comments

[EWS]

Eric Carlon found in /Volumes/Data/worker/Commit-Queue/build/LayoutTests/ChangeLog does not appear to be a valid reviewer according to contributors.json.
/Volumes/Data/worker/Commit-Queue/build/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

[Jean-Yves Avenard [:jya]]

Created attachment 432844 [details]
Patch

Fix Eric's name

[EWS]

Committed r279542 (239378@main): <https://commits.webkit.org/239378@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 432844 [details].

It looks like the changes in https://trac.webkit.org/changeset/279542/webkit caused these 4 tests to crash with the assert

imported/w3c/web-platform-tests/media-source/mediasource-changetype-play-implicit.html
imported/w3c/web-platform-tests/media-source/mediasource-changetype-play.html
imported/w3c/web-platform-tests/media-source/mediasource-changetype-play-without-codecs-parameter.html
imported/w3c/web-platform-tests/media-source/mediasource-changetype-play-negative.html

History: https://results.webkit.org/?suite=layout-tests&suite=layout-tests&suite=layout-tests&suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fmedia-source%2Fmediasource-changetype-play-implicit.html&test=imported%2Fw3c%2Fweb-platform-tests%2Fmedia-source%2Fmediasource-changetype-play-negative.html&test=imported%2Fw3c%2Fweb-platform-tests%2Fmedia-source%2Fmediasource-changetype-play-without-codecs-parameter.html&test=imported%2Fw3c%2Fweb-platform-tests%2Fmedia-source%2Fmediasource-changetype-play.html

https://build.webkit.org/results/Apple-BigSur-Debug-AppleSilicon-WK2-Tests/r279586%20(2011)/results.html

[Truitt Savell]

Reverted r279542 for reason:

Casued 4 test crashes

Committed r279622 (239440@main): <https://commits.webkit.org/239440@main>

[Jean-Yves Avenard [:jya]]

Created attachment 433203 [details]
Patch

Clear m_mediaSamples array on reset, re-schedule some methods

[Jean-Yves Avenard [:jya]]

Created attachment 433375 [details]
Patch

Redesign

[Jean-Yves Avenard [:jya]]

Created attachment 433378 [details]
Patch

Fix grammar

[Jean-Yves Avenard [:jya]]

Created attachment 433391 [details]
Patch

Prevent each sample received after a call to ChangeType to fire an error

[Jean-Yves Avenard [:jya]]

Comment on attachment 433391 [details]
Patch

unrelated failures.

[EWS]

Found 2 new test failures: fast/canvas/webgl/texImage2D-mse-flipY-false.html, fast/canvas/webgl/texImage2D-mse-flipY-true.html

[Jean-Yves Avenard [:jya]]

Created attachment 433472 [details]
Patch

Fix logic where queued samples wouldn't be processed once the init segment was processed in the content process

[EWS]

Committed r279904 (239653@main): <https://commits.webkit.org/239653@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 433472 [details].