805197 - Page scrolling has become juddery since DLBI landed

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[QOAL]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/20100101 Firefox/16.0
Build ID: 20121010144125

Steps to reproduce:

Visit: http://qoal.x10.mx/view.php?id=48
Scroll around the page (I'm using the middle click to pan)

I should this is on windows 7 without D2D support.


Actual results:

Scrolling performances has dropped an obvious amount compared to stable builds


Expected results:

It should have been as smooth, if not smoother than current stable builds

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: WIP: Offset visible regions.

We don't actually apply the inactive layer offset to the visible region of ThebesLayers because that would affect drawing.

But we *do* need to apply it when comparing visible regions in LayerTreeInvalidation, otherwise it moves on scroll.

This is causing us to invalidate the entire page on scroll, and this patch fixes that.

There might be other ways to fix this, like making nsDisplayOpacity/nsDisplaySVGEffects reference frames as well, but I don't think that's easier.

Comment 2

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

Comment on attachment 676915 [details] [diff] [review]
WIP: Offset visible regions.

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

::: layout/base/FrameLayerBuilder.cpp
@@ +628,5 @@
>     * offset specified by the parent ContainerLayer/
>     */
>    nsIntPoint mTranslation;
>  
> +  nsIntPoint mOffset;

Needs documentation

::: layout/base/FrameLayerBuilder.h
@@ +199,5 @@
>                           const nsDisplayList& aChildren,
>                           const ContainerParameters& aContainerParameters,
>                           const gfx3DMatrix* aTransform);
>  
> +  static const nsIntPoint& GetOffsetForThebesLayer(ThebesLayer* aLayer);

Needs documentation

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Offset visible regions

Comment 4

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

I'm actually going to renege on my previous feedback+. This doesn't make sense to me. Why doesn't LayerTreeInvalidation::MoveBy take care of this?

Comment 5

[Matt Woodrow (:mattwoodrow)]

The idea of the offsetting code was that the only thing that changes during scrolling is the translation component of the transform on the root ContainerLayer of an inactive subtree.

LayerTreeInvalidation::MoveBy then only needs adjust that transform.

I guess we could make it walk down through the cached layer tree and shift the visible regions of ThebesLayers?

We can't make the visible region of the ThebesLayers not move in the first place, because this affects what content gets drawn.

Other layer types get constant visible regions and don't have this problem.

Comment 6

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #5)
> The idea of the offsetting code was that the only thing that changes during
> scrolling is the translation component of the transform on the root
> ContainerLayer of an inactive subtree.
> LayerTreeInvalidation::MoveBy then only needs adjust that transform.

Right.

I think there's some coordinate system confusion here that this patch works arond, but I can't quite put my finger on what it is.

Comment 7

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

But it seems to me that everything in a LayerTreeInvalidation was in a consistent coordinate space, we wouldn't have a problem here and MoveBy alone would be enough.

Comment 8

[Matt Woodrow (:mattwoodrow)]

The obvious way to do this is to add rgn.MoveBy(mParameters.mOffset) here:

http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp#1594

Just like we have above for ColorLayers, and ImageLayers (inside ConfigureLayer).

That removes the need to touch LayerTreeInvalidation, but it breaks drawing. We'd need to un-shift in DrawThebesLayer I think.

Comment 9

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

Let's do that, I think.

Comment 10

[Matt Woodrow (:mattwoodrow)]

Actually, I don't know how to make that work.

The layers position on the screen is determined the visible area + transform. If we shift both of those and only adjust the ContainerLayer transform once, then the layer ends up in the wrong place. We can't shift the ContainerLayer transform twice, because then LayerTreeInvalidation would think it had changed and invalidates everything.

Alternatively we could shift both transform + visible area, and then make all the layer backends take GetThebesLayerOffset into account when drawing the layer, but I don't think that's winning.

Comment 11

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #5)
> I guess we could make it walk down through the cached layer tree and shift
> the visible regions of ThebesLayers?

Now I'm thinking that this is the right way to go. When we change the transform on the root, it makes sense that the visible regions of the layers should to be adjusted to compensate, since they should be constant after taking the transform into account.

What's not clear to me now is why the non-ThebesLayers don't need the same treatment.

Comment 12

[Matt Woodrow (:mattwoodrow)]

I think we need to retain the un-shifted visible region though, for determining what area changed when there is an actual change.

Comment 13

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

OK.

Comment 14

[Matt Woodrow (:mattwoodrow)]

I have a reduced test case now. This only occurs when the inactive layer extends off both ends of the visible page.

The change is visible region is legitimate, and when we Xor them we end up with two rects. One is the newly scrolled in content, and the other is the scrolled out content.

Unfortunately we simplify this region to a single rect before we intersect with the visible area of the destination.

I think switching LayerTreeInvalidation to use regions instead of rects is the easier solution here. Passing the ThebesLayer visible region down into the computation sounds harder since we'd have to adjust it for transforms at each step.

Comment 15

[Matt Woodrow (:mattwoodrow)]

There might be an easier solution, thoughts appreciated.

There seems to be 2 main reasons that the visible region of a layer would change.

1) Scrolling. We don't actually need to catch the visible region changes here at all, since DLBI will have picked up all the new content within the layer and set an appropriate rect on the ThebesLayer.

2) Occlusion changes - Something that was covering the layer previously has moved/been removed etc. We *do* need to pick the visible region change in this case, this was the cause of bug 795694.

I can't think of any way to separate these two cases though, so I'm still looking at the solution from comment 14.

Comment 16

[Matt Woodrow (:mattwoodrow)]

Attachment: Use regions for LayerTreeInvalidation

Comment 17

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

Comment on attachment 679951 [details] [diff] [review]
Use regions for LayerTreeInvalidation

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

This looks OK but I'm worried about over-complex regions. I think we should wrap the Or() calls in a helper, say AddRegion, and then have AddRegion and AddTransformedRegion call SimplifyOutward(4) or something on the destination after each operation.

Comment 18

[Matt Woodrow (:mattwoodrow)]

Attachment: Use regions for LayerTreeInvalidation v2

Added SimplifyOutward.

Comment 19

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/82cb8e969e0c

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/82cb8e969e0c

Comment 21

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

Should we take this patch here on Aurora?

Comment 22

[Matt Woodrow (:mattwoodrow)]

I'm not sure how many pages are affected by this, but it's a simple, low risk patch so maybe we should.

Comment 23

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 680679 [details] [diff] [review]
Use regions for LayerTreeInvalidation v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): DLBI
User impact if declined: Some pages have worse scrolling performance, unsure how many pages are affected by this though. Probably not too many.
Testing completed (on m-c, etc.): Been on m-c for a while.
Risk to taking this patch (and alternatives if risky): Very low risk, more or less just a find/replace swapping out rects for regions.
String or UUID changes made by this patch: None.

Comment 24

[Alex Keybl [:akeybl]]

Comment on attachment 680679 [details] [diff] [review]
Use regions for LayerTreeInvalidation v2

[Triage Comment]
Very low risk DLBI regression fix. Approving for Beta 18.

Comment 25

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/releases/mozilla-beta/rev/060374fbf9b8

Comment 26

[Mihai Morar, (:MihaiMorar)]

I confirm the fix is verified on FF 19b4 on Windows 7 x64.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/20100101 Firefox/19.0
20130130080006

Comment 27

[Bogdan Maris, Desktop Test Engineering]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/20100101 Firefox/19.0

Verified fixed in Firefox 19 beta 5 (buildID: 20130206083616), latest Nightly (buildID: 20130211031055), latest Aurora (buildID: 20130211042016)

Comment 28

[Bogdan Maris, Desktop Test Engineering]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0

Verified in Firefox 18.0.1 release as well (buildID: 20130116073211)