[PATCH v3 09/16] ipa: rkisp1: Refactor automatic/manual structure in IPAActiveState
Paul Elder
paul.elder at ideasonboard.com
Wed May 21 19:24:39 CEST 2025
Quoting Stefan Klug (2025-05-20 10:00:08)
> 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
Yeah, we can do that on top.
> while applying.
Cool :) with that fixed,
Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
>
> 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
> > >
More information about the libcamera-devel
mailing list