Closed Bug 776226 Opened 10 years ago Closed 10 years ago

async scrolling: we are not skating to the puck (again)


(Core :: Graphics: Layers, defect)

Not set





(Reporter: gal, Assigned: drs)




(2 files, 3 obsolete files)

Probably a known problem. The heuristics for the viewport sizing is too trivial. This is the main cause of checkerboarding on the otoro.
Attached patch patch (obsolete) — Splinter Review
This doesn't work right for some reason. We keep rendering white content after I stop moving. I probably calculate the viewport size wrong somehow at rest. This could directly use vX to predict the new start X/Y position based on current speed, but thats a follow-up. I would recommend the static strategy first (simpler, nearly as good).
On a related note the initial displayport seems to be set to the viewport somewhere using a different code path. It is not oversized for the first paint. This causes the unpleasant delay as soon as I pan around during page load. We have to oversize that first displayport too.
Assignee: nobody → bugzilla
Blocks: 745136
Improved version of previously posted patch. Doesn't remove extra sizing when stationary and properly clips rect.

This will need some followup work to implement axis locking (so that, for example, if you're trying to pan downward, a slight movement along the x axis won't make it go in that direction).
Attachment #644617 - Attachment is obsolete: true
Attachment #644791 - Flags: review?(jones.chris.g)
Attachment #644791 - Flags: review?(jones.chris.g) → review?(gal)
Comment on attachment 644791 [details] [diff] [review]
Improve displayport sizing, account for velocity, and properly clip edges

Review of attachment 644791 [details] [diff] [review]:

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +603,5 @@
>    // each of the imaginary extra pages. The @ symbol at the top left of the
>    // viewport marks the current scroll offset. From the @ symbol to the far left
>    // and far top, it is clear that this distance is 1/4 of the displayport's
>    // height/width dimension.
> +  gfx::Rect displayPort;

how about you just initialize displayPort here with viewport * SIZE_MULTIPLIER and then mutate it below, that saves a few lines and is clear

@@ +608,5 @@
> +  // Iff there's motion along only one axis of movement, and it's above a
> +  // threshold, then we want to paint a larger area in the direction of that
> +  // motion so that it's less likely to checkerboard. Also note that the other
> +  // axis needs no larger an area for its displayport than the viewport

Also note that the other axis doesn't need its displayport enlarged beyond the viewport dimension, since it is ...

@@ +613,5 @@
> +  // dimensions, since it is impossible for it to checkerboard along that axis
> +  // until motion begins on it.
> +  if (fabsf(velocity.x) > MIN_SKATE_SPEED && fabsf(velocity.y) < MIN_SKATE_SPEED) {
> +    desiredHeight = viewport.height;
> +    displayPort = gfx::Rect(velocity.x > 0 ? 0 : -desiredWidth / 2,

You might want to oversize by 3x in the direction of travel, or anyway, a separate constant would be nice. I am sure we have to tune this.

@@ +629,4 @@
>    }
> +  gfx::Rect shiftedDisplayPort = displayPort;
> +  // To be clear, both the scroll offset and displayport are in CSS pixels.

Remove the To be clear.
Attachment #644791 - Flags: review?(gal) → review+
Looks good. I will test right now, please fix up the patch in the meantime.
Related to velocity compensation for displayport sizing. Without this patch, that code is relatively useless as we will rarely run into a case where we're panning straight up or straight down (even small amounts of movement along the opposite axis bails us out of velocity compensation for displayports).
Attachment #644804 - Flags: review?(gal)
Review comments fixed. Carrying r+, but feel free to comment.
Attachment #644791 - Attachment is obsolete: true
Attachment #644811 - Flags: review+
Attachment #644804 - Flags: review?(gal) → review?(roc)
Comment on attachment 644804 [details] [diff] [review]
Implement axis locking when panning along one axis

Review of attachment 644804 [details] [diff] [review]:

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +18,5 @@
>  namespace layers {
>  static const float EPSILON = 0.0001;
> +static const float PI = 3.1415f;

#ifndef M_PI
#define M_PI 3.14159265358979323846

like the other places ... and, file a goodfirstbug to consolidate our definitions of M_PI, probably to a new mfbt/Math.h
Attachment #644804 - Flags: review?(roc) → review+
r+ carried

Also, see bug 776429.
Attachment #644804 - Attachment is obsolete: true
Attachment #644821 - Flags: review+
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Depends on: 781414
No longer depends on: 781414
You need to log in before you can comment on or make changes to this bug.