Closed Bug 1229462 Opened 4 years ago Closed 4 years ago

Use Android Scroller class for fling animation

Categories

(Firefox for Android :: Toolbar, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 49
Tracking Status
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: snorp, Assigned: rbarker)

References

Details

Attachments

(2 files, 6 obsolete files)

Sometimes we get complaints that Fennec panning physics does not match other apps on Android. We should try to use the same friction and maximum velocity to try to be closer there. It looks like the ViewConfiguration[0] class has some of that info exposed.

[0] https://developer.android.com/reference/android/view/ViewConfiguration.html#getScrollFriction%28%29
Looking at Scroller.java[0], I'm not sure if our stuff in APZ really matches that enough to just use the same constants. We should probably just look into using Scroller.java directly...

[0] http://androidxref.com/6.0.0_r1/xref/frameworks/base/core/java/android/widget/Scroller.java
May I suggest a demo page?  Comparing Fennec to Chrome on Android I can get to the bottom in one fling on Chrome and the least flings for Fennec is 5 for me.

http://www.html5rocks.com/static/demos/scrolling/demo.html
Duplicate of this bug: 933515
Any possibility if this could be prioritized higher? 
It would be great if it lands in the current Nightly versions.
Summary: Use system values for APZ physics → Use Android Scroller class for fling animation
(In reply to bull500 from comment #4)
> Any possibility if this could be prioritized higher? 
> It would be great if it lands in the current Nightly versions.

Randall is working on it, but we don't have a good idea yet of how well it's going to work.
Assignee: nobody → rbarker
Comment on attachment 8749743 [details] [diff] [review]
0001-Bug-1229462-Use-Android-OverScroller-class-for-fling-animation-16050611-12cb5ff.patch

Could you please move the new class you're adding to it's own file? See WheelScrollAnimation.* for an example.
Attachment #8749894 - Flags: review?(botond)
Comment on attachment 8749894 [details] [diff] [review]
0001-Bug-1229462-Use-Android-OverScroller-class-for-fling-animation-16050616-de5c233.patch

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

Looks generally good; I have a few comments. I'd also like Kats to weigh in on the vsync / timestamp issue.

::: gfx/layers/apz/src/FlingOverScrollerAnimation.cpp
@@ +15,5 @@
> +namespace layers {
> +
> +// Value used in boundary detection. Due to round off error,
> +// assume getting within 5 pixes of the boundary is close enough.
> +static const float BOUNDS_EPSILON = 5.0f;

This seems like a very high epsilon. Have you found it's necessary for it to be this high?

@@ +55,5 @@
> +    mApzc.mY.SetVelocity(0);
> +  }
> +
> +  ParentLayerPoint velocity = mApzc.GetVelocityVector();
> +  float magnitude = sqrtf((velocity.x * velocity.x) + (velocity.y * velocity.y));

Use |velocity.Length()|.

@@ +62,5 @@
> +      mFlingDirection.y = velocity.y / magnitude;
> +  }
> +
> +  mApzc.mLastFlingTime = now;
> +  mApzc.mLastFlingVelocity = velocity;

These are only used by FlingAnimation, so there's not much point in setting them here.

@@ +69,5 @@
> +  mStartOffset.y = mPreviousOffset.y = mApzc.mY.GetOrigin().value;
> +  mOverScroller->Fling((int32_t)mStartOffset.x, (int32_t)mStartOffset.y,
> +                       // Android needs the velocity in pixels per second and it is in pixels per ms.
> +                       (int32_t)(velocity.x * 1000.0f), (int32_t)(velocity.y * 1000.0f),
> +                       (int32_t)mApzc.mX.GetPageStart().value, (int32_t)(mApzc.mX.GetPageEnd().value - mApzc.mX.GetCompositionLength().value),

Better: (mApzc.mX.GetPageEnd() - mApzc.mX.GetCompositionLength()).value

i.e. do the arithmetic in typed units, and only then drop down to raw numbers. This way, we're leveraging the compiler's verification can the arithmetic is done on quantities with the same units.

@@ +72,5 @@
> +                       (int32_t)(velocity.x * 1000.0f), (int32_t)(velocity.y * 1000.0f),
> +                       (int32_t)mApzc.mX.GetPageStart().value, (int32_t)(mApzc.mX.GetPageEnd().value - mApzc.mX.GetCompositionLength().value),
> +                       (int32_t)mApzc.mY.GetPageStart().value, (int32_t)(mApzc.mY.GetPageEnd().value - mApzc.mY.GetCompositionLength().value),
> +                       0, 0);
> +}

Should we call OverScroller.setFriction() with something based on apz.fling_friction? Or are we deliberately using the system friction value without allowing the user to customize it? (In the latter case, we should document in the prefs file that apz.fling_friction is ignored on Android.)

@@ +85,5 @@
> +FlingOverScrollerAnimation::DoSample(FrameMetrics& aFrameMetrics,
> +                                     const TimeDuration& aDelta)
> +{
> +  bool shouldContinueFling = true;
> +  mOverScroller->ComputeScrollOffset(&shouldContinueFling);

As this call isn't receiving |aDelta| as an input, I assume it queries the current system time and calculates the scroll offset based on that.

However, |aDelta| isn't guaranteed to reflect the current system time. In particular, AsyncCompositionManager passes in the timestamp of the next vsync [1], so that what we calculate will reflect what we show on screen when the frame is actually composited.

Is it OK for us to just be using the system time instead?

[1] https://dxr.mozilla.org/mozilla-central/rev/043082cb7bd8490c60815f67fbd1f33323ad7663/gfx/layers/composite/AsyncCompositionManager.cpp#1432

@@ +93,5 @@
> +  speed = speed * 0.001f; // convert from pixels/sec to pixels/ms
> +
> +  if (!shouldContinueFling || (speed < gfxPrefs::APZFlingStoppedThreshold())) {
> +    if (shouldContinueFling) {
> +      // The OverScroller thinks it should continue but the speed is bellow

typo: bellow

@@ +110,5 @@
> +  ParentLayerPoint offset((float)currentX, (float)currentY);
> +
> +  float lengthX = offset.x - mStartOffset.x;
> +  float lengthY = offset.y - mStartOffset.y;
> +  float magnitude = sqrtf((lengthX * lengthX) + (lengthY * lengthY));

ParentLayerPoint displacement = offset - mStartOffset;
float magnitude = displacement.Length();

@@ +130,5 @@
> +
> +  // If the offset hasn't changed in over MAX_OVERSCROLL_COUNT we have overflowed
> +  // the OverScroller and it needs to be aborted.
> +  if (mOverScrollCount > MAX_OVERSCROLL_COUNT) {
> +    mOverScroller->AbortAnimation();

Should we set velocity to zero and return false if we've aborted the animation? Or will that happen on the next sample?

@@ +142,5 @@
> +  if (hitBoundX || hitBoundY) {
> +    ParentLayerPoint bounceVelocity = mFlingDirection * speed;
> +
> +    if (!mSentBounceX && hitBoundX && fabsf(offset.x - mStartOffset.x) > BOUNDS_EPSILON) {
> +       mSentBounceX = true;

indentation

@@ +154,5 @@
> +      bounceVelocity.y = 0.0f;
> +    }
> +    if (!IsZero(bounceVelocity)) {
> +      mDeferredTasks.AppendElement(
> +          NewRunnableMethod<ParentLayerPoint,

Why do you need to pass explicit template parameters to NewRunnableMethod? We don't seem to need to anywhere else.

@@ +168,5 @@
> +  return true;
> +}
> +
> +bool
> +FlingOverScrollerAnimation::CheckBounds(Axis& axis, float value, float* clamped)

aAxis, ...

@@ +176,5 @@
> +    result = true;
> +    if (clamped) {
> +      *clamped = axis.GetPageStart().value;
> +    }
> +  } else if ((value + BOUNDS_EPSILON) >= (axis.GetPageEnd().value - axis.GetCompositionLength().value)) {

Likewise: (axis.GetPageEnd() - axis.GetCompositionLength()).value

::: gfx/layers/apz/src/FlingOverScrollerAnimation.h
@@ +16,5 @@
> +
> +class FlingOverScrollerAnimation: public AsyncPanZoomAnimation {
> +public:
> +  FlingOverScrollerAnimation(AsyncPanZoomController& aApzc,
> +                 widget::sdk::OverScroller::GlobalRef aOverScroller,

Don't you need to include something for this?

@@ +23,5 @@
> +  virtual bool DoSample(FrameMetrics& aFrameMetrics,
> +                        const TimeDuration& aDelta) override;
> +private:
> +  // Returns true if value is on or outside of axis bounds.
> +  bool CheckBounds(Axis& axis, float value, float* clamped = nullptr);

as per naming conventions: aAxis, aValue, aOutClamped

Also, as we never call it without passing the |clamped| parameter, let's just remove the default and require that a non-nullptr pointer be passed in.

@@ +33,5 @@
> +  bool mSentBounceX;
> +  bool mSentBounceY;
> +  ParentLayerPoint mStartOffset;
> +  ParentLayerPoint mPreviousOffset;
> +  ParentLayerPoint mFlingDirection;

// a unit vector in the direction of the fling

::: mobile/android/app/mobile.js
@@ +600,5 @@
>  pref("apz.fling_curve_function_x2", "0.05");
>  pref("apz.fling_curve_function_y2", "1.00");
>  pref("apz.fling_curve_threshold_inches_per_ms", "0.01");
>  pref("apz.fling_friction", "0.004");
> +pref("apz.fling_stopped_threshold", "0.0");

The coment says "APZ physics settings reviewed by UX". Was this? :)
Attachment #8749894 - Flags: review?(botond) → feedback?(bugmail.mozilla)
I'm also not a huge fan of the proliferation of |#ifdef MOZ_ANDROID_APZ| in APZC code. I have some thoughts on how we could reduce it; I'll play around with them and post a follow-up patch (possibly in a different bug).
(In reply to Botond Ballo [:botond] from comment #12)
> Comment on attachment 8749894 [details] [diff] [review]
> 0001-Bug-1229462-Use-Android-OverScroller-class-for-fling-animation-16050616-
> de5c233.patch
> 
> Review of attachment 8749894 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks generally good; I have a few comments. I'd also like Kats to weigh in
> on the vsync / timestamp issue.
> 
> ::: gfx/layers/apz/src/FlingOverScrollerAnimation.cpp
> @@ +15,5 @@
> > +namespace layers {
> > +
> > +// Value used in boundary detection. Due to round off error,
> > +// assume getting within 5 pixes of the boundary is close enough.
> > +static const float BOUNDS_EPSILON = 5.0f;
> 
> This seems like a very high epsilon. Have you found it's necessary for it to
> be this high?
> 

Unfortunately yes, There appears to be some "slop" in the OverScroller calculations (rounding errors due to int32_t?).

> @@ +72,5 @@
> > +                       (int32_t)(velocity.x * 1000.0f), (int32_t)(velocity.y * 1000.0f),
> > +                       (int32_t)mApzc.mX.GetPageStart().value, (int32_t)(mApzc.mX.GetPageEnd().value - mApzc.mX.GetCompositionLength().value),
> > +                       (int32_t)mApzc.mY.GetPageStart().value, (int32_t)(mApzc.mY.GetPageEnd().value - mApzc.mY.GetCompositionLength().value),
> > +                       0, 0);
> > +}
> 
> Should we call OverScroller.setFriction() with something based on
> apz.fling_friction? Or are we deliberately using the system friction value
> without allowing the user to customize it? (In the latter case, we should
> document in the prefs file that apz.fling_friction is ignored on Android.)
> 

We are deliberately using the system friction. I'll make a note that apz.fling_friction ignored by Android.
 

> @@ +130,5 @@
> > +
> > +  // If the offset hasn't changed in over MAX_OVERSCROLL_COUNT we have overflowed
> > +  // the OverScroller and it needs to be aborted.
> > +  if (mOverScrollCount > MAX_OVERSCROLL_COUNT) {
> > +    mOverScroller->AbortAnimation();
> 
> Should we set velocity to zero and return false if we've aborted the
> animation? Or will that happen on the next sample?
> 

It happens the next sample.
 
> @@ +154,5 @@
> > +      bounceVelocity.y = 0.0f;
> > +    }
> > +    if (!IsZero(bounceVelocity)) {
> > +      mDeferredTasks.AppendElement(
> > +          NewRunnableMethod<ParentLayerPoint,
> 
> Why do you need to pass explicit template parameters to NewRunnableMethod?
> We don't seem to need to anywhere else.
> 

It stopped compiling after a rebase. I found it had been change in [1]. So I made the same change to get things compiling again.

[1] https://dxr.mozilla.org/mozilla-central/rev/043082cb7bd8490c60815f67fbd1f33323ad7663/gfx/layers/apz/src/AsyncPanZoomController.cpp#570
 
> ::: gfx/layers/apz/src/FlingOverScrollerAnimation.h
> @@ +16,5 @@
> > +
> > +class FlingOverScrollerAnimation: public AsyncPanZoomAnimation {
> > +public:
> > +  FlingOverScrollerAnimation(AsyncPanZoomController& aApzc,
> > +                 widget::sdk::OverScroller::GlobalRef aOverScroller,
> 
> Don't you need to include something for this?
>

I'm not sure what you are referring to specifically but I am including OverScroller.h which defines the widget::sdk::OverScroller class.

> ::: mobile/android/app/mobile.js
> @@ +600,5 @@
> >  pref("apz.fling_curve_function_x2", "0.05");
> >  pref("apz.fling_curve_function_y2", "1.00");
> >  pref("apz.fling_curve_threshold_inches_per_ms", "0.01");
> >  pref("apz.fling_friction", "0.004");
> > +pref("apz.fling_stopped_threshold", "0.0");
> 
> The coment says "APZ physics settings reviewed by UX". Was this? :)

No, I set it to 0.0 because I only really need to handle the apz.fling_stopped_threshold pref to pass try (It used to disable flinging). Otherwise it would be ignored like apz.fling_friction so setting it to 0.0 has the same effect as ignoring under normal operation.

But I can have some one look at it I guess.
(In reply to Randall Barker [:rbarker] from comment #14)
> It happens the next sample.
>  
> > @@ +154,5 @@
> > > +      bounceVelocity.y = 0.0f;
> > > +    }
> > > +    if (!IsZero(bounceVelocity)) {
> > > +      mDeferredTasks.AppendElement(
> > > +          NewRunnableMethod<ParentLayerPoint,
> > 
> > Why do you need to pass explicit template parameters to NewRunnableMethod?
> > We don't seem to need to anywhere else.
> > 
> 
> It stopped compiling after a rebase. I found it had been change in [1]. So I
> made the same change to get things compiling again.

Looks like I'm behind the times! This is fine, then.

> > ::: gfx/layers/apz/src/FlingOverScrollerAnimation.h
> > @@ +16,5 @@
> > > +
> > > +class FlingOverScrollerAnimation: public AsyncPanZoomAnimation {
> > > +public:
> > > +  FlingOverScrollerAnimation(AsyncPanZoomController& aApzc,
> > > +                 widget::sdk::OverScroller::GlobalRef aOverScroller,
> > 
> > Don't you need to include something for this?
> >
> 
> I'm not sure what you are referring to specifically but I am including
> OverScroller.h which defines the widget::sdk::OverScroller class.

You're not including OverScroller.h in FlingOverScrollerAnimation.h.

> > ::: mobile/android/app/mobile.js
> > @@ +600,5 @@
> > >  pref("apz.fling_curve_function_x2", "0.05");
> > >  pref("apz.fling_curve_function_y2", "1.00");
> > >  pref("apz.fling_curve_threshold_inches_per_ms", "0.01");
> > >  pref("apz.fling_friction", "0.004");
> > > +pref("apz.fling_stopped_threshold", "0.0");
> > 
> > The coment says "APZ physics settings reviewed by UX". Was this? :)
> 
> No, I set it to 0.0 because I only really need to handle the
> apz.fling_stopped_threshold pref to pass try (It used to disable flinging).
> Otherwise it would be ignored like apz.fling_friction so setting it to 0.0
> has the same effect as ignoring under normal operation.
> 
> But I can have some one look at it I guess.

OK, I understand now. Could you add a couple of comments please:

  - In the prefs file defining apz.fling_stopped_threshold, mention that it,
    like, apz.fling_friction, is ignored on Android.

  - In the code where you check its value, mention that it's only
    actually used by tests.

As for a UX review, it probably wouldn't hurt for someone to try out the change we're making as a whole. cc'ing Anthony.
Comment on attachment 8749894 [details] [diff] [review]
0001-Bug-1229462-Use-Android-OverScroller-class-for-fling-animation-16050616-de5c233.patch

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

I spotted a few more nits, I didn't look through the whole patch in that much detail. See comment below on the vsync/timestamp issue.

::: gfx/layers/apz/src/FlingOverScrollerAnimation.cpp
@@ +14,5 @@
> +namespace mozilla {
> +namespace layers {
> +
> +// Value used in boundary detection. Due to round off error,
> +// assume getting within 5 pixes of the boundary is close enough.

s/pixes/pixels/

@@ +58,5 @@
> +  ParentLayerPoint velocity = mApzc.GetVelocityVector();
> +  float magnitude = sqrtf((velocity.x * velocity.x) + (velocity.y * velocity.y));
> +  if (magnitude > 0.0f) {
> +      mFlingDirection.x = velocity.x / magnitude;
> +      mFlingDirection.y = velocity.y / magnitude;

indentation

@@ +85,5 @@
> +FlingOverScrollerAnimation::DoSample(FrameMetrics& aFrameMetrics,
> +                                     const TimeDuration& aDelta)
> +{
> +  bool shouldContinueFling = true;
> +  mOverScroller->ComputeScrollOffset(&shouldContinueFling);

Yeah it looks like Scroller/OverScroller use AnimationUtils.currentAnimationTimeMillis() as their timeline, which defaults to SystemClock.uptimeMillis(). I don't know if there's much we can do about this - while it would be nice to use the "next vsync" timestamp provided by aDelta, the difference may not be user-visible in practice. The only alternative I can think of is copying the OverScroller class and modifying it, which seems like overkill for this problem.

@@ +114,5 @@
> +  float magnitude = sqrtf((lengthX * lengthX) + (lengthY * lengthY));
> +  ParentLayerPoint velocity;
> +  if (magnitude > 0.0f) {
> +      velocity.x = (lengthX / magnitude) * speed;
> +      velocity.y = (lengthY / magnitude) * speed;

With botond's above suggestion you should be able to do

velocity = (displacement / magnitude) * speed;

::: gfx/layers/client/SingleTiledContentClient.cpp
@@ +6,5 @@
>  #include "mozilla/layers/SingleTiledContentClient.h"
>  
>  #include "ClientTiledPaintedLayer.h"
>  
> +using namespace mozilla::gfx; // for LogReason

In general we try to avoid namespace pollution like this, because it screws with unified builds. As LogReason is only used in one place, please just stick a gfx:: in front of it.
Attachment #8749894 - Flags: feedback?(bugmail.mozilla) → feedback+
(In reply to Botond Ballo [:botond] from comment #13)
> I'm also not a huge fan of the proliferation of |#ifdef MOZ_ANDROID_APZ| in
> APZC code. I have some thoughts on how we could reduce it; I'll play around
> with them and post a follow-up patch (possibly in a different bug).

Ditto on not being a fan of the #ifdef, if you can find a nice way to clean it up that would be great.
Comment on attachment 8750939 [details] [diff] [review]
0001-Bug-1229462-Use-Android-OverScroller-class-for-fling-animation-r-16051012-a229087.patch

I believe I have addressed all review comments.
Attachment #8750939 - Flags: review?(botond)
Attachment #8750939 - Flags: review?(botond) → review+
Attached patch Uplift patch for Aurora (obsolete) — Splinter Review
Blocks: 1272165
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #17)
> (In reply to Botond Ballo [:botond] from comment #13)
> > I'm also not a huge fan of the proliferation of |#ifdef MOZ_ANDROID_APZ| in
> > APZC code. I have some thoughts on how we could reduce it; I'll play around
> > with them and post a follow-up patch (possibly in a different bug).
> 
> Ditto on not being a fan of the #ifdef, if you can find a nice way to clean
> it up that would be great.

I'll play around with it in bug 1272165.
https://hg.mozilla.org/mozilla-central/rev/3271caad1b5c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Not salty about this at all.
See Also: → 703311
Version of patch for Aurora.
Attachment #8751519 - Attachment is obsolete: true
Comment on attachment 8755629 [details] [diff] [review]
Uplift patch for Aurora

Approval Request Comment
[Feature/regressing bug #]: Bug 1229462
[User impact if declined]:Fling physics will remain inconsistent with native Android applications that allow scrolling.
[Describe test coverage new/current, TreeHerder]: none.
[Risks and why]: Can cause increase in checker boarding due to higher top fling speeds.
[String/UUID change made/needed]: none.
Attachment #8755629 - Flags: approval-mozilla-aurora?
(In reply to Randall Barker [:rbarker] from comment #27)
> [Feature/regressing bug #]: Bug 1229462
This is this bug.
Is this a recent regression? If no, why uplifting this to aurora?
Flags: needinfo?(rbarker)
(In reply to Sylvestre Ledru [:sylvestre] from comment #28)
> (In reply to Randall Barker [:rbarker] from comment #27)
> > [Feature/regressing bug #]: Bug 1229462
> This is this bug.
> Is this a recent regression? If no, why uplifting this to aurora?

We're targeting 48 for enabling APZ. These changes are necessary to ship that.
Flags: needinfo?(rbarker)
Comment on attachment 8755629 [details] [diff] [review]
Uplift patch for Aurora

OK, thanks, taking it then.
Attachment #8755629 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.