Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
251183006:
Web Animations: Timeline should not advance during task execution (Closed)
|
Created:
6 years, 7 months ago by dstockwell Modified:
6 years, 7 months ago CC:
blink-reviews, rjwright, alancutter (OOO until 2018), Mike Lawther (Google), Timothy Loh, darktears, Steve Block, dino_apple.com, Eric Willigers Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionWeb Animations: Timeline should not advance during task execution
Previously the animation clock was only frozen during animation frame
callbacks. This meant that the timeline was able to advance during other
tasks.
This patch allows the clock to advance once per task, either to the
compositor supplied frame start time or to an approximation of the next
expected frame time.
http://dev.w3.org/fxtf/web-animations/#script-execution-and-live-updates-to-the-model
BUG=367903
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173583
Patch Set 1 : #Patch Set 2 : Fix test. #Patch Set 3 : Use task start time #
Total comments: 11
Patch Set 4 : Tick animation clock using approximate frame time. #
Total comments: 10
Patch Set 5 : Address review comments. #Patch Set 6 : Deflake element-animate-position-crash. #Messages
Total messages: 38 (0 generated)
lgtm
Putting yourself in the event loop might mean you get pushed many frames out, I don't think that's correct.
On 2014/04/29 23:04:44, esprehn wrote: > Putting yourself in the event loop might mean you get pushed many frames out, I > don't think that's correct. That's not necessarily a problem. We will still update the time on each new frame. Is there a better way to execute logic on script exit?
On 2014/04/29 23:09:20, dstockwell wrote: > On 2014/04/29 23:04:44, esprehn wrote: > > Putting yourself in the event loop might mean you get pushed many frames out, > I > > don't think that's correct. > > That's not necessarily a problem. We will still update the time on each new > frame. > > Is there a better way to execute logic on script exit? Why are you treating script special here? Script is just like C++, it shouldn't have any special behavior. What part of the spec are you trying to implement?
On 2014/04/29 23:29:36, esprehn wrote: > On 2014/04/29 23:09:20, dstockwell wrote: > > On 2014/04/29 23:04:44, esprehn wrote: > > > Putting yourself in the event loop might mean you get pushed many frames > out, > > I > > > don't think that's correct. > > > > That's not necessarily a problem. We will still update the time on each new > > frame. > > > > Is there a better way to execute logic on script exit? > > Why are you treating script special here? Script is just like C++, it shouldn't > have any special behavior. What part of the spec are you trying to implement? I'm not sure that we are. In C++ there are hooks where we can freeze and unfreeze the clock within a frame. I'm looking for the equivalent hooks when running script callbacks. The timer is one solution. http://dev.w3.org/fxtf/web-animations/#script-execution-and-live-updates-to-t... "The value returned by the currentTime attribute of a document timeline will not change within a script block."
On 2014/04/29 23:49:15, dstockwell wrote: > On 2014/04/29 23:29:36, esprehn wrote: > > On 2014/04/29 23:09:20, dstockwell wrote: > > > On 2014/04/29 23:04:44, esprehn wrote: > > > > Putting yourself in the event loop might mean you get pushed many frames > > out, > > > I > > > > don't think that's correct. > > > > > > That's not necessarily a problem. We will still update the time on each new > > > frame. > > > > > > Is there a better way to execute logic on script exit? > > > > Why are you treating script special here? Script is just like C++, it > shouldn't > > have any special behavior. What part of the spec are you trying to implement? > > I'm not sure that we are. In C++ there are hooks where we can freeze and > unfreeze the clock within a frame. I'm looking for the equivalent hooks when > running script callbacks. The timer is one solution. > > http://dev.w3.org/fxtf/web-animations/#script-execution-and-live-updates-to-t... > "The value returned by the currentTime attribute of a document timeline will not > change within a script block." That part of the spec is not implementable. The spec should change to decide clear points when the time is frozen and unfrozen. Per that line though we should freeze the timer in V8RecusionScope and unfreeze it when exiting V8. That means each RAF callback would have a different time though. The spec is wrong.
On 2014/04/30 00:05:07, esprehn wrote: > On 2014/04/29 23:49:15, dstockwell wrote: > > On 2014/04/29 23:29:36, esprehn wrote: > > > On 2014/04/29 23:09:20, dstockwell wrote: > > > > On 2014/04/29 23:04:44, esprehn wrote: > > > > > Putting yourself in the event loop might mean you get pushed many frames > > > out, > > > > I > > > > > don't think that's correct. > > > > > > > > That's not necessarily a problem. We will still update the time on each > new > > > > frame. > > > > > > > > Is there a better way to execute logic on script exit? > > > > > > Why are you treating script special here? Script is just like C++, it > > shouldn't > > > have any special behavior. What part of the spec are you trying to > implement? > > > > I'm not sure that we are. In C++ there are hooks where we can freeze and > > unfreeze the clock within a frame. I'm looking for the equivalent hooks when > > running script callbacks. The timer is one solution. > > > > > http://dev.w3.org/fxtf/web-animations/#script-execution-and-live-updates-to-t... > > "The value returned by the currentTime attribute of a document timeline will > not > > change within a script block." > > That part of the spec is not implementable. The spec should change to decide > clear points when the time is frozen and unfrozen. Why not? I have an implementation in this patch. > Per that line though we should freeze the timer in V8RecusionScope and unfreeze > it when exiting V8. That means each RAF callback would have a different time > though. Yes, if the spec was phrased as you suggest it would be wrong. Instead, the requirement is that the time does not change within a script block. It sounds like what you're saying is that the spec is missing the additional requirement that the time does not change across all the script blocks for RAF callbacks within the same frame? > The spec is wrong.
On 2014/04/30 00:15:00, dstockwell wrote: > ... > > > http://dev.w3.org/fxtf/web-animations/#script-execution-and-live-updates-to-t... > > > "The value returned by the currentTime attribute of a document timeline will > > not > > > change within a script block." > > > > That part of the spec is not implementable. The spec should change to decide > > clear points when the time is frozen and unfrozen. > > Why not? I have an implementation in this patch. This is non-deterministic. You can't just post a task, it could happen at any point or never. Indeed we could run your task between each raf callback, or even after raf but before layout when putting up a frame. > > > Per that line though we should freeze the timer in V8RecusionScope and > unfreeze > > it when exiting V8. That means each RAF callback would have a different time > > though. > > Yes, if the spec was phrased as you suggest it would be wrong. Instead, the > requirement is that the time does not change within a script block. What is a "script block"? We should not treat script special here. Script and C++ are the same thing. > > It sounds like what you're saying is that the spec is missing the additional > requirement that the time does not change across all the script blocks for RAF > callbacks within the same frame? > No, I'm saying that the spec text is not implementable and this patch is non-deterministic. There's no concept of a "script block". Please fix the spec. :)
On 2014/04/30 00:23:58, esprehn wrote: > On 2014/04/30 00:15:00, dstockwell wrote: > > ... > > > > > > http://dev.w3.org/fxtf/web-animations/#script-execution-and-live-updates-to-t... > > > > "The value returned by the currentTime attribute of a document timeline > will > > > not > > > > change within a script block." > > > > > > That part of the spec is not implementable. The spec should change to decide > > > clear points when the time is frozen and unfrozen. > > > > Why not? I have an implementation in this patch. > > This is non-deterministic. You can't just post a task, it could happen at any > point or never. Indeed we could run your task between each raf callback, or even > after raf but before layout when putting up a frame. > > > > > > Per that line though we should freeze the timer in V8RecusionScope and > > unfreeze > > > it when exiting V8. That means each RAF callback would have a different time > > > though. > > > > Yes, if the spec was phrased as you suggest it would be wrong. Instead, the > > requirement is that the time does not change within a script block. > > What is a "script block"? We should not treat script special here. Script and > C++ are the same thing. > > > > > It sounds like what you're saying is that the spec is missing the additional > > requirement that the time does not change across all the script blocks for RAF > > callbacks within the same frame? > > > > No, I'm saying that the spec text is not implementable and this patch is > non-deterministic. There's no concept of a "script block". Blocks are defined in ECMAScript. > Please fix the spec. :) I'm not really sure how, or exactly what you're requesting. Here are our problems: (1) We don't want animations to change in a non-deterministic manner (or even in a manner independent of script action) while JavaScript is operating on them. This is a firm requirement - operating on a changing data structure is Really Hard and we don't want to expose our users to this. Timer ticks are a (the?) source of non-deterministic, non-user-initiated change in JavaScript. We can't have the timer tick while JavaScript is executing. On the other hand, we want the timer to be pretty up-to-date on entry into script. (2) Just like rAF currently does, we'd like to present a strong view of the current frame to users. Hence, within a set of rAF callbacks executing for the same frame, we don't want the timer to change. So my questions: (A) what's an Elliott-approved approach to defining this in a specification? (B) if this is flat out impossible, what are you suggesting as an alternative?
On 2014/04/30 00:34:49, shans wrote: > [...] > > I'm not really sure how, or exactly what you're requesting. > > Here are our problems: > > (1) We don't want animations to change in a non-deterministic manner (or even in > a manner independent of script action) while JavaScript is operating on them. There is no script. C++ and script are the same thing. :) > This is a firm requirement - operating on a changing data structure is Really > Hard and we don't want to expose our users to this. > > Timer ticks are a (the?) source of non-deterministic, non-user-initiated change > in JavaScript. We can't have the timer tick while JavaScript is executing. On > the other hand, we want the timer to be pretty up-to-date on entry into script. There's no "entry into script", and JS shouldn't have magical animation freezing powers. > > (2) Just like rAF currently does, we'd like to present a strong view of the > current frame to users. Hence, within a set of rAF callbacks executing for the > same frame, we don't want the timer to change. > > So my questions: > > (A) what's an Elliott-approved approach to defining this in a specification? > (B) if this is flat out impossible, what are you suggesting as an alternative? You need to spec this in terms of micro task, or the DOM task sources, or something unrelated to script. There's no concept of script, the whole platform is script. I'd suggest specing that the timer freezes at a deterministic point (not first access), and that it unfreezes at another deterministic point. Maybe you want to unfreeze at end of microtask? I'd look at Microtask::enqueueMicrotask, but you also want to freeze the time deterministically. Otherwise the the frozen time will change based on when any API first touches the currentTime property. For example you might look at: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... and make it freeze at the start of every task, and unfreeze at the end.
On 2014/04/30 00:23:58, esprehn wrote: > On 2014/04/30 00:15:00, dstockwell wrote: > > ... > > > > > > http://dev.w3.org/fxtf/web-animations/#script-execution-and-live-updates-to-t... > > > > "The value returned by the currentTime attribute of a document timeline > will > > > not > > > > change within a script block." > > > > > > That part of the spec is not implementable. The spec should change to decide > > > clear points when the time is frozen and unfrozen. > > > > Why not? I have an implementation in this patch. > > This is non-deterministic. You can't just post a task, it could happen at any > point or never. Coming back to this. I don't understand the problem. How is this non-determinism ever visible? We're already dealing with real time. If the task never happens, then we're never going to execute any more calls against our API. > Indeed we could run your task between each raf callback, or even > after raf but before layout when putting up a frame. Are you suggesting that this could happen now, or that a change to blink or cc might cause this to start happening?
On 2014/04/30 00:44:00, esprehn wrote: > On 2014/04/30 00:34:49, shans wrote: > > [...] > > > > I'm not really sure how, or exactly what you're requesting. > > > > Here are our problems: > > > > (1) We don't want animations to change in a non-deterministic manner (or even > in > > a manner independent of script action) while JavaScript is operating on them. > > There is no script. C++ and script are the same thing. :) > > > This is a firm requirement - operating on a changing data structure is Really > > Hard and we don't want to expose our users to this. > > > > Timer ticks are a (the?) source of non-deterministic, non-user-initiated > change > > in JavaScript. We can't have the timer tick while JavaScript is executing. On > > the other hand, we want the timer to be pretty up-to-date on entry into > script. > > There's no "entry into script", and JS shouldn't have magical animation freezing > powers. > > > > > (2) Just like rAF currently does, we'd like to present a strong view of the > > current frame to users. Hence, within a set of rAF callbacks executing for the > > same frame, we don't want the timer to change. > > > > So my questions: > > > > (A) what's an Elliott-approved approach to defining this in a specification? > > (B) if this is flat out impossible, what are you suggesting as an alternative? > > You need to spec this in terms of micro task, or the DOM task sources, or > something unrelated to script. There's no concept of script, the whole platform > is script. > > I'd suggest specing that the timer freezes at a deterministic point (not first > access), and that it unfreezes at another deterministic point. Maybe you want to > unfreeze at end of microtask? > > I'd look at Microtask::enqueueMicrotask, but you also want to freeze the time > deterministically. Otherwise the the frozen time will change based on when any > API first touches the currentTime property. > > For example you might look at: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > and make it freeze at the start of every task, and unfreeze at the end. I'm going to try this. I think the spec could say something like this: The value returned by the currentTime attribute of a document timeline will be: -- If executing an animation frame request callback ---- a value consistent with the callback's context's time -- Otherwise ---- a value consistent with the result of invoking the now method of the Performance interface when the current task began execution in the event loop processing model
PTAL, I've moved to the approach where the timeline can advance to the time at which the current task started. This makes things much simpler and removes the need for freezing entirely. https://codereview.chromium.org/251183006/diff/60001/LayoutTests/web-animatio... File LayoutTests/web-animations-api/timeline-time.html (right): https://codereview.chromium.org/251183006/diff/60001/LayoutTests/web-animatio... LayoutTests/web-animations-api/timeline-time.html:29: var firstRafTime; I'm going to add a test to check the consistency between timeline time and and the time value passed to the RAF callbacks. https://codereview.chromium.org/251183006/diff/60001/Source/web/WebKit.cpp File Source/web/WebKit.cpp (right): https://codereview.chromium.org/251183006/diff/60001/Source/web/WebKit.cpp#ne... Source/web/WebKit.cpp:68: class EndOfTaskRunner : public WebThread::TaskObserver { We probably want to rename this, or create a separate observer. Suggestions?
lgtm way cleaner than the last approach :) https://codereview.chromium.org/251183006/diff/60001/Source/core/animation/An... File Source/core/animation/AnimationClock.h (right): https://codereview.chromium.org/251183006/diff/60001/Source/core/animation/An... Source/core/animation/AnimationClock.h:49: static double& taskTime() I think this needs a comment indicating its purpose https://codereview.chromium.org/251183006/diff/60001/Source/core/animation/An... File Source/core/animation/AnimationClockTest.cpp (right): https://codereview.chromium.org/251183006/diff/60001/Source/core/animation/An... Source/core/animation/AnimationClockTest.cpp:56: EXPECT_EQ(100, animationClock.currentTime()); maybe insert an artificial pause between these two?
+abarth for Source/web
https://codereview.chromium.org/251183006/diff/60001/Source/web/WebKit.cpp File Source/web/WebKit.cpp (right): https://codereview.chromium.org/251183006/diff/60001/Source/web/WebKit.cpp#ne... Source/web/WebKit.cpp:73: } Doesn't this make the entire engine slower? We execute many, many tasks...
https://codereview.chromium.org/251183006/diff/60001/Source/core/animation/An... File Source/core/animation/AnimationClock.h (right): https://codereview.chromium.org/251183006/diff/60001/Source/core/animation/An... Source/core/animation/AnimationClock.h:49: static double& taskTime() Can we use a regular getter/setter pair instead of returning the number by reference? https://codereview.chromium.org/251183006/diff/60001/Source/web/WebKit.cpp File Source/web/WebKit.cpp (right): https://codereview.chromium.org/251183006/diff/60001/Source/web/WebKit.cpp#ne... Source/web/WebKit.cpp:72: WebCore::AnimationClock::notifyTaskStartTime(monotonicallyIncreasingTime()); How expensive is monotonicallyIncreasingTime? Can we freeze the animation clock at frame boundaries and unfreeze always at end of microtask only if it's already frozen?
https://codereview.chromium.org/251183006/diff/60001/Source/core/animation/An... File Source/core/animation/AnimationClock.h (right): https://codereview.chromium.org/251183006/diff/60001/Source/core/animation/An... Source/core/animation/AnimationClock.h:49: static double& taskTime() On 2014/04/30 22:43:50, esprehn wrote: > Can we use a regular getter/setter pair instead of returning the number by > reference? It's a static local, I can't access it from two different methods. It doesn't need to be local if there's a good place to initialize it once. Is there? https://codereview.chromium.org/251183006/diff/60001/Source/web/WebKit.cpp File Source/web/WebKit.cpp (right): https://codereview.chromium.org/251183006/diff/60001/Source/web/WebKit.cpp#ne... Source/web/WebKit.cpp:72: WebCore::AnimationClock::notifyTaskStartTime(monotonicallyIncreasingTime()); On 2014/04/30 22:43:50, esprehn wrote: > How expensive is monotonicallyIncreasingTime? I will investigate. > Can we freeze the animation clock > at frame boundaries and unfreeze always at end of microtask only if it's already > frozen? I don't think this helps. For any task which is not a frame task we still need an up to date time value. A significant amount of time might pass before the next frame.
https://codereview.chromium.org/251183006/diff/60001/Source/core/animation/An... File Source/core/animation/AnimationClock.h (right): https://codereview.chromium.org/251183006/diff/60001/Source/core/animation/An... Source/core/animation/AnimationClock.h:49: static double& taskTime() On 2014/04/30 22:59:51, dstockwell wrote: > On 2014/04/30 22:43:50, esprehn wrote: > > Can we use a regular getter/setter pair instead of returning the number by > > reference? > > It's a static local, I can't access it from two different methods. It doesn't > need to be local if there's a good place to initialize it once. Is there? Don't use a static local, make it a static member of the AnimationClock and initialize it in the cpp file. Then put the getter and setter here.
https://codereview.chromium.org/251183006/diff/60001/Source/web/WebKit.cpp File Source/web/WebKit.cpp (right): https://codereview.chromium.org/251183006/diff/60001/Source/web/WebKit.cpp#ne... Source/web/WebKit.cpp:73: } On 2014/04/30 22:39:16, abarth wrote: > Doesn't this make the entire engine slower? We execute many, many tasks... I'm going to try a new approach which should reduce this to an increment before the task, or require a micro task.
PTAL, we now only need to do an increment at the start of each task. The clock uses an approximation of the next frame time which can be improved with future work.
lgtm, amazing. https://codereview.chromium.org/251183006/diff/100001/Source/core/animation/A... File Source/core/animation/AnimationClock.cpp (right): https://codereview.chromium.org/251183006/diff/100001/Source/core/animation/A... Source/core/animation/AnimationClock.cpp:60: if (m_time < currentTime) { How can the currentTime ever be less than m_time? https://codereview.chromium.org/251183006/diff/100001/Source/core/animation/A... File Source/core/animation/AnimationClock.h (right): https://codereview.chromium.org/251183006/diff/100001/Source/core/animation/A... Source/core/animation/AnimationClock.h:39: class AnimationClock { This should be non copyable since we return a reference to an AnimationClock& from the document getter. https://codereview.chromium.org/251183006/diff/100001/Source/core/animation/A... Source/core/animation/AnimationClock.h:41: AnimationClock(WTF::TimeFunction monotonicallyIncreasingTime = WTF::monotonicallyIncreasingTime) missing explicit https://codereview.chromium.org/251183006/diff/100001/Source/core/dom/Documen... File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/251183006/diff/100001/Source/core/dom/Documen... Source/core/dom/Document.cpp:478: , m_animationClock() remove this line. https://codereview.chromium.org/251183006/diff/100001/Source/web/WebKit.cpp File Source/web/WebKit.cpp (right): https://codereview.chromium.org/251183006/diff/100001/Source/web/WebKit.cpp#n... Source/web/WebKit.cpp:70: virtual void willProcessTask() Can you add OVERRIDE to both of these?
https://codereview.chromium.org/251183006/diff/100001/Source/core/animation/A... File Source/core/animation/AnimationClock.cpp (right): https://codereview.chromium.org/251183006/diff/100001/Source/core/animation/A... Source/core/animation/AnimationClock.cpp:60: if (m_time < currentTime) { On 2014/05/07 06:07:06, esprehn wrote: > How can the currentTime ever be less than m_time? Because we're advancing to the *next* frame time. If task-1 advances, then task-2 immediately queries, we will reach this point but the next frame time will still be in the future.
https://codereview.chromium.org/251183006/diff/100001/Source/core/animation/A... File Source/core/animation/AnimationClock.h (right): https://codereview.chromium.org/251183006/diff/100001/Source/core/animation/A... Source/core/animation/AnimationClock.h:39: class AnimationClock { On 2014/05/07 06:07:06, esprehn wrote: > This should be non copyable since we return a reference to an AnimationClock& > from the document getter. Done. https://codereview.chromium.org/251183006/diff/100001/Source/core/animation/A... Source/core/animation/AnimationClock.h:41: AnimationClock(WTF::TimeFunction monotonicallyIncreasingTime = WTF::monotonicallyIncreasingTime) On 2014/05/07 06:07:06, esprehn wrote: > missing explicit Done. https://codereview.chromium.org/251183006/diff/100001/Source/core/dom/Documen... File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/251183006/diff/100001/Source/core/dom/Documen... Source/core/dom/Document.cpp:478: , m_animationClock() On 2014/05/07 06:07:06, esprehn wrote: > remove this line. Done. https://codereview.chromium.org/251183006/diff/100001/Source/web/WebKit.cpp File Source/web/WebKit.cpp (right): https://codereview.chromium.org/251183006/diff/100001/Source/web/WebKit.cpp#n... Source/web/WebKit.cpp:70: virtual void willProcessTask() On 2014/05/07 06:07:06, esprehn wrote: > Can you add OVERRIDE to both of these? Done.
LGTM Thanks for iterating on the approach.
lgtm Looks really nice!
The CQ bit was checked by dstockwell@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dstockwell@chromium.org/251183006/120001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: linux_blink_rel on tryserver.blink
FYI, CQ is re-trying this CL (attempt #2). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: linux_blink_rel on tryserver.blink
The CQ bit was checked by dstockwell@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dstockwell@chromium.org/251183006/140001
Message was sent while issue was closed.
Change committed as 173583
Message was sent while issue was closed.
Looks like this hits an assert on android debug running ImageResourceTest.MultipartImage, which is in turn blocking the blink roll. I'm going to revert for now. I expect there's a bug here with SVG in image. http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...
Message was sent while issue was closed.
Revert: https://codereview.chromium.org/272543005/
Message was sent while issue was closed.
Looks like it also caused a bunch of crashes on Win Debug: https://code.google.com/p/chromium/issues/detail?id=371595. |
