[PATCH v2 10/17] ipa: rkisp1: ccm/lsc: Fix CCM/LSC based on manual color temperature
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Mar 31 19:30:41 CEST 2025
Quoting Stefan Klug (2025-03-19 16:11:15)
> 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 at ideasonboard.com>
>
> ---
>
> Changes in v2:
> - None
> ---
> src/ipa/rkisp1/algorithms/awb.cpp | 18 ++++++++++--------
> src/ipa/rkisp1/algorithms/ccm.cpp | 2 +-
> src/ipa/rkisp1/algorithms/lsc.cpp | 6 +++---
> src/ipa/rkisp1/ipa_context.h | 2 +-
> 4 files changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> index a9759e53f593..5e067e50cd52 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,10 +311,10 @@ 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;
>
> /* Metadata shall contain the up to date measurement */
> - metadata.set(controls::ColourTemperature, activeState.awb.temperatureK);
> + metadata.set(controls::ColourTemperature, activeState.awb.automatic.temperatureK);
I think we discussed different options for this behaviour.
Don't we expect to report the manually set colour temperature if that's
what was set on that framecontext ? (So that the colour gains match the
colour temperature or such ?)
Though I wish there was a way to report both so we can still what the
autoregulation 'would think' even if it's set to manual...
> /*
> * Clamp the gain values to the hardware, which expresses gains as Q2.8
> @@ -333,7 +335,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;
Hrm. ... we need to make sure Awb algo always runs before Ccm in this
instance.
I think we're "in luck" because of alphabetical sorting, but I think
this indicates we will need to indicate some sort of dependencies
between algos...
>
> /*
> * \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;
It is a good job Awb begins with 'a'...
> if (std::abs(static_cast<int>(ct) - static_cast<int>(lastAppliedCt_)) <
> kColourTemperatureChangeThreshhold)
> return;
> 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
>
More information about the libcamera-devel
mailing list