Skip to content
This repository has been archived by the owner on Jan 12, 2019. It is now read-only.

Fix qualityLevels setup for video with source #979

Merged
merged 5 commits into from
Jan 27, 2017

Conversation

ddunkin
Copy link
Contributor

@ddunkin ddunkin commented Jan 25, 2017

Description

If a player is created from a <video> with a <source>, qualityLevels integration is not set up.

Specific Changes proposed

videojs.players[playerId] is not set yet when setupQualityLevels_ is called, but the player is already associated with the tag. Use the videojs function to get the player instead.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
  • Reviewed by Two Core Contributors

with a source.

videojs.players[playerId] is not set yet when setupQualityLevels_ is
called, but the player is already associated with the tag.
@mjneil
Copy link
Contributor

mjneil commented Jan 25, 2017

Thanks for catching this bug and submitting a fix for it!

After some conversation with @gkatsev, videojs.players[playerId] should be set and how we should be access the player. It seems that since the Hls object is registered as a component, it gets initiated when the tech is created before videojs.players[playerId] is set. Calling videojs(playerId) to access the player before it is finished initializing could potentially cause a errors or a re-initialization of the player. Can you update your PR so that setupQualityLevels_ isn't called until the ready event is fired by the tech (or use tech.ready(callback). This should fix the issue and avoid any potential unwanted side effects.

@@ -547,7 +547,7 @@ class HlsHandler extends Component {
this.ignoreNextSeekingEvent_ = true;
});

this.setupQualityLevels_();
this.tech_.ready(this.setupQualityLevels_.bind(this));
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need .bind(this). Video.js takes care of that for you.

Copy link
Member

Choose a reason for hiding this comment

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

er, wait, never mind. Ignore me. However, we shouldn't use .bind because it's slow and doesn't run everywhere where we want to run.
You should be able to use an arrow function instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha, I used an arrow function then changed it to bind because I saw it used elsewhere.

@ddunkin
Copy link
Contributor Author

ddunkin commented Jan 26, 2017

There's a call to videojs() in the HlsHandler constructor.

let _player = videojs(tech.options_.playerId);

Is that also problematic for the reasons above?

@mjneil
Copy link
Contributor

mjneil commented Jan 26, 2017

I do believe for the same reasons that call could also be problematic, but for the purposes of this PR let's not worry about it. It'll be better to investigate that separately.

This looks good to me

@ddunkin
Copy link
Contributor Author

ddunkin commented Jan 27, 2017

@gkatsev I made the requested changes. Does it look good?

@mjneil mjneil merged commit 97f8ff4 into videojs:master Jan 27, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants