[libcamera-devel,v2,10/12] gstreamer: Split request creation to a separate function

16447 diff mbox series

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@ideasonboard.com>
Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
Changes since v1:

- Add comment about locking requirement
- Add a scope around the locker
---
 src/gstreamer/gstlibcamerasrc.cpp | 79 ++++++++++++++++++-------------
 1 file changed, 45 insertions(+), 34 deletions(-)

Hi Laurent,

On 6/30/22 05:32, Laurent Pinchart via libcamera-devel wrote:
> 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@ideasonboard.com>
> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> ---
> Changes since v1:
>
> - Add comment about locking requirement
> - Add a scope around the locker
> ---
>   src/gstreamer/gstlibcamerasrc.cpp | 79 ++++++++++++++++++-------------
>   1 file changed, 45 insertions(+), 34 deletions(-)
>
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index c92ca7d29fe6..d63083d0cd8f 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -133,6 +133,7 @@ struct GstLibcameraSrcState {
>   
>   	guint group_id_;
>   
> +	int queueRequest();
>   	void requestCompleted(Request *request);
>   };
>   
> @@ -170,6 +171,47 @@ GstStaticPadTemplate request_src_template = {
>   	"src_%u", GST_PAD_SRC, GST_PAD_REQUEST, TEMPLATE_CAPS
>   };
>   
> +/* Must be called with stream_lock held. */


Took a while to figure this out, where it's held for queueRequest but it 
was actually gst_task_set_lock()

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

> +int GstLibcameraSrcState::queueRequest()
> +{
> +	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));
> +	}
> +
> +	/* The RequestWrap will be deleted in the completion handler. */
> +	return 0;
> +}
> +
>   void
>   GstLibcameraSrcState::requestCompleted(Request *request)
>   {
> @@ -279,8 +321,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()),
> @@ -289,38 +331,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_);