From 2ae90950d509c065498fcf146970de84fcc4545b Mon Sep 17 00:00:00 2001 From: Will Harris Date: Tue, 18 Apr 2023 12:59:23 -0400 Subject: [PATCH] fix: Fix race that allows multiple text streams to be loaded (#5129) 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: https://github.com/shaka-project/shaka-player/issues/4821 --------- Co-authored-by: Dan Sparacio Co-authored-by: Casey Occhialini <1508707+littlespex@users.noreply.github.com> --- CONTRIBUTORS | 1 + lib/media/streaming_engine.js | 7 +++- test/player_integration.js | 39 ++++++++++++++++++ test/test/assets/text-clip-alt.vtt | 63 ++++++++++++++++++++++++++++++ 4 files changed, 109 insertions(+), 1 deletion(-) create mode 100644 test/test/assets/text-clip-alt.vtt diff --git a/CONTRIBUTORS b/CONTRIBUTORS index 2e42b38a4d..58af9023a2 100644 --- a/CONTRIBUTORS +++ b/CONTRIBUTORS @@ -131,6 +131,7 @@ Vincent Valot Vinod Balakrishnan Wayne Morgan Wen Ren +Will Harris Yohann Connell Raymond Cheng Janroel Koppen diff --git a/lib/media/streaming_engine.js b/lib/media/streaming_engine.js index c7b4b9dbda..6529873a5e 100644 --- a/lib/media/streaming_engine.js +++ b/lib/media/streaming_engine.js @@ -79,6 +79,9 @@ shaka.media.StreamingEngine = class { /** @private {?shaka.extern.Stream} */ this.currentTextStream_ = null; + /** @private {number} */ + this.textStreamSequenceId_ = 0; + /** @private {boolean} */ this.parsedPrftEventRaised_ = false; @@ -240,6 +243,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 @@ -262,7 +267,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); diff --git a/test/player_integration.js b/test/player_integration.js index 41a04205c1..e0a3832bd1 100644 --- a/test/player_integration.js +++ b/test/player_integration.js @@ -319,6 +319,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('autoShowText', () => { diff --git a/test/test/assets/text-clip-alt.vtt b/test/test/assets/text-clip-alt.vtt new file mode 100644 index 0000000000..92b922a603 --- /dev/null +++ b/test/test/assets/text-clip-alt.vtt @@ -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.