Summary: | Optimize painting of composited scrolling layers | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sami Kyöstilä <skyostil> | ||||||||||||||||||
Component: | Layout and Rendering | Assignee: | Sami Kyöstilä <skyostil> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | abarth, dglazkov, eric, fishd, gtk-ews, gustavo, jamesr, philn, simon.fraser, skyostil, tkent+wkapi, tonikitoo, webkit.review.bot, xan.lopez | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
Bug Depends on: | 78862, 96625, 97801, 100524, 102678 | ||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||
Attachments: |
|
Description
Sami Kyöstilä
2012-09-07 02:30:51 PDT
Created attachment 162817 [details]
Patch
Work-in-progress patch; tests missing.
Comment on attachment 162817 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162817&action=review > Source/WebCore/ChangeLog:19 > + We also rename the configuration directive ENABLE_OVERFLOW_SCROLLING to > + ENABLE_ACCELERATED_OVERFLOW_SCROLLING to better reflect its purpose. if we are to go for it, please do it on its own patch. Comment on attachment 162817 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162817&action=review > Source/WebCore/ChangeLog:14 > + scrolling layer. This is done to increase the changes there is valid layer changes -> chances (In reply to comment #2) > > + We also rename the configuration directive ENABLE_OVERFLOW_SCROLLING to > > + ENABLE_ACCELERATED_OVERFLOW_SCROLLING to better reflect its purpose. > > if we are to go for it, please do it on its own patch. Right you are: https://bugs.webkit.org/show_bug.cgi?id=96251. (In reply to comment #3) > changes -> chances Done. Also trimmed the CC list a bit now that we're not touching the various build files. Created attachment 163159 [details]
Patch
Here's what I hope is a complete patch with tests. Comment on attachment 163159 [details] Patch Attachment 163159 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13810286 Comment on attachment 163159 [details] Patch Attachment 163159 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13804642 Comment on attachment 163159 [details] Patch Attachment 163159 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13802749 Created attachment 163307 [details]
Patch
Unbreak the build.
Comment on attachment 163307 [details] Patch Attachment 163307 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13824177 Comment on attachment 163307 [details] Patch Attachment 163307 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/13818299 Comment on attachment 163307 [details] Patch Attachment 163307 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13810586 Created attachment 163569 [details]
Patch
Updated def files.
Comment on attachment 163569 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163569&action=review > Source/WebCore/ChangeLog:21 > + A new internal API for setting a layer debug paint color is introduced to > + allow testing the changes. All further paint operations to the associated > + RenderLayerBacking are marked with the given color. The tests use this feature > + to detect redundant and missing repaints. I'd prefer that we address this by making repaint testing work for composited layers, rather than doing something that requires matching pixel output. > Source/WebCore/rendering/RenderBox.cpp:663 > +void RenderBox::applyCachedClipAndScrollOffsetForRepaint(LayoutRect& paintRect) const > +{ > + paintRect.move(-scrolledContentOffset()); // For overflow:auto/scroll/hidden. > + > + // Do not clip scroll layer contents to reduce the number of repaints while scrolling. > + if (hasOverflowClip() && hasLayer() && layer()->usesCompositedScrolling()) > + return; > + > + // height() is inaccurate if we're in the middle of a layout of this RenderBox, so use the > + // layer's size instead. Even if the layer's size is wrong, the layer itself will repaint > + // anyway if its size does change. > + LayoutRect clipRect(LayoutPoint(), cachedSizeForOverflowClip()); > + paintRect = intersection(paintRect, clipRect); > +} Please do this refactor in its own patch. (In reply to comment #16) > I'd prefer that we address this by making repaint testing work for composited layers, rather than doing something that requires matching pixel output. Okay, I'll have to study how repaint tests work currently. I was under the impression that they're also based on exact pixel output, or maybe I misunderstood your comment. > > Source/WebCore/rendering/RenderBox.cpp:663 > > +void RenderBox::applyCachedClipAndScrollOffsetForRepaint(LayoutRect& paintRect) const > Please do this refactor in its own patch. Sure: https://bugs.webkit.org/show_bug.cgi?id=96625. Created attachment 165964 [details]
Patch
Made TestRunner.display() to (optionally) work with composited layers.
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI. Comment on attachment 165964 [details] Patch Attachment 165964 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14050256 New failing tests: compositing/overflow/scrolling-content-updating.html compositing/overflow/scrolling-content-prepainting.html Comment on attachment 165964 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165964&action=review > Source/WebCore/ChangeLog:22 > + This patch enables two optimization for composited scrolling layers: first, > + when the layer scroll offset changes, do not mark the entire layer as needing > + display, because the compositor will account for the corresponding layout > + change simply by translating the layer to the correct position. > + > + Second, overflow clipping is disabled when painting the contents of a > + composited scrolling layer. This is done to increase the chances there is > + valid layer content for the compositor to draw while scrolling the layer off > + the main thread. > + > + In order to test these changes, the existing TestRunner display() method is > + extended to also work with composited layers. This is done by painting all > + composited layers with a dark transparent mask. Any further repaints to the > + layer contents will not have this mask, so they will appear highlighted compared > + to the old contents. You should break this into three patches. Do the last one first. (In reply to comment #21) > > + In order to test these changes, the existing TestRunner display() method is > > + extended to also work with composited layers. This is done by painting all > > + composited layers with a dark transparent mask. Any further repaints to the > > + layer contents will not have this mask, so they will appear highlighted compared > > + to the old contents. > > You should break this into three patches. Do the last one first. Okay, I'll split it up. Any comment on how the repaint tracking is done? It's a bit different to how FrameView accumulates a list of paint rects, but doing the same with layers gets a little messy when transforms are involved. You could store a list of repaint rectangles in local layer coordinates on RenderLayerBacking. > > +
> > + Second, overflow clipping is disabled when painting the contents of a
> > + composited scrolling layer. This is done to increase the chances there is
> > + valid layer content for the compositor to draw while scrolling the layer off
> > + the main thread.
> > +
> > +
I've seen some repaint regressions related to this change when we change the clip rectangular of the overflow element dynamically (e.g. When we rotate a device).
(In reply to comment #24) > > > + > > > + Second, overflow clipping is disabled when painting the contents of a > > > + composited scrolling layer. This is done to increase the chances there is > > > + valid layer content for the compositor to draw while scrolling the layer off > > > + the main thread. > > > + > > > + > > I've seen some repaint regressions related to this change when we change the clip rectangular of the overflow element dynamically (e.g. When we rotate a device). Err S/rectangular/rect That is interesting. Would you happen to have a repro case? Created attachment 170940 [details] Patch Rebased against bug 100524. Antonio: I wasn't able to trigger the repaint regression based on your description. Could you elaborate? (In reply to comment #27) > Created an attachment (id=170940) [details] > Patch > > Rebased against bug 100524. > > Antonio: I wasn't able to trigger the repaint regression based on your description. Could you elaborate? Will try to get a reproducible test case Created attachment 172537 [details]
Patch
Rebased now that 97801 landed.
Comment on attachment 172537 [details]
Patch
Looks good to me. Will defer to smfr a final word though...
Created attachment 174170 [details]
Patch
Now that 100524 landed this one should be good to go. Simon, would you mind taking a look?
Comment on attachment 174170 [details] Patch Clearing flags on attachment: 174170 Committed r134628: <http://trac.webkit.org/changeset/134628> All reviewed patches have been landed. Closing bug. |