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

[Laurent Pinchart]

Hi again,

On Tue, Mar 02, 2021 at 12:34:10PM +0200, Laurent Pinchart wrote:
> 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.

Actually, we're not looping over data->converterQueue_, but over
converterQueue_.front(). While it should never be empty, I think it will
be difficult for compilers (and coverity) to know that, and a check here
can also act as a bit of defensive programming. I think I'd prefer
keeping the check.

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