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.

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

fix: do not add placeholder properties for AUDIO groups without a uri #1167

Merged
merged 4 commits into from
Jul 19, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
17 changes: 16 additions & 1 deletion src/manifest.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import videojs from 'video.js';
import window from 'global/window';
import { Parser as M3u8Parser } from 'm3u8-parser';
import { resolveUrl } from './resolve-url';
import { getLastParts } from './playlist.js';
import { getLastParts, isAudioOnly } from './playlist.js';

const { log } = videojs;

Expand Down Expand Up @@ -279,11 +279,26 @@ export const addPropertiesToMaster = (master, uri) => {
master.playlists[i].uri = phonyUri;
}
}
const audioOnlyMaster = isAudioOnly(master);

forEachMediaGroup(master, (properties, mediaType, groupKey, labelKey) => {
const groupId = `placeholder-uri-${mediaType}-${groupKey}-${labelKey}`;

// add a playlist array under properties
if (!properties.playlists || !properties.playlists.length) {
// If the manifest is audio only and this media group does not have a uri, check
// if the media group is located in the main list of playlists. If it is, don't add
// placeholder properties as it shouldn't be considered an alternate audio track.
if (audioOnlyMaster && mediaType === 'AUDIO' && !properties.uri) {
for (let i = 0; i < master.playlists.length; i++) {
const p = master.playlists[i];

if (p.attributes && p.attributes.AUDIO && p.attributes.AUDIO === groupKey) {
return;
}
}
}

properties.playlists = [Object.assign({}, properties)];
}

Expand Down
24 changes: 20 additions & 4 deletions src/master-playlist-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -397,12 +397,13 @@ export class MasterPlaylistController extends videojs.EventTarget {
*/
getAudioTrackPlaylists_() {
const master = this.master();
const defaultPlaylists = master && master.playlists || [];

// if we don't have any audio groups then we can only
// assume that the audio tracks are contained in masters
// playlist array, use that or an empty array.
if (!master || !master.mediaGroups || !master.mediaGroups.AUDIO) {
return master && master.playlists || [];
return defaultPlaylists;
}

const AUDIO = master.mediaGroups.AUDIO;
Expand All @@ -427,7 +428,7 @@ export class MasterPlaylistController extends videojs.EventTarget {

// no active track no playlists.
if (!track) {
return [];
return defaultPlaylists;
}

const playlists = [];
Expand All @@ -438,14 +439,29 @@ export class MasterPlaylistController extends videojs.EventTarget {
if (AUDIO[group][track.label]) {
const properties = AUDIO[group][track.label];

if (properties.playlists) {
if (properties.playlists && properties.playlists.length) {
playlists.push.apply(playlists, properties.playlists);
} else {
} else if (properties.uri) {
playlists.push(properties);
} else if (master.playlists.length) {
// if an audio group does not have a uri
// see if we have main playlists that use it as a group.
// if we do then add those to the playlists list.
for (let i = 0; i < master.playlists.length; i++) {
const playlist = master.playlists[i];

if (playlist.attributes && playlist.attributes.AUDIO && playlist.attributes.AUDIO === group) {
playlists.push(playlist);
}
}
}
}
}

if (!playlists.length) {
return defaultPlaylists;
}

return playlists;
}

Expand Down
19 changes: 19 additions & 0 deletions test/manifest.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -461,3 +461,22 @@ QUnit.module('manifest', function() {
);
});
});

QUnit.test('does not add placeholder properties if playlist is a master playlist', function(assert) {
const master = {
mediaGroups: {
AUDIO: {
default: {
en: {default: true}
}
}
},
playlists: [
{uri: 'foo', attributes: {AUDIO: 'default', CODECS: 'mp4a.40.2', BANDWIDTH: 20}}
]
};

addPropertiesToMaster(master, 'some-uri');

assert.notOk(master.mediaGroups.AUDIO.default.en.playlists, 'did not add playlists');
});
94 changes: 93 additions & 1 deletion test/master-playlist-controller.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ const sharedHooks = {

QUnit.module('MasterPlaylistController', sharedHooks);

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

Expand Down Expand Up @@ -176,6 +176,98 @@ QUnit.test('getAudioTrackPlaylists_', function(assert) {

});

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

mpc.master = () => master;

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

assert.deepEqual(
mpc.getAudioTrackPlaylists_(),
master.playlists,
'no default track, returns master playlists.'
);

mpc.mediaTypes_.AUDIO.groups = {foo: [{}]};
mpc.mediaTypes_.AUDIO.activeTrack = () => null;

assert.deepEqual(
mpc.getAudioTrackPlaylists_(),
master.playlists,
'no active track, returns master playlists.'
);

});

QUnit.test('getAudioTrackPlaylists_ with track but groups are main playlists', function(assert) {
const mpc = this.masterPlaylistController;
const master = {playlists: [
{uri: '720-audio', attributes: {AUDIO: '720'}},
{uri: '1080-audio', attributes: {AUDIO: '1080'}}
]};

mpc.master = () => master;

master.mediaGroups = {
AUDIO: {
720: {
audio: {default: true, label: 'audio'}
},
1080: {
audio: {default: true, label: 'audio'}
}
}
};

mpc.mediaTypes_.AUDIO.groups = {foo: [{}]};
mpc.mediaTypes_.AUDIO.activeTrack = () => ({label: 'audio'});

assert.deepEqual(
mpc.getAudioTrackPlaylists_(),
[master.playlists[0], master.playlists[1]],
'returns all audio label playlists'
);
});

QUnit.test('getAudioTrackPlaylists_ invalid audio groups', function(assert) {
const mpc = this.masterPlaylistController;
const master = {playlists: [
{uri: 'foo-playlist'},
{uri: 'bar-playlist'}
]};

mpc.master = () => master;

master.mediaGroups = {
AUDIO: {
720: {
audio: {default: true, label: 'audio'}
},
1080: {
audio: {default: true, label: 'audio'}
}
}
};

mpc.mediaTypes_.AUDIO.groups = {foo: [{}]};
mpc.mediaTypes_.AUDIO.activeTrack = () => ({label: 'audio'});

assert.deepEqual(
mpc.getAudioTrackPlaylists_(),
master.playlists,
'returns all master playlists'
);
});

QUnit.test('throws error when given an empty URL', function(assert) {
const options = {
src: 'test',
Expand Down