Bug 96087

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:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

78862, 96625, 97801, 100524, 102678

Bug Blocks:

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Sami Kyöstilä

Reported 2012-09-07 02:30:51 PDT

Currently composited scrolling layers (i.e., -webkit-overflow-scrolling: touch) are fully repainted every time the scroll offset changes. Also, painting is limited to the area of the overflow clip, so scrolling such a layer off the main thread will flicker because the layer has no valid content outside the clip.

Attachments
Patch (31.63 KB, patch) 2012-09-07 10:54 PDT, Sami Kyöstilä	no flags	Details Formatted Diff Diff
Patch (27.07 KB, patch) 2012-09-10 10:21 PDT, Sami Kyöstilä	no flags	Details Formatted Diff Diff
Patch (27.09 KB, patch) 2012-09-11 02:06 PDT, Sami Kyöstilä	no flags	Details Formatted Diff Diff
Patch (31.12 KB, patch) 2012-09-12 03:01 PDT, Sami Kyöstilä	no flags	Details Formatted Diff Diff
Patch (38.47 KB, patch) 2012-09-27 03:17 PDT, Sami Kyöstilä	no flags	Details Formatted Diff Diff
Patch (10.44 KB, patch) 2012-10-26 08:57 PDT, Sami Kyöstilä	no flags	Details Formatted Diff Diff
Patch (11.30 KB, patch) 2012-11-06 03:08 PST, Sami Kyöstilä	no flags	Details Formatted Diff Diff
Patch (11.18 KB, patch) 2012-11-14 08:40 PST, Sami Kyöstilä	no flags	Details Formatted Diff Diff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Sami Kyöstilä

Comment 1 2012-09-07 10:54:06 PDT

Created attachment 162817 [details] Patch Work-in-progress patch; tests missing.

Antonio Gomes

Comment 2 2012-09-08 12:12:45 PDT

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.

Satish Sampath

Comment 3 2012-09-09 21:08:27 PDT

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

Sami Kyöstilä

Comment 4 2012-09-10 02:56:43 PDT

(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.

Sami Kyöstilä

Comment 5 2012-09-10 02:59:00 PDT

(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.

Sami Kyöstilä

Comment 6 2012-09-10 10:21:27 PDT

Created attachment 163159 [details] Patch

Sami Kyöstilä

Comment 7 2012-09-10 10:26:41 PDT

Here's what I hope is a complete patch with tests.

Build Bot

Comment 8 2012-09-10 10:46:38 PDT

Comment on attachment 163159 [details] Patch Attachment 163159 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13810286

Build Bot

Comment 9 2012-09-10 10:57:01 PDT

Comment on attachment 163159 [details] Patch Attachment 163159 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13804642

Gyuyoung Kim

Comment 10 2012-09-10 11:03:42 PDT

Comment on attachment 163159 [details] Patch Attachment 163159 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13802749

Sami Kyöstilä

Comment 11 2012-09-11 02:06:28 PDT

Created attachment 163307 [details] Patch Unbreak the build.

Build Bot

Comment 12 2012-09-11 02:36:33 PDT

Comment on attachment 163307 [details] Patch Attachment 163307 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13824177

kov's GTK+ EWS bot

Comment 13 2012-09-11 02:43:33 PDT

Comment on attachment 163307 [details] Patch Attachment 163307 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/13818299

Build Bot

Comment 14 2012-09-11 03:20:47 PDT

Comment on attachment 163307 [details] Patch Attachment 163307 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13810586

Sami Kyöstilä

Comment 15 2012-09-12 03:01:00 PDT

Created attachment 163569 [details] Patch Updated def files.

Simon Fraser (smfr)

Comment 16 2012-09-12 15:44:05 PDT

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.

Sami Kyöstilä

Comment 17 2012-09-13 03:18:47 PDT

(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.

Sami Kyöstilä

Comment 18 2012-09-27 03:17:54 PDT

Created attachment 165964 [details] Patch Made TestRunner.display() to (optionally) work with composited layers.

WebKit Review Bot

Comment 19 2012-09-27 03:21:07 PDT

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.

Build Bot

Comment 20 2012-09-27 05:13:05 PDT

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

Simon Fraser (smfr)

Comment 21 2012-09-27 09:02:05 PDT

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.

Sami Kyostila

Comment 22 2012-09-27 09:08:15 PDT

(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.

Simon Fraser (smfr)

Comment 23 2012-09-27 09:43:42 PDT

You could store a list of repaint rectangles in local layer coordinates on RenderLayerBacking.

Antonio Gomes

Comment 24 2012-10-25 07:35:25 PDT

> > + > > + 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).

Antonio Gomes

Comment 25 2012-10-25 07:53:14 PDT

(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

Sami Kyöstilä

Comment 26 2012-10-25 08:20:37 PDT

That is interesting. Would you happen to have a repro case?

Sami Kyöstilä

Comment 27 2012-10-26 08:57:32 PDT

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?

Antonio Gomes

Comment 28 2012-10-26 09:33:49 PDT

(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

Sami Kyöstilä

Comment 29 2012-11-06 03:08:53 PST

Created attachment 172537 [details] Patch Rebased now that 97801 landed.

Antonio Gomes

Comment 30 2012-11-06 06:38:52 PST

Comment on attachment 172537 [details] Patch Looks good to me. Will defer to smfr a final word though...

Sami Kyöstilä

Comment 31 2012-11-14 08:40:02 PST

Created attachment 174170 [details] Patch Now that 100524 landed this one should be good to go. Simon, would you mind taking a look?

WebKit Review Bot

Comment 32 2012-11-14 10:52:24 PST

Comment on attachment 174170 [details] Patch Clearing flags on attachment: 174170 Committed r134628: <http://trac.webkit.org/changeset/134628>

WebKit Review Bot

Comment 33 2012-11-14 10:52:30 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.