[libcamera-devel] [PATCH 1/2] pipeline: raspberrypi: Fix under-allocation of embedded data buffers

[Jacopo Mondi]

Hi David

On Tue, Nov 30, 2021 at 11:22:58AM +0000, David Plowman wrote:
> The code was reducing the number of raw stream buffers allocated when
> the application is providing some of its own. However, it was not
> taking account of the fact that the application cannot supply embedded
> data buffers, so it must always allocate a reasonable minimum number
> of these buffers (possibly more than the number of raw stream buffers)
> to prevent frame drops.
>
> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>

Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
  j

> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 25 ++++++++++++-------
>  1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index e31fa3b2..045725dd 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1248,18 +1248,25 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>  	/* Decide how many internal buffers to allocate. */
>  	for (auto const stream : data->streams_) {
>  		unsigned int numBuffers;
> -
> -		if (stream == &data->unicam_[Unicam::Image] ||
> -		    stream == &data->unicam_[Unicam::Embedded]) {
> +		/*
> +		 * For Unicam, allocate a minimum of 4 buffers as we want
> +		 * to avoid any frame drops.
> +		 */
> +		constexpr unsigned int minBuffers = 4;
> +		if (stream == &data->unicam_[Unicam::Image]) {
>  			/*
> -			 * For Unicam, allocate a minimum of 4 buffers as we want
> -			 * to avoid any frame drops. If an application has configured
> -			 * a RAW stream, allocate additional buffers to make up the
> -			 * minimum, but ensure we have at least 2 sets of internal
> -			 * buffers to use to minimise frame drops.
> +			 * If an application has configured a RAW stream, allocate
> +			 * additional buffers to make up the minimum, but ensure
> +			 * we have at least 2 sets of internal buffers to use to
> +			 * minimise frame drops.
>  			 */
> -			constexpr unsigned int minBuffers = 4;
>  			numBuffers = std::max<int>(2, minBuffers - numRawBuffers);
> +		} else if (stream == &data->unicam_[Unicam::Embedded]) {
> +			/*
> +			 * Embedded data buffers are (currently) for internal use,
> +			 * so allocate the minimum required to avoid frame drops.
> +			 */
> +			numBuffers = minBuffers;
>  		} else {
>  			/*
>  			 * Since the ISP runs synchronous with the IPA and requests,
> --
> 2.30.2
>