[v3,11/16] ipa: rkisp1: ccm/lsc: Fix CCM/LSC based on manual color temperature
diff mbox series

Message ID 20250403154925.382973-12-stefan.klug@ideasonboard.com
State Accepted
Headers show
Series
  • Some rkisp1 awb improvements
Related show

Commit Message

Stefan Klug April 3, 2025, 3:49 p.m. UTC 
In RkISP1Awb::process(), the color temperature in the active state is
updated every time new statistics are available.  The CCM/LSC algorithms
use that value in prepare() to update the CCM/LSC. This is not correct
if the color temperature was specified manually and leads to visible
flicker even when AwbEnable is set to false.

To fix that, track the auto and manual color temperature separately in
active state. In Awb::prepare() the current frame context is updated
with the corresponding value from active state. Change the algorithms to
fetch the color temperature from the frame context instead of the active
state in prepare().

Fixes: 02308809548d ("ipa: rkisp1: awb: Implement ColourTemperature control")
Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>

---

Changes in v2:
- None

Changes in v3:
- Move incorrect colour temperature metadata to separate fixup patch
- Updated documentation
---
 src/ipa/rkisp1/algorithms/awb.cpp | 16 +++++++++-------
 src/ipa/rkisp1/algorithms/ccm.cpp |  2 +-
 src/ipa/rkisp1/algorithms/lsc.cpp |  6 +++---
 src/ipa/rkisp1/ipa_context.cpp    |  6 +++---
 src/ipa/rkisp1/ipa_context.h      |  2 +-
 5 files changed, 17 insertions(+), 15 deletions(-)

Comments

Kieran Bingham May 2, 2025, 8:10 a.m. UTC | #1 
Quoting Stefan Klug (2025-04-03 16:49:16)
> In RkISP1Awb::process(), the color temperature in the active state is
> updated every time new statistics are available.  The CCM/LSC algorithms
> use that value in prepare() to update the CCM/LSC. This is not correct
> if the color temperature was specified manually and leads to visible
> flicker even when AwbEnable is set to false.
> 
> To fix that, track the auto and manual color temperature separately in
> active state. In Awb::prepare() the current frame context is updated
> with the corresponding value from active state. Change the algorithms to
> fetch the color temperature from the frame context instead of the active
> state in prepare().
> 
> Fixes: 02308809548d ("ipa: rkisp1: awb: Implement ColourTemperature control")
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> 
> ---
> 
> Changes in v2:
> - None
> 
> Changes in v3:
> - Move incorrect colour temperature metadata to separate fixup patch

That answers my question on the previous patch ;-)

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> - Updated documentation
> ---
>  src/ipa/rkisp1/algorithms/awb.cpp | 16 +++++++++-------
>  src/ipa/rkisp1/algorithms/ccm.cpp |  2 +-
>  src/ipa/rkisp1/algorithms/lsc.cpp |  6 +++---
>  src/ipa/rkisp1/ipa_context.cpp    |  6 +++---
>  src/ipa/rkisp1/ipa_context.h      |  2 +-
>  5 files changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> index 79c4c658406d..0795b8e5b1e1 100644
> --- a/src/ipa/rkisp1/algorithms/awb.cpp
> +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> @@ -128,7 +128,8 @@ int Awb::configure(IPAContext &context,
>         context.activeState.awb.automatic.gains =
>                 awbAlgo_->gainsFromColourTemperature(kDefaultColourTemperature);
>         context.activeState.awb.autoEnabled = true;
> -       context.activeState.awb.temperatureK = kDefaultColourTemperature;
> +       context.activeState.awb.manual.temperatureK = kDefaultColourTemperature;
> +       context.activeState.awb.automatic.temperatureK = kDefaultColourTemperature;
>  
>         /*
>          * Define the measurement window for AWB as a centered rectangle
> @@ -185,7 +186,7 @@ void Awb::queueRequest(IPAContext &context,
>                 const auto &gains = awbAlgo_->gainsFromColourTemperature(*colourTemperature);
>                 awb.manual.gains.r() = gains.r();
>                 awb.manual.gains.b() = gains.b();
> -               awb.temperatureK = *colourTemperature;
> +               awb.manual.temperatureK = *colourTemperature;
>                 update = true;
>         }
>  
> @@ -194,7 +195,7 @@ void Awb::queueRequest(IPAContext &context,
>                         << "Set colour gains to " << awb.manual.gains;
>  
>         frameContext.awb.gains = awb.manual.gains;
> -       frameContext.awb.temperatureK = awb.temperatureK;
> +       frameContext.awb.temperatureK = awb.manual.temperatureK;
>  }
>  
>  /**
> @@ -208,8 +209,9 @@ 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.automatic.gains;
> -               frameContext.awb.temperatureK = context.activeState.awb.temperatureK;
> +               auto &awb = context.activeState.awb;
> +               frameContext.awb.gains = awb.automatic.gains;
> +               frameContext.awb.temperatureK = awb.automatic.temperatureK;
>         }
>  
>         auto gainConfig = params->block<BlockType::AwbGain>();
> @@ -309,7 +311,7 @@ void Awb::process(IPAContext &context,
>         RkISP1AwbStats awbStats{ rgbMeans };
>         AwbResult awbResult = awbAlgo_->calculateAwb(awbStats, frameContext.lux.lux);
>  
> -       activeState.awb.temperatureK = awbResult.colourTemperature;
> +       activeState.awb.automatic.temperatureK = awbResult.colourTemperature;
>  
>         /*
>          * Clamp the gain values to the hardware, which expresses gains as Q2.8
> @@ -330,7 +332,7 @@ void Awb::process(IPAContext &context,
>                 << std::showpoint
>                 << "Means " << rgbMeans << ", gains "
>                 << activeState.awb.automatic.gains << ", temp "
> -               << activeState.awb.temperatureK << "K";
> +               << activeState.awb.automatic.temperatureK << "K";
>  }
>  
>  RGB<double> Awb::calculateRgbMeans(const IPAFrameContext &frameContext, const rkisp1_cif_isp_awb_stat *awb) const
> diff --git a/src/ipa/rkisp1/algorithms/ccm.cpp b/src/ipa/rkisp1/algorithms/ccm.cpp
> index eb8ca39e56a8..2e5e91006b55 100644
> --- a/src/ipa/rkisp1/algorithms/ccm.cpp
> +++ b/src/ipa/rkisp1/algorithms/ccm.cpp
> @@ -88,7 +88,7 @@ void Ccm::setParameters(struct rkisp1_cif_isp_ctk_config &config,
>  void Ccm::prepare(IPAContext &context, const uint32_t frame,
>                   IPAFrameContext &frameContext, RkISP1Params *params)
>  {
> -       uint32_t ct = context.activeState.awb.temperatureK;
> +       uint32_t ct = frameContext.awb.temperatureK;
>  
>         /*
>          * \todo The colour temperature will likely be noisy, add filtering to
> diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp
> index e47aa2f0727e..e7301bfec863 100644
> --- a/src/ipa/rkisp1/algorithms/lsc.cpp
> +++ b/src/ipa/rkisp1/algorithms/lsc.cpp
> @@ -404,12 +404,12 @@ void LensShadingCorrection::copyTable(rkisp1_cif_isp_lsc_config &config,
>  /**
>   * \copydoc libcamera::ipa::Algorithm::prepare
>   */
> -void LensShadingCorrection::prepare(IPAContext &context,
> +void LensShadingCorrection::prepare([[maybe_unused]] IPAContext &context,
>                                     [[maybe_unused]] const uint32_t frame,
> -                                   [[maybe_unused]] IPAFrameContext &frameContext,
> +                                   IPAFrameContext &frameContext,
>                                     RkISP1Params *params)
>  {
> -       uint32_t ct = context.activeState.awb.temperatureK;
> +       uint32_t ct = frameContext.awb.temperatureK;
>         if (std::abs(static_cast<int>(ct) - static_cast<int>(lastAppliedCt_)) <
>             kColourTemperatureChangeThreshhold)
>                 return;
> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> index 39b97d143e95..7bc42e6de415 100644
> --- a/src/ipa/rkisp1/ipa_context.cpp
> +++ b/src/ipa/rkisp1/ipa_context.cpp
> @@ -197,15 +197,15 @@ namespace libcamera::ipa::rkisp1 {
>   * \struct IPAActiveState::awb::AwbState.gains
>   * \brief White balance gains
>   *
> + * \var IPAActiveState::awb::AwbState.temperatureK
> + * \brief Estimated color temperature
> + *
>   * \var IPAActiveState::awb.manual
>   * \brief Manual regulation state (set through requests)
>   *
>   * \var IPAActiveState::awb.automatic
>   * \brief Automatic regulation state (computed by the algorithm)
>   *
> - * \var IPAActiveState::awb.temperatureK
> - * \brief Estimated color temperature
> - *
>   * \var IPAActiveState::awb.autoEnabled
>   * \brief Whether the Auto White Balance algorithm is enabled
>   */
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index 6bc922a82971..769e9f114e23 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -91,12 +91,12 @@ struct IPAActiveState {
>         struct {
>                 struct AwbState {
>                         RGB<double> gains;
> +                       unsigned int temperatureK;
>                 };
>  
>                 AwbState manual;
>                 AwbState automatic;
>  
> -               unsigned int temperatureK;
>                 bool autoEnabled;
>         } awb;
>  
> -- 
> 2.43.0
>
Paul Elder May 7, 2025, 5 p.m. UTC | #2 
On Thu, Apr 03, 2025 at 05:49:16PM +0200, Stefan Klug wrote:
> In RkISP1Awb::process(), the color temperature in the active state is
> updated every time new statistics are available.  The CCM/LSC algorithms
> use that value in prepare() to update the CCM/LSC. This is not correct
> if the color temperature was specified manually and leads to visible
> flicker even when AwbEnable is set to false.
> 
> To fix that, track the auto and manual color temperature separately in
> active state. In Awb::prepare() the current frame context is updated
> with the corresponding value from active state. Change the algorithms to
> fetch the color temperature from the frame context instead of the active
> state in prepare().
> 
> Fixes: 02308809548d ("ipa: rkisp1: awb: Implement ColourTemperature control")
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> 
> ---
> 
> Changes in v2:
> - None
> 
> Changes in v3:
> - Move incorrect colour temperature metadata to separate fixup patch
> - Updated documentation
> ---
>  src/ipa/rkisp1/algorithms/awb.cpp | 16 +++++++++-------
>  src/ipa/rkisp1/algorithms/ccm.cpp |  2 +-
>  src/ipa/rkisp1/algorithms/lsc.cpp |  6 +++---
>  src/ipa/rkisp1/ipa_context.cpp    |  6 +++---
>  src/ipa/rkisp1/ipa_context.h      |  2 +-
>  5 files changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> index 79c4c658406d..0795b8e5b1e1 100644
> --- a/src/ipa/rkisp1/algorithms/awb.cpp
> +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> @@ -128,7 +128,8 @@ int Awb::configure(IPAContext &context,
>  	context.activeState.awb.automatic.gains =
>  		awbAlgo_->gainsFromColourTemperature(kDefaultColourTemperature);
>  	context.activeState.awb.autoEnabled = true;
> -	context.activeState.awb.temperatureK = kDefaultColourTemperature;
> +	context.activeState.awb.manual.temperatureK = kDefaultColourTemperature;
> +	context.activeState.awb.automatic.temperatureK = kDefaultColourTemperature;
>  
>  	/*
>  	 * Define the measurement window for AWB as a centered rectangle
> @@ -185,7 +186,7 @@ void Awb::queueRequest(IPAContext &context,
>  		const auto &gains = awbAlgo_->gainsFromColourTemperature(*colourTemperature);
>  		awb.manual.gains.r() = gains.r();
>  		awb.manual.gains.b() = gains.b();
> -		awb.temperatureK = *colourTemperature;
> +		awb.manual.temperatureK = *colourTemperature;
>  		update = true;
>  	}
>  
> @@ -194,7 +195,7 @@ void Awb::queueRequest(IPAContext &context,
>  			<< "Set colour gains to " << awb.manual.gains;
>  
>  	frameContext.awb.gains = awb.manual.gains;
> -	frameContext.awb.temperatureK = awb.temperatureK;
> +	frameContext.awb.temperatureK = awb.manual.temperatureK;
>  }
>  
>  /**
> @@ -208,8 +209,9 @@ 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.automatic.gains;
> -		frameContext.awb.temperatureK = context.activeState.awb.temperatureK;
> +		auto &awb = context.activeState.awb;
> +		frameContext.awb.gains = awb.automatic.gains;
> +		frameContext.awb.temperatureK = awb.automatic.temperatureK;
>  	}
>  
>  	auto gainConfig = params->block<BlockType::AwbGain>();
> @@ -309,7 +311,7 @@ void Awb::process(IPAContext &context,
>  	RkISP1AwbStats awbStats{ rgbMeans };
>  	AwbResult awbResult = awbAlgo_->calculateAwb(awbStats, frameContext.lux.lux);
>  
> -	activeState.awb.temperatureK = awbResult.colourTemperature;
> +	activeState.awb.automatic.temperatureK = awbResult.colourTemperature;
>  
>  	/*
>  	 * Clamp the gain values to the hardware, which expresses gains as Q2.8
> @@ -330,7 +332,7 @@ void Awb::process(IPAContext &context,
>  		<< std::showpoint
>  		<< "Means " << rgbMeans << ", gains "
>  		<< activeState.awb.automatic.gains << ", temp "
> -		<< activeState.awb.temperatureK << "K";
> +		<< activeState.awb.automatic.temperatureK << "K";
>  }
>  
>  RGB<double> Awb::calculateRgbMeans(const IPAFrameContext &frameContext, const rkisp1_cif_isp_awb_stat *awb) const
> diff --git a/src/ipa/rkisp1/algorithms/ccm.cpp b/src/ipa/rkisp1/algorithms/ccm.cpp
> index eb8ca39e56a8..2e5e91006b55 100644
> --- a/src/ipa/rkisp1/algorithms/ccm.cpp
> +++ b/src/ipa/rkisp1/algorithms/ccm.cpp
> @@ -88,7 +88,7 @@ void Ccm::setParameters(struct rkisp1_cif_isp_ctk_config &config,
>  void Ccm::prepare(IPAContext &context, const uint32_t frame,
>  		  IPAFrameContext &frameContext, RkISP1Params *params)
>  {
> -	uint32_t ct = context.activeState.awb.temperatureK;
> +	uint32_t ct = frameContext.awb.temperatureK;
>  
>  	/*
>  	 * \todo The colour temperature will likely be noisy, add filtering to
> diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp
> index e47aa2f0727e..e7301bfec863 100644
> --- a/src/ipa/rkisp1/algorithms/lsc.cpp
> +++ b/src/ipa/rkisp1/algorithms/lsc.cpp
> @@ -404,12 +404,12 @@ void LensShadingCorrection::copyTable(rkisp1_cif_isp_lsc_config &config,
>  /**
>   * \copydoc libcamera::ipa::Algorithm::prepare
>   */
> -void LensShadingCorrection::prepare(IPAContext &context,
> +void LensShadingCorrection::prepare([[maybe_unused]] IPAContext &context,
>  				    [[maybe_unused]] const uint32_t frame,
> -				    [[maybe_unused]] IPAFrameContext &frameContext,
> +				    IPAFrameContext &frameContext,
>  				    RkISP1Params *params)
>  {
> -	uint32_t ct = context.activeState.awb.temperatureK;
> +	uint32_t ct = frameContext.awb.temperatureK;
>  	if (std::abs(static_cast<int>(ct) - static_cast<int>(lastAppliedCt_)) <
>  	    kColourTemperatureChangeThreshhold)
>  		return;
> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> index 39b97d143e95..7bc42e6de415 100644
> --- a/src/ipa/rkisp1/ipa_context.cpp
> +++ b/src/ipa/rkisp1/ipa_context.cpp
> @@ -197,15 +197,15 @@ namespace libcamera::ipa::rkisp1 {
>   * \struct IPAActiveState::awb::AwbState.gains
>   * \brief White balance gains
>   *
> + * \var IPAActiveState::awb::AwbState.temperatureK
> + * \brief Estimated color temperature
> + *
>   * \var IPAActiveState::awb.manual
>   * \brief Manual regulation state (set through requests)
>   *
>   * \var IPAActiveState::awb.automatic
>   * \brief Automatic regulation state (computed by the algorithm)
>   *
> - * \var IPAActiveState::awb.temperatureK
> - * \brief Estimated color temperature
> - *
>   * \var IPAActiveState::awb.autoEnabled
>   * \brief Whether the Auto White Balance algorithm is enabled
>   */
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index 6bc922a82971..769e9f114e23 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -91,12 +91,12 @@ struct IPAActiveState {
>  	struct {
>  		struct AwbState {
>  			RGB<double> gains;
> +			unsigned int temperatureK;
>  		};
>  
>  		AwbState manual;
>  		AwbState automatic;
>  
> -		unsigned int temperatureK;
>  		bool autoEnabled;
>  	} awb;
>  
> -- 
> 2.43.0
>
Laurent Pinchart May 7, 2025, 8:39 p.m. UTC | #3 
Hi Stefan,

Thank you for the patch.

On Thu, Apr 03, 2025 at 05:49:16PM +0200, Stefan Klug wrote:
> In RkISP1Awb::process(), the color temperature in the active state is
> updated every time new statistics are available.  The CCM/LSC algorithms
> use that value in prepare() to update the CCM/LSC. This is not correct
> if the color temperature was specified manually and leads to visible
> flicker even when AwbEnable is set to false.
> 
> To fix that, track the auto and manual color temperature separately in
> active state. In Awb::prepare() the current frame context is updated
> with the corresponding value from active state. Change the algorithms to
> fetch the color temperature from the frame context instead of the active
> state in prepare().
> 
> Fixes: 02308809548d ("ipa: rkisp1: awb: Implement ColourTemperature control")
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> 
> ---
> 
> Changes in v2:
> - None
> 
> Changes in v3:
> - Move incorrect colour temperature metadata to separate fixup patch
> - Updated documentation
> ---
>  src/ipa/rkisp1/algorithms/awb.cpp | 16 +++++++++-------
>  src/ipa/rkisp1/algorithms/ccm.cpp |  2 +-
>  src/ipa/rkisp1/algorithms/lsc.cpp |  6 +++---
>  src/ipa/rkisp1/ipa_context.cpp    |  6 +++---
>  src/ipa/rkisp1/ipa_context.h      |  2 +-
>  5 files changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> index 79c4c658406d..0795b8e5b1e1 100644
> --- a/src/ipa/rkisp1/algorithms/awb.cpp
> +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> @@ -128,7 +128,8 @@ int Awb::configure(IPAContext &context,
>  	context.activeState.awb.automatic.gains =
>  		awbAlgo_->gainsFromColourTemperature(kDefaultColourTemperature);
>  	context.activeState.awb.autoEnabled = true;
> -	context.activeState.awb.temperatureK = kDefaultColourTemperature;
> +	context.activeState.awb.manual.temperatureK = kDefaultColourTemperature;
> +	context.activeState.awb.automatic.temperatureK = kDefaultColourTemperature;
>  
>  	/*
>  	 * Define the measurement window for AWB as a centered rectangle
> @@ -185,7 +186,7 @@ void Awb::queueRequest(IPAContext &context,
>  		const auto &gains = awbAlgo_->gainsFromColourTemperature(*colourTemperature);
>  		awb.manual.gains.r() = gains.r();
>  		awb.manual.gains.b() = gains.b();
> -		awb.temperatureK = *colourTemperature;
> +		awb.manual.temperatureK = *colourTemperature;
>  		update = true;
>  	}
>  
> @@ -194,7 +195,7 @@ void Awb::queueRequest(IPAContext &context,
>  			<< "Set colour gains to " << awb.manual.gains;
>  
>  	frameContext.awb.gains = awb.manual.gains;
> -	frameContext.awb.temperatureK = awb.temperatureK;
> +	frameContext.awb.temperatureK = awb.manual.temperatureK;
>  }
>  
>  /**
> @@ -208,8 +209,9 @@ 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.automatic.gains;
> -		frameContext.awb.temperatureK = context.activeState.awb.temperatureK;
> +		auto &awb = context.activeState.awb;

const

> +		frameContext.awb.gains = awb.automatic.gains;
> +		frameContext.awb.temperatureK = awb.automatic.temperatureK;
>  	}
>  
>  	auto gainConfig = params->block<BlockType::AwbGain>();
> @@ -309,7 +311,7 @@ void Awb::process(IPAContext &context,
>  	RkISP1AwbStats awbStats{ rgbMeans };
>  	AwbResult awbResult = awbAlgo_->calculateAwb(awbStats, frameContext.lux.lux);
>  
> -	activeState.awb.temperatureK = awbResult.colourTemperature;
> +	activeState.awb.automatic.temperatureK = awbResult.colourTemperature;
>  
>  	/*
>  	 * Clamp the gain values to the hardware, which expresses gains as Q2.8
> @@ -330,7 +332,7 @@ void Awb::process(IPAContext &context,
>  		<< std::showpoint
>  		<< "Means " << rgbMeans << ", gains "
>  		<< activeState.awb.automatic.gains << ", temp "
> -		<< activeState.awb.temperatureK << "K";
> +		<< activeState.awb.automatic.temperatureK << "K";
>  }
>  
>  RGB<double> Awb::calculateRgbMeans(const IPAFrameContext &frameContext, const rkisp1_cif_isp_awb_stat *awb) const
> diff --git a/src/ipa/rkisp1/algorithms/ccm.cpp b/src/ipa/rkisp1/algorithms/ccm.cpp
> index eb8ca39e56a8..2e5e91006b55 100644
> --- a/src/ipa/rkisp1/algorithms/ccm.cpp
> +++ b/src/ipa/rkisp1/algorithms/ccm.cpp
> @@ -88,7 +88,7 @@ void Ccm::setParameters(struct rkisp1_cif_isp_ctk_config &config,
>  void Ccm::prepare(IPAContext &context, const uint32_t frame,
>  		  IPAFrameContext &frameContext, RkISP1Params *params)
>  {
> -	uint32_t ct = context.activeState.awb.temperatureK;
> +	uint32_t ct = frameContext.awb.temperatureK;
>  
>  	/*
>  	 * \todo The colour temperature will likely be noisy, add filtering to
> diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp
> index e47aa2f0727e..e7301bfec863 100644
> --- a/src/ipa/rkisp1/algorithms/lsc.cpp
> +++ b/src/ipa/rkisp1/algorithms/lsc.cpp
> @@ -404,12 +404,12 @@ void LensShadingCorrection::copyTable(rkisp1_cif_isp_lsc_config &config,
>  /**
>   * \copydoc libcamera::ipa::Algorithm::prepare
>   */
> -void LensShadingCorrection::prepare(IPAContext &context,
> +void LensShadingCorrection::prepare([[maybe_unused]] IPAContext &context,
>  				    [[maybe_unused]] const uint32_t frame,
> -				    [[maybe_unused]] IPAFrameContext &frameContext,
> +				    IPAFrameContext &frameContext,
>  				    RkISP1Params *params)
>  {
> -	uint32_t ct = context.activeState.awb.temperatureK;
> +	uint32_t ct = frameContext.awb.temperatureK;
>  	if (std::abs(static_cast<int>(ct) - static_cast<int>(lastAppliedCt_)) <
>  	    kColourTemperatureChangeThreshhold)
>  		return;
> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> index 39b97d143e95..7bc42e6de415 100644
> --- a/src/ipa/rkisp1/ipa_context.cpp
> +++ b/src/ipa/rkisp1/ipa_context.cpp
> @@ -197,15 +197,15 @@ namespace libcamera::ipa::rkisp1 {
>   * \struct IPAActiveState::awb::AwbState.gains
>   * \brief White balance gains
>   *
> + * \var IPAActiveState::awb::AwbState.temperatureK
> + * \brief Estimated color temperature
> + *

Is that right ? automatic.temperatureK is indeed estimated by the AWB
algorithm, but manual.temperatureK is the value set through requests.
Being more precise would be nice, as the code is already confusing as-is
:-)

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>   * \var IPAActiveState::awb.manual
>   * \brief Manual regulation state (set through requests)
>   *
>   * \var IPAActiveState::awb.automatic
>   * \brief Automatic regulation state (computed by the algorithm)
>   *
> - * \var IPAActiveState::awb.temperatureK
> - * \brief Estimated color temperature
> - *
>   * \var IPAActiveState::awb.autoEnabled
>   * \brief Whether the Auto White Balance algorithm is enabled
>   */
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index 6bc922a82971..769e9f114e23 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -91,12 +91,12 @@ struct IPAActiveState {
>  	struct {
>  		struct AwbState {
>  			RGB<double> gains;
> +			unsigned int temperatureK;
>  		};
>  
>  		AwbState manual;
>  		AwbState automatic;
>  
> -		unsigned int temperatureK;
>  		bool autoEnabled;
>  	} awb;
>
Stefan Klug May 20, 2025, 9:19 a.m. UTC | #4 
Hi Laurent,

Thank you for the review.

Quoting Laurent Pinchart (2025-05-07 22:39:44)
> Hi Stefan,
> 
> Thank you for the patch.
> 
> On Thu, Apr 03, 2025 at 05:49:16PM +0200, Stefan Klug wrote:
> > In RkISP1Awb::process(), the color temperature in the active state is
> > updated every time new statistics are available.  The CCM/LSC algorithms
> > use that value in prepare() to update the CCM/LSC. This is not correct
> > if the color temperature was specified manually and leads to visible
> > flicker even when AwbEnable is set to false.
> > 
> > To fix that, track the auto and manual color temperature separately in
> > active state. In Awb::prepare() the current frame context is updated
> > with the corresponding value from active state. Change the algorithms to
> > fetch the color temperature from the frame context instead of the active
> > state in prepare().
> > 
> > Fixes: 02308809548d ("ipa: rkisp1: awb: Implement ColourTemperature control")
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > 
> > ---
> > 
> > Changes in v2:
> > - None
> > 
> > Changes in v3:
> > - Move incorrect colour temperature metadata to separate fixup patch
> > - Updated documentation
> > ---
> >  src/ipa/rkisp1/algorithms/awb.cpp | 16 +++++++++-------
> >  src/ipa/rkisp1/algorithms/ccm.cpp |  2 +-
> >  src/ipa/rkisp1/algorithms/lsc.cpp |  6 +++---
> >  src/ipa/rkisp1/ipa_context.cpp    |  6 +++---
> >  src/ipa/rkisp1/ipa_context.h      |  2 +-
> >  5 files changed, 17 insertions(+), 15 deletions(-)
> > 
> > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> > index 79c4c658406d..0795b8e5b1e1 100644
> > --- a/src/ipa/rkisp1/algorithms/awb.cpp
> > +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> > @@ -128,7 +128,8 @@ int Awb::configure(IPAContext &context,
> >       context.activeState.awb.automatic.gains =
> >               awbAlgo_->gainsFromColourTemperature(kDefaultColourTemperature);
> >       context.activeState.awb.autoEnabled = true;
> > -     context.activeState.awb.temperatureK = kDefaultColourTemperature;
> > +     context.activeState.awb.manual.temperatureK = kDefaultColourTemperature;
> > +     context.activeState.awb.automatic.temperatureK = kDefaultColourTemperature;
> >  
> >       /*
> >        * Define the measurement window for AWB as a centered rectangle
> > @@ -185,7 +186,7 @@ void Awb::queueRequest(IPAContext &context,
> >               const auto &gains = awbAlgo_->gainsFromColourTemperature(*colourTemperature);
> >               awb.manual.gains.r() = gains.r();
> >               awb.manual.gains.b() = gains.b();
> > -             awb.temperatureK = *colourTemperature;
> > +             awb.manual.temperatureK = *colourTemperature;
> >               update = true;
> >       }
> >  
> > @@ -194,7 +195,7 @@ void Awb::queueRequest(IPAContext &context,
> >                       << "Set colour gains to " << awb.manual.gains;
> >  
> >       frameContext.awb.gains = awb.manual.gains;
> > -     frameContext.awb.temperatureK = awb.temperatureK;
> > +     frameContext.awb.temperatureK = awb.manual.temperatureK;
> >  }
> >  
> >  /**
> > @@ -208,8 +209,9 @@ 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.automatic.gains;
> > -             frameContext.awb.temperatureK = context.activeState.awb.temperatureK;
> > +             auto &awb = context.activeState.awb;
> 
> const
> 
> > +             frameContext.awb.gains = awb.automatic.gains;
> > +             frameContext.awb.temperatureK = awb.automatic.temperatureK;
> >       }
> >  
> >       auto gainConfig = params->block<BlockType::AwbGain>();
> > @@ -309,7 +311,7 @@ void Awb::process(IPAContext &context,
> >       RkISP1AwbStats awbStats{ rgbMeans };
> >       AwbResult awbResult = awbAlgo_->calculateAwb(awbStats, frameContext.lux.lux);
> >  
> > -     activeState.awb.temperatureK = awbResult.colourTemperature;
> > +     activeState.awb.automatic.temperatureK = awbResult.colourTemperature;
> >  
> >       /*
> >        * Clamp the gain values to the hardware, which expresses gains as Q2.8
> > @@ -330,7 +332,7 @@ void Awb::process(IPAContext &context,
> >               << std::showpoint
> >               << "Means " << rgbMeans << ", gains "
> >               << activeState.awb.automatic.gains << ", temp "
> > -             << activeState.awb.temperatureK << "K";
> > +             << activeState.awb.automatic.temperatureK << "K";
> >  }
> >  
> >  RGB<double> Awb::calculateRgbMeans(const IPAFrameContext &frameContext, const rkisp1_cif_isp_awb_stat *awb) const
> > diff --git a/src/ipa/rkisp1/algorithms/ccm.cpp b/src/ipa/rkisp1/algorithms/ccm.cpp
> > index eb8ca39e56a8..2e5e91006b55 100644
> > --- a/src/ipa/rkisp1/algorithms/ccm.cpp
> > +++ b/src/ipa/rkisp1/algorithms/ccm.cpp
> > @@ -88,7 +88,7 @@ void Ccm::setParameters(struct rkisp1_cif_isp_ctk_config &config,
> >  void Ccm::prepare(IPAContext &context, const uint32_t frame,
> >                 IPAFrameContext &frameContext, RkISP1Params *params)
> >  {
> > -     uint32_t ct = context.activeState.awb.temperatureK;
> > +     uint32_t ct = frameContext.awb.temperatureK;
> >  
> >       /*
> >        * \todo The colour temperature will likely be noisy, add filtering to
> > diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp
> > index e47aa2f0727e..e7301bfec863 100644
> > --- a/src/ipa/rkisp1/algorithms/lsc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/lsc.cpp
> > @@ -404,12 +404,12 @@ void LensShadingCorrection::copyTable(rkisp1_cif_isp_lsc_config &config,
> >  /**
> >   * \copydoc libcamera::ipa::Algorithm::prepare
> >   */
> > -void LensShadingCorrection::prepare(IPAContext &context,
> > +void LensShadingCorrection::prepare([[maybe_unused]] IPAContext &context,
> >                                   [[maybe_unused]] const uint32_t frame,
> > -                                 [[maybe_unused]] IPAFrameContext &frameContext,
> > +                                 IPAFrameContext &frameContext,
> >                                   RkISP1Params *params)
> >  {
> > -     uint32_t ct = context.activeState.awb.temperatureK;
> > +     uint32_t ct = frameContext.awb.temperatureK;
> >       if (std::abs(static_cast<int>(ct) - static_cast<int>(lastAppliedCt_)) <
> >           kColourTemperatureChangeThreshhold)
> >               return;
> > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> > index 39b97d143e95..7bc42e6de415 100644
> > --- a/src/ipa/rkisp1/ipa_context.cpp
> > +++ b/src/ipa/rkisp1/ipa_context.cpp
> > @@ -197,15 +197,15 @@ namespace libcamera::ipa::rkisp1 {
> >   * \struct IPAActiveState::awb::AwbState.gains
> >   * \brief White balance gains
> >   *
> > + * \var IPAActiveState::awb::AwbState.temperatureK
> > + * \brief Estimated color temperature
> > + *
> 
> Is that right ? automatic.temperatureK is indeed estimated by the AWB
> algorithm, but manual.temperatureK is the value set through requests.
> Being more precise would be nice, as the code is already confusing as-is
> :-)

You are right. I improved it a bit before the merge. There are other
places in the docs that need a bit of love. Maybe I'll find some time to
include the docs in doxygen and use that to carry a bit of rework.

Best regards,
Stefan

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> >   * \var IPAActiveState::awb.manual
> >   * \brief Manual regulation state (set through requests)
> >   *
> >   * \var IPAActiveState::awb.automatic
> >   * \brief Automatic regulation state (computed by the algorithm)
> >   *
> > - * \var IPAActiveState::awb.temperatureK
> > - * \brief Estimated color temperature
> > - *
> >   * \var IPAActiveState::awb.autoEnabled
> >   * \brief Whether the Auto White Balance algorithm is enabled
> >   */
> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > index 6bc922a82971..769e9f114e23 100644
> > --- a/src/ipa/rkisp1/ipa_context.h
> > +++ b/src/ipa/rkisp1/ipa_context.h
> > @@ -91,12 +91,12 @@ struct IPAActiveState {
> >       struct {
> >               struct AwbState {
> >                       RGB<double> gains;
> > +                     unsigned int temperatureK;
> >               };
> >  
> >               AwbState manual;
> >               AwbState automatic;
> >  
> > -             unsigned int temperatureK;
> >               bool autoEnabled;
> >       } awb;
> >  
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
index 79c4c658406d..0795b8e5b1e1 100644
--- a/src/ipa/rkisp1/algorithms/awb.cpp
+++ b/src/ipa/rkisp1/algorithms/awb.cpp
@@ -128,7 +128,8 @@  int Awb::configure(IPAContext &context,
 	context.activeState.awb.automatic.gains =
 		awbAlgo_->gainsFromColourTemperature(kDefaultColourTemperature);
 	context.activeState.awb.autoEnabled = true;
-	context.activeState.awb.temperatureK = kDefaultColourTemperature;
+	context.activeState.awb.manual.temperatureK = kDefaultColourTemperature;
+	context.activeState.awb.automatic.temperatureK = kDefaultColourTemperature;
 
 	/*
 	 * Define the measurement window for AWB as a centered rectangle
@@ -185,7 +186,7 @@  void Awb::queueRequest(IPAContext &context,
 		const auto &gains = awbAlgo_->gainsFromColourTemperature(*colourTemperature);
 		awb.manual.gains.r() = gains.r();
 		awb.manual.gains.b() = gains.b();
-		awb.temperatureK = *colourTemperature;
+		awb.manual.temperatureK = *colourTemperature;
 		update = true;
 	}
 
@@ -194,7 +195,7 @@  void Awb::queueRequest(IPAContext &context,
 			<< "Set colour gains to " << awb.manual.gains;
 
 	frameContext.awb.gains = awb.manual.gains;
-	frameContext.awb.temperatureK = awb.temperatureK;
+	frameContext.awb.temperatureK = awb.manual.temperatureK;
 }
 
 /**
@@ -208,8 +209,9 @@  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.automatic.gains;
-		frameContext.awb.temperatureK = context.activeState.awb.temperatureK;
+		auto &awb = context.activeState.awb;
+		frameContext.awb.gains = awb.automatic.gains;
+		frameContext.awb.temperatureK = awb.automatic.temperatureK;
 	}
 
 	auto gainConfig = params->block<BlockType::AwbGain>();
@@ -309,7 +311,7 @@  void Awb::process(IPAContext &context,
 	RkISP1AwbStats awbStats{ rgbMeans };
 	AwbResult awbResult = awbAlgo_->calculateAwb(awbStats, frameContext.lux.lux);
 
-	activeState.awb.temperatureK = awbResult.colourTemperature;
+	activeState.awb.automatic.temperatureK = awbResult.colourTemperature;
 
 	/*
 	 * Clamp the gain values to the hardware, which expresses gains as Q2.8
@@ -330,7 +332,7 @@  void Awb::process(IPAContext &context,
 		<< std::showpoint
 		<< "Means " << rgbMeans << ", gains "
 		<< activeState.awb.automatic.gains << ", temp "
-		<< activeState.awb.temperatureK << "K";
+		<< activeState.awb.automatic.temperatureK << "K";
 }
 
 RGB<double> Awb::calculateRgbMeans(const IPAFrameContext &frameContext, const rkisp1_cif_isp_awb_stat *awb) const
diff --git a/src/ipa/rkisp1/algorithms/ccm.cpp b/src/ipa/rkisp1/algorithms/ccm.cpp
index eb8ca39e56a8..2e5e91006b55 100644
--- a/src/ipa/rkisp1/algorithms/ccm.cpp
+++ b/src/ipa/rkisp1/algorithms/ccm.cpp
@@ -88,7 +88,7 @@  void Ccm::setParameters(struct rkisp1_cif_isp_ctk_config &config,
 void Ccm::prepare(IPAContext &context, const uint32_t frame,
 		  IPAFrameContext &frameContext, RkISP1Params *params)
 {
-	uint32_t ct = context.activeState.awb.temperatureK;
+	uint32_t ct = frameContext.awb.temperatureK;
 
 	/*
 	 * \todo The colour temperature will likely be noisy, add filtering to
diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp
index e47aa2f0727e..e7301bfec863 100644
--- a/src/ipa/rkisp1/algorithms/lsc.cpp
+++ b/src/ipa/rkisp1/algorithms/lsc.cpp
@@ -404,12 +404,12 @@  void LensShadingCorrection::copyTable(rkisp1_cif_isp_lsc_config &config,
 /**
  * \copydoc libcamera::ipa::Algorithm::prepare
  */
-void LensShadingCorrection::prepare(IPAContext &context,
+void LensShadingCorrection::prepare([[maybe_unused]] IPAContext &context,
 				    [[maybe_unused]] const uint32_t frame,
-				    [[maybe_unused]] IPAFrameContext &frameContext,
+				    IPAFrameContext &frameContext,
 				    RkISP1Params *params)
 {
-	uint32_t ct = context.activeState.awb.temperatureK;
+	uint32_t ct = frameContext.awb.temperatureK;
 	if (std::abs(static_cast<int>(ct) - static_cast<int>(lastAppliedCt_)) <
 	    kColourTemperatureChangeThreshhold)
 		return;
diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
index 39b97d143e95..7bc42e6de415 100644
--- a/src/ipa/rkisp1/ipa_context.cpp
+++ b/src/ipa/rkisp1/ipa_context.cpp
@@ -197,15 +197,15 @@  namespace libcamera::ipa::rkisp1 {
  * \struct IPAActiveState::awb::AwbState.gains
  * \brief White balance gains
  *
+ * \var IPAActiveState::awb::AwbState.temperatureK
+ * \brief Estimated color temperature
+ *
  * \var IPAActiveState::awb.manual
  * \brief Manual regulation state (set through requests)
  *
  * \var IPAActiveState::awb.automatic
  * \brief Automatic regulation state (computed by the algorithm)
  *
- * \var IPAActiveState::awb.temperatureK
- * \brief Estimated color temperature
- *
  * \var IPAActiveState::awb.autoEnabled
  * \brief Whether the Auto White Balance algorithm is enabled
  */
diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
index 6bc922a82971..769e9f114e23 100644
--- a/src/ipa/rkisp1/ipa_context.h
+++ b/src/ipa/rkisp1/ipa_context.h
@@ -91,12 +91,12 @@  struct IPAActiveState {
 	struct {
 		struct AwbState {
 			RGB<double> gains;
+			unsigned int temperatureK;
 		};
 
 		AwbState manual;
 		AwbState automatic;
 
-		unsigned int temperatureK;
 		bool autoEnabled;
 	} awb;