[libcamera-devel] [PATCH v2 06/12] gstreamer: Handle completed requests in the libcamerasrc task

[Umang Jain]

Hi Laurent,

Thank you for the patch.

On 6/30/22 05:32, Laurent Pinchart via libcamera-devel wrote:
> Move the request wrap to a completed queue in the request completion
> handler to move more of the request completion processing to the
> libcamerasrc task. This lowers the amount of time spent in the
> completion handler, and prepares for reworking the usage of locks.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> Changes since v1:
>
> - Use GST_CLOCK_TIME_NONE and GST_CLOCK_TIME_IS_VALID()
> ---
>   src/gstreamer/gstlibcamerasrc.cpp | 82 ++++++++++++++++++-------------
>   1 file changed, 48 insertions(+), 34 deletions(-)
>
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index f2f48d53991a..2cb915637874 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -57,10 +57,13 @@ struct RequestWrap {
>   
>   	std::unique_ptr<Request> request_;
>   	std::map<Stream *, GstBuffer *> buffers_;
> +
> +	GstClockTime latency_;
> +	GstClockTime pts_;
>   };
>   
>   RequestWrap::RequestWrap(std::unique_ptr<Request> request)
> -	: request_(std::move(request))
> +	: request_(std::move(request)), latency_(0), pts_(GST_CLOCK_TIME_NONE)
>   {
>   }
>   
> @@ -109,6 +112,7 @@ struct GstLibcameraSrcState {
>   	std::unique_ptr<CameraConfiguration> config_;
>   	std::vector<GstPad *> srcpads_;
>   	std::queue<std::unique_ptr<RequestWrap>> queuedRequests_;
> +	std::queue<std::unique_ptr<RequestWrap>> completedRequests_;
>   	guint group_id_;
>   
>   	void requestCompleted(Request *request);
> @@ -165,9 +169,6 @@ GstLibcameraSrcState::requestCompleted(Request *request)
>   		return;
>   	}
>   
> -	GstClockTime latency;
> -	GstClockTime pts;
> -
>   	if (GST_ELEMENT_CLOCK(src_)) {
>   		int64_t timestamp = request->metadata().get(controls::SensorTimestamp);
>   
> @@ -178,31 +179,11 @@ GstLibcameraSrcState::requestCompleted(Request *request)
>   
>   		/* Deduced from: sys_now - sys_base_time == gst_now - gst_base_time */
>   		GstClockTime sys_base_time = sys_now - (gst_now - gst_base_time);
> -		pts = timestamp - sys_base_time;
> -		latency = sys_now - timestamp;
> -	} else {
> -		latency = 0;
> -		pts = GST_CLOCK_TIME_NONE;
> +		wrap->pts_ = timestamp - sys_base_time;
> +		wrap->latency_ = sys_now - timestamp;
>   	}
>   
> -	for (GstPad *srcpad : srcpads_) {
> -		Stream *stream = gst_libcamera_pad_get_stream(srcpad);
> -		GstBuffer *buffer = wrap->detachBuffer(stream);
> -
> -		FrameBuffer *fb = gst_libcamera_buffer_get_frame_buffer(buffer);
> -
> -		if (GST_CLOCK_TIME_IS_VALID(pts)) {
> -			GST_BUFFER_PTS(buffer) = pts;
> -			gst_libcamera_pad_set_latency(srcpad, latency);
> -		} else {
> -			GST_BUFFER_PTS(buffer) = 0;
> -		}
> -
> -		GST_BUFFER_OFFSET(buffer) = fb->metadata().sequence;
> -		GST_BUFFER_OFFSET_END(buffer) = fb->metadata().sequence;
> -
> -		gst_libcamera_pad_queue_buffer(srcpad, buffer);
> -	}
> +	completedRequests_.push(std::move(wrap));
>   
>   	gst_task_resume(src_->task);
>   }
> @@ -316,6 +297,39 @@ gst_libcamera_src_task_run(gpointer user_data)
>   		/* The RequestWrap will be deleted in the completion handler. */
>   	}
>   
> +	{
> +		GLibLocker lock(GST_OBJECT(self));
> +
> +		if (!state->completedRequests_.empty()) {
> +			wrap = std::move(state->completedRequests_.front());
> +			state->completedRequests_.pop();
> +		}
> +	}
> +
> +	if (!wrap) {
> +		gst_task_pause(self->task);
> +		return;
> +	}
> +
> +	for (GstPad *srcpad : state->srcpads_) {
> +		Stream *stream = gst_libcamera_pad_get_stream(srcpad);
> +		GstBuffer *buffer = wrap->detachBuffer(stream);
> +
> +		FrameBuffer *fb = gst_libcamera_buffer_get_frame_buffer(buffer);
> +
> +		if (GST_CLOCK_TIME_IS_VALID(wrap->pts_)) {
> +			GST_BUFFER_PTS(buffer) = wrap->pts_;
> +			gst_libcamera_pad_set_latency(srcpad, wrap->latency_);
> +		} else {
> +			GST_BUFFER_PTS(buffer) = 0;
> +		}
> +
> +		GST_BUFFER_OFFSET(buffer) = fb->metadata().sequence;
> +		GST_BUFFER_OFFSET_END(buffer) = fb->metadata().sequence;
> +
> +		gst_libcamera_pad_queue_buffer(srcpad, buffer);
> +	}
> +
>   	GstFlowReturn ret = GST_FLOW_OK;
>   	gst_flow_combiner_reset(self->flow_combiner);
>   	for (GstPad *srcpad : state->srcpads_) {
> @@ -344,13 +358,11 @@ gst_libcamera_src_task_run(gpointer user_data)
>   		 * happen in lock step with the callback thread which may want
>   		 * to resume the task and might push pending buffers.
>   		 */
> -		GLibLocker lock(GST_OBJECT(self));
> -		bool do_pause = true;
> -		for (GstPad *srcpad : state->srcpads_) {
> -			if (gst_libcamera_pad_has_pending(srcpad)) {
> -				do_pause = false;
> -				break;
> -			}
> +		bool do_pause;
> +
> +		{
> +			GLibLocker lock(GST_OBJECT(self));
> +			do_pause = state->completedRequests_.empty();
>   		}
>   
>   		if (do_pause)
> @@ -504,6 +516,8 @@ gst_libcamera_src_task_leave([[maybe_unused]] GstTask *task,
>   
>   	state->cam_->stop();
>   
> +	state->completedRequests_ = {};
> +


Ah so when we stop we might have some RequestWrap(s) being added to 
completedRequests_ but won't be processed by the libcamera_src_task_run 
- since we have are already in libcamera_src_task_leave.

I wonder if need to do any 'processing' for them before clearing them 
out here. Maybe not, those are just "cancelled" ones right?

So, the 'processing' part for completedRequests_ is just, detaching the 
buffer, setting up latency and presentation time(pts I believe) and 
re-queuing the buffer the pad. So that's minimal, so I guess it's safe 
to clear them out.

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

>   	for (GstPad *srcpad : state->srcpads_)
>   		gst_libcamera_pad_set_pool(srcpad, nullptr);
>