feat: switch to target latency for live sync and add dynamic target latency feature #6193
Conversation
|
Completely forgot about tests, I'll check on them tomorrow. |
avelad
added
type: enhancement
|
I believe this is ready for review. |
|
@gkatsev Please rebase and resolve the conflicts, Thanks! |
gkatsev
force-pushed
the
dynamic-target-latency
branch
from
4b28f58
to
96d5b79
Compare
|
@avelad rebased and fixed conflicts! |
|
OK, I'm assuming that if @avelad approved but didn't merge, he was either waiting for test results or my review. I'll take a look now. |
| * <code>liveSyncMaxLatency</code> after a rebuffering event. Defaults to | ||
| * <code>0.5</code>. | ||
| * @property {number} liveSyncDynamicTargetLatencyStabilityThreshold | ||
| * Number of seconds after a rebuffering before we are considered stable and |
There was a problem hiding this comment.
It seems to me that we are setting up a pendulum. Am I missing something in this analysis?
When we aim for a min latency of 0s (default), if that is not achievable, we could rebuffer. Then the latency will be backed off in 0.5s increments (default) until we find stable playback. If we sustain stable playback at a particular latency target for 60s (default), we will reduce the target again (increment not documented... same as for increases?) until we hit an unsustainable level and rebuffer.
It seems like this pendulum swings between the best latency target we can achieve, and a rebuffer event, back and forth. This would seem to set the user up for continued rebuffering events.
Am I missing something? I know I've missed a lot of changes in the time I've been away.
There was a problem hiding this comment.
I think you have a good overview of the changes. I've also previousl written a description of this here #6131 (comment).
For increment to go towards min latency is half of the distance between target latency and min latency.
Additonally, it isn't just backing off in 0.5s increments, but rather in a multiple of 0.5s per each rebuffering event. So, if there's a bunch of rebuffering, we'll increment further back. https://github.com/shaka-project/shaka-player/pull/6193/files#diff-7abf4aa2639707ce10a19703663841fae6c5638809faf4dfb2f1894791f154ddR5694.
I think that the pendulum that you mention is definitely a possibility, though, I assume that folks that may be using this option will probably want to tweak their target and minimum settings away from the defaults. It probably doesn't make much difference with the default min, max, and target latency values.
Also, I'm not sure that a pendulum here is necessarily an issue. During playback we do want to react to occasions when there are rebuffering, so, back off on being close to the live edge and if playback is stable, we can move closer to the edge. Given the unpredicatiblity of the network, I think it's reasonable, particularly for an opt-in feature. Some internal tests, a feature like this decreased the live latency and reduced the rebuffering count.
There was a problem hiding this comment.
maybe if the rebuffer count is high we should stick to the max latency and stop trying to move it towards min?
There was a problem hiding this comment.
I'm not sure that a pendulum here is necessarily an issue. During playback we do want to react to occasions when there are rebuffering, so, back off on being close to the live edge and if playback is stable, we can move closer to the edge. Given the unpredicatiblity of the network, I think it's reasonable, particularly for an opt-in feature. Some internal tests, a feature like this decreased the live latency and reduced the rebuffering count.
I hear you. But I wonder what the behavior of this will be further from the median experience. We may not know that until it is deployed and instrumented across a large number of sessions. Is your "internal testing" based on real user populations in an A/B experiment? Or just "seems good" from your fast connection at work or home? I mean no offense in this. I'm always forced to rely on the latter myself, but I just think we should be clear about the limitations of it when we discuss our risk/reward assessments.
I assume that folks that may be using this option will probably want to tweak their target and minimum settings away from the defaults. It probably doesn't make much difference with the default min, max, and target latency values.
While I'm sure plenty of developers are power users who understand the nuances and can tweak all these things, I bet there are still many who don't have a deep enough knowledge or experience to do so. Or who would Deploy an updated player without noticing that this feature is in it. Are there better defaults we could pick? I feel like we should assume that even if it's opt-in right now, it could become opt-out some day, and we should strive for defaults that are a good starting point for most scenarios, with low risk if possible.
maybe if the rebuffer count is high we should stick to the max latency and stop trying to move it towards min?
Maybe so. Maybe something like this, though I'm not certain what. I feel strongly that something should exist to stop a rebuffering cycle if it occurs.
I feel the same about latency as resolution: better to be a little father from the bleeding edge if you can avoid buffering in most cases.
But reasonable people could certainly disagree with me on that. I also don't have any user session data to back up my philosophies on any of this. So I would defer to real data if anyone has some.
There was a problem hiding this comment.
I totally see your concerns as well. Definitely a tricky situation trying to cater to power users while not letting others accidentally ruin their user experience.
I believe the tests were done with a network emulator and a bunch of network traces, but getting confirmation. Either way, it's definitely not the same as a full A/B experiment, though, definitely better than someone just hitting play on their laptop a bunch of times.
Would liveSyncDynamicTargetLatencyRebufferThreshold, where once the rebuffering count cross this boundary, it will choose the max latency and stick there without trying to push closer to the edge again assuage your concerns?
Also, given that this change is essentially two in one: switch to target latency, and dynamic target latency. With shaka v5 looming, if we don't have agreement on the dynamic part, would just the target latency be able to land? Since dynamic target latency could later be added as a feature, if it doesn't happen before v5 is done. Happy to separate this PR into two if that makes it more likely for the breaking change to land in time for v5.
There was a problem hiding this comment.
Confirmed my interpretation was correct regarding the test runner.
There was a problem hiding this comment.
What would be the next action?
There was a problem hiding this comment.
Oops, somehow missed your message.
For next actions, there are several options. The one that would be the least amount of work is likely to add the liveSyncDynamicTargetLatencyRebufferThreshold or similar option. Alternatively, I can split this PR between the refactor to target latency and the new feature of target latency. This was more necessary when v5 was scheduled soon.
I'd like us to have consensus on the next course of action before I update the PR. Is liveSyncDynamicTargetLatencyRebufferThreshold reasonable? Perhaps with a new name. Or maybe there are other ideas for resolving @joeyparrish's concern about the pendulum. Currently, I don't have any other ideas, but I'll spend some more time thinking about it next week. I would appreciate any other suggestions.
There was a problem hiding this comment.
Another potential change is to move the dynamic target latency options into a sub config object.
There was a problem hiding this comment.
Yes, I think it's better to split it into two PRs, it would be easier to review. As for the new setup, I'll let Joey decide.
This switches the live sync to use target latency (which is the only required part of DASH's service descriptors) and a configurable tolerance value, which defaults to 0.5s due to a recommendation from the LL-DASH spec.
Additionally, it adds a dynamic target latency feature which shifts the current target latency towards the minimum latency value when playback is stable (based on a configurable threshold, which defaults to 60s) and also shifts the latency towards the maximum latency on rebuffering events (by the configurable increment value, which defaults to 0.5s).
Closes #6131.