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.
<rdar://problem/50445998>
Created attachment 369080 [details] Patch
*** Bug 197594 has been marked as a duplicate of this bug. ***
Comment on attachment 369080 [details] Patch Attachment 369080 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12102569 New failing tests: media/media-continues-playing-after-replace-source.html media/media-ended.html
Created attachment 369082 [details] Archive of layout-test-results from ews101 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 369080 [details] Patch Attachment 369080 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12102575 New failing tests: media/media-continues-playing-after-replace-source.html media/media-ended.html
Created attachment 369083 [details] Archive of layout-test-results from ews106 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 369080 [details] Patch Attachment 369080 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12102571 Number of test failures exceeded the failure limit.
Created attachment 369085 [details] Archive of layout-test-results from ews114 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 369080 [details] Patch Attachment 369080 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12102657 New failing tests: security/contentSecurityPolicy/video-with-file-url-allowed-by-media-src-star.html
Created attachment 369087 [details] Archive of layout-test-results from ews214 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews214 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment on attachment 369080 [details] Patch Attachment 369080 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/12102636 New failing tests: fast/scrolling/ios/overflow-scroll-overlap-4.html media/media-continues-playing-after-replace-source.html media/track/track-cues-enter-exit.html media/media-ended.html media/track/track-cues-cuechange.html
Created attachment 369088 [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 on attachment 369080 [details] Patch Crashes are because traverseUnchangedSubtree() needs to do sharing stuff.
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.
(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 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?
Created attachment 369134 [details] Patch
Created attachment 369231 [details] Patch
Created attachment 369232 [details] Patch
Created attachment 369234 [details] Patch
Comment on attachment 369234 [details] Patch Attachment 369234 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12120290 New failing tests: compositing/shared-layers/partial-compositing-update.html media/modern-media-controls/volume-support/volume-support-drag-to-mute.html media/modern-media-controls/volume-support/volume-support-drag.html media/modern-media-controls/scrubber-support/scrubber-support-click.html media/modern-media-controls/fullscreen-support/fullscreen-support-press.html compositing/shared-layers/partial-compositing-update2.html media/modern-media-controls/playback-support/playback-support-button-click.html media/audio-concurrent-supported.html media/modern-media-controls/placard-support/placard-support-pip.html media/modern-media-controls/mute-support/mute-support-press-on-button.html media/modern-media-controls/scrubber-support/scrubber-support-drag.html media/modern-media-controls/volume-support/volume-support-click.html
Created attachment 369243 [details] Archive of layout-test-results from ews101 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 369234 [details] Patch Attachment 369234 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12120307 Number of test failures exceeded the failure limit.
Created attachment 369245 [details] Archive of layout-test-results from ews117 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 369234 [details] Patch Attachment 369234 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12120358 New failing tests: compositing/shared-layers/partial-compositing-update.html media/modern-media-controls/volume-support/volume-support-drag-to-mute.html media/modern-media-controls/volume-support/volume-support-drag.html media/audio-concurrent-supported.html media/modern-media-controls/fullscreen-support/fullscreen-support-press.html media/audio-mpeg-supported.html compositing/shared-layers/partial-compositing-update2.html media/modern-media-controls/playback-support/playback-support-button-click.html media/modern-media-controls/scrubber-support/scrubber-support-click.html media/media-ended.html media/media-continues-playing-after-replace-source.html media/modern-media-controls/mute-support/mute-support-press-on-button.html media/modern-media-controls/scrubber-support/scrubber-support-drag.html media/modern-media-controls/volume-support/volume-support-click.html media/media-ended-fired-once.html
Created attachment 369246 [details] Archive of layout-test-results from ews107 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 369234 [details] Patch Attachment 369234 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/12120398 New failing tests: compositing/shared-layers/partial-compositing-update.html media/audio-mpeg4-supported.html fast/scrolling/ios/overflow-scroll-overlap-4.html media/audio-concurrent-supported.html media/audio-playback-restriction-removed-muted.html compositing/shared-layers/partial-compositing-update2.html compositing/shared-layers/overflow-scroll/absolute-in-stacking-relative-in-scroller.html media/media-continues-playing-after-replace-source.html media/media-ended-fired-once.html
Created attachment 369251 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.14.4
Comment on attachment 369234 [details] Patch Attachment 369234 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12123996 New failing tests: svg/repaint/remove-border-property-on-root.html
Created attachment 369294 [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
Created attachment 369301 [details] Patch
Comment on attachment 369301 [details] Patch Attachment 369301 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/12125821 New failing tests: fast/scrolling/ios/overflow-scroll-overlap-4.html compositing/shared-layers/partial-compositing-update.html compositing/shared-layers/overflow-scroll/absolute-in-stacking-relative-in-scroller.html compositing/shared-layers/partial-compositing-update2.html
Created attachment 369316 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.14.4
Comment on attachment 369301 [details] Patch Attachment 369301 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12125836 New failing tests: compositing/shared-layers/partial-compositing-update2.html compositing/shared-layers/partial-compositing-update.html media/modern-media-controls/volume-support/volume-support-drag-to-mute.html media/modern-media-controls/volume-support/volume-support-drag.html media/modern-media-controls/scrubber-support/scrubber-support-click.html media/track/track-cue-rendering-snap-to-lines-not-set.html fast/hidpi/video-controls-in-hidpi.html media/modern-media-controls/playback-support/playback-support-button-click.html media/video-zoom-controls.html media/modern-media-controls/scrubber-support/scrubber-support-media-api.html media/track/track-cue-rendering-vertical.html media/modern-media-controls/mute-support/mute-support-press-on-button.html media/modern-media-controls/scrubber-support/scrubber-support-drag.html media/modern-media-controls/volume-support/volume-support-click.html media/video-trackmenu-selection.html
Created attachment 369320 [details] Archive of layout-test-results from ews115 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 369355 [details] Patch
Created attachment 369357 [details] Patch
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
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 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
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
> New failing tests: > fast/scrolling/ios/overflow-scroll-overlap-4.html This is failing because of the missing region code.
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 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
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
https://trac.webkit.org/r245058
> webkit.org/b/197695 [ Debug ] fast/hidpi/video-controls-in-hidpi.html [ Skip ] This patch skipped an existing test. How is this OK?
Reverted r245058 for reason: Causes crashes under ASan / GuardMalloc Committed r245085: <https://trac.webkit.org/changeset/245085>
Re-landed with fixes in https://trac.webkit.org/changeset/245170/webkit
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
Filed bug 197776 on that.
[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);
Er no, that's a disaster think-o, sorry. Should be simplified to: if (clipRectsContext.rootLayer != this && parent())
Will fix in: bug #197785. (Assuming the new logic is correct and only needs to be simplified.)