1008301 - Unnecessary invalidation during scrolling in this testcase due to changing visible region

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Markus Stange [:mstange]]

Attachment: testcase

In the testcase, there are two grey boxes. When the upper one scrolls out of the viewport, we invalidate the green box and a bit around it. That's because we change the visible region of the container layer in the basic layer manager of the inactive transform of the wrapper element, which contains the two grey boxes (but not the green one).

This visible region is created in FrameLayerBuilder::BuildContainerLayerFor using state.GetChildrenBounds() - it's a bounding box.

Comment 1

[Markus Stange [:mstange]]

Attachment: alternative testcase

This testcase is very similar to the first one, except that it has another wrapping transform.

Comment 2

[Markus Stange [:mstange]]

These kinds of constructs, i.e. many nested inactive transform layers, are extremely common in SVGs. Scrolling over SVGs currently causes lots of unnecessary invalidations, and this bug is one of the causes.

Comment 3

[Markus Stange [:mstange]]

Attachment: invalidation reftest

Comment 4

[Markus Stange [:mstange]]

Attachment: part 1: Don't invalidate if the visible region changes

I'd like to just kill this invalidation. All causes of visible region changes I could think of will either invalidate their frame manually or are handled by the other checks in LayerTreeInvalidation.cpp. The tryserver found one case where this patch causes us to miss something that needs to be invalidated, and I'm fixing that in the next patch.

Comment 5

[Markus Stange [:mstange]]

Attachment: part 2: Invalidate when container prescale changes

The only failure with the preceding patch was in http://dxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/978911-1.svg .

That testcase changes the transform of a basic container layer, but the container layer's prescale changes to match so that the layer still has 1:1 layer pixels to screen pixels, so Layer::GetTransform() multiplies that prescale into the transform it returns, so LayerPropertiesBase::ComputeChange doesn't see a transform change and doesn't invalidate the new layer contents.

This patch treats changes of pre and post scale like transform changes and invalidates both the old and the new bounds.

Comment 6

[Markus Stange [:mstange]]

Attachment: part 3: Annotate scale change invalidation

The 978911-1.svg reftest was getting partially invalidated even without the "part 2" patch, but only the pre-transform-change bounds were invalidated. And that invalidation only printed "invalidating entire layer etc." without giving the reason. This adds the reason logging.

Comment 7

[Markus Stange [:mstange]]

Attachment: part 4: Make clipping changes invalidate less

Part 1 is not enough to stop the invalidation on the testcases in this bug; there's another invalidation coming from the changes in clip. The current code there doesn't make much sense to me, but I'm not sure that this change makes more sense... does this look correct?
It stops the invalidation and passes reftests, at least.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8420244 [details] [diff] [review]
part 4: Make clipping changes invalidate less

Review of attachment 8420244 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/DisplayItemClip.cpp
@@ +275,5 @@
>    mRoundedClipRects.Clear();
>  }
>  
>  static void
> +AccumulateRectDifference(const nsRect& aR1, const nsRect& aR2, const nsRect& aClip, nsRegion* aOut)

Don't call this "aClip" since the caller is working with cliprects and aClip is not a clip rect there. Call this aBounds and document that this computes the difference between aR1 and aR2 and then limits that to aBounds.

@@ +300,5 @@
>      return;
>    }
>    if (mHaveClipRect) {
> +    AccumulateRectDifference(mClipRect + aOffset, aOther.mClipRect,
> +                             aBounds.Union(aOtherBounds),

It's certainly easier to understand that this is correct.

Comment 10

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/16ddb5b9eb6d
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1fab05aa5f5
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd16c8833501
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a9b7a423db8

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I backed this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/62b1d56d91e5 (alongside bug 1008578) for causing OSX 10.8 reftest failures: 
https://tbpl.mozilla.org/php/getParsedLog.php?id=39511663&tree=Mozilla-Inbound

Comment 12

[Markus Stange [:mstange]]

Bug 1008578 was responsible. Relanded:
https://hg.mozilla.org/integration/mozilla-inbound/rev/30b083624741
https://hg.mozilla.org/integration/mozilla-inbound/rev/237e5755e750
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c9aa8bf1dc9
https://hg.mozilla.org/integration/mozilla-inbound/rev/de4fccc2e2fa

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/30b083624741
https://hg.mozilla.org/mozilla-central/rev/237e5755e750
https://hg.mozilla.org/mozilla-central/rev/2c9aa8bf1dc9
https://hg.mozilla.org/mozilla-central/rev/de4fccc2e2fa

Comment 14

[Markus Stange [:mstange]]

Backed out from Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/0fcb1dc73d58

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Target milestone tracks trunk landing, which hasn't changed. Setting things accordingly.

Comment 16

[Markus Stange [:mstange]]

Backed out from Aurora again: https://hg.mozilla.org/releases/mozilla-aurora/rev/8c0e169eb2b9