Merge lp:~raof/mir/fix-drm-buffer-multiple-attach into lp:mir

[Chris Halse Rogers]

mgm::BufferAllocator: Handle a wl_buffer being attached multiple times.

As with the WlShmBuffer, it's valid for a wl_buffer to be wl_surface.attach()ed to multiple wl_surface-s at the same time. If a buffer is attached twice then the buffer.release() event needs to be sent exactly once, when the compositor is finished with *all* submissions.

So, do what we do with WlShmBuffer - associate a shared_ptr<WaylandBuffer> with each wl_buffer, and hand out references to it rather than creating a new WaylandBuffer on each (committed) attach().

[Alan Griffiths (alan-griffiths)]

Nice

review: Approve

[Gerry Boland (gerboland)]

lgtm

review: Approve

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

FAILED: Continuous integration, rev:4258
https://mir-jenkins.ubuntu.com/job/mir-ci/3674/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/5033/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/5269
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=artful/5257
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/5257
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/5257
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=artful/5076/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/5076
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/5076/artifact/output/*zip*/output.zip
    ABORTED: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=artful/5076/console
    ABORTED: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/5076/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=artful/5076
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=artful/5076/artifact/output/*zip*/output.zip
    ABORTED: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=zesty/5076/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/5076
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/5076/artifact/output/*zip*/output.zip

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

review: Needs Fixing (continuous-integration)

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

review: Approve (continuous-integration)

=== modified file 'include/platform/mir/graphics/wayland_allocator.h'
--- include/platform/mir/graphics/wayland_allocator.h	2017-09-01 07:00:55 +0000
+++ include/platform/mir/graphics/wayland_allocator.h	2017-09-21 08:03:22 +0000
@@ -37,7 +37,7 @@
     virtual ~WaylandAllocator() = default;
 
     virtual void bind_display(wl_display* display) = 0;
-    virtual std::unique_ptr<Buffer> buffer_from_resource (wl_resource* buffer, std::function<void ()>&& on_consumed) = 0;
+    virtual std::shared_ptr<Buffer> buffer_from_resource (wl_resource* buffer, std::function<void ()>&& on_consumed) = 0;
 };
 }
 }

=== modified file 'src/platforms/mesa/server/buffer_allocator.cpp'
--- src/platforms/mesa/server/buffer_allocator.cpp	2017-09-01 07:00:55 +0000
+++ src/platforms/mesa/server/buffer_allocator.cpp	2017-09-21 08:03:22 +0000
@@ -332,66 +332,54 @@
     public mir::renderer::gl::TextureSource
 {
 public:
-    WaylandBuffer(
+    static std::shared_ptr<mg::Buffer> mir_buffer_from_wl_buffer(
         EGLDisplay dpy,
         wl_resource* buffer,
         std::shared_ptr<mg::EGLExtensions> const& extensions,
         std::function<void()>&& on_consumed)
-        : buffer{buffer},
-          dpy{dpy},
-          egl_image{EGL_NO_IMAGE_KHR},
-          extensions{extensions},
-          on_consumed{std::move(on_consumed)}
     {
+        std::shared_ptr<WaylandBuffer> mir_buffer;
+        DestructionShim* shim;
+
         if (auto notifier = wl_resource_get_destroy_listener(buffer, &on_buffer_destroyed))
         {
-            DestructionShim* shim;
-
+            // We've already constructed a shim for this buffer, update it.
             shim = wl_container_of(notifier, shim, destruction_listener);
 
-            if (shim->associated_buffer)
-                BOOST_THROW_EXCEPTION(std::logic_error("Attempt to associate a single wl_buffer with multiple WaylandBuffer wrappers"));
-
-            shim->associated_buffer = this;
-            buffer_mutex = shim->mutex;
+            if (!(mir_buffer = shim->associated_buffer.lock()))
+            {
+                /*
+                 * We've seen this wl_buffer before, but all the WaylandBuffers associated with it
+                 * have been destroyed.
+                 *
+                 * Recreate a new WaylandBuffer to track the new compositor lifetime.
+                 */
+                mir_buffer = std::shared_ptr<WaylandBuffer>{
+                    new WaylandBuffer{
+                        dpy,
+                        buffer,
+                        extensions,
+                        std::move(on_consumed)}};
+                shim->associated_buffer = mir_buffer;
+            }
         }
         else
         {
-            auto shim = new DestructionShim;
+            mir_buffer = std::shared_ptr<WaylandBuffer>{
+                new WaylandBuffer{
+                    dpy,
+                    buffer,
+                    extensions,
+                    std::move(on_consumed)}};
+            shim = new DestructionShim;
             shim->destruction_listener.notify = &on_buffer_destroyed;
-            shim->associated_buffer = this;
-            buffer_mutex = shim->mutex;
+            shim->associated_buffer = mir_buffer;
 
             wl_resource_add_destroy_listener(buffer, &shim->destruction_listener);
         }
 
-        if (extensions->wayland->eglQueryWaylandBufferWL(dpy, buffer, EGL_WIDTH, &width) == EGL_FALSE)
-        {
-            BOOST_THROW_EXCEPTION(mg::egl_error("Failed to query WaylandAllocator buffer width"));
-        }
-        if (extensions->wayland->eglQueryWaylandBufferWL(dpy, buffer, EGL_HEIGHT, &height) == EGL_FALSE)
-        {
-            BOOST_THROW_EXCEPTION(mg::egl_error("Failed to query WaylandAllocator buffer height"));
-        }
-
-        EGLint texture_format;
-        if (!extensions->wayland->eglQueryWaylandBufferWL(dpy, buffer, EGL_TEXTURE_FORMAT, &texture_format))
-        {
-            BOOST_THROW_EXCEPTION(mg::egl_error("Failed to query WL buffer format"));
-        }
-
-        if (texture_format == EGL_TEXTURE_RGB)
-        {
-            format = mir_pixel_format_xrgb_8888;
-        }
-        else if (texture_format == EGL_TEXTURE_RGBA)
-        {
-            format = mir_pixel_format_argb_8888;
-        }
-        else
-        {
-            BOOST_THROW_EXCEPTION((std::invalid_argument{"YUV buffers are unimplemented"}));
-        }
+        mir_buffer->buffer_mutex = shim->mutex;
+        return mir_buffer;
     }
 
     ~WaylandBuffer()
@@ -403,12 +391,6 @@
         if (buffer)
         {
             wl_resource_queue_event(buffer, WL_BUFFER_RELEASE);
-            auto notifier = wl_resource_get_destroy_listener(buffer, &on_buffer_destroyed);
-            DestructionShim* shim;
-
-            shim = wl_container_of(notifier, shim, destruction_listener);
-
-            shim->associated_buffer = nullptr;
         }
     }
 
@@ -477,6 +459,46 @@
     }
 
 private:
+    WaylandBuffer(
+        EGLDisplay dpy,
+        wl_resource* buffer,
+        std::shared_ptr<mg::EGLExtensions> const& extensions,
+        std::function<void()>&& on_consumed)
+        : buffer{buffer},
+        dpy{dpy},
+        egl_image{EGL_NO_IMAGE_KHR},
+        extensions{extensions},
+        on_consumed{std::move(on_consumed)}
+    {
+        if (extensions->wayland->eglQueryWaylandBufferWL(dpy, buffer, EGL_WIDTH, &width) == EGL_FALSE)
+        {
+            BOOST_THROW_EXCEPTION(mg::egl_error("Failed to query WaylandAllocator buffer width"));
+        }
+        if (extensions->wayland->eglQueryWaylandBufferWL(dpy, buffer, EGL_HEIGHT, &height) == EGL_FALSE)
+        {
+            BOOST_THROW_EXCEPTION(mg::egl_error("Failed to query WaylandAllocator buffer height"));
+        }
+
+        EGLint texture_format;
+        if (!extensions->wayland->eglQueryWaylandBufferWL(dpy, buffer, EGL_TEXTURE_FORMAT, &texture_format))
+        {
+            BOOST_THROW_EXCEPTION(mg::egl_error("Failed to query WL buffer format"));
+        }
+
+        if (texture_format == EGL_TEXTURE_RGB)
+        {
+            format = mir_pixel_format_xrgb_8888;
+        }
+        else if (texture_format == EGL_TEXTURE_RGBA)
+        {
+            format = mir_pixel_format_argb_8888;
+        }
+        else
+        {
+            BOOST_THROW_EXCEPTION((std::invalid_argument{"YUV buffers are unimplemented"}));
+        }
+    }
+
     static void on_buffer_destroyed(wl_listener* listener, void*)
     {
         static_assert(
@@ -488,9 +510,9 @@
 
         {
             std::lock_guard<std::mutex> lock{*shim->mutex};
-            if (shim->associated_buffer)
+            if (auto mir_buffer = shim->associated_buffer.lock())
             {
-                shim->associated_buffer->buffer = nullptr;
+                mir_buffer->buffer = nullptr;
             }
         }
 
@@ -500,7 +522,7 @@
     struct DestructionShim
     {
         std::shared_ptr<std::mutex> const mutex = std::make_shared<std::mutex>();
-        WaylandBuffer* associated_buffer;
+        std::weak_ptr<WaylandBuffer> associated_buffer;
         wl_listener destruction_listener;
     };
 
@@ -542,9 +564,13 @@
     }
 }
 
-std::unique_ptr<mg::Buffer> mgm::BufferAllocator::buffer_from_resource (wl_resource* buffer, std::function<void ()>&& on_consumed)
+std::shared_ptr<mg::Buffer> mgm::BufferAllocator::buffer_from_resource (wl_resource* buffer, std::function<void ()>&& on_consumed)
 {
     if (egl_extensions->wayland)
-        return std::make_unique<WaylandBuffer>(dpy, buffer, egl_extensions, std::move(on_consumed));
+        return WaylandBuffer::mir_buffer_from_wl_buffer(
+            dpy,
+            buffer,
+            egl_extensions,
+            std::move(on_consumed));
     return nullptr;
 }

=== modified file 'src/platforms/mesa/server/buffer_allocator.h'
--- src/platforms/mesa/server/buffer_allocator.h	2017-08-31 07:40:41 +0000
+++ src/platforms/mesa/server/buffer_allocator.h	2017-09-21 08:03:22 +0000
@@ -63,7 +63,7 @@
     std::vector<MirPixelFormat> supported_pixel_formats() override;
 
     void bind_display(wl_display* display) override;
-    std::unique_ptr<Buffer> buffer_from_resource (wl_resource* buffer, std::function<void ()>&& on_consumed) override;
+    std::shared_ptr<Buffer> buffer_from_resource (wl_resource* buffer, std::function<void ()>&& on_consumed) override;
 private:
     std::shared_ptr<Buffer> alloc_hardware_buffer(
         graphics::BufferProperties const& buffer_properties);