Merge lp:~kdub/mir/protobuf-exchange-buffer into lp:mir

[Kevin DuBois]

frontend: add the stubs for the exchange_buffer rpc call

note: this isn't hooked up fully on the server side yet, nor is it used by the client.

frontend: add the stubs for the exchange_buffer rpc call

note: this isn't hooked up fully on the server side yet, nor is it used by the client.

This is just an addition to the protocol, so version number not bumped. I do have to break in a backwards incompatible way for the android platform only in the near future though, will bump then.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1888
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2561/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/1541
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/1547
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1523
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/1083
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/1083/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/460
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/460/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2600
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/12387

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

As per last discussion, I think continuing with any "exchange" semantics is a bad idea. Because it fails to solve our existing performance/parallelism issues (bug 1253868). And to solve those problems requires the elimination of exchange semantics.

review: Abstain

[Kevin DuBois (kdub)]

We discussed this a bit when I wanted to plan out this line of work on the mailing list (https://lists.ubuntu.com/archives/mir-devel/2014-July/000773.html).

Different buffer sending mechanisms are interesting, but in the name of not rewriting the entire system, I'm going with an incremental improvement on what we already have. My goal here is just to send a file descriptor back with the buffer that the client is returning. I don't want to go too far into buffer sending experiments when my goal is only to implement the fencing improvements that are useful to android.

[Alexandros Frantzis (afrantzis)]

Looks good.

review: Approve

[Kevin DuBois (kdub)]

I blocked the base branch to deal with the lifetimes of the buffers (and their associated ids). Will move to WIP until that is solved

[Kevin DuBois (kdub)]

prereq is in landing queue, moving back to the 'review-please' state

[Cemil Azizoglu (cemil-azizoglu)]

Since we're messing with symbols map. Have you tested in a sanitary environment (e.g. build mir using sbuild, then build U8 using local debs for mir that you just built), and then installing the packages for testing. This is as opposed to cross-compiling and then "adb push"ing the libraries.

In the past, we've had problems which popped up during release time (big PITA) because we did the latter, but not the former.

review: Needs Information

[Daniel van Vugt (vanvugt)]

(1) src/shared/symbols.map additions:
RAOF has pointed out to me today that function additions (which are not ABI breaking) need to be represented in new blocks in symbols.map. Because they do not form part of the current ABI, but are part of a future ABI. Confusingly, these new blocks can't be named according to the _next_ ABI either, for reasons RAOF just explained to me. So we need to come up with a naming scheme for new (temporary) symbols.map stanzas that exist until the next ABI break. On the next ABI break everything gets coalesced into a single block again.

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

(2) Text conflict in tests/unit-tests/frontend/test_session_mediator.cpp
1 conflicts encountered.

[Chris Halse Rogers (raof)]

I should probably write up a “proper care and feeding for your versioned DSOs” guide in docs/ :)

It doesn't *break* anything to add symbols to an existing stanza, but you lose one of the benefits of versioning - namely, that you guarantee at load time, rather than symbol resolution time, that your library contains the expected symbols.

[Daniel van Vugt (vanvugt)]

Oh, err, my concern is negated by doing a full ABI bump (which we are doing in Mir 0.8 anyway):
https://code.launchpad.net/~vanvugt/mir/mircommon2/+merge/233310

So once that lands, adding new entries into the main block of symbols.map is correct.

review: Abstain

[Kevin DuBois (kdub)]

So tested qtmir and usc (armhf build) with this change, and both could compile. platform api seems to be failing due to an unrelated header move.

[Cemil Azizoglu (cemil-azizoglu)]

> So tested qtmir and usc (armhf build) with this change, and both could
> compile. platform api seems to be failing due to an unrelated header move.

Is it a Mir header move? Should we file a separate bug?

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1894
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2666/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/1684
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/1690
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1663
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/1188
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/1188/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/600
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/600/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2718
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/12980

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

review: Approve (continuous-integration)

[Andreas Pokorny (andreas-pokorny)]

lgtm

review: Approve

[Daniel van Vugt (vanvugt)]

I'm still not a fan myself. But we seem to have enough support for top approval...

[Cemil Azizoglu (cemil-azizoglu)]

Let's not TA while there are still outstanding questions. Find issues during release is a much bigger pain.

[Kevin DuBois (kdub)]

So the failure seen was on trunk because platform api had the mir/client/private headers moved out of an accessible location. This branch does not aggrevate or cause the issue. I can report a bug about the issue.

[Cemil Azizoglu (cemil-azizoglu)]

> So the failure seen was on trunk because platform api had the
> mir/client/private headers moved out of an accessible location. This branch
> does not aggrevate or cause the issue. I can report a bug about the issue.

Ok that sounds good.

review: Approve

=== modified file 'src/common/protobuf/mir_protobuf.proto'
--- src/common/protobuf/mir_protobuf.proto	2014-09-08 04:12:17 +0000
+++ src/common/protobuf/mir_protobuf.proto	2014-09-09 20:45:42 +0000
@@ -25,6 +25,11 @@
   required int32 value = 1;
 };
 
+message BufferRequest {
+  optional SurfaceId id = 1;
+  optional Buffer buffer = 2;
+};
+
 message Buffer {
   optional int32 buffer_id = 1;
   repeated sint32 fd = 2;
@@ -221,4 +226,5 @@
 
   rpc start_prompt_session(PromptSessionParameters) returns (Void);
   rpc stop_prompt_session(Void) returns (Void);
+  rpc exchange_buffer(BufferRequest) returns (Buffer);
 }

=== modified file 'src/common/symbols.map'
--- src/common/symbols.map	2014-09-09 14:04:48 +0000
+++ src/common/symbols.map	2014-09-09 20:45:42 +0000
@@ -167,6 +167,23 @@
     mir::protobuf::Buffer::MergeFrom*;
     mir::protobuf::Buffer::MergePartialFromCodedStream*;
     mir::protobuf::Buffer::New*;
+    mir::protobuf::BufferRequest::?BufferRequest*;
+    mir::protobuf::BufferRequest::BufferRequest*;
+    mir::protobuf::BufferRequest::ByteSize*;
+    mir::protobuf::BufferRequest::Clear*;
+    mir::protobuf::BufferRequest::CopyFrom*;
+    mir::protobuf::BufferRequest::default_instance*;
+    mir::protobuf::BufferRequest::descriptor*;
+    mir::protobuf::BufferRequest::GetMetadata*;
+    mir::protobuf::BufferRequest::IsInitialized*;
+    mir::protobuf::BufferRequest::kBufferFieldNumber*;
+    mir::protobuf::BufferRequest::kIdFieldNumber*;
+    mir::protobuf::BufferRequest::MergeFrom*;
+    mir::protobuf::BufferRequest::MergePartialFromCodedStream*;
+    mir::protobuf::BufferRequest::New*;
+    mir::protobuf::BufferRequest::SerializeWithCachedSizes*;
+    mir::protobuf::BufferRequest::SerializeWithCachedSizesToArray*;
+    mir::protobuf::BufferRequest::Swap*;
     mir::protobuf::Buffer::SerializeWithCachedSizes*;
     mir::protobuf::Buffer::SerializeWithCachedSizesToArray*;
     mir::protobuf::Buffer::Swap*;
@@ -337,6 +354,7 @@
     mir::protobuf::DisplayServer::disconnect*;
     mir::protobuf::DisplayServer::?DisplayServer*;
     mir::protobuf::DisplayServer::drm_auth_magic*;
+    mir::protobuf::DisplayServer::exchange_buffer*;
     mir::protobuf::DisplayServer::GetDescriptor*;
     mir::protobuf::DisplayServer::GetRequestPrototype*;
     mir::protobuf::DisplayServer::GetResponsePrototype*;
@@ -357,6 +375,7 @@
     mir::protobuf::DisplayServer_Stub::?DisplayServer_Stub*;
     mir::protobuf::DisplayServer_Stub::DisplayServer_Stub*;
     mir::protobuf::DisplayServer_Stub::drm_auth_magic*;
+    mir::protobuf::DisplayServer_Stub::exchange_buffer*;
     mir::protobuf::DisplayServer_Stub::new_fds_for_prompt_providers*;
     mir::protobuf::DisplayServer_Stub::next_buffer*;
     mir::protobuf::DisplayServer_Stub::release_screencast*;

=== modified file 'src/server/frontend/session_mediator.cpp'
--- src/server/frontend/session_mediator.cpp	2014-09-08 04:12:17 +0000
+++ src/server/frontend/session_mediator.cpp	2014-09-09 20:45:42 +0000
@@ -248,6 +248,36 @@
         });
 }
 
+void mf::SessionMediator::exchange_buffer(
+    google::protobuf::RpcController*,
+    mir::protobuf::BufferRequest const* request,
+    mir::protobuf::Buffer* response,
+    google::protobuf::Closure* done)
+{
+    mf::SurfaceId const surface_id{request->id().value()};
+    mg::BufferID const buffer_id{static_cast<uint32_t>(request->buffer().buffer_id())};
+
+    auto const lock = std::make_shared<std::unique_lock<std::mutex>>(session_mutex);
+    auto const session = weak_session.lock();
+    if (!session)
+        BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
+
+    auto const& surface = session->get_surface(surface_id);
+    surface->swap_buffers(
+        surface_tracker.buffer_from(buffer_id),
+        [this, surface_id, lock, response, done](mg::Buffer* new_buffer)
+        {
+            lock->unlock();
+
+            if (surface_tracker.track_buffer(surface_id, new_buffer))
+                pack_protobuf_buffer(*response, new_buffer, mg::BufferIpcMsgType::update_msg);
+            else
+                pack_protobuf_buffer(*response, new_buffer, mg::BufferIpcMsgType::full_msg);
+
+            done->Run();
+        });
+}
+
 void mf::SessionMediator::release_surface(
     google::protobuf::RpcController* /*controller*/,
     const mir::protobuf::SurfaceId* request,

=== modified file 'src/server/frontend/session_mediator.h'
--- src/server/frontend/session_mediator.h	2014-09-08 04:12:17 +0000
+++ src/server/frontend/session_mediator.h	2014-09-09 20:45:42 +0000
@@ -98,6 +98,12 @@
         mir::protobuf::Buffer* response,
         google::protobuf::Closure* done) override;
 
+    void exchange_buffer(
+        google::protobuf::RpcController* controller,
+        mir::protobuf::BufferRequest const* request,
+        mir::protobuf::Buffer* response,
+        google::protobuf::Closure* done) override;
+
     void release_surface(google::protobuf::RpcController* controller,
                          const mir::protobuf::SurfaceId*,
                          mir::protobuf::Void*,

=== modified file 'tests/unit-tests/frontend/test_session_mediator.cpp'
--- tests/unit-tests/frontend/test_session_mediator.cpp	2014-09-08 04:12:17 +0000
+++ tests/unit-tests/frontend/test_session_mediator.cpp	2014-09-09 20:45:42 +0000
@@ -112,6 +112,8 @@
 
     std::shared_ptr<mf::Surface> get_surface(mf::SurfaceId surface) const
     {
+        if (mock_surfaces.find(surface) == mock_surfaces.end())
+            BOOST_THROW_EXCEPTION(std::logic_error("Invalid SurfaceId"));
         return mock_surfaces.at(surface);
     }
 
@@ -256,6 +258,7 @@
     mp::Buffer buffer_response;
     mp::DRMMagic drm_request;
     mp::DRMAuthMagicStatus drm_response;
+    mp::BufferRequest buffer_request;
 };
 }
 
@@ -306,6 +309,10 @@
     }, std::logic_error);
 
     EXPECT_THROW({
+        mediator.exchange_buffer(nullptr, &buffer_request, &buffer_response, null_callback.get());
+    }, std::logic_error);
+
+    EXPECT_THROW({
         mediator.release_surface(nullptr, &surface_id_request, nullptr, null_callback.get());
     }, std::logic_error);
 
@@ -346,6 +353,10 @@
     }, std::logic_error);
 
     EXPECT_THROW({
+        mediator.exchange_buffer(nullptr, &buffer_request, &buffer_response, null_callback.get());
+    }, std::logic_error);
+
+    EXPECT_THROW({
         mediator.release_surface(nullptr, &surface_id_request, nullptr, null_callback.get());
     }, std::logic_error);
 
@@ -656,3 +667,106 @@
 
     mediator.disconnect(nullptr, nullptr, nullptr, null_callback.get());
 }
+
+TEST_F(SessionMediator, exchange_buffer)
+{
+    using namespace testing;
+    auto const& mock_surface = stubbed_session->mock_surface_at(mf::SurfaceId{0});
+    mp::Buffer exchanged_buffer;
+    mtd::StubBuffer stub_buffer1;
+    mtd::StubBuffer stub_buffer2;
+
+    //create
+    Sequence seq;
+    EXPECT_CALL(*mock_surface, swap_buffers(_, _))
+        .InSequence(seq)
+        .WillOnce(InvokeArgument<1>(&stub_buffer1));
+    //exchange
+    EXPECT_CALL(*mock_surface, swap_buffers(&stub_buffer1,_))
+        .InSequence(seq)
+        .WillOnce(InvokeArgument<1>(&stub_buffer2));
+
+    mediator.connect(nullptr, &connect_parameters, &connection, null_callback.get());
+    mediator.create_surface(nullptr, &surface_parameters, &surface_response, null_callback.get());
+    EXPECT_THAT(surface_response.buffer().buffer_id(), Eq(stub_buffer1.id().as_value()));
+
+    *buffer_request.mutable_id() = surface_response.id();
+    buffer_request.mutable_buffer()->set_buffer_id(surface_response.buffer().buffer_id());
+    mediator.exchange_buffer(nullptr, &buffer_request, &exchanged_buffer, null_callback.get());
+    EXPECT_THAT(exchanged_buffer.buffer_id(), Eq(stub_buffer2.id().as_value()));
+}
+
+TEST_F(SessionMediator, session_exchange_buffer_sends_minimum_information)
+{
+    using namespace testing;
+    mp::Buffer exchanged_buffer;
+    mf::SurfaceId surf_id{0};
+    mtd::StubBuffer buffer1;
+    mtd::StubBuffer buffer2;
+    auto surface = stubbed_session->mock_surface_at(surf_id);
+    ON_CALL(*surface, swap_buffers(nullptr,_))
+        .WillByDefault(InvokeArgument<1>(&buffer2));
+    ON_CALL(*surface, swap_buffers(&buffer1,_))
+        .WillByDefault(InvokeArgument<1>(&buffer2));
+    ON_CALL(*surface, swap_buffers(&buffer2,_))
+        .WillByDefault(InvokeArgument<1>(&buffer1));
+
+    //create
+    Sequence seq;
+    EXPECT_CALL(*graphics_platform, fill_buffer_package(_, &buffer2, mg::BufferIpcMsgType::full_msg))
+        .InSequence(seq);
+    //swap1
+    EXPECT_CALL(*graphics_platform, fill_buffer_package(_, &buffer1, mg::BufferIpcMsgType::full_msg))
+        .InSequence(seq);
+    //swap2
+    EXPECT_CALL(*graphics_platform, fill_buffer_package(_, &buffer2, mg::BufferIpcMsgType::update_msg))
+        .InSequence(seq);
+    //swap3
+    EXPECT_CALL(*graphics_platform, fill_buffer_package(_, &buffer1, mg::BufferIpcMsgType::update_msg))
+        .InSequence(seq);
+
+    mediator.connect(nullptr, &connect_parameters, &connection, null_callback.get());
+
+    mediator.create_surface(nullptr, &surface_parameters, &surface_response, null_callback.get());
+    *buffer_request.mutable_id() = surface_response.id();
+    buffer_request.mutable_buffer()->set_buffer_id(surface_response.buffer().buffer_id());
+
+    mediator.exchange_buffer(nullptr, &buffer_request, &exchanged_buffer, null_callback.get());
+    buffer_request.mutable_buffer()->set_buffer_id(exchanged_buffer.buffer_id());
+
+    mediator.exchange_buffer(nullptr, &buffer_request, &exchanged_buffer, null_callback.get());
+    buffer_request.mutable_buffer()->set_buffer_id(exchanged_buffer.buffer_id());
+
+    mediator.exchange_buffer(nullptr, &buffer_request, &exchanged_buffer, null_callback.get());
+    mediator.disconnect(nullptr, nullptr, nullptr, null_callback.get());
+}
+
+TEST_F(SessionMediator, exchange_buffer_throws_if_client_submits_bad_request)
+{
+    using namespace testing;
+    auto const& mock_surface = stubbed_session->mock_surface_at(mf::SurfaceId{0});
+    mp::Buffer exchanged_buffer;
+    mtd::StubBuffer stub_buffer1;
+    mtd::StubBuffer stub_buffer2;
+
+    EXPECT_CALL(*mock_surface, swap_buffers(_, _))
+        .WillOnce(InvokeArgument<1>(&stub_buffer1));
+
+    mediator.connect(nullptr, &connect_parameters, &connection, null_callback.get());
+    mediator.create_surface(nullptr, &surface_parameters, &surface_response, null_callback.get());
+    EXPECT_THAT(surface_response.buffer().buffer_id(), Eq(stub_buffer1.id().as_value()));
+
+    *buffer_request.mutable_id() = surface_response.id();
+    //client doesnt own stub_buffer2
+    buffer_request.mutable_buffer()->set_buffer_id(stub_buffer2.id().as_value());
+    EXPECT_THROW({
+        mediator.exchange_buffer(nullptr, &buffer_request, &exchanged_buffer, null_callback.get());
+    }, std::logic_error);
+
+    //client made up its own surface id.
+    buffer_request.mutable_id()->set_value(surface_response.id().value() + 2); 
+    buffer_request.mutable_buffer()->set_buffer_id(stub_buffer1.id().as_value());
+    EXPECT_THROW({
+        mediator.exchange_buffer(nullptr, &buffer_request, &exchanged_buffer, null_callback.get());
+    }, std::logic_error);
+}