Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NEW TEST(276140@main): [ MacOS WK1 Debug ] media/media-source/worker/media-managedmse-worker.html is a constant crash #26236

Conversation

jyavenard
Copy link
Member

@jyavenard jyavenard commented Mar 21, 2024

b7d8be7

NEW TEST(276140@main): [ MacOS WK1 Debug ] media/media-source/worker/media-managedmse-worker.html  is a constant crash
https://bugs.webkit.org/show_bug.cgi?id=271323
rdar://125100787

Reviewed by Youenn Fablet.

WebPreferences are always set when running LayoutTests. MediaSource in a Worker is not functional
unless the GPU process is active and the MediaPlayers are to run in the GPU process.
MediaSource in a Worker can only work with a MediaSourcePrivate that is designed to be thread-safe
and at present there's only one kind: MediaSourcePrivateRemote.
As it is possible for a WebPreference to be turned on, we need to handle the case where it has
been accidentally set on a non-supported platform.
To achieve this we add a new MediaStrategy API: `hasThreadSafeSupport()` which will be checked
in addition to the `mediaSourceInWorkerEnabled` preference.

Updated existing test to ensure that enabling the pref on a non-supported configuration is
behaving properly.

* LayoutTests/media/media-source/worker/media-managedmse-worker-expected.txt:
* LayoutTests/media/media-source/worker/media-managedmse-worker.html:
* LayoutTests/media/media-source/worker/worker.js:
(onmessage):
* LayoutTests/platform/mac-wk1/TestExpectations:
* LayoutTests/platform/mac-wk1/media/media-source/worker/media-managedmse-worker-expected.txt: Added.
* Source/WTF/Scripts/Preferences/UnifiedWebPreferences.yaml:
* Source/WebCore/Modules/mediasource/MediaSource.cpp:
(WebCore::MediaSource::enabledForContext):
(WebCore::MediaSource::canConstructInDedicatedWorker):
* Source/WebCore/platform/MediaStrategy.cpp:
(WebCore::MediaStrategy::hasThreadSafeMediaSourceSupport const):
* Source/WebCore/platform/MediaStrategy.h:
* Source/WebCore/platform/PlatformStrategies.h:
(WebCore::PlatformStrategies::mediaStrategy): Allow creation of the MediaStrategy object on any threads, in practice this MediaStrategy creation is only
ever done on the main thread, but this could be different in the future.
* Source/WebKit/WebProcess/GPU/media/WebMediaStrategy.cpp:
(WebKit::WebMediaStrategy::createAudioDestination):
(WebKit::WebMediaStrategy::createNowPlayingManager const):
(WebKit::WebMediaStrategy::hasThreadSafeMediaSourceSupport const):
(WebKit::WebMediaStrategy::enableMockMediaSource):
* Source/WebKit/WebProcess/GPU/media/WebMediaStrategy.h:

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

59819db

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 βœ… πŸ›  wpe-skia
βœ… πŸ›  πŸ§ͺ jsc βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk2 βœ… πŸ›  gtk
βœ… πŸ›  πŸ§ͺ jsc-arm64 βœ… πŸ›  tv βœ… πŸ§ͺ mac-AS-debug-wk2 βœ… πŸ§ͺ gtk-wk2
βœ… πŸ›  tv-sim βœ… πŸ§ͺ mac-wk2-stress βœ… πŸ§ͺ api-gtk
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch βœ… πŸ›  jsc-armv7
βœ… πŸ›  watch-sim βœ… πŸ§ͺ jsc-armv7-tests

@jyavenard jyavenard requested a review from cdumez as a code owner March 21, 2024 10:11
@jyavenard jyavenard self-assigned this Mar 21, 2024
@jyavenard jyavenard added the New Bugs Unclassified bugs are placed in this component until the correct component can be determined. label Mar 21, 2024
@jyavenard jyavenard force-pushed the eng/NEW-TEST276140main--MacOS-WK1-Debug--mediamedia-sourceworkermedia-managedmse-worker-html--is-a-constant-crash branch from 6bd8c37 to 32578ff Compare March 21, 2024 10:26
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 21, 2024
@jyavenard jyavenard removed the merging-blocked Applied to prevent a change from being merged label Mar 21, 2024
@jyavenard jyavenard force-pushed the eng/NEW-TEST276140main--MacOS-WK1-Debug--mediamedia-sourceworkermedia-managedmse-worker-html--is-a-constant-crash branch from 32578ff to 3996e9f Compare March 21, 2024 11:21
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 21, 2024
@jyavenard jyavenard removed the merging-blocked Applied to prevent a change from being merged label Mar 21, 2024
@jyavenard jyavenard force-pushed the eng/NEW-TEST276140main--MacOS-WK1-Debug--mediamedia-sourceworkermedia-managedmse-worker-html--is-a-constant-crash branch from 3996e9f to 2ee6f48 Compare March 21, 2024 11:43
Source/WebCore/platform/MediaStrategy.cpp Outdated Show resolved Hide resolved
Source/WebCore/platform/MediaStrategy.cpp Outdated Show resolved Hide resolved
Source/WebCore/platform/graphics/MediaPlayer.cpp Outdated Show resolved Hide resolved
Source/WebKit/WebProcess/GPU/media/WebMediaStrategy.cpp Outdated Show resolved Hide resolved
@jyavenard jyavenard force-pushed the eng/NEW-TEST276140main--MacOS-WK1-Debug--mediamedia-sourceworkermedia-managedmse-worker-html--is-a-constant-crash branch from 2ee6f48 to 59819db Compare March 22, 2024 01:12
@jyavenard jyavenard requested a review from youennf March 22, 2024 01:20
Copy link
Contributor

@youennf youennf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
I wonder whether overriding mediaSourceInWorkerEnabled setting to false if GPU process is off would be simpler or not.

@jyavenard jyavenard added the merge-queue Applied to send a pull request to merge-queue label Mar 22, 2024
…media-managedmse-worker.html is a constant crash

https://bugs.webkit.org/show_bug.cgi?id=271323
rdar://125100787

Reviewed by Youenn Fablet.

WebPreferences are always set when running LayoutTests. MediaSource in a Worker is not functional
unless the GPU process is active and the MediaPlayers are to run in the GPU process.
MediaSource in a Worker can only work with a MediaSourcePrivate that is designed to be thread-safe
and at present there's only one kind: MediaSourcePrivateRemote.
As it is possible for a WebPreference to be turned on, we need to handle the case where it has
been accidentally set on a non-supported platform.
To achieve this we add a new MediaStrategy API: `hasThreadSafeSupport()` which will be checked
in addition to the `mediaSourceInWorkerEnabled` preference.

Updated existing test to ensure that enabling the pref on a non-supported configuration is
behaving properly.

* LayoutTests/media/media-source/worker/media-managedmse-worker-expected.txt:
* LayoutTests/media/media-source/worker/media-managedmse-worker.html:
* LayoutTests/media/media-source/worker/worker.js:
(onmessage):
* LayoutTests/platform/mac-wk1/TestExpectations:
* LayoutTests/platform/mac-wk1/media/media-source/worker/media-managedmse-worker-expected.txt: Added.
* Source/WTF/Scripts/Preferences/UnifiedWebPreferences.yaml:
* Source/WebCore/Modules/mediasource/MediaSource.cpp:
(WebCore::MediaSource::enabledForContext):
(WebCore::MediaSource::canConstructInDedicatedWorker):
* Source/WebCore/platform/MediaStrategy.cpp:
(WebCore::MediaStrategy::hasThreadSafeMediaSourceSupport const):
* Source/WebCore/platform/MediaStrategy.h:
* Source/WebCore/platform/PlatformStrategies.h:
(WebCore::PlatformStrategies::mediaStrategy): Allow creation of the MediaStrategy object on any threads, in practice this MediaStrategy creation is only
ever done on the main thread, but this could be different in the future.
* Source/WebKit/WebProcess/GPU/media/WebMediaStrategy.cpp:
(WebKit::WebMediaStrategy::createAudioDestination):
(WebKit::WebMediaStrategy::createNowPlayingManager const):
(WebKit::WebMediaStrategy::hasThreadSafeMediaSourceSupport const):
(WebKit::WebMediaStrategy::enableMockMediaSource):
* Source/WebKit/WebProcess/GPU/media/WebMediaStrategy.h:

Canonical link: https://commits.webkit.org/276534@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/NEW-TEST276140main--MacOS-WK1-Debug--mediamedia-sourceworkermedia-managedmse-worker-html--is-a-constant-crash branch from 59819db to b7d8be7 Compare March 22, 2024 09:56
@webkit-commit-queue
Copy link
Collaborator

Committed 276534@main (b7d8be7): https://commits.webkit.org/276534@main

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

@webkit-commit-queue webkit-commit-queue merged commit b7d8be7 into WebKit:main Mar 22, 2024
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Mar 22, 2024
@jyavenard jyavenard deleted the eng/NEW-TEST276140main--MacOS-WK1-Debug--mediamedia-sourceworkermedia-managedmse-worker-html--is-a-constant-crash branch June 13, 2024 00:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New Bugs Unclassified bugs are placed in this component until the correct component can be determined.
Projects
None yet
5 participants