Remove unused tracks from track list when changing source #3002
3 participants
gkatsev
and 1 other
commented on an outdated diff
| @@ -282,6 +293,33 @@ class Html5 extends Tech { | ||
| this.textTracks().removeTrack_(e.track); | ||
| } | ||
| + removeOldTextTracks() { |
|
Make sure you consistently call this Hmmm I kinda copied this from the Oh I see, I must've missed that, I guess carry on...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
gkatsev
and 1 other
commented on an outdated diff
| @@ -74,6 +74,7 @@ class Html5 extends Tech { | ||
| this.handleTextTrackChange_ = Fn.bind(this, this.handleTextTrackChange); | ||
| this.handleTextTrackAdd_ = Fn.bind(this, this.handleTextTrackAdd); | ||
| this.handleTextTrackRemove_ = Fn.bind(this, this.handleTextTrackRemove); | ||
| + this.removeOldTextTracks_ = Fn.bind(this, this.removeOldTextTracks); |
|
Why is this
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
| + let techTrack = this.textTracks()[i]; | ||
| + | ||
| + let found = false; | ||
| + for (let j = 0; j < this.el().textTracks.length; j++) { | ||
| + if (this.el().textTracks[j] === techTrack) { | ||
| + found = true; | ||
| + break; | ||
| + } | ||
| + } | ||
| + | ||
| + if (!found) { | ||
| + removeTracks.push(techTrack); | ||
| + } | ||
| + } | ||
| + | ||
| + for (let i = 0; i < removeTracks.length; i++) { |
removeTracks.forEach((track) => this.textTracks().removeTrack_(track))But really doesn't have to change.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
gkatsev
and 1 other
commented on an outdated diff
| @@ -282,6 +293,33 @@ class Html5 extends Tech { | ||
| this.textTracks().removeTrack_(e.track); | ||
| } | ||
| + removeOldTextTracks() { | ||
| + // This will loop over the texttracks and check if they are still used | ||
| + // If not, they will be removed from the emulated list | ||
| + let removeTracks = []; | ||
| + | ||
| + for (let i = 0; i < this.textTracks().length; i++) { | ||
| + let techTrack = this.textTracks()[i]; |
|
we probably should cache
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
gkatsev
and 1 other
commented on an outdated diff
| @@ -282,6 +293,33 @@ class Html5 extends Tech { | ||
| this.textTracks().removeTrack_(e.track); | ||
| } | ||
| + removeOldTextTracks() { | ||
| + // This will loop over the texttracks and check if they are still used | ||
| + // If not, they will be removed from the emulated list | ||
| + let removeTracks = []; | ||
| + | ||
| + for (let i = 0; i < this.textTracks().length; i++) { | ||
| + let techTrack = this.textTracks()[i]; | ||
| + | ||
| + let found = false; | ||
| + for (let j = 0; j < this.el().textTracks.length; j++) { | ||
| + if (this.el().textTracks[j] === techTrack) { |
|
we should also probably cache a reference to the native text track and possibly also do a nullcheck around it as well. Since it is conceivably possible to have an html5 browser that doesn't have text track support. And with this you mean caching
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
gkatsev
and 1 other
commented on an outdated diff
| @@ -282,6 +293,33 @@ class Html5 extends Tech { | ||
| this.textTracks().removeTrack_(e.track); | ||
| } | ||
| + removeOldTextTracks() { | ||
| + // This will loop over the texttracks and check if they are still used | ||
| + // If not, they will be removed from the emulated list | ||
| + let removeTracks = []; | ||
| + | ||
| + for (let i = 0; i < this.textTracks().length; i++) { |
let tt = this.textTracks();
let trackIndexOf = Array.prototype.indexOf.call.bind(this.el().textTracks);
Array.prototype.filter.call(tt,
(ourTrack) => trackIndexOf(ourTracks) === -1)
.forEach((trackToRemove) => tt.removeTrack_(trackToRemove));Why? Just felt like it. No change necessary. I kinda do like the other short version, but this one isn't really readable anymore
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
This needs to be rebased plus there's a few minor comments that need to be addressed.
Totally, completely forgot about this PR. I'll add some replies above so we can tackle this one.
I assume we do not have to remove video/audio tracks, as these are never native?
I think we might need to since we do listen to the addtrack on the native audio and video tracks as well. Thoughts, @brandonocasey?
I think Safari has support for native audio and video tracks so I think we will want to.
OK then I'll try to mimic your way of doing this for all three track kinds.
OK I think this does the trick. I removed all the Fn.bind goodness, as Events.on already does this. this also gets rid of a "strange" dance with the underscored and non-underscored version of the function.
This should fix #3000 . When using native text tracks it removes unused ones on
loadstart.@gkatsev suggested using
inbandmetadatatrackbut that seems to be""for my HLS subtitle tracks so I guess we can not use it.