[PATCH v1] gstreamer: Use copied camera name

[Laurent Pinchart]

On Tue, May 14, 2024 at 04:44:21PM -0400, Nicolas Dufresne wrote:
> Hey,
> 
> Le mardi 14 mai 2024 à 18:02 +0000, Barnabás Pőcze a écrit :
> > It seems the intent is to use the copied camera name to avoid
> > concurrency problems related to `gst_libcamera_src_set_property()`.
> > 
> > However, the current code makes the copy, but does not actually
> > use it. So fix that.
> > 
> > Fixes: 58feb69f852289 ("gst: libcamerasrc: Implement selection and acquisition")
> > Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>
> > ---
> >  src/gstreamer/gstlibcamerasrc.cpp | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> > index a284110b..9680d809 100644
> > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > @@ -377,10 +377,10 @@ gst_libcamera_src_open(GstLibcameraSrc *self)
> >  	}
> >  
> >  	if (camera_name) {
> > -		cam = cm->get(self->camera_name);
> > +		cam = cm->get(camera_name);
> >  		if (!cam) {
> >  			GST_ELEMENT_ERROR(self, RESOURCE, NOT_FOUND,
> > -					  ("Could not find a camera named '%s'.", self->camera_name),
> > +					  ("Could not find a camera named '%s'.", camera_name),
> >  					  ("libcamera::CameraMananger::get() returned nullptr"));
> 
> What can I say for my defence here ... oops.

You don't need to say anything. Your contributions to libcamerasrc are
so vast that, if you hadn't made a single mistake, I wouldn't believe
you are human :-)

> The point was that we can't hold
> the lock while calling GST_ELEMENT_ERROR(), as it sends a message on the bus,
> taking more mutex and possibly causing issues.

I'll record this in the commit message, I think it's useful information.

--------
gstreamer: Use copied camera name

The camera name is copied in gst_libcamera_src_open() as we can't hold
the lock protecting the name while calling GST_ELEMENT_ERROR(). The
GST_ELEMENT_ERROR() macro sends a message on the bus, taking more locks
and possibly causing issues.

However, the current code makes the copy, but does not actually use it.
So fix that.
--------

> Reviewed-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> 
> >  			return false;
> >  		}

-- 
Regards,

Laurent Pinchart