Skip to content

Commit

Permalink
fix: Fix A/V sync in unaligned HLS VOD streams (#4528)
Browse files Browse the repository at this point in the history
Fix is based on suggestions from @joeyparrish in
#4308 (comment).

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:
#4308 (comment)

Closes #4308
  • Loading branch information
JulianDomingo committed Oct 6, 2022
1 parent 30e36a4 commit 8b17c7f
Show file tree
Hide file tree
Showing 4 changed files with 69 additions and 15 deletions.
11 changes: 7 additions & 4 deletions lib/media/media_source_engine.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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,
Expand Down
33 changes: 27 additions & 6 deletions lib/media/streaming_engine.js
Original file line number Diff line number Diff line change
Expand Up @@ -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_) {
Expand All @@ -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);
}
}

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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');
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down
12 changes: 8 additions & 4 deletions lib/player.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down
28 changes: 27 additions & 1 deletion test/media/media_source_engine_unit.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ let MockTimeRanges;
* timestampOffset: number,
* appendWindowEnd: number,
* updateend: function(),
* error: function()
* error: function(),
* mode: string
* }}
*/
let MockSourceBuffer;
Expand Down Expand Up @@ -57,6 +58,7 @@ describe('MediaSourceEngine', () => {
let audioSourceBuffer;
let videoSourceBuffer;
let mockVideo;

/** @type {HTMLMediaElement} */
let video;
let mockMediaSource;
Expand Down Expand Up @@ -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', () => {
Expand Down Expand Up @@ -1197,6 +1222,7 @@ describe('MediaSourceEngine', () => {
appendWindowEnd: Infinity,
updateend: () => {},
error: () => {},
mode: 'segments',
};
}

Expand Down

0 comments on commit 8b17c7f

Please sign in to comment.