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.

Sign up for GitHub

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

Jump to bottom

In PWA, HTML Video Element may be unable to play stream from 'getUserMedia()' #15370

Merged
merged 1 commit into from
Jul 12, 2023
Merged

In PWA, HTML Video Element may be unable to play stream from 'getUserMedia()' #15370

merged 1 commit into from
Jul 12, 2023

Conversation

youennf
Copy link
Contributor

@youennf youennf commented Jun 28, 2023 •
edited by webkit-early-warning-system
Loading

bbf6021

In PWA, HTML Video Element may be unable to play stream from 'getUserMedia()'
https://bugs.webkit.org/show_bug.cgi?id=252465
rdar://111500785

Reviewed by Eric Carlson.

In case the same GPU process is used by several SafariViewServices, mediaserverd may think that
a capturing session is in the background while it is in foreground.
Ideally mediaserverd would compute this from an audit token or something similar we would pass them.

Currently, it is based on pid forwarding.
As a temporary workaround, we are now storing which pid is to be used for a given WebProcess in RemoteMediaSessionHelperProxy.
When starting to produce data for camera, we are then ensuring that this pid is correctly set whenever starting to produce data.

We allow overriding of pid in MediaSessionHelperiOS, by adding a new ShouldOverride boolean.
Only AVVideoCaptureSource is using ShouldOverride::Yes, which should limit potential fallouts to camera capture cases.

* Source/WebCore/platform/audio/ios/MediaSessionHelperIOS.h:
* Source/WebCore/platform/audio/ios/MediaSessionHelperIOS.mm:
(MediaSessionHelperiOS::providePresentingApplicationPID):
* Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:
(WebKit::GPUConnectionToWebProcess::overridePresentingApplicationPIDIfNeeded):
* Source/WebKit/GPUProcess/GPUConnectionToWebProcess.h:
* Source/WebKit/GPUProcess/media/ios/RemoteMediaSessionHelperProxy.cpp:
(WebKit::RemoteMediaSessionHelperProxy::providePresentingApplicationPID):
(WebKit::RemoteMediaSessionHelperProxy::overridePresentingApplicationPIDIfNeeded):
* Source/WebKit/GPUProcess/media/ios/RemoteMediaSessionHelperProxy.h:
* Source/WebKit/UIProcess/Cocoa/UserMediaCaptureManagerProxy.cpp:
(WebKit::UserMediaCaptureManagerProxy::startProducingData):
* Source/WebKit/UIProcess/Cocoa/UserMediaCaptureManagerProxy.h:
(WebKit::UserMediaCaptureManagerProxy::ConnectionProxy::startProducingData):
* Source/WebKit/WebProcess/GPU/media/ios/RemoteMediaSessionHelper.cpp:
(WebKit::RemoteMediaSessionHelper::providePresentingApplicationPID):
* Source/WebKit/WebProcess/GPU/media/ios/RemoteMediaSessionHelper.h:

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

30c22a9

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
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@youennf youennf self-assigned this Jun 28, 2023
@youennf youennf added the WebRTC For bugs in WebRTC label Jun 28, 2023
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 28, 2023 •
edited
Loading

EWS run on previous version of this PR (hash 77febf2)

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
✅ 🛠 watch
✅ 🛠 watch-sim

@youennf youennf force-pushed the eng/In-PWA-HTML-Video-Element-may-be-unable-to-play-stream-from-getUserMedia branch from 77febf2 to 2628bfa Compare June 28, 2023 15:51
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 28, 2023 •
edited
Loading

EWS run on previous version of this PR (hash 2628bfa)

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
✅ 🛠 watch
✅ 🛠 watch-sim

@youennf youennf marked this pull request as ready for review June 28, 2023 15:52
@youennf youennf requested a review from cdumez as a code owner June 28, 2023 15:52
@youennf youennf requested a review from eric-carlson June 28, 2023 15:57
eric-carlson
eric-carlson approved these changes Jun 28, 2023
View reviewed changes
Copy link
Contributor

@eric-carlson eric-carlson left a comment

Choose a reason for hiding this comment

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

r=me

@youennf youennf force-pushed the eng/In-PWA-HTML-Video-Element-may-be-unable-to-play-stream-from-getUserMedia branch from 2628bfa to 30c22a9 Compare June 29, 2023 08:40
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 29, 2023 •
edited
Loading

EWS run on current version of this PR (hash 30c22a9)

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
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@youennf youennf requested a review from jernoble June 29, 2023 08:42
@incognitojam incognitojam mentioned this pull request Jul 10, 2023
Closed
@youennf youennf added the merge-queue Applied to send a pull request to merge-queue label Jul 12, 2023
@youennf
In PWA, HTML Video Element may be unable to play stream from 'getUser…
bbf6021 
…Media()'

https://bugs.webkit.org/show_bug.cgi?id=252465
rdar://111500785

Reviewed by Eric Carlson.

In case the same GPU process is used by several SafariViewServices, mediaserverd may think that
a capturing session is in the background while it is in foreground.
Ideally mediaserverd would compute this from an audit token or something similar we would pass them.

Currently, it is based on pid forwarding.
As a temporary workaround, we are now storing which pid is to be used for a given WebProcess in RemoteMediaSessionHelperProxy.
When starting to produce data for camera, we are then ensuring that this pid is correctly set whenever starting to produce data.

We allow overriding of pid in MediaSessionHelperiOS, by adding a new ShouldOverride boolean.
Only AVVideoCaptureSource is using ShouldOverride::Yes, which should limit potential fallouts to camera capture cases.

* Source/WebCore/platform/audio/ios/MediaSessionHelperIOS.h:
* Source/WebCore/platform/audio/ios/MediaSessionHelperIOS.mm:
(MediaSessionHelperiOS::providePresentingApplicationPID):
* Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:
(WebKit::GPUConnectionToWebProcess::overridePresentingApplicationPIDIfNeeded):
* Source/WebKit/GPUProcess/GPUConnectionToWebProcess.h:
* Source/WebKit/GPUProcess/media/ios/RemoteMediaSessionHelperProxy.cpp:
(WebKit::RemoteMediaSessionHelperProxy::providePresentingApplicationPID):
(WebKit::RemoteMediaSessionHelperProxy::overridePresentingApplicationPIDIfNeeded):
* Source/WebKit/GPUProcess/media/ios/RemoteMediaSessionHelperProxy.h:
* Source/WebKit/UIProcess/Cocoa/UserMediaCaptureManagerProxy.cpp:
(WebKit::UserMediaCaptureManagerProxy::startProducingData):
* Source/WebKit/UIProcess/Cocoa/UserMediaCaptureManagerProxy.h:
(WebKit::UserMediaCaptureManagerProxy::ConnectionProxy::startProducingData):
* Source/WebKit/WebProcess/GPU/media/ios/RemoteMediaSessionHelper.cpp:
(WebKit::RemoteMediaSessionHelper::providePresentingApplicationPID):
* Source/WebKit/WebProcess/GPU/media/ios/RemoteMediaSessionHelper.h:

Canonical link: https://commits.webkit.org/265986@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/In-PWA-HTML-Video-Element-may-be-unable-to-play-stream-from-getUserMedia branch from 30c22a9 to bbf6021 Compare July 12, 2023 07:02
@webkit-commit-queue
Copy link
Collaborator

Committed 265986@main (bbf6021): https://commits.webkit.org/265986@main

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

@webkit-commit-queue webkit-commit-queue merged commit bbf6021 into WebKit:main Jul 12, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jul 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jernoble jernoble jernoble requested changes

@eric-carlson eric-carlson eric-carlson approved these changes

@cdumez cdumez Awaiting requested review from cdumez cdumez is a code owner

Assignees

@youennf youennf

Labels
WebRTC For bugs in WebRTC
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@youennf @webkit-early-warning-system @webkit-commit-queue @eric-carlson @jernoble