907179 - Tune APZC displayport heuristics

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

With multi-APZC landed we need to tune the displayport heuristics to make sure they perform well on a variety of devices. We also need to make sure they work well for non-browser apps, as we will be turning on APZC for other apps on B2G as well.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Expanding the scope of this bug to cover Metro as well.

Comment 3

[Jim Mathies [:jimm]]

marking [beta28] since we still have visible scroll edge drawing issues.

Comment 4

[Jim Mathies [:jimm]]

Per discussions between marcom and milan, we're requesting tracking on the remaining "big issues" related to apzc for metrofx, which is riding the 28 train. We expect these will get fixed during the long aurora train ride.

This bug is related to drawing issues on view bounds when you pan a page.

Comment 6

[Benjamin Kerensa [:bkerensa]]

Would this be a no go for shipping? We need to better understand what the reason for tracking is and it would be good to have someone assigned.

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

Jim's leaving it to me to assign, I'll do that.  On the other hand, Jim can explain why this is tracking for Metro.  I can tell you that it will probably track in 28 for Firefox OS 1.3 purpose.

Comment 8

[Jim Mathies [:jimm]]

(In reply to Milan Sreckovic [:milan] from comment #7)
> Jim's leaving it to me to assign, I'll do that.  On the other hand, Jim can
> explain why this is tracking for Metro.  I can tell you that it will
> probably track in 28 for Firefox OS 1.3 purpose.

I'm not sure this needs to block anymore, the black rect at bounds problem seems to be fixed for me on mc tip. 

I'll defer to kats for a decision about blocking on this, if he feels there's something we definitely need to tweak before we roll out.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

I don't know of anything specific that needs to be changed here, but we have various reports of seeing blank/checkerboarding in cases where we can probably avoid it. This bug is more of an investigatory bug to see if we can improve the behaviour. I would say that we should turn this into a meta bug and file specific bugs for reproducible scenarios where we run into checkerboarding and make those block 1.3 or 28 as needed, and leave this one not blocking.

Comment 10

[Benjamin Kerensa [:bkerensa]]

Based on the feedback we will not be tracking this at this time. Thank you for the prompt feedback!

Comment 11

[Preeti Raghunath(:Preeti)]

Milan,

Where are we with this blocker for 1.3?

It is a blocker for QC again.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

It is my #2 bug, after bug 947523. I plan to have it done by the end of the week.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Attachment: (1/2) - Change the type of the velocity vector

Velocity is represented in screen pixels / millisecond, so using a ScreenPoint seems more appropriate than a gfx::Point.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Attachment: (2/2) - Simplify/clean up the displayport code

Read this patch as a rewrite rather than a diff. (That is, it'll be easier to just read the new code and verify it makes sense, rather than comparing it hunk-by-hunk to the old code).

I made some changes to the constants based on what we discovered on Fennec (i.e. that when scrolling fast you want the displayport to be smaller so that you have more consistent and predictable paint times) and that when scrolling slower you can afford to spend more time painting the larger displayport.

Tested this on B2G and Metro - IMO there is a small reduction in checkerboarding but it subjective enough to not really count. It doesn't appear to regress anything.

Comment 15

[Botond Ballo [:botond]]

Comment on attachment 8357311 [details] [diff] [review]
(1/2) - Change the type of the velocity vector

Review of attachment 8357311 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not thrilled about using a "point" class of any sort to represent a velocity, but this is an improvement in that it at least documents that the distance component of the velocity is in "Screen" units.

Comment 16

[Botond Ballo [:botond]]

Comment on attachment 8357314 [details] [diff] [review]
(2/2) - Simplify/clean up the displayport code

Review of attachment 8357314 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! Just a few minor comments.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +223,3 @@
>   */
> +static float gXStationarySizeMultiplier = 3.0f;
> +static float gYStationarySizeMultiplier = 3.5f;

Let's add a comment that states the rationale for having the stationary multipliers be larger than the skate multipliers.

@@ +1165,5 @@
> + * to the currently visible area and will be transformed to the area we should
> + * be drawing to minimize checkerboarding.
> + */
> +static void
> +EnlargeDisplayPortAlongAxis(float& aOffset, float& aLength,

Is there a reason to switch from pointers to references here? I thought you preferred, and the style guide recommended, pointers?

@@ +1166,5 @@
> + * be drawing to minimize checkerboarding.
> + */
> +static void
> +EnlargeDisplayPortAlongAxis(float& aOffset, float& aLength,
> +                            double aEstimatedPaintDuration, float aVelocity,

In cases where we use a double to represent a duration, I wouldn't mind either adding the units into the variable name (e.g. aEstimatedPaintDurationMilliseconds), or using mozilla::TimeDuration instead.

Comment 17

[Kartikaya Gupta (email:[e-mail])]

Address comments (and changed back to pointers) and landed:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/49c6393d37f3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6f522af059d3

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/49c6393d37f3
https://hg.mozilla.org/mozilla-central/rev/6f522af059d3

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/07e1d179fdbf
https://hg.mozilla.org/releases/mozilla-aurora/rev/ad02749a7b0a