Bug 80744

Summary: [chromium] Threaded opacity animation jump to opacity of 0
Product: WebKit Reporter: Eric Penner <epenner>
Component: CSSAssignee: vollick
Status: RESOLVED FIXED    
Severity: Normal CC: cc-bugs, danakj, epenner, jamesr, nduca, shawnsingh, vollick, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: All   
Bug Depends on:    
Bug Blocks: 79536    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Eric Penner 2012-03-09 18:47:05 PST
Threaded opacity animations pop when the target opacity is 0. To see it, mouse over until the box becomes opaque, then move mouse away.

<!DOCTYPE html>
<html>
<head>
<style>
.box{
	width: 200px;
	height: 200px;
	border: 1px solid;
	background: #f9f900;
	opacity: 0.0;
  	-webkit-transition: opacity 2s ease-out;
}
.box:hover {
  opacity: .8;
}
</style>
</head>
<body>

<div class="box">
</div>

</body>
</html
Comment 1 Eric Penner 2012-03-12 16:24:54 PDT
FYI, I found the line that is causing the painting of the new layer to be skipped:

CCLayerTreeHostCommon.cpp:
static bool layerShouldBeSkipped(LayerType* layer)
...
if (!layer->drawsContent() || !layer->opacity() || layer->bounds().isEmpty())
    return true;
Comment 2 vollick 2012-03-13 10:28:21 PDT
Created attachment 131658 [details]
Patch
Comment 3 vollick 2012-03-13 10:37:13 PDT
(In reply to comment #1)
> FYI, I found the line that is causing the painting of the new layer to be skipped:
> 
> CCLayerTreeHostCommon.cpp:
> static bool layerShouldBeSkipped(LayerType* layer)
> ...
> if (!layer->drawsContent() || !layer->opacity() || layer->bounds().isEmpty())
>     return true;

As far as I can tell, the problem stems from the main thread's LayerChromiums being assigned the target value of an implicit animation and decisions are made based on that value. If we are transitioning to 0, then the LayerChromium gets set an opacity of 0 (even though the CCLayerImpl is busy animating towards that value) and that causes the subtree rooted at that layer to get skipped during drawing. I modified some code earlier in this file that filters subtrees based on opacity so that they don't do this while animating.

Another thing I noticed is that we commit a lot during implicit animations, and these values are getting transferred to the CCLayerImpl's. I updated the pushPropertiesTo function to take animations into account. If the CCLayerImpl has no animating property, values are copied to the CCLayerImpl, otherwise animated values are copied back to the LayerChromium.
Comment 4 Dana Jansens 2012-03-13 10:51:27 PDT
Comment on attachment 131658 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:529
> +        m_opacity = layer->opacity();

Is this going to work okay if the LayerChromium changes its opacity property during an animation? Maybe that's not possible but it worried me that the value just gets overwritten.
Comment 5 Nat Duca 2012-03-13 11:06:08 PDT
I'm still confused. If we're animating a layer opacity AND a commit comes through, that will clobber the opacity. Ok, but then the next draw should apply animations and re-set the opacity and we should be fine. What am I missing?
Comment 6 Dana Jansens 2012-03-13 11:17:21 PDT
Will this sequence work correctly?

1. Start animation to opacity X (Sets LayerChromium::m_opacity = X)
2. Animate animate animate on impl, currently at opacity Y
3. Invalidate something cause commit (Sets LayerChromium::m_opacity = Y)
4. Animation ends

What is the value of LayerChromium::m_opacity now?
Comment 7 Dana Jansens 2012-03-13 11:21:09 PDT
I'm not sure why we are copying back to the LayerChromium at all. Can't it just own the target values?

Maybe an API like

bool isAnimatingOpacity()
float opacity()
void setOpacity(float)

would make it easier to know what properties you should check isAnimating on before reading the value. Maybe reading the value of an animatable property should assert when isAnimatingThatProperty == true?
Comment 8 James Robinson 2012-03-13 18:50:14 PDT
Comment on attachment 131658 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:526
> +    if (!m_layerAnimationController->isAnimatingProperty(CCActiveAnimation::Opacity))

i feel a lot less confident about this change - why is it necessary? what values are we pushing during the animation itself?
Comment 9 James Robinson 2012-03-13 18:55:07 PDT
I don't think we should be copying any values "back" from the impl tree to the LayerChromium side ever.  The user of the compositor (WebCore in this instance, or other users) are setting up the animation parameters and it is their responsibility to interpolate the current value of an animated property if they want to know what that value is.  WebCore already has logic to do this for things like calling getComputedStyle() in the middle of an animation/transition and it's independent of the compositor.

I think we should just skip the opacity check when painting if there's an active animation on opacity.  We also need to be careful that culling takes into account the fact that the layer might not be opaque even if its opacity==1.0 if the opacity has an active animation.

The same issue exists for animating a transform as well - for example if a layer is rotated and its backface isn't visible, but there's an animation on the transform that will rotate the backface into view (perhaps only temporarily), then we should go ahead and paint that.  This problem is more general since a transform can cause some unknown amount of the layer to be exposed.  I don't know what the best solution is - the conservative thing would be raster as much as we can if the transform is being animated, and not let anything in the subtree influence culling.
Comment 10 Nat Duca 2012-03-13 22:26:44 PDT
Ian is reworking this patch based on a whiteboard meeting he, Eric and I had this morning. The new and improved version avoids some of the things that smell funny about this patch.
Comment 11 vollick 2012-03-14 07:40:37 PDT
Created attachment 131843 [details]
Patch

There's no more copying back to the LayerChromiums. This patch just ensures we don't skip subtrees due to opacity when opacity is animating.

I have also removed the code that bails when animation delay is set, since we get that for free. I have also updated CCLayerAnimationController::removeCompletedAnimations so that it does not attempt to destroy animations on the wrong thread.

I will be writing another patch which will tick animations on the main thread as well to supply better information to the culler, but this is a larger change.
Comment 12 Nat Duca 2012-03-14 12:40:50 PDT
Lets pull the deletion bugfix out to another patch.

The animation changes LGTM.
Comment 13 James Robinson 2012-03-14 14:19:59 PDT
Comment on attachment 131843 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:115
> +static bool subtreeShouldBeSkipped(LayerType* layer, float drawOpacity)

I think this is wrong for impl-side since we calculate this for every frame. If on the impl side a layer has opacity == 0 on a given frame then we definitely can and should skip it, even if there's an active animation, because we'll redo the calculation on the next frame.  On the main thread side I think we shouldn't skip the subtree if there is an animation active since we don't run through this code for every frame.

You could specialize this for the two layer types to get divergent behavior.
Comment 14 James Robinson 2012-03-14 14:25:41 PDT
(In reply to comment #12)
> Lets pull the deletion bugfix out to another patch.

+1 to this as well
Comment 15 vollick 2012-03-15 11:13:08 PDT
Created attachment 132080 [details]
Patch

(In reply to comment #13)
> (From update of attachment 131843 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131843&action=review
>
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:115
> > +static bool subtreeShouldBeSkipped(LayerType* layer, float drawOpacity)
>
> I think this is wrong for impl-side since we calculate this for every frame. If on the impl side a layer has opacity == 0 on a given frame then we definitely can and should skip it, even if there's an active animation, because we'll redo the calculation on the next frame.  On the main thread side I think we shouldn't skip the subtree if there is an animation active since we don't run through this code for every frame.
>
> You could specialize this for the two layer types to get divergent behavior.

Done.

I've also removed the crasher fix.
Comment 16 James Robinson 2012-03-15 11:38:25 PDT
Comment on attachment 132080 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/RenderSurfaceChromium.h:66
> +    bool drawOpacityIsAnimating() const { return m_drawOpacityIsAnimating; }

i see code that sets this bit on RenderSurfaces, but I can't find any code that uses this bit on render surfaces. Why do we need to track this on layers and surfaces?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:112
>  template<typename LayerType>
> +bool subtreeShouldBeSkipped(LayerType* layer)

this specialization is only for CCLayerImpls - right? can you make it explicitly on that type or will things explode then?

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:991
> +class CCLayerTreeHostTestDoNotSkipLayersWithAnimatedOpacity : public CCLayerTreeHostTestThreadOnly {

not sure i understand this test - how is it verifying that we aren't skipping the subtree?

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:1008
> +        m_numAnimates++;

what's this used for?
Comment 17 vollick 2012-03-15 18:15:25 PDT
Created attachment 132169 [details]
Patch

(In reply to comment #16)
> (From update of attachment 132080 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132080&action=review
>
> > Source/WebCore/platform/graphics/chromium/RenderSurfaceChromium.h:66
> > +    bool drawOpacityIsAnimating() const { return m_drawOpacityIsAnimating; }
>
> i see code that sets this bit on RenderSurfaces, but I can't find any code that uses this bit on render surfaces. Why do we need to track this on layers and surfaces?
This will be used in an upcoming patch of Dana's. In fact, Dana had sent me the logic for updating drawOpacityIsAnimating from that patch because I needed it for layers.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:112
> >  template<typename LayerType>
> > +bool subtreeShouldBeSkipped(LayerType* layer)
>
> this specialization is only for CCLayerImpls - right? can you make it explicitly on that type or will things explode then?
Changed it to overloads to bake in the two layer types.
>
> > Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:991
> > +class CCLayerTreeHostTestDoNotSkipLayersWithAnimatedOpacity : public CCLayerTreeHostTestThreadOnly {
>
> not sure i understand this test - how is it verifying that we aren't skipping the subtree?
If the subtree is skipped while preparing to draw, the draw opacity will not be updated (it will remain at 1). This is what would have happened without this patch since the LayerChromium's opacity was zero. With this patch, the presence of the animation will make sure the layer is not skipped. I've added a comment to the test.
>
> > Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:1008
> > +        m_numAnimates++;
>
> what's this used for?
D'oh. It's not used for anything. Removed.
Comment 18 James Robinson 2012-03-15 18:24:27 PDT
Comment on attachment 132169 [details]
Patch

R=me
Comment 19 WebKit Review Bot 2012-03-16 03:29:51 PDT
Comment on attachment 132169 [details]
Patch

Clearing flags on attachment: 132169

Committed r110980: <http://trac.webkit.org/changeset/110980>
Comment 20 WebKit Review Bot 2012-03-16 03:29:56 PDT
All reviewed patches have been landed.  Closing bug.