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

[Laurent Pinchart]

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 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 ?

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

-- 
Regards,

Laurent Pinchart