[libcamera-devel] [PATCH v4 8/9] libcamera: pipeline: raspberrypi: Add more robust stream buffer logic

[Kieran Bingham]

Hi Naush,

On 20/07/2020 10:13, Naushir Patuck wrote:
> Add further queueing into the RPiStream object to ensure that we always
> follow the buffer ordering (be it internal or external) given by
> incoming Requests.
> 
> This is essential, otherwise we risk dropping frames that are meant to
> be part of a Request, and can cause the pipeline to stall indefinitely.
> This also prevents any possibility of mismatched frame buffers going
> through the pipeline and out to the application.
> 

Sounds helpful... some questions below ... but I suspect I've just
misinterpreted something...

> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  .../pipeline/raspberrypi/rpi_stream.cpp       | 70 ++++++++++++++++---
>  .../pipeline/raspberrypi/rpi_stream.h         | 12 +++-
>  2 files changed, 71 insertions(+), 11 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> index 02f8d3e0..6635a751 100644
> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> @@ -112,25 +112,34 @@ int RPiStream::queueBuffer(FrameBuffer *buffer)
>  	 */
>  	if (!buffer) {
>  		if (availableBuffers_.empty()) {
> -			LOG(RPISTREAM, Warning) << "No buffers available for "
> +			LOG(RPISTREAM, Info) << "No buffers available for "
>  						<< name_;
> -			return -EINVAL;
> +			/*
> +			 * Note that we need to requeue an internal buffer as soon
> +			 * as one becomes available.
> +			 */
> +			requeueBuffers_.push(nullptr);
> +			return 0;
>  		}
>  
>  		buffer = availableBuffers_.front();
>  		availableBuffers_.pop();
>  	}
>  
> -	LOG(RPISTREAM, Debug) << "Queuing buffer " << buffer->cookie()
> -			      << " for " << name_;
> +	/*
> +	 * If no earlier requests are pending to be queued we can go ahead and
> +	 * queue the buffer into the device.
> +	 */
> +	if (requeueBuffers_.empty())
> +		return queueToDevice(buffer);
>  
> -	int ret = dev_->queueBuffer(buffer);
> -	if (ret) {
> -		LOG(RPISTREAM, Error) << "Failed to queue buffer for "
> -				      << name_;
> -	}
> +	/*
> +	 * There are earlier buffers to be queued, so this bufferm ust go on


s/bufferm ust/buffer must/

That could be fixed up while applying. It's minor.



> +	 * the waiting list.
> +	 */
> +	requeueBuffers_.push(buffer);
>  
> -	return ret;
> +	return 0;
>  }
>  
>  void RPiStream::returnBuffer(FrameBuffer *buffer)
> @@ -138,7 +147,35 @@ void RPiStream::returnBuffer(FrameBuffer *buffer)
>  	/* This can only be called for external streams. */
>  	assert(external_);
>  
> +	/* Push this buffer back into the queue to be used again. */
>  	availableBuffers_.push(buffer);
> +
> +	/*
> +	 * Do we have any buffers that are waiting to be queued?
> +	 * If so, do it now as availableBuffers_ will not be empty.
> +	 */
> +	while (!requeueBuffers_.empty()) {
> +		FrameBuffer *buffer = requeueBuffers_.front();
> +
> +		if (!buffer) {
> +			/*
> +			 * We want to queue an internal buffer, but none
> +			 * are available. Can't do anything, quit the loop.
> +			 */
> +			if (availableBuffers_.empty())
> +				break;

Can this happen?

This looks like:

while(requeueBuffer exist)
	buffer = first buffer from requeueBuffers
	if (no buffer) {
		/* unreachable code ? */
	}



> +
> +			/*
> +			 * We want to queue an internal buffer, and at least one
> +			 * is available.
> +			 */
> +			buffer = availableBuffers_.front();
> +			availableBuffers_.pop();
> +		}
> +
> +		requeueBuffers_.pop();
> +		queueToDevice(buffer);
> +	}
>  }
>  
>  bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const
> @@ -155,10 +192,23 @@ bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const
>  void RPiStream::clearBuffers()
>  {
>  	availableBuffers_ = std::queue<FrameBuffer *>{};
> +	requeueBuffers_ = std::queue<FrameBuffer *>{};
>  	internalBuffers_.clear();
>  	bufferList_.clear();
>  }
>  
> +int RPiStream::queueToDevice(FrameBuffer *buffer)
> +{
> +	LOG(RPISTREAM, Debug) << "Queuing buffer " << buffer->cookie()
> +			      << " for " << name_;
> +
> +	int ret = dev_->queueBuffer(buffer);
> +	if (ret)
> +		LOG(RPISTREAM, Error) << "Failed to queue buffer for "
> +				      << name_;
> +	return ret;
> +}
> +
>  } /* namespace RPi */
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> index af9c2ad2..6222c867 100644
> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> @@ -52,6 +52,7 @@ public:
>  
>  private:
>  	void clearBuffers();
> +	int queueToDevice(FrameBuffer *buffer);
>  	/*
>  	 * Indicates that this stream is active externally, i.e. the buffers
>  	 * might be provided by (and returned to) the application.
> @@ -63,7 +64,7 @@ private:
>  	std::string name_;
>  	/* The actual device stream. */
>  	std::unique_ptr<V4L2VideoDevice> dev_;
> -	/* All framebuffers associated with this device stream. */
> +	/* All frame buffers associated with this device stream. */
>  	std::vector<FrameBuffer *> bufferList_;
>  	/*
>  	 * List of frame buffer that we can use if none have been provided by
> @@ -71,6 +72,15 @@ private:
>  	 * buffers exported internally.
>  	 */
>  	std::queue<FrameBuffer *> availableBuffers_;
> +	/*
> +	 * List of frame buffer that are to be re-queued into the device.

"List of frame buffers ..."

> +	 * A nullptr indicates any internal buffer can be used (from availableBuffers_),
> +	 * whereas a valid pointer indicates an external buffer to be queued.

Oh ... are we pushing nullptrs on to the queues? ... perhaps that
answers my unreachable code question above ...


> +	 *
> +	 * Ordering buffers to be queued is important here as it must match the
> +	 * requests coming from the application.
> +	 */
> +	std::queue<FrameBuffer *> requeueBuffers_;
>  	/*
>  	 * This is a list of buffers exported internally. Need to keep this around
>  	 * as the stream needs to maintain ownership of these buffers.
> 

-- 
Regards
--
Kieran