[12/13] ipa: libipa: awb: convert to common queueRequest
diff mbox series

Message ID 20260407-kbingham-awb-split-v1-12-a39af3f4dc20@ideasonboard.com
State New
Headers show
Series
  • ipa: simple: Convert to libipa AWB implementation
Related show

Commit Message

Kieran Bingham April 7, 2026, 10:01 p.m. UTC 
Move the now duplicated implementation for both soft IPA and rkisp1 IPA
for managing requests at queue time into the common libipa AWB implementation.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/ipa/libipa/awb.cpp            | 59 +++++++++++++++++++++++++++++++++++++++
 src/ipa/libipa/awb.h              |  5 ++++
 src/ipa/rkisp1/algorithms/awb.cpp | 49 ++------------------------------
 src/ipa/simple/algorithms/awb.cpp | 52 ++++------------------------------
 4 files changed, 72 insertions(+), 93 deletions(-)

Comments

Jacopo Mondi April 8, 2026, 9:02 a.m. UTC | #1 
Hi Kieran

On Tue, Apr 07, 2026 at 11:01:15PM +0100, Kieran Bingham wrote:
> Move the now duplicated implementation for both soft IPA and rkisp1 IPA
> for managing requests at queue time into the common libipa AWB implementation.

requests ?

>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/ipa/libipa/awb.cpp            | 59 +++++++++++++++++++++++++++++++++++++++
>  src/ipa/libipa/awb.h              |  5 ++++
>  src/ipa/rkisp1/algorithms/awb.cpp | 49 ++------------------------------
>  src/ipa/simple/algorithms/awb.cpp | 52 ++++------------------------------
>  4 files changed, 72 insertions(+), 93 deletions(-)
>
> diff --git a/src/ipa/libipa/awb.cpp b/src/ipa/libipa/awb.cpp
> index d0c577958caebc0b7e24fe58506da964df2200fe..cac6b4bd206faa4f32680e16c7522685cfd30cac 100644
> --- a/src/ipa/libipa/awb.cpp
> +++ b/src/ipa/libipa/awb.cpp
> @@ -168,6 +168,65 @@ int AwbAlgorithm::configure(awb::ActiveState &state, awb::Session &session)
>  	return 0;
>  }
>
> +/**
> + * \brief Provide control values to the algorithm
> + * \param[in] state The AWB specific active state shared across frames
> + * \param[in] frame The frame number to apply the control values
> + * \param[in] frameContext The current frame's AWB specific context
> + * \param[in] controls The list of user controls
> + */
> +void AwbAlgorithm::queueRequest(awb::ActiveState &state,
> +				[[maybe_unused]] const uint32_t frame,

Should we aim at maintaining compatibility with the
Algorithm::queuRequest() interface or unused parameters should be dropped ?

> +				awb::FrameContext &frameContext,
> +				const ControlList &controls)
> +{
> +	const auto &awbEnable = controls.get(controls::AwbEnable);
> +	if (awbEnable && *awbEnable != state.autoEnabled) {
> +		state.autoEnabled = *awbEnable;
> +
> +		LOG(Awb, Debug)
> +			<< (*awbEnable ? "Enabling" : "Disabling") << " AWB";
> +	}
> +
> +	/* Handle controls from subclass algo (Grey or Bayes) */
> +	handleControls(controls);
> +
> +	frameContext.autoEnabled = state.autoEnabled;
> +
> +	/* Todo: Check to see if we should always parse the following controls */

Does this still apply ?

> +	if (frameContext.autoEnabled)
> +		return;
> +
> +	const auto &colourGains = controls.get(controls::ColourGains);
> +	const auto &colourTemperature = controls.get(controls::ColourTemperature);
> +	bool update = false;
> +	if (colourGains) {
> +		state.manual.gains.r() = (*colourGains)[0];
> +		state.manual.gains.b() = (*colourGains)[1];
> +		/*
> +		 * \todo Colour temperature reported in metadata is now
> +		 * incorrect, as we can't deduce the temperature from the gains.
> +		 * This will be fixed with the bayes AWB algorithm.

has it been fixed ? however I understand this is a copy of the
existing code..

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

> +		 */
> +		update = true;
> +	} else if (colourTemperature) {
> +		state.manual.temperatureK = *colourTemperature;
> +		const auto &gains = gainsFromColourTemperature(*colourTemperature);
> +		if (gains) {
> +			state.manual.gains.r() = gains->r();
> +			state.manual.gains.b() = gains->b();
> +			update = true;
> +		}
> +	}
> +
> +	if (update)
> +		LOG(Awb, Debug)
> +			<< "Set colour gains to " << state.manual.gains;
> +
> +	frameContext.gains = state.manual.gains;
> +	frameContext.temperatureK = state.manual.temperatureK;
> +}
> +
>  /**
>   * \fn AwbAlgorithm::calculateAwb()
>   * \brief Calculate AWB data from the given statistics
> diff --git a/src/ipa/libipa/awb.h b/src/ipa/libipa/awb.h
> index 4ceae537686f8f4c93686fab4b9efbc06e112b1d..0256ff8ca3429288c317d3ee940255c4a5391357 100644
> --- a/src/ipa/libipa/awb.h
> +++ b/src/ipa/libipa/awb.h
> @@ -68,6 +68,11 @@ public:
>
>  	int configure(awb::ActiveState &state, awb::Session &session);
>
> +	void queueRequest(awb::ActiveState &state,
> +			  [[maybe_unused]] const uint32_t frame,
> +			  awb::FrameContext &frameContext,
> +			  const ControlList &controls);
> +
>  	virtual AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) = 0;
>  	virtual std::optional<RGB<double>> gainsFromColourTemperature(double colourTemperature) = 0;
>
> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> index 86d5dfed3e1c2bb587705f05362229db3cdadafd..b91132fc6177650b9338867359583bf5429ea7e5 100644
> --- a/src/ipa/rkisp1/algorithms/awb.cpp
> +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> @@ -146,55 +146,12 @@ int Awb::configure(IPAContext &context,
>   * \copydoc libcamera::ipa::Algorithm::queueRequest
>   */
>  void Awb::queueRequest(IPAContext &context,
> -		       [[maybe_unused]] const uint32_t frame,
> +		       const uint32_t frame,
>  		       IPAFrameContext &frameContext,
>  		       const ControlList &controls)
>  {
> -	auto &awb = context.activeState.awb;
> -
> -	const auto &awbEnable = controls.get(controls::AwbEnable);
> -	if (awbEnable && *awbEnable != awb.autoEnabled) {
> -		awb.autoEnabled = *awbEnable;
> -
> -		LOG(RkISP1Awb, Debug)
> -			<< (*awbEnable ? "Enabling" : "Disabling") << " AWB";
> -	}
> -
> -	awbAlgo_->handleControls(controls);
> -
> -	frameContext.awb.autoEnabled = awb.autoEnabled;
> -
> -	if (awb.autoEnabled)
> -		return;
> -
> -	const auto &colourGains = controls.get(controls::ColourGains);
> -	const auto &colourTemperature = controls.get(controls::ColourTemperature);
> -	bool update = false;
> -	if (colourGains) {
> -		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.
> -		 * This will be fixed with the bayes AWB algorithm.
> -		 */
> -		update = true;
> -	} else if (colourTemperature) {
> -		awb.manual.temperatureK = *colourTemperature;
> -		const auto &gains = awbAlgo_->gainsFromColourTemperature(*colourTemperature);
> -		if (gains) {
> -			awb.manual.gains.r() = gains->r();
> -			awb.manual.gains.b() = gains->b();
> -			update = true;
> -		}
> -	}
> -
> -	if (update)
> -		LOG(RkISP1Awb, Debug)
> -			<< "Set colour gains to " << awb.manual.gains;
> -
> -	frameContext.awb.gains = awb.manual.gains;
> -	frameContext.awb.temperatureK = awb.manual.temperatureK;
> +	awbAlgo_->queueRequest(context.activeState.awb, frame, frameContext.awb,
> +			       controls);
>  }
>
>  /**
> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp
> index 230b7807075941f086911549066e39c0a04dff5c..d8516871562724fc65ffbb67161ff4fd988c4b1c 100644
> --- a/src/ipa/simple/algorithms/awb.cpp
> +++ b/src/ipa/simple/algorithms/awb.cpp
> @@ -114,55 +114,13 @@ int Awb::configure(IPAContext &context,
>   * \copydoc libcamera::ipa::Algorithm::queueRequest
>   */
>  void Awb::queueRequest(IPAContext &context,
> -		       [[maybe_unused]] const uint32_t frame,
> -		       [[maybe_unused]] IPAFrameContext &frameContext,
> +		       const uint32_t frame,
> +		       IPAFrameContext &frameContext,
>  		       const ControlList &controls)
>  {
> -	auto &awb = context.activeState.awb;
> -
> -	const auto &awbEnable = controls.get(controls::AwbEnable);
> -	if (awbEnable && *awbEnable != awb.autoEnabled) {
> -		awb.autoEnabled = *awbEnable;
> -
> -		LOG(IPASoftAwb, Debug)
> -			<< (*awbEnable ? "Enabling" : "Disabling") << " AWB";
> -	}
> -
> -	awbAlgo_->handleControls(controls);
> -
> -	frameContext.awb.autoEnabled = awb.autoEnabled;
> -
> -	if (awb.autoEnabled)
> -		return;
> -
> -	const auto &colourGains = controls.get(controls::ColourGains);
> -	const auto &colourTemperature = controls.get(controls::ColourTemperature);
> -	bool update = false;
> -	if (colourGains) {
> -		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.
> -		 * This will be fixed with the bayes AWB algorithm.
> -		 */
> -		update = true;
> -	} else if (colourTemperature) {
> -		awb.manual.temperatureK = *colourTemperature;
> -		const auto &gains = awbAlgo_->gainsFromColourTemperature(*colourTemperature);
> -		if (gains) {
> -			awb.manual.gains.r() = gains->r();
> -			awb.manual.gains.b() = gains->b();
> -			update = true;
> -		}
> -	}
> -
> -	if (update)
> -		LOG(IPASoftAwb, Debug)
> -			<< "Set colour gains to " << awb.manual.gains;
> -
> -	frameContext.awb.gains = awb.manual.gains;
> -	frameContext.awb.temperatureK = awb.manual.temperatureK;
> +	awbAlgo_->queueRequest(context.activeState.awb,
> +			       frame, frameContext.awb,
> +			       controls);
>  }
>
>  void Awb::prepare(IPAContext &context,
>
> --
> 2.53.0
>
Kieran Bingham April 8, 2026, 5:23 p.m. UTC | #2 
Quoting Jacopo Mondi (2026-04-08 10:02:18)
> Hi Kieran
> 
> On Tue, Apr 07, 2026 at 11:01:15PM +0100, Kieran Bingham wrote:
> > Move the now duplicated implementation for both soft IPA and rkisp1 IPA
> > for managing requests at queue time into the common libipa AWB implementation.
> 
> requests ?

I don't understand this question mark. Yes, this is queueReqeust which
parses requests ?

> 
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  src/ipa/libipa/awb.cpp            | 59 +++++++++++++++++++++++++++++++++++++++
> >  src/ipa/libipa/awb.h              |  5 ++++
> >  src/ipa/rkisp1/algorithms/awb.cpp | 49 ++------------------------------
> >  src/ipa/simple/algorithms/awb.cpp | 52 ++++------------------------------
> >  4 files changed, 72 insertions(+), 93 deletions(-)
> >
> > diff --git a/src/ipa/libipa/awb.cpp b/src/ipa/libipa/awb.cpp
> > index d0c577958caebc0b7e24fe58506da964df2200fe..cac6b4bd206faa4f32680e16c7522685cfd30cac 100644
> > --- a/src/ipa/libipa/awb.cpp
> > +++ b/src/ipa/libipa/awb.cpp
> > @@ -168,6 +168,65 @@ int AwbAlgorithm::configure(awb::ActiveState &state, awb::Session &session)
> >       return 0;
> >  }
> >
> > +/**
> > + * \brief Provide control values to the algorithm
> > + * \param[in] state The AWB specific active state shared across frames
> > + * \param[in] frame The frame number to apply the control values
> > + * \param[in] frameContext The current frame's AWB specific context
> > + * \param[in] controls The list of user controls
> > + */
> > +void AwbAlgorithm::queueRequest(awb::ActiveState &state,
> > +                             [[maybe_unused]] const uint32_t frame,
> 
> Should we aim at maintaining compatibility with the
> Algorithm::queuRequest() interface or unused parameters should be dropped ?

I didn't want to pass things like the bigger contexts, but the frame
index seemed important in my head so I've kept it for now.

I don't know why or what it's important for, but I anticipate it will be
important with more per-frame-control concepts or for knowing the
sequencing/timing/something ...

I guess they're easy enough to add later if needed ... I'd be keen to
know what Stefan thinks here as he's done a lot of rework around timings
lately

> 
> > +                             awb::FrameContext &frameContext,
> > +                             const ControlList &controls)
> > +{
> > +     const auto &awbEnable = controls.get(controls::AwbEnable);
> > +     if (awbEnable && *awbEnable != state.autoEnabled) {
> > +             state.autoEnabled = *awbEnable;
> > +
> > +             LOG(Awb, Debug)
> > +                     << (*awbEnable ? "Enabling" : "Disabling") << " AWB";
> > +     }
> > +
> > +     /* Handle controls from subclass algo (Grey or Bayes) */
> > +     handleControls(controls);
> > +
> > +     frameContext.autoEnabled = state.autoEnabled;
> > +
> > +     /* Todo: Check to see if we should always parse the following controls */
> 
> Does this still apply ?
> 
> > +     if (frameContext.autoEnabled)
> > +             return;
> > +
> > +     const auto &colourGains = controls.get(controls::ColourGains);
> > +     const auto &colourTemperature = controls.get(controls::ColourTemperature);
> > +     bool update = false;
> > +     if (colourGains) {
> > +             state.manual.gains.r() = (*colourGains)[0];
> > +             state.manual.gains.b() = (*colourGains)[1];
> > +             /*
> > +              * \todo Colour temperature reported in metadata is now
> > +              * incorrect, as we can't deduce the temperature from the gains.
> > +              * This will be fixed with the bayes AWB algorithm.
> 
> has it been fixed ? however I understand this is a copy of the
> existing code..

I don't know - indeed, I'm just trying to move existing code around to
reduce copying in each module. Still worth checking, but I'd still do it
on top and not in this patch anyway.

> 
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
> Thanks
>   j
> 
> > +              */
> > +             update = true;
> > +     } else if (colourTemperature) {
> > +             state.manual.temperatureK = *colourTemperature;
> > +             const auto &gains = gainsFromColourTemperature(*colourTemperature);
> > +             if (gains) {
> > +                     state.manual.gains.r() = gains->r();
> > +                     state.manual.gains.b() = gains->b();
> > +                     update = true;
> > +             }
> > +     }
> > +
> > +     if (update)
> > +             LOG(Awb, Debug)
> > +                     << "Set colour gains to " << state.manual.gains;
> > +
> > +     frameContext.gains = state.manual.gains;
> > +     frameContext.temperatureK = state.manual.temperatureK;
> > +}
> > +
> >  /**
> >   * \fn AwbAlgorithm::calculateAwb()
> >   * \brief Calculate AWB data from the given statistics
> > diff --git a/src/ipa/libipa/awb.h b/src/ipa/libipa/awb.h
> > index 4ceae537686f8f4c93686fab4b9efbc06e112b1d..0256ff8ca3429288c317d3ee940255c4a5391357 100644
> > --- a/src/ipa/libipa/awb.h
> > +++ b/src/ipa/libipa/awb.h
> > @@ -68,6 +68,11 @@ public:
> >
> >       int configure(awb::ActiveState &state, awb::Session &session);
> >
> > +     void queueRequest(awb::ActiveState &state,
> > +                       [[maybe_unused]] const uint32_t frame,
> > +                       awb::FrameContext &frameContext,
> > +                       const ControlList &controls);
> > +
> >       virtual AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) = 0;
> >       virtual std::optional<RGB<double>> gainsFromColourTemperature(double colourTemperature) = 0;
> >
> > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> > index 86d5dfed3e1c2bb587705f05362229db3cdadafd..b91132fc6177650b9338867359583bf5429ea7e5 100644
> > --- a/src/ipa/rkisp1/algorithms/awb.cpp
> > +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> > @@ -146,55 +146,12 @@ int Awb::configure(IPAContext &context,
> >   * \copydoc libcamera::ipa::Algorithm::queueRequest
> >   */
> >  void Awb::queueRequest(IPAContext &context,
> > -                    [[maybe_unused]] const uint32_t frame,
> > +                    const uint32_t frame,
> >                      IPAFrameContext &frameContext,
> >                      const ControlList &controls)
> >  {
> > -     auto &awb = context.activeState.awb;
> > -
> > -     const auto &awbEnable = controls.get(controls::AwbEnable);
> > -     if (awbEnable && *awbEnable != awb.autoEnabled) {
> > -             awb.autoEnabled = *awbEnable;
> > -
> > -             LOG(RkISP1Awb, Debug)
> > -                     << (*awbEnable ? "Enabling" : "Disabling") << " AWB";
> > -     }
> > -
> > -     awbAlgo_->handleControls(controls);
> > -
> > -     frameContext.awb.autoEnabled = awb.autoEnabled;
> > -
> > -     if (awb.autoEnabled)
> > -             return;
> > -
> > -     const auto &colourGains = controls.get(controls::ColourGains);
> > -     const auto &colourTemperature = controls.get(controls::ColourTemperature);
> > -     bool update = false;
> > -     if (colourGains) {
> > -             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.
> > -              * This will be fixed with the bayes AWB algorithm.
> > -              */
> > -             update = true;
> > -     } else if (colourTemperature) {
> > -             awb.manual.temperatureK = *colourTemperature;
> > -             const auto &gains = awbAlgo_->gainsFromColourTemperature(*colourTemperature);
> > -             if (gains) {
> > -                     awb.manual.gains.r() = gains->r();
> > -                     awb.manual.gains.b() = gains->b();
> > -                     update = true;
> > -             }
> > -     }
> > -
> > -     if (update)
> > -             LOG(RkISP1Awb, Debug)
> > -                     << "Set colour gains to " << awb.manual.gains;
> > -
> > -     frameContext.awb.gains = awb.manual.gains;
> > -     frameContext.awb.temperatureK = awb.manual.temperatureK;
> > +     awbAlgo_->queueRequest(context.activeState.awb, frame, frameContext.awb,
> > +                            controls);
> >  }
> >
> >  /**
> > diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp
> > index 230b7807075941f086911549066e39c0a04dff5c..d8516871562724fc65ffbb67161ff4fd988c4b1c 100644
> > --- a/src/ipa/simple/algorithms/awb.cpp
> > +++ b/src/ipa/simple/algorithms/awb.cpp
> > @@ -114,55 +114,13 @@ int Awb::configure(IPAContext &context,
> >   * \copydoc libcamera::ipa::Algorithm::queueRequest
> >   */
> >  void Awb::queueRequest(IPAContext &context,
> > -                    [[maybe_unused]] const uint32_t frame,
> > -                    [[maybe_unused]] IPAFrameContext &frameContext,
> > +                    const uint32_t frame,
> > +                    IPAFrameContext &frameContext,
> >                      const ControlList &controls)
> >  {
> > -     auto &awb = context.activeState.awb;
> > -
> > -     const auto &awbEnable = controls.get(controls::AwbEnable);
> > -     if (awbEnable && *awbEnable != awb.autoEnabled) {
> > -             awb.autoEnabled = *awbEnable;
> > -
> > -             LOG(IPASoftAwb, Debug)
> > -                     << (*awbEnable ? "Enabling" : "Disabling") << " AWB";
> > -     }
> > -
> > -     awbAlgo_->handleControls(controls);
> > -
> > -     frameContext.awb.autoEnabled = awb.autoEnabled;
> > -
> > -     if (awb.autoEnabled)
> > -             return;
> > -
> > -     const auto &colourGains = controls.get(controls::ColourGains);
> > -     const auto &colourTemperature = controls.get(controls::ColourTemperature);
> > -     bool update = false;
> > -     if (colourGains) {
> > -             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.
> > -              * This will be fixed with the bayes AWB algorithm.
> > -              */
> > -             update = true;
> > -     } else if (colourTemperature) {
> > -             awb.manual.temperatureK = *colourTemperature;
> > -             const auto &gains = awbAlgo_->gainsFromColourTemperature(*colourTemperature);
> > -             if (gains) {
> > -                     awb.manual.gains.r() = gains->r();
> > -                     awb.manual.gains.b() = gains->b();
> > -                     update = true;
> > -             }
> > -     }
> > -
> > -     if (update)
> > -             LOG(IPASoftAwb, Debug)
> > -                     << "Set colour gains to " << awb.manual.gains;
> > -
> > -     frameContext.awb.gains = awb.manual.gains;
> > -     frameContext.awb.temperatureK = awb.manual.temperatureK;
> > +     awbAlgo_->queueRequest(context.activeState.awb,
> > +                            frame, frameContext.awb,
> > +                            controls);
> >  }
> >
> >  void Awb::prepare(IPAContext &context,
> >
> > --
> > 2.53.0
> >
Jacopo Mondi April 9, 2026, 7:23 a.m. UTC | #3 
Hi Kieran

On Wed, Apr 08, 2026 at 06:23:50PM +0100, Kieran Bingham wrote:
> Quoting Jacopo Mondi (2026-04-08 10:02:18)
> > Hi Kieran
> >
> > On Tue, Apr 07, 2026 at 11:01:15PM +0100, Kieran Bingham wrote:
> > > Move the now duplicated implementation for both soft IPA and rkisp1 IPA
> > > for managing requests at queue time into the common libipa AWB implementation.
> >
> > requests ?
>
> I don't understand this question mark. Yes, this is queueReqeust which
> parses requests ?

I'm not sure either :) Maybe I mis-read "requests" in the commit message

>
> >
> > >
> > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > ---
> > >  src/ipa/libipa/awb.cpp            | 59 +++++++++++++++++++++++++++++++++++++++
> > >  src/ipa/libipa/awb.h              |  5 ++++
> > >  src/ipa/rkisp1/algorithms/awb.cpp | 49 ++------------------------------
> > >  src/ipa/simple/algorithms/awb.cpp | 52 ++++------------------------------
> > >  4 files changed, 72 insertions(+), 93 deletions(-)
> > >
> > > diff --git a/src/ipa/libipa/awb.cpp b/src/ipa/libipa/awb.cpp
> > > index d0c577958caebc0b7e24fe58506da964df2200fe..cac6b4bd206faa4f32680e16c7522685cfd30cac 100644
> > > --- a/src/ipa/libipa/awb.cpp
> > > +++ b/src/ipa/libipa/awb.cpp
> > > @@ -168,6 +168,65 @@ int AwbAlgorithm::configure(awb::ActiveState &state, awb::Session &session)
> > >       return 0;
> > >  }
> > >
> > > +/**
> > > + * \brief Provide control values to the algorithm
> > > + * \param[in] state The AWB specific active state shared across frames
> > > + * \param[in] frame The frame number to apply the control values
> > > + * \param[in] frameContext The current frame's AWB specific context
> > > + * \param[in] controls The list of user controls
> > > + */
> > > +void AwbAlgorithm::queueRequest(awb::ActiveState &state,
> > > +                             [[maybe_unused]] const uint32_t frame,
> >
> > Should we aim at maintaining compatibility with the
> > Algorithm::queuRequest() interface or unused parameters should be dropped ?
>
> I didn't want to pass things like the bigger contexts, but the frame
> index seemed important in my head so I've kept it for now.
>
> I don't know why or what it's important for, but I anticipate it will be
> important with more per-frame-control concepts or for knowing the
> sequencing/timing/something ...
>
> I guess they're easy enough to add later if needed ... I'd be keen to
> know what Stefan thinks here as he's done a lot of rework around timings
> lately

Both options work for me

>
> >
> > > +                             awb::FrameContext &frameContext,
> > > +                             const ControlList &controls)
> > > +{
> > > +     const auto &awbEnable = controls.get(controls::AwbEnable);
> > > +     if (awbEnable && *awbEnable != state.autoEnabled) {
> > > +             state.autoEnabled = *awbEnable;
> > > +
> > > +             LOG(Awb, Debug)
> > > +                     << (*awbEnable ? "Enabling" : "Disabling") << " AWB";
> > > +     }
> > > +
> > > +     /* Handle controls from subclass algo (Grey or Bayes) */
> > > +     handleControls(controls);
> > > +
> > > +     frameContext.autoEnabled = state.autoEnabled;
> > > +
> > > +     /* Todo: Check to see if we should always parse the following controls */
> >
> > Does this still apply ?
> >
> > > +     if (frameContext.autoEnabled)
> > > +             return;
> > > +
> > > +     const auto &colourGains = controls.get(controls::ColourGains);
> > > +     const auto &colourTemperature = controls.get(controls::ColourTemperature);
> > > +     bool update = false;
> > > +     if (colourGains) {
> > > +             state.manual.gains.r() = (*colourGains)[0];
> > > +             state.manual.gains.b() = (*colourGains)[1];
> > > +             /*
> > > +              * \todo Colour temperature reported in metadata is now
> > > +              * incorrect, as we can't deduce the temperature from the gains.
> > > +              * This will be fixed with the bayes AWB algorithm.
> >
> > has it been fixed ? however I understand this is a copy of the
> > existing code..
>
> I don't know - indeed, I'm just trying to move existing code around to
> reduce copying in each module. Still worth checking, but I'd still do it
> on top and not in this patch anyway.
>
> >
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> >
> > Thanks
> >   j
> >
> > > +              */
> > > +             update = true;
> > > +     } else if (colourTemperature) {
> > > +             state.manual.temperatureK = *colourTemperature;
> > > +             const auto &gains = gainsFromColourTemperature(*colourTemperature);
> > > +             if (gains) {
> > > +                     state.manual.gains.r() = gains->r();
> > > +                     state.manual.gains.b() = gains->b();
> > > +                     update = true;
> > > +             }
> > > +     }
> > > +
> > > +     if (update)
> > > +             LOG(Awb, Debug)
> > > +                     << "Set colour gains to " << state.manual.gains;
> > > +
> > > +     frameContext.gains = state.manual.gains;
> > > +     frameContext.temperatureK = state.manual.temperatureK;
> > > +}
> > > +
> > >  /**
> > >   * \fn AwbAlgorithm::calculateAwb()
> > >   * \brief Calculate AWB data from the given statistics
> > > diff --git a/src/ipa/libipa/awb.h b/src/ipa/libipa/awb.h
> > > index 4ceae537686f8f4c93686fab4b9efbc06e112b1d..0256ff8ca3429288c317d3ee940255c4a5391357 100644
> > > --- a/src/ipa/libipa/awb.h
> > > +++ b/src/ipa/libipa/awb.h
> > > @@ -68,6 +68,11 @@ public:
> > >
> > >       int configure(awb::ActiveState &state, awb::Session &session);
> > >
> > > +     void queueRequest(awb::ActiveState &state,
> > > +                       [[maybe_unused]] const uint32_t frame,
> > > +                       awb::FrameContext &frameContext,
> > > +                       const ControlList &controls);
> > > +
> > >       virtual AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) = 0;
> > >       virtual std::optional<RGB<double>> gainsFromColourTemperature(double colourTemperature) = 0;
> > >
> > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> > > index 86d5dfed3e1c2bb587705f05362229db3cdadafd..b91132fc6177650b9338867359583bf5429ea7e5 100644
> > > --- a/src/ipa/rkisp1/algorithms/awb.cpp
> > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> > > @@ -146,55 +146,12 @@ int Awb::configure(IPAContext &context,
> > >   * \copydoc libcamera::ipa::Algorithm::queueRequest
> > >   */
> > >  void Awb::queueRequest(IPAContext &context,
> > > -                    [[maybe_unused]] const uint32_t frame,
> > > +                    const uint32_t frame,
> > >                      IPAFrameContext &frameContext,
> > >                      const ControlList &controls)
> > >  {
> > > -     auto &awb = context.activeState.awb;
> > > -
> > > -     const auto &awbEnable = controls.get(controls::AwbEnable);
> > > -     if (awbEnable && *awbEnable != awb.autoEnabled) {
> > > -             awb.autoEnabled = *awbEnable;
> > > -
> > > -             LOG(RkISP1Awb, Debug)
> > > -                     << (*awbEnable ? "Enabling" : "Disabling") << " AWB";
> > > -     }
> > > -
> > > -     awbAlgo_->handleControls(controls);
> > > -
> > > -     frameContext.awb.autoEnabled = awb.autoEnabled;
> > > -
> > > -     if (awb.autoEnabled)
> > > -             return;
> > > -
> > > -     const auto &colourGains = controls.get(controls::ColourGains);
> > > -     const auto &colourTemperature = controls.get(controls::ColourTemperature);
> > > -     bool update = false;
> > > -     if (colourGains) {
> > > -             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.
> > > -              * This will be fixed with the bayes AWB algorithm.
> > > -              */
> > > -             update = true;
> > > -     } else if (colourTemperature) {
> > > -             awb.manual.temperatureK = *colourTemperature;
> > > -             const auto &gains = awbAlgo_->gainsFromColourTemperature(*colourTemperature);
> > > -             if (gains) {
> > > -                     awb.manual.gains.r() = gains->r();
> > > -                     awb.manual.gains.b() = gains->b();
> > > -                     update = true;
> > > -             }
> > > -     }
> > > -
> > > -     if (update)
> > > -             LOG(RkISP1Awb, Debug)
> > > -                     << "Set colour gains to " << awb.manual.gains;
> > > -
> > > -     frameContext.awb.gains = awb.manual.gains;
> > > -     frameContext.awb.temperatureK = awb.manual.temperatureK;
> > > +     awbAlgo_->queueRequest(context.activeState.awb, frame, frameContext.awb,
> > > +                            controls);
> > >  }
> > >
> > >  /**
> > > diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp
> > > index 230b7807075941f086911549066e39c0a04dff5c..d8516871562724fc65ffbb67161ff4fd988c4b1c 100644
> > > --- a/src/ipa/simple/algorithms/awb.cpp
> > > +++ b/src/ipa/simple/algorithms/awb.cpp
> > > @@ -114,55 +114,13 @@ int Awb::configure(IPAContext &context,
> > >   * \copydoc libcamera::ipa::Algorithm::queueRequest
> > >   */
> > >  void Awb::queueRequest(IPAContext &context,
> > > -                    [[maybe_unused]] const uint32_t frame,
> > > -                    [[maybe_unused]] IPAFrameContext &frameContext,
> > > +                    const uint32_t frame,
> > > +                    IPAFrameContext &frameContext,
> > >                      const ControlList &controls)
> > >  {
> > > -     auto &awb = context.activeState.awb;
> > > -
> > > -     const auto &awbEnable = controls.get(controls::AwbEnable);
> > > -     if (awbEnable && *awbEnable != awb.autoEnabled) {
> > > -             awb.autoEnabled = *awbEnable;
> > > -
> > > -             LOG(IPASoftAwb, Debug)
> > > -                     << (*awbEnable ? "Enabling" : "Disabling") << " AWB";
> > > -     }
> > > -
> > > -     awbAlgo_->handleControls(controls);
> > > -
> > > -     frameContext.awb.autoEnabled = awb.autoEnabled;
> > > -
> > > -     if (awb.autoEnabled)
> > > -             return;
> > > -
> > > -     const auto &colourGains = controls.get(controls::ColourGains);
> > > -     const auto &colourTemperature = controls.get(controls::ColourTemperature);
> > > -     bool update = false;
> > > -     if (colourGains) {
> > > -             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.
> > > -              * This will be fixed with the bayes AWB algorithm.
> > > -              */
> > > -             update = true;
> > > -     } else if (colourTemperature) {
> > > -             awb.manual.temperatureK = *colourTemperature;
> > > -             const auto &gains = awbAlgo_->gainsFromColourTemperature(*colourTemperature);
> > > -             if (gains) {
> > > -                     awb.manual.gains.r() = gains->r();
> > > -                     awb.manual.gains.b() = gains->b();
> > > -                     update = true;
> > > -             }
> > > -     }
> > > -
> > > -     if (update)
> > > -             LOG(IPASoftAwb, Debug)
> > > -                     << "Set colour gains to " << awb.manual.gains;
> > > -
> > > -     frameContext.awb.gains = awb.manual.gains;
> > > -     frameContext.awb.temperatureK = awb.manual.temperatureK;
> > > +     awbAlgo_->queueRequest(context.activeState.awb,
> > > +                            frame, frameContext.awb,
> > > +                            controls);
> > >  }
> > >
> > >  void Awb::prepare(IPAContext &context,
> > >
> > > --
> > > 2.53.0
> > >

Patch
diff mbox series 

diff --git a/src/ipa/libipa/awb.cpp b/src/ipa/libipa/awb.cpp
index d0c577958caebc0b7e24fe58506da964df2200fe..cac6b4bd206faa4f32680e16c7522685cfd30cac 100644
--- a/src/ipa/libipa/awb.cpp
+++ b/src/ipa/libipa/awb.cpp
@@ -168,6 +168,65 @@  int AwbAlgorithm::configure(awb::ActiveState &state, awb::Session &session)
 	return 0;
 }
 
+/**
+ * \brief Provide control values to the algorithm
+ * \param[in] state The AWB specific active state shared across frames
+ * \param[in] frame The frame number to apply the control values
+ * \param[in] frameContext The current frame's AWB specific context
+ * \param[in] controls The list of user controls
+ */
+void AwbAlgorithm::queueRequest(awb::ActiveState &state,
+				[[maybe_unused]] const uint32_t frame,
+				awb::FrameContext &frameContext,
+				const ControlList &controls)
+{
+	const auto &awbEnable = controls.get(controls::AwbEnable);
+	if (awbEnable && *awbEnable != state.autoEnabled) {
+		state.autoEnabled = *awbEnable;
+
+		LOG(Awb, Debug)
+			<< (*awbEnable ? "Enabling" : "Disabling") << " AWB";
+	}
+
+	/* Handle controls from subclass algo (Grey or Bayes) */
+	handleControls(controls);
+
+	frameContext.autoEnabled = state.autoEnabled;
+
+	/* Todo: Check to see if we should always parse the following controls */
+	if (frameContext.autoEnabled)
+		return;
+
+	const auto &colourGains = controls.get(controls::ColourGains);
+	const auto &colourTemperature = controls.get(controls::ColourTemperature);
+	bool update = false;
+	if (colourGains) {
+		state.manual.gains.r() = (*colourGains)[0];
+		state.manual.gains.b() = (*colourGains)[1];
+		/*
+		 * \todo Colour temperature reported in metadata is now
+		 * incorrect, as we can't deduce the temperature from the gains.
+		 * This will be fixed with the bayes AWB algorithm.
+		 */
+		update = true;
+	} else if (colourTemperature) {
+		state.manual.temperatureK = *colourTemperature;
+		const auto &gains = gainsFromColourTemperature(*colourTemperature);
+		if (gains) {
+			state.manual.gains.r() = gains->r();
+			state.manual.gains.b() = gains->b();
+			update = true;
+		}
+	}
+
+	if (update)
+		LOG(Awb, Debug)
+			<< "Set colour gains to " << state.manual.gains;
+
+	frameContext.gains = state.manual.gains;
+	frameContext.temperatureK = state.manual.temperatureK;
+}
+
 /**
  * \fn AwbAlgorithm::calculateAwb()
  * \brief Calculate AWB data from the given statistics
diff --git a/src/ipa/libipa/awb.h b/src/ipa/libipa/awb.h
index 4ceae537686f8f4c93686fab4b9efbc06e112b1d..0256ff8ca3429288c317d3ee940255c4a5391357 100644
--- a/src/ipa/libipa/awb.h
+++ b/src/ipa/libipa/awb.h
@@ -68,6 +68,11 @@  public:
 
 	int configure(awb::ActiveState &state, awb::Session &session);
 
+	void queueRequest(awb::ActiveState &state,
+			  [[maybe_unused]] const uint32_t frame,
+			  awb::FrameContext &frameContext,
+			  const ControlList &controls);
+
 	virtual AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) = 0;
 	virtual std::optional<RGB<double>> gainsFromColourTemperature(double colourTemperature) = 0;
 
diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
index 86d5dfed3e1c2bb587705f05362229db3cdadafd..b91132fc6177650b9338867359583bf5429ea7e5 100644
--- a/src/ipa/rkisp1/algorithms/awb.cpp
+++ b/src/ipa/rkisp1/algorithms/awb.cpp
@@ -146,55 +146,12 @@  int Awb::configure(IPAContext &context,
  * \copydoc libcamera::ipa::Algorithm::queueRequest
  */
 void Awb::queueRequest(IPAContext &context,
-		       [[maybe_unused]] const uint32_t frame,
+		       const uint32_t frame,
 		       IPAFrameContext &frameContext,
 		       const ControlList &controls)
 {
-	auto &awb = context.activeState.awb;
-
-	const auto &awbEnable = controls.get(controls::AwbEnable);
-	if (awbEnable && *awbEnable != awb.autoEnabled) {
-		awb.autoEnabled = *awbEnable;
-
-		LOG(RkISP1Awb, Debug)
-			<< (*awbEnable ? "Enabling" : "Disabling") << " AWB";
-	}
-
-	awbAlgo_->handleControls(controls);
-
-	frameContext.awb.autoEnabled = awb.autoEnabled;
-
-	if (awb.autoEnabled)
-		return;
-
-	const auto &colourGains = controls.get(controls::ColourGains);
-	const auto &colourTemperature = controls.get(controls::ColourTemperature);
-	bool update = false;
-	if (colourGains) {
-		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.
-		 * This will be fixed with the bayes AWB algorithm.
-		 */
-		update = true;
-	} else if (colourTemperature) {
-		awb.manual.temperatureK = *colourTemperature;
-		const auto &gains = awbAlgo_->gainsFromColourTemperature(*colourTemperature);
-		if (gains) {
-			awb.manual.gains.r() = gains->r();
-			awb.manual.gains.b() = gains->b();
-			update = true;
-		}
-	}
-
-	if (update)
-		LOG(RkISP1Awb, Debug)
-			<< "Set colour gains to " << awb.manual.gains;
-
-	frameContext.awb.gains = awb.manual.gains;
-	frameContext.awb.temperatureK = awb.manual.temperatureK;
+	awbAlgo_->queueRequest(context.activeState.awb, frame, frameContext.awb,
+			       controls);
 }
 
 /**
diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp
index 230b7807075941f086911549066e39c0a04dff5c..d8516871562724fc65ffbb67161ff4fd988c4b1c 100644
--- a/src/ipa/simple/algorithms/awb.cpp
+++ b/src/ipa/simple/algorithms/awb.cpp
@@ -114,55 +114,13 @@  int Awb::configure(IPAContext &context,
  * \copydoc libcamera::ipa::Algorithm::queueRequest
  */
 void Awb::queueRequest(IPAContext &context,
-		       [[maybe_unused]] const uint32_t frame,
-		       [[maybe_unused]] IPAFrameContext &frameContext,
+		       const uint32_t frame,
+		       IPAFrameContext &frameContext,
 		       const ControlList &controls)
 {
-	auto &awb = context.activeState.awb;
-
-	const auto &awbEnable = controls.get(controls::AwbEnable);
-	if (awbEnable && *awbEnable != awb.autoEnabled) {
-		awb.autoEnabled = *awbEnable;
-
-		LOG(IPASoftAwb, Debug)
-			<< (*awbEnable ? "Enabling" : "Disabling") << " AWB";
-	}
-
-	awbAlgo_->handleControls(controls);
-
-	frameContext.awb.autoEnabled = awb.autoEnabled;
-
-	if (awb.autoEnabled)
-		return;
-
-	const auto &colourGains = controls.get(controls::ColourGains);
-	const auto &colourTemperature = controls.get(controls::ColourTemperature);
-	bool update = false;
-	if (colourGains) {
-		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.
-		 * This will be fixed with the bayes AWB algorithm.
-		 */
-		update = true;
-	} else if (colourTemperature) {
-		awb.manual.temperatureK = *colourTemperature;
-		const auto &gains = awbAlgo_->gainsFromColourTemperature(*colourTemperature);
-		if (gains) {
-			awb.manual.gains.r() = gains->r();
-			awb.manual.gains.b() = gains->b();
-			update = true;
-		}
-	}
-
-	if (update)
-		LOG(IPASoftAwb, Debug)
-			<< "Set colour gains to " << awb.manual.gains;
-
-	frameContext.awb.gains = awb.manual.gains;
-	frameContext.awb.temperatureK = awb.manual.temperatureK;
+	awbAlgo_->queueRequest(context.activeState.awb,
+			       frame, frameContext.awb,
+			       controls);
 }
 
 void Awb::prepare(IPAContext &context,