Even on a device like a GN we jerk at the end of a pan. Even after I disable uploads and try to trace it using systrace I can't find evidence that it's because we're missing a frame. I'm starting to think it has to do with the offsets that the PZC is giving us that are jerky. Attached is a screenshot of systrace. The bottom section shows our composting being very steady (ideal) and waiting for vsync.

A quick look at the scroll offsets when doing async pan appears to show they are not smooth. I'll do more analysis on Monday.

Here's better analysis: https://docs.google.com/spreadsheet/ccc?key=0AvYoHvO43KG_dGItOFM3bTJZdmFxa0dfM1FYQXBOUFE We're compositing every 16-18ms, enough to hit the vsync everytime, yet the scroll offsets jump on 4 occasions. This is enough to give the impression of jerky panning.

Fascinating, thanks for doing this analysis. I always figured this was due to texture upload, but it might be something way simpler...

At what point in the code are you reading the offsets from?

Here's how I did it. (Ignore the change with pthread priority it doesn't help). 1) Only do one upload 2) Dump time+offset after SyncViewport

Tracking all upcoming release. The fix for this bug has the potential to have a good win/risk ratio.

(In reply to Patrick Walton (:pcwalton) from comment #4) > I always figured this was due > to texture upload, but it might be something way simpler... I think we have two problems instead of one. It does explain why I saw jank didn't always correlate with uploads.

Ah ok. So the most likely cause of the offset jump is then that the two timer threads (the one that calculates the fling animation and the compositor thread) are not synced up, and we're doing two fling animation updates between two compositor updates. If we moved the fling animation code onto the compositor thread it should in theory fix this while also reducing overhead. We just need to make sure we keep everything thread-safe, which is the tricky bit.

I'm experimenting with increasing the timer resolution. This will confirm the problem and also produce a patch which will be safe enough to uplift to aurora beta. After which we can design a proper patch and let it ride the trains.

Here's the analysis of a test run I did with a 250 Hz timer: https://docs.google.com/spreadsheet/ccc?key=0AvYoHvO43KG_dEF5ZXZkbzE1N2Ita1FvNk5NMS1FeFE The hick up happen more often but as much smaller and the improvement is noticeable with and without an A/B test.

Here's a 'hack' to the problem which we can uplift to aurora+beta to ship ASAP. It's not much worse then the currently implement, the timer only run while panning/fling. The proper fix will be to remove the animation timer and calculate the exact viewport when calling SyncViewport but that fix will be risky so I'd like to land this on central ASAP, then move to aurora beta shortly then drop a more involve fix that can ride the train. NOTE: The friction values still need adjustments.

Comment on attachment 669786 [details] [diff] [review] Run timer at 250 Hz Review of attachment 669786 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/ui/Axis.java @@ +87,5 @@ > } > > static void setPrefs(Map<String, Integer> prefs) { > + FRICTION_SLOW = getFloatPref(prefs, PREF_SCROLLING_FRICTION_SLOW, 993); > + FRICTION_FAST = getFloatPref(prefs, PREF_SCROLLING_FRICTION_FAST, 995); please file a follow up to get final values for these ::: mobile/android/base/ui/PanZoomController.java @@ +578,5 @@ > mAnimationRunnable = runnable; > mAnimationTimer.scheduleAtFixedRate(new TimerTask() { > @Override > public void run() { mTarget.post(runnable); } > + }, 0, (int)Axis.MS_PER_FRAME); MS_PER_FRAME is set to 1000.0f / 60.0f, so this isn't actually changing. Is that the intent?

(In reply to Brad Lassey [:blassey] from comment #14) > MS_PER_FRAME is set to 1000.0f / 60.0f, so this isn't actually changing. Is > that the intent? Note that I've also changed MS_PER_FRAME to 4 ms (250Hz)

This has the adjusted friction values and is ready to land.

Comment on attachment 669982 [details] [diff] [review] Run timer at 250 Hz I forgot to check: is the bounce animation also affected? The bounce animation takes 16 frames of animation, which previously would take about 1 second. With your changes it will probably be much faster, no?

I'll take a look, thanks Kats.

Fixed overscroll

Comment on attachment 670034 [details] [diff] [review] Run timer at 250 Hz Review of attachment 670034 [details] [diff] [review]: ----------------------------------------------------------------- Minusing this version based on the discussion we had, some of the other variables need to be adjusted as well. ::: mobile/android/base/ui/Axis.java @@ +85,5 @@ > } > }); > } > > + public static final float MS_PER_FRAME = 4.0f; nit: make this package-scope (just leave out the "public" keyword) @@ +92,5 @@ > + // > + // ln FRICTION > + // ------ > + // 16 / MS_PER_FRAME > + // x = e This equation is really hard to read. I mis-read it in two different ways before I realized what it was saying. I'd prefer if you just wrote it as FRICTION_ADJUSTED = e^(ln(FRICTION) / (16 / MS_PER_FRAME))

Comment on attachment 670129 [details] [diff] [review] Run timer at 250 Hz Review of attachment 670129 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/ui/Axis.java @@ +83,5 @@ > } > }); > } > > + public static final float MS_PER_FRAME = 4.0f; nit: remove "public" from this @@ +89,5 @@ > + // The values we use for friction are based on a 16.6ms frame, adjust them to MS_PER_FRAME: > + // FRICTION^1 = FRICTION_ADJUSTED^(16/MS_PER_FRAME) > + // FRICTION_ADJUSTED = e ^ ((ln(FRICTION))/(16/MS_PER_FRAME)) > + static float getFrameAdjustedFriction(float baseFriction) { > + return (float)Math.pow(Math.E, (Math.log(baseFriction) / (1000f/60f / Axis.MS_PER_FRAME))); The 16/MS_PER_FRAME gets used in three different places. It's probably worth it to add a class variable like so: private static final float FRAMERATE_MULTIPLIER = (1000f/60f) / MS_PER_FRAME; and use that here... @@ +96,4 @@ > static void setPrefs(Map<String, Integer> prefs) { > + FRICTION_SLOW = getFrameAdjustedFriction(getFloatPref(prefs, PREF_SCROLLING_FRICTION_SLOW, 850)); > + FRICTION_FAST = getFrameAdjustedFriction(getFloatPref(prefs, PREF_SCROLLING_FRICTION_FAST, 970)); > + VELOCITY_THRESHOLD = 10 / (1000f/60) * MS_PER_FRAME; ... and here "VELOCITY_THRESHOLD = 10 / FRAMERATE_MULTIPLIER;" ... @@ +185,5 @@ > > // If there's a direction change, or current velocity is very low, > // allow setting of the velocity outright. Otherwise, use the current > // velocity and a maximum change factor to set the new velocity. > + boolean curVelocityIsLow = Math.abs(mVelocity) < 1.0f * (1000/16/MS_PER_FRAME); ... and here. In fact, this one is wrong, I think. I think we were using 0.25f when we tested it, but 1.0f * (1000/16/MS_PER_FRAME) gives 15.625 (assuming everything gets treated as floats). This should be 1.0f / FRAMERATE_MULTIPLIER which gives 0.24.

Good catch. You saved me a bunch of time by catching these problems early.

Not clear this is a recent regression. Please nominate for uplift once you have a fix, and make sure to provide a risk assessment.

Comment on attachment 670152 [details] [diff] [review] Run timer at 250 Hz Looks good now, thanks!

This is responsible for tpan regression that I'm looking into. I'm leaving on central until we can get a run of eideticker against it. I tried reproducing the problem on the sholes and I don't notice a regression :(.

I'm looking at the code for the test. It looks like it grabs a start time then each frames is the delta between that and the start time. Then we process this data and look for gabs which contribute to our score. This test assumes that we're continuously panning+compositing during this period which likely isn't true with this patch.

It wasn't merged in time for the nightly. If we can't find this regression then I might just skip the increase timer solution that we could of uplift to beta and just have the real fix ride the trains. Would anyone be upset waiting for 17 weeks for that fix to ship?

I haven't noticed anything bad on Eideticker but I'm re-opening because of the pending regression. It would be useful if someone has time to test on a variety of devices, particularly low ends, (GN has been tested extensively already) with a recent nightly and a nightly before this patch landed and look for the fling smoothness. Without this patch every 10 frame should jump/jerk noticeably. With this patch the jumping should be much smaller.

It's not clear to me what needs to be done here. Are we looking for some sort of video to be taken in comparison? What should the base line be if so? before/after patch?

No video is needed. Just a manual compare with the before/after.

(In reply to Kartikaya Gupta (:kats) from comment #23) > @@ +89,5 @@ > > + // The values we use for friction are based on a 16.6ms frame, adjust them to MS_PER_FRAME: > > + // FRICTION^1 = FRICTION_ADJUSTED^(16/MS_PER_FRAME) > > + // FRICTION_ADJUSTED = e ^ ((ln(FRICTION))/(16/MS_PER_FRAME)) > > + static float getFrameAdjustedFriction(float baseFriction) { > > + return (float)Math.pow(Math.E, (Math.log(baseFriction) / (1000f/60f / Axis.MS_PER_FRAME))); > > The 16/MS_PER_FRAME gets used in three different places. It's probably worth > it to add a class variable like so: > private static final float FRAMERATE_MULTIPLIER = (1000f/60f) / > MS_PER_FRAME; and use that here... It's worth mentioning that on Chrome, Safari and Internet Explorer, all animations are synchronized to VSYNC (both Canvas2D, WebGL, smooth scroll animation, precision timing of requestAnimationFrame() -- all of them synchronized to VSYNC), since IE9+, Chrome19+, and Safari6+ (even on iOS). I'm surprized Mozilla hasn't nailed the "complete subsystem VSYNC" problem yet. In fact, a newer version of Chrome VSYNC's on Android, too! I suggest paying close attention to this one too: https://bugzilla.mozilla.org/show_bug.cgi?id=707884 Note: I've tested some advanced jitter behaviours. Apparently, due to window compositing managers, it doesn't have to be perfect synchronization to VSYNC -- as long as it's +/- a few milliseconds -- the compositing will cleanly bring it on time to VSYNC, even if frames are rendered at random jitters at +/- 3-4ms of VSYNC timing. As long as it's within a predetermined jitter (Safari more sensitive, Chrome less sensitive), the synchronization is 'perfect' - perfect scroll movements occur as long as the frames are generated fast enough. (on a Core i7 with a good GPU, most browser frames are rendered in less than 1 millisecond). Also, note, some of us (myself included), now use a 120Hz-native computer monitor -- Asus VG236H, Asus VG278H, Samsung S23A700D, Benq XL2420T, Acer GD235HZ, etc. Chrome works beautifully on those, while Mozilla has more motion blur due to its 60Hz compositing framerate limit.

P.S. I'm trying to contact the appropriate W3C workgroup for framerate limiter standardization and vsync standardization (e.g. window.maxFrameRate and window.syncOnVsync). For information why, see #707884.

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.