| Message ID | 20230626211747.101412-1-umang.jain@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi 2023. június 26., hétfő 23:17 keltezéssel, Umang Jain via libcamera-devel <libcamera-devel@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@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()`? Regards, Barnabás Pőcze
Quoting Barnabás Pőcze via libcamera-devel (2023-06-26 22:52:18) > Hi > > > 2023. június 26., hétfő 23:17 keltezéssel, Umang Jain via libcamera-devel <libcamera-devel@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. > > Excellent plan! Indeed - gstilbcamerasrc should definitely only use the public API. > > 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). I think that's reasonable in this instance. > > > > Signed-off-by: Umang Jain <umang.jain@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()`? Looking at https://docs.gtk.org/glib/method.Mutex.init.html it will also need a g_mutex_clear(); But with those: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > Regards, > Barnabás Pőcze
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@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@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. > > > Regards, > Barnabás Pőcze
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@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@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@collabora.com> > > > > > > > Regards, > > Barnabás Pőcze >
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_; + std::queue<std::unique_ptr<RequestWrap>> queuedRequests_; + std::queue<std::unique_ptr<RequestWrap>> completedRequests_; ControlList initControls_; guint group_id_; @@ -208,7 +204,7 @@ int GstLibcameraSrcState::queueRequest() cam_->queueRequest(wrap->request_.get()); { - MutexLocker locker(lock_); + GLibLocker locker(&lock_); queuedRequests_.push(std::move(wrap)); } @@ -224,7 +220,7 @@ GstLibcameraSrcState::requestCompleted(Request *request) std::unique_ptr<RequestWrap> wrap; { - MutexLocker locker(lock_); + GLibLocker locker(&lock_); wrap = std::move(queuedRequests_.front()); queuedRequests_.pop(); } @@ -251,7 +247,7 @@ GstLibcameraSrcState::requestCompleted(Request *request) } { - MutexLocker locker(lock_); + GLibLocker locker(&lock_); completedRequests_.push(std::move(wrap)); } @@ -265,7 +261,7 @@ int GstLibcameraSrcState::processRequest() int err = 0; { - MutexLocker locker(lock_); + GLibLocker locker(&lock_); if (!completedRequests_.empty()) { wrap = std::move(completedRequests_.front()); @@ -639,7 +635,7 @@ gst_libcamera_src_task_leave([[maybe_unused]] GstTask *task, state->cam_->stop(); { - MutexLocker locker(state->lock_); + GLibLocker locker(&state->lock_); state->completedRequests_ = {}; } diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build index eda246d7..77c79140 100644 --- a/src/gstreamer/meson.build +++ b/src/gstreamer/meson.build @@ -42,7 +42,7 @@ endif libcamera_gst = shared_library('gstlibcamera', libcamera_gst_sources, cpp_args : libcamera_gst_cpp_args, - dependencies : [libcamera_private, gstvideo_dep, gstallocator_dep], + dependencies : [libcamera_public, gstvideo_dep, gstallocator_dep], install: true, install_dir : '@0@/gstreamer-1.0'.format(get_option('libdir')), )
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@ideasonboard.com> --- src/gstreamer/gstlibcamerasrc.cpp | 20 ++++++++------------ src/gstreamer/meson.build | 2 +- 2 files changed, 9 insertions(+), 13 deletions(-)