Browse Source

ref(replay): only preload mobile replay videos when needed (#67657)

- 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
Michelle Zhang 11 months ago
parent
commit
bbec650131

+ 107 - 0
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,87 @@ 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]
+    // since we have [0, 1, 2] preloaded initially
+    // and only [4, 5, 6, 7] loaded when segment 7 is requested
+
+    // @ts-expect-error private
+    const videos = inst._videos;
+    // @ts-expect-error private
+    const getVideo = index => inst.getVideo(index);
+
+    expect(Object.keys(videos).length).toEqual(7);
+    expect(videos[0]).toEqual(getVideo(0));
+    expect(videos[2]).toEqual(getVideo(2));
+    expect(videos[3]).toEqual(undefined);
+    expect(videos[4]).toEqual(getVideo(4));
+    expect(videos[7]).toEqual(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);
+
+    // @ts-expect-error private
+    const videos = inst._videos;
+    // @ts-expect-error private
+    const getVideo = index => inst.getVideo(index);
+
+    // videos loaded should be [0, 1, 2, 3, 4, 5, 7, 8]
+    expect(Object.keys(videos).length).toEqual(8);
+    expect(videos[0]).toEqual(getVideo(0));
+    expect(videos[2]).toEqual(getVideo(2));
+    expect(videos[5]).toEqual(getVideo(5));
+    expect(videos[6]).toEqual(getVideo(6));
+    expect(videos[7]).toEqual(getVideo(7));
+  });
 });
 
 describe('VideoReplayer - with starting gap', () => {

+ 59 - 11
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);