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.

Sign up for GitHub

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

Jump to bottom

instanceof HTMLMediaElement failfast messes up with MSE polyfill #1309

Closed
AxelDelmas opened this issue Apr 6, 2016 · 5 comments
Closed

instanceof HTMLMediaElement failfast messes up with MSE polyfill #1309

AxelDelmas opened this issue Apr 6, 2016 · 5 comments
Labels
Code Refactor Question

Comments

@AxelDelmas
Copy link

AxelDelmas commented Apr 6, 2016 •
edited by dsparacio
Loading

Is it actually necessary to run this check ?

It fails if we pass a polyfill. We used to work around this by making our polyfill inherit from HTMLMediaElement, but this change causes an exception, as some checks are run in HTMLMediaElement's src setter.

See corresponding fmse issue here: streamroot/fmse#9
@dsparacio
Copy link
Contributor

prob not a big deal to pull out for your polyfil but I would ask that you do it and test it well please.

@AxelDelmas
Copy link
Author

Sure, should I just remove this block: https://github.com/Dash-Industry-Forum/dash.js/blob/development/src/streaming/utils/Capabilities.js#L65-L67 ?

Are there any unit check / sample pages that use this and that I should be aware of ?
Seems to be used only here, so I think we can safely remove the block and assume that the user will pass an instance of video tag, or knows what he's doing.
I can also log a warning instead of throwing, rather than removing the if block altogether.

@dsparacio
Copy link
Contributor

Yeah it is just used in that once place I think we are safe here.

AxelDelmas pushed a commit to streamroot/dash.js that referenced this issue Apr 7, 2016
Remove fail fast if video is not an instance of HTMLMediaElement (all…
33e429d 
…ow for

polyfills) Dash-Industry-Forum#1309
@AxelDelmas AxelDelmas mentioned this issue Apr 7, 2016
Merged
jscarmona pushed a commit to veo-televisa/dash.js that referenced this issue Apr 27, 2016
Merge branch 'master' of github.com:Dash-Industry-Forum/dash.js
1313d8e 
* 'master' of github.com:Dash-Industry-Forum/dash.js: (128 commits)
  Adding dist files to release.
  updating the version number and release notes for 2.1.1
  Made all Slack links point to signup page
  Added Slack info to readme
  Only seek forward on PST check
  pushing dist files to master
  adding release notes to support player
  Moving where we set the protection vars to null. Works fine if you set new prod data, switch streams or DRM or key systems etc.
  Prevent low values attempting to use representation -1
  Found issue with this line.  I added the isFirst but the return statement check solves the reason why I add isFirst check in the first place. With check some streams in some browsers do not start if non 0 pst
  manually resolve relative MPD URLs for browsers without xhr.responseURL
  Pass XHR object, not request, back to derived loader onsuccess
  Fix BaseURL resolution for old vtt demos. Are these are legitimately signalled?
  Fix Dash-Industry-Forum#1321 - resolve to baseuri correctly when MPD@BaseURL is relative
  Reapplying changes to live delay sample files
  Revert "fixing commit of incorrect controlbar files"
  fixing commit of incorrect controlbar files
  Revert "Adding a new live delay sample and renaming existing samples"
  Fix for Dash-Industry-Forum#1314
  Remove fail fast if video is not an instance of HTMLMediaElement (allow for polyfills) Dash-Industry-Forum#1309
  ...
@davemevans
Copy link
Contributor

This was fixed in 2.1.0 right?

@dsparacio
Copy link
Contributor

I believe so with PR #1312

@davemevans davemevans mentioned this issue Mar 27, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Code Refactor Question
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@davemevans @AxelDelmas @dsparacio