Merge lp:~vanvugt/mir/fix-StaleFrames-test-race into lp:mir

[Daniel van Vugt]

Fix a race in the StaleFrames integration tests and expand the tests
to cover both swap intervals (since zero exploits the problem most
easily).

[Mir CI Bot (mir-ci-bot)]

FAILED: Continuous integration, rev:3913
https://mir-jenkins.ubuntu.com/job/mir-ci/2644/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/3434/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/3501
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/3493
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/3493
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/3493
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/3463
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/3463/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/3463/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/3463
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/3463/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/3463/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/3463/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/3463/console

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

review: Needs Fixing (continuous-integration)

[Mir CI Bot (mir-ci-bot)]

PASSED: Continuous integration, rev:3915
https://mir-jenkins.ubuntu.com/job/mir-ci/2670/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/3468
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/3535
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/3525
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/3525
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/3525
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/3495
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/3495/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/3495
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/3495/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/3495
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/3495/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/3495
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/3495/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/3495
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/3495/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/3495
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/3495/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Cemil Azizoglu (cemil-azizoglu)]

The logic seems good, but now that we have renamed Surface as Window, we should make a habit of using the new name.

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

"Surface" refers to the server type mir::scene::Surface. There is no Window type in the server, is there?

[Cemil Azizoglu (cemil-azizoglu)]

> "Surface" refers to the server type mir::scene::Surface. There is no Window
> type in the server, is there?

We have not renamed everything, correct. But new functions we create should use the new name.

review: Approve

[Mir CI Bot (mir-ci-bot)]

review: Approve (continuous-integration)

=== modified file 'tests/integration-tests/test_stale_frames.cpp'
--- tests/integration-tests/test_stale_frames.cpp	2017-01-11 19:38:28 +0000
+++ tests/integration-tests/test_stale_frames.cpp	2017-01-12 03:45:34 +0000
@@ -20,6 +20,9 @@
 #include "mir_toolkit/debug/surface.h"
 
 #include "mir/compositor/compositor.h"
+#include "mir/scene/surface.h"
+#include "mir/scene/surface_factory.h"
+#include "mir/scene/null_surface_observer.h"
 #include "mir/renderer/renderer_factory.h"
 #include "mir/graphics/renderable.h"
 #include "mir/graphics/buffer.h"
@@ -36,6 +39,7 @@
 #include <mutex>
 #include <condition_variable>
 
+using namespace std::chrono_literals;
 namespace mtf = mir_test_framework;
 namespace mtd = mir::test::doubles;
 namespace mc = mir::compositor;
@@ -114,8 +118,66 @@
     StubRenderer* renderer_ = nullptr;
 };
 
+class PostObserver : public mir::scene::NullSurfaceObserver
+{
+public:
+    PostObserver(std::function<void(int)> cb) : cb{cb}
+    {
+    }
+
+    void frame_posted(int count, geom::Size const&) override
+    {
+        cb(count);
+    }
+
+private:
+    std::function<void(int)> const cb;
+};
+
+class PublicSurfaceFactory : public mir::scene::SurfaceFactory
+{
+public:
+    using Surface = mir::scene::Surface;
+
+    PublicSurfaceFactory(std::shared_ptr<mir::scene::SurfaceFactory> const& real) : real_surface_factory{real} {}
+
+    std::shared_ptr<Surface> create_surface(
+        std::list<mir::scene::StreamInfo> const& streams,
+        mir::scene::SurfaceCreationParameters const& params) override
+    {
+        latest_surface = real_surface_factory->create_surface(streams, params);
+        return latest_surface;
+    }
+
+    std::shared_ptr<Surface> const latest() const
+    {
+        return latest_surface;
+    }
+
+    void forget_latest()
+    {
+        latest_surface.reset();
+    }
+
+private:
+    std::shared_ptr<mir::scene::SurfaceFactory> const real_surface_factory;
+    std::shared_ptr<Surface> latest_surface;
+};
+
 struct StubServerConfig : mtf::StubbedServerConfiguration
 {
+    std::shared_ptr<PublicSurfaceFactory> the_public_surface_factory()
+    {
+        return public_surface_factory(
+                   [this]{ return std::make_shared<PublicSurfaceFactory>(
+                           mtf::StubbedServerConfiguration::the_surface_factory()); });
+    }
+
+    std::shared_ptr<mir::scene::SurfaceFactory> the_surface_factory() override
+    {
+        return the_public_surface_factory();
+    }
+
     std::shared_ptr<StubRendererFactory> the_stub_renderer_factory()
     {
         return stub_renderer_factory(
@@ -128,17 +190,30 @@
     }
 
     mir::CachedPtr<StubRendererFactory> stub_renderer_factory;
+    mir::CachedPtr<PublicSurfaceFactory> public_surface_factory;
 };
 
 using BasicFixture = mtf::BasicClientServerFixture<StubServerConfig>;
 
-struct StaleFrames : BasicFixture
+struct StaleFrames : BasicFixture,
+                     ::testing::WithParamInterface<int>
 {
+    StaleFrames()
+        : post_observer(std::make_shared<PostObserver>(
+            [this](int n){frame_posted(n);}))
+    {
+    }
+
     void SetUp()
     {
         BasicFixture::SetUp();
 
         client_create_surface();
+        auto pub = server_configuration.the_public_surface_factory();
+        auto surface = pub->latest();
+        ASSERT_TRUE(!!surface);
+        surface->add_observer(post_observer);
+        pub->forget_latest();
     }
 
     void TearDown()
@@ -170,12 +245,45 @@
         server_configuration.the_compositor()->start();
     }
 
+    /*
+     * NOTE that we wait for surface buffer posts as opposed to display posts.
+     * The difference is that surface buffer posts will precisely match the
+     * number of client swaps for any swap interval, but display posts may be
+     * fewer than the number of swaps if the client was quick and using
+     * interval zero.
+     */
+    void frame_posted(int count)
+    {
+        std::unique_lock<std::mutex> lock(mutex);
+        posts += count;
+        posted.notify_all();
+    }
+
+    bool wait_for_posts(int count, std::chrono::seconds timeout)
+    {
+        std::unique_lock<std::mutex> lock(mutex);
+        auto const deadline = std::chrono::steady_clock::now() + timeout;
+        while (posts < count)
+        {
+            if (posted.wait_until(lock, deadline) == std::cv_status::timeout)
+                return false;
+        }
+        posts -= count;
+        return true;
+    }
+
     MirWindow* window;
+
+private:
+    std::shared_ptr<PostObserver> post_observer;
+    std::mutex mutex;
+    std::condition_variable posted;
+    int posts = 0;
 };
 
 }
 
-TEST_F(StaleFrames, are_dropped_when_restarting_compositor)
+TEST_P(StaleFrames, are_dropped_when_restarting_compositor)
 {
     using namespace testing;
 
@@ -186,6 +294,7 @@
     stale_buffers.emplace(mir_debug_window_current_buffer_id(window));
 
     auto bs = mir_window_get_buffer_stream(window);
+    mir_buffer_stream_set_swapinterval(bs, GetParam());
     mir_buffer_stream_swap_buffers_sync(bs);
 
     stale_buffers.emplace(mir_debug_window_current_buffer_id(window));
@@ -196,6 +305,7 @@
     auto const fresh_buffer = mg::BufferID{mir_debug_window_current_buffer_id(window)};
     mir_buffer_stream_swap_buffers_sync(bs);
 
+    ASSERT_TRUE(wait_for_posts(3, 60s));
     start_compositor();
 
     // Note first stale buffer and fresh_buffer may be equal when defaulting to double buffers
@@ -206,22 +316,26 @@
     EXPECT_THAT(stale_buffers, Not(Contains(new_buffers[0])));
 }
 
-TEST_F(StaleFrames, only_fresh_frames_are_used_after_restarting_compositor)
+TEST_P(StaleFrames, only_fresh_frames_are_used_after_restarting_compositor)
 {
     using namespace testing;
 
     stop_compositor();
 
     auto bs = mir_window_get_buffer_stream(window);
+    mir_buffer_stream_set_swapinterval(bs, GetParam());
     mir_buffer_stream_swap_buffers_sync(bs);
     mir_buffer_stream_swap_buffers_sync(bs);
 
     auto const fresh_buffer = mg::BufferID{mir_debug_window_current_buffer_id(window)};
     mir_buffer_stream_swap_buffers_sync(bs);
 
+    ASSERT_TRUE(wait_for_posts(3, 60s));
     start_compositor();
 
     auto const new_buffers = wait_for_new_rendered_buffers();
     ASSERT_THAT(new_buffers.size(), Eq(1u));
     EXPECT_THAT(new_buffers[0], Eq(fresh_buffer));
 }
+
+INSTANTIATE_TEST_CASE_P(PerSwapInterval, StaleFrames, ::testing::Values(0,1));