Bug 87301 – [chromium] WebLayerTreeViewImpl should not hide methods in CCLayerTreeHost with signatures that match the Client interface

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dana Jansens]

[chromium] Make single-thread proxy tick animations and use the main thread tree host like multi-thread proxy does

[Dana Jansens]

Created attachment 143631 [details]
Patch

[James Robinson]

Comment on attachment 143631 [details]
Patch

I don't understand this at all.  content::RenderWidget is calling willBeginFrame()/updateAnimations()/layout() - you want to call them again?  This patch feels incomplete, at a minimum.

[Nat Duca]

Comment on attachment 143631 [details]
Patch

What does vollick think of this?

[Dana Jansens]

Vollick and I debugged this together, but he can speak to it if he likes :)

So, I see RenderWidget is calling layout() directly. I had no idea about that and was just trying to match CCThreadProxy. I'll remove that.

I put a CRASH() in CCLTH::willBeginFrame() and I'm not seeing that get called either.. but I don't know the chromium side well at all, so if you think that doesn't belong I'll remove that too.

But I think we really need this call to updateAnimations. What I am tracing from RenderWidget is:

RenderWidget calls CCLTH::composite().
This calls CCSingleThreadProxy::compositeImmediately().
This eventually calls CCLTHImpl::animate().

But nothing is currently calling CCLTH::updateAnimations() on the main thread, right? (Putting prints in tickAnimations verified that it never happens on main thread when in single thread mode.)

[James Robinson]

Read or step in a debugger through the calls between RenderWidget, WebViewImpl, and CCLayerTreeHost inside of RenderWidget::DoDeferredUpdate - maybe that'll make things clearer.  I think it is already calling updateAnimations() from WebViewImpl::animate().

It's important to remember that in threaded mode, the compositor is 100% in charge of frame scheduling and it drives every call.  In single-threaded mode, the responsibility of when to make frames is split across render_widget.cc in the chromium repo, WebViewImpl in WebKit, and CCSingleThreadProxy in the compositor.  This diffusion of responsibilities definitely makes things harder to understand.  It also means that typically adding or removing calls from any one of those components without modifying the others is almost always wrong.

[Dana Jansens]

Ohh, okay thanks. I think I see the problem then.

We have updateAnimations() in the CCLTHClient interface, but also in the CCLTH class, and it's not a no-op to call the client, like most methods in the Client interface.

WebLayerTreeViewImpl overrides updateAnimations() to implement the client interface, but this blocks CCLTH::updateAnimations() from being called. The right answer here isn't super obvious to me yet, but thanks for the pointers.

[Dana Jansens]

Created attachment 143659 [details]
Patch

[Dana Jansens]

This approach makes WebLayerTreeViewImpl not implement the CCLTHClient interface itself, so that WebViewImpl can call CCLayerTreeHost::updateAnimations().

We could alternately change the method signature of updateAnimations() on CCLayerTreeHost to not match the client interface, if you'd prefer that?

[James Robinson]

Comment on attachment 143659 [details]
Patch

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

I'm not entirely sure which problem this solves - could you expand a bit?

> Source/WebKit/chromium/src/WebLayerTreeViewImpl.h:46
> +    // Converts messages from CCLayerTreeHostClient to WebLayerTreeViewClient.
> +    class ClientConverter : public WebCore::CCLayerTreeHostClient {

does this have to be in the header (could it be in the cpp)?

[Dana Jansens]

(In reply to comment #9)
> (From update of attachment 143659 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143659&action=review
> 
> I'm not entirely sure which problem this solves - could you expand a bit?

Sure. WebViewImpl is calling updateAnimations() on WebLayerTreeView, which calls WebLayerTreeViewImpl. But in this case, it matches the interface for the CCLayerTreeHostClient, and it just forwards the method call on to its own client.

CCLayerTreeHost has an updateAnimations which unfortunately has the same signature as the client, and this method should be called on each commit, before calling updateLayers.

Without calling this, the main thread never ticks (or starts) animations, which means that its view of the world is wrong, and actually takes on the animation target values. If that is a layer sliding off screen, the commit considers the layer to be outside the viewport (and its visibleLayerRect is empty then). So if the layer also invalidates its contents while animating, the main thread does not paint or commit the tiles, causing missing tiles to appear for the animating layer.

Making CCLTH::updateAnimations be called during the commit fixes this all, as the animations tick appropriately, and the first commit with the animations has the starting value of the animation, not the ending value. And each frame the two sides will have the same view of the animations from then on.

It's really a C++ weirdness since we inherit the function from two places in WebLayerTreeViewImpl, which is bad. No other functions in the client interface do anything useful in the CCLayerTreeHost implementations (it just calls the client). In particular the applyScrollAndScale method appears to be a similar case, but its signature differs from the client in its parameters.

> > Source/WebKit/chromium/src/WebLayerTreeViewImpl.h:46
> > +    // Converts messages from CCLayerTreeHostClient to WebLayerTreeViewClient.
> > +    class ClientConverter : public WebCore::CCLayerTreeHostClient {
> 
> does this have to be in the header (could it be in the cpp)?

I think the WebLayerTreeView needs to hold an instance of the class, so that it can hold the WebLayerTreeViewClient*. So I think it needs to be in the header?

[Nat Duca]

Can't this fix just be a matter of factoring out the updateAnimations path on cclth to a standalone cclth method. Then, ccthreadproxy and ccsinglethreadproxy can call that in their respective call paths? Better to not tinker with the messed up single thread case and make it more messy...

[Dana Jansens]

Is that equivalent to just removing updateAnimations from the CCLTHClient, and the call to WebLayerTreeView from WebViewImpl when compositing is enabled?  (The client is the ui::Compositor which has an empty method for this.)

I guess that separates the compositing from software code paths a little more which is good/bad/not sure?

[James Robinson]

Splitting out the client impl from the WebLayerTreeView impl seems like a generally good thing.  I think Nat and I are a bit more nervous about changing the behavior around of the single threaded path, that's more scary.

[Nat Duca]

I'm fine with fixing the WLTVI, that was a huge mess to begin with and I might even have code that does that. That should be a standalone patch, not part of a bugfix.

Can we just make CCSingleThreadProxy::composite() call up to m_layerTreeHost->animateLayers()?

Comment on attachment 143631 [details]
Patch

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

(In reply to comment #14)
> Can we just make CCSingleThreadProxy::composite() call up to m_layerTreeHost->animateLayers()?

This is essentially the solution I'm hoping for, except that it needs to be called before updateLayers() so that the fresh, animated transforms can be used for culling, etc (we saw flashes when we didn't do this).

In CCThreadProxy, ticking the main thread animations is tied to commits. In CCThreadProxy::beginFrame, there is a call to m_layerTreeHost->updateAnimations(...) that precedes the call to m_layerTreeHost->updateLayers(...). I think CCSingleThreadProxy should behave similarly. I'm hoping we can strip down patch set #1 so that the only thing added to commitAndComposite() is the call to updateAnimations. Could that work?

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:359
> +    m_layerTreeHost->updateAnimations(monotonicTime);

Can we nix the calls to willBeginFrame() and layout() here?

[Dana Jansens]

I guess we need to remember that WebViewImpl is calling WebLayerTreeViewImpl::animateLayers() also. And if we fix the WLTVI client craziness, then we'd be calling animateLayers() on CCLTH twice. We'd need to remove the call from WebViewImpl, if/when we fixed WLVHI.

(In reply to comment #16)
> I guess we need to remember that WebViewImpl is calling WebLayerTreeViewImpl::animateLayers() also. And if we fix the WLTVI client craziness, then we'd be calling animateLayers() on CCLTH twice. We'd need to remove the call from WebViewImpl, if/when we fixed WLVHI.

Good point. If the fix for WLVHI ensures that updateAnimations is called before CCSingleThreadProxy::compositeImmediately (and it sounds like it might), then this is moot, but I think it's safe to tick the layer animations in both places (it should just freshen opacities and transforms). Nat suggested calling directly CCLTH::animateLayers directly. That seems safer than updateAnimations. What would you think of that?

[Dana Jansens]

If we move the updateAnimations() into CCSingleThreadProxy::commitAndComposite() it changes the call ordering. Current code always calls updateAnimations() -> layout() -> updateLayers().

If we move it to commitAndComposite() suddenly we are calling layout() -> updateAnimations() -> updateLayers(). Probably this isn't so good..

If we just fix the shadowing in WLTVI, then things are called in the correct order still. Or we could add another method on CCLayerTreeHost to do the animation step explicitly.. but that seems like what we already have?

[James Robinson]

This bug title sounds like it's just about a code smell (shadowing functions).  The recent set of comments appear to be about changing behavior around.  Which is this patch doing?  Can we please put the discussion of behavior changes on a bug that describes a problem the behavior changes are intended to solve?  I still don't understand exactly what the goal is here.

[Dana Jansens]

Right, I'm a bit confused too.

The patch stops shadowing functions, which makes CCLayerTreeHost::updateAnimations start getting used by WebViewImpl. We can rename it to something like that, if more appropriate?

The original bug is that updateAnimations is not called. And that is because m_layerTreeView.updateAnimations() gets shadowed by WLTVI.

So if we stop shadowing, the original bug goes away, while fixing the code weirdness.

So let's keep this about the shadowing.. and then refactor or change how updateAnimations is called in separate conversations?

[Nat Duca]

I buy it.

[Nat Duca]

Comment on attachment 143659 [details]
Patch

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

>>> Source/WebKit/chromium/src/WebLayerTreeViewImpl.h:46
>>> +    class ClientConverter : public WebCore::CCLayerTreeHostClient {
>> 
>> does this have to be in the header (could it be in the cpp)?
> 
> I think the WebLayerTreeView needs to hold an instance of the class, so that it can hold the WebLayerTreeViewClient*. So I think it needs to be in the header?

Nit: We've been calling these adapters. Also, we typically just declare these as non-nested classes then put the full implementation in the cpp. E.g. just say class CCLayerTreeViewToWebLayerTreViewImplCientAdapter, have an ownptr which is the adapter, then in the impl you can have the full implementation and implement the client functions directly in the class declaration.

[James Robinson]

Comment on attachment 143659 [details]
Patch

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

R=me

We chatted offline about this.  The problem today the call sequence looks like:

WebViewImpl -> WebLayerTreeView::updateAnimations() -> WebLayerTreeViewImpl::updateAnimations() -> WebLayerTreeViewClient::updateAnimations() -> WebViewImpl::updateAnimations()

essentially it's short-circuiting in WLTVI.  This is a problem because CCLayerTreeHost::updateAnimations isn't called, so CCLTH::m_animating is not set and CCLTH::animateLayers() is not called.  CCLTH::animateLayers() updates the main-thread animated state of layers, which isn't what is drawn (that's the impl-thread animated state of layers) but is what determines painting / uploading.

With this patch the call sequence becomes:

WebViewImpl -> WebLayerTreeView::updateAnimations() -> WebLayerTreeViewImpl::updateAnimations() -> CCLayerTreeHost::updateAnimations() -> CCLayerTreeHostClient::updateAnimations() -> WebLayerTreeViewImpl::ClientConverter::updateAnimations() -> WebLayerTreeViewClient::updateAnimations() -> WebViewImpl::updateAnimations()

for the win!

>>> Source/WebKit/chromium/src/WebLayerTreeViewImpl.h:46
>>> +    class ClientConverter : public WebCore::CCLayerTreeHostClient {
>> 
>> does this have to be in the header (could it be in the cpp)?
> 
> I think the WebLayerTreeView needs to hold an instance of the class, so that it can hold the WebLayerTreeViewClient*. So I think it needs to be in the header?

WebLayerTreeViewImpl could just have an OwnPtr<ClientConverter> to not need to see the definition, but it's a minor thing.

Our typical naming for this sort of type is "...Adapter"

[Dana Jansens]

Hmm. If I make the client adapter an OwnPtr in the WLTVI, then I have no pointer to pass to the CCLTH constructor from the WLTVI constructor yet. I think the choices are keep it in the header, or change CCLTH constructor to not take a client*, and add a setClient()?

static PassOwnPtr<CCLayerTreeHost> create(CCLayerTreeHostClient*, const CCSettings&);

[Dana Jansens]

Ian brought up a good point, in that it doesn't seem that updateAnimations() is *always* called before commit, and he was concerned that might cause flashing for all of the same reasons. If an animation starts but we draw a frame before we decide to call updateAnimations, layers have the wrong values on the main thread and we commit the wrong tiles.

I double checked this and sure enough, going to http://html5-demos.appspot.com/static/html5-whats-new/template/index.html#5 and pressing right-left-right-left-right-left keys enough times, you eventually see the tiles disappear with a flash before reappearing.

If we're going to updateAnimations from RenderWidget at all, then I think this is change still right, and we can worry about changing how we call animate() in RenderWidget in another bug.

But I'd like to verify that this is the right path to be taking before I submit this?

[Dana Jansens]

Created attachment 143927 [details]
Patch for landing

[Antoine Labour]

Mmh, lifetime issues: When the WLTVI gets destructed, the m_clientConverter is destroyed before the CCLayerTreeHost destructor runs. If it calls the client from there, (e.g. as a side effect of dropping the last reference on the root, which will destroy it, which may try to trigger a commit), we're going to access a destroyed object.

Now that I'm thinking of it, it's probably a theoretical problem in the current code, actually, but we're lucky in practice because "destroying" a pod is a noop?

[Dana Jansens]

Hm, yeh I'll add setClient() to CCLTH I guess, then we can setClient(0) in the destructor here, and move the Adapter into the .cpp.

[Nat Duca]

(In reply to comment #27)
> Mmh, lifetime issues: When the WLTVI gets destructed, the m_clientConverter is destroyed before the CCLayerTreeHost destructor runs. If it calls the client from there, (e.g. as a side effect of dropping the last reference on the root, which will destroy it, which may try to trigger a commit), we're going to access a destroyed object.
You can fix this by making the WLTVI own a CCLTH instead of being a CCLTHI. I think thats much cleaner anyway.

[Nat Duca]

(In reply to comment #28)
> Hm, yeh I'll add setClient() to CCLTH I guess, then we can setClient(0) in the destructor here, and move the Adapter into the .cpp.

That means we have to null check all client calls :(

[Dana Jansens]

Created attachment 144075 [details]
Patch

ok! The patch is a little bigger.. but ownership/lifetime issues resolved.

- WebLayerTreeViewImpl owns a CCLayerTreeHost.
- WebLTVI calls CCLTH::create() now so it does not need to call initialize() itself.
- Moved the ClientAdapter into the WLTVI.cpp
- Expose the CCLTH on WLTVI and call methods on it directly in WebLayerTreeView.

@jamesr Could you please take another look at this one?

[Nat Duca]

Comment on attachment 144075 [details]
Patch

nicely done. /me likes this incarnation.

[James Robinson]

Comment on attachment 144075 [details]
Patch

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

Looks good.  Not setting cq? since it seems this version doesn't apply cleanly.

> Source/WebKit/chromium/src/WebLayerTreeViewImpl.h:53
> +    OwnPtr<WebLayerTreeViewClientAdapter> m_clientAdapter;
> +    OwnPtr<WebCore::CCLayerTreeHost> m_layerTreeHost;

If you flip the declaration order of these the CCLTH will die before the clientAdapter without needing any code in the d'tor

[Dana Jansens]

Comment on attachment 144075 [details]
Patch

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

thanks!

>> Source/WebKit/chromium/src/WebLayerTreeViewImpl.h:53
>> +    OwnPtr<WebCore::CCLayerTreeHost> m_layerTreeHost;
> 
> If you flip the declaration order of these the CCLTH will die before the clientAdapter without needing any code in the d'tor

But then I have to initialize the layerTreeHost first, and it wants the m_clientAdapter's pointer right? I think it needs to be in this order for the constructor :/

[Dana Jansens]

Created attachment 144130 [details]
Patch for landing

[Antoine Labour]

Comment on attachment 144075 [details]
Patch

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

>>> Source/WebKit/chromium/src/WebLayerTreeViewImpl.h:53
>>> +    OwnPtr<WebCore::CCLayerTreeHost> m_layerTreeHost;
>> 
>> If you flip the declaration order of these the CCLTH will die before the clientAdapter without needing any code in the d'tor
> 
> But then I have to initialize the layerTreeHost first, and it wants the m_clientAdapter's pointer right? I think it needs to be in this order for the constructor :/

Err, aren't fields destroyed in the opposite order of the construction? This looks correct to me.

[Dana Jansens]

Comment on attachment 144130 [details]
Patch for landing

Oooh. then i can remove the clear

[Dana Jansens]

Here's the reference http://msdn.microsoft.com/en-us/library/8183zf3x(v=vs.80).aspx thanks to piman

[Dana Jansens]

Created attachment 144138 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 144138 [details]
Patch for landing

Clearing flags on attachment: 144138

Committed r118563: <http://trac.webkit.org/changeset/118563>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

(In reply to comment #36)
> (From update of attachment 144075 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144075&action=review
> 
> >>> Source/WebKit/chromium/src/WebLayerTreeViewImpl.h:53
> >>> +    OwnPtr<WebCore::CCLayerTreeHost> m_layerTreeHost;
> >> 
> >> If you flip the declaration order of these the CCLTH will die before the clientAdapter without needing any code in the d'tor
> > 
> > But then I have to initialize the layerTreeHost first, and it wants the m_clientAdapter's pointer right? I think it needs to be in this order for the constructor :/
> 
> Err, aren't fields destroyed in the opposite order of the construction? This looks correct to me.

That's right, I fail at reading.