MockSourceBufferPrivate should only process samples once init segment has been processed. #11599

jyavenard · 2023-03-16T09:27:51Z

`30fd635`

MockSourceBufferPrivate should only process samples once init segment has been processed.
https://bugs.webkit.org/show_bug.cgi?id=253997
rdar://106784564

Reviewed by Youenn Fablet.

The ability of a SourceBufferPrivate to run in the GPU process required
some steps to be done asynchronously.
In particular no demuxed media segments should be processed until all
track buffers had been created as they would otherwise be dropped.
Currently, only the SourceBufferPrivateAVFObjC implementation had been
modified to run asynchronously and contained very specific handlings to
do so.

We move the responsibility of queuing samples for until the init segment
has been fully processed to the base SourceBufferPrivate class.
We allows the ability for any SourceBufferPrivate implementation to be
wrapped in a SourceBufferPrivateRemote and be platform agnostic, including
the MockMediaSource.

Fly-by changes:
- Rename ReceiveResult::RecieveSucceeded to ReceiveResult::Succeeded (and fix typo)
- Rename AppendResult::AppendSucceeded to AppendResult::Succeeded
- By spec, the Reset Parser State should process any frames left in the
  input buffer.

* Source/WebCore/Modules/mediasource/SourceBuffer.cpp:
(WebCore::SourceBuffer::appendBufferTimerFired):
(WebCore::SourceBuffer::sourceBufferPrivateAppendComplete):
(WebCore::SourceBuffer::sourceBufferPrivateDidReceiveInitializationSegment):
* Source/WebCore/platform/graphics/SourceBufferPrivate.cpp:
(WebCore::SourceBufferPrivate::appendCompleted):
(WebCore::SourceBufferPrivate::didReceiveInitializationSegment):
(WebCore::SourceBufferPrivate::didReceiveSampleForTrackId):
(WebCore::SourceBufferPrivate::didReceiveSample):
(WebCore::SourceBufferPrivate::processPendingSamples):
(WebCore::SourceBufferPrivate::resetParserState): Make default implementation.
* Source/WebCore/platform/graphics/SourceBufferPrivate.h:
(WebCore::SourceBufferPrivate::appendCompleted):
* Source/WebCore/platform/graphics/SourceBufferPrivateClient.cpp:
(WebCore::convertEnumerationToString):
* Source/WebCore/platform/graphics/SourceBufferPrivateClient.h: Rename enum as name already contained in type.
* Source/WebCore/platform/graphics/avfoundation/objc/MediaSourcePrivateAVFObjC.h:
* Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.h:
* Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:
(WebCore::SourceBufferPrivateAVFObjC::didParseInitializationData):
(WebCore::SourceBufferPrivateAVFObjC::didProvideMediaDataForTrackId):
(WebCore::SourceBufferPrivateAVFObjC::processPendingTrackChangeTasks):
(WebCore::SourceBufferPrivateAVFObjC::didReceiveSampleForTrackId):
(WebCore::SourceBufferPrivateAVFObjC::append):
(WebCore::SourceBufferPrivateAVFObjC::appendCompleted):
(WebCore::SourceBufferPrivateAVFObjC::resetParserState):
* Source/WebCore/platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.cpp:
(WebCore::SourceBufferPrivateGStreamer::resetParserState):
* Source/WebCore/platform/mock/mediasource/MockSourceBufferPrivate.cpp: Remove unnecessary method.
(WebCore::MockSourceBufferPrivate::resetParserState): Deleted.
* Source/WebCore/platform/mock/mediasource/MockSourceBufferPrivate.h: Remove unnecessary method.

Canonical link: https://commits.webkit.org/261799@main

d1f4a65

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	~~🛠 ios~~	~~🛠 mac~~	~~🛠 wpe~~	~~🛠 wincairo~~
~~🧪 bindings~~	~~🛠 ios-sim~~	~~🛠 mac-AS-debug~~	~~🧪 wpe-wk2~~
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	~~🧪 api-mac~~	✅ 🛠 gtk
	~~🧪 api-ios~~	~~🧪 mac-wk1~~	~~🧪 gtk-wk2~~
	~~🛠 tv~~	~~🧪 mac-wk2~~	⏳ 🧪 api-gtk
	~~🛠 tv-sim~~	~~🧪 mac-AS-debug-wk2~~
	~~🛠 watch~~	~~🧪 mac-wk2-stress~~
✅ 🛠 🧪 merge	✅ 🛠 watch-sim

youennf · 2023-03-16T12:41:26Z

Source/WebCore/platform/graphics/SourceBufferPrivate.cpp

@@ -151,10 +152,17 @@ void SourceBufferPrivate::updateBufferedFromTrackBuffers(bool sourceIsEnded)
    setBufferedRanges(intersectionRanges);
 }

-void SourceBufferPrivate::appendCompleted(bool parsingSucceeded, bool isEnded)
+void SourceBufferPrivate::appendCompleted(bool parsingSucceeded, bool isEnded, Function<void()>&& preAppendCompletedTask)


Should it be a CompletionHandler?

No. As it may not always be run (such as if abort() is called in between or if an error occurred.

youennf · 2023-03-16T12:45:09Z

Source/WebCore/platform/graphics/SourceBufferPrivate.cpp

+
+    if (m_processingInitializationSegment) {
+        DEBUG_LOG(LOGIDENTIFIER, originalSample.get());
+        // TODO: MediaSample should use uint64_t for trackID.


ObjectIdentifier?

TrackID is used everywhere, and it's either an int or a uint everywhere but with MediaSample. so we have to keep doing string conversion back and forth

youennf · 2023-03-16T12:52:17Z

Source/WebCore/platform/graphics/SourceBufferPrivate.h


    WeakPtr<SourceBufferPrivateClient> m_client;

+    bool m_processingInitializationSegment { false };
+    Vector<std::pair<uint64_t, Ref<MediaSample>>> m_pendingMediaSamples;
+    bool m_hasPendingAppendCompleted { false };


Can we make these private instead?
m_processingInitializationSegment is accessed, how about just adding a protected getter but no setter for instance?

youennf · 2023-03-16T12:52:39Z

Source/WebCore/platform/graphics/SourceBufferPrivate.h

@@ -171,6 +179,14 @@ class SourceBufferPrivate
    void trySignalAllSamplesInTrackEnqueued(TrackBuffer&, const AtomString& trackID);
    MediaTime findPreviousSyncSamplePresentationTime(const MediaTime&);
    bool evictFrames(uint64_t newDataSize, uint64_t maximumBufferSize, const MediaTime& currentTime, bool isEnded);
+    void processPendingSamples();
+
+    struct appendCompletedArguments {


s/appendCompletedArguments/AppendCompletedArguments

youennf · 2023-03-16T12:53:14Z

Source/WebCore/platform/graphics/SourceBufferPrivate.cpp

 {
    DEBUG_LOG(LOGIDENTIFIER);

+    if (m_processingInitializationSegment) {


ASSERT(!m_appendCompletedPending); ?

no, there could be two init segment appended with a media segment.
like so:
init_segment | media_segment | init_segment

Actually, I think our handling of such scenario is problematic and would result in an undefined behaviour , in particular in regards to track ID ; it is possible for track id to change in between run, even get swapped between audio and video (we even have a test about it: as it used to cause a crash).

However, the behaviour following these changes is identical to what we had before

Should we file a bug and add a comment here?

Actually, the issue I described isn't happening here. appendCompleted() can be called whenever the SourceBufferParser has finished demuxing all data but the init segment is yet to be processed by the SourceBuffer in the content process.

I created https://bugs.webkit.org/show_bug.cgi?id=254079

youennf · 2023-03-16T12:57:56Z

Source/WebCore/platform/graphics/SourceBufferPrivate.cpp

+    m_processingInitializationSegment = false;
+    // 4.5.2 Reset Parser State
+    // 1. If the [[append state]] equals PARSING_MEDIA_SEGMENT and the [[input buffer]] contains some complete coded frames, then run the coded frame processing algorithm until all of these complete coded frames have been processed.
+    processPendingSamples();


What about m_appendCompletedPending, should we reset it?

I believe so yes. good spotting

youennf · 2023-03-16T13:01:10Z

Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.h

@@ -207,10 +208,8 @@ class SourceBufferPrivateAVFObjC final
    WeakPtrFactory<SourceBufferPrivateAVFObjC> m_appendWeakFactory;

    Ref<SourceBufferParser> m_parser;
-    bool m_processingInitializationSegment { false };
    bool m_hasPendingAppendCompletedCallback { false };


Is it still needed?

eric-carlson · 2023-03-16T13:09:07Z

Source/WebCore/platform/graphics/SourceBufferPrivate.cpp

+        m_processingInitializationSegment = false;
+        processPendingSamples();


You always clear m_processingInitializationSegment before calling processPendingSamples, maybe do it in processPendingSamples instead?

I think this would allow for unwanted side effects if it was to handle the case I described earlier (two init segments appended consecutively). So I prefer not to and leave this utility methods to do just what the name states.

Is there ever a case where m_processingInitializationSegment is true in processPendingSamples()

yes there is. It can be called when the first didReceivedInitializatingSegment completion handler is run, but in between we received a new init segment.

See bug https://bugs.webkit.org/show_bug.cgi?id=254079

webkit-early-warning-system · 2023-03-16T17:54:10Z

EWS run on previous version of this PR (hash 33bc608)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	❌ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	✅ 🧪 gtk-wk2
	✅ 🛠 tv	✅ 🧪 mac-wk2	~~🧪 api-gtk~~
	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-03-16T17:54:12Z

EWS run on previous version of this PR (hash 4ba99ec)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
⏳ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	~~🛠 wpe~~	✅ 🛠 wincairo
⏳ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~
⏳ 🧪 webkitperl	⏳ 🧪 ios-wk2	✅ 🧪 api-mac	~~🛠 gtk~~
⏳ 🧪 webkitpy	✅ 🧪 api-ios	✅ 🧪 mac-wk1	~~🧪 gtk-wk2~~
	✅ 🛠 tv	~~🧪 mac-wk2~~	~~🧪 api-gtk~~
	✅ 🛠 tv-sim	⏳ 🧪 mac-AS-debug-wk2
⏳ 🧪 services	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-03-17T17:02:47Z

EWS run on current version of this PR (hash d1f4a65)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	~~🛠 ios~~	~~🛠 mac~~	~~🛠 wpe~~	~~🛠 wincairo~~
~~🧪 bindings~~	~~🛠 ios-sim~~	~~🛠 mac-AS-debug~~	~~🧪 wpe-wk2~~
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	~~🧪 api-mac~~	✅ 🛠 gtk
	~~🧪 api-ios~~	~~🧪 mac-wk1~~	~~🧪 gtk-wk2~~
	~~🛠 tv~~	~~🧪 mac-wk2~~	⏳ 🧪 api-gtk
	~~🛠 tv-sim~~	~~🧪 mac-AS-debug-wk2~~
	~~🛠 watch~~	~~🧪 mac-wk2-stress~~
✅ 🛠 🧪 merge	✅ 🛠 watch-sim

… has been processed. https://bugs.webkit.org/show_bug.cgi?id=253997 rdar://106784564 Reviewed by Youenn Fablet. The ability of a SourceBufferPrivate to run in the GPU process required some steps to be done asynchronously. In particular no demuxed media segments should be processed until all track buffers had been created as they would otherwise be dropped. Currently, only the SourceBufferPrivateAVFObjC implementation had been modified to run asynchronously and contained very specific handlings to do so. We move the responsibility of queuing samples for until the init segment has been fully processed to the base SourceBufferPrivate class. We allows the ability for any SourceBufferPrivate implementation to be wrapped in a SourceBufferPrivateRemote and be platform agnostic, including the MockMediaSource. Fly-by changes: - Rename ReceiveResult::RecieveSucceeded to ReceiveResult::Succeeded (and fix typo) - Rename AppendResult::AppendSucceeded to AppendResult::Succeeded - By spec, the Reset Parser State should process any frames left in the input buffer. * Source/WebCore/Modules/mediasource/SourceBuffer.cpp: (WebCore::SourceBuffer::appendBufferTimerFired): (WebCore::SourceBuffer::sourceBufferPrivateAppendComplete): (WebCore::SourceBuffer::sourceBufferPrivateDidReceiveInitializationSegment): * Source/WebCore/platform/graphics/SourceBufferPrivate.cpp: (WebCore::SourceBufferPrivate::appendCompleted): (WebCore::SourceBufferPrivate::didReceiveInitializationSegment): (WebCore::SourceBufferPrivate::didReceiveSampleForTrackId): (WebCore::SourceBufferPrivate::didReceiveSample): (WebCore::SourceBufferPrivate::processPendingSamples): (WebCore::SourceBufferPrivate::resetParserState): Make default implementation. * Source/WebCore/platform/graphics/SourceBufferPrivate.h: (WebCore::SourceBufferPrivate::appendCompleted): * Source/WebCore/platform/graphics/SourceBufferPrivateClient.cpp: (WebCore::convertEnumerationToString): * Source/WebCore/platform/graphics/SourceBufferPrivateClient.h: Rename enum as name already contained in type. * Source/WebCore/platform/graphics/avfoundation/objc/MediaSourcePrivateAVFObjC.h: * Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.h: * Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm: (WebCore::SourceBufferPrivateAVFObjC::didParseInitializationData): (WebCore::SourceBufferPrivateAVFObjC::didProvideMediaDataForTrackId): (WebCore::SourceBufferPrivateAVFObjC::processPendingTrackChangeTasks): (WebCore::SourceBufferPrivateAVFObjC::didReceiveSampleForTrackId): (WebCore::SourceBufferPrivateAVFObjC::append): (WebCore::SourceBufferPrivateAVFObjC::appendCompleted): (WebCore::SourceBufferPrivateAVFObjC::resetParserState): * Source/WebCore/platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.cpp: (WebCore::SourceBufferPrivateGStreamer::resetParserState): * Source/WebCore/platform/mock/mediasource/MockSourceBufferPrivate.cpp: Remove unnecessary method. (WebCore::MockSourceBufferPrivate::resetParserState): Deleted. * Source/WebCore/platform/mock/mediasource/MockSourceBufferPrivate.h: Remove unnecessary method. Canonical link: https://commits.webkit.org/261799@main

webkit-commit-queue · 2023-03-17T17:43:29Z

Committed 261799@main (30fd635): https://commits.webkit.org/261799@main

Reviewed commits have been landed. Closing PR #11599 and removing active labels.

jyavenard requested a review from philn as a code owner March 16, 2023 09:27

jyavenard self-assigned this Mar 16, 2023

jyavenard added the Media Bugs related to the HTML 5 Media elements. label Mar 16, 2023

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 16, 2023

jyavenard removed the merging-blocked Applied to prevent a change from being merged label Mar 16, 2023

jyavenard force-pushed the eng/MockSourceBufferPrivate-should-only-process-samples-once-init-segment-has-been-processed- branch from 4ba99ec to 33bc608 Compare March 16, 2023 10:17

philn requested a review from ntrrgc March 16, 2023 11:46

jyavenard requested review from jernoble, youennf and eric-carlson March 16, 2023 12:25

youennf reviewed Mar 16, 2023

View reviewed changes

eric-carlson reviewed Mar 16, 2023

View reviewed changes

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 16, 2023

jyavenard removed the merging-blocked Applied to prevent a change from being merged label Mar 17, 2023

jyavenard force-pushed the eng/MockSourceBufferPrivate-should-only-process-samples-once-init-segment-has-been-processed- branch from 33bc608 to d1f4a65 Compare March 17, 2023 16:33

youennf approved these changes Mar 17, 2023

View reviewed changes

jyavenard added the merge-queue Applied to send a pull request to merge-queue label Mar 17, 2023

webkit-commit-queue force-pushed the eng/MockSourceBufferPrivate-should-only-process-samples-once-init-segment-has-been-processed- branch from d1f4a65 to 30fd635 Compare March 17, 2023 17:42

webkit-commit-queue merged commit 30fd635 into WebKit:main Mar 17, 2023

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Mar 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MockSourceBufferPrivate should only process samples once init segment has been processed. #11599

MockSourceBufferPrivate should only process samples once init segment has been processed. #11599

jyavenard commented Mar 16, 2023 •

edited by webkit-early-warning-system

youennf Mar 16, 2023

jyavenard Mar 16, 2023

youennf Mar 16, 2023

jyavenard Mar 16, 2023

youennf Mar 16, 2023

jyavenard Mar 16, 2023

youennf Mar 16, 2023

jyavenard Mar 16, 2023

youennf Mar 16, 2023

jyavenard Mar 16, 2023

youennf Mar 17, 2023

jyavenard Mar 17, 2023

jyavenard Mar 17, 2023

jyavenard Mar 17, 2023

youennf Mar 16, 2023

jyavenard Mar 16, 2023

youennf Mar 16, 2023

jyavenard Mar 16, 2023

eric-carlson Mar 16, 2023

jyavenard Mar 16, 2023

eric-carlson Mar 16, 2023

jyavenard Mar 17, 2023

webkit-early-warning-system commented Mar 16, 2023 •

edited

webkit-early-warning-system commented Mar 16, 2023

webkit-early-warning-system commented Mar 17, 2023 •

edited

webkit-commit-queue commented Mar 17, 2023

		m_processingInitializationSegment = false;
		processPendingSamples();

MockSourceBufferPrivate should only process samples once init segment has been processed. #11599

MockSourceBufferPrivate should only process samples once init segment has been processed. #11599

Conversation

jyavenard commented Mar 16, 2023 • edited by webkit-early-warning-system

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webkit-early-warning-system commented Mar 16, 2023 • edited

webkit-early-warning-system commented Mar 16, 2023

webkit-early-warning-system commented Mar 17, 2023 • edited

webkit-commit-queue commented Mar 17, 2023

jyavenard commented Mar 16, 2023 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Mar 16, 2023 •

edited

webkit-early-warning-system commented Mar 17, 2023 •

edited