From 996c7079312cc845721f9409291f2f724583bbee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=BDan=20Dober=C5=A1ek?= Date: Mon, 9 Sep 2019 14:55:36 +0200 Subject: [PATCH] wl_resource management fixes (#73) * view-backend-exportable-private: some cleanups and renaming around frame callbacks. * view-backend-exportable-private: improve frame callback wl_resource management Use a destroy listener on frame callback resources in order to be able to properly clean up in case the underlying client disappears. Internal management of these resources in ViewBackend now uses wl_list. * view-backend-exportable-fdo: improve buffer wl_resource management Use a destroy handle on the buffer wl_resource in order to properly track the resource's lifetime. In case the resource was destroyed while corresponding wl_resource was exported to the client, the subsequent release should avoid dispatching buffer release notification on the already-destroyed resource. To achieve this, buffer resources are tracked inside the ClientBundleBuffer implementation in a wl_list, with entries added to that list upon export and removed from it upon release or destruction. * view-backend-exportable-fdo-egl: improve buffer wl_resource management in the deprecated implementation Use a destroy handler on the buffer wl_resource in order to properly track resource's lifetime. In case the resource was destroyed while corresponding wl_resource was exported to the client, the subsequent release should avoid dispatching buffer release notification on the already-destroyed resource. ClientBundleEGLDeprecated tracks the exported resources (on which EGLImage objects are based) in a wl_list. Upon resource destruction, the tracked resource is removed. When the buffer resource is returned, it is released if it is still tracked and was not yet destroyed. The corresponding EGLImage is destroyed unconditionally. * view-backend-exportable-fdo-egl: clean up management of buffer resources in the wpe_fdo_egl_exported_image implementation The ClientBundleEGL implementat that operates with wpe_fdo_egl_exported_image objects already relies on destroy listeners to properly track wl_resource lifetime, so these changes only polish things. The basics remain the same: if the wl_resource is destroyed, the wpe_fdo_egl_exported_image object is destroyed immediately if it's not currently exported, otherwise it's destroyed when the client returns it. --- src/view-backend-exportable-fdo-egl.cpp | 110 ++++++++++++++---------- src/view-backend-exportable-fdo.cpp | 73 ++++++++++++++-- src/view-backend-exportable-private.cpp | 46 ++++++++-- src/view-backend-exportable-private.h | 16 +++- 4 files changed, 182 insertions(+), 63 deletions(-) diff --git a/src/view-backend-exportable-fdo-egl.cpp b/src/view-backend-exportable-fdo-egl.cpp index 5e9df6a..467d7a9 100644 --- a/src/view-backend-exportable-fdo-egl.cpp +++ b/src/view-backend-exportable-fdo-egl.cpp @@ -34,41 +34,54 @@ namespace { -struct buffer_data { - struct wl_resource *buffer_resource; - EGLImageKHR egl_image; -}; - class ClientBundleEGL final : public ClientBundle { public: + struct BufferResource { + struct wl_resource* resource; + EGLImageKHR image; + + struct wl_list link; + struct wl_listener destroyListener; + + static void destroyNotify(struct wl_listener*, void*); + }; + ClientBundleEGL(const struct wpe_view_backend_exportable_fdo_egl_client* _client, void* data, ViewBackend* viewBackend, uint32_t initialWidth, uint32_t initialHeight) : ClientBundle(data, viewBackend, initialWidth, initialHeight) , client(_client) { + wl_list_init(&bufferResources); } virtual ~ClientBundleEGL() { - for (auto* buf_data : m_buffers) { - assert(buf_data->egl_image); - WS::Instance::singleton().destroyImage(buf_data->egl_image); - - delete buf_data; + BufferResource* resource; + BufferResource* next; + wl_list_for_each_safe(resource, next, &bufferResources, link) { + WS::Instance::singleton().destroyImage(resource->image); + viewBackend->releaseBuffer(resource->resource); + + wl_list_remove(&resource->link); + wl_list_remove(&resource->destroyListener.link); + delete resource; } - m_buffers.clear(); + wl_list_init(&bufferResources); } - void exportBuffer(struct wl_resource *bufferResource) override + void exportBuffer(struct wl_resource *buffer) override { - EGLImageKHR image = WS::Instance::singleton().createImage(bufferResource); + EGLImageKHR image = WS::Instance::singleton().createImage(buffer); if (!image) return; - auto* buf_data = new struct buffer_data; - buf_data->buffer_resource = bufferResource; - buf_data->egl_image = image; - m_buffers.push_back(buf_data); + auto* resource = new BufferResource; + resource->resource = buffer; + resource->image = image; + resource->destroyListener.notify = BufferResource::destroyNotify; + + wl_resource_add_destroy_listener(buffer, &resource->destroyListener); + wl_list_insert(&bufferResources, &resource->link); client->export_egl_image(data, image); } @@ -79,33 +92,53 @@ class ClientBundleEGL final : public ClientBundle { if (!image) return; - auto* buf_data = new struct buffer_data; - buf_data->buffer_resource = nullptr; - buf_data->egl_image = image; - m_buffers.push_back(buf_data); + auto* resource = new BufferResource; + resource->resource = dmabuf_buffer->buffer_resource; + resource->image = image; + resource->destroyListener.notify = BufferResource::destroyNotify; + + wl_resource_add_destroy_listener(dmabuf_buffer->buffer_resource, &resource->destroyListener); + wl_list_insert(&bufferResources, &resource->link); client->export_egl_image(data, image); } - struct buffer_data* releaseImage(EGLImageKHR image) + void releaseImage(EGLImageKHR image) { - for (auto* buf_data : m_buffers) - if (buf_data->egl_image == image) { - m_buffers.remove(buf_data); - WS::Instance::singleton().destroyImage(buf_data->egl_image); - - return buf_data; + BufferResource* matchingResource = nullptr; + BufferResource* resource; + wl_list_for_each(resource, &bufferResources, link) { + if (resource->image == image) { + matchingResource = resource; + break; } + } + + WS::Instance::singleton().destroyImage(image); - return nullptr; + if (matchingResource) { + viewBackend->releaseBuffer(matchingResource->resource); + + wl_list_remove(&matchingResource->link); + wl_list_remove(&matchingResource->destroyListener.link); + delete matchingResource; + } } const struct wpe_view_backend_exportable_fdo_egl_client* client; -private: - std::list m_buffers; + struct wl_list bufferResources; }; +void ClientBundleEGL::BufferResource::destroyNotify(struct wl_listener* listener, void*) +{ + BufferResource* resource; + resource = wl_container_of(listener, resource, destroyListener); + + wl_list_remove(&resource->link); + delete resource; +} + } // namespace extern "C" { @@ -129,20 +162,7 @@ __attribute__((visibility("default"))) void wpe_view_backend_exportable_fdo_egl_dispatch_release_image(struct wpe_view_backend_exportable_fdo* exportable, EGLImageKHR image) { - auto* clientBundle = reinterpret_cast(exportable->clientBundle); - - auto* buffer_data = clientBundle->releaseImage(image); - if (!buffer_data) - return; - - /* the EGL image has already been destroyed by ClientBundleEGL */ - - if (buffer_data->buffer_resource) { - wpe_view_backend_exportable_fdo_dispatch_release_buffer(exportable, - buffer_data->buffer_resource); - } - - delete buffer_data; + static_cast(exportable->clientBundle)->releaseImage(image); } } diff --git a/src/view-backend-exportable-fdo.cpp b/src/view-backend-exportable-fdo.cpp index 2241729..6c28b39 100644 --- a/src/view-backend-exportable-fdo.cpp +++ b/src/view-backend-exportable-fdo.cpp @@ -31,18 +31,47 @@ namespace { class ClientBundleBuffer final : public ClientBundle { public: + struct BufferResource { + struct wl_resource* resource; + + struct wl_list link; + struct wl_listener destroyListener; + + static void destroyNotify(struct wl_listener*, void*); + }; + ClientBundleBuffer(const struct wpe_view_backend_exportable_fdo_client* _client, void* data, ViewBackend* viewBackend, uint32_t initialWidth, uint32_t initialHeight) : ClientBundle(data, viewBackend, initialWidth, initialHeight) , client(_client) { + wl_list_init(&bufferResources); } - virtual ~ClientBundleBuffer() = default; + virtual ~ClientBundleBuffer() + { + BufferResource* resource; + BufferResource* next; + wl_list_for_each_safe(resource, next, &bufferResources, link) { + viewBackend->releaseBuffer(resource->resource); + + wl_list_remove(&resource->link); + wl_list_remove(&resource->destroyListener.link); + delete resource; + } + wl_list_init(&bufferResources); + } - void exportBuffer(struct wl_resource *bufferResource) override + void exportBuffer(struct wl_resource *buffer) override { - client->export_buffer_resource(data, bufferResource); + auto* resource = new BufferResource; + resource->resource = buffer; + resource->destroyListener.notify = BufferResource::destroyNotify; + + wl_resource_add_destroy_listener(buffer, &resource->destroyListener); + wl_list_insert(&bufferResources, &resource->link); + + client->export_buffer_resource(data, buffer); } void exportBuffer(const struct linux_dmabuf_buffer *dmabuf_buffer) override @@ -50,9 +79,41 @@ class ClientBundleBuffer final : public ClientBundle { assert(!"This interface doesn't support Linux DMA buffers"); } + void releaseBuffer(struct wl_resource* buffer) + { + BufferResource* matchingResource = nullptr; + BufferResource* resource; + wl_list_for_each(resource, &bufferResources, link) { + if (resource->resource == buffer) { + matchingResource = resource; + break; + } + } + + if (!matchingResource) + return; + + viewBackend->releaseBuffer(buffer); + + wl_list_remove(&matchingResource->link); + wl_list_remove(&matchingResource->destroyListener.link); + delete matchingResource; + } + const struct wpe_view_backend_exportable_fdo_client* client; + + struct wl_list bufferResources; }; +void ClientBundleBuffer::BufferResource::destroyNotify(struct wl_listener* listener, void*) +{ + BufferResource* resource; + resource = wl_container_of(listener, resource, destroyListener); + + wl_list_remove(&resource->link); + delete resource; +} + } // namespace extern "C" { @@ -91,14 +152,14 @@ __attribute__((visibility("default"))) void wpe_view_backend_exportable_fdo_dispatch_frame_complete(struct wpe_view_backend_exportable_fdo* exportable) { - exportable->clientBundle->viewBackend->dispatchFrameCallback(); + exportable->clientBundle->viewBackend->dispatchFrameCallbacks(); } __attribute__((visibility("default"))) void -wpe_view_backend_exportable_fdo_dispatch_release_buffer(struct wpe_view_backend_exportable_fdo* exportable, struct wl_resource* buffer_resource) +wpe_view_backend_exportable_fdo_dispatch_release_buffer(struct wpe_view_backend_exportable_fdo* exportable, struct wl_resource* buffer) { - exportable->clientBundle->viewBackend->releaseBuffer(buffer_resource); + static_cast(exportable->clientBundle)->releaseBuffer(buffer); } } diff --git a/src/view-backend-exportable-private.cpp b/src/view-backend-exportable-private.cpp index 383a528..815151c 100644 --- a/src/view-backend-exportable-private.cpp +++ b/src/view-backend-exportable-private.cpp @@ -32,12 +32,13 @@ ViewBackend::ViewBackend(ClientBundle* clientBundle, struct wpe_view_backend* ba , m_backend(backend) { m_clientBundle->viewBackend = this; + + wl_list_init(&m_frameCallbacks); } ViewBackend::~ViewBackend() { - for (auto* resource : m_callbackResources) - wl_resource_destroy(resource); + clearFrameCallbacks(); WS::Instance::singleton().unregisterViewBackend(m_id); @@ -86,9 +87,13 @@ int ViewBackend::clientFd() return dup(m_clientFd); } -void ViewBackend::frameCallback(struct wl_resource* callbackResource) +void ViewBackend::frameCallback(struct wl_resource* callback) { - m_callbackResources.push_back(callbackResource); + auto* resource = new FrameCallbackResource; + resource->resource = callback; + resource->destroyListener.notify = FrameCallbackResource::destroyNotify; + wl_resource_add_destroy_listener(callback, &resource->destroyListener); + wl_list_insert(&m_frameCallbacks, &resource->link); } void ViewBackend::exportBufferResource(struct wl_resource* bufferResource) @@ -101,13 +106,14 @@ void ViewBackend::exportLinuxDmabuf(const struct linux_dmabuf_buffer *dmabuf_buf m_clientBundle->exportBuffer(dmabuf_buffer); } -void ViewBackend::dispatchFrameCallback() +void ViewBackend::dispatchFrameCallbacks() { - for (auto* resource : m_callbackResources) { - wl_callback_send_done(resource, 0); - wl_resource_destroy(resource); + FrameCallbackResource* resource; + wl_list_for_each(resource, &m_frameCallbacks, link) { + wl_callback_send_done(resource->resource, 0); } - m_callbackResources.clear(); + clearFrameCallbacks(); + wl_client_flush(m_client); wpe_view_backend_dispatch_frame_displayed(m_backend); } @@ -137,3 +143,25 @@ gboolean ViewBackend::s_socketCallback(GSocket* socket, GIOCondition condition, return TRUE; } + +void ViewBackend::clearFrameCallbacks() +{ + FrameCallbackResource* resource; + FrameCallbackResource* next; + wl_list_for_each_safe(resource, next, &m_frameCallbacks, link) { + wl_list_remove(&resource->link); + wl_list_remove(&resource->destroyListener.link); + wl_resource_destroy(resource->resource); + delete resource; + } + wl_list_init(&m_frameCallbacks); +} + +void ViewBackend::FrameCallbackResource::destroyNotify(struct wl_listener* listener, void*) +{ + FrameCallbackResource* resource; + resource = wl_container_of(listener, resource, destroyListener); + + wl_list_remove(&resource->link); + delete resource; +} diff --git a/src/view-backend-exportable-private.h b/src/view-backend-exportable-private.h index b27494e..a5a13df 100644 --- a/src/view-backend-exportable-private.h +++ b/src/view-backend-exportable-private.h @@ -27,7 +27,6 @@ #include "ws.h" #include -#include #include class ViewBackend; @@ -63,10 +62,21 @@ class ViewBackend : public WS::ExportableClient { void frameCallback(struct wl_resource* callbackResource) override; void exportBufferResource(struct wl_resource* bufferResource) override; void exportLinuxDmabuf(const struct linux_dmabuf_buffer *dmabuf_buffer) override; - void dispatchFrameCallback(); + void dispatchFrameCallbacks(); void releaseBuffer(struct wl_resource* buffer_resource); private: + struct FrameCallbackResource { + struct wl_resource* resource; + + struct wl_list link; + struct wl_listener destroyListener; + + static void destroyNotify(struct wl_listener*, void*); + }; + + void clearFrameCallbacks(); + static gboolean s_socketCallback(GSocket*, GIOCondition, gpointer); uint32_t m_id { 0 }; @@ -75,7 +85,7 @@ class ViewBackend : public WS::ExportableClient { ClientBundle* m_clientBundle; struct wpe_view_backend* m_backend; - std::vector m_callbackResources; + struct wl_list m_frameCallbacks; GSocket* m_socket; GSource* m_source;