Closed Bug 1236046 Opened 4 years ago Closed 4 years ago

[checkerboard-experiment] Velocity-based display port position prediction is erratic

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox46 --- fixed
firefox47 --- fixed

People

(Reporter: mstange, Assigned: kats)

References

Details

Attachments

(2 files, 1 obsolete file)

Steps to reproduce:
 0. Turn on apz.minimap.enabled.
 1. Go to http://hpmor.com/chapter/6 and scroll to the middle of the page.
 2. On your Macbook Pro touchpad, alternatingly scroll up and down, lifting your
    fingers from the touchpad every time you change the scroll direction.
    Basically a series of small flings.
 3. Observe the display port position in the minimap.

The displayport sometimes randomly jumps to the top or the bottom of the page, and in general looks very twitchy.

Setting apz.velocity_bias to zero is a workaround that works.
This is basically a more general description of the specific bug 1235906 that Jeff filed yesterday. I'll dupe that over.
Once the telemetry is in we can land a patch to drop the velocity bias to 0 and see how it affects things.
Depends on: 1238040
Summary: Velocity-based display port position prediction is erratic → [checkerboard-experiment] Velocity-based display port position prediction is erratic
So I dug into this a bit more and the first move event definitely can generate some very crazy velocities. I expanded the logging in Axis::UpdateWithTouchAtDevicePoint to also log the distance and time deltas that result in the |newVelocity| computation and I see that sometimes the time delta is 1ms (which is what results in the giant velocities) and in fact sometimes it is -1ms. Individual move events tend to be a more reasonable 16-17ms apart. Fixing the velocity computation should fix this bug, I think.
Assignee: nobody → bugmail.mozilla
Attached patch Patch (obsolete) — Splinter Review
Can you try this patch and see if you still encounter the problem? I don't see it any more with this patch applied. (Note you'll want to set velocity bias back to 1.0 to make sure).
Attachment #8713373 - Flags: review?(mstange)
Comment on attachment 8713373 [details] [diff] [review]
Patch

Markus is going on PTO. Jeff, can you apply this patch and see if it fixes your velocity bias problems?
Attachment #8713373 - Flags: review?(mstange) → feedback?(jmuizelaar)
Here's a more cleaned-up patch that can actually be landed.
Attachment #8713373 - Attachment is obsolete: true
Attachment #8713373 - Flags: feedback?(jmuizelaar)
Attachment #8715316 - Flags: review?(botond)
Attachment #8715316 - Flags: review?(botond) → review+
https://hg.mozilla.org/mozilla-central/rev/17a70b41806c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
This is in the Feb05 nightly onwards, so we should check telemetry after a few days to see if it made a difference in checkerboarding.
Based on the telemetry data I can't see a visible improvement. However it also didn't make anything worse, and while scrolling around on pages I see the displayport jump erratically much less, so I think this is worth uplifting anyway.
Comment on attachment 8715316 [details] [diff] [review]
Patch (cleaned up)

Approval Request Comment
[Feature/regressing bug #]: APZ
[User impact if declined]: When starting a scroll the displayport can jump erratically, causing checkerboarding. This patch fixes the velocity computation which fixes the erratic jumps, and should reduce instances of checkerboarding.
[Describe test coverage new/current, TreeHerder]: locally, on m-c for a while now
[Risks and why]: low risk, it just avoids sampling velocity over very small timespans. Code is well understood.
[String/UUID change made/needed]: none
Attachment #8715316 - Flags: approval-mozilla-aurora?
Comment on attachment 8715316 [details] [diff] [review]
Patch (cleaned up)

Sounds like this may avoid some apz scrolling issues, please uplift to aurora.
Attachment #8715316 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.