[libcamera-devel] [PATCH 19/20] libcamera: pipeline: simple: Support usage of multiple streams

[paul.elder at ideasonboard.com]

Hi Laurent,

On Mon, Feb 01, 2021 at 12:47:01AM +0200, Laurent Pinchart wrote:
> To extend the multi-stream support to runtime operation of the pipeline,
> expand the converter queue to store multiple output buffers, and update
> the request queuing and buffer completion handlers accordingly.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/libcamera/pipeline/simple/simple.cpp | 93 ++++++++++++++----------
>  1 file changed, 54 insertions(+), 39 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 58e5f0d23139..55a5528611c8 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -173,7 +173,7 @@ public:
>  
>  	std::vector<std::unique_ptr<FrameBuffer>> converterBuffers_;
>  	bool useConverter_;
> -	std::queue<FrameBuffer *> converterQueue_;
> +	std::queue<std::map<unsigned int, FrameBuffer *>> converterQueue_;
>  };
>  
>  class SimpleCameraConfiguration : public CameraConfiguration
> @@ -762,10 +762,12 @@ int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,
>  	 * Export buffers on the converter or capture video node, depending on
>  	 * whether the converter is used or not.
>  	 */
> -	if (data->useConverter_)
> -		return converter_->exportBuffers(0, count, buffers);
> -	else
> +	if (data->useConverter_) {
> +		unsigned int index = stream - &data->streams_.front();
> +		return converter_->exportBuffers(index, count, buffers);
> +	} else {
>  		return data->video_->exportBuffers(count, buffers);
> +	}
>  }
>  
>  int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] ControlList *controls)
> @@ -830,25 +832,30 @@ void SimplePipelineHandler::stop(Camera *camera)
>  int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)
>  {
>  	SimpleCameraData *data = cameraData(camera);
> -	Stream *stream = &data->streams_[0];
> +	int ret;
>  
> -	FrameBuffer *buffer = request->findBuffer(stream);
> -	if (!buffer) {
> -		LOG(SimplePipeline, Error)
> -			<< "Attempt to queue request with invalid stream";
> -		return -ENOENT;
> -	}
> +	std::map<unsigned int, FrameBuffer *> buffers;
>  
> -	/*
> -	 * If conversion is needed, push the buffer to the converter queue, it
> -	 * will be handed to the converter in the capture completion handler.
> -	 */
> -	if (data->useConverter_) {
> -		data->converterQueue_.push(buffer);
> -		return 0;
> +	for (auto &[stream, buffer] : request->buffers()) {
> +		/*
> +		 * If conversion is needed, push the buffer to the converter
> +		 * queue, it will be handed to the converter in the capture
> +		 * completion handler.
> +		 */
> +		if (data->useConverter_) {
> +			unsigned int index = stream - &data->streams_.front();
> +			buffers.emplace(index, buffer);
> +		} else {
> +			ret = data->video_->queueBuffer(buffer);
> +			if (ret < 0)
> +				return ret;
> +		}
>  	}
>  
> -	return data->video_->queueBuffer(buffer);
> +	if (data->useConverter_)
> +		data->converterQueue_.push(std::move(buffers));
> +
> +	return 0;
>  }
>  
>  /* -----------------------------------------------------------------------------
> @@ -1020,24 +1027,34 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer)
>  	 * point converting an erroneous buffer.
>  	 */
>  	if (buffer->metadata().status != FrameMetadata::FrameSuccess) {
> -		if (data->useConverter_) {
> -			/* Requeue the buffer for capture. */
> -			data->video_->queueBuffer(buffer);
> +		if (!data->useConverter_) {
> +			/* No conversion, just complete the request. */
> +			Request *request = buffer->request();
> +			completeBuffer(request, buffer);
> +			completeRequest(request);
> +			return;
> +		}
> +
> +		/*
> +		 * The converter is in use. Requeue the internal buffer for
> +		 * capture, and complete the request with all the user-facing
> +		 * buffers.
> +		 */
> +		data->video_->queueBuffer(buffer);
>  
> -			/*
> -			 * Get the next user-facing buffer to complete the
> -			 * request.
> -			 */
> -			if (data->converterQueue_.empty())
> -				return;
> +		if (data->converterQueue_.empty())
> +			return;
>  
> -			buffer = data->converterQueue_.front();
> -			data->converterQueue_.pop();
> +		Request *request = nullptr;
> +		for (auto &item : data->converterQueue_.front()) {
> +			FrameBuffer *outputBuffer = item.second;
> +			request = outputBuffer->request();
> +			completeBuffer(request, outputBuffer);
>  		}
> +		data->converterQueue_.pop();
>  
> -		Request *request = buffer->request();
> -		completeBuffer(request, buffer);
> -		completeRequest(request);
> +		if (request)

This check doesn't seem necessary, as we return early if the
converterQueue_ is empty, so the loop will always run once. Is it just
to appease coverity?


Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>

> +			completeRequest(request);
>  		return;
>  	}
>  
> @@ -1052,10 +1069,8 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer)
>  			return;
>  		}
>  
> -		FrameBuffer *output = data->converterQueue_.front();
> +		converter_->queueBuffers(buffer, data->converterQueue_.front());
>  		data->converterQueue_.pop();
> -
> -		converter_->queueBuffers(buffer, { { 0, output } });
>  		return;
>  	}
>  
> @@ -1078,10 +1093,10 @@ void SimplePipelineHandler::converterOutputDone(FrameBuffer *buffer)
>  {
>  	ASSERT(activeCamera_);
>  
> -	/* Complete the request. */
> +	/* Complete the buffer and the request. */
>  	Request *request = buffer->request();
> -	completeBuffer(request, buffer);
> -	completeRequest(request);
> +	if (completeBuffer(request, buffer))
> +		completeRequest(request);
>  }
>  
>  REGISTER_PIPELINE_HANDLER(SimplePipelineHandler)