Skip to content

Commit

Permalink
fix: Revert change that caused stalls with "cannot find endTime" (#7213)
Browse files Browse the repository at this point in the history 
Reverts #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.
  • Loading branch information
@joeyparrish
joeyparrish committed Aug 27, 2024
1 parent 4d4fc14 commit e69ade2
Show file tree
Hide file tree
Showing 3 changed files with 6 additions and 128 deletions.
71 changes: 5 additions & 66 deletions lib/media/streaming_engine.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -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.<string, !Array.<?shaka.media.SegmentIndex>>}
*/
this.deferredCloseSegmentIndex_ = new Map();

/** @private {number} */
this.bufferingGoalScale_ = 1;

Expand Down Expand Up @@ -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();
Expand All @@ -186,7 +170,6 @@ shaka.media.StreamingEngine = class {

this.mediaStates_.clear();
this.audioPrefetchMap_.clear();
this.deferredCloseSegmentIndex_.clear();

this.playerInterface_ = null;
this.manifest_ = null;
Expand Down Expand Up @@ -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.<?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.
*
Expand Down Expand Up @@ -612,39 +572,20 @@ 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();
}
}

mediaState.stream = stream;
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) {
Expand Down Expand Up @@ -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;
Expand Down
57 changes: 1 addition & 56 deletions test/media/streaming_engine_unit.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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?
});
});

Expand Down
6 changes: 0 additions & 6 deletions test/test/util/manifest_generator.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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']);
Expand All @@ -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} */
Expand Down

0 comments on commit e69ade2

Please sign in to comment.