We must flush compositing states of layers by the tree order: root layer,
non-composited layer, page-overlay layer and WebCore's root layer. If flushing
by wrong order, child layer cannot know which layer is its parent. This patch
flushes layer states by the tree order.
In addiation, this patch removes updateViewport() in
LayerTreeCoordinatorProxy::setRootCompositingLayer() because
LayerTreeCoordinatorProxy::didRenderFrame() is always called after
setting the root layer.
Comment on attachment 177156[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=177156&action=review> Source/WebKit2/ChangeLog:16
> + In addition, this patch removes updateViewport() in
> + LayerTreeCoordinatorProxy::setRootCompositingLayer() because
> + LayerTreeCoordinatorProxy::didRenderFrame() is always called after
> + setting the root layer.
Different patch.
> Source/WebKit2/ChangeLog:20
> + In addition, this patch clarifies LayerTreeRenderer::setRootLayerID() can be
> + called only once in its lifecycle. LayerTreeRenderer, LayerTreeCoordinator and
> + LayerTreeCoordinatorProxy belong to WebPage, so those have the same lifecycle
Different patch.
(In reply to comment #2)
> (From update of attachment 177156[details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177156&action=review
>
> > Source/WebKit2/ChangeLog:16
> > + In addition, this patch removes updateViewport() in
> > + LayerTreeCoordinatorProxy::setRootCompositingLayer() because
> > + LayerTreeCoordinatorProxy::didRenderFrame() is always called after
> > + setting the root layer.
>
> Different patch.
>
> > Source/WebKit2/ChangeLog:20
> > + In addition, this patch clarifies LayerTreeRenderer::setRootLayerID() can be
> > + called only once in its lifecycle. LayerTreeRenderer, LayerTreeCoordinator and
> > + LayerTreeCoordinatorProxy belong to WebPage, so those have the same lifecycle
>
> Different patch.
Thanks for review! I'll separate!
(In reply to comment #5)
> (From update of attachment 177164[details])
> Is there any way to test this?
I made this patch not due to bug.
I made this patch because I just found this weird seq.
Moreover, LayerTreeRenderer::flushLayerChanges() calls m_rootLayer->flushCompositingState(FloatRect()). If a bug exists, it must be a timing bug.
Unfortunately, I don't have a good idea.
Comment on attachment 177164[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=177164&action=review> Source/WebKit2/ChangeLog:11
> + We must flush compositing states of layers by the tree order: root layer,
> + non-composited layer, page-overlay layer and WebCore's root layer. If flushing
> + by wrong order, child layer cannot know which layer is its parent. This patch
> + flushes layer states by the tree order.
But setting the parent/children has nothing to do with flushing... I still don't think there's a bug here.
(In reply to comment #7)
> (From update of attachment 177164[details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177164&action=review
>
> > Source/WebKit2/ChangeLog:11
> > + We must flush compositing states of layers by the tree order: root layer,
> > + non-composited layer, page-overlay layer and WebCore's root layer. If flushing
> > + by wrong order, child layer cannot know which layer is its parent. This patch
> > + flushes layer states by the tree order.
>
> But setting the parent/children has nothing to do with flushing... I still don't think there's a bug here.
In my understanding, setting children causes flushing. During flushing, we set children to each TextureMapperLayer.
So, if animation calls updateViewport() between flushing root layer and DidRenderFrame, flash can occur.
void LayerTreeCoordinator::syncLayerChildren(WebLayerID id, const Vector<WebLayerID>& children)
{
m_shouldSyncFrame = true;
m_webPage->send(Messages::LayerTreeCoordinatorProxy::SetCompositingLayerChildren(id, children));
}
void TextureMapperLayer::flushCompositingStateSelf(GraphicsLayerTextureMapper* graphicsLayer, TextureMapper*)
{
...
if (changeMask & ParentChange) {
TextureMapperLayer* newParent = toTextureMapperLayer(graphicsLayer->parent());
if (newParent != m_parent) {
// Remove layer from current from child list first.
if (m_parent) {
size_t index = m_parent->m_children.find(this);
m_parent->m_children.remove(index);
m_parent = 0;
}
// Set new layer parent and add layer to the parents child list.
if (newParent) {
m_parent = newParent;
m_parent->m_children.append(this);
}
}
}
if (changeMask & ChildrenChange) {
// Clear children parent pointer to avoid unsync and crash on layer delete.
for (size_t i = 0; i < m_children.size(); i++)
m_children[i]->m_parent = 0;
m_children.clear();
for (size_t i = 0; i < graphicsLayer->children().size(); ++i) {
TextureMapperLayer* child = toTextureMapperLayer(graphicsLayer->children()[i]);
if (!child)
continue;
m_children.append(child);
child->m_parent = this;
}
}
...
}
Comment on attachment 177164[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=177164&action=review>>> Source/WebKit2/ChangeLog:11
>>> + flushes layer states by the tree order.
>>
>> But setting the parent/children has nothing to do with flushing... I still don't think there's a bug here.
>
> In my understanding, setting children causes flushing. During flushing, we set children to each TextureMapperLayer.
>
> So, if animation calls updateViewport() between flushing root layer and DidRenderFrame, flash can occur.
>
> void LayerTreeCoordinator::syncLayerChildren(WebLayerID id, const Vector<WebLayerID>& children)
> {
> m_shouldSyncFrame = true;
> m_webPage->send(Messages::LayerTreeCoordinatorProxy::SetCompositingLayerChildren(id, children));
> }
>
> void TextureMapperLayer::flushCompositingStateSelf(GraphicsLayerTextureMapper* graphicsLayer, TextureMapper*)
> {
> ...
> if (changeMask & ParentChange) {
> TextureMapperLayer* newParent = toTextureMapperLayer(graphicsLayer->parent());
> if (newParent != m_parent) {
> // Remove layer from current from child list first.
> if (m_parent) {
> size_t index = m_parent->m_children.find(this);
> m_parent->m_children.remove(index);
> m_parent = 0;
> }
> // Set new layer parent and add layer to the parents child list.
> if (newParent) {
> m_parent = newParent;
> m_parent->m_children.append(this);
> }
> }
> }
>
> if (changeMask & ChildrenChange) {
> // Clear children parent pointer to avoid unsync and crash on layer delete.
> for (size_t i = 0; i < m_children.size(); i++)
> m_children[i]->m_parent = 0;
>
> m_children.clear();
> for (size_t i = 0; i < graphicsLayer->children().size(); ++i) {
> TextureMapperLayer* child = toTextureMapperLayer(graphicsLayer->children()[i]);
> if (!child)
> continue;
> m_children.append(child);
> child->m_parent = this;
> }
> }
> ...
> }
Right, but in this case you're not flushing to TextureMapperLayer, you're flushing CoordinatedGraphicsLayer... Since we only call DidRenderFrame at the end anyway, I still fail to see your point :)
(In reply to comment #9)
> (From update of attachment 177164[details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177164&action=review
> Right, but in this case you're not flushing to TextureMapperLayer, you're flushing CoordinatedGraphicsLayer... Since we only call DidRenderFrame at the end anyway, I still fail to see your point :)
OMG, I understand what you mean.
Only DidRenderFrame can start to sync TextureMapperLayer tree, so this patch can not change any behavior.
This patch only improves readability slightly. Could you think this patch needed? If so, I'll change changelog and repost.
> This patch only improves readability slightly. Could you think this patch needed? If so, I'll change changelog and repost.
I think it's questionable whether this improves readability or not... I would propose to give it up :)
(In reply to comment #11)
> > This patch only improves readability slightly. Could you think this patch needed? If so, I'll change changelog and repost.
> I think it's questionable whether this improves readability or not... I would propose to give it up :)
Your concern is very reasonable. However, I can not give it up because of actor model.
After implementing actor model TextureMapperLayer, LayerTreeRenderer must manage tree traversing while GraphicsLayerTextureMapper manages tree traversing too. (like Bug 103366).
I have the plan that TextureMapperLayer has a queue like render queue of LayerTreeRenderer. and then LayerTreeRenderer dispatches a task like SetLayerState to TextureMapperLayer when receiving the given task. and then when LayerTreeRenderer calls TextureMapperLayer::flushCompositingState(), TextureMapperLayer will flush all tasks in the queue.
Before implementing above actor model, we need to refactor the communication between LayerTreeCoordinator and LayerTreeRenderer.
Currently, LayerTreeCoordinator sends messages by convenient order for implementation, but in WK1 the order is impossible.
For example,
1. this bug. It is possible to sync WebCore root layer before synchronizing AC root layer.
2. synchronizing layer states can be called before calling SetRootLayerID.
3. LayerTreeRenderer::setChildrend() can create children layers. I want to create children layers as soon as Web Process children layers are created.
4. etc..
If we make the communication order consistent with WK1, I think we can get two benefits.
1. Easy to understand call flow.
2. Avoid potential bugs like Bug 103522.
Most of all, LayerTreeRenderer can just dispatch tasks to TextureMapperLayer in that LayerTreeRenderer can believe LayerTreeCoordinator sends messages by right order!
Comment on attachment 177176[details]
Patch: Remove updateViewport() in LayerTreeCoordinatorProxy::setRootCompositingLayer()
Reapply changelog with new bug title.
Comment on attachment 177173[details]
Patch: flush compositing states of layers by the tree order
View in context: https://bugs.webkit.org/attachment.cgi?id=177173&action=review> Source/WebKit2/ChangeLog:14
> + It is a preparation patch to change TextureMapperLayer to actor model.
> +
> + We should flush compositing states of layers by the tree order: root layer,
> + non-composited layer, page-overlay layer and WebCore's root layer. Currently, it
> + is not a problem, but when implementing actor model, it can causes bugs, because
> + LayerTreeRenderer expects to receive the SetLayerState message by the tree
> + order. This patch flushes layer states by the tree order.
Reword:
Send messages to the UI process by the tree order.
This is in preparation for refactoring TextureMapper to work in an actor model ([put link to meta-bug for that refactor here]).
(In reply to comment #17)
> (From update of attachment 177173[details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177173&action=review
>
> > Source/WebKit2/ChangeLog:14
> > + It is a preparation patch to change TextureMapperLayer to actor model.
> > +
> > + We should flush compositing states of layers by the tree order: root layer,
> > + non-composited layer, page-overlay layer and WebCore's root layer. Currently, it
> > + is not a problem, but when implementing actor model, it can causes bugs, because
> > + LayerTreeRenderer expects to receive the SetLayerState message by the tree
> > + order. This patch flushes layer states by the tree order.
>
> Reword:
> Send messages to the UI process by the tree order.
> This is in preparation for refactoring TextureMapper to work in an actor model ([put link to meta-bug for that refactor here]).
Thx for rephrasing!!
Comment on attachment 177190[details]
Patch: Clarify LayerTreeRenderer::setRootLayerID() can be called only once
I have run layout test(compositing) and load various tests(e.g. snowstack) with Qt WK2 debug build.
I have problem about TiledBackingStore.
When I clear the life cycle of CoordinatedGraphicsLayer, I find CreateTile and RemoveTile messages can be sent to UI Process regardless of CoordinatedGraphicsLayer::flushCompositingStateForThisLayerOnly(). It means UI Process can receive CreateTile message before LayerTreeRenderer creates the given layer.
I need to refactor CoordinatedGraphicsLayer with TiledBackingStore before progressing about the life cycle of CoordinatedGraphicsLayer.
I'll refactor in Bug 103959
Comment on attachment 177189[details]
Patch: Send SetRootCompositingLayer message to the UI process before SetLayerState message.
This feels a bit awkward.
I think LayerTreeCoordinator should deal with this internally, e.g. by sending the rootLayer message once right before the first coordinating message, rather than have CoordinatedGraphicsLayer know about this.
Comment on attachment 177190[details]
Patch: Clarify LayerTreeRenderer::setRootLayerID() can be called only once
LGTM, not committing in case this depends on something else...
(In reply to comment #27)
> (From update of attachment 177189[details])
> This feels a bit awkward.
> I think LayerTreeCoordinator should deal with this internally, e.g. by sending the rootLayer message once right before the first coordinating message, rather than have CoordinatedGraphicsLayer know about this.
I agree on you. My first trial was similar to your comments, but I failed, because of two reasons.
1. RenderLayerBacking calls CoordinatedGraphicsLayer::flushCompositingState. I cannot handle it.
2. We can communicate only if LayerTreeCoordinator::m_layerFlushSchedulingEnabled is true, but the constructor of LayerTreeCoordinator set m_layerFlushSchedulingEnabled to false, so I can not send the SetRootLayer message in constructor.
Can I send the SetRootLayer message in constructor. If so, everything will be fine. If not, I'll investigate what can I do, and I need advice too. :-)
(In reply to comment #30)
> (In reply to comment #27)
> > (From update of attachment 177189[details] [details])
> > This feels a bit awkward.
> > I think LayerTreeCoordinator should deal with this internally, e.g. by sending the rootLayer message once right before the first coordinating message, rather than have CoordinatedGraphicsLayer know about this.
>
> I agree on you. My first trial was similar to your comments, but I failed, because of two reasons.
> 1. RenderLayerBacking calls CoordinatedGraphicsLayer::flushCompositingState. I cannot handle it.
> 2. We can communicate only if LayerTreeCoordinator::m_layerFlushSchedulingEnabled is true, but the constructor of LayerTreeCoordinator set m_layerFlushSchedulingEnabled to false, so I can not send the SetRootLayer message in constructor.
>
> Can I send the SetRootLayer message in constructor. If so, everything will be fine. If not, I'll investigate what can I do, and I need advice too. :-)
Oops, I found the constructor of LayerTreeCoordinator set m_layerFlushSchedulingEnabled to true. I think we can send SetRootLayer message in the constructor as you suggest. I'll post.
Comment on attachment 177429[details]
Patch: Send SetRootCompositingLayer message to the UI process before SetLayerState message.
View in context: https://bugs.webkit.org/attachment.cgi?id=177429&action=review> Source/WebKit2/ChangeLog:8
> + Send SetRootCompositingLayer message to the UI process firstly.
Send SetRootCompositingLayer message to the UI process in the constructor instead of sending it on the first flush.
> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.h:138
> + void setRootCompositingLayer();
initializeRootCompositingLayer
Comment on attachment 177431[details]
Patch: Clarify LayerTreeRenderer::setRootLayerID() can be called only once
Rejecting attachment 177431[details] from commit-queue.
Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2
Last 500 characters of output:
: /mnt/git/webkit-commit-queue
Parsed 2 diffs from patch file(s).
patching file Source/WebKit2/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp
Hunk #1 FAILED at 390.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp.rej
Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue
Full output: http://queues.webkit.org/results/15100918
Comment on attachment 177441[details]
Patch: Clarify LayerTreeRenderer::setRootLayerID() can be called only once
Rejecting attachment 177441[details] from commit-queue.
Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2
Last 500 characters of output:
: /mnt/git/webkit-commit-queue
Parsed 2 diffs from patch file(s).
patching file Source/WebKit2/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp
Hunk #1 FAILED at 390.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp.rej
Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue
Full output: http://queues.webkit.org/results/15119720
Comment on attachment 177439[details]
Patch: Send SetRootCompositingLayer message to the UI process before SetLayerState message.
Rejecting attachment 177439[details] from commit-queue.
Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2
Last 500 characters of output:
ripts/update-webkit line 152.
Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 2
Updating OpenSource
From http://git.chromium.org/external/Webkit
e1d9afb..eb9e8cc HEAD -> origin/HEAD
error: Ref refs/remotes/origin/master is at eb9e8ccd030f66f658b9fcdabeeb134aaaf82783 but expected e1d9afb15f764221f3a35669b6a775bba2812d7f
! e1d9afb..eb9e8cc master -> origin/master (unable to update local ref)
Died at Tools/Scripts/update-webkit line 152.
Full output: http://queues.webkit.org/results/15138117
Comment on attachment 177439[details]
Patch: Send SetRootCompositingLayer message to the UI process before SetLayerState message.
I think commit bot can not handle this edge case of changelog. I'll post again
Comment on attachment 177452[details]
Patch: Send SetRootCompositingLayer message to the UI process before SetLayerState message.
View in context: https://bugs.webkit.org/attachment.cgi?id=177452&action=review> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:122
> + initializeRootCompositingLayer();
EFL can not send messages to UI Process at this time, although Qt can.
It causes EFL hitting ASSERT in Bug 104257.
I'll send SetRootCompositingLayer message at the first flush time.
(In reply to comment #56)
> Created an attachment (id=179199) [details]
> Patch
This patch rollouted in Bug 104260 due to other reason.
Bug 104518 fixed the reason, so I post again this patch.
Comment on attachment 179199[details]
Patch: Clarify LayerTreeRenderer::setRootLayerID() can be called only once
Turn on r? because code is little changed.
Previous patch added following assertion.
+ ASSERT(layerID != m_rootLayerID);
+ ASSERT(layerID != InvalidWebLayerID);
This patch added following assertion.
+ ASSERT(layerID != InvalidCoordinatedLayerID);
+ ASSERT(m_rootLayerID == InvalidCoordinatedLayerID);
This assertion is more strict.
2012-12-02 15:36 PST, Dongseong Hwang
2012-12-02 17:19 PST, Dongseong Hwang
2012-12-02 21:29 PST, Dongseong Hwang
2012-12-02 21:37 PST, Dongseong Hwang
2012-12-02 22:40 PST, Dongseong Hwang
2012-12-02 22:44 PST, Dongseong Hwang
2012-12-02 23:07 PST, Dongseong Hwang
2012-12-02 23:14 PST, Dongseong Hwang
dongseong.hwang: commit-queue-
2012-12-04 00:08 PST, Dongseong Hwang
noam: commit-queue-
2012-12-04 00:11 PST, Dongseong Hwang
2012-12-04 01:07 PST, Dongseong Hwang
2012-12-04 01:10 PST, Dongseong Hwang
2012-12-04 02:12 PST, Dongseong Hwang
2012-12-05 20:26 PST, Dongseong Hwang
2012-12-09 20:50 PST, Dongseong Hwang
2012-12-12 22:37 PST, Dongseong Hwang
noam: commit-queue-
2012-12-13 16:22 PST, Dongseong Hwang