Repository: getsentry/sentry
Pull request: ref(replay): only preload mobile replay videos when needed
- Optimize loading of videos for mobile replays
- This PR modifies the `_videos` array by converting it into a dictionary that maps segment index to the video element. This makes it easier to only load videos when requested, rather than preloading them all at once.
- Also fixes a bug where `currentIndex` was consistently undefined in the `createVideo` function, but it looks like it's working as expected now
- Closes https://github.com/getsentry/sentry/issues/67430

Problem statement:
[Replay Details] Performance optimizations for not pre-loading all videos
For the mobile replay player, we currently eagerly load all videos for the replay. This means we can potentially be loading ~720 videos at once, which is not ideal. Instead we want to intelligently load videos that are right before and right after the current time in the player. Since our "go back" button is 10 seconds, we might want to load 15 or 20 seconds before and after the current playing segment.

Hints from issue discussion:

Commit message: add tests

Patch to review:
diff --git a/static/app/components/replays/videoReplayer.spec.tsx b/static/app/components/replays/videoReplayer.spec.tsx
index 9834e04a9f5745..175119f97040d2 100644
--- a/static/app/components/replays/videoReplayer.spec.tsx
+++ b/static/app/components/replays/videoReplayer.spec.tsx
@@ -49,6 +49,32 @@ describe('VideoReplayer - no starting gap', () => {
     },
   ];
 
+  const extra = [
+    {
+      id: 6,
+      timestamp: 40_002,
+      duration: 5000,
+    },
+    {
+      id: 7,
+      timestamp: 45_002,
+      duration: 5000,
+    },
+  ];
+
+  const skip = [
+    {
+      id: 7,
+      timestamp: 45_002,
+      duration: 5000,
+    },
+    {
+      id: 8,
+      timestamp: 50_002,
+      duration: 5000,
+    },
+  ];
+
   it('plays and seeks inside of a segment', async () => {
     const root = document.createElement('div');
     const inst = new VideoReplayer(attachments, {
@@ -116,6 +142,86 @@ describe('VideoReplayer - no starting gap', () => {
     // @ts-expect-error private
     expect(inst.getVideo(inst._currentIndex)?.currentTime).toEqual(5);
   });
+
+  it('initially only loads videos from 0 to BUFFER', async () => {
+    const root = document.createElement('div');
+    const inst = new VideoReplayer(attachments, {
+      videoApiPrefix: '/foo/',
+      root,
+      start: 0,
+      onFinished: jest.fn(),
+      onLoaded: jest.fn(),
+    });
+    const playPromise = inst.play(0);
+    jest.advanceTimersByTime(2500);
+    await playPromise;
+    // @ts-expect-error private
+    expect(inst._currentIndex).toEqual(0);
+    // @ts-expect-error private
+    expect(Object.keys(inst._videos).length).toEqual(3);
+  });
+
+  it('should load the correct videos after playing at a timestamp', async () => {
+    const root = document.createElement('div');
+    const inst = new VideoReplayer(attachments.concat(extra), {
+      videoApiPrefix: '/foo/',
+      root,
+      start: 0,
+      onFinished: jest.fn(),
+      onLoaded: jest.fn(),
+    });
+    // play at segment 7
+    const playPromise = inst.play(45_003);
+    jest.advanceTimersByTime(2500);
+    await playPromise;
+    // @ts-expect-error private
+    expect(inst._currentIndex).toEqual(7);
+
+    // videos loaded should be [0, 1, 2, 4, 5, 6, 7]
+    // @ts-expect-error private
+    expect(Object.keys(inst._videos).length).toEqual(7);
+    // @ts-expect-error private
+    expect(inst._videos[0]).toEqual(inst.getVideo(0));
+    // @ts-expect-error private
+    expect(inst._videos[2]).toEqual(inst.getVideo(2));
+    // @ts-expect-error private
+    expect(inst._videos[3]).toEqual(undefined);
+    // @ts-expect-error private
+    expect(inst._videos[4]).toEqual(inst.getVideo(4));
+    // @ts-expect-error private
+    expect(inst._videos[7]).toEqual(inst.getVideo(7));
+  });
+
+  it('should work correctly if we have missing segments', async () => {
+    const root = document.createElement('div');
+    const inst = new VideoReplayer(attachments.concat(skip), {
+      videoApiPrefix: '/foo/',
+      root,
+      start: 0,
+      onFinished: jest.fn(),
+      onLoaded: jest.fn(),
+    });
+    // play at segment 7
+    const playPromise = inst.play(45_003);
+    jest.advanceTimersByTime(2500);
+    await playPromise;
+    // @ts-expect-error private
+    expect(inst._currentIndex).toEqual(6);
+
+    // videos loaded should be [0, 1, 2, 3, 4, 5, 7, 8]
+    // @ts-expect-error private
+    expect(Object.keys(inst._videos).length).toEqual(8);
+    // @ts-expect-error private
+    expect(inst._videos[0]).toEqual(inst.getVideo(0));
+    // @ts-expect-error private
+    expect(inst._videos[2]).toEqual(inst.getVideo(2));
+    // @ts-expect-error private
+    expect(inst._videos[5]).toEqual(inst.getVideo(5));
+    // @ts-expect-error private
+    expect(inst._videos[6]).toEqual(inst.getVideo(6));
+    // @ts-expect-error private
+    expect(inst._videos[7]).toEqual(inst.getVideo(7));
+  });
 });
 
 describe('VideoReplayer - with starting gap', () => {
diff --git a/static/app/components/replays/videoReplayer.tsx b/static/app/components/replays/videoReplayer.tsx
index 994550bc6c8dbb..ce98accf7370c7 100644
--- a/static/app/components/replays/videoReplayer.tsx
+++ b/static/app/components/replays/videoReplayer.tsx
@@ -5,6 +5,10 @@ import {findVideoSegmentIndex} from './utils';
 
 type RootElem = HTMLDivElement | null;
 
+// The number of segments to load on either side of the requested segment (around 15 seconds)
+// Also the number of segments we load initially
+const PRELOAD_BUFFER = 3;
+
 interface OffsetOptions {
   segmentOffsetMs?: number;
 }
@@ -38,7 +42,11 @@ export class VideoReplayer {
   private _startTimestamp: number;
   private _timer = new Timer();
   private _trackList: [ts: number, index: number][];
-  private _videos: HTMLVideoElement[];
+  /**
+   * _videos is a dict that maps attachment index to the video element.
+   * Video elements in this dict are preloaded and ready to be played.
+   */
+  private _videos: Record<number, HTMLVideoElement>;
   private _videoApiPrefix: string;
   public config: VideoReplayerConfig = {
     skipInactive: false,
@@ -59,15 +67,16 @@ export class VideoReplayer {
       onFinished,
       onLoaded,
     };
+    this._videos = {};
 
     this.wrapper = document.createElement('div');
     if (root) {
       root.appendChild(this.wrapper);
     }
 
-    this._videos = this._attachments.map((attachment, index) =>
-      this.createVideo(attachment, index)
-    );
+    // Initially, only load some videos
+    this.createVideoForRange({low: 0, high: PRELOAD_BUFFER});
+
     this._trackList = this._attachments.map(({timestamp}, i) => [timestamp, i]);
     this.loadSegment(0);
   }
@@ -90,7 +99,7 @@ export class VideoReplayer {
         this._callbacks.onLoaded(event);
       }
     });
-    // TODO: Only preload when necessary
+
     el.preload = 'auto';
     // TODO: Timer needs to also account for playback speed
     el.playbackRate = this.config.speed;
@@ -114,6 +123,20 @@ export class VideoReplayer {
     this.playSegmentAtIndex(nextIndex);
   }
 
+  /**
+   * Create videos from a slice of _attachments, given the start and end index.
+   */
+  protected createVideoForRange({low, high}: {high: number; low: number}) {
+    return this._attachments.slice(low, high).forEach((attachment, index) => {
+      const dictIndex = index + low;
+
+      // Might be some videos we've already loaded before
+      if (!this._videos[dictIndex]) {
+        this._videos[dictIndex] = this.createVideo(attachment, dictIndex);
+      }
+    });
+  }
+
   /**
    * Given a relative time offset, get the segment number where the time offset would be contained in
    */
@@ -141,18 +164,43 @@ export class VideoReplayer {
   }
 
   protected getSegment(index?: number | undefined): VideoEvent | undefined {
-    if (typeof index === 'undefined') {
+    if (index === undefined) {
       return undefined;
     }
 
     return this._attachments[index];
   }
 
+  /**
+   * Returns the video in the dictionary at the requested index.
+   */
   protected getVideo(index: number | undefined): HTMLVideoElement | undefined {
-    if (typeof index === 'undefined') {
+    if (index === undefined || index < 0 || index >= this._attachments.length) {
+      return undefined;
+    }
+
+    return this._videos[index];
+  }
+
+  /**
+   * Fetches the video if it exists, otherwise creates the video and adds to the _videos dictionary.
+   */
+  protected getOrCreateVideo(index: number | undefined): HTMLVideoElement | undefined {
+    const video = this.getVideo(index);
+
+    if (video) {
+      return video;
+    }
+
+    if (index === undefined) {
       return undefined;
     }
 
+    // If we haven't loaded the current video yet, we should load videos on either side too
+    const low = Math.max(0, index - PRELOAD_BUFFER);
+    const high = Math.min(index + PRELOAD_BUFFER, this._attachments.length + 1);
+    this.createVideoForRange({low, high});
+
     return this._videos[index];
   }
 
@@ -236,7 +284,7 @@ export class VideoReplayer {
     // Hide current video
     this.hideVideo(this._currentIndex);
 
-    const nextVideo = this.getVideo(index);
+    const nextVideo = this.getOrCreateVideo(index);
     // Show the next video
     this.showVideo(nextVideo);
 
@@ -260,7 +308,7 @@ export class VideoReplayer {
     const loadedSegmentIndex = await this.loadSegment(index, {segmentOffsetMs: 0});
 
     if (loadedSegmentIndex !== undefined) {
-      this.playVideo(this.getVideo(loadedSegmentIndex));
+      this.playVideo(this.getOrCreateVideo(loadedSegmentIndex));
     }
   }
 
@@ -337,7 +385,7 @@ export class VideoReplayer {
       return Promise.resolve();
     }
 
-    return this.playVideo(this.getVideo(loadedSegmentIndex));
+    return this.playVideo(this.getOrCreateVideo(loadedSegmentIndex));
   }
 
   /**
@@ -366,7 +414,7 @@ export class VideoReplayer {
    */
   public pause(videoOffsetMs: number) {
     // Pause the current video
-    const currentVideo = this.getVideo(this._currentIndex);
+    const currentVideo = this.getOrCreateVideo(this._currentIndex);
     currentVideo?.pause();
     this._timer.stop(videoOffsetMs);
 


Code review comments:
diff_hunk: @@ -116,6 +142,86 @@ describe('VideoReplayer - no starting gap', () => {
     // @ts-expect-error private
     expect(inst.getVideo(inst._currentIndex)?.currentTime).toEqual(5);
   });
+
+  it('initially only loads videos from 0 to BUFFER', async () => {
+    const root = document.createElement('div');
+    const inst = new VideoReplayer(attachments, {
+      videoApiPrefix: '/foo/',
+      root,
+      start: 0,
+      onFinished: jest.fn(),
+      onLoaded: jest.fn(),
+    });
+    const playPromise = inst.play(0);
+    jest.advanceTimersByTime(2500);
+    await playPromise;
+    // @ts-expect-error private
+    expect(inst._currentIndex).toEqual(0);
+    // @ts-expect-error private
+    expect(Object.keys(inst._videos).length).toEqual(3);
+  });
+
+  it('should load the correct videos after playing at a timestamp', async () => {
+    const root = document.createElement('div');
+    const inst = new VideoReplayer(attachments.concat(extra), {
+      videoApiPrefix: '/foo/',
+      root,
+      start: 0,
+      onFinished: jest.fn(),
+      onLoaded: jest.fn(),
+    });
+    // play at segment 7
+    const playPromise = inst.play(45_003);
+    jest.advanceTimersByTime(2500);
+    await playPromise;
+    // @ts-expect-error private
+    expect(inst._currentIndex).toEqual(7);
+
+    // videos loaded should be [0, 1, 2, 4, 5, 6, 7]
+    // @ts-expect-error private
path: static/app/components/replays/videoReplayer.spec.tsx
text: @user1:
🙈 

I would make an intermediary var to hold `inst._videos` so that we don't repeat `ts-expect-error`