[libcamera-devel] [PATCH 06/12] ipa: ipu3: Change limits and split loops in calculateBdsGrid()

[Kieran Bingham]

On Thu, Sep 23, 2021 at 10:16:19AM +0200, Jean-Michel Hautbois wrote:
> The loops over the width and height of the image when calculating the
> BDS grid parameters are imbricated, but they're actually independent.

What's an imbricated ?

Ok - I had to look it up ... it means overlapped ...?



> Split them to reduce the complexity.
> 
> While at it, change the limits with the known limitations for the grid
> size.

Didn't we already change the limits earlier?
Perhaps you mean

"While at it, split out the constants to documented const expressions
for the grid sizes." ?

> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>

The calculations look a lot more readable I think.

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

> ---
>  src/ipa/ipu3/ipu3.cpp | 66 ++++++++++++++++++++++++++-----------------
>  1 file changed, 40 insertions(+), 26 deletions(-)
> 
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index b97aff80..06d9bcb8 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -136,8 +136,18 @@
>   * <linux/intel-ipu3.h> struct ipu3_uapi_gamma_corr_lut for further details.
>   */
>  
> +/* Minimum width of a cell of the grid */
> +static constexpr uint32_t kMinCellWidthPerSet = 16;
> +/* Maximum width of a cell of the grid */
>  static constexpr uint32_t kMaxCellWidthPerSet = 80;
> +/* Minimum height of a cell of the grid */
> +static constexpr uint32_t kMinCellHeightPerSet = 16;
> +/* Minimum height of a cell of the grid */
>  static constexpr uint32_t kMaxCellHeightPerSet = 60;
> +/* Minimum log2 of the width of each cell in pixels of the grid */
> +static constexpr uint32_t kMinCellSizeLog2 = 3;
> +/* Maximum log2 of the width of each cell in pixels of the grid */
> +static constexpr uint32_t kMaxCellSizeLog2 = 6;
>  
>  namespace libcamera {
>  
> @@ -285,41 +295,45 @@ int IPAIPU3::start()
>   * Its input is the BDS output size calculated in the ImgU.
>   * It is limited for now to the simplest method: find the lesser error
>   * with the width/height and respective log2 width/height of the cells.
> - *
> - * \todo The frame is divided into cells which can be 8x8 => 128x128.
> - * As a smaller cell improves the algorithm precision, adapting the
> - * x_start and y_start parameters of the grid would provoke a loss of
> - * some pixels but would also result in more accurate algorithms.
>   */
>  void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)
>  {
> -	uint32_t minError = std::numeric_limits<uint32_t>::max();
>  	Size best;
>  	Size bestLog2;
> +	uint32_t shift;
>  
>  	/* Set the BDS output size in the IPAConfiguration structure */
>  	context_.configuration.grid.bdsOutputSize = bdsOutputSize;
>  
> -	for (uint32_t widthShift = 3; widthShift <= 6; ++widthShift) {
> -		uint32_t width = std::min(kMaxCellWidthPerSet,
> -					  bdsOutputSize.width >> widthShift);
> -		width = width << widthShift;
> -		for (uint32_t heightShift = 3; heightShift <= 6; ++heightShift) {
> -			int32_t height = std::min(kMaxCellHeightPerSet,
> -						  bdsOutputSize.height >> heightShift);
> -			height = height << heightShift;
> -			uint32_t error  = std::abs(static_cast<int>(width - bdsOutputSize.width))
> -							+ std::abs(static_cast<int>(height - bdsOutputSize.height));
> -
> -			if (error > minError)
> -				continue;
> -
> -			minError = error;
> -			best.width = width;
> -			best.height = height;
> -			bestLog2.width = widthShift;
> -			bestLog2.height = heightShift;
> -		}
> +	uint32_t minError = std::numeric_limits<uint32_t>::max();
> +	for (shift = kMinCellSizeLog2; shift <= kMaxCellSizeLog2; ++shift) {
> +		uint32_t width = std::clamp(bdsOutputSize.width >> shift,
> +					    kMinCellWidthPerSet,
> +					    kMaxCellWidthPerSet);
> +
> +		width = width << shift;
> +		uint32_t error = std::abs(static_cast<int>(width - bdsOutputSize.width));
> +		if (error >= minError)
> +			continue;
> +
> +		minError = error;
> +		best.width = width;
> +		bestLog2.width = shift;
> +	}
> +
> +	for (shift = kMinCellSizeLog2; shift <= kMaxCellSizeLog2; ++shift) {
> +		uint32_t height = std::clamp(bdsOutputSize.height >> shift,
> +					     kMinCellHeightPerSet,
> +					     kMaxCellHeightPerSet);
> +
> +		height = height << shift;
> +		uint32_t error = std::abs(static_cast<int>(height - bdsOutputSize.height));
> +		if (error >= minError)
> +			continue;
> +
> +		minError = error;
> +		best.height = height;
> +		bestLog2.height = shift;
>  	}
>  
>  	struct ipu3_uapi_grid_config &bdsGrid = context_.configuration.grid.bdsGrid;
> -- 
> 2.30.2
> 

-- 
--
Kieran