Merge lp:~kdub/mir/fix-1626503 into lp:mir

[Kevin DuBois]

mir_presentation_chain api:
fix situation where where the server would not notify a client of a buffers return, following the destruction of the MirPresentationChain that the buffer was associated with.

fixes: LP: #1626503

mir_presentation_chain api:
fix situation where where the server would not notify a client of a buffers return, following the destruction of the MirPresentationChain that the buffer was associated with.

fixes: LP: #1626503

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

FAILED: Continuous integration, rev:3718
https://mir-jenkins.ubuntu.com/job/mir-ci/1784/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/2234/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2297
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2288
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2288
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2288
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2262/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2262
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2262/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2262
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2262/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/2262
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2262/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2262/console

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

review: Needs Fixing (continuous-integration)

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

PASSED: Continuous integration, rev:3720
https://mir-jenkins.ubuntu.com/job/mir-ci/1789/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/2239
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2302
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2293
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2293
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2293
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2267
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2267/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/2267
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2267/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2267
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2267/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/2267
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2267/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/2267
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2267/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

It's worth noting MultiMonitorArbiter will get deprecated and possibly removed soon enough (hopefully). As swap interval 1 throttling moves out of libmirserver and into libmirclient (using high-precision frame sync timing), it won't be needed any more.

[Brandon Schaefer (brandontschaefer)]

Im not super familiar with the context but the code looks good to me.

review: Approve

[Kevin DuBois (kdub)]

> It's worth noting MultiMonitorArbiter will get deprecated and possibly removed
> soon enough (hopefully). As swap interval 1 throttling moves out of
> libmirserver and into libmirclient (using high-precision frame sync timing),
> it won't be needed any more.

Hmm, news to me. I suppose some logic in there won't be needed, but some system of handing the buffers out will still be present, we might to need to sync on the plans.

[Alexandros Frantzis (afrantzis)]

Looks good.

Nit:
+ for(auto it = onscreen_buffers.begin(); it != onscreen_buffers.end(); it++)
+ {
+ if (it->use_count == 0)
+ map->send_buffer(it->buffer->id());
+ }

Why not use a range-based for loop?

review: Approve

=== modified file 'src/client/buffer_vault.cpp'
--- src/client/buffer_vault.cpp	2016-07-05 12:40:56 +0000
+++ src/client/buffer_vault.cpp	2016-09-22 19:03:27 +0000
@@ -45,6 +45,9 @@
 
 namespace
 {
+void ignore_buffer(MirBuffer*, void*)
+{
+}
 void incoming_buffer(MirBuffer* buffer, void* context)
 {
     auto vault = static_cast<mcl::BufferVault*>(context);
@@ -80,9 +83,15 @@
         return;
 
     buffer_factory->cancel_requests_with_context(this);
+    std::unique_lock<std::mutex> lk(mutex);
     for (auto& it : buffers)
     try
     {
+        if (auto map = surface_map.lock())
+        {
+            auto buffer = map->buffer(it.first);
+            buffer->set_callback(ignore_buffer, nullptr);
+        } 
         free_buffer(it.first);
     }
     catch (...)
 
=== modified file 'src/client/mir_connection.cpp'
--- src/client/mir_connection.cpp	2016-08-31 10:47:16 +0000
+++ src/client/mir_connection.cpp	2016-09-22 19:03:27 +0000
@@ -1223,7 +1223,7 @@
 void MirConnection::release_presentation_chain(MirPresentationChain* chain)
 {
     auto id = chain->rpc_id();
-    if (id > 0)
+    if (id >= 0)
     {
         StreamRelease stream_release{nullptr, nullptr, nullptr, nullptr, chain->rpc_id()};
         mp::BufferStreamId buffer_stream_id;
 
=== modified file 'src/client/surface_map.cpp'
--- src/client/surface_map.cpp	2016-06-06 11:51:31 +0000
+++ src/client/surface_map.cpp	2016-09-22 19:03:27 +0000
@@ -60,7 +60,7 @@
 void mcl::ConnectionSurfaceMap::with_stream_do(
     mf::BufferStreamId stream_id, std::function<void(ClientBufferStream*)> const& exec) const
 {
-    std::shared_lock<decltype(guard)> lk(guard);
+    std::shared_lock<decltype(stream_guard)> lk(stream_guard);
     auto const it = streams.find(stream_id);
     if (it != streams.end())
     {
@@ -78,7 +78,7 @@
 
 void mcl::ConnectionSurfaceMap::with_all_streams_do(std::function<void(ClientBufferStream*)> const& fn) const
 {
-    std::shared_lock<decltype(guard)> lk(guard);
+    std::shared_lock<decltype(stream_guard)> lk(stream_guard);
     for(auto const& stream : streams)
         fn(stream.second.get());
 }
@@ -86,7 +86,7 @@
 void mcl::ConnectionSurfaceMap::insert(
     mf::BufferStreamId stream_id, std::shared_ptr<ClientBufferStream> const& stream)
 {
-    std::lock_guard<decltype(guard)> lk(guard);
+    std::lock_guard<decltype(stream_guard)> lk(stream_guard);
     streams[stream_id] = stream;
 }
 
@@ -99,7 +99,7 @@
 
 void mcl::ConnectionSurfaceMap::erase(mf::BufferStreamId stream_id)
 {
-    std::lock_guard<decltype(guard)> lk(guard);
+    std::lock_guard<decltype(stream_guard)> lk(stream_guard);
     auto stream_it = streams.find(stream_id);
     auto chain_it = chains.find(stream_id);
     if (stream_it != streams.end())
@@ -110,19 +110,19 @@
 
 void mcl::ConnectionSurfaceMap::insert(int buffer_id, std::shared_ptr<mcl::MirBuffer> const& buffer)
 {
-    std::lock_guard<decltype(guard)> lk(guard);
+    std::lock_guard<decltype(buffer_guard)> lk(buffer_guard);
     buffers[buffer_id] = buffer;
 }
 
 void mcl::ConnectionSurfaceMap::erase(int buffer_id)
 {
-    std::lock_guard<decltype(guard)> lk(guard);
+    std::lock_guard<decltype(buffer_guard)> lk(buffer_guard);
     buffers.erase(buffer_id);
 }
 
 std::shared_ptr<mcl::MirBuffer> mcl::ConnectionSurfaceMap::buffer(int buffer_id) const
 {
-    std::shared_lock<decltype(guard)> lk(guard);
+    std::shared_lock<decltype(buffer_guard)> lk(buffer_guard);
     auto const it = buffers.find(buffer_id);
     if (it != buffers.end())
         return it->second;
 
=== modified file 'src/server/compositor/multi_monitor_arbiter.cpp'
--- src/server/compositor/multi_monitor_arbiter.cpp	2016-05-03 06:55:25 +0000
+++ src/server/compositor/multi_monitor_arbiter.cpp	2016-09-22 19:03:27 +0000
@@ -39,6 +39,17 @@
 {
 }
 
+mc::MultiMonitorArbiter::~MultiMonitorArbiter()
+{
+    std::lock_guard<decltype(mutex)> lk(mutex);
+    for(auto it = onscreen_buffers.begin(); it != onscreen_buffers.end(); it++)
+    {
+        if (it->use_count == 0)
+            map->send_buffer(it->buffer->id());
+    }
+
+}
+
 std::shared_ptr<mg::Buffer> mc::MultiMonitorArbiter::compositor_acquire(compositor::CompositorID id)
 {
     std::lock_guard<decltype(mutex)> lk(mutex);
 
=== modified file 'src/server/compositor/multi_monitor_arbiter.h'
--- src/server/compositor/multi_monitor_arbiter.h	2016-05-03 06:55:25 +0000
+++ src/server/compositor/multi_monitor_arbiter.h	2016-09-22 19:03:27 +0000
@@ -41,6 +41,7 @@
         MultiMonitorMode mode,
         std::shared_ptr<frontend::ClientBuffers> const& map,
         std::shared_ptr<Schedule> const& schedule);
+    ~MultiMonitorArbiter();
 
     std::shared_ptr<graphics::Buffer> compositor_acquire(compositor::CompositorID id) override;
     void compositor_release(std::shared_ptr<graphics::Buffer> const&) override;
 
=== modified file 'src/server/compositor/stream.cpp'
--- src/server/compositor/stream.cpp	2016-06-02 05:33:50 +0000
+++ src/server/compositor/stream.cpp	2016-09-22 19:03:27 +0000
@@ -73,6 +73,12 @@
 {
 }
 
+mc::Stream::~Stream()
+{
+    while(schedule->num_scheduled())
+        buffers->send_buffer(schedule->next_buffer()->id());
+}
+
 unsigned int mc::Stream::client_owned_buffer_count(std::lock_guard<decltype(mutex)> const&) const
 {
     auto server_count = schedule->num_scheduled();
 
=== modified file 'src/server/compositor/stream.h'
--- src/server/compositor/stream.h	2016-05-04 01:48:16 +0000
+++ src/server/compositor/stream.h	2016-09-22 19:03:27 +0000
@@ -44,6 +44,7 @@
     Stream(
         FrameDroppingPolicyFactory const& policy_factory,
         std::shared_ptr<frontend::ClientBuffers>, geometry::Size sz, MirPixelFormat format);
+    ~Stream();
 
     void swap_buffers(
         graphics::Buffer* old_buffer, std::function<void(graphics::Buffer* new_buffer)> complete) override;
 
=== modified file 'tests/acceptance-tests/throwback/test_presentation_chain.cpp'
--- tests/acceptance-tests/throwback/test_presentation_chain.cpp	2016-08-23 16:18:08 +0000
+++ tests/acceptance-tests/throwback/test_presentation_chain.cpp	2016-09-22 19:03:27 +0000
@@ -188,6 +188,10 @@
     unsigned int callback_count = 0;
 };
 
+void ignore_callback(MirBuffer*, void*)
+{
+}
+
 void buffer_callback(MirBuffer* buffer, void* context)
 {
     auto sync = reinterpret_cast<MirBufferSync*>(context);
@@ -292,6 +296,9 @@
         if (i != 0)
             ASSERT_TRUE(contexts[(i-1) % num_buffers].wait_for_buffer(10s)) << "iteration " << i;
     }
+
+    for (auto& context : contexts)
+        mir_buffer_set_callback(context.buffer(), ignore_callback, nullptr);
 }
 
 TEST_F(PresentationChain, submission_will_eventually_call_callback_reassociated)
@@ -318,6 +325,8 @@
         if (i != 0)
             ASSERT_TRUE(contexts[(i-1) % num_buffers].wait_for_buffer(10s)) << "iteration " << i;
     }
+    for (auto& context : contexts)
+        mir_buffer_set_callback(context.buffer(), ignore_callback, nullptr);
 }
 
 TEST_F(PresentationChain, buffers_can_be_destroyed_before_theyre_returned)
@@ -336,6 +345,45 @@
     mir_buffer_release(context.buffer());
 }
 
+TEST_F(PresentationChain, destroying_a_chain_will_return_buffers_associated_with_chain)
+{
+    auto chain = mir_connection_create_presentation_chain_sync(connection);
+    auto stream = mir_connection_create_buffer_stream_sync(connection, 25, 12, mir_pixel_format_abgr_8888, mir_buffer_usage_hardware);
+    ASSERT_TRUE(mir_presentation_chain_is_valid(chain));
+
+    auto spec = mir_connection_create_spec_for_normal_surface(
+        connection, size.width.as_int(), size.height.as_int(), pf);
+    auto surface = mir_surface_create_sync(spec);
+    mir_surface_spec_release(spec);
+
+    spec = mir_connection_create_spec_for_changes(connection);
+    mir_surface_spec_add_presentation_chain(
+        spec, size.width.as_int(), size.height.as_int(), 0, 0, chain);
+    mir_surface_apply_spec(surface, spec);
+    mir_surface_spec_release(spec);
+
+    MirBufferSync context;
+    mir_connection_allocate_buffer(
+        connection,
+        size.width.as_int(), size.height.as_int(), pf, usage,
+        buffer_callback, &context);
+    ASSERT_TRUE(context.wait_for_buffer(10s));
+    context.unavailable();
+    mir_presentation_chain_submit_buffer(chain, context.buffer());
+
+    spec = mir_connection_create_spec_for_changes(connection);
+    mir_surface_spec_add_buffer_stream(spec, 0, 0, stream);
+    mir_surface_apply_spec(surface, spec);
+    mir_surface_spec_release(spec);
+    mir_presentation_chain_release(chain);
+    mir_buffer_stream_swap_buffers_sync(stream);
+
+    ASSERT_TRUE(context.wait_for_buffer(10s));
+
+    mir_buffer_stream_release_sync(stream);
+    mir_surface_release_sync(surface);
+}
+
 TEST_F(PresentationChain, can_access_basic_buffer_properties)
 {
     MirBufferSync context;
@@ -417,4 +465,7 @@
 
     ASSERT_TRUE(another_context.wait_for_buffer(10s));
     ASSERT_THAT(another_context.buffer(), Ne(nullptr));
+
+    mir_buffer_set_callback(context.buffer(), ignore_callback, nullptr);
+    mir_buffer_set_callback(second_buffer_context.buffer(), ignore_callback, nullptr);
 }
 
=== modified file 'tests/integration-tests/test_buffer_scheduling.cpp'
--- tests/integration-tests/test_buffer_scheduling.cpp	2016-08-16 19:34:28 +0000
+++ tests/integration-tests/test_buffer_scheduling.cpp	2016-09-22 19:03:27 +0000
@@ -453,6 +453,10 @@
             vault.set_size(sz);
         });
     }
+    ~ScheduledProducer()
+    {
+        ipc->on_client_bound_transfer([this](mp::BufferRequest&){});
+    }
 
     bool can_produce()
     {
 
=== modified file 'tests/unit-tests/client/test_mir_connection.cpp'
--- tests/unit-tests/client/test_mir_connection.cpp	2016-06-28 12:18:18 +0000
+++ tests/unit-tests/client/test_mir_connection.cpp	2016-09-22 19:03:27 +0000
@@ -820,7 +820,7 @@
         static_cast<MirPresentationChain*>(callback.resulting_chain)->rpc_id());
 
     EXPECT_CALL(*mock_channel, buffer_stream_release(ReleaseRequestHasId(expected_request)))
-        .Times(0);
+        .Times(1);
 
     connection->release_presentation_chain(callback.resulting_chain);
 }
 
=== modified file 'tests/unit-tests/compositor/test_multi_monitor_arbiter.cpp'
--- tests/unit-tests/compositor/test_multi_monitor_arbiter.cpp	2016-06-02 05:33:50 +0000
+++ tests/unit-tests/compositor/test_multi_monitor_arbiter.cpp	2016-09-22 19:03:27 +0000
@@ -142,6 +142,7 @@
     EXPECT_THAT(cbuffer1, Ne(cbuffer2));
     arbiter.compositor_release(cbuffer2);
     arbiter.compositor_release(cbuffer1);
+    Mock::VerifyAndClearExpectations(&mock_map);
 }
 
 TEST_F(MultiMonitorArbiterWithAnyFrameGuarantee, compositor_buffer_syncs_to_fastest_compositor)
@@ -394,6 +395,8 @@
     arbiter.compositor_release(b4);
     auto b6 = arbiter.compositor_acquire(&comp_id1);
     arbiter.compositor_release(b6);
+
+    Mock::VerifyAndClearExpectations(&mock_map);
 }
 
 TEST_F(MultiMonitorArbiter, can_check_if_buffers_are_ready)
@@ -458,6 +461,7 @@
     EXPECT_THAT(b2, Eq(buffers[1]));
     arbiter.compositor_release(b1);
     arbiter.compositor_release(b2);
+    Mock::VerifyAndClearExpectations(&mock_map);
 } 
 
 TEST_F(MultiMonitorArbiter, will_release_buffer_in_nbuffers_2_starvation_scenario)
@@ -513,6 +517,7 @@
     EXPECT_THAT(b3, Eq(buffers[1]));
     EXPECT_THAT(b4, Eq(buffers[1]));
     EXPECT_THAT(b5, Eq(buffers[2]));
+    Mock::VerifyAndClearExpectations(&mock_map);
 } 
 
 TEST_F(MultiMonitorArbiter, can_advance_buffer_manually)
@@ -532,3 +537,11 @@
     auto b3 = arbiter.compositor_acquire(&comp_id1);
     EXPECT_THAT(b3->id(), Eq(buffers[2]->id()));
 }
+
+TEST_F(MultiMonitorArbiter, releases_buffer_on_destruction)
+{
+    mc::MultiMonitorArbiter arbiter{guarantee, mt::fake_shared(mock_map), mt::fake_shared(schedule)};
+    EXPECT_CALL(mock_map, send_buffer(buffers[0]->id()));
+    schedule.set_schedule({buffers[0]});
+    arbiter.advance_schedule();
+}
 
=== modified file 'tests/unit-tests/compositor/test_stream.cpp'
--- tests/unit-tests/compositor/test_stream.cpp	2016-05-04 01:48:16 +0000
+++ tests/unit-tests/compositor/test_stream.cpp	2016-09-22 19:03:27 +0000
@@ -123,6 +123,7 @@
         cbuffers.push_back(stream.lock_compositor_buffer(this));
     ASSERT_THAT(cbuffers, SizeIs(1));
     EXPECT_THAT(cbuffers[0]->id(), Eq(buffers.back()->id()));
+    Mock::VerifyAndClearExpectations(&mock_sink);
 }
 
 TEST_F(Stream, transitions_from_framedropping_to_queuing)
@@ -325,6 +326,7 @@
     Mock::VerifyAndClearExpectations(&mock_sink);
     EXPECT_CALL(mock_sink, send_buffer(_,_,_));
     framedrop_factory.trigger_policies();
+    Mock::VerifyAndClearExpectations(&mock_sink);
 }
 
 TEST_F(Stream, doesnt_drop_the_only_frame_when_arbiter_has_none)
@@ -334,6 +336,7 @@
     EXPECT_CALL(mock_sink, send_buffer(_,_,_))
         .Times(0);
     framedrop_factory.trigger_policies();
+    Mock::VerifyAndClearExpectations(&mock_sink);
 }
 
 TEST_F(Stream, doesnt_drop_the_latest_frame_with_a_longer_queue)
@@ -347,6 +350,7 @@
     EXPECT_CALL(mock_sink, send_buffer(_,Ref(*buffers[1]),_))
         .Times(1);
     framedrop_factory.trigger_policies();
+    Mock::VerifyAndClearExpectations(&mock_sink);
 }
 
 TEST_F(Stream, doesnt_drop_the_latest_frame_with_a_2_buffer_queue)
@@ -359,6 +363,7 @@
     EXPECT_CALL(mock_sink, send_buffer(_,Ref(*buffers[1]),_))
         .Times(0);
     framedrop_factory.trigger_policies();
+    Mock::VerifyAndClearExpectations(&mock_sink);
 }
 
 TEST_F(Stream, returns_buffers_to_client_when_told_to_bring_queue_up_to_date)
@@ -371,4 +376,5 @@
     EXPECT_CALL(mock_sink, send_buffer(_,Ref(*buffers[0]),_));
     EXPECT_CALL(mock_sink, send_buffer(_,Ref(*buffers[1]),_));
     stream.drop_old_buffers();
+    Mock::VerifyAndClearExpectations(&mock_sink);
 }