892684 - Work - Implement axis locking for panning in APZC

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Brian R. Bondy [:bbondy]]

When panning up and down a page, it is very jittery left to right (within a few pixels).  Before we enable APZC on Metro we need to fix this.  To do this kats previously mentioned we should implement axis locking in gfx code.

From kats:
The Java code starts at http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/JavaPanZoomController.java#153 - look through the rest of the file for the mMode variable and you'll see the rest of it.

Comment 1

[Matt Brubeck (:mbrubeck)]

Attachment: WIP

This is the start of a direct port of the locking code from JavaPanZoomController.  Not yet working correctly.

Comment 2

[Matt Brubeck (:mbrubeck)]

Attachment: part 1: cleanup

This is just some minor cleanup to reduce some boilerplate by changing GetFirstSingleTouch to return a ScreenIntPoint directly.  This is totally optional; it just makes the next patch a bit less verbose.  If you'd rather leave this as-is, that's fine.

This also fixes the Vim modeline comments to set 2-space indents like the Emacs ones (and the actual file indentation).

Comment 3

[Matt Brubeck (:mbrubeck)]

Attachment: part 2: port axis locking code from Android

This is a direct port of all of the axis locking logic from JavaPanZoomController.  It is disabled by default, so B2G won't be affected until they opt in.

Comment 4

[Matt Brubeck (:mbrubeck)]

Attachment: part 3: fix some remaining uses of PANNING

I just noticed a couple of lines I missed in part 2.  Uploading this separately for review purposes, but I'll fold it into part 2 before pushing.

Comment 5

[Matt Brubeck (:mbrubeck)]

Pushed to Try:
https://tbpl.mozilla.org/?tree=Try&rev=39c5fa32bb6f

Comment 6

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

Comment on attachment 812188 [details] [diff] [review]
part 2: port axis locking code from Android

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

This looks ok to me, but I'd like to have Botond take a quick look at this as well to make sure it won't interfere with his overscroll handoff code. That code is new to APZC and doesn't have any equivalent in the JavaPanZoomController so I'm not sure if there will be tricky interactions there.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +741,5 @@
> +  mX.StartTouch(point.x);
> +  mY.StartTouch(point.y);
> +  mLastEventTime = aEvent.mTime;
> +
> +  AxisLockMode mode = static_cast<AxisLockMode>(gAxisLockMode);

I'd prefer to add a private getAxisLockMode() function that returns a AxisLockMode, so as to hide the static_cast.

Comment 7

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

Attachment: Crash stack on B2G when axis locking is turned on

Turning on axis locking on B2G and attempting to scroll results in a crash. Stack attached.

Comment 8

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

Comment on attachment 812188 [details] [diff] [review]
part 2: port axis locking code from Android

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

::: gfx/layers/ipc/Axis.cpp
@@ +109,5 @@
>    InitAxisPrefs();
>  }
>  
>  void Axis::UpdateWithTouchAtDevicePoint(int32_t aPos, const TimeDuration& aTimeDelta) {
> +  float newVelocity = Scrollable() ? (mPos - aPos) / aTimeDelta.ToMilliseconds() : 0;

Actually this call to Scrollable() is happening even if axis lock is turned off, and so crashes debug builds regardless of the pref. It should crash metro debug builds too, I think. Minusing for now.

Comment 9

[Matt Brubeck (:mbrubeck)]

Comment on attachment 812790 [details]
Crash stack on B2G when axis locking is turned on

>#4  0x41b8da90 in mozilla::ReentrantMonitor::AssertCurrentThreadIn
>#5  mozilla::layers::AsyncPanZoomController::GetFrameMetrics
>#6  0x41b90ae8 in mozilla::layers::Axis::GetCompositionLength
>#7  0x41b90d90 in mozilla::layers::Axis::Scrollable
>#8  0x41b911b4 in mozilla::layers::Axis::UpdateWithTouchAtDevicePoint

So I guess I need to make sure anything that ends up calling Axis::Scrollable is protected by a ReentrantMonitorAutoEnter, right?

We could also avoid it in this particular case by using mScrollingDisabled instead of Scrollable.

I'm updating my debug Windows build to see if I can reproduce the assertion there.

Comment 10

[Matt Brubeck (:mbrubeck)]

Attachment: part 2: port axis locking code from Android (v2)

This folds together the old "part 2" and "part 3" patches, addresses review comments, and fixes the assertion.

If you want to see an interdiff of the assertion fix alone, it is:
https://hg.mozilla.org/users/mbrubeck_mozilla.com/elm-patches/file/899c5789fafa/scrollable-assertion

Comment 11

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

Comment on attachment 812966 [details] [diff] [review]
part 2: port axis locking code from Android (v2)

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

LGTM. I tested it on B2G and it doesn't crash any more. With axis locking turned on it's pretty hard to not do a pan that's not axis-locked, so that might need some tuning but we can do that if/when we turn it on on B2G by default.

Comment 12

[Botond Ballo [:botond]]

Comment on attachment 812966 [details] [diff] [review]
part 2: port axis locking code from Android (v2)

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

Looks good, doesn't seem to interfere with overscroll handoff. r=me with the requested documentation change.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +758,5 @@
>    angle = fabs(angle); // range [0, pi]
>  
> +  if (!mX.Scrollable() || !mY.Scrollable()) {
> +    SetState(PANNING);
> +  } else if (angle < AXIS_LOCK_ANGLE || angle > (M_PI - AXIS_LOCK_ANGLE)) {

Suggestion: factor these comparisons out into a function like IsCloseToHorizontal(angle) (and likewise for the vertical case).

::: gfx/layers/ipc/Axis.cpp
@@ +141,5 @@
> +  if (mScrollingDisabled) {
> +    aOverscrollAmountOut = 0;
> +    return 0;
> +  }
> +

Please update the comment describing what AdjustDisplacement() does (in Axis.h) to mention that it adjusts for axis locking too.

Comment 13

[Matt Brubeck (:mbrubeck)]

Thanks, guys! Pushed to Try again with review comments addressed:
https://tbpl.mozilla.org/?tree=Try&rev=e57ff24df4f8

Comment 14

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/integration/fx-team/rev/9fa7b7e3138b
https://hg.mozilla.org/integration/fx-team/rev/15da9a9ae411

Comment 15

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/9fa7b7e3138b
https://hg.mozilla.org/mozilla-central/rev/15da9a9ae411

Comment 16

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #11)
> With axis locking
> turned on it's pretty hard to not do a pan that's not axis-locked

Whoops, too many negatives. I meant to say, "With axis locking turned on, it's hard to do a pan that's not axis locked". i.e. it's hard to hit that narrow pan angle window around 45deg where you don't get axis locked.