Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(replays): sync replay timer with shown duration #69074

Merged
merged 5 commits into from
Apr 17, 2024
Merged

fix(replays): sync replay timer with shown duration #69074

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0f59d47
fix(replays): sync replay timer with shown duration
michellewzhang Apr 16, 2024
0e6c97e
tests
michellewzhang Apr 16, 2024
3db9c6a
rename to durationMS
michellewzhang Apr 17, 2024
bf9fc98
fixes and add tests
michellewzhang Apr 17, 2024
f181a00
simplify
michellewzhang Apr 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions static/app/components/replays/replayContext.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -495,6 +495,7 @@ function ProviderNonMemo({
setVideoBuffering(buffering);
},
clipWindow,
durationMs,
});
// `.current` is marked as readonly, but it's safe to set the value from
// inside a `useEffect` hook.
Expand All @@ -521,6 +522,7 @@ function ProviderNonMemo({
startTimestampMs,
startTimeOffsetMs,
clipWindow,
durationMs,
]
);

Expand Down
121 changes: 121 additions & 0 deletions static/app/components/replays/videoReplayer.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ describe('VideoReplayer - no starting gap', () => {
onFinished: jest.fn(),
onLoaded: jest.fn(),
onBuffer: jest.fn(),
durationMs: 40000,
});
// @ts-expect-error private
expect(inst._currentIndex).toEqual(0);
Expand All @@ -112,6 +113,7 @@ describe('VideoReplayer - no starting gap', () => {
onFinished: jest.fn(),
onLoaded: jest.fn(),
onBuffer: jest.fn(),
durationMs: 40000,
});
const playPromise = inst.play(18100);
// @ts-expect-error private
Expand Down Expand Up @@ -140,6 +142,7 @@ describe('VideoReplayer - no starting gap', () => {
onFinished: jest.fn(),
onLoaded: jest.fn(),
onBuffer: jest.fn(),
durationMs: 40000,
});
const playPromise = inst.play(50000);
// 15000 -> 20000 is a gap, so player should start playing @ index 3, from
Expand All @@ -162,6 +165,7 @@ describe('VideoReplayer - no starting gap', () => {
onFinished: jest.fn(),
onLoaded: jest.fn(),
onBuffer: jest.fn(),
durationMs: 40000,
});
const playPromise = inst.play(0);
jest.advanceTimersByTime(2500);
Expand All @@ -181,6 +185,7 @@ describe('VideoReplayer - no starting gap', () => {
onFinished: jest.fn(),
onLoaded: jest.fn(),
onBuffer: jest.fn(),
durationMs: 50000,
});
// play at segment 7
const playPromise = inst.play(45_003);
Expand Down Expand Up @@ -215,6 +220,7 @@ describe('VideoReplayer - no starting gap', () => {
onFinished: jest.fn(),
onLoaded: jest.fn(),
onBuffer: jest.fn(),
durationMs: 55000,
});
// play at segment 7
const playPromise = inst.play(45_003);
Expand Down Expand Up @@ -288,6 +294,7 @@ describe('VideoReplayer - with starting gap', () => {
onFinished: jest.fn(),
onLoaded: jest.fn(),
onBuffer: jest.fn(),
durationMs: 40000,
});
// @ts-expect-error private
expect(inst._currentIndex).toEqual(0);
Expand All @@ -311,6 +318,7 @@ describe('VideoReplayer - with starting gap', () => {
onFinished: jest.fn(),
onLoaded: jest.fn(),
onBuffer: jest.fn(),
durationMs: 40000,
});
const playPromise = inst.play(18100);
// @ts-expect-error private
Expand Down Expand Up @@ -339,6 +347,7 @@ describe('VideoReplayer - with starting gap', () => {
onFinished: jest.fn(),
onLoaded: jest.fn(),
onBuffer: jest.fn(),
durationMs: 40000,
});
const playPromise = inst.play(50000);
// 15000 -> 20000 is a gap, so player should start playing @ index 3, from
Expand All @@ -352,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);
Comment on lines +425 to +441
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@billyvg i tried to test that here

});

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);
});
});
35 changes: 30 additions & 5 deletions static/app/components/replays/videoReplayer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ interface OffsetOptions {
}

interface VideoReplayerOptions {
durationMs: number;
onBuffer: (isBuffering: boolean) => void;
onFinished: () => void;
onLoaded: (event: any) => void;
Expand Down Expand Up @@ -56,6 +57,7 @@ export class VideoReplayer {
private _videos: Map<any, HTMLVideoElement>;
private _videoApiPrefix: string;
private _clipDuration: number | undefined;
private _durationMs: number;
public config: VideoReplayerConfig = {
skipInactive: false,
speed: 1.0,
Expand All @@ -73,6 +75,7 @@ export class VideoReplayer {
onFinished,
onLoaded,
clipWindow,
durationMs,
}: VideoReplayerOptions
) {
this._attachments = attachments;
Expand All @@ -86,6 +89,7 @@ export class VideoReplayer {
};
this._videos = new Map<any, HTMLVideoElement>();
this._clipDuration = undefined;
this._durationMs = durationMs;

this.wrapper = document.createElement('div');
if (root) {
Expand All @@ -111,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
Expand Down Expand Up @@ -247,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();
});
michellewzhang marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down Expand Up @@ -297,8 +309,16 @@ export class VideoReplayer {

// 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;
}
Comment on lines +309 to +317
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this tested?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#69074 (comment)

this.stopReplay();
return;
}

// Final check in case replay was stopped immediately after a video
Expand Down Expand Up @@ -598,8 +618,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._durationMs, () => {
this.stopReplay();
});
return Promise.resolve();
}

Expand Down
Loading