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

Add a layout test for volatile front buffer (forcing a sync GPUP swap). #22958

Conversation

mattwoodrow
Copy link
Contributor

@mattwoodrow mattwoodrow commented Jan 19, 2024

247a813

Add a layout test for volatile front buffer (forcing a sync GPUP swap).
https://bugs.webkit.org/show_bug.cgi?id=267749
<rdar://121232490>

Reviewed by Simon Fraser.

Adds a new 'markFrontBufferVolatileForTesting' function to Internals, which sets
the volatile state on the current front buffer of the Element's layer.

Adds a test that uses this, and would catch the failure from bug 267300.

* LayoutTests/compositing/repaint/needs-no-display-volatile-expected.html: Added.
* LayoutTests/compositing/repaint/needs-no-display-volatile.html: Added.
* Source/WebCore/platform/graphics/GraphicsLayer.h:
(WebCore::GraphicsLayer::markFrontBufferVolatileForTesting):
* Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:
(WebCore::GraphicsLayerCA::markFrontBufferVolatileForTesting):
* Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:
* Source/WebCore/platform/graphics/ca/PlatformCALayer.h:
* Source/WebCore/rendering/RenderLayer.cpp:
(WebCore::RenderLayer::calculateClipRects const):
* Source/WebCore/rendering/RenderLayer.h:
* Source/WebCore/rendering/RenderLayerBacking.cpp:
(WebCore::RenderLayerBacking::markFrontBufferVolatileForTesting):
* Source/WebCore/rendering/RenderLayerBacking.h:
* Source/WebCore/testing/Internals.cpp:
(WebCore::Internals::markFrontBufferVolatile):
* Source/WebCore/testing/Internals.h:
* Source/WebCore/testing/Internals.idl:
* Source/WebKit/GPUProcess/graphics/RemoteImageBufferSet.cpp:
(WebKit::RemoteImageBufferSet::ensureBufferForDisplay):
* Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.h:
* Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.mm:
(WebKit::RemoteLayerBackingStore::markFrontBufferVolatileForTesting):
* Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStoreCollection.h:
* Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStoreCollection.mm:
(WebKit::RemoteLayerBackingStoreCollection::prepareBackingStoresForDisplay):
(WebKit::RemoteLayerBackingStoreCollection::markFrontBufferVolatileForTesting):
* Source/WebKit/Shared/RemoteLayerTree/RemoteLayerWithInProcessRenderingBackingStore.h:
* Source/WebKit/Shared/RemoteLayerTree/RemoteLayerWithRemoteRenderingBackingStore.h:
* Source/WebKit/Shared/RemoteLayerTree/RemoteLayerWithRemoteRenderingBackingStore.mm:
(WebKit::RemoteLayerWithRemoteRenderingBackingStore::prepareToDisplay):
* Source/WebKit/WebProcess/WebPage/RemoteLayerTree/PlatformCALayerRemote.h:
* Source/WebKit/WebProcess/WebPage/RemoteLayerTree/PlatformCALayerRemote.mm:
(WebKit::PlatformCALayerRemote::markFrontBufferVolatileForTesting):

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

c5bdc06

Misc iOS, tvOS & watchOS macOS Linux Windows
❌ πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe   πŸ›  wincairo
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug   πŸ§ͺ wpe-wk2
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac βœ… πŸ§ͺ api-wpe
  πŸ§ͺ ios-wk2-wpt βœ… πŸ§ͺ mac-wk1 βœ… πŸ›  gtk
βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk2 βœ… πŸ§ͺ gtk-wk2
βœ… πŸ›  tv βœ… πŸ§ͺ mac-AS-debug-wk2 ❌ πŸ§ͺ api-gtk
  πŸ›  tv-sim βœ… πŸ§ͺ mac-wk2-stress
  πŸ›  watch
  πŸ›  πŸ§ͺ unsafe-merge βœ… πŸ›  watch-sim

@mattwoodrow mattwoodrow self-assigned this Jan 19, 2024
@mattwoodrow mattwoodrow added the Layout and Rendering For bugs with layout and rendering of Web pages. label Jan 19, 2024
webkit-commit-queue pushed a commit to mattwoodrow/WebKit that referenced this pull request Jan 25, 2024
https://bugs.webkit.org/show_bug.cgi?id=268025
<rdar://121488303>

Reviewed by Simon Fraser.

Creating a flusher for a RemoteLayerBackingStore now waits for backend handles to
be delivered as well as flushing any drawing. We need to call this even when the dirty
region is empty, so that we get handles delivered.

Adds an enum to describe what we want to wait for, so that can avoid flushing the drawing
(or creating a flusher entirely) if no drawing commands have been issued.

https://bugs.webkit.org/show_bug.cgi?id=267749 (PR WebKit#22958) adds a test that will catch this.

* Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.h:
* Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.mm:
(WebKit::RemoteLayerBackingStore::paintContents):
* Source/WebKit/Shared/RemoteLayerTree/RemoteLayerWithInProcessRenderingBackingStore.h:
* Source/WebKit/Shared/RemoteLayerTree/RemoteLayerWithInProcessRenderingBackingStore.mm:
(WebKit::RemoteLayerWithInProcessRenderingBackingStore::createFlusher):
* Source/WebKit/Shared/RemoteLayerTree/RemoteLayerWithRemoteRenderingBackingStore.h:
* Source/WebKit/Shared/RemoteLayerTree/RemoteLayerWithRemoteRenderingBackingStore.mm:
(WebKit::RemoteLayerWithRemoteRenderingBackingStore::createFlusher):
* Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferSetProxy.cpp:
(WebKit::RemoteImageBufferSetProxyFlushFence::create):
(WebKit::RemoteImageBufferSetProxyFlushFence::waitFor):
(WebKit::RemoteImageBufferSetProxyFlushFence::tryTakeEvent):
(WebKit::RemoteImageBufferSetProxyFlushFence::RemoteImageBufferSetProxyFlushFence):
(WebKit::RemoteImageBufferSetProxy::flushFrontBufferAsync):
* Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferSetProxy.h:

Canonical link: https://commits.webkit.org/273475@main
void RemoteLayerBackingStoreCollection::markFrontBufferVolatileForTesting(RemoteLayerBackingStore& backingStore)
{
if (auto* remoteBackingStore = dynamicDowncast<RemoteLayerWithRemoteRenderingBackingStore>(&backingStore)) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Blank line here.

if (auto* remoteBackingStore = dynamicDowncast<RemoteLayerWithRemoteRenderingBackingStore>(&backingStore)) {

auto bufferSet = remoteBackingStore->protectedBufferSet();
if (bufferSet) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if (auto bufferSet = remoteBackingStore->protectedBufferSet())

Copy link
Contributor

Choose a reason for hiding this comment

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

We normally prefer:

if (RefPtr bufferSet = remoteBackingStore->bufferSet())

The protectedFoo() functions are to avoid local variables.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't hide smart pointers behind auto. As we're adopting smart pointers, it is very convenient to see right away that we have a RefPtr here.

@@ -5078,6 +5078,12 @@ void GraphicsLayerCA::purgeBackBufferForTesting()
primaryLayer()->purgeBackBufferForTesting();
}

void GraphicsLayerCA::markFrontBufferVolatileForTesting()
{
if (primaryLayer())
Copy link
Contributor

Choose a reason for hiding this comment

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

Please should probably use a RefPtr here per our guidelines:

if (RefPtr layer = primaryLayer())
    layer->markFrontBufferVolatileForTesting();

@@ -5803,6 +5803,12 @@ void RenderLayer::purgeBackBufferForTesting()
backing()->purgeBackBufferForTesting();
}

void RenderLayer::markFrontBufferVolatileForTesting()
{
if (backing())
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally we'd use a CheckedPtr here since the backing is not ref counted.

@@ -4326,4 +4326,10 @@ void RenderLayerBacking::purgeBackBufferForTesting()
m_graphicsLayer->purgeBackBufferForTesting();
}

void RenderLayerBacking::markFrontBufferVolatileForTesting()
{
if (m_graphicsLayer)
Copy link
Contributor

Choose a reason for hiding this comment

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

You need a local smart pointer, per our guidelines:

if (RefPtr graphicsLayer = m_graphicsLayer)
    graphicsLayer->markFrontBufferVolatileForTesting();

@mattwoodrow mattwoodrow changed the title Add a layout test for volatile front buffer (forcing a sync GPUP swap). Corrupt tiles when scrolling and selecting text quickly. Jan 31, 2024
@mattwoodrow mattwoodrow force-pushed the eng/books-unlock-flicker-with-test branch from 14657d4 to 3ae821b Compare January 31, 2024 02:40
@@ -288,10 +288,12 @@ class RenderLayerBacking final : public GraphicsLayerClient {
WEBCORE_EXPORT void setIsTrackingDisplayListReplay(bool);
WEBCORE_EXPORT String replayDisplayListAsText(OptionSet<DisplayList::AsTextFlag>) const;


Copy link
Contributor

Choose a reason for hiding this comment

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

Extra blank line here.

void Internals::markFrontBufferVolatile(Element& element)
{
if (element.renderer() && element.renderer()->enclosingLayer())
element.renderer()->enclosingLayer()->markFrontBufferVolatileForTesting();
Copy link
Contributor

Choose a reason for hiding this comment

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

enclosingLayer should be in a CheckedPtr before calling the member function.

@@ -600,6 +600,12 @@ static bool hasValue(const ImageBufferBackendHandle& backendHandle)
collection->purgeBackBufferForTesting(*this);
}

void RemoteLayerBackingStore::markFrontBufferVolatileForTesting()
{
if (auto* collection = backingStoreCollection())
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally would use a CheckedPtr since not ref counted.

sendMarkBuffersVolatile(WTFMove(identifiers), [](bool) { });
}
} else {
auto& inProcessBackingStore = downcast<RemoteLayerWithInProcessRenderingBackingStore>(backingStore);
Copy link
Contributor

Choose a reason for hiding this comment

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

CheckedRef.

@@ -1101,5 +1101,11 @@ static inline bool isEquivalentLayer(const PlatformCALayer* layer, const std::op
return m_properties.backingStoreOrProperties.store->purgeBackBufferForTesting();
}

void PlatformCALayerRemote::markFrontBufferVolatileForTesting()
{
if (m_properties.backingStoreOrProperties.store)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should put the store in a CheckedPtr local before calling the member function on it (since not ref counted).

@mattwoodrow mattwoodrow changed the title Corrupt tiles when scrolling and selecting text quickly. Add a layout test for volatile front buffer (forcing a sync GPUP swap). Jan 31, 2024
@mattwoodrow mattwoodrow force-pushed the eng/books-unlock-flicker-with-test branch from 3ae821b to 5972f19 Compare January 31, 2024 03:15
@mattwoodrow mattwoodrow force-pushed the eng/books-unlock-flicker-with-test branch from 5972f19 to c5bdc06 Compare January 31, 2024 03:28
@mattwoodrow mattwoodrow added merge-queue Applied to send a pull request to merge-queue unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing and removed unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing labels Jan 31, 2024
https://bugs.webkit.org/show_bug.cgi?id=267749
<rdar://121232490>

Reviewed by Simon Fraser.

Adds a new 'markFrontBufferVolatileForTesting' function to Internals, which sets
the volatile state on the current front buffer of the Element's layer.

Adds a test that uses this, and would catch the failure from bug 267300.

* LayoutTests/compositing/repaint/needs-no-display-volatile-expected.html: Added.
* LayoutTests/compositing/repaint/needs-no-display-volatile.html: Added.
* Source/WebCore/platform/graphics/GraphicsLayer.h:
(WebCore::GraphicsLayer::markFrontBufferVolatileForTesting):
* Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:
(WebCore::GraphicsLayerCA::markFrontBufferVolatileForTesting):
* Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:
* Source/WebCore/platform/graphics/ca/PlatformCALayer.h:
* Source/WebCore/rendering/RenderLayer.cpp:
(WebCore::RenderLayer::calculateClipRects const):
* Source/WebCore/rendering/RenderLayer.h:
* Source/WebCore/rendering/RenderLayerBacking.cpp:
(WebCore::RenderLayerBacking::markFrontBufferVolatileForTesting):
* Source/WebCore/rendering/RenderLayerBacking.h:
* Source/WebCore/testing/Internals.cpp:
(WebCore::Internals::markFrontBufferVolatile):
* Source/WebCore/testing/Internals.h:
* Source/WebCore/testing/Internals.idl:
* Source/WebKit/GPUProcess/graphics/RemoteImageBufferSet.cpp:
(WebKit::RemoteImageBufferSet::ensureBufferForDisplay):
* Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.h:
* Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.mm:
(WebKit::RemoteLayerBackingStore::markFrontBufferVolatileForTesting):
* Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStoreCollection.h:
* Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStoreCollection.mm:
(WebKit::RemoteLayerBackingStoreCollection::prepareBackingStoresForDisplay):
(WebKit::RemoteLayerBackingStoreCollection::markFrontBufferVolatileForTesting):
* Source/WebKit/Shared/RemoteLayerTree/RemoteLayerWithInProcessRenderingBackingStore.h:
* Source/WebKit/Shared/RemoteLayerTree/RemoteLayerWithRemoteRenderingBackingStore.h:
* Source/WebKit/Shared/RemoteLayerTree/RemoteLayerWithRemoteRenderingBackingStore.mm:
(WebKit::RemoteLayerWithRemoteRenderingBackingStore::prepareToDisplay):
* Source/WebKit/WebProcess/WebPage/RemoteLayerTree/PlatformCALayerRemote.h:
* Source/WebKit/WebProcess/WebPage/RemoteLayerTree/PlatformCALayerRemote.mm:
(WebKit::PlatformCALayerRemote::markFrontBufferVolatileForTesting):

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

Committed 273814@main (247a813): https://commits.webkit.org/273814@main

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

@webkit-commit-queue webkit-commit-queue merged commit 247a813 into WebKit:main Jan 31, 2024
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Layout and Rendering For bugs with layout and rendering of Web pages.
Projects
None yet
5 participants