Closed Bug 1457586 Opened 2 years ago Closed Last year

Fennec's initial fling velocity is lower than in Chrome, even when using Chrome's fling physics

Categories

(Core :: Panning and Zooming, defect, P3)

All
Android
defect

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
relnote-firefox --- 64+
geckoview62 --- wontfix
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- wontfix
firefox64 --- fixed

People

(Reporter: mstange, Assigned: botond)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [geckoview:klar:p2][gfx-noted])

Attachments

(9 files, 7 obsolete files)

2.86 MB, video/mp4
Details
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
Steps to reproduce:
 1. On Firefox for Android, go to about:config and set apz.android.chrome_fling_physics.enabled to true.
 2. Go to any scrollable page, for example https://en.m.wikipedia.org/wiki/Gomphus_clavatus
 3. Scroll up and down alternatingly, using short but quick finger motions.

Expected results:
Scrolling should feel the same in Firefox as it does in Chrome.

Actual results:
Firefox scrolls a much shorter distance based on the same touch input.

Firefox also reacts later than Chrome, or at least that's what it feels like. Maybe we ignore the first few touch moves because of some tolerance for bad touch screens?
The main difference seems to be the fling start velocity. Maybe we're averaging too many touch deltas when computing that velocity?
Attached video video recording
I'm happy to demonstrate this in person. Here's a video I just took of it.
Tagging [geckoview:klar:p2] because this would be nice to fix for Klar+GeckoView, but is not a blocker.
Blocks: 1448439
Summary: Initial fling velocity is lower than in Chrome, even when using Chrome's fling physics → Fennec's initial fling velocity is lower than in Chrome, even when using Chrome's fling physics
Whiteboard: [geckoview:klar:p2]
Yes, it's possible that there's still a difference in the starting fling velocity. How Chrome determines the initial fling velocity is not one of the things I investigated in bug 1448376; I sort of assumed it would correspond to the physical velocity of the page at the point of the fling's release, the way it does for us, but maybe it doesn't.
Priority: -- → P3
Whiteboard: [geckoview:klar:p2] → [geckoview:klar:p2][gfx-noted]
Blocks: 1458653
I tried tracking down in the Chromium codebase where the fling's start velocity comes from on Android, but it's pretty convoluted. I think I traced it as far as the parameters to WebView.flingScroll(), which is an Android API, and so (I guess) has its callers in Android code rather than Chromium code?
(In reply to Botond Ballo [:botond] from comment #4)
> I think I traced
> it as far as the parameters to WebView.flingScroll(), which is an Android
> API, and so (I guess) has its callers in Android code rather than Chromium
> code?

It looks like this was a false direction. WebView.flingScroll() is a way for apps using Android's WebView to start a fling animation with an initial velocity of their choosing, it's not the codepath by which the Chrome app itself initiates a fling animation. Markus helped me locate the relevant code [1].

(In reply to Botond Ballo [:botond] from comment #3)
> How Chrome determines the initial fling velocity is not one of the
> things I investigated in bug 1448376; I sort of assumed it would correspond
> to the physical velocity of the page at the point of the fling's release,
> the way it does for us, but maybe it doesn't.

I glossed over something here: we don't quite use the page's physical velocity at the point of the fling's release, but rather an average based on the last few touch events.

The difference seems to be how we compute this "average": in the Firefox code, it literally is just an average, whereas in the Chromium code, they use a more sophisticated model (second-degree least squares linear regression) to compute it [2].

[1] https://cs.chromium.org/chromium/src/ui/events/gesture_detection/gesture_detector.cc?l=376&rcl=85d94fd334e5df67195bbef8513b0df5bbb570b8
[2] https://cs.chromium.org/chromium/src/ui/events/gesture_detection/velocity_tracker.cc?l=101&rcl=9ea9a086d4f54c702ec9a38e55fb3eb8bbc2401b
Going to start working on this.
Assignee: nobody → botond
Awesome! If you land the fix in Nightly 63, we should consider uplifting it to Beta 62 so we can ship it together with new fling physics (bug 1460206).
The scroll flinging solution seem very similar to the solutions to throwing in VR - https://www.gamasutra.com/blogs/CharlieDeck/20161118/285808/Why_Throwing_in_VR_Sucksand_How_to_Make_it_Better.php

Extrapolating from that there might be a responsiveness win in tying either the fling velocity calculation or fling trigger to an apparent drop in touch pressure instead of touch release as it better signals the user's intended point of release.

I was thinking of opening an issue for this but I'm uncertain if it would just be unnecessary noise.
(In reply to rishel.nick from comment #8)
> Extrapolating from that there might be a responsiveness win in tying either
> the fling velocity calculation or fling trigger to an apparent drop in touch
> pressure instead of touch release as it better signals the user's intended
> point of release.
> 
> I was thinking of opening an issue for this but I'm uncertain if it would
> just be unnecessary noise.

Please feel free to file an issue for this suggestion. We can discuss it further there.
glamon (UX) says he is able to reproduce this bug in Fennec 61 and it is fairly noticeable. The effect seems more pronounced while scrolling up than down. He would prioritize this below "scrolling hitch" bug 1425739 since this feels more like a "difference" than an "error".
This is an initial patch series; it needs debugging and is not ready for review yet.
Attachment #8997615 - Flags: review?(mstange)
Markus, could you try the patches (you can use a Try build from [1]) and let me know if it fixes the problem as you originally reported it? Thanks!

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=24ec0cece2e7e546ef8928e186b7dca1237450d5
Flags: needinfo?(mstange)
(In reply to Chris Peterson [:cpeterson] from comment #10)
> He would prioritize this below "scrolling hitch" bug 1425739 since
> this feels more like a "difference" than an "error".

I did see this comment, but I was already in the middle of working on this, and it seemed more realistic to finish this before going on PTO, than to change tack and work on bug 1425739.

I asked Markus if he could look at bug 1425739; if not, it'll be my first priority after returning from PTO.
Comment on attachment 8997757 [details]
Bug 1457586 - Refactor APZCTreeManager::ProcessTouchVelocity() and related functions to take the time and position deltas separately.

https://reviewboard.mozilla.org/r/261492/#review268594
Attachment #8997757 - Flags: review?(kats) → review+
Comment on attachment 8997616 [details]
Bug 1457586 - Introduce a VelocityTracker abstraction for handling velocity computation along an axis.

https://reviewboard.mozilla.org/r/261314/#review268596

::: gfx/layers/apz/src/SimpleVelocityTracker.cpp:7
(Diff revision 2)
> +#include <cstdint>
> +#include <utility>

nit: don't need these includes here, since they are already in the .h file
Attachment #8997616 - Flags: review?(kats) → review+
Comment on attachment 8997758 [details]
Bug 1457586 - Initialize AsyncPanZoomController::mPlatformSpecificState before the axes.

https://reviewboard.mozilla.org/r/261494/#review268602
Attachment #8997758 - Flags: review?(kats) → review+
Comment on attachment 8997617 [details]
Bug 1457586 - Allow PlatformSpecificState to choose the VelocityTracker implementation.

https://reviewboard.mozilla.org/r/261316/#review268604

Somewhat convoluted, but ok. Personally I would have made the APZC constructor do something like mX.SetVelocityTracker(...), but I don't feel strongly about it.
Attachment #8997617 - Flags: review?(kats) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) (parental leave soon) from comment #27)
> Comment on attachment 8997616 [details]
> Bug 1457586 - Introduce a VelocityTracker abstraction for handling velocity
> computation along an axis.
> 
> https://reviewboard.mozilla.org/r/261314/#review268596
> 
> ::: gfx/layers/apz/src/SimpleVelocityTracker.cpp:7
> (Diff revision 2)
> > +#include <cstdint>
> > +#include <utility>
> 
> nit: don't need these includes here, since they are already in the .h file

Fixed.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) (parental leave soon) from comment #29)
> Comment on attachment 8997617 [details]
> Bug 1457586 - Allow PlatformSpecificState to choose the VelocityTracker
> implementation.
> 
> https://reviewboard.mozilla.org/r/261316/#review268604
> 
> Somewhat convoluted, but ok. Personally I would have made the APZC
> constructor do something like mX.SetVelocityTracker(...), but I don't feel
> strongly about it.

In defense of this approach, it avoids repeating that setup code for mX and mY.
Comment on attachment 8997618 [details]
Bug 1457586 - Add an AndroidVelocityTracker class that implement Chrome's default velocity tracking strategy.

https://reviewboard.mozilla.org/r/261318/#review268606

Minor comments below, but looks good in general.

::: gfx/layers/apz/src/AndroidVelocityTracker.cpp:139
(Diff revision 3)
> + * goodness of fit of the model for the given data.  It is a value between 0
> + * and 1, where 1 indicates perfect correspondence.
> + *
> + * This function first expands the X vector to a m by n matrix A such that
> + * A[i][0] = 1, A[i][1] = X[i], A[i][2] = X[i]^2, ..., A[i][n] = X[i]^n, then
> + * multiplies it by w[i]./

nit: trailing slash

::: gfx/layers/apz/src/AndroidVelocityTracker.cpp:164
(Diff revision 3)
> +                              uint32_t n,
> +                              float* out_b,
> +                              float* out_det) {
> +  // MSVC does not support variable-length arrays (used by the original Android
> +  // implementation of this function).
> +#if defined(COMPILER_MSVC)

This file should never be getting compiled with MSVC. I don't mind if you leave this in though

::: gfx/layers/apz/src/AndroidVelocityTracker.cpp:274
(Diff revision 3)
> +  for (size_t i = 0; i <= kMaxDegree; i++) {
> +    xcoeff[i] = 0;
> +  }
> +
> +  // Iterate over movement samples in reverse time order and collect samples.
> +  float x[kHistorySize];

"pos" might be a better than than "x" since this code is used for both axes

::: gfx/layers/apz/src/AndroidVelocityTracker.cpp:304
(Diff revision 3)
> +
> +  // Calculate a least squares polynomial fit.
> +
> +  // Polynomial degree (number of coefficients), or zero if no information is
> +  // available.
> +  uint32_t degree = 2;

Let's use "kMaxDegree - 1" instead of hard-coding "2" here. I'm assuming that would be semantically correct, given the comment for kMaxDegree says ".. degree of the approximation plus 1"

::: gfx/layers/apz/src/AndroidVelocityTracker.cpp:305
(Diff revision 3)
> +  if (degree > m - 1)
> +    degree = m - 1;

nit: braces

::: gfx/layers/apz/src/AndroidVelocityTracker.cpp:311
(Diff revision 3)
> +    degree = m - 1;
> +
> +  if (degree >= 1) {  // otherwise, no velocity data available
> +    float xdet;
> +    uint32_t n = degree + 1;
> +    if (SolveLeastSquares(time, x, w, m, n, xcoeff, &xdet)) {

Since we're not using xdet, maybe we should just rip that out along with the code that computes it? Or allow passing a null for that arg and put the code that computes it behind a null guard (if you want to keep the code). As-is it's doing a bunch of easily-avoidable wasted computation.
Attachment #8997618 - Flags: review?(kats) → review+
Comment on attachment 8997759 [details]
Bug 1457586 - Implement AndroidVelocityTrakcer::HandleDynamicToolbarMovement() usefully.

https://reviewboard.mozilla.org/r/261496/#review268718
Attachment #8997759 - Flags: review?(kats) → review+
(In reply to Botond Ballo [:botond] [on PTO, back Aug 27] from comment #22)
> Markus, could you try the patches (you can use a Try build from [1]) and let
> me know if it fixes the problem as you originally reported it? Thanks!
> 
> [1]
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=24ec0cece2e7e546ef8928e186b7dca1237450d5

This try build is showing rather erratic behavior unfortunately: If I keep flinging up and down alternatingly, I can get into a state where the fling velocity and direction is completely arbitrary. For example, up/down/up may end up doing up/down/down, or it may just halt the fling entirely, or it may teleport me to the end of the page.
Flags: needinfo?(mstange)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) (parental leave) from comment #36)
> ::: gfx/layers/apz/src/AndroidVelocityTracker.cpp:139
> (Diff revision 3)
> > + * goodness of fit of the model for the given data.  It is a value between 0
> > + * and 1, where 1 indicates perfect correspondence.
> > + *
> > + * This function first expands the X vector to a m by n matrix A such that
> > + * A[i][0] = 1, A[i][1] = X[i], A[i][2] = X[i]^2, ..., A[i][n] = X[i]^n, then
> > + * multiplies it by w[i]./
> 
> nit: trailing slash

Fixed.

> ::: gfx/layers/apz/src/AndroidVelocityTracker.cpp:164
> (Diff revision 3)
> > +                              uint32_t n,
> > +                              float* out_b,
> > +                              float* out_det) {
> > +  // MSVC does not support variable-length arrays (used by the original Android
> > +  // implementation of this function).
> > +#if defined(COMPILER_MSVC)
> 
> This file should never be getting compiled with MSVC. I don't mind if you
> leave this in though

I left it in, in case we like this physics model enough that we want to use it on desktop as well.

> ::: gfx/layers/apz/src/AndroidVelocityTracker.cpp:274
> (Diff revision 3)
> > +  for (size_t i = 0; i <= kMaxDegree; i++) {
> > +    xcoeff[i] = 0;
> > +  }
> > +
> > +  // Iterate over movement samples in reverse time order and collect samples.
> > +  float x[kHistorySize];
> 
> "pos" might be a better than than "x" since this code is used for both axes

Fixed.

> ::: gfx/layers/apz/src/AndroidVelocityTracker.cpp:304
> (Diff revision 3)
> > +
> > +  // Calculate a least squares polynomial fit.
> > +
> > +  // Polynomial degree (number of coefficients), or zero if no information is
> > +  // available.
> > +  uint32_t degree = 2;
> 
> Let's use "kMaxDegree - 1" instead of hard-coding "2" here. I'm assuming
> that would be semantically correct, given the comment for kMaxDegree says
> ".. degree of the approximation plus 1"

I cleaned this up a bit, including renaming "kMaxDegree" (the "max" came from the Chromium codebase where the same data structures were used for approximations of different degrees).

> ::: gfx/layers/apz/src/AndroidVelocityTracker.cpp:305
> (Diff revision 3)
> > +  if (degree > m - 1)
> > +    degree = m - 1;
> 
> nit: braces

Fixed.

> ::: gfx/layers/apz/src/AndroidVelocityTracker.cpp:311
> (Diff revision 3)
> > +    degree = m - 1;
> > +
> > +  if (degree >= 1) {  // otherwise, no velocity data available
> > +    float xdet;
> > +    uint32_t n = degree + 1;
> > +    if (SolveLeastSquares(time, x, w, m, n, xcoeff, &xdet)) {
> 
> Since we're not using xdet, maybe we should just rip that out along with the
> code that computes it? Or allow passing a null for that arg and put the code
> that computes it behind a null guard (if you want to keep the code). As-is
> it's doing a bunch of easily-avoidable wasted computation.

I removed the parameter and the code that computes it. The Chromium code used it to compute a "confidence" value, but then didn't actually use that confidence value for anything.
Comment on attachment 8997615 [details]
Bug 1457586 - Simplify Axis::UpdateWithTouchAtDevicePoint() by removing the aAdditionalDelta parameter.

> diff --git a/gfx/layers/apz/src/AsyncPanZoomController.cpp b/gfx/layers/apz/src/AsyncPanZoomController.cpp

> @@ -2448,18 +2448,18 @@ nsEventStatus AsyncPanZoomController::On
>    MOZ_ASSERT(GetCurrentPanGestureBlock());
>    AdjustDeltaForAllowedScrollDirections(logicalPanDisplacement,
>        GetCurrentPanGestureBlock()->GetAllowedScrollDirections());
>  
>    // We need to update the axis velocity in order to get a useful display port
>    // size and position. We need to do so even if this is a momentum pan (i.e.
>    // aFingersOnTouchpad == false); in that case the "with touch" part is not
>    // really appropriate, so we may want to rethink this at some point.
> -  mX.UpdateWithTouchAtDevicePoint(aEvent.mLocalPanStartPoint.x, logicalPanDisplacement.x, aEvent.mTime);
> -  mY.UpdateWithTouchAtDevicePoint(aEvent.mLocalPanStartPoint.y, logicalPanDisplacement.y, aEvent.mTime);
> +  mX.UpdateWithTouchAtDevicePoint(mX.GetPos() + logicalPanDisplacement.x, aEvent.mTime);
> +  mY.UpdateWithTouchAtDevicePoint(mY.GetPos() + logicalPanDisplacement.y, aEvent.mTime);

Please add a comment here saying that, during a pan, we have to make all simulated positions relative to Axis::GetPos(), because the current position is an invented position, and because resetting the position to the mouse position (e.g. aEvent.mLocalPanStartPoint) would mess up velocity calculation.
Maybe we should also mention that other callers of UpdateWithTouchAtDevicePoint can't reset the position because this is the only caller of UpdateWithTouchAtDevicePoint for pan events.
Attachment #8997615 - Flags: review?(mstange) → review+
status-firefox63=fix-optional because it would be nice to uplift scrolling improvements to GeckoView 63 because scrolling issues are a common complaint for Fennec users.
Apologies for the delay here; I've been dealing with regressions from bug 656036 work which is riding 63. I will try to get this landed ASAP.
The motivation is to support velocity tracking implementations (added in a
later patch) that need the position delta rather than resulting velocity.

Also rename the functions to make it clearer that they have to do with
dynamic toolbar movement.

MozReview-Commit-ID: G0IVJHYTurB

Depends on D7654
The current velocity computation code is factored out into an implementation
called SimpleVelocityTracker.

MozReview-Commit-ID: G0VnvREdIX3

Depends on D7655
This allows the Axis constructors to use the platform-specific state.

MozReview-Commit-ID: KWtDX4XVpjF

Depends on D7656
MozReview-Commit-ID: LyW9N2H7fv7

Depends on D7657
MozReview-Commit-ID: JYqiViaucmY

Depends on D7659
I addressed comment 42, and reuploaded the patch series to Phabricator. The "unified build bustage" patch is new, the others are just "carry the r+ from Mozreview".

I haven't had any luck reproducing the issue described in comment 38 with any reliability so far.
Attachment #8997615 - Attachment is obsolete: true
Attachment #8997616 - Attachment is obsolete: true
Attachment #8997617 - Attachment is obsolete: true
Attachment #8997618 - Attachment is obsolete: true
Attachment #8997758 - Attachment is obsolete: true
Attachment #8997757 - Attachment is obsolete: true
Attachment #8997759 - Attachment is obsolete: true
(In reply to Botond Ballo [:botond] from comment #53)
> I haven't had any luck reproducing the issue described in comment 38 with
> any reliability so far.

Discussed this with Markus; with the Try build from comment 54, he can also only reproduce the issue very intermittently. The behaviour in question may be an existing issue with the dynamic toolbar which has improved due to recent dynamic toolbar related fixes.

As this was the last outstanding issue, and we have a green Try push, I believe this should be good to land.
QA Contact: kats
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/65403cf5da8a
Fix unified build bustage from bug 1420996. r=kats
https://hg.mozilla.org/integration/autoland/rev/664a7c0f8183
Simplify Axis::UpdateWithTouchAtDevicePoint() by removing the aAdditionalDelta parameter. r=mstange
https://hg.mozilla.org/integration/autoland/rev/cd68d246a34d
Refactor APZCTreeManager::ProcessTouchVelocity() and related functions to take the time and position deltas separately. r=kats
https://hg.mozilla.org/integration/autoland/rev/5d40107ce896
Introduce a VelocityTracker abstraction for handling velocity computation along an axis. r=kats
https://hg.mozilla.org/integration/autoland/rev/2ab8498738f4
Initialize AsyncPanZoomController::mPlatformSpecificState before the axes. r=kats
https://hg.mozilla.org/integration/autoland/rev/1507f6511c87
Allow PlatformSpecificState to choose the VelocityTracker implementation. r=kats
https://hg.mozilla.org/integration/autoland/rev/f266ca6d096a
Add an AndroidVelocityTracker class that implement Chrome's default velocity tracking strategy. r=kats
https://hg.mozilla.org/integration/autoland/rev/77ace67c85de
Implement AndroidVelocityTrakcer::HandleDynamicToolbarMovement() usefully. r=kats
QA Contact: kats
Thanks, Botond!

63=wontfix because this is probably too much code to uplift to 63 Beta.
Blocks: 1498329
Duplicate of this bug: 1425739
No longer blocks: 1498329
Depends on: 1498329
I haven't filed a bug yet, but I've noticed a distinct increase in 'weird' flings since this landed - flings where it'll start scrolling in the opposite direction of my swipe, or at an odd speed. Note, however, that the feel of things is otherwise much improved and feels far more like the rest of Android now.
Same as :cwiiis 
Something feels weird but its more closer to android scroll.  

I've noticed trying to do a double fling(or swipe) to immediately cover the page results in the scroll being cancelled and stopped.
Depends on: 1499163
Depends on: 1499941
No longer depends on: 1499163
Release Note Request (optional, but appreciated)
[Why is this notable]: Scrolling improvements which are a common complaint 
[Affects Firefox for Android]: Only
[Suggested wording]: Scrolling is faster and more responsive
[Links (documentation, blog post, etc)]:
relnote-firefox: --- → ?
Adding to 64 release notes as "Scrolling faster and more responsive".
You need to log in before you can comment on or make changes to this bug.