1247452 - Use GetEffectiveVisibleRegion instead of GetVisibleRegion consistently in LayerTreeInvalidation

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Botond Ballo [:botond]]

In bug 1161978, Sotaro fixed a bug where LayerTreeInvalidation was calling GetVisibleRegion() instead of GetEffectiveVisibleRegion(), which gave wrong results on the compositor side (where the visible region can have changed by async transformations, and GetEffectiveVisibleRegion() accounts for these changes by GetVisibleRegion() does not).

I think that, for the same reason, all calls to GetVisibleRegion() in LayerTreeInvalidation.cpp should be calls to GetEffectiveVisibleRegion() instead.

Sotaro, please correct me if I'm mistaken.

Comment 1

[Botond Ballo [:botond]]

(Note that GetEffectiveVisibleRegion() will be renamed to GetLocalVisibleRegion() in bug 1247445.)

Comment 2

[Botond Ballo [:botond]]

A survey of callers of GetVisibleRegion() revealed a few other call sites in the compositor that should probably be using GetEffectiveVisibleRegion() as well. My patch updates all of these.

Comment 3

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1247452 - Use the effective visible region rather than the visible region where appropriate in the compositor. r=mattwoodrow

Review commit: https://reviewboard.mozilla.org/r/34839/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34839/

Comment 4

[Botond Ballo [:botond]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf3c5da9ed46

Comment 5

[Botond Ballo [:botond]]

layout/reftests/async-scrolling/checkerboard-3.html is failing. I'm investigating.

Comment 6

[Botond Ballo [:botond]]

Comment on attachment 8719005 [details]
MozReview Request: Bug 1247452 - Use the effective visible region rather than the visible region where appropriate in the compositor. r=mattwoodrow

Dropping review flag until I figure out what's going on with the test.

Comment 7

[Botond Ballo [:botond]]

The short answer is: this patch makes compositor-side culling more effective - so effective, in fact, that we're breaking a scenario related to checkerboard background color drawing where we were relying on it *not* happening. Basically, more aggressive culling exposed a bug (much as in bug 1237827).

Comment 8

[Botond Ballo [:botond]]

Specifically, the change to CalculateScissorRect() is causing the checkerboarding layer's clip rect, as calculated in ContainerPrepare(), to become empty. This leads to culling the layer, but even if we avoid that (by adding a guard the way we did to the "empty visible region" culling path in bug 1013385), an empty clip rect will lead to no checkerboard color being drawn (because the checkerboard color is restricted to the clip rect).

Comment 9

[Botond Ballo [:botond]]

Comment on attachment 8719005 [details]
MozReview Request: Bug 1247452 - Use the effective visible region rather than the visible region where appropriate in the compositor. r=mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34839/diff/1-2/

Comment 10

[Botond Ballo [:botond]]

The updated patch omits the change to CalculateScissorRect(), since it interferes with the desired checkerboard background drawing behaviour.

Comment 11

[Botond Ballo [:botond]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=408980cfa2d1

Comment 12

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8719005 [details]
MozReview Request: Bug 1247452 - Use the effective visible region rather than the visible region where appropriate in the compositor. r=mattwoodrow

https://reviewboard.mozilla.org/r/34839/#review31493

Probably worth adding a comment to the scissor rect code about why we need to use client side visible region instead of the effective/shadow one.

Comment 13

[Botond Ballo [:botond]]

Rebased across bug 1247445, added comment as requested, and landed.

Comment 14

[Pulsebot]

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

Comment 15

[Carsten Book [:Tomcat]]

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