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

fix: CEA708 subtitle doesn't get correct index on native #514

Merged
merged 8 commits into from
Dec 27, 2020

Conversation

Yuvalke
Copy link
Contributor

@Yuvalke Yuvalke commented Nov 29, 2020

Description of the Changes

Issue: addTrack on safari add the track to the first position on TextTrackList.
Solution: register after native tracks loaded to addtexttrack to make sure if new tracks adding on index 0 it'll change the indexes for all tracks otherwise it won't change it.

CheckLists

  • changes have been done against master branch, and PR does not conflict
  • new unit / functional tests have been added (whenever applicable)
  • test are passing in local environment
  • Travis tests are passing (or test results are not worse than on master branch :))
  • Docs have been updated

@Yuvalke Yuvalke requested a review from a team November 29, 2020 09:33
@Yuvalke Yuvalke self-assigned this Nov 29, 2020
src/player.js Show resolved Hide resolved
* @private
*/
_onTextTrackAdded(event: FakeEvent): void {
const videoElement = this.getVideoElement();
Copy link
Contributor

Choose a reason for hiding this comment

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

we want to avoid accessing the native video element as much as possible, for example, this will fail for flash and youtube or any other external non HTML5 engine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@OrenMe why? get video element will be null and then nothing happens on the next check, otherwise, text tracks will be empty and if both are not empty for other engines it's ok that the logic will work

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, but this logic was not needed till now for non - native text track, so what has changed?
We should strive to get away from this and not add more use cases.

Copy link
Contributor Author

@Yuvalke Yuvalke Dec 27, 2020

Choose a reason for hiding this comment

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

Flash and youtube won't raise this event anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's for native text track only, listener added only for useNativeTextTrack=true

@Yuvalke Yuvalke merged commit d87e7b1 into master Dec 27, 2020
@Yuvalke Yuvalke deleted the fix-indexes-external branch December 27, 2020 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants