[PATCH v3 11/16] ipa: rkisp1: ccm/lsc: Fix CCM/LSC based on manual color temperature
Stefan Klug
stefan.klug at ideasonboard.com
Tue May 20 11:19:38 CEST 2025
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 at 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 at 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
More information about the libcamera-devel
mailing list