Closed Bug 1112332 Opened 5 years ago Closed 5 years ago

Disable all paint heuristics for layers not actively scrolled by APZ

Categories

(Core :: Graphics: Layers, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox35 --- wontfix
firefox36 --- wontfix
firefox37 --- fixed
b2g-v2.1 --- fixed
b2g-v2.1S --- fixed
b2g-v2.2 --- fixed

People

(Reporter: BenWa, Assigned: BenWa)

References

Details

Attachments

(3 files, 7 obsolete files)

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.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Attachment #8539452 - Flags: review?(bugmail.mozilla)
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.
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.
Attachment #8539452 - Attachment is obsolete: true
Attachment #8539452 - Flags: review?(bugmail.mozilla)
Attachment #8539457 - Flags: review?(bugmail.mozilla)
I normally do this before landing but here it is.
Attachment #8539457 - Attachment is obsolete: true
Attachment #8539457 - Flags: review?(bugmail.mozilla)
Attachment #8539460 - Flags: review?(bugmail.mozilla)
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.
Attachment #8539460 - Flags: review?(bugmail.mozilla) → feedback+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Blocks: 1113435
Attachment #8539460 - Attachment is obsolete: true
Attachment #8543023 - Flags: review?(bugmail.mozilla)
Blocks: 1114731
(In reply to Benoit Girard (:BenWa) from comment #7)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 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.
Attachment #8543023 - Flags: review?(bugmail.mozilla) → review+
Waiting on inbound to open.
Attachment #8543023 - Attachment is obsolete: true
Attachment #8543292 - Flags: review+
Depends on: 1117155
Duplicate of this bug: 1117155
Attached file R-6 failures
Blocks: 1109518
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)
Attachment #8543335 - Attachment is obsolete: true
Attachment #8546073 - Flags: review+
Attached patch patchSplinter Review
cleanup qfold
Attachment #8546073 - Attachment is obsolete: true
Attachment #8546087 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/9c5d2d742c54
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
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.
2.1 blocker is hitting this in bug 1119726.
Blocks: 1119726
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
Attachment #8552638 - Flags: review+
Attachment #8552638 - Flags: approval-mozilla-b2g34?
Comment on attachment 8552638 [details] [diff] [review]
rebased on b2g 2.1

Helps 1119726
Attachment #8552638 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
This has been tagged as the possible cause for regressing bug 1121871. I need to investigate this before we uplift this patch.
No longer blocks: 1113435
No longer blocks: 1106611
Blocks: 1134762
You need to log in before you can comment on or make changes to this bug.