Bug 50418 – Optimize computation of visual overflow when opacity changes

• Status: NEW
• Component: Layout and Rendering

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/47805 added this FIXME comment:

    if ((rareNonInheritedData->opacity == 1 && other->rareNonInheritedData->opacity < 1) ||
        (rareNonInheritedData->opacity < 1 && other->rareNonInheritedData->opacity == 1)) {
        // FIXME: We should add an optimized form of layout that just recomputes visual overflow.
        return StyleDifferenceLayout;
    }

This causes extra repaints of compositing layers, because layout diffs result in more repainting. This has performance implications on some devices. The comment suggests that this could be optimized.

[Simon Fraser (smfr)]

I don't understand the comment; opacity doesn't affect visual overflow, but it does affect whether RenderLayers get created.

[Simon Fraser (smfr)]

dhyatt: visual overflow doesn't cross layers
dhyatt: so if layers come and go you have to recompute it
dhyatt: opacity layers are self-painting
dhyatt: so if they go away, their visual overflow now has to be part of some parent layer

[Simon Fraser (smfr)]

dhyatt: a more general way of looking at it is just "in the absence of overflow, would this property require a relayout"
dhyatt: smfr: at the time i did this patch
dhyatt: smfr: you could probably do a simplified layout
dhyatt: smfr: in these cases
dhyatt: but you'd need a new hint
dhyatt: StyleDifferenceSimplifiedLayout
smfr: dhyatt: is this the same as positioned objects layout, or another kind of simplified layout?
dhyatt: you can do normal flow simplified layout too

[Bruno Abinader (history only)]

Created attachment 194363 [details]
Proposed patch

This patch adds an option to check if a scheduled scripted animation is ongoing, and if so, we avoid forcing a layout when opacity has toggled between opaque and transparent. This solves the issue on extra repaints of compositing layers.

[Build Bot]

Comment on attachment 194363 [details]
Proposed patch

Attachment 194363 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17294048

[Build Bot]

Comment on attachment 194363 [details]
Proposed patch

Attachment 194363 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17112654

[Bruno Abinader (history only)]

Created attachment 194536 [details]
Proposed patch

Fixed a typo (catch by mac* EWS bots).

[Bruno Abinader (history only)]

Comment on attachment 194536 [details]
Proposed patch

Removing review flag, I've noticed that Document::isScriptedAnimationScheduled() always returns false when called from RenderObject (as we're handling the computed style generated from the previously-scheduled animation step).

[Bruno Abinader (history only)]

Created attachment 194614 [details]
Proposed patch

Now checking if there are ongoing scripted animations by looking for fired callbacks.

[James Robinson]

Comment on attachment 194614 [details]
Proposed patch

Why would needing a layout or not depend on if we're in a scripted animation callback?

[Bruno Abinader (history only)]

(In reply to comment #10)
> (From update of attachment 194614 [details])
> Why would needing a layout or not depend on if we're in a scripted animation callback?

Rationale is that when animating, we can assume that the layout was previously computed before the animation has begun, thus causing no harm. Pure CSS animations (using keyframes) goes under another code path, which also does not cause a full layout from toggling between opaque and transparent.

[Antonio Gomes]

Comment on attachment 194614 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194614&action=review

Some comments, though it definitively needs smfr or hyatt's review.

> Source/WebCore/ChangeLog:13
> +        Patch covered by existing tests.

It is important to list the tests. 

Is that a test that gets fixed by this? Not possible to add a test?

> Source/WebCore/dom/Document.cpp:4963
> +bool Document::isScriptedAnimationActive()

const

> Source/WebCore/dom/ScriptedAnimationController.cpp:85
> +bool ScriptedAnimationController::isActive()

const

> Source/WebCore/rendering/RenderObject.cpp:1732
> +        // FIXME: Force a layout if not on a scheduled scripted animation (see
> +        // RenderStyle::diff() for details.

I do not like this "see RenderStyle::diff for details" part here. It is already hard to keep comments up to date. Keep comments referring other parts of the code it even fragiler, as the other part can change and it won't get updated here.

[Simon Fraser (smfr)]

Comment on attachment 194614 [details]
Proposed patch

I don't think looking for running animations is appropriate.

[Bruno Abinader (history only)]

(In reply to comment #13)
> (From update of attachment 194614 [details])
> I don't think looking for running animations is appropriate.

I understand the "intrusiveness" level this patch proposes is quite high, however I'm pretty sure those extra texture uploads are not necessary. Do you have another approach suggestion in mind? I thought about instead of having this check when selecting which style diff to use, we could avoid the texture upload by not marking the backing store tiles dirty depending if we know they already have their own RenderLayer.

[James Robinson]

If you care about layout or style updates can't you query the layout/style systems for the information you need?

[Bruno Abinader (history only)]

Created attachment 195426 [details]
Proposed patch

Fixed implementation by checking if the RenderObject already has its own layer and a layout test to verify that we're not uploading redundant textures. Includes added window.internals* helper functions to obtain repaint count data. I'll wait for EWS results to request review flag.

[Simon Fraser (smfr)]

Comment on attachment 195426 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195426&action=review

> Source/WebCore/page/Frame.h:139
> +        String layerTreeRepaintCountAsText() const;

You should not add a new function for this. Just add a new flag to LayerTreeFlags, and optionally dump repaint counts as part of the layer tree dump.

> Source/WebCore/rendering/RenderObject.cpp:1733
> +        if (contextSensitiveProperties & ContextSensitivePropertyOpacityToggle && !hasLayer())
> +            diff = StyleDifferenceLayout;
> +        else if (!isText() && (!hasLayer() || !toRenderLayerModelObject(this)->layer()->isComposited()))

This is wrong. Opacity may have toggled on a renderer with a layer (maybe it has overflow), but we still have to do a layout because opacity affects stacking context.

> LayoutTests/ChangeLog:9
> +        Added layout test to check if we are avoiding redundant texture uploads
> +        in composite layers when toggling opacity.

Please don't call these "texture uploads"; that's very specific to your point of view. They are just paints.

[Simon Fraser (smfr)]

(In reply to comment #17)
> (From update of attachment 195426 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=195426&action=review
> 
> > Source/WebCore/page/Frame.h:139
> > +        String layerTreeRepaintCountAsText() const;
> 
> You should not add a new function for this. Just add a new flag to LayerTreeFlags, and optionally dump repaint counts as part of the layer tree dump.

I remembered that we can already dump repaint rects in composting layers, which should be enough to test your changes here. No new testing infrastructure required.

[Bruno Abinader (history only)]

(In reply to comment #18)
> (In reply to comment #17)
> > (From update of attachment 195426 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=195426&action=review
> > 
> > > Source/WebCore/page/Frame.h:139
> > > +        String layerTreeRepaintCountAsText() const;
> > 
> > You should not add a new function for this. Just add a new flag to LayerTreeFlags, and optionally dump repaint counts as part of the layer tree dump.
> 
> I remembered that we can already dump repaint rects in composting layers, which should be enough to test your changes here. No new testing infrastructure required.

You are right, Simon. I erroneously assumed that repaint rects were collected for another purpose. Using repaint rects fits the purpose of the layout test. I am about to upload a new version of this patch, w/o changes to window.internals, after patch from bug 113732 lands.

[Bruno Abinader (history only)]

Created attachment 197160 [details]
Proposed patch

Fix from bug 113732 only affects visual output of the layout test, but generated render layer is the same.

[Build Bot]

Comment on attachment 197160 [details]
Proposed patch

Attachment 197160 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17649066

New failing tests:
media/video-zoom.html
compositing/repaint/avoid-repaint-when-changing-opacity.html

[Build Bot]

Created attachment 197190 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.2

[Build Bot]

Comment on attachment 197160 [details]
Proposed patch

Attachment 197160 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17722124

New failing tests:
compositing/repaint/avoid-repaint-when-changing-opacity.html

[Build Bot]

Created attachment 197246 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2

[Bruno Abinader (history only)]

Created attachment 197252 [details]
Proposed patch

Added mac-specific test result / invalidate_rect.html fail is unrelated and handled in r148066.

[Bruno Abinader (history only)]

Created attachment 197256 [details]
Proposed patch

Mac results were actually correct for all platforms, after changes from r148048 which fixed window.internals.repaintRectAsText() behavior.

[Simon Fraser (smfr)]

Comment on attachment 197256 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=197256&action=review

> Source/WebCore/rendering/RenderObject.cpp:1735
> +    // if we are not composited or have no children neither overflow.

Do you mean "no children that overflow" or "no children, nor overflow"?

> Source/WebCore/rendering/RenderObject.cpp:1739
> +        if ((contextSensitiveProperties & ContextSensitivePropertyOpacityToggle) && (!isCompositedLayer || (!isEmpty() && !hasOverflowClip())))
> +            diff = StyleDifferenceLayout;

I don't think this (!isEmpty() && !hasOverflowClip()) test makes sense. You need to do layout when opacity toggles to update z-order lists, visual overflow etc.

[Bruno Abinader (history only)]

Created attachment 198193 [details]
Patch

Now when opacity toggles, full layout behavior is preserved, however full repaint is avoided in favor of recomposite when layer is composited.

[Simon Fraser (smfr)]

*** Bug 116047 has been marked as a duplicate of this bug. ***

[Ralph T]

I am running into a full relayout and repaint of a composited layer which gets some opacity (it goes from fully opaque to translucent).

Would adding a StyleDifferentLayoutIfUncomposited flag or passing whether the styled element is already composited to RenderStyle::diff work?

I think it'd be OK so long as the element isn't composited merely because it has opacity (which might be the case on iOS).

RenderStyle doesn't know anything about what it's applied to currently, so adding the knowledge of whether something is composited is ugly. Adding a new StyleDifference flag that's almost the same as Layout is lame too, since there are several places that check for StyleDifferenceLayout.

Simon, what do you think about:
 1. Handling this just for "already composited" elements?
 2. Adding a StyleDifferentLayoutIfUncomposited flag versus passing a ElementIsComposited flag to RenderStyle::diff?

[Radu Stavila]

Comment on attachment 198193 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=198193&action=review

> Source/WebCore/rendering/RenderLayerModelObject.cpp:126
>                      || oldStyle->transform() != newStyle->transform()

The negative condition on the result of the || operation makes it harder to read. Maybe "... && oldStyle->opacity() != 1 && newStyle->opacity() != 1)" would be clearer?

> Source/WebCore/rendering/RenderObject.cpp:1811
> +

I think it would be better to avoid the if/else sequence and just use the condition as a parameter to the setter.

[Simon Fraser (smfr)]

Comment on attachment 198193 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=198193&action=review

> Source/WebCore/rendering/RenderObject.h:1105
> +        ADD_BOOLEAN_BITFIELD(needsLayoutRecompositeOnly, NeedsLayoutRecompositeOnly);

I don't think it's right to use a RenderObject bit for this. First, it doesn't apply to all RenderObjects (only RenderElements). Second, it's state that only persists between style changes and not for the life of the object.