Support for multiple CEA608 tracks

[squarebracket]

This change supports the work done in videojs/mux.js#150. However, it should be backwards compatible with older mux.js versions.

Rather than sourceBuffer.inbandTextTrack_ being a single object, it is now sourceBuffer.inbandTextTracks_, which is a map that stores track => object. When new captions come in, caption.track is inspected, and cues are added to the appropriate track.

[mjneil]

Why are tracks with kind subtitles also removed here?

[squarebracket]

I'm not sure why I did this. I'm assuming subtitle tracks are cleaned elsewhere?

[mjneil]

They are supposed to be (this comment made me realize I've been using the addRemoteTextTrack api wrong all this time as they aren't actually getting properly cleaned). The subtitle tracks for webvtt are taken care of in MasterPlaylistController of contrib-hls.

[squarebracket]

Yeah, that makes a lot more sense from what I know now about the architecture. I'll chock this one up to being a n00b mistake :)

[mjneil]

This will have to loop through every caption of every segment loaded even after the text tracks have been created and added to the player. Though probably not a big deal, seems like a lot of wasted computation. Is there anyway we can attach to the transmuxers output the information of what streams the captions array uses so you can check whether theyve already been made without having to loop through the entire captions array again?

[squarebracket]

I'm not 100% sure I follow, so please do correct me if I've misunderstood. You're suggesting that the transmuxer keeps a local record of the tracks on which it's seen captions, and dispatches that information to media-sources? Like, say, when it dispatches a caption for a "new" track, it will set something like caption.new = true, which media-sources would then see and know it needs to create a new remoteTextTrack for that caption's track value?

That's certainly doable, and I agree that this loop on every transmuxer push just feels wasteful. I'm not sure keeping state in the transmuxer is necessary, though. video.js historians might be more knowledgeable on this subject, but I'd hazard a guess that this code is only here anyway because the code in videojs/videojs-contrib-hls#1096 hasn't existed -- leaving this "track detection" code as the sole method for detecting and creating text tracks for CEA608 captions.

Perhaps some sort of logic could be included in videojs/videojs-contrib-hls#1096, something like, if closed captions are advertised in the playlist, then this code here would not be run, and we'd trust that all necessary caption tracks were declared in the playlist. If no text track exists, then we throw them into the void. And if captions aren't declared, but actually exist in the segments (I hope that is rare), then this loop would run.

That's potentially a breaking change from previous behaviour though, so please do let me know what you think.

[mjneil]

Something along those lines, just a matter of where and how to communicate that information. You're definitely right that this code exists because we haven't had a previous way of detecting which channels were in use until after we've transmuxed a segment. Since the CC tag information is optional in the manifest, having the detection and track creation here is still important. Trusting the manifest for optional information is usually a recipe for issues down the road. I don't think the muxer would need to hold extra state as the information is already there in the captions. The transmuxer itself already needs to loop through the captions to adjust start and end times (https://github.com/videojs/mux.js/blob/master/lib/mp4/transmuxer.js#L973). This is also the last point that the transmuxer sees the captions before it sends it back to here. I think it would be reasonable to attach a bit of extra information to the event variable. Maybe something like event.captionStreams (or a more appropriate name) (Note: this would also have to be done in the CoalesceStream of the FLV' as well)

event.captionStreams = {
  CC1: false,
  CC2: false,
  CC3: false,
  CC4: false
};

and set the bool to true when you encounter it during that loop. Then instead of looping through the captions again here, you can just check segment.captionStreams and compare that to available tracks to determine if one needs to be made. I think we can avoid any breaking changes with this approach

[squarebracket]

Yeah fair enough, I ctrl+f'd around the spec last night looking for anything that might suggest EXT-X-MEDIA was marked as a MUST for caption rendering but didn't see anything.

I'll code up something when I get a minute.

[squarebracket]

For some reason, I'm still getting a failure on a flash test. Printing out the event data in createTextTracksIfNecessary, I don't see captionTracks. Any idea why that might be? I've added the required stuff into the FLV transmuxer in mux.js....

[mjneil]

I assume you mean one of the unit tests? The tests in this project pretty much entirely mock the output of the transmuxer. I suspect you need to edit this function to take some extra information and attach captionTracks to its return value.

[squarebracket]

Ah, yeah, you're right. I saw mux.js being pulled in, but didn't see that you were overwriting certain functions :\

Ok, so I think everything is good now... although Firefox keeps dying in the Travis tests for some reason?

[mjneil]

Let's move this into the if block below. That way it will only go looking for the track on the player if the source buffer doesn't already have a inbandTextTrack stored for the trackId

[squarebracket]

Yup, great point, done.

[mjneil]

I restarted the travis test. Looks like Firefox kept timing out. Usually re-running the test resolves that

edit: This might be travis. I restarted the build for 4.4.8, which did pass travis 24 days ago when it was released, but now it fails in the same way, firefox disconnecting over for each build

[squarebracket]

Firefox v55 was just released like a couple days ago, wasn't it? Maybe something is breaking there. I'm still running v54 and it's working for me.

[mjneil]

Oh that could be related. I'm also on 54 locally, I'll try updating and see if that causes failure locally. If not I'll probably open in issue with TravisCI

[mjneil]

FF55 locally still passing. submitted travis-ci/travis-ci#8242 we'll see what they say

[mjneil]

I just pushed to master a change to travis.yml to lock firefox at v54. Can you please rebase on top of master, it looks like restarting this travis build it is still picking up latest instead of 54

[squarebracket]

Done.

[mjneil]

Now that we are letting contrib-hls create tracks for CC and this else block will only be entered if there is no text track on the player with trackId, I think this line is no longer needed as there won't be an existing track with trackId to remove.

[squarebracket]

Question: if I'm adding the CC tracks in contrib-hls, should I also be removing them in contrib-hls when the source changes?

[mjneil]

Good call. We shouldn't have to worry about removing them if we pass false as the second parameter to addRemoteTextTrack. I made a comment on your contrib-hls PR noting this.

[squarebracket]

Right, cool. Yeah in that case we should never have a situation where we have a CC1 text track that has the wrong language/label.

Removing it caused this test to fail, which makes sense. Think it's safe to just adapt the test, will deleting the removeExistingTrack have other possible side effects?

[mjneil]

Ya I think you should be safe just adapting the test. I don't think removing the call to removeExistingTrack here will have other side effects because of the way the if statements are laid out.

[squarebracket]

Huh. Can you kick off that Travis test again?

Edit: n/m found a text error, pushing again.

[mjneil]

Can you update package.json to use v4.2.0 of mux.js

[squarebracket]

Done.