Skip to content

Commit

Permalink
fix(CEA): Fix missing captions when switching streams (#2672)
Browse files Browse the repository at this point in the history
During video playback, if the user switches the caption stream (e.g. CC1 to CC3 which changes languages), the first caption in the next fragment is missing.

In fragmented MP4s, the end time of a caption is determined by the start time of the next caption. Thus for the last caption in a fragment, the end time cannot be determined until the next fragment is parsed.

Before this fix: the clearing of the caption stream was being called from a chain of function calls originating from clearBuffer_() on the Media source engine. But clearing a buffer and resetting a caption stream are two independent operations. As a result, the caption parser was being reset (its buffer cleared) during video seeks, and during stream switches. This makes sense for video seeks, because the end time of the last caption in the fragment can't be determined if the entire presentation timestamp changes. However for stream switches, resetting the parser doesn't make sense. Clearing the caption parser during a stream switch would actually get rid of the last caption in that fragment (which wasn't emitted since its end time wasn't determined yet), and we would lose the data, causing the problem.

The fix is to reset (and hence clear) the caption parser during seeks, but not during stream switches.

Issue #2648
  • Loading branch information
muhammadharis authored and joeyparrish committed Jul 22, 2020
1 parent 41e5206 commit df84867
Show file tree
Hide file tree
Showing 4 changed files with 36 additions and 11 deletions.
18 changes: 7 additions & 11 deletions lib/media/media_source_engine.js
Original file line number Diff line number Diff line change
Expand Up @@ -573,17 +573,6 @@ shaka.media.MediaSourceEngine = class {
if (!this.textEngine_) {
return;
}

// CaptionParser tracks the latest timestamp and uses this to filter
// for duplicate captions. We do this ourselves, so we must reset
// the CaptionParser when we seek. The best indicator of an
// unbuffered seek in MediaSourceEngine is clear(). This causes a
// small glitch when we change languages (which also calls clear()),
// where the first caption in the new language may be missing.
// TODO: Ask mux.js for a switch to remove this timestamp-tracking
// feature so that we can do away with these hacks.
this.captionParser_.reset();

await this.textEngine_.remove(0, Infinity);
} else {
// Note that not all platforms allow clearing to Infinity.
Expand All @@ -593,6 +582,13 @@ shaka.media.MediaSourceEngine = class {
}
}

/**
* Fully reset the state of the caption parser owned by MediaSourceEngine.
*/
resetCaptionParser() {
this.captionParser_.reset();
}

/**
* Enqueue an operation to flush the SourceBuffer.
* This is a workaround for what we believe is a Chromecast bug.
Expand Down
7 changes: 7 additions & 0 deletions lib/media/streaming_engine.js
Original file line number Diff line number Diff line change
Expand Up @@ -585,6 +585,7 @@ shaka.media.StreamingEngine = class {
seeked() {
const presentationTime = this.playerInterface_.getPresentationTime();
const smallGapLimit = this.config_.smallGapLimit;
const ContentType = shaka.util.ManifestParserUtils.ContentType;
const newTimeIsBuffered = (type) => {
return this.playerInterface_.mediaSourceEngine.isBuffered(
type, presentationTime, smallGapLimit);
Expand All @@ -600,6 +601,12 @@ shaka.media.StreamingEngine = class {
if (somethingBuffered && !newTimeIsBuffered(type)) {
// This stream exists, and isn't buffered.
this.forceClearBuffer_(this.mediaStates_.get(type));

// The pts has shifted from the seek, invalidating captions currently
// in the text buffer. Thus, clear and reset the caption parser.
if (type === ContentType.TEXT) {
this.playerInterface_.mediaSourceEngine.resetCaptionParser();
}
streamCleared = true;
}
}
Expand Down
18 changes: 18 additions & 0 deletions test/media/streaming_engine_unit.js
Original file line number Diff line number Diff line change
Expand Up @@ -774,6 +774,7 @@ describe('StreamingEngine', () => {
let sameAudioVariant;
let sameVideoVariant;
let initialTextStream;
let newTextStream;

beforeEach(() => {
// Set up a manifest with multiple variants and a text stream.
Expand Down Expand Up @@ -801,12 +802,16 @@ describe('StreamingEngine', () => {
manifest.addTextStream(20, (stream) => {
stream.useSegmentTemplate('text-20-%d.mp4', 10);
});
manifest.addTextStream(21, (stream) => {
stream.useSegmentTemplate('text-21-%d.mp4', 10);
});
});

initialVariant = manifest.variants[0];
sameAudioVariant = manifest.variants[1];
sameVideoVariant = manifest.variants[2];
initialTextStream = manifest.textStreams[0];
newTextStream = manifest.textStreams[1];

// For these tests, we don't care about specific data appended.
// Just return any old ArrayBuffer for any requested segment.
Expand Down Expand Up @@ -875,6 +880,17 @@ describe('StreamingEngine', () => {
await Util.fakeEventLoop(1);
expect(mediaSourceEngine.clear).not.toHaveBeenCalled();
});

it('will not reset caption parser when text streams change', async () => {
await streamingEngine.start();
playing = true;

mediaSourceEngine.clear.calls.reset();
streamingEngine.switchTextStream(newTextStream);
await Util.fakeEventLoop(1);
expect(mediaSourceEngine.clear).toHaveBeenCalled();
expect(mediaSourceEngine.resetCaptionParser).not.toHaveBeenCalled();
});
});

describe('handles seeks (VOD)', () => {
Expand Down Expand Up @@ -1089,6 +1105,8 @@ describe('StreamingEngine', () => {
expect(presentationTimeInSeconds).toBeLessThan(20);
streamingEngine.seeked();

expect(mediaSourceEngine.resetCaptionParser).toHaveBeenCalled();

onTick.and.callFake(() => {
// Verify that all buffers have been cleared.
expect(mediaSourceEngine.clear)
Expand Down
4 changes: 4 additions & 0 deletions test/test/util/fake_media_source_engine.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,10 @@ shaka.test.FakeMediaSourceEngine = class {
this.clear = jasmine.createSpy('clear')
.and.callFake((type) => this.clearImpl_(type));

/** @type {!jasmine.Spy} */
this.resetCaptionParser = jasmine.createSpy('resetCaptionParser')
.and.stub();

/** @type {!jasmine.Spy} */
this.bufferStart = jasmine.createSpy('bufferStart')
.and.callFake((type) => this.bufferStartImpl_(type));
Expand Down

0 comments on commit df84867

Please sign in to comment.