Bug 96087 - Optimize painting of composited scrolling layers
Summary: Optimize painting of composited scrolling layers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sami Kyöstilä
URL:
Keywords:
Depends on: 78862 96625 97801 100524 102678
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-07 02:30 PDT by Sami Kyöstilä
Modified: 2012-11-19 05:02 PST (History)
14 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Sami Kyöstilä 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.
Comment 1 Sami Kyöstilä 2012-09-07 10:54:06 PDT
Created attachment 162817 [details]
Patch

Work-in-progress patch; tests missing.
Comment 2 Antonio Gomes 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.
Comment 3 Satish Sampath 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
Comment 4 Sami Kyöstilä 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.
Comment 5 Sami Kyöstilä 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.
Comment 6 Sami Kyöstilä 2012-09-10 10:21:27 PDT
Created attachment 163159 [details]
Patch
Comment 7 Sami Kyöstilä 2012-09-10 10:26:41 PDT
Here's what I hope is a complete patch with tests.
Comment 8 Build Bot 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
Comment 9 Build Bot 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
Comment 10 Gyuyoung Kim 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
Comment 11 Sami Kyöstilä 2012-09-11 02:06:28 PDT
Created attachment 163307 [details]
Patch

Unbreak the build.
Comment 12 Build Bot 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
Comment 13 kov's GTK+ EWS bot 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
Comment 14 Build Bot 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
Comment 15 Sami Kyöstilä 2012-09-12 03:01:00 PDT
Created attachment 163569 [details]
Patch

Updated def files.
Comment 16 Simon Fraser (smfr) 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.
Comment 17 Sami Kyöstilä 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.
Comment 18 Sami Kyöstilä 2012-09-27 03:17:54 PDT
Created attachment 165964 [details]
Patch

Made TestRunner.display() to (optionally) work with composited layers.
Comment 19 WebKit Review Bot 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.
Comment 20 Build Bot 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
Comment 21 Simon Fraser (smfr) 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.
Comment 22 Sami Kyostila 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.
Comment 23 Simon Fraser (smfr) 2012-09-27 09:43:42 PDT
You could store a list of repaint rectangles in local layer coordinates on RenderLayerBacking.
Comment 24 Antonio Gomes 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).
Comment 25 Antonio Gomes 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
Comment 26 Sami Kyöstilä 2012-10-25 08:20:37 PDT
That is interesting. Would you happen to have a repro case?
Comment 27 Sami Kyöstilä 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?
Comment 28 Antonio Gomes 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
Comment 29 Sami Kyöstilä 2012-11-06 03:08:53 PST
Created attachment 172537 [details]
Patch

Rebased now that 97801 landed.
Comment 30 Antonio Gomes 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...
Comment 31 Sami Kyöstilä 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?
Comment 32 WebKit Review Bot 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>
Comment 33 WebKit Review Bot 2012-11-14 10:52:30 PST
All reviewed patches have been landed.  Closing bug.