[libcamera-devel] [PATCH] pipeline: raspberrypi: Fix erroneous bayer buffer requeue on buffer matching

[Kieran Bingham]

Hi Naush,

On 24/11/2020 17:59, Naushir Patuck wrote:
> With the recent change in the bayer/embedded buffer matching code,
> a condition would make the bayer buffer be requeued back to the device,
> even though it could potentially be queued for matching. This would
> cause unnecessary frame drops as sync would be lost.
> 
> The fix is to ensure the bayer buffer only gets requeued if the embedded
> data buffer queue is not empty, i.e. the buffer truly is orphaned.
> Additionally, we do this test before deciding to flush any of the two
> queues of all their buffers.
> 
> Fixes: 909882b (pipeline: raspberrypi: Rework bayer/embedded data buffer matching)
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>

Thanks for the quick turn around on this fix ;-)

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

> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 38 +++++++++++--------
>  1 file changed, 23 insertions(+), 15 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 7271573a..fd306066 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1679,8 +1679,23 @@ bool RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer *
>  		}
>  
>  		if (!embeddedBuffer) {
> +			bool flushedBuffers = false;
> +
>  			LOG(RPI, Debug) << "Could not find matching embedded buffer";
>  
> +			if (!embeddedQueue_.empty()) {
> +				/*
> +				 * Not found a matching embedded buffer for the bayer buffer in
> +				 * the front of the queue. This buffer is now orphaned, so requeue
> +				 * it back to the device.
> +				 */
> +				unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());
> +				bayerQueue_.pop();
> +				bayerRequeueCount++;
> +				LOG(RPI, Warning) << "Dropping unmatched input frame in stream "
> +						  << unicam_[Unicam::Image].name();
> +			}
> +
>  			/*
>  			 * If we have requeued all available embedded data buffers in this loop,
>  			 * then we are fully out of sync, so might as well requeue all the pending
> @@ -1695,20 +1710,9 @@ bool RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer *
>  					unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());
>  					bayerQueue_.pop();
>  				}
> -				return false;
> +				flushedBuffers = true;
>  			}
>  
> -			/*
> -			 * Not found a matching embedded buffer for the bayer buffer in
> -			 * the front of the queue. This buffer is now orphaned, so requeue
> -			 * it back to the device.
> -			 */
> -			unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());
> -			bayerQueue_.pop();
> -			bayerRequeueCount++;
> -			LOG(RPI, Warning) << "Dropping unmatched input frame in stream "
> -					  << unicam_[Unicam::Image].name();
> -
>  			/*
>  			 * Similar to the above, if we have requeued all available bayer buffers in

Is this statement now outdated? ('similar to "above"') or is it ok?

>  			 * the loop, then we are fully out of sync, so might as well requeue all the
> @@ -1723,11 +1727,15 @@ bool RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer *
>  					unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());
>  					embeddedQueue_.pop();
>  				}
> -				return false;
> +				flushedBuffers = true;
>  			}
>  
> -			/* If the embedded queue has become empty, we cannot do any more. */
> -			if (embeddedQueue_.empty())
> +			/*
> +			 * If the embedded queue has become empty, we cannot do any more.
> +			 * Similarly, if we have flushed any one of our queues, we cannot do
> +			 * any more. Return from here without a buffer pair.
> +			 */
> +			if (embeddedQueue_.empty() || flushedBuffers)
>  				return false;
>  		} else {
>  			/*
> 

-- 
Regards
--
Kieran