Merge lp:~afrantzis/mir/fix-out-of-order-buffers-1216472 into lp:mir

[Alexandros Frantzis]

Properly track buffers per-surface (not per session). Failure to do so was
causing premature buffer release and re-use in cases with multiple surfaces
per client/session. In XMir this was seen as out of order, glitchy and slow
frames when using multiple monitors (LP: #1216472)

frontend: Properly track buffers per-surface (not per session)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1091
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~afrantzis/mir/fix-out-of-order-buffers-1216472/+merge/187841/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/40/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2153
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2038
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/40
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/40/artifact/work/output/*zip*/output.zip

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

review: Needs Fixing (continuous-integration)

[Kevin DuBois (kdub)]

change looks good to me.

I don't understand why that would cause the lag, so I couldn't reason through whether this would affect internal clients as well... will mark 'needs info'

review: Needs Information

[Daniel van Vugt (vanvugt)]

Looks reasonable.

I haven't re-tested packaging and multi-monitors with XMir, but it worked brilliantly when I did so last week.

review: Approve

[Daniel van Vugt (vanvugt)]

Re-tested with XMir. Verified bug 1216472 is fixed!

review: Approve

[Alan Griffiths (alan-griffiths)]

Seems Ok

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1091
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/50/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2164
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2049
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/50
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/50/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

=== modified file 'include/server/mir/frontend/session_mediator.h'
--- include/server/mir/frontend/session_mediator.h	2013-08-28 03:41:48 +0000
+++ include/server/mir/frontend/session_mediator.h	2013-09-26 16:31:43 +0000
@@ -21,8 +21,9 @@
 #define MIR_FRONTEND_SESSION_MEDIATOR_H_
 
 #include "mir_protobuf.pb.h"
+#include "mir/frontend/surface_id.h"
 
-#include <map>
+#include <unordered_map>
 #include <memory>
 #include <mutex>
 
@@ -108,6 +109,10 @@
                         google::protobuf::Closure* done) override;
 
 private:
+    void pack_protobuf_buffer(protobuf::Buffer& protobuf_buffer,
+                              std::shared_ptr<graphics::Buffer> const& graphics_buffer,
+                              bool need_full_ipc);
+
     std::shared_ptr<Shell> const shell;
     std::shared_ptr<graphics::Platform> const graphics_platform;
 
@@ -119,7 +124,7 @@
     std::shared_ptr<EventSink> const event_sink;
     std::shared_ptr<ResourceCache> const resource_cache;
 
-    std::shared_ptr<graphics::Buffer> client_buffer_resource;
+    std::unordered_map<SurfaceId,std::shared_ptr<graphics::Buffer>> client_buffer_resource;
 
     std::mutex session_mutex;
     std::weak_ptr<Session> weak_session;

=== modified file 'src/server/frontend/session_mediator.cpp'
--- src/server/frontend/session_mediator.cpp	2013-09-24 07:02:12 +0000
+++ src/server/frontend/session_mediator.cpp	2013-09-26 16:31:43 +0000
@@ -126,15 +126,15 @@
     
     report->session_create_surface_called(session->name());
 
-    auto const id = session->create_surface(msh::SurfaceCreationParameters()
+    auto const surf_id = session->create_surface(msh::SurfaceCreationParameters()
         .of_name(request->surface_name())
         .of_size(request->width(), request->height())
         .of_buffer_usage(static_cast<graphics::BufferUsage>(request->buffer_usage()))
         .of_pixel_format(static_cast<geometry::PixelFormat>(request->pixel_format()))
         .with_output_id(graphics::DisplayConfigurationOutputId(request->output_id())));
     {
-        auto surface = session->get_surface(id);
-        response->mutable_id()->set_value(id.as_value());
+        auto surface = session->get_surface(surf_id);
+        response->mutable_id()->set_value(surf_id.as_value());
         response->set_width(surface->size().width.as_uint32_t());
         response->set_height(surface->size().height.as_uint32_t());
         response->set_pixel_format((int)surface->pixel_format());
@@ -144,17 +144,11 @@
             response->add_fd(surface->client_input_fd());
 
         bool need_full_ipc;
-        client_buffer_resource = surface->advance_client_buffer(need_full_ipc);
-        auto const& id = client_buffer_resource->id();
+        auto client_buffer = surface->advance_client_buffer(need_full_ipc);
+        client_buffer_resource[surf_id] = client_buffer;
 
         auto buffer = response->mutable_buffer();
-        buffer->set_buffer_id(id.as_uint32_t());
-
-        if (need_full_ipc)
-        {
-            auto packer = std::make_shared<mfd::ProtobufBufferPacker>(buffer);
-            graphics_platform->fill_ipc_package(packer, client_buffer_resource);
-        }
+        pack_protobuf_buffer(*buffer, client_buffer, need_full_ipc);
     }
 
     // TODO: NOTE: We use the ordering here to ensure the shell acts on the surface after the surface ID is sent over the wire.
@@ -171,7 +165,10 @@
     ::mir::protobuf::Buffer* response,
     ::google::protobuf::Closure* done)
 {
-    bool needs_full_ipc;
+    bool need_full_ipc;
+    SurfaceId const surf_id{request->value()};
+    std::shared_ptr<graphics::Buffer> client_buffer;
+
     {
         std::unique_lock<std::mutex> lock(session_mutex);
         
@@ -188,20 +185,15 @@
         // outputs.
         display_changer->ensure_display_powered(session);
 
-        auto surface = session->get_surface(SurfaceId(request->value()));
-
-        client_buffer_resource.reset();
-        client_buffer_resource = surface->advance_client_buffer(needs_full_ipc);
-    }
-
-    auto const& id = client_buffer_resource->id();
-    response->set_buffer_id(id.as_uint32_t());
-
-    if (needs_full_ipc)
-    {
-        auto packer = std::make_shared<mfd::ProtobufBufferPacker>(response);
-        graphics_platform->fill_ipc_package(packer, client_buffer_resource);
-    }
+        auto surface = session->get_surface(surf_id);
+
+        client_buffer_resource[surf_id].reset();
+        client_buffer = surface->advance_client_buffer(need_full_ipc);
+        client_buffer_resource[surf_id] = client_buffer;
+    }
+
+    pack_protobuf_buffer(*response, client_buffer, need_full_ipc);
+
     done->Run();
 }
 
@@ -316,3 +308,17 @@
     }
     done->Run();
 }
+
+void mf::SessionMediator::pack_protobuf_buffer(
+    protobuf::Buffer& protobuf_buffer,
+    std::shared_ptr<graphics::Buffer> const& graphics_buffer,
+    bool need_full_ipc)
+{
+    protobuf_buffer.set_buffer_id(graphics_buffer->id().as_uint32_t());
+
+    if (need_full_ipc)
+    {
+        auto packer = std::make_shared<mfd::ProtobufBufferPacker>(&protobuf_buffer);
+        graphics_platform->fill_ipc_package(packer, graphics_buffer);
+    }
+}

=== modified file 'tests/unit-tests/frontend/test_session_mediator.cpp'
--- tests/unit-tests/frontend/test_session_mediator.cpp	2013-09-12 21:36:55 +0000
+++ tests/unit-tests/frontend/test_session_mediator.cpp	2013-09-26 16:31:43 +0000
@@ -545,6 +545,44 @@
     mediator.disconnect(nullptr, nullptr, nullptr, null_callback.get());
 }
 
+TEST_F(SessionMediatorTest, buffer_resource_for_surface_held_over_operations_on_other_surfaces)
+{
+    using namespace testing;
+
+    auto stub_buffer1 = std::make_shared<mtd::StubBuffer>();
+
+    mp::ConnectParameters connect_parameters;
+    mp::Connection connection;
+
+    mediator.connect(nullptr, &connect_parameters, &connection, null_callback.get());
+    mp::SurfaceParameters surface_request;
+    mp::Surface surface_response;
+
+    /*
+     * Note that the surface created by the first create_surface() call is
+     * the pre-created stubbed_session->mock_surface. Further create_surface()
+     * invocations create new surfaces in stubbed_session->mock_surfaces[].
+     */
+    EXPECT_CALL(*stubbed_session->mock_surface, advance_client_buffer())
+        .WillOnce(Return(stub_buffer1));
+
+    mediator.create_surface(nullptr, &surface_request, &surface_response, null_callback.get());
+    auto refcount = stub_buffer1.use_count();
+
+    /* Creating a new surface should not affect other surfaces' buffers */
+    mediator.create_surface(nullptr, &surface_request, &surface_response, null_callback.get());
+    EXPECT_EQ(refcount, stub_buffer1.use_count());
+
+    mp::SurfaceId buffer_request{surface_response.id()};
+    mp::Buffer buffer_response;
+
+    /* Getting the next buffer of a surface should not affect other surfaces' buffers */
+    mediator.next_buffer(nullptr, &buffer_request, &buffer_response, null_callback.get());
+    EXPECT_EQ(refcount, stub_buffer1.use_count());
+
+    mediator.disconnect(nullptr, nullptr, nullptr, null_callback.get());
+}
+
 TEST_F(SessionMediatorTest, display_config_request)
 {
     using namespace testing;