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

Remove cue position alignment by adding 50 for Firefox #2229

Merged
merged 1 commit into from
May 7, 2019

Conversation

mackode
Copy link
Contributor

@mackode mackode commented Apr 21, 2019

This PR will...

Remove IMO unnecessary cue position alignment.

Why is this Pull Request needed?

Cues are wrongly positioned in current Firefox. Why we use const (50) for Firefox?

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

Yes, if we keep backward compatibility, maybe this is still needed on older Firefoxes.

Resolves issues:

Checklist

  • changes have been done against master branch, and PR does not conflict
  • new unit / functional tests have been added (whenever applicable)
  • API or design changes are documented in API.md

@itsjamie
Copy link
Collaborator

The default value for cue positions must have changed at some Firefox version.

Copy link
Member

@michaelcunningham19 michaelcunningham19 left a comment

Choose a reason for hiding this comment

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

Briefly tested this out with some assets with subtitles. It works fine in Firefox without this browser-specific compensation.

However, the clamping rule (0-100) is still enforced by the browser(s). From my testing on Firefox 66 and Chrome 73, both throw exceptions when setting position out of that range.

It would be nice to know more about this Firefox-specific compensation.. @johnBartos do you have any details to share there? You were the last one to make a change in this code: b7f8468 - by adding the clamp 0 <-> 100

@michaelcunningham19 michaelcunningham19 added Browser issue If there is an underlying issue with the browser that hls.js is running on, this tag should be used. browser: Firefox Needs review Needs testing labels Apr 24, 2019
mackode added a commit to castlabs/hls.js that referenced this pull request Apr 29, 2019
@johnBartos
Copy link
Collaborator

@michaelcunningham19 It looks like this code should have never been committed as such: jwplayer#87. We removed this from our fork long ago, but neglected to upstream that PR. Thanks for fixing this @mackode

@johnBartos johnBartos merged commit 634946a into video-dev:master May 7, 2019
@johnBartos johnBartos added this to In progress in 0.13.0 via automation May 7, 2019
@johnBartos johnBartos added this to the 0.12.5 milestone Jul 26, 2019
@johnBartos johnBartos removed this from In progress in 0.13.0 Jul 26, 2019
@johnBartos johnBartos modified the milestones: 0.12.5, 0.13.0 Aug 13, 2019
@robwalch robwalch added this to Done in 0.13.0 Oct 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
browser: Firefox Browser issue If there is an underlying issue with the browser that hls.js is running on, this tag should be used. Bug
Projects
No open projects
0.13.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants