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

[Kieran Bingham]

On 02/03/2021 10:39, Laurent Pinchart wrote:
> 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;

We used to validate the stream here...

>>>> -	}
>>>> +	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();

Should we have a helper to convert from stream to index, which includes
a check to validate that it is within streams_.size() ?

That would add some safety to here, and the usage above in
exportFrameBuffers().


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

Does this incorrectly requeue buffers if we're stopping? (I.e. if we
were 'FrameCancelled' or such?


>>>>  
>>>> -			/*
>>>> -			 * 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.

I presume at this point no buffer has been passed to any convertor, so
there can't be anything happening in parallel at this point.

I don't think there can be so:

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>



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