[PATCH v1 07/11] ipa: rkisp1: Use grey world algorithm from libipa
Stefan Klug
stefan.klug at ideasonboard.com
Tue Jan 21 07:08:12 CET 2025
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 at 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_;
> > };
More information about the libcamera-devel
mailing list