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

Bad behaviour when no type specified on source #4469

Closed
davidgg opened this issue Jul 6, 2017 · 11 comments
Closed

Bad behaviour when no type specified on source #4469

davidgg opened this issue Jul 6, 2017 · 11 comments

Comments

@davidgg
Copy link
Contributor

davidgg commented Jul 6, 2017

Description

Right now, when using a video with source tag but without a type attribute, video.js takes the extension of the file and checks if the browser is able to play it. You can see it here:

Html5.nativeSourceHandler.canHandleSource = function(source, options) {

Html5.nativeSourceHandler.canHandleSource = function(source, options) {

  // If a type was provided we should rely on that
  if (source.type) {
    return Html5.nativeSourceHandler.canPlayType(source.type);

  // If no type, fall back to checking 'video/[EXTENSION]'
  } else if (source.src) {
    const ext = Url.getFileExtension(source.src);

    return Html5.nativeSourceHandler.canPlayType(`video/${ext}`);
  }

  return '';
};

There are some valid formats that are not working because of this, some samples:

.opus -> video/ogg
.mov -> video/mp4
.aac -> audio/mp4
...

Steps to reproduce

  1. Create a new video or audio tag and use a .mov, .opus or .aac file without type
  2. Video.js shows an error message

Results

Expected

Legit audio/video formats are accepted.

Actual

Some legit audio/video formats cause an error

Proposal

Maintain a hash with popular extensions and MIME types, check against this hash the extensions to get the MIME type.

@misteroneill
Copy link
Member

Your proposal would be relatively easy to implement, I think. I wonder if there's a historical reason why Video.js does not maintain such a mapping.

Any thoughts from @videojs/core-committers?

@gkatsev
Copy link
Member

gkatsev commented Dec 4, 2017

Having a full mime-type mapping will greatly increase the size of the player. If we trim it, how do we decide which to include and which to exclude? .mp4 is probably one of the only ones that's pretty reliable. In addition to this, source urls can and often come without a filetype, so, you can't even reliably detect the type based on the url and even worth is if you have a url like akamai where it has stuff like .mp4 and .m3u8 in the same url.
I'm not inherently opposed but unfortunately, it isn't as simple as just including known mimetypes for video.

@davidgg
Copy link
Contributor Author

davidgg commented Dec 5, 2017

Hi @gkatsev thank you for the reply.

I'm talking only about some popular types, not a full mime-type list. The samples I said (.opus, .mov, .aac) are quite popular and are not the only. Right now video.js is breaking them, we have to answer if this is a better situation than having some types in the code.

All the scenarios you are describing are happening now, if you check the code you can see that it is using the extension when no type is provided, so it's not a problem related with the solution suggested.

It's a proposal to make video.js better, not perfect.

@gkatsev
Copy link
Member

gkatsev commented Dec 5, 2017

Yup, maybe it isn't that bad to have the just a few of known common ones.

@gkatsev
Copy link
Member

gkatsev commented Feb 8, 2018

So, I think I've been convinced that a minimal amount of checking would be fine, especially after #4851.
@davidgg it's been a while, would you be able for making a PR to do a check for ones like mp4, mp3, aac, m3u8?

@gkatsev
Copy link
Member

gkatsev commented Feb 8, 2018

Probably should happen in here:

} else if (typeof src === 'string' && src.trim()) {
// convert string into object
src = [{src}];

@davidgg
Copy link
Contributor Author

davidgg commented Feb 9, 2018

yeah, sure!

@gkatsev
Copy link
Member

gkatsev commented Feb 9, 2018

Awesome. Let me know here or in a PR or on our slack if you have any questions or issues.

davidgg added a commit to davidgg/video.js that referenced this issue Feb 11, 2018
File mimetype is filled when the file extension is known and type is not provided.
@davidgg
Copy link
Contributor Author

davidgg commented Feb 11, 2018

@gkatsev done, take a look at #4947

@rwlaschin
Copy link

How about allowing the src to be an Object and we can pass in the {url,mimeType} ??

easy enough to check/implement.

@gkatsev
Copy link
Member

gkatsev commented Dec 13, 2020

A source object can be passed in, and we handle it properly. This issue was about how to handle being given a string only.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants