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

[Laurent Pinchart]

On Tue, Sep 28, 2021 at 04:34:40PM +0100, Kieran Bingham wrote:
> 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 ...?

I think "nested" would be a better word here.

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

That's not right, this is the minimum number of cells in the grid, not
the minimum width of a cell. Same for the other comments.

> > +static constexpr uint32_t kMinCellWidthPerSet = 16;

The name of the constant is also not great. I'd go for

/* Minimum grid width, expressed as a number of cells */
static constexpr uint32_t kMinGridWidth = 16;

assuming we will always express the grid size in cells units. If there's
a need to mix grid sizes in cells and in pixels, then we need more
precise names.

> > +/* 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 */

/* log2 of the minimum grid cell width and height, in pixels */

> > +static constexpr uint32_t kMinCellSizeLog2 = 3;
> > +/* Maximum log2 of the width of each cell in pixels of the grid */

/* log2 of the maximum grid cell width and height, in pixels */

> > +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.

Why is this dropped ?

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

	for (unsigned int shift = ...)

and you can drop the variable declaration above.

> > +		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;

-- 
Regards,

Laurent Pinchart