stream.dash: use suggestedPresentationDelay to calculate the live edge for dynamic manifests #5657
Conversation
There was a problem hiding this comment.
Thanks for the PR.
I don't know how I feel about adding a new separate session option and CLI argument for DASH streams. I'm already in the process of de-duplicating logic between segmented stream implementations (#5601) by using the same options/arguments for all of them.
If we're going to add a live-edge option that's similar to HLS to the DASH implementation, then IMO the right path would be deprecating the HLS option and adding a new common option/argument.
However, I also don't know how useful a live-edge option for DASH actually is that works the same way as the HLS one. What I mean is that it'd be much better to make this based on a time offset, not on a segment count offset. There's already the MPD.minBuffer and MPD.suggestedPresentationDelay attributes. And these attributes are already taken into account for the timeline-based segments, so an option which overrides the MPD's default values would make much more sense. I've already talked about that in one of the issues where the DASH delay was talked about. See:
- https://github.com/streamlink/streamlink/blob/6.3.1/src/streamlink/stream/dash/manifest.py#L352-L361
- https://github.com/streamlink/streamlink/blob/6.3.1/src/streamlink/stream/dash/manifest.py#L832C18-L838
Implementing a live-edge just for segment-lists doesn't seem right.
|
I see, thanks for the feedback. I'll try to make it work by calculating the optimal offset based on the segment duration and the suggestedPresentationDelay. |
michaelarnauts
changed the title
|
I've reworked this to use the suggestedPresentationDelay to calculate the live edge for dynamic manifests as suggested. |
…e for dynamic manifests
|
I'm only using the I'm not looking at minBuffer now, due to this, but maybe we should use the max value of both values? What do you think? |
There was a problem hiding this comment.
Thanks. Looking good...
The nested if-else-blocks could be flattened to elif-blocks for the sake of having better code, but I don't think this makes sense here, because I will need to add a warning message later anyway when the manifest has skipped segments after a reload (similar to #5607).
The optional segment duration needs to be fixed though before we can merge this...
| """Calculate the optimal segment number to start based on the suggestedPresentationDelay""" | ||
| suggested_delay = self.root.suggestedPresentationDelay | ||
|
|
||
| offset = max(0, math.ceil(suggested_delay.total_seconds() / self.duration_seconds)) |
There was a problem hiding this comment.
self.duration_seconds can be 0.0 if the optional duration attribute is missing (unknown segment duration). This probably will never be the case for dynamic manifests, but we still have to cover this case, or we raise a ZeroDivisionError.
In this case, the method should choose a constant number of segments that's referenced from a class attribute (see MPD.DEFAULT_MINBUFFERTIME for example). Not sure though what would be a good value, maybe 3 (default value of hls-live-edge). In the future, we can make this value, as well as the live-edge offset calculation customizable using a new session option.
My idea is deprecating and renaming the hls-live-edge option. Then we can change it so time values can be set too with a time-unit suffix, instead of just whole segment numbers.
There was a problem hiding this comment.
I've added this check, and also added a test for this scenario.
…ULT_LIVE_EDGE_SEGMENTS
|
Thanks, appreciated! |
Follow-up for #5654, also mentioned in #5582
Reworked this PR to calculate the live edge based on the
suggestedPresentationDelay.