[RFC] examples/ola_recorder: mitigate against drifting playback offset #1680
Conversation
Previous commit assumed that unsigned int is less wide than uint64_t when handling unsigned overflow. But since InMilliSeconds() returns a signed 64-bit value, discard all the unsigned math and just do signed delta calculations (even though we are losing one bit. Shouldn't really matter for the majority of cases, unless someone's going to play for hundereds of years). Also show a log message when the delay is negative, implying that the system might be too slow to keep up with the current file. (We might want to tune that to be < some negative number instead of just < 0 to avoid spamming too much?). fixes: b82b970
We need to track the start point not at the playback time of the next frame but the playback time of the current frame / partial frame. fixes: b82b970
examples/ShowPlayer.cpp
Outdated
| int64_t delay = target_delta - current_delta; | ||
| if (delay < 0) { | ||
| OLA_WARN << "Frame at line " << m_loader.GetCurrentLineNumber() | ||
| << " was meant to have completed " << -delay << " ms ago." | ||
| << " System too slow?"; | ||
| delay = 0; | ||
| } | ||
| timeout = delay; |
There was a problem hiding this comment.
I wonder if this could be simplified a bit by just writing the (more accurate delay) directly into the timeout variable, i.e. when we're in real time mode we can calculate the actual value, not an idealised simulate one.
There was a problem hiding this comment.
The delay value is already the actual timeout value (which may be negative if the system is too slow to keep up with the frames in the recording, or positive if the player needs to delay for the next frame).
The idealized simulated one comes from unsigned int timeout = entry.next_wait;, which will be overwritten in the !m_simulate branch with the delay value.
Unless you mean to have the idealized calculation inside the branch instead?
There was a problem hiding this comment.
I was meaning something like this:
int64_t delay = entry.next_wait;
if (!m_simulate) {
// Using int64_t for target_delta here because we have to lose 1 bit anyway
// as InMilliSeconds() returns a signed 64-bit integer.
ola::TimeStamp now;
m_clock.CurrentMonotonicTime(&now);
int64_t target_delta = m_playback_pos - m_start_playback_pos;
int64_t current_delta = (now - m_start_ts).InMilliSeconds();
delay = target_delta - current_delta;
if (delay < 0) {
OLA_WARN << "Frame at line " << m_loader.GetCurrentLineNumber()
<< " was meant to have completed " << -delay << " ms ago."
<< " System too slow?";
delay = 0;
}
#if UINT_MAX < INT64_MAX
if (delay > UINT_MAX) {
OLA_WARN << "Calculated delay of " << delay << " ms"
<< " exceeded maximum delay of " << UINT_MAX << " ms."
<< " Clamping to maximum.";
delay = UINT_MAX;
}
#endif
}
// Set when next to send data
RegisterNextTimeout(delay);
Unless you were going to say add some debug level stats about how delay and ideal timeout compare each time, rather than just when it won't fit, then tracking them both independently seemed a bit irrelevant.
Given they're essentially equivalent. Or you could possibly even do something like this (i.e. the non-simulate run corrects the idealised view):
int64_t timeout = m_playback_pos - m_start_playback_pos;
if (!m_simulate) {
// Get now
int64_t current_delta = (now - m_start_ts).InMilliSeconds();
timeout = timeout - current_delta;
if (delay < 0) {
OLA_WARN << "Frame at line " << m_loader.GetCurrentLineNumber()
<< " was meant to have completed " << -delay << " ms ago."
<< " System too slow?";
delay = 0;
}
// Clamping
}
// Set when next to send data
RegisterNextTimeout(delay);
There was a problem hiding this comment.
Hm, m_start_ts actually starts tracking from playback start, and it doesn't track from the end of the previous frame.
That's why I needed to evaluate both target and current, since I need to know how far off we are from the idealized time to change the timeout.
If we subtract timeout by current_delta we'd most likely get a negative value 😆
There was a problem hiding this comment.
Yes, but m_playback_pos is where we should be when we've finished this delay (in relative time) and m_start_playback_pos is where in the showfile we started (relative).
So m_playback_pos - m_start_playback_pos should be the difference using maths from relative points and now - m_start_ts should be the actual difference using clock time.
If we subtract timeout by
current_deltawe'd most likely get a negative value
I don't follow, the maths is the same as your PR currently does, it's just I've reduced the number of variables and slightly tweaked how its laid out.
Given they're essentially equivalent. Or you could possibly even do something like this (i.e. the non-simulate run corrects the idealised view):
Looks like I made a typo before, it still needs a little bit of work but I think something along those lines should be possible.
There was a problem hiding this comment.
If you still remember how this works I think you could make that change? I didn't quite get what you meant last time, and unwinding everything now is even more... confusing 💀
Co-authored-by: Peter Newman <peternewman@users.noreply.github.com>
In cases where UINT_MAX < INT64_MAX, and entry.next_wait is close to UINT_MAX, delay may exceed UINT_MAX and lead to an overflow of the timeout value.
| @@ -31,6 +31,7 @@ | |||
| #include <ola/base/SysExits.h> | |||
| #include <ola/client/ClientWrapper.h> | |||
| #include <ola/client/OlaClient.h> | |||
| #include <climits> | |||
There was a problem hiding this comment.
I think #include <stdint.h> should provide INT64_MAX too and we've already used that successfully on lots of platforms.
There was a problem hiding this comment.
I actually pulled in <climits> because we needed UINT_MAX, and that's not something that <stdint.h> is specified to provide.
There was a problem hiding this comment.
Though in hindsight it's only needed for clamping, so I should have guarded that with an #if... #endif too....
There was a problem hiding this comment.
Ah, we use #include <limits.h> for that (I think) in
ola/plugins/pathport/PathportPlugin.cpp
Line 113 in ea3a18a
|
Hi @shenghaoyang , did you get a chance to look at the last few comments? It would be great to get this merged in thanks! |
|
Hi @peternewman, Sorry - a litte bit busy with the semester. Haven't forgotten about this PR though, hopefully I'd get more time to work on this in the next few weeks as we get closer to term break. |
Fair enough, no worries and certainly no rush! |
|
Gentle nudge @shenghaoyang . Did you get an Easter or similar holiday or were you tied up with other stuff? |
|
Hey @peternewman yeah, I'm still alive - just that it's finals soon :( |
|
I'll hop back on this as soon as finals are over! |
Glad to hear it! Oh yeah, you should definitely concentrate on finals not this! Good luck with them! |
|
Can I do anything to help with this @shenghaoyang ? It would be great to get it across the line! I'm also waiting on #1607 but I figure it's good to get this in first before I make your life even harder with some merge conflicts. 😄 |
|
Ah, I kinda forgot about this - sorry :/. Will hack on it during the weekend - there's not much left besides the switch from |
One idea to fix #1677.
See #1677 for more details.