setSrc clears currentSource_ after loadstart #3285

Closed
wants to merge 8 commits into
from

Projects

None yet

4 participants

@incompl
Contributor
incompl commented Apr 26, 2016 edited

Description

currentSrc will now report correct source after the video source was changed by a third party.

This PR replaces #2957

Specific Changes proposed

  • setSrc now clears currentSource_ after loadstart
  • EventTarget.prototype.one removes the addEventListener alias before calling Events.on so we don't get into an infinite type loop

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
  • Reviewed by Two Core Contributors
@incompl incompl setSrc clears currentSource_ after loadstart
06dcc61
@gkatsev
Member
gkatsev commented Apr 26, 2016

Fixes #1951. This still needs disposing the source handler, right?

@incompl
Contributor
incompl commented Apr 26, 2016

@gkatsev It has this.disposeSourceHandler(); in setSrc

@incompl incompl Move disposeSourceHandler from setSrc to loadStartListener_
8d89829
@incompl
Contributor
incompl commented Apr 26, 2016

Moved disposeSourceHandler to the right place

@misteroneill misteroneill and 1 other commented on an outdated diff Apr 27, 2016
src/js/event-target.js
@@ -23,7 +23,12 @@ EventTarget.prototype.off = function(type, fn) {
EventTarget.prototype.removeEventListener = EventTarget.prototype.off;
EventTarget.prototype.one = function(type, fn) {
+ // Remove the addEventListener alias before calling Events.on
+ // so we don't get into an infinite type loop
+ let ael = this.addEventListener;
+ this.addEventListener = Function.prototype;
@misteroneill
misteroneill Apr 27, 2016 Member

We have encountered issues with using Function.prototype as a no-op due to the way video.js tracks functions by a GUID property. I don't think it would be an issue here, but I think it's safer to never use it. Unfortunately. 😞

@incompl
incompl Apr 27, 2016 Contributor

What do you propose? () => {}?

@misteroneill misteroneill and 1 other commented on an outdated diff Apr 27, 2016
src/js/tech/html5.js
@@ -99,7 +99,7 @@ class Html5 extends Tech {
if (this.featuresNativeTextTracks) {
if (crossoriginTracks) {
- log.warn(tsml`Text Tracks are being loaded from another origin but the crossorigin attribute isn't used.
+ log.warn(tsml`Text Tracks are being loaded from another origin but the crossorigin attribute isn't used.
@misteroneill
misteroneill Apr 27, 2016 Member

tsml will strip spaces between lines so this will end up with "isn't used.This may", which isn't the end of the world, but we could use my alternative package tsmlj which will join each line with a space.

@incompl
incompl Apr 27, 2016 Contributor

Will tell me editor to cut it out with the whitespace trimming.

@gkatsev gkatsev and 1 other commented on an outdated diff Apr 27, 2016
test/unit/tech/html5.test.js
@@ -442,3 +445,28 @@ test('Html5#reset calls Html5.resetMediaElement when called', function() {
Html5.resetMediaElement = oldResetMedia;
});
+
+test('Html5#setSrc clears currentSource_ after loadstart', function() {
+
+ let thing = {
+ off: () => {},
+ one: (el, type, fun) => {
+ el.one(type, Fn.bind(thing, fun));
+ },
+ disposeSourceHandler: () => {},
@gkatsev
gkatsev Apr 27, 2016 Member

we should verify that this has been called.

@incompl
incompl Apr 27, 2016 Contributor

Good idea

@gkatsev
Member
gkatsev commented Apr 27, 2016

@dmlap or @imbcmdth can you check this out as well and let us know if you think there could be any issues with it for contrib-hls.

incompl added some commits Apr 27, 2016
@incompl incompl Improvements based on code review
132e107
@incompl incompl Remove unused import
47873df
@dmlap dmlap commented on the diff Apr 27, 2016
src/js/event-target.js
@@ -11,7 +11,7 @@ EventTarget.prototype.on = function(type, fn) {
// Remove the addEventListener alias before calling Events.on
// so we don't get into an infinite type loop
let ael = this.addEventListener;
- this.addEventListener = Function.prototype;
+ this.addEventListener = () => {};
@dmlap
dmlap Apr 27, 2016 Member

Point for discussion: should we be careful about using fat-arrows all over the place? I know calling addEventListener() with a non-this receiver is a weird thing to do but we could use a plain function here and avoid possibly subverting people's expectations.

@dmlap
dmlap Apr 27, 2016 Member

We're gonna change this to function() {}

@gkatsev
gkatsev Apr 27, 2016 Member

I don't think it matters here at all since we just want an empty function while Events.on runs so we don't get into an infinite loop, so, there's no expectations being subverted here.
As for elsewhere, it really depends on what you want the context of your function to be since in arrow functions this and arguments are never bound.

@dmlap
dmlap May 2, 2016 Member

That's the crux of the discussion I think we should have. Using fat-arrows removes the ability of the caller to rebind this. I've felt it necessary to rebind this more than once in my life. Are we intentionally removing that ability or just doing it to save some keystrokes?

@dmlap dmlap commented on the diff Apr 27, 2016
src/js/event-target.js
@@ -23,7 +23,12 @@ EventTarget.prototype.off = function(type, fn) {
EventTarget.prototype.removeEventListener = EventTarget.prototype.off;
EventTarget.prototype.one = function(type, fn) {
+ // Remove the addEventListener alias before calling Events.on
+ // so we don't get into an infinite type loop
+ let ael = this.addEventListener;
+ this.addEventListener = () => {};
@dmlap
dmlap Apr 27, 2016 Member

What would happen if one of the listeners registers an event listener? Something like this:

function maybeReRegister() {
  if (someCondition()) {
    player.one('play', maybeReRegister);
    return;
  }
  alert('done!');
};
player.one('play', maybeReRegister);
@dmlap
dmlap Apr 27, 2016 Member

After reviewing, I don't think this is a problem right now but the event stuff sure is confusing.

@gkatsev
gkatsev Apr 27, 2016 Member

I think it works fine because the event system creates a clone of the handlers before it starts triggering events: https://github.com/videojs/video.js/blob/2e2dbde4b4b70ecac22736477ca60d20a12cfadd/src/js/utils/events.js#L52-L63

@dmlap dmlap and 1 other commented on an outdated diff Apr 27, 2016
src/js/tech/html5.js
@@ -549,9 +549,19 @@ class Html5 extends Tech {
* @method setSrc
*/
setSrc(src) {
+ let loadstartlistener = Html5.prototype.loadStartListener_;
+
+ this.off(this.el_, 'loadstart', loadstartlistener);
+ this.one(this.el_, 'loadstart', () => this.one(this.el_, 'loadstart', loadstartlistener));
@dmlap
dmlap Apr 27, 2016 Member

Does this assume setSrc() is the only valid way to modify the video element's source? What about setSource()?

Is there a way this can be written that doesn't require adding and removing listeners after the tech's creation? You might be able to use the readyState of the HTML element, for instance.

@dmlap
dmlap Apr 27, 2016 Member

Also, this function literal wouldn't get cleared properly on subsequent calls to setSrc() which would cause the loadstartlistener_ to get invoked on the wrong loadstart

@gkatsev
gkatsev Apr 27, 2016 Member

Yeah, this doesn't account for setSource which is what ends up being used if a source handler is in effect.
https://github.com/videojs/video.js/blob/2e2dbde4b4b70ecac22736477ca60d20a12cfadd/src/js/player.js#L1916-L1919

@gkatsev
gkatsev Apr 27, 2016 Member

why would loadstartlistener get called on the wrong loadstart?

@dmlap
dmlap Apr 28, 2016 Member

Here's the scenario:

  1. Call setSrc()
  2. Video element fires loadstart
  3. Call setSrc()

loadStartListener_() would be removed on the second call to setSrc() but the inline listener that triggers the loadstartlistener on the next loadstart would not. So, the very next loadstart would run loadstartlistener.

@gkatsev
gkatsev Apr 28, 2016 Member

Oh, right, the first loadstart listener would not be removed and would trigger again.

@dmlap dmlap and 2 others commented on an outdated diff Apr 27, 2016
test/unit/tech/html5.test.js
@@ -442,3 +444,30 @@ test('Html5#reset calls Html5.resetMediaElement when called', function() {
Html5.resetMediaElement = oldResetMedia;
});
+
+test('Html5#setSrc clears currentSource_ after loadstart', function() {
+
+ let disposed = false;
+ let thing = {
+ off: () => {},
+ one: (el, type, fun) => {
+ el.one(type, Fn.bind(thing, fun));
+ },
@dmlap
dmlap Apr 27, 2016 Member

Mocking one() and off() may make this test a little too artificial.

@incompl
incompl Apr 27, 2016 Contributor

It does successfully test that setSrc clears contentSource_ after loadstart. Do you have any suggestions on how to make this more real?

@dmlap
dmlap Apr 28, 2016 Member

I'd create an object that inherited from EventTarget right in the test here and use that.

@gkatsev
gkatsev Apr 28, 2016 Member

We tried that. thing is a a tech mock who's one method signature is different. We could make it be a standalone Component instance, possibly.

@misteroneill misteroneill and 1 other commented on an outdated diff Apr 28, 2016
src/js/tech/html5.js
@@ -549,9 +549,19 @@ class Html5 extends Tech {
* @method setSrc
*/
setSrc(src) {
+ let loadstartlistener = Html5.prototype.loadStartListener_;
@misteroneill
misteroneill Apr 28, 2016 Member

Are we explicitly using the Html5#loadStartListener_ function instead of this.loadStartListener_ because we want to be sure that property hasn't been added to an instance of the tech as something else? Maybe this is a candidate for being module-level private instead of class-level pseudo-private?

@incompl
incompl Apr 28, 2016 Contributor

No, it's just arbitrary.

incompl added some commits Apr 28, 2016
@incompl incompl Move currentSource_ clearing behavior from html5 tech to tech.js f10fc92
@incompl incompl Restore coveralls
84810c2
@incompl incompl Improve method names and comments in setSource
64be2ab
@incompl incompl Clearing currentSource_ happens on successive loadstarts
43aac04
@gkatsev
Member
gkatsev commented Apr 29, 2016

That looks pretty good to me. @dmlap can you re-review?

@gkatsev
Member
gkatsev commented May 2, 2016

Oh neat, github picked up that it's the same code (I pushed this branch to the repo so that it can run in browserstack).

@dmlap dmlap commented on the diff May 2, 2016
src/js/tech/tech.js
this.sourceHandler_ = sh.handleSource(source, this, this.options_);
this.on('dispose', this.disposeSourceHandler);
return this;
};
- /*
- * Clean up any existing source handler
- */
- _Tech.prototype.disposeSourceHandler = function(){
+ // On the first loadstart after setSource
+ _Tech.prototype.firstLoadStartListener_ = function() {
+ this.one(this.el_, 'loadstart', _Tech.prototype.successiveLoadStartListener_);
@dmlap
dmlap May 2, 2016 Member

Why use one() if you want the successiveLoadStartListener_ to run on every subsequent loadstart?

@incompl
incompl May 2, 2016 Contributor

It's because disposeSourceHandler removes it. We work around this by re-adding it in successiveLoadStartListener_ after disposeSourceHandler has been called.

@dmlap dmlap commented on the diff May 2, 2016
src/js/tech/tech.js
@@ -814,18 +814,44 @@ Tech.withSourceHandlers = function(_Tech){
if (this.currentSource_) {
this.clearTracks(['audio', 'video']);
}
- this.currentSource_ = source;
+
+ if (sh !== _Tech.nativeSourceHandler) {
+
+ this.currentSource_ = source;
+
+ // Catch if someone replaced the src without calling setSource.
+ // If they do, set currentSource_ to null and dispose our source handler.
+ this.off(this.el_, 'loadstart', _Tech.prototype.firstLoadStartListener_);
+ this.off(this.el_, 'loadstart', _Tech.prototype.successiveLoadStartListener_);
+ this.one(this.el_, 'loadstart', _Tech.prototype.firstLoadStartListener_);
@dmlap
dmlap May 2, 2016 Member

I'm not a fan of using event handler registration as instance-level state. The current set of registered event handlers are difficult to introspect and control flow is harder to trace. I don't see any logical issues with this code so we can move ahead with this but in general, I think adding an instance variable is preferable to this kind of solution.

@dmlap
Member
dmlap commented May 2, 2016

I'm not in love with the solution but I can live with it. LGTM

@dmlap dmlap closed this in 249532a May 2, 2016
@wmfgerrit wmfgerrit pushed a commit to wikimedia/mediawiki-extensions-TimedMediaHandler that referenced this pull request May 8, 2016
@paladox @hartman paladox + hartman Update videojs to 5.10.1
Changelog is at

5.9.0
* https://github.com/videojs/video.js/releases/tag/v5.9.0

5.9.1
* https://github.com/videojs/video.js/releases/tag/v5.9.1

5.9.2
* https://github.com/videojs/video.js/releases/tag/v5.9.2

Microsoft Edge change
* updated IS_CHROME to not be true on MS Edge

5.10.1
*https://github.com/videojs/video.js/releases/tag/v5.10.1

Important: Changing resolution now works as of this change
videojs/video.js#3285

Which is in 5.10.1.

Ive tested and changing resolution now works.

Bug: T131544
Change-Id: I191d92f24609b22424c402d0ef7f4b44cf1e6b20
2074f78
@wmfgerrit wmfgerrit pushed a commit to wikimedia/mediawiki-extensions that referenced this pull request May 8, 2016
@paladox paladox + Gerrit Code Review Updated mediawiki/extensions
Project: mediawiki/extensions/TimedMediaHandler  2074f78f19c3b4c028c4566f2a416b44ac61b140

Update videojs to 5.10.1

Changelog is at

5.9.0
* https://github.com/videojs/video.js/releases/tag/v5.9.0

5.9.1
* https://github.com/videojs/video.js/releases/tag/v5.9.1

5.9.2
* https://github.com/videojs/video.js/releases/tag/v5.9.2

Microsoft Edge change
* updated IS_CHROME to not be true on MS Edge

5.10.1
*https://github.com/videojs/video.js/releases/tag/v5.10.1

Important: Changing resolution now works as of this change
videojs/video.js#3285

Which is in 5.10.1.

Ive tested and changing resolution now works.

Bug: T131544
Change-Id: I191d92f24609b22424c402d0ef7f4b44cf1e6b20
2c5883a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment