Bug 197561 - Implement "shared" compositing layers, allowing overlap layers to paint into the backing store of another layer
Summary: Implement "shared" compositing layers, allowing overlap layers to paint into ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Compositing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
: 197594 (view as bug list)
Depends on: 197562 197564 197565 197595 197692 197694 197766 197768 197776 197783 197811 197824
Blocks:
  Show dependency treegraph
 
Reported: 2019-05-03 09:23 PDT by Simon Fraser (smfr)
Modified: 2019-05-12 11:28 PDT (History)
9 users (show)

See Also:


Attachments
Patch (157.56 KB, patch)
2019-05-04 19:28 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-highsierra (3.48 MB, application/zip)
2019-05-04 20:37 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (2.77 MB, application/zip)
2019-05-04 20:48 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-highsierra (1.62 MB, application/zip)
2019-05-04 21:01 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews214 for win-future (13.43 MB, application/zip)
2019-05-04 21:23 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.82 MB, application/zip)
2019-05-04 21:33 PDT, Build Bot
no flags Details
Patch (168.02 KB, patch)
2019-05-06 10:35 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (161.38 KB, patch)
2019-05-06 22:23 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (178.87 KB, patch)
2019-05-06 22:27 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (178.06 KB, patch)
2019-05-06 22:35 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-highsierra (17.22 MB, application/zip)
2019-05-06 23:34 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-highsierra (12.63 MB, application/zip)
2019-05-06 23:52 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-highsierra-wk2 (17.38 MB, application/zip)
2019-05-06 23:57 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (15.18 MB, application/zip)
2019-05-07 00:30 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews210 for win-future (22.65 MB, application/zip)
2019-05-07 09:19 PDT, Build Bot
no flags Details
Patch (181.93 KB, patch)
2019-05-07 10:57 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (8.16 MB, application/zip)
2019-05-07 12:57 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-highsierra (3.86 MB, application/zip)
2019-05-07 13:05 PDT, Build Bot
no flags Details
Patch (190.62 KB, patch)
2019-05-07 20:51 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (189.23 KB, patch)
2019-05-07 21:03 PDT, Simon Fraser (smfr)
koivisto: review+
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews112 for mac-highsierra (3.00 MB, application/zip)
2019-05-07 22:53 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (11.94 MB, application/zip)
2019-05-07 23:04 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews210 for win-future (13.62 MB, application/zip)
2019-05-08 01:44 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2019-05-03 09:23:18 PDT
An "inclusive" compositing layer is one that can paint a set of other (usually sibling) layers that would otherwise be composited for overlap. They will reduce the amount of backing store, and the need for scrolling tree nodes in some cases.
Comment 1 Radar WebKit Bug Importer 2019-05-03 09:23:50 PDT
<rdar://problem/50445998>
Comment 2 Simon Fraser (smfr) 2019-05-04 19:28:17 PDT
Created attachment 369080 [details]
Patch
Comment 3 Simon Fraser (smfr) 2019-05-04 19:35:09 PDT
*** Bug 197594 has been marked as a duplicate of this bug. ***
Comment 4 Build Bot 2019-05-04 20:37:06 PDT Comment hidden (obsolete)
Comment 5 Build Bot 2019-05-04 20:37:08 PDT Comment hidden (obsolete)
Comment 6 Build Bot 2019-05-04 20:48:53 PDT Comment hidden (obsolete)
Comment 7 Build Bot 2019-05-04 20:48:55 PDT Comment hidden (obsolete)
Comment 8 Build Bot 2019-05-04 21:01:53 PDT Comment hidden (obsolete)
Comment 9 Build Bot 2019-05-04 21:01:55 PDT Comment hidden (obsolete)
Comment 10 Build Bot 2019-05-04 21:23:31 PDT Comment hidden (obsolete)
Comment 11 Build Bot 2019-05-04 21:23:33 PDT Comment hidden (obsolete)
Comment 12 Build Bot 2019-05-04 21:33:27 PDT Comment hidden (obsolete)
Comment 13 Build Bot 2019-05-04 21:33:29 PDT Comment hidden (obsolete)
Comment 14 Simon Fraser (smfr) 2019-05-04 21:41:31 PDT
Comment on attachment 369080 [details]
Patch

Crashes are because traverseUnchangedSubtree() needs to do sharing stuff.
Comment 15 Antti Koivisto 2019-05-06 01:40:35 PDT
Comment on attachment 369080 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=369080&action=review

> Source/WebCore/rendering/RenderLayer.cpp:1817
> +    RenderLayer* repaintTarget;
> +    if (includeSelf == IncludeSelf && (repaintTarget = repaintTargetForLayer(*this)))
> +        return repaintTarget;

No reason to leave repaintTarget uninitialized.

> Source/WebCore/rendering/RenderLayer.h:818
> +    // If non,null, a non-ancestor composited layer that this layer paints into (it is sharing its backing store with this layer).

"non-null"

> Source/WebCore/rendering/RenderLayer.h:822
> +    RenderLayer* sharedLayer() const { return m_sharedLayer; }
> +    void setSharedLayer(RenderLayer* sharedLayer) { m_sharedLayer = sharedLayer; }
> +
> +    bool paintsIntoSharedLayer() const { return m_sharedLayer; }

"Shared layer" does not really describe the feature. I feel it would be clearer to express this in terms of "shared backing" in code. This might be just a matter of renaming and probably also switching to RenderLayerBacking ptrs here.

I wonder if there some sort of future refactoring here that actually makes RenderLayerBacking a shared object?

> Source/WebCore/rendering/RenderLayer.h:1268
> +    // If non,null, a non-ancestor composited layer that this layer paints into.

"non-null"

> Source/WebCore/rendering/RenderLayerBacking.cpp:264
> +    for (auto* layer : m_sharingLayers)
> +        layer->setSharedLayer(nullptr);

This kind of stuff is scary. Layer tree should use WeakPtrs.

It would be better to call clearSharingLayers() to avoid duplication.

> Source/WebCore/rendering/RenderLayerBacking.cpp:2678
> +        for (auto* layer : m_sharingLayers)
> +            paintOneLayer(*layer, sharingLayerPaintFlags);

I suppose region gathering code needs to replicate this logic too. Please file a bug.

> Source/WebCore/rendering/RenderLayerBacking.h:398
> +    // When this layer is a shared layer, sharing layers other than m_owningLayer are in this list, and paint into our backing store.

This is a confusing sentence.
Comment 16 Simon Fraser (smfr) 2019-05-06 09:32:34 PDT
(In reply to Antti Koivisto from comment #15)
> Comment on attachment 369080 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=369080&action=review
> 
> > Source/WebCore/rendering/RenderLayer.cpp:1817
> > +    RenderLayer* repaintTarget;
> > +    if (includeSelf == IncludeSelf && (repaintTarget = repaintTargetForLayer(*this)))
> > +        return repaintTarget;
> 
> No reason to leave repaintTarget uninitialized.
> 
> > Source/WebCore/rendering/RenderLayer.h:818
> > +    // If non,null, a non-ancestor composited layer that this layer paints into (it is sharing its backing store with this layer).
> 
> "non-null"
> 
> > Source/WebCore/rendering/RenderLayer.h:822
> > +    RenderLayer* sharedLayer() const { return m_sharedLayer; }
> > +    void setSharedLayer(RenderLayer* sharedLayer) { m_sharedLayer = sharedLayer; }
> > +
> > +    bool paintsIntoSharedLayer() const { return m_sharedLayer; }
> 
> "Shared layer" does not really describe the feature. I feel it would be
> clearer to express this in terms of "shared backing" in code. This might be
> just a matter of renaming and probably also switching to RenderLayerBacking
> ptrs here.

I agree that "shared" isn't too descriptive. Other implementations have called them "inclusive" which has clearer directionality ("inclusive, included") but Zalan didn't like that.

I don't want to use RLB pointers because I don't want to introduce a dependency on when backing actually gets created.

> I wonder if there some sort of future refactoring here that actually makes
> RenderLayerBacking a shared object?

Perhaps, though there is still a clear "primary" owner.

> > Source/WebCore/rendering/RenderLayer.h:1268
> > +    // If non,null, a non-ancestor composited layer that this layer paints into.
> 
> "non-null"
> 
> > Source/WebCore/rendering/RenderLayerBacking.cpp:264
> > +    for (auto* layer : m_sharingLayers)
> > +        layer->setSharedLayer(nullptr);
> 
> This kind of stuff is scary. Layer tree should use WeakPtrs.

Will fix.

> It would be better to call clearSharingLayers() to avoid duplication.
> 
> > Source/WebCore/rendering/RenderLayerBacking.cpp:2678
> > +        for (auto* layer : m_sharingLayers)
> > +            paintOneLayer(*layer, sharingLayerPaintFlags);
> 
> I suppose region gathering code needs to replicate this logic too. Please
> file a bug.

Good point.

> > Source/WebCore/rendering/RenderLayerBacking.h:398
> > +    // When this layer is a shared layer, sharing layers other than m_owningLayer are in this list, and paint into our backing store.
> 
> This is a confusing sentence.

Will fix.
Comment 17 Antti Koivisto 2019-05-06 09:47:32 PDT
Comment on attachment 369080 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=369080&action=review

> Source/WebCore/rendering/RenderLayer.cpp:4082
> +        paintFlags.add(PaintLayerTemporaryClipRects);

This disables clip caching right? Should we worry about performance?
Comment 18 Simon Fraser (smfr) 2019-05-06 10:35:02 PDT Comment hidden (obsolete)
Comment 19 Simon Fraser (smfr) 2019-05-06 22:23:04 PDT Comment hidden (obsolete)
Comment 20 Simon Fraser (smfr) 2019-05-06 22:27:10 PDT Comment hidden (obsolete)
Comment 21 Simon Fraser (smfr) 2019-05-06 22:35:40 PDT Comment hidden (obsolete)
Comment 22 Build Bot 2019-05-06 23:34:34 PDT Comment hidden (obsolete)
Comment 23 Build Bot 2019-05-06 23:34:37 PDT Comment hidden (obsolete)
Comment 24 Build Bot 2019-05-06 23:52:19 PDT Comment hidden (obsolete)
Comment 25 Build Bot 2019-05-06 23:52:23 PDT Comment hidden (obsolete)
Comment 26 Build Bot 2019-05-06 23:57:15 PDT Comment hidden (obsolete)
Comment 27 Build Bot 2019-05-06 23:57:18 PDT Comment hidden (obsolete)
Comment 28 Build Bot 2019-05-07 00:30:19 PDT Comment hidden (obsolete)
Comment 29 Build Bot 2019-05-07 00:30:25 PDT Comment hidden (obsolete)
Comment 30 Build Bot 2019-05-07 09:19:48 PDT Comment hidden (obsolete)
Comment 31 Build Bot 2019-05-07 09:19:53 PDT Comment hidden (obsolete)
Comment 32 Simon Fraser (smfr) 2019-05-07 10:57:17 PDT Comment hidden (obsolete)
Comment 33 Build Bot 2019-05-07 12:57:21 PDT Comment hidden (obsolete)
Comment 34 Build Bot 2019-05-07 12:57:24 PDT Comment hidden (obsolete)
Comment 35 Build Bot 2019-05-07 13:05:48 PDT Comment hidden (obsolete)
Comment 36 Build Bot 2019-05-07 13:05:50 PDT Comment hidden (obsolete)
Comment 37 Simon Fraser (smfr) 2019-05-07 20:51:51 PDT Comment hidden (obsolete)
Comment 38 Simon Fraser (smfr) 2019-05-07 21:03:24 PDT
Created attachment 369357 [details]
Patch
Comment 39 Build Bot 2019-05-07 22:53:40 PDT
Comment on attachment 369357 [details]
Patch

Attachment 369357 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12130330

New failing tests:
fast/hidpi/video-controls-in-hidpi.html
Comment 40 Build Bot 2019-05-07 22:53:42 PDT
Created attachment 369359 [details]
Archive of layout-test-results from ews112 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 41 Build Bot 2019-05-07 23:04:36 PDT
Comment on attachment 369357 [details]
Patch

Attachment 369357 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/12130361

New failing tests:
fast/scrolling/ios/overflow-scroll-overlap-4.html
Comment 42 Build Bot 2019-05-07 23:04:39 PDT
Created attachment 369360 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.14.4
Comment 43 Antti Koivisto 2019-05-08 00:00:56 PDT
> New failing tests:
> fast/scrolling/ios/overflow-scroll-overlap-4.html

This is failing because of the missing region code.
Comment 44 Antti Koivisto 2019-05-08 00:16:55 PDT
Comment on attachment 369357 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=369357&action=review

Looks fine. Fixing updateEventRegion shouldn't be difficult either (same thing as paintIntoLayer) but it can be done separately if you prefer.

> Source/WebCore/rendering/RenderLayer.cpp:1774
> +//    ASSERT_IMPLIES(backingProvider, !isComposited());

Commented-out code.

> Source/WebCore/rendering/RenderLayerBacking.cpp:679
> +        if (!layerWeakPtr) {
> +            ASSERT_NOT_REACHED();
> +            continue;
> +        }

This seems overly defensive. We should just crash with nullptr if the logic is supposed to guarantee that these are non-null (here and elsewhere).
Comment 45 Build Bot 2019-05-08 01:44:15 PDT
Comment on attachment 369357 [details]
Patch

Attachment 369357 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12131207

New failing tests:
http/tests/css/filters-on-iframes.html
Comment 46 Build Bot 2019-05-08 01:44:22 PDT
Created attachment 369364 [details]
Archive of layout-test-results from ews210 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews210  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 47 Simon Fraser (smfr) 2019-05-08 10:24:45 PDT
https://trac.webkit.org/r245058
Comment 48 Alexey Proskuryakov 2019-05-08 17:42:24 PDT
> webkit.org/b/197695 [ Debug ] fast/hidpi/video-controls-in-hidpi.html [ Skip ]

This patch skipped an existing test. How is this OK?
Comment 49 Ryan Haddad 2019-05-08 18:20:12 PDT
Reverted r245058 for reason:

Causes crashes under ASan / GuardMalloc

Committed r245085: <https://trac.webkit.org/changeset/245085>
Comment 50 Simon Fraser (smfr) 2019-05-09 22:20:49 PDT
Re-landed with fixes in https://trac.webkit.org/changeset/245170/webkit
Comment 51 Antti Koivisto 2019-05-10 05:20:59 PDT
This hits debug asserts on many sites (say, nytimes.com)

void RenderLayer::paintLayerContents(GraphicsContext& context, const LayerPaintingInfo& paintingInfo, OptionSet<PaintLayerFlag> paintFlags)
{
    ASSERT(isSelfPaintingLayer() || hasSelfPaintingLayerDescendant());


(lldb) bt 10
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xbbadbeef)
  * frame #0: 0x000000035a69285e JavaScriptCore`::WTFCrash() at Assertions.cpp:305:35
    frame #1: 0x000000035ee1ce2b WebCore`WTFCrashWithInfo((null)=4368, (null)="rendering/RenderLayer.cpp", (null)="void WebCore::RenderLayer::paintLayerContents(WebCore::GraphicsContext &, const WebCore::RenderLayer::LayerPaintingInfo &, OptionSet<WebCore::RenderLayer::PaintLayerFlag>)", (null)=2340) at Assertions.h:568:5
    frame #2: 0x00000003622b75f6 WebCore`WebCore::RenderLayer::paintLayerContents(this=0x00000003946d8c18, context=0x00007ffee42dc358, paintingInfo=0x00007ffee42dbde0, paintFlags=(m_storage = 96)) at RenderLayer.cpp:4368:5
    frame #3: 0x00000003622d81ac WebCore`WebCore::RenderLayerBacking::paintIntoLayer(this=0x00007ffee42dbec8, layer=0x00000003946d8c18, paintFlags=(m_storage = 96))::$_9::operator()(WebCore::RenderLayer&, WTF::OptionSet<WebCore::RenderLayer::PaintLayerFlag>) const at RenderLayerBacking.cpp:2674:15
    frame #4: 0x00000003622d7f70 WebCore`WebCore::RenderLayerBacking::paintIntoLayer(this=0x000000039460fa20, graphicsLayer=0x000000039473d000, context=0x00007ffee42dc358, paintDirtyRect=0x00007ffee42dc0d8, paintBehavior=(m_storage = 2048), paintingPhase='\x03') at RenderLayerBacking.cpp:2701:13
    frame #5: 0x00000003622d869d WebCore`WebCore::RenderLayerBacking::paintContents(this=0x000000039460fa20, graphicsLayer=0x000000039473d000, context=0x00007ffee42dc358, paintingPhase='\x03', clip=0x00007ffee42dc160, layerPaintBehavior=2) at RenderLayerBacking.cpp:2744:9
    frame #6: 0x0000000361de23b7 WebCore`WebCore::GraphicsLayer::paintGraphicsLayerContents(this=0x000000039473d000, context=0x00007ffee42dc358, clip=0x00000003947f49b8, layerPaintBehavior=2) at GraphicsLayer.cpp:501:14
    frame #7: 0x0000000361e5a8ad WebCore`WebCore::GraphicsLayerCA::platformCALayerPaintContents(this=0x000000039473d000, (null)=0x0000000392633000, context=0x00007ffee42dc358, clip=0x00000003947f49b8, layerPaintBehavior=2) at GraphicsLayerCA.cpp:1678:5
    frame #8: 0x000000035f4df4c3 WebCore`WebCore::PlatformCALayer::drawLayerContents(context=0x00007fc3dced2ba0, platformCALayer=0x0000000392633000, dirtyRects=0x00000003947f49a8, layerPaintBehavior=2) at PlatformCALayerCocoa.mm:1198:28
    frame #9: 0x000000010c5056f0 WebKit`WebKit::RemoteLayerBackingStore::drawInContext(this=0x00000003947f4958, context=0x000000039ff97840, backImage=0x0000000000000000) at RemoteLayerBackingStore.mm:355:9
Comment 52 Simon Fraser (smfr) 2019-05-10 08:52:22 PDT
Filed bug 197776 on that.
Comment 53 Michael Catanzaro 2019-05-10 10:06:57 PDT
[885/2246] Building CXX object Source/WebCore/CMakeFiles/...es/WebCore/unified-sources/UnifiedSource-043dd90b-8.cpp.o
In file included from DerivedSources/WebCore/unified-sources/UnifiedSource-043dd90b-8.cpp:5:
../../Source/WebCore/rendering/RenderLayer.cpp: In member function ‘WTF::Ref<WebCore::ClipRects> WebCore::RenderLayer::updateClipRects(const WebCore::RenderLayer::ClipRectsContext&)’:
../../Source/WebCore/rendering/RenderLayer.cpp:5503:15: warning: unused variable ‘parentLayer’ [-Wunused-variable]
 5503 |     if (auto* parentLayer = (clipRectsContext.rootLayer != this ? parent() : nullptr))
      |               ^~~~~~~~~~~

This condition here:

    RefPtr<ClipRects> parentClipRects;
    // For transformed layers, the root layer was shifted to be us, so there is no need to
    // examine the parent. We want to cache clip rects with us as the root.
    if (auto* parentLayer = (clipRectsContext.rootLayer != this ? parent() : nullptr))
        parentClipRects = this->parentClipRects(clipRectsContext);

Should now be simplified to:

    RefPtr<ClipRects> parentClipRects;
    // For transformed layers, the root layer was shifted to be us, so there is no need to
    // examine the parent. We want to cache clip rects with us as the root.
    if (clipRectsContext.rootLayer != this)
        parentClipRects = this->parentClipRects(clipRectsContext);
Comment 54 Michael Catanzaro 2019-05-10 10:09:41 PDT
Er no, that's a disaster think-o, sorry. Should be simplified to:

if (clipRectsContext.rootLayer != this && parent())
Comment 55 Michael Catanzaro 2019-05-10 10:25:45 PDT
Will fix in: bug #197785. (Assuming the new logic is correct and only needs to be simplified.)