1112332 - Disable all paint heuristics for layers not actively scrolled by APZ

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Benoit Girard (:BenWa)]

After a discussion with kats, cwiiis, jrmuizel and mstange I think everyone agree that don't want to use paint heuristics for layers that aren't being scrolled by APZ.

For example if the page is animating something it's unexpected to get incomplete or low resolution drawing. See bug 1106611 for instance.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 2

[Benoit Girard (:BenWa)]

Comment on attachment 8539452 [details] [diff] [review]
patch

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

::: gfx/layers/client/ClientTiledPaintedLayer.cpp
@@ +204,5 @@
> +
> +  FrameMetrics compositorMetrics;
> +  if (!compositor->LookupCompositorFrameMetrics(parentMetrics.GetScrollId(),
> +                                                compositorMetrics)) {
> +    return false;

ahh, these two return statements should be return true now that I think about it.

Comment 3

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

Can you add a commit message that describes what the patch is intending to accomplish? Also -U8 for more lines of context would be nice.

Comment 4

[Benoit Girard (:BenWa)]

Attachment: Disable all paint heuristics for layers not actively scrolled by APZ.

Comment 5

[Benoit Girard (:BenWa)]

Attachment: Disable all paint heuristics for layers not actively scrolled by APZ.

I normally do this before landing but here it is.

Comment 6

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

Comment on attachment 8539460 [details] [diff] [review]
Disable all paint heuristics for layers not actively scrolled by APZ.

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

A few things I'd like to see changed:
- avoid running the new code if any of the preexisting conditions is true. i.e. do an early return true if !multipleTransactionsNeeded || fixed || !isScrollable.
- the new code should be moved into a helper function with meaningful name
- the new code should only be run #if !defined(MOZ_WIDGET_ANDROID) || defined(MOZ_ANDROID_APZ) since by default this will always fail on fennec as it doesn't use APZ.

Comment 7

[Benoit Girard (:BenWa)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)
> - the new code should only be run #if !defined(MOZ_WIDGET_ANDROID) ||
> defined(MOZ_ANDROID_APZ) since by default this will always fail on fennec as
> it doesn't use APZ.

I don't like these defines because it's too easy to miss updating them if we start running this code on desktop.

Comment 8

[Benoit Girard (:BenWa)]

Attachment: Disable all paint heuristics for layers not actively scrolled by APZ.

Comment 9

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=0e7abe966620

Comment 10

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

(In reply to Benoit Girard (:BenWa) from comment #7)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)
> > - the new code should only be run #if !defined(MOZ_WIDGET_ANDROID) ||
> > defined(MOZ_ANDROID_APZ) since by default this will always fail on fennec as
> > it doesn't use APZ.
> 
> I don't like these defines because it's too easy to miss updating them if we
> start running this code on desktop.

There shouldn't be any need to update this for desktop. The #if condition only prevents this code from running on fennec when it's using the java PZC. All other cases including desktop will run this code.

Comment 11

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

Comment on attachment 8543023 [details] [diff] [review]
Disable all paint heuristics for layers not actively scrolled by APZ.

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

You need to move the semicolon outside the #if or it will fail to build on fennec. r+ with that.

Comment 12

[Benoit Girard (:BenWa)]

Attachment: Disable all paint heuristics for layers not actively scrolled by APZ. r=kats

Waiting on inbound to open.

Comment 13

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/827fa9b5f9b4

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for linux debug build failures:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8ab478e9ba9

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=5034349&repo=mozilla-inbound

Comment 15

[Benoit Girard (:BenWa)]

Attachment: Disable all paint heuristics for layers not actively scrolled by APZ. r=kats

https://hg.mozilla.org/integration/mozilla-inbound/rev/47683fccda29

Folded bug 1117155.

Comment 17

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b2acfbc039a0 for b2g reftest-6 failures, https://treeherder.mozilla.org/logviewer.html#?job_id=5039497&repo=mozilla-inbound

Comment 18

[Benoit Girard (:BenWa)]

Attachment: R-6 failures

Comment 19

[Benoit Girard (:BenWa)]

Attachment: Disable all paint heuristics for layers not actively scrolled by APZ. r=kats

I'm adjusting the fuzzy-ness for the test to match android.

Previously we had:
fuzzy-if(Android,11,40) fuzzy-if(B2G,11,35)
And now we will have:
fuzzy-if(Android,11,40) fuzzy-if(B2G,11,40)

Comment 20

[Benoit Girard (:BenWa)]

Attachment: patch

cleanup qfold

Comment 21

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c5d2d742c54

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9c5d2d742c54

Comment 23

[Jonathan Watt [:jwatt]]

Bug 1109315 comment 23 claims this fixed that bug. Not sure if the FxOS people will want to uplift this to FxOS 2.1, but just a heads up in case they decide that over there.

Comment 24

[Benoit Girard (:BenWa)]

2.1 blocker is hitting this in bug 1119726.

Comment 25

[Benoit Girard (:BenWa)]

Attachment: rebased on b2g 2.1

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Enabling low res painting
User impact if declined: Several small and temporary visual glitches during animations.
Testing completed: Has been on master for several months. Tested locally to confirm that the patch works and addresses the problem. Has minor test CI coverage.
Risk to taking this patch (and alternatives if risky): Medium but this has been mitigated by being on central for a while.
String or UUID changes made by this patch: none

Comment 26

[bhavana bajaj [:bajaj]]

Comment on attachment 8552638 [details] [diff] [review]
rebased on b2g 2.1

Helps 1119726

Comment 27

[Benoit Girard (:BenWa)]

This has been tagged as the possible cause for regressing bug 1121871. I need to investigate this before we uplift this patch.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/4d0243277717

Comment 29

[Vincent Liu[:vliu]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/4d0243277717