[{"id":19102,"web_url":"https://patchwork.libcamera.org/comment/19102/","msgid":"","date":"2021-08-26T13:42:23","subject":"Re: [libcamera-devel] [PATCH v2 2/3] gstreamer: Fix concurrent\n\taccess issues to CameraManager","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nicolas,\n\nThank you for the patch.\n\nOn Thu, Aug 26, 2021 at 09:23:45AM -0400, Nicolas Dufresne wrote:\n> From: Nicolas Dufresne \n> \n> It's not allowed to have multiple instances of CameraManager. This\n> requirement is not easy for GStreamer were the device monitor and\n> the camerasrc, or two camerasrc instances don't usually have any\n> interaction between each other. Fix this by implementing a minimalist\n> singleton around CameraManager constructor and start()/stop()\n> operations.\n> \n> Signed-off-by: Nicolas Dufresne \n\nReviewed-by: Laurent Pinchart \n\n> ---\n> src/gstreamer/gstlibcamera-utils.cpp | 24 ++++++++++++++++++++++++\n> src/gstreamer/gstlibcamera-utils.h | 6 ++++--\n> src/gstreamer/gstlibcameraprovider.cpp | 22 ++--------------------\n> src/gstreamer/gstlibcamerasrc.cpp | 11 +++++------\n> 4 files changed, 35 insertions(+), 28 deletions(-)\n> \n> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> index 007d6a64..029f031f 100644\n> --- a/src/gstreamer/gstlibcamera-utils.cpp\n> +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> @@ -221,3 +221,27 @@ gst_libcamera_resume_task(GstTask *task)\n> \t\tGST_TASK_SIGNAL(task);\n> \t}\n> }\n> +\n> +G_LOCK_DEFINE_STATIC(cm_singleton_lock);\n> +std::weak_ptr cm_singleton_ptr;\n> +\n> +std::shared_ptr\n> +gst_libcamera_get_camera_mananger(int &ret)\n> +{\n> +\tstd::shared_ptr cm;\n> +\n> +\tG_LOCK(cm_singleton_lock);\n> +\n> +\tcm = cm_singleton_ptr.lock();\n> +\tif (!cm) {\n> +\t\tcm = std::make_shared();\n> +\t\tcm_singleton_ptr = cm;\n> +\t\tret = cm->start();\n> +\t} else {\n> +\t\tret = 0;\n> +\t}\n> +\n> +\tG_UNLOCK(cm_singleton_lock);\n> +\n> +\treturn cm;\n> +}\n> diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h\n> index 2b3f26b6..67a06db3 100644\n> --- a/src/gstreamer/gstlibcamera-utils.h\n> +++ b/src/gstreamer/gstlibcamera-utils.h\n> @@ -9,16 +9,18 @@\n> #ifndef __GST_LIBCAMERA_UTILS_H__\n> #define __GST_LIBCAMERA_UTILS_H__\n> \n> +#include \n> +#include \n> +\n> #include \n> #include \n> \n> -#include \n> -\n> GstCaps *gst_libcamera_stream_formats_to_caps(const libcamera::StreamFormats &formats);\n> GstCaps *gst_libcamera_stream_configuration_to_caps(const libcamera::StreamConfiguration &stream_cfg);\n> void gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration &stream_cfg,\n> \t\t\t\t\t GstCaps *caps);\n> void gst_libcamera_resume_task(GstTask *task);\n> +std::shared_ptr gst_libcamera_get_camera_mananger(int &ret);\n> \n> /**\n> * \\class GLibLocker\n> diff --git a/src/gstreamer/gstlibcameraprovider.cpp b/src/gstreamer/gstlibcameraprovider.cpp\n> index 29da6c32..948ba0d1 100644\n> --- a/src/gstreamer/gstlibcameraprovider.cpp\n> +++ b/src/gstreamer/gstlibcameraprovider.cpp\n> @@ -158,7 +158,6 @@ gst_libcamera_device_new(const std::shared_ptr &camera)\n> \n> struct _GstLibcameraProvider {\n> \tGstDeviceProvider parent;\n> -\tCameraManager *cm;\n> };\n> \n> G_DEFINE_TYPE_WITH_CODE(GstLibcameraProvider, gst_libcamera_provider,\n> @@ -170,7 +169,7 @@ static GList *\n> gst_libcamera_provider_probe(GstDeviceProvider *provider)\n> {\n> \tGstLibcameraProvider *self = GST_LIBCAMERA_PROVIDER(provider);\n> -\tCameraManager *cm = self->cm;\n> +\tstd::shared_ptr cm;\n> \tGList *devices = nullptr;\n> \tgint ret;\n> \n> @@ -181,7 +180,7 @@ gst_libcamera_provider_probe(GstDeviceProvider *provider)\n> \t * gains monitoring support. Meanwhile we need to cycle start()/stop()\n> \t * to ensure every probe() calls return the latest list.\n> \t */\n> -\tret = cm->start();\n> +\tcm = gst_libcamera_get_camera_mananger(ret);\n> \tif (ret) {\n> \t\tGST_ERROR_OBJECT(self, \"Failed to retrieve device list: %s\",\n> \t\t\t\t g_strerror(-ret));\n> @@ -194,8 +193,6 @@ gst_libcamera_provider_probe(GstDeviceProvider *provider)\n> \t\t\t\t\tg_object_ref_sink(gst_libcamera_device_new(camera)));\n> \t}\n> \n> -\tcm->stop();\n> -\n> \treturn devices;\n> }\n> \n> @@ -204,31 +201,16 @@ gst_libcamera_provider_init(GstLibcameraProvider *self)\n> {\n> \tGstDeviceProvider *provider = GST_DEVICE_PROVIDER(self);\n> \n> -\tself->cm = new CameraManager();\n> -\n> \t/* Avoid devices being duplicated. */\n> \tgst_device_provider_hide_provider(provider, \"v4l2deviceprovider\");\n> }\n> \n> -static void\n> -gst_libcamera_provider_finalize(GObject *object)\n> -{\n> -\tGstLibcameraProvider *self = GST_LIBCAMERA_PROVIDER(object);\n> -\tgpointer klass = gst_libcamera_provider_parent_class;\n> -\n> -\tdelete self->cm;\n> -\n> -\treturn G_OBJECT_CLASS(klass)->finalize(object);\n> -}\n> -\n> static void\n> gst_libcamera_provider_class_init(GstLibcameraProviderClass *klass)\n> {\n> \tGstDeviceProviderClass *provider_class = GST_DEVICE_PROVIDER_CLASS(klass);\n> -\tGObjectClass *object_class = G_OBJECT_CLASS(klass);\n> \n> \tprovider_class->probe = gst_libcamera_provider_probe;\n> -\tobject_class->finalize = gst_libcamera_provider_finalize;\n> \n> \tgst_device_provider_class_set_metadata(provider_class,\n> \t\t\t\t\t \"libcamera Device Provider\",\n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> index 4c7b36ae..bdd9f88c 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -104,7 +104,7 @@ GstBuffer *RequestWrap::detachBuffer(Stream *stream)\n> struct GstLibcameraSrcState {\n> \tGstLibcameraSrc *src_;\n> \n> -\tstd::unique_ptr cm_;\n> +\tstd::shared_ptr cm_;\n> \tstd::shared_ptr cam_;\n> \tstd::unique_ptr config_;\n> \tstd::vector srcpads_;\n> @@ -198,13 +198,13 @@ GstLibcameraSrcState::requestCompleted(Request *request)\n> static bool\n> gst_libcamera_src_open(GstLibcameraSrc *self)\n> {\n> -\tstd::unique_ptr cm = std::make_unique();\n> +\tstd::shared_ptr cm;\n> \tstd::shared_ptr cam;\n> -\tgint ret = 0;\n> +\tgint ret;\n> \n> \tGST_DEBUG_OBJECT(self, \"Opening camera device ...\");\n> \n> -\tret = cm->start();\n> +\tcm = gst_libcamera_get_camera_mananger(ret);\n> \tif (ret) {\n> \t\tGST_ELEMENT_ERROR(self, LIBRARY, INIT,\n> \t\t\t\t (\"Failed listing cameras.\"),\n> @@ -250,7 +250,7 @@ gst_libcamera_src_open(GstLibcameraSrc *self)\n> \tcam->requestCompleted.connect(self->state, &GstLibcameraSrcState::requestCompleted);\n> \n> \t/* No need to lock here, we didn't start our threads yet. */\n> -\tself->state->cm_ = std::move(cm);\n> +\tself->state->cm_ = cm;\n> \tself->state->cam_ = cam;\n> \n> \treturn true;\n> @@ -517,7 +517,6 @@ gst_libcamera_src_close(GstLibcameraSrc *self)\n> \t}\n> \n> \tstate->cam_.reset();\n> -\tstate->cm_->stop();\n> \tstate->cm_.reset();\n> }\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 50813BD87C\n\tfor ;\n\tThu, 26 Aug 2021 13:42:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C6AA96890C;\n\tThu, 26 Aug 2021 15:42:36 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1F13B6888F\n\tfor ;\n\tThu, 26 Aug 2021 15:42:36 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 99B08191F;\n\tThu, 26 Aug 2021 15:42:35 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"tbegqK0u\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629985355;\n\tbh=E8gVzzRFsSlZDOHGRh3P3aBsEVvP/Zn3ihMkHxS8kKQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tbegqK0uw4vEuxSu7kgGZzG5K1FxemWkEb1H9cO7I8aY7jOpc7fbQIbNxdiZulefb\n\t426fspzmXnD2LueILrDkB9hMKu2pKv1jzd0Xz6L85D5LvcXe8E/3MBrm0K3/aYWQ9p\n\t56i4f+vUyl08ewKEoGALU81ADd25kJ8XJGobd3vE=","Date":"Thu, 26 Aug 2021 16:42:23 +0300","From":"Laurent Pinchart ","To":"Nicolas Dufresne ","Message-ID":"","References":"<20210826132346.1238420-1-nicolas@ndufresne.ca>\n\t<20210826132346.1238420-3-nicolas@ndufresne.ca>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210826132346.1238420-3-nicolas@ndufresne.ca>","Subject":"Re: [libcamera-devel] [PATCH v2 2/3] gstreamer: Fix concurrent\n\taccess issues to CameraManager","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org,\n\tNicolas Dufresne ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]