[libcamera-devel] [PATCH 09/13] gstreamer: Use dedicated lock for request queues

[Nicolas Dufresne]

Le mardi 28 juin 2022 à 16:45 +0300, Laurent Pinchart a écrit :
> Hi Nicolas,
> 
> On Tue, Jun 28, 2022 at 09:05:52AM -0400, Nicolas Dufresne wrote:
> > Le vendredi 24 juin 2022 à 02:22 +0300, Laurent Pinchart a écrit :
> > > Add a new lock to the GstLibcameraSrcState class to protect the queued
> > > and completed requests queues. This replaces the GstObject lock, and
> > > minimizes the lock contention between the request completion handler and
> > > the task run handler as the former must run as fast as possible.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > ---
> > >  src/gstreamer/gstlibcamerasrc.cpp | 39 ++++++++++++++++++++++---------
> > >  1 file changed, 28 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> > > index e30d45fa2223..b85ba39fb808 100644
> > > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > > @@ -32,6 +32,8 @@
> > >  #include <queue>
> > >  #include <vector>
> > >  
> > > +#include <libcamera/base/mutex.h>
> > > +
> > >  #include <libcamera/camera.h>
> > >  #include <libcamera/camera_manager.h>
> > >  #include <libcamera/control_ids.h>
> > > @@ -111,8 +113,13 @@ struct GstLibcameraSrcState {
> > >  	std::shared_ptr<Camera> cam_;
> > >  	std::unique_ptr<CameraConfiguration> config_;
> > >  	std::vector<GstPad *> srcpads_;
> > > -	std::queue<std::unique_ptr<RequestWrap>> queuedRequests_;
> > > -	std::queue<std::unique_ptr<RequestWrap>> completedRequests_;
> > > +
> > > +	Mutex lock_;
> > 
> > I would love to see a comment explaining what this lock is for so that future
> > contributors knows when to take it, and when not.
> > 
> > Did you know that GLib Mutex are a lot faster then pthread_mutex ? Might sounds
> > surprising, but they don't obey all of the POSIX requirement, they remains a
> > perfect fit for streaming. I personally would have used these over a wrapper on
> > top of pthread, the change is fine though, I just thought of mentioning as you
> > already made pretty micro optimization removing few function call earlier in the
> > patchset. IIRC this is in the 20x level of improvement, at least on Linux.
> > 
> > > +	std::queue<std::unique_ptr<RequestWrap>> queuedRequests_
> > > +		LIBCAMERA_TSA_GUARDED_BY(lock_);
> > > +	std::queue<std::unique_ptr<RequestWrap>> completedRequests_
> > > +		LIBCAMERA_TSA_GUARDED_BY(lock_);
> > 
> > Of course, that would require extra effort to implement this "guarded by" thing.
> > I'm just mentioning, not action needed. There is also GstAtomicQueue if (and
> > only if) contention isn't expected.
> 
> The thread-safefy annotation is the reason why I picked the Mutex class,
> as it addresses your comment about documenting the data protected by the
> lock. It's also nice to get the compiler to validate the locking
> patterns. It's not strictly mandatory though.
> 
> I wasn't aware of the differences in performance between the pthread
> mutex implementation and the GLib implementation, and I don't know how
> much difference it would make in practice here. Would you rather replace
> Mutex + TSA with GLibMutex + a comment ? Or should this be done on top
> when/if needed ?

Only if needed. It does matter for lower latency audio, but for video which is
pretty low rate it will rarely make a big difference. This comment was just to
keep you inform that GMutex is not a pthread_mutex_t (and not POSIX compliant,
which is the reason pthread under perform). It might matter when we start
handling frame burst and highly concurrent streaming (e.g. 100 or more streams).

> 
> > > +
> > >  	guint group_id_;
> > >  
> > >  	void requestCompleted(Request *request);
> > > @@ -155,12 +162,15 @@ GstStaticPadTemplate request_src_template = {
> > >  void
> > >  GstLibcameraSrcState::requestCompleted(Request *request)
> > >  {
> > > -	GLibLocker lock(GST_OBJECT(src_));
> > > -
> > >  	GST_DEBUG_OBJECT(src_, "buffers are ready");
> > >  
> > > -	std::unique_ptr<RequestWrap> wrap = std::move(queuedRequests_.front());
> > > -	queuedRequests_.pop();
> > > +	std::unique_ptr<RequestWrap> wrap;
> > > +
> > > +	{
> > > +		MutexLocker locker(lock_);
> > > +		wrap = std::move(queuedRequests_.front());
> > > +		queuedRequests_.pop();
> > > +	}
> > >  
> > >  	g_return_if_fail(wrap->request_.get() == request);
> > >  
> > > @@ -183,7 +193,10 @@ GstLibcameraSrcState::requestCompleted(Request *request)
> > >  		wrap->latency_ = sys_now - timestamp;
> > >  	}
> > >  
> > > -	completedRequests_.push(std::move(wrap));
> > > +	{
> > > +		MutexLocker locker(lock_);
> > > +		completedRequests_.push(std::move(wrap));
> > > +	}
> > >  
> > >  	gst_task_resume(src_->task);
> > >  }
> > > @@ -289,16 +302,17 @@ gst_libcamera_src_task_run(gpointer user_data)
> > >  	}
> > >  
> > >  	if (wrap) {
> > > -		GLibLocker lock(GST_OBJECT(self));
> > >  		GST_TRACE_OBJECT(self, "Requesting buffers");
> > >  		state->cam_->queueRequest(wrap->request_.get());
> > > +
> > > +		MutexLocker locker(state->lock_);
> > >  		state->queuedRequests_.push(std::move(wrap));
> > >  
> > >  		/* The RequestWrap will be deleted in the completion handler. */
> > >  	}
> > >  
> > >  	{
> > > -		GLibLocker lock(GST_OBJECT(self));
> > > +		MutexLocker locker(state->lock_);
> > >  
> > >  		if (!state->completedRequests_.empty()) {
> > >  			wrap = std::move(state->completedRequests_.front());
> > > @@ -358,7 +372,7 @@ gst_libcamera_src_task_run(gpointer user_data)
> > >  		bool do_pause;
> > >  
> > >  		{
> > > -			GLibLocker lock(GST_OBJECT(self));
> > > +			MutexLocker locker(state->lock_);
> > >  			do_pause = state->completedRequests_.empty();
> > >  		}
> > >  
> > > @@ -513,7 +527,10 @@ gst_libcamera_src_task_leave([[maybe_unused]] GstTask *task,
> > >  
> > >  	state->cam_->stop();
> > >  
> > > -	state->completedRequests_ = {};
> > > +	{
> > > +		MutexLocker locker(state->lock_);
> > > +		state->completedRequests_ = {};
> > > +	}
> > >  
> > >  	for (GstPad *srcpad : state->srcpads_)
> > >  		gst_libcamera_pad_set_pool(srcpad, nullptr);
> > 
> > Does not solve concurrency between pads iteration and request/release pads, but
> > this wasn't the goal and was already broken.
> 
> Hopefully the next patches do :-)
> 
> > Reviewed-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>
>