538682 - Skip frames during kinetic panning

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Panning/Zooming, defect)

Description

[Benjamin Stover (:stechz)]

Attachment: Patch

Helps with animation during intensive operations, like loading pages, while still being smooth in optimal conditions.

Comment 1

[Roy Frostig :froystig]

Comment on attachment 420829 [details] [diff] [review]
Patch

This

>+  this._pos = new Point(0, 0);
>+  this._vel = new Point(0, 0);

means that later in the timer callback we do things like

>+        let v0 = self._vel;
>+        let a = self._acc;
[...]
>+        let x = v0.clone().scale(t).add(a.clone().scale(.5 * t * t));

where |clone()| causes a few new objects to be created in the callback, which is supposedly called often.  We want to avoid GCs during pan.  Does this make a big difference?  Fine change if not, but if so it might be worth the pain to use explicit numeric values or give clone() a destination object.


r+ on the actual patch in any case.

Comment 2

[Benjamin Stover (:stechz)]

Attachment: Code review

Addresses Roy's thoughts, added a few comments.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 420915 [details] [diff] [review]
Code review

># HG changeset patch
># User Benjamin Stover <[e-mail]>
># Date 1263072604 28800
># Node ID 360880408962e974efd324585ba5ab50cdf84fc2
># Parent  a3a539a74507902c73090e8f7245c3d952af27a6
>[mq]: velocity
>
>diff --git a/app/mobile.js b/app/mobile.js
>--- a/app/mobile.js
>+++ b/app/mobile.js
>@@ -350,19 +350,20 @@ pref("javascript.options.mem.gc_frequenc
> 
> pref("dom.max_chrome_script_run_time", 30);
> pref("dom.max_script_run_time", 20);
> 
> // JS error console
> pref("browser.console.showInPanel", false);
> 
> // kinetic tweakables
>-pref("browser.ui.kinetic.updateInterval", 33);
>-pref("browser.ui.kinetic.ema.alphaValue", 8);
>-pref("browser.ui.kinetic.decelerationRate", 15);
>+pref("browser.ui.kinetic.updateInterval", 30);
>+pref("browser.ui.kinetic.decelerationRate", 20);
>+pref("browser.ui.kinetic.speedSensitivity", 80);
>+pref("browser.ui.kinetic.swipeLength", 160);
> 
> // Disable default plugin
> pref("plugin.default_plugin_disabled", true);
> 
> // product URLs
> // pref("app.releaseNotesURL", "http://www.mozilla.com/%LOCALE%/mobile/%VERSION%/releasenotes/");
> pref("app.releaseNotesURL", "http://www.mozilla.org/projects/fennec/%VERSION%/releasenotes/");
> //pref("app.support.baseURL", "http://support.mozilla.com/1/mobile/%VERSION%/%OS%/%LOCALE%/");
>diff --git a/chrome/content/InputHandler.js b/chrome/content/InputHandler.js
>--- a/chrome/content/InputHandler.js
>+++ b/chrome/content/InputHandler.js
>@@ -37,34 +37,22 @@
>  * use your version of this file under the terms of the MPL, indicate your
>  * decision by deleting the provisions above and replace them with the notice
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
>-// how much movement input to take before mouse up for calculating kinetic speed
>-const kSwipeLength = 160;
>-
> // how many msecs elapse before two taps are not a double tap
> const kDoubleClickInterval = 400;
> 
> // threshold in pixels for sensing a tap as opposed to a pan
> const kTapRadius = 25;
> 
>-// how many milliseconds between each kinetic pan update
>-const kKineticUpdateInterval = 25;
>-
>-// How much speed is removed every update
>-const kDecelerationRate = .09;
>-
>-// How sensitive kinetic scroll is to mouse movement
>-const kSpeedSensitivity = 1.1;
>-
> // Same as NS_EVENT_STATE_ACTIVE from nsIEventStateManager.h
> const kStateActive = 0x00000001;
> 
> /**
>  * InputHandler
>  *
>  * The input handler is an arbiter between the Fennec chrome window inputs and any
>  * registered input modules.  It keeps an array of input module objects.  Incoming
>@@ -996,111 +984,114 @@ DragData.prototype = {
>  * aPanBy is a function that will be called with the dx and dy
>  * generated by the kinetic algorithm.  It should return true if the
>  * object was panned, false if there was no movement.
>  */
> function KineticController(aPanBy, aEndCallback) {
>   this._panBy = aPanBy;
>   this._timer = null;
>   this._beforeEnd = aEndCallback;
>+  this._pos = new Point(0, 0);
>+  this._vel = new Point(0, 0);
>+  this._acc = new Point(0, 0);
>+  this._t = 0;
>+  this._t0 = 0;

These abbreviated variable names kill me, and make it hard to read the code. Point isn't exactly the best class to represent velocity and acceleration either.

Very little in the way of comments so far to help caveman programmers like me figure out what's going on.

>   _startTimer: function _startTimer() {

>+    // Temporary bins so that we don't create new objects during pan
>+    let aBin = new Point(0, 0);
>+    let v0Bin = new Point(0, 0);

Bins ?

>+    let v0 = this._vel;
>+    let a = this._acc;

v0 and a -> we are getting smaller names! is there a v1? why v0? why not just v?

>+        // x(t) = v0*t + .5*t^2*a
>+        // Distance moved is indepedent of the time between each frame.

independent

>+        let x = v0Bin.set(self._vel).scale(t).add(aBin.set(self._acc).scale(.5 * t * t));

I'm old school. I like leading 0 on decimals (0.5)

Comments got better in the hard parts, but this code is complicated enough to almost need a froystig ASCII art drawing

I haven't run this patch long enough in my build to shake out the edge cases, but it's working. If you can get the naming and leading zero nits addressed that'd be swell.

Comment 4

[Benjamin Stover (:stechz)]

> >+  this._t = 0;
> >+  this._t0 = 0;
> 
> These abbreviated variable names kill me, and make it hard to read the code.
> Point isn't exactly the best class to represent velocity and acceleration
> either.

If you think of a point as a vector it makes a lot of sense.  Because I slipped into mechanical physics mode I started using shorter names, sorry.  Things that end with 0 is a typical notation for "initial", so t0 is initial time and v0 is initial velocity.

> Very little in the way of comments so far to help caveman programmers like me
> figure out what's going on.

Could you be more specific?  Are there places in the diff that are missing comments that could use them, are the explanations unclear, or are the explanations lacking important information?

> Bins ?

This addressed Roy's comment of not creating new objects every callback.  That's a good example of critical information I didn't include.

> v0 and a -> we are getting smaller names! is there a v1? why v0? why not just
> v?

See above.

> I'm old school. I like leading 0 on decimals (0.5)

Sure.  Consistency is good.

> Comments got better in the hard parts, but this code is complicated enough to
> almost need a froystig ASCII art drawing

Hmm, I'm no artist like Roy but I could try :)

Comment 5

[Benjamin Stover (:stechz)]

Attachment: Code review 2

Carrying r+ from IRC with mfinkle

Comment 6

[Benjamin Stover (:stechz)]

Pushed http://hg.mozilla.org/mobile-browser/rev/736269688cd5

Comment 7

[Dão Gottwald [:dao]]

bugspam