-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
Return currentSource_.src instead of blob-urls in html5 tech #2232
Conversation
…ers in html5 tech
Awesome. I was worried we'd have a problem switching to other source handlers an not clearing out the previous currentSource_, but it looks like we always set currentSrc_, so the This does create a difference from the video element we might want to think about. The video element's currentSrc does't return the source that's "next". When you set It's weird, but I think that's the whole reason currentSrc exists instead of just relying on src. See any easy ways to make that work the same? |
Gary and I came up with an interesting test: http://jsbin.com/sepajuyada/4/edit Seems like the |
Interesting, good find. I always assumed it waited for load(), but clearly not. I remembered the other reason currentSrc exists, which is that you can set the source via the src attribute or the source child elements. vidEl.src should always return the src attribute value, which will be an empty string if the source was set with children. At the tech level we always set the source with the src property not children, so that's not really a problem for us. So we could either assume we have more info than the video element in this case and return the set source as currentSrc immediately, or try to mimic the asynchronous side of it so it matches exactly with how the video element works, returning the previous source for any synchronous calls to currentSrc. Any other thoughts? I added on to the jsbin to test some things. You can comment out the source child block to see how that works a little differently. Some other notes... When you set an empty string as a source, you'll see there's some differences there. The src attribute will return the absolute URL of the page, while currentSrc will return an empty string in Firefox and Safari, or the previous source URL in Chrome. Go figure. It looks like the loadstart event should fire before currentSrc is updated, but the browsers are either ignoring that or setting it in parallel before the listener gets to it. |
I tried to capture the asynchronous behavior that you mentioned in a test and made some changes to media to emulate that behavior. I think I may have gone too far with the way I handle src when it is an empty string. It's probably best to just allow currentSrc_ to be set to an empty string as well since that seems to be the majority behavior. |
if (source && source.src !== '') { | ||
tech.currentSource_ = source; | ||
} | ||
}, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice. If you use this.setTimeout
it will automatically kill the timeout when the tech is disposed. Just to clean up.
Adding async always makes me worried about edge cases we're not thinking of... I can't think of anything specific though so we could pull this in and see how it goes. @videojs/core-committers any final opinions on this (setting currentSrc asynchronously or not)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this matches <video>
behavior better, I think it's the way to go. +1 to this.setTimeout
.
Two small comments but otherwise LGTM |
// asynchronous execution of the `resource selection algorithm` | ||
this.setTimeout(function () { | ||
if (source && source.src !== '') { | ||
tech.currentSource_ = source; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should bind this method instead of using the tech
variable.
Minor comment, otherwise, LGTM. |
LGTM |
…ml5 tech. Fixes videojs#2232. closes videojs#2232
Closed via 20b46b9 |
The Flash tech already does this and it makes sense to return the URL of the resource instead of a blob-url if the SourceHandler uses MSE.