[PATCH v5 3/6] ipa: simple: Report the ColourGains in metadata

[Kieran Bingham]

Quoting Milan Zamazal (2025-03-27 20:18:35)
> Hi Kieran,
> 
> thank you for review.
> 
> Kieran Bingham <kieran.bingham at ideasonboard.com> writes:
> 
> > Quoting Milan Zamazal (2025-03-26 12:48:52)
> >> From: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >> 
> >
> >> Provide the determined colour gains back into the metadata
> >> to add to completed requests.
> >> 
> >> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
> >> ---
> >>  src/ipa/simple/algorithms/awb.cpp | 21 ++++++++++++++++++++-
> >>  src/ipa/simple/algorithms/awb.h   |  6 +++++-
> >>  src/ipa/simple/ipa_context.h      |  4 ++++
> >>  3 files changed, 29 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp
> >> index ec77c6e5..55719059 100644
> >> --- a/src/ipa/simple/algorithms/awb.cpp
> >> +++ b/src/ipa/simple/algorithms/awb.cpp
> >> @@ -17,6 +17,8 @@
> >>  #include "libipa/colours.h"
> >>  #include "simple/ipa_context.h"
> >>  
> >> +#include "control_ids.h"
> >> +
> >>  namespace libcamera {
> >>  
> >>  LOG_DEFINE_CATEGORY(IPASoftAwb)
> >> @@ -32,15 +34,32 @@ int Awb::configure(IPAContext &context,
> >>         return 0;
> >>  }
> >>  
> >> +void Awb::prepare(IPAContext &context,
> >> +                 [[maybe_unused]] const uint32_t frame,
> >> +                 IPAFrameContext &frameContext,
> >> +                 [[maybe_unused]] DebayerParams *params)
> >> +{
> >> +       auto &gains = context.activeState.awb.gains;
> >> +       frameContext.gains.red = gains.r();
> >> +       frameContext.gains.blue = gains.b();
> >
> > Interesting that this highlights the white balance parameters are
> > probably being used from the wrong 'time' as they should be determined
> > here in prepare... (by taking the most recent active state and combining
> > any decision from the controls for any manual white balance).
> 
> I'm not sure I understand what you mean.  They come from the most recent
> active state, don't they?  We don't support manual AWB balance but if we
> did the gains could be amended here accordingly.  And later used in Lut
> algorithm.  The gains stored here and used to process the frame are then
> reported in metadata for this frame.
> 
> What's wrong then?

I'm sorry - I was probably thrown off by the fact that I couldn't see
any configuration being set into the output params at this point. If
it's handled in a separate algo then I guess that's the correct timing
... but means we should probably have a note in here to say that the
actual handling of AWB is combined with the LUT component.

But my original point was likely wrong above.

--
Kieran




> > But - as this series is just trying to get the reporting in - lets leave
> > that issue for later, and try to keep this moving forwards:
> >
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >
> >
> >
> >> +}
> >> +
> >>  void Awb::process(IPAContext &context,
> >>                   [[maybe_unused]] const uint32_t frame,
> >> -                 [[maybe_unused]] IPAFrameContext &frameContext,
> >> +                 IPAFrameContext &frameContext,
> >>                   const SwIspStats *stats,
> >>                   ControlList &metadata)
> >>  {
> >>         const SwIspStats::Histogram &histogram = stats->yHistogram;
> >>         const uint8_t blackLevel = context.activeState.blc.level;
> >>  
> >> +       const float maxGain = 1024.0;
> >> +       const float mdGains[] = {
> >> +               static_cast<float>(frameContext.gains.red / maxGain),
> >> +               static_cast<float>(frameContext.gains.blue / maxGain)
> >> +       };
> >> +       metadata.set(controls::ColourGains, mdGains);
> >> +
> >>         /*
> >>          * Black level must be subtracted to get the correct AWB ratios, they
> >>          * would be off if they were computed from the whole brightness range
> >> diff --git a/src/ipa/simple/algorithms/awb.h b/src/ipa/simple/algorithms/awb.h
> >> index db1496cd..ad993f39 100644
> >> --- a/src/ipa/simple/algorithms/awb.h
> >> +++ b/src/ipa/simple/algorithms/awb.h
> >> @@ -1,6 +1,6 @@
> >>  /* SPDX-License-Identifier: LGPL-2.1-or-later */
> >>  /*
> >> - * Copyright (C) 2024, Red Hat Inc.
> >> + * Copyright (C) 2024-2025 Red Hat Inc.
> >>   *
> >>   * Auto white balance
> >>   */
> >> @@ -20,6 +20,10 @@ public:
> >>         ~Awb() = default;
> >>  
> >>         int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
> >> +       void prepare(IPAContext &context,
> >> +                    const uint32_t frame,
> >> +                    IPAFrameContext &frameContext,
> >> +                    DebayerParams *params) override;
> >>         void process(IPAContext &context,
> >>                      const uint32_t frame,
> >>                      IPAFrameContext &frameContext,
> >> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
> >> index 17bcd4ca..bfac835b 100644
> >> --- a/src/ipa/simple/ipa_context.h
> >> +++ b/src/ipa/simple/ipa_context.h
> >> @@ -70,6 +70,10 @@ struct IPAFrameContext : public FrameContext {
> >>                 int32_t exposure;
> >>                 double gain;
> >>         } sensor;
> >> +       struct {
> >> +               double red;
> >> +               double blue;
> >> +       } gains;
> >>  };
> >>  
> >>  struct IPAContext {
> >> -- 
> >> 2.49.0
> >>
>