Closed
Bug 1247452
Opened 8 years ago
Closed 8 years ago
Use GetEffectiveVisibleRegion instead of GetVisibleRegion consistently in LayerTreeInvalidation
Categories
(Core :: Graphics: Layers, defect)
Core
Graphics: Layers
Tracking
()
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: botond, Assigned: botond)
References
Details
(Whiteboard: [gfx-noted])
Attachments
(1 file)
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.
Assignee | ||
Comment 1•8 years ago
|
||
(Note that GetEffectiveVisibleRegion() will be renamed to GetLocalVisibleRegion() in bug 1247445.)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → botond
Assignee | ||
Comment 2•8 years ago
|
||
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.
Assignee | ||
Comment 3•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/34839/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/34839/
Attachment #8719005 -
Flags: review?(matt.woodrow)
Assignee | ||
Comment 4•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf3c5da9ed46
Assignee | ||
Comment 5•8 years ago
|
||
layout/reftests/async-scrolling/checkerboard-3.html is failing. I'm investigating.
Assignee | ||
Comment 6•8 years ago
|
||
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.
Attachment #8719005 -
Flags: review?(matt.woodrow)
Assignee | ||
Comment 7•8 years ago
|
||
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).
Assignee | ||
Comment 8•8 years ago
|
||
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).
Assignee | ||
Comment 9•8 years ago
|
||
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/
Attachment #8719005 -
Flags: review?(matt.woodrow)
Assignee | ||
Comment 10•8 years ago
|
||
The updated patch omits the change to CalculateScissorRect(), since it interferes with the desired checkerboard background drawing behaviour.
Assignee | ||
Comment 11•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=408980cfa2d1
Comment 12•8 years ago
|
||
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.
Attachment #8719005 -
Flags: review?(matt.woodrow) → review+
Assignee | ||
Comment 13•8 years ago
|
||
Rebased across bug 1247445, added comment as requested, and landed.
Comment 15•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/afeaae6740e5
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in
before you can comment on or make changes to this bug.
Description
•