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

feat: Easier manually playlist switching, add codecs to renditions #850

Merged
merged 6 commits into from
Jun 19, 2020

Conversation

brandonocasey
Copy link
Contributor

Right now we don't have a great way to manually switch playlists without calling blacklistCurrentPlaylist and hoping for the best or using the renditions api to exclude all renditions that we don't want. This change allows us to pass a playlist into smoothQualityChange_ and fastQualityChange_ so that we can directly change to the playlist we want. It also adds playlist codecs to the rendition API.

gesinger
gesinger previously approved these changes May 21, 2020
gkatsev
gkatsev previously approved these changes May 21, 2020
@gkatsev gkatsev added this to the 2.1 milestone May 21, 2020
@brandonocasey brandonocasey dismissed stale reviews from gkatsev and gesinger via c776658 June 17, 2020 19:03
@brandonocasey brandonocasey force-pushed the feat/switch-playlist branch from a67512c to c776658 Compare June 17, 2020 19:03
@brandonocasey
Copy link
Contributor Author

Added new tests and fixed tests that were failing after the rebase.

Copy link
Member

@gkatsev gkatsev left a comment

Choose a reason for hiding this comment

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

Now that smoothQualityChange_ and fastQualityChange_ can accept a new playlist to switch to, should we have a test that verifies that we try to switch to that one appropriately?

@@ -41,20 +49,23 @@ const makeMockPlaylist = function(options) {
};

const makeMockVhsHandler = function(playlistOptions, handlerOptions) {
const mcp = {
const mpc = {
Copy link
Member

Choose a reason for hiding this comment

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

oops, 😆

gkatsev
gkatsev previously approved these changes Jun 18, 2020
@@ -66,6 +67,10 @@ class Representation {

this.bandwidth = playlist.attributes.BANDWIDTH;

this.codecs = codecsForPlaylist(mpc.master(), playlist);

this.playlist = playlist;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to add the whole playlist object to the representation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It makes it easier to pass to smoothQualityChange/fastQualityChange

Comment on lines 368 to 370
assert.deepEqual(renditions[0].playlist, vhsHandler.playlists.master.playlists[0], 'rendition 1 has correct codec');
assert.deepEqual(renditions[1].playlist, vhsHandler.playlists.master.playlists[1], 'rendition 2 has correct codec');
assert.deepEqual(renditions[2].playlist, vhsHandler.playlists.master.playlists[2], 'rendition 3 has no codec');
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assert.deepEqual(renditions[0].playlist, vhsHandler.playlists.master.playlists[0], 'rendition 1 has correct codec');
assert.deepEqual(renditions[1].playlist, vhsHandler.playlists.master.playlists[1], 'rendition 2 has correct codec');
assert.deepEqual(renditions[2].playlist, vhsHandler.playlists.master.playlists[2], 'rendition 3 has no codec');
assert.deepEqual(renditions[0].playlist, vhsHandler.playlists.master.playlists[0], 'rendition 1 has playlist property');
assert.deepEqual(renditions[1].playlist, vhsHandler.playlists.master.playlists[1], 'rendition 2 has playlist property');
assert.deepEqual(renditions[2].playlist, vhsHandler.playlists.master.playlists[2], 'rendition 3 has playlist property');

@gkatsev gkatsev changed the base branch from master to main June 19, 2020 16:50
@gkatsev gkatsev merged commit f60fa1f into main Jun 19, 2020
@gkatsev gkatsev deleted the feat/switch-playlist branch June 19, 2020 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants