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

Revert 4.12.9 and fix the Html5 tech with sourceHandlers that use MSE #2271

Closed
wants to merge 8 commits into from

Conversation

imbcmdth
Copy link
Member

4.12.9 introduced many small problems ranging from incompatibility with old techs to subtly breaking videojs-contrib-ads. Too many things depend on the existing behavior to change it without a major release so this PR reverts those changes.

On top of that, the PR fixes the issue we were having with blob urls in the src and currentSrc functions when using MSE. It does this in the simplest and least-likely-to-affect-anything-else-way possible. For that reason, all changes are confined to the Html5 tech instead of Media so that the changes do not alter the behavior for any other techs.

@@ -314,10 +314,20 @@ vjs.Html5.prototype.exitFullScreen = function(){
this.el_.webkitExitFullScreen();
};

vjs.Html5.prototype.returnFakeIfBlobURI_ = function (realValue, fakeValue) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain this with a comment?

@dmlap
Copy link
Member

dmlap commented Jun 23, 2015

@videojs/core-committers we spent awhile discussing this issue offline. No one likes this solution but we think it's the minimal change that can be made that will allow MSE-based source handlers to appear native-ish in 4.x. We need to figure out the right way to accomplish this for 5.0, though.

Ideas we came up with:

  • Do nothing and give up on the idea of source handlers completely abstracting over new playback content types.
  • Add callbacks that allow src and currentSrc to be overridden in source handlers and then delegate to them in MediaTechController when defined.
  • Give up on source handlers entirely and make writing new techs easier.
  • Allow source handlers to return a decorated tech that is used for playback.
  • Try to solve this generically in MediaTechController without additional source handler callbacks.

@dmlap
Copy link
Member

dmlap commented Jun 23, 2015

With the caveat about 5.0 above, LGTM

var blobURIRegExp = /^blob\:/i;

if (realValue && blobURIRegExp.test(realValue)) {
return fakeValue;
Copy link
Member

Choose a reason for hiding this comment

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

should there be null checking here?

dmlap pushed a commit to dmlap/video.js that referenced this pull request Jun 23, 2015
@dmlap dmlap closed this Jun 23, 2015
@heff
Copy link
Member

heff commented Jun 29, 2015

Here's my opinion.

  1. Make currentSrc synchronous. It was a valiant effort trying to mimic the video element exactly, but it's added too much complexity, and I can't list any real world use cases that need currentSrc to be set async in the way the video element is.
  2. Always store the src/currentSrc values at the tech level and return those values, at least for techs with source handlers (i.e. may not be needed for videojs-youtube). The player only sets the source through the src() function on the tech, and nothing in the tech should be changing the src value, except for potentially making it an absolute URL. The user of the player shouldn't be surprised by what gets returned from src/currentSrc. The only problem I can think of that might come from this approach is if a player user circumvents the player and changes the source on the video element itself, but that's just bad behavior.

Does that address all the issues or am I missing anything?

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.

4 participants