923431 - Axis::ScaleWillOverscroll contains a unit mismatch

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

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

Botond said in bug 915831 comment 8:

(In reply to Botond Ballo [:botond] from comment #8)
> While reading some of the code relevant to these patches, I came across
> something suspicious:
> 
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/Axis.cpp#257
> adds GetOrigin(), which is a coordinate of FrameMetrics::mScrollOffset,
> which is a CSSPoint, and aFocus, which
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/
> AsyncPanZoomController.cpp#583 initializes with a coordinate of a
> ScreenPoint.
> 
> Do we have a units mismatch here?

Yes, this does look like a units mismatch. I took a deeper look the code that controls overscroll and ScaleWithFocus and it looks like there are a few things there that could be improved and tested. Filing this bug for cleaning all that up. Time for some test-driven development!

Comment 1

[Botond Ballo [:botond]]

To avoid unit mismatches like this in the future, what do you think about introducing a Coord class that wraps a float and is templated on a pixel type? I think this would be very useful in Axis.

It can be introduced gradually, with no changes to other code (like Point) at first (i.e. initally we'd just unwrap two Coords to construct a Point).

Comment 2

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

We discussed this offline but for bug posterity I'm in favour of doing this. Botond filed bug 923512 for this.

Comment 3

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

Attachment: Part 1 - test for scroll offset adjustment

Comment 4

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

Attachment: Part 2 - fix the code

This makes the test in part 1 pass but I haven't tried it on a device yet. Will do that and then add some more tests.

Comment 5

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

Attachment: Fix scroll offset adjustment when pinching

Squashed the patches together since they don't really make sense standalone. I also added a test for pinching while in a corner and fixed up the code for that. Works fine on my Geeksphone Peak.

Comment 6

[Botond Ballo [:botond]]

Comment on attachment 814506 [details] [diff] [review]
Fix scroll offset adjustment when pinching

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

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +855,5 @@
>  }
>  
>  void AsyncPanZoomController::ScaleWithFocus(const CSSToScreenScale& aZoom,
> +                                            const CSSPoint& aFocus) {
> +  gfx::ScaleFactor<CSSPixel, CSSPixel> zoomFactor(aZoom.scale / mFrameMetrics.mZoom.scale);

The argument passed for 'aZoom' by OnScale() (the only call site) is obtained by multiplying something by mFrameMetrics.mZoom. Can we simplify this?

Also, can we introduce a CSSToCSSScale typedef for consistency?

@@ +860,4 @@
>    SetZoomAndResolution(aZoom);
>  
>    // If the new scale is very small, we risk multiplying in huge rounding
>    // errors, so don't bother adjusting the scroll offset.

This comment referred to an 'if' which this patch removed. We should either remove the comment too, or reinstate the 'if'.

@@ +860,5 @@
>    SetZoomAndResolution(aZoom);
>  
>    // If the new scale is very small, we risk multiplying in huge rounding
>    // errors, so don't bother adjusting the scroll offset.
> +  mFrameMetrics.mScrollOffset = (mFrameMetrics.mScrollOffset + aFocus) - (aFocus / zoomFactor);

It took me quite a while to convince myself that this is correct. Here is the model that I came up with:

  - At any point in time T, let
      - scrollOffset(T) be the scroll offset
      - relativeFocus(T) be the focus point relative to the scroll offset
      - focus(T) be the focus point relative to the document origin
    (all in CSS coordinates).
  - For any point in time T, the relationship 
      focus(T) = scrollOffset(T) + relativeFocus(T) 
    holds.
  - Let A be the point in time before the scale, and B the point in time after.
  - We want the focus point relative to the document origin to stay constant
    across the scale. That is, we want
      focus(A) = focus(B)
    which means we want
      scrollOffset(A) + relativeFocus(A) = scrollOffset(B) + relativeFocus(B)
    which can be rearranged to 
      scrollOffset(B) = scrollOffset(A) + relativeFocus(A) - relativeFocus(B).
  - Mapping these to actual variable names:
      - scrollOffset(A) is the old value of mFrameMetrics.mScrollOffset
      - scrollOffset(B) is the new value of mFrameMetrics.mScrollOffset
      - relativeFocus(A) is aFocus
      - relativeFocus(B) is aFocus / zoomFactor
    and we get exactly this line of code.

Can we say this in a comment, or perhaps just add a link to this bugzilla comment, so that people reading this code in the future don't have to reconstruct this model?

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +360,5 @@
>     * Scales the viewport by an amount (note that it multiplies this scale in to
>     * the current scale, it doesn't set it to |aScale|). Also considers a focus
>     * point so that the page zooms outward from that point.
>     *
>     * XXX: Fix focus point calculations.

Does this patch resolve this XXX, or is there more that remains to be fixed?

::: gfx/layers/ipc/Axis.cpp
@@ +252,5 @@
>    default: return 0;
>    }
>  }
>  
>  Axis::Overscroll Axis::ScaleWillOverscroll(ScreenToScreenScale aScale, float aFocus) {

Now that aFocus is in CSS coordinates, should aScale be a CSSToCSSScale?

@@ +258,2 @@
>  
>    bool both = ScaleWillOverscrollBothSides(aScale);

Out of curiosity, do you know why we need the ScaleWillOverscrollBothSides function? Does it catch some cases that the 'minus' and 'plus' checks below miss?

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +186,5 @@
> +  EXPECT_EQ(fm.mZoom.scale, 1.0f);
> +  EXPECT_EQ(fm.mScrollOffset.x, 880);
> +  EXPECT_EQ(fm.mScrollOffset.y, 0);
> +}
> +

Nice test!

Comment 7

[Jim Mathies [:jimm]]

Comment on attachment 814506 [details] [diff] [review]
Fix scroll offset adjustment when pinching

This doesn't seem to help any of the origin issues we have on metro, but also it doesn't seem to hurt, so.. :)

Comment 8

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

Attachment: Fix scroll offset adjustment when pinching (v2)

After debating with myself for a while I decided to replace all the ScreenToScreenScale instances with just a regular float. I don't think the ScreenToScreenScale was really buying us anything because we had to extract the .scale most of the time anyway, and it just got in the way when trying to scale a CSS value. I incorporated all the other review comments as well.

(In reply to Botond Ballo [:botond] from comment #6)
> Out of curiosity, do you know why we need the ScaleWillOverscrollBothSides
> function? Does it catch some cases that the 'minus' and 'plus' checks below
> miss?

In a world where overscroll is allowed (such as in Fennec where this code originated), it is possible to pinch-zoom out on the page so that it is in OVERSCROLL_BOTH, but then pan so that it is clipped by the screen. If, for example, you move the page to the left so that the left edge of the page is clipped, it is technically no longer in "minus" overscroll, even though it is still in "both" overscroll.

Comment 9

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

Attachment: Part 2 - Kill ScreenToScreenScale entirely

I don't believe having the ScreenToScreenScale provides much value at all so this gets rid of it completely.

Comment 10

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

https://hg.mozilla.org/integration/b2g-inbound/rev/e7cab9a2279f
https://hg.mozilla.org/integration/b2g-inbound/rev/ca931d9d940c

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/e7cab9a2279f
https://hg.mozilla.org/mozilla-central/rev/ca931d9d940c