[libcamera-devel] [PATCH 11/13] gstreamer: Split request creation to a separate function

[Laurent Pinchart]

Hi Nicolas,

On Tue, Jun 28, 2022 at 09:26:13AM -0400, Nicolas Dufresne wrote:
> Le vendredi 24 juin 2022 à 02:22 +0300, Laurent Pinchart a écrit :
> > In order to prepare for creation and queuing of multiple requests, move
> > the request creation and queueing code to a separate function. No
> > functional change intended.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  src/gstreamer/gstlibcamerasrc.cpp | 76 +++++++++++++++++--------------
> >  1 file changed, 42 insertions(+), 34 deletions(-)
> > 
> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> > index 58a322b251c7..fb39d6093a3f 100644
> > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > @@ -123,6 +123,7 @@ struct GstLibcameraSrcState {
> >  
> >  	guint group_id_;
> >  
> > +	int queueRequest();
> >  	void requestCompleted(Request *request);
> >  };
> >  
> > @@ -160,6 +161,44 @@ GstStaticPadTemplate request_src_template = {
> >  	"src_%u", GST_PAD_SRC, GST_PAD_REQUEST, TEMPLATE_CAPS
> >  };
> >  
> > +int GstLibcameraSrcState::queueRequest()
> 
> This function must be called with stream lock held, please comment above the
> function.
> 
> > +{
> > +	std::unique_ptr<Request> request = cam_->createRequest();
> > +	if (!request)
> > +		return -ENOMEM;
> > +
> > +	std::unique_ptr<RequestWrap> wrap =
> > +		std::make_unique<RequestWrap>(std::move(request));
> > +
> > +	for (GstPad *srcpad : srcpads_) {
> > +		Stream *stream = gst_libcamera_pad_get_stream(srcpad);
> > +		GstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad);
> > +		GstBuffer *buffer;
> > +		GstFlowReturn ret;
> > +
> > +		ret = gst_buffer_pool_acquire_buffer(GST_BUFFER_POOL(pool),
> > +						     &buffer, nullptr);
> > +		if (ret != GST_FLOW_OK) {
> > +			/*
> > +			 * RequestWrap has ownership of the request, and we
> > +			 * won't be queueing this one due to lack of buffers.
> > +			 */
> > +			return -ENOBUFS;
> > +		}
> > +
> > +		wrap->attachBuffer(stream, buffer);
> > +	}
> > +
> > +	GST_TRACE_OBJECT(src_, "Requesting buffers");
> > +	cam_->queueRequest(wrap->request_.get());
> > +
> > +	MutexLocker locker(lock_);
> > +	queuedRequests_.push(std::move(wrap));
> 
> Possible maintenance trap, add a scope around the locker perhaps ?

Both are good points, I'll fix that.

> > +
> > +	/* The RequestWrap will be deleted in the completion handler. */
> > +	return 0;
> > +}
> > +
> >  void
> >  GstLibcameraSrcState::requestCompleted(Request *request)
> >  {
> > @@ -269,8 +308,8 @@ gst_libcamera_src_task_run(gpointer user_data)
> >  	GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);
> >  	GstLibcameraSrcState *state = self->state;
> >  
> > -	std::unique_ptr<Request> request = state->cam_->createRequest();
> > -	if (!request) {
> > +	int err = state->queueRequest();
> > +	if (err == -ENOMEM) {
> >  		GST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT,
> >  				  ("Failed to allocate request for camera '%s'.",
> >  				   state->cam_->id().c_str()),
> > @@ -279,38 +318,7 @@ gst_libcamera_src_task_run(gpointer user_data)
> >  		return;
> >  	}
> >  
> > -	std::unique_ptr<RequestWrap> wrap =
> > -		std::make_unique<RequestWrap>(std::move(request));
> > -
> > -	for (GstPad *srcpad : state->srcpads_) {
> > -		Stream *stream = gst_libcamera_pad_get_stream(srcpad);
> > -		GstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad);
> > -		GstBuffer *buffer;
> > -		GstFlowReturn ret;
> > -
> > -		ret = gst_buffer_pool_acquire_buffer(GST_BUFFER_POOL(pool),
> > -						     &buffer, nullptr);
> > -		if (ret != GST_FLOW_OK) {
> > -			/*
> > -			 * RequestWrap has ownership of the request, and we
> > -			 * won't be queueing this one due to lack of buffers.
> > -			 */
> > -			wrap.release();
> > -			break;
> > -		}
> > -
> > -		wrap->attachBuffer(stream, buffer);
> > -	}
> > -
> > -	if (wrap) {
> > -		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. */
> > -	}
> > +	std::unique_ptr<RequestWrap> wrap;
> >  
> >  	{
> >  		MutexLocker locker(state->lock_);
> 
> Nothing major here:
> 
> Reviewed-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>

-- 
Regards,

Laurent Pinchart