We need up-to-date opacity and transform values to do proper culling on the main thread. Copying these values back from the impl thread introduces unacceptable latency. A better solution is to freshen these values on the main thread as necessary.
(In reply to comment #0) > We need up-to-date opacity and transform values to do proper culling on the main thread. Copying these values back from the impl thread introduces unacceptable latency. A better solution is to freshen these values on the main thread as necessary. Lets talk on VC when you get to doing this. I'm not sure I follow.
Created attachment 131961 [details] Patch This will fix the issue mentioned in 80744 as well as providing more reliable values for main thread culling code. Highlights: - CCLayerAnimationController and CCLayerAnimationControllerImpl have been merged (so that the controllers can be ticked on the main thread). - PLEASE NOTE: o CCLayerAnimationController::purgeFinishedAnimations(..) now creates animation finished events (which now hold the final, animated value). o CCLayerAnimationController::pushAnimationUpdatesTo(..) no longer pulls back any information from the impl thread. In particular, we no longer need to keep track of which animations finished on the impl thread. CCActiveAnimation::AnimationSignature was used only for this purpose and has been removed. o Although the animations are ticked on both threads, we only send animation events in response to things that happen on the impl thread. For that reason, passing in a vector of animation events to CCLayerAnimationController::animate is now optional (it takes a pointer that is null when called from the main thread). - CCLayerImpl and LayerChromium both implement CCLayerAnimationControllerClient - thing to note, CCLayerImpl's implementation allows animations to affect the live opacity and transform, but LayerChromium's modify animatedOpacity and animatedTransform so animations do _not_ affect a LayerChromium's opacity and transform (the real values are set when animations complete). - LayerChromium::setAnimationEvent now sets final values in response to animation finished events. - GraphicsLayerChromium does not need to remove animation names from its name-to-id map, and this functionality has been removed (from here and from CCLayerAnimationDelegate).
It seems pretty dodgy to tick animations on the main thread the same way we do on the impl side. On the main thread side, if we have an active animation we have to prepare a frame that is valid for some set of future frames of the animation (since we won't get a tick for each frame that we actually produce). That's a pretty different problem from the one on the impl thread, where we can do work like calculate geometry + do culling after each and every tick.
Let me expand a bit. My goal here is to try to enable the animation system for CSS animations as soon as possible in chrome. I think we're really close to that point and getting that last step would be awesome for everything. I'd like to get to a minimum viable state for enabling by default first and then refine from there. Today, when we have a CSS animation/transition active we update everything on the main thread. This means we get a chance to paint and do culling+viewport calculations for every intermediate frame in the animation and don't run the risk of culling something out that should be visible or clipping something that is inside the viewport (unless those systems have bugs independent of animations). This is great for correctness but kind of sucks performance-wise since we're relying on the style system to update animations and we have to get sufficient time on the main thread on every frame of the animation or we hitch. Once we start processing animations on the impl thread things get a little trickier - we can't necessarily tick on the main thread while the animation is active, but the impl thread will keep on going. This means we might expose some part of a layer that culled or clipped out the last time we ran a commit. If we don't have that content, we flash currently. I don't think this is acceptable for our default behavior, so we have to do something about it before we can turn this on. Keep in mind that lots of people are designing + testing pages with animations in Safari, which doesn't produce these sorts of flashes when content is exposed (since they just raster a whole bunch of stuff). If it flashes in chromium and not in Safari people will (rightly) consider that a bug in chromium no matter how good the framerate is or how resource-slim we are. With that in mind, there are a few ways to approach the problem. One is to simply produce all the content that we might need on the main thread once we know there is an animation active so we can run the rest of the animation on the thread. This would be perfect except that we can't always afford the resources to hold the textures or the time to raster as much content as we'd like. Another option is to raster a best guess on the main thread and then hitch on the impl thread if we hit content that we don't have instead of producing a frame lacking some content. This is A-OK behavior wise, since it's no worse than what we have today (if the main thread blocks, you hitch) but we don't have logic for it AFAIK. Longer term I think we want to make intelligent guesses on the main thread based on what content the animation will expose as it runs. To do this we definitely need to have good knowledge about what the animation is going to do on the main thread before we raster. However, I don't think that this should be a blocker for enabling the threaded animation path by default. Safari isn't really doing much in this area so there's an existence proof that we can hit the use cases that people are designing for today and then refine from there. Long story short I think being able to have more direct access to the animation curves from the main thread is a great thing to have long term but it's not going to let us turn this on. In order to get to a minimum viable state I think we should: 1.) Not produce frames from the impl thread if we're lacking content due to an animation because we culled or clipped part of the layer the last time we were on the main thread. Instead, stall drawing from the impl thread until we get a commit that has new data. 2.) Raster enough content on the main thread when we know that animations are active so we don't hit the hitch introduced by (1) too often. (1) is a purely impl-side patch. (2) can be done in many ways and it's fundamentally a problem of heuristics. For opacity animations I think this is as simple as ignoring the opacity for layers that have active animations on opacity (don't skip it for having opacity==0, don't consider it opaque for culling). For transforms it's harder. Most layers are probably small enough that we can just raster the whole thing (and that's what Safari does up to some fairly large layer size) but it won't work in all cases. Many of the cases are simply sliding layers around or rotating them in place, though. Interpolating to a given place when doing the main thread raster can be helpful as part of (2) but it doesn't address (1) and IMO isn't going to be all we need. I think we'll end up having to look at the state over time in order to make the best choices for rastering on the main thread. Many transform animations can be bounded pretty easily - if the animation is just translating Y from 0 to 50 we can easily calculate the visible and cull areas for the entire animation. Some animations are going to be crazy keyframe things with blending and be a real mess to figure out. I think we'll be better of tackling this problem piece by piece. Let me know if this jibes with your thinking, Nat. As far as what blocks https://bugs.webkit.org/show_bug.cgi?id=79536, I think we definitely need a (1) for that bug and we can have other bugs improve on the situation but I don't think that we necessarily need to do anything in regards to (2) before we flip the switch.
I believe Dana has a patch that will simply stop paint culling an animating layer. Ian posted a patch for always-calcDrawEtc'ing opaque layers. These two things are the real blockers for b79536. I agree that this bug does not strictly block b79536. Lets get those first two done and in the bag. We do need to start messing around with this, as I suspect we need something along these lines to get good performance. On Android w/ Gmail, Eric reported that we have a substantial hitch at animation start because we do a ton of uploads. There are two reasons: 1. The new layer gets put in its final location, so we paint it. 2. The layer moving out gets moved offscreen right away, so as I understand it, we promptly discard its textures even though its still onscreen. I imagine Dana's patch might fix #2 but I assume #1 will still happen. It may be we need more-agressive prepainting --- "oh, this layer is offscreen but we'll paint it anyway becausue we have memory to burn." But, this needs the texture manager to come online fully, which is blocked on resolution of the front/backbuffer stuff. Which is not happening fast. So, I think we need something on the main thread. This bug is probably poorly titled --- I've updated it a bit. We can maybe put this behind a flag while we run the "correct-enough" implementation through a canary/android/etc. Note, we need not just curves. We need blending/scheduling logic. Thats in the animation controller. The main thread probably cares less about the immediate value as much as "the possible value range over the next 0.25 seconds"
(In reply to comment #4) > 1.) Not produce frames from the impl thread if we're lacking content due to an animation because we culled or clipped part of the layer the last time we were on the main thread. Instead, stall drawing from the impl thread until we get a commit that has new data. One note. This should probably 'not tick animations' rather than 'not produce frames' so that scrolling can still be unblocked. (In reply to comment #5) As a means of enabling in a limited fashion: Could we possibly determine if the animating layers are 'small enough' and only enable animations that fit in memory? I suppose even then we would want to cull them at some point (if the bounds of the entire their entire animation curve is offscreen)... Animations that can be completely pre-loaded will perform a lot better, especially if we delay the start time until all loading has completed. So it seems like a distinction between streaming and fully-pre-loaded would be good.
(In reply to comment #6) > (In reply to comment #4) > > 1.) Not produce frames from the impl thread if we're lacking content due to an animation because we culled or clipped part of the layer the last time we were on the main thread. Instead, stall drawing from the impl thread until we get a commit that has new data. > > One note. This should probably 'not tick animations' rather than 'not produce frames' so that scrolling can still be unblocked. If we're lacking content, we can't make any frames due to scrolling either or they will be lacking content. So we can't produce any frames at all.
(In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #4) > > > 1.) Not produce frames from the impl thread if we're lacking content due to an animation because we culled or clipped part of the layer the last time we were on the main thread. Instead, stall drawing from the impl thread until we get a commit that has new data. > > > > One note. This should probably 'not tick animations' rather than 'not produce frames' so that scrolling can still be unblocked. > > If we're lacking content, we can't make any frames due to scrolling either or they will be lacking content. So we can't produce any frames at all. I think there is a distinction between content missing due to scrolling vs content missing due to animations. Even right now, with animations running on the webkit thread, it is easy to get missing animating content due to scrolling (the same way that it is missing for other scrolled content). So allowing scrolling won't be a change from the status quo.
Created attachment 132465 [details] Patch (In reply to comment #4) Freshened patch. > 1.) Not produce frames from the impl thread if we're lacking content due to an animation because we culled or clipped part of the layer the last time we were on the main thread. Instead, stall drawing from the impl thread until we get a commit that has new data. > (1) is a purely impl-side patch. As I understand it, (1) is saying that if we're about to draw invalid textures during an impl thread animation, we should stop and wait for a valid frame from the main thread. This assumes that the main thread can and will produce this frame, but I don't think it can right now. The style system will tell the main thread what the target value of an animation is, but will not continue to supply us with intermediate values. In order for the main thread to create this frame, it will need to know the current animated values. So I think that (1) involves three patches: a) (81437) Don't draw invalid textures while animating (this will cause us to hitch until the end of the animation, currently). b) (This patch) during a commit, get the current animated values. c) (no bug yet) Use the animated values to paint, cull, and commit, if necessary. I think this also means that this patch is a blocker for 79536.
Comment on attachment 132465 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132465&action=review > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:546 > +void LayerChromium::setAnimatedOpacity(float animatedOpacity) Spoke offline with Ian and leaving a comment here on what we thought. It probably makes more sense to just update the "real" values with the animating values, and drop these animatedOpacity/animatedTransform things. This mimics the behaviour of a non-accelerated animation now. And since our goal is to make a correct frame for some time in the animation (when impl needs help), we'd just end up special-casing a lot of code to use these values instead. There's some logic that needs to go in when setting opacity during an animation, to not setNeedsCommit for changes due to animation, but that's reasonable to do IMO. I think it could make sense to save the target transform, so that we can figure out the target visibleLayerRect, for prepainting. But it's not something we need to do in the first version of this.
Created attachment 132495 [details] Patch Update patch so that m_opacity and m_transform are modified directly by animations on the main thread.
Comment on attachment 132495 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132495&action=review some nits for you :) > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:548 > + m_opacity = animatedOpacity; I think these deserve a comment saying why it's not using setOpacity(). > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:63 > + virtual float animatedOpacity() const { return m_opacity; } Is this needed, since both threads set m_opacity now? > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:65 > + virtual const TransformationMatrix& animatedTransform() const { return m_transform; } ditto? > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:215 > // We may have added an animation during the tree sync. This will cause hostImpl to visit its controllers. both sides visit them now i guess? > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:394 > // We may have added an animation during the tree sync. This will cause hostImpl to visit its controllers. ditto?
Comment on attachment 132495 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132495&action=review Seems pretty reasonable. Dana's comments are good, I've added some more in a similar vein. > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:608 > + case CCAnimationEvent::Started: { > + m_layerAnimationDelegate->notifyAnimationStarted(wallClockTime); > + break; we normally don't put braces on case : sections unless we need block scoping for a local var > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:185 > +void CCLayerAnimationController::add(PassOwnPtr<CCActiveAnimation> anim) don't abbreviate parameter/variable names. 'animation' would be fine here > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:410 > + } // switch > + } // if running > + } // for each animation we don't typically comment ends of blocks like this. this function doesn't seem long enough to need these, but if there are functions with enough nesting levels to require comments that's when we would normally break bits out into helper functions > Source/WebKit/chromium/tests/CCLayerAnimationControllerTest.cpp:181 > + EXPECT_EQ(0.5f, dummy.animatedOpacity()); don't think you need the trailing "f"s for this and the other floating point constants in this patch > Source/WebKit/chromium/tests/CCLayerAnimationControllerTest.cpp:201 > + controller->animate(0.5, events.get()); // second anim starts NOW. if it's worth having a comment, it's probably worth expanding out to a sentence with complete words > Source/WebKit/chromium/tests/CCLayerAnimationControllerTest.cpp:228 > + // The the float animation should have started at time 1 and should be done. typo "The the" > Source/WebKit/chromium/tests/CCLayerAnimationControllerTest.cpp:248 > + // Anims with id 1 should both start now. Animations please
Created attachment 132518 [details] Patch I've r?'d again because of some of the renaming I did in response to these reviews. (In reply to comment #12) > (From update of attachment 132495 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=132495&action=review > > some nits for you :) > > > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:548 > > + m_opacity = animatedOpacity; > > I think these deserve a comment saying why it's not using setOpacity(). Good call. Added. > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:63 > > + virtual float animatedOpacity() const { return m_opacity; } > > Is this needed, since both threads set m_opacity now? It definitely doesn't need to be called opacity, but it does need to be part of the interface. I've made opacity() and transform() part of the interface again, and renamed setAnimatedTransform to setTransformFromAnimation (likewise for opacity). At the moment, it looks like these special setters are still required. > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:65 > > + virtual const TransformationMatrix& animatedTransform() const { return m_transform; } > > ditto? Fixed. > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:215 > > // We may have added an animation during the tree sync. This will cause hostImpl to visit its controllers. > > both sides visit them now i guess? Thanks for catching this. I've updated the comments. > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:394 > > // We may have added an animation during the tree sync. This will cause hostImpl to visit its controllers. > > ditto? Fixed. (In reply to comment #12) > (From update of attachment 132495 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=132495&action=review > > some nits for you :) > > > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:548 > > + m_opacity = animatedOpacity; > > I think these deserve a comment saying why it's not using setOpacity(). > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:63 > > + virtual float animatedOpacity() const { return m_opacity; } > > Is this needed, since both threads set m_opacity now? > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:65 > > + virtual const TransformationMatrix& animatedTransform() const { return m_transform; } > > ditto? > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:215 > > // We may have added an animation during the tree sync. This will cause hostImpl to visit its controllers. > > both sides visit them now i guess? > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:394 > > // We may have added an animation during the tree sync. This will cause hostImpl to visit its controllers. > > ditto? (In reply to comment #13) > (From update of attachment 132495 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=132495&action=review > > Seems pretty reasonable. Dana's comments are good, I've added some more in a similar vein. > > > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:608 > > + case CCAnimationEvent::Started: { > > + m_layerAnimationDelegate->notifyAnimationStarted(wallClockTime); > > + break; > > we normally don't put braces on case : sections unless we need block scoping for a local var > Fixed. > > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:185 > > +void CCLayerAnimationController::add(PassOwnPtr<CCActiveAnimation> anim) > > don't abbreviate parameter/variable names. 'animation' would be fine here Done. > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:410 > > + } // switch > > + } // if running > > + } // for each animation > > we don't typically comment ends of blocks like this. this function doesn't seem long enough to need these, but if there are functions with enough nesting levels to require comments that's when we would normally break bits out into helper functions Make sense. Removed. > > > Source/WebKit/chromium/tests/CCLayerAnimationControllerTest.cpp:181 > > + EXPECT_EQ(0.5f, dummy.animatedOpacity()); > > don't think you need the trailing "f"s for this and the other floating point constants in this patch Fixed. > > > Source/WebKit/chromium/tests/CCLayerAnimationControllerTest.cpp:201 > > + controller->animate(0.5, events.get()); // second anim starts NOW. > > if it's worth having a comment, it's probably worth expanding out to a sentence with complete words You're right -- that's an awful comment. Fixed. > > > Source/WebKit/chromium/tests/CCLayerAnimationControllerTest.cpp:228 > > + // The the float animation should have started at time 1 and should be done. > > typo "The the" > Fixed. > > Source/WebKit/chromium/tests/CCLayerAnimationControllerTest.cpp:248 > > + // Anims with id 1 should both start now. > > Animations please Fixed.
Created attachment 132579 [details] Patch Freshening patch
*** Bug 80745 has been marked as a duplicate of this bug. ***
Comment on attachment 132579 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132579&action=review Looking good! A few comments inline. > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:547 > + nit: extra newline > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:30 > +#include "TransformOperations.h" are we using this #include anywhere? > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.h:43 > +class TransformOperations; I don't see this forward declaration being used anywhere - can we remove it? I think it's nice if this class doesn't have to depend on TransformOperations > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:396 > + m_pendingBeginFrameRequest = adoptPtr(new BeginFrameAndCommitState(monotonicallyIncreasingTime())); I think I made this change in a different way in another patch. Unfortunately on the main thread we need to use a currentTime() based clock - stuff like requestAnimationFrame needs it. We plan to change that eventually, but for now you either have to use that clock or add some renormalizing logic before you pass the time into the CCLayerTreeHostClient. > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.h:95 > + BeginFrameAndCommitState(double frameBeginTime) : frameBeginTime(frameBeginTime) { } explicit, prz
Created attachment 132816 [details] Patch (In reply to comment #17) > (From update of attachment 132579 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=132579&action=review > > Looking good! A few comments inline. > > > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:547 > > + > > nit: extra newline > Fixed. > > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:30 > > +#include "TransformOperations.h" > > are we using this #include anywhere? > Nope. Removed. > > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.h:43 > > +class TransformOperations; > > I don't see this forward declaration being used anywhere - can we remove it? I think it's nice if this class doesn't have to depend on TransformOperations > Removed. > > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:396 > > + m_pendingBeginFrameRequest = adoptPtr(new BeginFrameAndCommitState(monotonicallyIncreasingTime())); > > I think I made this change in a different way in another patch. > > Unfortunately on the main thread we need to use a currentTime() based clock - stuff like requestAnimationFrame needs it. We plan to change that eventually, but for now you either have to use that clock or add some renormalizing logic before you pass the time into the CCLayerTreeHostClient. > It also turned out that we get to CCLayerTreeHost::updateAnimations(frameBeginTime) along another code path that passes the currentTime() rather than the monotonicallyIncreasingTime(). I've reverted these changes and I grab the monotonicallyIncreasingTime() in updateAnimations instead. > > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.h:95 > > + BeginFrameAndCommitState(double frameBeginTime) : frameBeginTime(frameBeginTime) { } > > explicit, prz Reverted.
Comment on attachment 132816 [details] Patch Clearing flags on attachment: 132816 Committed r111393: <http://trac.webkit.org/changeset/111393>
All reviewed patches have been landed. Closing bug.