If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Jerkiness at the end of panning

REOPENED
Unassigned

Status

()

Firefox for Android
Graphics, Panning and Zooming
REOPENED
5 years ago
a year ago

People

(Reporter: BenWa, Unassigned)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 19
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox17-, firefox18-, firefox19-)

Details

Attachments

(4 attachments, 7 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 668592 [details]
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.
(Reporter)

Comment 1

5 years ago
Created attachment 668593 [details]
tracefile (use webkit for -webkit prefix)
(Reporter)

Comment 2

5 years ago
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.
(Reporter)

Comment 3

5 years ago
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.
(Reporter)

Updated

5 years ago
Blocks: 749062
(Reporter)

Updated

5 years ago
Depends on: 711959
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?
(Reporter)

Comment 6

5 years ago
Created attachment 669600 [details] [diff] [review]
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
(Reporter)

Comment 7

5 years ago
Tracking all upcoming release. The fix for this bug has the potential to have a good win/risk ratio.
tracking-firefox17: --- → ?
tracking-firefox18: --- → ?
tracking-firefox19: --- → ?
(Reporter)

Comment 8

5 years ago
(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.
(Reporter)

Comment 10

5 years ago
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.
(Reporter)

Comment 11

5 years ago
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.
(Reporter)

Comment 12

5 years ago
Created attachment 669783 [details] [diff] [review]
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.
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Attachment #669783 - Flags: review?(blassey.bugs)
(Reporter)

Comment 13

5 years ago
Created attachment 669786 [details] [diff] [review]
Run timer at 250 Hz
Attachment #669783 - Attachment is obsolete: true
Attachment #669783 - Flags: review?(blassey.bugs)
Attachment #669786 - Flags: review?(blassey.bugs)
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?
(Reporter)

Comment 15

5 years ago
(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)
(Reporter)

Comment 16

5 years ago
Created attachment 669982 [details] [diff] [review]
Run timer at 250 Hz

This has the adjusted friction values and is ready to land.
Attachment #669982 - Flags: review?(blassey.bugs)
(Reporter)

Comment 17

5 years ago
Created attachment 669983 [details] [diff] [review]
Run timer at 250 Hz
Attachment #669786 - Attachment is obsolete: true
Attachment #669982 - Attachment is obsolete: true
Attachment #669786 - Flags: review?(blassey.bugs)
Attachment #669982 - Flags: review?(blassey.bugs)
Attachment #669983 - Flags: review?(blassey.bugs)
Attachment #669983 - Flags: review?(blassey.bugs) → review+
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?
(Reporter)

Comment 19

5 years ago
I'll take a look, thanks Kats.
(Reporter)

Comment 20

5 years ago
Created attachment 670034 [details] [diff] [review]
Run timer at 250 Hz

Fixed overscroll
Attachment #669983 - Attachment is obsolete: true
Attachment #670034 - Flags: review?(bugmail.mozilla)
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))
Attachment #670034 - Flags: review?(bugmail.mozilla) → review-
(Reporter)

Comment 22

5 years ago
Created attachment 670129 [details] [diff] [review]
Run timer at 250 Hz
Attachment #670034 - Attachment is obsolete: true
Attachment #670129 - Flags: review?(bugmail.mozilla)
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.
(Reporter)

Comment 24

5 years ago
Created attachment 670151 [details] [diff] [review]
Run timer at 250 Hz

Good catch. You saved me a bunch of time by catching these problems early.
Attachment #670129 - Attachment is obsolete: true
Attachment #670129 - Flags: review?(bugmail.mozilla)
Attachment #670151 - Flags: review?(bugmail.mozilla)
(Reporter)

Comment 25

5 years ago
Created attachment 670152 [details] [diff] [review]
Run timer at 250 Hz
Attachment #670151 - Attachment is obsolete: true
Attachment #670151 - Flags: review?(bugmail.mozilla)
Attachment #670152 - Flags: review?(bugmail.mozilla)
Not clear this is a recent regression. Please nominate for uplift once you have a fix, and make sure to provide a risk assessment.
tracking-firefox17: ? → -
tracking-firefox18: ? → -
tracking-firefox19: ? → -
Comment on attachment 670152 [details] [diff] [review]
Run timer at 250 Hz

Looks good now, thanks!
Attachment #670152 - Flags: review?(bugmail.mozilla) → review+
(Reporter)

Comment 28

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=5293f8e27789
(Reporter)

Comment 29

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c58bfc3f6b48
(Reporter)

Comment 30

5 years ago
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 :(.
(Reporter)

Comment 31

5 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/c58bfc3f6b48
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
(Reporter)

Comment 33

5 years ago
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?
(Reporter)

Comment 34

5 years ago
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.
Status: RESOLVED → REOPENED
Keywords: qawanted
Resolution: FIXED → ---

Updated

5 years ago
Depends on: 802400
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?
QA Contact: nhirata.bugzilla
(Reporter)

Comment 36

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

Comment 37

5 years ago
(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

5 years ago
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.

Updated

5 years ago
Keywords: qawanted
(Reporter)

Updated

5 years ago
Assignee: bgirard → nobody
You need to log in before you can comment on or make changes to this bug.