No videos play if opened in full screen on first play #12422

pvollan · 2023-04-05T20:45:30Z

`62b9ab7`

No videos play if opened in full screen on first play
https://bugs.webkit.org/show_bug.cgi?id=255048
rdar://107540521

Reviewed by Eric Carlson and Jean-Yves Avenard.

When CA layers are not remotely hosted in both the GPU process and the WebContent process, it is not
guaranteed that the remote context ID will be set in the HTML media element when entering fullscreen.
This patch works around that by delaying setting up the fullscreen until the video dimensions are set.
At this point, the remote context ID should be available.

* Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.h:
* Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.mm:
(WebKit::VideoFullscreenManager::enterVideoFullscreenForVideoElement):
(WebKit::VideoFullscreenManager::videoDimensionsChanged):

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

4c6e1cb

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 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

webkit-early-warning-system · 2023-04-05T20:46:40Z

EWS run on previous version of this PR (hash 2c4b031)

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

webkit-early-warning-system · 2023-04-05T21:11:07Z

EWS run on previous version of this PR (hash 977c100)

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

eric-carlson · 2023-04-05T21:36:05Z

Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.h

@@ -190,6 +192,7 @@ class VideoFullscreenManager : public RefCounted<VideoFullscreenManager>, privat
    HashMap<PlaybackSessionContextIdentifier, int> m_clientCounts;
    WeakPtr<WebCore::HTMLVideoElement, WebCore::WeakPtrImplWithEventTargetData> m_videoElementInPictureInPicture;
    bool m_currentlyInFullscreen { false };
+    WTF::Function<void(LayerHostingContextID)> m_setupFullscreen;


Nit: I'm not wild about this name, maybe m_setupFullscreenHandler?

Fixed in latest patch.

Thanks for reviewing!

jyavenard

typo in the commit log:
s/guranteed/guaranteed

some comments

jyavenard · 2023-04-05T21:19:40Z

Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.h

@@ -190,6 +192,7 @@ class VideoFullscreenManager : public RefCounted<VideoFullscreenManager>, privat
    HashMap<PlaybackSessionContextIdentifier, int> m_clientCounts;
    WeakPtr<WebCore::HTMLVideoElement, WebCore::WeakPtrImplWithEventTargetData> m_videoElementInPictureInPicture;
    bool m_currentlyInFullscreen { false };
+    WTF::Function<void(LayerHostingContextID)> m_setupFullscreen;


Can this be a CompletionHandler?

Can this be a CompletionHandler?

Yes, I think it could. I have kept is as Function, though, since I think both make sense in this context.

jyavenard · 2023-04-05T22:03:09Z

Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.mm

+
+    if (m_setupFullscreen) {
+        auto [model, interface] = ensureModelAndInterface(contextId);
+        RefPtr<HTMLVideoElement> videoElement = model->videoElement();


auto videoElement

need null check

Good catch! Fixed in latest patch.

Thanks for reviewing!

webkit-early-warning-system · 2023-04-05T22:08:11Z

EWS run on previous version of this PR (hash 413c282)

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

webkit-early-warning-system · 2023-04-05T22:43:36Z

EWS run on previous version of this PR (hash ab15958)

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

webkit-early-warning-system · 2023-04-05T22:48:50Z

EWS run on current version of this PR (hash 4c6e1cb)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 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

https://bugs.webkit.org/show_bug.cgi?id=255048 rdar://107540521 Reviewed by Eric Carlson and Jean-Yves Avenard. When CA layers are not remotely hosted in both the GPU process and the WebContent process, it is not guaranteed that the remote context ID will be set in the HTML media element when entering fullscreen. This patch works around that by delaying setting up the fullscreen until the video dimensions are set. At this point, the remote context ID should be available. * Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.h: * Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.mm: (WebKit::VideoFullscreenManager::enterVideoFullscreenForVideoElement): (WebKit::VideoFullscreenManager::videoDimensionsChanged): Canonical link: https://commits.webkit.org/262654@main

webkit-commit-queue · 2023-04-06T02:34:14Z

Committed 262654@main (62b9ab7): https://commits.webkit.org/262654@main

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

pvollan requested a review from cdumez as a code owner April 5, 2023 20:45

pvollan self-assigned this Apr 5, 2023

pvollan added the WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore). label Apr 5, 2023

pvollan requested review from eric-carlson, jernoble and geoffreygaren April 5, 2023 20:47

pvollan force-pushed the eng/No-videos-play-if-opened-in-full-screen-on-first-play branch from 2c4b031 to 977c100 Compare April 5, 2023 21:08

eric-carlson approved these changes Apr 5, 2023

View reviewed changes

jyavenard approved these changes Apr 5, 2023

View reviewed changes

pvollan force-pushed the eng/No-videos-play-if-opened-in-full-screen-on-first-play branch from 977c100 to 413c282 Compare April 5, 2023 22:07

pvollan force-pushed the eng/No-videos-play-if-opened-in-full-screen-on-first-play branch from 413c282 to ab15958 Compare April 5, 2023 22:43

pvollan force-pushed the eng/No-videos-play-if-opened-in-full-screen-on-first-play branch from ab15958 to 4c6e1cb Compare April 5, 2023 22:48

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

pvollan added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels Apr 6, 2023

webkit-commit-queue force-pushed the eng/No-videos-play-if-opened-in-full-screen-on-first-play branch from 4c6e1cb to 62b9ab7 Compare April 6, 2023 02:34

webkit-commit-queue merged commit 62b9ab7 into WebKit:main Apr 6, 2023

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

No videos play if opened in full screen on first play #12422

No videos play if opened in full screen on first play #12422

pvollan commented Apr 5, 2023 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Apr 5, 2023 •

edited

webkit-early-warning-system commented Apr 5, 2023 •

edited

eric-carlson Apr 5, 2023

pvollan Apr 5, 2023

jyavenard left a comment

jyavenard Apr 5, 2023

pvollan Apr 5, 2023

jyavenard Apr 5, 2023

pvollan Apr 5, 2023

webkit-early-warning-system commented Apr 5, 2023 •

edited

webkit-early-warning-system commented Apr 5, 2023 •

edited

webkit-early-warning-system commented Apr 5, 2023 •

edited

webkit-commit-queue commented Apr 6, 2023

No videos play if opened in full screen on first play #12422

No videos play if opened in full screen on first play #12422

Conversation

pvollan commented Apr 5, 2023 • edited by webkit-early-warning-system

62b9ab7

webkit-early-warning-system commented Apr 5, 2023 • edited

webkit-early-warning-system commented Apr 5, 2023 • edited

eric-carlson Apr 5, 2023

Choose a reason for hiding this comment

pvollan Apr 5, 2023

Choose a reason for hiding this comment

jyavenard left a comment

Choose a reason for hiding this comment

jyavenard Apr 5, 2023

Choose a reason for hiding this comment

pvollan Apr 5, 2023

Choose a reason for hiding this comment

jyavenard Apr 5, 2023

Choose a reason for hiding this comment

pvollan Apr 5, 2023

Choose a reason for hiding this comment

webkit-early-warning-system commented Apr 5, 2023 • edited

webkit-early-warning-system commented Apr 5, 2023 • edited

webkit-early-warning-system commented Apr 5, 2023 • edited

webkit-commit-queue commented Apr 6, 2023

pvollan commented Apr 5, 2023 •

edited by webkit-early-warning-system

`62b9ab7`

webkit-early-warning-system commented Apr 5, 2023 •

edited

webkit-early-warning-system commented Apr 5, 2023 •

edited

webkit-early-warning-system commented Apr 5, 2023 •

edited

webkit-early-warning-system commented Apr 5, 2023 •

edited

webkit-early-warning-system commented Apr 5, 2023 •

edited