Closed Bug 1008301 Opened 6 years ago Closed 6 years ago

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


(Core :: Graphics: Layers, defect)

Not set



Tracking Status
firefox32 --- disabled
firefox33 --- disabled
firefox34 --- fixed


(Reporter: mstange, Assigned: mstange)


(Depends on 1 open bug)



(7 files)

Attached file 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.
Attached file alternative testcase
This testcase is very similar to the first one, except that it has another wrapping transform.
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.
Attachment #8420230 - Attachment description: invalidat reftest → invalidation reftest
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.
Attachment #8420234 - Flags: review?(roc)
The only failure with the preceding patch was in .

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.
Attachment #8420237 - Flags: review?(roc)
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.
Attachment #8420243 - Flags: review?(roc)
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.
Attachment #8420244 - Flags: review?(roc)
No longer blocks: 913463
Duplicate of this bug: 913463
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.
Attachment #8420244 - Flags: review?(roc) → review+
Blocks: 993890
Depends on: 1020556
Depends on: 1021564
Depends on: 1025914
Depends on: 1032658
See Also: → 1032658
No longer depends on: 1020556
Depends on: 1038712
Backed out from Aurora:
Target Milestone: mozilla32 → mozilla33
Target milestone tracks trunk landing, which hasn't changed. Setting things accordingly.
Target Milestone: mozilla33 → mozilla32
Depends on: 1050493
Depends on: 1168046
You need to log in before you can comment on or make changes to this bug.