Summary: | [GPUProcess] Video does not resume if the GPU Process crashes while in fullscreen | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||
Component: | Media | Assignee: | Chris Dumez <cdumez> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | darin, eric.carlson, ews-watchlist, glenn, jer.noble, peng.liu6, philipj, sergio, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Chris Dumez
2020-12-08 15:45:52 PST
Created attachment 415685 [details]
Patch
Comment on attachment 415685 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415685&action=review > Source/WebCore/ChangeLog:11 > + again. However, the full screen logic lives in the UIProcess and I could no figure out how to s/no/not/ > Source/WebCore/ChangeLog:13 > + I have verified that the video renders properly if the user re-enter full screen after a crash. Nice! I assume this patch works for the picture-in-picture case as well, right? Did you see any issue in the animation (exit fullscreen)? (In reply to Peng Liu from comment #2) > Comment on attachment 415685 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=415685&action=review > > > Source/WebCore/ChangeLog:11 > > + again. However, the full screen logic lives in the UIProcess and I could no figure out how to > > s/no/not/ > > > Source/WebCore/ChangeLog:13 > > + I have verified that the video renders properly if the user re-enter full screen after a crash. > > Nice! I assume this patch works for the picture-in-picture case as well, > right? Did you see any issue in the animation (exit fullscreen)? I see no reason why it would no work in PIP but I have not tested. I personally don't see an animation when full screen exits due to a crash. I am not sure why since we are not calling the exitFullScreenWithoutAnination. That said, it does not look bad, especially considering something crashed. Created attachment 415687 [details]
Patch
Comment on attachment 415687 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415687&action=review > Source/WebCore/platform/graphics/MediaPlayer.cpp:535 > + if (client().mediaPlayerFullscreenMode() != MediaPlayer::VideoFullscreenModeNone) > + client().mediaPlayerExitFullscreen(); Not sure it’s self explanatory that "we would play in full screen if we could, but we can’t make that work, so exit full screen to work around that" is the intention here. Maybe add a brief comment? Comment on attachment 415687 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415687&action=review >> Source/WebCore/platform/graphics/MediaPlayer.cpp:535 >> + client().mediaPlayerExitFullscreen(); > > Not sure it’s self explanatory that "we would play in full screen if we could, but we can’t make that work, so exit full screen to work around that" is the intention here. Maybe add a brief comment? Maybe something like: "It would be even better if we could resume in full screen mode, but, for now, exiting full screen makes the video rendering work." Committed r270586: <https://trac.webkit.org/changeset/270586> |