| Message ID | 20211013154125.133419-2-jeanmichel.hautbois@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Jean-Michel Hautbois (2021-10-13 16:41:13) > We can have a saturation ratio per cell, giving the percentage of pixels > over a threshold within a cell where 100% is set to 0xff. > > The parameter structure 'ipu3_uapi_awb_config_s' contains four fields to > set the threshold, one per channel. > The blue field is also used to configure the ImgU and make it calculate > the saturation ratio or not. > > Set a green value saturated when it is more than 230 (90% of the maximum > value 255, coded as 8191). Why do we only determine a lower saturation on one component? Shouldn't they be balanced (perhaps in accordance with the white balance somehow?). > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > --- > src/ipa/ipu3/algorithms/awb.cpp | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp > index e2b18336..5574bd44 100644 > --- a/src/ipa/ipu3/algorithms/awb.cpp > +++ b/src/ipa/ipu3/algorithms/awb.cpp > @@ -326,9 +326,9 @@ void Awb::process(IPAContext &context, const ipu3_uapi_stats_3a *stats) > > void Awb::prepare(IPAContext &context, ipu3_uapi_params *params) > { > - params->acc_param.awb.config.rgbs_thr_gr = 8191; > + params->acc_param.awb.config.rgbs_thr_gr = (90 * 8191 / 100); > params->acc_param.awb.config.rgbs_thr_r = 8191; > - params->acc_param.awb.config.rgbs_thr_gb = 8191; > + params->acc_param.awb.config.rgbs_thr_gb = (90 * 8191 / 100); > params->acc_param.awb.config.rgbs_thr_b = IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT > | IPU3_UAPI_AWB_RGBS_THR_B_EN > | 8191; That's interesting that thr_b has those enable flags. Do they apply only to B? or does that enable all of them? I wonder if a helper function would make the values clearer: (Awb private:) uint16_t Awb::Threshold(float value) { /* AWB Thresholds are represented in FixedPoint 3.13 */ constexpr uint16_t kFixedPoint3_13 = 8191; return value * kFixedPoint3_13; } void Awb::prepare(IPAContext &context, ipu3_uapi_params *params) { /* * Green saturation thresholds are reduced because... */ params->acc_param.awb.config.rgbs_thr_r = Threshold(1.0); params->acc_param.awb.config.rgbs_thr_gr = Threshold(0.9); params->acc_param.awb.config.rgbs_thr_gb = Threshold(0.9); params->acc_param.awb.config.rgbs_thr_b = Threshold(1.0); /* Enable saturation inclusion on thr_b because ... */ params->acc_param.awb.config.rgbs_thr_b |= IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT | IPU3_UAPI_AWB_RGBS_THR_B_EN; ... } > -- > 2.30.2 >
Hello, On Thu, Oct 14, 2021 at 11:23:13AM +0100, Kieran Bingham wrote: > Quoting Jean-Michel Hautbois (2021-10-13 16:41:13) > > We can have a saturation ratio per cell, giving the percentage of pixels > > over a threshold within a cell where 100% is set to 0xff. > > > > The parameter structure 'ipu3_uapi_awb_config_s' contains four fields to > > set the threshold, one per channel. > > The blue field is also used to configure the ImgU and make it calculate > > the saturation ratio or not. > > > > Set a green value saturated when it is more than 230 (90% of the maximum > > value 255, coded as 8191). > > Why do we only determine a lower saturation on one component? I assume because we only use the stats for that component :-) > Shouldn't they be balanced (perhaps in accordance with the white balance > somehow?). If the assumption is correct, then we could, and it would make no difference. > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > > --- > > src/ipa/ipu3/algorithms/awb.cpp | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp > > index e2b18336..5574bd44 100644 > > --- a/src/ipa/ipu3/algorithms/awb.cpp > > +++ b/src/ipa/ipu3/algorithms/awb.cpp > > @@ -326,9 +326,9 @@ void Awb::process(IPAContext &context, const ipu3_uapi_stats_3a *stats) > > > > void Awb::prepare(IPAContext &context, ipu3_uapi_params *params) > > { > > - params->acc_param.awb.config.rgbs_thr_gr = 8191; > > + params->acc_param.awb.config.rgbs_thr_gr = (90 * 8191 / 100); No need for parentheses. > > params->acc_param.awb.config.rgbs_thr_r = 8191; > > - params->acc_param.awb.config.rgbs_thr_gb = 8191; > > + params->acc_param.awb.config.rgbs_thr_gb = (90 * 8191 / 100); > > params->acc_param.awb.config.rgbs_thr_b = IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT > > | IPU3_UAPI_AWB_RGBS_THR_B_EN > > | 8191; > > That's interesting that thr_b has those enable flags. Do they apply > only to B? or does that enable all of them? As far as I undertand, the single flag applies to all channels. > I wonder if a helper function would make the values clearer: > > (Awb private:) > uint16_t Awb::Threshold(float value) Make it constexpr uint16_t Awb::Threshold(float value) > { > /* AWB Thresholds are represented in FixedPoint 3.13 */ > constexpr uint16_t kFixedPoint3_13 = 8191; > > return value * kFixedPoint3_13; > } In a 3.13 fixed-point format, 1.0 would be 8192, not 8191. The above would thus be misleading. 8191 is the maximum value of a 12bpp pixel, it's not a fixed-point number in this context. > void Awb::prepare(IPAContext &context, ipu3_uapi_params *params) > { > /* > * Green saturation thresholds are reduced because... > */ > params->acc_param.awb.config.rgbs_thr_r = Threshold(1.0); > params->acc_param.awb.config.rgbs_thr_gr = Threshold(0.9); > params->acc_param.awb.config.rgbs_thr_gb = Threshold(0.9); > params->acc_param.awb.config.rgbs_thr_b = Threshold(1.0); > > /* Enable saturation inclusion on thr_b because ... */ > params->acc_param.awb.config.rgbs_thr_b |= IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT | > IPU3_UAPI_AWB_RGBS_THR_B_EN; I like splitting this from the previous line. > ... > }
Hi ! On 14/10/2021 23:56, Laurent Pinchart wrote: > Hello, > > On Thu, Oct 14, 2021 at 11:23:13AM +0100, Kieran Bingham wrote: >> Quoting Jean-Michel Hautbois (2021-10-13 16:41:13) >>> We can have a saturation ratio per cell, giving the percentage of pixels >>> over a threshold within a cell where 100% is set to 0xff. >>> >>> The parameter structure 'ipu3_uapi_awb_config_s' contains four fields to >>> set the threshold, one per channel. >>> The blue field is also used to configure the ImgU and make it calculate >>> the saturation ratio or not. >>> >>> Set a green value saturated when it is more than 230 (90% of the maximum >>> value 255, coded as 8191). >> >> Why do we only determine a lower saturation on one component? > > I assume because we only use the stats for that component :-) > Exactly, and also because most of the perceived luminance is well represented by the green channel (red and blud have smaller contributions, 60% for green, 30% for red and 10% for blue to simplify). >> Shouldn't they be balanced (perhaps in accordance with the white balance >> somehow?). > > If the assumption is correct, then we could, and it would make no > difference. > Yes, it is not a bad thing but does not improve anything :-). >>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> >>> --- >>> src/ipa/ipu3/algorithms/awb.cpp | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp >>> index e2b18336..5574bd44 100644 >>> --- a/src/ipa/ipu3/algorithms/awb.cpp >>> +++ b/src/ipa/ipu3/algorithms/awb.cpp >>> @@ -326,9 +326,9 @@ void Awb::process(IPAContext &context, const ipu3_uapi_stats_3a *stats) >>> >>> void Awb::prepare(IPAContext &context, ipu3_uapi_params *params) >>> { >>> - params->acc_param.awb.config.rgbs_thr_gr = 8191; >>> + params->acc_param.awb.config.rgbs_thr_gr = (90 * 8191 / 100); > > No need for parentheses. > >>> params->acc_param.awb.config.rgbs_thr_r = 8191; >>> - params->acc_param.awb.config.rgbs_thr_gb = 8191; >>> + params->acc_param.awb.config.rgbs_thr_gb = (90 * 8191 / 100); >>> params->acc_param.awb.config.rgbs_thr_b = IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT >>> | IPU3_UAPI_AWB_RGBS_THR_B_EN >>> | 8191; >> >> That's interesting that thr_b has those enable flags. Do they apply >> only to B? or does that enable all of them? > > As far as I undertand, the single flag applies to all channels. > >> I wonder if a helper function would make the values clearer: >> >> (Awb private:) >> uint16_t Awb::Threshold(float value) > > Make it > > constexpr uint16_t Awb::Threshold(float value) > >> { >> /* AWB Thresholds are represented in FixedPoint 3.13 */ >> constexpr uint16_t kFixedPoint3_13 = 8191; >> >> return value * kFixedPoint3_13; >> } > > In a 3.13 fixed-point format, 1.0 would be 8192, not 8191. The above > would thus be misleading. 8191 is the maximum value of a 12bpp pixel, > it's not a fixed-point number in this context. > >> void Awb::prepare(IPAContext &context, ipu3_uapi_params *params) >> { >> /* >> * Green saturation thresholds are reduced because... >> */ >> params->acc_param.awb.config.rgbs_thr_r = Threshold(1.0); >> params->acc_param.awb.config.rgbs_thr_gr = Threshold(0.9); >> params->acc_param.awb.config.rgbs_thr_gb = Threshold(0.9); >> params->acc_param.awb.config.rgbs_thr_b = Threshold(1.0); >> >> /* Enable saturation inclusion on thr_b because ... */ >> params->acc_param.awb.config.rgbs_thr_b |= IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT | >> IPU3_UAPI_AWB_RGBS_THR_B_EN; > > I like splitting this from the previous line. > >> ... >> } >
Hi Laurent, On 14/10/2021 23:56, Laurent Pinchart wrote: > Hello, > > On Thu, Oct 14, 2021 at 11:23:13AM +0100, Kieran Bingham wrote: >> Quoting Jean-Michel Hautbois (2021-10-13 16:41:13) >>> We can have a saturation ratio per cell, giving the percentage of pixels >>> over a threshold within a cell where 100% is set to 0xff. >>> >>> The parameter structure 'ipu3_uapi_awb_config_s' contains four fields to >>> set the threshold, one per channel. >>> The blue field is also used to configure the ImgU and make it calculate >>> the saturation ratio or not. >>> >>> Set a green value saturated when it is more than 230 (90% of the maximum >>> value 255, coded as 8191). >> >> Why do we only determine a lower saturation on one component? > > I assume because we only use the stats for that component :-) > >> Shouldn't they be balanced (perhaps in accordance with the white balance >> somehow?). > > If the assumption is correct, then we could, and it would make no > difference. > >>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> >>> --- >>> src/ipa/ipu3/algorithms/awb.cpp | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp >>> index e2b18336..5574bd44 100644 >>> --- a/src/ipa/ipu3/algorithms/awb.cpp >>> +++ b/src/ipa/ipu3/algorithms/awb.cpp >>> @@ -326,9 +326,9 @@ void Awb::process(IPAContext &context, const ipu3_uapi_stats_3a *stats) >>> >>> void Awb::prepare(IPAContext &context, ipu3_uapi_params *params) >>> { >>> - params->acc_param.awb.config.rgbs_thr_gr = 8191; >>> + params->acc_param.awb.config.rgbs_thr_gr = (90 * 8191 / 100); > > No need for parentheses. > >>> params->acc_param.awb.config.rgbs_thr_r = 8191; >>> - params->acc_param.awb.config.rgbs_thr_gb = 8191; >>> + params->acc_param.awb.config.rgbs_thr_gb = (90 * 8191 / 100); >>> params->acc_param.awb.config.rgbs_thr_b = IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT >>> | IPU3_UAPI_AWB_RGBS_THR_B_EN >>> | 8191; >> >> That's interesting that thr_b has those enable flags. Do they apply >> only to B? or does that enable all of them? > > As far as I undertand, the single flag applies to all channels. > >> I wonder if a helper function would make the values clearer: >> >> (Awb private:) >> uint16_t Awb::Threshold(float value) > > Make it > > constexpr uint16_t Awb::Threshold(float value) > >> { >> /* AWB Thresholds are represented in FixedPoint 3.13 */ >> constexpr uint16_t kFixedPoint3_13 = 8191; >> >> return value * kFixedPoint3_13; >> } > > In a 3.13 fixed-point format, 1.0 would be 8192, not 8191. The above > would thus be misleading. 8191 is the maximum value of a 12bpp pixel, > it's not a fixed-point number in this context. I am not sure I understand this comment :-/. I started by changing - constexpr uint16_t kFixedPoint3_13 = 8191; + constexpr uint16_t kFixedPoint3_13 = 8192; But now I read it again, and I think I might have misunderstand ? > >> void Awb::prepare(IPAContext &context, ipu3_uapi_params *params) >> { >> /* >> * Green saturation thresholds are reduced because... >> */ >> params->acc_param.awb.config.rgbs_thr_r = Threshold(1.0); >> params->acc_param.awb.config.rgbs_thr_gr = Threshold(0.9); >> params->acc_param.awb.config.rgbs_thr_gb = Threshold(0.9); >> params->acc_param.awb.config.rgbs_thr_b = Threshold(1.0); >> >> /* Enable saturation inclusion on thr_b because ... */ >> params->acc_param.awb.config.rgbs_thr_b |= IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT | >> IPU3_UAPI_AWB_RGBS_THR_B_EN; > > I like splitting this from the previous line. > >> ... >> } >
On 20/10/2021 10:41, Jean-Michel Hautbois wrote: > Hi Laurent, > > On 14/10/2021 23:56, Laurent Pinchart wrote: >> Hello, >> >> On Thu, Oct 14, 2021 at 11:23:13AM +0100, Kieran Bingham wrote: >>> Quoting Jean-Michel Hautbois (2021-10-13 16:41:13) >>>> We can have a saturation ratio per cell, giving the percentage of >>>> pixels >>>> over a threshold within a cell where 100% is set to 0xff. >>>> >>>> The parameter structure 'ipu3_uapi_awb_config_s' contains four >>>> fields to >>>> set the threshold, one per channel. >>>> The blue field is also used to configure the ImgU and make it calculate >>>> the saturation ratio or not. >>>> >>>> Set a green value saturated when it is more than 230 (90% of the >>>> maximum >>>> value 255, coded as 8191). >>> >>> Why do we only determine a lower saturation on one component? >> >> I assume because we only use the stats for that component :-) >> >>> Shouldn't they be balanced (perhaps in accordance with the white balance >>> somehow?). >> >> If the assumption is correct, then we could, and it would make no >> difference. >> >>>> Signed-off-by: Jean-Michel Hautbois >>>> <jeanmichel.hautbois@ideasonboard.com> >>>> --- >>>> src/ipa/ipu3/algorithms/awb.cpp | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/src/ipa/ipu3/algorithms/awb.cpp >>>> b/src/ipa/ipu3/algorithms/awb.cpp >>>> index e2b18336..5574bd44 100644 >>>> --- a/src/ipa/ipu3/algorithms/awb.cpp >>>> +++ b/src/ipa/ipu3/algorithms/awb.cpp >>>> @@ -326,9 +326,9 @@ void Awb::process(IPAContext &context, const >>>> ipu3_uapi_stats_3a *stats) >>>> void Awb::prepare(IPAContext &context, ipu3_uapi_params *params) >>>> { >>>> - params->acc_param.awb.config.rgbs_thr_gr = 8191; >>>> + params->acc_param.awb.config.rgbs_thr_gr = (90 * 8191 / 100); >> >> No need for parentheses. >> >>>> params->acc_param.awb.config.rgbs_thr_r = 8191; >>>> - params->acc_param.awb.config.rgbs_thr_gb = 8191; >>>> + params->acc_param.awb.config.rgbs_thr_gb = (90 * 8191 / 100); >>>> params->acc_param.awb.config.rgbs_thr_b = >>>> IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT >>>> | >>>> IPU3_UAPI_AWB_RGBS_THR_B_EN >>>> | 8191; >>> >>> That's interesting that thr_b has those enable flags. Do they apply >>> only to B? or does that enable all of them? >> >> As far as I undertand, the single flag applies to all channels. >> >>> I wonder if a helper function would make the values clearer: >>> >>> (Awb private:) >>> uint16_t Awb::Threshold(float value) >> >> Make it >> >> constexpr uint16_t Awb::Threshold(float value) >> >>> { >>> /* AWB Thresholds are represented in FixedPoint 3.13 */ >>> constexpr uint16_t kFixedPoint3_13 = 8191; >>> >>> return value * kFixedPoint3_13; >>> } >> >> In a 3.13 fixed-point format, 1.0 would be 8192, not 8191. The above >> would thus be misleading. 8191 is the maximum value of a 12bpp pixel, >> it's not a fixed-point number in this context. > > I am not sure I understand this comment :-/. I started by changing > - constexpr uint16_t kFixedPoint3_13 = 8191; > + constexpr uint16_t kFixedPoint3_13 = 8192; > > But now I read it again, and I think I might have misunderstand ? I have done that, I think this is what you meant ? +constexpr uint16_t Awb::Threshold(float value) +{ + /* AWB Thresholds are in the range [0, 8191] */ + constexpr uint16_t kMaxThreshold = 8191; + + return value * kMaxThreshold; +} + > >> >>> void Awb::prepare(IPAContext &context, ipu3_uapi_params *params) >>> { >>> /* >>> * Green saturation thresholds are reduced because... >>> */ >>> params->acc_param.awb.config.rgbs_thr_r = Threshold(1.0); >>> params->acc_param.awb.config.rgbs_thr_gr = Threshold(0.9); >>> params->acc_param.awb.config.rgbs_thr_gb = Threshold(0.9); >>> params->acc_param.awb.config.rgbs_thr_b = Threshold(1.0); >>> >>> /* Enable saturation inclusion on thr_b because ... */ >>> params->acc_param.awb.config.rgbs_thr_b |= >>> IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT | >>> IPU3_UAPI_AWB_RGBS_THR_B_EN; >> >> I like splitting this from the previous line. >> >>> ... >>> } >>
Quoting Jean-Michel Hautbois (2021-10-20 09:53:54) > > > On 20/10/2021 10:41, Jean-Michel Hautbois wrote: > > Hi Laurent, > > > > On 14/10/2021 23:56, Laurent Pinchart wrote: > >> Hello, > >> > >> On Thu, Oct 14, 2021 at 11:23:13AM +0100, Kieran Bingham wrote: > >>> Quoting Jean-Michel Hautbois (2021-10-13 16:41:13) > >>>> We can have a saturation ratio per cell, giving the percentage of > >>>> pixels > >>>> over a threshold within a cell where 100% is set to 0xff. > >>>> > >>>> The parameter structure 'ipu3_uapi_awb_config_s' contains four > >>>> fields to > >>>> set the threshold, one per channel. > >>>> The blue field is also used to configure the ImgU and make it calculate > >>>> the saturation ratio or not. > >>>> > >>>> Set a green value saturated when it is more than 230 (90% of the > >>>> maximum > >>>> value 255, coded as 8191). > >>> > >>> Why do we only determine a lower saturation on one component? > >> > >> I assume because we only use the stats for that component :-) > >> > >>> Shouldn't they be balanced (perhaps in accordance with the white balance > >>> somehow?). > >> > >> If the assumption is correct, then we could, and it would make no > >> difference. > >> > >>>> Signed-off-by: Jean-Michel Hautbois > >>>> <jeanmichel.hautbois@ideasonboard.com> > >>>> --- > >>>> src/ipa/ipu3/algorithms/awb.cpp | 4 ++-- > >>>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/src/ipa/ipu3/algorithms/awb.cpp > >>>> b/src/ipa/ipu3/algorithms/awb.cpp > >>>> index e2b18336..5574bd44 100644 > >>>> --- a/src/ipa/ipu3/algorithms/awb.cpp > >>>> +++ b/src/ipa/ipu3/algorithms/awb.cpp > >>>> @@ -326,9 +326,9 @@ void Awb::process(IPAContext &context, const > >>>> ipu3_uapi_stats_3a *stats) > >>>> void Awb::prepare(IPAContext &context, ipu3_uapi_params *params) > >>>> { > >>>> - params->acc_param.awb.config.rgbs_thr_gr = 8191; > >>>> + params->acc_param.awb.config.rgbs_thr_gr = (90 * 8191 / 100); > >> > >> No need for parentheses. > >> > >>>> params->acc_param.awb.config.rgbs_thr_r = 8191; > >>>> - params->acc_param.awb.config.rgbs_thr_gb = 8191; > >>>> + params->acc_param.awb.config.rgbs_thr_gb = (90 * 8191 / 100); > >>>> params->acc_param.awb.config.rgbs_thr_b = > >>>> IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT > >>>> | > >>>> IPU3_UAPI_AWB_RGBS_THR_B_EN > >>>> | 8191; > >>> > >>> That's interesting that thr_b has those enable flags. Do they apply > >>> only to B? or does that enable all of them? > >> > >> As far as I undertand, the single flag applies to all channels. > >> > >>> I wonder if a helper function would make the values clearer: > >>> > >>> (Awb private:) > >>> uint16_t Awb::Threshold(float value) > >> > >> Make it > >> > >> constexpr uint16_t Awb::Threshold(float value) > >> > >>> { > >>> /* AWB Thresholds are represented in FixedPoint 3.13 */ > >>> constexpr uint16_t kFixedPoint3_13 = 8191; > >>> > >>> return value * kFixedPoint3_13; > >>> } > >> > >> In a 3.13 fixed-point format, 1.0 would be 8192, not 8191. The above > >> would thus be misleading. 8191 is the maximum value of a 12bpp pixel, > >> it's not a fixed-point number in this context. > > > > I am not sure I understand this comment :-/. I started by changing > > - constexpr uint16_t kFixedPoint3_13 = 8191; > > + constexpr uint16_t kFixedPoint3_13 = 8192; > > > > But now I read it again, and I think I might have misunderstand ? > > I have done that, I think this is what you meant ? > > +constexpr uint16_t Awb::Threshold(float value) > +{ > + /* AWB Thresholds are in the range [0, 8191] */ > + constexpr uint16_t kMaxThreshold = 8191; > + That's probably better, yes. Do we need to clamp the input values? Maybe not as we're in control of them anyway. Perhaps a compile time ASSERT(value >= 0 && value <= 1.0) ? or such? > + return value * kMaxThreshold; > +} > + > > > > >> > >>> void Awb::prepare(IPAContext &context, ipu3_uapi_params *params) > >>> { > >>> /* > >>> * Green saturation thresholds are reduced because... > >>> */ > >>> params->acc_param.awb.config.rgbs_thr_r = Threshold(1.0); > >>> params->acc_param.awb.config.rgbs_thr_gr = Threshold(0.9); > >>> params->acc_param.awb.config.rgbs_thr_gb = Threshold(0.9); > >>> params->acc_param.awb.config.rgbs_thr_b = Threshold(1.0); > >>> > >>> /* Enable saturation inclusion on thr_b because ... */ > >>> params->acc_param.awb.config.rgbs_thr_b |= > >>> IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT | > >>> IPU3_UAPI_AWB_RGBS_THR_B_EN; > >> > >> I like splitting this from the previous line. > >> > >>> ... > >>> } > >>
On Wed, Oct 20, 2021 at 10:34:21AM +0100, Kieran Bingham wrote: > Quoting Jean-Michel Hautbois (2021-10-20 09:53:54) > > On 20/10/2021 10:41, Jean-Michel Hautbois wrote: > > > On 14/10/2021 23:56, Laurent Pinchart wrote: > > >> On Thu, Oct 14, 2021 at 11:23:13AM +0100, Kieran Bingham wrote: > > >>> Quoting Jean-Michel Hautbois (2021-10-13 16:41:13) > > >>>> We can have a saturation ratio per cell, giving the percentage of > > >>>> pixels > > >>>> over a threshold within a cell where 100% is set to 0xff. > > >>>> > > >>>> The parameter structure 'ipu3_uapi_awb_config_s' contains four > > >>>> fields to > > >>>> set the threshold, one per channel. > > >>>> The blue field is also used to configure the ImgU and make it calculate > > >>>> the saturation ratio or not. > > >>>> > > >>>> Set a green value saturated when it is more than 230 (90% of the > > >>>> maximum > > >>>> value 255, coded as 8191). > > >>> > > >>> Why do we only determine a lower saturation on one component? > > >> > > >> I assume because we only use the stats for that component :-) > > >> > > >>> Shouldn't they be balanced (perhaps in accordance with the white balance > > >>> somehow?). > > >> > > >> If the assumption is correct, then we could, and it would make no > > >> difference. > > >> > > >>>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > > >>>> --- > > >>>> src/ipa/ipu3/algorithms/awb.cpp | 4 ++-- > > >>>> 1 file changed, 2 insertions(+), 2 deletions(-) > > >>>> > > >>>> diff --git a/src/ipa/ipu3/algorithms/awb.cpp > > >>>> b/src/ipa/ipu3/algorithms/awb.cpp > > >>>> index e2b18336..5574bd44 100644 > > >>>> --- a/src/ipa/ipu3/algorithms/awb.cpp > > >>>> +++ b/src/ipa/ipu3/algorithms/awb.cpp > > >>>> @@ -326,9 +326,9 @@ void Awb::process(IPAContext &context, const > > >>>> ipu3_uapi_stats_3a *stats) > > >>>> void Awb::prepare(IPAContext &context, ipu3_uapi_params *params) > > >>>> { > > >>>> - params->acc_param.awb.config.rgbs_thr_gr = 8191; > > >>>> + params->acc_param.awb.config.rgbs_thr_gr = (90 * 8191 / 100); > > >> > > >> No need for parentheses. > > >> > > >>>> params->acc_param.awb.config.rgbs_thr_r = 8191; > > >>>> - params->acc_param.awb.config.rgbs_thr_gb = 8191; > > >>>> + params->acc_param.awb.config.rgbs_thr_gb = (90 * 8191 / 100); > > >>>> params->acc_param.awb.config.rgbs_thr_b = > > >>>> IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT > > >>>> | > > >>>> IPU3_UAPI_AWB_RGBS_THR_B_EN > > >>>> | 8191; > > >>> > > >>> That's interesting that thr_b has those enable flags. Do they apply > > >>> only to B? or does that enable all of them? > > >> > > >> As far as I undertand, the single flag applies to all channels. > > >> > > >>> I wonder if a helper function would make the values clearer: > > >>> > > >>> (Awb private:) > > >>> uint16_t Awb::Threshold(float value) > > >> > > >> Make it > > >> > > >> constexpr uint16_t Awb::Threshold(float value) > > >> > > >>> { > > >>> /* AWB Thresholds are represented in FixedPoint 3.13 */ > > >>> constexpr uint16_t kFixedPoint3_13 = 8191; > > >>> > > >>> return value * kFixedPoint3_13; > > >>> } > > >> > > >> In a 3.13 fixed-point format, 1.0 would be 8192, not 8191. The above > > >> would thus be misleading. 8191 is the maximum value of a 12bpp pixel, > > >> it's not a fixed-point number in this context. > > > > > > I am not sure I understand this comment :-/. I started by changing > > > - constexpr uint16_t kFixedPoint3_13 = 8191; > > > + constexpr uint16_t kFixedPoint3_13 = 8192; > > > > > > But now I read it again, and I think I might have misunderstand ? The point was that in U3.13 fixed-point format, 1.0 would be encoded as 8192, not 8191. The values we deal with here are not fixed-point decimal numbers, they're just integer pixel values in the range [0, 8191]. If we want to map these to a floating-point [0.0, 1.0] range internally to make it more readable (0.8 immediately shows as 80%, while 6553 doesn't), that's fine, but we shouldn't pretend the values are fixed-point decimals. > > I have done that, I think this is what you meant ? > > > > +constexpr uint16_t Awb::Threshold(float value) s/Threshold/threshold/ > > +{ > > + /* AWB Thresholds are in the range [0, 8191] */ > > + constexpr uint16_t kMaxThreshold = 8191; > > + > > That's probably better, yes. > > Do we need to clamp the input values? Maybe not as we're in control of > them anyway. Perhaps a compile time ASSERT(value >= 0 && value <= 1.0) ? or such? That's not compile-time :-) static_assert could be used, but that would require the argument to be a constexpr. That's not a problem here. > > + return value * kMaxThreshold; > > +} > > + > > > > >>> void Awb::prepare(IPAContext &context, ipu3_uapi_params *params) > > >>> { > > >>> /* > > >>> * Green saturation thresholds are reduced because... > > >>> */ > > >>> params->acc_param.awb.config.rgbs_thr_r = Threshold(1.0); > > >>> params->acc_param.awb.config.rgbs_thr_gr = Threshold(0.9); > > >>> params->acc_param.awb.config.rgbs_thr_gb = Threshold(0.9); > > >>> params->acc_param.awb.config.rgbs_thr_b = Threshold(1.0); > > >>> > > >>> /* Enable saturation inclusion on thr_b because ... */ > > >>> params->acc_param.awb.config.rgbs_thr_b |= > > >>> IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT | > > >>> IPU3_UAPI_AWB_RGBS_THR_B_EN; > > >> > > >> I like splitting this from the previous line. > > >> > > >>> ... > > >>> }
Quoting Laurent Pinchart (2021-10-20 10:46:01) > On Wed, Oct 20, 2021 at 10:34:21AM +0100, Kieran Bingham wrote: > > Quoting Jean-Michel Hautbois (2021-10-20 09:53:54) > > > On 20/10/2021 10:41, Jean-Michel Hautbois wrote: > > > > On 14/10/2021 23:56, Laurent Pinchart wrote: > > > >> On Thu, Oct 14, 2021 at 11:23:13AM +0100, Kieran Bingham wrote: > > > >>> Quoting Jean-Michel Hautbois (2021-10-13 16:41:13) > > > >>>> We can have a saturation ratio per cell, giving the percentage of > > > >>>> pixels > > > >>>> over a threshold within a cell where 100% is set to 0xff. > > > >>>> > > > >>>> The parameter structure 'ipu3_uapi_awb_config_s' contains four > > > >>>> fields to > > > >>>> set the threshold, one per channel. > > > >>>> The blue field is also used to configure the ImgU and make it calculate > > > >>>> the saturation ratio or not. > > > >>>> > > > >>>> Set a green value saturated when it is more than 230 (90% of the > > > >>>> maximum > > > >>>> value 255, coded as 8191). > > > >>> > > > >>> Why do we only determine a lower saturation on one component? > > > >> > > > >> I assume because we only use the stats for that component :-) > > > >> > > > >>> Shouldn't they be balanced (perhaps in accordance with the white balance > > > >>> somehow?). > > > >> > > > >> If the assumption is correct, then we could, and it would make no > > > >> difference. > > > >> > > > >>>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > > > >>>> --- > > > >>>> src/ipa/ipu3/algorithms/awb.cpp | 4 ++-- > > > >>>> 1 file changed, 2 insertions(+), 2 deletions(-) > > > >>>> > > > >>>> diff --git a/src/ipa/ipu3/algorithms/awb.cpp > > > >>>> b/src/ipa/ipu3/algorithms/awb.cpp > > > >>>> index e2b18336..5574bd44 100644 > > > >>>> --- a/src/ipa/ipu3/algorithms/awb.cpp > > > >>>> +++ b/src/ipa/ipu3/algorithms/awb.cpp > > > >>>> @@ -326,9 +326,9 @@ void Awb::process(IPAContext &context, const > > > >>>> ipu3_uapi_stats_3a *stats) > > > >>>> void Awb::prepare(IPAContext &context, ipu3_uapi_params *params) > > > >>>> { > > > >>>> - params->acc_param.awb.config.rgbs_thr_gr = 8191; > > > >>>> + params->acc_param.awb.config.rgbs_thr_gr = (90 * 8191 / 100); > > > >> > > > >> No need for parentheses. > > > >> > > > >>>> params->acc_param.awb.config.rgbs_thr_r = 8191; > > > >>>> - params->acc_param.awb.config.rgbs_thr_gb = 8191; > > > >>>> + params->acc_param.awb.config.rgbs_thr_gb = (90 * 8191 / 100); > > > >>>> params->acc_param.awb.config.rgbs_thr_b = > > > >>>> IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT > > > >>>> | > > > >>>> IPU3_UAPI_AWB_RGBS_THR_B_EN > > > >>>> | 8191; > > > >>> > > > >>> That's interesting that thr_b has those enable flags. Do they apply > > > >>> only to B? or does that enable all of them? > > > >> > > > >> As far as I undertand, the single flag applies to all channels. > > > >> > > > >>> I wonder if a helper function would make the values clearer: > > > >>> > > > >>> (Awb private:) > > > >>> uint16_t Awb::Threshold(float value) > > > >> > > > >> Make it > > > >> > > > >> constexpr uint16_t Awb::Threshold(float value) > > > >> > > > >>> { > > > >>> /* AWB Thresholds are represented in FixedPoint 3.13 */ > > > >>> constexpr uint16_t kFixedPoint3_13 = 8191; > > > >>> > > > >>> return value * kFixedPoint3_13; > > > >>> } > > > >> > > > >> In a 3.13 fixed-point format, 1.0 would be 8192, not 8191. The above > > > >> would thus be misleading. 8191 is the maximum value of a 12bpp pixel, > > > >> it's not a fixed-point number in this context. > > > > > > > > I am not sure I understand this comment :-/. I started by changing > > > > - constexpr uint16_t kFixedPoint3_13 = 8191; > > > > + constexpr uint16_t kFixedPoint3_13 = 8192; > > > > > > > > But now I read it again, and I think I might have misunderstand ? > > The point was that in U3.13 fixed-point format, 1.0 would be encoded as > 8192, not 8191. The values we deal with here are not fixed-point decimal > numbers, they're just integer pixel values in the range [0, 8191]. If we > want to map these to a floating-point [0.0, 1.0] range internally to > make it more readable (0.8 immediately shows as 80%, while 6553 > doesn't), that's fine, but we shouldn't pretend the values are > fixed-point decimals. > > > > I have done that, I think this is what you meant ? > > > > > > +constexpr uint16_t Awb::Threshold(float value) > > s/Threshold/threshold/ > > > > +{ > > > + /* AWB Thresholds are in the range [0, 8191] */ > > > + constexpr uint16_t kMaxThreshold = 8191; > > > + > > > > That's probably better, yes. > > > > Do we need to clamp the input values? Maybe not as we're in control of > > them anyway. Perhaps a compile time ASSERT(value >= 0 && value <= 1.0) ? or such? > > That's not compile-time :-) static_assert could be used, but that would > require the argument to be a constexpr. That's not a problem here. > Well then, that's what I meant ;-) -- Kieran > > > + return value * kMaxThreshold; > > > +} > > > + > > > > > > >>> void Awb::prepare(IPAContext &context, ipu3_uapi_params *params) > > > >>> { > > > >>> /* > > > >>> * Green saturation thresholds are reduced because... > > > >>> */ > > > >>> params->acc_param.awb.config.rgbs_thr_r = Threshold(1.0); > > > >>> params->acc_param.awb.config.rgbs_thr_gr = Threshold(0.9); > > > >>> params->acc_param.awb.config.rgbs_thr_gb = Threshold(0.9); > > > >>> params->acc_param.awb.config.rgbs_thr_b = Threshold(1.0); > > > >>> > > > >>> /* Enable saturation inclusion on thr_b because ... */ > > > >>> params->acc_param.awb.config.rgbs_thr_b |= > > > >>> IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT | > > > >>> IPU3_UAPI_AWB_RGBS_THR_B_EN; > > > >> > > > >> I like splitting this from the previous line. > > > >> > > > >>> ... > > > >>> } > > -- > Regards, > > Laurent Pinchart
diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp index e2b18336..5574bd44 100644 --- a/src/ipa/ipu3/algorithms/awb.cpp +++ b/src/ipa/ipu3/algorithms/awb.cpp @@ -326,9 +326,9 @@ void Awb::process(IPAContext &context, const ipu3_uapi_stats_3a *stats) void Awb::prepare(IPAContext &context, ipu3_uapi_params *params) { - params->acc_param.awb.config.rgbs_thr_gr = 8191; + params->acc_param.awb.config.rgbs_thr_gr = (90 * 8191 / 100); params->acc_param.awb.config.rgbs_thr_r = 8191; - params->acc_param.awb.config.rgbs_thr_gb = 8191; + params->acc_param.awb.config.rgbs_thr_gb = (90 * 8191 / 100); params->acc_param.awb.config.rgbs_thr_b = IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT | IPU3_UAPI_AWB_RGBS_THR_B_EN | 8191;
We can have a saturation ratio per cell, giving the percentage of pixels over a threshold within a cell where 100% is set to 0xff. The parameter structure 'ipu3_uapi_awb_config_s' contains four fields to set the threshold, one per channel. The blue field is also used to configure the ImgU and make it calculate the saturation ratio or not. Set a green value saturated when it is more than 230 (90% of the maximum value 255, coded as 8191). Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> --- src/ipa/ipu3/algorithms/awb.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)