1076163 - (apz-css-trans-stage1) Include the css-driven resolution in FrameMetrics::mCumulativeResolution

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

This is Stage 1 of the apz-css-transforms work. Quoting from https://bugzilla.mozilla.org/show_bug.cgi?id=993525#c3:

  1) The resolution induced by a CSS scale transform on a scrollable layer or any 
     ancestor layer thereof (henceforth, "css-driven resolution") is not taken into 
     account by anything in the FrameMetrics of that scrollable layer. In particular, 
     I believe it should be included in FrameMetrics::mCumulativeResolution.

Comment 1

[Botond Ballo [:botond]]

Just wanted to give a progress update on this: 

As I was implementing this, I realized that it has implications for what coordinate system we store the display port in. We currently store it in Layer pixels, but insofar as our intention is to keep the size of the displayport as a fraction of the screen size constant, this doesn't reflect our intention accurately: since Layout can choose to render a layer at a resolution different from what would give Layer pixels a 1-to-1 correspondence with Screen pixels, the same number of Layer pixels can represent a different fraction of the screen size depending on this choice.

I discussed this with Timothy, and we believe we should instead be storing the display port in Screen pixels. I'll do that as part of the changes in this bug.

Comment 2

[Botond Ballo [:botond]]

Attachment: Part 1 - Include the css-driven resolution in FrameMetrics::mCumulativeResolution, and store the displayport in Screen pixels

This patch implements the approach that Timothy and I have been discussing.

Conceptually, the patch makes two distinct changes:

  1) Including the css-driven resolution in FrameMetrics::mCumulativeResolution.

  2) Storing the displayport in Screen pixels rather than Layer pixels.

I don't think the patch can effectively be split into two parts along those lines, however, becaues the two are intertwined. Doing just (1) would have broken the storing and retrieving of displayports (because we retrieve it during display-list building, when we don't know the full css-driven resolution yet) without writing a bunch of code that (2) makes unnecessary; doing just (2) wouldn't have worked because FrameMetrics doesn't have enough information to convert to Screen pixels without (1).

So, I'm posting it in one part.

Comment 3

[Botond Ballo [:botond]]

This Part 1 patch contains all the functional changes. Part 2 will be a pure refactoring that renames FrameMetrics::mResolution to mPresShellResolution and removes FrameMetrics::GetParentResolution() to avoid confusion.

Comment 4

[Botond Ballo [:botond]]

Manual testing of this patch looks good so far.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=3dcc1ada1c3c

Comment 5

[Botond Ballo [:botond]]

Comment on attachment 8502109 [details] [diff] [review]
Part 1 - Include the css-driven resolution in FrameMetrics::mCumulativeResolution, and store the displayport in Screen pixels

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

I think this patch deserves some additional explanation.

We can view the "full resolution" which converts from LayoutDevice pixels to the actual Layer pixels that we draw, as consisting of three components:

  (1) The cumulative pres-shell resolution.
      This is calculated by nsIPresShell::GetCumulativeResolution().

  (2) The resolution corresponding to the scale of any css transforms the apply to the layer.
      This is calculated by nsLayoutUtils::GetTransformToAncestorScale(scrollFrame, nullptr);

  (3) Any 'extra' resolution beyond (1) and (2) at which Layout chooses to render the layer.
      This is introduced here [1], and, AFAIU, serves the purpose of reducing the number of 
      times we repaint at a new resolution when the resolution is changing (e.g. being animated).

With this patch:

  - FrameMetrics::mCumulativeResolution stores (1) * (2) * (3)

      - except when we set it in CalculateFrameMetricsForDisplayPort(), where we use (1) * (2)
        because the logic to determine (3) hasn't run yet; however, this is OK

      - When calculating this in ComputeFrameMetrics(), aContainerParameters.m[X|Y]Scale
        contains (1) * (2) * (3).

  - FrameMetrics::mExtraResolution stores (3)

      - Calculated in ComputeFrameMetrics() by dividing aContainerParameters.m[X|Y]Scale,
        by (1) * (2)

  - The displayport is stored in LayoutDevice * (1) * (2). I believe this corresponds to
    Screen pixels, disregarding any transient async zoom, because:

      - any extra resolution is cancelled by an extra pre-scale

      - otherwise, pre-scales and base transforms above this cancel out, as do the post-scale
        and the non-transient async transform's scale

    (Note: this "correspondence" is in terms of *size* only. There can be rotation transforms
    and such between this space and true Screen space, but those don't affect the size of
    things.)

      - I accordingly call a method in FrameMetrics that returns device-scale * (1) * (2)
        DisplayportPixelsPerToCSSPixel(). Feel free to bikeshed this name.


[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp?rev=c0e4ec5fc709#3808

Comment 6

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

Comment on attachment 8502109 [details] [diff] [review]
Part 1 - Include the css-driven resolution in FrameMetrics::mCumulativeResolution, and store the displayport in Screen pixels

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

This seems reasonable enough. My only major concern is calling the displayport space "ScreenPixels" even though it doesn't include the transient async transform. It's certainly not global screen pixels, so I would rather come up with a different name for it. Maybe DisplayportPixels?

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +180,3 @@
>     * The base rect values are in app units.
>     */
>    void setDisplayPortMarginsForElement(in float aLeftMargin,

Fennec calls this function too, so make sure this change doesn't regress anything there.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +2282,2 @@
>  
>    return layerMargins;

inline the expression into the return. Having a local variable called "layerMargins" doesn't help much here.

@@ +2353,5 @@
>  
>    // If we're trying to paint what we already think is painted, discard this
>    // request since it's a pointless paint.
> +  ScreenMargin marginDelta = (mLastPaintRequestMetrics.GetDisplayPortMargins()
> +                          - aFrameMetrics.GetDisplayPortMargins());

nit: indent the second line by a space to maintain alignment

Comment 7

[Botond Ballo [:botond]]

Attachment: Part 1 - Include the css-driven resolution in FrameMetrics::mCumulativeResolution, and store the displayport in Screen pixels

Updated patch to address review comments.

While investigating Fennec's use of setDisplayPortMarginsForElement() (which appears to not need any changes), I discovered a pre-existing issue about how we store displayports: when requesting a content repaint, we calculate the display port in CSS pixels based on the composited size in CSS pixels which takes into account the async zoom [1], but then convert it to Layer pixels [2] which do not take the async zoom into account. This results in a displayport that's not comparable to the screen size like we want it to be, even with the adjustment for mExtraResolution that the original patch introduced.

To account for this, I modified the code to take the async zoom into account when converting the displayport to Screen (formerly Layer) pixels.

In light of this change, Kats and I agree that ScreenPixel is now an appropriate name for the coordinate space the displayport is stored in, so I did not introduce a DisplayportPixel.

Carrying r+ from Kats. I would still like a review from Timothy.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp#2244
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp#2281

Comment 8

[Botond Ballo [:botond]]

Another Try push, that includes Talos tests for Fennec, to make sure we didn't regress anything.

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

Comment 9

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #8)
> Another Try push, that includes Talos tests for Fennec, to make sure we
> didn't regress anything.
> 
> https://tbpl.mozilla.org/?tree=Try&rev=b5cf31ec101c

Looks green, including Talos tests!

Comment 10

[Botond Ballo [:botond]]

Attachment: Part 2 - Clean up resolution-related fields and methods in FrameMetrics

This patch does some cleanup as a follow-up to Part 1:

  - Renames FrameMetrics::mResolution to mPresShellResolution.

  - Revises the comments above FrameMetrics::mPresShellResolution 
    and FrameMetrics::mCumulativeResolution.

  - Removes FrameMetrics::GetParentResolution(), which no longer
    make much sense as a conceptual quantity in light of the
    revised meaning of mCumulativeResolution. 

    The call sites still compute the same quantity numerically,
    but express the computation in a different way, using a new
    helper FrameMetrics::GetAsyncZoom(). I believe this refactoring
    increases the clarity of the call sites.

Comment 11

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

Comment on attachment 8505779 [details] [diff] [review]
Part 2 - Clean up resolution-related fields and methods in FrameMetrics

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

r=me with comments addressed as indicated. If I'm wrong on either of them let's discuss further.

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +191,2 @@
>      visible = visible.Intersect(touchSensitiveRegion
> +                                * aMetrics.GetZoomToParent());

This change I think returns a different value numerically. The old value here didn't include resolution on the layer itself but the new one does. I think what you want here is:
touchSensitiveRegion * aMetrics.LayersPixelsPerCSSPixel() / aMetrics.mPresShellResolution

There's a similar conversion that you did in AsyncCompositionManager::TransformScrollableLayer except there the conversion was mPresShellResolution/LayersPixelsPerCSSPixel()

::: gfx/layers/client/TiledContentClient.cpp
@@ +1309,5 @@
>    }
>  
>  #ifdef GFX_TILEDLAYER_DEBUG_OVERLAY
> +  DrawDebugOverlay(drawTarget, aTileOrigin.x * mPresShellResolution,
> +                   aTileOrigin.y * mPresShellResolution, GetTileLength(), GetTileLength());

The mResolution in this block is not the FrameMetrics one, and should stay as mResolution.

Comment 12

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #11)
> Comment on attachment 8505779 [details] [diff] [review]
> Part 2 - Clean up resolution-related fields and methods in FrameMetrics
> 
> Review of attachment 8505779 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comments addressed as indicated. If I'm wrong on either of them
> let's discuss further.
> 
> ::: gfx/layers/apz/src/APZCTreeManager.cpp
> @@ +191,2 @@
> >      visible = visible.Intersect(touchSensitiveRegion
> > +                                * aMetrics.GetZoomToParent());
> 
> This change I think returns a different value numerically. The old value
> here didn't include resolution on the layer itself but the new one does. I
> think what you want here is:
> touchSensitiveRegion * aMetrics.LayersPixelsPerCSSPixel() /
> aMetrics.mPresShellResolution

I think it doesn't matter, as this is only actually used for the keyboard app, which is not zoomed.

Quoting from bug 935219, comment 76:

  "touchSensitiveRegion ultimately comes from layout code, where I find it hard 
   to reason about coordinate systems, but since the only place I'm aware of a 
   touch-sensitive region being used right now is for the keyboard app, and that 
   cannot be zoomed, M's CSS pixels are the same as L's CSS pixels and it does 
   not matter.

   Since we are about to get rid of GetTouchSensitiveRegion() when we implement 
   bug 918288, I think this is good enough for now."

That said, I'm happy to change it to

    touchSensitiveRegion 
  * aMetrics.LayersPixelsPerCSSPixel() 
  / aMetrics.mPresShellResolution

if you prefer.

> There's a similar conversion that you did in
> AsyncCompositionManager::TransformScrollableLayer except there the
> conversion was mPresShellResolution/LayersPixelsPerCSSPixel()

Assuming you're talking about this hunk:

  -  ParentLayerToScreenScale scale = userZoom
  -                                  / metrics.mDevPixelsPerCSSPixel
  -                                  / metrics.GetParentResolution();
  +
  +  LayerToScreenScale asyncZoom = userZoom / metrics.LayersPixelsPerCSSPixel();
  +  ParentLayerToScreenScale scale = metrics.mPresShellResolution
  +                                 * asyncZoom;

I don't think it's similar; the resulting scale includes the layer's own resolution both before and after.

> ::: gfx/layers/client/TiledContentClient.cpp
> @@ +1309,5 @@
> >    }
> >  
> >  #ifdef GFX_TILEDLAYER_DEBUG_OVERLAY
> > +  DrawDebugOverlay(drawTarget, aTileOrigin.x * mPresShellResolution,
> > +                   aTileOrigin.y * mPresShellResolution, GetTileLength(), GetTileLength());
> 
> The mResolution in this block is not the FrameMetrics one, and should stay
> as mResolution.

Heh. I used Eclipse's rename refactoring on FrameMetrics::mResolution; looks like it got a bit overeager here.

Comment 13

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

(In reply to Botond Ballo [:botond] from comment #12)
> I think it doesn't matter, as this is only actually used for the keyboard
> app, which is not zoomed.

Ah, good point.

> That said, I'm happy to change it to
> 
>     touchSensitiveRegion 
>   * aMetrics.LayersPixelsPerCSSPixel() 
>   / aMetrics.mPresShellResolution
> 
> if you prefer.

Yeah I think it would be safer to do it this way, even if it doesn't end up mattering in practice.

> > There's a similar conversion that you did in
> > AsyncCompositionManager::TransformScrollableLayer except there the
> > conversion was mPresShellResolution/LayersPixelsPerCSSPixel()
> 
> Assuming you're talking about this hunk:
> 
>   -  ParentLayerToScreenScale scale = userZoom
>   -                                  / metrics.mDevPixelsPerCSSPixel
>   -                                  / metrics.GetParentResolution();
>   +
>   +  LayerToScreenScale asyncZoom = userZoom /
> metrics.LayersPixelsPerCSSPixel();
>   +  ParentLayerToScreenScale scale = metrics.mPresShellResolution
>   +                                 * asyncZoom;
> 
> I don't think it's similar; the resulting scale includes the layer's own
> resolution both before and after.

What I meant was that in this hunk you're taking some of the form:
  foo / (mDevPixelsPerCSSPixel * GetParentResolution())
and converting it to the form:
  foo / (LayersPixelsPerCSSPixel() / mPresShellResolution)
which, given your "compute the same quantity numerically" in comment 10 implies that:
  (mDevPixelsPerCSSPixel * GetParentResolution()) === (LayersPixelsPerCSSPixel() / mPresShellResolution)
and which I agree.

My point was that therefore, the touchSensitiveRegion hunk, which also contains the quantity "mDevPixelsPerCSSPixel * GetParentResolution()" should be converted using the same equality and using GetZoomToParent() which is not always the same.

Comment 14

[Botond Ballo [:botond]]

Attachment: addendum to Part 1

I realized that Part 1 was missing some things related to the calculation of root composition size. This pach adds those things. I'll fold it into the Part 1 patch before landing, but I'm posting it separately for easier review:

  - First, CalculateRootCompositionSize() and
    CalculateFrameMetricsForDisplayPort() are still using the parent
    resolution instead of the cumulative resolution for calculating
    the composition bounds / size. This was changed in bug 1000423
    in RecordFrameMetrics(), but no corresponding change was made
    to these other functions.

  - Second, in calculating the root frame's composition bounds in
    a target frame's CSS pixels, CalculateRootCompositionSize()
    uses Layer pixels to interface between the two layers. Since
    the purpose of the root composition size is to bound the
    displayport size, I think it's more appropriate to use Screen
    pixels here.

Comment 15

[Botond Ballo [:botond]]

Attachment: Part 2 - Clean up resolution-related fields and methods in FrameMetrics

Addressed review comments for Part 2. Carrying r+ from Kats.

Comment 16

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

Comment on attachment 8507056 [details] [diff] [review]
addendum to Part 1

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

I don't know if I can meaningfully r+ this. Changing to f+ and deferring to tn. I do have one question, though: does this mean the rootCompositionSize in the FrameMetrics has been wrong for a while? Might this have caused bug 1080205 or similar issues? If this is an actual correction it would be good to land it separately in case it needs to be uplifted to fix some last-minute 2.1 blocker or something.

Comment 17

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #16)
> does this mean the rootCompositionSize
> in the FrameMetrics has been wrong for a while? 

On account of the parent resolution being used instead of the cumulative resolution, yes. The other changes are specific to CSS transforms.

> Might this have caused bug 1080205 or similar issues? 

I still see bug 1080205 with this patch applied, but it's possible that this has caused other issues.

> If this is an actual correction it would be good
> to land it separately in case it needs to be uplifted to fix some
> last-minute 2.1 blocker or something.

That sounds reasonable. I'll split out the parent resolution -> cumulative resolution changes to a separate patch.

Comment 18

[Botond Ballo [:botond]]

Attachment: Part 2 - Clean up resolution-related fields and methods in FrameMetrics

Updated Part 2 patch to reinstate a comment explaining the conversion of the touch-sensitive region to layer pixels (and updated the explanation to account for css-driven resolution). Carrying r+.

Comment 19

[Botond Ballo [:botond]]

Attachment: addendum to Part 1

(In reply to Botond Ballo [:botond] from comment #17)
> That sounds reasonable. I'll split out the parent resolution -> cumulative
> resolution changes to a separate patch.

Split out to bug 1085569. 

Updated this 'addendum' patch to apply on top of that, and contain only the {CSS transforms}-related changes to CalculateRootCompositionSize().

Comment 20

[Botond Ballo [:botond]]

Attachment: Part 2 - Clean up resolution-related fields and methods in FrameMetrics

Updated Part 2 patch to fix Android bustage. Carrying r+.

Comment 21

[Botond Ballo [:botond]]

Attachment: addendum to Part 1

Rebased 'addendum to part 1' patch to apply on top of the updated patch in bug 1085569.

Comment 22

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8505632 [details] [diff] [review]
Part 1 - Include the css-driven resolution in FrameMetrics::mCumulativeResolution, and store the displayport in Screen pixels

We talked about how alignment should still be in layer pixels on irc.

To be 100% correct GetTransformToAncestorScale should probably get the transform to the nearest display root. You can use nsLayoutUtils::GetDisplayRootFrame for that.

Otherwise looks good.

Comment 23

[Botond Ballo [:botond]]

Attachment: Part 1 - Include the css-driven resolution in FrameMetrics::mCumulativeResolution, and store the displayport in Screen pixels

Addressed review comments for Part 1.

I took the opportunity to make the scales in GetDisplayPortFromMarginsData strongly-typed. I realize this means that some width/height scales got turned into just width, but that's all going to be fixed in bug 1036967 anyways.

I also extracted out some helpers for the app units <--> LayoutDevice conversions.

Comment 24

[Botond Ballo [:botond]]

Attachment: Part 1 - Include the css-driven resolution in FrameMetrics::mCumulativeResolution, and store the displayport in Screen pixels

As discussed on IRC, the previous changes were misguided: while we do ideally want to apply the alignment in Layer space, the space the previous patch used was not Layer space, and we don't have enough information to determine the actual Layer space. 

Screen space is a better approximation to the actual Layer space than the space used in the previous patch, so I changed the patch back to apply the alignment in Layer space, while keeping the strong-typing improvements and adding a clarifying comment.

Comment 25

[Botond Ballo [:botond]]

The push of the patches in this bug + bug 1055741: https://tbpl.mozilla.org/?tree=Try&rev=64cb0111f38c

Comment 26

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #25)
> The push of the patches in this bug + bug 1055741:

Er, *Try* push

Comment 27

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #25)
> Try push of the patches in this bug + bug 1055741:
> https://tbpl.mozilla.org/?tree=Try&rev=64cb0111f38c

Clean! Landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/fbb9dc943109
https://hg.mozilla.org/integration/mozilla-inbound/rev/71fe4233208a

Comment 28

[Carsten Book [:Tomcat]]

Sorry had to back this change out in https://treeherder.mozilla.org/ui/#/jobs?repo=mozilla-inbound&revision=8cb112e23067 since this changes from this bug or bug 1055741 caused Android 4.0 M2 Crashes like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3849267&repo=mozilla-inbound

Comment 29

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

The crash appears to be bug 1021367 (Botond helpfully starred the perma-fail on his try push with this bug). I've pushed a try run with the patch on that bug to see if it fixes the problem: https://tbpl.mozilla.org/?tree=Try&rev=c475291bd4c5

I also requested review on that patch so it can land (and then I can re-land this on top of it). I'll also fix the build bustage these patches caused on re-landing.

Comment 30

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

Try is green. I already landed bug 1021367. Re-landing this on top with a one-line change to LayerManagerComposite so it doesn't burn on android:

https://hg.mozilla.org/integration/mozilla-inbound/rev/248c057bbfca
https://hg.mozilla.org/integration/mozilla-inbound/rev/9106c8d44533

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/248c057bbfca
https://hg.mozilla.org/mozilla-central/rev/9106c8d44533