Bug 103366 – [TexMap] Perform the layer-tree traversal in GraphicsLayerTextureMapper.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Dongseong Hwang]

Move layer-tree traverse from TextureMapperLaye to GraphicsLayerTextureMapper.

It is because of two reasons.
1. It is a part of implementing actor model. After implementing actor model,
each GraphicsLayerTextureMapper needs to send a layer state message to
TextureMapperLayer. It means the outside instance of TextureMapperLayer should
perform recursive traverse.
2. LayerTreeRenderer needs to syncronize a layer state of TextureMapperLayer when
receiving the SetCompositingLayerState message. See Bug 103171.

[Dongseong Hwang]

Created attachment 176176 [details]
Patch

[Dongseong Hwang]

Now GraphicsLayerTextureMapper::flushCompositingStateForThisLayerOnly() synchronize a layer state and animation on this layer only.

Currently, GraphicsLayerTextureMapper::flushCompositingStateForThisLayerOnly() performs synchronizing the layer state of the given layer and the animations of children as well as the given layer. However, there is no requirement that flushCompositingStateForThisLayerOnly() synchronizes all animations in layer tree.

All clients that call flushCompositingStateForThisLayerOnly() uses GraphicsLayer in the same way.
rootLayer->flushCompositingStateForThisLayerOnly()
nonCompositingLayer->flushCompositingStateForThisLayerOnly()
frameView->flushCompositingStateIncludingSubframes()

It means that flushCompositingStateForThisLayerOnly() needs to synchronize the animation of only the given layer.
Currently, we traverse 3 times to sync animation redundantly.

[Dongseong Hwang]

Created attachment 176200 [details]
Patch

[Dongseong Hwang]

Comment on attachment 176200 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176200&action=review

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:434
> +        return;

Now we can call GraphicsLayerTextureMapper::flushCompositingState() before setting TextureMapper to the root TextureMapperLayer.
GraphicsLayerTextureMapper::flushCompositingState() can sync layer states except for completing prepareBackingStore().
prepareBackingStore() can not be perform without TextureMapper, but it is safe because we keep m_needsDisplay and united m_needsDisplayRect until prepareBackingStore() completes after setting TextureMapper to the root layer.

[Dongseong Hwang]

Created attachment 178471 [details]
Patch

[Noam Rosenthal]

Comment on attachment 178471 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178471&action=review

You're also removing the SyncOptions thingy... it's ok but you need to specific in the changelog why/how this is ok.

> Source/WebCore/ChangeLog:11
> +        urrently, CoordinatedGraphicsLayer performs layer tree traversal by
> +        itself, but GraphicsLayerTextureMapper delegates the tree traversal to
> +        TextureMapperLayer. This patch decides the tree traversal is the role of
> +        GraphicsLayer.

Perform the layer-tree traversal in GraphicsLayerTextureMapper.

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:420
> +        toGraphicsLayerTextureMapper(maskLayer())->flushCompositingState(rect);

you don't need toGraphicsLayerTextureMapper

[Dongseong Hwang]

Created attachment 178479 [details]
Patch

[Dongseong Hwang]

(In reply to comment #6)
> (From update of attachment 178471 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178471&action=review

Thank you for review!

> You're also removing the SyncOptions thingy... it's ok but you need to specific in the changelog why/how this is ok.

Oops, I missed.

> > Source/WebCore/ChangeLog:11
> > +        urrently, CoordinatedGraphicsLayer performs layer tree traversal by
> > +        itself, but GraphicsLayerTextureMapper delegates the tree traversal to
> > +        TextureMapperLayer. This patch decides the tree traversal is the role of
> > +        GraphicsLayer.
> 
> Perform the layer-tree traversal in GraphicsLayerTextureMapper.

I rephrased based on your help :)

> 
> > Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:420
> > +        toGraphicsLayerTextureMapper(maskLayer())->flushCompositingState(rect);
> 
> you don't need toGraphicsLayerTextureMapper

Done.

[Dongseong Hwang]

mrobinson, noamr and I talked about GTK+ AC regresstion in IRC.

Bug 103046 made the problem, and this bug fixes the problem.

I did not understand how Bug 103046 can cause the problem yet.

[Dongseong Hwang]

(In reply to comment #9)
> mrobinson, noamr and I talked about GTK+ AC regresstion in IRC.
> 
> Bug 103046 made the problem, and this bug fixes the problem.
> 
> I did not understand how Bug 103046 can cause the problem yet.

I filed Bug 104640 to fix the bug.

[Dongseong Hwang]

Created attachment 178739 [details]
Patch

[Dongseong Hwang]

(In reply to comment #11)
> Created an attachment (id=178739) [details]
> Patch

Fix Bug 104640 the first and refactor this bug the second.

[Noam Rosenthal]

Comment on attachment 178739 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178739&action=review

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:367
> +    flushCompositingStateSelf(graphicsLayer);
> +    syncAnimations();

IS this needed? can't we have one function, that calls syncAnimations in the end?

[Dongseong Hwang]

Created attachment 178934 [details]
Patch

[Dongseong Hwang]

(In reply to comment #13)
> (From update of attachment 178739 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178739&action=review
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:367
> > +    flushCompositingStateSelf(graphicsLayer);
> > +    syncAnimations();
> 
> IS this needed? can't we have one function, that calls syncAnimations in the end?

Good suggestion. I changed :)

[Noam Rosenthal]

Comment on attachment 178934 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178934&action=review

I'm OK with this. I'd rather someone else cq+ if things are stable enough for this.

> Source/WebCore/ChangeLog:19
> +        In addition, this patch removes SyncOptions enum. Currently, when using
> +        ComputationsOnly, the layer-tree traversal synchronizes only animations
> +        without layer states. However, there is no client that calls
> +        flushCompositingState() for the purpose. Even we already have the API
> +        for the purpose: applyAnimationsRecursively(). So from now on, we have
> +        only flushCompositingStateForThisLayerOnly() API in TextureMapperLayer
> +        for brevity.

Just say:
Also removed the SyncOptions enum, which is redundant since no client calls it with ComputationsOnly.

[Dongseong Hwang]

Created attachment 179367 [details]
Patch

[Dongseong Hwang]

(In reply to comment #16)
> (From update of attachment 178934 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178934&action=review
> 
> I'm OK with this. I'd rather someone else cq+ if things are stable enough for this.

I think it is stable. Now a day, I tested much :)

> Just say:
> Also removed the SyncOptions enum, which is redundant since no client calls it with ComputationsOnly.

Thank you for rephrasing!

[WebKit Review Bot]

Comment on attachment 179367 [details]
Patch

Clearing flags on attachment: 179367

Committed r137867: <http://trac.webkit.org/changeset/137867>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.