[libcamera-devel] [PATCH 01/13] ipa: ipu3: awb: Set a threshold for the green saturation

[Kieran Bingham]

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 at 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.
> >>
> >>>     ...
> >>> }
> >>