MEDIA_ERR_ABORTED after restore page with media from bfcache - #1721
Open
pgorszkowski-igalia wants to merge 1 commit into
Open
Conversation
https://bugs.webkit.org/show_bug.cgi?id=319665 Reviewed by Alicia Boya Garcia and Nikolas Zimmermann. This bundles three related fixes uncovered while restoring pages containing a media element (e.g.: "video") from the back/forward cache: 1. HTMLMediaElement::suspend() destroyed the media player on entering the cache (via stopWithoutDestroyingMediaPlayer() -> userCancelledLoad()), so a restored page reported video.error.code == MEDIA_ERR_ABORTED. userCancelledLoad() now takes a destroyMediaPlayer flag, false for the BackForwardCache suspension path, so it early-returns instead of tearing the player down. 2. A <video> that was playing before being cached did not resume playing after restore. HTMLMediaElement::resume() schedules an updatePlayState() task while the media session is still interrupted. mayResumePlayback() now also retries updatePlayState() when the element isn't paused. 3. A <video> that was paused before being cached showed a blank/white frame instead of its last decoded frame after restore. Restoring a page rebuilds the render tree, so CoordinatedPlatformLayerBufferProxy gets attached to a brand new CoordinatedPlatformLayer with no buffer of its own, and a paused GStreamer pipeline has no new sample to push. The proxy now invokes a layer-attached callback whenever a new non-null layer is attached, which MediaPlayerPrivateGStreamer uses to re-deliver its current sample so the new layer isn't left blank. Also fixes a related macOS-only assert crash (ASSERTION FAILED: m_mediaSessions.contains(identifier) in MediaUsageManagerCocoa:: updateMediaUsage): the UI process unconditionally clears its usage-tracking map on every navigation commit, including a back/forward cache restore, but MediaElementSession kept believing it had already added its session across the whole suspend/resume cycle. mediaUsageManagerSessionWillBeSuspended() resets that flag on suspend so the next update re-adds the session first. A LayoutTest for the playing-before-caching scenario (fix #2): it plays a video, waits for the playing event, navigates away and back via history.back(), and asserts the video is still not paused and currentTime has advanced after restore. Added a LayoutTest based on media/restore-from-page-cache.html for a video that was paused before entering the cache, as a real pixel/screenshot comparison to catch the blank-frame regression from fix #3. Tests: media/restore-from-page-cache-paused.html media/restore-from-page-cache-playing.html * LayoutTests/media/restore-from-page-cache-paused-expected.png: Added. * LayoutTests/media/restore-from-page-cache-paused-expected.txt: Added. * LayoutTests/media/restore-from-page-cache-paused.html: Added. * LayoutTests/media/restore-from-page-cache-playing-expected.txt: Added. * LayoutTests/media/restore-from-page-cache-playing.html: Added. * LayoutTests/platform/ios/TestExpectations: * Source/WebCore/html/HTMLMediaElement.cpp: (WebCore::HTMLMediaElement::userCancelledLoad): (WebCore::HTMLMediaElement::stopWithoutDestroyingMediaPlayer): (WebCore::HTMLMediaElement::suspend): (WebCore::HTMLMediaElement::mayResumePlayback): * Source/WebCore/html/HTMLMediaElement.h: * Source/WebCore/html/MediaElementSession.cpp: (WebCore::MediaElementSession::mediaUsageManagerSessionWillBeSuspended): * Source/WebCore/html/MediaElementSession.h: * Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp: (WebCore::MediaPlayerPrivateGStreamer::MediaPlayerPrivateGStreamer): (WebCore::MediaPlayerPrivateGStreamer::pushTextureToCompositor): (WebCore::MediaPlayerPrivateGStreamer::triggerRepaint): * Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h: * Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedPlatformLayerBufferProxy.cpp: (WebCore::CoordinatedPlatformLayerBufferProxy::CoordinatedPlatformLayerBufferProxy): (WebCore::CoordinatedPlatformLayerBufferProxy::create): (WebCore::CoordinatedPlatformLayerBufferProxy::setTargetLayer): * Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedPlatformLayerBufferProxy.h: Canonical link: https://commits.webkit.org/319472@main
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://bugs.webkit.org/show_bug.cgi?id=319665
Reviewed by Alicia Boya Garcia and Nikolas Zimmermann.
This bundles three related fixes uncovered while restoring pages containing a media element (e.g.: "video") from the back/forward cache:
HTMLMediaElement::suspend() destroyed the media player on entering the cache (via stopWithoutDestroyingMediaPlayer() -> userCancelledLoad()), so a restored page reported video.error.code == MEDIA_ERR_ABORTED. userCancelledLoad() now takes a destroyMediaPlayer flag, false for the BackForwardCache suspension path, so it early-returns instead of tearing the player down.
A
A
Also fixes a related macOS-only assert crash (ASSERTION FAILED: m_mediaSessions.contains(identifier) in MediaUsageManagerCocoa:: updateMediaUsage): the UI process unconditionally clears its usage-tracking map on every navigation commit, including a back/forward cache restore, but MediaElementSession kept believing it had already added its session across the whole suspend/resume cycle. mediaUsageManagerSessionWillBeSuspended() resets that flag on suspend so the next update re-adds the session first.
A LayoutTest for the playing-before-caching scenario (fix #2): it plays a video, waits for the playing event, navigates away and back via history.back(), and asserts the video is still not paused and currentTime has advanced after restore.
Added a LayoutTest based on media/restore-from-page-cache.html for a video that was paused before entering the cache, as a real pixel/screenshot comparison to catch the blank-frame regression from fix #3.
Tests: media/restore-from-page-cache-paused.html
media/restore-from-page-cache-playing.html
(WebCore::HTMLMediaElement::stopWithoutDestroyingMediaPlayer): (WebCore::HTMLMediaElement::suspend):
(WebCore::HTMLMediaElement::mayResumePlayback):
Canonical link: https://commits.webkit.org/319472@main
3d22762