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: dash manifest not refreshed if only some playlists are updated #949

Merged
merged 1 commit into from
Sep 22, 2020

Conversation

alex-barstow
Copy link
Contributor

@alex-barstow alex-barstow commented Sep 10, 2020

Description

This fixes an issue where an MPD update can be missed if some media playlists are updated while the last playlist remains unchanged (see here)

Specific Changes proposed

In updateMaster(), update noChanges to default to true, and set it to false if any playlist is changed.

@@ -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!

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.

Do we want to merge this in now and then have a separate PR for the period stuff if necessary?

@gkatsev gkatsev added this to the 2.2 milestone Sep 10, 2020
@gkatsev
Copy link
Member

gkatsev commented Sep 10, 2020

Looks like with #942, either that or this PR may need to be updated slightly.

@@ -61,7 +61,7 @@ export const parseMasterXml = ({ masterXml, srcUrl, clientOffset, sidxMapping })
* playlists merged in
*/
export const updateMaster = (oldMaster, newMaster) => {
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. 👍

@gkatsev gkatsev merged commit 31d3441 into main Sep 22, 2020
@gkatsev gkatsev deleted the fix-playlist-update branch September 22, 2020 21:38
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.

4 participants