Skip to content

[RFC] examples/ola_recorder: mitigate against drifting playback offset #1680

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.

Sign up for GitHub

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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

[RFC] examples/ola_recorder: mitigate against drifting playback offset #1680

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b82b970
examples/ola_recorder: mitigate against drifting playback offset
shenghaoyang Oct 31, 2020
c8938be
examples/ola_recorder: fix overflow handling and warn on slow system.
shenghaoyang Nov 1, 2020
9bdf9df
examples/ola_recorder: provide localized warnings on slow system
shenghaoyang Nov 2, 2020
7ac4cc6
examples/ola_recorder: fix start point tracking
shenghaoyang Nov 2, 2020
b25674b
Merge branch 'master' into mitigate-drift
shenghaoyang Nov 3, 2020
2569a48
examples/ola_recorder: switch to using monotonic timestamps.
shenghaoyang Nov 3, 2020
28256a2
examples/ola_recorder: Correct for roundoff errors in writing show file
shenghaoyang Nov 4, 2020
dc605a9
Merge branch 'master' into mitigate-drift
peternewman Dec 22, 2020
1f783db
Update examples/ShowPlayer.cpp
shenghaoyang Jan 8, 2021
a8f5c23
examples/ola_recorder: add clamping for timeout value to avoid overflow
shenghaoyang Jan 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 23 additions & 2 deletions examples/ShowPlayer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,15 +216,18 @@ ShowLoader::State ShowPlayer::SeekTo(uint64_t seek_time) {
break;
}
}
uint64_t timeout = playhead_time - seek_time;
m_playback_pos = playhead_time;
m_clock.CurrentTime(&m_start_ts);
m_start_playback_pos = m_playback_pos - timeout;

// Send data in the state it would be in at the given time
map<unsigned int, ShowEntry>::iterator entry_it;
for (entry_it = entries.begin(); entry_it != entries.end(); ++entry_it) {
SendFrame(entry_it->second);
}
// Adjust the timeout to handle landing in the middle of the entry's timeout
RegisterNextTimeout(playhead_time-seek_time);
RegisterNextTimeout(timeout);

return ShowLoader::OK;
}
Expand Down Expand Up @@ -270,8 +273,26 @@ void ShowPlayer::SendEntry(const ShowEntry &entry) {
SendFrame(entry);
m_playback_pos += entry.next_wait;

unsigned int timeout = 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.CurrentTime(&now);
int64_t target_delta = m_playback_pos - m_start_playback_pos;
int64_t current_delta = (now - m_start_ts).InMilliSeconds();
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;
Copy link
Member

@peternewman peternewman Jan 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@shenghaoyang shenghaoyang Jan 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@peternewman peternewman Jan 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Contributor Author

@shenghaoyang shenghaoyang Feb 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😆

Copy link
Member

@peternewman peternewman Feb 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_delta we'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.

Copy link
Contributor Author

@shenghaoyang shenghaoyang Aug 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 💀

}
// Set when next to send data
RegisterNextTimeout(entry.next_wait);
RegisterNextTimeout(timeout);
}


Expand Down
4 changes: 4 additions & 0 deletions examples/ShowPlayer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
* Copyright (C) 2011 Simon Newton
*/

#include <ola/Clock.h>
#include <ola/DmxBuffer.h>
#include <ola/client/ClientWrapper.h>

Expand Down Expand Up @@ -89,6 +90,9 @@ class ShowPlayer {
uint64_t m_run_time;
std::map<unsigned int, uint64_t> m_frame_count;
bool m_simulate;
ola::Clock m_clock;
ola::TimeStamp m_start_ts;
uint64_t m_start_playback_pos;

/** Used for tracking simulation progress */
typedef enum {
Expand Down