-
Notifications
You must be signed in to change notification settings - Fork 425
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
Conversation
@@ -61,7 +61,7 @@ export const parseMasterXml = ({ masterXml, srcUrl, clientOffset, sidxMapping }) | |||
* playlists merged in | |||
*/ | |||
export const updateMaster = (oldMaster, newMaster) => { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good!
There was a problem hiding this 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?
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; |
There was a problem hiding this comment.
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. 👍
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()
, updatenoChanges
to default totrue
, and set it tofalse
if any playlist is changed.