912148 - Displayport, low-res displayport, and tiling code need some love

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

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

Attachment: Page which reproduces the problems on Galaxy Q

I've been using Fennec Nightly on my Galaxy Q running 2.3 and lately I've been seeing a degradation in the low-res painting behaviour. I'm not sure exactly when this started, but the various symptoms I'm seeing are:

- Content that should have a high-res version gets drawn in low-res temporarily. For example if I wait for a page to finish loading, scroll down a little bit, and then scroll back up, it might show the top part in low-res even though I only scrolled down a small amount (within the displayport area).
- The tiles that get displayed for low-res content correspond to other parts of the page and do not match the area of the content I'm panned to
- Low-res tiles are shown at the wrong resolution (i.e. corresponding to a zoomed-out view of the page rather than the zoomed-in zoom level I'm actually at)

Some of these might be separate issues so this bug might be better turned into a metabug but I wanted to file something rather than waiting to do it "properly" because I've been waiting for a week or two and haven't had the time to isolate the different misbehaviours. Most of this I've seen on my bugmail dashboard page which is not publicly accessible, but I am attaching a static snapshot of the page which reproduces the problem as well.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Let's try to get a window around when we started to get worse

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Kats, reassign as you see fit.

Comment 3

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

Re-assigning to BenWa for now although we should probably give this to somebody else who has more free time and BenWa can mentor them through this code. Otherwise knowledge of this code is too concentrated in too few people.

Comment 4

[Chris Lord [:cwiiis]]

I'll take this, I assume that that's ok and BenWa hasn't started on it yet what with having a million other things to do and being on PTO - apologies if that's a bad assumption, please reassign if so.

Comment 5

[Chris Lord [:cwiiis]]

I expect that most(/all?) of these issues will be solved by the patch on bug 931823 - let's use this bug to get the code using typed units, and perhaps clarify some behaviour in the comments. It's already reasonably well commented, I think.

Comment 6

[Chris Lord [:cwiiis]]

Attachment: Part 1 - Document BasicTiledLayerPaintData

This was a big piece of missing documentation :/ Annotate this struct so that it ought to be clearer what it's used for and what the values inside the struct are.

Comment 7

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

Comment on attachment 824747 [details] [diff] [review]
Part 1 - Document BasicTiledLayerPaintData

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

::: gfx/layers/client/TiledContentClient.h
@@ +111,4 @@
>    CSSPoint mScrollOffset;
> +
> +  /*
> +   * The same as the above, but from the last layer update transaction.

Code has a way of moving around so "same as the above" might end up referring to something else. I'd rather duplicate the comment.

@@ +131,5 @@
>    nsIntRect mLayerCriticalDisplayPort;
> +
> +  /*
> +   * The render resolution of the document that the content this layer
> +   * represents is in.

This is hard to parse. Maybe "The render resolution of the content represented by this layer." ? As it is it's not a document that has a resolution but a presShell, so referring to the resolution of a document is no more accurate than referring to the resolution of content.

Comment 9

[Chris Lord [:cwiiis]]

Attachment: Fix tiled update data calculation / convert to typed units

I wasn't clever enough to be able to do this in separate patches, but the final patch isn't too large. In my limited (but reasonable, I hope) testing, this fixes the calculation of these values and the problems that occurred due to their miscalculation.

Comment 10

[Chris Lord [:cwiiis]]

Comment on attachment 828704 [details] [diff] [review]
Fix tiled update data calculation / convert to typed units

Be good to have your feedback Randall, seeing as you're working on this area for apzc/b2g. I hope I haven't trodden on your toes here, let me know if you'd like to land anything first.

Comment 11

[Chris Lord [:cwiiis]]

Attachment: Fix tiled update data calculation / convert to typed units

whoops, left-over incorrect critical display port in there, one-line change (or rather, removal of a one line change).

Comment 12

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

Comment on attachment 828704 [details] [diff] [review]
Fix tiled update data calculation / convert to typed units

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

So I think it makes more sense for the stuff in BaseTiledLayerPaintData (mScrollOffset, mLastScrollOffset, mResolution) to use Layer instead of Screen. Not 100% sure on that though. Minusing for now until we discuss this more and/or you figure out the issues on the N4 you were seeing.

::: gfx/layers/client/ClientTiledThebesLayer.cpp
@@ +66,5 @@
>      }
>    }
> +  mPaintData.mTransformScreenToLayer.ScalePost(1.f/metrics.mDevPixelsPerCSSPixel.scale,
> +                                               1.f/metrics.mDevPixelsPerCSSPixel.scale,
> +                                               1);

I don't see why anything that transforms between layer and screen units should be using mDevPixelsPerCSSPixel. Also, this part of the code should probably use a local variable called "layerToScreenTransform" to calculate the transform one-way, and then assign the Inverse() of that to mPaintData.mTransformScreenToLayer. It's less confusing that way.

@@ +85,5 @@
>                                               transformedCriticalDisplayPort.height);
>    }
>  
>    // Calculate the frame resolution.
> +  mPaintData.mResolution = metrics.LayersPixelsPerCSSPixel() * LayerToScreenScale(1.0);

The LayerToScreenScale needs a comment if it's correct. It almost seems like you want to be using a CSSToLayerScale rather than a CSSToScreenScale for mResolution.

::: gfx/layers/client/TiledContentClient.cpp
@@ +427,3 @@
>  {
> +  // Transform the viewport that's in current screen space to what screen space
> +  // was when this content began rendering, then subtract the scroll offset to

I don't understand the "what screen space was when this content began rendering"

::: gfx/layers/composite/ThebesLayerComposite.cpp
@@ +175,4 @@
>  ThebesLayerComposite::GetEffectiveResolution()
>  {
> +  const FrameMetrics& metrics = GetParent()->GetFrameMetrics();
> +  return metrics.LayersPixelsPerCSSPixel() * LayerToScreenScale(1.0);

Why the LayerToScreenScale(1.0)? This needs a comment if it's correct. Also, will GetParent() ever return null here? The old for loop implicitly guarded against that but this code does not.

Comment 13

[Randall Barker [:rbarker]]

Comment on attachment 829252 [details] [diff] [review]
Fix tiled update data calculation / convert to typed units

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

I don't see anything that will cause problems with the work done on 895358 other than the need to change a few parameter types on my end.

::: gfx/layers/client/ClientLayerManager.cpp
@@ +432,5 @@
>  
>  bool
>  ClientLayerManager::ProgressiveUpdateCallback(bool aHasPendingNewThebesContent,
> +                                              ScreenRect& aViewport,
> +                                              CSSToScreenScale& aZoom,

Is aViewport actually the viewport as defined in the FrameMetrics struct or is it the mCompositionBounds? When I traced the code back into the Android callback, I was under the impression that this was the current Composition Bounds and that viewport was its name on the Android side.

::: gfx/layers/composite/ThebesLayerComposite.cpp
@@ +175,4 @@
>  ThebesLayerComposite::GetEffectiveResolution()
>  {
> +  const FrameMetrics& metrics = GetParent()->GetFrameMetrics();
> +  return metrics.LayersPixelsPerCSSPixel() * LayerToScreenScale(1.0);

I'm not certain what Effective Resolution refers to, maybe a comment some where would help? Also how is it different from mZoom in the FrameMetrics struct?

Comment 14

[Chris Lord [:cwiiis]]

Thanks for this review, as usual thorough and helpful :)

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #12)
> Comment on attachment 828704 [details] [diff] [review]
> Fix tiled update data calculation / convert to typed units
> 
> Review of attachment 828704 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So I think it makes more sense for the stuff in BaseTiledLayerPaintData
> (mScrollOffset, mLastScrollOffset, mResolution) to use Layer instead of
> Screen. Not 100% sure on that though. Minusing for now until we discuss this
> more and/or you figure out the issues on the N4 you were seeing.

I dug deeper into the numbers and realised they were wrong - fixing the patch properly also fixes the performance, so false alarm, thankfully. mScrollOffset and mLastScrollOffset are only used to detect what direction the viewport is moving in to determine what order to do tile-by-tile drawing in, so it really doesn't matter what space they're in. In this regard, I'll change them to CSS units to save a conversion. I think you're right about resolution.

> ::: gfx/layers/client/ClientTiledThebesLayer.cpp
> @@ +66,5 @@
> >      }
> >    }
> > +  mPaintData.mTransformScreenToLayer.ScalePost(1.f/metrics.mDevPixelsPerCSSPixel.scale,
> > +                                               1.f/metrics.mDevPixelsPerCSSPixel.scale,
> > +                                               1);
> 
> I don't see why anything that transforms between layer and screen units
> should be using mDevPixelsPerCSSPixel. Also, this part of the code should
> probably use a local variable called "layerToScreenTransform" to calculate
> the transform one-way, and then assign the Inverse() of that to
> mPaintData.mTransformScreenToLayer. It's less confusing that way.

This ScalePost is bogus. Agree on the inverse, will do.

> @@ +85,5 @@
> >                                               transformedCriticalDisplayPort.height);
> >    }
> >  
> >    // Calculate the frame resolution.
> > +  mPaintData.mResolution = metrics.LayersPixelsPerCSSPixel() * LayerToScreenScale(1.0);
> 
> The LayerToScreenScale needs a comment if it's correct. It almost seems like
> you want to be using a CSSToLayerScale rather than a CSSToScreenScale for
> mResolution.

I think you might be right with this assertion.

> ::: gfx/layers/client/TiledContentClient.cpp
> @@ +427,3 @@
> >  {
> > +  // Transform the viewport that's in current screen space to what screen space
> > +  // was when this content began rendering, then subtract the scroll offset to
> 
> I don't understand the "what screen space was when this content began
> rendering"

I'll revise this comment.

> ::: gfx/layers/composite/ThebesLayerComposite.cpp
> @@ +175,4 @@
> >  ThebesLayerComposite::GetEffectiveResolution()
> >  {
> > +  const FrameMetrics& metrics = GetParent()->GetFrameMetrics();
> > +  return metrics.LayersPixelsPerCSSPixel() * LayerToScreenScale(1.0);
> 
> Why the LayerToScreenScale(1.0)? This needs a comment if it's correct. Also,
> will GetParent() ever return null here? The old for loop implicitly guarded
> against that but this code does not.

It's not possible to have a thebes layer without a parent, so this should be safe.

Comment 15

[Chris Lord [:cwiiis]]

(In reply to Chris Lord [:cwiiis] from comment #14)
> > Why the LayerToScreenScale(1.0)? This needs a comment if it's correct. Also,
> > will GetParent() ever return null here? The old for loop implicitly guarded
> > against that but this code does not.
> 
> It's not possible to have a thebes layer without a parent, so this should be
> safe.

Whoops, I meant to verify this more thoroughly before hitting Save Changes... I believe this to be the case, but I'm not certain. A NULL check certainly wouldn't hurt.

Comment 16

[Chris Lord [:cwiiis]]

(In reply to Chris Lord [:cwiiis] from comment #14)
> Thanks for this review, as usual thorough and helpful :)
> 
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #12)
> > Comment on attachment 828704 [details] [diff] [review]
> > Fix tiled update data calculation / convert to typed units
> > 
> > Review of attachment 828704 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > So I think it makes more sense for the stuff in BaseTiledLayerPaintData
> > (mScrollOffset, mLastScrollOffset, mResolution) to use Layer instead of
> > Screen. Not 100% sure on that though. Minusing for now until we discuss this
> > more and/or you figure out the issues on the N4 you were seeing.
> 
> I dug deeper into the numbers and realised they were wrong - fixing the
> patch properly also fixes the performance, so false alarm, thankfully.
> mScrollOffset and mLastScrollOffset are only used to detect what direction
> the viewport is moving in to determine what order to do tile-by-tile drawing
> in, so it really doesn't matter what space they're in. In this regard, I'll
> change them to CSS units to save a conversion. I think you're right about
> resolution.

Oh, this isn't quite true either, it gets used to transform the screen rect gotten from Java... In that case, I think it is convenient for these to stay in Screen units, and similarly for mResolution.

Comment 17

[Chris Lord [:cwiiis]]

(In reply to Randall Barker from comment #13)
> Comment on attachment 829252 [details] [diff] [review]
> Fix tiled update data calculation / convert to typed units
> 
> Review of attachment 829252 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't see anything that will cause problems with the work done on 895358
> other than the need to change a few parameter types on my end.
> 
> ::: gfx/layers/client/ClientLayerManager.cpp
> @@ +432,5 @@
> >  
> >  bool
> >  ClientLayerManager::ProgressiveUpdateCallback(bool aHasPendingNewThebesContent,
> > +                                              ScreenRect& aViewport,
> > +                                              CSSToScreenScale& aZoom,
> 
> Is aViewport actually the viewport as defined in the FrameMetrics struct or
> is it the mCompositionBounds? When I traced the code back into the Android
> callback, I was under the impression that this was the current Composition
> Bounds and that viewport was its name on the Android side.

It's the composition bounds, which is referred to as the viewport on the Android side, right. This is pretty confusing, let's take this opportunity to change it Gecko-side.

> ::: gfx/layers/composite/ThebesLayerComposite.cpp
> @@ +175,4 @@
> >  ThebesLayerComposite::GetEffectiveResolution()
> >  {
> > +  const FrameMetrics& metrics = GetParent()->GetFrameMetrics();
> > +  return metrics.LayersPixelsPerCSSPixel() * LayerToScreenScale(1.0);
> 
> I'm not certain what Effective Resolution refers to, maybe a comment some
> where would help? Also how is it different from mZoom in the FrameMetrics
> struct?

Nice catch, it isn't different to mZoom. I've made this mistake elsewhere too, thanks :)

Comment 18

[Chris Lord [:cwiiis]]

Attachment: Fix tiled update data calculation / convert to typed units (v2)

As well as actually appearing to work (numbers look good, no visible perf regression, updates are (mostly) tile-aligned, units seem consistent at least), I hope this addresses your comments too.

Comment 19

[Chris Lord [:cwiiis]]

Comment on attachment 830304 [details] [diff] [review]
Fix tiled update data calculation / convert to typed units (v2)

(In reply to Chris Lord [:cwiiis] from comment #18)
> Created attachment 830304 [details] [diff] [review]
> Fix tiled update data calculation / convert to typed units (v2)
> 
> As well as actually appearing to work (numbers look good, no visible perf
> regression, updates are (mostly) tile-aligned, units seem consistent at
> least), I hope this addresses your comments too.

And of course, as soon as I say this I discover a case where it's broken. Working on it...

Comment 20

[Chris Lord [:cwiiis]]

Attachment: Fix tiled update data calculation / convert to typed units (v3)

Ok, I think this might be the one. Numbers look good, testing looks good, no obvious perf regression (though I expect it will affect numbers one way or another - probably worse jank but possibly better checkerboarding?)

I still use screen coordinates in places, as it just feels like the most convenient coordinate space to store in (in terms of cutting down the number of conversions that happen), but I'm open to suggestions.

Comment 21

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

Comment on attachment 830958 [details] [diff] [review]
Fix tiled update data calculation / convert to typed units (v3)

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

Dropping review flag for now, see my comments below.

::: gfx/layers/client/ClientLayerManager.h
@@ +119,5 @@
>    CompositorChild *GetRemoteRenderer();
>  
>    /**
>     * Called for each iteration of a progressive tile update. Fills
> +   * aCompositionBounds, aScaleX and aScaleY with the current scale and

s/aScaleX and aScaleY/aZoom/

::: gfx/layers/client/ClientTiledThebesLayer.cpp
@@ +74,5 @@
> +  layerToScreen.ScalePost(metrics.mCumulativeResolution.scale,
> +                          metrics.mCumulativeResolution.scale,
> +                          1.f);
> +
> +  mPaintData.mTransformScreenToLayer = layerToScreen.Inverse();

Ok, so I spent some time thinking about this calculation, and I believe (a) it is mislabeled and (b) it might applying things in the wrong order.

(a) I think this transform is actually from screen to layout-device space. In order to test this theory I put together a patch that applies on top of this and propagated the units. You can see the patch at https://github.com/staktrace/mozilla-central/commit/e4dc5deb0f49121517a668aa4d7c636a392fc9ef - it basically works out that with this change the nsIntRect objects you end up creating are coming from LayoutDeviceRects instead of LayerRects, which makes a lot more sense to me. This also explains why you need to multiply in the mCumulativeResolution above, because that is exactly the difference between the Layer and LayoutDevice pixel spaces. This is a non-functional change, it just changes the labelling so that things are in the right units.

(b) Based on my reading of GetEffectiveTransform and DefaultComputeEffectiveTransforms, to go from layer space to screen space, you need to multiply the EffectiveTransforms from innermost to outermost. This means you want GetEffectiveTransform() * GetParent()->GetEffectiveTransform() * ...
However your code is using PreMultiply, which does ... * GetParent()->GetEffectiveTransform() * GetEffectiveTransform(). So I think you want to use regular post-multiply instead of pre-multiply. The ScalePost that you're using for mCumulativeResolution should also be a regular scale, because that gets applied first in the conversion chain from LayoutDevicePixels to ScreenPixels. You can see what I mean in https://github.com/staktrace/mozilla-central/commit/6f6d88e5b87d530db6bf501e4e3b97d86f67fc26
Unless you've tested with a page that has layers with CSS transforms and/or intermediate surfaces both computations will give you the same result.

With the above changes here I'm happy with the units labelling and things more or less make sense. The reason I was confused about the use of "screen" before is because what you're calling "screen" here is really a previously-unnamed coordinate system in which the CSS transforms are applied but the async transforms are not. Botond and I have been referring to this as the "gecko" coordinate system but that's not really a good name and until we come up with a better one using "screen" here is fine by me.

::: gfx/layers/composite/TiledContentHost.cpp
@@ +238,2 @@
>      layerScale.width = layerResolution.width / localResolution.width;
>      layerScale.height = layerResolution.height / localResolution.height;

You should be able to keep layerResolution and localResolution as a CSSToScreenScale, and then populate layerScale.(width|height) using the division as needed. It should save you from making a couple of unnecessary gfxSize objects.

Comment 22

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #21)
> Comment on attachment 830958 [details] [diff] [review]
> Fix tiled update data calculation / convert to typed units (v3)
> 
> Review of attachment 830958 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Dropping review flag for now, see my comments below.
> 
> ::: gfx/layers/client/ClientLayerManager.h
> @@ +119,5 @@
> >    CompositorChild *GetRemoteRenderer();
> >  
> >    /**
> >     * Called for each iteration of a progressive tile update. Fills
> > +   * aCompositionBounds, aScaleX and aScaleY with the current scale and
> 
> s/aScaleX and aScaleY/aZoom/
> 
> ::: gfx/layers/client/ClientTiledThebesLayer.cpp
> @@ +74,5 @@
> > +  layerToScreen.ScalePost(metrics.mCumulativeResolution.scale,
> > +                          metrics.mCumulativeResolution.scale,
> > +                          1.f);
> > +
> > +  mPaintData.mTransformScreenToLayer = layerToScreen.Inverse();
> 
> Ok, so I spent some time thinking about this calculation, and I believe (a)
> it is mislabeled and (b) it might applying things in the wrong order.
> 
> (a) I think this transform is actually from screen to layout-device space.
> In order to test this theory I put together a patch that applies on top of
> this and propagated the units. You can see the patch at
> https://github.com/staktrace/mozilla-central/commit/
> e4dc5deb0f49121517a668aa4d7c636a392fc9ef - it basically works out that with
> this change the nsIntRect objects you end up creating are coming from
> LayoutDeviceRects instead of LayerRects, which makes a lot more sense to me.
> This also explains why you need to multiply in the mCumulativeResolution
> above, because that is exactly the difference between the Layer and
> LayoutDevice pixel spaces. This is a non-functional change, it just changes
> the labelling so that things are in the right units.
> 
> (b) Based on my reading of GetEffectiveTransform and
> DefaultComputeEffectiveTransforms, to go from layer space to screen space,
> you need to multiply the EffectiveTransforms from innermost to outermost.
> This means you want GetEffectiveTransform() *
> GetParent()->GetEffectiveTransform() * ...
> However your code is using PreMultiply, which does ... *
> GetParent()->GetEffectiveTransform() * GetEffectiveTransform(). So I think
> you want to use regular post-multiply instead of pre-multiply. The ScalePost
> that you're using for mCumulativeResolution should also be a regular scale,
> because that gets applied first in the conversion chain from
> LayoutDevicePixels to ScreenPixels. You can see what I mean in
> https://github.com/staktrace/mozilla-central/commit/
> 6f6d88e5b87d530db6bf501e4e3b97d86f67fc26
> Unless you've tested with a page that has layers with CSS transforms and/or
> intermediate surfaces both computations will give you the same result.
> 
> With the above changes here I'm happy with the units labelling and things
> more or less make sense. The reason I was confused about the use of "screen"
> before is because what you're calling "screen" here is really a
> previously-unnamed coordinate system in which the CSS transforms are applied
> but the async transforms are not. Botond and I have been referring to this
> as the "gecko" coordinate system but that's not really a good name and until
> we come up with a better one using "screen" here is fine by me.
> 
> ::: gfx/layers/composite/TiledContentHost.cpp
> @@ +238,2 @@
> >      layerScale.width = layerResolution.width / localResolution.width;
> >      layerScale.height = layerResolution.height / localResolution.height;
> 
> You should be able to keep layerResolution and localResolution as a
> CSSToScreenScale, and then populate layerScale.(width|height) using the
> division as needed. It should save you from making a couple of unnecessary
> gfxSize objects.

This is all correct, except I think for (b), where I still think pre-multiplying is correct. Consider that transforms get applied in the reverse order to the matrix multiplication (i.e. if you do Scale matrix * Translate matrix, first it translates, then it scales), applying your second patch (https://github.com/staktrace/mozilla-central/commit/6f6d88e5b87d530db6bf501e4e3b97d86f67fc26) breaks the calculations when you zoom in, but changing the matrix multiplies to PreMultiply gives the right results. Maybe I'm missing something else here?

My last patch also caused lots of robocop failures, which I need to look at :/ I'll resubmit this revised version, but I don't expect the results to be different - it could be a race, or perhaps some bad rounding. More likely the latter.

Comment 23

[Chris Lord [:cwiiis]]

Attachment: Fix tiled update data calculation / convert to typed units (v4)

Also r? from botond, as kats has contributed to this patch and the more eyes the better.

Comment 24

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

(In reply to Chris Lord [:cwiiis] from comment #22)
> Consider that transforms get applied in the
> reverse order to the matrix multiplication (i.e. if you do Scale matrix *
> Translate matrix, first it translates, then it scales)

I used to think this was the case as well but I don't think so any more. I spent a lot of time recently on the code at [1] and [2] and I convinced myself that the matrix on the left side of the multiplication operator is applied first. I'll write a quick gtest to confirm.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.h#40
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp#503

Comment 25

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

This test passes:

--- a/gfx/tests/gtest/TestAsyncPanZoomController.cpp
+++ b/gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ -570,6 +570,18 @@ TEST(APZCTreeManager, HitTesting1) {
   manager->ClearTree();
 }

+TEST(Matrix, MultOrder) {
+  gfx3DMatrix scale = gfx3DMatrix::ScalingMatrix(2.0, 2.0, 1.0);
+  gfx3DMatrix translate = gfx3DMatrix::Translation(50, 50, 0);
+  gfx3DMatrix combination = scale * translate;
+  gfxPoint transformed = combination.Transform(gfxPoint(10, 10));
+  EXPECT_EQ(gfxPoint(70, 70), transformed);
+
+  combination = translate * scale;
+  transformed = combination.Transform(gfxPoint(10, 10));
+  EXPECT_EQ(gfxPoint(120, 120), transformed);
+}
+
 // A more involved hit testing test that involves css and async transforms.
 TEST(APZCTreeManager, HitTesting2) {
   nsTArray<nsRefPtr<Layer> > layers;

Comment 26

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #25)
> This test passes:
> 
> --- a/gfx/tests/gtest/TestAsyncPanZoomController.cpp
> +++ b/gfx/tests/gtest/TestAsyncPanZoomController.cpp
> @@ -570,6 +570,18 @@ TEST(APZCTreeManager, HitTesting1) {
>    manager->ClearTree();
>  }
> 
> +TEST(Matrix, MultOrder) {
> +  gfx3DMatrix scale = gfx3DMatrix::ScalingMatrix(2.0, 2.0, 1.0);
> +  gfx3DMatrix translate = gfx3DMatrix::Translation(50, 50, 0);
> +  gfx3DMatrix combination = scale * translate;
> +  gfxPoint transformed = combination.Transform(gfxPoint(10, 10));
> +  EXPECT_EQ(gfxPoint(70, 70), transformed);
> +
> +  combination = translate * scale;
> +  transformed = combination.Transform(gfxPoint(10, 10));
> +  EXPECT_EQ(gfxPoint(120, 120), transformed);
> +}
> +
>  // A more involved hit testing test that involves css and async transforms.
>  TEST(APZCTreeManager, HitTesting2) {
>    nsTArray<nsRefPtr<Layer> > layers;

ok, thinking about it, that still feels like premultiply is correct then. Assuming a layer tree:

Layer 1 -> Layer 2 -> Layer 3,

the transform to get to layout device space on layer 3 would be (layer 1 transform * layer 2 transform * layer 3 transform * cumulative res), right? In that case, premultiply is correct (and this is born out by the results).

Comment 27

[Botond Ballo [:botond]]

Comment on attachment 831515 [details] [diff] [review]
Fix tiled update data calculation / convert to typed units (v4)

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

I don't have a very good understanding of this code, so this is less a review and more just some questions/observations/suggestions.

::: gfx/layers/client/ClientTiledThebesLayer.cpp
@@ +49,5 @@
> +  gfxRect input(aScreenRect.x, aScreenRect.y, aScreenRect.width, aScreenRect.height);
> +  gfxRect output = aTransform.TransformBounds(input);
> +  return LayoutDeviceRect(output.x, output.y, output.width, output.height);
> +}
> +

We can probably generalize this a bit to:

template <typename TargetUnits, typename SourceUnits>
RectTyped<TargetUnits>
TransformTo(const gfx3DMatrix& aTransform, const RectTyped<SourceUnits>& aSourceRect)
{
    gfxRect input(aSourceRect.x, aSourceRect.y, aSourceRect.width, aSourceRect.height);
    gfxRect output = aTransform.TransformBounds(input);
    return RectTyped<TargetUnits>(output.x, output.y, output.width, output.height);
}

Example usage:

ScreenRect input = ...;
LayoutDeviceRect result = TransformTo<LayoutDevicePixel>(transform, input);

(We might then want to place this in a different file so more things can use it.)

@@ +60,5 @@
>  
>    mPaintData.mLowPrecisionPaintCount = 0;
>    mPaintData.mPaintFinished = false;
>  
> +  const FrameMetrics& metrics = AsContainerLayer() ?

Can a thebes layer ever be a container layer?

@@ +65,5 @@
> +    AsContainerLayer()->GetFrameMetrics() : GetParent()->GetFrameMetrics();
> +
> +  // Calculate the transform required to convert screen space into layout device space
> +  gfx3DMatrix layoutToScreen =
> +    gfx3DMatrix::ScalingMatrix(metrics.mCumulativeResolution.scale,

I believe most fields of FrameMetrics, including mCumulativeResolution, are only set to correct values if the frame metrics is for a scrollable frame, i.e. metrics.mScrollId != NULL_SCROLL_ID. Do we know that's the case here?

@@ +82,5 @@
>    // Compute the critical display port in layer space.
>    mPaintData.mLayerCriticalDisplayPort.SetEmpty();
> +  if (!metrics.mCriticalDisplayPort.IsEmpty()) {
> +    // Convert the display port to screen space first so that we can transform
> +    // it into layout device space.

The display port is in CSS coordinates. The chain of conversions from CSS coordinates to Screen coordinates is CSS -> LayoutDevice -> Layer -> Screen. If we just want the display port in LayoutDevice coordinates, what is the purpose of going through Screen coordinates as an intermediate step?

::: gfx/layers/client/TiledContentClient.cpp
@@ +434,2 @@
>  
> +  // Return those composition bounds in local layer space.

Comment ("layer") does not match code ("layout device").

::: gfx/layers/client/TiledContentClient.h
@@ +117,5 @@
> +   */
> +  ScreenPoint mLastScrollOffset;
> +
> +  /*
> +   * The transform matrix to go from Screen units to local LayoutDevice units.

What does "local" mean here?

@@ +133,5 @@
>    nsIntRect mLayerCriticalDisplayPort;
> +
> +  /*
> +   * The render resolution of the document that the content this layer
> +   * represents is in.

I wonder if "render resolution of the document" is an appropriate term for something that includes CSS transforms. Elsewhere (FrameMetrics and PresShell methods), "resolution" refers to just the the scale between layout device and layer pixels, not counting CSS transforms.

::: gfx/layers/composite/ThebesLayerComposite.cpp
@@ +175,4 @@
>  ThebesLayerComposite::GetEffectiveResolution()
>  {
> +  ContainerLayer* parent = GetParent();
> +  return parent ? parent->GetFrameMetrics().mZoom : CSSToScreenScale(1.0);

I believe mZoom is also only set if the frame is scrollable. Note that the old code didn't run into a problem like this because if the frame isn't scrollable, its resolution is 1, and we initialize mResolution to 1 in the FrameMetrics constructor.

::: layout/base/Units.h
@@ +157,5 @@
>  
> +  static nsIntRect ToUntyped(const LayoutDeviceIntRect& aRect) {
> +    return nsIntRect(aRect.x, aRect.y, aRect.width, aRect.height);
> +  }
> +

Might there be value in having a similar method that returns a gfxRect?

Comment 28

[Chris Lord [:cwiiis]]

(In reply to Botond Ballo [:botond] from comment #27)
> Comment on attachment 831515 [details] [diff] [review]
> Fix tiled update data calculation / convert to typed units (v4)
> 
> Review of attachment 831515 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't have a very good understanding of this code, so this is less a
> review and more just some questions/observations/suggestions.
> 
> ::: gfx/layers/client/ClientTiledThebesLayer.cpp
> @@ +49,5 @@
> > +  gfxRect input(aScreenRect.x, aScreenRect.y, aScreenRect.width, aScreenRect.height);
> > +  gfxRect output = aTransform.TransformBounds(input);
> > +  return LayoutDeviceRect(output.x, output.y, output.width, output.height);
> > +}
> > +
> 
> We can probably generalize this a bit to:
> 
> template <typename TargetUnits, typename SourceUnits>
> RectTyped<TargetUnits>
> TransformTo(const gfx3DMatrix& aTransform, const RectTyped<SourceUnits>&
> aSourceRect)
> {
>     gfxRect input(aSourceRect.x, aSourceRect.y, aSourceRect.width,
> aSourceRect.height);
>     gfxRect output = aTransform.TransformBounds(input);
>     return RectTyped<TargetUnits>(output.x, output.y, output.width,
> output.height);
> }
> 
> Example usage:
> 
> ScreenRect input = ...;
> LayoutDeviceRect result = TransformTo<LayoutDevicePixel>(transform, input);
> 
> (We might then want to place this in a different file so more things can use
> it.)

Nice idea, like it.

> @@ +60,5 @@
> >  
> >    mPaintData.mLowPrecisionPaintCount = 0;
> >    mPaintData.mPaintFinished = false;
> >  
> > +  const FrameMetrics& metrics = AsContainerLayer() ?
> 
> Can a thebes layer ever be a container layer?

I believe so, but thinking more, even if it is, we want the container metrics anyway.

> @@ +65,5 @@
> > +    AsContainerLayer()->GetFrameMetrics() : GetParent()->GetFrameMetrics();
> > +
> > +  // Calculate the transform required to convert screen space into layout device space
> > +  gfx3DMatrix layoutToScreen =
> > +    gfx3DMatrix::ScalingMatrix(metrics.mCumulativeResolution.scale,
> 
> I believe most fields of FrameMetrics, including mCumulativeResolution, are
> only set to correct values if the frame metrics is for a scrollable frame,
> i.e. metrics.mScrollId != NULL_SCROLL_ID. Do we know that's the case here?

So we need to recurse upwards if the FrameMetrics don't correspond to a scrollable frame? Does that ever happen?

> @@ +82,5 @@
> >    // Compute the critical display port in layer space.
> >    mPaintData.mLayerCriticalDisplayPort.SetEmpty();
> > +  if (!metrics.mCriticalDisplayPort.IsEmpty()) {
> > +    // Convert the display port to screen space first so that we can transform
> > +    // it into layout device space.
> 
> The display port is in CSS coordinates. The chain of conversions from CSS
> coordinates to Screen coordinates is CSS -> LayoutDevice -> Layer -> Screen.
> If we just want the display port in LayoutDevice coordinates, what is the
> purpose of going through Screen coordinates as an intermediate step?

Because we have the transform to go to transformed layout device coordinates from screen coordinates. So it's more CSS -> Transforms -> LayoutDevice -> Layer -> Screen, I suppose(?).

> ::: gfx/layers/client/TiledContentClient.cpp
> @@ +434,2 @@
> >  
> > +  // Return those composition bounds in local layer space.
> 
> Comment ("layer") does not match code ("layout device").

Thanks, nice spot.

> ::: gfx/layers/client/TiledContentClient.h
> @@ +117,5 @@
> > +   */
> > +  ScreenPoint mLastScrollOffset;
> > +
> > +  /*
> > +   * The transform matrix to go from Screen units to local LayoutDevice units.
> 
> What does "local" mean here?

It means transformed, I should change this.

> @@ +133,5 @@
> >    nsIntRect mLayerCriticalDisplayPort;
> > +
> > +  /*
> > +   * The render resolution of the document that the content this layer
> > +   * represents is in.
> 
> I wonder if "render resolution of the document" is an appropriate term for
> something that includes CSS transforms. Elsewhere (FrameMetrics and
> PresShell methods), "resolution" refers to just the the scale between layout
> device and layer pixels, not counting CSS transforms.

I don't believe this includes CSS transforms at this point(?)

> ::: gfx/layers/composite/ThebesLayerComposite.cpp
> @@ +175,4 @@
> >  ThebesLayerComposite::GetEffectiveResolution()
> >  {
> > +  ContainerLayer* parent = GetParent();
> > +  return parent ? parent->GetFrameMetrics().mZoom : CSSToScreenScale(1.0);
> 
> I believe mZoom is also only set if the frame is scrollable. Note that the
> old code didn't run into a problem like this because if the frame isn't
> scrollable, its resolution is 1, and we initialize mResolution to 1 in the
> FrameMetrics constructor.

This is good to know, I would assume from this comment that the answer to my first question (will you ever get FrameMetrics on a container that isn't scrollable) is probably yes and we do need to recurse.

> ::: layout/base/Units.h
> @@ +157,5 @@
> >  
> > +  static nsIntRect ToUntyped(const LayoutDeviceIntRect& aRect) {
> > +    return nsIntRect(aRect.x, aRect.y, aRect.width, aRect.height);
> > +  }
> > +
> 
> Might there be value in having a similar method that returns a gfxRect?

I think having typed transforms would probably be enough here? I'll leave this down to you and kats, you have a better grasp of this code.

Comment 29

[Chris Lord [:cwiiis]]

So, taking other parts of the code-base as examples of what to do;

It's correct to multiply from child to parent order for transforms, as shown by GetTransformToAncestor: http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#1685
It's incorrect to apply the layout device scale before that transform, as shown by PredictScaleForContent: http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp#3091

So I think the correct thing will be to say transform = GetEffectiveTransform, multiply parent transforms on intermediate surfaces, then multiply the scale. So what kats suggests, except the scale comes last instead of first. I'm going to revise the patch and we can discuss it later.

Comment 30

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

Comment on attachment 831515 [details] [diff] [review]
Fix tiled update data calculation / convert to typed units (v4)

I still have some reservations about the transform that's calculated and I'm not sure it's 100% right but if it works in the appropriate scenarios then we can land it now and come back to it when we have a specific case that it breaks on.

Comment 31

[Botond Ballo [:botond]]

> > @@ +65,5 @@
> > > +    AsContainerLayer()->GetFrameMetrics() : GetParent()->GetFrameMetrics();
> > > +
> > > +  // Calculate the transform required to convert screen space into layout device space
> > > +  gfx3DMatrix layoutToScreen =
> > > +    gfx3DMatrix::ScalingMatrix(metrics.mCumulativeResolution.scale,
> > 
> > I believe most fields of FrameMetrics, including mCumulativeResolution, are
> > only set to correct values if the frame metrics is for a scrollable frame,
> > i.e. metrics.mScrollId != NULL_SCROLL_ID. Do we know that's the case here?
> 
> So we need to recurse upwards if the FrameMetrics don't correspond to a
> scrollable frame? Does that ever happen?

I notice that further down in this function they get a FrameMetrics by getting the primary scrollable layer. Perhaps we should be doing that here too?

> > @@ +82,5 @@
> > >    // Compute the critical display port in layer space.
> > >    mPaintData.mLayerCriticalDisplayPort.SetEmpty();
> > > +  if (!metrics.mCriticalDisplayPort.IsEmpty()) {
> > > +    // Convert the display port to screen space first so that we can transform
> > > +    // it into layout device space.
> > 
> > The display port is in CSS coordinates. The chain of conversions from CSS
> > coordinates to Screen coordinates is CSS -> LayoutDevice -> Layer -> Screen.
> > If we just want the display port in LayoutDevice coordinates, what is the
> > purpose of going through Screen coordinates as an intermediate step?
> 
> Because we have the transform to go to transformed layout device coordinates
> from screen coordinates. So it's more CSS -> Transforms -> LayoutDevice ->
> Layer -> Screen, I suppose(?).

Hmm. I thought the transforms existed between Layer and Screen coordinates.

> > @@ +133,5 @@
> > >    nsIntRect mLayerCriticalDisplayPort;
> > > +
> > > +  /*
> > > +   * The render resolution of the document that the content this layer
> > > +   * represents is in.
> > 
> > I wonder if "render resolution of the document" is an appropriate term for
> > something that includes CSS transforms. Elsewhere (FrameMetrics and
> > PresShell methods), "resolution" refers to just the the scale between layout
> > device and layer pixels, not counting CSS transforms.
> 
> I don't believe this includes CSS transforms at this point(?)

You're right, it's initialized using FrameMetrics::mZoom, which does not include CSS transforms. I think we overload the term Screen coordinates for several things (not just here but in APZC too) :(

Comment 32

[Chris Lord [:cwiiis]]

Attachment: Fix tiled update data calculation / convert to typed units (v5)

I didn't do the neat TransformTo idea because I think we'd rather have typed Transforms in the way we have typed Scales (so something like a Type1ToType2Transform that has Transform and Inverse methods, or something like that) - but let's leave that for another day.

Otherwise, I hope this addresses botond's comments and is correct in terms of what it does... Now I need to investigate the robocop failures :/

Comment 33

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

Comment on attachment 832338 [details] [diff] [review]
Fix tiled update data calculation / convert to typed units (v5)

Please re-request review once the robocop stuff is sorted out. If it turns out to require changes to the patch I don't want to waste time reviewing this version.

Comment 34

[Chris Lord [:cwiiis]]

So, the reason this breaks robocop is that setFirstPaintViewport happens *after* the draw that it relates too, somewhat obviously I suppose. The cancelling code doesn't check if it's the first paint, and because the first draw happens all at once due to the page fitting in the composition bounds and the composition bounds now being correct, there's no repeated transaction that would then unmask the error.

I think the correct thing to do here is disable cancelling on the first paint, because in this case, Gecko's zoom is about to override Java's.

Comment 35

[Chris Lord [:cwiiis]]

Attachment: Fix tiled update data calculation / convert to typed units (v6)

This fixes robocop for me locally, I'm running try anyway but I'm pretty confident it's ok. Unfortunately, Java still spews a load of warnings in this situation - we could tell Java if it's the first paint and allow it to make the decision about not cancelling I suppose, but I prefer this way round. The debug log may still help in some situations. Perhaps we could silence this warning while the page is loading, but I don't think it's too big a deal.

The pertinent diff is this:

--- a/gfx/layers/client/TiledContentClient.cpp
+++ b/gfx/layers/client/TiledContentClient.cpp
@@ -459,27 +459,30 @@ BasicTiledLayerBuffer::ComputeProgressiv
 
   // Find out if we have any non-stale content to update.
   nsIntRegion staleRegion;
   staleRegion.And(aInvalidRegion, aOldValidRegion);
 
   // Find out the current view transform to determine which tiles to draw
   // first, and see if we should just abort this paint. Aborting is usually
   // caused by there being an incoming, more relevant paint.
+  // We ignore if front-end wants to abort if this is the first paint, as in
+  // that situation, we're about to override front-end's page/viewport metrics.
   ScreenRect compositionBounds;
   CSSToScreenScale zoom;
   if (mManager->ProgressiveUpdateCallback(!staleRegion.Contains(aInvalidRegion),
                                           compositionBounds, zoom,
-                                          !drawingLowPrecision)) {
+                                          !drawingLowPrecision) &&
+      !aPaintData->mFirstPaint) {
     PROFILER_LABEL("ContentClient", "Abort painting");
     aRegionToPaint.SetEmpty();
     return aIsRepeated;
   }

Comment 36

[Chris Lord [:cwiiis]]

Comment on attachment 832893 [details] [diff] [review]
Fix tiled update data calculation / convert to typed units (v6)

UGH, robocop is green now (yay), but a load of reftests now fail (boo). Oddly, the reftest failures are mostly slight fuzzing, then one outright failure...

I won't bother r? without confirmed green try now, this is getting silly.

Comment 37

[Chris Lord [:cwiiis]]

https://tbpl.mozilla.org/?tree=Try&rev=1b60fbb5f7ac - The relevant try run, for reference.

Comment 38

[Chris Lord [:cwiiis]]

Ok, so I have it green on everything, but it appears to be causing red on Talos remote-svgx on the 2.2 machines :/

https://tbpl.mozilla.org/?tree=Try&rev=f6ca4c68bc8a

I'm worried that given this was an intermittent red before, this slight performance regression (which can't be helped, the old code skipped draws and broke up draws when it shouldn't have) may mean it now consistently takes too long and times out.

Comment 39

[Chris Lord [:cwiiis]]

Attachment: Fix tiled update data calculation / convert to typed units (v7, rebased)

Just uploading the most recent, rebased patch. Won't set review flags until it's green, but putting this here for reference.

Comment 40

[Chris Lord [:cwiiis]]

Comment on attachment 8335250 [details] [diff] [review]
Fix tiled update data calculation / convert to typed units (v7, rebased)

So ends up the 's' is because it's causing remote-svgx to take much longer, because previously it ended up skipping all drawing (but possibly rendering in low resolution).

Adding flags now, will post a follow up to reduce the number of iterations so that it turns green again.

Comment 41

[Chris Lord [:cwiiis]]

cc'ing Avi, as this will almost certainly regress tsvgx. I'm verifying things atm to make sure what I said is correct.

Comment 42

[Chris Lord [:cwiiis]]

This is proving harder than expected to verify visually because tsvgx doesn't actually display anything on the screen while running, due to bug 908810.

Comment 43

[Botond Ballo [:botond]]

Comment on attachment 8335250 [details] [diff] [review]
Fix tiled update data calculation / convert to typed units (v7, rebased)

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

::: gfx/layers/client/ClientTiledThebesLayer.cpp
@@ +78,5 @@
> +    mPaintData.mCompositionBounds.SetEmpty();
> +    return;
> +  }
> +
> +  const FrameMetrics& metrics = scrollParent->GetFrameMetrics();

I'm still a bit worried about how we're getting FrameMetrics objects in this function.

We work with two FrameMetrics instances in this function: this one, gotten by looping upward from the current layer to the first scrollable layer; and one further down, gotten from the layer returned by GetPrimaryScrollableLayer().

We then populate some fields of mPaintData based on the first FrameMetrics instance (mTransformScreenToLayout, mLayoutCriticalDisplayPort, mResolution), and others based on the second instance (mCompositionBounds).

Could these two FrameMetrics instances be different?

If yes, does it make sense to populate some of the fields of mPaintData based on one of them and some based on the other?

If not, why do we get them in different ways?

Comment 44

[Chris Lord [:cwiiis]]

(In reply to Botond Ballo [:botond] from comment #43)
> Comment on attachment 8335250 [details] [diff] [review]
> Fix tiled update data calculation / convert to typed units (v7, rebased)
> 
> Review of attachment 8335250 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/client/ClientTiledThebesLayer.cpp
> @@ +78,5 @@
> > +    mPaintData.mCompositionBounds.SetEmpty();
> > +    return;
> > +  }
> > +
> > +  const FrameMetrics& metrics = scrollParent->GetFrameMetrics();
> 
> I'm still a bit worried about how we're getting FrameMetrics objects in this
> function.
> 
> We work with two FrameMetrics instances in this function: this one, gotten
> by looping upward from the current layer to the first scrollable layer; and
> one further down, gotten from the layer returned by
> GetPrimaryScrollableLayer().
> 
> We then populate some fields of mPaintData based on the first FrameMetrics
> instance (mTransformScreenToLayout, mLayoutCriticalDisplayPort,
> mResolution), and others based on the second instance (mCompositionBounds).
> 
> Could these two FrameMetrics instances be different?
> 
> If yes, does it make sense to populate some of the fields of mPaintData
> based on one of them and some based on the other?
> 
> If not, why do we get them in different ways?

This is purposeful - for the transform, resolution and critical display port, we want those that apply to the layer in question - for the other metrics, we're interested in the top-level metrics, to see how the paint relates to the screen (this is documented, to some extent, in the paint data struct docs)

Comment 45

[Chris Lord [:cwiiis]]

Try run with a patch from jmaher to reduce the number of iterations of tsvgx: https://tbpl.mozilla.org/?tree=Try&rev=82e103a7a04d

Comment 46

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

Comment on attachment 8335250 [details] [diff] [review]
Fix tiled update data calculation / convert to typed units (v7, rebased)

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

Once again ignoring my misgivings about potential coordinate system conversion wonkiness.

::: gfx/layers/client/ClientTiledThebesLayer.cpp
@@ +64,5 @@
> +  // Get the metrics of the nearest scroll container.
> +  ContainerLayer* scrollParent = nullptr;
> +  for (ContainerLayer* parent = GetParent(); parent; parent = parent->GetParent()) {
> +    const FrameMetrics& metrics = parent->GetFrameMetrics();
> +    if (metrics.mScrollId != FrameMetrics::NULL_SCROLL_ID) {

I'd prefer metrics.IsScrollable() for readability.

::: gfx/layers/composite/ThebesLayerComposite.cpp
@@ +175,3 @@
>    for (ContainerLayer* parent = GetParent(); parent; parent = parent->GetParent()) {
>      const FrameMetrics& metrics = parent->GetFrameMetrics();
> +    if (metrics.mScrollId != FrameMetrics::NULL_SCROLL_ID) {

Ditto on IsScrollable()

Comment 47

[Botond Ballo [:botond]]

Comment on attachment 8335250 [details] [diff] [review]
Fix tiled update data calculation / convert to typed units (v7, rebased)

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

r+, once again with the caveat that I don't understand this code well enough to be able to verify the correctness of these calculations with any confidence.

::: gfx/layers/client/TiledContentClient.cpp
@@ +428,5 @@
>  {
> +  // Transform the current composition bounds into transformed layout device
> +  // space by compensating for the difference in resolution and subtracting the
> +  // old composition bounds origin.
> +  ScreenRect offsetViewportRect = (aCompositionBounds / aZoom) * aResolution;

Since we're changing terminology from "viewport" to "composition bounds", can we do it for this variable too?

::: gfx/layers/composite/ThebesLayerComposite.cpp
@@ +175,5 @@
>    for (ContainerLayer* parent = GetParent(); parent; parent = parent->GetParent()) {
>      const FrameMetrics& metrics = parent->GetFrameMetrics();
> +    if (metrics.mScrollId != FrameMetrics::NULL_SCROLL_ID) {
> +      return metrics.mZoom;
> +    }

It might be worth writing a helper function 'ContainerLayer* GetNearestScrollableLayer(Layer*)', as a loop of this form appears both here and in ClientTiledThebesLayer::BeginPaint.

::: layout/base/Units.h
@@ +157,5 @@
>  
> +  static nsIntRect ToUntyped(const LayoutDeviceIntRect& aRect) {
> +    return nsIntRect(aRect.x, aRect.y, aRect.width, aRect.height);
> +  }
> +

Slightly off-topic musing: if we added the FromUntyped/ToUntyped functions to BaseRect instead of the Pixel classes, could we avoid duplicating them for every Pixel class?

Comment 48

[Botond Ballo [:botond]]

Comment on attachment 8335250 [details] [diff] [review]
Fix tiled update data calculation / convert to typed units (v7, rebased)

Whoops, didn't meant to re-request review from Kats (my browser had been open to the review page since before his r+).

Comment 49

[Chris Lord [:cwiiis]]

(In reply to Botond Ballo [:botond] from comment #47)
> Comment on attachment 8335250 [details] [diff] [review]
> Fix tiled update data calculation / convert to typed units (v7, rebased)
> 
> Review of attachment 8335250 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+, once again with the caveat that I don't understand this code well enough
> to be able to verify the correctness of these calculations with any
> confidence.
> 
> ::: gfx/layers/client/TiledContentClient.cpp
> @@ +428,5 @@
> >  {
> > +  // Transform the current composition bounds into transformed layout device
> > +  // space by compensating for the difference in resolution and subtracting the
> > +  // old composition bounds origin.
> > +  ScreenRect offsetViewportRect = (aCompositionBounds / aZoom) * aResolution;
> 
> Since we're changing terminology from "viewport" to "composition bounds",
> can we do it for this variable too?

Well spotted, will do.

> ::: gfx/layers/composite/ThebesLayerComposite.cpp
> @@ +175,5 @@
> >    for (ContainerLayer* parent = GetParent(); parent; parent = parent->GetParent()) {
> >      const FrameMetrics& metrics = parent->GetFrameMetrics();
> > +    if (metrics.mScrollId != FrameMetrics::NULL_SCROLL_ID) {
> > +      return metrics.mZoom;
> > +    }
> 
> It might be worth writing a helper function 'ContainerLayer*
> GetNearestScrollableLayer(Layer*)', as a loop of this form appears both here
> and in ClientTiledThebesLayer::BeginPaint.

Sounds reasonable, I guess we'd add it to Layer.h?

> ::: layout/base/Units.h
> @@ +157,5 @@
> >  
> > +  static nsIntRect ToUntyped(const LayoutDeviceIntRect& aRect) {
> > +    return nsIntRect(aRect.x, aRect.y, aRect.width, aRect.height);
> > +  }
> > +
> 
> Slightly off-topic musing: if we added the FromUntyped/ToUntyped functions
> to BaseRect instead of the Pixel classes, could we avoid duplicating them
> for every Pixel class?

The untyped rects are specific to the type of rect it is I suppose (for LayoutDevice we want the ns types, for Layer and Screen, I guess we want either gfx or gfx::?) - ideally, at some point we'll have no untyped rects at all :)

Comment 50

[Chris Lord [:cwiiis]]

Pushed to inbound :O https://hg.mozilla.org/integration/mozilla-inbound/rev/2402eb839597

Comment 51

[Chris Lord [:cwiiis]]

Whoops, logged against the dup'd bug. Anyway, fixed:

https://hg.mozilla.org/mozilla-central/rev/2402eb839597

Comment 52

[Joshua Mitchell (Inactive)]

Issue is resolved - clearing old keywords - qa-wanted clean-up