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

RESOLVED FIXED in mozilla17

Status

()

Core
Graphics: Layers
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gal, Assigned: drs)

Tracking

Trunk
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
Probably a known problem. The heuristics for the viewport sizing is too trivial. This is the main cause of checkerboarding on the otoro.
(Reporter)

Comment 1

5 years ago
Created attachment 644617 [details] [diff] [review]
patch

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

Comment 2

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

Updated

5 years ago
Blocks: 745136
(Assignee)

Comment 3

5 years ago
Created attachment 644791 [details] [diff] [review]
Improve displayport sizing, account for velocity, and properly clip edges

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

Updated

5 years ago
Attachment #644791 - Flags: review?(jones.chris.g) → review?(gal)
(Reporter)

Comment 4

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

Comment 5

5 years ago
Looks good. I will test right now, please fix up the patch in the meantime.
(Assignee)

Comment 6

5 years ago
Created attachment 644804 [details] [diff] [review]
Implement axis locking when panning along one axis

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

Comment 7

5 years ago
Created attachment 644811 [details] [diff] [review]
Improve displayport sizing, account for velocity, and properly clip edges

Review comments fixed. Carrying r+, but feel free to comment.
Attachment #644791 - Attachment is obsolete: true
Attachment #644811 - Flags: review+
(Assignee)

Comment 8

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb066d5e9f7b
Whiteboard: DON'T CLOSE
(Assignee)

Updated

5 years ago
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
#endif

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

Comment 10

5 years ago
Created attachment 644821 [details] [diff] [review]
Implement axis locking when panning along one axis

r+ carried

Also, see bug 776429.
Attachment #644804 - Attachment is obsolete: true
Attachment #644821 - Flags: review+
(Assignee)

Comment 11

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/058d3e6887cd
Whiteboard: DON'T CLOSE
https://hg.mozilla.org/mozilla-central/rev/eb066d5e9f7b
https://hg.mozilla.org/mozilla-central/rev/058d3e6887cd
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17

Updated

5 years ago
Depends on: 781414
No longer depends on: 781414
You need to log in before you can comment on or make changes to this bug.