Skip to content

Commit

Permalink
fix: Fix race that allows multiple text streams to be loaded (#5129)
Browse files Browse the repository at this point in the history
When enabling text visibility and selecting a new text language
simultaneously, a race condition can occur that allows multiple text
streams to be loaded in the same text media state.
Tracking a sequence id, updates `loadNewTextStream_` to only create a
new text media state with the text stream from the last call to
`loadNewTextStream_`.

Resolves: #4821

---------

Co-authored-by: Dan Sparacio <daniel.sparacio@cbsinteractive.com>
Co-authored-by: Casey Occhialini <1508707+littlespex@users.noreply.github.com>
  • Loading branch information
3 people authored and joeyparrish committed Apr 26, 2023
1 parent cbfaa70 commit 1942d1d
Show file tree
Hide file tree
Showing 4 changed files with 110 additions and 1 deletion.
2 changes: 2 additions & 0 deletions CONTRIBUTORS
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,8 @@ Vasanth Polipelli <vasanthap@google.com>
Vignesh Venkatasubramanian <vigneshv@google.com>
Vincent Valot <valot.vince@gmail.com>
Wayne Morgan <wayne.morgan.dev@gmail.com>
Wen Ren <renwen0615@gmail.com>
Will Harris <will.harris@paramount.com>
Yohann Connell <robinconnell@google.com>
Raymond Cheng <raycheng100@gmail.com>
Janroel Koppen <j.koppen@bluebillywig.com>
Expand Down
7 changes: 6 additions & 1 deletion lib/media/streaming_engine.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,9 @@ shaka.media.StreamingEngine = class {
/** @private {?shaka.extern.Stream} */
this.currentTextStream_ = null;

/** @private {number} */
this.textStreamSequenceId_ = 0;

/**
* Maps a content type, e.g., 'audio', 'video', or 'text', to a MediaState.
*
Expand Down Expand Up @@ -220,6 +223,8 @@ shaka.media.StreamingEngine = class {
const ContentType = shaka.util.ManifestParserUtils.ContentType;
goog.asserts.assert(!this.mediaStates_.has(ContentType.TEXT),
'Should not call loadNewTextStream_ while streaming text!');
this.textStreamSequenceId_++;
const currentSequenceId = this.textStreamSequenceId_;

try {
// Clear MediaSource's buffered text, so that the new text stream will
Expand All @@ -241,7 +246,7 @@ shaka.media.StreamingEngine = class {
const streamText =
textDisplayer.isTextVisible() || this.config_.alwaysStreamText;

if (streamText) {
if (streamText && (this.textStreamSequenceId_ == currentSequenceId)) {
const state = this.createMediaState_(stream);
this.mediaStates_.set(ContentType.TEXT, state);
this.scheduleUpdate_(state, 0);
Expand Down
39 changes: 39 additions & 0 deletions test/player_integration.js
Original file line number Diff line number Diff line change
Expand Up @@ -398,6 +398,45 @@ describe('Player', () => {
player.setTextTrackVisibility(true);
expect(getTracksActive()).toEqual([false, true]);
});

// https://github.com/shaka-project/shaka-player/issues/4821
it('loads a single text stream', async () => {
player.configure({preferredTextLanguage: 'en'});
await player.load('test:sintel_no_text_compiled');

// Add preferred language text track.
const locationUri = new goog.Uri(location.href);
const partialUri = new goog.Uri('/base/test/test/assets/text-clip.vtt');
const absoluteUri = locationUri.resolve(partialUri);
await player.addTextTrackAsync(
absoluteUri.toString(), 'en', 'subtitles', 'text/vtt');

// Add alternate language text track.
// Two text tracks with same timings but different text
// are necessary for test.
const partialUri2 =
new goog.Uri('/base/test/test/assets/text-clip-alt.vtt');
const absoluteUri2 = locationUri.resolve(partialUri2);
await player.addTextTrackAsync(
absoluteUri2.toString(), 'fr', 'subtitles', 'text/vtt');

const textTracks = player.getTextTracks();
expect(textTracks.length).toBe(2);
expect(textTracks[0].language).toBe('en');
expect(textTracks[1].language).toBe('fr');

// Enable text visibilty and immediately change language.
// Only one set of cues should be active.
// Cues should be of the selected language track.
player.setTextTrackVisibility(true);
player.selectTextLanguage('fr');
video.currentTime = 5;
video.play();
await waiter.waitForMovementOrFailOnTimeout(video, 10);

expect(video.textTracks[0].activeCues.length).toBe(1);
expect(player.getTextTracks()[1].active).toBe(true);
});
}); // describe('setTextTrackVisibility')

describe('plays', () => {
Expand Down
63 changes: 63 additions & 0 deletions test/test/assets/text-clip-alt.vtt
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
WEBVTT
00:04.000 --> 00:07.300
Journal de bord du capitaine.
Date stellaire 41636.9.

00:07.500 --> 00:11.800
Comme nous le craignions, l'épave
du cargo Odin de la Fédération,

00:12.100 --> 00:16.400
percuté par un astéroïde,
ne révèle aucun signe de vie.

00:16.700 --> 00:19.000
Mais trois nacelles
de sauvetage manquaient,

00:19.300 --> 00:21.900
laissant supposer
qu'il y aurait des survivants.

00:22.700 --> 00:27.700
- Mise sur orbite Angel One, paré.
- Quel genre d'endroit, Data ?

00:28.200 --> 00:31.600
Planète de Classe-M.
Faune et flore à base de carbone.

00:31.900 --> 00:34.300
Population éparse
et forme de vie intelligente.

00:34.600 --> 00:38.400
Développement technique équivalent
au milieu du 20ème siècle terrien.

00:38.800 --> 00:41.000
C'est presque un retour aux sources.

00:41.300 --> 00:43.600
Si jamais des survivants
ont pu aller si loin,

00:43.800 --> 00:49.000
c'était la planète la plus proche.
La distance qui nous a pris 2 jours

00:49.500 --> 00:52.400
aurait pris 5 mois aux nacelles
de sauvetage du Odin.

00:52.600 --> 00:54.700
Cinq mois, six jours, onze heures,
deux min...

00:54.900 --> 00:58.400
- Merci, Data.
- ..et 57 secondes.

00:58.600 --> 01:01.300
Signal audio provenant d'Angel One.

0 comments on commit 1942d1d

Please sign in to comment.