[libcamera-devel,v2,2/3] gstreamer: Fix concurrent access issues to CameraManager

13512 diff mbox series

From: Nicolas Dufresne <nicolas.dufresne@collabora.com>

It's not allowed to have multiple instances of CameraManager. This
requirement is not easy for GStreamer were the device monitor and
the camerasrc, or two camerasrc instances don't usually have any
interaction between each other. Fix this by implementing a minimalist
singleton around CameraManager constructor and start()/stop()
operations.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 src/gstreamer/gstlibcamera-utils.cpp   | 24 ++++++++++++++++++++++++
 src/gstreamer/gstlibcamera-utils.h     |  6 ++++--
 src/gstreamer/gstlibcameraprovider.cpp | 22 ++--------------------
 src/gstreamer/gstlibcamerasrc.cpp      | 11 +++++------
 4 files changed, 35 insertions(+), 28 deletions(-)

Hi Nicolas,

Thank you for the patch.

On Thu, Aug 26, 2021 at 09:23:45AM -0400, Nicolas Dufresne wrote:
> From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> 
> It's not allowed to have multiple instances of CameraManager. This
> requirement is not easy for GStreamer were the device monitor and
> the camerasrc, or two camerasrc instances don't usually have any
> interaction between each other. Fix this by implementing a minimalist
> singleton around CameraManager constructor and start()/stop()
> operations.
> 
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  src/gstreamer/gstlibcamera-utils.cpp   | 24 ++++++++++++++++++++++++
>  src/gstreamer/gstlibcamera-utils.h     |  6 ++++--
>  src/gstreamer/gstlibcameraprovider.cpp | 22 ++--------------------
>  src/gstreamer/gstlibcamerasrc.cpp      | 11 +++++------
>  4 files changed, 35 insertions(+), 28 deletions(-)
> 
> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> index 007d6a64..029f031f 100644
> --- a/src/gstreamer/gstlibcamera-utils.cpp
> +++ b/src/gstreamer/gstlibcamera-utils.cpp
> @@ -221,3 +221,27 @@ gst_libcamera_resume_task(GstTask *task)
>  		GST_TASK_SIGNAL(task);
>  	}
>  }
> +
> +G_LOCK_DEFINE_STATIC(cm_singleton_lock);
> +std::weak_ptr<CameraManager> cm_singleton_ptr;
> +
> +std::shared_ptr<CameraManager>
> +gst_libcamera_get_camera_mananger(int &ret)
> +{
> +	std::shared_ptr<CameraManager> cm;
> +
> +	G_LOCK(cm_singleton_lock);
> +
> +	cm = cm_singleton_ptr.lock();
> +	if (!cm) {
> +		cm = std::make_shared<CameraManager>();
> +		cm_singleton_ptr = cm;
> +		ret = cm->start();
> +	} else {
> +		ret = 0;
> +	}
> +
> +	G_UNLOCK(cm_singleton_lock);
> +
> +	return cm;
> +}
> diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h
> index 2b3f26b6..67a06db3 100644
> --- a/src/gstreamer/gstlibcamera-utils.h
> +++ b/src/gstreamer/gstlibcamera-utils.h
> @@ -9,16 +9,18 @@
>  #ifndef __GST_LIBCAMERA_UTILS_H__
>  #define __GST_LIBCAMERA_UTILS_H__
>  
> +#include <libcamera/camera_manager.h>
> +#include <libcamera/stream.h>
> +
>  #include <gst/gst.h>
>  #include <gst/video/video.h>
>  
> -#include <libcamera/stream.h>
> -
>  GstCaps *gst_libcamera_stream_formats_to_caps(const libcamera::StreamFormats &formats);
>  GstCaps *gst_libcamera_stream_configuration_to_caps(const libcamera::StreamConfiguration &stream_cfg);
>  void gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration &stream_cfg,
>  					      GstCaps *caps);
>  void gst_libcamera_resume_task(GstTask *task);
> +std::shared_ptr<libcamera::CameraManager> gst_libcamera_get_camera_mananger(int &ret);
>  
>  /**
>   * \class GLibLocker
> diff --git a/src/gstreamer/gstlibcameraprovider.cpp b/src/gstreamer/gstlibcameraprovider.cpp
> index 29da6c32..948ba0d1 100644
> --- a/src/gstreamer/gstlibcameraprovider.cpp
> +++ b/src/gstreamer/gstlibcameraprovider.cpp
> @@ -158,7 +158,6 @@ gst_libcamera_device_new(const std::shared_ptr<Camera> &camera)
>  
>  struct _GstLibcameraProvider {
>  	GstDeviceProvider parent;
> -	CameraManager *cm;
>  };
>  
>  G_DEFINE_TYPE_WITH_CODE(GstLibcameraProvider, gst_libcamera_provider,
> @@ -170,7 +169,7 @@ static GList *
>  gst_libcamera_provider_probe(GstDeviceProvider *provider)
>  {
>  	GstLibcameraProvider *self = GST_LIBCAMERA_PROVIDER(provider);
> -	CameraManager *cm = self->cm;
> +	std::shared_ptr<CameraManager> cm;
>  	GList *devices = nullptr;
>  	gint ret;
>  
> @@ -181,7 +180,7 @@ gst_libcamera_provider_probe(GstDeviceProvider *provider)
>  	 * gains monitoring support. Meanwhile we need to cycle start()/stop()
>  	 * to ensure every probe() calls return the latest list.
>  	 */
> -	ret = cm->start();
> +	cm = gst_libcamera_get_camera_mananger(ret);
>  	if (ret) {
>  		GST_ERROR_OBJECT(self, "Failed to retrieve device list: %s",
>  				 g_strerror(-ret));
> @@ -194,8 +193,6 @@ gst_libcamera_provider_probe(GstDeviceProvider *provider)
>  					g_object_ref_sink(gst_libcamera_device_new(camera)));
>  	}
>  
> -	cm->stop();
> -
>  	return devices;
>  }
>  
> @@ -204,31 +201,16 @@ gst_libcamera_provider_init(GstLibcameraProvider *self)
>  {
>  	GstDeviceProvider *provider = GST_DEVICE_PROVIDER(self);
>  
> -	self->cm = new CameraManager();
> -
>  	/* Avoid devices being duplicated. */
>  	gst_device_provider_hide_provider(provider, "v4l2deviceprovider");
>  }
>  
> -static void
> -gst_libcamera_provider_finalize(GObject *object)
> -{
> -	GstLibcameraProvider *self = GST_LIBCAMERA_PROVIDER(object);
> -	gpointer klass = gst_libcamera_provider_parent_class;
> -
> -	delete self->cm;
> -
> -	return G_OBJECT_CLASS(klass)->finalize(object);
> -}
> -
>  static void
>  gst_libcamera_provider_class_init(GstLibcameraProviderClass *klass)
>  {
>  	GstDeviceProviderClass *provider_class = GST_DEVICE_PROVIDER_CLASS(klass);
> -	GObjectClass *object_class = G_OBJECT_CLASS(klass);
>  
>  	provider_class->probe = gst_libcamera_provider_probe;
> -	object_class->finalize = gst_libcamera_provider_finalize;
>  
>  	gst_device_provider_class_set_metadata(provider_class,
>  					       "libcamera Device Provider",
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index 4c7b36ae..bdd9f88c 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -104,7 +104,7 @@ GstBuffer *RequestWrap::detachBuffer(Stream *stream)
>  struct GstLibcameraSrcState {
>  	GstLibcameraSrc *src_;
>  
> -	std::unique_ptr<CameraManager> cm_;
> +	std::shared_ptr<CameraManager> cm_;
>  	std::shared_ptr<Camera> cam_;
>  	std::unique_ptr<CameraConfiguration> config_;
>  	std::vector<GstPad *> srcpads_;
> @@ -198,13 +198,13 @@ GstLibcameraSrcState::requestCompleted(Request *request)
>  static bool
>  gst_libcamera_src_open(GstLibcameraSrc *self)
>  {
> -	std::unique_ptr<CameraManager> cm = std::make_unique<CameraManager>();
> +	std::shared_ptr<CameraManager> cm;
>  	std::shared_ptr<Camera> cam;
> -	gint ret = 0;
> +	gint ret;
>  
>  	GST_DEBUG_OBJECT(self, "Opening camera device ...");
>  
> -	ret = cm->start();
> +	cm = gst_libcamera_get_camera_mananger(ret);
>  	if (ret) {
>  		GST_ELEMENT_ERROR(self, LIBRARY, INIT,
>  				  ("Failed listing cameras."),
> @@ -250,7 +250,7 @@ gst_libcamera_src_open(GstLibcameraSrc *self)
>  	cam->requestCompleted.connect(self->state, &GstLibcameraSrcState::requestCompleted);
>  
>  	/* No need to lock here, we didn't start our threads yet. */
> -	self->state->cm_ = std::move(cm);
> +	self->state->cm_ = cm;
>  	self->state->cam_ = cam;
>  
>  	return true;
> @@ -517,7 +517,6 @@ gst_libcamera_src_close(GstLibcameraSrc *self)
>  	}
>  
>  	state->cam_.reset();
> -	state->cm_->stop();
>  	state->cm_.reset();
>  }
>