Issue 512293002: Fix paint invalidation when composited layers change from paints-into-ancestor to self-painting.

Issue 512293002: Fix paint invalidation when composited layers change from paints-into-ancestor to self-painting. (Closed)

Created:
6 years, 3 months ago by chrishtr

Modified:
6 years, 3 months ago

Reviewers:
Ian Vollick, pdr.

CC:
abarth-chromium, blink-reviews, blink-reviews-rendering, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, rune+blink, zoltan1

Base URL:
svn://svn.chromium.org/blink/trunk

Project:
blink

Visibility:
Public.

More Reviews

Description

Fix paint invalidation when composited layers change from paints-into-ancestor to self-painting. Previously it was the case that we would update the setting before issuing the paint invalidation. This doesn't work, because the paint invalidation code uses the current state to find the right invalidation backing, not the previous state. BUG=405250 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181114

Patch Set 1 #

Total comments: 4

Patch Set 2 : Added a test. #

Patch Set 3 : Added comment. #

Patch Set 4 : Added doctype. #

Patch Set 5 : Fix nits. #

Patch Set 6 : Fixed test. #

Patch Set 7 : Fix test. #

Created: 6 years, 3 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+137 lines, -18 lines)			Patch
M	LayoutTests/TestExpectations	View	1 2 3 4 5 6	1 chunk	+0 lines, -1 line	0 comments	Download
A	LayoutTests/compositing/change-from-paints-into-ancestor-to-paints-into-own-backing-for-ancestor-reason.html	View	1 2 3	1 chunk	+18 lines, -0 lines	0 comments	Download
A	LayoutTests/compositing/change-from-paints-into-ancestor-to-paints-into-own-backing-for-ancestor-reason-expected.txt	View	1	1 chunk	+54 lines, -0 lines	0 comments	Download
A	LayoutTests/compositing/change-from-paints-into-ancestor-to-paints-into-own-backing-for-intrinsic-reason.html	View	1 2 3 4 5	1 chunk	+31 lines, -0 lines	0 comments	Download
A +	LayoutTests/compositing/change-from-paints-into-ancestor-to-paints-into-own-backing-for-intrinsic-reason-expected.txt	View	1 2 3 4 5 6	1 chunk	+17 lines, -12 lines	0 comments	Download
M	LayoutTests/compositing/squashing/invalidate-when-leaving-squashed-layer-expected.txt	View		1 chunk	+3 lines, -0 lines	0 comments	Download
M	LayoutTests/compositing/squashing/squash-above-fixed-1-expected.txt	View		2 chunks	+2 lines, -0 lines	0 comments	Download
M	LayoutTests/fast/repaint/filter-invalidation-with-composited-container-change-expected.txt	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/core/rendering/compositing/CompositedLayerMapping.cpp	View	1	2 chunks	+11 lines, -5 lines	0 comments	Download

Messages

Total messages: 20 (0 generated)

Expand Messages | Collapse Messages

chrishtr

chrishtr@chromium.org changed reviewers: + pdr@chromium.org, vollick@chromium.org

6 years, 3 months ago (2014-08-28 17:11:32 UTC) #1

chrishtr

Working now on a way to write a test for the "ancestor reasons" case. Ian, ...

6 years, 3 months ago (2014-08-28 17:11:32 UTC) #2

pdr.

https://codereview.chromium.org/512293002/diff/1/LayoutTests/compositing/change-from-paints-into-ancestor-to-paints-into-own-backing-for-intrinsic-reason.html File LayoutTests/compositing/change-from-paints-into-ancestor-to-paints-into-own-backing-for-intrinsic-reason.html (right): https://codereview.chromium.org/512293002/diff/1/LayoutTests/compositing/change-from-paints-into-ancestor-to-paints-into-own-backing-for-intrinsic-reason.html#newcode1 LayoutTests/compositing/change-from-paints-into-ancestor-to-paints-into-own-backing-for-intrinsic-reason.html:1: <style> Nits: Add <!DOCTYPE html>, remove extra space between ...

6 years, 3 months ago (2014-08-28 20:21:14 UTC) #4

chrishtr

6 years, 3 months ago (2014-08-28 20:27:56 UTC) #5

https://codereview.chromium.org/512293002/diff/1/LayoutTests/compositing/chan...
File
LayoutTests/compositing/change-from-paints-into-ancestor-to-paints-into-own-backing-for-intrinsic-reason.html
(right):

https://codereview.chromium.org/512293002/diff/1/LayoutTests/compositing/chan...
LayoutTests/compositing/change-from-paints-into-ancestor-to-paints-into-own-backing-for-intrinsic-reason.html:1:
<style>
On 2014/08/28 20:21:13, pdr wrote:
> Nits: Add <!DOCTYPE html>, remove extra space between "transitioning" and "the
> container", change Close button to "Run Test" (or similar).

Done.

https://codereview.chromium.org/512293002/diff/1/Source/core/rendering/compos...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/512293002/diff/1/Source/core/rendering/compos...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:1893:
m_requiresOwnBackingStoreForAncestorReasons = !canPaintIntoAncestor;
On 2014/08/28 20:21:13, pdr wrote:
> This code is really only tracking changes to
> m_requiresOwnBackingStoreForAncestorReasons changes but it's deciding that
using
> paintsIntoCompositedAncestor() as a proxy. What about refactoring this to just
> track changes to m_requiresOwnBackingStoreForAncestorReasons directly:
> 
> bool newRequiresOwnBackingStoreForAncestorReasons = !compositingAncestorLayer
>     ||
>
!(compositingAncestorLayer->compositedLayerMapping()->mainGraphicsLayer()->drawsContent()
>         ||
>
compositingAncestorLayer->compositedLayerMapping()->paintsIntoCompositedAncestor());
> 
> bool backingStoreRequirementChanging =
> m_requiresOwnBackingStoreForAncestorReasons !=
> newRequiresOwnBackingStoreForAncestorReasons;
> if (backingStoreRequirementChanging)
>     compositor()->paintInvalidationOnCompositingChange(&m_owningLayer);
> 
> m_requiresOwnBackingStoreForAncestorReasons =
> newRequiresOwnBackingStoreForAncestorReasons;
> return backingStoreRequirementChanging;

That would invalidate more than necessary. Only if we transition between the
states where paintsIntoCompositedAncestor() returns a different value will any
compositing state change that requires invalidation. And that is more than just
m_requiresOwnBackingStoreForAncestorReasons changing, in the case when
m_requiresOwnBackingStoreForIntrinsicReasons was already true.

pdr.

On 2014/08/28 20:27:56, chrishtr wrote: > That would invalidate more than necessary. Only if we ...

6 years, 3 months ago (2014-08-28 20:47:14 UTC) #6