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.
<rdar://problem/79996056>
*** Bug 222495 has been marked as a duplicate of this bug. ***
Created attachment 432766 [details] Patch
Created attachment 432767 [details] Patch Fix compilation on machine with no VP9
Created attachment 432770 [details] Patch Handle case where sourceBuffer.remove is called immediately after abort
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/
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.
Created attachment 432843 [details] Patch Apply comments
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).
Created attachment 432844 [details] Patch Fix Eric's name
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
Reverted r279542 for reason: Casued 4 test crashes Committed r279622 (239440@main): <https://commits.webkit.org/239440@main>
Created attachment 433203 [details] Patch Clear m_mediaSamples array on reset, re-schedule some methods
Created attachment 433375 [details] Patch Redesign
Created attachment 433378 [details] Patch Fix grammar
Created attachment 433391 [details] Patch Prevent each sample received after a call to ChangeType to fire an error
Comment on attachment 433391 [details] Patch unrelated failures.
Found 2 new test failures: fast/canvas/webgl/texImage2D-mse-flipY-false.html, fast/canvas/webgl/texImage2D-mse-flipY-true.html
Created attachment 433472 [details] Patch Fix logic where queued samples wouldn't be processed once the init segment was processed in the content process
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].