| Message ID | 20250319161152.63625-11-stefan.klug@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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 >
On Mon, Mar 31, 2025 at 06:30:41PM +0100, Kieran Bingham wrote: > 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@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 ?) That's the general rule for libcamera controls. The less we depart from general control rules, the better (but there will always be some exceptions). > 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... I'm increasingly leaning towards having two controls if we really need to expose the measured temperature while in manual mode. We'll have to define how those controls work in auto mode, and also decide if all cameras that support colour temperature need to support measuring in manual mode. > > /* > > * 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... It's not an alphabetical order matter, at least not alphabetical order of the C++ class names. Algorithms are run in the order they are instantiated in the tuning file. I however agree we should be able to specify dependencies, that would be safer. > > > > /* > > * \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; > >
On Tue, Apr 01, 2025 at 11:16:09PM +0300, Laurent Pinchart wrote: > On Mon, Mar 31, 2025 at 06:30:41PM +0100, Kieran Bingham wrote: > > 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@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 ?) > > That's the general rule for libcamera controls. The less we depart from > general control rules, the better (but there will always be some > exceptions). > > > 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... > > I'm increasingly leaning towards having two controls if we really need > to expose the measured temperature while in manual mode. We'll have to > define how those controls work in auto mode, and also decide if all > cameras that support colour temperature need to support measuring in > manual mode. > > > > /* > > > * 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... > > It's not an alphabetical order matter, at least not alphabetical order > of the C++ class names. Algorithms are run in the order they are > instantiated in the tuning file. I however agree we should be able to > specify dependencies, that would be safer. Yes, declaring dependencies would be nice. At the moment the correct order is only manually ensured inside the tuning file generator. That will break as soon as people start to manually modify the tuning files without knowing all the internals. With WDR there is another algo where process() needs to run after AEGC has processed the stats but prepare() needs to run before AEGC does prepare()... At the moment it works because our AEGC implementation is not completely correct (it calculates the gain exposure split in process which imho needs to be moved to prepare() in the future). Limitless fun for the future :-) Cheers, Stefan > > > > > > > /* > > > * \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; > > > > > -- > Regards, > > Laurent Pinchart
On Mon, Mar 31, 2025 at 06:30:41PM +0100, Kieran Bingham wrote: > 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@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 ?) Ouch good catch. Thank you. That line shouldn't be there at all. The metadata gets set a few lines above. Turns out that it was a rebase error that slipped in in b60bd37b1a49 ("ipa: rkisp1: Move calculation of RGB means into own function"). I'll fix in v3. Best regards, Stefan > > > 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 > >
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); /* * 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; /* * \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.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;
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 --- 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(-)