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

In Chrome, setting up a video using flash tech too early with another video, will cause the flash player's control bar to fail #1407

Closed
gkatsev opened this issue Aug 7, 2014 · 21 comments

Comments

@gkatsev
Copy link
Member

gkatsev commented Aug 7, 2014

If you have two videos on a page that aren't auto-setup but rather setup manually by calling videojs, if you setup the flash player and then setup another player within a certain threshold (I've found it to be about 250ms on chrome), the flash player's control bar will fail and not report any progress or duration while the html player will work fine.
This actually seems to be a problem if you just try to setup both videos within the threshold previous, it also breaks, but outside of that period it works.
Here's a test case for the former case making the control bar not work: http://jsbin.com/cupeyevi/1
And there's a test case for the latter case making it work with a long timeout: http://jsbin.com/vewipexi/1

@jnwng
Copy link
Contributor

jnwng commented Aug 8, 2014

@gkatsev i was able to confirm that this doesn't work, although in both of the examples you gave me i can't get past the big play button that loads the video. HTML5 video worked for both examples, and adding a larger threshold did not get the flash video to work. I'm on Chrome Version 36.0.1985.125

@heff
Copy link
Member

heff commented Aug 11, 2014

@gkatsev Flash doesn't work in jsbin except in the fullscreen version, so that's part of the confusion. And you can't open the fullscreen version of those examples without cloning them first.

I did that, and the Flash version is actually working for me in Chrome. Does it work for you here?
http://jsbin.com/pufoq/1

@gkatsev
Copy link
Member Author

gkatsev commented Aug 11, 2014

It's broken for me: http://cl.ly/image/472z2j2y3B3l

@jnwng
Copy link
Contributor

jnwng commented Aug 11, 2014

broken for me as well:
screen shot 2014-08-11 at 1 55 40 pm

@heff
Copy link
Member

heff commented Aug 11, 2014

Weird. Chrome 36 Mac? The first video works for me but the second doesn't.

screen shot 2014-08-11 at 2 06 30 pm

@heff
Copy link
Member

heff commented Aug 11, 2014

Ah, I had another copy of the mp4 open in another tab, which was breaking the html5 version. Thanks Chrome! But that was letting the Flash version work. So there's definitely some interaction between the two.

@gkatsev
Copy link
Member Author

gkatsev commented Aug 11, 2014

Yep, chrome 36 on osx.

@heff
Copy link
Member

heff commented Aug 11, 2014

My guess is this has to do with manualTimeUpdates being turned off somehow. Flash doesn't trigger time updates itself.

this.tech.one('timeupdate', function(){

@gkatsev think you can dig in and see if manualTimeUpdatesOff is getting called on the first player?

@gkatsev
Copy link
Member Author

gkatsev commented Aug 11, 2014

Looks like this is the problem. Calling manualTimeUpdatesOn on the broken player seems to fix it. Looking more directly into whether it's being turned off or just never getting called in the first place.

@gkatsev
Copy link
Member Author

gkatsev commented Aug 11, 2014

Seems like manualTimeUpdatesOn is never called in the flash player in this particular case.

@gkatsev
Copy link
Member Author

gkatsev commented Aug 11, 2014

Seems like the html tech is overwriting vjs.MediaTechController.prototype.features before flash finishes loading, so, by the time flash loads it thinks that it doesn't need manual progress events.

@gkatsev
Copy link
Member Author

gkatsev commented Aug 11, 2014

I don't understand why here

this.features === vjs.MediaTechController.prototype.features

The above statement is true.

@heff
Copy link
Member

heff commented Aug 11, 2014

Ah, so this line actually refers to the MediaTech prototype.features, not HTML5's own copy of features.

this.features['timeupdateEvents'] = true;

The features feature needs to be refactored so that each tech gets its own copy of the features object. Does that sound right?

@gkatsev
Copy link
Member Author

gkatsev commented Aug 11, 2014

Yeah, basically. I'm not sure why though, since it seems like the whole prototype stuff should be taking care that it isn't the same.

@heff
Copy link
Member

heff commented Aug 11, 2014

The prototype stuff would work if we were overwriting the object, like this.features = {}, but until we do that the getter would always return the object instance from the media tech prototype.

@gkatsev
Copy link
Member Author

gkatsev commented Aug 11, 2014

Yeah, I guess I was misremembering how the prototype stuff worked.
Because we're overwriting a property of features, rather than features itself.

@heff
Copy link
Member

heff commented Aug 11, 2014

Yeah, exactly.

@gkatsev
Copy link
Member Author

gkatsev commented Aug 11, 2014

I guess in the MediaTechController we want to do something like this.features = vjs.obj.create(vjs.MediaTechController.prototype.features).

@heff
Copy link
Member

heff commented Aug 11, 2014

Yeah, or just merge like options does. In fact one option would be to move features into options. Not sure if that would be overloading options.

@gkatsev
Copy link
Member Author

gkatsev commented Aug 11, 2014

Yep, doing that fixed it and because we're using create in there, changes from vjs.MediaTechController.prototype.features would propagate down to the techs unless a property was overridden.

@heff
Copy link
Member

heff commented Sep 2, 2014

The fix for this (#1470) has been merged into master.

@heff heff closed this as completed Sep 2, 2014
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants