From 8ef30a9ce59500cb1b1c644905fab05da01c221a Mon Sep 17 00:00:00 2001 From: Philippe Normand Date: Thu, 10 Sep 2020 14:39:58 +0100 Subject: [PATCH 1/4] wpe: Move webview load waiting to WPEView As waiting for the load to be finished is specific to the WebView, it should be done from our WPEView, not from the WPEContextThread. This fixes issues where multiple wpesrc elements are created in sequence. Without this patch the first view might receive erroneous buffer notifications. Fixes #1386 Part-of: --- ext/wpe/WPEThreadedView.cpp | 45 +++++++++++++++++++++---------------- ext/wpe/WPEThreadedView.h | 13 ++++++----- 2 files changed, 34 insertions(+), 24 deletions(-) diff --git a/ext/wpe/WPEThreadedView.cpp b/ext/wpe/WPEThreadedView.cpp index bf888350f3..797751738f 100644 --- a/ext/wpe/WPEThreadedView.cpp +++ b/ext/wpe/WPEThreadedView.cpp @@ -76,8 +76,6 @@ WPEContextThread::WPEContextThread() { g_mutex_init(&threading.mutex); g_cond_init(&threading.cond); - g_mutex_init(&threading.ready_mutex); - g_cond_init(&threading.ready_cond); { GMutexHolder lock(threading.mutex); @@ -96,8 +94,6 @@ WPEContextThread::~WPEContextThread() g_mutex_clear(&threading.mutex); g_cond_clear(&threading.cond); - g_mutex_clear(&threading.ready_mutex); - g_cond_clear(&threading.ready_cond); } template @@ -167,7 +163,6 @@ gpointer WPEContextThread::s_viewThread(gpointer data) WPEView* WPEContextThread::createWPEView(GstWpeSrc* src, GstGLContext* context, GstGLDisplay* display, int width, int height) { GST_DEBUG("context %p display %p, size (%d,%d)", context, display, width, height); - threading.ready = FALSE; static std::once_flag s_loaderFlag; std::call_once(s_loaderFlag, @@ -190,24 +185,13 @@ WPEView* WPEContextThread::createWPEView(GstWpeSrc* src, GstGLContext* context, if (view && view->hasUri()) { GST_DEBUG("waiting load to finish"); - GMutexHolder lock(threading.ready_mutex); - while (!threading.ready) - g_cond_wait(&threading.ready_cond, &threading.ready_mutex); + view->waitLoadCompletion(); GST_DEBUG("done"); } return view; } -void WPEContextThread::notifyLoadFinished() -{ - GMutexHolder lock(threading.ready_mutex); - if (!threading.ready) { - threading.ready = TRUE; - g_cond_signal(&threading.ready_cond); - } -} - static gboolean s_loadFailed(WebKitWebView*, WebKitLoadEvent, gchar* failing_uri, GError* error, gpointer data) { GstWpeSrc* src = GST_WPE_SRC(data); @@ -223,6 +207,10 @@ static gboolean s_loadFailedWithTLSErrors(WebKitWebView*, gchar* failing_uri, G WPEView::WPEView(WebKitWebContext* web_context, GstWpeSrc* src, GstGLContext* context, GstGLDisplay* display, int width, int height) { + g_mutex_init(&threading.ready_mutex); + g_cond_init(&threading.ready_cond); + threading.ready = FALSE; + g_mutex_init(&images_mutex); if (context) gst.context = GST_GL_CONTEXT(gst_object_ref(context)); @@ -282,6 +270,9 @@ WPEView::WPEView(WebKitWebContext* web_context, GstWpeSrc* src, GstGLContext* co WPEView::~WPEView() { + g_mutex_clear(&threading.ready_mutex); + g_cond_clear(&threading.ready_cond); + { GMutexHolder lock(images_mutex); @@ -319,6 +310,22 @@ WPEView::~WPEView() g_mutex_clear(&images_mutex); } +void WPEView::notifyLoadFinished() +{ + GMutexHolder lock(threading.ready_mutex); + if (!threading.ready) { + threading.ready = TRUE; + g_cond_signal(&threading.ready_cond); + } +} + +void WPEView::waitLoadCompletion() +{ + GMutexHolder lock(threading.ready_mutex); + while (!threading.ready) + g_cond_wait(&threading.ready_cond, &threading.ready_mutex); +} + GstEGLImage* WPEView::image() { GstEGLImage* ret = nullptr; @@ -481,7 +488,7 @@ void WPEView::handleExportedImage(gpointer image) GST_TRACE("EGLImage %p wrapped in GstEGLImage %" GST_PTR_FORMAT, eglImage, gstImage); egl.pending = gstImage; - s_view->notifyLoadFinished(); + notifyLoadFinished(); } } @@ -538,7 +545,7 @@ void WPEView::handleExportedBuffer(struct wpe_fdo_shm_exported_buffer* buffer) GMutexHolder lock(images_mutex); GST_TRACE("SHM buffer %p wrapped in buffer %" GST_PTR_FORMAT, buffer, gstBuffer); shm.pending = gstBuffer; - s_view->notifyLoadFinished(); + notifyLoadFinished(); } } #endif diff --git a/ext/wpe/WPEThreadedView.h b/ext/wpe/WPEThreadedView.h index 359e80ea94..c5633abc5e 100644 --- a/ext/wpe/WPEThreadedView.h +++ b/ext/wpe/WPEThreadedView.h @@ -59,6 +59,7 @@ public: /* Used by WPEContextThread */ bool hasUri() const { return webkit.uri; } void disconnectLoadFailedSignal(); + void waitLoadCompletion(); protected: void handleExportedImage(gpointer); @@ -70,6 +71,7 @@ private: struct wpe_view_backend* backend() const; void frameComplete(); void loadUriUnlocked(const gchar*); + void notifyLoadFinished(); void releaseImage(gpointer); #if ENABLE_SHM_BUFFER_SUPPORT @@ -101,6 +103,12 @@ private: bool m_isValid { false }; + struct { + GMutex ready_mutex; + GCond ready_cond; + gboolean ready; + } threading; + // This mutex guards access to either egl or shm resources declared below, // depending on the runtime behavior. GMutex images_mutex; @@ -129,16 +137,11 @@ public: template void dispatch(Function); - void notifyLoadFinished(); - private: static gpointer s_viewThread(gpointer); struct { GMutex mutex; GCond cond; - GMutex ready_mutex; - GCond ready_cond; - gboolean ready; GThread* thread { nullptr }; } threading; -- GitLab From c3659cd61194e9c68c5b56e526f025f3e01ef87f Mon Sep 17 00:00:00 2001 From: Philippe Normand Date: Fri, 11 Sep 2020 12:51:56 +0100 Subject: [PATCH 2/4] wpe: Plug SHM buffer leaks Fixes #1409 Part-of: --- ext/wpe/WPEThreadedView.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/ext/wpe/WPEThreadedView.cpp b/ext/wpe/WPEThreadedView.cpp index 797751738f..de05e3c831 100644 --- a/ext/wpe/WPEThreadedView.cpp +++ b/ext/wpe/WPEThreadedView.cpp @@ -284,6 +284,14 @@ WPEView::~WPEView() gst_egl_image_unref(egl.committed); egl.committed = nullptr; } + if (shm.pending) { + gst_buffer_unref(shm.pending); + shm.pending = nullptr; + } + if (shm.committed) { + gst_buffer_unref(shm.committed); + shm.committed = nullptr; + } } WPEContextThread::singleton().dispatch([&]() { -- GitLab From 6fc7455881b5d9bf2e432e1a532377601be8946d Mon Sep 17 00:00:00 2001 From: Jan Schmidt Date: Tue, 25 Aug 2020 01:57:55 +1000 Subject: [PATCH 3/4] wpesrc: Don't crash if WPE doesn't generate a buffer. On creating a 2nd wpesrc in a new pipeline in an app that already has a runnig wpesrc, WPE sometimes doesn't return a buffer on request, leading to a crash. This commit fixes the crash, but not the underlying failure - a 2nd wpesrc can still error out instead. Partially fixes https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/-/issues/1386 Part-of: --- ext/wpe/gstwpesrc.cpp | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/ext/wpe/gstwpesrc.cpp b/ext/wpe/gstwpesrc.cpp index 9e81ab1227..970c742152 100644 --- a/ext/wpe/gstwpesrc.cpp +++ b/ext/wpe/gstwpesrc.cpp @@ -169,11 +169,13 @@ gst_wpe_src_create (GstBaseSrc * bsrc, guint64 offset, guint length, GstBuffer * } locked_buffer = src->view->buffer (); - - if (locked_buffer != NULL) { - *buf = gst_buffer_copy_deep (locked_buffer); - ret = GST_FLOW_OK; + if (locked_buffer == NULL) { + GST_OBJECT_UNLOCK (src); + GST_ELEMENT_ERROR (src, RESOURCE, FAILED, + ("WPE View did not render a buffer"), (NULL)); + return ret; } + *buf = gst_buffer_copy_deep (locked_buffer); g_object_get(gl_src, "timestamp-offset", &ts_offset, NULL); @@ -195,6 +197,7 @@ gst_wpe_src_create (GstBaseSrc * bsrc, guint64 offset, guint length, GstBuffer * gl_src->running_time = next_time; + ret = GST_FLOW_OK; GST_OBJECT_UNLOCK (src); return ret; } -- GitLab From 2e8927ce936b8229315ca3fcee35226ab1b2e8ee Mon Sep 17 00:00:00 2001 From: Philippe Normand Date: Mon, 14 Sep 2020 09:48:48 +0100 Subject: [PATCH 4/4] wpe: Plug event leak Handled events don't go through the default pad event handler, so they need to be unreffed in this case. Part-of: --- ext/wpe/gstwpesrc.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ext/wpe/gstwpesrc.cpp b/ext/wpe/gstwpesrc.cpp index 970c742152..94afd322ba 100644 --- a/ext/wpe/gstwpesrc.cpp +++ b/ext/wpe/gstwpesrc.cpp @@ -585,6 +585,8 @@ gst_wpe_src_event (GstPad * pad, GstObject * parent, GstEvent * event) if (!ret) { ret = gst_pad_event_default (pad, parent, event); + } else { + gst_event_unref (event); } return ret; } -- GitLab