From 8b17c7f6a60f0cb0d43a5732b78bee102e0228ad Mon Sep 17 00:00:00 2001 From: Julian Domingo Date: Mon, 3 Oct 2022 13:49:23 -0700 Subject: [PATCH] fix: Fix A/V sync in unaligned HLS VOD streams (#4528) Fix is based on suggestions from @joeyparrish in https://github.com/shaka-project/shaka-player/issues/4308#issuecomment-1167816267. During automatic adaptations, Shaka will now reset the timestamp offset to ensure the newly active track is properly aligned in the presentation. Verified by disabling ABR and manually triggering variant switches (`shaka.Player.selectVariantTrack()`) between seemingly problematic combinations (e.g., `400k` bps => `6000k` bps stream). Behavior was compared against production. Since `adaptation` events are only triggered by ABR logic (and ABR was disabled for manual testing), `selectVariantTrack()` logic was temporarily changed from: `this.switchVariant_(variant, /* fromAdaptation= */ false, clearBuffer, safeMargin);` => `this.switchVariant_(variant, /* fromAdaptation= */ true, clearBuffer, safeMargin);` to ensure the fixes proposed in this PR were taken into effect and being used during manual testing. Tested content is from the reported bug, located here: https://github.com/shaka-project/shaka-player/issues/4308#issue-1279357794 Closes #4308 --- lib/media/media_source_engine.js | 11 +++++---- lib/media/streaming_engine.js | 33 +++++++++++++++++++++----- lib/player.js | 12 ++++++---- test/media/media_source_engine_unit.js | 28 +++++++++++++++++++++- 4 files changed, 69 insertions(+), 15 deletions(-) diff --git a/lib/media/media_source_engine.js b/lib/media/media_source_engine.js index c303adce02..dda23a627c 100644 --- a/lib/media/media_source_engine.js +++ b/lib/media/media_source_engine.js @@ -522,9 +522,12 @@ shaka.media.MediaSourceEngine = class { * @param {?boolean} hasClosedCaptions True if the buffer contains CEA closed * captions * @param {boolean=} seeked True if we just seeked + * @param {boolean=} adaptation True if we just automatically switched active + * variant(s). * @return {!Promise} */ - async appendBuffer(contentType, data, reference, hasClosedCaptions, seeked) { + async appendBuffer( + contentType, data, reference, hasClosedCaptions, seeked, adaptation) { const ContentType = shaka.util.ManifestParserUtils.ContentType; if (contentType == ContentType.TEXT) { @@ -650,9 +653,9 @@ shaka.media.MediaSourceEngine = class { if (reference && this.sequenceMode_ && contentType != ContentType.TEXT) { // In sequence mode, for non-text streams, if we just cleared the buffer - // and are performing an unbuffered seek, we need to set a new - // timestampOffset on the sourceBuffer. - if (seeked) { + // and are either performing an unbuffered seek or handling an automatic + // adaptation, we need to set a new timestampOffset on the sourceBuffer. + if (seeked || adaptation) { const timestampOffset = reference.startTime; this.enqueueOperation_( contentType, diff --git a/lib/media/streaming_engine.js b/lib/media/streaming_engine.js index e1e6b1f45e..35c4d17649 100644 --- a/lib/media/streaming_engine.js +++ b/lib/media/streaming_engine.js @@ -320,8 +320,14 @@ shaka.media.StreamingEngine = class { * @param {number=} safeMargin * @param {boolean=} force * If true, reload the variant even if it did not change. + * @param {boolean=} adaptation + * If true, update the media state to indicate MediaSourceEngine should + * reset the timestamp offset to ensure the new track segments are correctly + * placed on the timeline. */ - switchVariant(variant, clearBuffer = false, safeMargin = 0, force = false) { + switchVariant( + variant, clearBuffer = false, safeMargin = 0, force = false, + adaptation = false) { this.currentVariant_ = variant; if (!this.startupComplete_) { @@ -332,12 +338,14 @@ shaka.media.StreamingEngine = class { if (variant.video) { this.switchInternal_( variant.video, /* clearBuffer= */ clearBuffer, - /* safeMargin= */ safeMargin, /* force= */ force); + /* safeMargin= */ safeMargin, /* force= */ force, + /* adaptation= */ adaptation); } if (variant.audio) { this.switchInternal_( variant.audio, /* clearBuffer= */ clearBuffer, - /* safeMargin= */ safeMargin, /* force= */ force); + /* safeMargin= */ safeMargin, /* force= */ force, + /* adaptation= */ adaptation); } } @@ -383,9 +391,13 @@ shaka.media.StreamingEngine = class { * @param {number} safeMargin * @param {boolean} force * If true, reload the text stream even if it did not change. + * @param {boolean=} adaptation + * If true, update the media state to indicate MediaSourceEngine should + * reset the timestamp offset to ensure the new track segments are correctly + * placed on the timeline. * @private */ - switchInternal_(stream, clearBuffer, safeMargin, force) { + switchInternal_(stream, clearBuffer, safeMargin, force, adaptation) { const ContentType = shaka.util.ManifestParserUtils.ContentType; const type = /** @type {!ContentType} */(stream.type); const mediaState = this.mediaStates_.get(type); @@ -440,6 +452,7 @@ shaka.media.StreamingEngine = class { mediaState.stream = stream; mediaState.segmentIterator = null; + mediaState.adaptation = !!adaptation; const streamTag = shaka.media.StreamingEngine.logPrefix_(mediaState); shaka.log.debug('switch: switching to Stream ' + streamTag); @@ -769,7 +782,6 @@ shaka.media.StreamingEngine = class { const mediaSourceEngine = this.playerInterface_.mediaSourceEngine; const forceTransmuxTS = this.config_.forceTransmuxTS; - await mediaSourceEngine.init(streamsByType, forceTransmuxTS, this.manifest_.sequenceMode); this.destroyer_.ensureNotDestroyed(); @@ -1638,14 +1650,20 @@ shaka.media.StreamingEngine = class { shaka.log.v1(logPrefix, 'appending media segment at', (reference.syncTime == null ? 'unknown' : reference.syncTime)); + // 'seeked' or 'adaptation' triggered logic applies only to this + // appendBuffer() call. const seeked = mediaState.seeked; mediaState.seeked = false; + const adaptation = mediaState.adaptation; + mediaState.adaptation = false; + await this.playerInterface_.mediaSourceEngine.appendBuffer( mediaState.type, segment, reference, hasClosedCaptions, - seeked); + seeked, + adaptation); this.destroyer_.ensureNotDestroyed(); shaka.log.v2(logPrefix, 'appended media segment'); } @@ -2073,6 +2091,7 @@ shaka.media.StreamingEngine.PlayerInterface; * clearBufferSafeMargin: number, * clearingBuffer: boolean, * seeked: boolean, + * adaptation: boolean, * recovering: boolean, * hasError: boolean, * operation: shaka.net.NetworkingEngine.PendingRequest @@ -2119,6 +2138,8 @@ shaka.media.StreamingEngine.PlayerInterface; * True indicates that the buffer is being cleared. * @property {boolean} seeked * True indicates that the presentation just seeked. + * @property {boolean} adaptation + * True indicates that the presentation just automatically switched variants. * @property {boolean} recovering * True indicates that the last segment was not appended because it could not * fit in the buffer. diff --git a/lib/player.js b/lib/player.js index 140464d38f..54920c1cc8 100644 --- a/lib/player.js +++ b/lib/player.js @@ -3836,8 +3836,8 @@ shaka.Player = class extends shaka.util.FakeEventTarget { return; } - this.switchVariant_(variant, /* fromAdaptation= */ false, clearBuffer, - safeMargin); + this.switchVariant_( + variant, /* fromAdaptation= */ false, clearBuffer, safeMargin); // Workaround for // https://github.com/shaka-project/shaka-player/issues/1299 @@ -5374,7 +5374,9 @@ shaka.Player = class extends shaka.util.FakeEventTarget { const currentVariant = this.streamingEngine_.getCurrentVariant(); if (variant == currentVariant) { shaka.log.debug('Variant already selected.'); - // If you want to clear the buffer, we force to reselect the same variant + // If you want to clear the buffer, we force to reselect the same variant. + // We don't need to reset the timestampOffset since it's the same variant, + // so 'adaptation' isn't passed here. if (clearBuffer) { this.streamingEngine_.switchVariant(variant, clearBuffer, safeMargin, /* force= */ true); @@ -5384,7 +5386,9 @@ shaka.Player = class extends shaka.util.FakeEventTarget { // Add entries to the history. this.addVariantToSwitchHistory_(variant, fromAdaptation); - this.streamingEngine_.switchVariant(variant, clearBuffer, safeMargin); + this.streamingEngine_.switchVariant( + variant, clearBuffer, safeMargin, /* force= */ undefined, + /* adaptation= */ fromAdaptation); let oldTrack = null; if (currentVariant) { oldTrack = shaka.util.StreamUtils.variantToTrack(currentVariant); diff --git a/test/media/media_source_engine_unit.js b/test/media/media_source_engine_unit.js index 2ef9e190db..be0feb2b48 100644 --- a/test/media/media_source_engine_unit.js +++ b/test/media/media_source_engine_unit.js @@ -26,7 +26,8 @@ let MockTimeRanges; * timestampOffset: number, * appendWindowEnd: number, * updateend: function(), - * error: function() + * error: function(), + * mode: string * }} */ let MockSourceBuffer; @@ -57,6 +58,7 @@ describe('MediaSourceEngine', () => { let audioSourceBuffer; let videoSourceBuffer; let mockVideo; + /** @type {HTMLMediaElement} */ let video; let mockMediaSource; @@ -622,6 +624,29 @@ describe('MediaSourceEngine', () => { expect(mockTextEngine.storeAndAppendClosedCaptions).toHaveBeenCalled(); }); + + it('sets timestampOffset on adaptations in sequence mode', async () => { + const initObject = new Map(); + initObject.set(ContentType.VIDEO, fakeVideoStream); + videoSourceBuffer.mode = 'sequence'; + + await mediaSourceEngine.init( + initObject, /* forceTransmuxTS= */ false, /* sequenceMode= */ true); + + expect(videoSourceBuffer.timestampOffset).toBe(0); + + // Mocks appending a segment from a newly adapted variant with a 0.50 + // second misalignment from the old variant. + const reference = dummyReference(0, 1000); + reference.startTime = 0.50; + const appendVideo = mediaSourceEngine.appendBuffer( + ContentType.VIDEO, buffer, reference, /* hasClosedCaptions= */ false, + /* seeked= */ false, /* adaptation= */ true); + videoSourceBuffer.updateend(); + await appendVideo; + + expect(videoSourceBuffer.timestampOffset).toBe(0.50); + }); }); describe('remove', () => { @@ -1197,6 +1222,7 @@ describe('MediaSourceEngine', () => { appendWindowEnd: Infinity, updateend: () => {}, error: () => {}, + mode: 'segments', }; }