824511 - Remove Axis.cpp?mark=76,79,82#73 dead code and make Axis less sensitive to random move events distance

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Oleg Romashin (:romaxa)]

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/Axis.cpp?mark=76,79,82#73
Line 82 has code which make 73-80 lines wasting of time...

Comment 1

[Doug Sherk (:drs) (inactive)]

+cjones

I think cjones was going to patch this out or at least brought it up before. Not sure what happened to that. Also not sure if it was him or someone else.

Comment 2

[Doug Sherk (:drs) (inactive)]

For reference though, yeah, this code is clearly not working as intended. It came from here:
https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/ui/Axis.java#192

Since we don't know how important this code was and the Java code has had a lot of work put into it, I think the best way to go is remove l82 and make sure it doesn't break anything.

I can take this.

Comment 3

[Oleg Romashin (:romaxa)]

Attachment: Remove dead code.

not sure how it works in Java code, but on first velocity check, we have mVelocity == 0, and new Velocity > minimal
so we endup with second condition line 78, where maxChange become 0.

Also it does not make sense to calculate velocity when aPos == mPos...
I guess this code worked in Java because apos/mpos - are float.

Comment 4

[Doug Sherk (:drs) (inactive)]

Comment on attachment 695535 [details] [diff] [review]
Remove dead code.

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

::: gfx/layers/ipc/Axis.cpp
@@ +77,4 @@
>  
>    // If a direction change has happened, or the current velocity due to this new
>    // touch is relatively low, then just apply it. If not, throttle it.
> +  if (!mVelocity || curVelocityIsLow || (directionChange && fabs(newVelocity) - EPSILON <= 0.0f)) {

This will have unintended consequences. If the velocity is 0 and we get some garbage point that's very far away we'll accelerate very quickly to it, and the acceleration capping code won't get a chance to reduce it.

Comment 5

[Doug Sherk (:drs) (inactive)]

Assigning to romaxa as discussed on IRC.

Comment 6

[Oleg Romashin (:romaxa)]

Attachment: Use cap limit functionality for asyncPanZoomController

This should be better, just use default 1.0 value when mVelocity == 0, also change MAX_EVENT_ACCELERATION to 12 same as fennec java code

Comment 7

[Oleg Romashin (:romaxa)]

Comment on attachment 696458 [details] [diff] [review]
Use cap limit functionality for asyncPanZoomController

No this does not look right either
with this cap implementation we limiting  velocity in such way that when velocity is very low during fling (end of kinetic motion) and one more motion performed with normal speed (even fast), cap limit maxChange calculated as very low value, and does not allow to speedup fling in this case.

Comment 8

[Oleg Romashin (:romaxa)]

I got it working good only when velocity cap function was simple as this 
mVelocity = NS_MIN(NS_MAX(newVelocity, -MAX_EVENT_ACCELERATION), MAX_EVENT_ACCELERATION);

just limit it by hard...

Comment 9

[Tatiana Meshkova (:tatiana)]

Attachment: use last five velocity records

Comment 10

[Tatiana Meshkova (:tatiana)]

Attachment: use last five velocity records (v2)

CancelTouch handling is missing in previous version, fixed here.

Comment 11

[Doug Sherk (:drs) (inactive)]

Comment on attachment 712831 [details] [diff] [review]
use last five velocity records (v2)

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

Hey, sorry about the late review. I should be able to get to them much more quickly now. Here it is.

::: gfx/layers/ipc/Axis.cpp
@@ +51,5 @@
>   */
>  static float gFlingStoppedThreshold = 0.01f;
>  
> +/**
> + * Maximum size of velocity queue

I'd prefer a more useful comment explaining what the velocity queue is.

::: gfx/layers/ipc/Axis.h
@@ +9,5 @@
>  
>  #include "nsGUIEvent.h"
>  #include "mozilla/TimeStamp.h"
>  #include "mozilla/gfx/2D.h"
> +#include <queue>

I'd prefer to just use an nsTArray for this.

Comment 12

[Tatiana Meshkova (:tatiana)]

Attachment: use nsTArray for velocity queue

Comment 13

[Doug Sherk (:drs) (inactive)]

Comment on attachment 716275 [details] [diff] [review]
use nsTArray for velocity queue

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

Good. Just 2 changes.

::: gfx/layers/ipc/Axis.cpp
@@ +52,5 @@
>  static float gFlingStoppedThreshold = 0.01f;
>  
> +/**
> + * Maximum size of velocity queue. The queue contains last N velocity records.
> + * On touch end we calculate the average velocity

Fix spacing here please. This is an awkwardly placed line break.

@@ +64,5 @@
>    Preferences::AddFloatVarCache(&gFlingFriction, "gfx.axis.fling_friction", gFlingFriction);
>    Preferences::AddFloatVarCache(&gVelocityThreshold, "gfx.axis.velocity_threshold", gVelocityThreshold);
>    Preferences::AddFloatVarCache(&gAccelerationMultiplier, "gfx.axis.acceleration_multiplier", gAccelerationMultiplier);
>    Preferences::AddFloatVarCache(&gFlingStoppedThreshold, "gfx.axis.fling_stopped_threshold", gFlingStoppedThreshold);
> +  Preferences::AddIntVarCache(&gMaxVelocityQueueSize, "gfx.max_velocity_queue", gMaxVelocityQueueSize);

"gfx.axis.max_velocity_queue_size"

Comment 14

[Tatiana Meshkova (:tatiana)]

Attachment: use nsTArray for velocity queue, fixed spacing and naming

moving r+ from the previous patch

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f2c0e6e56a5

Comment 16

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/5f2c0e6e56a5