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

When releasing a resize in full screen video, the video content briefly flashes to its original size #17448

Merged
merged 1 commit into from
Sep 5, 2023

Conversation

rr-codes
Copy link
Contributor

@rr-codes rr-codes commented Sep 5, 2023

0de5d9c

When releasing a resize in full screen video, the video content briefly flashes to its original size
https://bugs.webkit.org/show_bug.cgi?id=260558
rdar://113771996

Reviewed by Tim Horton.

When releasing a resize of a window on visionOS that contains a fixed-position element, the element's size
briefly becomes its original size.

In `_endLiveResize`, we create a snapshot view and put it on top of the web content. The snapshot is then
removed in the completion handler of a `doAfterNextPresentationUpdate` call.

However, by the time the transaction commit happens in the presentation update, two important
updates have yet to happen:
1. A visible content rect update
2. A geometry update

The visible content rect update needs to happen before the commit so that all the new sizes are updated.
However, `doAfterNextPresentationUpdate` does not ensure this. To fix, bundle a VCR update inside the
`UpdateGeometry` message, and then have the web page update the VCRs when receiving the message.

The geometry update of the drawing area was not happening because its size was never being set. When a
geometry uodate is deferred, `_frameOrBoundsMayHaveChanged` skips setting all the relevant new values.
Instead, all these values are (theoretically) set in `_didStopDeferringGeometryUpdates`. However,
`_didStopDeferringGeometryUpdates` was missing setting the size of the drawing area.

As a result, in `sendUpdateGeometry`, the layout size and other changes were being updated, but the
drawing area size was the old value. It then eventually does get updated on the next UIKit layout via
`frameOrBoundsMayHaveChanged`, but by that point the snapshot view has already been removed.

Fix by simply ensuring that `_didStopDeferringGeometryUpdates` also updates the drawing area size.

* Source/WebKit/UIProcess/API/ios/WKWebViewIOS.h:
* Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:
(-[WKWebView _createVisibleRectUpdateInfo]):
(-[WKWebView _didStopDeferringGeometryUpdates]):
* Source/WebKit/UIProcess/PageClient.h:
* Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeDrawingAreaProxy.h:
(WebKit::RemoteLayerTreeDrawingAreaProxy::setLastVisibleContentRectUpdate):
(WebKit::RemoteLayerTreeDrawingAreaProxy::lastVisibleContentRectUpdate const):
* Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeDrawingAreaProxy.mm:
(WebKit::RemoteLayerTreeDrawingAreaProxy::sendUpdateGeometry):
* Source/WebKit/UIProcess/ios/PageClientImplIOS.h:
* Source/WebKit/UIProcess/ios/PageClientImplIOS.mm:
(WebKit::PageClientImpl::createVisibleRectUpdateInfo):
* Source/WebKit/UIProcess/ios/WKContentView.h:
* Source/WebKit/UIProcess/ios/WKContentView.mm:
(-[WKContentView createVisibleRectUpdateInfoFromVisibleRect:unobscuredRect:contentInsets:unobscuredRectInScrollViewCoordinates:obscuredInsets:unobscuredSafeAreaInsets:inputViewBounds:scale:minimumScale:viewStability:enclosedInScrollableAncestorView:sendEvenIfUnchanged:]):
(-[WKContentView didUpdateVisibleRect:unobscuredRect:contentInsets:unobscuredRectInScrollViewCoordinates:obscuredInsets:unobscuredSafeAreaInsets:inputViewBounds:scale:minimumScale:viewStability:enclosedInScrollableAncestorView:sendEvenIfUnchanged:]):
* Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:
* Source/WebKit/WebProcess/WebPage/DrawingArea.h:
* Source/WebKit/WebProcess/WebPage/DrawingArea.messages.in:
* Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.h:
* Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.mm:
(WebKit::RemoteLayerTreeDrawingArea::updateGeometry):

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

d3311a1

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
βœ… πŸ§ͺ ios-wk2-wpt βœ… πŸ§ͺ gtk-wk2
βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk2 βœ… πŸ§ͺ api-gtk
βœ… πŸ›  tv   πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  tv-sim
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch
βœ… πŸ›  watch-sim

@rr-codes rr-codes self-assigned this Sep 5, 2023
@rr-codes rr-codes added the Media Bugs related to the HTML 5 Media elements. label Sep 5, 2023
@mwyrzykowski mwyrzykowski self-requested a review September 5, 2023 19:27
@@ -77,7 +77,7 @@ class RemoteLayerTreeDrawingAreaProxy : public DrawingAreaProxy {

// For testing.
unsigned countOfTransactionsWithNonEmptyLayerChanges() const { return m_countOfTransactionsWithNonEmptyLayerChanges; }

Copy link
Contributor

Choose a reason for hiding this comment

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

is this change needed?


std::optional<VisibleContentRectUpdateInfo> visibleRectUpdateInfo;
#if PLATFORM(IOS_FAMILY)
visibleRectUpdateInfo = m_webPageProxy.pageClient().createVisibleContentRectUpdateInfo();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we send an UpdateGeometry message when the window size does not change?

I am wondering if we should only be sending this visibleRectUpdateInfo when the window size changes to avoid a potential performance regression on iOS? @hortont424 do you know?

Or will we always early return if duplicate visibleRectUpdateInfo are sent? Its not immediately clear to me looking at the implementation of WebPage::updateVisibleContentRects

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like there's an early return in DrawingAreaProxy::setSize
(which is upstream of sendUpdateGeometry), so I think it's fine.

@rr-codes rr-codes added the merge-queue Applied to send a pull request to merge-queue label Sep 5, 2023
…ly flashes to its original size

https://bugs.webkit.org/show_bug.cgi?id=260558
rdar://113771996

Reviewed by Tim Horton.

When releasing a resize of a window on visionOS that contains a fixed-position element, the element's size
briefly becomes its original size.

In `_endLiveResize`, we create a snapshot view and put it on top of the web content. The snapshot is then
removed in the completion handler of a `doAfterNextPresentationUpdate` call.

However, by the time the transaction commit happens in the presentation update, two important
updates have yet to happen:
1. A visible content rect update
2. A geometry update

The visible content rect update needs to happen before the commit so that all the new sizes are updated.
However, `doAfterNextPresentationUpdate` does not ensure this. To fix, bundle a VCR update inside the
`UpdateGeometry` message, and then have the web page update the VCRs when receiving the message.

The geometry update of the drawing area was not happening because its size was never being set. When a
geometry uodate is deferred, `_frameOrBoundsMayHaveChanged` skips setting all the relevant new values.
Instead, all these values are (theoretically) set in `_didStopDeferringGeometryUpdates`. However,
`_didStopDeferringGeometryUpdates` was missing setting the size of the drawing area.

As a result, in `sendUpdateGeometry`, the layout size and other changes were being updated, but the
drawing area size was the old value. It then eventually does get updated on the next UIKit layout via
`frameOrBoundsMayHaveChanged`, but by that point the snapshot view has already been removed.

Fix by simply ensuring that `_didStopDeferringGeometryUpdates` also updates the drawing area size.

* Source/WebKit/UIProcess/API/ios/WKWebViewIOS.h:
* Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:
(-[WKWebView _createVisibleRectUpdateInfo]):
(-[WKWebView _didStopDeferringGeometryUpdates]):
* Source/WebKit/UIProcess/PageClient.h:
* Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeDrawingAreaProxy.h:
(WebKit::RemoteLayerTreeDrawingAreaProxy::setLastVisibleContentRectUpdate):
(WebKit::RemoteLayerTreeDrawingAreaProxy::lastVisibleContentRectUpdate const):
* Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeDrawingAreaProxy.mm:
(WebKit::RemoteLayerTreeDrawingAreaProxy::sendUpdateGeometry):
* Source/WebKit/UIProcess/ios/PageClientImplIOS.h:
* Source/WebKit/UIProcess/ios/PageClientImplIOS.mm:
(WebKit::PageClientImpl::createVisibleRectUpdateInfo):
* Source/WebKit/UIProcess/ios/WKContentView.h:
* Source/WebKit/UIProcess/ios/WKContentView.mm:
(-[WKContentView createVisibleRectUpdateInfoFromVisibleRect:unobscuredRect:contentInsets:unobscuredRectInScrollViewCoordinates:obscuredInsets:unobscuredSafeAreaInsets:inputViewBounds:scale:minimumScale:viewStability:enclosedInScrollableAncestorView:sendEvenIfUnchanged:]):
(-[WKContentView didUpdateVisibleRect:unobscuredRect:contentInsets:unobscuredRectInScrollViewCoordinates:obscuredInsets:unobscuredSafeAreaInsets:inputViewBounds:scale:minimumScale:viewStability:enclosedInScrollableAncestorView:sendEvenIfUnchanged:]):
* Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:
* Source/WebKit/WebProcess/WebPage/DrawingArea.h:
* Source/WebKit/WebProcess/WebPage/DrawingArea.messages.in:
* Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.h:
* Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.mm:
(WebKit::RemoteLayerTreeDrawingArea::updateGeometry):

Canonical link: https://commits.webkit.org/267652@main
@webkit-commit-queue
Copy link
Collaborator

Committed 267652@main (0de5d9c): https://commits.webkit.org/267652@main

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

@webkit-commit-queue webkit-commit-queue merged commit 0de5d9c into WebKit:main Sep 5, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Media Bugs related to the HTML 5 Media elements.
Projects
None yet
5 participants