Fix text track change edge cases #1582

OrenMe · 2018-02-25T15:06:57Z

Description of the Changes

Fix text track change edge cases:

If text track mode is set to disabled in middle of VTTParser operation, then bailout as currentTrack.cues will be nullified when track is disabled.
If accessing currentTrack.cues.getCueById when the mode is disabled then an exception is thrown
If subtitles are changed in the middle of downloading another subtitle track then in certain edge cases the next subtitle track gets stuck and doesn't download fragments

CheckLists

changes have been done against master branch, and PR does not conflict
no commits have been done in dist folder (we will take care of updating it)
new unit / functional tests have been added (whenever applicable)
Travis tests are passing (or test results are not worse than on master branch :))
API or design changes are documented in API.md

If text track mode is set to `disabled` in middle of VTTParser, then bailout as `currentTrack.cues` will be nullified when track is disabled. If accessing `currentTrack.cues.getCueById` when the mode is `disabled` then an exception is thrown

If subtitles are changed in the middle of downloading another subtitle track then in certain edge cases the next subtitle track gets stuck and doesn't download fragments

johnBartos · 2018-02-26T22:40:13Z

disabled check LGTM. The other check seems ok, but I'm also concerned it could cause a fragment to fail to process if a subsequent SUBTITLE_TRACK_LOADED event comes at a bad time. What is the case in which subtitle tracks get stuck - are we stuck in some state other than IDLE?

OrenMe · 2018-02-27T07:34:34Z

Yes @johnBartos we are stuck in FRAG_LOADING state.
The reason is exactly connected to the other fix.
You start downloading one text track, and you are in the middle of the fragment loading.
Fragments are being loaded, by being called to nextFrag and after downloaded they are passed to _parseVTTs in timeline-controller and in the middle here a user changes track.
until the disabled fix the code crashed there when trying to access currentTrack.cues.getCueById(cue.id) cause the current track got disabled, leading cues object to become null.
Now after the fix it won't crash as we protect against this case.
But, in both cases the SUBTITLE_FRAG_PROCESSED event in _parseVTTs is not sent leading to subtitle-stream-controller to remain stuck on FRAG_LOADING state.

In general I think this makes sense as we rely to heavily on the native track object and if it's null you can't do anything.
The downside is that 1 or 2 cues that were already downloaded will be lost and need to be downloaded again if user picks that language again, but I think this downside is small in comparison to what needs to be done to remedy this situation.
Also, you can see that we do the same when a fragment is loaded successfully:

  onSubtitleFragProcessed(data) {
    if(data.success) {
      this.vttFragSNsProcessed[data.frag.trackId].push(data.frag.sn);
    }
    this.currentlyProcessing = null;
    this.state = State.IDLE;
    this.nextFrag();
  }

and our use case is more like onSubtitleFragFailedProcessed and we need to reset the state.

After writing all of this I think that maybe the better fix will be to actually use the success flag in the SUBTITLE_FRAG_PROCESSED event fired from the _parseVTTs method.
This will keep the state machine is sync and we won't have to use this reset on onSubtitleTrackLoaded.

But now I stumbled on an even worse case I think.
If you start downloading fragments of a text track and then switch in the middle to another track where we have some frags fro previous track that weren't even downloaded then we call clearVttFragQueues on onSubtitleTrackSwitch in subtitle-stream-controller.
This means that if we switch back to the first track it will never resume downloading the frags that were left.

I pushed another fix, please review it.
I now check on onSubtitleTrackSwitch if the switched level has details, and make sure we resume the download.
I also don't clear the vttFragQueues, so we retain the list of pending frags.

OrenMe · 2018-02-28T17:30:45Z

Hi @johnBartos, any chance you got to review this?

johnBartos · 2018-03-01T22:00:17Z

@OrenMe Taking a look. Do you have a test stream and repro steps for the tricky edge cases?

OrenMe · 2018-03-01T22:33:04Z

@johnBartos take any media that has more then one caption language.
Switch to one language and then immediately switch to the other, make sure not all the VTT segments of the first one were downloaded.
Then switch to the first one back and see that the segments that weren’t downloaded are never downloaded, so you won’t see captions for that part.

I discovered it because I had the following use case:
I have a manifest with one of the captions set as default, Russian language, but my player checks if user selected a text language before and tries to set it up when player loads.
I use English so my player loaded the manifest with the Russian default track set in the manifest and then immediately tried to switch to English, and then when I tried to select Russian again I saw that it’s vtt segments are never downloaded again.

In general, I think we should just be able to set initial audio/text/video preferences in the settings so initial load will be the one the application chooses.
This is without relation to what I described above, which seems to be a bug.

OrenMe · 2018-03-05T06:14:18Z

@johnBartos, were you able to review?

johnBartos · 2018-03-06T00:15:37Z

@OrenMe Sorry wasn't able to today, I will tomorrow

tchakabam · 2018-03-06T13:20:43Z

Hey dear contributors, we have recently applied linting rules to our code.

This might have created conflicts in your current WIP contribution.

No worries though, all the changes we did were applying consistent indent and style rules across the code :)

These can be applied automatically with the new lint:fix npm script (run npm run lint:fix).

But you still need to pull in changes from master to be able to merge this PR and formaly resolve any "conflicts".

To easily resolve that, simplest way will be a plain merge without rebase, resolving the conflicts in a "trivial" way by keeping "yours" and apply the auto-fixing tool on top of that, then committing the result :)

You can't do anything wrońg here: the CI output will be red if the diff of your PR causes linting errors :)

Step by step:

git fetch origin
git checkout my-pr-branch
git merge --strategy-option ours master

See: https://git-scm.com/docs/merge-strategies#merge-strategies-ours

Now you have resolved the conflicts by enforcing your change upon the conflicting ones on master branch.

Finally, run: npm run lint:fix.

Ideally, this should return with zero exit code, no errors (even if many warnings for now remaining).

If you have lint errors after the auto-fix script runs, you will have to manually fix these as they probably come from your changes :)

After this is done, and npm run lint terminates without errors, commit all the necessary fixes on your changes, and get a green build on CI without conflicts

Next destination, a super nicely readable and safely maintainable Hls.js codebase 👍

Thanks for your contribution!

EDIT: Finally-finally, please also check that you have only force-resolved conflicts that were related to linting, and not anything functional which has been done on master since your branch forked off. However that would show up in the PR diff anyhow here, and be subject of review - just a hint, as this is hypothetically also possible :)

johnBartos

Overall LGTM, just a few nitpicky things

johnBartos · 2018-03-06T23:32:35Z

src/controller/subtitle-stream-controller.js

+    // downloading its frags, if not all have been downloaded yet
+    const currentTrack = this.tracks[this.currentTrackId];
+    let details = currentTrack.details;
+    if (details !== undefined && details.live !== true) {


Why does it matter if we're live? I think a comment would be helpful here.

johnBartos · 2018-03-06T23:37:12Z

src/controller/timeline-controller.js

@@ -270,6 +270,12 @@ class TimelineController extends EventHandler {
    // Parse the WebVTT file contents.
    WebVTTParser.parse(payload, this.initPTS, vttCCs, frag.cc, function (cues) {
        const currentTrack = textTracks[frag.trackId];
+        // If text track is disabled in middle of process bailout as


This comment needs a bit of proofreading

@johnBartos I'm open to suggestions...

OrenMe · 2018-03-13T15:44:30Z

Hi @johnBartos, can you please check.

johnBartos · 2018-03-13T17:30:47Z

@OrenMe OK I just pulled the branch, will do a bit of manual testing

johnBartos

Just one last thing

johnBartos · 2018-03-14T01:32:22Z

src/controller/subtitle-stream-controller.js

+    // For live the tracks are reloaded anyhow every couple of sec so tick will be called than
+    const currentTrack = this.tracks[this.currentTrackId];
+    let details = currentTrack.details;
+    if (details !== undefined && details.live !== true)


The live check is unnecessary; the tick function debounces itself so that it's always called on the same cadence. Allowing it to be called if it's live is no problem

johnBartos

@OrenMe LGTM! Thanks for addressing the feedback

OrenMe · 2018-03-14T16:55:43Z

Thanks @johnBartos!

due to the way hls.js manages track selection, especialy when default text track is set in manifest, we had some incorrect logic. This simplfies and fixes the various paths we had to handle this. The fix is dependent on a fix on video-dev/hls.js#1582 and kaltura/playkit-js-hls#65.

OrenMe added 2 commits February 25, 2018 17:02

reset subtitle stream controller on track change

770f03d

If subtitles are changed in the middle of downloading another subtitle track then in certain edge cases the next subtitle track gets stuck and doesn't download fragments

make sure state machine is not stuck on text track change

73c93c7

johnBartos requested changes Mar 6, 2018

View reviewed changes

OrenMe added 2 commits March 13, 2018 17:10

Merge remote-tracking branch 'upstream/master' into fixTextTrackChange

9a41689

eslint fixes

335b8b1

johnBartos added this to the 0.8.10 milestone Mar 13, 2018

fix comment

5f93a70

johnBartos requested changes Mar 14, 2018

View reviewed changes

remove redundent live check

f59281b

johnBartos approved these changes Mar 14, 2018

View reviewed changes

johnBartos merged commit 7b92b1a into video-dev:master Mar 14, 2018

mangui changed the title ~~Fix text track change~~ Fix text track change edge cases Mar 22, 2018

OrenMe mentioned this pull request Jul 10, 2018

fix(FEC-7935): fix text selection logic with hlsjs kaltura/playkit-js#263

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix text track change edge cases #1582

Fix text track change edge cases #1582

OrenMe commented Feb 25, 2018 •

edited

Loading

johnBartos commented Feb 26, 2018

OrenMe commented Feb 27, 2018

OrenMe commented Feb 28, 2018 •

edited

Loading

johnBartos commented Mar 1, 2018

OrenMe commented Mar 1, 2018

OrenMe commented Mar 5, 2018 •

edited

Loading

johnBartos commented Mar 6, 2018

tchakabam commented Mar 6, 2018 •

edited

Loading

johnBartos left a comment

johnBartos Mar 6, 2018

johnBartos Mar 6, 2018

OrenMe Mar 13, 2018

OrenMe commented Mar 13, 2018

johnBartos commented Mar 13, 2018

johnBartos left a comment

johnBartos Mar 14, 2018

johnBartos left a comment

OrenMe commented Mar 14, 2018

Fix text track change edge cases #1582

Fix text track change edge cases #1582

Conversation

OrenMe commented Feb 25, 2018 • edited Loading

Description of the Changes

CheckLists

johnBartos commented Feb 26, 2018

OrenMe commented Feb 27, 2018

OrenMe commented Feb 28, 2018 • edited Loading

johnBartos commented Mar 1, 2018

OrenMe commented Mar 1, 2018

OrenMe commented Mar 5, 2018 • edited Loading

johnBartos commented Mar 6, 2018

tchakabam commented Mar 6, 2018 • edited Loading

johnBartos left a comment

Choose a reason for hiding this comment

johnBartos Mar 6, 2018

Choose a reason for hiding this comment

johnBartos Mar 6, 2018

Choose a reason for hiding this comment

OrenMe Mar 13, 2018

Choose a reason for hiding this comment

OrenMe commented Mar 13, 2018

johnBartos commented Mar 13, 2018

johnBartos left a comment

Choose a reason for hiding this comment

johnBartos Mar 14, 2018

Choose a reason for hiding this comment

johnBartos left a comment

Choose a reason for hiding this comment

OrenMe commented Mar 14, 2018

OrenMe commented Feb 25, 2018 •

edited

Loading

OrenMe commented Feb 28, 2018 •

edited

Loading

OrenMe commented Mar 5, 2018 •

edited

Loading

tchakabam commented Mar 6, 2018 •

edited

Loading