Bug 96087 – Optimize painting of composited scrolling layers

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Sami Kyöstilä]

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.

[Sami Kyöstilä]

Created attachment 162817 [details]
Patch

Work-in-progress patch; tests missing.

[Antonio Gomes]

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 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ä]

(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ä]

(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ä]

Created attachment 163159 [details]
Patch

[Sami Kyöstilä]

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

[Build Bot]

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 on attachment 163159 [details]
Patch

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

[Gyuyoung Kim]

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ä]

Created attachment 163307 [details]
Patch

Unbreak the build.

[Build Bot]

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 on attachment 163307 [details]
Patch

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

[Build Bot]

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ä]

Created attachment 163569 [details]
Patch

Updated def files.

[Simon Fraser (smfr)]

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ä]

(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ä]

Created attachment 165964 [details]
Patch

Made TestRunner.display() to (optionally) work with composited layers.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Build Bot]

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 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]

(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)]

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

[Antonio Gomes]

> > +
> > +        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]

(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ä]

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

[Sami Kyöstilä]

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]

(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ä]

Created attachment 172537 [details]
Patch

Rebased now that 97801 landed.

[Antonio Gomes]

Comment on attachment 172537 [details]
Patch

Looks good to me. Will defer to smfr a final word though...

[Sami Kyöstilä]

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 on attachment 174170 [details]
Patch

Clearing flags on attachment: 174170

Committed r134628: <http://trac.webkit.org/changeset/134628>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.