Closed Bug 892684 Opened 6 years ago Closed 6 years ago

Work - Implement axis locking for panning in APZC

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: bbondy, Assigned: mbrubeck)

References

Details

Attachments

(3 files, 3 obsolete files)

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.
Blocks: metro-apzc
Summary: Implement axis locking for panning in APZC → Work -Implement axis locking for panning in APZC
Summary: Work -Implement axis locking for panning in APZC → Work - Implement axis locking for panning in APZC
Whiteboard: [preview]
No longer blocks: metro-apzc
Assignee: netzen → nobody
Blocks: 915328
Assignee: nobody → mbrubeck
Attached patch WIP (obsolete) — Splinter Review
This is the start of a direct port of the locking code from JavaPanZoomController.  Not yet working correctly.
Attached patch part 1: cleanupSplinter Review
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).
Attachment #812122 - Attachment is obsolete: true
Attachment #812187 - Flags: review?(bugmail.mozilla)
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.
Attachment #812188 - Flags: review?(bugmail.mozilla)
Status: NEW → ASSIGNED
Attachment #812187 - Flags: review?(bugmail.mozilla) → review+
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.
Attachment #812315 - Flags: review?(bugmail.mozilla)
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.
Attachment #812188 - Flags: review?(bugmail.mozilla)
Attachment #812188 - Flags: review?(botond)
Attachment #812188 - Flags: review+
Attachment #812315 - Flags: review?(bugmail.mozilla) → review+
Turning on axis locking on B2G and attempting to scroll results in a crash. Stack attached.
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.
Attachment #812188 - Flags: review+ → review-
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.
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
Attachment #812188 - Attachment is obsolete: true
Attachment #812315 - Attachment is obsolete: true
Attachment #812188 - Flags: review?(botond)
Attachment #812966 - Flags: review?(bugmail.mozilla)
Attachment #812966 - Flags: review?(botond)
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.
Attachment #812966 - Flags: review?(bugmail.mozilla) → review+
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.
Attachment #812966 - Flags: review?(botond) → review+
Thanks, guys! Pushed to Try again with review comments addressed:
https://tbpl.mozilla.org/?tree=Try&rev=e57ff24df4f8
https://hg.mozilla.org/mozilla-central/rev/9fa7b7e3138b
https://hg.mozilla.org/mozilla-central/rev/15da9a9ae411
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
No longer blocks: metrov2defect&change, 915328
Whiteboard: [preview]
You need to log in before you can comment on or make changes to this bug.