Skip to content
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

Add option to override native audio and video to html5 tech #5074

Merged
merged 17 commits into from
Apr 19, 2018
82 changes: 67 additions & 15 deletions src/js/tech/html5.js
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,40 @@ class Html5 extends Tech {
});
}

/**
* Attempt to force override of native video tracks.
*
* @param {Boolean} override - If set to true native video will be overridden,
* otherwise native video will potentially be used.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be good to update this to mention that this is just for audio and video tracks

*/
overrideNativeTracks(override) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be good to make this a no-op if we aren't changing the override.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should add either an empty method or a default implementation in tech.js for this.

const removeTracks = (trackType) => {
const props = TRACK_TYPES[trackType];
const elTracks = this.el()[props.getterName];

if (this.trackListeners[props.capitalName]) {
this.trackListeners[props.capitalName].forEach(trackListener => {
elTracks.removeEventListener(trackListener.eventName, trackListener.listener);
});
}
};

this.featuresNativeVideoTracks = !override;
this.featuresNativeAudioTracks = !override;

if (!this.trackListeners) {
this.trackListeners = [];
}

removeTracks('video');
removeTracks('audio');

this.trackListeners.Video = [];
this.trackListeners.Audio = [];

this.proxyNativeTracks_();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so basically, everything above will remove listeners and then we call proxyNativeTracks_ so that if we turn native back on it'll run but if we turned it off, proxyNativeTracks_ will exit early and not run?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds right to me.

}

/**
* Proxy all native track list events to our track lists if the browser we are playing
* in supports that type of track list.
Expand All @@ -217,22 +251,26 @@ class Html5 extends Tech {
!elTracks.addEventListener) {
return;
}
const listeners = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be good to revert this object's change to make the PR more streamlined.

change(e) {
techTracks.trigger({
type: 'change',
target: techTracks,
currentTarget: techTracks,
srcElement: techTracks
});
},
addtrack(e) {
techTracks.addTrack(e.track);
},
removetrack(e) {
techTracks.removeTrack(e.track);
}

const listeners = {};

listeners.change = (e) => {
techTracks.trigger({
type: 'change',
target: techTracks,
currentTarget: techTracks,
srcElement: techTracks
});
};

listeners.addtrack = (e) => {
techTracks.addTrack(e.track);
};

listeners.removetrack = (e) => {
techTracks.removeTrack(e.track);
};

const removeOldTracks = function() {
const removeTracks = [];

Expand Down Expand Up @@ -260,6 +298,16 @@ class Html5 extends Tech {
const listener = listeners[eventName];

elTracks.addEventListener(eventName, listener);

if (!this.trackListeners) {
this.trackListeners = [];
}

if (!this.trackListeners[props.capitalName]) {
this.trackListeners[props.capitalName] = [];
}

this.trackListeners[props.capitalName].push({ eventName, listener });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about instead of making an array of stuff, do something like the following outside of this Object.keys().forEach:

this[props.getterName + 'Listeners_'] = listeners;

then to remove these, outside of this method we can do something like:

Object.keys(this.audioTracksListeners_).forEach((eventName) => {
  elTracks.removeEventListener(eventName, this.audioTracksListeners_[eventName]
});
Object.keys(this.videoTracksListeners_).forEach((eventName) => {
  elTracks.removeEventListener(eventName, this.videoTracksListeners_[eventName]
});

this.on('dispose', (e) => elTracks.removeEventListener(eventName, listener));
});

Expand Down Expand Up @@ -613,6 +661,10 @@ class Html5 extends Tech {
this.setSrc(src);
}

setSrc(src) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this shouldn't be necessary

super.setSrc(src);
}

/**
* Reset the tech by removing all sources and then calling
* {@link Html5.resetMediaElement}.
Expand Down
2 changes: 1 addition & 1 deletion src/js/tech/tech.js
Original file line number Diff line number Diff line change
Expand Up @@ -1218,7 +1218,7 @@ Tech.withSourceHandlers = function(_Tech) {
if (_Tech.nativeSourceHandler) {
sh = _Tech.nativeSourceHandler;
} else {
log.error('No source hander found for the current source.');
log.error('No source handler found for the current source.');
}
}

Expand Down
86 changes: 86 additions & 0 deletions test/unit/tech/html5.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -535,6 +535,49 @@ if (Html5.supportsNativeAudioTracks()) {
assert.equal(adds[1][0], rems[1][0], 'addtrack event handler removed');
assert.equal(adds[2][0], rems[2][0], 'removetrack event handler removed');
});

QUnit.test('should use overrideNativeTracks on audio correctly', function(assert) {
assert.expect(6);

const adds = [];
const rems = [];
const at = {
length: 0,
addEventListener: (type, fn) => {
adds.push({ type, fn });
},
removeEventListener: (type, fn) => {
rems.push({ type, fn });
}
};

const el = document.createElement('div');

el.audioTracks = at;

const htmlTech = new Html5({el});

assert.equal(adds.length, 3,
'should have added change, remove, add listeners');
assert.equal(rems.length, 0,
'no listeners should be removed');

htmlTech.overrideNativeTracks(true);

assert.equal(adds.length, 3,
'should not have added additional listeners');
assert.equal(rems.length, 3,
'should have removed previous three listeners');

htmlTech.overrideNativeTracks(false);

assert.equal(adds.length, 6,
'should have added 3 listeners back');
assert.equal(rems.length, 3,
'no listeners should be removed');

htmlTech.dispose();
});
}

if (Html5.supportsNativeVideoTracks()) {
Expand Down Expand Up @@ -603,6 +646,49 @@ if (Html5.supportsNativeVideoTracks()) {
assert.equal(adds[1][0], rems[1][0], 'addtrack event handler removed');
assert.equal(adds[2][0], rems[2][0], 'removetrack event handler removed');
});

QUnit.test('should use overrideNativeTracks on video correctly', function(assert) {
assert.expect(6);

const adds = [];
const rems = [];
const vt = {
length: 0,
addEventListener: (type, fn) => {
adds.push({ type, fn });
},
removeEventListener: (type, fn) => {
rems.push({ type, fn });
}
};

const el = document.createElement('div');

el.videoTracks = vt;

const htmlTech = new Html5({el});

assert.equal(adds.length, 3,
'should have added change, remove, add listeners');
assert.equal(rems.length, 0,
'no listeners should be removed');

htmlTech.overrideNativeTracks(true);

assert.equal(adds.length, 3,
'should not have added additional listeners');
assert.equal(rems.length, 3,
'should have removed previous three listeners');

htmlTech.overrideNativeTracks(false);

assert.equal(adds.length, 6,
'should have added 3 listeners back');
assert.equal(rems.length, 3,
'no listeners should be removed');

htmlTech.dispose();
});
}

QUnit.test('should always return currentSource_ if set', function(assert) {
Expand Down