Integrate video & audio tracks with video.js #3173
Conversation
| videoTracks() { | ||
| // cannot use techGet_ directly because it checks to see whether the tech is ready. | ||
| // Flash is unlikely to be ready in time but videoTracks should still work. | ||
| return this.tech_ && this.tech_['videoTracks'](); |
There was a problem hiding this comment.
This will return undefined when the tech is not ready. The spec says it should return an empty VideoTrackList in that case which seems nice if it's easy to pull off.
|
@benjipott would be great to get your input on this PR! |
| @@ -70,6 +72,13 @@ class Html5 extends Tech { | |||
| } | |||
| } | |||
|
|
|||
| if (this.featuresNativeVideoTracks) { | |||
| this.handleVideoTrackChange_ = Fn.bind(this, this.handleVideoTrackChange); | |||
There was a problem hiding this comment.
This is inline with how the native text tracks are done. The reason for them is that in one vendor, the functions weren't being properly bound if we were using this.handle<Thing>Track<Action> directly. Binding them here and then using the bound version in the listener fixes it.
There was a problem hiding this comment.
I'm still confused :) Why weren't they bound properly? By "vendor", do you mean the HTML tech or Flash tech for example?
There was a problem hiding this comment.
Vendor like safari, unfortunately, I don't remember the exact details. Was merged as part of #2425.
There was a problem hiding this comment.
I don't like the fact we don't remember why we have to do this weird dance here. @brandonocasey would you mind giving this a shot without the binding thing in Safari?
There was a problem hiding this comment.
I would be wary to change how it works for the text tracks themselves but OK with not doing it for the new video and audio track support.
merged track dispose code in html5
use forEach rather than for
only trigger one change event per enabled/selected toggle
empty audio/video track list when getting a new source while we have one already
changed emptyTrackList_ to public function clearTracks added notes on the usage of clearTracks for Techs with no source handler
|
LGTM |
dmlap
added
needs: LGTM
|
LGTM |
misteroneill
removed
in progress
needs: LGTM
|
And tests passed in browserstack: https://travis-ci.org/videojs/video.js/builds/125066414 |
Description
Implement
VideoTrack,AudioTrack,VideoTrackList, andAudioTrackListusing common functionality fromTextTrackandTextTrackList. Which is now shared in a base classes calledTrackandTrackListSpecific Changes proposed
Requirements Checklist