[libcamera-devel] [PATCH v1 2/3] gstreamer: Fix concurrent access issues to CameraManager
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Aug 26 01:18:08 CEST 2021
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) {
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
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 ?
>
> 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();
> }
>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list