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

More fixes for async currentSrc behavior #2256

Closed
wants to merge 4 commits into from

Conversation

imbcmdth
Copy link
Member

Two main fixes here:

  1. The player.currentSrc function didn't allow an empty string to be returned from techs - it would always use src in that case
  2. In my testing it appears that currentSrc should always be updated by the time any other loadstart listener is triggered which wasn't always the case using only a setTimeout of 0.

The second bug has the potential to cause problems in plugins that rely on currentSrc being properly updated by the time loadstart is emitted. Since events are triggered synchronously, there are cases where the setTimeout is simply "too late" to match the video element's behavior.

These changes bring it much more in-line with what the video element does

As an example, I ran the test @heff created http://jsbin.com/wulomalopu/1/edit?js,console,output (but modified to use video.js) and the output with these changes on Chrome 43 is:

"el created"
""
""
2
"source set"
"http://vjs.zencdn.net/v/oceans.mp4"
""
3
"async"
"http://vjs.zencdn.net/v/oceans.mp4"
"http://vjs.zencdn.net/v/oceans.mp4"
4
"source set again"
"http://vjs.zencdn.net/v/oceans.webm"
"http://vjs.zencdn.net/v/oceans.mp4"
5
"async again"
"http://vjs.zencdn.net/v/oceans.webm"
"http://vjs.zencdn.net/v/oceans.webm"
6
"empty source set"
"file:///Users/jrivera/projects/video.js/tester.html"
"http://vjs.zencdn.net/v/oceans.webm"
7
"loadstart"
"file:///Users/jrivera/projects/video.js/tester.html"
"http://vjs.zencdn.net/v/oceans.webm"
8
"async for empty source"
"file:///Users/jrivera/projects/video.js/tester.html"
"http://vjs.zencdn.net/v/oceans.webm"

..and that is very, very close to the test's output on Chrome.

@gkatsev
Copy link
Member

gkatsev commented Jun 12, 2015

The other option is to just make setting currentSrc synchronous.

* in a timeout to make sure it is set asynchronously before anything else
* but before other loadstart handlers have had a chance to execute
*/
vjs.MediaTechController.prototype.updateCurrentSource = function () {
Copy link
Member

Choose a reason for hiding this comment

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

Should this method be _-suffixed?

@dmlap
Copy link
Member

dmlap commented Jun 15, 2015

@imbcmdth updateCurrentSource feels like it should be marked as internal. Otherwise, LGTM.

gkatsev pushed a commit to gkatsev/video.js that referenced this pull request Jun 15, 2015
@gkatsev
Copy link
Member

gkatsev commented Jun 15, 2015

Closed via 463ba4e

@gkatsev gkatsev closed this Jun 15, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants