Merge into development-branch : generalise-next_frame

Status:	Merged
Merged at revision:	4170
Proposed branch:	lp:~vanvugt/mir/generalise-next_frame_after
Merge into:	lp:mir
Diff against target:	107 lines (+41/-5) 3 files modified src/client/frame_clock.cpp (+10/-2) src/client/frame_clock.h (+4/-3) tests/unit-tests/client/test_frame_clock.cpp (+27/-0)
To merge this branch:	bzr merge lp:~vanvugt/mir/generalise-next_frame_after
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Mir CI Bot	continuous-integration		Needs Fixing on 2017-05-17
Alan Griffiths		2017-05-01	Approve on 2017-05-16
Review via email: mp+323450@code.launchpad.net

Commit message

Generalise FrameClock::next_frame_after() to accept arbitrary
timestamps rather than assuming the input parameter is always
an in-phase frame timestamp.

Description of the change

Although I no longer have a hard need for this change, it's still worth landing so as to avoid future mistakes where someone might think next_frame_after(time) works correctly for an arbitrary time. This branch makes it so.

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2017-05-01:

#

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/3383/rebuild

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2017-05-16:

#

I don't find the naming easy to follow, but that's pre-existing

review: Approve

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2017-05-17:

#

review: Needs Fixing (continuous-integration)

 === modified file 'src/client/frame_clock.cpp'
 --- src/client/frame_clock.cpp	2016-11-29 08:00:45 +0000
 +++ src/client/frame_clock.cpp	2017-05-01 07:03:11 +0000
@@ -31,6 +31,7 @@
  FrameClock::FrameClock(FrameClock::GetCurrentTime gct)
      : get_current_time{gct}
      , config_changed{false}
++    , phase{0}
      , period{0}
      , resync_callback{std::bind(&FrameClock::fallback_resync_callback, this)}
+ {
@@ -61,7 +62,7 @@
      return period != period.zero() ? now - (now % period) : now;
+ }
--PosixTimestamp FrameClock::next_frame_after(PosixTimestamp prev) const
++PosixTimestamp FrameClock::next_frame_after(PosixTimestamp when) const
+ {
      Lock lock(mutex);
      /*
@@ -70,7 +71,12 @@
       * it has no physical screen to sync to. Hence not throttled at all.
       */
      if (period == period.zero())
--        return prev;
++        return when;
++
++    /* Phase correction in case 'when' isn't in phase: */
++    auto const prev = when - ((when % period) - phase);
++    /* Verify it's been corrected: */
++    assert(prev % period == phase);
      /*
       * Regardless of render times and scheduling delays, we should always
@@ -109,6 +115,8 @@
          auto const server_frame = resync_callback();
          lock.lock();
++        phase = server_frame % period;
++
          /*
           * Avoid mismatches (which will throw) and ensure we're always
           * comparing timestamps of the same clock ID. This means our result
 === modified file 'src/client/frame_clock.h'
 --- src/client/frame_clock.h	2016-12-15 08:15:54 +0000
 +++ src/client/frame_clock.h	2017-05-01 07:03:11 +0000
@@ -52,10 +52,10 @@
      /**
       * Return the next timestamp to sleep_until, which comes after the last one
--     * that was slept till. On the first frame you can just provide an
--     * uninitialized timestamp.
++     * that was slept till (or more generally after time 'when'). On the first
++     * frame you can just provide an uninitialized timestamp.
       */
--    time::PosixTimestamp next_frame_after(time::PosixTimestamp prev) const;
++    time::PosixTimestamp next_frame_after(time::PosixTimestamp when) const;
  private:
      time::PosixTimestamp fallback_resync_callback() const;
@@ -64,6 +64,7 @@
      mutable std::mutex mutex;  // Protects below fields:
      mutable bool config_changed;
++    mutable std::chrono::nanoseconds phase;
      std::chrono::nanoseconds period;
      ResyncCallback resync_callback;
  };
 === modified file 'tests/unit-tests/client/test_frame_clock.cpp'
 --- tests/unit-tests/client/test_frame_clock.cpp	2017-03-13 08:12:52 +0000
 +++ tests/unit-tests/client/test_frame_clock.cpp	2017-05-01 07:03:11 +0000
@@ -116,6 +116,33 @@
      EXPECT_EQ(one_frame, e - d);
+ }
++TEST_F(FrameClockTest, interval_is_perfectly_smooth_despite_parameter_jitter)
++{
++    FrameClock clock(with_fake_time);
++    clock.set_period(one_frame);
++
++    PosixTimestamp a;
++    auto b = clock.next_frame_after(a);
++
++    fake_sleep_until(b);
++    fake_sleep_for(one_frame/13);  // short render time
++
++    auto c = clock.next_frame_after(b + 3ms);
++    EXPECT_EQ(one_frame, c - b);
++
++    fake_sleep_until(c);
++    fake_sleep_for(one_frame/7);  // short render time
++
++    auto d = clock.next_frame_after(c + 1ms);
++    EXPECT_EQ(one_frame, d - c);
++
++    fake_sleep_until(d);
++    fake_sleep_for(one_frame/5);  // short render time
++
++    auto e = clock.next_frame_after(d + 7ms);
++    EXPECT_EQ(one_frame, e - d);
++}
++
  TEST_F(FrameClockTest, long_render_time_is_recoverable_without_decimation)
+ {
      FrameClock clock(with_fake_time);

Mir

Merge lp:~vanvugt/mir/generalise-next_frame_after into lp:mir

Commit message

Description of the change

Preview Diff

Subscribers