Bug 1276463 - Allow Android native fling animation to correctly handoff velocity r=botond

gfx/layers/apz/src/AndroidAPZ.cpp

@@ -18,18 +18,6 @@
 namespace mozilla {
 namespace layers {
 
-// Value used in boundary detection. Due to round off error,
-// assume getting within 5 pixels of the boundary is close enough.
-static const float BOUNDS_EPSILON = 5.0f;
-// Maximum number of times the animator can calculate the same offset
-// before the animation is aborted. This is due to a bug in the Android
-// OverScroller class where under certain conditions the OverScroller
-// will overflow some internal value and begin scrolling beyond the bounds
-// of the page. Since we are clamping the results from the OverScroller,
-// if the offset does not change over the past 30 frames, we assume the
-// OverScroller has overflowed.
-static const int32_t MAX_OVERSCROLL_COUNT = 30;
-
 AndroidSpecificState::AndroidSpecificState() {
   widget::sdk::OverScroller::LocalRef scroller;
   if (widget::sdk::OverScroller::New(widget::GeckoAppShell::GetApplicationContext(), &scroller) != NS_OK) {
@@ -39,6 +27,30 @@ AndroidSpecificState::AndroidSpecificState() {
   mOverScroller = scroller;
 }
 
+const float BOUNDS_EPSILON = 1.0f;
+
+// This function is used to convert the scroll offset from a float to an integer
+// suitable for using with the Android OverScroller Class.
+// The Android OverScroller class (unfortunately) operates in integers instead of floats.
+// When casting a float value such as 1.5 to an integer, the value is converted to 1.
+// If this value represents the max scroll offset, the OverScroller class will never scroll
+// to the end of the page as it will always be 0.5 pixels short. To work around this issue,
+// the min and max scroll extents are floor/ceil to convert them to the nearest integer
+// just outside of the actual scroll extents. This means, the starting
+// scroll offset must be converted the same way so that if the frame has already been
+// scrolled 1.5 pixels, it won't be snapped back when converted to an integer. This integer
+// rounding error was one of several causes of Bug 1276463.
+static int32_t
+ClampStart(float aOrigin, float aMin, float aMax)
+{
+  if (aOrigin <= aMin) {
+    return (int32_t)floor(aMin);
+  } else if (aOrigin >= aMax) {
+    return (int32_t)ceil(aMax);
+  }
+  return (int32_t)aOrigin;
+}
+
 AndroidFlingAnimation::AndroidFlingAnimation(AsyncPanZoomController& aApzc,
                                              PlatformSpecificStateBase* aPlatformSpecificState,
                                              const RefPtr<const OverscrollHandoffChain>& aOverscrollHandoffChain,
@@ -49,7 +61,6 @@ AndroidFlingAnimation::AndroidFlingAnimation(AsyncPanZoomController& aApzc,
   , mScrolledApzc(aScrolledApzc)
   , mSentBounceX(false)
   , mSentBounceY(false)
-  , mOverScrollCount(0)
 {
   MOZ_ASSERT(mOverscrollHandoffChain);
   MOZ_ASSERT(aPlatformSpecificState->AsAndroidSpecificState());
@@ -70,18 +81,26 @@ AndroidFlingAnimation::AndroidFlingAnimation(AsyncPanZoomController& aApzc,
   }
 
   ParentLayerPoint velocity = mApzc.GetVelocityVector();
+  mPreviousVelocity = velocity;
+
+  float scrollRangeStartX = mApzc.mX.GetPageStart().value;
+  float scrollRangeEndX = mApzc.mX.GetScrollRangeEnd().value;
+  float scrollRangeStartY = mApzc.mY.GetPageStart().value;
+  float scrollRangeEndY = mApzc.mY.GetScrollRangeEnd().value;
+  mStartOffset.x = mPreviousOffset.x = mApzc.mX.GetOrigin().value;
+  mStartOffset.y = mPreviousOffset.y = mApzc.mY.GetOrigin().value;
   float length = velocity.Length();
   if (length > 0.0f) {
     mFlingDirection = velocity / length;
   }
 
-  mStartOffset.x = mPreviousOffset.x = mApzc.mX.GetOrigin().value;
-  mStartOffset.y = mPreviousOffset.y = mApzc.mY.GetOrigin().value;
-  mOverScroller->Fling((int32_t)mStartOffset.x, (int32_t)mStartOffset.y,
+  int32_t originX = ClampStart(mStartOffset.x, scrollRangeStartX, scrollRangeEndX);
+  int32_t originY = ClampStart(mStartOffset.y, scrollRangeStartY, scrollRangeEndY);
+  mOverScroller->Fling(originX, originY,
                        // 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() - mApzc.mX.GetCompositionLength()).value,
-                       (int32_t)mApzc.mY.GetPageStart().value, (int32_t)(mApzc.mY.GetPageEnd() - mApzc.mY.GetCompositionLength()).value,
+                       (int32_t)floor(scrollRangeStartX), (int32_t)ceil(scrollRangeEndX),
+                       (int32_t)floor(scrollRangeStartY), (int32_t)ceil(scrollRangeEndY),
                        0, 0);
 }
 
@@ -96,11 +115,38 @@ AndroidFlingAnimation::DoSample(FrameMetrics& aFrameMetrics,
                                 const TimeDuration& aDelta)
 {
   bool shouldContinueFling = true;
-  mOverScroller->ComputeScrollOffset(&shouldContinueFling);
 
-  float speed = 0.0f;
-  mOverScroller->GetCurrVelocity(&speed);
-  speed = speed * 0.001f; // convert from pixels/sec to pixels/ms
+  mOverScroller->ComputeScrollOffset(&shouldContinueFling);
+  // OverScroller::GetCurrVelocity will sometimes return NaN. So need to externally
+  // calculate current velocity and not rely on what the OverScroller calculates.
+  // mOverScroller->GetCurrVelocity(&speed);
+  int32_t currentX = 0;
+  int32_t currentY = 0;
+  mOverScroller->GetCurrX(&currentX);
+  mOverScroller->GetCurrY(&currentY);
+  ParentLayerPoint offset((float)currentX, (float)currentY);
+
+  bool hitBoundX = CheckBounds(mApzc.mX, offset.x, mFlingDirection.x, &(offset.x));
+  bool hitBoundY = CheckBounds(mApzc.mY, offset.y, mFlingDirection.y, &(offset.y));
+
+  ParentLayerPoint velocity = mPreviousVelocity;
+
+  // Sometimes the OverScroller fails to update the offset for a frame.
+  // If the frame can still scroll we just use the velocity from the previous
+  // frame. However, if the frame can no longer scroll in the direction
+  // of the fling, then end the animation.
+  if (offset != mPreviousOffset) {
+    if (aDelta.ToMilliseconds() > 0) {
+      velocity = (mPreviousOffset - offset) / (float)aDelta.ToMilliseconds();
+      mPreviousVelocity = velocity;
+    }
+  } else if (hitBoundX || hitBoundY) {
+    // We have reached the end of the scroll in one of the directions being scrolled and the offset has not
+    // changed so end animation.
+    shouldContinueFling = false;
+  }
+
+  float speed = velocity.Length();
 
   // gfxPrefs::APZFlingStoppedThreshold is only used in tests.
   if (!shouldContinueFling || (speed < gfxPrefs::APZFlingStoppedThreshold())) {
@@ -109,41 +155,24 @@ AndroidFlingAnimation::DoSample(FrameMetrics& aFrameMetrics,
       // the stopping threshold so abort the animation.
       mOverScroller->AbortAnimation();
     }
-    mApzc.mX.SetVelocity(0);
-    mApzc.mY.SetVelocity(0);
+    // This animation is going to end. If DeferHandleFlingOverscroll
+    // has not been called and there is still some velocity left,
+    // call it so that fling hand off may occur if applicable.
+    if (!mSentBounceX && !mSentBounceY && (speed > 0.0f)) {
+      DeferHandleFlingOverscroll(velocity);
+    }
     return false;
   }
 
-  int32_t currentX = 0;
-  int32_t currentY = 0;
-  mOverScroller->GetCurrX(&currentX);
-  mOverScroller->GetCurrY(&currentY);
-  ParentLayerPoint offset((float)currentX, (float)currentY);
-  ParentLayerPoint velocity = mFlingDirection * speed;
-
-  bool hitBoundX = CheckBounds(mApzc.mX, offset.x, &(offset.x));
-  bool hitBoundY = CheckBounds(mApzc.mY, offset.y, &(offset.y));
-
-  if (IsZero(mPreviousOffset - offset)) {
-    mOverScrollCount++;
-  } else {
-    mOverScrollCount = 0;
-  }
-
-  // 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) {
-    velocity.x = velocity.y = 0.0f;
-    mOverScroller->AbortAnimation();
-  }
-
   mPreviousOffset = offset;
 
   mApzc.SetVelocityVector(velocity);
   aFrameMetrics.SetScrollOffset(offset / aFrameMetrics.GetZoom());
 
+  // If we hit a bounds while flinging, send the velocity so that the bounce
+  // animation can play.
   if (hitBoundX || hitBoundY) {
-    ParentLayerPoint bounceVelocity = mFlingDirection * speed;
+    ParentLayerPoint bounceVelocity = velocity;
 
     if (!mSentBounceX && hitBoundX && fabsf(offset.x - mStartOffset.x) > BOUNDS_EPSILON) {
       mSentBounceX = true;
@@ -157,36 +186,42 @@ AndroidFlingAnimation::DoSample(FrameMetrics& aFrameMetrics,
       bounceVelocity.y = 0.0f;
     }
     if (!IsZero(bounceVelocity)) {
-      mDeferredTasks.AppendElement(
-          NewRunnableMethod<ParentLayerPoint,
-                            RefPtr<const OverscrollHandoffChain>,
-                            RefPtr<const AsyncPanZoomController>>(&mApzc,
-                                                                  &AsyncPanZoomController::HandleFlingOverscroll,
-                                                                  bounceVelocity,
-                                                                  mOverscrollHandoffChain,
-                                                                  mScrolledApzc));
+      DeferHandleFlingOverscroll(bounceVelocity);
     }
   }
 
   return true;
 }
 
-bool
-AndroidFlingAnimation::CheckBounds(Axis& aAxis, float aValue, float* aClamped)
+void
+AndroidFlingAnimation::DeferHandleFlingOverscroll(ParentLayerPoint& aVelocity)
 {
-  bool result = false;
-  if ((aValue - BOUNDS_EPSILON) <= aAxis.GetPageStart().value) {
-    result = true;
+  mDeferredTasks.AppendElement(
+      NewRunnableMethod<ParentLayerPoint,
+                        RefPtr<const OverscrollHandoffChain>,
+                        RefPtr<const AsyncPanZoomController>>(&mApzc,
+                                                              &AsyncPanZoomController::HandleFlingOverscroll,
+                                                              aVelocity,
+                                                              mOverscrollHandoffChain,
+                                                              mScrolledApzc));
+
+}
+
+bool
+AndroidFlingAnimation::CheckBounds(Axis& aAxis, float aValue, float aDirection, float* aClamped)
+{
+  if ((aDirection < 0.0f) && (aValue <= aAxis.GetPageStart().value)) {
     if (aClamped) {
       *aClamped = aAxis.GetPageStart().value;
     }
-  } else if ((aValue + BOUNDS_EPSILON) >= (aAxis.GetPageEnd() - aAxis.GetCompositionLength()).value) {
-    result = true;
+    return true;
+  } else if ((aDirection > 0.0f) && (aValue >= aAxis.GetScrollRangeEnd().value)) {
     if (aClamped) {
-      *aClamped = (aAxis.GetPageEnd() - aAxis.GetCompositionLength()).value;
+      *aClamped = aAxis.GetScrollRangeEnd().value;
     }
+    return true;
   }
-  return result;
+  return false;
 }
 
 } // namespace layers

gfx/layers/apz/src/AndroidAPZ.h

@@ -36,8 +36,9 @@ public:
   virtual bool DoSample(FrameMetrics& aFrameMetrics,
                         const TimeDuration& aDelta) override;
 private:
+  void DeferHandleFlingOverscroll(ParentLayerPoint& aVelocity);
   // Returns true if value is on or outside of axis bounds.
-  bool CheckBounds(Axis& aAxis, float aValue, float* aClamped);
+  bool CheckBounds(Axis& aAxis, float aValue, float aDirection, float* aClamped);
 
   AsyncPanZoomController& mApzc;
   widget::sdk::OverScroller::GlobalRef mOverScroller;
@@ -49,7 +50,7 @@ private:
   ParentLayerPoint mPreviousOffset;
   // Unit vector in the direction of the fling.
   ParentLayerPoint mFlingDirection;
-  int32_t mOverScrollCount;
+  ParentLayerPoint mPreviousVelocity;
 };
 
 

gfx/layers/apz/src/Axis.cpp

@@ -570,6 +570,10 @@ ParentLayerCoord Axis::GetPageEnd() const {
   return GetPageStart() + GetPageLength();
 }
 
+ParentLayerCoord Axis::GetScrollRangeEnd() const {
+  return GetPageEnd() - GetCompositionLength();
+}
+
 ParentLayerCoord Axis::GetOrigin() const {
   ParentLayerPoint origin = GetFrameMetrics().GetScrollOffset() * GetFrameMetrics().GetZoom();
   return GetPointOffset(origin);

gfx/layers/apz/src/Axis.h

@@ -244,6 +244,7 @@ public:
   ParentLayerCoord GetPageLength() const;
   ParentLayerCoord GetCompositionEnd() const;
   ParentLayerCoord GetPageEnd() const;
+  ParentLayerCoord GetScrollRangeEnd() const;
 
   ParentLayerCoord GetPos() const { return mPos; }