Merge lp:~mir-team/mir/add-client-cursor-conf-from-argb into lp:mir

[Robert Carr]

Add a method for clients to upload ARGB cursor images from pixel data.

Add a method for clients to upload ARGB cursor images from pixel data. Previously noted as required, just not added. Doing now to enable nested cursor API passthrough (user session would be responsible for loading cursor themes and pass images up to USC...), and complete Qt cursor support.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2100
http://jenkins.qa.ubuntu.com/job/mir-ci/2240/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/321
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/321/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/295/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/229/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/295/console

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2101
http://jenkins.qa.ubuntu.com/job/mir-ci/2242/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/324
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/324/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/298/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/231/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/298/console

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2102
http://jenkins.qa.ubuntu.com/job/mir-ci/2244/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/326
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/326/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/300/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/233/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/300
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/300/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3549/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/16206

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

review: Needs Fixing (continuous-integration)

[Chris Halse Rogers (raof)]

Hm. Could this instead be

MirSurface* create_cursor_surface();

MirCursorConfiguration* mir_cursor_configuration_from_surface(MirSurface, hotspot_x, hotspot_y)?

[Robert Carr (robertcarr)]

Hey Chris, I started taking a stab at it this morning...the more I thought about it the less im convinced it's what we want though.

From the client perspective, the surface API is a little more difficult:
1. Create cursor-image-surface
2. Map cursor-image-surface and copy contents
3. Swap buffers
4. Attach cursor-imagesurface to cursor configuration and cursor configuration to surface

vs.

1. Create cursor conf with contents
2. Attach cursor conf to surface.

What does the client gain from the more difficult API though? my perspective is not much...besides perhaps pixel format negotiation.

Once I noticed this I started thinking about if it was really a surface? I think it bears some resemblance to a surface as it is a format of buffer upload to the server...but I think it omits the key aspects of a surface:

1. Presence in scene
2. Frame synchronization/buffer exchange ala frame-dropping is always on for cursor-image-surfaces.
3. Attribute/state-fullness, e.g. cursor upload is fire and forget

Maybe I am missing a benefit to having it represented as a surface. Maybe the alarming bit is that it isn't using MirPixelFormat, etc? Perhaps with a few tweaks it could be mir_cursor_configuration_from_graphics_region

Let me know what you think. Thanks :)

review: Approve

[Robert Carr (robertcarr)]

review: Abstain

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2103
http://jenkins.qa.ubuntu.com/job/mir-ci/2260/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/349
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/349
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/323
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/249
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/249/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/323
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/323/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3569
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/16251

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

review: Approve (continuous-integration)

Unmerged revisions

2103. By Robert Carr on 2014-11-25

Fix pointer arithmetic

2102. By Robert Carr on 2014-11-25

Fix uninitialized memory

2101. By Robert Carr on 2014-11-24

Do not allow setting hotspot past bounds of cursor

2100. By Robert Carr on 2014-11-24

Only support default sized cursors

2099. By Robert Carr on 2014-11-24

Implement hotspot in raw image API

2098. By Robert Carr on 2014-11-24

First impl

2097. By Robert Carr on 2014-11-24

Test skeleton for image cursors

2096. By Robert Carr on 2014-11-24

Test skeleton for image cursors

=== modified file 'client-ABI-sha1sums'
--- client-ABI-sha1sums	2014-11-25 02:34:24 +0000
+++ client-ABI-sha1sums	2014-11-25 19:34:43 +0000
@@ -2,7 +2,7 @@
 b53736bcb22ddc09aab8275cfdd05c109fdd0f12  include/client/mir_toolkit/mir_client_library_drm.h
 18b2b173b3baeaace89cd777e91e0ece5a611cee  include/client/mir_toolkit/mir_client_library.h
 626fbcc008539c5f1713d8f6eef8c2acc4929af9  include/client/mir_toolkit/mir_connection.h
-1ef8f51a3e3f8d1559266c5af58fbfde7cfabf0a  include/client/mir_toolkit/mir_cursor_configuration.h
[phone]a52b87f4e28bd5f452d82eaa50da692  include/client/mir_toolkit/mir_cursor_configuration.h
 9d50df5a141ca03ee8a79f7e844ed4b8b3b7d5d3  include/client/mir_toolkit/mir_prompt_session.h
 21d07e655e85eeec8a3523e1c6f9c2252176ec01  include/client/mir_toolkit/mir_screencast.h
 b9a11709737d6178c4bf30d067922ca823957045  include/client/mir_toolkit/mir_surface.h

=== modified file 'include/client/mir_toolkit/mir_cursor_configuration.h'
--- include/client/mir_toolkit/mir_cursor_configuration.h	2014-10-29 06:21:10 +0000
+++ include/client/mir_toolkit/mir_cursor_configuration.h	2014-11-25 19:34:43 +0000
@@ -20,6 +20,8 @@
 
 #include <mir_toolkit/common.h>
 
+#include <stdint.h>
+
 /**
  * Opaque structure containing cursor parameterization. Create with mir_cursor* family.
  * Used with mir_surface_configure_cursor.
@@ -52,6 +54,20 @@
  */
 MirCursorConfiguration *mir_cursor_configuration_from_name(char const* name);
 
+/**
+ * Returns a new MirCursorConfiguration representing a cursor visualizing given pixel data.
+ * 
+ *     \param[in] pixels The unstrided pixel data, of size width*height.
+ *     \param[in] width Width of a line in pixel data.
+ *     \param[in] height Height of pixel data
+ *     \param[in] hotspot_x X displacement in to image on which to place pointer
+ *     \param[in] hotspot_y X displacement in to image on which to place pointer
+ *     \return A cursor parameters object which must be passed
+ *            to_mir_cursor_configuration_destroy
+ */
+MirCursorConfiguration *mir_cursor_configuration_from_argb_8888(uint32_t const* pixels, unsigned width,
+    unsigned height, unsigned hotspot_x, unsigned hotspot_y);
+
 #ifdef __cplusplus
 }
 /**@}*/

=== modified file 'src/client/cursor_configuration.h'
--- src/client/cursor_configuration.h	2014-10-29 06:21:10 +0000
+++ src/client/cursor_configuration.h	2014-11-25 19:34:43 +0000
@@ -19,15 +19,35 @@
 #ifndef MIR_CLIENT_CURSOR_CONFIGURATION_H_
 #define MIR_CLIENT_CURSOR_CONFIGURATION_H_
 
+#include "mir/geometry/size.h"
+#include "mir/geometry/displacement.h"
+
 #include <string>
+#include <tuple>
+#include <memory>
+
+#include <stdint.h>
 
 // Parameters for configuring the apperance and behavior of the system cursor. 
 // Will grow to include cursors specified by raw RGBA data, hotspots, etc...
-struct MirCursorConfiguration 
+class MirCursorConfiguration 
 {
+public:
     MirCursorConfiguration(char const* name);
-
-    std::string name;
+    MirCursorConfiguration(uint32_t const* pixels, mir::geometry::Size const& size, mir::geometry::Displacement const& hotspot);
+
+    std::string cursor_name() const;
+    
+    bool has_pixels() const;
+    std::tuple<uint32_t const*, mir::geometry::Size> pixels() const;
+    mir::geometry::Displacement hotspot() const;
+private:
+    std::string const name;
+
+    std::unique_ptr<uint32_t[]> pixel_data;
+    mir::geometry::Size const pixels_size;
+    
+    mir::geometry::Displacement hotspot_;
 };
 
 #endif // MIR_CLIENT_CURSOR_CONFIGURATION_H_

=== modified file 'src/client/mir_cursor_api.cpp'
--- src/client/mir_cursor_api.cpp	2014-10-29 06:21:10 +0000
+++ src/client/mir_cursor_api.cpp	2014-11-25 19:34:43 +0000
@@ -21,6 +21,76 @@
 
 #include <memory>
 
+#include <string.h>
+
+namespace geom = mir::geometry;
+
+MirCursorConfiguration::MirCursorConfiguration(char const* name) :
+    name{name ? name : std::string()},
+    pixel_data(nullptr)
+{
+}
+
+MirCursorConfiguration::MirCursorConfiguration(uint32_t const* pixels, geom::Size const& size,
+    geom::Displacement const& hotspot) :
+    name(std::string()),
+    pixel_data(new uint32_t[size.width.as_int()*size.height.as_int()]),
+    pixels_size(size),
+    hotspot_(hotspot)
+{
+    memcpy(pixel_data.get(), pixels, size.width.as_int()*size.height.as_int()*sizeof(uint32_t));
+}
+
+std::string MirCursorConfiguration::cursor_name() const
+{
+    return name;
+}
+
+bool MirCursorConfiguration::has_pixels() const
+{
+    return pixel_data != nullptr;
+}
+
+std::tuple<uint32_t const*, geom::Size> MirCursorConfiguration::pixels() const
+{
+    return std::tuple<uint32_t const*, geom::Size>(pixel_data.get(), pixels_size);
+}
+
+geom::Displacement MirCursorConfiguration::hotspot() const
+{
+    return hotspot_;
+}
+
+void mir_cursor_configuration_destroy(MirCursorConfiguration *cursor)
+{
+    delete cursor;
+}
+
+MirCursorConfiguration* mir_cursor_configuration_from_name(char const* name)
+{
+    try 
+    {
+        return new MirCursorConfiguration(name);
+    }
+    catch (...)
+    {
+        return nullptr;
+    }
+}
+
+MirCursorConfiguration* mir_cursor_configuration_from_argb_8888(uint32_t const* pixels, unsigned width,
+    unsigned height, unsigned hotspot_x, unsigned hotspot_y)
+{
+    try 
+    {
+        return new MirCursorConfiguration(pixels, {width, height}, {hotspot_x, hotspot_y});
+    }
+    catch (...)
+    {
+        return nullptr;
+    }
+}
+
 char const *const mir_default_cursor_name = "default";
 char const *const mir_disabled_cursor_name = "disabled";
 char const* const mir_arrow_cursor_name = "arrow";
@@ -36,25 +106,3 @@
 char const* const mir_omnidirectional_resize_cursor_name = "omnidirectional-resize";
 char const* const mir_vsplit_resize_cursor_name = "vsplit-resize";
 char const* const mir_hsplit_resize_cursor_name = "hsplit-resize";
-
-MirCursorConfiguration::MirCursorConfiguration(char const* name) :
-    name{name ? name : std::string()}
-{
-}
-
-void mir_cursor_configuration_destroy(MirCursorConfiguration *cursor)
-{
-    delete cursor;
-}
-
-MirCursorConfiguration* mir_cursor_configuration_from_name(char const* name)
-{
-    try 
-    {
-        return new MirCursorConfiguration(name);
-    }
-    catch (...)
-    {
-        return nullptr;
-    }
-}

=== modified file 'src/client/mir_surface.cpp'
--- src/client/mir_surface.cpp	2014-11-24 03:10:55 +0000
+++ src/client/mir_surface.cpp	2014-11-25 19:34:43 +0000
@@ -414,8 +414,26 @@
     {
         std::unique_lock<decltype(mutex)> lock(mutex);
         setting.mutable_surfaceid()->CopyFrom(surface.id());
-        if (cursor && cursor->name != mir_disabled_cursor_name)
-            setting.set_name(cursor->name.c_str());
+        if (cursor && cursor->has_pixels())
+        {
+            auto image = cursor->pixels();
+            auto pixels = std::get<0>(image);
+            auto size = std::get<1>(image);
+            
+            google::protobuf::RepeatedField<uint32_t> r(pixels,
+                pixels + (size.width.as_int() * size.height.as_int()));
+            setting.mutable_pixels()->Swap(&r);
+            setting.set_width(size.width.as_uint32_t());
+            setting.set_height(size.height.as_uint32_t());
+            
+            auto hotspot = cursor->hotspot();
+            setting.set_hotspot_x(hotspot.dx.as_uint32_t());
+            setting.set_hotspot_y(hotspot.dy.as_uint32_t());
+        }
+        else if (cursor && cursor->cursor_name() != mir_disabled_cursor_name)
+        {
+            setting.set_name(cursor->cursor_name().c_str());
+        }
     }
     
     configure_cursor_wait_handle.expect_result();

=== modified file 'src/protobuf/mir_protobuf.proto'
--- src/protobuf/mir_protobuf.proto	2014-10-29 06:21:10 +0000
+++ src/protobuf/mir_protobuf.proto	2014-11-25 19:34:43 +0000
@@ -184,8 +184,16 @@
 
 message CursorSetting {
   required SurfaceId surfaceid = 1;
-  // No name is interpreted as disabled cursor.
+
   optional string name = 2;
+  // If both pixels and name erroneously included, name will be used over pixels.
+  // If pixels is included width and height must be included.
+  // If both fields are omitted the cursor will be disabled.
+  repeated uint32 pixels = 3;
+  optional uint32 width = 4;
+  optional uint32 height = 5;
+  optional uint32 hotspot_x = 6;
+  optional uint32 hotspot_y = 7;
 }
 
 message SocketFDRequest {

=== modified file 'src/server/frontend/session_mediator.cpp'
--- src/server/frontend/session_mediator.cpp	2014-11-21 12:16:36 +0000
+++ src/server/frontend/session_mediator.cpp	2014-11-25 19:34:43 +0000
@@ -37,6 +37,7 @@
 #include "mir/graphics/pixel_format_utils.h"
 #include "mir/graphics/platform_ipc_operations.h"
 #include "mir/graphics/platform_ipc_package.h"
+#include "mir/graphics/cursor_image.h"
 #include "mir/frontend/client_constants.h"
 #include "mir/frontend/event_sink.h"
 #include "mir/frontend/screencast.h"
@@ -483,6 +484,39 @@
     };
 }
 
+namespace
+{
+struct Argb8888CursorImage : public mg::CursorImage
+{
+    Argb8888CursorImage(uint32_t const* pixels, geom::Size const& size, geom::Displacement hotspot) :
+        pixel_data(new uint32_t[size.width.as_int()*size.height.as_int()]),
+        pixels_size(size),
+        hotspot_(hotspot)
+    {
+        memcpy(pixel_data.get(), pixels, size.width.as_int()*size.height.as_int()*sizeof(uint32_t));
+    }
+
+    void const* as_argb_8888() const
+    {
+        return pixel_data.get();
+    }
+
+    geom::Size size() const
+    {
+        return pixels_size;
+    }
+
+    geom::Displacement hotspot() const
+    {
+        return hotspot_;
+    }
+  
+    std::unique_ptr<uint32_t[]> pixel_data;
+    geom::Size pixels_size;
+    geom::Displacement hotspot_;
+};
+}
+
 void mf::SessionMediator::configure_cursor(
     google::protobuf::RpcController*,
     mir::protobuf::CursorSetting const* cursor_request,
@@ -507,6 +541,28 @@
             auto const& image = cursor_images->image(cursor_request->name(), mi::default_cursor_size);
             surface->set_cursor_image(image);
         }
+        else if (cursor_request->has_width() && cursor_request->has_height())
+        {
+            int width = cursor_request->width();
+            int height = cursor_request->height();
+            int hotspot_x = cursor_request->hotspot_x();
+            int hotspot_y = cursor_request->hotspot_y();
+            
+            auto const& pixels = cursor_request->pixels();
+
+            if (pixels.size() != width*height)
+                BOOST_THROW_EXCEPTION(std::logic_error("cursor image pixel size does not match dimensions"));
+            
+            auto size = geom::Size{width, height};
+            if (size != mi::default_cursor_size)
+                BOOST_THROW_EXCEPTION(std::logic_error("invalid cursor size only 24x24 cursors are currently supported"));
+            if (hotspot_x > width || hotspot_y > height)
+                BOOST_THROW_EXCEPTION(std::logic_error("hotspot past bounds of cursor"));
+
+            auto image = std::make_shared<Argb8888CursorImage>(pixels.data(), geom::Size{width, height},
+                geom::Displacement{hotspot_x, hotspot_y});
+            surface->set_cursor_image(image);
+        }
         else
         {
             surface->set_cursor_image({});

=== modified file 'tests/acceptance-tests/test_client_cursor_api.cpp'
--- tests/acceptance-tests/test_client_cursor_api.cpp	2014-10-29 06:21:10 +0000
+++ tests/acceptance-tests/test_client_cursor_api.cpp	2014-11-25 19:34:43 +0000
@@ -97,6 +97,21 @@
     return cursor_is_named(arg, name);
 }
 
+MATCHER_P5(CursorWithPixels, pixels, width, height, hotspot_x, hotspot_y, "")
+{
+    if (width != arg.size().width.as_uint32_t())
+        return false;
+    if (height != arg.size().height.as_uint32_t())
+        return false;
+    if (hotspot_x != arg.hotspot().dx.as_uint32_t())
+        return false;
+    if (hotspot_y != arg.hotspot().dy.as_uint32_t())
+        return false;
+    if (memcmp(arg.as_argb_8888(), pixels, width*height*sizeof(uint32_t)) != 0)
+        return false;
+    return true;
+}
+
 struct CursorClient
 {
     CursorClient(std::string const& connect_string, std::string const& client_name)
@@ -207,6 +222,35 @@
     std::string const cursor_name;
 };
 
+struct ImageCursorClient : CursorClient
+{
+    ImageCursorClient(
+        std::string const& connect_string,
+        std::string const& client_name,
+        uint32_t const* argb_8888,
+        unsigned width, unsigned height,
+        unsigned hotspot_x, unsigned hotspot_y)
+        : CursorClient{connect_string, client_name},
+          argb_8888(argb_8888),
+          width(width),
+          height(height),
+          hotspot_x(hotspot_x),
+          hotspot_y(hotspot_y)
+    {
+    }
+
+    void setup_cursor(MirSurface* surface) override
+    {
+        auto conf = mir_cursor_configuration_from_argb_8888(argb_8888, width, height, hotspot_x, hotspot_y);
+        mir_wait_for(mir_surface_configure_cursor(surface, conf));
+        mir_cursor_configuration_destroy(conf);
+    }
+
+    std::string const cursor_name;
+    uint32_t const* argb_8888;
+    unsigned width, height, hotspot_x, hotspot_y;
+};
+
 struct TestServerConfiguration : mtf::FakeEventHubServerConfiguration
 {
     std::shared_ptr<mir::scene::PlacementStrategy> the_placement_strategy() override
@@ -403,3 +447,38 @@
 
     client_shutdown_expectations();
 }
+
+TEST_F(TestClientCursorAPI, pixels_from_image_cursor_given_to_server_cursor)
+{
+    using namespace ::testing;
+
+    unsigned width = 24, height = 24, hotspot_x = 7, hotspot_y = 9;
+    uint32_t pixels[24*24];
+    uint32_t black = 0xffffffff, transparent = 0x00000000;
+    for (unsigned i = 0; i < height; i++)
+    {
+        for (unsigned j = 0; j < width; j++)
+        {
+            auto &pixel = pixels[i*width+j];
+            if (i % 2 == 0)
+                pixel = black;
+            else
+                pixel = transparent;
+        }
+    }
+
+
+    test_server_config().client_geometries[client_name_1] =
+        geom::Rectangle{{1, 0}, {1, 1}};
+
+    ImageCursorClient client{new_connection(), client_name_1, pixels, width, height, hotspot_x, hotspot_y};
+    client.run();
+
+    EXPECT_CALL(test_server_config().cursor, show(CursorWithPixels(pixels, width, height, hotspot_x, hotspot_y)))
+        .WillOnce(mt::WakeUp(&expectations_satisfied));
+    fake_event_hub()->synthesize_event(mis::a_motion_event().with_movement(1, 0));
+
+    expectations_satisfied.wait_for_at_most_seconds(5);
+
+    client_shutdown_expectations();
+}