Closed Bug 907179 Opened 6 years ago Closed 6 years ago

Tune APZC displayport heuristics

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29
blocking-b2g 1.3+
Tracking Status
firefox27 --- wontfix
firefox28 - fixed
firefox29 --- fixed
b2g-v1.2 --- wontfix
b2g-v1.3 --- fixed

People

(Reporter: kats, Assigned: kats)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [beta28] [gfx])

Attachments

(2 files)

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.
Keywords: regression
Depends on: 908958
Blocks: 915723
No longer blocks: 907100
Component: Graphics: Layers → Panning and Zooming
Whiteboard: [release28]
Expanding the scope of this bug to cover Metro as well.
OS: Gonk (Firefox OS) → All
Version: 26 Branch → Trunk
Duplicate of this bug: 939573
No longer blocks: 915723
marking [beta28] since we still have visible scroll edge drawing issues.
Whiteboard: [release28] → [beta28]
Blocks: metrov1backlog, metrov1omtc&apzc
No longer blocks: metro-apzc
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.
Duplicate of this bug: 946983
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.
Flags: needinfo?(jmathies)
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.
Assignee: nobody → bugmail.mozilla
blocking-b2g: --- → 1.3+
(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.
Flags: needinfo?(jmathies) → needinfo?(bugmail.mozilla)
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.
Flags: needinfo?(bugmail.mozilla)
Based on the feedback we will not be tracking this at this time. Thank you for the prompt feedback!
Whiteboard: [beta28] → [beta28] [gfx]
Milan,

Where are we with this blocker for 1.3?

It is a blocker for QC again.
Flags: needinfo?(milan)
It is my #2 bug, after bug 947523. I plan to have it done by the end of the week.
Flags: needinfo?(milan)
Velocity is represented in screen pixels / millisecond, so using a ScreenPoint seems more appropriate than a gfx::Point.
Attachment #8357311 - Flags: review?(botond)
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.
Attachment #8357314 - Flags: review?(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.
Attachment #8357311 - Flags: review?(botond) → review+
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.
Attachment #8357314 - Flags: review?(botond) → review+
https://hg.mozilla.org/mozilla-central/rev/49c6393d37f3
https://hg.mozilla.org/mozilla-central/rev/6f522af059d3
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
No longer blocks: metrov1backlog
You need to log in before you can comment on or make changes to this bug.