From 0f59d4702781fbcb64c09855c0ad5db9bdd3a9c7 Mon Sep 17 00:00:00 2001 From: Michelle Zhang <56095982+michellewzhang@users.noreply.github.com> Date: Tue, 16 Apr 2024 16:10:13 -0700 Subject: [PATCH 1/5] fix(replays): sync replay timer with shown duration --- .../app/components/replays/replayContext.tsx | 2 ++ .../app/components/replays/videoReplayer.tsx | 20 ++++++++++++++++--- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/static/app/components/replays/replayContext.tsx b/static/app/components/replays/replayContext.tsx index 8b90d20f6571ab..9fc799030b5cfb 100644 --- a/static/app/components/replays/replayContext.tsx +++ b/static/app/components/replays/replayContext.tsx @@ -495,6 +495,7 @@ function ProviderNonMemo({ setVideoBuffering(buffering); }, clipWindow, + duration: durationMs, }); // `.current` is marked as readonly, but it's safe to set the value from // inside a `useEffect` hook. @@ -521,6 +522,7 @@ function ProviderNonMemo({ startTimestampMs, startTimeOffsetMs, clipWindow, + durationMs, ] ); diff --git a/static/app/components/replays/videoReplayer.tsx b/static/app/components/replays/videoReplayer.tsx index 48ddb6b0fbe906..aa916d21639ffc 100644 --- a/static/app/components/replays/videoReplayer.tsx +++ b/static/app/components/replays/videoReplayer.tsx @@ -14,6 +14,7 @@ interface OffsetOptions { } interface VideoReplayerOptions { + duration: number; onBuffer: (isBuffering: boolean) => void; onFinished: () => void; onLoaded: (event: any) => void; @@ -56,6 +57,7 @@ export class VideoReplayer { private _videos: Map; private _videoApiPrefix: string; private _clipDuration: number | undefined; + private _duration: number; public config: VideoReplayerConfig = { skipInactive: false, speed: 1.0, @@ -73,6 +75,7 @@ export class VideoReplayer { onFinished, onLoaded, clipWindow, + duration, }: VideoReplayerOptions ) { this._attachments = attachments; @@ -86,6 +89,7 @@ export class VideoReplayer { }; this._videos = new Map(); this._clipDuration = undefined; + this._duration = duration; this.wrapper = document.createElement('div'); if (root) { @@ -297,7 +301,12 @@ export class VideoReplayer { // No more segments if (nextIndex >= this._attachments.length) { - this.stopReplay(); + // 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(); + }); return; } @@ -598,8 +607,13 @@ export class VideoReplayer { const loadedSegmentIndex = await this.loadSegmentAtTime(videoOffsetMs); if (loadedSegmentIndex === undefined) { - // TODO: this shouldn't happen, loadSegment should load the previous - // segment until it's time to start the next segment + // If we end up here, we seeked into a gap + // at the end of the replay. + // This tells the timer to stop at the specified duration + // and prevents the timer from running infinitely. + this._timer.addNotificationAtTime(this._duration, () => { + this.stopReplay(); + }); return Promise.resolve(); } From 0e6c97e850af3ad03cb6fc5ddce152c497a3f3ec Mon Sep 17 00:00:00 2001 From: Michelle Zhang <56095982+michellewzhang@users.noreply.github.com> Date: Tue, 16 Apr 2024 16:14:32 -0700 Subject: [PATCH 2/5] tests --- static/app/components/replays/videoReplayer.spec.tsx | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/static/app/components/replays/videoReplayer.spec.tsx b/static/app/components/replays/videoReplayer.spec.tsx index ad7363e904ae76..8a4b2099957566 100644 --- a/static/app/components/replays/videoReplayer.spec.tsx +++ b/static/app/components/replays/videoReplayer.spec.tsx @@ -87,6 +87,7 @@ describe('VideoReplayer - no starting gap', () => { onFinished: jest.fn(), onLoaded: jest.fn(), onBuffer: jest.fn(), + duration: 40, }); // @ts-expect-error private expect(inst._currentIndex).toEqual(0); @@ -112,6 +113,7 @@ describe('VideoReplayer - no starting gap', () => { onFinished: jest.fn(), onLoaded: jest.fn(), onBuffer: jest.fn(), + duration: 40, }); const playPromise = inst.play(18100); // @ts-expect-error private @@ -140,6 +142,7 @@ describe('VideoReplayer - no starting gap', () => { onFinished: jest.fn(), onLoaded: jest.fn(), onBuffer: jest.fn(), + duration: 40, }); const playPromise = inst.play(50000); // 15000 -> 20000 is a gap, so player should start playing @ index 3, from @@ -162,6 +165,7 @@ describe('VideoReplayer - no starting gap', () => { onFinished: jest.fn(), onLoaded: jest.fn(), onBuffer: jest.fn(), + duration: 40, }); const playPromise = inst.play(0); jest.advanceTimersByTime(2500); @@ -181,6 +185,7 @@ describe('VideoReplayer - no starting gap', () => { onFinished: jest.fn(), onLoaded: jest.fn(), onBuffer: jest.fn(), + duration: 50, }); // play at segment 7 const playPromise = inst.play(45_003); @@ -215,6 +220,7 @@ describe('VideoReplayer - no starting gap', () => { onFinished: jest.fn(), onLoaded: jest.fn(), onBuffer: jest.fn(), + duration: 60, }); // play at segment 7 const playPromise = inst.play(45_003); @@ -288,6 +294,7 @@ describe('VideoReplayer - with starting gap', () => { onFinished: jest.fn(), onLoaded: jest.fn(), onBuffer: jest.fn(), + duration: 40, }); // @ts-expect-error private expect(inst._currentIndex).toEqual(0); @@ -311,6 +318,7 @@ describe('VideoReplayer - with starting gap', () => { onFinished: jest.fn(), onLoaded: jest.fn(), onBuffer: jest.fn(), + duration: 40, }); const playPromise = inst.play(18100); // @ts-expect-error private @@ -339,6 +347,7 @@ describe('VideoReplayer - with starting gap', () => { onFinished: jest.fn(), onLoaded: jest.fn(), onBuffer: jest.fn(), + duration: 40, }); const playPromise = inst.play(50000); // 15000 -> 20000 is a gap, so player should start playing @ index 3, from From 3db9c6ac1918b1979fcef00e1266d68e8affd61f Mon Sep 17 00:00:00 2001 From: Michelle Zhang <56095982+michellewzhang@users.noreply.github.com> Date: Wed, 17 Apr 2024 10:31:11 -0700 Subject: [PATCH 3/5] rename to durationMS --- .../app/components/replays/replayContext.tsx | 2 +- .../components/replays/videoReplayer.spec.tsx | 18 +++++++++--------- .../app/components/replays/videoReplayer.tsx | 12 ++++++------ 3 files changed, 16 insertions(+), 16 deletions(-) diff --git a/static/app/components/replays/replayContext.tsx b/static/app/components/replays/replayContext.tsx index 9fc799030b5cfb..621e00095fba4c 100644 --- a/static/app/components/replays/replayContext.tsx +++ b/static/app/components/replays/replayContext.tsx @@ -495,7 +495,7 @@ function ProviderNonMemo({ setVideoBuffering(buffering); }, clipWindow, - duration: durationMs, + durationMs: durationMs, }); // `.current` is marked as readonly, but it's safe to set the value from // inside a `useEffect` hook. diff --git a/static/app/components/replays/videoReplayer.spec.tsx b/static/app/components/replays/videoReplayer.spec.tsx index 8a4b2099957566..6d5b8c6c41f9e0 100644 --- a/static/app/components/replays/videoReplayer.spec.tsx +++ b/static/app/components/replays/videoReplayer.spec.tsx @@ -87,7 +87,7 @@ describe('VideoReplayer - no starting gap', () => { onFinished: jest.fn(), onLoaded: jest.fn(), onBuffer: jest.fn(), - duration: 40, + durationMs: 40, }); // @ts-expect-error private expect(inst._currentIndex).toEqual(0); @@ -113,7 +113,7 @@ describe('VideoReplayer - no starting gap', () => { onFinished: jest.fn(), onLoaded: jest.fn(), onBuffer: jest.fn(), - duration: 40, + durationMs: 40, }); const playPromise = inst.play(18100); // @ts-expect-error private @@ -142,7 +142,7 @@ describe('VideoReplayer - no starting gap', () => { onFinished: jest.fn(), onLoaded: jest.fn(), onBuffer: jest.fn(), - duration: 40, + durationMs: 40, }); const playPromise = inst.play(50000); // 15000 -> 20000 is a gap, so player should start playing @ index 3, from @@ -165,7 +165,7 @@ describe('VideoReplayer - no starting gap', () => { onFinished: jest.fn(), onLoaded: jest.fn(), onBuffer: jest.fn(), - duration: 40, + durationMs: 40, }); const playPromise = inst.play(0); jest.advanceTimersByTime(2500); @@ -185,7 +185,7 @@ describe('VideoReplayer - no starting gap', () => { onFinished: jest.fn(), onLoaded: jest.fn(), onBuffer: jest.fn(), - duration: 50, + durationMs: 50, }); // play at segment 7 const playPromise = inst.play(45_003); @@ -220,7 +220,7 @@ describe('VideoReplayer - no starting gap', () => { onFinished: jest.fn(), onLoaded: jest.fn(), onBuffer: jest.fn(), - duration: 60, + durationMs: 60, }); // play at segment 7 const playPromise = inst.play(45_003); @@ -294,7 +294,7 @@ describe('VideoReplayer - with starting gap', () => { onFinished: jest.fn(), onLoaded: jest.fn(), onBuffer: jest.fn(), - duration: 40, + durationMs: 40, }); // @ts-expect-error private expect(inst._currentIndex).toEqual(0); @@ -318,7 +318,7 @@ describe('VideoReplayer - with starting gap', () => { onFinished: jest.fn(), onLoaded: jest.fn(), onBuffer: jest.fn(), - duration: 40, + durationMs: 40, }); const playPromise = inst.play(18100); // @ts-expect-error private @@ -347,7 +347,7 @@ describe('VideoReplayer - with starting gap', () => { onFinished: jest.fn(), onLoaded: jest.fn(), onBuffer: jest.fn(), - duration: 40, + durationMs: 40, }); const playPromise = inst.play(50000); // 15000 -> 20000 is a gap, so player should start playing @ index 3, from diff --git a/static/app/components/replays/videoReplayer.tsx b/static/app/components/replays/videoReplayer.tsx index aa916d21639ffc..f7a5f4f4b5dcd3 100644 --- a/static/app/components/replays/videoReplayer.tsx +++ b/static/app/components/replays/videoReplayer.tsx @@ -14,7 +14,7 @@ interface OffsetOptions { } interface VideoReplayerOptions { - duration: number; + durationMs: number; onBuffer: (isBuffering: boolean) => void; onFinished: () => void; onLoaded: (event: any) => void; @@ -57,7 +57,7 @@ export class VideoReplayer { private _videos: Map; private _videoApiPrefix: string; private _clipDuration: number | undefined; - private _duration: number; + private _durationMs: number; public config: VideoReplayerConfig = { skipInactive: false, speed: 1.0, @@ -75,7 +75,7 @@ export class VideoReplayer { onFinished, onLoaded, clipWindow, - duration, + durationMs, }: VideoReplayerOptions ) { this._attachments = attachments; @@ -89,7 +89,7 @@ export class VideoReplayer { }; this._videos = new Map(); this._clipDuration = undefined; - this._duration = duration; + this._durationMs = durationMs; this.wrapper = document.createElement('div'); if (root) { @@ -304,7 +304,7 @@ export class VideoReplayer { // 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._timer.addNotificationAtTime(this._durationMs, () => { this.stopReplay(); }); return; @@ -611,7 +611,7 @@ export class VideoReplayer { // at the end of the replay. // This tells the timer to stop at the specified duration // and prevents the timer from running infinitely. - this._timer.addNotificationAtTime(this._duration, () => { + this._timer.addNotificationAtTime(this._durationMs, () => { this.stopReplay(); }); return Promise.resolve(); From bf9fc98f7ef09e8018f2560636f9beb30cfb8cb5 Mon Sep 17 00:00:00 2001 From: Michelle Zhang <56095982+michellewzhang@users.noreply.github.com> Date: Wed, 17 Apr 2024 12:22:36 -0700 Subject: [PATCH 4/5] fixes and add tests --- .../app/components/replays/replayContext.tsx | 2 +- .../components/replays/videoReplayer.spec.tsx | 130 ++++++++++++++++-- .../app/components/replays/videoReplayer.tsx | 29 ++-- 3 files changed, 142 insertions(+), 19 deletions(-) diff --git a/static/app/components/replays/replayContext.tsx b/static/app/components/replays/replayContext.tsx index 621e00095fba4c..633199e05397a1 100644 --- a/static/app/components/replays/replayContext.tsx +++ b/static/app/components/replays/replayContext.tsx @@ -495,7 +495,7 @@ function ProviderNonMemo({ setVideoBuffering(buffering); }, clipWindow, - durationMs: durationMs, + durationMs, }); // `.current` is marked as readonly, but it's safe to set the value from // inside a `useEffect` hook. diff --git a/static/app/components/replays/videoReplayer.spec.tsx b/static/app/components/replays/videoReplayer.spec.tsx index 6d5b8c6c41f9e0..41e80e16547f03 100644 --- a/static/app/components/replays/videoReplayer.spec.tsx +++ b/static/app/components/replays/videoReplayer.spec.tsx @@ -87,7 +87,7 @@ describe('VideoReplayer - no starting gap', () => { onFinished: jest.fn(), onLoaded: jest.fn(), onBuffer: jest.fn(), - durationMs: 40, + durationMs: 40000, }); // @ts-expect-error private expect(inst._currentIndex).toEqual(0); @@ -113,7 +113,7 @@ describe('VideoReplayer - no starting gap', () => { onFinished: jest.fn(), onLoaded: jest.fn(), onBuffer: jest.fn(), - durationMs: 40, + durationMs: 40000, }); const playPromise = inst.play(18100); // @ts-expect-error private @@ -142,7 +142,7 @@ describe('VideoReplayer - no starting gap', () => { onFinished: jest.fn(), onLoaded: jest.fn(), onBuffer: jest.fn(), - durationMs: 40, + durationMs: 40000, }); const playPromise = inst.play(50000); // 15000 -> 20000 is a gap, so player should start playing @ index 3, from @@ -165,7 +165,7 @@ describe('VideoReplayer - no starting gap', () => { onFinished: jest.fn(), onLoaded: jest.fn(), onBuffer: jest.fn(), - durationMs: 40, + durationMs: 40000, }); const playPromise = inst.play(0); jest.advanceTimersByTime(2500); @@ -185,7 +185,7 @@ describe('VideoReplayer - no starting gap', () => { onFinished: jest.fn(), onLoaded: jest.fn(), onBuffer: jest.fn(), - durationMs: 50, + durationMs: 50000, }); // play at segment 7 const playPromise = inst.play(45_003); @@ -220,7 +220,7 @@ describe('VideoReplayer - no starting gap', () => { onFinished: jest.fn(), onLoaded: jest.fn(), onBuffer: jest.fn(), - durationMs: 60, + durationMs: 55000, }); // play at segment 7 const playPromise = inst.play(45_003); @@ -294,7 +294,7 @@ describe('VideoReplayer - with starting gap', () => { onFinished: jest.fn(), onLoaded: jest.fn(), onBuffer: jest.fn(), - durationMs: 40, + durationMs: 40000, }); // @ts-expect-error private expect(inst._currentIndex).toEqual(0); @@ -318,7 +318,7 @@ describe('VideoReplayer - with starting gap', () => { onFinished: jest.fn(), onLoaded: jest.fn(), onBuffer: jest.fn(), - durationMs: 40, + durationMs: 40000, }); const playPromise = inst.play(18100); // @ts-expect-error private @@ -347,7 +347,7 @@ describe('VideoReplayer - with starting gap', () => { onFinished: jest.fn(), onLoaded: jest.fn(), onBuffer: jest.fn(), - durationMs: 40, + durationMs: 40000, }); const playPromise = inst.play(50000); // 15000 -> 20000 is a gap, so player should start playing @ index 3, from @@ -361,3 +361,115 @@ describe('VideoReplayer - with starting gap', () => { expect(inst.getVideo(inst._currentIndex)?.currentTime).toEqual(5); }); }); + +describe('VideoReplayer - with ending gap', () => { + beforeEach(() => { + jest.clearAllTimers(); + }); + + const attachments = [ + { + id: 0, + timestamp: 2500, + duration: 5000, + }, + // no gap + { + id: 1, + timestamp: 5000, + duration: 5000, + }, + { + id: 2, + timestamp: 10_001, + duration: 5000, + }, + // 5 second gap + { + id: 3, + timestamp: 20_000, + duration: 5000, + }, + // 5 second gap + { + id: 4, + timestamp: 30_000, + duration: 5000, + }, + { + id: 5, + timestamp: 35_002, + duration: 5000, + }, + ]; + + it('keeps playing until the end if there is an ending gap', async () => { + const root = document.createElement('div'); + const inst = new VideoReplayer(attachments, { + videoApiPrefix: '/foo/', + root, + start: 0, + onFinished: jest.fn(), + onLoaded: jest.fn(), + onBuffer: jest.fn(), + durationMs: 50000, + }); + // actual length of the segments is 40s + // 10s gap at the end + + // play at the last segment + const playPromise = inst.play(36000); + await playPromise; + jest.advanceTimersByTime(4000); + + // 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); + }); + + it('ends at the proper time if seeking into a gap at the end', async () => { + const root = document.createElement('div'); + const inst = new VideoReplayer(attachments, { + videoApiPrefix: '/foo/', + root, + start: 0, + onFinished: jest.fn(), + onLoaded: jest.fn(), + onBuffer: jest.fn(), + durationMs: 50000, + }); + // actual length of the segments is 40s + // 10s gap at the end + + // play at the gap + const playPromise = inst.play(40002); + await playPromise; + jest.advanceTimersByTime(4000); + + // we should be still playing in the gap + expect(inst.getCurrentTime()).toEqual(44002); + // @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()).toBeLessThan(50100); + // @ts-expect-error private + expect(inst._isPlaying).toEqual(false); + }); +}); diff --git a/static/app/components/replays/videoReplayer.tsx b/static/app/components/replays/videoReplayer.tsx index f7a5f4f4b5dcd3..ff1aa889524f5a 100644 --- a/static/app/components/replays/videoReplayer.tsx +++ b/static/app/components/replays/videoReplayer.tsx @@ -115,7 +115,11 @@ export class VideoReplayer { this.stopReplay(); }); } else { - // Otherwise, if there's no clip window set, we should + // Tell the timer to stop at the replay end + this._timer.addNotificationAtTime(this._durationMs, () => { + this.stopReplay(); + }); + // If there's no clip window set, we should // load the first segment by default so that users are not staring at a // blank replay. This initially caused some issues // (https://github.com/getsentry/sentry/pull/67911), but the problem was @@ -251,12 +255,16 @@ export class VideoReplayer { this._isPlaying = true; this._timer.start(videoOffsetMs); - // This is used when a replay clip is restarted + // This is used when a replay is restarted // Add another stop notification so the timer doesn't run over if (this._clipDuration) { this._timer.addNotificationAtTime(this._clipDuration, () => { this.stopReplay(); }); + } else { + this._timer.addNotificationAtTime(this._durationMs, () => { + this.stopReplay(); + }); } } @@ -301,13 +309,16 @@ export class VideoReplayer { // No more segments if (nextIndex >= this._attachments.length) { - // 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; + 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(); } // Final check in case replay was stopped immediately after a video From f181a0041cc11520436862c933c099e1b4ca03bb Mon Sep 17 00:00:00 2001 From: Michelle Zhang <56095982+michellewzhang@users.noreply.github.com> Date: Wed, 17 Apr 2024 12:32:27 -0700 Subject: [PATCH 5/5] simplify --- static/app/components/replays/videoReplayer.tsx | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/static/app/components/replays/videoReplayer.tsx b/static/app/components/replays/videoReplayer.tsx index ff1aa889524f5a..cc6e877442ce01 100644 --- a/static/app/components/replays/videoReplayer.tsx +++ b/static/app/components/replays/videoReplayer.tsx @@ -257,15 +257,12 @@ export class VideoReplayer { // This is used when a replay is restarted // Add another stop notification so the timer doesn't run over - if (this._clipDuration) { - this._timer.addNotificationAtTime(this._clipDuration, () => { - this.stopReplay(); - }); - } else { - this._timer.addNotificationAtTime(this._durationMs, () => { + this._timer.addNotificationAtTime( + this._clipDuration ? this._clipDuration : this._durationMs, + () => { this.stopReplay(); - }); - } + } + ); } /**