Merge lp:~afrantzis/mir/drop-stale-frames into lp:mir

[Alexandros Frantzis]

server: Drop stale frames when a surface becomes exposed

When a surface changes from occluded to exposed, at best there is no
point in showing older, stale frames in quick succession, at worst the
stale frames result in visual glitches. We only care about the latest
surface buffer.

server: Drop stale frames when a surface becomes exposed

When a surface changes from occluded to exposed, at best there is no
point in showing older, stale frames in quick succession, at worst the
stale frames result in visual glitches. We only care about the latest
surface buffer.

This fixes one aspect of https://bugs.launchpad.net/unity-system-compositor/+bug/1340510, the stale frame issue, the other aspect being the greeter rendering delay.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1790
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2220/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/1055
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/1061
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1047
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/742
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/742/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/743
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/743/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/3619
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/3619/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2164
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/10300

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/2220/rebuild

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

Nice

review: Approve

[Robert Carr (robertcarr)]

LGTM

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1791
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2235/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/1086
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/1092
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1075
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/757
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/757/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/758
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/758/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/10
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/10/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2193
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/10418

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/2235/rebuild

review: Approve (continuous-integration)

[Kevin DuBois (kdub)]

lgtm too

review: Approve

=== modified file 'include/server/mir/compositor/buffer_stream.h'
--- include/server/mir/compositor/buffer_stream.h	2014-05-22 10:00:05 +0000
+++ include/server/mir/compositor/buffer_stream.h	2014-07-25 10:04:16 +0000
@@ -53,6 +53,7 @@
     virtual void allow_framedropping(bool) = 0;
     virtual void force_requests_to_complete() = 0;
     virtual int buffers_ready_for_compositor() const = 0;
+    virtual void drop_old_buffers() = 0;
 };
 
 }

=== modified file 'include/test/mir_test_doubles/mock_buffer_bundle.h'
--- include/test/mir_test_doubles/mock_buffer_bundle.h	2014-05-21 05:30:50 +0000
+++ include/test/mir_test_doubles/mock_buffer_bundle.h	2014-07-25 10:04:16 +0000
@@ -48,6 +48,7 @@
     MOCK_METHOD0(force_client_abort, void());
     MOCK_METHOD0(force_requests_to_complete, void());
     MOCK_METHOD1(resize, void(const geometry::Size &));
+    MOCK_METHOD0(drop_old_buffers, void());
     int buffers_ready_for_compositor() const override { return 1; }
     int buffers_free_for_client() const override { return 1; }
 };

=== modified file 'include/test/mir_test_doubles/mock_buffer_stream.h'
--- include/test/mir_test_doubles/mock_buffer_stream.h	2014-05-22 10:00:05 +0000
+++ include/test/mir_test_doubles/mock_buffer_stream.h	2014-07-25 10:04:16 +0000
@@ -57,6 +57,7 @@
     MOCK_METHOD1(allow_framedropping, void(bool));
     MOCK_METHOD0(force_requests_to_complete, void());
     MOCK_CONST_METHOD0(buffers_ready_for_compositor, int());
+    MOCK_METHOD0(drop_old_buffers, void());
 };
 }
 }

=== modified file 'include/test/mir_test_doubles/stub_buffer_stream.h'
--- include/test/mir_test_doubles/stub_buffer_stream.h	2014-05-22 10:00:05 +0000
+++ include/test/mir_test_doubles/stub_buffer_stream.h	2014-07-25 10:04:16 +0000
@@ -81,6 +81,8 @@
 
     int buffers_ready_for_compositor() const override { return 1; }
 
+    void drop_old_buffers() override {}
+
     StubBuffer stub_client_buffer;
     std::shared_ptr<graphics::Buffer> stub_compositor_buffer;
 };

=== modified file 'server-ABI-sha1sums'
--- server-ABI-sha1sums	2014-07-24 11:44:53 +0000
+++ server-ABI-sha1sums	2014-07-25 10:04:16 +0000
@@ -9,7 +9,7 @@
 ba79552edce545fafc9a4f401411c44d0cb3b2cf  include/server/mir/graphics/gl_extensions_base.h
 7ef3e99901168cda296d74d05a979f47bf9c3ff1  include/server/mir/server_action_queue.h
 a3e8658107c0c13d41eaf2ca6a1be71d1dc59df6  include/server/mir/compositor/compositor_report.h
-e439cc4a7fb8e975bed0771756e7949aad968c14  include/server/mir/compositor/buffer_stream.h
+a4c8f0b0c30784ea6930a8ee6651f1d6efa47231  include/server/mir/compositor/buffer_stream.h
 e787879afa46d9c8257ff1f4ae8c7900d4b5f1a4  include/server/mir/compositor/frame_dropping_policy.h
 e0860c43bf5196a792ffd7216826925e16275d07  include/server/mir/compositor/renderer_factory.h
 a9f284ba4b05d58fd3eeb628d1f56fe4ac188526  include/server/mir/compositor/compositor_id.h

=== modified file 'src/server/compositor/buffer_bundle.h'
--- src/server/compositor/buffer_bundle.h	2014-05-21 08:38:47 +0000
+++ src/server/compositor/buffer_bundle.h	2014-07-25 10:04:16 +0000
@@ -67,6 +67,7 @@
      * nbuffers-1 as it might be less.
      */
     virtual int buffers_free_for_client() const = 0;
+    virtual void drop_old_buffers() = 0;
 
 protected:
     BufferBundle() = default;

=== modified file 'src/server/compositor/buffer_queue.cpp'
--- src/server/compositor/buffer_queue.cpp	2014-07-21 03:35:31 +0000
+++ src/server/compositor/buffer_queue.cpp	2014-07-25 10:04:16 +0000
@@ -99,6 +99,7 @@
     : nbuffers{nbuffers},
       frame_dropping_enabled{false},
       the_properties{props},
+      force_new_compositor_buffer{false},
       gralloc{gralloc}
 {
     if (nbuffers < 1)
@@ -240,7 +241,16 @@
         use_current_buffer = true;
         current_buffer_users.push_back(user_id);
     }
-    use_current_buffer |= ready_to_composite_queue.empty();
+
+    if (ready_to_composite_queue.empty())
+    {
+        use_current_buffer = true;
+    }
+    else if (force_new_compositor_buffer)
+    {
+        use_current_buffer = false;
+        force_new_compositor_buffer = false;
+    }
 
     mg::Buffer* buffer_to_release = nullptr;
     if (!use_current_buffer)
@@ -473,3 +483,22 @@
     }
     give_buffer_to_client(buffer_to_give, std::move(lock));
 }
+
+void mc::BufferQueue::drop_old_buffers()
+{
+    std::vector<mg::Buffer*> to_release;
+
+    {
+        std::lock_guard<decltype(guard)> lock{guard};
+        force_new_compositor_buffer = true;
+
+        while (ready_to_composite_queue.size() > 1)
+            to_release.push_back(pop(ready_to_composite_queue));
+    }
+
+    for (auto buffer : to_release)
+    {
+       std::unique_lock<decltype(guard)> lock{guard};
+       release(buffer, std::move(lock));
+    }
+}

=== modified file 'src/server/compositor/buffer_queue.h'
--- src/server/compositor/buffer_queue.h	2014-07-17 10:28:51 +0000
+++ src/server/compositor/buffer_queue.h	2014-07-25 10:04:16 +0000
@@ -62,6 +62,7 @@
     int buffers_free_for_client() const override;
     bool framedropping_allowed() const;
     bool is_a_current_buffer_user(void const* user_id) const;
+    void drop_old_buffers() override;
 
 private:
     void give_buffer_to_client(graphics::Buffer* buffer,
@@ -87,6 +88,7 @@
     int nbuffers;
     bool frame_dropping_enabled;
     graphics::BufferProperties the_properties;
+    bool force_new_compositor_buffer;
 
     std::condition_variable snapshot_released;
     std::shared_ptr<graphics::GraphicBufferAllocator> gralloc;

=== modified file 'src/server/compositor/buffer_stream_surfaces.cpp'
--- src/server/compositor/buffer_stream_surfaces.cpp	2014-05-22 10:00:05 +0000
+++ src/server/compositor/buffer_stream_surfaces.cpp	2014-07-25 10:04:16 +0000
@@ -89,3 +89,8 @@
 {
     return buffer_bundle->buffers_ready_for_compositor();
 }
+
+void mc::BufferStreamSurfaces::drop_old_buffers()
+{
+    buffer_bundle->drop_old_buffers();
+}

=== modified file 'src/server/compositor/buffer_stream_surfaces.h'
--- src/server/compositor/buffer_stream_surfaces.h	2014-05-22 10:00:05 +0000
+++ src/server/compositor/buffer_stream_surfaces.h	2014-07-25 10:04:16 +0000
@@ -52,6 +52,7 @@
     void allow_framedropping(bool) override;
     void force_requests_to_complete() override;
     int buffers_ready_for_compositor() const override;
+    void drop_old_buffers() override;
 
 protected:
     BufferStreamSurfaces(const BufferStreamSurfaces&) = delete;

=== modified file 'src/server/scene/basic_surface.cpp'
--- src/server/scene/basic_surface.cpp	2014-07-21 03:35:31 +0000
+++ src/server/scene/basic_surface.cpp	2014-07-25 10:04:16 +0000
@@ -583,6 +583,8 @@
     {
         attrib_values[mir_surface_attrib_visibility] = new_visibility;
         lg.unlock();
+        if (new_visibility == mir_surface_visibility_exposed)
+            surface_buffer_stream->drop_old_buffers();
         observers.attrib_changed(mir_surface_attrib_visibility, attrib_values[mir_surface_attrib_visibility]);
     }
 

=== modified file 'tests/acceptance-tests/CMakeLists.txt'
--- tests/acceptance-tests/CMakeLists.txt	2014-07-22 03:26:35 +0000
+++ tests/acceptance-tests/CMakeLists.txt	2014-07-25 10:04:16 +0000
@@ -40,6 +40,7 @@
   test_client_surface_visibility.cpp
   test_client_with_custom_display_config_deadlock.cpp
   test_macros.cpp
+  test_stale_frames.cpp
   ${GENERATED_PROTOBUF_SRCS}
   ${GENERATED_PROTOBUF_HDRS}
 )

=== added file 'tests/acceptance-tests/test_stale_frames.cpp'
--- tests/acceptance-tests/test_stale_frames.cpp	1970-01-01 00:00:00 +0000
+++ tests/acceptance-tests/test_stale_frames.cpp	2014-07-25 10:04:16 +0000
@@ -0,0 +1,206 @@
+/*
+ * Copyright © 2014 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: Alexandros Frantzis <[e-mail]>
+ */
+
+#include "mir_toolkit/mir_client_library.h"
+#include "mir_toolkit/mir_client_library_debug.h"
+
+#include "mir/compositor/compositor.h"
+#include "mir/compositor/renderer_factory.h"
+#include "mir/graphics/renderable.h"
+#include "mir/graphics/buffer.h"
+#include "mir/graphics/buffer_id.h"
+
+#include "mir_test_framework/using_stub_client_platform.h"
+#include "mir_test_framework/stubbed_server_configuration.h"
+#include "mir_test_framework/basic_client_server_fixture.h"
+#include "mir_test_doubles/stub_renderer.h"
+
+#include <gtest/gtest.h>
+#include <gmock/gmock.h>
+
+#include <mutex>
+#include <condition_variable>
+
+namespace mtf = mir_test_framework;
+namespace mtd = mir::test::doubles;
+namespace mc = mir::compositor;
+namespace mg = mir::graphics;
+namespace geom = mir::geometry;
+
+namespace
+{
+
+struct StubRenderer : mtd::StubRenderer
+{
+    void render(mg::RenderableList const& renderables) const override
+    {
+        std::lock_guard<std::mutex> lock{mutex};
+        for (auto const& r : renderables)
+            rendered_buffers_.push_back(r->buffer()->id());
+
+        if (renderables.size() > 0)
+            new_rendered_buffer_cv.notify_all();
+    }
+
+    std::vector<mg::BufferID> rendered_buffers()
+    {
+        std::lock_guard<std::mutex> lock{mutex};
+        return rendered_buffers_;
+    }
+
+    std::vector<mg::BufferID> wait_for_new_rendered_buffers()
+    {
+        std::unique_lock<std::mutex> lock{mutex};
+
+        new_rendered_buffer_cv.wait_for(
+            lock, std::chrono::seconds{2},
+            [this] { return rendered_buffers_.size() != 0; });
+
+        auto const rendered = std::move(rendered_buffers_);
+        return rendered;
+    }
+
+    mutable std::mutex mutex;
+    mutable std::condition_variable new_rendered_buffer_cv;
+    mutable std::vector<mg::BufferID> rendered_buffers_;
+};
+
+class StubRendererFactory : public mc::RendererFactory
+{
+public:
+    std::unique_ptr<mc::Renderer> create_renderer_for(
+        geom::Rectangle const&, mc::DestinationAlpha) override
+    {
+        std::lock_guard<std::mutex> lock{mutex};
+        renderer_ = new StubRenderer();
+        renderer_created_cv.notify_all();
+        return std::unique_ptr<mc::Renderer>{renderer_};
+    }
+
+    StubRenderer* renderer()
+    {
+        std::unique_lock<std::mutex> lock{mutex};
+
+        renderer_created_cv.wait_for(
+            lock, std::chrono::seconds{2},
+            [this] { return renderer_ != nullptr; });
+
+        return renderer_;
+    }
+
+    void clear_renderer()
+    {
+        std::lock_guard<std::mutex> lock{mutex};
+        renderer_ = nullptr;
+    }
+
+    std::mutex mutex;
+    std::condition_variable renderer_created_cv;
+    StubRenderer* renderer_ = nullptr;
+};
+
+struct StubServerConfig : mtf::StubbedServerConfiguration
+{
+    std::shared_ptr<StubRendererFactory> the_stub_renderer_factory()
+    {
+        return stub_renderer_factory(
+            [] { return std::make_shared<StubRendererFactory>(); });
+    }
+
+    std::shared_ptr<mc::RendererFactory> the_renderer_factory() override
+    {
+        return the_stub_renderer_factory();
+    }
+
+    mir::CachedPtr<StubRendererFactory> stub_renderer_factory;
+};
+
+using BasicFixture = mtf::BasicClientServerFixture<StubServerConfig>;
+
+struct StaleFrames : BasicFixture
+{
+    void SetUp()
+    {
+        BasicFixture::SetUp();
+
+        client_create_surface();
+    }
+
+    void TearDown()
+    {
+        mir_surface_release_sync(surface);
+
+        BasicFixture::TearDown();
+    }
+
+    void client_create_surface()
+    {
+        MirSurfaceParameters const request_params =
+        {
+            __PRETTY_FUNCTION__,
+            640, 480,
+            mir_pixel_format_abgr_8888,
+            mir_buffer_usage_hardware,
+            mir_display_output_id_invalid
+        };
+
+        surface = mir_connection_create_surface_sync(connection, &request_params);
+        ASSERT_TRUE(mir_surface_is_valid(surface));
+    }
+
+    std::vector<mg::BufferID> wait_for_new_rendered_buffers()
+    {
+        return server_configuration.the_stub_renderer_factory()->renderer()->wait_for_new_rendered_buffers();
+    }
+
+    void stop_compositor()
+    {
+        server_configuration.the_compositor()->stop();
+        server_configuration.the_stub_renderer_factory()->clear_renderer();
+    }
+
+    void start_compositor()
+    {
+        server_configuration.the_compositor()->start();
+    }
+
+    MirSurface* surface;
+    mtf::UsingStubClientPlatform using_stub_client_platform;
+};
+
+}
+
+TEST_F(StaleFrames, are_dropped_when_restarting_compositor)
+{
+    using namespace testing;
+
+    stop_compositor();
+
+    auto const stale_buffer_id1 = mg::BufferID{mir_debug_surface_current_buffer_id(surface)};
+    mir_surface_swap_buffers_sync(surface);
+
+    auto const stale_buffer_id2 = mg::BufferID{mir_debug_surface_current_buffer_id(surface)};
+    mir_surface_swap_buffers_sync(surface);
+
+    mir_surface_swap_buffers_sync(surface);
+
+    start_compositor();
+
+    auto const new_buffers = wait_for_new_rendered_buffers();
+    EXPECT_THAT(new_buffers, Not(AnyOf(Contains(stale_buffer_id1), Contains(stale_buffer_id2))));
+}

=== modified file 'tests/integration-tests/test_swapinterval.cpp'
--- tests/integration-tests/test_swapinterval.cpp	2014-05-22 10:00:05 +0000
+++ tests/integration-tests/test_swapinterval.cpp	2014-07-25 10:04:16 +0000
@@ -28,6 +28,7 @@
 
 #include "mir_test_framework/display_server_test_fixture.h"
 #include "mir_test_doubles/stub_buffer.h"
+#include "mir_test_doubles/stub_buffer_stream.h"
 
 #include "mir_toolkit/mir_client_library.h"
 
@@ -48,7 +49,7 @@
 {
 char const* const mir_test_socket = mtf::test_socket_file().c_str();
 
-class CountingBufferStream : public mc::BufferStream
+class CountingBufferStream : public mtd::StubBufferStream
 {
 public:
     CountingBufferStream(int render_operations_fd)
@@ -56,16 +57,6 @@
     {
     }
 
-    void acquire_client_buffer(
-        std::function<void(mg::Buffer* buffer)> complete) override
-    {
-        complete(&stub_buffer);
-    }
-
-    void release_client_buffer(mg::Buffer*) override
-    {
-    }
-
     std::shared_ptr<mg::Buffer> lock_compositor_buffer(void const*) override
         { return std::make_shared<mtd::StubBuffer>(); }
 
@@ -75,18 +66,13 @@
     MirPixelFormat get_stream_pixel_format() override
         { return mir_pixel_format_abgr_8888; }
 
-    geom::Size stream_size() override { return geom::Size{}; }
-    void resize(geom::Size const&) override {}
-    void force_requests_to_complete() override {}
     void allow_framedropping(bool) override
     {
         while (write(render_operations_fd, "a", 1) != 1) continue;
     }
-    int buffers_ready_for_compositor() const override { return 1; }
 
 private:
     int render_operations_fd;
-    mtd::StubBuffer stub_buffer;
 };
 
 class StubStreamFactory : public ms::BufferStreamFactory

=== modified file 'tests/unit-tests/compositor/test_buffer_queue.cpp'
--- tests/unit-tests/compositor/test_buffer_queue.cpp	2014-07-11 05:14:40 +0000
+++ tests/unit-tests/compositor/test_buffer_queue.cpp	2014-07-25 10:04:16 +0000
@@ -1455,3 +1455,62 @@
         q.compositor_release(comp);
     }
 }
+
+TEST_F(BufferQueueTest, gives_compositor_a_valid_buffer_after_dropping_old_buffers_without_clients)
+{
+    int const nbuffers = 3;
+    mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
+
+    q.drop_old_buffers();
+
+    auto comp = q.compositor_acquire(this);
+    ASSERT_THAT(comp->id(), Ne(mg::BufferID{}));
+}
+
+TEST_F(BufferQueueTest, gives_compositor_the_newest_buffer_after_dropping_old_buffers)
+{
+    int const nbuffers = 3;
+    mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
+
+    auto handle1 = client_acquire_async(q);
+    ASSERT_THAT(handle1->has_acquired_buffer(), Eq(true));
+    handle1->release_buffer();
+
+    auto handle2 = client_acquire_async(q);
+    ASSERT_THAT(handle2->has_acquired_buffer(), Eq(true));
+    handle2->release_buffer();
+
+    q.drop_old_buffers();
+
+    auto comp = q.compositor_acquire(this);
+    ASSERT_THAT(comp->id(), Eq(handle2->id()));
+    q.compositor_release(comp);
+
+    comp = q.compositor_acquire(this);
+    ASSERT_THAT(comp->id(), Eq(handle2->id()));
+}
+
+TEST_F(BufferQueueTest, gives_new_compositor_the_newest_buffer_after_dropping_old_buffers)
+{
+    int const nbuffers = 3;
+    void const* const new_compositor_id{&nbuffers};
+
+    mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
+
+    auto handle1 = client_acquire_async(q);
+    ASSERT_THAT(handle1->has_acquired_buffer(), Eq(true));
+    handle1->release_buffer();
+
+    auto comp = q.compositor_acquire(this);
+    ASSERT_THAT(comp->id(), Eq(handle1->id()));
+    q.compositor_release(comp);
+
+    auto handle2 = client_acquire_async(q);
+    ASSERT_THAT(handle2->has_acquired_buffer(), Eq(true));
+    handle2->release_buffer();
+
+    q.drop_old_buffers();
+
+    auto comp2 = q.compositor_acquire(new_compositor_id);
+    ASSERT_THAT(comp2->id(), Eq(handle2->id()));
+}