80744 – [chromium] Threaded opacity animation jump to opacity of 0

RESOLVED FIXED Bug 80744

[chromium] Threaded opacity animation jump to opacity of 0

https://bugs.webkit.org/show_bug.cgi?id=80744

Summary [chromium] Threaded opacity animation jump to opacity of 0

Eric Penner

Reported 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

Attachments
Patch (13.49 KB, patch) 2012-03-13 10:28 PDT, vollick	no flags	Details Formatted Diff Diff
Patch (17.17 KB, patch) 2012-03-14 07:40 PDT, vollick	no flags	Details Formatted Diff Diff
Patch (25.41 KB, patch) 2012-03-15 11:13 PDT, vollick	no flags	Details Formatted Diff Diff
Patch (26.24 KB, patch) 2012-03-15 18:15 PDT, vollick	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Eric Penner

Comment 1 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;

vollick

Comment 2 2012-03-13 10:28:21 PDT

Created attachment 131658 [details] Patch

vollick

Comment 3 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.

Dana Jansens

Comment 4 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.

Nat Duca

Comment 5 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?

Dana Jansens

Comment 6 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?

Dana Jansens

Comment 7 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?

James Robinson

Comment 8 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?

James Robinson

Comment 9 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.

Nat Duca

Comment 10 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.

vollick

Comment 11 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.

Nat Duca

Comment 12 2012-03-14 12:40:50 PDT

Lets pull the deletion bugfix out to another patch. The animation changes LGTM.

James Robinson

Comment 13 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.

James Robinson

Comment 14 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

vollick

Comment 15 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.

James Robinson

Comment 16 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?

vollick

Comment 17 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.

James Robinson

Comment 18 2012-03-15 18:24:27 PDT

Comment on attachment 132169 [details] Patch R=me

WebKit Review Bot

Comment 19 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>

WebKit Review Bot

Comment 20 2012-03-16 03:29:56 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Other

OS All

Product WebKit

Component CSS

Assignee

vollick

Reported

2012-03-09 18:47 PST

Modified

2012-03-16 03:29 PDT History

CC List

8 users Show

URL

Keywords

Depends on

Blocks

79536

Dependancies

tree graph