Merge lp:~alan-griffiths/mir/fix-1496849 into lp:mir

[Alan Griffiths]

graphics: when supplying the host Mir with a cursor don't discard the buffer stream that supports it

graphics: when supplying the host Mir with a cursor don't discard the buffer stream that supports it

NB This fixes the linked bug, but isn't the last problem in this area. In particular, testing with "animated_cursor" shows that when moving outside the client window the default cursor isn't restored.

[Alan Griffiths (alan-griffiths)]

@testing with "animated_cursor" - the problem is that the displayed cursor is a frame behind the nested server. Will tackle in a follow-up MP.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2953
http://jenkins.qa.ubuntu.com/job/mir-ci/4988/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/4086
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2993
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4028
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1138
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1138/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4029
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4029/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6727
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/23570

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4988/rebuild

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

Looks good.

Nit:

+mgn::Cursor::~Cursor()
+{
+}

mgn::Cursor::~Cursor() = default

review: Approve

[Daniel van Vugt (vanvugt)]

I haven't looked into how the cursor buffer stream stuff works yet, but based on your description alone I'm concerned we might have a bad client API design. The client should be able to set-and-forget a cursor. Don't we support that?

[Daniel van Vugt (vanvugt)]

review: Needs Information

[Alan Griffiths (alan-griffiths)]

> I haven't looked into how the cursor buffer stream stuff works yet, but based
> on your description alone I'm concerned we might have a bad client API design.
> The client should be able to set-and-forget a cursor. Don't we support that?

The original code didn't "forget" the buffer stream - it released it.

I'm not trying to defend the design (or implementation) of cursors. (As anyone hearing my recent complaints would realize.) I'm working to get the cursor code under some useful tests[1] as preparation for some rework.

[1] Note that the existing interaction between tests and implementation are "interesting". For example cursor_request_applied_from_buffer_stream configures the cursor before posting any buffers in this scenario BasicSurface::set_cursor_stream() creates an image from the stream before the stream has received a buffer.

=== modified file 'src/server/graphics/nested/cursor.cpp'
--- src/server/graphics/nested/cursor.cpp	2015-05-19 21:34:34 +0000
+++ src/server/graphics/nested/cursor.cpp	2015-09-22 12:00:32 +0000
@@ -31,9 +31,12 @@
     connection->set_cursor_image(*default_image);
 }
 
+mgn::Cursor::~Cursor()
+{
+}
+
 void mgn::Cursor::move_to(geom::Point)
 {
-    // TODO Should this throw?
 }
 
 void mgn::Cursor::show(mg::CursorImage const& cursor_image)

=== modified file 'src/server/graphics/nested/cursor.h'
--- src/server/graphics/nested/cursor.h	2015-05-19 21:34:34 +0000
+++ src/server/graphics/nested/cursor.h	2015-09-22 12:00:32 +0000
@@ -32,7 +32,7 @@
 {
 public:
     Cursor(std::shared_ptr<HostConnection> const& host_connection, std::shared_ptr<CursorImage> const& default_image);
-    ~Cursor() = default;
+    ~Cursor();
 
     void show(CursorImage const& image) override;
     void show() override;

=== modified file 'src/server/graphics/nested/mir_client_host_connection.cpp'
--- src/server/graphics/nested/mir_client_host_connection.cpp	2015-09-07 15:28:22 +0000
+++ src/server/graphics/nested/mir_client_host_connection.cpp	2015-09-22 12:00:32 +0000
@@ -74,6 +74,7 @@
 
     ~MirClientHostSurface()
     {
+        if (cursor) mir_buffer_stream_release_sync(cursor);
         mir_surface_release_sync(mir_surface);
     }
 
@@ -90,36 +91,50 @@
 
     void set_cursor_image(mg::CursorImage const& image)
     {
-        auto image_width = image.size().width.as_int();
-        auto image_height = image.size().height.as_int();
-        auto pixels_size = image_width * image_height
+        auto const image_width = image.size().width.as_int();
+        auto const image_height = image.size().height.as_int();
+        auto const pixels_size = image_width * image_height
             * MIR_BYTES_PER_PIXEL(mir_pixel_format_argb_8888);
 
-        // TODO: Maybe the stream should be preserved.
-        auto bs = mir_connection_create_buffer_stream_sync(mir_connection,
-                                                           image_width,
-                                                           image_height,
-                                                           mir_pixel_format_argb_8888,
-                                                           mir_buffer_usage_software);
-        
         MirGraphicsRegion g;
-        mir_buffer_stream_get_graphics_region(bs, &g);
-        if ((g.height * g.stride) !=
-            pixels_size)
-            BOOST_THROW_EXCEPTION(std::runtime_error("Cursor BufferStream not compatible with requested cursor image"));
+
+        if (cursor)
+        {
+            mir_buffer_stream_get_graphics_region(cursor, &g);
+
+            if (image_width != g.width || image_height != g.height)
+            {
+                mir_buffer_stream_release_sync(cursor);
+                cursor = nullptr;
+            }
+        }
+
+        if (!cursor)
+        {
+            cursor = mir_connection_create_buffer_stream_sync(
+                mir_connection,
+                image_width,
+                image_height,
+                mir_pixel_format_argb_8888,
+                mir_buffer_usage_software);
+
+            mir_buffer_stream_get_graphics_region(cursor, &g);
+        }
+
         memcpy(g.vaddr, image.as_argb_8888(), pixels_size);
-        mir_buffer_stream_swap_buffers_sync(bs);
+        mir_buffer_stream_swap_buffers_sync(cursor);
 
-        auto conf = mir_cursor_configuration_from_buffer_stream(bs,
+        auto conf = mir_cursor_configuration_from_buffer_stream(cursor,
             image.hotspot().dx.as_int(), image.hotspot().dy.as_int());
         
         mir_surface_configure_cursor(mir_surface, conf);
         mir_cursor_configuration_destroy(conf);
-        mir_buffer_stream_release_sync(bs);
     }
 
     void hide_cursor()
     {
+        if (cursor) { mir_buffer_stream_release_sync(cursor); cursor = nullptr; }
+
         auto conf = mir_cursor_configuration_from_name(mir_disabled_cursor_name);
         mir_surface_configure_cursor(mir_surface, conf);
         mir_cursor_configuration_destroy(conf);
@@ -128,7 +143,7 @@
 private:
     MirConnection* const mir_connection;
     MirSurface* const mir_surface;
-
+    MirBufferStream* cursor{nullptr};
 };
 
 }