Bug 87515 – [chromium] New animations are not always started in CCLayerTreeHost before commit

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Dana Jansens]

crbug: http://code.google.com/p/chromium/issues/detail?id=123090

Animations now tick on the main thread after bug #87301. But we are not guaranteed that new animations are ticked on the first commit in which they exist. This causes the same missing tile symptoms to occur, though they are corrected quickly when the main thread does tick with a future frame.

[Dana Jansens]

(FYI This is all in single-thread mode)

Created attachment 147865 [details]
Patch

This new patch guarantees that we will have up-to-date layer positions on the
main thread before calling updateLayers.

[James Robinson]

Comment on attachment 147865 [details]
Patch

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

So the problem here is that in single-threaded mode we don't get an animate() call from the RenderWidget in some cases because we haven't called scheduleAnimation(), right?  Where is scheduleAnimation() normally called in the single-threaded path?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:154
> +    virtual void updateClientAnimations(double monotonicFrameBeginTime);
> +    virtual void updateLayerAnimations(double monotonicFrameBeginTime);

the "client" vs "layer" distinction doesn't seem very helpful.  all animations are set on a layer and all animations have a client

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:350
> +    double monotonicTime = monotonicallyIncreasingTime();
> +    m_layerTreeHost->updateLayerAnimations(monotonicTime);

does this mean you're using a different timebase for compositor-driven animations vs WebCore-driven animations? that seems like a really bad idea unless it's completely unavoidable - we should try to use the same timebase for all animations

[James Robinson]

Looking at the code perhaps we don't call scheduleAnimation() at all in the single-threaded path, which is probably the root cause of the bug.  If we want render_widget to call animate() on us we have to call scheduleAnimation()

[James Robinson]

Ping - I'm pretty curious about the answer to comment #4

(In reply to comment #5)
> Ping - I'm pretty curious about the answer to comment #4

Sorry for the delay. It took me a bit to do some research to give you a proper answer.

(In reply to comment #3)
> (From update of attachment 147865 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147865&action=review
> 
> So the problem here is that in single-threaded mode we don't get an animate() call from the RenderWidget in some cases because we haven't called scheduleAnimation(), right?  

It turns out that there are several problems.

1) We do not schedule an animation when an animation is added to a layer (see below for an explanation of how schedule animation is normally called). I have written code to do this, but this does not fix the issue, because

2) RW::AnimateIfNeeded in RW::DoDeferredUpdate is called before webwidget_->layout(), which can cause animations to be added (specifically, if a new layer is added to the tree that has an animation. See LayerChromium::setLayerTreeHost), and changing the order of these appears to be a bad idea (I saw lots of flickering that I did not investigate). But, even if we did reorder these

3) RW::AnimateIfNeeded is not guaranteed to tick the animations. It is rate limited, which seems fundamental to its design?

We need to guarantee that compositor-driven animations are updated before compositing, and it doesn't appear that this approach can make this guarantee. Please let me know if I've missed something.

> Where is scheduleAnimation() normally called in the single-threaded path?

This is normally called via AnimationControllerPrivate's updateStyleIfNeeded timer, which bubbles its way out through the FrameView.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:154
> > +    virtual void updateClientAnimations(double monotonicFrameBeginTime);
> > +    virtual void updateLayerAnimations(double monotonicFrameBeginTime);
> 
> the "client" vs "layer" distinction doesn't seem very helpful.  all animations are set on a layer and all animations have a client
It's true. How about s/Client//, and s/Layer/Compositor/ ?
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:350
> > +    double monotonicTime = monotonicallyIncreasingTime();
> > +    m_layerTreeHost->updateLayerAnimations(monotonicTime);
> 
> does this mean you're using a different timebase for compositor-driven animations vs WebCore-driven animations? that seems like a really bad idea unless it's completely unavoidable - we should try to use the same timebase for all animations

I've been using monotonic times to be consistent with the other cc code. I'm not sure how involved this change would be, but does seem like a good idea. Could it be done in another CL, though?

Created attachment 150827 [details]
Patch

Rebasing patch.

Also renamed update animation functions. To remain consistent with WebCore,
I've renamed the function to update the compositor driven animations
updateAcceleratedAnimations, and I've switched updateClientAnimations back to
updateAnimations.

[James Robinson]

Is this patch still valid? Does it still apply?

Created attachment 155057 [details]
Patch

(In reply to comment #8)
> Is this patch still valid? Does it still apply?

No it didn't, but I've just rebased it.

[James Robinson]

(In reply to comment #6)
> (In reply to comment #5)
> > Ping - I'm pretty curious about the answer to comment #4
> 
> Sorry for the delay. It took me a bit to do some research to give you a proper answer.
> 
> (In reply to comment #3)
> > (From update of attachment 147865 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=147865&action=review
> > 
> > So the problem here is that in single-threaded mode we don't get an animate() call from the RenderWidget in some cases because we haven't called scheduleAnimation(), right?  
> 
> It turns out that there are several problems.
> 
> 1) We do not schedule an animation when an animation is added to a layer (see below for an explanation of how schedule animation is normally called). I have written code to do this, but this does not fix the issue, because

I think fixing this would fix your problem (see below)

> 
> 2) RW::AnimateIfNeeded in RW::DoDeferredUpdate is called before webwidget_->layout(), which can cause animations to be added (specifically, if a new layer is added to the tree that has an animation. See LayerChromium::setLayerTreeHost), and changing the order of these appears to be a bad idea (I saw lots of flickering that I did not investigate). But, even if we did reorder these

Yes, you always want to animate() before layout().  That should happen.

> 
> 3) RW::AnimateIfNeeded is not guaranteed to tick the animations. It is rate limited, which seems fundamental to its design?

The rate limiting doesn't happen in compositing mode, so I don't see how that is relevant here (assuming that the context supports the swap buffers extension, which is always the case).

> 
> We need to guarantee that compositor-driven animations are updated before compositing, and it doesn't appear that this approach can make this guarantee. Please let me know if I've missed something.

That's already taken care of.  I don't think you need or want to sidestep the RenderWidget flow control mechanisms.

> > 2) RW::AnimateIfNeeded in RW::DoDeferredUpdate is called before webwidget_->layout(), which can cause animations to be added (specifically, if a new layer is added to the tree that has an animation. See LayerChromium::setLayerTreeHost), and changing the order of these appears to be a bad idea (I saw lots of flickering that I did not investigate). But, even if we did reorder these
> 
> Yes, you always want to animate() before layout().  That should happen.

The problem is that when layout() creates an animation, we will end up calling updateLayers() before ticking this animation, which can cause flickering. So, it seems to me that if we continue to lump all the animation servicing together, we'll need to run animate() before layout() (so that layout() has the values from the WebCore animations needed for it to run correctly), and also after layout() (so that updateLayers() has the values from the compositor-driven animations it needs to run correctly). This made me think it would be simpler to split up the animate() so that each of its components could be guaranteed to happen when it needs to.

[James Robinson]

(In reply to comment #12)
> > > 2) RW::AnimateIfNeeded in RW::DoDeferredUpdate is called before webwidget_->layout(), which can cause animations to be added (specifically, if a new layer is added to the tree that has an animation. See LayerChromium::setLayerTreeHost), and changing the order of these appears to be a bad idea (I saw lots of flickering that I did not investigate). But, even if we did reorder these
> > 
> > Yes, you always want to animate() before layout().  That should happen.
> 
> The problem is that when layout() creates an animation, we will end up calling updateLayers() before ticking this animation, which can cause flickering. So, it seems to me that if we continue to lump all the animation servicing together, we'll need to run animate() before layout() (so that layout() has the values from the WebCore animations needed for it to run correctly), and also after layout() (so that updateLayers() has the values from the compositor-driven animations it needs to run correctly). This made me think it would be simpler to split up the animate() so that each of its components could be guaranteed to happen when it needs to.

That's a really good point.

I actually wonder if we should squash animateAndLayout() together to one call and pump compositor-driven animations after that call - but sharing the timestamp from animate().  We don't want to run all animate()s after layout(), just the compositor-driven ones that (by definition) can't change layout.

[Stephen Chenney]

Marked LayoutTest bugs, bugs with Chromium IDs, and some others as WontFix. Test failure bugs still are trackable via TestExpectations or disabled unit tests.