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

Return currentSource_.src instead of blob-urls in html5 tech #2232

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/js/media/html5.js
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,13 @@ vjs.Html5.prototype.setSrc = function(src) {
};

vjs.Html5.prototype.load = function(){ this.el_.load(); };
vjs.Html5.prototype.currentSrc = function(){ return this.el_.currentSrc; };
vjs.Html5.prototype.currentSrc = function(){
if (this.currentSource_) {
return this.currentSource_.src;
} else {
return this.el_.currentSrc;
}
};

vjs.Html5.prototype.poster = function(){ return this.el_.poster; };
vjs.Html5.prototype.setPoster = function(val){ this.el_.poster = val; };
Expand Down
10 changes: 9 additions & 1 deletion src/js/media/media.js
Original file line number Diff line number Diff line change
Expand Up @@ -495,6 +495,7 @@ vjs.MediaTechController.withSourceHandlers = function(Tech){
*/
Tech.prototype.setSource = function(source){
var sh = Tech.selectSourceHandler(source);
var tech = this;

if (!sh) {
// Fall back to a native source hander when unsupported sources are
Expand All @@ -510,7 +511,14 @@ vjs.MediaTechController.withSourceHandlers = function(Tech){
this.disposeSourceHandler();
this.off('dispose', this.disposeSourceHandler);

this.currentSource_ = source;
// Set currentSource_ asynchronously to simulate the media element's
// asynchronous execution of the `resource selection algorithm`
this.setTimeout(function () {
if (source && source.src !== '') {
tech.currentSource_ = source;
Copy link
Member

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.

}
}, 0);
Copy link
Member

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)?

Copy link
Member

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.


this.sourceHandler_ = sh.handleSource(source, this);
this.on('dispose', this.disposeSourceHandler);

Expand Down
56 changes: 55 additions & 1 deletion test/unit/media.js
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,10 @@ test('should add the source hanlder interface to a tech', function(){

// Pass a source through the source handler process of a tech instance
tech.setSource(sourceA);

// Increment clock since currentSource_ is set asynchronously
this.clock.tick(1);

strictEqual(tech.currentSource_, sourceA, 'sourceA was handled and stored');
ok(tech.sourceHandler_.dispose, 'the handlerOne state instance was stored');

Expand Down Expand Up @@ -250,4 +254,54 @@ test('should handle unsupported sources with the source hanlder API', function()

tech.setSource('');
ok(usedNative, 'native source handler was used when an unsupported source was set');
});
});

test('should emulate the video element\'s behavior for currentSrc when src is set', function(){
var mockPlayer = {
off: this.noop,
trigger: this.noop
};
var sourceA = { src: 'foo.mp4', type: 'video/mp4' };
var sourceB = { src: '', type: 'video/mp4' };

// Define a new tech class
var Tech = videojs.MediaTechController.extend();

// Extend Tech with source handlers
vjs.MediaTechController.withSourceHandlers(Tech);

// Create an instance of Tech
var tech = new Tech(mockPlayer);

// Create source handlers
var handler = {
canHandleSource: function(source){
return 'probably';
},
handleSource: function(s, t){return {};}
};

Tech.registerSourceHandler(handler);

// Pass a source through the source handler process of a tech instance
tech.setSource(sourceA);

// Test that currentSource_ is not immediately specified
strictEqual(tech.currentSource_, undefined, 'sourceA was not stored immediately');
Copy link
Member

Choose a reason for hiding this comment

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

Should we be testing tech.currentSrc() here instead? That seems closer to what we're trying to achieve.

Copy link
Member Author

Choose a reason for hiding this comment

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

The currentSrc function is only provided on html5 and flash techs. It isn't a base function on the MediaTechController. This is testing that MediaTechController does what it should to allow higher-level techs to operate as closely to the media element as possible.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm. Given all the state to implement currentSrc() lives in MediaTechController and both the html and flash tech have copy-pasted implementations, it seems like we should just pull the implementation up and make this a real test case.

Copy link
Member Author

Choose a reason for hiding this comment

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

The current implementation is backwards compatible with existing techs. Moving anything to MediaController either doesn't buy us code savings or requires potentially breaking changes which we shouldn't be making in VJS4.

I agree that for VJS5 we should strongly consider moving much the currentSrc function to Tech and have each child-tech implement something like a currentSrc_ function that Tech#currentSrc will look for and call if currentSource_ is not defined.

Copy link
Member

Choose a reason for hiding this comment

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

If it's a breaking change and we feel this is useful, we should consider it quickly so we can add it to 5.0 before it's too late. Though, really, now would be the time to just iterate through the major versions quickly, once it settles down, we should move more carefully.


this.clock.tick(1);

// Test that currentSource_ is specified after yielding to the event loop
strictEqual(tech.currentSource_, sourceA, 'sourceA was handled and stored');

// Pass a source with an empty src
tech.setSource(sourceB);

// Test that currentSource_ is not immediately changed
strictEqual(tech.currentSource_, sourceA, 'sourceB was not stored immediately');

this.clock.tick(1);

// Test that currentSource_ is still unchanged
strictEqual(tech.currentSource_, sourceA, 'sourceB was not stored if equal to the empty string');
});