Merge lp:~kdub/mir/fix-1663062-step2 into lp:mir

[Kevin DuBois]

fix 1663062 for the 1.0 series by uploading ShmBuffers properly in nested servers. This re-enables the software/mesa buffer passthrough pass, while ensuring that lp: #1663062 doesn't happen.

fix 1663062 for the 1.0 series by uploading ShmBuffers properly in nested servers. This re-enables the software/mesa buffer passthrough pass, while ensuring that lp: #1663062 doesn't happen.

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

FAILED: Continuous integration, rev:4030
https://mir-jenkins.ubuntu.com/job/mir-ci/2996/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/3991/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/4077
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/4067
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/4067
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/4067
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4018/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4018
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4018/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4018
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4018/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4018/console
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4018/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/4018
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4018/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/4018/console

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

review: Needs Fixing (continuous-integration)

[Brandon Schaefer (brandontschaefer)]

Small nit

19:09:59 I: mir_demo_client_animated_cursor
19:09:59 I: Smoke testing complete with returncode -1

https://bugs.launchpad.net/mir/+bug/1660889

review: Approve

[Brandon Schaefer (brandontschaefer)]

The nit is in a diff comment, the other comment is about the CI failure!

[Daniel van Vugt (vanvugt)]

Nit:

#include <GLES2/gl2.h>

should be:

#include MIR_SERVER_GL_H

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Another nit:

+ * Authored by: Kevin DuBois <email address hidden>

Since I wrote that function (whose cpp file has Alexandros' name on it still), it would be less inaccurate to just remove the author comment(s).

[Kevin DuBois (kdub)]

Corrected to fit the current system of compile time gl switching.

re authorship, best I can tell, we just we need a name on it for copyright person (and canonical owns the copyright either way).
Should it be the person who created the file, or who wrote the logic behind the header, or who wrote the function header doesn't seem like a fruitful discussion. Removed to avert having to figure out (or come up with) the policy specifics.

[Kevin DuBois (kdub)]

and maybe we don't need names on it at all? (c) Canonical Ltd 2017 at the top might be sufficient

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

PASSED: Continuous integration, rev:4033
https://mir-jenkins.ubuntu.com/job/mir-ci/3018/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/4030
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/4117
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/4107
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/4107
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/4107
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4057
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4057/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/4057
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4057/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4057
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4057/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/4057
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4057/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/4057
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4057/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/4057
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4057/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Great, thanks.

I haven't repeated manual testing but if anything is still broken in this area we'll notice soon enough.

review: Approve

=== added file 'include/platform/mir/graphics/gl_format.h'
--- include/platform/mir/graphics/gl_format.h	1970-01-01 00:00:00 +0000
+++ include/platform/mir/graphics/gl_format.h	2017-02-20 13:18:16 +0000
@@ -0,0 +1,31 @@
+/*
+ * Copyright © 2017 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#ifndef MIR_GRAPHICS_COMMON_GL_FORMAT_H_
+#define MIR_GRAPHICS_COMMON_GL_FORMAT_H_
+#include MIR_SERVER_GL_H
+#include "mir_toolkit/common.h"
+
+namespace mir
+{
+namespace graphics
+{
+bool get_gl_pixel_format(MirPixelFormat mir_format,
+                         GLenum& gl_format, GLenum& gl_type);
+}
+}
+#endif /* MIR_GRAPHICS_COMMON_GL_FORMAT_H_ */

=== modified file 'src/platforms/common/server/shm_buffer.cpp'
--- src/platforms/common/server/shm_buffer.cpp	2017-01-18 02:29:37 +0000
+++ src/platforms/common/server/shm_buffer.cpp	2017-02-20 13:18:16 +0000
@@ -17,9 +17,11 @@
  *   Alexandros Frantzis <[e-mail]>
  */
 
+#include "mir/graphics/gl_format.h"
 #include "shm_file.h"
 #include "shm_buffer.h"
 #include "buffer_texture_binder.h"
+
 #include MIR_SERVER_GL_H
 #include MIR_SERVER_GLEXT_H
 
@@ -34,9 +36,7 @@
 namespace mgc = mir::graphics::common;
 namespace geom = mir::geometry;
 
-namespace {
-
-bool get_gl_pixel_format(MirPixelFormat mir_format,
+bool mg::get_gl_pixel_format(MirPixelFormat mir_format,
                          GLenum& gl_format, GLenum& gl_type)
 {
 #if __BYTE_ORDER == __LITTLE_ENDIAN
@@ -84,12 +84,10 @@
     return gl_format != GL_INVALID_ENUM && gl_type != GL_INVALID_ENUM;
 }
 
-} // anonymous namespace
-
 bool mgc::ShmBuffer::supports(MirPixelFormat mir_format)
 {
     GLenum gl_format, gl_type;
-    return get_gl_pixel_format(mir_format, gl_format, gl_type);
+    return mg::get_gl_pixel_format(mir_format, gl_format, gl_type);
 }
 
 mgc::ShmBuffer::ShmBuffer(
@@ -127,7 +125,7 @@
 {
     GLenum format, type;
 
-    if (get_gl_pixel_format(pixel_format_, format, type))
+    if (mg::get_gl_pixel_format(pixel_format_, format, type))
     {
         /*
          * All existing Mir logic assumes that strides are whole multiples of

=== modified file 'src/server/CMakeLists.txt'
--- src/server/CMakeLists.txt	2017-02-15 07:38:33 +0000
+++ src/server/CMakeLists.txt	2017-02-20 13:18:16 +0000
@@ -110,6 +110,7 @@
   mirprotobuf
   mircookie
 
+  server_platform_common
   ${GLog_LIBRARY}
   ${GFlags_LIBRARY}
   ${EGL_LDFLAGS} ${EGL_LIBRARIES}

=== modified file 'src/server/graphics/nested/CMakeLists.txt'
--- src/server/graphics/nested/CMakeLists.txt	2017-02-17 07:03:56 +0000
+++ src/server/graphics/nested/CMakeLists.txt	2017-02-20 13:18:16 +0000
@@ -1,6 +1,7 @@
 include_directories(
   ${PROJECT_SOURCE_DIR}/include/renderers/gl
   ${PROJECT_SOURCE_DIR}/include/platforms/mesa
+  ${PROJECT_SOURCE_DIR}/include/platforms/common
 )
 
 add_library(

=== modified file 'src/server/graphics/nested/buffer.cpp'
--- src/server/graphics/nested/buffer.cpp	2017-02-15 07:38:33 +0000
+++ src/server/graphics/nested/buffer.cpp	2017-02-20 13:18:16 +0000
@@ -21,6 +21,7 @@
 #include "mir/renderer/gl/texture_target.h"
 #include "mir/graphics/egl_extensions.h"
 #include "mir/graphics/buffer_ipc_message.h"
+#include "mir/graphics/gl_format.h"
 #include "mir_toolkit/mir_buffer.h"
 #include "host_connection.h"
 #include "buffer.h"
@@ -47,10 +48,8 @@
 {
 public:
     TextureAccess(
-        mgn::Buffer& buffer,
         std::shared_ptr<mgn::NativeBuffer> const& native_buffer,
         std::shared_ptr<mgn::HostConnection> const& connection) :
-        buffer(buffer),
         native_buffer(native_buffer),
         connection(connection)
     {
@@ -108,11 +107,9 @@
         bind();
     }
 
-protected:
-    mgn::Buffer& buffer;
+private:
     std::shared_ptr<mgn::NativeBuffer> const native_buffer;
     std::shared_ptr<mgn::HostConnection> const connection;
-private:
     mg::EGLExtensions extensions;
     typedef std::pair<EGLDisplay, EGLContext> ImageResources;
     typedef std::unique_ptr<EGLImageKHR, std::function<void(EGLImageKHR*)>> EGLImageUPtr;
@@ -120,17 +117,20 @@
 };
 
 class PixelAndTextureAccess :
-    public TextureAccess,
-    public mrs::PixelSource
+    public mg::NativeBufferBase,
+    public mrs::PixelSource,
+    public mrg::TextureSource
 {
 public:
     PixelAndTextureAccess(
         mgn::Buffer& buffer,
-        std::shared_ptr<mgn::NativeBuffer> const& native_buffer,
-        std::shared_ptr<mgn::HostConnection> const& connection) :
-        TextureAccess(buffer, native_buffer, connection),
+        std::shared_ptr<mgn::NativeBuffer> const& native_buffer) :
+        buffer(buffer),
+        native_buffer(native_buffer),
         stride_(geom::Stride{native_buffer->get_graphics_region()->stride})
     {
+        if (!mg::get_gl_pixel_format(buffer.pixel_format(), format, type))
+            BOOST_THROW_EXCEPTION(std::logic_error("buffer cannot be used as texture"));
     }
 
     void write(unsigned char const* pixels, size_t pixel_size) override
@@ -163,17 +163,39 @@
         return stride_;
     }
 
+    void bind() override
+    {
+        glPixelStorei(GL_UNPACK_ALIGNMENT, 1);
+        glTexImage2D(
+            GL_TEXTURE_2D, 0, format,
+            buffer.size().width.as_int(), buffer.size().height.as_int(),
+            0, format, type, native_buffer->get_graphics_region()->vaddr);
+    }
+
+    void gl_bind_to_texture() override
+    {
+        bind();
+    }
+
+    void secure_for_render() override
+    {
+    }
+
 private:
+    mgn::Buffer& buffer;
+    std::shared_ptr<mgn::NativeBuffer> const native_buffer;
     geom::Stride const stride_;
+    GLenum format;
+    GLenum type;
 };
 }
 
 std::shared_ptr<mg::NativeBufferBase> mgn::Buffer::create_native_base(mg::BufferUsage const usage)
 {
     if (usage == mg::BufferUsage::software)
-        return std::make_shared<PixelAndTextureAccess>(*this, buffer, connection);
+        return std::make_shared<PixelAndTextureAccess>(*this, buffer);
     else if (usage == mg::BufferUsage::hardware)
-        return std::make_shared<TextureAccess>(*this, buffer, connection);
+        return std::make_shared<TextureAccess>(buffer, connection);
     else
         BOOST_THROW_EXCEPTION(std::invalid_argument("usage not supported when creating nested::Buffer"));
 }
@@ -192,7 +214,7 @@
     geom::Size size, uint32_t native_format, uint32_t native_flags) :
     connection(connection),
     buffer(connection->create_buffer(size, native_format, native_flags)),
-    native_base(std::make_shared<TextureAccess>(*this, buffer, connection))
+    native_base(std::make_shared<TextureAccess>(buffer, connection))
 {
 }
 
@@ -202,7 +224,7 @@
     MirPixelFormat format) :
     connection(connection),
     buffer(connection->create_buffer(size, format)),
-    native_base(std::make_shared<PixelAndTextureAccess>(*this, buffer, connection))
+    native_base(std::make_shared<PixelAndTextureAccess>(*this, buffer))
 {
 }
 

=== modified file 'src/server/graphics/nested/mir_client_host_connection.cpp'
--- src/server/graphics/nested/mir_client_host_connection.cpp	2017-02-17 07:03:56 +0000
+++ src/server/graphics/nested/mir_client_host_connection.cpp	2017-02-20 13:18:16 +0000
@@ -685,13 +685,10 @@
     return std::make_unique<SurfaceSpec>(mir_connection);
 }
 
-bool mgn::MirClientHostConnection::supports_passthrough(mg::BufferUsage usage)
+bool mgn::MirClientHostConnection::supports_passthrough(mg::BufferUsage)
 {
-    //FIXME: ShmBuffers currently don't upload properly. The logic here uses
-    //       passthrough where supported (ANativeWindowBuffer and gbm_bo)
-    //       (LP: #1663062 for more details)
-    return (mir_extension_android_buffer_v1(mir_connection) || 
-           ((mir_extension_gbm_buffer_v1(mir_connection) && usage == mg::BufferUsage::hardware)));
+    return mir_extension_android_buffer_v1(mir_connection) || 
+           mir_extension_gbm_buffer_v1(mir_connection);
 }
 
 void mgn::MirClientHostConnection::apply_input_configuration(MirInputConfig const* config)

=== modified file 'src/server/graphics/nested/platform.cpp'
--- src/server/graphics/nested/platform.cpp	2017-02-15 07:38:33 +0000
+++ src/server/graphics/nested/platform.cpp	2017-02-20 13:18:16 +0000
@@ -75,10 +75,7 @@
 
     std::shared_ptr<mg::Buffer> alloc_buffer(mg::BufferProperties const& properties) override
     {
-        if (passthrough_candidate(properties.size, properties.usage))
-            return std::make_shared<mgn::Buffer>(connection, properties);
-        else
-            return guest_allocator->alloc_buffer(properties);
+        return guest_allocator->alloc_buffer(properties);
     }
 
     std::shared_ptr<mg::Buffer> alloc_buffer(

=== modified file 'tests/unit-tests/platforms/nested/test_buffer.cpp'
--- tests/unit-tests/platforms/nested/test_buffer.cpp	2017-02-15 07:38:33 +0000
+++ tests/unit-tests/platforms/nested/test_buffer.cpp	2017-02-20 13:18:16 +0000
@@ -234,6 +234,30 @@
     texture_source->gl_bind_to_texture();
 }
 
+TEST_F(NestedBuffer, binds_to_texture_gl_buffer)
+{
+    auto pf = sw_properties.format;
+    auto format = GL_RGBA;
+    auto type = GL_UNSIGNED_BYTE;
+
+    mgn::Buffer buffer(mt::fake_shared(mock_connection), size, pf);
+
+    auto native_base = buffer.native_buffer_base();
+    ASSERT_THAT(native_base, Ne(nullptr));
+    auto texture_source = dynamic_cast<mir::renderer::gl::TextureSource*>(native_base);
+    ASSERT_THAT(texture_source, Ne(nullptr));
+
+    EXPECT_CALL(mock_egl, glEGLImageTargetTexture2DOES(GL_TEXTURE_2D, _))
+        .Times(0);
+
+    EXPECT_CALL(mock_gl, glPixelStorei(GL_UNPACK_ALIGNMENT, 1));
+    EXPECT_CALL(mock_gl, glTexImage2D(
+            GL_TEXTURE_2D, 0, format,
+            buffer.size().width.as_int(), buffer.size().height.as_int(),
+            0, format, type, _));
+    texture_source->gl_bind_to_texture();
+}
+
 TEST_F(NestedBuffer, just_makes_one_bind_per_display_context_pair)
 {
     ON_CALL(*client_buffer, egl_image_creation_hints())