Use NativePromise chaining in place of operations queue in SourceBufferPrivate #20516

jyavenard · 2023-11-15T02:25:44Z

`19836c7`

Use NativePromise chaining in place of operations queue in SourceBufferPrivate
https://bugs.webkit.org/show_bug.cgi?id=264854
rdar://118429088

Reviewed by Jer Noble.

By using promise chaining we can remove the need for an operations queue to serialise the tasks.
It also makes the code easier to read/follow as the entire appendBuffer operation is now available at a
glance.
This will simplify making SourceBufferPrivate::append/removeCodedFrames be made asynchronous and use
NativePromise.

* Source/WebCore/platform/graphics/SourceBufferPrivate.cpp:
(WebCore::SourceBufferPrivate::didReceiveInitializationSegment):
(WebCore::SourceBufferPrivate::didUpdateFormatDescriptionForTrackId):
(WebCore::SourceBufferPrivate::didReceiveSample):
(WebCore::SourceBufferPrivate::append):
(WebCore::SourceBufferPrivate::processPendingMediaSamples):
(WebCore::SourceBufferPrivate::processMediaSample):
(WebCore::SourceBufferPrivate::resetParserState):
(WebCore::SourceBufferPrivate::memoryPressure):
(WebCore::SourceBufferPrivate::~SourceBufferPrivate): Deleted.
(WebCore::SourceBufferPrivate::advanceOperationState): Deleted.
(WebCore::SourceBufferPrivate::rewindOperationState): Deleted.
(WebCore::SourceBufferPrivate::processAppendCompletedOperation): Deleted.
(WebCore::SourceBufferPrivate::queueOperation): Deleted.
(WebCore::SourceBufferPrivate::processPendingOperations): Deleted.
(WebCore::SourceBufferPrivate::abortPendingOperations): Deleted.
(WebCore::SourceBufferPrivate::processError): Deleted.
(WebCore::SourceBufferPrivate::processInitOperation): Deleted.
(WebCore::SourceBufferPrivate::processMediaSamplesOperation): Deleted.
* Source/WebCore/platform/graphics/SourceBufferPrivate.h:
* Source/WebCore/platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.cpp:
(WebCore::SourceBufferPrivateGStreamer::didReceiveAllPendingSamples): Test that the promise wasn't previously resolved.
fallout from bug 264847.
(WebCore::SourceBufferPrivateGStreamer::appendParsingFailed):

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

96a3f81

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	❌ 🧪 api-wpe
	~~🧪 ios-wk2-wpt~~	✅ 🧪 mac-wk1	❌ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	❌ 🧪 gtk-wk2
	✅ 🛠 tv	~~🧪 mac-AS-debug-wk2~~	❌ 🧪 api-gtk
	✅ 🛠 tv-sim
✅ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-11-15T02:26:09Z

EWS run on previous version of this PR (hash 61a0298)

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
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress
❌ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

jernoble · 2023-11-15T17:00:04Z

Source/WebCore/Modules/mediasource/MediaSource.h

optional and Vector.h don't appear to be used in the header. Can these be removed?

they were used, I added them because compilation broke elsewhere following unified build changes.
We have Vector<PlatformTimeRanges> activeRanges() method .
and we have std::optional<SeekTarget> m_pendingSeekTarget; class member

jernoble · 2023-11-15T17:04:34Z

Source/WebCore/Modules/mediasource/MediaSource.cpp

This pattern is repeated a lot; I assume that -1 will be replaced with a PlatformMediaError in a future patch?

Yes... The "PR#4" adds PlatformMediaError and those are replaced everywhere

jernoble · 2023-11-15T17:07:38Z

Source/WebCore/platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.cpp

Seems like this could just be removed entirely.

yes indeed.

jernoble · 2023-11-15T17:09:10Z

Source/WebCore/platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.cpp

Should this be -1? (Or it doesn't matter because this will be a PlatformMediaError in the future.)

jernoble · 2023-11-15T17:09:38Z

Source/WebCore/platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.h

Looks like the optional.h from above needs to move here.

jernoble · 2023-11-15T17:11:52Z

Source/WebKit/GPUProcess/media/RemoteSourceBufferProxy.cpp

Seems like sendWithPromisedReply(...) should have a variant that doesn't take a lambda, and just creates and returns a GenericPromise with the result.

sendWithPromisedReply doesn't take anything, it returns a NativePromise. You don't have to act on the promise

The issue is returning the IPC error code; this wasn't handled before, and code always assumed that sending an IPC message was successful. We needed a way to pass that error. Merging the error code with the PlatformMediaError would be nice, but I don't know how that will play for consistency.

sendWithMediaPromisedReply could do that maybe

jernoble · 2023-11-15T17:13:18Z

Source/WebKit/WebProcess/GPU/media/SourceBufferPrivateRemote.cpp

Ditto here. For a result that's an optional<>, it could create or reject based on whether the optional has a value.

the result is an Expected<T, U> The IPC Promise wraps the type set in the messages.in file, and an IPCError as exception.

It forces you to deal with the error if there's one.

webkit-early-warning-system · 2023-11-15T22:09:35Z

EWS run on current version of this PR (hash 506ce07)

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
	🧪 ios-wk2-wpt	🧪 mac-wk1	🧪 gtk-wk2
⏳ 🛠 🧪 jsc	🧪 api-ios	🧪 mac-wk2	🧪 api-gtk
	⏳ 🛠 tv	⏳ 🧪 mac-AS-debug-wk2
🧪 services	🛠 tv-sim	🧪 mac-wk2-stress
	⏳ 🛠 watch
	⏳ 🛠 watch-sim

webkit-early-warning-system · 2023-11-15T22:09:36Z

EWS run on previous version of this PR (hash 506ce07)

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~~	✅ 🧪 api-wpe
	⏳ 🧪 ios-wk2-wpt	~~🧪 mac-wk1~~	✅ 🛠 gtk
	~~🧪 api-ios~~	~~🧪 mac-wk2~~	~~🧪 gtk-wk2~~
	⏳ 🛠 tv	~~🧪 mac-AS-debug-wk2~~	✅ 🧪 api-gtk
	✅ 🛠 tv-sim	~~🧪 mac-wk2-stress~~
	⏳ 🛠 watch
	⏳ 🛠 watch-sim

webkit-early-warning-system · 2023-11-15T22:40:39Z

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

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~~	❌ 🧪 api-wpe
	⏳ 🧪 ios-wk2-wpt	~~🧪 mac-wk1~~	❌ 🛠 gtk
	~~🧪 api-ios~~	~~🧪 mac-wk2~~	❌ 🧪 gtk-wk2
	~~🛠 tv~~	~~🧪 mac-AS-debug-wk2~~	❌ 🧪 api-gtk
	✅ 🛠 tv-sim	~~🧪 mac-wk2-stress~~
	~~🛠 watch~~
	⏳ 🛠 watch-sim

webkit-early-warning-system · 2023-11-15T23:50:45Z

EWS run on previous version of this PR (hash 8a6a086)

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~~	~~🧪 api-wpe~~
	⏳ 🧪 ios-wk2-wpt	~~🧪 mac-wk1~~	~~🛠 gtk~~
	⏳ 🧪 api-ios	~~🧪 mac-wk2~~	~~🧪 gtk-wk2~~
	⏳ 🛠 tv	⏳ 🧪 mac-AS-debug-wk2	~~🧪 api-gtk~~
	⏳ 🛠 tv-sim	~~🧪 mac-wk2-stress~~
	⏳ 🛠 watch
	⏳ 🛠 watch-sim

webkit-early-warning-system · 2023-11-16T00:12:32Z

EWS run on previous version of this PR (hash 9aedfbe)

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	~~🧪 api-wpe~~
	⏳ 🧪 ios-wk2-wpt	⏳ 🧪 mac-wk1	✅ 🛠 gtk
	~~🧪 api-ios~~	⏳ 🧪 mac-wk2	~~🧪 gtk-wk2~~
	⏳ 🛠 tv	~~🧪 mac-AS-debug-wk2~~	✅ 🧪 api-gtk
	✅ 🛠 tv-sim	⏳ 🧪 mac-wk2-stress
	⏳ 🛠 watch
	⏳ 🛠 watch-sim

webkit-early-warning-system · 2023-11-16T01:05:39Z

EWS run on previous version of this PR (hash 0e53637)

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~~	❌ 🧪 api-wpe
	⏳ 🧪 ios-wk2-wpt	~~🧪 mac-wk1~~	✅ 🛠 gtk
	~~🧪 api-ios~~	~~🧪 mac-wk2~~	❌ 🧪 gtk-wk2
	✅ 🛠 tv	⏳ 🧪 mac-AS-debug-wk2	❌ 🧪 api-gtk
	✅ 🛠 tv-sim
	⏳ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-11-16T03:07:07Z

EWS run on current version of this PR (hash 96a3f81)

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	❌ 🧪 api-wpe
	~~🧪 ios-wk2-wpt~~	✅ 🧪 mac-wk1	❌ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	❌ 🧪 gtk-wk2
	✅ 🛠 tv	~~🧪 mac-AS-debug-wk2~~	❌ 🧪 api-gtk
	✅ 🛠 tv-sim
✅ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

…erPrivate https://bugs.webkit.org/show_bug.cgi?id=264854 rdar://118429088 Reviewed by Jer Noble. By using promise chaining we can remove the need for an operations queue to serialise the tasks. It also makes the code easier to read/follow as the entire appendBuffer operation is now available at a glance. This will simplify making SourceBufferPrivate::append/removeCodedFrames be made asynchronous and use NativePromise. * Source/WebCore/platform/graphics/SourceBufferPrivate.cpp: (WebCore::SourceBufferPrivate::didReceiveInitializationSegment): (WebCore::SourceBufferPrivate::didUpdateFormatDescriptionForTrackId): (WebCore::SourceBufferPrivate::didReceiveSample): (WebCore::SourceBufferPrivate::append): (WebCore::SourceBufferPrivate::processPendingMediaSamples): (WebCore::SourceBufferPrivate::processMediaSample): (WebCore::SourceBufferPrivate::resetParserState): (WebCore::SourceBufferPrivate::memoryPressure): (WebCore::SourceBufferPrivate::~SourceBufferPrivate): Deleted. (WebCore::SourceBufferPrivate::advanceOperationState): Deleted. (WebCore::SourceBufferPrivate::rewindOperationState): Deleted. (WebCore::SourceBufferPrivate::processAppendCompletedOperation): Deleted. (WebCore::SourceBufferPrivate::queueOperation): Deleted. (WebCore::SourceBufferPrivate::processPendingOperations): Deleted. (WebCore::SourceBufferPrivate::abortPendingOperations): Deleted. (WebCore::SourceBufferPrivate::processError): Deleted. (WebCore::SourceBufferPrivate::processInitOperation): Deleted. (WebCore::SourceBufferPrivate::processMediaSamplesOperation): Deleted. * Source/WebCore/platform/graphics/SourceBufferPrivate.h: * Source/WebCore/platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.cpp: (WebCore::SourceBufferPrivateGStreamer::didReceiveAllPendingSamples): Test that the promise wasn't previously resolved. fallout from bug 264847. (WebCore::SourceBufferPrivateGStreamer::appendParsingFailed): Canonical link: https://commits.webkit.org/270808@main

webkit-commit-queue · 2023-11-16T06:15:37Z

Committed 270808@main (19836c7): https://commits.webkit.org/270808@main

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

jyavenard requested review from cdumez and philn as code owners November 15, 2023 02:25

jyavenard self-assigned this Nov 15, 2023

jyavenard added the Media Bugs related to the HTML 5 Media elements. label Nov 15, 2023

jyavenard requested review from jernoble and youennf November 15, 2023 04:09

jernoble approved these changes Nov 15, 2023

View reviewed changes

jyavenard added merge-queue Applied to send a pull request to merge-queue and removed merge-queue Applied to send a pull request to merge-queue labels Nov 15, 2023

jyavenard force-pushed the bz/264854_Use_NativePromise_chaining_in_place_of_operations_queue_in_SourceBufferPrivate branch from 61a0298 to 506ce07 Compare November 15, 2023 22:09

jyavenard force-pushed the bz/264854_Use_NativePromise_chaining_in_place_of_operations_queue_in_SourceBufferPrivate branch from 506ce07 to 4db2465 Compare November 15, 2023 22:40

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

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

jyavenard force-pushed the bz/264854_Use_NativePromise_chaining_in_place_of_operations_queue_in_SourceBufferPrivate branch from 4db2465 to 8a6a086 Compare November 15, 2023 23:19

jyavenard force-pushed the bz/264854_Use_NativePromise_chaining_in_place_of_operations_queue_in_SourceBufferPrivate branch from 8a6a086 to 9aedfbe Compare November 16, 2023 00:12

jyavenard force-pushed the bz/264854_Use_NativePromise_chaining_in_place_of_operations_queue_in_SourceBufferPrivate branch from 9aedfbe to 0e53637 Compare November 16, 2023 00:59

jyavenard force-pushed the bz/264854_Use_NativePromise_chaining_in_place_of_operations_queue_in_SourceBufferPrivate branch from 0e53637 to 96a3f81 Compare November 16, 2023 03:04

jyavenard added the merge-queue Applied to send a pull request to merge-queue label Nov 16, 2023

webkit-commit-queue force-pushed the bz/264854_Use_NativePromise_chaining_in_place_of_operations_queue_in_SourceBufferPrivate branch from 96a3f81 to 543ec10 Compare November 16, 2023 06:10

webkit-commit-queue force-pushed the bz/264854_Use_NativePromise_chaining_in_place_of_operations_queue_in_SourceBufferPrivate branch from 543ec10 to 19836c7 Compare November 16, 2023 06:15

webkit-commit-queue merged commit 19836c7 into WebKit:main Nov 16, 2023

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

Use NativePromise chaining in place of operations queue in SourceBufferPrivate #20516

Use NativePromise chaining in place of operations queue in SourceBufferPrivate #20516

Uh oh!

Conversation

jyavenard commented Nov 15, 2023 • edited by webkit-early-warning-system Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Nov 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webkit-early-warning-system commented Nov 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Nov 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Nov 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Nov 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Nov 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Nov 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Nov 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-commit-queue commented Nov 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jyavenard commented Nov 15, 2023 •

edited by webkit-early-warning-system

Loading

webkit-early-warning-system commented Nov 15, 2023 •

edited

Loading

webkit-early-warning-system commented Nov 15, 2023 •

edited

Loading

webkit-early-warning-system commented Nov 15, 2023 •

edited

Loading

webkit-early-warning-system commented Nov 15, 2023 •

edited

Loading

webkit-early-warning-system commented Nov 15, 2023 •

edited

Loading

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

edited

Loading

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

edited

Loading

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

edited

Loading