| Message ID | 20210923081625.60276-8-jeanmichel.hautbois@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
On Thu, Sep 23, 2021 at 10:16:20AM +0200, Jean-Michel Hautbois wrote: > The algorithm uses the statistics of a cell only if there is not too > much saturated pixels in it. The grey world algorithm works fine when > there are a limited number of outliers. > Consider a valid zone to be at least 80% of unsaturated cells in it. > This value could very well be configurable, and make the algorithm more > or less tolerant. > > While at it, implement it in a configure() call as it will not change > during execution, and cache the cellsPerZone values. > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/ipa/ipu3/algorithms/awb.cpp | 32 +++++++++++++++++++++++--------- > src/ipa/ipu3/algorithms/awb.h | 5 +++++ > 2 files changed, 28 insertions(+), 9 deletions(-) > > diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp > index 51a38d05..800d023c 100644 > --- a/src/ipa/ipu3/algorithms/awb.cpp > +++ b/src/ipa/ipu3/algorithms/awb.cpp > @@ -17,7 +17,6 @@ namespace ipa::ipu3::algorithms { > > LOG_DEFINE_CATEGORY(IPU3Awb) > > -static constexpr uint32_t kMinZonesCounted = 16; > static constexpr uint32_t kMinGreenLevelInZone = 32; > > /** > @@ -169,6 +168,24 @@ Awb::Awb() > > Awb::~Awb() = default; > > +int Awb::configure(IPAContext &context, > + [[maybe_unused]] const IPAConfigInfo &configInfo) > +{ > + const ipu3_uapi_grid_config &grid = context.configuration.grid.bdsGrid; > + > + cellsPerZoneX_ = round(grid.width / static_cast<double>(kAwbStatsSizeX)); > + cellsPerZoneY_ = round(grid.height / static_cast<double>(kAwbStatsSizeY)); > + > + /* > + * Configure the minimum proportion of cells counted within a zone > + * for it to be relevant for the grey world algorithm. > + * \todo This proportion could be configured. > + */ > + cellsPerZoneThreshold_ = (cellsPerZoneX_ * cellsPerZoneY_) * 80 / 100; > + > + return 0; > +} > + > /** > * The function estimates the correlated color temperature using > * from RGB color space input. > @@ -205,7 +222,7 @@ void Awb::generateZones(std::vector<RGB> &zones) > for (unsigned int i = 0; i < kAwbStatsSizeX * kAwbStatsSizeY; i++) { > RGB zone; > double counted = awbStats_[i].counted; > - if (counted >= kMinZonesCounted) { > + if (counted >= cellsPerZoneThreshold_) { > zone.G = awbStats_[i].sum.green / counted; > if (zone.G >= kMinGreenLevelInZone) { > zone.R = awbStats_[i].sum.red / counted; > @@ -220,19 +237,16 @@ void Awb::generateZones(std::vector<RGB> &zones) > void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats, > const ipu3_uapi_grid_config &grid) > { > - uint32_t cellsPerZoneX = round(grid.width / static_cast<double>(kAwbStatsSizeX)); > - uint32_t cellsPerZoneY = round(grid.height / static_cast<double>(kAwbStatsSizeY)); > - > /* > * Generate a (kAwbStatsSizeX x kAwbStatsSizeY) array from the IPU3 grid which is > * (grid.width x grid.height). > */ > - for (unsigned int cellY = 0; cellY < kAwbStatsSizeY * cellsPerZoneY; cellY++) { > - for (unsigned int cellX = 0; cellX < kAwbStatsSizeX * cellsPerZoneX; cellX++) { > + for (unsigned int cellY = 0; cellY < kAwbStatsSizeY * cellsPerZoneY_; cellY++) { > + for (unsigned int cellX = 0; cellX < kAwbStatsSizeX * cellsPerZoneX_; cellX++) { > uint32_t cellPosition = (cellY * grid.width + cellX) > * sizeof(Ipu3AwbCell); > - uint32_t zoneX = cellX / cellsPerZoneX; > - uint32_t zoneY = cellY / cellsPerZoneY; > + uint32_t zoneX = cellX / cellsPerZoneX_; > + uint32_t zoneY = cellY / cellsPerZoneY_; > > uint32_t awbZonePosition = zoneY * kAwbStatsSizeX + zoneX; > > diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h > index 3385ebe7..681d8c2b 100644 > --- a/src/ipa/ipu3/algorithms/awb.h > +++ b/src/ipa/ipu3/algorithms/awb.h > @@ -48,6 +48,7 @@ public: > Awb(); > ~Awb(); > > + int configure(IPAContext &context, const IPAConfigInfo &configInfo) override; > void prepare(IPAContext &context, ipu3_uapi_params *params) override; > void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override; > > @@ -85,6 +86,10 @@ private: > std::vector<RGB> zones_; > Accumulator awbStats_[kAwbStatsSizeX * kAwbStatsSizeY]; > AwbStatus asyncResults_; > + > + uint32_t cellsPerZoneX_; > + uint32_t cellsPerZoneY_; > + uint32_t cellsPerZoneThreshold_; > }; > > } /* namespace ipa::ipu3::algorithms */ > -- > 2.30.2 >
Hi Jean-Michel, Thank you for the patch. On Thu, Sep 23, 2021 at 10:16:20AM +0200, Jean-Michel Hautbois wrote: > The algorithm uses the statistics of a cell only if there is not too > much saturated pixels in it. The grey world algorithm works fine when > there are a limited number of outliers. > Consider a valid zone to be at least 80% of unsaturated cells in it. > This value could very well be configurable, and make the algorithm more > or less tolerant. > > While at it, implement it in a configure() call as it will not change > during execution, and cache the cellsPerZone values. > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > --- > src/ipa/ipu3/algorithms/awb.cpp | 32 +++++++++++++++++++++++--------- > src/ipa/ipu3/algorithms/awb.h | 5 +++++ > 2 files changed, 28 insertions(+), 9 deletions(-) > > diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp > index 51a38d05..800d023c 100644 > --- a/src/ipa/ipu3/algorithms/awb.cpp > +++ b/src/ipa/ipu3/algorithms/awb.cpp > @@ -17,7 +17,6 @@ namespace ipa::ipu3::algorithms { > > LOG_DEFINE_CATEGORY(IPU3Awb) > > -static constexpr uint32_t kMinZonesCounted = 16; > static constexpr uint32_t kMinGreenLevelInZone = 32; > > /** > @@ -169,6 +168,24 @@ Awb::Awb() > > Awb::~Awb() = default; > > +int Awb::configure(IPAContext &context, > + [[maybe_unused]] const IPAConfigInfo &configInfo) > +{ > + const ipu3_uapi_grid_config &grid = context.configuration.grid.bdsGrid; > + > + cellsPerZoneX_ = round(grid.width / static_cast<double>(kAwbStatsSizeX)); s/round/std::round/ as you're using cmath (with a small corresponding comment in the commit message). > + cellsPerZoneY_ = round(grid.height / static_cast<double>(kAwbStatsSizeY)); > + > + /* > + * Configure the minimum proportion of cells counted within a zone > + * for it to be relevant for the grey world algorithm. > + * \todo This proportion could be configured. > + */ > + cellsPerZoneThreshold_ = (cellsPerZoneX_ * cellsPerZoneY_) * 80 / 100; No need for parentheses. The patch looks fine to me, so Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> This however led me to notice that if the grid width isn't a multiple of 16, we'll ignore some of the cells. If the grid width is 79, for instance, 15 cells will be ignored. Would it be worth fixing that ? > + > + return 0; > +} > + > /** > * The function estimates the correlated color temperature using > * from RGB color space input. > @@ -205,7 +222,7 @@ void Awb::generateZones(std::vector<RGB> &zones) > for (unsigned int i = 0; i < kAwbStatsSizeX * kAwbStatsSizeY; i++) { > RGB zone; > double counted = awbStats_[i].counted; > - if (counted >= kMinZonesCounted) { > + if (counted >= cellsPerZoneThreshold_) { > zone.G = awbStats_[i].sum.green / counted; > if (zone.G >= kMinGreenLevelInZone) { > zone.R = awbStats_[i].sum.red / counted; > @@ -220,19 +237,16 @@ void Awb::generateZones(std::vector<RGB> &zones) > void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats, > const ipu3_uapi_grid_config &grid) > { > - uint32_t cellsPerZoneX = round(grid.width / static_cast<double>(kAwbStatsSizeX)); > - uint32_t cellsPerZoneY = round(grid.height / static_cast<double>(kAwbStatsSizeY)); > - > /* > * Generate a (kAwbStatsSizeX x kAwbStatsSizeY) array from the IPU3 grid which is > * (grid.width x grid.height). > */ > - for (unsigned int cellY = 0; cellY < kAwbStatsSizeY * cellsPerZoneY; cellY++) { > - for (unsigned int cellX = 0; cellX < kAwbStatsSizeX * cellsPerZoneX; cellX++) { > + for (unsigned int cellY = 0; cellY < kAwbStatsSizeY * cellsPerZoneY_; cellY++) { > + for (unsigned int cellX = 0; cellX < kAwbStatsSizeX * cellsPerZoneX_; cellX++) { > uint32_t cellPosition = (cellY * grid.width + cellX) > * sizeof(Ipu3AwbCell); > - uint32_t zoneX = cellX / cellsPerZoneX; > - uint32_t zoneY = cellY / cellsPerZoneY; > + uint32_t zoneX = cellX / cellsPerZoneX_; > + uint32_t zoneY = cellY / cellsPerZoneY_; > > uint32_t awbZonePosition = zoneY * kAwbStatsSizeX + zoneX; > > diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h > index 3385ebe7..681d8c2b 100644 > --- a/src/ipa/ipu3/algorithms/awb.h > +++ b/src/ipa/ipu3/algorithms/awb.h > @@ -48,6 +48,7 @@ public: > Awb(); > ~Awb(); > > + int configure(IPAContext &context, const IPAConfigInfo &configInfo) override; > void prepare(IPAContext &context, ipu3_uapi_params *params) override; > void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override; > > @@ -85,6 +86,10 @@ private: > std::vector<RGB> zones_; > Accumulator awbStats_[kAwbStatsSizeX * kAwbStatsSizeY]; > AwbStatus asyncResults_; > + > + uint32_t cellsPerZoneX_; > + uint32_t cellsPerZoneY_; > + uint32_t cellsPerZoneThreshold_; > }; > > } /* namespace ipa::ipu3::algorithms */
Hi Laurent, On 29/09/2021 14:15, Laurent Pinchart wrote: > Hi Jean-Michel, > > Thank you for the patch. > > On Thu, Sep 23, 2021 at 10:16:20AM +0200, Jean-Michel Hautbois wrote: >> The algorithm uses the statistics of a cell only if there is not too >> much saturated pixels in it. The grey world algorithm works fine when >> there are a limited number of outliers. >> Consider a valid zone to be at least 80% of unsaturated cells in it. >> This value could very well be configurable, and make the algorithm more >> or less tolerant. >> >> While at it, implement it in a configure() call as it will not change >> during execution, and cache the cellsPerZone values. >> >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> >> --- >> src/ipa/ipu3/algorithms/awb.cpp | 32 +++++++++++++++++++++++--------- >> src/ipa/ipu3/algorithms/awb.h | 5 +++++ >> 2 files changed, 28 insertions(+), 9 deletions(-) >> >> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp >> index 51a38d05..800d023c 100644 >> --- a/src/ipa/ipu3/algorithms/awb.cpp >> +++ b/src/ipa/ipu3/algorithms/awb.cpp >> @@ -17,7 +17,6 @@ namespace ipa::ipu3::algorithms { >> >> LOG_DEFINE_CATEGORY(IPU3Awb) >> >> -static constexpr uint32_t kMinZonesCounted = 16; >> static constexpr uint32_t kMinGreenLevelInZone = 32; >> >> /** >> @@ -169,6 +168,24 @@ Awb::Awb() >> >> Awb::~Awb() = default; >> >> +int Awb::configure(IPAContext &context, >> + [[maybe_unused]] const IPAConfigInfo &configInfo) >> +{ >> + const ipu3_uapi_grid_config &grid = context.configuration.grid.bdsGrid; >> + >> + cellsPerZoneX_ = round(grid.width / static_cast<double>(kAwbStatsSizeX)); > > s/round/std::round/ > > as you're using cmath (with a small corresponding comment in the commit > message). OK :-). > >> + cellsPerZoneY_ = round(grid.height / static_cast<double>(kAwbStatsSizeY)); >> + >> + /* >> + * Configure the minimum proportion of cells counted within a zone >> + * for it to be relevant for the grey world algorithm. >> + * \todo This proportion could be configured. >> + */ >> + cellsPerZoneThreshold_ = (cellsPerZoneX_ * cellsPerZoneY_) * 80 / 100; > > No need for parentheses. > > The patch looks fine to me, so > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > This however led me to notice that if the grid width isn't a multiple of > 16, we'll ignore some of the cells. If the grid width is 79, for > instance, 15 cells will be ignored. Would it be worth fixing that ? We could still do : cellsPerZoneX_ = std::round(stride_ / static_cast<double>(kAwbStatsSizeX)); But we haven't included stride yet :-) > >> + >> + return 0; >> +} >> + >> /** >> * The function estimates the correlated color temperature using >> * from RGB color space input. >> @@ -205,7 +222,7 @@ void Awb::generateZones(std::vector<RGB> &zones) >> for (unsigned int i = 0; i < kAwbStatsSizeX * kAwbStatsSizeY; i++) { >> RGB zone; >> double counted = awbStats_[i].counted; >> - if (counted >= kMinZonesCounted) { >> + if (counted >= cellsPerZoneThreshold_) { >> zone.G = awbStats_[i].sum.green / counted; >> if (zone.G >= kMinGreenLevelInZone) { >> zone.R = awbStats_[i].sum.red / counted; >> @@ -220,19 +237,16 @@ void Awb::generateZones(std::vector<RGB> &zones) >> void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats, >> const ipu3_uapi_grid_config &grid) >> { >> - uint32_t cellsPerZoneX = round(grid.width / static_cast<double>(kAwbStatsSizeX)); >> - uint32_t cellsPerZoneY = round(grid.height / static_cast<double>(kAwbStatsSizeY)); >> - >> /* >> * Generate a (kAwbStatsSizeX x kAwbStatsSizeY) array from the IPU3 grid which is >> * (grid.width x grid.height). >> */ >> - for (unsigned int cellY = 0; cellY < kAwbStatsSizeY * cellsPerZoneY; cellY++) { >> - for (unsigned int cellX = 0; cellX < kAwbStatsSizeX * cellsPerZoneX; cellX++) { >> + for (unsigned int cellY = 0; cellY < kAwbStatsSizeY * cellsPerZoneY_; cellY++) { >> + for (unsigned int cellX = 0; cellX < kAwbStatsSizeX * cellsPerZoneX_; cellX++) { >> uint32_t cellPosition = (cellY * grid.width + cellX) >> * sizeof(Ipu3AwbCell); >> - uint32_t zoneX = cellX / cellsPerZoneX; >> - uint32_t zoneY = cellY / cellsPerZoneY; >> + uint32_t zoneX = cellX / cellsPerZoneX_; >> + uint32_t zoneY = cellY / cellsPerZoneY_; >> >> uint32_t awbZonePosition = zoneY * kAwbStatsSizeX + zoneX; >> >> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h >> index 3385ebe7..681d8c2b 100644 >> --- a/src/ipa/ipu3/algorithms/awb.h >> +++ b/src/ipa/ipu3/algorithms/awb.h >> @@ -48,6 +48,7 @@ public: >> Awb(); >> ~Awb(); >> >> + int configure(IPAContext &context, const IPAConfigInfo &configInfo) override; >> void prepare(IPAContext &context, ipu3_uapi_params *params) override; >> void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override; >> >> @@ -85,6 +86,10 @@ private: >> std::vector<RGB> zones_; >> Accumulator awbStats_[kAwbStatsSizeX * kAwbStatsSizeY]; >> AwbStatus asyncResults_; >> + >> + uint32_t cellsPerZoneX_; >> + uint32_t cellsPerZoneY_; >> + uint32_t cellsPerZoneThreshold_; >> }; >> >> } /* namespace ipa::ipu3::algorithms */ >
Hi Jean-Michel, On Wed, Sep 29, 2021 at 02:32:00PM +0200, Jean-Michel Hautbois wrote: > On 29/09/2021 14:15, Laurent Pinchart wrote: > > On Thu, Sep 23, 2021 at 10:16:20AM +0200, Jean-Michel Hautbois wrote: > >> The algorithm uses the statistics of a cell only if there is not too > >> much saturated pixels in it. The grey world algorithm works fine when > >> there are a limited number of outliers. > >> Consider a valid zone to be at least 80% of unsaturated cells in it. > >> This value could very well be configurable, and make the algorithm more > >> or less tolerant. > >> > >> While at it, implement it in a configure() call as it will not change > >> during execution, and cache the cellsPerZone values. > >> > >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > >> --- > >> src/ipa/ipu3/algorithms/awb.cpp | 32 +++++++++++++++++++++++--------- > >> src/ipa/ipu3/algorithms/awb.h | 5 +++++ > >> 2 files changed, 28 insertions(+), 9 deletions(-) > >> > >> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp > >> index 51a38d05..800d023c 100644 > >> --- a/src/ipa/ipu3/algorithms/awb.cpp > >> +++ b/src/ipa/ipu3/algorithms/awb.cpp > >> @@ -17,7 +17,6 @@ namespace ipa::ipu3::algorithms { > >> > >> LOG_DEFINE_CATEGORY(IPU3Awb) > >> > >> -static constexpr uint32_t kMinZonesCounted = 16; > >> static constexpr uint32_t kMinGreenLevelInZone = 32; > >> > >> /** > >> @@ -169,6 +168,24 @@ Awb::Awb() > >> > >> Awb::~Awb() = default; > >> > >> +int Awb::configure(IPAContext &context, > >> + [[maybe_unused]] const IPAConfigInfo &configInfo) > >> +{ > >> + const ipu3_uapi_grid_config &grid = context.configuration.grid.bdsGrid; > >> + > >> + cellsPerZoneX_ = round(grid.width / static_cast<double>(kAwbStatsSizeX)); > > > > s/round/std::round/ > > > > as you're using cmath (with a small corresponding comment in the commit > > message). > > OK :-). > > >> + cellsPerZoneY_ = round(grid.height / static_cast<double>(kAwbStatsSizeY)); > >> + > >> + /* > >> + * Configure the minimum proportion of cells counted within a zone > >> + * for it to be relevant for the grey world algorithm. > >> + * \todo This proportion could be configured. > >> + */ > >> + cellsPerZoneThreshold_ = (cellsPerZoneX_ * cellsPerZoneY_) * 80 / 100; > > > > No need for parentheses. > > > > The patch looks fine to me, so > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > This however led me to notice that if the grid width isn't a multiple of > > 16, we'll ignore some of the cells. If the grid width is 79, for > > instance, 15 cells will be ignored. Would it be worth fixing that ? > > We could still do : > cellsPerZoneX_ = std::round(stride_ / static_cast<double>(kAwbStatsSizeX)); > > But we haven't included stride yet :-) I think the best solution for this issue may be a variable number of cells per zone. The question still holds though, is this an issue that needs to be addressed, or is it harmless in practice ? > >> + > >> + return 0; > >> +} > >> + > >> /** > >> * The function estimates the correlated color temperature using > >> * from RGB color space input. > >> @@ -205,7 +222,7 @@ void Awb::generateZones(std::vector<RGB> &zones) > >> for (unsigned int i = 0; i < kAwbStatsSizeX * kAwbStatsSizeY; i++) { > >> RGB zone; > >> double counted = awbStats_[i].counted; > >> - if (counted >= kMinZonesCounted) { > >> + if (counted >= cellsPerZoneThreshold_) { > >> zone.G = awbStats_[i].sum.green / counted; > >> if (zone.G >= kMinGreenLevelInZone) { > >> zone.R = awbStats_[i].sum.red / counted; > >> @@ -220,19 +237,16 @@ void Awb::generateZones(std::vector<RGB> &zones) > >> void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats, > >> const ipu3_uapi_grid_config &grid) > >> { > >> - uint32_t cellsPerZoneX = round(grid.width / static_cast<double>(kAwbStatsSizeX)); > >> - uint32_t cellsPerZoneY = round(grid.height / static_cast<double>(kAwbStatsSizeY)); > >> - > >> /* > >> * Generate a (kAwbStatsSizeX x kAwbStatsSizeY) array from the IPU3 grid which is > >> * (grid.width x grid.height). > >> */ > >> - for (unsigned int cellY = 0; cellY < kAwbStatsSizeY * cellsPerZoneY; cellY++) { > >> - for (unsigned int cellX = 0; cellX < kAwbStatsSizeX * cellsPerZoneX; cellX++) { > >> + for (unsigned int cellY = 0; cellY < kAwbStatsSizeY * cellsPerZoneY_; cellY++) { > >> + for (unsigned int cellX = 0; cellX < kAwbStatsSizeX * cellsPerZoneX_; cellX++) { > >> uint32_t cellPosition = (cellY * grid.width + cellX) > >> * sizeof(Ipu3AwbCell); > >> - uint32_t zoneX = cellX / cellsPerZoneX; > >> - uint32_t zoneY = cellY / cellsPerZoneY; > >> + uint32_t zoneX = cellX / cellsPerZoneX_; > >> + uint32_t zoneY = cellY / cellsPerZoneY_; > >> > >> uint32_t awbZonePosition = zoneY * kAwbStatsSizeX + zoneX; > >> > >> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h > >> index 3385ebe7..681d8c2b 100644 > >> --- a/src/ipa/ipu3/algorithms/awb.h > >> +++ b/src/ipa/ipu3/algorithms/awb.h > >> @@ -48,6 +48,7 @@ public: > >> Awb(); > >> ~Awb(); > >> > >> + int configure(IPAContext &context, const IPAConfigInfo &configInfo) override; > >> void prepare(IPAContext &context, ipu3_uapi_params *params) override; > >> void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override; > >> > >> @@ -85,6 +86,10 @@ private: > >> std::vector<RGB> zones_; > >> Accumulator awbStats_[kAwbStatsSizeX * kAwbStatsSizeY]; > >> AwbStatus asyncResults_; > >> + > >> + uint32_t cellsPerZoneX_; > >> + uint32_t cellsPerZoneY_; > >> + uint32_t cellsPerZoneThreshold_; > >> }; > >> > >> } /* namespace ipa::ipu3::algorithms */
Hi Laurent, On 29/09/2021 14:36, Laurent Pinchart wrote: > Hi Jean-Michel, > > On Wed, Sep 29, 2021 at 02:32:00PM +0200, Jean-Michel Hautbois wrote: >> On 29/09/2021 14:15, Laurent Pinchart wrote: >>> On Thu, Sep 23, 2021 at 10:16:20AM +0200, Jean-Michel Hautbois wrote: >>>> The algorithm uses the statistics of a cell only if there is not too >>>> much saturated pixels in it. The grey world algorithm works fine when >>>> there are a limited number of outliers. >>>> Consider a valid zone to be at least 80% of unsaturated cells in it. >>>> This value could very well be configurable, and make the algorithm more >>>> or less tolerant. >>>> >>>> While at it, implement it in a configure() call as it will not change >>>> during execution, and cache the cellsPerZone values. >>>> >>>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> >>>> --- >>>> src/ipa/ipu3/algorithms/awb.cpp | 32 +++++++++++++++++++++++--------- >>>> src/ipa/ipu3/algorithms/awb.h | 5 +++++ >>>> 2 files changed, 28 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp >>>> index 51a38d05..800d023c 100644 >>>> --- a/src/ipa/ipu3/algorithms/awb.cpp >>>> +++ b/src/ipa/ipu3/algorithms/awb.cpp >>>> @@ -17,7 +17,6 @@ namespace ipa::ipu3::algorithms { >>>> >>>> LOG_DEFINE_CATEGORY(IPU3Awb) >>>> >>>> -static constexpr uint32_t kMinZonesCounted = 16; >>>> static constexpr uint32_t kMinGreenLevelInZone = 32; >>>> >>>> /** >>>> @@ -169,6 +168,24 @@ Awb::Awb() >>>> >>>> Awb::~Awb() = default; >>>> >>>> +int Awb::configure(IPAContext &context, >>>> + [[maybe_unused]] const IPAConfigInfo &configInfo) >>>> +{ >>>> + const ipu3_uapi_grid_config &grid = context.configuration.grid.bdsGrid; >>>> + >>>> + cellsPerZoneX_ = round(grid.width / static_cast<double>(kAwbStatsSizeX)); >>> >>> s/round/std::round/ >>> >>> as you're using cmath (with a small corresponding comment in the commit >>> message). >> >> OK :-). >> >>>> + cellsPerZoneY_ = round(grid.height / static_cast<double>(kAwbStatsSizeY)); >>>> + >>>> + /* >>>> + * Configure the minimum proportion of cells counted within a zone >>>> + * for it to be relevant for the grey world algorithm. >>>> + * \todo This proportion could be configured. >>>> + */ >>>> + cellsPerZoneThreshold_ = (cellsPerZoneX_ * cellsPerZoneY_) * 80 / 100; >>> >>> No need for parentheses. >>> >>> The patch looks fine to me, so >>> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> >>> This however led me to notice that if the grid width isn't a multiple of >>> 16, we'll ignore some of the cells. If the grid width is 79, for >>> instance, 15 cells will be ignored. Would it be worth fixing that ? >> >> We could still do : >> cellsPerZoneX_ = std::round(stride_ / static_cast<double>(kAwbStatsSizeX)); >> >> But we haven't included stride yet :-) > > I think the best solution for this issue may be a variable number of > cells per zone. The question still holds though, is this an issue that > needs to be addressed, or is it harmless in practice ? > I think it is not needed, as it would not improve the behavior significantly imo (I may be wrong though !). >>>> + >>>> + return 0; >>>> +} >>>> + >>>> /** >>>> * The function estimates the correlated color temperature using >>>> * from RGB color space input. >>>> @@ -205,7 +222,7 @@ void Awb::generateZones(std::vector<RGB> &zones) >>>> for (unsigned int i = 0; i < kAwbStatsSizeX * kAwbStatsSizeY; i++) { >>>> RGB zone; >>>> double counted = awbStats_[i].counted; >>>> - if (counted >= kMinZonesCounted) { >>>> + if (counted >= cellsPerZoneThreshold_) { >>>> zone.G = awbStats_[i].sum.green / counted; >>>> if (zone.G >= kMinGreenLevelInZone) { >>>> zone.R = awbStats_[i].sum.red / counted; >>>> @@ -220,19 +237,16 @@ void Awb::generateZones(std::vector<RGB> &zones) >>>> void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats, >>>> const ipu3_uapi_grid_config &grid) >>>> { >>>> - uint32_t cellsPerZoneX = round(grid.width / static_cast<double>(kAwbStatsSizeX)); >>>> - uint32_t cellsPerZoneY = round(grid.height / static_cast<double>(kAwbStatsSizeY)); >>>> - >>>> /* >>>> * Generate a (kAwbStatsSizeX x kAwbStatsSizeY) array from the IPU3 grid which is >>>> * (grid.width x grid.height). >>>> */ >>>> - for (unsigned int cellY = 0; cellY < kAwbStatsSizeY * cellsPerZoneY; cellY++) { >>>> - for (unsigned int cellX = 0; cellX < kAwbStatsSizeX * cellsPerZoneX; cellX++) { >>>> + for (unsigned int cellY = 0; cellY < kAwbStatsSizeY * cellsPerZoneY_; cellY++) { >>>> + for (unsigned int cellX = 0; cellX < kAwbStatsSizeX * cellsPerZoneX_; cellX++) { >>>> uint32_t cellPosition = (cellY * grid.width + cellX) >>>> * sizeof(Ipu3AwbCell); >>>> - uint32_t zoneX = cellX / cellsPerZoneX; >>>> - uint32_t zoneY = cellY / cellsPerZoneY; >>>> + uint32_t zoneX = cellX / cellsPerZoneX_; >>>> + uint32_t zoneY = cellY / cellsPerZoneY_; >>>> >>>> uint32_t awbZonePosition = zoneY * kAwbStatsSizeX + zoneX; >>>> >>>> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h >>>> index 3385ebe7..681d8c2b 100644 >>>> --- a/src/ipa/ipu3/algorithms/awb.h >>>> +++ b/src/ipa/ipu3/algorithms/awb.h >>>> @@ -48,6 +48,7 @@ public: >>>> Awb(); >>>> ~Awb(); >>>> >>>> + int configure(IPAContext &context, const IPAConfigInfo &configInfo) override; >>>> void prepare(IPAContext &context, ipu3_uapi_params *params) override; >>>> void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override; >>>> >>>> @@ -85,6 +86,10 @@ private: >>>> std::vector<RGB> zones_; >>>> Accumulator awbStats_[kAwbStatsSizeX * kAwbStatsSizeY]; >>>> AwbStatus asyncResults_; >>>> + >>>> + uint32_t cellsPerZoneX_; >>>> + uint32_t cellsPerZoneY_; >>>> + uint32_t cellsPerZoneThreshold_; >>>> }; >>>> >>>> } /* namespace ipa::ipu3::algorithms */ >
diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp index 51a38d05..800d023c 100644 --- a/src/ipa/ipu3/algorithms/awb.cpp +++ b/src/ipa/ipu3/algorithms/awb.cpp @@ -17,7 +17,6 @@ namespace ipa::ipu3::algorithms { LOG_DEFINE_CATEGORY(IPU3Awb) -static constexpr uint32_t kMinZonesCounted = 16; static constexpr uint32_t kMinGreenLevelInZone = 32; /** @@ -169,6 +168,24 @@ Awb::Awb() Awb::~Awb() = default; +int Awb::configure(IPAContext &context, + [[maybe_unused]] const IPAConfigInfo &configInfo) +{ + const ipu3_uapi_grid_config &grid = context.configuration.grid.bdsGrid; + + cellsPerZoneX_ = round(grid.width / static_cast<double>(kAwbStatsSizeX)); + cellsPerZoneY_ = round(grid.height / static_cast<double>(kAwbStatsSizeY)); + + /* + * Configure the minimum proportion of cells counted within a zone + * for it to be relevant for the grey world algorithm. + * \todo This proportion could be configured. + */ + cellsPerZoneThreshold_ = (cellsPerZoneX_ * cellsPerZoneY_) * 80 / 100; + + return 0; +} + /** * The function estimates the correlated color temperature using * from RGB color space input. @@ -205,7 +222,7 @@ void Awb::generateZones(std::vector<RGB> &zones) for (unsigned int i = 0; i < kAwbStatsSizeX * kAwbStatsSizeY; i++) { RGB zone; double counted = awbStats_[i].counted; - if (counted >= kMinZonesCounted) { + if (counted >= cellsPerZoneThreshold_) { zone.G = awbStats_[i].sum.green / counted; if (zone.G >= kMinGreenLevelInZone) { zone.R = awbStats_[i].sum.red / counted; @@ -220,19 +237,16 @@ void Awb::generateZones(std::vector<RGB> &zones) void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats, const ipu3_uapi_grid_config &grid) { - uint32_t cellsPerZoneX = round(grid.width / static_cast<double>(kAwbStatsSizeX)); - uint32_t cellsPerZoneY = round(grid.height / static_cast<double>(kAwbStatsSizeY)); - /* * Generate a (kAwbStatsSizeX x kAwbStatsSizeY) array from the IPU3 grid which is * (grid.width x grid.height). */ - for (unsigned int cellY = 0; cellY < kAwbStatsSizeY * cellsPerZoneY; cellY++) { - for (unsigned int cellX = 0; cellX < kAwbStatsSizeX * cellsPerZoneX; cellX++) { + for (unsigned int cellY = 0; cellY < kAwbStatsSizeY * cellsPerZoneY_; cellY++) { + for (unsigned int cellX = 0; cellX < kAwbStatsSizeX * cellsPerZoneX_; cellX++) { uint32_t cellPosition = (cellY * grid.width + cellX) * sizeof(Ipu3AwbCell); - uint32_t zoneX = cellX / cellsPerZoneX; - uint32_t zoneY = cellY / cellsPerZoneY; + uint32_t zoneX = cellX / cellsPerZoneX_; + uint32_t zoneY = cellY / cellsPerZoneY_; uint32_t awbZonePosition = zoneY * kAwbStatsSizeX + zoneX; diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h index 3385ebe7..681d8c2b 100644 --- a/src/ipa/ipu3/algorithms/awb.h +++ b/src/ipa/ipu3/algorithms/awb.h @@ -48,6 +48,7 @@ public: Awb(); ~Awb(); + int configure(IPAContext &context, const IPAConfigInfo &configInfo) override; void prepare(IPAContext &context, ipu3_uapi_params *params) override; void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override; @@ -85,6 +86,10 @@ private: std::vector<RGB> zones_; Accumulator awbStats_[kAwbStatsSizeX * kAwbStatsSizeY]; AwbStatus asyncResults_; + + uint32_t cellsPerZoneX_; + uint32_t cellsPerZoneY_; + uint32_t cellsPerZoneThreshold_; }; } /* namespace ipa::ipu3::algorithms */
The algorithm uses the statistics of a cell only if there is not too much saturated pixels in it. The grey world algorithm works fine when there are a limited number of outliers. Consider a valid zone to be at least 80% of unsaturated cells in it. This value could very well be configurable, and make the algorithm more or less tolerant. While at it, implement it in a configure() call as it will not change during execution, and cache the cellsPerZone values. Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> --- src/ipa/ipu3/algorithms/awb.cpp | 32 +++++++++++++++++++++++--------- src/ipa/ipu3/algorithms/awb.h | 5 +++++ 2 files changed, 28 insertions(+), 9 deletions(-)