| Message ID | 20210909082516.26055-3-jeanmichel.hautbois@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
On 09/09/2021 09:25, Jean-Michel Hautbois wrote: > The IspStatsRegion structure was introduced as an attempt to prepare for > a generic AWB algorithm structure. The structure name by itself is not > explicit and it is too optimistic to try and make a generic one for now. > > Its role is to accumulate the pixels in a given region. Rename it to > accumulator, and remove the uncounted field at the same time. It is > always possible to know how many pixels are not relevant for the > algorithm by calculating total-counted. The uncounted field was only > declared and not used. Amend the documentation accordingly. > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > --- > src/ipa/ipu3/algorithms/awb.cpp | 36 ++++++++++++++++++++------------- > src/ipa/ipu3/algorithms/awb.h | 5 ++--- > 2 files changed, 24 insertions(+), 17 deletions(-) > > diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp > index e05647c9..97f5839f 100644 > --- a/src/ipa/ipu3/algorithms/awb.cpp > +++ b/src/ipa/ipu3/algorithms/awb.cpp > @@ -21,26 +21,35 @@ static constexpr uint32_t kMinZonesCounted = 16; > static constexpr uint32_t kMinGreenLevelInZone = 32; > > /** > - * \struct IspStatsRegion > + * \struct Accumulator > * \brief RGB statistics for a given region > * > - * The IspStatsRegion structure is intended to abstract the ISP specific > - * statistics and use an agnostic algorithm to compute AWB. > + * A frame is divided into zones, each zone is made of multiple regions which > + * are defined by the grid configuration. The algorithm works with a fixed > + * number of zones \a kAwbStatsSizeX x \a kAwbStatsSizeY. > + * For example, a frame of 1280x720 is divided into 81x45 regions of [16x16] > + * pixels because the BDS output size will be calculated as 1296x720. In the > + * case of \a kAwbStatsSizeX=16 and \a kAwbStatsSizeY=12 the zones are made of > + * [5x4] regions. The regions are left-aligned and calculated by > + * IPAIPU3::calculateBdsGrid(). > * > - * \var IspStatsRegion::counted > - * \brief Number of pixels used to calculate the sums > + * The Accumulator structure stores the sum of the pixel values in a zone of > + * the image, as well as the number of relevant regions in this same zone. A > + * relevant region is an unsaturated region here, depending on the awb > + * thresholds. > + * \todo extend the notion of relevant to something else ? What else could it be? You've defined relevant as "unsaturated regions" so perhaps we could just use that ? """ * The Accumulator structure stores the sum of the pixel values in a zone of * the image, as well as the number of unsaturated regions in this same zone. * * Zones are determined to be saturated if their value exceeds the * threshold given by the thresholds in * ipu3_uapi_awb_config_s.rgbs_thr_{gr,r,gb,b}. * * \todo It is not fully clear from the IPU3 documentation what defines a * saturated zone, This should be investigated further. """ <fix anything above as necessary, I do not assert correctness above> > * > - * \var IspStatsRegion::uncounted > - * \brief Remaining number of pixels in the region > + * \var Accumulator::counted > + * \brief Number of relevant regions used to calculate the sums s/relevant/unsaturated/ With those: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > * > - * \var IspStatsRegion::rSum > - * \brief Sum of the red values in the region > + * \var Accumulator::rSum > + * \brief Sum of the red values in the zone > * > - * \var IspStatsRegion::gSum > - * \brief Sum of the green values in the region > + * \var Accumulator::gSum > + * \brief Sum of the green values in the zone > * > - * \var IspStatsRegion::bSum > - * \brief Sum of the blue values in the region > + * \var Accumulator::bSum > + * \brief Sum of the blue values in the zone > */ > > /** > @@ -215,7 +224,6 @@ void Awb::clearAwbStats() > awbStats_[i].rSum = 0; > awbStats_[i].gSum = 0; > awbStats_[i].counted = 0; > - awbStats_[i].uncounted = 0; > } > } > > diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h > index cc848060..ac8ccc84 100644 > --- a/src/ipa/ipu3/algorithms/awb.h > +++ b/src/ipa/ipu3/algorithms/awb.h > @@ -33,9 +33,8 @@ struct Ipu3AwbCell { > unsigned char padding[3]; > } __attribute__((packed)); > > -struct IspStatsRegion { > +struct Accumulator { > unsigned int counted; > - unsigned int uncounted; > unsigned long long rSum; > unsigned long long gSum; > unsigned long long bSum; > @@ -82,7 +81,7 @@ private: > uint32_t estimateCCT(double red, double green, double blue); > > std::vector<RGB> zones_; > - IspStatsRegion awbStats_[kAwbStatsSizeX * kAwbStatsSizeY]; > + Accumulator awbStats_[kAwbStatsSizeX * kAwbStatsSizeY]; > AwbStatus asyncResults_; > }; > >
On 09/09/2021 09:25, Jean-Michel Hautbois wrote: > The IspStatsRegion structure was introduced as an attempt to prepare for > a generic AWB algorithm structure. The structure name by itself is not > explicit and it is too optimistic to try and make a generic one for now. > > Its role is to accumulate the pixels in a given region. Rename it to s/region/zone/? > accumulator, and remove the uncounted field at the same time. It is > always possible to know how many pixels are not relevant for the > algorithm by calculating total-counted. The uncounted field was only > declared and not used. Amend the documentation accordingly. > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > --- > src/ipa/ipu3/algorithms/awb.cpp | 36 ++++++++++++++++++++------------- > src/ipa/ipu3/algorithms/awb.h | 5 ++--- > 2 files changed, 24 insertions(+), 17 deletions(-) > > diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp > index e05647c9..97f5839f 100644 > --- a/src/ipa/ipu3/algorithms/awb.cpp > +++ b/src/ipa/ipu3/algorithms/awb.cpp > @@ -21,26 +21,35 @@ static constexpr uint32_t kMinZonesCounted = 16; > static constexpr uint32_t kMinGreenLevelInZone = 32; > > /** > - * \struct IspStatsRegion > + * \struct Accumulator > * \brief RGB statistics for a given region > * > - * The IspStatsRegion structure is intended to abstract the ISP specific > - * statistics and use an agnostic algorithm to compute AWB. > + * A frame is divided into zones, each zone is made of multiple regions which s/region/cell/ > + * are defined by the grid configuration. The algorithm works with a fixed > + * number of zones \a kAwbStatsSizeX x \a kAwbStatsSizeY. > + * For example, a frame of 1280x720 is divided into 81x45 regions of [16x16] s/regions/cells/ > + * pixels because the BDS output size will be calculated as 1296x720. In the pixels with a BDS output size of 1296x720 to account for the 16x16 pixel alignment restrictions. > + * case of \a kAwbStatsSizeX=16 and \a kAwbStatsSizeY=12 the zones are made of > + * [5x4] regions. The regions are left-aligned and calculated by s/regions/cells/ > + * IPAIPU3::calculateBdsGrid(). > * > - * \var IspStatsRegion::counted > - * \brief Number of pixels used to calculate the sums > + * The Accumulator structure stores the sum of the pixel values in a zone of > + * the image, as well as the number of relevant regions in this same zone. A > + * relevant region is an unsaturated region here, depending on the awb > + * thresholds. > + * \todo extend the notion of relevant to something else ? Trying again with my new understandings: """ Each statistics cell represents the average value of the pixels in that cell split by colour components. The Accumulator structure stores the sum of the average of each cell in a zone of the image, as well as the number of cells which were unsaturated and therefore included in the average. Cells which are saturated beyond the threshold defined in ipu3_uapi_awb_config_s are not included in the average. """ > * > - * \var IspStatsRegion::uncounted > - * \brief Remaining number of pixels in the region > + * \var Accumulator::counted > + * \brief Number of relevant regions used to calculate the sums > * > - * \var IspStatsRegion::rSum > - * \brief Sum of the red values in the region > + * \var Accumulator::rSum > + * \brief Sum of the red values in the zone From my understanding reading the following patches and how this is calculated, these briefs are (slightly) incorrect. Is it more correct to say: \brief Sum of the average red values of each cell in the zone Also (as discussed directly with you), I was confused between cell, zone, and region. We've come up with this diagram together to clarify this which might be worth adding somewhere as documentation of the grid: - Cells are defined in Pixels - Zones are defined in Cells - The total zones may overlap the image width and height to meet hardware constraints 81 cells /───────────── 1280 pixels ───────────\ 16 zones 16 ┌────┬────┬────┬────┬────┬─ ──────┬────┐ \ │Cell│ │ │ │ │ | │ │ │ 16 │ px │ │ │ │ │ | │ │ │ ├────┼────┼────┼────┼────┼─ ──────┼────┤ │ │ │ │ │ │ │ | │ │ │ │ │ │ │ │ | │ │ 7 │ ── │ ── │ ── │ ── │ ── │ ── ── ─┤ ── │ 1 2 4 │ │ │ │ │ │ | │ │ 2 0 5 │ │ │ │ │ │ | │ │ z p c ├────┼────┼────┼────┼────┼─ ──────┼────┤ o i e │ │ │ │ │ │ | │ │ n x l │ │ | │ │ e e l ├─── ───┼─ ──────┼────┤ s l s │ │ | │ │ s │ │ | │ │ ├─── Zone of Cells ───┼─ ──────┼────┤ │ │ (5 x 4) │ | │ │ │ │ │ | │ │ │ ├── ───┼─ ──────┼────┤ │ │ │ │ | │ │ │ │ │ │ │ │ │ | │ │ │ └────┴────┴────┴────┴────┴─ ──────┴────┘ / I now understand that you have used the term region to mean cell. Could you try to incorporate that into the beginning of this documentation? or maybe we should document this as the grid separately and reference it from here? (We could always put something here for now, and then write up the grid later, moving this out to that new documentation). > * > - * \var IspStatsRegion::gSum > - * \brief Sum of the green values in the region > + * \var Accumulator::gSum > + * \brief Sum of the green values in the zone > * > - * \var IspStatsRegion::bSum > - * \brief Sum of the blue values in the region > + * \var Accumulator::bSum > + * \brief Sum of the blue values in the zone > */ > > /** > @@ -215,7 +224,6 @@ void Awb::clearAwbStats() > awbStats_[i].rSum = 0; > awbStats_[i].gSum = 0; > awbStats_[i].counted = 0; > - awbStats_[i].uncounted = 0; > } > } > > diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h > index cc848060..ac8ccc84 100644 > --- a/src/ipa/ipu3/algorithms/awb.h > +++ b/src/ipa/ipu3/algorithms/awb.h > @@ -33,9 +33,8 @@ struct Ipu3AwbCell { > unsigned char padding[3]; > } __attribute__((packed)); > > -struct IspStatsRegion { > +struct Accumulator { > unsigned int counted; > - unsigned int uncounted; > unsigned long long rSum; > unsigned long long gSum; > unsigned long long bSum; > @@ -82,7 +81,7 @@ private: > uint32_t estimateCCT(double red, double green, double blue); > > std::vector<RGB> zones_; > - IspStatsRegion awbStats_[kAwbStatsSizeX * kAwbStatsSizeY]; > + Accumulator awbStats_[kAwbStatsSizeX * kAwbStatsSizeY]; > AwbStatus asyncResults_; > }; > >
diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp index e05647c9..97f5839f 100644 --- a/src/ipa/ipu3/algorithms/awb.cpp +++ b/src/ipa/ipu3/algorithms/awb.cpp @@ -21,26 +21,35 @@ static constexpr uint32_t kMinZonesCounted = 16; static constexpr uint32_t kMinGreenLevelInZone = 32; /** - * \struct IspStatsRegion + * \struct Accumulator * \brief RGB statistics for a given region * - * The IspStatsRegion structure is intended to abstract the ISP specific - * statistics and use an agnostic algorithm to compute AWB. + * A frame is divided into zones, each zone is made of multiple regions which + * are defined by the grid configuration. The algorithm works with a fixed + * number of zones \a kAwbStatsSizeX x \a kAwbStatsSizeY. + * For example, a frame of 1280x720 is divided into 81x45 regions of [16x16] + * pixels because the BDS output size will be calculated as 1296x720. In the + * case of \a kAwbStatsSizeX=16 and \a kAwbStatsSizeY=12 the zones are made of + * [5x4] regions. The regions are left-aligned and calculated by + * IPAIPU3::calculateBdsGrid(). * - * \var IspStatsRegion::counted - * \brief Number of pixels used to calculate the sums + * The Accumulator structure stores the sum of the pixel values in a zone of + * the image, as well as the number of relevant regions in this same zone. A + * relevant region is an unsaturated region here, depending on the awb + * thresholds. + * \todo extend the notion of relevant to something else ? * - * \var IspStatsRegion::uncounted - * \brief Remaining number of pixels in the region + * \var Accumulator::counted + * \brief Number of relevant regions used to calculate the sums * - * \var IspStatsRegion::rSum - * \brief Sum of the red values in the region + * \var Accumulator::rSum + * \brief Sum of the red values in the zone * - * \var IspStatsRegion::gSum - * \brief Sum of the green values in the region + * \var Accumulator::gSum + * \brief Sum of the green values in the zone * - * \var IspStatsRegion::bSum - * \brief Sum of the blue values in the region + * \var Accumulator::bSum + * \brief Sum of the blue values in the zone */ /** @@ -215,7 +224,6 @@ void Awb::clearAwbStats() awbStats_[i].rSum = 0; awbStats_[i].gSum = 0; awbStats_[i].counted = 0; - awbStats_[i].uncounted = 0; } } diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h index cc848060..ac8ccc84 100644 --- a/src/ipa/ipu3/algorithms/awb.h +++ b/src/ipa/ipu3/algorithms/awb.h @@ -33,9 +33,8 @@ struct Ipu3AwbCell { unsigned char padding[3]; } __attribute__((packed)); -struct IspStatsRegion { +struct Accumulator { unsigned int counted; - unsigned int uncounted; unsigned long long rSum; unsigned long long gSum; unsigned long long bSum; @@ -82,7 +81,7 @@ private: uint32_t estimateCCT(double red, double green, double blue); std::vector<RGB> zones_; - IspStatsRegion awbStats_[kAwbStatsSizeX * kAwbStatsSizeY]; + Accumulator awbStats_[kAwbStatsSizeX * kAwbStatsSizeY]; AwbStatus asyncResults_; };
The IspStatsRegion structure was introduced as an attempt to prepare for a generic AWB algorithm structure. The structure name by itself is not explicit and it is too optimistic to try and make a generic one for now. Its role is to accumulate the pixels in a given region. Rename it to accumulator, and remove the uncounted field at the same time. It is always possible to know how many pixels are not relevant for the algorithm by calculating total-counted. The uncounted field was only declared and not used. Amend the documentation accordingly. Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> --- src/ipa/ipu3/algorithms/awb.cpp | 36 ++++++++++++++++++++------------- src/ipa/ipu3/algorithms/awb.h | 5 ++--- 2 files changed, 24 insertions(+), 17 deletions(-)