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

fix: dash manifest not refreshed if only some playlists are updated #949

Merged
merged 1 commit into from
Sep 22, 2020
Merged

fix: dash manifest not refreshed if only some playlists are updated #949

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 2 additions & 3 deletions src/dash-playlist-loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ export const parseMasterXml = ({ masterXml, srcUrl, clientOffset, sidxMapping })
* playlists merged in
*/
export const updateMaster = (oldMaster, newMaster) => {
Copy link
Member

Choose a reason for hiding this comment

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

one thing that's missing that we need to account for potentially is that since we are looping through the newMaster's playlists, if an item was removed we wouldn't notice it as a change.
Or are we waiting to make that change until we have a better understanding of when and how periods can change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since MPD updates can only add/remove representations by adding/removing periods, I think we should wait to see how period changes will be handled, as it may not be necessary to compare the representations themselves in those cases.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good!

let noChanges;
let noChanges = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

nice was about to comment that we should have this default to true on one of the other pull requests as I was seeing a hole in the logic. 👍

let update = mergeOptions(oldMaster, {
// These are top level properties that can be updated
duration: newMaster.duration,
Expand All @@ -74,8 +74,7 @@ export const updateMaster = (oldMaster, newMaster) => {

if (playlistUpdate) {
update = playlistUpdate;
} else {
noChanges = true;
noChanges = false;
}
}

Expand Down
44 changes: 27 additions & 17 deletions test/dash-playlist-loader.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,15 @@ QUnit.test('updateMaster: returns falsy when there are no changes', function(ass

QUnit.test('updateMaster: updates playlists', function(assert) {
const master = {
playlists: {
length: 1,
0: { uri: '0', id: '0' }
playlists: [{
uri: '0',
id: '0'
},
{
uri: '1',
id: '1',
segments: []
}],
mediaGroups: {
AUDIO: {},
SUBTITLES: {}
Expand All @@ -92,15 +97,18 @@ QUnit.test('updateMaster: updates playlists', function(assert) {
minimumUpdatePeriod: 0
};

// Only the first playlist is changed
const update = {
playlists: {
length: 1,
0: {
id: '0',
uri: '0',
segments: []
}
playlists: [{
id: '0',
uri: '0',
segments: []
},
{
uri: '1',
id: '1',
segments: []
}],
mediaGroups: {
AUDIO: {},
SUBTITLES: {}
Expand All @@ -112,14 +120,16 @@ QUnit.test('updateMaster: updates playlists', function(assert) {
assert.deepEqual(
updateMaster(master, update),
{
playlists: {
length: 1,
0: {
id: '0',
uri: '0',
segments: []
}
playlists: [{
id: '0',
uri: '0',
segments: []
},
{
uri: '1',
id: '1',
segments: []
}],
mediaGroups: {
AUDIO: {},
SUBTITLES: {}
Expand Down