| Message ID | 20261106125807.1238919-2-barnabas.pocze@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting Barnabás Pőcze (2026-11-06 12:58:06) > The color temperature is already calculated by the "Awb" algorithm, > and since usually that runs first, that value goes in the metadata. > The "Awb" value is used by the "Ccm" algorithm, the "Agc" value is > used by the "Lsc" algorithm. > > So remove the calculation and adjust the "Lsc" algorithm to use > the same color temperature as "Ccm". Oh yes, This should be normalised to a single path. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > src/ipa/mali-c55/algorithms/agc.cpp | 5 ----- > src/ipa/mali-c55/algorithms/lsc.cpp | 6 +++--- > src/ipa/mali-c55/ipa_context.h | 5 +---- > 3 files changed, 4 insertions(+), 12 deletions(-) > > diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp > index 2c747da891..bf9d3f7d4a 100644 > --- a/src/ipa/mali-c55/algorithms/agc.cpp > +++ b/src/ipa/mali-c55/algorithms/agc.cpp > @@ -257,9 +257,6 @@ void Agc::process(IPAContext &context, > } > > statistics_.parseStatistics(stats); > - context.activeState.agc.temperatureK = estimateCCT({ { statistics_.rHist.interQuantileMean(0, 1), > - statistics_.gHist.interQuantileMean(0, 1), > - statistics_.bHist.interQuantileMean(0, 1) } }); > > agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {{ > .traits = AgcTraits(statistics_), > @@ -267,8 +264,6 @@ void Agc::process(IPAContext &context, > .exposure = frameContext.sensor.exposure, > .gain = frameContext.sensor.gain, > }}, metadata); > - > - metadata.set(controls::ColourTemperature, context.activeState.agc.temperatureK); > } > > REGISTER_IPA_ALGORITHM(Agc, "Agc") > diff --git a/src/ipa/mali-c55/algorithms/lsc.cpp b/src/ipa/mali-c55/algorithms/lsc.cpp > index e65e2a1807..4944f2aae4 100644 > --- a/src/ipa/mali-c55/algorithms/lsc.cpp > +++ b/src/ipa/mali-c55/algorithms/lsc.cpp > @@ -180,8 +180,8 @@ std::tuple<uint8_t, uint8_t> Lsc::findBankAndAlpha(uint32_t ct) const > return { i, alpha }; > } > > -void Lsc::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame, > - [[maybe_unused]] IPAFrameContext &frameContext, > +void Lsc::prepare([[maybe_unused]] IPAContext &context, [[maybe_unused]] const uint32_t frame, > + IPAFrameContext &frameContext, > MaliC55Params *params) > { > /* > @@ -192,7 +192,7 @@ void Lsc::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame, > * that table and if we're past the first frame then we can just do > * nothing - the config will never change. > */ > - uint32_t temperatureK = context.activeState.agc.temperatureK; > + uint32_t temperatureK = frameContext.awb.colourTemperature; This might subtly change the timings. Also the comment above here says "we assess the colour temperature of the last frame ..." Is that still accurate ? It's so hard to track which data is representative of the current frame or used for the current frame. Ohh interestingly, this means it will now use the manual colour temperature when set manually I think ? (which is actually a good correction here?) Stefan - any insights? -- Kieran > uint8_t bank, alpha; > > if (colourTemperatures_.size() == 1) { > diff --git a/src/ipa/mali-c55/ipa_context.h b/src/ipa/mali-c55/ipa_context.h > index 030e0052ad..74585351c7 100644 > --- a/src/ipa/mali-c55/ipa_context.h > +++ b/src/ipa/mali-c55/ipa_context.h > @@ -38,10 +38,7 @@ struct IPASessionConfiguration { > }; > > struct IPAActiveState { > - struct Agc : agc::ActiveState { > - uint32_t temperatureK; > - } agc; > - > + ipa::agc::ActiveState agc; > ipa::awb::ActiveState awb; > ipa::ccm::ActiveState ccm; > ipa::gamma::ActiveState gamma; > -- > 2.56.0 >
Quoting Kieran Bingham (2026-11-06 16:00:30) > Quoting Barnabás Pőcze (2026-11-06 12:58:06) > > The color temperature is already calculated by the "Awb" algorithm, > > and since usually that runs first, that value goes in the metadata. > > The "Awb" value is used by the "Ccm" algorithm, the "Agc" value is > > used by the "Lsc" algorithm. > > > > So remove the calculation and adjust the "Lsc" algorithm to use > > the same color temperature as "Ccm". > > Oh yes, This should be normalised to a single path. > > > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > --- > > src/ipa/mali-c55/algorithms/agc.cpp | 5 ----- > > src/ipa/mali-c55/algorithms/lsc.cpp | 6 +++--- > > src/ipa/mali-c55/ipa_context.h | 5 +---- > > 3 files changed, 4 insertions(+), 12 deletions(-) > > > > diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp > > index 2c747da891..bf9d3f7d4a 100644 > > --- a/src/ipa/mali-c55/algorithms/agc.cpp > > +++ b/src/ipa/mali-c55/algorithms/agc.cpp > > @@ -257,9 +257,6 @@ void Agc::process(IPAContext &context, > > } > > > > statistics_.parseStatistics(stats); > > - context.activeState.agc.temperatureK = estimateCCT({ { statistics_.rHist.interQuantileMean(0, 1), > > - statistics_.gHist.interQuantileMean(0, 1), > > - statistics_.bHist.interQuantileMean(0, 1) } }); > > > > agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {{ > > .traits = AgcTraits(statistics_), > > @@ -267,8 +264,6 @@ void Agc::process(IPAContext &context, > > .exposure = frameContext.sensor.exposure, > > .gain = frameContext.sensor.gain, > > }}, metadata); > > - > > - metadata.set(controls::ColourTemperature, context.activeState.agc.temperatureK); > > } > > > > REGISTER_IPA_ALGORITHM(Agc, "Agc") > > diff --git a/src/ipa/mali-c55/algorithms/lsc.cpp b/src/ipa/mali-c55/algorithms/lsc.cpp > > index e65e2a1807..4944f2aae4 100644 > > --- a/src/ipa/mali-c55/algorithms/lsc.cpp > > +++ b/src/ipa/mali-c55/algorithms/lsc.cpp > > @@ -180,8 +180,8 @@ std::tuple<uint8_t, uint8_t> Lsc::findBankAndAlpha(uint32_t ct) const > > return { i, alpha }; > > } > > > > -void Lsc::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame, > > - [[maybe_unused]] IPAFrameContext &frameContext, > > +void Lsc::prepare([[maybe_unused]] IPAContext &context, [[maybe_unused]] const uint32_t frame, > > + IPAFrameContext &frameContext, > > MaliC55Params *params) > > { > > /* > > @@ -192,7 +192,7 @@ void Lsc::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame, > > * that table and if we're past the first frame then we can just do > > * nothing - the config will never change. > > */ > > - uint32_t temperatureK = context.activeState.agc.temperatureK; > > + uint32_t temperatureK = frameContext.awb.colourTemperature; > > This might subtly change the timings. > > Also the comment above here says "we assess the colour temperature of > the last frame ..." > > Is that still accurate ? It's so hard to track which data is > representative of the current frame or used for the current frame. > > Ohh interestingly, this means it will now use the manual colour > temperature when set manually I think ? (which is actually a good > correction here?) > > Stefan - any insights? This is in line with what we do in rkisp1. Conceptually it is correct. prepare() fixes all parameters for a given frame. And awb fixes the colourTemperatur value for that frame and stores it in the frameContext (and thereby supports manual colour temperature). The only downside is that we need to enusre that the algorithms run in the correct order, which isn't implemented. Therefore: Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> Best regards, Stefan > > -- > Kieran > > > uint8_t bank, alpha; > > > > if (colourTemperatures_.size() == 1) { > > diff --git a/src/ipa/mali-c55/ipa_context.h b/src/ipa/mali-c55/ipa_context.h > > index 030e0052ad..74585351c7 100644 > > --- a/src/ipa/mali-c55/ipa_context.h > > +++ b/src/ipa/mali-c55/ipa_context.h > > @@ -38,10 +38,7 @@ struct IPASessionConfiguration { > > }; > > > > struct IPAActiveState { > > - struct Agc : agc::ActiveState { > > - uint32_t temperatureK; > > - } agc; > > - > > + ipa::agc::ActiveState agc; > > ipa::awb::ActiveState awb; > > ipa::ccm::ActiveState ccm; > > ipa::gamma::ActiveState gamma; > > -- > > 2.56.0 > >
diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp index 2c747da891..bf9d3f7d4a 100644 --- a/src/ipa/mali-c55/algorithms/agc.cpp +++ b/src/ipa/mali-c55/algorithms/agc.cpp @@ -257,9 +257,6 @@ void Agc::process(IPAContext &context, } statistics_.parseStatistics(stats); - context.activeState.agc.temperatureK = estimateCCT({ { statistics_.rHist.interQuantileMean(0, 1), - statistics_.gHist.interQuantileMean(0, 1), - statistics_.bHist.interQuantileMean(0, 1) } }); agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {{ .traits = AgcTraits(statistics_), @@ -267,8 +264,6 @@ void Agc::process(IPAContext &context, .exposure = frameContext.sensor.exposure, .gain = frameContext.sensor.gain, }}, metadata); - - metadata.set(controls::ColourTemperature, context.activeState.agc.temperatureK); } REGISTER_IPA_ALGORITHM(Agc, "Agc") diff --git a/src/ipa/mali-c55/algorithms/lsc.cpp b/src/ipa/mali-c55/algorithms/lsc.cpp index e65e2a1807..4944f2aae4 100644 --- a/src/ipa/mali-c55/algorithms/lsc.cpp +++ b/src/ipa/mali-c55/algorithms/lsc.cpp @@ -180,8 +180,8 @@ std::tuple<uint8_t, uint8_t> Lsc::findBankAndAlpha(uint32_t ct) const return { i, alpha }; } -void Lsc::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame, - [[maybe_unused]] IPAFrameContext &frameContext, +void Lsc::prepare([[maybe_unused]] IPAContext &context, [[maybe_unused]] const uint32_t frame, + IPAFrameContext &frameContext, MaliC55Params *params) { /* @@ -192,7 +192,7 @@ void Lsc::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame, * that table and if we're past the first frame then we can just do * nothing - the config will never change. */ - uint32_t temperatureK = context.activeState.agc.temperatureK; + uint32_t temperatureK = frameContext.awb.colourTemperature; uint8_t bank, alpha; if (colourTemperatures_.size() == 1) { diff --git a/src/ipa/mali-c55/ipa_context.h b/src/ipa/mali-c55/ipa_context.h index 030e0052ad..74585351c7 100644 --- a/src/ipa/mali-c55/ipa_context.h +++ b/src/ipa/mali-c55/ipa_context.h @@ -38,10 +38,7 @@ struct IPASessionConfiguration { }; struct IPAActiveState { - struct Agc : agc::ActiveState { - uint32_t temperatureK; - } agc; - + ipa::agc::ActiveState agc; ipa::awb::ActiveState awb; ipa::ccm::ActiveState ccm; ipa::gamma::ActiveState gamma;
The color temperature is already calculated by the "Awb" algorithm, and since usually that runs first, that value goes in the metadata. The "Awb" value is used by the "Ccm" algorithm, the "Agc" value is used by the "Lsc" algorithm. So remove the calculation and adjust the "Lsc" algorithm to use the same color temperature as "Ccm". Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- src/ipa/mali-c55/algorithms/agc.cpp | 5 ----- src/ipa/mali-c55/algorithms/lsc.cpp | 6 +++--- src/ipa/mali-c55/ipa_context.h | 5 +---- 3 files changed, 4 insertions(+), 12 deletions(-)