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: Handle audio only audio track change correctly #1100

Merged
merged 23 commits into from
Apr 6, 2021
Merged

fix: Handle audio only audio track change correctly #1100

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
50615df
fix: Handle audio only playlists without main playlists
brandonocasey Mar 15, 2021
7270777
fix jsdocs
brandonocasey Mar 30, 2021
b48ee53
code review
brandonocasey Mar 30, 2021
1c6b5af
Merge branch 'main' into fix/audio-only-groups
brandonocasey Mar 30, 2021
2da9d29
fixes from code review
brandonocasey Mar 30, 2021
7ad23c7
test fixes
brandonocasey Mar 30, 2021
c957dde
more test fixes
brandonocasey Mar 30, 2021
8005060
code review
brandonocasey Apr 5, 2021
812205a
fix tests
brandonocasey Apr 5, 2021
d0353d2
Merge remote-tracking branch 'origin/main' into fix/audio-only-groups
brandonocasey Apr 5, 2021
7ce84dc
add tests for isAudioOnly/playlistMatch
brandonocasey Apr 5, 2021
827edce
test audio only selector
brandonocasey Apr 5, 2021
ceece05
media group test improvements
brandonocasey Apr 5, 2021
01396be
Merge remote-tracking branch 'origin/main' into fix/audio-only-groups
brandonocasey Apr 5, 2021
83b3e7c
test more code
brandonocasey Apr 5, 2021
fba0523
test getAudioTrackPlaylists_
brandonocasey Apr 5, 2021
4a4813f
ie 11 fix
brandonocasey Apr 5, 2021
46b00e7
code review
brandonocasey Apr 6, 2021
c1b4501
improve coverage
brandonocasey Apr 6, 2021
94bdda3
switch instantly to alternative main audio
brandonocasey Apr 6, 2021
5a485ab
Revert "switch instantly to alternative main audio"
brandonocasey Apr 6, 2021
5691211
Update test/media-groups.test.js
brandonocasey Apr 6, 2021
a9bfc51
switch to assert true/false
brandonocasey Apr 6, 2021
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
4 changes: 2 additions & 2 deletions src/master-playlist-controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,8 +352,8 @@ export class MasterPlaylistController extends videojs.EventTarget {

// if we don't have any demuxed audio tracks
// return an empty array
if (!master && !master.mediaGroups && !master.mediaGroups.AUDIO) {
return [];
if (!master || !master.mediaGroups || !master.mediaGroups.AUDIO) {
return master && master.playlists;
Copy link
Contributor

Choose a reason for hiding this comment

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

If there's no master, is it OK that this returns undefined? Would it be better to return []?

The comment above may need to be changed too otherwise. And would returning master.playlists end up returning non audio only tracks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will make it return an empty array without a master. It will could end up returning non-audio playlists but if we don't have audio groups and this function is called, which should only happen if we detect that the playlist is audio only, Then we should just return the main playlists.

}

const AUDIO = master.mediaGroups.AUDIO;
Expand Down
59 changes: 59 additions & 0 deletions test/master-playlist-controller.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,64 @@ const sharedHooks = {

QUnit.module('MasterPlaylistController', sharedHooks);

QUnit.test('getAudioTrackPlaylists_', function(assert) {
const mpc = this.masterPlaylistController;
const master = {playlists: [{uri: 'testing'}]};

mpc.master = () => master;

assert.deepEqual(
mpc.getAudioTrackPlaylists_(),
master.playlists,
'no media groups, return main playlists'
);

master.mediaGroups = {
AUDIO: {
main: {
en: {default: true, label: 'en', playlists: [{uri: 'foo'}, {uri: 'bar'}]},
fr: {label: 'fr', playlists: [{uri: 'foo-fr'}, {uri: 'bar-fr'}]}
},
alt: {
en: {default: true, label: 'en', playlists: [{uri: 'fizz'}, {uri: 'bazz'}]},
fr: {label: 'fr', playlists: [{uri: 'fizz-fr'}, {uri: 'bazz-fr'}]}
}
}
};

assert.deepEqual(mpc.getAudioTrackPlaylists_(), [
{uri: 'foo'},
{uri: 'bar'},
{uri: 'fizz'},
{uri: 'bazz'}
], 'returns all dash style en playlist');

mpc.mediaTypes_.AUDIO.groups = {
main: Object.values(master.mediaGroups.AUDIO.main),
alt: Object.values(master.mediaGroups.AUDIO.alt)
};
mpc.mediaTypes_.AUDIO.activeTrack = () => ({label: 'fr'});

assert.deepEqual(mpc.getAudioTrackPlaylists_(), [
{uri: 'foo-fr'},
{uri: 'bar-fr'},
{uri: 'fizz-fr'},
{uri: 'bazz-fr'}
], 'returns all dash style fr playlists');

delete master.mediaGroups.AUDIO.main.fr.playlists;
master.mediaGroups.AUDIO.main.fr.uri = 'fizz-fr';

delete master.mediaGroups.AUDIO.alt.fr.playlists;
master.mediaGroups.AUDIO.alt.fr.uri = 'buzz-fr';

assert.deepEqual(mpc.getAudioTrackPlaylists_(), [
{uri: 'fizz-fr', label: 'fr'},
{uri: 'buzz-fr', label: 'fr'}
], 'returns all fr hls style playlists');

});

QUnit.test('throws error when given an empty URL', function(assert) {
const options = {
src: 'test',
Expand Down Expand Up @@ -5886,3 +5944,4 @@ QUnit.test('should delay loading of new playlist if lastRequest was less than ha

this.env.log.warn.callCount = 0;
});