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

Add option to select lowest rendition available on startup #1212

Merged
merged 19 commits into from
Aug 16, 2017
Merged
Show file tree
Hide file tree
Changes from 5 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
41 changes: 9 additions & 32 deletions src/master-playlist-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { translateLegacyCodecs } from 'videojs-contrib-media-sources/es5/codec-u
import worker from 'webworkify';
import Decrypter from './decrypter-worker';
import Config from './config';
import { parseCodecs } from './util/codecs.js';

let Hls;

Expand Down Expand Up @@ -65,36 +66,6 @@ const objectChanged = function(a, b) {
return false;
};

/**
* Parses a codec string to retrieve the number of codecs specified,
Copy link
Contributor Author

@OshinKaramian OshinKaramian Jul 26, 2017

Choose a reason for hiding this comment

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

This function doesn't seem to be reliant on being here (there are a few other functions here that could be moved as well). I shifted this into a util module as I wanted to use this to check whether a playlist contained video or not in the playlist selection algorithm below.

* the video codec and object type indicator, and the audio profile.
*
* @private
*/
const parseCodecs = function(codecs) {
let result = {
codecCount: 0
};
let parsed;

result.codecCount = codecs.split(',').length;
result.codecCount = result.codecCount || 2;

// parse the video codec
parsed = (/(^|\s|,)+(avc1)([^ ,]*)/i).exec(codecs);
if (parsed) {
result.videoCodec = parsed[2];
result.videoObjectTypeIndicator = parsed[3];
}

// parse the last field of the audio codec
result.audioProfile =
(/(^|\s|,)+mp4a.[0-9A-Fa-f]+\.([0-9A-Fa-f]+)/i).exec(codecs);
result.audioProfile = result.audioProfile && result.audioProfile[2];

return result;
};

/**
* Replace codecs in the codec string with the old apple-style `avc1.<dd>.<dd>` to the
* standard `avc1.<hhhhhh>`.
Expand Down Expand Up @@ -283,7 +254,8 @@ export class MasterPlaylistController extends videojs.EventTarget {
bandwidth,
externHls,
useCueTags,
blacklistDuration
blacklistDuration,
enableLowInitialPlaylist
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll want this added to the README eventually

} = options;

if (!url) {
Expand All @@ -298,6 +270,7 @@ export class MasterPlaylistController extends videojs.EventTarget {
this.mode_ = mode;
this.useCueTags_ = useCueTags;
this.blacklistDuration = blacklistDuration;
this.enableLowInitialPlaylist = enableLowInitialPlaylist;
if (this.useCueTags_) {
this.cueTagsTrack_ = this.tech_.addTextTrack('metadata',
'ad-cues');
Expand Down Expand Up @@ -430,7 +403,11 @@ export class MasterPlaylistController extends videojs.EventTarget {

if (!updatedPlaylist) {
// select the initial variant
this.initialMedia_ = this.selectPlaylist();
const initialPlaylistSelector =
this.enableLowInitialPlaylist ?
this.selectInitialPlaylist : this.selectPlaylist;

this.initialMedia_ = initialPlaylistSelector();
Copy link
Contributor

Choose a reason for hiding this comment

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

You can do this without needing to create the initialPlaylistSelector variable

this.masterPlaylistLoader_.media(this.initialMedia_);
return;
}
Expand Down
25 changes: 25 additions & 0 deletions src/playlist-selectors.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import Config from './config';
import Playlist from './playlist';
import { parseCodecs } from './util/codecs.js';

// Utilities

Expand Down Expand Up @@ -356,3 +357,27 @@ export const minRebufferMaxBandwidthSelector = function(settings) {

return rebufferingEstimates[0] || null;
};

/**
* Chooses the appropriate media playlist, which in this case is the lowest bitrate
* one with video. If no renditions with video exist, return the lowest audio rendition.
*
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add Expects to be called within the context of an instance of HlsHandler to this. see lastBandwidthSelector jsdoc

* @return {Object|null}
* {Object} return.playlist
* The lowest bitrate playlist that contains a video codec. If no such rendition
* exists pick the lowest audio rendition.
*/
export const lowestBitrateCompatibleVariantSelector = function() {
const { playlists } = this.playlists.master;
Copy link
Contributor

Choose a reason for hiding this comment

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

since sort messes with the array, you should use a copy of playslists with slice


// Sort ascending by bitrate
stableSort(playlists,
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on the findings from the 404 unit test failing (discussion in other comment thread), this selector should probably filter by isEnabled similar to the simpleSelector since this may try and re-select a playlist that just 404'd

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 catch, I believe the latest commit should deal with that scenario.

(a, b) => comparePlaylistBandwidth(a, b));

const playlistsWithVideo =
playlists.filter(playlist => parseCodecs(playlist.attributes.CODECS).videoCodec);

const selectedPlaylists = playlistsWithVideo.length !== 0 ? playlistsWithVideo : playlists;
Copy link
Contributor

Choose a reason for hiding this comment

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

you could omit this and just return playlistsWithVideo[0] || playlists[0];


return selectedPlaylists[0] || null;
};
34 changes: 34 additions & 0 deletions src/util/codecs.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@

/**
* @file - codecs.js - Handles tasks regarding codec strings such as translating them to
* codec strings, or translating codec strings into objects that can be examined.
*/

/**
* Parses a codec string to retrieve the number of codecs specified,
* the video codec and object type indicator, and the audio profile.
*/

export const parseCodecs = function(codecs = '') {
let result = {
codecCount: 0
};
let parsed;

result.codecCount = codecs.split(',').length;
result.codecCount = result.codecCount || 2;

// parse the video codec
parsed = (/(^|\s|,)+(avc1)([^ ,]*)/i).exec(codecs);
if (parsed) {
result.videoCodec = parsed[2];
result.videoObjectTypeIndicator = parsed[3];
}

// parse the last field of the audio codec
result.audioProfile =
(/(^|\s|,)+mp4a.[0-9A-Fa-f]+\.([0-9A-Fa-f]+)/i).exec(codecs);
result.audioProfile = result.audioProfile && result.audioProfile[2];

return result;
};
7 changes: 7 additions & 0 deletions src/videojs-contrib-hls.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import PlaybackWatcher from './playback-watcher';
import reloadSourceOnError from './reload-source-on-error';
import {
lastBandwidthSelector,
lowestBitrateCompatibleVariantSelector,
comparePlaylistBandwidth,
comparePlaylistResolution
} from './playlist-selectors.js';
Expand All @@ -39,6 +40,7 @@ const Hls = {
utils,

STANDARD_PLAYLIST_SELECTOR: lastBandwidthSelector,
INITIAL_PLAYLIST_SELECTOR: lowestBitrateCompatibleVariantSelector,
comparePlaylistBandwidth,
comparePlaylistResolution,

Expand Down Expand Up @@ -271,6 +273,7 @@ class HlsHandler extends Component {
setOptions_() {
// defaults
this.options_.withCredentials = this.options_.withCredentials || false;
this.options_.enableLowInitialPlaylist = this.options_.enableLowInitialPlaylist || false;

if (typeof this.options_.blacklistDuration !== 'number') {
this.options_.blacklistDuration = 5 * 60;
Expand Down Expand Up @@ -328,6 +331,10 @@ class HlsHandler extends Component {
this.selectPlaylist ?
this.selectPlaylist.bind(this) : Hls.STANDARD_PLAYLIST_SELECTOR.bind(this);

this.masterPlaylistController_.selectInitialPlaylist =
this.selectInitialPlaylist ?
this.selectInitialPlaylist.bind(this) : Hls.INITIAL_PLAYLIST_SELECTOR.bind(this);

// re-expose some internal objects for backwards compatibility with < v2
this.playlists = this.masterPlaylistController_.masterPlaylistLoader_;
this.mediaSource = this.masterPlaylistController_.mediaSource;
Expand Down
44 changes: 44 additions & 0 deletions test/master-playlist-controller.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,50 @@ QUnit.test('resets SegmentLoader when seeking in flash for both in and out of bu

});

QUnit.only('selects lowest bitrate rendition when enableLowInitialPlaylist is set',
function(assert) {
this.player = createPlayer({ html5: { hls: { enableLowInitialPlaylist: true } } });
Copy link
Contributor

Choose a reason for hiding this comment

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

since the initial selector is called within the loadedplaylist event handler which fires everytime a live playlist is refreshed, I think it would be worth simulating a live refresh and confirming that the initial selector is not called a second time


this.player.src({
src: 'manifest/master.m3u8',
type: 'application/vnd.apple.mpegurl'
});

this.clock.tick(1);

const masterPlaylistController = this.player.tech_.hls.masterPlaylistController_;
const masterPlaylistLoader = masterPlaylistController.masterPlaylistLoader_;

let numCallsToSelectInitialPlaylistCalls = 0;
let numCallsToSelectPlaylist = 0;

// master
this.standardXHRResponse(this.requests.shift());
Copy link
Contributor

Choose a reason for hiding this comment

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

These two responses should go after the sourceopen trigger, also I would expect that the numCallsToSelectInitialPlaylistCalls should increment after just the master response

// media
this.standardXHRResponse(this.requests.shift());

// This shouldn't get called, but if it does we should track it so that
// we can catch this failure case.
masterPlaylistController.selectPlaylist = () => {
numCallsToSelectPlaylist++;
return masterPlaylistController.master().playlists[0];
};

masterPlaylistController.selectInitialPlaylist = () => {
numCallsToSelectInitialPlaylistCalls++;
return masterPlaylistController.master().playlists[0];
};

masterPlaylistController.mediaSource.trigger('sourceopen');

// Trigger playlist event which should utilize selectInitialPlaylist and
Copy link
Contributor

Choose a reason for hiding this comment

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

this comment is a bit outdated now

// not selectPlaylist
masterPlaylistLoader.trigger('loadedplaylist');

assert.equal(numCallsToSelectInitialPlaylistCalls, 1, 'selectInitialPlaylist');
assert.equal(numCallsToSelectPlaylist, 0, 'selectPlaylist');
});

QUnit.test('resyncs SegmentLoader for a fast quality change', function(assert) {
let resyncs = 0;

Expand Down