Merge lp:~vanvugt/mir/fix-1116452 into lp:~mir-team/mir/trunk

[Daniel van Vugt]

Resolve the two differing interpretations of "stride" which were causing
software surfaces to be distorted on Android. (LP: #1116452)

Just make sure that "stride" in any Android structure is a number of pixels
and in Mir types it is always a number of bytes.

[Alan Griffiths (alan-griffiths)]

Instead of using base types perhaps we should be using boost.Units. "Scaled Base Units" solves this conversion problem and give us type safety.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:736
http://jenkins.qa.ubuntu.com/job/mir-ci/712/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/881
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/763
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-raring-amd64-ci/197
        deb: http://jenkins.qa.ubuntu.com/job/mir-raring-amd64-ci/197/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/401

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

Looks sensible. Not sure how to test all the possible cases though.

review: Approve

[Alexandros Frantzis (afrantzis)]

It would be cleaner if the translation from pixel to byte units was done inside the android subystem in the server, which is the source of the stride values.

mga::AndroidBuffer::stride() seems like a good place to perform the translation.

review: Needs Fixing

[Alexandros Frantzis (afrantzis)]

> It would be cleaner if the translation from pixel to byte units was done
> inside the android subystem in the server, which is the source of the stride
> values.
>
> mga::AndroidBuffer::stride() seems like a good place to perform the
> translation.

Sorry, misread the diff. Looks good.

review: Approve

[Kevin DuBois (kdub)]

seems ok.

review: Approve

=== modified file 'include/shared/mir/geometry/pixel_format.h'
--- include/shared/mir/geometry/pixel_format.h	2013-04-24 05:22:20 +0000
+++ include/shared/mir/geometry/pixel_format.h	2013-06-12 09:48:51 +0000
@@ -36,6 +36,12 @@
     xrgb_8888,
     bgr_888
 };
+
+static inline size_t bytes_per_pixel(PixelFormat fmt)
+{
+    return (fmt == PixelFormat::bgr_888) ? 3 : 4;
+}
+
 }
 }
 

=== modified file 'src/client/android/android_client_buffer.cpp'
--- src/client/android/android_client_buffer.cpp	2013-05-03 22:53:42 +0000
+++ src/client/android/android_client_buffer.cpp	2013-06-12 09:48:51 +0000
@@ -48,7 +48,8 @@
 {
     native_window_buffer->height = static_cast<int32_t>(rect.size.height.as_uint32_t());
     native_window_buffer->width =  static_cast<int32_t>(rect.size.width.as_uint32_t());
-    native_window_buffer->stride = creation_package->stride;
+    native_window_buffer->stride = creation_package->stride /
+                                   geom::bytes_per_pixel(buffer_pf);
     native_window_buffer->usage = GRALLOC_USAGE_HW_TEXTURE | GRALLOC_USAGE_HW_RENDER;
 
     native_window_buffer->handle = native_handle.get();

=== modified file 'src/server/graphics/android/buffer.cpp'
--- src/server/graphics/android/buffer.cpp	2013-06-04 14:23:56 +0000
+++ src/server/graphics/android/buffer.cpp	2013-06-12 09:48:51 +0000
@@ -65,7 +65,8 @@
 
 geom::Stride mga::Buffer::stride() const
 {
-    return geom::Stride{native_buffer->stride};
+    return geom::Stride{native_buffer->stride *
+                        geom::bytes_per_pixel(pixel_format())};
 }
 
 geom::PixelFormat mga::Buffer::pixel_format() const

=== modified file 'tests/unit-tests/client/android/test_client_android_buffer.cpp'
--- tests/unit-tests/client/android/test_client_android_buffer.cpp	2013-05-02 19:05:14 +0000
+++ tests/unit-tests/client/android/test_client_android_buffer.cpp	2013-06-12 09:48:51 +0000
@@ -320,7 +320,9 @@
 
     auto native_handle = buffer->native_buffer_handle();
     ASSERT_NE(nullptr, native_handle);
-    EXPECT_EQ(static_cast<int32_t>(stride.as_uint32_t()), native_handle->stride);
+    int32_t const expected_stride_in_pixels =
+        static_cast<int32_t>(stride.as_uint32_t() / geom::bytes_per_pixel(pf));
+    EXPECT_EQ(expected_stride_in_pixels, native_handle->stride);
 }
 
 TEST_F(ClientAndroidBufferTest, buffer_packs_anativewindowbuffer_refcounters_set)

=== modified file 'tests/unit-tests/graphics/android/test_buffer.cpp'
--- tests/unit-tests/graphics/android/test_buffer.cpp	2013-05-21 21:43:25 +0000
+++ tests/unit-tests/graphics/android/test_buffer.cpp	2013-06-12 09:48:51 +0000
@@ -99,7 +99,8 @@
 {
     using namespace testing;
 
-    geom::Stride expected_stride{mock_buffer_handle->stride};
+    geom::Stride expected_stride{mock_buffer_handle->stride *
+                                 geom::bytes_per_pixel(pf)};
     mga::Buffer buffer(mock_alloc_device, size, pf, default_use);
     EXPECT_EQ(expected_stride, buffer.stride());
 }