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

Remove extraneous methods from PlaylistLoader #1286

Merged
merged 4 commits into from
Nov 6, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
15 changes: 11 additions & 4 deletions src/master-playlist-controller.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
/**
* @file master-playlist-controller.js
*/
import PlaylistLoader from './playlist-loader';
import {
default as PlaylistLoader,
isLowestEnabledRendition
} from './playlist-loader';
import { isEnabled } from './playlist.js';
import SegmentLoader from './segment-loader';
import VTTSegmentLoader from './vtt-segment-loader';
import Ranges from './ranges';
Expand Down Expand Up @@ -336,7 +340,8 @@ export class MasterPlaylistController extends videojs.EventTarget {

// If we don't have any more available playlists, we don't want to
// timeout the request.
if (this.masterPlaylistLoader_.isLowestEnabledRendition_()) {
if (isLowestEnabledRendition(
this.masterPlaylistLoader_.master, this.masterPlaylistLoader_.media())) {
this.requestOptions_.timeout = 0;
} else {
this.requestOptions_.timeout = requestTimeout;
Expand Down Expand Up @@ -455,7 +460,8 @@ export class MasterPlaylistController extends videojs.EventTarget {

// If we don't have any more available playlists, we don't want to
// timeout the request.
if (this.masterPlaylistLoader_.isLowestEnabledRendition_()) {
if (isLowestEnabledRendition(
this.masterPlaylistLoader_.master, this.masterPlaylistLoader_.media())) {
this.requestOptions_.timeout = 0;
} else {
this.requestOptions_.timeout = requestTimeout;
Expand Down Expand Up @@ -866,7 +872,8 @@ export class MasterPlaylistController extends videojs.EventTarget {
}
}

let isFinalRendition = this.masterPlaylistLoader_.isFinalRendition_();
let isFinalRendition =
this.masterPlaylistLoader_.master.playlists.filter(isEnabled).length === 1;

if (isFinalRendition) {
// Never blacklisting this playlist because it's final rendition
Expand Down
84 changes: 35 additions & 49 deletions src/playlist-loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,18 @@ export const updateSegments = (original, update, offset) => {
return result;
};

export const resolveSegmentUris = (segment, baseUri) => {
if (!segment.resolvedUri) {
segment.resolvedUri = resolveUrl(baseUri, segment.uri);
}
if (segment.key && !segment.key.resolvedUri) {
segment.key.resolvedUri = resolveUrl(baseUri, segment.key.uri);
}
if (segment.map && !segment.map.resolvedUri) {
segment.map.resolvedUri = resolveUrl(baseUri, segment.map.uri);
}
};

/**
* Returns a new master playlist that is the result of merging an
* updated media playlist into the original version. If the
Expand Down Expand Up @@ -80,15 +92,7 @@ export const updateMaster = (master, media) => {

// resolve any segment URIs to prevent us from having to do it later
mergedPlaylist.segments.forEach((segment) => {
if (!segment.resolvedUri) {
segment.resolvedUri = resolveUrl(mergedPlaylist.resolvedUri, segment.uri);
}
if (segment.key && !segment.key.resolvedUri) {
segment.key.resolvedUri = resolveUrl(mergedPlaylist.resolvedUri, segment.key.uri);
}
if (segment.map && !segment.map.resolvedUri) {
segment.map.resolvedUri = resolveUrl(mergedPlaylist.resolvedUri, segment.map.uri);
}
resolveSegmentUris(segment, mergedPlaylist.resolvedUri);
});

// TODO Right now in the playlists array there are two references to each playlist, one
Expand Down Expand Up @@ -165,6 +169,28 @@ export const refreshDelay = (media, update) => {
return delay;
};

/*
* Returns whether the current playlist is the lowest rendition
*
* @return {Boolean} true if on lowest rendition
*/
export const isLowestEnabledRendition = (master, media) => {
Copy link
Contributor

@mjneil mjneil Oct 26, 2017

Choose a reason for hiding this comment

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

I feel like this would make more sense being in the playlist.js file now that its not tied to the playlist-loader instance

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. Looks much nicer there 👍

if (master.playlists.length === 1) {
return true;
}

const currentBandwidth = media.attributes.BANDWIDTH || Number.MAX_VALUE;

return (master.playlists.filter((playlist) => {
if (!isEnabled(playlist)) {
return false;
}

return (playlist.attributes.BANDWIDTH || 0) < currentBandwidth;

}).length === 0);
};

/**
* Load a playlist from a remote location
*
Expand Down Expand Up @@ -293,46 +319,6 @@ export default class PlaylistLoader extends EventTarget {
}
}

/**
* Returns the number of enabled playlists on the master playlist object
*
* @return {Number} number of eneabled playlists
*/
enabledPlaylists_() {
return this.master.playlists.filter(isEnabled).length;
}

/**
* Returns whether the current playlist is the lowest rendition
*
* @return {Boolean} true if on lowest rendition
*/
isLowestEnabledRendition_() {
if (this.master.playlists.length === 1) {
return true;
}

const currentBandwidth = this.media().attributes.BANDWIDTH || Number.MAX_VALUE;

return (this.master.playlists.filter((playlist) => {
if (!isEnabled(playlist)) {
return false;
}

return (playlist.attributes.BANDWIDTH || 0) < currentBandwidth;

}).length === 0);
}

/**
* Returns whether the current playlist is the final available rendition
*
* @return {Boolean} true if on final rendition
*/
isFinalRendition_() {
return (this.master.playlists.filter(isEnabled).length === 1);
}

/**
* When called without any arguments, returns the currently
* active media playlist. When called with a single argument,
Expand Down
14 changes: 9 additions & 5 deletions test/master-playlist-controller.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import { Hls } from '../src/videojs-contrib-hls';
/* eslint-enable no-unused-vars */
import Playlist from '../src/playlist';
import Config from '../src/config';
import { isLowestEnabledRendition } from '../src/playlist-loader';

const generateMedia = function(isMaat, isMuxed, hasVideoCodec, hasAudioCodec, isFMP4) {
const codec = (hasVideoCodec ? 'avc1.deadbeef' : '') +
Expand Down Expand Up @@ -1453,9 +1454,10 @@ function(assert) {
1000,
'default request timeout');

assert.ok(!this.masterPlaylistController
.masterPlaylistLoader_
.isLowestEnabledRendition_(), 'Not lowest rendition');
assert.ok(!isLowestEnabledRendition(
this.masterPlaylistController.masterPlaylistLoader_.master,
this.masterPlaylistController.masterPlaylistLoader_.media()),
'not on lowest rendition');

// Cause segment to timeout to force player into lowest rendition
this.requests[2].timedout = true;
Expand All @@ -1466,8 +1468,10 @@ function(assert) {
// Download new segment after media change
this.standardXHRResponse(this.requests[3]);

assert.ok(this.masterPlaylistController
.masterPlaylistLoader_.isLowestEnabledRendition_(), 'On lowest rendition');
assert.ok(isLowestEnabledRendition(
this.masterPlaylistController.masterPlaylistLoader_.master,
this.masterPlaylistController.masterPlaylistLoader_.media()),
'on lowest rendition');

assert.equal(this.masterPlaylistController.requestOptions_.timeout, 0,
'request timeout 0');
Expand Down
91 changes: 39 additions & 52 deletions test/playlist-loader.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@ import {
updateMaster,
setupMediaPlaylists,
resolveMediaGroupUris,
refreshDelay
refreshDelay,
isLowestEnabledRendition
} from '../src/playlist-loader';
import xhrFactory from '../src/xhr';
import { useFakeEnvironment } from './test-helpers';
Expand Down Expand Up @@ -791,6 +792,43 @@ QUnit.test('uses last segment duration for refresh delay', function(assert) {
'used half targetDuration when update is false');
});

QUnit.test('isLowestEnabledRendition detects if we are on lowest rendition',
Copy link
Contributor

Choose a reason for hiding this comment

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

I think putting these tests in playlist.test.js would be more appropriate and then you can test on just plain objects with the info thats needed instead of all this extra creating a loader, responding to request, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

master-playlist-controller.test.js also already has integration tests for isLowestEnabledRendition, so even more reason to convert these to unit tests

function(assert) {
let loader = new PlaylistLoader('master.m3u8', this.fakeHls);

loader.load();
this.requests.shift().respond(200, null,
'#EXTM3U\n' +
'#EXT-X-STREAM-INF:BANDWIDTH=1\n' +
'video1/media.m3u8\n' +
'#EXT-X-STREAM-INF:BANDWIDTH=1\n' +
'video2/media.m3u8\n');
loader.media = function() {
return {attributes: {BANDWIDTH: 10}};
};

loader.master.playlists = [{attributes: {BANDWIDTH: 10}},
{attributes: {BANDWIDTH: 20}}];
assert.ok(isLowestEnabledRendition(loader.master, loader.media()),
'Detected on lowest rendition');

loader.master.playlists = [{attributes: {BANDWIDTH: 10}},
{attributes: {BANDWIDTH: 10}},
{attributes: {BANDWIDTH: 10}},
{attributes: {BANDWIDTH: 20}}];
assert.ok(isLowestEnabledRendition(loader.master, loader.media()),
'Detected on lowest rendition');

loader.media = function() {
return {attributes: {BANDWIDTH: 20}};
};

loader.master.playlists = [{attributes: {BANDWIDTH: 10}},
{attributes: {BANDWIDTH: 20}}];
assert.ok(!isLowestEnabledRendition(loader.master, loader.media()),
'Detected not on lowest rendition');
});

QUnit.test('throws if the playlist url is empty or undefined', function(assert) {
assert.throws(function() {
PlaylistLoader();
Expand Down Expand Up @@ -895,57 +933,6 @@ QUnit.test('resolves relative media playlist URIs', function(assert) {
'resolved media URI');
});

QUnit.test('playlist loader returns the correct amount of enabled playlists',
function(assert) {
let loader = new PlaylistLoader('master.m3u8', this.fakeHls);

loader.load();

this.requests.shift().respond(200, null,
'#EXTM3U\n' +
'#EXT-X-STREAM-INF:BANDWIDTH=1\n' +
'video1/media.m3u8\n' +
'#EXT-X-STREAM-INF:BANDWIDTH=1\n' +
'video2/media.m3u8\n');
assert.equal(loader.enabledPlaylists_(), 2, 'Returned initial amount of playlists');
loader.master.playlists[0].excludeUntil = Date.now() + 100000;
this.clock.tick(1000);
assert.equal(loader.enabledPlaylists_(), 1, 'Returned one less playlist');
});

QUnit.test('playlist loader detects if we are on lowest rendition', function(assert) {
let loader = new PlaylistLoader('master.m3u8', this.fakeHls);

loader.load();
this.requests.shift().respond(200, null,
'#EXTM3U\n' +
'#EXT-X-STREAM-INF:BANDWIDTH=1\n' +
'video1/media.m3u8\n' +
'#EXT-X-STREAM-INF:BANDWIDTH=1\n' +
'video2/media.m3u8\n');
loader.media = function() {
return {attributes: {BANDWIDTH: 10}};
};

loader.master.playlists = [{attributes: {BANDWIDTH: 10}},
{attributes: {BANDWIDTH: 20}}];
assert.ok(loader.isLowestEnabledRendition_(), 'Detected on lowest rendition');

loader.master.playlists = [{attributes: {BANDWIDTH: 10}},
{attributes: {BANDWIDTH: 10}},
{attributes: {BANDWIDTH: 10}},
{attributes: {BANDWIDTH: 20}}];
assert.ok(loader.isLowestEnabledRendition_(), 'Detected on lowest rendition');

loader.media = function() {
return {attributes: {BANDWIDTH: 20}};
};

loader.master.playlists = [{attributes: {BANDWIDTH: 10}},
{attributes: {BANDWIDTH: 20}}];
assert.ok(!loader.isLowestEnabledRendition_(), 'Detected not on lowest rendition');
});

QUnit.test('resolves media initialization segment URIs', function(assert) {
let loader = new PlaylistLoader('video/fmp4.m3u8', this.fakeHls);

Expand Down
58 changes: 58 additions & 0 deletions test/playlist.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,64 @@ import QUnit from 'qunit';
import xhrFactory from '../src/xhr';
import { useFakeEnvironment } from './test-helpers';

QUnit.module('Playlist Status');
Copy link
Contributor

Choose a reason for hiding this comment

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

There is already an entire module of tests for blacklisting and enabled states near the bottom of this file...
https://github.com/videojs/videojs-contrib-hls/blob/master/test/playlist.test.js#L791


QUnit.test('isBlacklisted returns if a playlist is blacklisted', function(assert) {
assert.ok(!Playlist.isBlacklisted({}), 'not blacklisted if empty object');
assert.ok(!Playlist.isBlacklisted({
disabled: true,
enabled: false
}), 'not blacklisted if no excludeUntil');
assert.ok(!Playlist.isBlacklisted({
disabled: true,
enabled: false,
uri: 'test-uri',
duration: 4
}), 'not blacklisted if no excludeUntil');
assert.ok(!Playlist.isBlacklisted({
disabled: true,
enabled: false,
uri: 'test-uri',
duration: 4
}), 'not blacklisted if no excludeUntil');
assert.ok(!Playlist.isBlacklisted({
disabled: true,
enabled: false,
uri: 'test-uri',
duration: 4,
excludeUntil: Date.now()
}), 'not blacklisted if excludeUntil is not in future');
assert.ok(Playlist.isBlacklisted({
disabled: true,
enabled: false,
uri: 'test-uri',
duration: 4,
excludeUntil: Date.now() + 1
}), 'blacklisted if excludeUntil is in future');
});

QUnit.test('isEnabled returns if a playlist is enabled', function(assert) {
assert.ok(Playlist.isEnabled({}), 'is enabled if empty object');
assert.ok(Playlist.isEnabled({
disabled: false
}), 'is enabled if disabled is false');
assert.ok(!Playlist.isEnabled({
disabled: true
}), 'is not enabled if disabled is true');
assert.ok(Playlist.isEnabled({
disabled: false,
excludeUntil: Date.now()
}), 'is enabled if not currently blacklisted');
assert.ok(!Playlist.isEnabled({
disabled: false,
excludeUntil: Date.now() + 1
}), 'is not enabled if excludeUntil is in future');
assert.ok(!Playlist.isEnabled({
disabled: true,
excludeUntil: Date.now() + 1
}), 'is not enabled if excludeUntil is in future and disabled');
});

QUnit.module('Playlist Duration');

QUnit.test('total duration for live playlists is Infinity', function(assert) {
Expand Down