Last Comment Bug 776226 - async scrolling: we are not skating to the puck (again)
: async scrolling: we are not skating to the puck (again)
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics: Layers (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Doug Sherk (:drs) (inactive)
:
Mentors:
Depends on:
Blocks: 745136
  Show dependency treegraph
 
Reported: 2012-07-21 03:29 PDT by Andreas Gal :gal
Modified: 2012-08-09 15:08 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (3.23 KB, patch)
2012-07-21 03:30 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
Improve displayport sizing, account for velocity, and properly clip edges (6.01 KB, patch)
2012-07-22 13:54 PDT, Doug Sherk (:drs) (inactive)
gal: review+
Details | Diff | Splinter Review
Implement axis locking when panning along one axis (11.83 KB, patch)
2012-07-22 17:31 PDT, Doug Sherk (:drs) (inactive)
roc: review+
Details | Diff | Splinter Review
Improve displayport sizing, account for velocity, and properly clip edges (6.30 KB, patch)
2012-07-22 17:58 PDT, Doug Sherk (:drs) (inactive)
bugzilla: review+
Details | Diff | Splinter Review
Implement axis locking when panning along one axis (11.85 KB, patch)
2012-07-22 20:48 PDT, Doug Sherk (:drs) (inactive)
bugzilla: review+
Details | Diff | Splinter Review

Description Andreas Gal :gal 2012-07-21 03:29:00 PDT
Probably a known problem. The heuristics for the viewport sizing is too trivial. This is the main cause of checkerboarding on the otoro.
Comment 1 Andreas Gal :gal 2012-07-21 03:30:28 PDT
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).
Comment 2 Andreas Gal :gal 2012-07-21 03:40:13 PDT
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.
Comment 3 Doug Sherk (:drs) (inactive) 2012-07-22 13:54:22 PDT
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).
Comment 4 Andreas Gal :gal 2012-07-22 14:11:09 PDT
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.
Comment 5 Andreas Gal :gal 2012-07-22 14:11:29 PDT
Looks good. I will test right now, please fix up the patch in the meantime.
Comment 6 Doug Sherk (:drs) (inactive) 2012-07-22 17:31:54 PDT
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).
Comment 7 Doug Sherk (:drs) (inactive) 2012-07-22 17:58:51 PDT
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.
Comment 8 Doug Sherk (:drs) (inactive) 2012-07-22 18:53:40 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb066d5e9f7b
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-22 20:23:34 PDT
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
Comment 10 Doug Sherk (:drs) (inactive) 2012-07-22 20:48:37 PDT
Created attachment 644821 [details] [diff] [review]
Implement axis locking when panning along one axis

r+ carried

Also, see bug 776429.
Comment 11 Doug Sherk (:drs) (inactive) 2012-07-22 20:51:51 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/058d3e6887cd

Note You need to log in before you can comment on or make changes to this bug.