967502 - Add a pref to disallow checkerboarding

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Chris Lord [:cwiiis]]

So at the moment, it's allowed for our asynchronous scroll position to vary from the last content scroll position enough that you can see areas we haven't rendered yet. I pose that if we never want checkerboarding, then we can just not allow it.

About to attach a patch that gets rid of checkerboarding - in the situations where we would have checkerboarded, this patch will cause us to experience 'jank' instead. In my short testing, it's actually quite nice - the perfect render coherency makes things feel more 'solid'.

We may want to consider turning this on *and* working to reduce the situations where we'd checkerboard so that it rarely gets triggered on b2g.

I also propose that we may want this always on on desktop too, where the times we checkerboard are very brief to the point that dropping a frame or two might be more desirable.

Comment 1

[Chris Lord [:cwiiis]]

Attachment: Add a pref that can disable checkerboarding

Comment 2

[Chris Lord [:cwiiis]]

Attachment: Disallow checkerboarding on b2g

For testing.

Comment 3

[Chris Lord [:cwiiis]]

Attachment: Disallow checkerboarding on Metro

Comment 4

[Chris Lord [:cwiiis]]

I'm not sure what we want to do with these patches just yet, so I'm leaving off flags - we can discuss it in today's gfx daily meeting.

Comment 5

[Chris Lord [:cwiiis]]

Do you think you could cook up a patch to add a switch in UI to disable checkerboarding? (i.e. setting pref layers.allow-checkerboarding to false).

Comment 6

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

Comment on attachment 8370045 [details] [diff] [review]
Add a pref that can disable checkerboarding

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

I think you could just put the pref in AsyncPanZoomController.cpp and call it apz.allow-checkerboarding. It seems fairly contained; no need to expose it to the rest of the gfx code if it's not going to be needed there.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1525,5 @@
> +  if (!gfxPlatform::GetPrefLayersAllowCheckerboarding() &&
> +      !mLastContentPaintMetrics.mDisplayPort.IsEmpty()) {
> +    CSSPoint maxScrollOffset = lastPaintScrollOffset +
> +      CSSPoint(mLastContentPaintMetrics.mDisplayPort.XMost() - mLastContentPaintMetrics.mViewport.width,
> +               mLastContentPaintMetrics.mDisplayPort.YMost() - mLastContentPaintMetrics.mViewport.height);

Instead of mViewport I think you want to be using CalculateCompositedRectInCSSPixels(). mViewport is supposed to be the CSS viewport which is irrelevant here.

Comment 7

[Chris Lord [:cwiiis]]

Note, this will not interact correctly with progressive rendering - you may still see checkerboarding with progressive rendering and tiles enabled (though if the coherency checks are working (they likely aren't), it's quite unlikely).

Comment 8

[Chris Lord [:cwiiis]]

Attachment: Add a pref that can disable checkerboarding

Made kats' suggested changes - pref is now apz.allow-checkerboarding.

Comment 9

[Chris Lord [:cwiiis]]

Attachment: Disallow checkerboarding on b2g

Comment 10

[Chris Lord [:cwiiis]]

Attachment: Disallow checkerboarding on Metro

Comment 11

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

Comment on attachment 8370059 [details] [diff] [review]
Add a pref that can disable checkerboarding

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

This looks ok to me, but I'd like botond to also check that I didn't miss anything, since GetCurrentAsyncTransform is used as part of our input transformation chain as well. I think this change is fine as it will affect both of those in the same way, and so input will still be untransformed properly.

Comment 12

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

Attachment: Expose developer setting (Gecko patch)

I can do the pref hookup

Comment 13

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

Attachment: Expose developer setting (Gaia patch)

Comment 14

[Chris Lord [:cwiiis]]

Comment on attachment 8370093 [details] [diff] [review]
Expose developer setting (Gecko patch)

So I don't think this pref alone is going to compare favourably with a better set of displayport settings. Can we have a separate option to turn it on/off in addition to the displayport tweaks?

Comment 15

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

Rather than introduce a combinatorial explosion of options for people to test, I would rather find a set of settings that you think works well with this pref, and set those as well in settings.js. Feel free to update that patch directly or let me know which settings you think would be good.

Comment 16

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

Are you sure we're not introducing too much bias by combining different settings ourselves?  I would actually prefer the combinatorial explosion in this case.

Comment 17

[Botond Ballo [:botond]]

Comment on attachment 8370059 [details] [diff] [review]
Add a pref that can disable checkerboarding

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

r=me with the min <= max check added.

Note that, as far as I can tell, this patch does not disallow checkerboarding when zooming out, so characterizing it as such and calling the pref "apz.allow-checkerboarding" might be a bit inaccurate. Don't want to hold it up because of this though.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1533,5 @@
> +    CSSRect compositedRect = mLastContentPaintMetrics.CalculateCompositedRectInCssPixels();
> +    CSSPoint maxScrollOffset = lastPaintScrollOffset +
> +      CSSPoint(mLastContentPaintMetrics.mDisplayPort.XMost() - compositedRect.width,
> +               mLastContentPaintMetrics.mDisplayPort.YMost() - compositedRect.height);
> +    CSSPoint minScrollOffset = lastPaintScrollOffset + mLastContentPaintMetrics.mDisplayPort.TopLeft();

We should check that minScrollOffset <= maxScrollOffset. I think this can be false if we are sufficiently zoomed out that the last painted display port only covers part of the current composition bounds. In such a case, we have to checkerboard unless we also clamp the zoom here.

@@ +1539,5 @@
> +    if (currentScrollOffset.x < minScrollOffset.x) {
> +      currentScrollOffset.x = minScrollOffset.x;
> +    } else if (currentScrollOffset.x > maxScrollOffset.x) {
> +      currentScrollOffset.x = maxScrollOffset.x;
> +    }

This can be written:

currentScrollOffset.x = clamped(currentScrollOffset.x, minScrollOffset.x, maxScrollOffset.x);

(up to you if you like this better).

Comment 18

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #17)
> r=me with the min <= max check added.

Er, f+ rather. You know what I mean :)

Comment 19

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

(In reply to Milan Sreckovic [:milan] from comment #16)
> Are you sure we're not introducing too much bias by combining different
> settings ourselves?  I would actually prefer the combinatorial explosion in
> this case.

To some extent we are introducing bias, yes. However I think it's unlikely that QA will be able to test the entire range of APZ use cases for each of the combinations of settings - there's just too many possibilities. It makes sense for us to remove those settings which we know will fall down in some cases. For example, reducing the displayport width to 1.0 will work fine in gaia apps but will cause horizontal checkerboarding in the browser because they can scroll horizontally whereas gaia apps cannot. So it doesn't make sense to offer that option as it will clearly regress some cases.

That being said, yes, we can definitely add more options to the list - I would be ok with just calling them "Heuristic 1", "Heuristic 2" and so on and add lots of different combinations. The ones I started off with I thought were sufficiently different from each other to provide a good range of options, and then if some emerged as better overall or in particular scenarios we could further refine or hybridize them.

/me wants to write an evolutionary algorithm that finds the best one...

Comment 20

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8370061 [details] [diff] [review]
Disallow checkerboarding on Metro

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

Seems to work fine on Metro, and lack of black flashes is nice.  I only have fairly high-end Win8 hardware here though, so checkerboarding is already quite rare.  Should probably test on low-end hardware and/or slower-painting pages.

Comment 21

[Chris Lord [:cwiiis]]

"Add a pref that can disable checkerboarding" pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/1b0e02d99ab1

Comment 22

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

PR for the gaia patch:

https://github.com/mozilla-b2g/gaia/pull/15989

Comment 23

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

Comment on attachment 8370093 [details] [diff] [review]
Expose developer setting (Gecko patch)

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8d39ea09148

Comment 24

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://github.com/mozilla-b2g/gaia/commit/3711792ac529fd8ca0b065fa303fe03e64e065e1

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1b0e02d99ab1

Comment 26

[Benoit Girard (:BenWa)]

I did some profiling with this:
http://people.mozilla.org/~bgirard/cleopatra/#report=275f2b22aa8f38d13890ed0be9a3afe88e5dd501

This patch doesn't stop us from compositing frames so we could actually save a ton of memory bandwidth by fixing that speeding up the main thread painting.

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a8d39ea09148

Comment 28

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

We want this stuff uplifted to 1.3 to try and mitigate bug 942750

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/8b23dde77abe
v1.3: 51e166e07e582027c2d57498232a5476918e3b41

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/e2efd22ca22d