[libcamera-devel] [PATCH] pipeline: raspberrypi: Improve image/embedded buffer matching logic

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Wed, Jul 13, 2022 at 01:00:35PM +0100, Naushir Patuck via libcamera-devel wrote:
> The logic used to match asynchronous image and embedded buffers was being overly
> aggressive by possibly allowing an unmatched image buffer to be sent to the IPA
> if the matching embedded buffer had not yet been dequeued. This condition only
> occurs when the system is heavily loaded and dropping frames.
> 
> Fix this by holding image buffer in the queue during these conditions until the
> next embedded buffer dequeue event.
> 
> Reported-by: Nick Hollinghurst <nick.hollinghurst at raspberrypi.com>
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 66a84b1dfb97..ef3c2d11d253 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -2159,16 +2159,12 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&em
>  	if (bayerQueue_.empty())
>  		return false;

Not strictly related to this patch, but this can never happen, as it's
tested by the caller.

>  
> -	/* Start with the front of the bayer queue. */
> -	bayerFrame = std::move(bayerQueue_.front());
> -	bayerQueue_.pop();
> -
>  	/*
>  	 * Find the embedded data buffer with a matching timestamp to pass to
>  	 * the IPA. Any embedded buffers with a timestamp lower than the
>  	 * current bayer buffer will be removed and re-queued to the driver.
>  	 */
> -	uint64_t ts = bayerFrame.buffer->metadata().timestamp;
> +	uint64_t ts = bayerQueue_.front().buffer->metadata().timestamp;
>  	embeddedBuffer = nullptr;
>  	while (!embeddedQueue_.empty()) {
>  		FrameBuffer *b = embeddedQueue_.front();
> @@ -2188,10 +2184,23 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&em
>  	}
>  
>  	if (!embeddedBuffer && sensorMetadata_) {
> +		if (embeddedQueue_.empty()) {
> +			/*
> +			 * If the embedded buffer queue is empty, wait for the next
> +			 * buffer to arrive - dequeue ordering may send the image
> +			 * buffer first.
> +			 */
> +			LOG(RPI, Debug) << "Waiting for next embedded buffer.";
> +			return false;
> +		}
> +
>  		/* Log if there is no matching embedded data buffer found. */
>  		LOG(RPI, Debug) << "Returning bayer frame without a matching embedded buffer.";
>  	}
>  
> +	bayerFrame = std::move(bayerQueue_.front());
> +	bayerQueue_.pop();
> +

This looks fine, but I wonder if it could be simplified by first
handling the case where the sensor doesn't produce metadata (possibly in
RPiCameraData::tryRunPipeline()). This can be done on top, so I'll apply
this patch with

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

>  	return true;
>  }
>  

-- 
Regards,

Laurent Pinchart