In-Manifest WebVTT Support #1057

mjneil · 2017-03-14T22:32:17Z

Description

This adds support for in-manifest VTT

Specific Changes proposed

uses PlaylistLoader to continuously refresh subtitle playlist in live playback
Adds VTTSegmentLoader for loading vtt segments
updates m3u8-parser to v2.1.0
uses vtt.js to parse X-TIMESTAMP-MAP header from vtt segments (Requires gkatsev/vtt.js#4)
supports using an init segment via EXT-X-MAP tag in the playlist

Requirements Checklist

Feature implemented / Bug fixed
Unit Tests updated or fixed
Docs/guides updated
Example created (starter template on JSBin)
Reviewed by Two Core Contributors

mjneil · 2017-03-15T14:42:03Z

src/master-playlist-controller.js

      }
    };

    this.setupSegmentLoaderListeners_();

-    this.masterPlaylistLoader_.start();
+    this.masterPlaylistLoader_.load();


From #1043
@imbcmdth: Just curious: why this was renamed?

@gesinger and I noticed that load calls start if start hasn't been called yet and is safe to call multiple times, but start should only be called once. Since this call is in the MasterPlaylistController constructor, theres no risk of running into that scenario, but we decided to just change any calls to start into load so we didn't have to concern ourselves with it. We can change it back if you think start makes more sense semantically

Sounds good to me. 👍

mjneil · 2017-03-15T14:43:21Z

src/master-playlist-controller.js

   * Call load on our SegmentLoaders
   */
  load() {
    this.mainSegmentLoader_.load();
    if (this.audioPlaylistLoader_) {
      this.audioSegmentLoader_.load();
    }
+    if (this.subtitlePlaylistLoader_) {
+      this.subtitleSegmentLoader_.load();


from #1043
@imbcmdth: At some point I feel like it might just be nice to have an array of active segment loaders. Then we can just do this.segmentLoaders_.forEach((l) => l.load());... not necessary now but maybe a good idea to think about soon.

mjneil · 2017-03-15T14:44:20Z

src/master-playlist-controller.js

+      track.mode = 'disabled';
+    }
+
+    this.setupSubtitles();


from #1043
@imbcmdth: If handleSubtitleError_ calls setupSubtitles is there a possibility that an infinite loop of failure will result?

It shouldn't since just above we set the current active tracks mode to 'disabled'. This has the same effect as turning off subtitles through the UI.

mjneil · 2017-03-15T14:45:25Z

src/master-playlist-controller.js

+      }
+
+      // reset the segment loader
+      this.subtitleSegmentLoader_.resetEverything();


from #1043
@imbcmdth: Why not move this to where the SegmentLoader is paused above. It's easier to reason about the state of the loader if it is manipulated in one place. Also, shouldn't it be reset even if there is no track?

setupSubtitles gets called everytime the player's TextTrackList fires a change event. When changing subtitle tracks through the player ui, depending on which track was selected and which track you are changing to, this event will fire multiple times. (it also fires a change event if you select the already active track in the UI) Only one of these calls will actually reset the playlist loader and segment loader to start loading from a new track. By only resetting when we are actually changing tracks, we avoid reseting the segment loader after its already started from the previous call, or if the user selected the already active track, then all that happens is the segment loader is paused and unpaused. Also, yes it should be reset even if there is no track, but since we always reset before starting a new track, it will get reset when the user turns subtitles back on.

I feel like a short version of this explanation needs to be in this function somewhere!

mjneil · 2017-03-15T14:46:26Z

src/vtt-segment-loader.js

+  }
+
+  timestampOffset() {
+    return this.syncController_.timestampOffsetForTimeline(this.currentTimeline_);


from #1043
@imbcmdth: Is this function used anywhere? If so, it needs a comment.

Well, is it? 😆

For the vtt-loader, no it isnt

mjneil · 2017-03-15T14:48:29Z

src/vtt-segment-loader.js

+
+      combinedSegment.set(initSegment.bytes);
+      combinedSegment.set(vttLineTerminators, initSegment.bytes.byteLength);
+      segment.map.bytes = combinedSegment;


from #1043
@imbcmdth: Couple of thoughts on this whole block:

We should pre-calculate the vttLineTerminators value because it's a typed array that never changes.

I think we should do the entire append process when we first get the response for the initSegment so that we the object stored in this.initSegments_[initId] already has the value that would be in combinedSegment.

Agree with both, good ideas

Do it! 😃

I think it will be better to do these changes when we integrate mediaSegmentRequest since that changes when the maps are stored in initSegments_

mjneil · 2017-03-15T14:48:54Z

src/vtt-segment-loader.js

+      this.parseVTTCues_(segmentInfo);
+    } catch (e) {
+      this.error({
+        message: e.message


from #1043
@imbcmdth: If error is fatal it should have some code set in addition to the message. The code is important for reporting reasons.

We decided not to do this now as we plan on revisiting how and when we emit errors at some point in the near future.

mjneil · 2017-03-15T14:49:31Z

src/vtt-segment-loader.js

+    const midPoint = (firstStart + lastStart) / 2;
+
+    segment.start = midPoint - (segment.duration / 2);
+    segment.end = midPoint + (segment.duration / 2);


from #1043
@imbcmdth: Everything about this feels wrong and dirty! 😱

We decided to try not storing segment start and end times and letting the conservative guess make it work since web vtt segments should be much smaller than video or audio segments.

One thing to be careful about is that the "resync" functionality needs to be removed since it will continuously reset the segment loader on poor guess instead of just walking forward.

the resync functionality actually checks for the existence of segment.end so we are safe leaving it here if we never set it

mjneil · 2017-03-15T22:58:19Z

utils/stats/index.html

@@ -56,7 +56,7 @@
    <br>
    <label>
      Player Options:
-      <input id="player-options" type="text" value='{}'>
+      <input id="player-options" type="text" value='{"vtt.js": "PATH/TO/vtt.js"}'>


reminder to self to revert back before releasing

imbcmdth · 2017-03-15T23:05:17Z

src/vtt-segment-loader.js

-        this.mediaSource_.readyState === 'ended' &&
-        !this.seeking_()) {
+      this.syncController_.on('timestampoffset', checkTimestampOffset);
+      this.state = 'WAITING_ON_TIMELINE';


What happens if we switch subtitles or seek while the state is 'WAITING_ON_TIMELINE'?

While the state is not WAITING any aborts will just clear pending segment (which at this point there isn't one yet) and does nothing else. The callback will set the state to ready and call monitorBuffer_ to restart the lifecycle. Since it starts back from the beginning, any seeks or subtitle changes that happened in the waiting time shouldn't have adverse effects

imbcmdth · 2017-03-15T23:06:11Z

src/vtt-segment-loader.js

+        this.handleSegment_();
+      };
+
+      this.state = 'WAITING_ON_VTTJS';


What happens if we switch subtitles or seek while the state is 'WAITING_ON_VTTJS'?

This is basically the same as aborting while waiting in the DECRYPTING stage, the callback will call handleSegment_ again and if it was aborted while we were waiting, the first if block will reset the loader to READY

imbcmdth · 2017-03-15T23:20:02Z

src/vtt-segment-loader.js

+      };
+
+      this.state = 'WAITING_ON_VTTJS';
+      this.subtitlesTrack_.tech_.on('vttjsloaded', loadHandler);


This seems like the perfect case to use this.subtitlesTrack_.tech_.one('vttjsloaded', ...)

imbcmdth · 2017-03-15T23:20:29Z

src/vtt-segment-loader.js

-    if (segmentInfo.mediaIndex === this.playlist_.segments.length - 1 &&
-        this.mediaSource_.readyState === 'ended' &&
-        !this.seeking_()) {
+      this.syncController_.on('timestampoffset', checkTimestampOffset);


This seems like the perfect case to use this.syncController_.one('timestampoffset', ...)

imbcmdth · 2017-03-15T23:46:32Z

test/segment-loader.test.js

@@ -403,7 +403,7 @@ QUnit.test('only appends one segment at a time', function(assert) {
  assert.equal(loader.mediaRequests, 1, '1 request');
 });

-QUnit.test('adjusts the playlist offset if no buffering progress is made', function(assert) {
+QUnit.skip('adjusts the playlist offset if no buffering progress is made', function(assert) {


Why was this test skipped?

This test is actually outdated from before simple-fetcher times when there was the offset correction variable to handle incorrect guesses, (now we just walk forward), it just wasnt skipped/removed previously because it happened to pass after segment loader changes. If you notice the other playlist offset tests are also skipped. I was planning on cleaning up skipped tests when the loaders get refactored

…subtitles

…le data

…tadata

* In-Manifest WebVTT Support (#1057) * Minor loader fixes (#1065)

mjneil assigned mjneil and gesinger Mar 14, 2017

mjneil mentioned this pull request Mar 14, 2017

In-Manifest WebVTT Support (WIP) #1043

Closed

5 tasks

mjneil commented Mar 15, 2017

View reviewed changes

imbcmdth reviewed Mar 15, 2017

View reviewed changes

mjneil force-pushed the feature/webvtt branch from 4492067 to ab9a398 Compare March 20, 2017 16:37

mjneil and others added 12 commits March 20, 2017 13:50

added vtt playlist loader

4220a0b

start of vtt-segment-loader, request segment and parse cues

29d7487

Add cues when handling subtitle segment and clear cues when changing …

0a5fc1e

…subtitles

Don't call endOfStream from vtt segment loader

75bf022

Move creation of new subtitles playlist loader on URI change

0c788ba

Reset subtitle segment loader only when using new subtitles

06a6c65

Fill out buffered function for vtt segment loader

5fdf933

Wait for video timestamp offsets in vtt segment loader

7b156bc

Fix some loads and disposes for subtitle segment loader

69b5e5e

Don't use a default subtitle track

739ba90

Fix some subtitle playlist and segment loader pausing and clearing

39741d0

Fill out vtt segment loader's remove function

e7103e3

mjneil and others added 21 commits March 20, 2017 14:17

fix vtt loader waiting on timeline and vttjs

fa021d4

test vtt segment time mapping

1791581

add vttjs error handling

5a5cfe5

Removed unused subtitleupdate event

bb96ff8

Add test for adding subtitle tracks when media playlist is loaded

3907bc9

Add test for switching off subtitles on subtitle errors

9941345

Add test for pausing subtitle loader on tech error

ab2fbfc

Add test for disposing subtitle segment and playlist loaders

379bfc4

Add test for resetting subtitle segment loader on seeks

32dd51c

Add test for getting active subtitle group

9b78a9b

Add test for getting active subtitle track

443be25

Add test for handling subtitle errors from master playlist controller

1afd470

Add test for subtitles setup

5102875

update readme

178fef2

remove forced logging from vtt-loader

46ffa3e

Add master-subtitles.m3u8 manifest for tests

f850fdf

ignore: copy segment-loader files for diffing

47fcb24

merge fixes

85aba5e

prevent infinitely requesting same segment when segment has no subtit…

f66f3f1

…le data

Fix MasterPlaylistController subtitle tests by considering segment-me…

047083c

…tadata

ignore: fix missing semicolon

514b06b

mjneil force-pushed the feature/webvtt branch from 8b1b97c to a4f377b Compare March 20, 2017 18:53

ignore: rebase fix

4dbe477

mjneil force-pushed the feature/webvtt branch from a4f377b to 4dbe477 Compare March 20, 2017 18:59

put init segment getting/setting into function

fed4daa

mjneil force-pushed the feature/webvtt branch from 0d9a098 to fed4daa Compare March 20, 2017 20:03

remove path to vttjs

ebba86d

imbcmdth merged commit 28bbd57 into videojs:feature/webvtt Mar 20, 2017

mjneil mentioned this pull request Mar 21, 2017

WebVTT Cleanup #1059

Merged

imbcmdth added a commit that referenced this pull request Mar 31, 2017

Finally, WebVTT (#1064)

a7ec735

* In-Manifest WebVTT Support (#1057) * Minor loader fixes (#1065)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

In-Manifest WebVTT Support #1057

In-Manifest WebVTT Support #1057

mjneil commented Mar 14, 2017

mjneil Mar 15, 2017

mjneil Mar 15, 2017

imbcmdth Mar 15, 2017

mjneil Mar 15, 2017 •

edited

mjneil Mar 15, 2017

mjneil Mar 15, 2017

mjneil Mar 15, 2017

mjneil Mar 15, 2017

imbcmdth Mar 15, 2017

mjneil Mar 15, 2017

imbcmdth Mar 15, 2017

mjneil Mar 15, 2017

mjneil Mar 15, 2017

mjneil Mar 15, 2017

imbcmdth Mar 15, 2017

mjneil Mar 16, 2017

mjneil Mar 15, 2017

imbcmdth Mar 15, 2017

mjneil Mar 15, 2017

imbcmdth Mar 15, 2017

mjneil Mar 15, 2017

mjneil Mar 15, 2017

imbcmdth Mar 15, 2017 •

edited

mjneil Mar 16, 2017

imbcmdth Mar 15, 2017 •

edited

mjneil Mar 16, 2017

imbcmdth Mar 15, 2017

imbcmdth Mar 15, 2017

imbcmdth Mar 15, 2017

mjneil Mar 16, 2017

In-Manifest WebVTT Support #1057

In-Manifest WebVTT Support #1057

Conversation

mjneil commented Mar 14, 2017

Description

Specific Changes proposed

Requirements Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjneil Mar 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imbcmdth Mar 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imbcmdth Mar 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjneil Mar 15, 2017 •

edited

imbcmdth Mar 15, 2017 •

edited

imbcmdth Mar 15, 2017 •

edited