Mir

include/client/mir_toolkit/mir_buffer_stream.h (+0/-1)
include/client/mir_toolkit/mir_surface.h (+0/-1)
include/core/mir_toolkit/common.h (+7/-4)
src/client/CMakeLists.txt (+1/-0)
src/client/buffer_stream.cpp (+111/-6)
src/client/buffer_stream.h (+12/-1)
src/client/error_stream.cpp (+6/-0)
src/client/error_stream.h (+2/-0)
src/client/frame_clock.cpp (+156/-0)
src/client/frame_clock.h (+73/-0)
src/client/mir_buffer_stream_api.cpp (+8/-1)
src/client/mir_surface.cpp (+113/-5)
src/client/mir_surface.h (+10/-0)
src/client/mir_surface_api.cpp (+1/-1)
src/client/screencast_stream.cpp (+8/-0)
src/client/screencast_stream.h (+2/-0)
src/include/client/mir/mir_buffer_stream.h (+2/-0)
tests/acceptance-tests/test_client_library.cpp (+5/-0)
tests/acceptance-tests/test_latency.cpp (+27/-2)
tests/include/mir/test/doubles/mock_mir_buffer_stream.h (+2/-0)
tests/unit-tests/client/CMakeLists.txt (+1/-0)
tests/unit-tests/client/test_client_mir_surface.cpp (+81/-0)
tests/unit-tests/client/test_frame_clock.cpp (+420/-0)
tests/unit-tests/platforms/android/server/test_hwc_device.cpp (+1/-0)

To merge this branch:

bzr merge lp:~vanvugt/mir/client-side-vsync

Related bugs:

Bug #1240909: [performance] Restore support for better-than-triple buffering by default.	Medium	Fix Released
Bug #1388490: Frame rate is artificially low on Diamondville Intel Atom systems due to aggressive power management	Medium	Fix Released
Bug #1591328: Pointer/cursor input lag in unity8 session	High	Fix Released
Bug #1624188: [ FAILED ] ClientLatency.throttled_input_rate_yields_lower_latency	Medium	Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Alan Griffiths			Approve on 2017-01-17
Kevin DuBois (community)			Approve on 2017-01-17
Mir CI Bot	continuous-integration		Approve on 2017-01-17
Cemil Azizoglu (community)		2017-01-09	Approve on 2017-01-16
Review via email: mp+314313@code.launchpad.net

Commit message

Introducing client-side vsync, part 1.
For dramatically lower latency and improved smoothness.
Fixes LP: #1240909, LP: #1388490, LP: #1591328, LP: #1624188

Part 1 covers all users of:
eglSwapBuffers
mir_buffer_stream_swap_buffers_sync

Part 2 will cover the rest (mir_buffer_stream_swap_buffers) but we're not
in a hurry for it as that still has working server-side vsync. Plus I
think Xmir is the only user of that function at present.

Part 3 will introduce IPC to communicate hardware vsync timestamps to the
client for phase correction and increased precision (although that's not
a prerequisite to this).

Immediate benefits:

* Greatly reduced latency from the client to the screen. Theoretically
   three frames lower latency for windowed apps of a nested server like
   Unity8. And this has been confirmed with real measurements:
   https://docs.google.com/spreadsheets/d/1RbTVDbx04ohkF4-md3wAlgmxbSI1DttstnT6xdcXhZQ/pubchart?oid=1566479835&format=interactive
   You can also confirm using your own eyes: mir_demo_client_target

* No client/toolkit changes required.

* Latency and smoothness is no longer affected by process scheduling
   randomness or poor server performance. Although we presently use input
   resampling to work around some of the latency problem, that's not
   immune to latency increasing again when the server has insufficient
   time to keep up with the client. This approach is.

* Lower latency even than double buffering (when nested), and also
without the risk of decimation when a client takes slightly too long
to render.

Description of the change

Future benefits:

* We can remove Mir's input resampling now, since client-side vsync
should make the QML touch compression algorithm safely nestable.

* We get closer to not needing server-side queuing and multi-monitor
   frame sync logic. Although some people would like to keep it for other
   reasons, this approach (after mir_buffer_stream_swap_buffers gets
   ported) eliminates most of the need for it.

* Overall this approach could deprecate many more lines of code than it
adds (see above).

* Toolkits can have greater flexibility in how they choose to implement
   their event loops without threading. If we shared timing information
   in the client API in future then a toolkit could just choose interval 0
   and do its own vsync baked into its event loop.

* Mir is free to support dropping in the system compositor if it wants,
and no longer requires additional fixing to throttle drivers (like
software renderers) that don't support page flipping.

Risks:

* We rely on the graphics driver to report accurate frame timing.
   Although it's only a serious problem if the driver is out by orders of
   magnitude (LP: #1639725). Otherwise only being slightly inaccurate is
   actually the same problem as what we experience presently with the
   59Hz input resampling when user input is driving animation.

Revision history for this message

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

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

review: Approve (continuous-integration)

Revision history for this message

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

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2017-01-12:

^^^
Bug 1523621, bug 1646558, bug 1646375

Revision history for this message

Cemil Azizoglu (cemil-azizoglu) wrote on 2017-01-12:

Nice latency numbers.

Needs fixing:

MirSurface references should be MirWindow now.

review: Needs Fixing

Revision history for this message

Kevin DuBois (kdub) wrote on 2017-01-13:

LGTM overall (still haven't fully understood the logic in FrameClock, so more comments on that later, perhaps)

discussion:
135 + * TODO: Remove these functions in future, after
136 + * mir_buffer_stream_swap_buffers has been converted to use
137 + * client-side vsync, and the server has been told to always use
138 + * dropping for BufferStream. Then there will be no need to ever
139 + * change the server swap interval from zero.

Would prefer if this read "and the server has other ways to express buffer queuing other than swapinterval". Would leave room for some of the vulkan submission modes to work via server-side queuing (jury's still out on whether this is a good idea, but I don't know if we want to limit ourselves to client-side queuing only)

needs info:
1)
+void mcl::BufferStream::adopted_by(MirSurface* surface)

Other than BufferStream's currently-terrible constructor, why couldn't the frame clock be given as a build dependency via the constructor? One more parameter to the constructor seems better than a too-long constructor list + a 2step initialization

2)
Where the code currently is might be only useful to android, post-MirRenderSurface/MirSurface/mir-egl-platform. (which still uses MirBufferStream as part of its egl stack for now). From what I've heard from Cemil, mesa is using MirPresentationChains + MirBuffers, so might need some way via an extension or a public-api to get the timing info. (android might also might be better suited for that as well, for now its just easier to re-use MirBufferStream for that platform)

nit:
348 +typedef std::unique_lock<std::mutex> Lock;
IMHO, doesn't improve readability.

I guess that's mostly a 'discussion' vote.

review: Needs Information

Revision history for this message

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

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2017-01-15:

^^^
Bug 1523621

Revision history for this message

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

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2017-01-16:

^^^
Bug 1647244, and more random examples failures on krillin:

13:14:00 I: The following clients failed to execute successfully:
13:14:00 I: mir_demo_client_multistream
13:14:00 I: Smoke testing complete with returncode -1

Revision history for this message

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

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2017-01-16:

^^^
Bug 1646558

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2017-01-16:

kdub:

(0) Sure you can keep server-side queueing if you want it. So long as this code still has the ability to turn it off. Because having it queueing on will reintroduce the bufferbloat-nested-queuing-lag problems that are fixed here. So you probably don't want that, but up to you :)

(1) No FrameClock is not set at construction. It's set (and changes) whenever the window moves to a new monitor (different FrameClock). We may also wish to change the FrameClock depending on occlusions in future (to replace the 10Hz dropping hack)...

(2) Yes, I expect the logic to move around with Cemil's future changes but where it is right now works for what we use right now. As for a public API, yes I agree that will be wanted; already mentioned it in the description above, but also a public API will be needed by Xmir to implement https://www.opengl.org/registry/specs/OML/glx_sync_control.txt

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2017-01-16:

Also a public API would let clients do the equivalent of client-side predictive bypass for even lower latency: https://www.opengl.org/registry/specs/NV/glx_delay_before_swap.txt

Revision history for this message

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

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2017-01-16:

^^^
Bug 1655929

Revision history for this message

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

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Cemil Azizoglu (cemil-azizoglu) wrote on 2017-01-16:

Ok by me when CI is happy

review: Approve

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2017-01-17:

^^^
CI aborts are bug 1655929

Revision history for this message

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

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2017-01-17:

^^^
Random krillin failures running mir-demos:

03:40:30 I: The following clients failed to execute successfully:
03:40:30 I: mir_demo_client_animated_cursor
03:40:30 I: mir_demo_client_multistream
03:40:30 I: Smoke testing complete with returncode -1

Revision history for this message

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

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2017-01-17:

^^^
Bug 1616312

Revision history for this message

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

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

review: Approve (continuous-integration)

Revision history for this message

Kevin DuBois (kdub) wrote on 2017-01-17:

> kdub:
>
> (0) Sure you can keep server-side queueing if you want it. So long as this
> code still has the ability to turn it off. Because having it queueing on will
> reintroduce the bufferbloat-nested-queuing-lag problems that are fixed here.
> So you probably don't want that, but up to you :)

Sure, we need an option to turn it off, it might just be useful to have it around.

> (2) Yes, I expect the logic to move around with Cemil's future changes but
> where it is right now works for what we use right now. As for a public API,
> yes I agree that will be wanted; already mentioned it in the description
> above, but also a public API will be needed by Xmir to implement
> https://www.opengl.org/registry/specs/OML/glx_sync_control.txt

Alright, so we might need some extensions for this to work in the "MirRenderSurface" world, and the driver would manage what BufferStream is managing now.

Revision history for this message

Kevin DuBois (kdub) wrote on 2017-01-17:

well, lgtm. The frame clock management still seems like 2-step initialization, but given the state of BufferStream, doesn't bother me as much.

review: Approve

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2017-01-17:

Is the year really right?

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2017-01-17:

> well, lgtm. The frame clock management still seems like 2-step initialization,
> but given the state of BufferStream, doesn't bother me as much.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Daniel van Vugt

Emanuele Antonio Faraone

Gerry Boland

Mir development team

 === modified file 'include/client/mir_toolkit/mir_buffer_stream.h'
 --- include/client/mir_toolkit/mir_buffer_stream.h	2016-12-17 00:52:45 +0000
 +++ include/client/mir_toolkit/mir_buffer_stream.h	2017-01-17 06:23:26 +0000
@@ -219,7 +219,6 @@
  /**
   * Set the swapinterval for the stream.
   *   \warning EGL users should use eglSwapInterval directly.
-- *   \warning Only swapinterval of 0 or 1 is supported.
   *   \param [in] stream   The buffer stream
   *   \param [in] interval The number of vblank signals that
   *                        mir_buffer_stream_swap_buffers will wait for
 === modified file 'include/client/mir_toolkit/mir_surface.h'
 --- include/client/mir_toolkit/mir_surface.h	2017-01-17 05:51:54 +0000
 +++ include/client/mir_toolkit/mir_surface.h	2017-01-17 06:23:26 +0000
@@ -978,7 +978,6 @@
  /**
   * Set the swapinterval for the default stream.
   *   \warning EGL users should use eglSwapInterval directly.
-- *   \warning Only swapinterval of 0 or 1 is supported.
   *   \warning If the surface was created with, or modified to have a
   *            MirSurfaceSpec containing streams added through
   *            mir_window_spec_set_streams(), the default stream will
 === modified file 'include/core/mir_toolkit/common.h'
 --- include/core/mir_toolkit/common.h	2017-01-17 05:51:54 +0000
 +++ include/core/mir_toolkit/common.h	2017-01-17 06:23:26 +0000
@@ -37,9 +37,9 @@
      mir_surface_attrib_type,
      mir_surface_attrib_state,
      mir_surface_attrib_swapinterval, /**< \deprecated  Do not listen for events
--        reporting this attribute. Use the
--        "mir_*_get_swapinterval()" functions
--        instead if you wish query its value */
++                                          reporting this attribute. Use the
++                                          "mir_*_get_swapinterval()" functions
++                                          instead if you wish query its value */
      mir_surface_attrib_focus,
      mir_surface_attrib_dpi,
      mir_surface_attrib_visibility,
@@ -57,7 +57,10 @@
      /* Do not specify values...code relies on 0...N ordering. */
      mir_window_attrib_type,
      mir_window_attrib_state,
--    mir_window_attrib_swapinterval,
++    mir_window_attrib_swapinterval, /**< \deprecated  Do not listen for events
++                                          reporting this attribute. Use the
++                                          "mir_*_get_swapinterval()" functions
++                                          instead if you wish query its value */
      mir_window_attrib_focus,
      mir_window_attrib_dpi,
      mir_window_attrib_visibility,
 === modified file 'src/client/CMakeLists.txt'
 --- src/client/CMakeLists.txt	2016-12-13 06:00:32 +0000
 +++ src/client/CMakeLists.txt	2017-01-17 06:23:26 +0000
@@ -62,6 +62,7 @@
    default_connection_configuration.cpp
    connection_surface_map.cpp
    private.cpp
++  frame_clock.cpp
    mir_screencast.cpp
    mir_screencast_api.cpp
    mir_cursor_api.cpp
 === modified file 'src/client/buffer_stream.cpp'
 --- src/client/buffer_stream.cpp	2017-01-17 05:51:54 +0000
 +++ src/client/buffer_stream.cpp	2017-01-17 06:23:26 +0000
@@ -253,6 +253,7 @@
        client_platform(client_platform),
        protobuf_bs{mcl::make_protobuf_object<mir::protobuf::BufferStream>(a_protobuf_bs)},
        user_swap_interval(parse_env_for_swap_interval()),
++      using_client_side_vsync(false),
        interval_config{server, frontend::BufferStreamId{a_protobuf_bs.id().value()}},
        scale_(1.0f),
        perf_report(perf_report),
@@ -264,6 +265,16 @@
        factory(factory),
        render_surface_(render_surface)
+ {
++    /*
++     * Since we try to use client-side vsync where possible, a separate
++     * copy of the current swap interval is required to represent that
++     * the stream might have a non-zero interval while we want the server
++     * to use a zero interval. This is not stored inside interval_config
++     * because with some luck interval_config will eventually go away
++     * leaving just int current_swap_interval.
++     */
++    current_swap_interval = interval_config.swap_interval();
++
      if (!protobuf_bs->has_id())
+     {
          if (!protobuf_bs->has_error())
@@ -354,6 +365,10 @@
      std::unique_lock<decltype(mutex)> lock(mutex);
      perf_report->end_frame(id);
++    if (!using_client_side_vsync &&
++        current_swap_interval != interval_config.swap_interval())
++        set_server_swap_interval(current_swap_interval);
++
      secured_region.reset();
      // TODO: We can fix the strange "ID casting" used below in the second phase
@@ -406,9 +421,73 @@
          mir_display_output_id_invalid};
+ }
++void mcl::BufferStream::wait_for_vsync()
++{
++    mir::time::PosixTimestamp last, target;
++    std::shared_ptr<FrameClock> clock;
++
++    {
++        std::lock_guard<decltype(mutex)> lock(mutex);
++        if (!frame_clock)
++            return;
++        last = last_vsync;
++        clock = frame_clock;
++    }
++
++    target = clock->next_frame_after(last);
++    sleep_until(target);
++
++    {
++        std::lock_guard<decltype(mutex)> lock(mutex);
++        if (target > last_vsync)
++            last_vsync = target;
++    }
++}
++
++MirWaitHandle* mcl::BufferStream::set_server_swap_interval(int i)
++{
++    /*
++     * TODO: Remove these functions in future, after
++     *       mir_buffer_stream_swap_buffers has been converted to use
++     *       client-side vsync, and the server has been told to always use
++     *       dropping for BufferStream. Then there will be no need to ever
++     *       change the server swap interval from zero.
++     */
++    buffer_depository->set_interval(i);
++    return interval_config.set_swap_interval(i);
++}
++
  void mcl::BufferStream::swap_buffers_sync()
+ {
++    {
++        std::lock_guard<decltype(mutex)> lock(mutex);
++        using_client_side_vsync = true;
++    }
++
++    /*
++     * Until recently it seemed like server-side vsync could co-exist with
++     * client-side vsync. However my original theory that it was risky has
++     * proven to be true. If we leave server-side vsync throttling to interval
++     * one at the same time as using client-side, there's a risk the server
++     * will not get scheduled sufficiently to drain the queue as fast as
++     * we fill it, creating lag. The acceptance test `ClientLatency' has now
++     * proven this is a real problem so we must be sure to put the server in
++     * interval 0 when using client-side vsync. This guarantees that random
++     * scheduling imperfections won't create queuing lag.
++     */
++    if (interval_config.swap_interval() != 0)
++        set_server_swap_interval(0);
++
      swap_buffers([](){})->wait_for_all();
++
++    {
++        std::lock_guard<decltype(mutex)> lock(mutex);
++        using_client_side_vsync = false;
++    }
++
++    int interval = swap_interval();
++    for (int i = 0; i < interval; ++i)
++        wait_for_vsync();
+ }
  void mcl::BufferStream::request_and_wait_for_configure(MirWindowAttrib attrib, int interval)
@@ -428,16 +507,42 @@
  int mcl::BufferStream::swap_interval() const
+ {
--    return interval_config.swap_interval();
++    std::lock_guard<decltype(mutex)> lock(mutex);
++    return current_swap_interval;
+ }
  MirWaitHandle* mcl::BufferStream::set_swap_interval(int interval)
+ {
--    if (user_swap_interval.is_set())
--        interval = user_swap_interval.value();
--
--    buffer_depository->set_interval(interval);
--    return interval_config.set_swap_interval(interval);
++    std::lock_guard<decltype(mutex)> lock(mutex);
++    current_swap_interval = user_swap_interval.is_set() ?
++        user_swap_interval.value() : interval;
++
++    return set_server_swap_interval(current_swap_interval);
++}
++
++void mcl::BufferStream::adopted_by(MirWindow* win)
++{
++    std::lock_guard<decltype(mutex)> lock(mutex);
++    /*
++     * Yes, we're storing raw pointers here. That's safe so long as MirWindow
++     * remembers to always call unadopted_by prior to its destruction.
++     *   The alternative of MirWindow passing in a shared_ptr to itself is
++     * actually uglier than this...
++     */
++    users.insert(win);
++    if (!frame_clock)
++        frame_clock = win->get_frame_clock();
++}
++
++void mcl::BufferStream::unadopted_by(MirWindow* win)
++{
++    std::lock_guard<decltype(mutex)> lock(mutex);
++    users.erase(win);
++    if (frame_clock == win->get_frame_clock())
++    {
++        frame_clock = users.empty() ? nullptr
++                                    : (*users.begin())->get_frame_clock();
++    }
+ }
  MirNativeBuffer* mcl::BufferStream::get_current_buffer_package()
 === modified file 'src/client/buffer_stream.h'
 --- src/client/buffer_stream.h	2017-01-17 05:51:54 +0000
 +++ src/client/buffer_stream.h	2017-01-17 06:23:26 +0000
@@ -26,11 +26,13 @@
  #include "mir/geometry/size.h"
  #include "mir/optional_value.h"
  #include "buffer_stream_configuration.h"
++#include "frame_clock.h"
  #include "mir_toolkit/client_types.h"
  #include <EGL/eglplatform.h>
++#include <unordered_set>
  #include <queue>
  #include <memory>
  #include <mutex>
@@ -97,6 +99,8 @@
      int swap_interval() const override;
      MirWaitHandle* set_swap_interval(int interval) override;
++    void adopted_by(MirWindow*) override;
++    void unadopted_by(MirWindow*) override;
      void set_buffer_cache_size(unsigned int) override;
      EGLNativeWindowType egl_native_window() override;
@@ -133,8 +137,9 @@
      void process_buffer(protobuf::Buffer const& buffer, std::unique_lock<std::mutex>&);
      void on_scale_set(float scale);
      void release_cpu_region();
--    MirWaitHandle* force_swap_interval(int interval);
++    MirWaitHandle* set_server_swap_interval(int i);
      void init_swap_interval();
++    void wait_for_vsync();
      mutable std::mutex mutex; // Protects all members of *this
@@ -143,6 +148,8 @@
      std::unique_ptr<mir::protobuf::BufferStream> protobuf_bs;
      optional_value<int> user_swap_interval;
++    int current_swap_interval;
++    bool using_client_side_vsync;
      BufferStreamConfiguration interval_config;
      float scale_;
@@ -161,6 +168,10 @@
      std::weak_ptr<SurfaceMap> const map;
      std::shared_ptr<AsyncBufferFactory> const factory;
      MirRenderSurface* render_surface_;
++
++    std::unordered_set<MirWindow*> users;
++    std::shared_ptr<FrameClock> frame_clock;
++    mir::time::PosixTimestamp last_vsync;
  };
+ }
 === modified file 'src/client/error_stream.cpp'
 --- src/client/error_stream.cpp	2017-01-17 05:51:54 +0000
 +++ src/client/error_stream.cpp	2017-01-17 06:23:26 +0000
@@ -94,6 +94,12 @@
+ {
      BOOST_THROW_EXCEPTION(std::runtime_error(error));
+ }
++void mcl::ErrorStream::adopted_by(MirWindow*)
++{
++}
++void mcl::ErrorStream::unadopted_by(MirWindow*)
++{
++}
  MirNativeBuffer* mcl::ErrorStream::get_current_buffer_package()
+ {
      BOOST_THROW_EXCEPTION(std::runtime_error(error));
 === modified file 'src/client/error_stream.h'
 --- src/client/error_stream.h	2017-01-17 05:51:54 +0000
 +++ src/client/error_stream.h	2017-01-17 06:23:26 +0000
@@ -41,6 +41,8 @@
      std::shared_ptr<MemoryRegion> secure_for_cpu_write() override;
      int swap_interval() const override;
      MirWaitHandle* set_swap_interval(int interval) override;
++    void adopted_by(MirWindow*) override;
++    void unadopted_by(MirWindow*) override;
      MirNativeBuffer* get_current_buffer_package() override;
      MirPlatformType platform_type() override;
      frontend::BufferStreamId rpc_id() const override;
 === added file 'src/client/frame_clock.cpp'
 --- src/client/frame_clock.cpp	1970-01-01 00:00:00 +0000
 +++ src/client/frame_clock.cpp	2017-01-17 06:23:26 +0000
@@ -0,0 +1,156 @@
++/*
++ * Copyright © 2016 Canonical Ltd.
++ *
++ * This program is free software: you can redistribute it and/or modify it
++ * under the terms of the GNU Lesser General Public License version 3,
++ * as published by the Free Software Foundation.
++ *
++ * This program is distributed in the hope that it will be useful,
++ * but WITHOUT ANY WARRANTY; without even the implied warranty of
++ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
++ * GNU Lesser General Public License for more details.
++ *
++ * You should have received a copy of the GNU Lesser General Public License
++ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
++ *
++ * Authored by: Daniel van Vugt <daniel.van.vugt@canonical.com>
++ */
++
++#include "frame_clock.h"
++#include <stdexcept>
++#include <cassert>
++
++using mir::client::FrameClock;
++using mir::time::PosixTimestamp;
++
++namespace
++{
++typedef std::unique_lock<std::mutex> Lock;
++} // namespace
++
++FrameClock::FrameClock(FrameClock::GetCurrentTime gct)
++    : get_current_time{gct}
++    , config_changed{false}
++    , period{0}
++    , resync_callback{std::bind(&FrameClock::fallback_resync_callback, this)}
++{
++}
++
++void FrameClock::set_period(std::chrono::nanoseconds ns)
++{
++    Lock lock(mutex);
++    period = ns;
++    config_changed = true;
++}
++
++void FrameClock::set_resync_callback(ResyncCallback cb)
++{
++    Lock lock(mutex);
++    resync_callback = cb;
++    config_changed = true;
++}
++
++PosixTimestamp FrameClock::fallback_resync_callback() const
++{
++    auto const now = get_current_time(PosixTimestamp().clock_id);
++    Lock lock(mutex);
++    /*
++     * The result here needs to be in phase for all processes that call it,
++     * so that nesting servers does not add lag.
++     */
++    return period != period.zero() ? now - (now % period) : now;
++}
++
++PosixTimestamp FrameClock::next_frame_after(PosixTimestamp prev) const
++{
++    Lock lock(mutex);
++    /*
++     * Unthrottled is an option too. But why?... Because a stream might exist
++     * that's not bound to a surface. And if it's not bound to a surface then
++     * it has no physical screen to sync to. Hence not throttled at all.
++     */
++    if (period == period.zero())
++        return prev;
++
++    /*
++     * Regardless of render times and scheduling delays, we should always
++     * target a perfectly even interval. This results in the greatest
++     * visual smoothness as well as providing a catch-up for cases where
++     * the client's render time was a little too long.
++     */
++    auto target = prev + period;
++
++    /*
++     * Count missed frames. Note how the target time would need to be more than
++     * one frame in the past to count as a missed frame. If the target time
++     * is less than one frame in the past then there's still a chance to catch
++     * up by rendering the next frame without delay. This avoids decimating
++     * to half frame rate.
++     *   Compared to triple buffering, this approach is reactive rather than
++     * proactive. Both approaches have the same catch-up ability to avoid
++     * half frame rate, but this approach has the added benefit of only
++     * one frame of lag to the compositor compared to triple buffering's two
++     * frames of lag. But we're also doing better than double buffering here
++     * in that this approach avoids any additive lag when nested.
++     */
++    auto now = get_current_time(target.clock_id);
++    long const missed_frames = now > target ? (now - target) / period : 0L;
++
++    /*
++     * On the first frame and any resumption frame (after the client goes
++     * from idle to busy) this will trigger a query to ask for the latest
++     * hardware vsync timestamp. So we get in phase with the display.
++     * Crucially this is not required on most frames, so that even if it is
++     * implemented as a round trip to the server, that won't happen often.
++     */
++    if (missed_frames > 1 || config_changed)
++    {
++        lock.unlock();
++        auto const server_frame = resync_callback();
++        lock.lock();
++
++        /*
++         * Avoid mismatches (which will throw) and ensure we're always
++         * comparing timestamps of the same clock ID. This means our result
++         * 'target' might have a different clock to that of 'prev'. And that's
++         * OK... we want to use whatever clock ID the driver is using for
++         * its hardware vsync timestamps. We support migrating between clocks.
++         */
++        now = get_current_time(server_frame.clock_id);
++
++        /*
++         * It's important to target a future time and not allow 'now'. This
++         * is because we will have some buffer queues for the time being that
++         * have swapinterval 1, which if overfilled by being too agressive will
++         * cause visual lag. After all buffer queues move to always dropping
++         * (mailbox mode), this delay won't be required as a safety.
++         */
++        if (server_frame > now)
++        {
++            target = server_frame;
++        }
++        else
++        {
++            auto const age_ns = now - server_frame;
++            /*
++             * Ensure age_frames gets truncated if not already.
++             * C++ just guarantees a "signed integer type of at least 64 bits"
++             * for std::chrono::nanoseconds::rep
++             */
++            auto const age_frames = age_ns / period;
++            target = server_frame + (age_frames + 1) * period;
++        }
++        assert(target > now);
++        config_changed = false;
++    }
++    else if (missed_frames > 0)
++    {
++        /*
++         * Low number of missed frames. Try catching up without missing more...
++         */
++        // FIXME? Missing += operator
++        target = target + missed_frames * period;
++    }
++
++    return target;
++}
 === added file 'src/client/frame_clock.h'
 --- src/client/frame_clock.h	1970-01-01 00:00:00 +0000
 +++ src/client/frame_clock.h	2017-01-17 06:23:26 +0000
@@ -0,0 +1,73 @@
++/*
++ * Copyright © 2016 Canonical Ltd.
++ *
++ * This program is free software: you can redistribute it and/or modify it
++ * under the terms of the GNU Lesser General Public License version 3,
++ * as published by the Free Software Foundation.
++ *
++ * This program is distributed in the hope that it will be useful,
++ * but WITHOUT ANY WARRANTY; without even the implied warranty of
++ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
++ * GNU Lesser General Public License for more details.
++ *
++ * You should have received a copy of the GNU Lesser General Public License
++ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
++ *
++ * Authored by: Daniel van Vugt <daniel.van.vugt@canonical.com>
++ */
++
++#ifndef MIR_CLIENT_FRAME_CLOCK_H_
++#define MIR_CLIENT_FRAME_CLOCK_H_
++
++#include "mir/time/posix_timestamp.h"
++#include <chrono>
++#include <functional>
++#include <mutex>
++
++namespace mir { namespace client {
++
++class FrameClock
++{
++public:
++    typedef std::function<time::PosixTimestamp()> ResyncCallback;
++    typedef std::function<time::PosixTimestamp(clockid_t)> GetCurrentTime;
++
++    FrameClock(GetCurrentTime);
++    FrameClock() : FrameClock(mir::time::PosixTimestamp::now) {}
++
++    /**
++     * Set the precise frame period in nanoseconds (1000000000/Hz).
++     */
++    void set_period(std::chrono::nanoseconds);
++
++    /**
++     * Optionally set a callback that obtains the latest hardware vsync
++     * timestamp. This provides phase correction for increased precision but is
++     * not strictly required.
++     *   Lowest precision: Don't provide a callback.
++     *   Medium precision: Provide a callback which returns a recent timestamp.
++     *   Highest precision: Provide a callback that queries the server.
++     */
++    void set_resync_callback(ResyncCallback);
++
++    /**
++     * 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.
++     */
++    time::PosixTimestamp next_frame_after(time::PosixTimestamp prev) const;
++
++private:
++    time::PosixTimestamp fallback_resync_callback() const;
++
++    GetCurrentTime const get_current_time;
++
++    mutable std::mutex mutex;  // Protects below fields:
++    mutable bool config_changed;
++    std::chrono::nanoseconds period;
++    ResyncCallback resync_callback;
++};
++
++}} // namespace mir::client
++
++#endif // MIR_CLIENT_FRAME_CLOCK_H_
 === modified file 'src/client/mir_buffer_stream_api.cpp'
 --- src/client/mir_buffer_stream_api.cpp	2017-01-17 05:51:54 +0000
 +++ src/client/mir_buffer_stream_api.cpp	2017-01-17 06:23:26 +0000
@@ -113,6 +113,13 @@
      void* context)
  try
+ {
++    /*
++     * TODO: Add client-side vsync support for mir_buffer_stream_swap_buffers()
++     *       Not in a hurry though, because the old server-side vsync is still
++     *       present and AFAIK the only user of swap_buffers callbacks is Xmir.
++     *       There are many ways to approach the problem and some more
++     *       contentious than others, so do it later.
++     */
      return buffer_stream->swap_buffers([buffer_stream, callback, context]{
              if (callback)
                  callback(buffer_stream, context);
@@ -209,7 +216,7 @@
  MirWaitHandle* mir_buffer_stream_set_swapinterval(MirBufferStream* buffer_stream, int interval)
  try
+ {
--    if ((interval < 0) || (interval > 1))
++    if (interval < 0)
          return nullptr;
      if (!buffer_stream)
 === modified file 'src/client/mir_surface.cpp'
 --- src/client/mir_surface.cpp	2017-01-17 05:51:54 +0000
 +++ src/client/mir_surface.cpp	2017-01-17 06:23:26 +0000
@@ -21,6 +21,7 @@
  #include "cursor_configuration.h"
  #include "make_protobuf_object.h"
  #include "mir_protobuf.pb.h"
++#include "connection_surface_map.h"
  #include "mir_toolkit/mir_client_library.h"
  #include "mir/frontend/client_constants.h"
@@ -31,6 +32,7 @@
  #include "mir/input/xkb_mapper.h"
  #include "mir/cookie/cookie.h"
  #include "mir_cookie.h"
++#include "mir/time/posix_timestamp.h"
  #include <cassert>
  #include <unistd.h>
@@ -46,6 +48,8 @@
  namespace gp = google::protobuf;
  namespace md = mir::dispatch;
++using mir::client::FrameClock;
++
  namespace
+ {
  std::mutex handle_mutex;
@@ -96,6 +100,7 @@
      std::shared_ptr<MirWaitHandle> const& handle) :
      surface{mcl::make_protobuf_object<mir::protobuf::Surface>()},
      connection_(conn),
++    frame_clock(std::make_shared<FrameClock>()),
      creation_handle(handle)
+ {
      surface->set_error(error);
@@ -103,6 +108,8 @@
      std::lock_guard<decltype(handle_mutex)> lock(handle_mutex);
      valid_surfaces.insert(this);
++
++    configure_frame_clock();
+ }
  MirSurface::MirSurface(
@@ -126,12 +133,19 @@
        input_platform(input_platform),
        keymapper(std::make_shared<mircv::XKBMapper>()),
        configure_result{mcl::make_protobuf_object<mir::protobuf::SurfaceSetting>()},
++      frame_clock(std::make_shared<FrameClock>()),
        creation_handle(handle),
        size({surface_proto.width(), surface_proto.height()}),
        format(static_cast<MirPixelFormat>(surface_proto.pixel_format())),
        usage(static_cast<MirBufferUsage>(surface_proto.buffer_usage())),
        output_id(spec.output_id.is_set() ? spec.output_id.value() : static_cast<uint32_t>(mir_display_output_id_invalid))
+ {
++    if (default_stream)
++    {
++        streams.insert(default_stream);
++        default_stream->adopted_by(this);
++    }
++
      for(int i = 0; i < surface_proto.attributes_size(); i++)
+     {
          auto const& attrib = surface_proto.attributes(i);
@@ -155,10 +169,23 @@
      std::lock_guard<decltype(handle_mutex)> lock(handle_mutex);
      valid_surfaces.insert(this);
++
++    configure_frame_clock();
+ }
  MirSurface::~MirSurface()
+ {
++    StreamSet old_streams;
++
++    {
++        std::lock_guard<decltype(mutex)> lock(mutex);
++        old_streams = std::move(streams);
++    }
++
++    // Do callbacks without holding the lock
++    for (auto const& s : old_streams)
++        s->unadopted_by(this);
++
+     {
          std::lock_guard<decltype(handle_mutex)> lock(handle_mutex);
          valid_surfaces.erase(this);
@@ -172,6 +199,18 @@
          close(surface->fd(i));
+ }
++void MirSurface::configure_frame_clock()
++{
++    /*
++     * TODO: Implement frame_clock->set_resync_callback(...) when IPC to get
++     *       timestamps from the server exists.
++     *       Until we do, client-side vsync will perform suboptimally (it is
++     *       randomly up to one frame out of phase with the real display).
++     *       However even while it's suboptimal it's dramatically lower latency
++     *       than the old approach and still totally eliminates nesting lag.
++     */
++}
++
  MirWindowParameters MirSurface::get_parameters() const
+ {
      std::lock_guard<decltype(mutex)> lock(mutex);
@@ -458,6 +497,36 @@
              default_stream->set_size(size);
          break;
+     }
++    case mir_event_type_surface_output:
++    {
++        /*
++         * We set the frame clock according to the (maximum) refresh rate of
++         * the display. Note that we don't do the obvious thing and measure
++         * previous vsyncs mainly because that's a very unreliable predictor
++         * of the desired maximum refresh rate in the case of GSync/FreeSync
++         * and panel self-refresh. You can't assume the previous frame
++         * represents the display running at full native speed and so should
++         * not measure it. Instead we conveniently have
++         * mir_surface_output_event_get_refresh_rate that tells us the full
++         * native speed of the most relevant output...
++         */
++        auto soevent = mir_event_get_surface_output_event(&e);
++        auto rate = mir_surface_output_event_get_refresh_rate(soevent);
++        if (rate > 10.0)  // should be >0, but 10 to workaround LP: #1639725
++        {
++            std::chrono::nanoseconds const ns(
++                static_cast<long>(1000000000L / rate));
++            frame_clock->set_period(ns);
++        }
++        /* else: The graphics driver has not provided valid timing so we will
++         *       default to swap interval 0 behaviour.
++         *       Or would people prefer some different fallback?
++         *         - Re-enable server-side vsync?
++         *         - Rate limit to 60Hz?
++         *         - Just fix LP: #1639725?
++         */
++        break;
++    }
      default:
          break;
      };
@@ -609,17 +678,30 @@
      if (spec.streams.is_set())
+     {
++        std::shared_ptr<mir::client::ConnectionSurfaceMap> map;
++        StreamSet old_streams, new_streams;
++
          /*
--         * Note that we don't check for errors from modify_surface. So in
--         * updating default_stream here, we're just assuming it will succeed.
--         * Seems to be a harmless assumption to have made so far and much
--         * simpler than communicating back suceessful mappings from the server,
--         * but there is a prototype started for that: lp:~vanvugt/mir/adoption
++         * Note that we are updating our local copy of 'streams' before the
++         * server has updated its copy. That's mainly because we have never yet
++         * implemented any kind of client-side error handling for when
++         * modify_surface fails and is rejected. There's a prototype started
++         * along those lines in lp:~vanvugt/mir/adoption, but it does not seem
++         * to be important to block on...
++         *   The worst case is still perfectly acceptable: That is if the server
++         * rejects the surface spec our local copy of streams is wrong. That's
++         * a harmless consequence which just means the stream is now
++         * synchronized to the output the client intended even though the server
++         * is refusing to display it.
           */
+         {
              std::lock_guard<decltype(mutex)> lock(mutex);
              default_stream = nullptr;
++            old_streams = std::move(streams);
++            streams.clear();  // Leave the container valid before we unlock
++            map = connection_->connection_surface_map();
+         }
++
          for(auto const& stream : spec.streams.value())
+         {
              auto const new_stream = surface_specification->add_stream();
@@ -631,6 +713,27 @@
                  new_stream->set_width(stream.size.value().width.as_int());
                  new_stream->set_height(stream.size.value().height.as_int());
+             }
++
++            mir::frontend::BufferStreamId id(stream.stream_id);
++            if (auto bs = map->stream(id))
++                new_streams.insert(bs);
++            /* else: we could implement stream ID validation here, which we've
++             * never had before.
++             * The only problem with that plan is that we have some tests
++             * that rely on the ability to pass in invalid stream IDs so those
++             * need fixing.
++             */
++        }
++
++        // Worth optimizing and avoiding the intersection of both sets?....
++        for (auto const& old_stream : old_streams)
++            old_stream->unadopted_by(this);
++        for (auto const& new_stream : new_streams)
++            new_stream->adopted_by(this);
++        // ... probably not, since we can std::move(new_streams) this way...
++        {
++            std::unique_lock<decltype(mutex)> lock(mutex);
++            streams = std::move(new_streams);
+         }
+     }
@@ -672,3 +775,8 @@
+ {
      return connection_;
+ }
++
++std::shared_ptr<FrameClock> MirSurface::get_frame_clock() const
++{
++    return frame_clock;
++}
 === modified file 'src/client/mir_surface.h'
 --- src/client/mir_surface.h	2017-01-17 05:51:54 +0000
 +++ src/client/mir_surface.h	2017-01-17 06:23:26 +0000
@@ -21,6 +21,7 @@
  #include "cursor_configuration.h"
  #include "mir/mir_buffer_stream.h"
  #include "mir_wait_handle.h"
++#include "frame_clock.h"
  #include "rpc/mir_display_server.h"
  #include "rpc/mir_display_server_debug.h"
@@ -32,6 +33,7 @@
  #include "mir_toolkit/common.h"
  #include "mir_toolkit/mir_client_library.h"
  #include "mir/graphics/native_buffer.h"
++#include "mir/time/posix_timestamp.h"
  #include <memory>
  #include <functional>
@@ -214,9 +216,13 @@
      MirWaitHandle* request_persistent_id(mir_surface_id_callback callback, void* context);
      MirConnection* connection() const;
++
++    std::shared_ptr<mir::client::FrameClock> get_frame_clock() const;
++
  private:
      std::mutex mutable mutex; // Protects all members of *this
++    void configure_frame_clock();
      void on_configured();
      void on_cursor_configured();
      void acquired_persistent_id(mir_surface_id_callback callback, void* context);
@@ -242,6 +248,8 @@
      //Deprecated functions can cause MirSurfaces to be created with a default stream
      std::shared_ptr<MirBufferStream> default_stream;
++    typedef std::unordered_set<std::shared_ptr<MirBufferStream>> StreamSet;
++    StreamSet streams;
      std::shared_ptr<mir::input::receiver::InputPlatform> const input_platform;
      std::shared_ptr<mir::input::receiver::XKBMapper> const keymapper;
@@ -251,6 +259,8 @@
      int attrib_cache[mir_window_attribs];
      MirOrientation orientation = mir_orientation_normal;
++    std::shared_ptr<mir::client::FrameClock> const frame_clock;
++
      std::function<void(MirEvent const*)> handle_event_callback;
      std::shared_ptr<mir::dispatch::ThreadedDispatcher> input_thread;
 === modified file 'src/client/mir_surface_api.cpp'
 --- src/client/mir_surface_api.cpp	2017-01-17 05:51:54 +0000
 +++ src/client/mir_surface_api.cpp	2017-01-17 06:23:26 +0000
@@ -1204,7 +1204,7 @@
  MirWaitHandle* mir_surface_set_swapinterval(MirSurface* surf, int interval)
+ {
--    if ((interval < 0) || (interval > 1))
++    if (interval < 0)
          return nullptr;
      try
 === modified file 'src/client/screencast_stream.cpp'
 --- src/client/screencast_stream.cpp	2017-01-17 05:51:54 +0000
 +++ src/client/screencast_stream.cpp	2017-01-17 06:23:26 +0000
@@ -264,6 +264,14 @@
      BOOST_THROW_EXCEPTION(std::logic_error("Attempt to set swap interval on screencast is invalid"));
+ }
++void mcl::ScreencastStream::adopted_by(MirWindow*)
++{
++}
++
++void mcl::ScreencastStream::unadopted_by(MirWindow*)
++{
++}
++
  void mcl::ScreencastStream::set_size(geom::Size)
+ {
      BOOST_THROW_EXCEPTION(std::logic_error("Attempt to set size on screencast is invalid"));
 === modified file 'src/client/screencast_stream.h'
 --- src/client/screencast_stream.h	2017-01-17 05:51:54 +0000
 +++ src/client/screencast_stream.h	2017-01-17 06:23:26 +0000
@@ -78,6 +78,8 @@
      uint32_t get_current_buffer_id() override;
      int swap_interval() const override;
      MirWaitHandle* set_swap_interval(int interval) override;
++    void adopted_by(MirWindow*) override;
++    void unadopted_by(MirWindow*) override;
      void set_buffer_cache_size(unsigned int) override;
      EGLNativeWindowType egl_native_window() override;
 === modified file 'src/include/client/mir/mir_buffer_stream.h'
 --- src/include/client/mir/mir_buffer_stream.h	2017-01-17 05:51:54 +0000
 +++ src/include/client/mir/mir_buffer_stream.h	2017-01-17 06:23:26 +0000
@@ -88,6 +88,8 @@
      virtual int swap_interval() const = 0;
      virtual MirWaitHandle* set_swap_interval(int interval) = 0;
++    virtual void adopted_by(MirWindow*) = 0;
++    virtual void unadopted_by(MirWindow*) = 0;
      virtual MirNativeBuffer* get_current_buffer_package() = 0;
      virtual MirPlatformType platform_type() = 0;
 === modified file 'tests/acceptance-tests/test_client_library.cpp'
 --- tests/acceptance-tests/test_client_library.cpp	2017-01-17 05:51:54 +0000
 +++ tests/acceptance-tests/test_client_library.cpp	2017-01-17 06:23:26 +0000
@@ -809,6 +809,11 @@
      buffers = 0;
++    // StubDisplayConfiguration is set to 60Hz and we now actually honour
++    // that. So to avoid 1024 frames taking 17 seconds or so...
++    mir_buffer_stream_set_swapinterval(mir_surface_get_buffer_stream(surf_one), 0);
++    mir_buffer_stream_set_swapinterval(mir_surface_get_buffer_stream(surf_two), 0);
++
      while (buffers < 1024)
+     {
          mir_buffer_stream_swap_buffers_sync(
 === modified file 'tests/acceptance-tests/test_latency.cpp'
 --- tests/acceptance-tests/test_latency.cpp	2017-01-17 05:51:54 +0000
 +++ tests/acceptance-tests/test_latency.cpp	2017-01-17 06:23:26 +0000
@@ -24,6 +24,7 @@
  #include "mir/test/doubles/null_display.h"
  #include "mir/test/doubles/null_display_buffer.h"
  #include "mir/test/doubles/null_display_sync_group.h"
++#include "mir/test/doubles/stub_display_configuration.h"
  #include "mir/test/fake_shared.h"
  #include "mir_test_framework/visible_surface.h"
  #include "mir/options/option.h"
@@ -240,12 +241,36 @@
      TimeTrackingDisplay(Stats& stats)
          : group{stats}
+     {
++        mg::DisplayConfigurationOutput output{
++            mg::DisplayConfigurationOutputId{1},
++            mg::DisplayConfigurationCardId{1},
++            mg::DisplayConfigurationOutputType::hdmia,
++            std::vector<MirPixelFormat>{mir_pixel_format_abgr_8888},
++            {{{1920,1080}, refresh_rate}},  // <=== Refresh rate must be valid
++            0, mir::geometry::Size{}, true, true, mir::geometry::Point{}, 0,
++            mir_pixel_format_abgr_8888, mir_power_mode_on,
++            mir_orientation_normal,
++            1.0f,
++            mir_form_factor_monitor,
++            mir_subpixel_arrangement_unknown,
++            {},
++            mir_output_gamma_unsupported,
++            {}
++        };
++        outputs.push_back(output);
++    }
++
++    std::unique_ptr<mg::DisplayConfiguration> configuration() const override
++    {
++        return std::make_unique<mtd::StubDisplayConfig>(outputs);
+     }
      void for_each_display_sync_group(std::function<void(mg::DisplaySyncGroup&)> const& f) override
+     {
          f(group);
+     }
++
++    std::vector<mg::DisplayConfigurationOutput> outputs;
      TimeTrackingGroup group;
  };
@@ -288,7 +313,7 @@
  };
+ }
--TEST_F(ClientLatency, average_latency_is_less_than_nbuffers)
++TEST_F(ClientLatency, average_swap_buffers_sync_latency_is_one_frame)
+ {
      auto stream = mir_window_get_buffer_stream(surface);
      auto const deadline = steady_clock::now() + 60s;
@@ -308,7 +333,7 @@
      auto average_latency = display.group.average_latency();
--    EXPECT_THAT(average_latency, Lt(expected_client_buffers));
++    EXPECT_NEAR(1.0f, average_latency, error_margin);
+ }
  TEST_F(ClientLatency, max_latency_is_limited_to_nbuffers)
 === modified file 'tests/include/mir/test/doubles/mock_mir_buffer_stream.h'
 --- tests/include/mir/test/doubles/mock_mir_buffer_stream.h	2017-01-17 05:51:54 +0000
 +++ tests/include/mir/test/doubles/mock_mir_buffer_stream.h	2017-01-17 06:23:26 +0000
@@ -43,6 +43,8 @@
      MOCK_METHOD0(secure_for_cpu_write, std::shared_ptr<client::MemoryRegion>());
      MOCK_CONST_METHOD0(swap_interval, int());
      MOCK_METHOD1(set_swap_interval, MirWaitHandle*(int));
++    MOCK_METHOD1(adopted_by, void(MirWindow*));
++    MOCK_METHOD1(unadopted_by, void(MirWindow*));
      MOCK_METHOD0(platform_type, MirPlatformType(void));
      MOCK_METHOD0(get_current_buffer_package, MirNativeBuffer*(void));
      MOCK_METHOD0(get_create_wait_handle, MirWaitHandle*(void));
 === modified file 'tests/unit-tests/client/CMakeLists.txt'
 --- tests/unit-tests/client/CMakeLists.txt	2017-01-17 05:51:54 +0000
 +++ tests/unit-tests/client/CMakeLists.txt	2017-01-17 06:23:26 +0000
@@ -24,6 +24,7 @@
    ${CMAKE_CURRENT_SOURCE_DIR}/test_client_mir_error.cpp
    ${CMAKE_CURRENT_SOURCE_DIR}/test_no_tls_future.cpp
    ${CMAKE_CURRENT_SOURCE_DIR}/test_mir_render_surface.cpp
++  ${CMAKE_CURRENT_SOURCE_DIR}/test_frame_clock.cpp
+ )
  if (NOT MIR_DISABLE_INPUT)
 === modified file 'tests/unit-tests/client/test_client_mir_surface.cpp'
 --- tests/unit-tests/client/test_client_mir_surface.cpp	2017-01-17 05:51:54 +0000
 +++ tests/unit-tests/client/test_client_mir_surface.cpp	2017-01-17 06:23:26 +0000
@@ -416,6 +416,87 @@
      EXPECT_TRUE(dispatched->wait_for(std::chrono::seconds{5}));
+ }
++TEST_F(MirClientSurfaceTest, adopts_the_default_stream)
++{
++    using namespace ::testing;
++
++    auto mock_input_platform = std::make_shared<MockClientInputPlatform>();
++    auto mock_stream = std::make_shared<mtd::MockMirBufferStream>();
++
++    MirWindow* adopted_by = nullptr;
++    MirWindow* unadopted_by = nullptr;
++    EXPECT_CALL(*mock_stream, adopted_by(_))
++        .WillOnce(SaveArg<0>(&adopted_by));
++    EXPECT_CALL(*mock_stream, unadopted_by(_))
++        .WillOnce(SaveArg<0>(&unadopted_by));
++
++    {
++        MirWindow win{connection.get(), *client_comm_channel, nullptr,
++            mock_stream, mock_input_platform, spec, surface_proto, wh};
++        EXPECT_EQ(&win,    adopted_by);
++        EXPECT_EQ(nullptr, unadopted_by);
++    }
++
++    EXPECT_NE(nullptr,    unadopted_by);
++    EXPECT_EQ(adopted_by, unadopted_by);
++}
++
++TEST_F(MirClientSurfaceTest, adopts_custom_streams_if_set)
++{
++    using namespace testing;
++
++    auto mock_input_platform = std::make_shared<MockClientInputPlatform>();
++
++    mir::frontend::BufferStreamId const mock_old_stream_id(11);
++    auto mock_old_stream = std::make_shared<mtd::MockMirBufferStream>();
++    MirWindow* old_adopted_by = nullptr;
++    MirWindow* old_unadopted_by = nullptr;
++    EXPECT_CALL(*mock_old_stream, adopted_by(_))
++        .WillOnce(SaveArg<0>(&old_adopted_by));
++    EXPECT_CALL(*mock_old_stream, unadopted_by(_))
++        .WillOnce(SaveArg<0>(&old_unadopted_by));
++    ON_CALL(*mock_old_stream, rpc_id())
++        .WillByDefault(Return(mock_old_stream_id));
++
++    mir::frontend::BufferStreamId const mock_new_stream_id(22);
++    auto mock_new_stream = std::make_shared<mtd::MockMirBufferStream>();
++    MirWindow* new_adopted_by = nullptr;
++    MirWindow* new_unadopted_by = nullptr;
++    EXPECT_CALL(*mock_new_stream, adopted_by(_))
++        .WillOnce(SaveArg<0>(&new_adopted_by));
++    EXPECT_CALL(*mock_new_stream, unadopted_by(_))
++        .WillOnce(SaveArg<0>(&new_unadopted_by));
++    ON_CALL(*mock_new_stream, rpc_id())
++        .WillByDefault(Return(mock_new_stream_id));
++
++    surface_map->insert(mock_old_stream_id, mock_old_stream);
++    surface_map->insert(mock_new_stream_id, mock_new_stream);
++    {
++        MirWindow win{connection.get(), *client_comm_channel, nullptr,
++            mock_old_stream, mock_input_platform, spec, surface_proto, wh};
++
++        EXPECT_EQ(&win,    old_adopted_by);
++        EXPECT_EQ(nullptr, old_unadopted_by);
++        EXPECT_EQ(nullptr, new_adopted_by);
++        EXPECT_EQ(nullptr, new_unadopted_by);
++
++        MirWindowSpec spec;
++        std::vector<ContentInfo> replacements
++        {
++            {geom::Displacement{0,0}, mock_new_stream_id.as_value(), geom::Size{1,1}}
++        };
++        spec.streams = replacements;
++        win.modify(spec)->wait_for_all();
++
++        EXPECT_EQ(&win,    old_unadopted_by);
++        EXPECT_EQ(&win,    new_adopted_by);
++        EXPECT_EQ(nullptr, new_unadopted_by);
++    }
++    EXPECT_EQ(new_adopted_by, new_unadopted_by);
++    surface_map->erase(mock_old_stream_id);
++    surface_map->erase(mock_new_stream_id);
++}
++
  TEST_F(MirClientSurfaceTest, replacing_delegate_with_nullptr_prevents_further_dispatch)
+ {
      using namespace ::testing;
 === added file 'tests/unit-tests/client/test_frame_clock.cpp'
 --- tests/unit-tests/client/test_frame_clock.cpp	1970-01-01 00:00:00 +0000
 +++ tests/unit-tests/client/test_frame_clock.cpp	2017-01-17 06:23:26 +0000
@@ -0,0 +1,420 @@
++/*
++ * Copyright © 2016 Canonical Ltd.
++ *
++ * This program is free software: you can redistribute it and/or modify
++ * it under the terms of the GNU General Public License version 3 as
++ * published by the Free Software Foundation.
++ *
++ * This program is distributed in the hope that it will be useful,
++ * but WITHOUT ANY WARRANTY; without even the implied warranty of
++ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
++ * GNU General Public License for more details.
++ *
++ * You should have received a copy of the GNU General Public License
++ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
++ *
++ * Authored by: Daniel van Vugt <daniel.van.vugt@canonical.com>
++ */
++
++#include "src/client/frame_clock.h"
++#include <gtest/gtest.h>
++#include <unordered_map>
++
++using namespace ::testing;
++using namespace ::std::chrono_literals;
++using mir::time::PosixTimestamp;
++using mir::client::FrameClock;
++
++class FrameClockTest : public Test
++{
++public:
++    FrameClockTest()
++    {
++        with_fake_time = [this](clockid_t clk){ return fake_time[clk]; };
++    }
++
++    void SetUp()
++    {
++        for (auto& c : {CLOCK_MONOTONIC, CLOCK_REALTIME,
++                        CLOCK_MONOTONIC_RAW, CLOCK_REALTIME_COARSE,
++                        CLOCK_MONOTONIC_COARSE, CLOCK_BOOTTIME})
++            fake_time[c] = PosixTimestamp(c, c * c * 1234567ns);
++        int const hz = 60;
++        one_frame = std::chrono::nanoseconds(1000000000L/hz);
++    }
++
++    void fake_sleep_for(std::chrono::nanoseconds ns)
++    {
++        for (auto& f : fake_time)
++            f.second.nanoseconds += ns;
++    }
++
++    void fake_sleep_until(PosixTimestamp t)
++    {
++        auto& now = fake_time[t.clock_id];
++        if (t > now)
++        {
++            auto delta = t.nanoseconds - now.nanoseconds;
++            fake_sleep_for(delta);
++        }
++    }
++
++protected:
++    FrameClock::GetCurrentTime with_fake_time;
++    std::unordered_map<clockid_t,PosixTimestamp> fake_time;
++    std::chrono::nanoseconds one_frame;
++};
++
++TEST_F(FrameClockTest, unthrottled_without_a_period)
++{
++    FrameClock clock(with_fake_time);
++
++    PosixTimestamp a;
++    auto b = clock.next_frame_after(a);
++    EXPECT_EQ(a, b);
++    auto c = clock.next_frame_after(b);
++    EXPECT_EQ(b, c);
++}
++
++TEST_F(FrameClockTest, first_frame_is_within_one_frame)
++{
++    auto& now = fake_time[CLOCK_MONOTONIC];
++    auto a = now;
++    FrameClock clock(with_fake_time);
++    clock.set_period(one_frame);
++
++    PosixTimestamp b;
++    auto c = clock.next_frame_after(b);
++    EXPECT_GE(c, a);
++    EXPECT_LE(c-a, one_frame);
++}
++
++TEST_F(FrameClockTest, interval_is_perfectly_smooth)
++{
++    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);
++    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);
++    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);
++    EXPECT_EQ(one_frame, e - d);
++}
++
++TEST_F(FrameClockTest, long_render_time_is_recoverable_without_decimation)
++{
++    FrameClock clock(with_fake_time);
++    clock.set_period(one_frame);
++
++    auto& now = fake_time[CLOCK_MONOTONIC];
++    auto a = now;
++    auto b = clock.next_frame_after(a);
++
++    fake_sleep_until(b);
++    fake_sleep_for(one_frame * 5 / 4);  // long render time; over a frame
++
++    auto c = clock.next_frame_after(b);
++    EXPECT_EQ(one_frame, c - b);
++
++    fake_sleep_until(c);
++    fake_sleep_for(one_frame * 7 / 6);  // long render time; over a frame
++
++    auto d = clock.next_frame_after(c);
++    EXPECT_EQ(one_frame, d - c);
++
++    EXPECT_LT(d, now);
++
++    fake_sleep_until(d);
++    fake_sleep_for(one_frame/4);  // short render time
++
++    // We can recover since we had a short render time...
++    auto e = clock.next_frame_after(d);
++    EXPECT_EQ(one_frame, e - d);
++}
++
++TEST_F(FrameClockTest, resuming_from_sleep_targets_the_future)
++{
++    FrameClock clock(with_fake_time);
++    clock.set_period(one_frame);
++
++    auto& now = fake_time[CLOCK_MONOTONIC];
++    PosixTimestamp a = now;
++    auto b = clock.next_frame_after(a);
++    fake_sleep_until(b);
++    auto c = clock.next_frame_after(b);
++    EXPECT_EQ(one_frame, c - b);
++    fake_sleep_until(c);
++
++    // Client idles for a while without producing new frames:
++    fake_sleep_for(567 * one_frame);
++
++    auto d = clock.next_frame_after(c);
++    EXPECT_GT(d, now);  // Resumption must be in the future
++    EXPECT_LE(d, now+one_frame);  // But not too far in the future
++}
++
++TEST_F(FrameClockTest, multiple_streams_in_sync)
++{
++    FrameClock clock(with_fake_time);
++    clock.set_period(one_frame);
++
++    PosixTimestamp left;
++    left = clock.next_frame_after(left);
++
++    fake_sleep_for(one_frame / 9);
++
++    PosixTimestamp right;
++    right = clock.next_frame_after(right);
++
++    ASSERT_EQ(left, right);
++    fake_sleep_until(left);
++    fake_sleep_until(right);
++
++    left = clock.next_frame_after(left);
++    fake_sleep_for(one_frame / 5);
++    right = clock.next_frame_after(right);
++
++    ASSERT_EQ(left, right);
++    fake_sleep_until(left);
++    fake_sleep_until(right);
++
++    left = clock.next_frame_after(left);
++    fake_sleep_for(one_frame / 7);
++    right = clock.next_frame_after(right);
++
++    ASSERT_EQ(left, right);
++}
++
++TEST_F(FrameClockTest, moving_between_displays_adapts_to_new_rate)
++{
++    FrameClock clock(with_fake_time);
++    clock.set_period(one_frame);
++
++    auto const one_tv_frame = std::chrono::nanoseconds(1000000000L / 25);
++    ASSERT_NE(one_frame, one_tv_frame);
++
++    PosixTimestamp a;
++    auto b = clock.next_frame_after(a);
++    fake_sleep_until(b);
++    auto c = clock.next_frame_after(b);
++    EXPECT_EQ(one_frame, c - b);
++
++    fake_sleep_until(c);
++
++    // Window moves to a new slower display:
++    clock.set_period(one_tv_frame);
++    auto d = clock.next_frame_after(c);
++    // Clock keeps ticking into the future for the new display:
++    EXPECT_GT(d, c);
++    // But not too far in the future:
++    EXPECT_LE(d, c + one_tv_frame);
++
++    // Vsync is now at the slower rate of the TV:
++    fake_sleep_until(d);
++    auto e = clock.next_frame_after(d);
++    EXPECT_EQ(one_tv_frame, e - d);
++    fake_sleep_until(e);
++    auto f = clock.next_frame_after(e);
++    EXPECT_EQ(one_tv_frame, f - e);
++
++    fake_sleep_until(f);
++
++    // Window moves back to the faster display:
++    clock.set_period(one_frame);
++    auto g = clock.next_frame_after(f);
++    // Clock keeps ticking into the future for the new display:
++    EXPECT_GT(g, f);
++    // But not too far in the future:
++    EXPECT_LE(g, f + one_tv_frame);
++
++    // Vsync is now at the faster rate again:
++    fake_sleep_until(g);
++    auto h = clock.next_frame_after(g);
++    EXPECT_EQ(one_frame, h - g);
++    fake_sleep_until(e);
++    auto i = clock.next_frame_after(h);
++    EXPECT_EQ(one_frame, i - h);
++}
++
++TEST_F(FrameClockTest, resuming_comes_in_phase_with_server_vsync)
++{
++    FrameClock clock(with_fake_time);
++    clock.set_period(one_frame);
++
++    auto& now = fake_time[CLOCK_MONOTONIC];
++    PosixTimestamp a = now;
++    auto b = clock.next_frame_after(a);
++    fake_sleep_until(b);
++    auto c = clock.next_frame_after(b);
++    EXPECT_EQ(one_frame, c - b);
++    fake_sleep_until(c);
++
++    // Client idles for a while without producing new frames:
++    fake_sleep_for(789 * one_frame);
++
++    auto last_server_frame = now - 556677ns;
++    clock.set_resync_callback([last_server_frame](){return last_server_frame;});
++
++    auto d = clock.next_frame_after(c);
++    EXPECT_GT(d, now);  // Resumption must be in the future
++    EXPECT_LE(d, now+one_frame);  // But not too far in the future
++
++    auto server_phase = last_server_frame % one_frame;
++    EXPECT_NE(server_phase, c % one_frame);  // wasn't in phase before
++    EXPECT_EQ(server_phase, d % one_frame);  // but is in phase now
++
++    // Not only did we come in phase but we're targeting the soonest frame
++    EXPECT_EQ(last_server_frame+one_frame, d);
++}
++
++TEST_F(FrameClockTest, switches_to_the_server_clock_on_startup)
++{
++    FrameClock clock(with_fake_time);
++    clock.set_period(one_frame);
++
++    PosixTimestamp a(CLOCK_MONOTONIC_COARSE, 0ns);
++
++    PosixTimestamp b;
++    EXPECT_NO_THROW({
++        b = clock.next_frame_after(a);
++    });
++
++    // The default resync callback uses CLOCK_MONOTONIC...
++    EXPECT_NE(a.clock_id, b.clock_id);
++}
++
++TEST_F(FrameClockTest, can_migrate_between_different_driver_clocks)
++{
++    FrameClock clock(with_fake_time);
++    clock.set_period(one_frame);
++
++    PosixTimestamp a = fake_time[CLOCK_MONOTONIC];
++    auto b = clock.next_frame_after(a);
++    fake_sleep_until(b);
++    auto c = clock.next_frame_after(b);
++    EXPECT_EQ(one_frame, c - b);
++
++    fake_sleep_until(c);
++
++    // Window moves between displays and in the glorious future that might even
++    // mean it switches cards, with different drivers, different clocks...
++    auto last_server_frame = fake_time[CLOCK_MONOTONIC_COARSE] - 556677ns;
++    clock.set_resync_callback([last_server_frame](){return last_server_frame;});
++
++    EXPECT_EQ(CLOCK_MONOTONIC, c.clock_id);
++    auto d = clock.next_frame_after(c);
++    EXPECT_EQ(CLOCK_MONOTONIC_COARSE, d.clock_id);
++
++    EXPECT_GT(d, fake_time[CLOCK_MONOTONIC_COARSE]);
++    EXPECT_LE(d, fake_time[CLOCK_MONOTONIC_COARSE]+one_frame);
++
++    auto server_phase = last_server_frame % one_frame;
++    EXPECT_NE(server_phase, c % one_frame);  // wasn't in phase before
++    EXPECT_EQ(server_phase, d % one_frame);  // but is in phase now
++
++    // Not only did we come in phase but we're targeting the soonest frame
++    EXPECT_EQ(last_server_frame+one_frame, d);
++}
++
++TEST_F(FrameClockTest, does_not_resync_on_most_frames)
++{
++    int callbacks = 0;
++
++    FrameClock clock(with_fake_time);
++    clock.set_period(one_frame);
++    clock.set_resync_callback([&callbacks]
++    {
++        ++callbacks;
++        return PosixTimestamp();
++    });
++
++    PosixTimestamp v;
++    v = clock.next_frame_after(v);
++    EXPECT_EQ(1, callbacks);  // sync with server happens on first frame
++
++    fake_sleep_until(v);
++
++    v = clock.next_frame_after(v);
++    EXPECT_EQ(1, callbacks);
++
++    fake_sleep_until(v);
++
++    v = clock.next_frame_after(v);
++    EXPECT_EQ(1, callbacks);
++
++    fake_sleep_until(v);
++
++    v = clock.next_frame_after(v);
++    EXPECT_EQ(1, callbacks);
++
++    fake_sleep_for(one_frame * 10);
++
++    clock.next_frame_after(v);
++    EXPECT_EQ(2, callbacks);   // resync because we went idle too long
++}
++
++TEST_F(FrameClockTest, one_frame_skipped_only_after_2_frames_take_3_periods)
++{
++    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 * 3 / 2);  // Render time: 1.5 frames
++
++    auto c = clock.next_frame_after(b);
++    EXPECT_EQ(one_frame, c - b);        // No frame skipped
++
++    fake_sleep_until(c);
++    fake_sleep_for(one_frame * 8 / 5);  // Render time: 1.6 frames
++
++    auto d = clock.next_frame_after(c);
++    EXPECT_EQ(2*one_frame, d - c);      // One frame skipped
++
++    fake_sleep_until(d);
++    fake_sleep_for(one_frame/4);        // Short render time, immediate recovery
++
++    auto e = clock.next_frame_after(d);
++    EXPECT_EQ(one_frame, e - d);        // No frame skipped. We have recovered.
++}
++
++TEST_F(FrameClockTest, nesting_adds_zero_lag)
++{
++    FrameClock inner(with_fake_time);
++    FrameClock outer(with_fake_time);
++    inner.set_period(one_frame);
++    outer.set_period(one_frame);
++
++    PosixTimestamp in0, out0;
++    auto in1 = inner.next_frame_after(in0);
++    fake_sleep_for(1234567ns);  // scheduling and IPC delay
++    auto out1 = outer.next_frame_after(out0);
++    EXPECT_EQ(in1, out1);
++
++    fake_sleep_until(in1);
++    fake_sleep_until(out1);
++
++    auto in2 = inner.next_frame_after(in1);
++    fake_sleep_for(8765432ns);  // scheduling and IPC delay
++    auto out2 = outer.next_frame_after(out1);
++    EXPECT_EQ(in2, out2);
++    EXPECT_EQ(one_frame, in2 - in1);
++    EXPECT_EQ(one_frame, out2 - out1);
++}
 === modified file 'tests/unit-tests/platforms/android/server/test_hwc_device.cpp'
 --- tests/unit-tests/platforms/android/server/test_hwc_device.cpp	2016-12-09 02:54:31 +0000
 +++ tests/unit-tests/platforms/android/server/test_hwc_device.cpp	2017-01-17 06:23:26 +0000
@@ -601,6 +601,7 @@
  //LP: #1369763. We could get swapinterval 0 to work with overlays, but we'd need
  //the vsync signal from hwc to reach the client, so the client can rate-limit its submissions.
++// Update: We have client-side vsync now so it should be possible to do this...
  TEST_F(HwcDevice, rejects_list_containing_interval_0)
+ {
      mga::HwcDevice device(mock_device);