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.

Sign up for GitHub

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

Jump to bottom

Fix: Retain quality setting when setting/switching audio tracks #720

Merged
merged 6 commits into from
Mar 29, 2018
Merged

Fix: Retain quality setting when setting/switching audio tracks #720

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0530593
Fix: Retain quality setting when setting/switching audio tracks
Mar 15, 2018
c64d23b
Chore: Responding to comments and adding tests
Mar 16, 2018
837502b
Merge branch 'master' into video
DanDeMicco Mar 27, 2018
4f7ebaa
Merge branch 'master' into video
DanDeMicco Mar 27, 2018
e5eb4d5
Merge branch 'master' into video
Mar 28, 2018
3a9f0d6
Merge branch 'master' into video
Mar 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 44 additions & 20 deletions src/lib/viewers/media/DashViewer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ class DashViewer extends VideoBaseViewer {
this.loadeddataHandler = this.loadeddataHandler.bind(this);
this.adaptationHandler = this.adaptationHandler.bind(this);
this.shakaErrorHandler = this.shakaErrorHandler.bind(this);
this.shakaManifestLoadedHandler = this.shakaManifestLoadedHandler.bind(this);
this.requestFilter = this.requestFilter.bind(this);
this.handleQuality = this.handleQuality.bind(this);
this.handleSubtitle = this.handleSubtitle.bind(this);
Expand Down Expand Up @@ -152,10 +153,11 @@ class DashViewer extends VideoBaseViewer {
this.adapting = true;
this.player = new shaka.Player(this.mediaEl);
this.player.addEventListener('adaptation', this.adaptationHandler);
this.player.addEventListener('streaming', this.shakaManifestLoadedHandler);
this.player.addEventListener('error', this.shakaErrorHandler);
this.player.configure({
abr: {
enabled: true
enabled: false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Setting this to false by default, it will get overridden if 'auto' is saved in our cache as the current quality setting.

},
streaming: {
bufferingGoal: MAX_BUFFER,
Expand Down Expand Up @@ -239,6 +241,25 @@ class DashViewer extends VideoBaseViewer {
}
}

/**
* Given a an audio ID (e.g. english track audio ID), enables the track with that audio ID
* while maintaining the SAME VIDEO as the active track.
*
* @private
* @param {number} role - The role of the audio used in the variant (provided by Shaka)
* @return {void}
*/
enableAudioId(role) {
const tracks = this.player.getVariantTracks();
const activeTrack = this.getActiveTrack();
// We select a track that has the desired audio role but maintains the same video ID as our currently active track.
const newTrack = tracks.find((track) => track.roles[0] === role && track.videoId === activeTrack.videoId);
Copy link
Contributor

Choose a reason for hiding this comment

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

a comment here would be good

if (newTrack && newTrack.audioId !== activeTrack.audioId) {
this.showLoadingIcon(newTrack.id);
this.player.selectVariantTrack(newTrack, true);
}
}

/**
* Enables or disables automatic adaptation
*
Expand Down Expand Up @@ -280,9 +301,9 @@ class DashViewer extends VideoBaseViewer {
*/
handleAudioTrack() {
const audioIdx = parseInt(this.cache.get('media-audiotracks'), 10);
if (this.audioTracks[audioIdx] !== undefined) {
const track = this.audioTracks[audioIdx];
this.player.selectAudioLanguage(track.language, track.role);
const newAudioTrack = this.audioTracks[audioIdx];
if (newAudioTrack !== undefined) {
this.enableAudioId(newAudioTrack.role);
Copy link
Contributor

Choose a reason for hiding this comment

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

This'll throw an error if newAudioTrack is anything but an Object, due to accessing role, and only checking if it's not undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Dang, I gotcha. So if there can only ever be undefined OR an object like this, that sounds fine by me as is

}
}

Expand Down Expand Up @@ -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.Player.html#event:StreamingEvent
*
* @private
* @param {Object} shakaError - Error to handle
* @return {void}
*/
shakaManifestLoadedHandler() {
this.calculateVideoDimensions();
this.loadUI();

if (this.hdVideoId !== -1) {
this.mediaControls.enableHDSettings();
}

this.loadSubtitles();
this.loadAlternateAudio();
}

/**
* Adds event listeners to the media controls.
* Makes changes to the media element.
Expand Down Expand Up @@ -463,15 +503,10 @@ class DashViewer extends VideoBaseViewer {
this.autoplay();
}

this.calculateVideoDimensions();
this.loadUI();
this.loadFilmStrip();
this.resize();
this.handleVolume();
this.startBandwidthTracking();
this.handleQuality(); // should come after gettings rep ids
this.loadSubtitles();
this.loadAlternateAudio();
this.showPlayButton();

this.loaded = true;
Expand All @@ -483,17 +518,6 @@ class DashViewer extends VideoBaseViewer {
this.mediaContainerEl.focus();
}

/**
* @inheritdoc
*/
loadUI() {
super.loadUI();

if (this.hdVideoId !== -1) {
this.mediaControls.enableHDSettings();
}
}

/**
* Loads the film strip
*
Expand Down
128 changes: 90 additions & 38 deletions src/lib/viewers/media/__tests__/DashViewer-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ describe('lib/viewers/media/DashViewer', () => {

dash.mediaControls = {
addListener: () => {},
enableHDSettings: () => {},
destroy: () => {},
initFilmstrip: () => {},
initSubtitles: () => {},
Expand Down Expand Up @@ -216,6 +217,7 @@ describe('lib/viewers/media/DashViewer', () => {
dash.mediaUrl = 'url';
sandbox.stub(shaka, 'Player').returns(dash.player);
stubs.mockPlayer.expects('addEventListener').withArgs('adaptation', sinon.match.func);
stubs.mockPlayer.expects('addEventListener').withArgs('streaming', sinon.match.func);
stubs.mockPlayer.expects('addEventListener').withArgs('error', sinon.match.func);
stubs.mockPlayer.expects('configure');
stubs.mockPlayer.expects('load').withArgs('url').returns(Promise.resolve());;
Expand Down Expand Up @@ -357,6 +359,53 @@ describe('lib/viewers/media/DashViewer', () => {
});
});

describe('enableAudioId()', () => {
it('should enable audioId while maintaining the same video ID', () => {
const variant1 = { id: 1, videoId: 1, audioId: 5, active: false, roles: ['1'] };
const variant2 = { id: 2, videoId: 2, audioId: 5, active: false, roles: ['1'] };
const variant3 = { id: 3, videoId: 1, audioId: 6, active: false, roles: ['2'] };
const variant4 = { id: 4, videoId: 2, audioId: 6, active: true, roles: ['2'] };
const variant5 = { id: 5, videoId: 1, audioId: 7, active: false, roles: ['3'] };
const variant6 = { id: 6, videoId: 2, audioId: 7, active: false, roles: ['3'] };
stubs.mockPlayer
.expects('getVariantTracks')
.returns([variant1, variant2, variant3, variant4, variant5, variant6]);
sandbox.stub(dash, 'getActiveTrack').returns(variant4);
sandbox.stub(dash, 'showLoadingIcon');
stubs.mockPlayer.expects('selectVariantTrack').withArgs(variant6, true);

dash.enableAudioId('3');

expect(dash.showLoadingIcon).to.be.calledWith(6);
});

it('should do nothing if enabling a audioId which is already active', () => {
const variant1 = { id: 1, videoId: 1, audioId: 5, active: false, roles: ['1'] };
const variant2 = { id: 2, videoId: 2, audioId: 6, active: true, roles: ['2'] };
stubs.mockPlayer.expects('getVariantTracks').returns([variant1, variant2]);
sandbox.stub(dash, 'getActiveTrack').returns(variant2);
sandbox.stub(dash, 'showLoadingIcon');
stubs.mockPlayer.expects('selectVariantTrack').never();

dash.enableAudioId('2');

expect(dash.showLoadingIcon).to.not.be.called;
});

it('should do nothing if enabling an invalid audioId', () => {
const variant1 = { id: 1, videoId: 1, audioId: 5, active: false, roles: ['1'] };
const variant2 = { id: 2, videoId: 2, audioId: 6, active: true, roles: ['2'] };
stubs.mockPlayer.expects('getVariantTracks').returns([variant1, variant2]);
sandbox.stub(dash, 'getActiveTrack').returns(variant2);
sandbox.stub(dash, 'showLoadingIcon');
stubs.mockPlayer.expects('selectVariantTrack').never();

dash.enableAudioId(-1);

expect(dash.showLoadingIcon).to.not.be.called;
});
});

describe('enableAdaptation()', () => {
it('should configure player to enable adaptation by default', () => {
stubs.mockPlayer.expects('configure').withArgs({ abr: { enabled: true } });
Expand Down Expand Up @@ -535,6 +584,40 @@ describe('lib/viewers/media/DashViewer', () => {
});
});

describe('shakaManifestLoadedHandler()', () => {
beforeEach(() => {
sandbox.stub(dash, 'calculateVideoDimensions');
sandbox.stub(dash, 'loadUI');
sandbox.stub(dash, 'loadSubtitles');
sandbox.stub(dash, 'loadAlternateAudio');
});

it('should calculate video dimensions and load UI', () => {
dash.shakaManifestLoadedHandler();

expect(dash.calculateVideoDimensions).to.be.called;
expect(dash.loadUI).to.be.called;
});

it('should enable HD settings if there is an HD rep', () => {
dash.hdVideoId = -1;
stubs.mockControls.expects('enableHDSettings').never();
dash.shakaManifestLoadedHandler();

dash.hdVideoId = 1;
stubs.mockControls.expects('enableHDSettings');
dash.shakaManifestLoadedHandler();

});

it('should load subtitles and additional audio tracks', () => {
dash.shakaManifestLoadedHandler();

expect(dash.loadSubtitles).to.be.called;
expect(dash.loadAlternateAudio).to.be.called;
});
});

describe('addEventListenersForMediaControls()', () => {
const listenerFunc = DashViewer.prototype.addEventListenersForMediaControls;

Expand Down Expand Up @@ -568,60 +651,24 @@ describe('lib/viewers/media/DashViewer', () => {
sandbox.stub(dash, 'showMedia');
sandbox.stub(dash, 'isAutoplayEnabled').returns(true);
sandbox.stub(dash, 'autoplay');
sandbox.stub(dash, 'calculateVideoDimensions');
sandbox.stub(dash, 'loadUI');
sandbox.stub(dash, 'loadFilmStrip');
sandbox.stub(dash, 'loadAlternateAudio');
sandbox.stub(dash, 'resize');
sandbox.stub(dash, 'handleVolume');
sandbox.stub(dash, 'startBandwidthTracking');
sandbox.stub(dash, 'handleQuality');
sandbox.stub(dash, 'loadSubtitles');
sandbox.stub(dash, 'showPlayButton');

dash.loadeddataHandler();
expect(dash.autoplay).to.be.called;
expect(dash.showMedia).to.be.called;
expect(dash.showPlayButton).to.be.called;
expect(dash.loadSubtitles).to.be.called;
expect(dash.loadAlternateAudio).to.be.called;
expect(dash.emit).to.be.calledWith(VIEWER_EVENT.load);
expect(dash.loaded).to.be.true;
expect(document.activeElement).to.equal(dash.mediaContainerEl);
expect(dash.mediaControls.show).to.be.called;
});
});

describe('loadUI()', () => {
beforeEach(() => {
stubs.loadUI = DashViewer.prototype.loadUI;
dash.mediaControls = {
enableHDSettings: sandbox.stub(),
removeListener: sandbox.stub(),
removeAllListeners: sandbox.stub(),
destroy: sandbox.stub()
};

Object.defineProperty(VideoBaseViewer.prototype, 'loadUI', { value: sandbox.mock() });
});

afterEach(() => {
Object.defineProperty(VideoBaseViewer.prototype, 'loadUI', { value: stubs.loadUI });
});

it('should enable HD settings if an HD rep exists', () => {
dash.hdVideoId = 3;
dash.loadUI();
expect(dash.mediaControls.enableHDSettings).to.be.called;
});

it('should do nothing if there is no HD rep', () => {
dash.hdVideoId = -1;
dash.loadUI();
expect(dash.mediaControls.enableHDSettings).to.not.be.called;
});
});

describe('loadFilmStrip()', () => {
beforeEach(() => {
dash.options = {
Expand Down Expand Up @@ -853,16 +900,20 @@ describe('lib/viewers/media/DashViewer', () => {
});

describe('handleAudioTrack()', () => {
beforeEach(() => {
sandbox.stub(dash, 'enableAudioId');
});

it('should select correct audio', () => {
dash.audioTracks = [
{ language: 'eng', role: 'audio0' },
{ language: 'eng', role: 'audio1' },
{ language: 'eng', role: 'audio2' }
];
sandbox.stub(dash.cache, 'get').returns('1');
stubs.mockPlayer.expects('selectAudioLanguage').withArgs('eng', 'audio1');

dash.handleAudioTrack();
expect(dash.enableAudioId).to.be.calledWith('audio1')
});

it('should not select audio if index out of bounds', () => {
Expand All @@ -872,9 +923,10 @@ describe('lib/viewers/media/DashViewer', () => {
{ language: 'eng', role: 'audio2' }
];
sandbox.stub(dash.cache, 'get').returns('3');
stubs.mockPlayer.expects('selectAudioLanguage').never();

dash.handleAudioTrack();
expect(dash.enableAudioId).to.not.be.called;

});
});

Expand Down