Mir

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

Proposed by Daniel van Vugt on 2017-05-01
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
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.

To post a comment you must log in.
Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:4160
https://mir-jenkins.ubuntu.com/job/mir-ci/3383/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/4581
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/4709
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/4698
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/4698
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4613
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4613/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4613
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4613/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4613
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4613/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=zesty/4613
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=zesty/4613/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4613
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4613/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)
Alan Griffiths (alan-griffiths) wrote :

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

review: Approve
Mir CI Bot (mir-ci-bot) wrote :

FAILED: Autolanding.
More details in the following jenkins job:
https://mir-jenkins.ubuntu.com/job/mir-autolanding/1307/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/4625
    FAILURE: https://mir-jenkins.ubuntu.com/job/generic-land-mp/1392/console
    None: https://mir-jenkins.ubuntu.com/job/generic-land-mp/1393/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/4754
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/4743
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/4743
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4657
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4657/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4657
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4657/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4657
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4657/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=zesty/4657
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=zesty/4657/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4657
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4657/artifact/output/*zip*/output.zip

review: Needs Fixing (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/client/frame_clock.cpp'
2--- src/client/frame_clock.cpp 2016-11-29 08:00:45 +0000
3+++ src/client/frame_clock.cpp 2017-05-01 07:03:11 +0000
4@@ -31,6 +31,7 @@
5 FrameClock::FrameClock(FrameClock::GetCurrentTime gct)
6 : get_current_time{gct}
7 , config_changed{false}
8+ , phase{0}
9 , period{0}
10 , resync_callback{std::bind(&FrameClock::fallback_resync_callback, this)}
11 {
12@@ -61,7 +62,7 @@
13 return period != period.zero() ? now - (now % period) : now;
14 }
15
16-PosixTimestamp FrameClock::next_frame_after(PosixTimestamp prev) const
17+PosixTimestamp FrameClock::next_frame_after(PosixTimestamp when) const
18 {
19 Lock lock(mutex);
20 /*
21@@ -70,7 +71,12 @@
22 * it has no physical screen to sync to. Hence not throttled at all.
23 */
24 if (period == period.zero())
25- return prev;
26+ return when;
27+
28+ /* Phase correction in case 'when' isn't in phase: */
29+ auto const prev = when - ((when % period) - phase);
30+ /* Verify it's been corrected: */
31+ assert(prev % period == phase);
32
33 /*
34 * Regardless of render times and scheduling delays, we should always
35@@ -109,6 +115,8 @@
36 auto const server_frame = resync_callback();
37 lock.lock();
38
39+ phase = server_frame % period;
40+
41 /*
42 * Avoid mismatches (which will throw) and ensure we're always
43 * comparing timestamps of the same clock ID. This means our result
44
45=== modified file 'src/client/frame_clock.h'
46--- src/client/frame_clock.h 2016-12-15 08:15:54 +0000
47+++ src/client/frame_clock.h 2017-05-01 07:03:11 +0000
48@@ -52,10 +52,10 @@
49
50 /**
51 * Return the next timestamp to sleep_until, which comes after the last one
52- * that was slept till. On the first frame you can just provide an
53- * uninitialized timestamp.
54+ * that was slept till (or more generally after time 'when'). On the first
55+ * frame you can just provide an uninitialized timestamp.
56 */
57- time::PosixTimestamp next_frame_after(time::PosixTimestamp prev) const;
58+ time::PosixTimestamp next_frame_after(time::PosixTimestamp when) const;
59
60 private:
61 time::PosixTimestamp fallback_resync_callback() const;
62@@ -64,6 +64,7 @@
63
64 mutable std::mutex mutex; // Protects below fields:
65 mutable bool config_changed;
66+ mutable std::chrono::nanoseconds phase;
67 std::chrono::nanoseconds period;
68 ResyncCallback resync_callback;
69 };
70
71=== modified file 'tests/unit-tests/client/test_frame_clock.cpp'
72--- tests/unit-tests/client/test_frame_clock.cpp 2017-03-13 08:12:52 +0000
73+++ tests/unit-tests/client/test_frame_clock.cpp 2017-05-01 07:03:11 +0000
74@@ -116,6 +116,33 @@
75 EXPECT_EQ(one_frame, e - d);
76 }
77
78+TEST_F(FrameClockTest, interval_is_perfectly_smooth_despite_parameter_jitter)
79+{
80+ FrameClock clock(with_fake_time);
81+ clock.set_period(one_frame);
82+
83+ PosixTimestamp a;
84+ auto b = clock.next_frame_after(a);
85+
86+ fake_sleep_until(b);
87+ fake_sleep_for(one_frame/13); // short render time
88+
89+ auto c = clock.next_frame_after(b + 3ms);
90+ EXPECT_EQ(one_frame, c - b);
91+
92+ fake_sleep_until(c);
93+ fake_sleep_for(one_frame/7); // short render time
94+
95+ auto d = clock.next_frame_after(c + 1ms);
96+ EXPECT_EQ(one_frame, d - c);
97+
98+ fake_sleep_until(d);
99+ fake_sleep_for(one_frame/5); // short render time
100+
101+ auto e = clock.next_frame_after(d + 7ms);
102+ EXPECT_EQ(one_frame, e - d);
103+}
104+
105 TEST_F(FrameClockTest, long_render_time_is_recoverable_without_decimation)
106 {
107 FrameClock clock(with_fake_time);

Subscribers

People subscribed via source and target branches