Bug 51952

Created attachment 78040 [details] patch, build systems not yet updated

Here's an initial patch. I still need to add AnimationTimeController.h/cpp to the build system files for all ports and wire up the appropriate clearCurrentAnimation() calls.

Created attachment 78058 [details] Patch

This patch adds the new files to all build systems (I hope) and hooks up the clear...() call for the Mac port. Please double check my edits in WebKit/mac - they appear to work as I expect based off of quick checks, but I'm not very familiar with that part of the code and may be doing things in a suboptimal way or missing a case. clearCurrentAnimationTime() is a bit subtle. The idea is that once script queries window.webkitAnimationTime, that timestamp should be preserved through the next paint so that all imperative and declarative animations can be updated and rendered with the same time parameter. WebCore does not control the painting/compositing logic so the embedder has to notify WebCore when it has produced a frame and the animation time can be forgotten. I've wired this up for Chromium and Mac, but not the other ports. The trickiness is that it's possible that after .webkitAnimationTime is queried an arbitrary amount of time can pass before the next frame is produced, for example if the page is in a background tab or a minimized window. It doesn't make sense to use an arbitrarily old animation time in this case, so the AnimationTimeController will reset the time itself after some timeout. Gecko's behavior for this in background tabs is to paint a 1Hz while the page is hidden and allow animations to go out of sync by up to one second, which I think is terrible and don't intend to copy. The timer value is set at 15ms right now so that animations can still proceed smoothly in the ports that don't currently call clearCurrentAnimationTime(). The danger of having the timeout this low is that it is more likely to fire in between the script running and the paint occuring even if the page is in the foreground which could result in declarative and imperative animations getting slightly out of sync. I think this is something we can tweak over time.

Comment on attachment 78058 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78058&action=review r=me, but think about the WebHTMLView changes. > WebKit/mac/WebView/WebHTMLView.mm:3512 > + Frame* frame = [webView _mainCoreFrame]; > + if (frame && frame->page()) > + frame->page()->animationTime()->clearCurrentAnimationTime(); > + Is it correct to call clearCurrentAnimationTime() for each subframe? You may also want to think about whether it's appropriate to call this for printing, and when views are being snapshotted. You should make this work in WebKit2 as well.

(In reply to comment #5) > (From update of attachment 78058 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=78058&action=review > > r=me, but think about the WebHTMLView changes. > > > WebKit/mac/WebView/WebHTMLView.mm:3512 > > + Frame* frame = [webView _mainCoreFrame]; > > + if (frame && frame->page()) > > + frame->page()->animationTime()->clearCurrentAnimationTime(); > > + > > Is it correct to call clearCurrentAnimationTime() for each subframe? I'm not quite sure what you mean. The AnimationTimeController is per-page, so the intent here is to call clearCurrentAnimationTime() on the page associated with this WebHTMLView. If there are multiple Frames, is there a WebHTMLView associated with each FrameView (and thus a drawRect call for each?). If so, this should be fine (if a bit redundant). > > You may also want to think about whether it's appropriate to call this for printing, and when views are being snapshotted. > > You should make this work in WebKit2 as well. Sure thing. I believe it should be sufficient to call clearCurrentAnimationTime() after the paint in ChunkedUpdateDrawingArea::paintIntoUpdateChunk() for non-composited pages and after syncCompositingLayers() in LayerBackedDrawingArea::updateLayoutRunLoopObserverFired() for composited pages. Does that seem appropriate? I'm not sure how to run layout tests using WebKit2 but I've tried a few manually in MiniBrowser and they seem all right.

Created attachment 78081 [details] webkit2 support

Comment on attachment 78081 [details] webkit2 support (In reply to comment #6) > (In reply to comment #5) > > (From update of attachment 78058 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=78058&action=review > > > > r=me, but think about the WebHTMLView changes. > > > > > WebKit/mac/WebView/WebHTMLView.mm:3512 > > > + Frame* frame = [webView _mainCoreFrame]; > > > + if (frame && frame->page()) > > > + frame->page()->animationTime()->clearCurrentAnimationTime(); > > > + > > > > Is it correct to call clearCurrentAnimationTime() for each subframe? > > I'm not quite sure what you mean. The AnimationTimeController is per-page, so the intent here is to call clearCurrentAnimationTime() on the page associated with this WebHTMLView. If there are multiple Frames, is there a WebHTMLView associated with each FrameView (and thus a drawRect call for each?) In WebKit1, this is true. This is probably OK since scripts and animations won't run between painting different frames, but you may get a paint notification for a subframe when the main frame has nothing to paint. > > You may also want to think about whether it's appropriate to call this for printing, and when views are being snapshotted. > > > > You should make this work in WebKit2 as well. > > Sure thing. I believe it should be sufficient to call clearCurrentAnimationTime() after the paint in ChunkedUpdateDrawingArea::paintIntoUpdateChunk() for non-composited pages I think ChunkedUpdateDrawingArea::display() would be better. Later optimizations may result in paintIntoUpdateChunk() being called more than once. > and after syncCompositingLayers() in LayerBackedDrawingArea::updateLayoutRunLoopObserverFired() for composited pages. I think syncCompositingLayers() is a better place.

(In reply to comment #8) > In WebKit1, this is true. This is probably OK since scripts and animations won't run between painting different frames, but you may get a paint notification for a subframe when the main frame has nothing to paint. OK, that sounds find. I'll leave bit alone, then. > > > > You may also want to think about whether it's appropriate to call this for printing, and when views are being snapshotted. > > > > > > You should make this work in WebKit2 as well. > > > > Sure thing. I believe it should be sufficient to call clearCurrentAnimationTime() after the paint in ChunkedUpdateDrawingArea::paintIntoUpdateChunk() for non-composited pages > > I think ChunkedUpdateDrawingArea::display() would be better. Later optimizations may result in paintIntoUpdateChunk() being called more than once. OK. That means I'll also need to add the call to ::setSize() as that directly calls paintIntoUpdateChunk(). > > > and after syncCompositingLayers() in LayerBackedDrawingArea::updateLayoutRunLoopObserverFired() for composited pages. > > I think syncCompositingLayers() is a better place. Is that always called _after_ the main content has been painted? I can't tell quite what LayerBackedDrawingArea::m_syncTimer is supposed to do (and it's commented out currently).

Created attachment 78085 [details] webkit2 calls moved

Comment on attachment 78085 [details] webkit2 calls moved Thanks for the review. I'll have to land this by hand to coordinate with some chromium-side changes.

Committed r75169: <http://trac.webkit.org/changeset/75169>

http://trac.webkit.org/changeset/75169 might have broken GTK Linux 32-bit Release

Simon pointed out in chat that this approach doesn't work as well with hardware-driven animations as it could. Vangelis and I discussed a different approach, I'll update this bug later tonight with details.

For reference: https://developer.mozilla.org/en/DOM/window.mozAnimationStartTime and the closely related https://developer.mozilla.org/en/DOM/window.mozRequestAnimationFrame. After chatting with Simon and Vangelis today, I think the approach made in my patch isn't quite ideal. There are a few potentially useful properties of animations but it seems they are not all satisfiable at the same time: a) Multiple animations started in the same block of script execution should be in sync b) Software-driven and hardware-driven animations started at the same time (as far as script is concerned) should stay in sync (although the software-driven animation may be choppier) c) The exact start times for animations should be available to script so that it can sync imperative animations with declarative animations d) The user should be able to see the entire animation I think (c) and (d) are in slight conflict. Consider the following pseudo-JS: -- start -- function startAnimation() { element1.style.animatedProperty = newValue; complexCalculationThatTakes100ms(); element2.style.animatedProperty = newValue; } function updateAnimation() { element3.style.nonAnimatedProperty = calculateValueToMatchDeclarativeAnimation(currentTime); } -- end -- To satisfy properties (a) and (b), it's important that element1 and element2's animations have the same start time. I believe this is currently the case for CoreAnimation-driven animations but not for other types (without this patch). Additionally for the script to be able to synchronize imperative animations with element1 and element2's animations, it needs to know the start time of the animations. For the script to sync up element3 perfectly, it also needs to know what time parameter will be used for the declarative animations for the paint following the updateAnimation() call. Mozilla's proposal (which http://trac.webkit.org/changeset/75169 roughly implemented) solves this by adding a new property mozAnimationStartTime. The startAnimation() function can read this value to know the when the element1 and element2 animations start. During the updateAnimation() function the value of mozAnimationStartTime is the value used for the animation updates in the subsequent paint. The downside of this approach is that if startAnimation() takes a long time to execute (as it does in this case) the next frame isn't painted for some time after the time reported by mozAnimationStartTime/webkitAnimationTime. Additionally there isn't a great way to calculate the value of mozAnimationStartTime for updateAnimation() if that function takes a long time to complete. In both cases we have to end up either fibbing to the script or cutting off the start of the animation. This doesn't seem so good. Therefore, instead of exposing a time to script I think it makes more sense to store an internal timestamp for synchronizing animations but only expose it to script via transitionstart events. We can start animations after script finishes executing to ensure that the user sees the start of the animation no matter how long the script takes to execute, and also to sync the start time with CoreAnimation or other compositing systems. I don't think there is any way to provide a better value to updateAnimations() than the current time, and that's probably OK. This should end up satisfying (a), (b), and (d).

I think I came to the same conclusion last night. Doesn't this imply that you're only implementing something like window.mozRequestAnimationFrame(), and not mozAnimationStartTime?

(In reply to comment #16) > I think I came to the same conclusion last night. Doesn't this imply that you're only implementing something like window.mozRequestAnimationFrame(), and not mozAnimationStartTime? Yes, and additionally not providing a timestamp to the mozRequestAnimationFrame() equivalent. Should make things easier to spec at least!

(In reply to comment #17) > (In reply to comment #16) > > I think I came to the same conclusion last night. Doesn't this imply that you're only implementing something like window.mozRequestAnimationFrame(), and not mozAnimationStartTime? > > Yes, and additionally not providing a timestamp to the mozRequestAnimationFrame() equivalent. Should make things easier to spec at least! Not providing a timestamp to the callback?

(In reply to comment #15) > For reference: https://developer.mozilla.org/en/DOM/window.mozAnimationStartTime and the closely related https://developer.mozilla.org/en/DOM/window.mozRequestAnimationFrame. After chatting with Simon and Vangelis today, I think the approach made in my patch isn't quite ideal. > > There are a few potentially useful properties of animations but it seems they are not all satisfiable at the same time: > > a) Multiple animations started in the same block of script execution should be in sync > b) Software-driven and hardware-driven animations started at the same time (as far as script is concerned) should stay in sync (although the software-driven animation may be choppier) > c) The exact start times for animations should be available to script so that it can sync imperative animations with declarative animations > d) The user should be able to see the entire animation I'm not sure what it (d) means. There can be times when the user sees no animation at all. This would happen, for instance, if it is a software animation with a short duration where rendering a frame takes longer than the duration. The animation sync we added to AnimationController simply makes sure that hardware and software animations fired in the same style change pass have the same start time. There are times where this is useful in just keeping 2 software animations in sync as well. In your example below, with all software animations and no sync you'd see one animation 100ms ahead of the other. With sync, they will both have the same start time, which is the time the style change cycle ended. With hardware animations all the software animations are still in sync, but their start time is the the start time of the first hardware animation that fires its notification that it has started. We make an assumption that all hardware animations started in the same frame will have the same start start time. This is true in both our Windows and Mac implementations. > > I think (c) and (d) are in slight conflict. Consider the following pseudo-JS: > > -- start -- > function startAnimation() { > element1.style.animatedProperty = newValue; > complexCalculationThatTakes100ms(); > element2.style.animatedProperty = newValue; > } > function updateAnimation() { > element3.style.nonAnimatedProperty = calculateValueToMatchDeclarativeAnimation(currentTime); > } > -- end -- > > To satisfy properties (a) and (b), it's important that element1 and element2's animations have the same start time. I believe this is currently the case for CoreAnimation-driven animations but not for other types (without this patch). That's not so. In a pure software animation system, all animations will get the same start time. Here's how AnimationController works: 1) Style resolution for all elements starts. beginAnimationUpdate() is called. 2) For each element that has had a style change, AnimationController::updateAnimations is called with the old and new style 3) If a property with a transition has changed or an animation is found that hasn't started and should start, we put it in a "waiting for start time" state. 4) Putting it in this state also adds it to a list of animations that are waiting for a start time. 5) If a notifyAnimationStarted call is made, it means a hardware animation has started and we flush out all the waiters and give them this start time. 6) Style resolution for all elements completes. endAnimationUpdate() is called 7) When endAnimationUpdate() is called, if there are still waiters they are flushed with the current time. > Therefore, instead of exposing a time to script I think it makes more sense to store an internal timestamp for synchronizing animations but only expose it to script via transitionstart events. We can start animations after script finishes executing to ensure that the user sees the start of the animation no matter how long the script takes to execute, and also to sync the start time with CoreAnimation or other compositing systems. I don't think there is any way to provide a better value to updateAnimations() than the current time, and that's probably OK. This should end up satisfying (a), (b), and (d). Are you saying that the only way an author can get a start time is from transitionStart? What if the author doing animations purely with Mozilla calls? Or am I missing something? It seems to me that AnimationController should be the authority for timing. The problem is when beginAnimationUpdate() and endAnimationUpdate() methods are called to bracket the calls. I'm not sure how JS calls and style resolution relate temporally. Do the JS calls all happen between when style resolution starts and when it finishes? Anyway, I think if we can get the begin/end brackets right and always ask AnimationController for the time we should be fine.

(In reply to comment #19) > (In reply to comment #15) > > For reference: https://developer.mozilla.org/en/DOM/window.mozAnimationStartTime and the closely related https://developer.mozilla.org/en/DOM/window.mozRequestAnimationFrame. After chatting with Simon and Vangelis today, I think the approach made in my patch isn't quite ideal. > > > > There are a few potentially useful properties of animations but it seems they are not all satisfiable at the same time: > > > > a) Multiple animations started in the same block of script execution should be in sync > > b) Software-driven and hardware-driven animations started at the same time (as far as script is concerned) should stay in sync (although the software-driven animation may be choppier) > > c) The exact start times for animations should be available to script so that it can sync imperative animations with declarative animations > > d) The user should be able to see the entire animation > > > > I'm not sure what it (d) means. There can be times when the user sees no animation at all. This would happen, for instance, if it is a software animation with a short duration where rendering a frame takes longer than the duration. The animation sync we added to AnimationController simply makes sure that hardware and software animations fired in the same style change pass have the same start time. There are times where this is useful in just keeping 2 software animations in sync as well. In your example below, with all software animations and no sync you'd see one animation 100ms ahead of the other. With sync, they will both have the same start time, which is the time the style change cycle ended. Right. The difficulty is that style changes can be evaluated while script is still running, which leads to odd results. Looking at your comments below, though, I think it's actually relatively easy to rationalize this. > > With hardware animations all the software animations are still in sync, but their start time is the the start time of the first hardware animation that fires its notification that it has started. We make an assumption that all hardware animations started in the same frame will have the same start start time. This is true in both our Windows and Mac implementations. Yeah - that seems necessary. > > > > > > > I think (c) and (d) are in slight conflict. Consider the following pseudo-JS: > > > > -- start -- > > function startAnimation() { > > element1.style.animatedProperty = newValue; > > complexCalculationThatTakes100ms(); > > element2.style.animatedProperty = newValue; > > } > > function updateAnimation() { > > element3.style.nonAnimatedProperty = calculateValueToMatchDeclarativeAnimation(currentTime); > > } > > -- end -- > > > > To satisfy properties (a) and (b), it's important that element1 and element2's animations have the same start time. I believe this is currently the case for CoreAnimation-driven animations but not for other types (without this patch). > > > > That's not so. In a pure software animation system, all animations will get the same start time. Here's how AnimationController works: Not if complexCalculationThatTakes100ms() forces a style recalculation (which is really easy to do from javascript, just touch any property that requires layout to be up to date). This behavior seems like a bug, though. > > 1) Style resolution for all elements starts. beginAnimationUpdate() is called. > 2) For each element that has had a style change, AnimationController::updateAnimations is called with the old and new style > 3) If a property with a transition has changed or an animation is found that hasn't started and should start, we put it in a "waiting for start time" state. > 4) Putting it in this state also adds it to a list of animations that are waiting for a start time. > 5) If a notifyAnimationStarted call is made, it means a hardware animation has started and we flush out all the waiters and give them this start time. > 6) Style resolution for all elements completes. endAnimationUpdate() is called > 7) When endAnimationUpdate() is called, if there are still waiters they are flushed with the current time. > Thanks for the clear overview - I didn't quite understand everything that was going on in AnimationController. I think the reason I'm seeing some out of sync animations is that the "waiting for start time" mechanism does not always wait long enough for software driven animations. In particular, if style resolution happens during script execution then step (7) will flush the start time for software animations potentially prematurely. What we should do is wait for the script to finish executing before resolving the start time for waiters so that all software animations started in the same script run can get the same start time and, if any hardware driven animations are also started in the same run, the software animations can adopt the same start time as hardware driven animations. > > > > Therefore, instead of exposing a time to script I think it makes more sense to store an internal timestamp for synchronizing animations but only expose it to script via transitionstart events. We can start animations after script finishes executing to ensure that the user sees the start of the animation no matter how long the script takes to execute, and also to sync the start time with CoreAnimation or other compositing systems. I don't think there is any way to provide a better value to updateAnimations() than the current time, and that's probably OK. This should end up satisfying (a), (b), and (d). > > > > Are you saying that the only way an author can get a start time is from transitionStart? What if the author doing animations purely with Mozilla calls? Or am I missing something? > > It seems to me that AnimationController should be the authority for timing. The problem is when beginAnimationUpdate() and endAnimationUpdate() methods are called to bracket the calls. I'm not sure how JS calls and style resolution relate temporally. Do the JS calls all happen between when style resolution starts and when it finishes? Anyway, I think if we can get the begin/end brackets right and always ask AnimationController for the time we should be fine. Style resolution can be interleaved with JS calls (and often is). My actual test page looks like this: element1.style.left='600px'; document.body.offsetTop; // force style recalc busyLoopMs(150); element2.style.left='600px'; If instead of resolving start times for software driven animations in the endAnimationUpdate() call we did it via an async mechanism that synchronized with hardware driven animations (if there were any) then it sounds like everything would work out perfectly. I'll see about implementing that.

Committed r75276: <http://trac.webkit.org/changeset/75276>

Patch was reverted, reopening.

Changing the bug title since I think we've decided not to implement mozilla's API directly.

http://trac.webkit.org/changeset/75276 might have broken Qt Windows 32-bit Release, Qt Windows 32-bit Debug, and Chromium Linux Release

Created attachment 78301 [details] Patch

I'm concerned about the current bracketing being done by beginAnimationUpdate()/endAnimationUpdate(). Seems like it would break sync if a midline styleChange (calling e.g., offsetTop to get a styleRecalc in the middle of the frame) is done. Have you tested in the 3 environments (WebKit HW anim, Chrome HW anim, pure SW anim)?

(In reply to comment #26) > I'm concerned about the current bracketing being done by beginAnimationUpdate()/endAnimationUpdate(). Seems like it would break sync if a midline styleChange (calling e.g., offsetTop to get a styleRecalc in the middle of the frame) is done. I'm not quite sure what you mean here - could you describe an example? The path that SW animations take now is essentially the same as what HW animations took before - they remain in the pending start state until a callback fires. > > Have you tested in the 3 environments (WebKit HW anim, Chrome HW anim, pure SW anim)? I've tested pure SW animations and mixes of HW and software animations.

ping?

You should revisit the patch. Dean has made some pretty significant changes in this part of the code. The code and maybe the technique you're using will likely have to change...

Comment on attachment 78301 [details] Patch r- per Chris's comment.

The test (animation-sync) still fails on ToT, so I think some solution is still needed here. I'll try to revive this patch against ToT when I get some time.