[libcamera-devel] [PATCH v3 08/10] libcamera: pipeline: raspberrypi: Add more robust stream buffer logic

[Niklas Söderlund]

Hi Naushir,

Thanks for your work,

On 2020-07-17 09:54:08 +0100, 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.
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  .../pipeline/raspberrypi/rpi_stream.cpp       | 73 ++++++++++++++++---
>  .../pipeline/raspberrypi/rpi_stream.h         | 12 ++-
>  2 files changed, 73 insertions(+), 12 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> index 53a335e3..4818117e 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_;
> -
> -	int ret = dev_->queueBuffer(buffer);
> -	if (ret) {
> -		LOG(RPISTREAM, Error) << "Failed to queue buffer for "
> -				      << name_;
> +	if (requeueBuffers_.empty()) {
> +		/*
> +		 * No earlier requests are pending to be queued, so we can
> +		 * go ahead and queue the buffer into the device.
> +		 */
> +		return queueToDevice(buffer);
> +	} else {
> +		/*
> +		 * There are earlier buffers to be queued, so this buffer
> +		 * must go on the waiting list.
> +		 */
> +		requeueBuffers_.push(buffer);
> +		return 0;
>  	}

This could possibly be made easier to read,

    /*
     * 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);

    /*
     * There are earlier buffers to be queued, so this buffer
     * must go on the waiting list.
     */
    requeueBuffers_.push(buffer);

    return 0;

> -
> -	return ret;
>  }
>  
>  void RPiStream::returnBuffer(FrameBuffer *buffer)
> @@ -138,7 +147,36 @@ 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 && availableBuffers_.empty()) {
> +			/*
> +			 * We want to queue an internal buffer, but none
> +			 * are available. Can't do anything, quit the loop.
> +			 */
> +			break;
> +		}
> +
> +		if (!buffer) {
> +			/*
> +			 * We want to queue an internal buffer, and at least one
> +			 * is available.
> +			 */
> +			buffer = availableBuffers_.front();
> +			availableBuffers_.pop();
> +		}

Also this could be made easier to read.

    if (!buffer) {
        /*
         * We want to queue an internal buffer, but none
         * are available. Can't do anything, quit the loop.
         */
        if (availableBuffers_.empty())
            break;

        /*
         * We want to queue an internal buffer, and at least one
         * is available.
         */
        buffer = availableBuffers_.front();
        availableBuffers_.pop();
    }

Both my comments are nit on styler so with or without them addressed,

Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>

> +
> +		requeueBuffers_.pop();
> +		queueToDevice(buffer);
> +	}
>  }
>  
>  bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const
> @@ -155,10 +193,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 019e236d..6c8dfa83 100644
> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> @@ -51,6 +51,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.
> @@ -62,7 +63,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
> @@ -70,6 +71,15 @@ private:
>  	 * buffers exported internally.
>  	 */
>  	std::queue<FrameBuffer *> availableBuffers_;
> +	/*
> +	 * List of frame buffer that are to be re-queued into the device.
> +	 * A nullptr indicates any internal buffer can be used (from availableBuffers_),
> +	 * whereas a valid pointer indicates an external buffer to be queued.
> +	 *
> +	 * 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.
> -- 
> 2.25.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund