fix(replays): sync replay timer with shown duration #69074
Conversation
github-actions
bot
added
the
Scope: Frontend
Bundle ReportChanges will decrease total bundle size by 106.54kB ⬇️
|
| // If we're at the end of a segment, but there's a gap | ||
| // at the end, force the replay to play until the end duration | ||
| // rather than stopping right away. | ||
| this._timer.addNotificationAtTime(this._duration, () => { | ||
| this.stopReplay(); | ||
| }); |
There was a problem hiding this comment.
Is there a chance of a race condition here where by the time this notification is added, the timer is already passed duration and will continue on?
There was a problem hiding this comment.
this one relates to the case where the replay stops early because we've hit a gap/the end of the segment -- the old code here was just this.stopReplay(). i think we might be okay here because we don't want to stop early, but rather keep playing until the end, which means we haven't hit duration yet
There was a problem hiding this comment.
I'm thinking about the case where we do not have a gap, and we want to stop the replay:
- segment ends -> handleSegmentEnd is called
- no next segment so this code block is called
- add notification for duration (e.g. 20)
- due to external factors (data, hardware, etc), the current time is > 20
- ?? does the callback get fired in this case?
There was a problem hiding this comment.
ohhh i see what you're saying! let me add another check that stops the replay if the time is over
There was a problem hiding this comment.
something like
// No more segments
if (nextIndex >= this._attachments.length) {
if (this.getCurrentTime() < this._durationMs) {
// If we're at the end of a segment, but there's a gap
// at the end, force the replay to play until the end duration
// rather than stopping right away.
this._timer.addNotificationAtTime(this._durationMs, () => {
this.stopReplay();
});
return;
}
this.stopReplay();
}
michellewzhang
force-pushed
the
mz/timestamp-fix
branch
from
be69d5f
to
bf9fc98
Compare
| if (this.getCurrentTime() < this._durationMs) { | ||
| // If we're at the end of a segment, but there's a gap | ||
| // at the end, force the replay to play until the end duration | ||
| // rather than stopping right away. | ||
| this._timer.addNotificationAtTime(this._durationMs, () => { | ||
| this.stopReplay(); | ||
| }); | ||
| return; | ||
| } |
| // we're still within the last segment (5) | ||
| // @ts-expect-error private | ||
| expect(inst._currentIndex).toEqual(5); | ||
| expect(inst.getCurrentTime()).toEqual(40000); | ||
|
|
||
| // now we are in the gap | ||
| // timer should still be going since the duration is 50s | ||
| jest.advanceTimersByTime(5000); | ||
| // @ts-expect-error private | ||
| expect(inst._isPlaying).toEqual(true); | ||
|
|
||
| // a long time passes | ||
| // ensure the timer stops at the end duration (50s) | ||
| jest.advanceTimersByTime(60000); | ||
| expect(inst.getCurrentTime()).toEqual(50000); | ||
| // @ts-expect-error private | ||
| expect(inst._isPlaying).toEqual(false); |
There was a problem hiding this comment.
@billyvg i tried to test that here
This PR fixes 2 bugs related to the video replay timer:
1. If the replay has a gap at the end (e.g. the segment ends but for some reason the duration we get from the SDK extends beyond that), the old behavior was that the replay would just stop.
BEFORE:
Screen.Recording.2024-04-16.at.4.17.02.PM.mov
Notice how the replay stops after 9 seconds. This is because the rest of the replay (the other 4 hours) is junk. The ideal behavior would be to have the replay play until the end, so that it doesn't look weird on the UI.
AFTER:
Screen.Recording.2024-04-16.at.4.20.36.PM.mov
Notice how the replay keeps going after 9 seconds (even though technically there isn't any video content to display. It's just showing the previous segment)
2. If we seek into the gap at the end of the video, the old behavior of the video replayer was to keep running the timer infinitely. This is because the replayer wanted to run the timer until we can play the next segment (which doesn't exist in this case).
BEFORE (video trimmed because you'd be watching paint dry but i seeked to somewhere about 10s from the end):
before.mov
Notice how the timer continues running even after the replay "ends".
The fix here is to add a notification to the timer to stop at the replay duration (passed in from
replayContext) when we seek into a gap at the end. This lets the replayer know that we should stop searching for the next (nonexistent) segment.AFTER (video trimmed, same seeking place):
after.mov
Video ends at the expected timestamp.
Fixes #68496
Also fixes #68509