[PATCH v5 2/3] libcamera: simple: Track requests in conversionQueue_

[Hans de Goede]

Hi,

On 21-Oct-24 3:37 PM, Milan Zamazal wrote:
> Simple pipeline retrieves the requests to complete from the
> conversionQueue_.  This patch stores the requests in conversionQueue_
> explicitly.  This explicit tracking is supposed to be preferred to
> implicit retrieval and it simplifies the completion code a bit here and
> in the followup patch that adds request cleanup on stop.
> 
> The change as implemented assumes that all the buffers in each of the
> conversionQueue_ elements point to the same request, the one specified.
> 
> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
> Tested-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede at redhat.com>

Regards,

Hans




> ---
>  src/libcamera/pipeline/simple/simple.cpp | 26 ++++++++++++------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 3ddce71d3..f3a6b1635 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -282,7 +282,11 @@ public:
>  	std::unique_ptr<DelayedControls> delayedCtrls_;
>  
>  	std::vector<std::unique_ptr<FrameBuffer>> conversionBuffers_;
> -	std::queue<std::map<const Stream *, FrameBuffer *>> conversionQueue_;
> +	struct RequestOutputs {
> +		Request *request;
> +		std::map<const Stream *, FrameBuffer *> outputs;
> +	};
> +	std::queue<RequestOutputs> conversionQueue_;
>  	bool useConversion_;
>  
>  	std::unique_ptr<Converter> converter_;
> @@ -813,16 +817,12 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)
>  		if (conversionQueue_.empty())
>  			return;
>  
> -		Request *request = nullptr;
> -		for (auto &item : conversionQueue_.front()) {
> -			FrameBuffer *outputBuffer = item.second;
> -			request = outputBuffer->request();
> -			pipe->completeBuffer(request, outputBuffer);
> -		}
> +		Request *request = conversionQueue_.front().request;
> +		for (auto &item : conversionQueue_.front().outputs)
> +			pipe->completeBuffer(request, item.second);
> +		pipe->completeRequest(request);
>  		conversionQueue_.pop();
>  
> -		if (request)
> -			pipe->completeRequest(request);
>  		return;
>  	}
>  
> @@ -838,7 +838,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)
>  
>  	if (useConversion_ && !conversionQueue_.empty()) {
>  		const std::map<const Stream *, FrameBuffer *> &outputs =
> -			conversionQueue_.front();
> +			conversionQueue_.front().outputs;
>  		if (!outputs.empty()) {
>  			FrameBuffer *outputBuffer = outputs.begin()->second;
>  			if (outputBuffer)
> @@ -862,7 +862,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)
>  		}
>  
>  		if (converter_)
> -			converter_->queueBuffers(buffer, conversionQueue_.front());
> +			converter_->queueBuffers(buffer, conversionQueue_.front().outputs);
>  		else
>  			/*
>  			 * request->sequence() cannot be retrieved from `buffer' inside
> @@ -870,7 +870,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)
>  			 * already here.
>  			 */
>  			swIsp_->queueBuffers(request->sequence(), buffer,
> -					     conversionQueue_.front());
> +					     conversionQueue_.front().outputs);
>  
>  		conversionQueue_.pop();
>  		return;
> @@ -1434,7 +1434,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)
>  	}
>  
>  	if (data->useConversion_) {
> -		data->conversionQueue_.push(std::move(buffers));
> +		data->conversionQueue_.push({ .request = request, .outputs = std::move(buffers) });
>  		if (data->swIsp_)
>  			data->swIsp_->queueRequest(request->sequence(), request->controls());
>  	}