New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unused tracks from track list when changing source #3002

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
3 participants
@nickygerritsen
Contributor

nickygerritsen commented Jan 14, 2016

This should fix #3000 . When using native text tracks it removes unused ones on loadstart.

@gkatsev suggested using inbandmetadatatrack but that seems to be "" for my HLS subtitle tracks so I guess we can not use it.

Show outdated Hide outdated src/js/tech/html5.js
@@ -282,6 +293,33 @@ class Html5 extends Tech {
this.textTracks().removeTrack_(e.track);
}
removeOldTextTracks() {

This comment has been minimized.

@gkatsev

gkatsev Feb 3, 2016

Member

Make sure you consistently call this removeOldTextTracks_.

@gkatsev

gkatsev Feb 3, 2016

Member

Make sure you consistently call this removeOldTextTracks_.

This comment has been minimized.

@nickygerritsen

nickygerritsen May 3, 2016

Contributor

Hmmm I kinda copied this from the handle... functions. Why are they not postfixed with _ but this one should be? As far as I know they actually should all be underscored?

@nickygerritsen

nickygerritsen May 3, 2016

Contributor

Hmmm I kinda copied this from the handle... functions. Why are they not postfixed with _ but this one should be? As far as I know they actually should all be underscored?

This comment has been minimized.

@gkatsev

gkatsev May 3, 2016

Member

Oh I see, I must've missed that, I guess carry on...

@gkatsev

gkatsev May 3, 2016

Member

Oh I see, I must've missed that, I guess carry on...

Show outdated Hide outdated src/js/tech/html5.js
@@ -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);

This comment has been minimized.

@gkatsev

gkatsev Feb 3, 2016

Member

missing the _.

@gkatsev

gkatsev Feb 3, 2016

Member

missing the _.

This comment has been minimized.

@nickygerritsen

nickygerritsen May 3, 2016

Contributor

Why is this Fn.bind dance here anyway btw? I'm wondering if it is needed for removeOldTextTracks_?

@nickygerritsen

nickygerritsen May 3, 2016

Contributor

Why is this Fn.bind dance here anyway btw? I'm wondering if it is needed for removeOldTextTracks_?

}
}
for (let i = 0; i < removeTracks.length; i++) {

This comment has been minimized.

@gkatsev

gkatsev Feb 3, 2016

Member
removeTracks.forEach((track) => this.textTracks().removeTrack_(track))

But really doesn't have to change.

@gkatsev

gkatsev Feb 3, 2016

Member
removeTracks.forEach((track) => this.textTracks().removeTrack_(track))

But really doesn't have to change.

Show outdated Hide outdated src/js/tech/html5.js
let removeTracks = [];
for (let i = 0; i < this.textTracks().length; i++) {
let techTrack = this.textTracks()[i];

This comment has been minimized.

@gkatsev

gkatsev Feb 3, 2016

Member

we probably should cache this.textTracks() in a variable outside the forloops so we don't have to make the look up each time.

@gkatsev

gkatsev Feb 3, 2016

Member

we probably should cache this.textTracks() in a variable outside the forloops so we don't have to make the look up each time.

This comment has been minimized.

@nickygerritsen

nickygerritsen May 3, 2016

Contributor

You mean like

const textTracks = this.textTracks();

?

@nickygerritsen

nickygerritsen May 3, 2016

Contributor

You mean like

const textTracks = this.textTracks();

?

This comment has been minimized.

@gkatsev

gkatsev May 3, 2016

Member

yep

Show outdated Hide outdated src/js/tech/html5.js
let found = false;
for (let j = 0; j < this.el().textTracks.length; j++) {
if (this.el().textTracks[j] === techTrack) {

This comment has been minimized.

@gkatsev

gkatsev Feb 3, 2016

Member

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.

@gkatsev

gkatsev Feb 3, 2016

Member

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.

This comment has been minimized.

@nickygerritsen

nickygerritsen May 3, 2016

Contributor

And with this you mean caching this.el().textTracks in some variable / constant and check if this.el().textTracks is null?

@nickygerritsen

nickygerritsen May 3, 2016

Contributor

And with this you mean caching this.el().textTracks in some variable / constant and check if this.el().textTracks is null?

This comment has been minimized.

@gkatsev

gkatsev May 3, 2016

Member

yep

Show outdated Hide outdated src/js/tech/html5.js
// If not, they will be removed from the emulated list
let removeTracks = [];
for (let i = 0; i < this.textTracks().length; i++) {

This comment has been minimized.

@gkatsev

gkatsev Feb 3, 2016

Member
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.

@gkatsev

gkatsev Feb 3, 2016

Member
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.

This comment has been minimized.

@nickygerritsen

nickygerritsen May 3, 2016

Contributor

I kinda do like the other short version, but this one isn't really readable anymore 😋

@nickygerritsen

nickygerritsen May 3, 2016

Contributor

I kinda do like the other short version, but this one isn't really readable anymore 😋

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Apr 29, 2016

Member

This needs to be rebased plus there's a few minor comments that need to be addressed.

Member

gkatsev commented Apr 29, 2016

This needs to be rebased plus there's a few minor comments that need to be addressed.

@nickygerritsen

This comment has been minimized.

Show comment
Hide comment
@nickygerritsen

nickygerritsen May 3, 2016

Contributor

Totally, completely forgot about this PR. I'll add some replies above so we can tackle this one.

Contributor

nickygerritsen commented May 3, 2016

Totally, completely forgot about this PR. I'll add some replies above so we can tackle this one.

@nickygerritsen

This comment has been minimized.

Show comment
Hide comment
@nickygerritsen

nickygerritsen May 4, 2016

Contributor

I assume we do not have to remove video/audio tracks, as these are never native?

Contributor

nickygerritsen commented May 4, 2016

I assume we do not have to remove video/audio tracks, as these are never native?

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev May 4, 2016

Member

I think we might need to since we do listen to the addtrack on the native audio and video tracks as well. Thoughts, @BrandonOCasey?

Member

gkatsev commented May 4, 2016

I think we might need to since we do listen to the addtrack on the native audio and video tracks as well. Thoughts, @BrandonOCasey?

@BrandonOCasey

This comment has been minimized.

Show comment
Hide comment
@BrandonOCasey

BrandonOCasey May 4, 2016

Contributor

I think Safari has support for native audio and video tracks so I think we will want to.

Contributor

BrandonOCasey commented May 4, 2016

I think Safari has support for native audio and video tracks so I think we will want to.

@nickygerritsen

This comment has been minimized.

Show comment
Hide comment
@nickygerritsen

nickygerritsen May 4, 2016

Contributor

OK then I'll try to mimic your way of doing this for all three track kinds.

Contributor

nickygerritsen commented May 4, 2016

OK then I'll try to mimic your way of doing this for all three track kinds.

@nickygerritsen

This comment has been minimized.

Show comment
Hide comment
@nickygerritsen

nickygerritsen May 6, 2016

Contributor

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.

Contributor

nickygerritsen commented May 6, 2016

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.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev May 11, 2016

Member

LGTM

Member

gkatsev commented May 11, 2016

LGTM

@gkatsev gkatsev added the confirmed label May 11, 2016

@gkatsev gkatsev closed this in 9f37a64 Jun 28, 2016

@@ -94,6 +94,9 @@ class Html5 extends Tech {
tl.addEventListener('change', Fn.bind(this, this[`handle${capitalType}TrackChange_`]));
tl.addEventListener('addtrack', Fn.bind(this, this[`handle${capitalType}TrackAdd_`]));
tl.addEventListener('removetrack', Fn.bind(this, this[`handle${capitalType}TrackRemove_`]));
// Remove (native) trackts that are not used anymore

This comment has been minimized.

@gkatsev

gkatsev Aug 12, 2016

Member

oops, just realized this comment has tracks misspelt.

@gkatsev

gkatsev Aug 12, 2016

Member

oops, just realized this comment has tracks misspelt.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment