Skip to content

Commit 48f51a9

Browse files
fix(replays): sync replay timer with shown duration (#69074)
This PR fixes 2 bugs related to the video replay timer: <img width="827" alt="SCR-20240416-oqes" src="https://github.com/getsentry/sentry/assets/56095982/3dd16316-ed2b-4d24-a682-f3032ef4742a"> ### 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: https://github.com/getsentry/sentry/assets/56095982/9f76acc1-d9e6-4a15-a357-0c711da17b4a 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: https://github.com/getsentry/sentry/assets/56095982/e300e349-d606-4a57-88df-3c49d612a24a 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): https://github.com/getsentry/sentry/assets/56095982/67ce3e16-2ac5-4432-9bee-defd5fc61a34 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): https://github.com/getsentry/sentry/assets/56095982/d05b9f49-e4c4-4e49-98d6-49a652b65b9a Video ends at the expected timestamp. Fixes #68496 Also fixes #68509
1 parent e849a2a commit 48f51a9

File tree

3 files changed

+154
-9
lines changed

3 files changed

+154
-9
lines changed

static/app/components/replays/replayContext.tsx

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -495,6 +495,7 @@ function ProviderNonMemo({
495495
setVideoBuffering(buffering);
496496
},
497497
clipWindow,
498+
durationMs,
498499
});
499500
// `.current` is marked as readonly, but it's safe to set the value from
500501
// inside a `useEffect` hook.
@@ -521,6 +522,7 @@ function ProviderNonMemo({
521522
startTimestampMs,
522523
startTimeOffsetMs,
523524
clipWindow,
525+
durationMs,
524526
]
525527
);
526528

static/app/components/replays/videoReplayer.spec.tsx

Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,7 @@ describe('VideoReplayer - no starting gap', () => {
8787
onFinished: jest.fn(),
8888
onLoaded: jest.fn(),
8989
onBuffer: jest.fn(),
90+
durationMs: 40000,
9091
});
9192
// @ts-expect-error private
9293
expect(inst._currentIndex).toEqual(0);
@@ -112,6 +113,7 @@ describe('VideoReplayer - no starting gap', () => {
112113
onFinished: jest.fn(),
113114
onLoaded: jest.fn(),
114115
onBuffer: jest.fn(),
116+
durationMs: 40000,
115117
});
116118
const playPromise = inst.play(18100);
117119
// @ts-expect-error private
@@ -140,6 +142,7 @@ describe('VideoReplayer - no starting gap', () => {
140142
onFinished: jest.fn(),
141143
onLoaded: jest.fn(),
142144
onBuffer: jest.fn(),
145+
durationMs: 40000,
143146
});
144147
const playPromise = inst.play(50000);
145148
// 15000 -> 20000 is a gap, so player should start playing @ index 3, from
@@ -162,6 +165,7 @@ describe('VideoReplayer - no starting gap', () => {
162165
onFinished: jest.fn(),
163166
onLoaded: jest.fn(),
164167
onBuffer: jest.fn(),
168+
durationMs: 40000,
165169
});
166170
const playPromise = inst.play(0);
167171
jest.advanceTimersByTime(2500);
@@ -181,6 +185,7 @@ describe('VideoReplayer - no starting gap', () => {
181185
onFinished: jest.fn(),
182186
onLoaded: jest.fn(),
183187
onBuffer: jest.fn(),
188+
durationMs: 50000,
184189
});
185190
// play at segment 7
186191
const playPromise = inst.play(45_003);
@@ -215,6 +220,7 @@ describe('VideoReplayer - no starting gap', () => {
215220
onFinished: jest.fn(),
216221
onLoaded: jest.fn(),
217222
onBuffer: jest.fn(),
223+
durationMs: 55000,
218224
});
219225
// play at segment 7
220226
const playPromise = inst.play(45_003);
@@ -288,6 +294,7 @@ describe('VideoReplayer - with starting gap', () => {
288294
onFinished: jest.fn(),
289295
onLoaded: jest.fn(),
290296
onBuffer: jest.fn(),
297+
durationMs: 40000,
291298
});
292299
// @ts-expect-error private
293300
expect(inst._currentIndex).toEqual(0);
@@ -311,6 +318,7 @@ describe('VideoReplayer - with starting gap', () => {
311318
onFinished: jest.fn(),
312319
onLoaded: jest.fn(),
313320
onBuffer: jest.fn(),
321+
durationMs: 40000,
314322
});
315323
const playPromise = inst.play(18100);
316324
// @ts-expect-error private
@@ -339,6 +347,7 @@ describe('VideoReplayer - with starting gap', () => {
339347
onFinished: jest.fn(),
340348
onLoaded: jest.fn(),
341349
onBuffer: jest.fn(),
350+
durationMs: 40000,
342351
});
343352
const playPromise = inst.play(50000);
344353
// 15000 -> 20000 is a gap, so player should start playing @ index 3, from
@@ -352,3 +361,115 @@ describe('VideoReplayer - with starting gap', () => {
352361
expect(inst.getVideo(inst._currentIndex)?.currentTime).toEqual(5);
353362
});
354363
});
364+
365+
describe('VideoReplayer - with ending gap', () => {
366+
beforeEach(() => {
367+
jest.clearAllTimers();
368+
});
369+
370+
const attachments = [
371+
{
372+
id: 0,
373+
timestamp: 2500,
374+
duration: 5000,
375+
},
376+
// no gap
377+
{
378+
id: 1,
379+
timestamp: 5000,
380+
duration: 5000,
381+
},
382+
{
383+
id: 2,
384+
timestamp: 10_001,
385+
duration: 5000,
386+
},
387+
// 5 second gap
388+
{
389+
id: 3,
390+
timestamp: 20_000,
391+
duration: 5000,
392+
},
393+
// 5 second gap
394+
{
395+
id: 4,
396+
timestamp: 30_000,
397+
duration: 5000,
398+
},
399+
{
400+
id: 5,
401+
timestamp: 35_002,
402+
duration: 5000,
403+
},
404+
];
405+
406+
it('keeps playing until the end if there is an ending gap', async () => {
407+
const root = document.createElement('div');
408+
const inst = new VideoReplayer(attachments, {
409+
videoApiPrefix: '/foo/',
410+
root,
411+
start: 0,
412+
onFinished: jest.fn(),
413+
onLoaded: jest.fn(),
414+
onBuffer: jest.fn(),
415+
durationMs: 50000,
416+
});
417+
// actual length of the segments is 40s
418+
// 10s gap at the end
419+
420+
// play at the last segment
421+
const playPromise = inst.play(36000);
422+
await playPromise;
423+
jest.advanceTimersByTime(4000);
424+
425+
// we're still within the last segment (5)
426+
// @ts-expect-error private
427+
expect(inst._currentIndex).toEqual(5);
428+
expect(inst.getCurrentTime()).toEqual(40000);
429+
430+
// now we are in the gap
431+
// timer should still be going since the duration is 50s
432+
jest.advanceTimersByTime(5000);
433+
// @ts-expect-error private
434+
expect(inst._isPlaying).toEqual(true);
435+
436+
// a long time passes
437+
// ensure the timer stops at the end duration (50s)
438+
jest.advanceTimersByTime(60000);
439+
expect(inst.getCurrentTime()).toEqual(50000);
440+
// @ts-expect-error private
441+
expect(inst._isPlaying).toEqual(false);
442+
});
443+
444+
it('ends at the proper time if seeking into a gap at the end', async () => {
445+
const root = document.createElement('div');
446+
const inst = new VideoReplayer(attachments, {
447+
videoApiPrefix: '/foo/',
448+
root,
449+
start: 0,
450+
onFinished: jest.fn(),
451+
onLoaded: jest.fn(),
452+
onBuffer: jest.fn(),
453+
durationMs: 50000,
454+
});
455+
// actual length of the segments is 40s
456+
// 10s gap at the end
457+
458+
// play at the gap
459+
const playPromise = inst.play(40002);
460+
await playPromise;
461+
jest.advanceTimersByTime(4000);
462+
463+
// we should be still playing in the gap
464+
expect(inst.getCurrentTime()).toEqual(44002);
465+
// @ts-expect-error private
466+
expect(inst._isPlaying).toEqual(true);
467+
468+
// a long time passes
469+
// ensure the timer stops at the end duration (50s)
470+
jest.advanceTimersByTime(60000);
471+
expect(inst.getCurrentTime()).toBeLessThan(50100);
472+
// @ts-expect-error private
473+
expect(inst._isPlaying).toEqual(false);
474+
});
475+
});

static/app/components/replays/videoReplayer.tsx

Lines changed: 31 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ interface OffsetOptions {
1414
}
1515

1616
interface VideoReplayerOptions {
17+
durationMs: number;
1718
onBuffer: (isBuffering: boolean) => void;
1819
onFinished: () => void;
1920
onLoaded: (event: any) => void;
@@ -56,6 +57,7 @@ export class VideoReplayer {
5657
private _videos: Map<any, HTMLVideoElement>;
5758
private _videoApiPrefix: string;
5859
private _clipDuration: number | undefined;
60+
private _durationMs: number;
5961
public config: VideoReplayerConfig = {
6062
skipInactive: false,
6163
speed: 1.0,
@@ -73,6 +75,7 @@ export class VideoReplayer {
7375
onFinished,
7476
onLoaded,
7577
clipWindow,
78+
durationMs,
7679
}: VideoReplayerOptions
7780
) {
7881
this._attachments = attachments;
@@ -86,6 +89,7 @@ export class VideoReplayer {
8689
};
8790
this._videos = new Map<any, HTMLVideoElement>();
8891
this._clipDuration = undefined;
92+
this._durationMs = durationMs;
8993

9094
this.wrapper = document.createElement('div');
9195
if (root) {
@@ -111,7 +115,11 @@ export class VideoReplayer {
111115
this.stopReplay();
112116
});
113117
} else {
114-
// Otherwise, if there's no clip window set, we should
118+
// Tell the timer to stop at the replay end
119+
this._timer.addNotificationAtTime(this._durationMs, () => {
120+
this.stopReplay();
121+
});
122+
// If there's no clip window set, we should
115123
// load the first segment by default so that users are not staring at a
116124
// blank replay. This initially caused some issues
117125
// (https://github.com/getsentry/sentry/pull/67911), but the problem was
@@ -247,13 +255,14 @@ export class VideoReplayer {
247255
this._isPlaying = true;
248256
this._timer.start(videoOffsetMs);
249257

250-
// This is used when a replay clip is restarted
258+
// This is used when a replay is restarted
251259
// Add another stop notification so the timer doesn't run over
252-
if (this._clipDuration) {
253-
this._timer.addNotificationAtTime(this._clipDuration, () => {
260+
this._timer.addNotificationAtTime(
261+
this._clipDuration ? this._clipDuration : this._durationMs,
262+
() => {
254263
this.stopReplay();
255-
});
256-
}
264+
}
265+
);
257266
}
258267

259268
/**
@@ -297,8 +306,16 @@ export class VideoReplayer {
297306

298307
// No more segments
299308
if (nextIndex >= this._attachments.length) {
309+
if (this.getCurrentTime() < this._durationMs) {
310+
// If we're at the end of a segment, but there's a gap
311+
// at the end, force the replay to play until the end duration
312+
// rather than stopping right away.
313+
this._timer.addNotificationAtTime(this._durationMs, () => {
314+
this.stopReplay();
315+
});
316+
return;
317+
}
300318
this.stopReplay();
301-
return;
302319
}
303320

304321
// Final check in case replay was stopped immediately after a video
@@ -598,8 +615,13 @@ export class VideoReplayer {
598615
const loadedSegmentIndex = await this.loadSegmentAtTime(videoOffsetMs);
599616

600617
if (loadedSegmentIndex === undefined) {
601-
// TODO: this shouldn't happen, loadSegment should load the previous
602-
// segment until it's time to start the next segment
618+
// If we end up here, we seeked into a gap
619+
// at the end of the replay.
620+
// This tells the timer to stop at the specified duration
621+
// and prevents the timer from running infinitely.
622+
this._timer.addNotificationAtTime(this._durationMs, () => {
623+
this.stopReplay();
624+
});
603625
return Promise.resolve();
604626
}
605627

0 commit comments

Comments
 (0)