[PATCH v3 09/16] ipa: rkisp1: Refactor automatic/manual structure in IPAActiveState

[Stefan Klug]

Hi Paul,

Thank you for the review.

Quoting Paul Elder (2025-05-07 18:51:17)
> On Thu, Apr 03, 2025 at 05:49:14PM +0200, Stefan Klug wrote:
> > Swap gains and automatic/manual in the IPAActiveState structure. This is
> > in preparation to adding another member, which is easier in the new
> > structure. The patch contains no functional changes.
> > 
> > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > 
> > ---
> > 
> > Changes in v2:
> > - Use one named struct instead of two anonymous ones
> > 
> > Changes in v3:
> > - Collected tags
> > - Updated documentation
> > ---
> >  src/ipa/rkisp1/algorithms/awb.cpp | 24 ++++++++++++------------
> >  src/ipa/rkisp1/ipa_context.cpp    | 13 ++++++++-----
> >  src/ipa/rkisp1/ipa_context.h      | 10 ++++++----
> >  3 files changed, 26 insertions(+), 21 deletions(-)
> > 
> > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> > index eafe93081bb1..a9759e53f593 100644
> > --- a/src/ipa/rkisp1/algorithms/awb.cpp
> > +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> > @@ -124,8 +124,8 @@ int Awb::init(IPAContext &context, const YamlObject &tuningData)
> >  int Awb::configure(IPAContext &context,
> >                  const IPACameraSensorInfo &configInfo)
> >  {
> > -     context.activeState.awb.gains.manual = RGB<double>{ 1.0 };
> > -     context.activeState.awb.gains.automatic =
> > +     context.activeState.awb.manual.gains = RGB<double>{ 1.0 };
> > +     context.activeState.awb.automatic.gains =
> >               awbAlgo_->gainsFromColourTemperature(kDefaultColourTemperature);
> >       context.activeState.awb.autoEnabled = true;
> >       context.activeState.awb.temperatureK = kDefaultColourTemperature;
> > @@ -173,8 +173,8 @@ void Awb::queueRequest(IPAContext &context,
> >       const auto &colourTemperature = controls.get(controls::ColourTemperature);
> >       bool update = false;
> >       if (colourGains) {
> > -             awb.gains.manual.r() = (*colourGains)[0];
> > -             awb.gains.manual.b() = (*colourGains)[1];
> > +             awb.manual.gains.r() = (*colourGains)[0];
> > +             awb.manual.gains.b() = (*colourGains)[1];
> >               /*
> >                * \todo Colour temperature reported in metadata is now
> >                * incorrect, as we can't deduce the temperature from the gains.
> > @@ -183,17 +183,17 @@ void Awb::queueRequest(IPAContext &context,
> >               update = true;
> >       } else if (colourTemperature) {
> >               const auto &gains = awbAlgo_->gainsFromColourTemperature(*colourTemperature);
> > -             awb.gains.manual.r() = gains.r();
> > -             awb.gains.manual.b() = gains.b();
> > +             awb.manual.gains.r() = gains.r();
> > +             awb.manual.gains.b() = gains.b();
> >               awb.temperatureK = *colourTemperature;
> >               update = true;
> >       }
> >  
> >       if (update)
> >               LOG(RkISP1Awb, Debug)
> > -                     << "Set colour gains to " << awb.gains.manual;
> > +                     << "Set colour gains to " << awb.manual.gains;
> >  
> > -     frameContext.awb.gains = awb.gains.manual;
> > +     frameContext.awb.gains = awb.manual.gains;
> >       frameContext.awb.temperatureK = awb.temperatureK;
> >  }
> >  
> > @@ -208,7 +208,7 @@ void Awb::prepare(IPAContext &context, const uint32_t frame,
> >        * most up-to-date automatic values we can read.
> >        */
> >       if (frameContext.awb.autoEnabled) {
> > -             frameContext.awb.gains = context.activeState.awb.gains.automatic;
> > +             frameContext.awb.gains = context.activeState.awb.automatic.gains;
> >               frameContext.awb.temperatureK = context.activeState.awb.temperatureK;
> >       }
> >  
> > @@ -325,14 +325,14 @@ void Awb::process(IPAContext &context,
> >       /* Filter the values to avoid oscillations. */
> >       double speed = 0.2;
> >       awbResult.gains = awbResult.gains * speed +
> > -                       activeState.awb.gains.automatic * (1 - speed);
> > +                       activeState.awb.automatic.gains * (1 - speed);
> >  
> > -     activeState.awb.gains.automatic = awbResult.gains;
> > +     activeState.awb.automatic.gains = awbResult.gains;
> >  
> >       LOG(RkISP1Awb, Debug)
> >               << std::showpoint
> >               << "Means " << rgbMeans << ", gains "
> > -             << activeState.awb.gains.automatic << ", temp "
> > +             << activeState.awb.automatic.gains << ", temp "
> >               << activeState.awb.temperatureK << "K";
> >  }
> >  
> > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> > index 99611bd5b390..39b97d143e95 100644
> > --- a/src/ipa/rkisp1/ipa_context.cpp
> > +++ b/src/ipa/rkisp1/ipa_context.cpp
> > @@ -191,14 +191,17 @@ namespace libcamera::ipa::rkisp1 {
> >   * \var IPAActiveState::awb
> >   * \brief State for the Automatic White Balance algorithm
> >   *
> > - * \struct IPAActiveState::awb.gains
> > + * \var IPAActiveState::awb::AwbState
> 
> Shouldn't this be \struct ?
> 
> > + * \brief Struct for the AWB regulation state
> > + *
> > + * \struct IPAActiveState::awb::AwbState.gains
> 
> And this \var ?

I think you are right on both cases. As the rkisp1 is currently excluded
from doxygen generation, these errors don't pop up. We should enable
doxygen for that, but that is for another series :-). I'll change that
while applying.

Cheers,
Stefan

> 
> 
> Paul
> 
> >   * \brief White balance gains
> >   *
> > - * \var IPAActiveState::awb.gains.manual
> > - * \brief Manual white balance gains (set through requests)
> > + * \var IPAActiveState::awb.manual
> > + * \brief Manual regulation state (set through requests)
> >   *
> > - * \var IPAActiveState::awb.gains.automatic
> > - * \brief Automatic white balance gains (computed by the algorithm)
> > + * \var IPAActiveState::awb.automatic
> > + * \brief Automatic regulation state (computed by the algorithm)
> >   *
> >   * \var IPAActiveState::awb.temperatureK
> >   * \brief Estimated color temperature
> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > index 474f7036f003..6bc922a82971 100644
> > --- a/src/ipa/rkisp1/ipa_context.h
> > +++ b/src/ipa/rkisp1/ipa_context.h
> > @@ -89,10 +89,12 @@ struct IPAActiveState {
> >       } agc;
> >  
> >       struct {
> > -             struct {
> > -                     RGB<double> manual;
> > -                     RGB<double> automatic;
> > -             } gains;
> > +             struct AwbState {
> > +                     RGB<double> gains;
> > +             };
> > +
> > +             AwbState manual;
> > +             AwbState automatic;
> >  
> >               unsigned int temperatureK;
> >               bool autoEnabled;
> > -- 
> > 2.43.0
> >