fix(animationFrames): emit the timestamp from the rAF's callback #5438

tmair · 2020-05-15T18:40:11Z

Description:
Emit the timestamp provided by rAF to align better with the rAF
specification. Multiple scheduled rAF callbacks within the same frame
will be called with the same timestamp.

Related issue (if exists):
This fixes #5194

Further notes
I kept the possibility to supply a custom TimestampProvider to the animationFrames function.

cartant

Thanks for opening the PR. There are a couple of things that I think need to be addressed.

cartant · 2020-05-15T23:36:14Z

src/internal/observable/dom/animationFrames.ts

@@ -77,20 +77,23 @@ export interface TimestampProvider {
 *
 * @param timestampProvider An object with a `now` method that provides a numeric timestamp
 */
-export function animationFrames(timestampProvider: TimestampProvider = Date) {
+export function animationFrames(timestampProvider?: TimestampProvider) {
  return timestampProvider === Date ? DEFAULT_ANIMATION_FRAMES : animationFramesFactory(timestampProvider);


The default - DEFAULT_ANIMATION_FRAMES - should be changed to not use Date as the timestamp provider, IMO. The purpose of the default is to avoid creating observable instances when animationFrames is called with the expected/default timestamp provider and the default is to use the timestamp provided by the API.

Yes, I did forget to change the default here.

cartant · 2020-05-15T23:57:18Z

src/internal/observable/dom/animationFrames.ts

+      if (start === null) {
+        start = timestampProvider ? timestampProvider.now() : timestamp;
+      }
+      subscriber.next((timestampProvider ? timestampProvider.now() : timestamp) - start);


If the timestamps provided by the API are being used, IMO they should be passed through without modification.

That is, the start should only be subtracted when using a caller-specified timestamp provider.

AFAICT, the point of using the API-provider timestamp is to synchronize animations and if an elapsed time is calculated from the start time, synchronization would only be possible between animations that started on the same animation frame. This seems like an unnecessary restriction, to me.

@benlesh What is your opinion on this? When should the operator emit an elapsed time: always, never or only if a caller-specified timestamp provider is specified?

I see. So you'd have users calculate the elapsed time themselves?

The problem I see is they would then need to capture the initial time themselves. Maybe we can provide both? { timestamp: number, elapsed: number }

Then again, if they already have the timestamp provider, it's trivial to get the current timestamp within whatever operation you're performing. But less trivial to know the recipe to get the elapsed time, which is more useful in animations.

I think providing both - { timestamp: number, elapsed: number } - would be the best. It'll cover all bases and is essentially self-documenting. Thanks.

cartant

LGTM. Thanks.

benlesh · 2020-05-27T12:31:28Z

I'm not convinced that this is a solid change. With this change, we are back to world where people need to know a recipe in order to get the time that has elapsed since the beginning of the observable. Which is what you usually need when you're doing animations. That, in the face of the fact that it's as simple as calling Date.now() anywhere in your code to get the current timestamp.

I suppose as a compromise we could add an operator provided the elapsed time. But that's just one more thing, right? I'm not sure I like that either.

I'm not saying I'm against this change. I'm just saying that I'm not convinced.

cartant · 2020-05-27T12:43:02Z

With this change, we are back to world where people need to know a recipe in order to get the time that has elapsed since the beginning of the observable.

I'm not sure what's meant be 'recipe' here. If the value emitted from the rAF observable has both the timestamp and elapsed properties, isn't the latter obvious and immediately available in the next callback?

benlesh · 2020-05-29T02:30:11Z

Ah. You know what, I was reviewing this from my phone. I missed that. My mistake. I'll think about it some more.

benlesh

Just some minor changes.

I'm VERY sorry about any confusion earlier with my hasty reviews. I misunderstood the intent.

benlesh · 2020-06-09T18:08:40Z

src/internal/observable/dom/animationFrames.ts

+    const run = (timestamp: DOMHighResTimeStamp | number) => {
+      const currentTimestamp = timestampProvider ? timestampProvider.now() : timestamp;
+      if (start === null) {
+        start = currentTimestamp;


Okay, the only issue I have with this now is that the start time is now non-deterministic. I think collection of start should be moved back to where it was. This is no longer the "start" it's the first animation frame.

Consider the following scenario: A user triggers an animation that is supposed to take 30 seconds, then, before the animation frame fires they switch tabs. If the start isn't collected until the first animationFrame, when the user tabs back, they'll be forced to watch the full animation.

If the start is collected when the observable is subscribed to, when they come back, the animation will be done as expected.

I understand that maybe this change probably arose from the confusion about it being a DOMHighResTimeStamp, and the code was previously using Date as default timestamp provider. Instead, it's safe to use performance as the default TimestampProvider, as it is supported by all browsers and platforms we currently support.

+1 to perf time as the default for animation frames, per https://www.w3.org/TR/hr-time-2/#introduction subtracting 2 ntp times is not safe, it can result in negative durations

benlesh · 2020-06-09T18:10:17Z

@cartant We might want to consider updating TestScheduler.run to temporarily monkey patch known TimestampProviders like Date and Performance. I think we're going to end up breaking people's tests, including maybe our own. I'll open an issue.

benlesh

Actually, I'm sorry again, but given the implications on testing this using a TestScheduler's run method, I'm not sure we can use rAF's values from it's callback. We may just have to use a light wrapper around performance.now() which should be the same thing.

This is because the TestScheduler needs to be able to control what is returned by now() for deterministic testing. Although, I'll admit that doing this for animation frames is a bit of an uncharted territory for the test scheduler, it is currently doing this for the animationFrameScheduler, for better or worse.

benlesh · 2020-06-09T18:18:19Z

We might just have to block on this one, until we iron on #5475 ... Sorry again, @tmair! I'll add this to the next team meeting agenda so we can discuss it.

benlesh · 2020-06-09T18:18:55Z

(Really, really sorry, haha... I promise I'm not intentionally trying to be a PITA on this one)

tmair · 2020-06-18T07:37:26Z

(Really, really sorry, haha... I promise I'm not intentionally trying to be a PITA on this one)

No worries. I totally understand why these things are problematic. I will try to update the PR soon.

cartant · 2020-06-18T07:41:50Z

@tmair I don't think there's anything that really needs to be updated in this PR, ATM. The animationFrame observable itself is blocked because it's not testable in user code. What's being looked into, ATM, is the best way of addressing that issue. Once that's resolved, we can look again at this PR.

joshribakoff · 2020-06-22T00:33:32Z

Keep in mind:

The callback method is passed a single argument, a DOMHighResTimeStamp, which indicates the current time (based on the number of milliseconds since time origin). When callbacks queued by requestAnimationFrame() begin to fire multiple callbacks in a single frame, each receives the same timestamp even though time has passed during the computation of every previous callback's workload. This timestamp is a decimal number, in milliseconds, but with a minimal precision of 1ms (1000 µs).

Sampling the time with performance.now beats using Date.now, but can still cause animations to be out of sync from each other in a give animation frame, because of this part specifically:

each receives the same timestamp even though time has passed during the computation of every previous callback's workload.

which can affect the animation & it make it look not smooth. Unless the time is sampled once per animation frame, which RAF's callback provided DOMHighResTime already handles. If we're sampling a timestampProvider, we should ideally add the same affordance the native RAF callback has.

src/internal/observable/dom/animationFrames.ts

benlesh · 2020-07-26T18:00:47Z

src/internal/observable/dom/animationFrames.ts

+    // queued for a considerable period of time and the elapsed time should
+    // represent the time elapsed since subscription - not the time since the
+    // first rendered animation frame.
+    const start = provider.now();
    const run = (timestamp: DOMHighResTimeStamp | number) => {


Note that this still doesn't really solve the problem that this is untestable in TestScheduler run mode, which automatically patches our schedulers.

I think the solution here is going to be creating a lightweight wrapper around rAF, such that we can override it in run mode. That's what the animationFrameScheduler is supposed to be, but it's inadequate, because it doesn't provide the timestamp, and our schedulers in general are perhaps a bit overkill for this use case.

Something like this should suffice:

export const rAF = { schedule(callback: (timestamp: number) => void): Subscription { const id = this.impl(callback); return new Subscription(() => cancelAnimationFrame(id)); }, impl: requestAnimationFrame, }

Then, in our run mode, we just need to pull that in and patch the impl to override it's behavior where we are using it.

We could also just monkey patch rAF directly, but that has other implications I'm not keen on.

We also need some sort of way to declare when animation frames should be fired in a test. Probably another, idempotent helper that we add to the context passed to TestScheduler.run. like:

rxTest.run(({ cold, expectObservable, animationFrames }) => { animationFrames('-----x----x----x----x---x-----'); // rest of the test here });

benlesh · 2020-07-26T18:14:39Z

src/internal/observable/dom/animationFrames.ts

+    // queued for a considerable period of time and the elapsed time should
+    // represent the time elapsed since subscription - not the time since the
+    // first rendered animation frame.
+    const start = provider.now();
    const run = (timestamp: DOMHighResTimeStamp | number) => {


I think the solution here is going to be creating a lightweight wrapper around rAF, such that we can override it in run mode. That's what the animationFrameScheduler is supposed to be, but it's inadequate, because it doesn't provide the timestamp, and our schedulers in general are perhaps a bit overkill for this use case.

Something like this should suffice:

export const rAF = { schedule(callback: (timestamp: number) => void): Subscription { const id = this.impl(callback); return new Subscription(() => cancelAnimationFrame(id)); }, impl: requestAnimationFrame, }

Then, in our run mode, we just need to pull that in and patch the impl to override it's behavior where we are using it.

We could also just monkey patch rAF directly, but that has other implications I'm not keen on.

We also need some sort of way to declare when animation frames should be fired in a test. Probably another, idempotent helper that we add to the context passed to TestScheduler.run. like:

rxTest.run(({ cold, expectObservable, animationFrames }) => { animationFrames('-----x----x----x----x---x-----'); // rest of the test here });

cartant · 2020-07-26T22:05:24Z

@benlesh To separate it from the annoying weirdness of the rAF callback timestamp business, I'm going to put the run-mode-patchable providers into a separate PR - referenced above - that targets this issue. I'll come back to this once the providers have been reviewed and merged.

benlesh · 2020-07-27T00:52:13Z

Okay, LGTM, in that I think it's a good iteration, but we have failing tests now that need cleared up.

benlesh · 2020-07-30T18:37:21Z

@cartant Just because I wasn't being clear. I'm good with these changes, the only thing stopping the merge is getting the tests working.

cartant · 2020-07-30T20:28:22Z

@benlesh AFAICT, there are no test failures here. The problem is that the API guardian is failing because of the 1 | 0 | -1 business. I will have another look, but that was what appeared to be the problem and that prompted me to open #5608 - which I don't understand (in light of OJ's comment) and don't know how to fix. I really should wait until I've had my morning coffee before responding to any sort of correspondence. I thought your comment was to do with #5607. My plan was to return to this PR after #5607 is merged.

cartant · 2020-07-30T21:26:29Z

@benlesh FWIW, making these tests pass - before #5607 is merged - requires too much effort and it's effort that will be for nothing, as tests are replaced with animate tests in #5607. When #5607 is merged, I will update this PR.

benlesh · 2020-07-30T22:19:12Z

I guess just mark the tests as skipped for now

cartant · 2020-07-30T22:39:22Z

@benlesh

I guess just mark the tests as skipped for now

Donesies

Emit the timestamp provided by rAF to align better with the rAF specification. Multiple scheduled rAF callbacks within the same frame will be called with the same timestamp.

And use performance.now() for the elapsed calculation instead of the passed timestamp - see comment for explanation.

cartant requested changes May 15, 2020

View reviewed changes

cartant approved these changes May 17, 2020

View reviewed changes

cartant added the target: master only label May 18, 2020

benlesh requested changes Jun 9, 2020

View reviewed changes

benlesh added AGENDA ITEM Flagged for discussion at core team meetings blocked labels Jun 9, 2020

cartant force-pushed the fix-animation-frame-callback branch 2 times, most recently from ca6c077 to d7c7c0b Compare July 26, 2020 09:04

benlesh reviewed Jul 26, 2020

View reviewed changes

src/internal/observable/dom/animationFrames.ts Show resolved Hide resolved

benlesh reviewed Jul 26, 2020

View reviewed changes

benlesh requested changes Jul 26, 2020

View reviewed changes

cartant mentioned this pull request Jul 26, 2020

feat(TestScheduler): add an animate "run mode" helper #5607

Merged

benlesh approved these changes Jul 27, 2020

View reviewed changes

tmair and others added 7 commits August 1, 2020 08:37

fix(animationFrames): emit the timestamp from the rAF's callback

70ac9ba

Emit the timestamp provided by rAF to align better with the rAF specification. Multiple scheduled rAF callbacks within the same frame will be called with the same timestamp.

fix(animationFrames): use shared observable for common use case

ff47146

feat(animationFrames): provide timestamp and elapsed time

6451d25

fix(animationFrames): fix type tests

4b45d7f

fix: base elapsed on subscription time

f343c1b

And use performance.now() for the elapsed calculation instead of the passed timestamp - see comment for explanation.

chore: update API guardian

ae8ff49

chore: update animate-based tests

54b4327

cartant force-pushed the fix-animation-frame-callback branch from a6a10d0 to 54b4327 Compare July 31, 2020 22:58

cartant removed blocked AGENDA ITEM Flagged for discussion at core team meetings labels Jul 31, 2020

cartant requested a review from benlesh July 31, 2020 22:59

benlesh approved these changes Jul 31, 2020

View reviewed changes

benlesh merged commit c980ae6 into ReactiveX:master Jul 31, 2020

tmair deleted the fix-animation-frame-callback branch March 7, 2021 14:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(animationFrames): emit the timestamp from the rAF's callback #5438

fix(animationFrames): emit the timestamp from the rAF's callback #5438

tmair commented May 15, 2020

cartant left a comment

cartant May 15, 2020 •

edited

tmair May 16, 2020

cartant May 15, 2020

benlesh May 16, 2020

benlesh May 16, 2020

cartant May 16, 2020

cartant left a comment

benlesh commented May 27, 2020

cartant commented May 27, 2020

benlesh commented May 29, 2020

benlesh left a comment

benlesh Jun 9, 2020

joshribakoff Jun 17, 2020

benlesh commented Jun 9, 2020

benlesh left a comment

benlesh commented Jun 9, 2020

benlesh commented Jun 9, 2020

tmair commented Jun 18, 2020

cartant commented Jun 18, 2020

joshribakoff commented Jun 22, 2020 •

edited

benlesh Jul 26, 2020

benlesh Jul 26, 2020

benlesh Jul 26, 2020

cartant commented Jul 26, 2020

benlesh commented Jul 27, 2020

benlesh commented Jul 30, 2020

cartant commented Jul 30, 2020 •

edited

cartant commented Jul 30, 2020 •

edited

benlesh commented Jul 30, 2020

cartant commented Jul 30, 2020

fix(animationFrames): emit the timestamp from the rAF's callback #5438

fix(animationFrames): emit the timestamp from the rAF's callback #5438

Conversation

tmair commented May 15, 2020

cartant left a comment

Choose a reason for hiding this comment

cartant May 15, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cartant left a comment

Choose a reason for hiding this comment

benlesh commented May 27, 2020

cartant commented May 27, 2020

benlesh commented May 29, 2020

benlesh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benlesh commented Jun 9, 2020

benlesh left a comment

Choose a reason for hiding this comment

benlesh commented Jun 9, 2020

benlesh commented Jun 9, 2020

tmair commented Jun 18, 2020

cartant commented Jun 18, 2020

joshribakoff commented Jun 22, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cartant commented Jul 26, 2020

benlesh commented Jul 27, 2020

benlesh commented Jul 30, 2020

cartant commented Jul 30, 2020 • edited

cartant commented Jul 30, 2020 • edited

benlesh commented Jul 30, 2020

cartant commented Jul 30, 2020

cartant May 15, 2020 •

edited

joshribakoff commented Jun 22, 2020 •

edited

cartant commented Jul 30, 2020 •

edited

cartant commented Jul 30, 2020 •

edited