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

[Laurent Pinchart]

Hi Paul,

On Tue, Mar 02, 2021 at 04:09:33PM +0900, paul.elder at ideasonboard.com wrote:
> 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?

You're right, I'll drop that.

> 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)

-- 
Regards,

Laurent Pinchart