Merge lp:~kdub/mir/mir-buffer-get-stride into lp:mir

[Kevin DuBois]

client: add mir_buffer_get_stride() to the MirBuffer api.

client: add mir_buffer_get_stride() to the MirBuffer api.

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

FAILED: Continuous integration, rev:3656
https://mir-jenkins.ubuntu.com/job/mir-ci/1475/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/1827/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/1881
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/1872
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/1872
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/1872
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/1851
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/1851/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/1851
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/1851/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/1851
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/1851/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1851
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1851/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/1851
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/1851/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/1851/console

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

review: Needs Fixing (continuous-integration)

[Kevin DuBois (kdub)]

bug 1613352

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

PASSED: Continuous integration, rev:3656
https://mir-jenkins.ubuntu.com/job/mir-ci/1477/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/1829
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/1883
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/1874
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/1874
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/1874
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/1853
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/1853/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/1853
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/1853/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/1853
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/1853/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1853
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1853/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/1853
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/1853/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/1853
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/1853/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Does this make sense? Stride is usually only meaningful to clients using linear software buffers. And they already get the stride from MirGraphicsRegion.

review: Needs Information

[Kevin DuBois (kdub)]

The driver for the addition is https://code.launchpad.net/~kdub/mir/nested-buffer/+merge/303251, where we have a need to know the stride to implement mg::Buffer.

Stride could be hidden behind linear/software buffers only, but as things stand now, its a core buffer property thats used in different parts of the code.

[Cemil Azizoglu (cemil-azizoglu)]

ok

review: Approve

[Chris Halse Rogers (raof)]

Not blocking, but how much effort would it be to do the right thing here and remove stride(), write() and read() from the mg::Buffer API and move it to the appropriate NativeBuffer interfaces? These things really only apply to (a currently hypothetical) mg::SoftwareTexture.

review: Needs Information

[Chris Halse Rogers (raof)]

A quick search of where those functions are used suggests that we have exactly two users of write() and *no* users of stride()‽

[Daniel van Vugt (vanvugt)]

Theoretically stride() only really is applicable to ShmBuffer and even that doesn't use the stride() interface, just provides it.

The reason is that an arbitrary stride is not supported by OpenGL when uploading textures. We resolve this by calling glPixelStorei to tell GL to round to single pixels, so that everyone then agrees stride==width*bpp in ShmBuffer.

I don't mind it existing, but it seems to be an Android-specific requirement.

review: Abstain

[Kevin DuBois (kdub)]

moving stride() from mg::Buffer to mir::renderer::software::PixelSource is the right thing to do. It could be a bit of work (seems a bit entrenched though in our ipc and platform-specific code), will spike around moving it.

[Daniel van Vugt (vanvugt)]

Absolutely. I didn't realise that a software renderer was coming. That's definitely a use case requiring stride().

review: Approve

[Chris Halse Rogers (raof)]

A putative software renderer will dynamically cast to PixelSource and use that interface; it won't use mg::Buffer::stride() (which, like read() and write() don't make sense for non-software buffers).

[Chris Halse Rogers (raof)]

So I *think* this is work-in-progress, pending Kevin's PixelSource spike that will completely remove the need for it?

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

FAILED: Autolanding.
More details in the following jenkins job:
https://mir-jenkins.ubuntu.com/job/mir-autolanding/519/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/1867/console
    None: https://mir-jenkins.ubuntu.com/job/generic-land-mp/553/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/1921
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/1912
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/1912
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/1912
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/1891/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/1891
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/1891/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/1891/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1891/console
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1891/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/1891
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/1891/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/1891
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/1891/artifact/output/*zip*/output.zip

review: Needs Fixing (continuous-integration)

[Kevin DuBois (kdub)]

> So I *think* this is work-in-progress, pending Kevin's PixelSource spike that
> will completely remove the need for it?
I think it can be rejected

=== modified file 'src/client/buffer.cpp'
--- src/client/buffer.cpp	2016-08-01 07:24:32 +0000
+++ src/client/buffer.cpp	2016-08-17 14:06:24 +0000
@@ -129,6 +129,12 @@
 {
     return buffer->size();
 }
+
+mir::geometry::Stride mcl::Buffer::stride() const
+{
+    return buffer->stride();
+}
+
 std::shared_ptr<mcl::ClientBuffer> mcl::Buffer::client_buffer() const
 {
     return buffer;

=== modified file 'src/client/buffer.h'
--- src/client/buffer.h	2016-08-01 07:24:32 +0000
+++ src/client/buffer.h	2016-08-17 14:06:24 +0000
@@ -60,6 +60,7 @@
     MirBufferUsage buffer_usage() const override;
     MirPixelFormat pixel_format() const override;
     geometry::Size size() const override;
+    geometry::Stride stride() const override;
 
     MirConnection* allocating_connection() const override;
 

=== modified file 'src/client/error_buffer.cpp'
--- src/client/error_buffer.cpp	2016-08-01 07:24:32 +0000
+++ src/client/error_buffer.cpp	2016-08-17 14:06:24 +0000
@@ -74,5 +74,6 @@
 MirBufferUsage mcl::ErrorBuffer::buffer_usage() const THROW_EXCEPTION
 MirPixelFormat mcl::ErrorBuffer::pixel_format() const THROW_EXCEPTION
 geom::Size mcl::ErrorBuffer::size() const THROW_EXCEPTION
+geom::Stride mcl::ErrorBuffer::stride() const THROW_EXCEPTION
 void mcl::ErrorBuffer::increment_age() THROW_EXCEPTION
 void mcl::ErrorBuffer::set_callback(mir_buffer_callback, void*) THROW_EXCEPTION

=== modified file 'src/client/error_buffer.h'
--- src/client/error_buffer.h	2016-08-01 07:24:32 +0000
+++ src/client/error_buffer.h	2016-08-17 14:06:24 +0000
@@ -45,6 +45,7 @@
     MirBufferUsage buffer_usage() const override;
     MirPixelFormat pixel_format() const override;
     geometry::Size size() const override;
+    geometry::Stride stride() const override;
     MirConnection* allocating_connection() const override;
     void increment_age() override;
     void set_callback(mir_buffer_callback callback, void* context) override;

=== modified file 'src/client/mir_buffer.h'
--- src/client/mir_buffer.h	2016-08-01 07:24:32 +0000
+++ src/client/mir_buffer.h	2016-08-17 14:06:24 +0000
@@ -50,6 +50,8 @@
     virtual MirBufferUsage buffer_usage() const = 0;
     virtual MirPixelFormat pixel_format() const = 0;
     virtual geometry::Size size() const = 0;
+    virtual geometry::Stride stride() const = 0;
+
     virtual MirConnection* allocating_connection() const = 0;
     virtual void increment_age() = 0;
     virtual bool valid() const = 0;

=== modified file 'src/client/mir_buffer_api.cpp'
--- src/client/mir_buffer_api.cpp	2016-08-01 07:24:32 +0000
+++ src/client/mir_buffer_api.cpp	2016-08-17 14:06:24 +0000
@@ -142,6 +142,19 @@
     return 0;
 }
 
+unsigned int mir_buffer_get_stride(MirBuffer* b)
+try
+{
+    mir::require(b);
+    auto buffer = reinterpret_cast<mcl::MirBuffer*>(b);
+    return buffer->stride().as_uint32_t();
+}
+catch (std::exception const& ex)
+{
+    MIR_LOG_UNCAUGHT_EXCEPTION(ex);
+    return 0;
+}
+
 unsigned int mir_buffer_get_height(MirBuffer* b)
 try
 {

=== modified file 'src/client/symbols.map'
--- src/client/symbols.map	2016-08-09 06:44:52 +0000
+++ src/client/symbols.map	2016-08-17 14:06:24 +0000
@@ -381,6 +381,7 @@
     mir_buffer_get_height;
     mir_buffer_get_pixel_format;
     mir_buffer_get_buffer_usage;
+    mir_buffer_get_stride;
     mir_buffer_is_valid;
     mir_buffer_get_error_message;
   local:

=== modified file 'src/include/client/mir_toolkit/mir_buffer.h'
--- src/include/client/mir_toolkit/mir_buffer.h	2016-08-01 07:24:32 +0000
+++ src/include/client/mir_toolkit/mir_buffer.h	2016-08-17 14:06:24 +0000
@@ -170,6 +170,13 @@
  **/
 unsigned int mir_buffer_get_height(MirBuffer* buffer);
 
+/** Retrieve the stride of the buffer in bytes.
+ *
+ *   \param [in] buffer   The buffer
+ *   \return              The stride of the buffer in bytes
+ **/
+unsigned int mir_buffer_get_stride(MirBuffer* buffer);
+
 /** Retrieve the pixel format of the buffer.
  *
  *   \param [in] buffer   The buffer

=== modified file 'tests/acceptance-tests/throwback/test_presentation_chain.cpp'
--- tests/acceptance-tests/throwback/test_presentation_chain.cpp	2016-07-05 12:40:56 +0000
+++ tests/acceptance-tests/throwback/test_presentation_chain.cpp	2016-08-17 14:06:24 +0000
@@ -343,6 +343,7 @@
     geom::Height height { 33 };
     auto format = mir_pixel_format_abgr_8888;
     auto usage = mir_buffer_usage_software;
+    auto stride = geom::Stride { width.as_int() * MIR_BYTES_PER_PIXEL(format) };
 
     SurfaceWithChainFromStart surface(connection, size, pf);
     mir_connection_allocate_buffer(
@@ -354,6 +355,7 @@
     EXPECT_THAT(mir_buffer_get_height(buffer), Eq(height.as_uint32_t()));
     EXPECT_THAT(mir_buffer_get_buffer_usage(buffer), Eq(usage));
     EXPECT_THAT(mir_buffer_get_pixel_format(buffer), Eq(format));
+    EXPECT_THAT(mir_buffer_get_stride(buffer), Eq(stride.as_uint32_t()));
 }
 
 TEST_F(PresentationChain, can_check_valid_buffers)

=== modified file 'tests/include/mir/test/doubles/mock_mir_buffer.h'
--- tests/include/mir/test/doubles/mock_mir_buffer.h	2016-08-01 07:24:32 +0000
+++ tests/include/mir/test/doubles/mock_mir_buffer.h	2016-08-17 14:06:24 +0000
@@ -53,6 +53,7 @@
     MOCK_CONST_METHOD0(buffer_usage, MirBufferUsage());
     MOCK_CONST_METHOD0(pixel_format, MirPixelFormat());
     MOCK_CONST_METHOD0(size, geometry::Size());
+    MOCK_CONST_METHOD0(stride, geometry::Stride());
     MOCK_CONST_METHOD0(allocating_connection, MirConnection*());
     MOCK_METHOD0(increment_age, void());
     MOCK_CONST_METHOD0(valid, bool());

=== modified file 'tests/unit-tests/client/test_mir_buffer.cpp'
--- tests/unit-tests/client/test_mir_buffer.cpp	2016-06-24 11:41:37 +0000
+++ tests/unit-tests/client/test_mir_buffer.cpp	2016-08-17 14:06:24 +0000
@@ -43,6 +43,8 @@
             .WillByDefault(Return(geom::Size{width, height}));
         ON_CALL(*mock_client_buffer, pixel_format())
             .WillByDefault(Return(format)); 
+        ON_CALL(*mock_client_buffer, stride())
+            .WillByDefault(Return(stride));
     }
     MirGraphicsRegion region;
     int buffer_id { 32 };
@@ -228,3 +230,9 @@
     EXPECT_THAT(buffer.size().height, Eq(height));
     EXPECT_THAT(buffer.size().width, Eq(width));
 }
+
+TEST_F(MirBufferTest, returns_correct_stride)
+{
+    mcl::Buffer buffer(cb, nullptr, buffer_id, mock_client_buffer, nullptr, usage);
+    EXPECT_THAT(buffer.stride(), Eq(stride));
+}