Fix: Retain quality setting when setting/switching audio tracks #720
Conversation
|
Verified that @jeremypress has signed the CLA. Thanks for the pull request! |
| this.player.addEventListener('error', this.shakaErrorHandler); | ||
| this.player.configure({ | ||
| abr: { | ||
| enabled: true | ||
| enabled: false |
There was a problem hiding this comment.
Setting this to false by default, it will get overridden if 'auto' is saved in our cache as the current quality setting.
src/lib/viewers/media/DashViewer.js
Outdated
| * @param {Object} shakaError - Error to handle | ||
| * @return {void} | ||
| */ | ||
| shakaManifestHandler() { |
There was a problem hiding this comment.
I only moved things that are dependent on the manifest here. This should give a slight performance gain as we will zero in on our target quality faster than if we waited for loadeddata.
There was a problem hiding this comment.
💯 I should look into adding startAt here as well. I think there was a bug which I had to work around before but pretty sure they fixed it
src/lib/viewers/media/DashViewer.js
Outdated
| const track = this.audioTracks[audioIdx]; | ||
| this.player.selectAudioLanguage(track.language, track.role); | ||
| this.enableAudioId(track.role); |
There was a problem hiding this comment.
this should be newAudioTrack, delete previous line
src/lib/viewers/media/DashViewer.js
Outdated
| * @param {Object} shakaError - Error to handle | ||
| * @return {void} | ||
| */ | ||
| shakaManifestHandler() { |
There was a problem hiding this comment.
💯 I should look into adding startAt here as well. I think there was a bug which I had to work around before but pretty sure they fixed it
src/lib/viewers/media/DashViewer.js
Outdated
| enableAudioId(role) { | ||
| const tracks = this.player.getVariantTracks(); | ||
| const activeTrack = this.getActiveTrack(); | ||
| const newTrack = tracks.find((track) => track.roles[0] === role && track.videoId === activeTrack.videoId); |
There was a problem hiding this comment.
a comment here would be good
src/lib/viewers/media/DashViewer.js
Outdated
| @@ -387,6 +408,25 @@ class DashViewer extends VideoBaseViewer { | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
| * Handles streaming event which is the first time the manifest is available. See https://shaka-player-demo.appspot.com/docs/api/shaka.util.Error.html | |||
There was a problem hiding this comment.
Update these docs. Looks like you're no longer handling/accepting a shakaError param
| dash.startTimeInSeconds = START_TIME_IN_SECONDS; | ||
| sandbox.stub(shaka, 'Player').returns(dash.player); | ||
| stubs.mockPlayer.expects('load').withArgs('url', START_TIME_IN_SECONDS).returns(Promise.resolve()); | ||
| // it('should load the player with the start time', () => { |
There was a problem hiding this comment.
did you mean to comment this out?
| sandbox.stub(shaka, 'Player').returns(dash.player); | ||
| stubs.mockPlayer.expects('load').withArgs('url', START_TIME_IN_SECONDS).returns(Promise.resolve()); | ||
| // it('should load the player with the start time', () => { | ||
| // const START_TIME_IN_SECONDS = 3; |
src/lib/viewers/media/DashViewer.js
Outdated
| this.loadUI(); | ||
|
|
||
| this.player.configure({ | ||
| playRangeStart: this.startTimeInSeconds |
src/lib/viewers/media/DashViewer.js
Outdated
| @@ -171,7 +173,7 @@ class DashViewer extends VideoBaseViewer { | |||
| this.player.getNetworkingEngine().registerRequestFilter(this.requestFilter); | |||
|
|
|||
| this.startLoadTimer(); | |||
| this.player.load(this.mediaUrl, this.startTimeInSeconds).catch(this.shakaErrorHandler); | |||
| this.player.load(this.mediaUrl).catch(this.shakaErrorHandler); | |||
There was a problem hiding this comment.
add the startAt back here
| this.player.selectAudioLanguage(track.language, track.role); | ||
| const newAudioTrack = this.audioTracks[audioIdx]; | ||
| if (newAudioTrack !== undefined) { | ||
| this.enableAudioId(newAudioTrack.role); |
There was a problem hiding this comment.
This'll throw an error if newAudioTrack is anything but an Object, due to accessing role, and only checking if it's not undefined
There was a problem hiding this comment.
this is how we build the audio tracks
this.audioTracks = uniqueAudioVariants.map((track) => ({
language: track.language,
role: track.roles[0]
}));
We make this assumption that tracks coming back from the manifest are objects all over the code. Should I trust this as a fact or add the check everywhere else?
There was a problem hiding this comment.
Dang, I gotcha. So if there can only ever be undefined OR an object like this, that sounds fine by me as is
This fixes a few bugs around track selection by shaka player. We will now retain the current quality when changing audio tracks.
Tests on the way