| Message ID | 20210923081625.60276-7-jeanmichel.hautbois@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@ideasonboard.com> > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> The calculations look a lot more readable I think. Reviewed-by: Kieran Bingham <kieran.bingham@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 >
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@ideasonboard.com> > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > > The calculations look a lot more readable I think. > > Reviewed-by: Kieran Bingham <kieran.bingham@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;
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;