Closed
Bug 1229462
Opened 8 years ago
Closed 8 years ago
Use Android Scroller class for fling animation
Categories
(Firefox for Android Graveyard :: Toolbar, defect)
Firefox for Android Graveyard
Toolbar
Tracking
(firefox48 fixed, firefox49 fixed)
RESOLVED
FIXED
Firefox 49
People
(Reporter: snorp, Assigned: rbarker)
References
Details
Attachments
(2 files, 6 obsolete files)
25.77 KB,
patch
|
botond
:
review+
|
Details | Diff | Splinter Review |
25.52 KB,
patch
|
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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
Updated•8 years ago
|
Blocks: apz-fennec
Reporter | ||
Comment 1•8 years ago
|
||
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
Any possibility if this could be prioritized higher? It would be great if it lands in the current Nightly versions.
Reporter | ||
Updated•8 years ago
|
Summary: Use system values for APZ physics → Use Android Scroller class for fling animation
Reporter | ||
Comment 5•8 years ago
|
||
(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
Assignee | ||
Comment 6•8 years ago
|
||
Comment 7•8 years ago
|
||
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.
Assignee | ||
Comment 8•8 years ago
|
||
Attachment #8749743 -
Attachment is obsolete: true
Assignee | ||
Comment 9•8 years ago
|
||
Attachment #8749776 -
Attachment is obsolete: true
Assignee | ||
Comment 10•8 years ago
|
||
Fix try failure.
Attachment #8749796 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8749894 -
Flags: review?(botond)
Assignee | ||
Comment 11•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa6d9846fd6e
Comment 12•8 years ago
|
||
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)
Comment 13•8 years ago
|
||
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).
Assignee | ||
Comment 14•8 years ago
|
||
(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.
Comment 15•8 years ago
|
||
(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 16•8 years ago
|
||
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+
Comment 17•8 years ago
|
||
(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.
Assignee | ||
Comment 18•8 years ago
|
||
Attachment #8749894 -
Attachment is obsolete: true
Assignee | ||
Comment 19•8 years ago
|
||
Attachment #8750932 -
Attachment is obsolete: true
Assignee | ||
Comment 20•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8750939 -
Flags: review?(botond) → review+
Assignee | ||
Comment 22•8 years ago
|
||
Comment 23•8 years ago
|
||
(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.
Comment 24•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3271caad1b5c
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Assignee | ||
Comment 26•8 years ago
|
||
Version of patch for Aurora.
Attachment #8751519 -
Attachment is obsolete: true
Assignee | ||
Comment 27•8 years ago
|
||
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?
Updated•8 years ago
|
status-firefox48:
--- → affected
Comment 28•8 years ago
|
||
(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?
Updated•8 years ago
|
Flags: needinfo?(rbarker)
Reporter | ||
Comment 29•8 years ago
|
||
(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 30•8 years ago
|
||
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+
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•