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

Fix fast quality change for alternate audio #1046

Merged
merged 2 commits into from
Mar 31, 2017
Merged

Fix fast quality change for alternate audio #1046

merged 2 commits into from
Mar 31, 2017

Conversation

gesinger
Copy link
Contributor

@gesinger gesinger commented Mar 8, 2017

No description provided.

@gesinger gesinger mentioned this pull request Mar 8, 2017
Merged
6 tasks
mjneil
mjneil reviewed Mar 16, 2017
View reviewed changes
src/master-playlist-controller.js
@@ -698,7 +698,7 @@ export class MasterPlaylistController extends videojs.EventTarget {
this.masterPlaylistLoader_.media(media);

this.mainSegmentLoader_.resetLoader();
if (this.audiosegmentloader_) {
Copy link
Contributor

@mjneil mjneil Mar 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

surprised this went unnoticed for so long being cased incorrectly 😄

mjneil
mjneil reviewed Mar 16, 2017
View reviewed changes
src/master-playlist-controller.js Outdated
@@ -698,7 +698,7 @@ export class MasterPlaylistController extends videojs.EventTarget {
this.masterPlaylistLoader_.media(media);

this.mainSegmentLoader_.resetLoader();
if (this.audiosegmentloader_) {
if (this.audioPlaylistLoader_) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why should this be audioPlaylistLoader_ vs audioSegmentLoader_?

Copy link
Contributor Author

@gesinger gesinger Mar 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

audioSegmentLoader_ always exists, even if not being used. audioPlaylistLoader_ only exists if we are using alt audio

@mjneil
Copy link
Contributor

mjneil commented Mar 17, 2017

Is it even necessary to reset the audio loader on fast quality change? If fastQualityChange_ causes a rendition switch, the mediachange event listener will call setUpAudio if the active audio group has changed. setUpAudio will tear down the playlist loader and reset the segment loader

@mjneil mjneil self-assigned this Mar 30, 2017
@imbcmdth
Copy link
Member

We decided to remove the code if it is redundant and to add a test to prove that it was not necessary.

@imbcmdth imbcmdth merged commit a134c68 into videojs:master Mar 31, 2017
@cmellinas cmellinas mentioned this pull request Nov 21, 2017
Closed
@ghost ghost mentioned this pull request Jul 2, 2018
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mjneil mjneil mjneil left review comments

Assignees

@mjneil mjneil

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gesinger @mjneil @imbcmdth