Issue 2765053002: Avoid exposing cc::Layer tree to CompositorProxy

Issue 2765053002: Avoid exposing cc::Layer tree to CompositorProxy (Closed)

Created:
3 years, 9 months ago by smcgruer

Modified:
3 years, 7 months ago

Reviewers:
flackr

CC:
chromium-reviews, krit, dshwang, drott+blinkwatch_chromium.org, blink-reviews-platform-graphics_chromium.org, sof, eae+blinkwatch, jbroman, fmalita+watch_chromium.org, blink-reviews-dom_chromium.org, dglazkov+blink, Rik, Stephen Chennney, Justin Novosad, pdr+graphicswatchlist_chromium.org, blink-reviews, kinuko+watch, ajuma+watch_chromium.org, danakj+watch_chromium.org, rwlbuis

Target Ref:
refs/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Avoid exposing cc::Layer tree to CompositorProxy This is the first step in splitting out the AnimationWorklet/CompositorWorker code to run in a separate thread. Instead of exposing LayerImpl objects to the CompositorProxy, create a snapshot of the necessary layers and their mutable properties and pass that down. After mutation, the output set of mutations is used to update the layer tree. There is a small behavioral change here. After this patch, when a single element is proxied multiple times each proxy will only see the initial input value from the layer tree and not any modifications made to it by other proxies. Competing writes by multiple proxies are currently handled in an ad-hoc last write wins approach. BUG=694532 patch from issue 2756703002 at patchset 60001 (http://crrev.com/2756703002#ps60001)

Patch Set 1 #

Patch Set 2 : Minor changes #

Total comments: 15

Patch Set 3 : Address reviewer comments #

Patch Set 4 : Fix usage of element id #

Patch Set 5 : Rebase onto blink reformat #

Total comments: 7

Created: 3 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+435 lines, -189 lines)			Patch
M	third_party/WebKit/Source/core/dom/CompositorProxy.h	View	1 2 3 4	2 chunks	+2 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/dom/CompositorProxy.cpp	View	1 2 3 4	2 chunks	+7 lines, -1 line	3 comments	Download
M	third_party/WebKit/Source/platform/graphics/CompositorMutableProperties.h	View	1 2 3 4	1 chunk	+19 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/platform/graphics/CompositorMutableState.h	View	1 2 3 4	2 chunks	+3 lines, -9 lines	0 comments	Download
M	third_party/WebKit/Source/platform/graphics/CompositorMutableState.cpp	View	1 2 3 4	1 chunk	+13 lines, -37 lines	0 comments	Download
M	third_party/WebKit/Source/platform/graphics/CompositorMutableStateProvider.h	View	1 2 3 4	2 chunks	+6 lines, -9 lines	0 comments	Download
M	third_party/WebKit/Source/platform/graphics/CompositorMutableStateProvider.cpp	View	1 2 3 4	2 chunks	+10 lines, -15 lines	4 comments	Download
M	third_party/WebKit/Source/platform/graphics/CompositorMutableStateTest.cpp	View	1 2 3 4	2 chunks	+34 lines, -110 lines	0 comments	Download
M	third_party/WebKit/Source/platform/graphics/CompositorMutation.h	View	1 2 3 4	1 chunk	+2 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/platform/graphics/CompositorMutatorClient.h	View	1 2 3 4	3 chunks	+21 lines, -1 line	0 comments	Download
M	third_party/WebKit/Source/platform/graphics/CompositorMutatorClient.cpp	View	1 2 3 4	3 chunks	+165 lines, -1 line	0 comments	Download
M	third_party/WebKit/Source/platform/graphics/CompositorMutatorClientTest.cpp	View	1 2 3 4	3 chunks	+133 lines, -5 lines	0 comments	Download
M	third_party/WebKit/Source/web/CompositorMutatorImpl.h	View	1 2 3 4	2 chunks	+4 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/web/CompositorMutatorImpl.cpp	View	1 2 3 4	1 chunk	+15 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/web/CompositorWorkerProxyClientImpl.cpp	View	1 2 3 4	1 chunk	+1 line, -1 line	0 comments	Download

Messages

Total messages: 16 (6 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

flackr

https://codereview.chromium.org/2765053002/diff/20001/third_party/WebKit/Source/core/dom/CompositorProxy.h File third_party/WebKit/Source/core/dom/CompositorProxy.h (right): https://codereview.chromium.org/2765053002/diff/20001/third_party/WebKit/Source/core/dom/CompositorProxy.h#newcode76 third_party/WebKit/Source/core/dom/CompositorProxy.h:76: const uint64_t m_proxyId = 0; Do we need proxyId? ...

3 years, 8 months ago (2017-04-03 17:09:19 UTC) #7

smcgruer

3 years, 8 months ago (2017-04-03 17:57:31 UTC) #8

smcgruer

https://codereview.chromium.org/2765053002/diff/20001/third_party/WebKit/Source/platform/graphics/CompositorMutableProperties.h File third_party/WebKit/Source/platform/graphics/CompositorMutableProperties.h (right): https://codereview.chromium.org/2765053002/diff/20001/third_party/WebKit/Source/platform/graphics/CompositorMutableProperties.h#newcode16 third_party/WebKit/Source/platform/graphics/CompositorMutableProperties.h:16: class CompositorMutableProperties { On 2017/04/03 17:09:19, flackr wrote: > ...

3 years, 8 months ago (2017-04-03 19:37:11 UTC) #9

https://codereview.chromium.org/2765053002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/CompositorMutableProperties.h
(right):

https://codereview.chromium.org/2765053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CompositorMutableProperties.h:16:
class CompositorMutableProperties {
On 2017/04/03 17:09:19, flackr wrote:
> Can you add a comment what this is used for?

Done, although I don't love my own explanation...

https://codereview.chromium.org/2765053002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/CompositorMutatorClient.cpp
(right):

https://codereview.chromium.org/2765053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CompositorMutatorClient.cpp:26:
cc::LayerImpl* layer = treeImpl->LayerByElementId(
On 2017/04/03 17:09:19, flackr wrote:
> Since (I'm assuming) the LayerByElementId requires a lookup whereas isMutated
> calls are just checking a bit, we should only do the lookup if one of the
> relevant properties has changed (you'll probably need to expose the bitmask to
> avoid multiple lookups).

Done, but it's real ugly now that we're using bitmasks checks rather than calls
into CompositorMutation :(

https://codereview.chromium.org/2765053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CompositorMutatorClient.cpp:99:
m_mutations = WTF::wrapUnique(new CompositorMutations);
On 2017/04/03 17:09:19, flackr wrote:
> With this patch we could limit this workaround to exactly where the problem
> occurs, i.e. immediately after calling OnScrollOffsetAnimated, and ensure that
> it continues to behave as expected (i.e. DCHECK that m_mutations is always
> released).

Done

https://codereview.chromium.org/2765053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CompositorMutatorClient.cpp:162:
createCompositorElementId(elementId, CompositorSubElementId::Primary));
On 2017/04/03 17:09:19, flackr wrote:
> Same comment as above.

Done, though it's a bit ugly to keep the if (!layer && !scrollLayer) behavior
below.

https://codereview.chromium.org/2765053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CompositorMutatorClient.cpp:168:
(*inputMap)[proxyId].elementId = elementId;
On 2017/04/03 17:09:19, flackr wrote:
> I'm not sure if the repeated calls to (*inputMap)[proxyId] will optimize
> correctly. I think you should save and reuse a reference instead.

Done, I think.

https://codereview.chromium.org/2765053002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/CompositorMutatorClient.h
(right):

https://codereview.chromium.org/2765053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CompositorMutatorClient.h:52:
std::map<uint64_t, std::pair<uint64_t, uint32_t>> m_inputProperties;
On 2017/04/03 17:09:19, flackr wrote:
> Comment what the map is from / to.

Done.

flackr

3 years, 8 months ago (2017-04-11 17:51:36 UTC) #10

https://codereview.chromium.org/2765053002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/CompositorProxy.h (right):

https://codereview.chromium.org/2765053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/CompositorProxy.h:76: const uint64_t
m_proxyId = 0;
On 2017/04/03 17:57:31, smcgruer wrote:
> On 2017/04/03 17:09:19, flackr wrote:
> > Do we need proxyId? Can we not key off of elementId?
> 
> Is there not a N:1 relationship between CompositorProxy and elementId?  As an
> element can be involved in multiple CompositorWorker animations, creating a
> CompositorProxy for it in each animation?

At least there is in the current implementation where you can create many
CompositorProxy objects for a single element. Acknowledged.

https://codereview.chromium.org/2765053002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/CompositorProxy.cpp (right):

https://codereview.chromium.org/2765053002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/CompositorProxy.cpp:154: if (IsMainThread())
{
This check implies that this constructor can be called on multiple threads, but
if it can than the proxy id increment will be racy. Also, is there not an
intention to match the proxy id selected by the main thread if we create a proxy
on the compositor thread?

https://codereview.chromium.org/2765053002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/CompositorMutableStateProvider.cpp
(right):

https://codereview.chromium.org/2765053002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CompositorMutableStateProvider.cpp:23:
CompositorMutableStateProvider::GetMutableStateFor(uint64_t proxy_id) {
I'm not sure I understand why the change to getting mutable state for a certain
proxy id rather than the element. Ultimately we just need to copy the current
values for the element which could be shared between all of the proxies
referencing it, couldn't it?

smcgruer

https://codereview.chromium.org/2765053002/diff/80001/third_party/WebKit/Source/core/dom/CompositorProxy.cpp File third_party/WebKit/Source/core/dom/CompositorProxy.cpp (right): https://codereview.chromium.org/2765053002/diff/80001/third_party/WebKit/Source/core/dom/CompositorProxy.cpp#newcode154 third_party/WebKit/Source/core/dom/CompositorProxy.cpp:154: if (IsMainThread()) { On 2017/04/11 17:51:36, flackr wrote: > ...

3 years, 8 months ago (2017-04-19 15:38:39 UTC) #11

https://codereview.chromium.org/2765053002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/CompositorProxy.cpp (right):

https://codereview.chromium.org/2765053002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/CompositorProxy.cpp:154: if (IsMainThread())
{
On 2017/04/11 17:51:36, flackr wrote:
> This check implies that this constructor can be called on multiple threads,
but
> if it can than the proxy id increment will be racy. Also, is there not an
> intention to match the proxy id selected by the main thread if we create a
proxy
> on the compositor thread?

Hrm, you are correct. I think the correct thing to do is to generate the proxy
ID on the main thread's Create() method, require it as a parameter to the
compositor side Create() method, and pass it over the wire during serialization
- does that sgty?

https://codereview.chromium.org/2765053002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/CompositorMutableStateProvider.cpp
(right):

https://codereview.chromium.org/2765053002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CompositorMutableStateProvider.cpp:23:
CompositorMutableStateProvider::GetMutableStateFor(uint64_t proxy_id) {
On 2017/04/11 17:51:36, flackr wrote:
> I'm not sure I understand why the change to getting mutable state for a
certain
> proxy id rather than the element. Ultimately we just need to copy the current
> values for the element which could be shared between all of the proxies
> referencing it, couldn't it?

I might not be following you correctly, but the CL is designed so that each
animator retains read/write access to the mutatable properties, whilst not
affecting animators. I.e. if A1 and A2 both proxy element P via proxies P1 and
P2, then you get the following behavior:

A1: read P1.opacity /* returns current value, say 0.5 */
A1: write 1.0 to P1.opacity
A1: read P1.opacity /* returns 1.0 */
A2: read P2.opacity /* returns 0.5 */

==> After all animators run, mutations[P].opacity is 1.0

To provide this behavior we have CompositorMutableProperties which is per-proxy,
and CompositorMutation which is per-element.

flackr

3 years, 8 months ago (2017-04-20 06:54:14 UTC) #12

https://codereview.chromium.org/2765053002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/CompositorProxy.cpp (right):

https://codereview.chromium.org/2765053002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/CompositorProxy.cpp:154: if (IsMainThread())
{
On 2017/04/19 15:38:38, smcgruer wrote:
> On 2017/04/11 17:51:36, flackr wrote:
> > This check implies that this constructor can be called on multiple threads,
> but
> > if it can than the proxy id increment will be racy. Also, is there not an
> > intention to match the proxy id selected by the main thread if we create a
> proxy
> > on the compositor thread?
> 
> Hrm, you are correct. I think the correct thing to do is to generate the proxy
> ID on the main thread's Create() method, require it as a parameter to the
> compositor side Create() method, and pass it over the wire during
serialization
> - does that sgty?

Yes, this SGTM

https://codereview.chromium.org/2765053002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/CompositorMutableStateProvider.cpp
(right):

https://codereview.chromium.org/2765053002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CompositorMutableStateProvider.cpp:23:
CompositorMutableStateProvider::GetMutableStateFor(uint64_t proxy_id) {
On 2017/04/19 15:38:38, smcgruer wrote:
> On 2017/04/11 17:51:36, flackr wrote:
> > I'm not sure I understand why the change to getting mutable state for a
> certain
> > proxy id rather than the element. Ultimately we just need to copy the
current
> > values for the element which could be shared between all of the proxies
> > referencing it, couldn't it?
> 
> I might not be following you correctly, but the CL is designed so that each
> animator retains read/write access to the mutatable properties, whilst not
> affecting animators. I.e. if A1 and A2 both proxy element P via proxies P1 and
> P2, then you get the following behavior:
> 
> A1: read P1.opacity /* returns current value, say 0.5 */
> A1: write 1.0 to P1.opacity
> A1: read P1.opacity /* returns 1.0 */
> A2: read P2.opacity /* returns 0.5 */
> 
> ==> After all animators run, mutations[P].opacity is 1.0
> 
> 
> To provide this behavior we have CompositorMutableProperties which is
per-proxy,
> and CompositorMutation which is per-element.

Ah I see now, can we intead store the "current" values on the CompositorProxy
objects, copying the values into the CompositorProxy object at the same time as
we give them a CompositorMutableState? I feel like this would be less complex.

smcgruer

https://codereview.chromium.org/2765053002/diff/80001/third_party/WebKit/Source/platform/graphics/CompositorMutableStateProvider.cpp File third_party/WebKit/Source/platform/graphics/CompositorMutableStateProvider.cpp (right): https://codereview.chromium.org/2765053002/diff/80001/third_party/WebKit/Source/platform/graphics/CompositorMutableStateProvider.cpp#newcode23 third_party/WebKit/Source/platform/graphics/CompositorMutableStateProvider.cpp:23: CompositorMutableStateProvider::GetMutableStateFor(uint64_t proxy_id) { On 2017/04/20 06:54:14, flackr wrote: > ...

3 years, 8 months ago (2017-04-21 14:42:25 UTC) #13

flackr

On 2017/04/21 14:42:25, smcgruer wrote: > https://codereview.chromium.org/2765053002/diff/80001/third_party/WebKit/Source/platform/graphics/CompositorMutableStateProvider.cpp > File > third_party/WebKit/Source/platform/graphics/CompositorMutableStateProvider.cpp > (right): > > ...

3 years, 8 months ago (2017-04-26 15:50:10 UTC) #14

flackr

3 years, 8 months ago (2017-04-26 15:51:13 UTC) #15

On 2017/04/26 15:50:10, flackr wrote:
> On 2017/04/21 14:42:25, smcgruer wrote:
> >
>
https://codereview.chromium.org/2765053002/diff/80001/third_party/WebKit/Sour...
> > File
> >
third_party/WebKit/Source/platform/graphics/CompositorMutableStateProvider.cpp
> > (right):
> > 
> >
>
https://codereview.chromium.org/2765053002/diff/80001/third_party/WebKit/Sour...
> >
>
third_party/WebKit/Source/platform/graphics/CompositorMutableStateProvider.cpp:23:
> > CompositorMutableStateProvider::GetMutableStateFor(uint64_t proxy_id) {
> > On 2017/04/20 06:54:14, flackr wrote:
> > > Ah I see now, can we intead store the "current" values on the
> CompositorProxy
> > > objects, copying the values into the CompositorProxy object at the same
time
> > as
> > > we give them a CompositorMutableState? I feel like this would be less
> complex.
> > 
> > I'm looking at this, but I don't really see how it simplifies things? We
have
> to
> > store per-proxy state all the way back to CompositorMutatorClient because
each
> > proxy can specify different mutable properties for the element, and we use
> that
> > list of mutable properties to decide what to snapshot in
> > CompositorMutatorClient::SnapshotLayerTree.
> > 
> > We could remove the concept of proxy id, but we would then have to snapshot
> > everything in CompositorMutatorClient::SnapshotLayerTree because there would
> be
> > no way to tell which properties are allowed for an element. That said each
> > individual CompositorProxy already filters for mutable properties in it's
> > getters/setters, so perhaps this is fine?
> 
> I think we resolve this by collecting the union of all proxied properties (in
a
> proxied property bitmask) so we know which properties we need to snapshot to
> read everything that all CompositorProxies may read.

See
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Eleme...
where we increment the counts of proxied properties for each element.

smcgruer

3 years, 7 months ago (2017-05-19 13:22:57 UTC) #16

Description was changed from

==========
Avoid exposing cc::Layer tree to CompositorProxy

This is the first step in splitting out the AnimationWorklet/CompositorWorker
code to run in a separate thread. Instead of exposing LayerImpl objects to the
CompositorProxy, create a snapshot of the necessary layers and their mutable
properties and pass that down. After mutation, the output set of mutations is
used to update the layer tree.

There is a small behavioral change here. After this patch, when a single
element is proxied multiple times each proxy will only see the initial input
value from the layer tree and not any modifications made to it by other
proxies. Competing writes by multiple proxies are currently handled in an
ad-hoc last write wins approach.

BUG=694532

patch from issue 2756703002 at patchset 60001
(http://crrev.com/2756703002#ps60001)
==========

to

==========
Avoid exposing cc::Layer tree to CompositorProxy

This is the first step in splitting out the AnimationWorklet/CompositorWorker
code to run in a separate thread. Instead of exposing LayerImpl objects to the
CompositorProxy, create a snapshot of the necessary layers and their mutable
properties and pass that down. After mutation, the output set of mutations is
used to update the layer tree.

There is a small behavioral change here. After this patch, when a single
element is proxied multiple times each proxy will only see the initial input
value from the layer tree and not any modifications made to it by other
proxies. Competing writes by multiple proxies are currently handled in an
ad-hoc last write wins approach.

BUG=694532

patch from issue 2756703002 at patchset 60001
(http://crrev.com/2756703002#ps60001)
==========

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages