From e69ade2787e314275e1c8ade087e34533590c919 Mon Sep 17 00:00:00 2001 From: Joey Parrish Date: Mon, 26 Aug 2024 10:08:51 -0700 Subject: [PATCH] fix: Revert change that caused stalls with "cannot find endTime" (#7213) Reverts shaka-project/shaka-player#7157 ("fix: Defer `closeSegmentIndex()` for old streams during ABR switches when segment fetches are ongoing") A regression was reported in video-dev.org Slack, and they narrowed it down to this PR. Playing https://d24rwxnt7vw9qb.cloudfront.net/v1/dash/e6d234965645b411ad572802b6c9d5a10799c9c1/All_Reference_Streams/4577dca5f8a44756875ab5cc913cd1f1/index.mpd with this PR, reportedly: > playback stops rendering with a warning "cannot find segment endTime" just a few seconds prior to the rendering alt. This was initially observed on Samsung TVs, but then later on LG and even in Chrome. --- lib/media/streaming_engine.js | 71 ++-------------------------- test/media/streaming_engine_unit.js | 57 +--------------------- test/test/util/manifest_generator.js | 6 --- 3 files changed, 6 insertions(+), 128 deletions(-) diff --git a/lib/media/streaming_engine.js b/lib/media/streaming_engine.js index c7a2a8d45c..7321bbb7d8 100644 --- a/lib/media/streaming_engine.js +++ b/lib/media/streaming_engine.js @@ -15,10 +15,9 @@ 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.SegmentPrefetch'); goog.require('shaka.media.SegmentReference'); +goog.require('shaka.media.SegmentPrefetch'); goog.require('shaka.net.Backoff'); goog.require('shaka.net.NetworkingEngine'); goog.require('shaka.util.BufferUtils'); @@ -74,20 +73,6 @@ 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.>} - */ - this.deferredCloseSegmentIndex_ = new Map(); - /** @private {number} */ this.bufferingGoalScale_ = 1; @@ -176,7 +161,6 @@ shaka.media.StreamingEngine = class { state.segmentPrefetch.clearAll(); state.segmentPrefetch = null; } - this.handleDeferredCloseSegmentIndexes_(state); } for (const prefetch of this.audioPrefetchMap_.values()) { prefetch.clearAll(); @@ -186,7 +170,6 @@ shaka.media.StreamingEngine = class { this.mediaStates_.clear(); this.audioPrefetchMap_.clear(); - this.deferredCloseSegmentIndex_.clear(); this.playerInterface_ = null; this.manifest_ = null; @@ -521,29 +504,6 @@ 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.} */ (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. * @@ -612,30 +572,11 @@ shaka.media.StreamingEngine = class { fullMimeType, this.manifest_.sequenceMode, stream.external); } - // Releases the segmentIndex of the old stream if safe to do so. + // Releases the segmentIndex of the old stream. // Do not close segment indexes we are prefetching. if (!this.audioPrefetchMap_.has(mediaState.stream)) { if (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.closeSegmentIndex(); } } @@ -643,8 +584,8 @@ shaka.media.StreamingEngine = class { mediaState.segmentIterator = null; mediaState.adaptation = !!adaptation; - const newStreamTag = shaka.media.StreamingEngine.logPrefix_(mediaState); - shaka.log.debug('switch: switching to Stream ' + newStreamTag); + const streamTag = shaka.media.StreamingEngine.logPrefix_(mediaState); + shaka.log.debug('switch: switching to Stream ' + streamTag); if (clearBuffer) { if (mediaState.clearingBuffer) { @@ -1228,8 +1169,6 @@ 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; diff --git a/test/media/streaming_engine_unit.js b/test/media/streaming_engine_unit.js index aaca8426a4..1d812cdbed 100644 --- a/test/media/streaming_engine_unit.js +++ b/test/media/streaming_engine_unit.js @@ -1124,62 +1124,6 @@ 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 @@ -1243,6 +1187,7 @@ 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? }); }); diff --git a/test/test/util/manifest_generator.js b/test/test/util/manifest_generator.js index e05c65d30d..3dd1174d01 100644 --- a/test/test/util/manifest_generator.js +++ b/test/test/util/manifest_generator.js @@ -514,10 +514,6 @@ 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']); @@ -528,8 +524,6 @@ 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} */