[libcamera-devel] [RFC PATCH] gstreamer: Drop libcamera_private dependency

[Nicolas Dufresne]

Le mercredi 28 juin 2023 à 10:26 -0400, Nicolas Dufresne a écrit :
> Le lundi 26 juin 2023 à 21:52 +0000, Barnabás Pőcze via libcamera-devel a
> écrit :
> > Hi
> > 
> > 
> > 2023. június 26., hétfő 23:17 keltezéssel, Umang Jain via libcamera-devel <libcamera-devel at lists.libcamera.org> írta:
> > 
> > > Drop libcamera_private dependency entirely as to avoid libcamerasrc
> > > getting more dependent on it. In order to achieve that, one of the
> > > mutex locks in GstLibcameraSrcState needs to be replaced with GMutex.
> > > 
> > > However doing so, this won't let us to use the clang's thread annotation
> > > macros in libcamera (which should be fine as libcamerasrc would move
> > > out of libcamera repo once matured).
> > > 
> > > Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> > > ---
> > >  src/gstreamer/gstlibcamerasrc.cpp | 20 ++++++++------------
> > >  src/gstreamer/meson.build         |  2 +-
> > >  2 files changed, 9 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> > > index 11f15068..fda5610f 100644
> > > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > > @@ -32,8 +32,6 @@
> > >  #include <queue>
> > >  #include <vector>
> > > 
> > > -#include <libcamera/base/mutex.h>
> > > -
> > >  #include <libcamera/camera.h>
> > >  #include <libcamera/camera_manager.h>
> > >  #include <libcamera/control_ids.h>
> > > @@ -125,11 +123,9 @@ struct GstLibcameraSrcState {
> > >  	 * be held while calling into other graph elements (e.g. when calling
> > >  	 * gst_pad_query()).
> > >  	 */
> > > -	Mutex lock_;
> > > -	std::queue<std::unique_ptr<RequestWrap>> queuedRequests_
> > > -		LIBCAMERA_TSA_GUARDED_BY(lock_);
> > > -	std::queue<std::unique_ptr<RequestWrap>> completedRequests_
> > > -		LIBCAMERA_TSA_GUARDED_BY(lock_);
> > > +	GMutex lock_;
> > > [...]
> > 
> > Shouldn't this be initialized? i.e. `g_mutex_init()`?
> 
> A quote from the doc:
>    
>    Notice that the GMutex is not initialised to any particular value. Its
>    placement in static storage ensures that it will be initialised to all-zeros,
>    which is appropriate.
>    
>    If a GMutex is placed in other contexts (eg: embedded in a struct) then it
>    must be explicitly initialised using g_mutex_init().
> 
> So yes, in the constructor we need to drop g_mutex_init() since C++ object are
> not memset to zero, unlike GObject.

Oh, and with that fixed, you can add a second Rb

Reviewed-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>

> 
> > 
> > 
> > Regards,
> > Barnabás Pőcze
>