Skip to content

[UnifiedPDF] There's a tile flicker after resizing has finished#34010

Closed
aprotyas wants to merge 1 commit intoWebKit:mainfrom
aprotyas:eng/270041
Closed

[UnifiedPDF] There's a tile flicker after resizing has finished#34010
aprotyas wants to merge 1 commit intoWebKit:mainfrom
aprotyas:eng/270041

Conversation

@aprotyas
Copy link
Member

@aprotyas aprotyas commented Sep 20, 2024

cbececa

[UnifiedPDF] There's a tile flicker after resizing has finished
https://bugs.webkit.org/show_bug.cgi?id=270041
rdar://123566595

Reviewed by NOBODY (OOPS!).

WIP.

* Source/WebCore/WebCore.xcodeproj/project.pbxproj:
* Source/WebCore/platform/graphics/TileConfigurationChangeIdentifier.h: Added.
* Source/WebCore/platform/graphics/TiledBacking.h:
* Source/WebCore/platform/graphics/ca/TileController.cpp:
(WebCore::TileController::activeConfigurationChange const):
(WebCore::TileController::destroyExistingTileConfigurationChange):
(WebCore::TileController::addPendingTilesToActiveTileConfigurationChangeIfNeeded):
(WebCore::TileController::makeTileConfigurationChange):
(WebCore::TileController::didPrepareContentForTile):
(WebCore::TileController::swapPrimaryGridWithPendingTileGrid):
(WebCore::TileController::setNeedsDisplay):
(WebCore::TileController::setNeedsDisplayInRect):
(WebCore::TileController::setContentsScale):
(WebCore::TileController::revalidateTiles):
(WebCore::TileController::tileSize const):
(WebCore::TileController::rectForTile const):
(WebCore::TileController::computeTileSize):
(WebCore::TileController::didRevalidateTiles):
(WebCore::TileController::tileGridExtent const):
(WebCore::TileController::retainedTileBackingStoreMemory const):
(WebCore::TileController::tileCoverageRect const):
* Source/WebCore/platform/graphics/ca/TileController.h:
* Source/WebCore/platform/graphics/ca/TileGrid.cpp:
(WebCore::TileGrid::revalidateTiles):
* Source/WebCore/platform/graphics/ca/TileGrid.h:
* Source/WebCore/platform/graphics/ca/TileGridTypes.h: Added.
* Source/WebKit/WebProcess/Plugins/PDF/UnifiedPDF/AsyncPDFRenderer.h:
* Source/WebKit/WebProcess/Plugins/PDF/UnifiedPDF/AsyncPDFRenderer.mm:
(WebKit::AsyncPDFRenderer::willRepaintTile):
(WebKit::AsyncPDFRenderer::willRemoveTile):
(WebKit::AsyncPDFRenderer::willRepaintAllTiles):
(WebKit::AsyncPDFRenderer::tilingScaleFactorDidChange):
(WebKit::AsyncPDFRenderer::prepareContentForTile):
(WebKit::AsyncPDFRenderer::didPrepareContentForTile):
(WebKit::AsyncPDFRenderer::cancelPrepareContentForTile):
(WebKit::AsyncPDFRenderer::didCancelTileConfigurationChange):
(WebKit::AsyncPDFRenderer::paintPDFIntoBuffer):
(WebKit::AsyncPDFRenderer::transferBufferToMainThread):
(WebKit::AsyncPDFRenderer::tileConfigurationChangeCompletionHandler):
(WebKit::AsyncPDFRenderer::pdfContentScaleChanged):
(WebKit::AsyncPDFRenderer::pdfContentChangedInRect):
* Source/WebKit/WebProcess/Plugins/PDF/UnifiedPDF/PDFDiscretePresentationController.h:
* Source/WebKit/WebProcess/Plugins/PDF/UnifiedPDF/PDFDiscretePresentationController.mm:
(WebKit::PDFDiscretePresentationController::applyWheelEventDelta):
(WebKit::PDFDiscretePresentationController::updateLayersOnLayoutChange):
* Source/WebKit/WebProcess/Plugins/PDF/UnifiedPDF/PDFPresentationController.h:
(WebKit::PDFPresentationController::LayoutChangeInformation::scaleFactorChangedAfterInitialLayout const):
* Source/WebKit/WebProcess/Plugins/PDF/UnifiedPDF/PDFScrollingPresentationController.h:
* Source/WebKit/WebProcess/Plugins/PDF/UnifiedPDF/PDFScrollingPresentationController.mm:
(WebKit::PDFScrollingPresentationController::updateLayersOnLayoutChange):
* Source/WebKit/WebProcess/Plugins/PDF/UnifiedPDF/UnifiedPDFPlugin.h:
* Source/WebKit/WebProcess/Plugins/PDF/UnifiedPDF/UnifiedPDFPlugin.mm:
(WebKit::UnifiedPDFPlugin::updateLayerHierarchy):
(WebKit::UnifiedPDFPlugin::setScaleFactor):
(WebKit::UnifiedPDFPlugin::updateLayout):
* Source/WebKit/WebProcess/WebPage/wc/GraphicsLayerWC.cpp:

cbececa

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ❌ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ❌ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2
✅ 🛠 tv loading 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@aprotyas aprotyas self-assigned this Sep 20, 2024
@aprotyas aprotyas requested a review from cdumez as a code owner September 20, 2024 23:39
@aprotyas aprotyas added the PDF For bugs in WebKit's built-in PDF support. label Sep 20, 2024
Copy link
Member Author

Choose a reason for hiding this comment

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

All of this should get rolled into TileController::tileGrid()

Copy link
Member Author

Choose a reason for hiding this comment

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

i.e. m_asyncTileConfigurationChangeData ? *m_asyncTileConfigurationChangeData->pendingTileGrid : tileGrid();

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Sep 21, 2024
@aprotyas aprotyas removed the merging-blocked Applied to prevent a change from being merged label Sep 22, 2024
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Sep 22, 2024
@aprotyas aprotyas removed the merging-blocked Applied to prevent a change from being merged label Sep 23, 2024
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this really need its own file?

Copy link
Member Author

Choose a reason for hiding this comment

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

It does not, yeah, I just followed precedence set by some other identifier types. I'll probably roll this all into TileGridTypes.h eventually.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe initiateTileConfigurationChange?

Copy link
Contributor

Choose a reason for hiding this comment

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

What is newGrid if DidCancelTileConfigurationChange is Yes?

Copy link
Member Author

Choose a reason for hiding this comment

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

newGrid is unnecessary data if DidCancelTileConfigurationChange::Yes. I'm rolling this together into an optional argument.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would call this completeTileConfigurationChange or commitTileConfigurationChange. It should probably pass the identifier back, and that can be used to identify the old and new grids?

Copy link
Member Author

Choose a reason for hiding this comment

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

The way I've structured the flow now, we need to know the old/new grids before commitTileConfigurationChange is called (since this would be the last step that actually does the grid swap).

So I'm renaming the method, to abstract away from the tile grid swap mechanics, but I'm keeping the signature void(TileGridIdentifier, TileGridIdentifier) for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not DidCancelTileConfigurationChange::Yes unconditionally?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, my comment is wrong. I was thinking of unconditionally DidCancelTileConfigurationChange::Yes here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not convinced that we should tell the client about the new grid yet, but maybe we should before we start asking it to prepare tiles for it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed. Worth an API test.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, we're in an ambiguous situation now; maybe the callers need the rect in the existing grid (if we allow it to remain "live"). Another option is to explicitly specify that the old grid becomes a zombie and won't continue repainting.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say that the client should not get a prepareContentForTile and a willRepaintTile for initial tile creation. But if prepareContentForTile was issued and then state changed, a willRepaintTile could be issued.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why isn't this kicking off PDF buffer generation?

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Sep 23, 2024
@aprotyas aprotyas removed the merging-blocked Applied to prevent a change from being merged label Oct 2, 2024
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 2, 2024
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems potentially problematic.

If we're pinch zooming the PDF, and new scales are arriving to the main thread continuously at a rate faster than the plugin can paint them, won't this just cancel all of the paints?

I think that'd mean you'd just have the initial tiles displayed the whole tile (stretched to the new scale), while we keep doing lots of painting work and not displaying it.

If anything else inside the plugin is changing rendering at the same time (like an animation or something), then those changes wouldn't get displayed until you stopped pinch zooming.

It seems like we should probably suppress scale changes from reaching the plugin/tilecontroller if an existing configuration change is in progress. We'd then need to wait for the commit, the following rendering update (which is where the committed new tile grid gets serialised into a RemoteLayerTreeTransaction), and then schedule yet another rendering update. At that point we can pick up the new scale, and initiate a new configuration change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Am I right in saying that the GraphicsLayers for the individual tiles will be parented to the controller's GraphicsLayer from both the new and old grid simultaneously?

Is that the behaviour we want? I guess the new tiles can asynchronously change from blank to having content and cover the old tiles, and ordering hopefully means they're on top.

Copy link
Contributor

Choose a reason for hiding this comment

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

This might fall into the category of 'too annoying at this point', but could it make sense to have the old tile grid be the one that's stored in a new variable, and the 'pending' grid be the primary tileGrid?

It seems like most operations want to operate on the pending one, we just want to keep the old one around so that it doesn't remove the tiles.

@aprotyas aprotyas removed the merging-blocked Applied to prevent a change from being merged label Oct 10, 2024
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 10, 2024
@aprotyas aprotyas removed the merging-blocked Applied to prevent a change from being merged label Oct 10, 2024
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 10, 2024
@aprotyas aprotyas removed the merging-blocked Applied to prevent a change from being merged label Oct 10, 2024
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 10, 2024
https://bugs.webkit.org/show_bug.cgi?id=270041
rdar://123566595

Reviewed by NOBODY (OOPS!).

WIP.

* Source/WebCore/WebCore.xcodeproj/project.pbxproj:
* Source/WebCore/platform/graphics/TileConfigurationChangeIdentifier.h: Added.
* Source/WebCore/platform/graphics/TiledBacking.h:
* Source/WebCore/platform/graphics/ca/TileController.cpp:
(WebCore::TileController::activeConfigurationChange const):
(WebCore::TileController::destroyExistingTileConfigurationChange):
(WebCore::TileController::addPendingTilesToActiveTileConfigurationChangeIfNeeded):
(WebCore::TileController::makeTileConfigurationChange):
(WebCore::TileController::didPrepareContentForTile):
(WebCore::TileController::swapPrimaryGridWithPendingTileGrid):
(WebCore::TileController::setNeedsDisplay):
(WebCore::TileController::setNeedsDisplayInRect):
(WebCore::TileController::setContentsScale):
(WebCore::TileController::revalidateTiles):
(WebCore::TileController::tileSize const):
(WebCore::TileController::rectForTile const):
(WebCore::TileController::computeTileSize):
(WebCore::TileController::didRevalidateTiles):
(WebCore::TileController::tileGridExtent const):
(WebCore::TileController::retainedTileBackingStoreMemory const):
(WebCore::TileController::tileCoverageRect const):
* Source/WebCore/platform/graphics/ca/TileController.h:
* Source/WebCore/platform/graphics/ca/TileGrid.cpp:
(WebCore::TileGrid::revalidateTiles):
* Source/WebCore/platform/graphics/ca/TileGrid.h:
* Source/WebCore/platform/graphics/ca/TileGridTypes.h: Added.
* Source/WebKit/WebProcess/Plugins/PDF/UnifiedPDF/AsyncPDFRenderer.h:
* Source/WebKit/WebProcess/Plugins/PDF/UnifiedPDF/AsyncPDFRenderer.mm:
(WebKit::AsyncPDFRenderer::willRepaintTile):
(WebKit::AsyncPDFRenderer::willRemoveTile):
(WebKit::AsyncPDFRenderer::willRepaintAllTiles):
(WebKit::AsyncPDFRenderer::tilingScaleFactorDidChange):
(WebKit::AsyncPDFRenderer::prepareContentForTile):
(WebKit::AsyncPDFRenderer::didPrepareContentForTile):
(WebKit::AsyncPDFRenderer::cancelPrepareContentForTile):
(WebKit::AsyncPDFRenderer::didCancelTileConfigurationChange):
(WebKit::AsyncPDFRenderer::paintPDFIntoBuffer):
(WebKit::AsyncPDFRenderer::transferBufferToMainThread):
(WebKit::AsyncPDFRenderer::tileConfigurationChangeCompletionHandler):
(WebKit::AsyncPDFRenderer::pdfContentScaleChanged):
(WebKit::AsyncPDFRenderer::pdfContentChangedInRect):
* Source/WebKit/WebProcess/Plugins/PDF/UnifiedPDF/PDFDiscretePresentationController.h:
* Source/WebKit/WebProcess/Plugins/PDF/UnifiedPDF/PDFDiscretePresentationController.mm:
(WebKit::PDFDiscretePresentationController::applyWheelEventDelta):
(WebKit::PDFDiscretePresentationController::updateLayersOnLayoutChange):
* Source/WebKit/WebProcess/Plugins/PDF/UnifiedPDF/PDFPresentationController.h:
(WebKit::PDFPresentationController::LayoutChangeInformation::scaleFactorChangedAfterInitialLayout const):
* Source/WebKit/WebProcess/Plugins/PDF/UnifiedPDF/PDFScrollingPresentationController.h:
* Source/WebKit/WebProcess/Plugins/PDF/UnifiedPDF/PDFScrollingPresentationController.mm:
(WebKit::PDFScrollingPresentationController::updateLayersOnLayoutChange):
* Source/WebKit/WebProcess/Plugins/PDF/UnifiedPDF/UnifiedPDFPlugin.h:
* Source/WebKit/WebProcess/Plugins/PDF/UnifiedPDF/UnifiedPDFPlugin.mm:
(WebKit::UnifiedPDFPlugin::updateLayerHierarchy):
(WebKit::UnifiedPDFPlugin::setScaleFactor):
(WebKit::UnifiedPDFPlugin::updateLayout):
* Source/WebKit/WebProcess/WebPage/wc/GraphicsLayerWC.cpp:
@aprotyas aprotyas removed the merging-blocked Applied to prevent a change from being merged label Oct 10, 2024
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 11, 2024
@aprotyas
Copy link
Member Author

Succeeded by #35189

@aprotyas aprotyas closed this Oct 15, 2024
@aprotyas aprotyas deleted the eng/270041 branch October 15, 2024 01:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merging-blocked Applied to prevent a change from being merged PDF For bugs in WebKit's built-in PDF support.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants