Merge lp:~kdub/mir/rm-depository-from-screencast into lp:mir

[Kevin DuBois]

client: remove usage of BufferVault in the ScreencastStream. Disentangles ScreencastStream, preparing for BufferVault's removal once we're ready to remove that code.

This also reduces the client-side number of mapped buffers to just the current one, whereas before, we were keeping 3 around, and kicking out the oldest one as new ones arrived.

Also add a bonus test where I noticed that a wait handle could have been left hanging if an exception threw around buffer creation.

client: remove usage of BufferVault in the ScreencastStream. Disentangles ScreencastStream, preparing for BufferVault's removal once we're ready to remove that code.

This also reduces the client-side number of mapped buffers to just the current one, whereas before, we were keeping 3 around, and kicking out the oldest one as new ones arrived.

Also add a bonus test where I noticed that a wait handle could have been left hanging if an exception threw around buffer creation.

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

FAILED: Continuous integration, rev:3534
https://mir-jenkins.ubuntu.com/job/mir-ci/1195/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/1356/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/1407
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/1398
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/1398
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/1370/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/1370/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1370/console
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1370/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/1370/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/1370/console

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

review: Needs Fixing (continuous-integration)

[Chris Halse Rogers (raof)]

Seems sensible.

review: Approve

[Alexandros Frantzis (afrantzis)]

On the server side we are still tracking screencast buffers and support sending mg::BufferIpcMsgType::update_msg buffer messages. The new code (i.e. without BufferDepository) doesn't seem to support this scenario.

review: Needs Fixing

[Kevin DuBois (kdub)]

recent changes to screencasting have us rotating through the buffers, instead of allocing a new one every time. This did work on the devices I tested with, but we might have to keep BufferVault around after all. Will take MP down to investigate

Unmerged revisions

3534. By Kevin DuBois on 2016-06-24

merge in mir

3533. By Kevin DuBois on 2016-06-07

merge in mir

3532. By Kevin DuBois on 2016-06-07

test to avoid hanging to pass

=== modified file 'src/client/screencast_stream.cpp'
--- src/client/screencast_stream.cpp	2016-06-02 05:33:50 +0000
+++ src/client/screencast_stream.cpp	2016-06-24 12:39:34 +0000
@@ -55,8 +55,7 @@
     client_platform(client_platform),
     factory(client_platform->create_buffer_factory()),
     protobuf_bs{mcl::make_protobuf_object<mir::protobuf::BufferStream>(a_protobuf_bs)},
-    protobuf_void{mcl::make_protobuf_object<mir::protobuf::Void>()},
-    buffer_depository{factory}
+    protobuf_void{mcl::make_protobuf_object<mir::protobuf::Void>()}
 {
     if (!protobuf_bs->has_id())
     {
@@ -93,9 +92,12 @@
     try
     {
         auto const pixel_format = static_cast<MirPixelFormat>(protobuf_bs->pixel_format());
-        buffer_depository.deposit_package(mcl::protobuf_to_native_buffer(buffer),
-                                         buffer.buffer_id(), buffer_size,
-                                         pixel_format);
+        if (current_buffer_id != buffer.buffer_id())
+        {
+            current_buffer = factory->create_buffer(
+                mcl::protobuf_to_native_buffer(buffer), buffer_size, pixel_format);
+            current_buffer_id = buffer.buffer_id();
+        }
     }
     catch (const std::runtime_error& error)
     {
@@ -104,6 +106,8 @@
         protobuf_bs->set_error(
             std::string{"Error processing buffer stream creating response:"} +
             boost::diagnostic_information(error));
+        if (screencast_wait_handle.is_pending())
+            screencast_wait_handle.result_received();
         throw error;
     }
 }
@@ -132,7 +136,7 @@
 std::shared_ptr<mcl::ClientBuffer> mcl::ScreencastStream::get_current_buffer()
 {
     std::lock_guard<decltype(mutex)> lock(mutex);
-    return buffer_depository.current_buffer();
+    return current_buffer;
 }
 
 EGLNativeWindowType mcl::ScreencastStream::egl_native_window()
@@ -143,9 +147,8 @@
 std::shared_ptr<mcl::MemoryRegion> mcl::ScreencastStream::secure_for_cpu_write()
 {
     std::lock_guard<decltype(mutex)> lock(mutex);
-    auto buffer = buffer_depository.current_buffer();
     if (!secured_region)
-        secured_region = buffer->secure_for_cpu_write();
+        secured_region = current_buffer->secure_for_cpu_write();
     return secured_region;
 }
 
@@ -179,7 +182,7 @@
 uint32_t mcl::ScreencastStream::get_current_buffer_id()
 {
     std::lock_guard<decltype(mutex)> lock(mutex);
-    return buffer_depository.current_buffer_id();
+    return current_buffer_id;
 }
 
 int mcl::ScreencastStream::swap_interval() const

=== modified file 'src/client/screencast_stream.h'
--- src/client/screencast_stream.h	2016-05-03 06:55:25 +0000
+++ src/client/screencast_stream.h	2016-06-24 12:39:34 +0000
@@ -24,7 +24,6 @@
 #include "mir/egl_native_surface.h"
 #include "mir/client_buffer.h"
 #include "client_buffer_stream.h"
-#include "client_buffer_depository.h"
 #include "mir/geometry/size.h"
 
 #include "mir_toolkit/client_types.h"
@@ -126,7 +125,9 @@
 
     geometry::Size buffer_size;
     std::string error_message;
-    ClientBufferDepository buffer_depository;
+
+    std::shared_ptr<ClientBuffer> current_buffer;
+    int32_t current_buffer_id;
 };
 
 }

=== modified file 'tests/unit-tests/client/test_screencast_stream.cpp'
--- tests/unit-tests/client/test_screencast_stream.cpp	2016-01-28 19:05:20 +0000
+++ tests/unit-tests/client/test_screencast_stream.cpp	2016-06-24 12:39:34 +0000
@@ -27,6 +27,7 @@
 #include "mir/test/fake_shared.h"
 #include "mir_toolkit/mir_client_library.h"
 
+#include <exception>
 #include <atomic>
 
 namespace mp = mir::protobuf;
@@ -153,3 +154,59 @@
 
     EXPECT_THAT(stream.get_current_buffer_id(), Eq(id1));
 }
+
+TEST_F(ScreencastStream, exception_does_not_leave_wait_handle_hanging)
+{
+    struct FailingBufferFactory : mcl::ClientBufferFactory
+    {
+        std::shared_ptr<mcl::ClientBuffer> create_buffer(
+            std::shared_ptr<MirBufferPackage> const&, geom::Size, MirPixelFormat)
+        {
+            if (fail)
+                throw std::runtime_error("monkey wrench");
+            else
+                return nullptr;
+        }
+
+        void start_failing()
+        {
+            fail = true;
+        }
+
+    private:
+        bool fail = false;
+    };
+
+    struct BufferCreationFailingPlatform : mir_test_framework::StubClientPlatform
+    {
+        BufferCreationFailingPlatform(
+            std::shared_ptr<mcl::ClientBufferFactory> const& factory, mcl::ClientContext* context) :
+            StubClientPlatform(context),
+            factory(factory)
+        {
+        }
+        std::shared_ptr<mcl::ClientBufferFactory> create_buffer_factory()
+        {
+            return factory;
+        }
+        std::shared_ptr<mcl::ClientBufferFactory> const factory;
+    };
+
+    EXPECT_CALL(mock_protobuf_server, screencast_buffer(_,_,_))
+        .WillOnce(Invoke(
+            [](mp::ScreencastId const*, mp::Buffer* b, google::protobuf::Closure* c)
+            {
+                int different_id = 33;
+                b->set_buffer_id(different_id);
+                EXPECT_THROW({ c->Run(); }, std::runtime_error);
+            }));
+    auto factory = std::make_shared<FailingBufferFactory>();
+    auto platform = std::make_shared<BufferCreationFailingPlatform>(factory, nullptr);
+
+    mcl::ScreencastStream stream(nullptr, mock_protobuf_server, platform, response);
+    factory->start_failing();
+
+    auto wh = stream.next_buffer([]{});
+    wh->wait_for_all();
+    EXPECT_FALSE(wh->is_pending());
+}