| Message ID | 20250109115412.356768-8-stefan.klug@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
On Thu, Jan 09, 2025 at 12:53:58PM +0100, Stefan Klug wrote: > Now that libipa contains a grey world algorithm, use that. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > src/ipa/rkisp1/algorithms/awb.cpp | 72 ++++++++++++++++++++----------- > src/ipa/rkisp1/algorithms/awb.h | 4 +- > 2 files changed, 49 insertions(+), 27 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > index f6449565834b..42a4784998bc 100644 > --- a/src/ipa/rkisp1/algorithms/awb.cpp > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > @@ -16,6 +16,7 @@ > > #include <libcamera/ipa/core_ipa_interface.h> > > +#include "libipa/awb_grey.h" > #include "libipa/colours.h" > > /** > @@ -40,6 +41,28 @@ constexpr int32_t kDefaultColourTemperature = 5000; > /* Minimum mean value below which AWB can't operate. */ > constexpr double kMeanMinThreshold = 2.0; > > +class RkISP1AwbStats : public AwbStats > +{ > +public: > + RkISP1AwbStats(const RGB<double> &rgbMeans) > + : rgbMeans_(rgbMeans) {} > + > + double computeColourError([[maybe_unused]] const RGB<double> &gains) const override > + { > + LOG(RkISP1Awb, Error) > + << "RkISP1AwbStats::computeColourError is not implemented"; > + return 0.0; > + } > + > + RGB<double> getRGBMeans() const override > + { > + return rgbMeans_; > + }; > + > +private: > + RGB<double> rgbMeans_; > +}; > + > Awb::Awb() > : rgbMode_(false) > { > @@ -55,15 +78,12 @@ int Awb::init(IPAContext &context, const YamlObject &tuningData) > kMaxColourTemperature, > kDefaultColourTemperature); > > - Interpolator<Vector<double, 2>> gainCurve; > - int ret = gainCurve.readYaml(tuningData["colourGains"], "ct", "gains"); > - if (ret < 0) > - LOG(RkISP1Awb, Warning) > - << "Failed to parse 'colourGains' " > - << "parameter from tuning file; " > - << "manual colour temperature will not work properly"; > - else > - colourGainCurve_ = gainCurve; > + awbAlgo_ = std::make_unique<AwbGrey>(); > + int ret = awbAlgo_->init(tuningData); > + if (ret) { > + LOG(RkISP1Awb, Error) << "Failed to init awb algorithm"; > + return ret; > + } > > return 0; > } > @@ -128,10 +148,10 @@ void Awb::queueRequest(IPAContext &context, > * This will be fixed with the bayes AWB algorithm. > */ > update = true; > - } else if (colourTemperature && colourGainCurve_) { > - const auto &gains = colourGainCurve_->getInterpolated(*colourTemperature); > - awb.gains.manual.r() = gains[0]; > - awb.gains.manual.b() = gains[1]; > + } else if (colourTemperature) { > + const auto &gains = awbAlgo_->gainsFromColourTemperature(*colourTemperature); > + awb.gains.manual.r() = gains.r(); > + awb.gains.manual.b() = gains.b(); > awb.temperatureK = *colourTemperature; > update = true; > } > @@ -251,33 +271,33 @@ void Awb::process(IPAContext &context, > rgbMeans.b() < kMeanMinThreshold) > return; > > - activeState.awb.temperatureK = estimateCCT(rgbMeans); > + /* > + * \Todo: Hardcode lux to a fixed value, until an estimation is > + * implemented. > + */ > + int lux = 1000; > + > + RkISP1AwbStats awbStats{ rgbMeans }; > + AwbResult r = awbAlgo_->calculateAwb(awbStats, lux); > + > + activeState.awb.temperatureK = r.colourTemperature; > > /* Metadata shall contain the up to date measurement */ > metadata.set(controls::ColourTemperature, activeState.awb.temperatureK); > > - /* > - * Estimate the red and blue gains to apply in a grey world. The green > - * gain is hardcoded to 1.0. Avoid divisions by zero by clamping the > - * divisor to a minimum value of 1.0. > - */ > - RGB<double> gains({ rgbMeans.g() / std::max(rgbMeans.r(), 1.0), > - 1.0, > - rgbMeans.g() / std::max(rgbMeans.b(), 1.0) }); > - > /* > * Clamp the gain values to the hardware, which expresses gains as Q2.8 > * unsigned integer values. Set the minimum just above zero to avoid > * divisions by zero when computing the raw means in subsequent > * iterations. > */ > - gains = gains.max(1.0 / 256).min(1023.0 / 256); > + r.gains = r.gains.max(1.0 / 256).min(1023.0 / 256); > > /* Filter the values to avoid oscillations. */ > double speed = 0.2; > - gains = gains * speed + activeState.awb.gains.automatic * (1 - speed); > + r.gains = r.gains * speed + activeState.awb.gains.automatic * (1 - speed); > > - activeState.awb.gains.automatic = gains; > + activeState.awb.gains.automatic = r.gains; > > LOG(RkISP1Awb, Debug) > << std::showpoint > diff --git a/src/ipa/rkisp1/algorithms/awb.h b/src/ipa/rkisp1/algorithms/awb.h > index 2a64cb60d5f4..3382b2178567 100644 > --- a/src/ipa/rkisp1/algorithms/awb.h > +++ b/src/ipa/rkisp1/algorithms/awb.h > @@ -9,6 +9,7 @@ > > #include <optional> > > +#include "libipa/awb.h" > #include "libipa/interpolator.h" > #include "libipa/vector.h" > > @@ -41,7 +42,8 @@ private: > RGB<double> calculateRgbMeans(const IPAFrameContext &frameContext, > const rkisp1_cif_isp_awb_stat *awb) const; > > - std::optional<Interpolator<Vector<double, 2>>> colourGainCurve_; > + std::unique_ptr<AwbAlgorithm> awbAlgo_; > + > bool rgbMode_; > }; > > -- > 2.43.0 >
Hi Stefan On 09/01/2025 11:53, Stefan Klug wrote: > Now that libipa contains a grey world algorithm, use that. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > src/ipa/rkisp1/algorithms/awb.cpp | 72 ++++++++++++++++++++----------- > src/ipa/rkisp1/algorithms/awb.h | 4 +- > 2 files changed, 49 insertions(+), 27 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > index f6449565834b..42a4784998bc 100644 > --- a/src/ipa/rkisp1/algorithms/awb.cpp > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > @@ -16,6 +16,7 @@ > > #include <libcamera/ipa/core_ipa_interface.h> > > +#include "libipa/awb_grey.h" > #include "libipa/colours.h" > > /** > @@ -40,6 +41,28 @@ constexpr int32_t kDefaultColourTemperature = 5000; > /* Minimum mean value below which AWB can't operate. */ > constexpr double kMeanMinThreshold = 2.0; > > +class RkISP1AwbStats : public AwbStats > +{ > +public: > + RkISP1AwbStats(const RGB<double> &rgbMeans) > + : rgbMeans_(rgbMeans) {} > + > + double computeColourError([[maybe_unused]] const RGB<double> &gains) const override > + { > + LOG(RkISP1Awb, Error) > + << "RkISP1AwbStats::computeColourError is not implemented"; > + return 0.0; > + } If it's optional, perhaps rather than a pure virtual function it should have this "not implemented" variant in the base class? > + > + RGB<double> getRGBMeans() const override > + { > + return rgbMeans_; > + }; > + > +private: > + RGB<double> rgbMeans_; > +}; > + > Awb::Awb() > : rgbMode_(false) > { > @@ -55,15 +78,12 @@ int Awb::init(IPAContext &context, const YamlObject &tuningData) > kMaxColourTemperature, > kDefaultColourTemperature); > > - Interpolator<Vector<double, 2>> gainCurve; > - int ret = gainCurve.readYaml(tuningData["colourGains"], "ct", "gains"); > - if (ret < 0) > - LOG(RkISP1Awb, Warning) > - << "Failed to parse 'colourGains' " > - << "parameter from tuning file; " > - << "manual colour temperature will not work properly"; > - else > - colourGainCurve_ = gainCurve; > + awbAlgo_ = std::make_unique<AwbGrey>(); > + int ret = awbAlgo_->init(tuningData); > + if (ret) { > + LOG(RkISP1Awb, Error) << "Failed to init awb algorithm"; > + return ret; > + } I'm a bit surprised to see a pointer to the base class, I was expecting this class to derive the other one (and just call the base class init() here). Why'd you choose to do it this way? > > return 0; > } > @@ -128,10 +148,10 @@ void Awb::queueRequest(IPAContext &context, > * This will be fixed with the bayes AWB algorithm. > */ > update = true; > - } else if (colourTemperature && colourGainCurve_) { > - const auto &gains = colourGainCurve_->getInterpolated(*colourTemperature); > - awb.gains.manual.r() = gains[0]; > - awb.gains.manual.b() = gains[1]; > + } else if (colourTemperature) { > + const auto &gains = awbAlgo_->gainsFromColourTemperature(*colourTemperature); > + awb.gains.manual.r() = gains.r(); > + awb.gains.manual.b() = gains.b(); > awb.temperatureK = *colourTemperature; > update = true; > } > @@ -251,33 +271,33 @@ void Awb::process(IPAContext &context, > rgbMeans.b() < kMeanMinThreshold) > return; > > - activeState.awb.temperatureK = estimateCCT(rgbMeans); > + /* > + * \Todo: Hardcode lux to a fixed value, until an estimation is > + * implemented. > + */ > + int lux = 1000; > + > + RkISP1AwbStats awbStats{ rgbMeans }; > + AwbResult r = awbAlgo_->calculateAwb(awbStats, lux); A more descriptive variable name would be slightly better I think; "r" in this context makes me think "red" not "results". > + > + activeState.awb.temperatureK = r.colourTemperature; > > /* Metadata shall contain the up to date measurement */ > metadata.set(controls::ColourTemperature, activeState.awb.temperatureK); > > - /* > - * Estimate the red and blue gains to apply in a grey world. The green > - * gain is hardcoded to 1.0. Avoid divisions by zero by clamping the > - * divisor to a minimum value of 1.0. > - */ > - RGB<double> gains({ rgbMeans.g() / std::max(rgbMeans.r(), 1.0), > - 1.0, > - rgbMeans.g() / std::max(rgbMeans.b(), 1.0) }); > - > /* > * Clamp the gain values to the hardware, which expresses gains as Q2.8 > * unsigned integer values. Set the minimum just above zero to avoid > * divisions by zero when computing the raw means in subsequent > * iterations. > */ > - gains = gains.max(1.0 / 256).min(1023.0 / 256); > + r.gains = r.gains.max(1.0 / 256).min(1023.0 / 256); > > /* Filter the values to avoid oscillations. */ > double speed = 0.2; > - gains = gains * speed + activeState.awb.gains.automatic * (1 - speed); > + r.gains = r.gains * speed + activeState.awb.gains.automatic * (1 - speed); > > - activeState.awb.gains.automatic = gains; > + activeState.awb.gains.automatic = r.gains; > > LOG(RkISP1Awb, Debug) > << std::showpoint > diff --git a/src/ipa/rkisp1/algorithms/awb.h b/src/ipa/rkisp1/algorithms/awb.h > index 2a64cb60d5f4..3382b2178567 100644 > --- a/src/ipa/rkisp1/algorithms/awb.h > +++ b/src/ipa/rkisp1/algorithms/awb.h > @@ -9,6 +9,7 @@ > > #include <optional> > > +#include "libipa/awb.h" > #include "libipa/interpolator.h" > #include "libipa/vector.h" > > @@ -41,7 +42,8 @@ private: > RGB<double> calculateRgbMeans(const IPAFrameContext &frameContext, > const rkisp1_cif_isp_awb_stat *awb) const; > > - std::optional<Interpolator<Vector<double, 2>>> colourGainCurve_; > + std::unique_ptr<AwbAlgorithm> awbAlgo_; > + > bool rgbMode_; > }; >
Hi Dan, Thank you for the review. On Mon, Jan 20, 2025 at 11:59:56AM +0000, Dan Scally wrote: > Hi Stefan > > On 09/01/2025 11:53, Stefan Klug wrote: > > Now that libipa contains a grey world algorithm, use that. > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > --- > > src/ipa/rkisp1/algorithms/awb.cpp | 72 ++++++++++++++++++++----------- > > src/ipa/rkisp1/algorithms/awb.h | 4 +- > > 2 files changed, 49 insertions(+), 27 deletions(-) > > > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > > index f6449565834b..42a4784998bc 100644 > > --- a/src/ipa/rkisp1/algorithms/awb.cpp > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > > @@ -16,6 +16,7 @@ > > #include <libcamera/ipa/core_ipa_interface.h> > > +#include "libipa/awb_grey.h" > > #include "libipa/colours.h" > > /** > > @@ -40,6 +41,28 @@ constexpr int32_t kDefaultColourTemperature = 5000; > > /* Minimum mean value below which AWB can't operate. */ > > constexpr double kMeanMinThreshold = 2.0; > > +class RkISP1AwbStats : public AwbStats > > +{ > > +public: > > + RkISP1AwbStats(const RGB<double> &rgbMeans) > > + : rgbMeans_(rgbMeans) {} > > + > > + double computeColourError([[maybe_unused]] const RGB<double> &gains) const override > > + { > > + LOG(RkISP1Awb, Error) > > + << "RkISP1AwbStats::computeColourError is not implemented"; > > + return 0.0; > > + } > If it's optional, perhaps rather than a pure virtual function it should have > this "not implemented" variant in the base class? Yes that would be possible. This was merely to get the patch series into logical blocks. It is removed in patch 10/11. I believe the "not implemented" shouldn't be a default case, so that the IPA implementation needs to take active action if something should not be implemented. > > + > > + RGB<double> getRGBMeans() const override > > + { > > + return rgbMeans_; > > + }; > > + > > +private: > > + RGB<double> rgbMeans_; > > +}; > > + > > Awb::Awb() > > : rgbMode_(false) > > { > > @@ -55,15 +78,12 @@ int Awb::init(IPAContext &context, const YamlObject &tuningData) > > kMaxColourTemperature, > > kDefaultColourTemperature); > > - Interpolator<Vector<double, 2>> gainCurve; > > - int ret = gainCurve.readYaml(tuningData["colourGains"], "ct", "gains"); > > - if (ret < 0) > > - LOG(RkISP1Awb, Warning) > > - << "Failed to parse 'colourGains' " > > - << "parameter from tuning file; " > > - << "manual colour temperature will not work properly"; > > - else > > - colourGainCurve_ = gainCurve; > > + awbAlgo_ = std::make_unique<AwbGrey>(); > > + int ret = awbAlgo_->init(tuningData); > > + if (ret) { > > + LOG(RkISP1Awb, Error) << "Failed to init awb algorithm"; > > + return ret; > > + } > I'm a bit surprised to see a pointer to the base class, I was expecting this > class to derive the other one (and just call the base class init() here). > Why'd you choose to do it this way? The reason was that the top level (lets' call it RkISPAwb) algorithm should support both cases grey world and bayes and be able to switch based on the tuning file. I like the clear separation of these two algorithms as it keeps the door open to easily improve by adding another one without breaking the old ones. We could add a "master-AWB" algorithm that contains the switching/instantiation logic and add that to libipa. This could then be subclassed by RkISPAwb. I don't know exactly if it is worth it. It depends a bit on how we see libipa: a) the place to put some often used building blocks for IPAs or b) the place where we put as much of the IPA logic as we can so that all IPAs behave the same eventually. This implements a) but b) could later be added on top. What do you think? Cheers, Stefan > > return 0; > > } > > @@ -128,10 +148,10 @@ void Awb::queueRequest(IPAContext &context, > > * This will be fixed with the bayes AWB algorithm. > > */ > > update = true; > > - } else if (colourTemperature && colourGainCurve_) { > > - const auto &gains = colourGainCurve_->getInterpolated(*colourTemperature); > > - awb.gains.manual.r() = gains[0]; > > - awb.gains.manual.b() = gains[1]; > > + } else if (colourTemperature) { > > + const auto &gains = awbAlgo_->gainsFromColourTemperature(*colourTemperature); > > + awb.gains.manual.r() = gains.r(); > > + awb.gains.manual.b() = gains.b(); > > awb.temperatureK = *colourTemperature; > > update = true; > > } > > @@ -251,33 +271,33 @@ void Awb::process(IPAContext &context, > > rgbMeans.b() < kMeanMinThreshold) > > return; > > - activeState.awb.temperatureK = estimateCCT(rgbMeans); > > + /* > > + * \Todo: Hardcode lux to a fixed value, until an estimation is > > + * implemented. > > + */ > > + int lux = 1000; > > + > > + RkISP1AwbStats awbStats{ rgbMeans }; > > + AwbResult r = awbAlgo_->calculateAwb(awbStats, lux); > A more descriptive variable name would be slightly better I think; "r" in > this context makes me think "red" not "results". > > + > > + activeState.awb.temperatureK = r.colourTemperature; > > /* Metadata shall contain the up to date measurement */ > > metadata.set(controls::ColourTemperature, activeState.awb.temperatureK); > > - /* > > - * Estimate the red and blue gains to apply in a grey world. The green > > - * gain is hardcoded to 1.0. Avoid divisions by zero by clamping the > > - * divisor to a minimum value of 1.0. > > - */ > > - RGB<double> gains({ rgbMeans.g() / std::max(rgbMeans.r(), 1.0), > > - 1.0, > > - rgbMeans.g() / std::max(rgbMeans.b(), 1.0) }); > > - > > /* > > * Clamp the gain values to the hardware, which expresses gains as Q2.8 > > * unsigned integer values. Set the minimum just above zero to avoid > > * divisions by zero when computing the raw means in subsequent > > * iterations. > > */ > > - gains = gains.max(1.0 / 256).min(1023.0 / 256); > > + r.gains = r.gains.max(1.0 / 256).min(1023.0 / 256); > > /* Filter the values to avoid oscillations. */ > > double speed = 0.2; > > - gains = gains * speed + activeState.awb.gains.automatic * (1 - speed); > > + r.gains = r.gains * speed + activeState.awb.gains.automatic * (1 - speed); > > - activeState.awb.gains.automatic = gains; > > + activeState.awb.gains.automatic = r.gains; > > LOG(RkISP1Awb, Debug) > > << std::showpoint > > diff --git a/src/ipa/rkisp1/algorithms/awb.h b/src/ipa/rkisp1/algorithms/awb.h > > index 2a64cb60d5f4..3382b2178567 100644 > > --- a/src/ipa/rkisp1/algorithms/awb.h > > +++ b/src/ipa/rkisp1/algorithms/awb.h > > @@ -9,6 +9,7 @@ > > #include <optional> > > +#include "libipa/awb.h" > > #include "libipa/interpolator.h" > > #include "libipa/vector.h" > > @@ -41,7 +42,8 @@ private: > > RGB<double> calculateRgbMeans(const IPAFrameContext &frameContext, > > const rkisp1_cif_isp_awb_stat *awb) const; > > - std::optional<Interpolator<Vector<double, 2>>> colourGainCurve_; > > + std::unique_ptr<AwbAlgorithm> awbAlgo_; > > + > > bool rgbMode_; > > };
Morning Stefan On 21/01/2025 06:08, Stefan Klug wrote: > Hi Dan, > > Thank you for the review. > > On Mon, Jan 20, 2025 at 11:59:56AM +0000, Dan Scally wrote: >> Hi Stefan >> >> On 09/01/2025 11:53, Stefan Klug wrote: >>> Now that libipa contains a grey world algorithm, use that. >>> >>> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> >>> --- >>> src/ipa/rkisp1/algorithms/awb.cpp | 72 ++++++++++++++++++++----------- >>> src/ipa/rkisp1/algorithms/awb.h | 4 +- >>> 2 files changed, 49 insertions(+), 27 deletions(-) >>> >>> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp >>> index f6449565834b..42a4784998bc 100644 >>> --- a/src/ipa/rkisp1/algorithms/awb.cpp >>> +++ b/src/ipa/rkisp1/algorithms/awb.cpp >>> @@ -16,6 +16,7 @@ >>> #include <libcamera/ipa/core_ipa_interface.h> >>> +#include "libipa/awb_grey.h" >>> #include "libipa/colours.h" >>> /** >>> @@ -40,6 +41,28 @@ constexpr int32_t kDefaultColourTemperature = 5000; >>> /* Minimum mean value below which AWB can't operate. */ >>> constexpr double kMeanMinThreshold = 2.0; >>> +class RkISP1AwbStats : public AwbStats >>> +{ >>> +public: >>> + RkISP1AwbStats(const RGB<double> &rgbMeans) >>> + : rgbMeans_(rgbMeans) {} >>> + >>> + double computeColourError([[maybe_unused]] const RGB<double> &gains) const override >>> + { >>> + LOG(RkISP1Awb, Error) >>> + << "RkISP1AwbStats::computeColourError is not implemented"; >>> + return 0.0; >>> + } >> If it's optional, perhaps rather than a pure virtual function it should have >> this "not implemented" variant in the base class? > Yes that would be possible. This was merely to get the patch series into > logical blocks. It is removed in patch 10/11. I believe the "not > implemented" shouldn't be a default case, so that the IPA implementation > needs to take active action if something should not be implemented. Alright, works for me > >>> + >>> + RGB<double> getRGBMeans() const override >>> + { >>> + return rgbMeans_; >>> + }; >>> + >>> +private: >>> + RGB<double> rgbMeans_; >>> +}; >>> + >>> Awb::Awb() >>> : rgbMode_(false) >>> { >>> @@ -55,15 +78,12 @@ int Awb::init(IPAContext &context, const YamlObject &tuningData) >>> kMaxColourTemperature, >>> kDefaultColourTemperature); >>> - Interpolator<Vector<double, 2>> gainCurve; >>> - int ret = gainCurve.readYaml(tuningData["colourGains"], "ct", "gains"); >>> - if (ret < 0) >>> - LOG(RkISP1Awb, Warning) >>> - << "Failed to parse 'colourGains' " >>> - << "parameter from tuning file; " >>> - << "manual colour temperature will not work properly"; >>> - else >>> - colourGainCurve_ = gainCurve; >>> + awbAlgo_ = std::make_unique<AwbGrey>(); >>> + int ret = awbAlgo_->init(tuningData); >>> + if (ret) { >>> + LOG(RkISP1Awb, Error) << "Failed to init awb algorithm"; >>> + return ret; >>> + } >> I'm a bit surprised to see a pointer to the base class, I was expecting this >> class to derive the other one (and just call the base class init() here). >> Why'd you choose to do it this way? > The reason was that the top level (lets' call it RkISPAwb) algorithm > should support both cases grey world and bayes and be able to switch > based on the tuning file. Yes I encountered that when I reached the later patches - maybe I should read the whole series before commenting on anything :D. It's different but better than how I was expecting it to work, so it's good. > I like the clear separation of these two > algorithms as it keeps the door open to easily improve by adding another > one without breaking the old ones. > > We could add a "master-AWB" algorithm that contains the > switching/instantiation logic and add that to libipa. This could then be > subclassed by RkISPAwb. I don't know exactly if it is worth it. It > depends a bit on how we see libipa: a) the place to put some often used > building blocks for IPAs or b) the place where we put as much of the IPA > logic as we can so that all IPAs behave the same eventually. This > implements a) but b) could later be added on top. What do you think? Personally I think b is desirable, but I don't think it needs to be done right now - particularly since no other IPA to my knowledge has multiple implementations of the same algorithm yet. So, all good by me: Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> > > Cheers, > Stefan > >>> return 0; >>> } >>> @@ -128,10 +148,10 @@ void Awb::queueRequest(IPAContext &context, >>> * This will be fixed with the bayes AWB algorithm. >>> */ >>> update = true; >>> - } else if (colourTemperature && colourGainCurve_) { >>> - const auto &gains = colourGainCurve_->getInterpolated(*colourTemperature); >>> - awb.gains.manual.r() = gains[0]; >>> - awb.gains.manual.b() = gains[1]; >>> + } else if (colourTemperature) { >>> + const auto &gains = awbAlgo_->gainsFromColourTemperature(*colourTemperature); >>> + awb.gains.manual.r() = gains.r(); >>> + awb.gains.manual.b() = gains.b(); >>> awb.temperatureK = *colourTemperature; >>> update = true; >>> } >>> @@ -251,33 +271,33 @@ void Awb::process(IPAContext &context, >>> rgbMeans.b() < kMeanMinThreshold) >>> return; >>> - activeState.awb.temperatureK = estimateCCT(rgbMeans); >>> + /* >>> + * \Todo: Hardcode lux to a fixed value, until an estimation is >>> + * implemented. >>> + */ >>> + int lux = 1000; >>> + >>> + RkISP1AwbStats awbStats{ rgbMeans }; >>> + AwbResult r = awbAlgo_->calculateAwb(awbStats, lux); >> A more descriptive variable name would be slightly better I think; "r" in >> this context makes me think "red" not "results". >>> + >>> + activeState.awb.temperatureK = r.colourTemperature; >>> /* Metadata shall contain the up to date measurement */ >>> metadata.set(controls::ColourTemperature, activeState.awb.temperatureK); >>> - /* >>> - * Estimate the red and blue gains to apply in a grey world. The green >>> - * gain is hardcoded to 1.0. Avoid divisions by zero by clamping the >>> - * divisor to a minimum value of 1.0. >>> - */ >>> - RGB<double> gains({ rgbMeans.g() / std::max(rgbMeans.r(), 1.0), >>> - 1.0, >>> - rgbMeans.g() / std::max(rgbMeans.b(), 1.0) }); >>> - >>> /* >>> * Clamp the gain values to the hardware, which expresses gains as Q2.8 >>> * unsigned integer values. Set the minimum just above zero to avoid >>> * divisions by zero when computing the raw means in subsequent >>> * iterations. >>> */ >>> - gains = gains.max(1.0 / 256).min(1023.0 / 256); >>> + r.gains = r.gains.max(1.0 / 256).min(1023.0 / 256); >>> /* Filter the values to avoid oscillations. */ >>> double speed = 0.2; >>> - gains = gains * speed + activeState.awb.gains.automatic * (1 - speed); >>> + r.gains = r.gains * speed + activeState.awb.gains.automatic * (1 - speed); >>> - activeState.awb.gains.automatic = gains; >>> + activeState.awb.gains.automatic = r.gains; >>> LOG(RkISP1Awb, Debug) >>> << std::showpoint >>> diff --git a/src/ipa/rkisp1/algorithms/awb.h b/src/ipa/rkisp1/algorithms/awb.h >>> index 2a64cb60d5f4..3382b2178567 100644 >>> --- a/src/ipa/rkisp1/algorithms/awb.h >>> +++ b/src/ipa/rkisp1/algorithms/awb.h >>> @@ -9,6 +9,7 @@ >>> #include <optional> >>> +#include "libipa/awb.h" >>> #include "libipa/interpolator.h" >>> #include "libipa/vector.h" >>> @@ -41,7 +42,8 @@ private: >>> RGB<double> calculateRgbMeans(const IPAFrameContext &frameContext, >>> const rkisp1_cif_isp_awb_stat *awb) const; >>> - std::optional<Interpolator<Vector<double, 2>>> colourGainCurve_; >>> + std::unique_ptr<AwbAlgorithm> awbAlgo_; >>> + >>> bool rgbMode_; >>> };
diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp index f6449565834b..42a4784998bc 100644 --- a/src/ipa/rkisp1/algorithms/awb.cpp +++ b/src/ipa/rkisp1/algorithms/awb.cpp @@ -16,6 +16,7 @@ #include <libcamera/ipa/core_ipa_interface.h> +#include "libipa/awb_grey.h" #include "libipa/colours.h" /** @@ -40,6 +41,28 @@ constexpr int32_t kDefaultColourTemperature = 5000; /* Minimum mean value below which AWB can't operate. */ constexpr double kMeanMinThreshold = 2.0; +class RkISP1AwbStats : public AwbStats +{ +public: + RkISP1AwbStats(const RGB<double> &rgbMeans) + : rgbMeans_(rgbMeans) {} + + double computeColourError([[maybe_unused]] const RGB<double> &gains) const override + { + LOG(RkISP1Awb, Error) + << "RkISP1AwbStats::computeColourError is not implemented"; + return 0.0; + } + + RGB<double> getRGBMeans() const override + { + return rgbMeans_; + }; + +private: + RGB<double> rgbMeans_; +}; + Awb::Awb() : rgbMode_(false) { @@ -55,15 +78,12 @@ int Awb::init(IPAContext &context, const YamlObject &tuningData) kMaxColourTemperature, kDefaultColourTemperature); - Interpolator<Vector<double, 2>> gainCurve; - int ret = gainCurve.readYaml(tuningData["colourGains"], "ct", "gains"); - if (ret < 0) - LOG(RkISP1Awb, Warning) - << "Failed to parse 'colourGains' " - << "parameter from tuning file; " - << "manual colour temperature will not work properly"; - else - colourGainCurve_ = gainCurve; + awbAlgo_ = std::make_unique<AwbGrey>(); + int ret = awbAlgo_->init(tuningData); + if (ret) { + LOG(RkISP1Awb, Error) << "Failed to init awb algorithm"; + return ret; + } return 0; } @@ -128,10 +148,10 @@ void Awb::queueRequest(IPAContext &context, * This will be fixed with the bayes AWB algorithm. */ update = true; - } else if (colourTemperature && colourGainCurve_) { - const auto &gains = colourGainCurve_->getInterpolated(*colourTemperature); - awb.gains.manual.r() = gains[0]; - awb.gains.manual.b() = gains[1]; + } else if (colourTemperature) { + const auto &gains = awbAlgo_->gainsFromColourTemperature(*colourTemperature); + awb.gains.manual.r() = gains.r(); + awb.gains.manual.b() = gains.b(); awb.temperatureK = *colourTemperature; update = true; } @@ -251,33 +271,33 @@ void Awb::process(IPAContext &context, rgbMeans.b() < kMeanMinThreshold) return; - activeState.awb.temperatureK = estimateCCT(rgbMeans); + /* + * \Todo: Hardcode lux to a fixed value, until an estimation is + * implemented. + */ + int lux = 1000; + + RkISP1AwbStats awbStats{ rgbMeans }; + AwbResult r = awbAlgo_->calculateAwb(awbStats, lux); + + activeState.awb.temperatureK = r.colourTemperature; /* Metadata shall contain the up to date measurement */ metadata.set(controls::ColourTemperature, activeState.awb.temperatureK); - /* - * Estimate the red and blue gains to apply in a grey world. The green - * gain is hardcoded to 1.0. Avoid divisions by zero by clamping the - * divisor to a minimum value of 1.0. - */ - RGB<double> gains({ rgbMeans.g() / std::max(rgbMeans.r(), 1.0), - 1.0, - rgbMeans.g() / std::max(rgbMeans.b(), 1.0) }); - /* * Clamp the gain values to the hardware, which expresses gains as Q2.8 * unsigned integer values. Set the minimum just above zero to avoid * divisions by zero when computing the raw means in subsequent * iterations. */ - gains = gains.max(1.0 / 256).min(1023.0 / 256); + r.gains = r.gains.max(1.0 / 256).min(1023.0 / 256); /* Filter the values to avoid oscillations. */ double speed = 0.2; - gains = gains * speed + activeState.awb.gains.automatic * (1 - speed); + r.gains = r.gains * speed + activeState.awb.gains.automatic * (1 - speed); - activeState.awb.gains.automatic = gains; + activeState.awb.gains.automatic = r.gains; LOG(RkISP1Awb, Debug) << std::showpoint diff --git a/src/ipa/rkisp1/algorithms/awb.h b/src/ipa/rkisp1/algorithms/awb.h index 2a64cb60d5f4..3382b2178567 100644 --- a/src/ipa/rkisp1/algorithms/awb.h +++ b/src/ipa/rkisp1/algorithms/awb.h @@ -9,6 +9,7 @@ #include <optional> +#include "libipa/awb.h" #include "libipa/interpolator.h" #include "libipa/vector.h" @@ -41,7 +42,8 @@ private: RGB<double> calculateRgbMeans(const IPAFrameContext &frameContext, const rkisp1_cif_isp_awb_stat *awb) const; - std::optional<Interpolator<Vector<double, 2>>> colourGainCurve_; + std::unique_ptr<AwbAlgorithm> awbAlgo_; + bool rgbMode_; };
Now that libipa contains a grey world algorithm, use that. Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- src/ipa/rkisp1/algorithms/awb.cpp | 72 ++++++++++++++++++++----------- src/ipa/rkisp1/algorithms/awb.h | 4 +- 2 files changed, 49 insertions(+), 27 deletions(-)