Merge lp:~afrantzis/mir/fix-incorrect-client-color-1172163 into lp:~mir-team/mir/trunk

[Alexandros Frantzis]

client: Properly report a CPU region's pixel format

client: Properly report a CPU region's pixel format

[Alan Griffiths (alan-griffiths)]

LGTM

review: Approve

[Daniel van Vugt (vanvugt)]

Works for me. Bug fixed.

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:659
http://jenkins.qa.ubuntu.com/job/mir-ci/544/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/621
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/504
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-raring-amd64-ci/28
        deb: http://jenkins.qa.ubuntu.com/job/mir-raring-amd64-ci/28/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=precise,flavor=amd64/280

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

review: Approve (continuous-integration)

=== modified file 'examples/multiwin.c'
--- examples/multiwin.c	2013-04-24 07:50:13 +0000
+++ examples/multiwin.c	2013-05-01 23:52:31 +0000
@@ -52,12 +52,6 @@
     switch (format)
     {
     case mir_pixel_format_abgr_8888:
-       /*
-        * XXX
-        * This is right according to the docs in mir_toolkit/c_types.h.
-        * However Mir seems to be mixing up red and blue, rendering this
-        * as ARGB on my system.
-        */
         pixel = 
             (uint32_t)color->a << 24 |
             (uint32_t)color->b << 16 |

=== modified file 'src/client/mir_surface.cpp'
--- src/client/mir_surface.cpp	2013-04-24 05:22:20 +0000
+++ src/client/mir_surface.cpp	2013-05-01 23:52:31 +0000
@@ -109,8 +109,7 @@
     region_out.width = secured_region->width.as_uint32_t();
     region_out.height = secured_region->height.as_uint32_t();
     region_out.stride = secured_region->stride.as_uint32_t();
-    //todo: fix
-    region_out.pixel_format = mir_pixel_format_abgr_8888;
+    region_out.pixel_format = static_cast<MirPixelFormat>(secured_region->format);
 
     region_out.vaddr = secured_region->vaddr.get();
 }
@@ -142,9 +141,7 @@
          better to have a more developed geometry::PixelFormat that can handle this */
 geom::PixelFormat MirSurface::convert_ipc_pf_to_geometry(gp::int32 pf)
 {
-    if (pf == mir_pixel_format_abgr_8888)
-        return geom::PixelFormat::abgr_8888;
-    return geom::PixelFormat::invalid;
+    return static_cast<geom::PixelFormat>(pf);
 }
 
 void MirSurface::process_incoming_buffer()

=== modified file 'tests/unit-tests/client/test_client_mir_surface.cpp'
--- tests/unit-tests/client/test_client_mir_surface.cpp	2013-04-25 09:48:54 +0000
+++ tests/unit-tests/client/test_client_mir_surface.cpp	2013-05-01 23:52:31 +0000
@@ -60,6 +60,7 @@
         width_sent  = 891;
         height_sent = 458;
         pf_sent = mir_pixel_format_abgr_8888;
+        stride_sent = 66;
 
         input_fd = open("/dev/null", O_APPEND);
     }
@@ -108,7 +109,7 @@
         {
             server_package.data[i] = (global_buffer_id + i) * 2;
         }
-        server_package.stride = 66;
+        server_package.stride = stride_sent;
     }
 
     MirBufferPackage server_package;
@@ -116,6 +117,7 @@
     int width_sent;
     int height_sent;
     int pf_sent;
+    int stride_sent;
     
     int input_fd;
 
@@ -558,3 +560,108 @@
     EXPECT_EQ(mir_surface_state_unknown,
               surface->attrib(mir_surface_attrib_state));
 }
+
+namespace
+{
+
+struct StubBuffer : public mcl::ClientBuffer
+{
+    StubBuffer(geom::Size size, geom::Stride stride, geom::PixelFormat pf)
+        : size_{size}, stride_{stride}, pf_{pf}
+    {
+    }
+
+    std::shared_ptr<mcl::MemoryRegion> secure_for_cpu_write()
+    {
+        auto raw = new mcl::MemoryRegion{size_.width,
+                                         size_.height,
+                                         stride_,
+                                         pf_,
+                                         nullptr};
+
+        return std::shared_ptr<mcl::MemoryRegion>(raw);
+    }
+
+    geom::Size size() const { return size_; }
+    geom::Stride stride() const { return stride_; }
+    geom::PixelFormat pixel_format() const { return pf_; }
+    uint32_t age() const { return 0; }
+    void increment_age() {}
+    void mark_as_submitted() {}
+
+    std::shared_ptr<MirBufferPackage> get_buffer_package() const
+    {
+        return std::shared_ptr<MirBufferPackage>();
+    }
+
+    MirNativeBuffer get_native_handle()
+    {
+        return MirNativeBuffer();
+    }
+
+    geom::Size size_;
+    geom::Stride stride_;
+    geom::PixelFormat pf_;
+};
+
+struct StubClientBufferFactory : public mcl::ClientBufferFactory
+{
+    std::shared_ptr<mcl::ClientBuffer> create_buffer(
+        std::shared_ptr<MirBufferPackage> const& package,
+        geom::Size size, geom::PixelFormat pf)
+    {
+        return std::make_shared<StubBuffer>(size,
+                                            geom::Stride{package->stride},
+                                            pf);
+    }
+};
+
+}
+TEST_F(MirClientSurfaceTest, get_cpu_region_returns_correct_data)
+{
+    using namespace testing;
+
+    struct TestDataEntry
+    {
+        int width;
+        int height;
+        int stride;
+        MirPixelFormat pf;
+    };
+
+    std::vector<TestDataEntry> test_data{
+        {100, 200, 300, mir_pixel_format_argb_8888},
+        {101, 201, 301, mir_pixel_format_xrgb_8888},
+        {102, 202, 302, mir_pixel_format_bgr_888}
+    };
+
+    auto stub_buffer_factory = std::make_shared<StubClientBufferFactory>();
+
+    for (auto& td : test_data)
+    {
+        mock_server_tool->width_sent = td.width;
+        mock_server_tool->height_sent = td.height;
+        mock_server_tool->stride_sent = td.stride;
+        mock_server_tool->pf_sent = td.pf;
+
+        auto surface = std::make_shared<MirSurface>(connection.get(),
+                                                    *client_comm_channel,
+                                                    logger,
+                                                    stub_buffer_factory,
+                                                    input_platform,
+                                                    params,
+                                                    &empty_callback,
+                                                    nullptr);
+
+        auto wait_handle = surface->get_create_wait_handle();
+        wait_handle->wait_for_result();
+
+        MirGraphicsRegion region;
+        surface->get_cpu_region(region);
+
+        EXPECT_EQ(mock_server_tool->width_sent, region.width);
+        EXPECT_EQ(mock_server_tool->height_sent, region.height);
+        EXPECT_EQ(mock_server_tool->stride_sent, region.stride);
+        EXPECT_EQ(mock_server_tool->pf_sent, region.pixel_format);
+    }
+}