1016558 - Use the right critical display port when painting a layer with a 2D integer-translation CSS transform

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

As a result of the problem described in the comment at [1], layers which have a 2D integer-translation css transform end up getting tiled, but use incorrect values for their displayport properties. This results in low-res content possibly getting painted instead of high-res content.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/client/ClientTiledThebesLayer.cpp?rev=bd59c5ef0677#78

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Log critical displayport

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Rename a bunch of stuff and update comments

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: (WIP) Part 3 - Stop special-casing the scroll offset and deal with transforms more generically

Comment 4

[Kartikaya Gupta (email:[e-mail])]

This code is not easy to exercise locally so I did a try push to catch any obvious problems. https://tbpl.mozilla.org/?tree=Try&rev=908d83eb186e

Comment 5

[Chris Lord [:cwiiis]]

Comment on attachment 8429507 [details] [diff] [review]
Part 1 - Log critical displayport

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

LGTM.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

The try in comment 4 looks fine, but that push has low-res disabled.

I also did a full try run with low-res/progressive enabled, it is at https://tbpl.mozilla.org/?tree=Try&rev=06d71a0ef764. It shows some new failures, presumably caused by the WIPs above. I'll repro them locally and see if I can figure out what's going on.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Attachment: (WIP) Part 3v2 - Stop special-casing the scroll offset and deal with transforms more generically

Had a small bug in the previous version, should be fixed now. Did another try run, waiting on results before flagging for review.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Rename a bunch of stuff and update comments

This part should be ready for review.

Comment 9

[Botond Ballo [:botond] [away until Oct 19]]

Comment on attachment 8430389 [details] [diff] [review]
Part 2 - Rename a bunch of stuff and update comments

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

::: gfx/layers/client/ClientTiledThebesLayer.cpp
@@ +148,5 @@
>    // Calculate the transform required to convert ParentLayer space of our
>    // display port parent to LayoutDevice space of this layer.
> +  gfx::Matrix4x4 transform = scrollAncestor->GetTransform();
> +  ContainerLayer* displayPortAncestorParent = displayPortAncestor->GetParent() ?
> +    displayPortAncestor->GetParent()->GetParent() : nullptr;

Technically, this would be a displayPortAncestorGrandparent ;)

Comment 10

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #9)
> Technically, this would be a displayPortAncestorGrandparent ;)

True. Part 3 actually gets rid of this by refactoring it, and does end up using the parent instead of the grandparent. I don't even know why it was using the grandparent here.

Comment 11

[Chris Lord [:cwiiis]]

Comment on attachment 8430389 [details] [diff] [review]
Part 2 - Rename a bunch of stuff and update comments

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

Looks good, I think some of the comments could use some tweaking (see below), but it's definitely better than what was there before.

::: gfx/layers/client/ClientTiledThebesLayer.cpp
@@ +114,5 @@
>  
>    // Get the metrics of the nearest scrollable layer and the nearest layer
>    // with a displayport.
> +  ContainerLayer* displayPortAncestor = nullptr;
> +  ContainerLayer* scrollAncestor = nullptr;

nit: It's verging on ocd, but it'd be nicer if these two were declared the other way round (scrollAncestor first) to match the comment, and to get a nice line-length progression :) Not sure why I didn't do this in the first place...

@@ +139,5 @@
>      return;
>    }
>  
>    // Note, not handling transformed layers lets us assume that LayoutDevice
>    // space of the scroll parent layer is the same as LayoutDevice space of

'of the scroll ancestor layer'

@@ +147,3 @@
>  
>    // Calculate the transform required to convert ParentLayer space of our
>    // display port parent to LayoutDevice space of this layer.

'display port ancestor'

@@ +148,5 @@
>    // Calculate the transform required to convert ParentLayer space of our
>    // display port parent to LayoutDevice space of this layer.
> +  gfx::Matrix4x4 transform = scrollAncestor->GetTransform();
> +  ContainerLayer* displayPortAncestorParent = displayPortAncestor->GetParent() ?
> +    displayPortAncestor->GetParent()->GetParent() : nullptr;

Botond is right, but I think it's actually worth renaming this to reflect that - displayPortAncestorGrandParent or ParentParent would do... Or displayPortAncestorContainerParent, perhaps?

@@ +150,5 @@
> +  gfx::Matrix4x4 transform = scrollAncestor->GetTransform();
> +  ContainerLayer* displayPortAncestorParent = displayPortAncestor->GetParent() ?
> +    displayPortAncestor->GetParent()->GetParent() : nullptr;
> +  for (ContainerLayer* ancestor = scrollAncestor->GetParent();
> +       ancestor != displayPortAncestorParent;

Alternatively, we could just use the actual displayPortAncestorParent and reformat this loop into a do { ... } while (ancestor != displayPortAncestorParent); ?

@@ +173,3 @@
>                                        criticalDisplayPort)));
>  
> +  // Compute the viewport that applies to this layer in the LayoutDevice

This comment has been changed, is this correct? Should it not read 'Compute the viewport that applies to the displayport ancestor layer in the LayoutDevice space of this layer' ?

@@ +182,2 @@
>  
> +  // Store the resolution from the displayport layer. Because this is Gecko-side,

nit: 'displayport ancestor layer', for consistency?

::: gfx/layers/client/TiledContentClient.cpp
@@ +892,2 @@
>  {
>    // Transform the current composition bounds into ParentLayer coordinates

Maybe modify this comment to read 'into the same space as the displayport ancestor of this layer' ?

@@ +967,5 @@
>      }
>    }
>  
>    // Transform the composition bounds, which is in the ParentLayer coordinates
>    // of the nearest ContainerLayer with a valid displayport to LayoutDevice

and same modification here (which is already saying that, but is wordier than it needs to be now).

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Rename a bunch of stuff and update comments

Updated to address review comments, carrying r+. Unless noted below I fixed it as per the review comment.

(In reply to Chris Lord [:cwiiis] from comment #11)
> @@ +150,5 @@
> > +  gfx::Matrix4x4 transform = scrollAncestor->GetTransform();
> > +  ContainerLayer* displayPortAncestorParent = displayPortAncestor->GetParent() ?
> > +    displayPortAncestor->GetParent()->GetParent() : nullptr;
> > +  for (ContainerLayer* ancestor = scrollAncestor->GetParent();
> > +       ancestor != displayPortAncestorParent;
> 
> Alternatively, we could just use the actual displayPortAncestorParent and
> reformat this loop into a do { ... } while (ancestor !=
> displayPortAncestorParent); ?

I do something like this in part 3. For part 2 I'll just rename it to displayPortAncestorGrandParent

> @@ +173,3 @@
> >                                        criticalDisplayPort)));
> >  
> > +  // Compute the viewport that applies to this layer in the LayoutDevice
> 
> This comment has been changed, is this correct? Should it not read 'Compute
> the viewport that applies to the displayport ancestor layer in the
> LayoutDevice space of this layer' ?

Except we don't really care if the viewport applies to the displayport ancestor layer. The reason we're converting it to the LayoutDevice space of *this* layer is because we care about *this* layer, and that viewport is the closest ancestor viewport that is useful. That's why I updated the comment to read the way it does. The relevance of the viewport is that it applies to *this* layer.

> ::: gfx/layers/client/TiledContentClient.cpp
> @@ +892,2 @@
> >  {
> >    // Transform the current composition bounds into ParentLayer coordinates
> 
> Maybe modify this comment to read 'into the same space as the displayport
> ancestor of this layer' ?
> 
> @@ +967,5 @@
> >      }
> >    }
> >  
> >    // Transform the composition bounds, which is in the ParentLayer coordinates
> >    // of the nearest ContainerLayer with a valid displayport to LayoutDevice
> 
> and same modification here (which is already saying that, but is wordier
> than it needs to be now).

I merged the two comments into and clarified it a bit. I didn't see the point of having two comments that basically the same thing, given that one is inside a helper function and the other was at the only callsite.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 3 - Stop special-casing the scroll offset and deal with transforms more generically

Comment 14

[Kartikaya Gupta (email:[e-mail])]

My previous green reftest run with these patches is at https://tbpl.mozilla.org/?tree=Try&rev=d046a9fca42f and I just did an all push at https://tbpl.mozilla.org/?tree=Try&rev=1bea98312267

Comment 15

[Chris Lord [:cwiiis]]

Comment on attachment 8430782 [details] [diff] [review]
Part 3 - Stop special-casing the scroll offset and deal with transforms more generically

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

This looks sound, but it's hard to wrap your head around things like this. Do we still not have a way of testing these functions? Let's at least file a bug (if there isn't already one) to get some tests in for this. I'm sure it works now, but I can see it being broken...

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Filed bug 1017632 for testing that code.

Comment 17

[Botond Ballo [:botond] [away until Oct 19]]

Comment on attachment 8430782 [details] [diff] [review]
Part 3 - Stop special-casing the scroll offset and deal with transforms more generically

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

Does this patch resolve the FIXME near the beginning of BeginPaint()? If so, we should remove it.

::: gfx/layers/client/ClientTiledThebesLayer.cpp
@@ +61,5 @@
> +static gfx3DMatrix
> +GetTransformToAncestorsParentLayer(Layer* aStart, Layer* aAncestor)
> +{
> +  gfx::Matrix4x4 transform;
> +  Layer* aAncestorParent = aAncestor->GetParent();

s/aAncestorParent/ancestorParent

@@ +157,5 @@
>  
>    // Calculate the transform required to convert ParentLayer space of our
> +  // display port ancestor to the LayoutDevice space of this layer.
> +  gfx3DMatrix layoutDeviceToDisplayPort =
> +    GetTransformToAncestorsParentLayer(this, displayPortAncestor);

It looks like, compared to the previous code, we are excluding the transform of the ancestor-parent itself. Is this intentional?

@@ +168,5 @@
>    // Compute the critical display port that applies to this layer in the
>    // LayoutDevice space of this layer.
>    ParentLayerRect criticalDisplayPort =
> +    (displayportMetrics.mCriticalDisplayPort * displayportMetrics.GetZoomToParent())
> +    + displayportMetrics.mCompositionBounds.TopLeft();

Is the change of coordinate system here (i.e. not adding the scroll offset) related to the other changes in this patch?

Comment 18

[Botond Ballo [:botond] [away until Oct 19]]

(In reply to Botond Ballo [:botond] from comment #17)
> Does this patch resolve the FIXME near the beginning of BeginPaint()? If so,
> we should remove it.

In fact, does it remove the need for the entire "if (!GetBaseTransform().Is2DIntegerTranslation())" block?

Comment 19

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #17)
> Does this patch resolve the FIXME near the beginning of BeginPaint()? If so,
> we should remove it.

I believe that it resolves it partially. We should now be able to deal with any 2d translation and scale transforms. I'd rather adjust that in a follow-up bug because it might flush out more bugs. Ok I file a follow-up for it? We might also be able to get rid of the fennec special-casing there.

> ::: gfx/layers/client/ClientTiledThebesLayer.cpp
> @@ +61,5 @@
> > +static gfx3DMatrix
> > +GetTransformToAncestorsParentLayer(Layer* aStart, Layer* aAncestor)
> > +{
> > +  gfx::Matrix4x4 transform;
> > +  Layer* aAncestorParent = aAncestor->GetParent();
> 
> s/aAncestorParent/ancestorParent

Oops, good catch. Will fix.

> @@ +157,5 @@
> >  
> >    // Calculate the transform required to convert ParentLayer space of our
> > +  // display port ancestor to the LayoutDevice space of this layer.
> > +  gfx3DMatrix layoutDeviceToDisplayPort =
> > +    GetTransformToAncestorsParentLayer(this, displayPortAncestor);
> 
> It looks like, compared to the previous code, we are excluding the transform
> of the ancestor-parent itself. Is this intentional?

Yes. I see no reason to include that transform (i.e. I don't know why it was going up to the grandparent before), and the new code doesn't seem to break any cases that I could find.

> @@ +168,5 @@
> >    // Compute the critical display port that applies to this layer in the
> >    // LayoutDevice space of this layer.
> >    ParentLayerRect criticalDisplayPort =
> > +    (displayportMetrics.mCriticalDisplayPort * displayportMetrics.GetZoomToParent())
> > +    + displayportMetrics.mCompositionBounds.TopLeft();
> 
> Is the change of coordinate system here (i.e. not adding the scroll offset)
> related to the other changes in this patch?

Yes. Because the transform on the layer is related to the scroll offset, I removed the scroll offset and started using the transform instead. That handles the more general case so we don't need special code for detecting "scroll" vs "CSS transform". Adding the composition bounds is still needed because if you ignore the scroll offset, then the critical displayport is relative to the composition bounds and we need to convert it to be relative to the layer origin.

Comment 20

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #19)
> I believe that it resolves it partially. We should now be able to deal with
> any 2d translation and scale transforms. I'd rather adjust that in a
> follow-up bug because it might flush out more bugs. Ok I file a follow-up
> for it? We might also be able to get rid of the fennec special-casing there.

Note in particular I suspect it will require a bunch of tests to be fuzzed because more code will be going through this transformation stuff which likely introduces rounding errors.

Comment 21

[Botond Ballo [:botond] [away until Oct 19]]

Comment on attachment 8430782 [details] [diff] [review]
Part 3 - Stop special-casing the scroll offset and deal with transforms more generically

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

Thanks for the explanations! A follow-up for removing the FIXME and such is fine.

Comment 22

[Kartikaya Gupta (email:[e-mail])]

Thanks. I filed bug 1017680 for the FIXME.

Comment 23

[Kartikaya Gupta (email:[e-mail])]

Landed:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3220a5316abd
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/089ba7c06da3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/48227bbdb213

For reference the most try pushes are at:
https://tbpl.mozilla.org/?tree=Try&rev=1bea98312267
https://tbpl.mozilla.org/?tree=Try&rev=47ce44eb72ec

Comment 24

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

https://hg.mozilla.org/mozilla-central/rev/3220a5316abd
https://hg.mozilla.org/mozilla-central/rev/089ba7c06da3
https://hg.mozilla.org/mozilla-central/rev/48227bbdb213