798539 - Jerkiness at the end of panning

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Benoit Girard (:BenWa)]

Attachment: systrace screenshot, no frames skipped

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.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: tracefile (use webkit for -webkit prefix)

Comment 2

[Benoit Girard (:BenWa)]

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.

Comment 3

[Benoit Girard (:BenWa)]

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.

Comment 4

[Patrick Walton (:pcwalton)]

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

Comment 5

[Kartikaya Gupta (email:[e-mail])]

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

Comment 6

[Benoit Girard (:BenWa)]

Attachment: Diagnose patch (Not a fix)

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

Comment 7

[Benoit Girard (:BenWa)]

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

Comment 8

[Benoit Girard (:BenWa)]

(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.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 10

[Benoit Girard (:BenWa)]

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.

Comment 11

[Benoit Girard (:BenWa)]

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.

Comment 12

[Benoit Girard (:BenWa)]

Attachment: Run timer at 250 Hz

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 13

[Benoit Girard (:BenWa)]

Attachment: Run timer at 250 Hz

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

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?

Comment 15

[Benoit Girard (:BenWa)]

(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)

Comment 16

[Benoit Girard (:BenWa)]

Attachment: Run timer at 250 Hz

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

Comment 17

[Benoit Girard (:BenWa)]

Attachment: Run timer at 250 Hz

Comment 18

[Kartikaya Gupta (email:[e-mail])]

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?

Comment 19

[Benoit Girard (:BenWa)]

I'll take a look, thanks Kats.

Comment 20

[Benoit Girard (:BenWa)]

Attachment: Run timer at 250 Hz

Fixed overscroll

Comment 21

[Kartikaya Gupta (email:[e-mail])]

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 22

[Benoit Girard (:BenWa)]

Attachment: Run timer at 250 Hz

Comment 23

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 24

[Benoit Girard (:BenWa)]

Attachment: Run timer at 250 Hz

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

Comment 25

[Benoit Girard (:BenWa)]

Attachment: Run timer at 250 Hz

Comment 26

[Alex Keybl [:akeybl]]

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 27

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 670152 [details] [diff] [review]
Run timer at 250 Hz

Looks good now, thanks!

Comment 28

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=5293f8e27789

Comment 29

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c58bfc3f6b48

Comment 30

[Benoit Girard (:BenWa)]

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 :(.

Comment 31

[Benoit Girard (:BenWa)]

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.

Comment 32

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c58bfc3f6b48

Comment 33

[Benoit Girard (:BenWa)]

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?

Comment 34

[Benoit Girard (:BenWa)]

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.

Comment 35

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

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?

Comment 36

[Benoit Girard (:BenWa)]

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

Comment 37

[Mark Rejhon]

(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.

Comment 38

[Mark Rejhon]

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.

Comment 39

[BMO Automation]

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.