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

[Nicolas Dufresne]

Le jeudi 26 août 2021 à 02:18 +0300, Laurent Pinchart a écrit :
> Hi Nicolas,
> 
> Thank you for the patch.
> 
> On Wed, Aug 25, 2021 at 05:18:51PM -0400, Nicolas Dufresne wrote:
> > From: Nicolas Dufresne <nicolas.dufresne at 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. This this by implementing a minimalist
> 
> s/This this/Fix this/ ?
> 
> > singleton around CameraManager constructor and start()/stop()
> > operations.
> > 
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> > ---
> >  src/gstreamer/gstlibcamera-utils.cpp   | 31 ++++++++++++++++++++++++++
> >  src/gstreamer/gstlibcamera-utils.h     |  6 +++--
> >  src/gstreamer/gstlibcameraprovider.cpp | 22 ++----------------
> >  src/gstreamer/gstlibcamerasrc.cpp      | 11 +++++----
> >  4 files changed, 42 insertions(+), 28 deletions(-)
> > 
> > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> > index 007d6a64..34b64c4a 100644
> > --- a/src/gstreamer/gstlibcamera-utils.cpp
> > +++ b/src/gstreamer/gstlibcamera-utils.cpp
> > @@ -221,3 +221,34 @@ gst_libcamera_resume_task(GstTask *task)
> >  		GST_TASK_SIGNAL(task);
> >  	}
> >  }
> > +
> > +G_LOCK_DEFINE_STATIC(cm_singleton_lock);
> > +std::weak_ptr<CameraManager> cm_singleton_ptr;
> > +
> > +void _cm_deleter(CameraManager *cm)
> > +{
> > +	g_print("Delete CameraManager\n");
> > +	cm->stop();
> > +	delete cm;
> 
> The CameraManager destructor calls stop(), so if you don't care much
> about the g_print(), you could drop this function.
> 
> > +}
> > +
> > +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_singleton_ptr.expired()) {
> 
> I think you could write this
> 
> 	cm = cm_singleton_ptr.lock();
> 	if (cm) {

With "if (!cm)" that works.

> 
> as std::weak_ptr<>::lock() return a null std::shared_ptr<> if it has
> expired.
> 
> > +		std::shared_ptr<CameraManager> ptr(new CameraManager(), _cm_deleter);
> > +		cm_singleton_ptr = cm = ptr;
> 
> And there's no need for the local ptr variable here, you can write
> 
> 		cm = std::shared_ptr<CameraManager>(new CameraManager(), _cm_deleter);
> 		cm_singleton_ptr = cm;
> 
> If you don't care about the deleter, it could also become

I'll drop the deleter if I don't need to call stop() explicitly.

> 
> 		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;
> >  };
> 
> Is this constructed on demand to call probe() and then immediately
> destroyed that it makes no sense anymore to cache the pointer ?

Correct, this is a bit of an intermediate. In this temporary implementation
(non-monitoring). I was doing start() enum stop(), so that each time probe is
called the list was fresh. This is as close as I can get using a singleton here.
But consider this a temporary state.

To support the new API, we need a C++ class to hold the shared ptr (like
libcamersrc). I'll introduce this while adding monitoring support. This is just
to keep the the thing minimally working and small.

> 
> >  
> >  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();
> >  }
> >  
>