| Message ID | 20240514180206.198938-1-pobrn@protonmail.com |
|---|---|
| State | Accepted |
| Commit | e43d2a35fa671c18dd8bae39ab21df9671b5f0a1 |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás, Thank you for the patch. On Tue, May 14, 2024 at 06:02:07PM +0000, Barnabás Pőcze wrote: > 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. Indeed, the local camera_name variable seems pointless otherwise. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Did you catch the issue by chance while reading the code, or did something trigger an investigation ? > Fixes: 58feb69f852289 ("gst: libcamerasrc: Implement selection and acquisition") > Signed-off-by: Barnabás Pőcze <pobrn@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")); > return false; > } >
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@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. 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. Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > return false; > }
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@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@collabora.com> > > > return false; > > }
Hi 2024. május 14., kedd 21:16 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta: > Hi Barnabás, > > Thank you for the patch. > > On Tue, May 14, 2024 at 06:02:07PM +0000, Barnabás Pőcze wrote: > > 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. > > Indeed, the local camera_name variable seems pointless otherwise. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Did you catch the issue by chance while reading the code, or did > something trigger an investigation ? I was experimenting with some code changes, and I noticed this discrepancy during that. > > > Fixes: 58feb69f852289 ("gst: libcamerasrc: Implement selection and acquisition") > > Signed-off-by: Barnabás Pőcze <pobrn@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")); > > return false; > > } > [...] Regards, Barnabás Pőcze
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")); return false; }
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@protonmail.com> --- src/gstreamer/gstlibcamerasrc.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)