fix: Defer closeSegmentIndex() for old streams during ABR switches when segment fetches are ongoing

lib/media/streaming_engine.js

@@ -15,9 +15,10 @@ goog.require('shaka.log');
 goog.require('shaka.media.InitSegmentReference');
 goog.require('shaka.media.ManifestParser');
 goog.require('shaka.media.MediaSourceEngine');
+goog.require('shaka.media.SegmentIndex');
 goog.require('shaka.media.SegmentIterator');
-goog.require('shaka.media.SegmentReference');
 goog.require('shaka.media.SegmentPrefetch');
+goog.require('shaka.media.SegmentReference');
 goog.require('shaka.media.SegmentUtils');
 goog.require('shaka.net.Backoff');
 goog.require('shaka.net.NetworkingEngine');
@@ -73,6 +74,20 @@ shaka.media.StreamingEngine = class {
     /** @private {?shaka.extern.StreamingConfiguration} */
     this.config_ = null;
 
+    /**
+     * Retains deferred SegmentIndex objects for streams which were switched
+     * away from during an ongoing fetchAndAppend_().
+     * They are released and cleared from this map when the next onUpdate_() for
+     * a stream's content type occurs.
+     * This is because at that point, it is guaranteed no logic is actively
+     * using information for these old streams.
+     *
+     * Lastly, the operations are stored in arrays in the off chance multiple
+     * ABR switches happen during an ongoing fetchAndAppend_().
+     * @private {!Map.<string, !Array.<?shaka.media.SegmentIndex>>}
+     */
+    this.deferredCloseSegmentIndex_ = new Map();
+
     /** @private {number} */
     this.bufferingGoalScale_ = 1;
 
@@ -161,6 +176,7 @@ shaka.media.StreamingEngine = class {
         state.segmentPrefetch.clearAll();
         state.segmentPrefetch = null;
       }
+      this.handleDeferredCloseSegmentIndexes_(state);
     }
     for (const prefetch of this.audioPrefetchMap_.values()) {
       prefetch.clearAll();
@@ -170,6 +186,7 @@ shaka.media.StreamingEngine = class {
 
     this.mediaStates_.clear();
     this.audioPrefetchMap_.clear();
+    this.deferredCloseSegmentIndex_.clear();
 
     this.playerInterface_ = null;
     this.manifest_ = null;
@@ -504,6 +521,29 @@ shaka.media.StreamingEngine = class {
   }
 
 
+  /**
+   * Handles deferred releases of old SegmentIndexes for the mediaState's
+   * content type from a previous update.
+   * @param {!shaka.media.StreamingEngine.MediaState_} mediaState
+   * @private
+   */
+  handleDeferredCloseSegmentIndexes_(mediaState) {
+    for (const [key, value] of this.deferredCloseSegmentIndex_.entries()) {
+      const streamId = /** @type {string} */ (key);
+      const segmentIndexArray =
+      /** @type {!Array.<?shaka.media.SegmentIndex>} */ (value);
+      if (streamId.includes(mediaState.type)) {
+        for (const segmentIndex of segmentIndexArray) {
+          if (segmentIndex) {
+            segmentIndex.release();
+          }
+        }
+        this.deferredCloseSegmentIndex_.delete(streamId);
+      }
+    }
+  }
+
+
   /**
    * Switches to the given Stream. |stream| may be from any Variant.
    *
@@ -572,20 +612,39 @@ shaka.media.StreamingEngine = class {
           fullMimeType, this.manifest_.sequenceMode, stream.external);
     }
 
-    // Releases the segmentIndex of the old stream.
+    // Releases the segmentIndex of the old stream if safe to do so.
     // Do not close segment indexes we are prefetching.
     if (!this.audioPrefetchMap_.has(mediaState.stream)) {
       if (mediaState.stream.closeSegmentIndex) {
-        mediaState.stream.closeSegmentIndex();
+        if (mediaState.performingUpdate) {
+          const oldStreamTag =
+              shaka.media.StreamingEngine.logPrefix_(mediaState);
+          // The ongoing update is still using the old stream's segment
+          // reference information.
+          // If we close the old stream now, the update will not complete
+          // correctly.
+          // onUpdate_() will resume the closeSegmentIndex() operation for the
+          // old stream when the update has finished.
+          if (!this.deferredCloseSegmentIndex_.has(oldStreamTag)) {
+            this.deferredCloseSegmentIndex_.set(
+                oldStreamTag, [mediaState.stream.segmentIndex]);
+          } else {
+            // We switched from stream A -> B -> back to A.
+            this.deferredCloseSegmentIndex_.get(oldStreamTag).push(
+                mediaState.stream.segmentIndex);
+          }
+        } else {
+          mediaState.stream.closeSegmentIndex();
+        }
       }
     }
 
     mediaState.stream = stream;
     mediaState.segmentIterator = null;
     mediaState.adaptation = !!adaptation;
 
-    const streamTag = shaka.media.StreamingEngine.logPrefix_(mediaState);
-    shaka.log.debug('switch: switching to Stream ' + streamTag);
+    const newStreamTag = shaka.media.StreamingEngine.logPrefix_(mediaState);
+    shaka.log.debug('switch: switching to Stream ' + newStreamTag);
 
     if (clearBuffer) {
       if (mediaState.clearingBuffer) {
@@ -1169,6 +1228,8 @@ shaka.media.StreamingEngine = class {
       return;
     }
 
+    this.handleDeferredCloseSegmentIndexes_(mediaState);
+
     // Make sure the segment index exists. If not, create the segment index.
     if (!mediaState.stream.segmentIndex) {
       const thisStream = mediaState.stream;

test/media/streaming_engine_unit.js

@@ -1125,6 +1125,62 @@ describe('StreamingEngine', () => {
       expect(mediaSourceEngine.resetCaptionParser).not.toHaveBeenCalled();
     });
 
+    it('defers old stream cleanup on switchVariant during update', async () => {
+      // Delay the appendBuffer call until later so we are waiting for this to
+      // finish when we switch.
+      let p = new shaka.util.PublicPromise();
+      const old = mediaSourceEngine.appendBuffer;
+      // Replace the whole spy since we want to call the original.
+      mediaSourceEngine.appendBuffer =
+          jasmine.createSpy('appendBuffer')
+              .and.callFake(async (type, data, reference) => {
+                await p;
+                return Util.invokeSpy(old, type, data, reference);
+              });
+
+      // Starts with 'initialVariant' (video-11-%d/audio-10-%d).
+      await streamingEngine.start();
+      playing = true;
+
+      await Util.fakeEventLoop(1);
+
+      // Grab a reference to initialVariant's segmentIndex before the switch so
+      // we can test how it's internal fields change overtime.
+      const initialVariantSegmentIndex = initialVariant.video.segmentIndex;
+
+      // Switch to 'differentVariant' (video-14-%d/audio-15-%d) in the middle of
+      // the update.
+      streamingEngine.switchVariant(differentVariant, /* clearBuffer= */ true);
+
+      // Finish the update for 'initialVariant'.
+      p.resolve();
+      // Create a new promise to delay the appendBuffer for 'differentVariant'.
+      p = new shaka.util.PublicPromise();
+      await Util.fakeEventLoop(1);
+
+      const segmentType = shaka.net.NetworkingEngine.RequestType.SEGMENT;
+      const segmentContext = {
+        type: shaka.net.NetworkingEngine.AdvancedRequestType.MEDIA_SEGMENT,
+      };
+
+      // Since a switch occurred in the middle of a fetch for a 'initialVariant'
+      // segment, the closing of the segment index for 'initialVariant' was
+      // deferred.
+      // We check the length of the segment references array to determine
+      // whether it was closed or not.
+      expect(initialVariantSegmentIndex.references.length).toBeGreaterThan(0);
+      netEngine.expectRequest('video-11-0.mp4', segmentType, segmentContext);
+      netEngine.expectRequest('audio-10-0.mp4', segmentType, segmentContext);
+      netEngine.expectNoRequest('video-14-0.mp4', segmentType, segmentContext);
+      netEngine.expectNoRequest('audio-15-0.mp4', segmentType, segmentContext);
+
+      // Finish the update for 'differentVariant'. At this point, the
+      // segmentIndex for 'initialVariant' has been closed.
+      p.resolve();
+      await Util.fakeEventLoop(2);
+      expect(initialVariantSegmentIndex.references.length).toBe(0);
+    });
+
     // See https://github.com/shaka-project/shaka-player/issues/2956
     it('works with fast variant switches during update', async () => {
       // Delay the appendBuffer call until later so we are waiting for this to
@@ -1188,7 +1244,6 @@ describe('StreamingEngine', () => {
       netEngine.expectRequest('text-20-0.mp4', segmentType, segmentContext);
       netEngine.expectNoRequest('text-20-init', segmentType, segmentContext);
       netEngine.expectNoRequest('text-21-init', segmentType, segmentContext);
-      // TODO: huh?
     });
   });
 

test/test/util/manifest_generator.js

@@ -534,6 +534,10 @@ shaka.test.ManifestGenerator.Stream = class {
           jasmine.createSpy('createSegmentIndex').and.callFake(() => {
             return Promise.resolve();
           });
+      const close =
+          jasmine.createSpy('closeSegmentIndex').and.callFake(() => {
+            return Promise.resolve();
+          });
       const shaka_ = manifest ? manifest.shaka_ : shaka;
       const segmentIndex = shaka_.media.SegmentIndex.forSingleSegment(
           /* startTime= */ 0, /* duration= */ 10, ['testUri']);
@@ -544,6 +548,8 @@ shaka.test.ManifestGenerator.Stream = class {
       this.groupId = null;
       /** @type {shaka.extern.CreateSegmentIndexFunction} */
       this.createSegmentIndex = shaka.test.Util.spyFunc(create);
+      /** @type {!function()|undefined} */
+      this.closeSegmentIndex = shaka.test.Util.spyFunc(close);
       /** @type {shaka.media.SegmentIndex} */
       this.segmentIndex = segmentIndex;
       /** @type {string} */