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

Add a regex to check different mp4 suffix. #1734

Merged
merged 4 commits into from
Jun 1, 2018
Merged

Add a regex to check different mp4 suffix. #1734

merged 4 commits into from
Jun 1, 2018

Conversation

oldmtn
Copy link
Contributor

@oldmtn oldmtn commented May 28, 2018

This PR will add regex to check mp4 suffix.

Why is this Pull Request needed?

The original code need a an regex to handle other type of suffix.

Are there any points in the code the reviewer needs to double check?

Resolves issues:

Fixed old "FIXME".

Checklist

  • changes have been done against master branch, and PR does not conflict
  • no commits have been done in dist folder (we will take care of updating it)
  • new unit / functional tests have been added (whenever applicable)
  • API or design changes are documented in API.md

Copy link
Member

@tjenkinson tjenkinson left a comment

Choose a reason for hiding this comment

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

Please can you extract the regex outside the function to a constant, maybe at the top of the file? Otherwise it has to be evaluated every time this runs.

Also I think you need to escape the '.' s otherwise they mean any character

 /\.(mp4|m4s|m4v|m4a)$/i

Should do it and mean you don't need to convert to lower case

@tjenkinson
Copy link
Member

Also I think this could just be a standard function. It doesn't need to be exported given nothing else is using it. If minifies better that way

@oldmtn
Copy link
Contributor Author

oldmtn commented May 29, 2018

you don't need to convert to lower case

Some file suffix could be .MP4 or .M4S, so why don't need to convert to lower case.

@tjenkinson
Copy link
Member

@oldmtn the /i flag makes the regex case insensitive. Also creating RegExp isn't needed here.

const MP4_SUFFIX = /\.(mp4|m4s|m4v|m4a)$/i;

function checkMP4Suffix(str) {
  return MP4_SUFFIX.test(str)
}

should be fine, and I'd then remove the function and just inline that

if (level.fragments.every((frag) => MP4_SUFFIX.test(frag.relurl))) {

Copy link
Member

@tjenkinson tjenkinson left a comment

Choose a reason for hiding this comment

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

Great

@tjenkinson tjenkinson merged commit a7e49cb into video-dev:master Jun 1, 2018
@tjenkinson
Copy link
Member

thanks @oldmtn

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.

None yet

2 participants