| Message ID | 20260615-libipa-algorithms-v1-4-e949c937422e@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Jacopo Mondi (2026-06-15 15:05:29) > From: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Port the SoftISP Awb algorithm to use the new libipa implementation > of AwbAlgorithm. > > The awbAlgo_ class member is initialized with the Q<2, 8> type even if It gets instantiated with a UQ<4, 8>. > there is no physical register representation for SoftISP. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> I know this originated from me, but I think it's been updated a bit for the new implementations so with the comment above fixed: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> And also the same generic worry about float/double conversions - but that's more global to the series. > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > .../internal/software_isp/debayer_params.h | 2 +- > src/ipa/simple/algorithms/awb.cpp | 112 ++++++++++++++------- > src/ipa/simple/algorithms/awb.h | 29 ++++++ > src/ipa/simple/algorithms/ccm.cpp | 2 +- > src/ipa/simple/ipa_context.h | 14 +-- > src/libcamera/software_isp/debayer_cpu.cpp | 2 +- > 6 files changed, 117 insertions(+), 44 deletions(-) > > diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h > index 6772b43bced4..b46bbd7a8273 100644 > --- a/include/libcamera/internal/software_isp/debayer_params.h > +++ b/include/libcamera/internal/software_isp/debayer_params.h > @@ -24,7 +24,7 @@ struct DebayerParams { > RGB<float> blackLevel = RGB<float>({ 0.0, 0.0, 0.0 }); > float gamma = 1.0; > float contrastExp = 1.0; > - RGB<float> gains = RGB<float>({ 1.0, 1.0, 1.0 }); > + RGB<double> gains = RGB<double>({ 1.0, 1.0, 1.0 }); > }; > > } /* namespace libcamera */ > diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp > index 05155c83d172..01323d9779c5 100644 > --- a/src/ipa/simple/algorithms/awb.cpp > +++ b/src/ipa/simple/algorithms/awb.cpp > @@ -15,7 +15,6 @@ > #include <libcamera/control_ids.h> > > #include "libipa/colours.h" > -#include "simple/ipa_context.h" > > namespace libcamera { > > @@ -23,41 +22,79 @@ LOG_DEFINE_CATEGORY(IPASoftAwb) > > namespace ipa::soft::algorithms { > > +/* > + * \todo Replace it with a proper Lux algorithm > + */ > +static constexpr unsigned int kDefaultLux = 500; > + > +class SimpleAwbStats final : public AwbStats > +{ > +public: > + SimpleAwbStats() {} > + SimpleAwbStats(const RGB<double> &rgbMeans) > + : AwbStats(rgbMeans) > + { > + } > + > + /* Minimum mean value below which AWB can't operate. */ > + double minColourValue() const override > + { > + return 0.2; > + } > +}; > + > +/** > + * \copydoc libcamera::ipa::Algorithm::init > + */ > +int Awb::init(IPAContext &context, const ValueNode &tuningData) > +{ > + return awbAlgo_.init(tuningData, context.ctrlMap); > +} > + > +/** > + * \copydoc libcamera::ipa::Algorithm::configure > + */ > int Awb::configure(IPAContext &context, > [[maybe_unused]] const IPAConfigInfo &configInfo) > { > - auto &gains = context.activeState.awb.gains; > - gains = { { 1.0, 1.0, 1.0 } }; > + return awbAlgo_.configure(context.activeState.awb, > + context.configuration.awb); > +} > > - return 0; > +/** > + * \copydoc libcamera::ipa::Algorithm::queueRequest > + */ > +void Awb::queueRequest(IPAContext &context, > + const uint32_t frame, > + IPAFrameContext &frameContext, > + const ControlList &controls) > +{ > + awbAlgo_.queueRequest(context.activeState.awb, frame, frameContext.awb, > + controls); > } > > +/** > + * \copydoc libcamera::ipa::Algorithm::prepare > + */ > void Awb::prepare(IPAContext &context, > [[maybe_unused]] const uint32_t frame, > IPAFrameContext &frameContext, > DebayerParams *params) > { > - auto &gains = context.activeState.awb.gains; > + awbAlgo_.prepare(context.activeState.awb, frameContext.awb); > > - frameContext.gains = gains; > - params->gains = gains; > + params->gains = frameContext.awb.gains; > } > > -void Awb::process(IPAContext &context, > - [[maybe_unused]] const uint32_t frame, > - IPAFrameContext &frameContext, > - const SwIspStats *stats, > - ControlList &metadata) > +SimpleAwbStats Awb::calculateRgbMeans(IPAContext &context, > + const SwIspStats *stats) const > { > + if (!stats->valid) > + return {}; > + > const SwIspStats::Histogram &histogram = stats->yHistogram; > const uint8_t blackLevel = context.activeState.blc.level; > > - metadata.set(controls::ColourGains, { frameContext.gains.r(), > - frameContext.gains.b() }); > - > - if (!stats->valid) > - return; > - > /* > * Black level must be subtracted to get the correct AWB ratios, they > * would be off if they were computed from the whole brightness range > @@ -67,30 +104,37 @@ void Awb::process(IPAContext &context, > histogram.begin(), histogram.end(), uint64_t(0)); > const uint64_t offset = blackLevel * nPixels; > const uint64_t minValid = 1; > + > /* > * Make sure the sums are at least minValid, while preventing unsigned > * integer underflow. > */ > const RGB<uint64_t> sum = stats->sum_.max(offset + minValid) - offset; > > + RGB<double> rgbMeans = { { static_cast<double>(sum.r() / nPixels), > + static_cast<double>(sum.g() / nPixels), > + static_cast<double>(sum.b() / nPixels) } }; > + > /* > - * Calculate red and blue gains for AWB. > - * Clamp max gain at 4.0, this also avoids 0 division. > + * \todo Determine the minimum allowed thresholds from the mean > + * but we currently have the sum - not the mean value! > + * > + * Currently set to SimpleAwbStats::minColourValue() = 0.2. > */ > - auto &gains = context.activeState.awb.gains; > - gains = { { > - sum.r() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.r(), > - 1.0, > - sum.b() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.b(), > - } }; > - > - RGB<double> rgbGains{ { 1 / gains.r(), 1 / gains.g(), 1 / gains.b() } }; > - context.activeState.awb.temperatureK = estimateCCT(rgbGains); > - metadata.set(controls::ColourTemperature, context.activeState.awb.temperatureK); > - > - LOG(IPASoftAwb, Debug) > - << "gain R/B: " << gains << "; temperature: " > - << context.activeState.awb.temperatureK; > + return SimpleAwbStats(rgbMeans); > +} > + > +/** > + * \copydoc libcamera::ipa::Algorithm::process > + */ > +void Awb::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > + IPAFrameContext &frameContext, const SwIspStats *stats, > + ControlList &metadata) > +{ > + SimpleAwbStats awbStats = calculateRgbMeans(context, stats); > + > + awbAlgo_.process(context.activeState.awb, frameContext.awb, awbStats, > + kDefaultLux, metadata); > } > > REGISTER_IPA_ALGORITHM(Awb, "Awb") > diff --git a/src/ipa/simple/algorithms/awb.h b/src/ipa/simple/algorithms/awb.h > index ad993f39c180..cb36cd092e51 100644 > --- a/src/ipa/simple/algorithms/awb.h > +++ b/src/ipa/simple/algorithms/awb.h > @@ -7,19 +7,37 @@ > > #pragma once > > +#include <libcamera/controls.h> > + > +#include "libcamera/internal/software_isp/debayer_params.h" > +#include "libcamera/internal/value_node.h" > + > +#include "libipa/awb.h" > +#include "libipa/fixedpoint.h" > +#include "simple/ipa_context.h" > + > #include "algorithm.h" > > namespace libcamera { > > namespace ipa::soft::algorithms { > > +class SimpleAwbStats; > + > class Awb : public Algorithm > { > public: > Awb() = default; > ~Awb() = default; > > + int init(IPAContext &context, > + const ValueNode &tuningData) override; > int configure(IPAContext &context, const IPAConfigInfo &configInfo) override; > + > + void queueRequest(IPAContext &context, > + [[maybe_unused]] const uint32_t frame, > + IPAFrameContext &frameContext, > + const ControlList &controls) override; > void prepare(IPAContext &context, > const uint32_t frame, > IPAFrameContext &frameContext, > @@ -29,6 +47,17 @@ public: > IPAFrameContext &frameContext, > const SwIspStats *stats, > ControlList &metadata) override; > + > +private: > + SimpleAwbStats calculateRgbMeans(IPAContext &context, > + const SwIspStats *stats) const; > + > + /* > + * There actually is no Q register format for SoftISP, but allow the > + * colour gains to range in the [0.0f, 15.999f] interval, which seems > + * reasonable. > + */ > + AwbAlgorithm<UQ<4, 8>> awbAlgo_; > }; > > } /* namespace ipa::soft::algorithms */ > diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp > index ace9c35dc462..ff37c718c6e4 100644 > --- a/src/ipa/simple/algorithms/ccm.cpp > +++ b/src/ipa/simple/algorithms/ccm.cpp > @@ -44,7 +44,7 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const ValueNode &tuningData) > void Ccm::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame, > IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params) > { > - const unsigned int ct = context.activeState.awb.temperatureK; > + const unsigned int ct = frameContext.awb.temperatureK; > > /* Change CCM only on bigger temperature changes. */ > if (!currentCcm_ || > diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h > index 8ccfacb46a59..0646f42d5618 100644 > --- a/src/ipa/simple/ipa_context.h > +++ b/src/ipa/simple/ipa_context.h > @@ -16,6 +16,7 @@ > #include "libcamera/internal/matrix.h" > #include "libcamera/internal/vector.h" > > +#include <libipa/awb.h> > #include <libipa/fc_queue.h> > > #include "core_ipa_interface.h" > @@ -25,6 +26,8 @@ namespace libcamera { > namespace ipa::soft { > > struct IPASessionConfiguration { > + ipa::awb::Session awb; > + > struct { > int32_t exposureMin, exposureMax; > double againMin, againMax, again10, againMinStep; > @@ -36,6 +39,8 @@ struct IPASessionConfiguration { > }; > > struct IPAActiveState { > + ipa::awb::ActiveState awb; > + > struct { > int32_t exposure; > double again; > @@ -48,11 +53,6 @@ struct IPAActiveState { > double lastGain; > } blc; > > - struct { > - RGB<float> gains; > - unsigned int temperatureK; > - } awb; > - > Matrix<float, 3, 3> combinedMatrix; > > struct { > @@ -64,6 +64,8 @@ struct IPAActiveState { > }; > > struct IPAFrameContext : public FrameContext { > + ipa::awb::FrameContext awb; > + > Matrix<float, 3, 3> ccm; > > struct { > @@ -71,8 +73,6 @@ struct IPAFrameContext : public FrameContext { > double gain; > } sensor; > > - RGB<float> gains; > - > float gamma; > std::optional<float> contrast; > std::optional<float> saturation; > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp > index d2596d32bbcd..fc171f874833 100644 > --- a/src/libcamera/software_isp/debayer_cpu.cpp > +++ b/src/libcamera/software_isp/debayer_cpu.cpp > @@ -1051,7 +1051,7 @@ void DebayerCpu::updateLookupTables(const DebayerParams ¶ms) > auto &blue = swapRedBlueGains_ ? red_ : blue_; > for (unsigned int i = 0; i < kRGBLookupSize; i++) { > /* Apply gamma after gain! */ > - const RGB<float> lutGains = (gains * i / div).min(gammaTableSize - 1); > + const RGB<double> lutGains = (gains * i / div).min(gammaTableSize - 1); > red[i] = gammaTable_[static_cast<unsigned int>(lutGains.r())]; > green[i] = gammaTable_[static_cast<unsigned int>(lutGains.g())]; > blue[i] = gammaTable_[static_cast<unsigned int>(lutGains.b())]; > > -- > 2.54.0 >
Hi,
just a quick note from testing the series with software ISP. No later
than with this patch:
- I get this error when running `cam -c1 -C -D':
[6:44:58.389852507] [8392] ERROR Awb awb_grey.cpp:108 No gains defined
- Camshark reports the following errors and there are no controls
(contrast, gamma) present in camshark:
[6:45:28.924530053] [8436] ERROR Awb awb_grey.cpp:108 No gains defined
[6:45:28.924557223] [8436] INFO IPASoft soft_simple.cpp:258 IPASoft: Exposure 4-3522, gain 1-10.6667 (0.0966667)
[6:45:28.924618610] [8436] INFO SoftwareIsp software_isp.cpp:280 Input 3280x2464-RGGB-10 stride 6560
INFO:server:cam1-stream0: stream config 3276x2464-ABGR8888/sRGB <libcamera.ColorSpace 'sRGB'>
Traceback (most recent call last):
File "/home/pdm/git/gitlab.freedesktop.org/camera/camshark/venv/lib64/python3.13/site-packages/pyqtgraph/parametertree/Parameter.py", line 785, in child
param = self.names[names[0]]
~~~~~~~~~~^^^^^^^^^^
KeyError: 'ColourGains'
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
File "/home/pdm/git/gitlab.freedesktop.org/camera/camshark/src/camshark/camshark.py", line 578, in on_camera_info
param = controlsParam.child(name)
File "/home/pdm/git/gitlab.freedesktop.org/camera/camshark/venv/lib64/python3.13/site-packages/pyqtgraph/parametertree/Parameter.py", line 787, in child
raise KeyError(f"Parameter {self.name()} has no child named {names[0]}") from e
KeyError: 'Parameter Controls has no child named ColourGains'
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/home/pdm/git/gitlab.freedesktop.org/camera/camshark/src/camshark/camshark.py", line 580, in on_camera_info
p = self.create_parameter_for_control(name, desc)
File "/home/pdm/git/gitlab.freedesktop.org/camera/camshark/src/camshark/camshark.py", line 536, in create_parameter_for_control
if desc['default'] not in span:
^^^^^^^^^^^^^^^^^^^^^^^^^^^
ValueError: operands could not be broadcast together with shapes (201,) (2,)
cam1-stream0: Allocated 4 buffers
I can look what's wrong but I may not have the opportunity to do so this
week, so reporting it for now at least.
Milan Zamazal <mzamazal@redhat.com> writes: > Hi, > > just a quick note from testing the series with software ISP. No later > than with this patch: > > - I get this error when running `cam -c1 -C -D': > > [6:44:58.389852507] [8392] ERROR Awb awb_grey.cpp:108 No gains defined This is apparently because AwbAlgorithmBase::configure calls impl_->gainsFromColourTemperature(kDefaultColourTemperature); and AwbGrey::gainsFromColourTemperature reports this error when there is no gain curve. At least in this case, it's OK not to have the gain curve and the log message should be omitted or Debug, I think.
Hi Milan + Stefan On Wed, Jun 17, 2026 at 01:18:19PM +0200, Milan Zamazal wrote: > Milan Zamazal <mzamazal@redhat.com> writes: > > > Hi, > > > > just a quick note from testing the series with software ISP. No later > > than with this patch: > > > > - I get this error when running `cam -c1 -C -D': > > > > [6:44:58.389852507] [8392] ERROR Awb awb_grey.cpp:108 No gains defined > > This is apparently because AwbAlgorithmBase::configure calls > > impl_->gainsFromColourTemperature(kDefaultColourTemperature); > > and AwbGrey::gainsFromColourTemperature reports this error when there is > no gain curve. At least in this case, it's OK not to have the gain > curve and the log message should be omitted or Debug, I think. > Thanks for testing. Is it enough to demote the message to Debug/Info or should we avoid returning nullopts ? /** * \brief Compute white balance gains from a colour temperature * \param[in] colourTemperature The colour temperature in Kelvin * * Compute the white balance gains from a \a colourTemperature. This function * does not take any statistics into account. It simply interpolates the colour * gains configured in the colour temperature curve. * * \return The colour gains if a colour temperature curve is available, * [1, 1, 1] otherwise. */ std::optional<RGB<double>> AwbGrey::gainsFromColourTemperature(double colourTemperature) { if (!colourGainCurve_) { LOG(Awb, Error) << "No gains defined"; return std::nullopt; } auto gains = colourGainCurve_->getInterpolated(colourTemperature); return RGB<double>{ { gains[0], 1.0, gains[1] } }; } Also, the \return documentation doesn't seem to match the implementation.
On Wed, Jun 17, 2026 at 01:30:07PM +0200, Jacopo Mondi wrote: > Hi Milan > + Stefan > > On Wed, Jun 17, 2026 at 01:18:19PM +0200, Milan Zamazal wrote: > > Milan Zamazal <mzamazal@redhat.com> writes: > > > > > Hi, > > > > > > just a quick note from testing the series with software ISP. No later > > > than with this patch: > > > > > > - I get this error when running `cam -c1 -C -D': > > > > > > [6:44:58.389852507] [8392] ERROR Awb awb_grey.cpp:108 No gains defined > > > > This is apparently because AwbAlgorithmBase::configure calls > > > > impl_->gainsFromColourTemperature(kDefaultColourTemperature); > > > > and AwbGrey::gainsFromColourTemperature reports this error when there is > > no gain curve. At least in this case, it's OK not to have the gain > > curve and the log message should be omitted or Debug, I think. > > > > Thanks for testing. > > Is it enough to demote the message to Debug/Info or should we avoid > returning nullopts ? Ah no, we should probably continue returning std::nullopt (or find other means to signal we can't interpolate gains on a colour temperature as there are no gains defined in the tuning file). Specifically in void AwbAlgorithmBase::queueRequest(awb::ActiveState &state, [[maybe_unused]] const uint32_t frame, awb::FrameContext &frameContext, const ControlList &controls) ... } else if (colourTemperature) { state.manual.temperatureK = *colourTemperature; const auto &gains = impl_->gainsFromColourTemperature(*colourTemperature); if (gains) { state.manual.gains.r() = gains->r(); state.manual.gains.b() = gains->b(); update = true; } } We shouldn't update the gains if we can't interpolate (which basically means ignoring controls::ColourTemperature). > > /** > * \brief Compute white balance gains from a colour temperature > * \param[in] colourTemperature The colour temperature in Kelvin > * > * Compute the white balance gains from a \a colourTemperature. This function > * does not take any statistics into account. It simply interpolates the colour > * gains configured in the colour temperature curve. > * > * \return The colour gains if a colour temperature curve is available, > * [1, 1, 1] otherwise. > */ > std::optional<RGB<double>> AwbGrey::gainsFromColourTemperature(double colourTemperature) > { > if (!colourGainCurve_) { > LOG(Awb, Error) << "No gains defined"; > return std::nullopt; > } > > auto gains = colourGainCurve_->getInterpolated(colourTemperature); > return RGB<double>{ { gains[0], 1.0, gains[1] } }; > } > > > Also, the \return documentation doesn't seem to match the > implementation. I should indeed change this >
Hi Jacopo, Jacopo Mondi <jacopo.mondi@ideasonboard.com> writes: > From: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Port the SoftISP Awb algorithm to use the new libipa implementation > of AwbAlgorithm. > > The awbAlgo_ class member is initialized with the Q<2, 8> type even if > there is no physical register representation for SoftISP. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > .../internal/software_isp/debayer_params.h | 2 +- > src/ipa/simple/algorithms/awb.cpp | 112 ++++++++++++++------- > src/ipa/simple/algorithms/awb.h | 29 ++++++ > src/ipa/simple/algorithms/ccm.cpp | 2 +- > src/ipa/simple/ipa_context.h | 14 +-- > src/libcamera/software_isp/debayer_cpu.cpp | 2 +- > 6 files changed, 117 insertions(+), 44 deletions(-) > > diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h > index 6772b43bced4..b46bbd7a8273 100644 > --- a/include/libcamera/internal/software_isp/debayer_params.h > +++ b/include/libcamera/internal/software_isp/debayer_params.h > @@ -24,7 +24,7 @@ struct DebayerParams { > RGB<float> blackLevel = RGB<float>({ 0.0, 0.0, 0.0 }); > float gamma = 1.0; > float contrastExp = 1.0; > - RGB<float> gains = RGB<float>({ 1.0, 1.0, 1.0 }); > + RGB<double> gains = RGB<double>({ 1.0, 1.0, 1.0 }); > }; > > } /* namespace libcamera */ > diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp > index 05155c83d172..01323d9779c5 100644 > --- a/src/ipa/simple/algorithms/awb.cpp > +++ b/src/ipa/simple/algorithms/awb.cpp > @@ -15,7 +15,6 @@ > #include <libcamera/control_ids.h> > > #include "libipa/colours.h" > -#include "simple/ipa_context.h" > > namespace libcamera { > > @@ -23,41 +22,79 @@ LOG_DEFINE_CATEGORY(IPASoftAwb) > > namespace ipa::soft::algorithms { > > +/* > + * \todo Replace it with a proper Lux algorithm > + */ > +static constexpr unsigned int kDefaultLux = 500; > + > +class SimpleAwbStats final : public AwbStats > +{ > +public: > + SimpleAwbStats() {} > + SimpleAwbStats(const RGB<double> &rgbMeans) > + : AwbStats(rgbMeans) > + { > + } > + > + /* Minimum mean value below which AWB can't operate. */ > + double minColourValue() const override > + { > + return 0.2; > + } > +}; > + > +/** > + * \copydoc libcamera::ipa::Algorithm::init > + */ > +int Awb::init(IPAContext &context, const ValueNode &tuningData) > +{ > + return awbAlgo_.init(tuningData, context.ctrlMap); > +} > + > +/** > + * \copydoc libcamera::ipa::Algorithm::configure > + */ > int Awb::configure(IPAContext &context, > [[maybe_unused]] const IPAConfigInfo &configInfo) > { > - auto &gains = context.activeState.awb.gains; > - gains = { { 1.0, 1.0, 1.0 } }; > + return awbAlgo_.configure(context.activeState.awb, > + context.configuration.awb); > +} > > - return 0; > +/** > + * \copydoc libcamera::ipa::Algorithm::queueRequest > + */ > +void Awb::queueRequest(IPAContext &context, > + const uint32_t frame, > + IPAFrameContext &frameContext, > + const ControlList &controls) > +{ > + awbAlgo_.queueRequest(context.activeState.awb, frame, frameContext.awb, > + controls); > } > > +/** > + * \copydoc libcamera::ipa::Algorithm::prepare > + */ > void Awb::prepare(IPAContext &context, > [[maybe_unused]] const uint32_t frame, > IPAFrameContext &frameContext, > DebayerParams *params) > { > - auto &gains = context.activeState.awb.gains; > + awbAlgo_.prepare(context.activeState.awb, frameContext.awb); > > - frameContext.gains = gains; > - params->gains = gains; > + params->gains = frameContext.awb.gains; > } > > -void Awb::process(IPAContext &context, > - [[maybe_unused]] const uint32_t frame, > - IPAFrameContext &frameContext, > - const SwIspStats *stats, > - ControlList &metadata) > +SimpleAwbStats Awb::calculateRgbMeans(IPAContext &context, > + const SwIspStats *stats) const > { > + if (!stats->valid) > + return {}; > + > const SwIspStats::Histogram &histogram = stats->yHistogram; > const uint8_t blackLevel = context.activeState.blc.level; > > - metadata.set(controls::ColourGains, { frameContext.gains.r(), > - frameContext.gains.b() }); > - > - if (!stats->valid) > - return; > - > /* > * Black level must be subtracted to get the correct AWB ratios, they > * would be off if they were computed from the whole brightness range > @@ -67,30 +104,37 @@ void Awb::process(IPAContext &context, > histogram.begin(), histogram.end(), uint64_t(0)); > const uint64_t offset = blackLevel * nPixels; > const uint64_t minValid = 1; > + > /* > * Make sure the sums are at least minValid, while preventing unsigned > * integer underflow. > */ > const RGB<uint64_t> sum = stats->sum_.max(offset + minValid) - offset; > > + RGB<double> rgbMeans = { { static_cast<double>(sum.r() / nPixels), > + static_cast<double>(sum.g() / nPixels), > + static_cast<double>(sum.b() / nPixels) } }; > + > /* > - * Calculate red and blue gains for AWB. > - * Clamp max gain at 4.0, this also avoids 0 division. > + * \todo Determine the minimum allowed thresholds from the mean > + * but we currently have the sum - not the mean value! I don't understand the todo -- aren't rgbMeans means? Otherwise it looks good to me. I don't know what causes the camshark problem, it looks to me from the code that the controls should be initialised correctly and I cannot test/debug it now. > + * > + * Currently set to SimpleAwbStats::minColourValue() = 0.2. > */ > - auto &gains = context.activeState.awb.gains; > - gains = { { > - sum.r() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.r(), > - 1.0, > - sum.b() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.b(), > - } }; > - > - RGB<double> rgbGains{ { 1 / gains.r(), 1 / gains.g(), 1 / gains.b() } }; > - context.activeState.awb.temperatureK = estimateCCT(rgbGains); > - metadata.set(controls::ColourTemperature, context.activeState.awb.temperatureK); > - > - LOG(IPASoftAwb, Debug) > - << "gain R/B: " << gains << "; temperature: " > - << context.activeState.awb.temperatureK; > + return SimpleAwbStats(rgbMeans); > +} > + > +/** > + * \copydoc libcamera::ipa::Algorithm::process > + */ > +void Awb::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > + IPAFrameContext &frameContext, const SwIspStats *stats, > + ControlList &metadata) > +{ > + SimpleAwbStats awbStats = calculateRgbMeans(context, stats); > + > + awbAlgo_.process(context.activeState.awb, frameContext.awb, awbStats, > + kDefaultLux, metadata); > } > > REGISTER_IPA_ALGORITHM(Awb, "Awb") > diff --git a/src/ipa/simple/algorithms/awb.h b/src/ipa/simple/algorithms/awb.h > index ad993f39c180..cb36cd092e51 100644 > --- a/src/ipa/simple/algorithms/awb.h > +++ b/src/ipa/simple/algorithms/awb.h > @@ -7,19 +7,37 @@ > > #pragma once > > +#include <libcamera/controls.h> > + > +#include "libcamera/internal/software_isp/debayer_params.h" > +#include "libcamera/internal/value_node.h" > + > +#include "libipa/awb.h" > +#include "libipa/fixedpoint.h" > +#include "simple/ipa_context.h" > + > #include "algorithm.h" > > namespace libcamera { > > namespace ipa::soft::algorithms { > > +class SimpleAwbStats; > + > class Awb : public Algorithm > { > public: > Awb() = default; > ~Awb() = default; > > + int init(IPAContext &context, > + const ValueNode &tuningData) override; > int configure(IPAContext &context, const IPAConfigInfo &configInfo) override; > + > + void queueRequest(IPAContext &context, > + [[maybe_unused]] const uint32_t frame, > + IPAFrameContext &frameContext, > + const ControlList &controls) override; > void prepare(IPAContext &context, > const uint32_t frame, > IPAFrameContext &frameContext, > @@ -29,6 +47,17 @@ public: > IPAFrameContext &frameContext, > const SwIspStats *stats, > ControlList &metadata) override; > + > +private: > + SimpleAwbStats calculateRgbMeans(IPAContext &context, > + const SwIspStats *stats) const; > + > + /* > + * There actually is no Q register format for SoftISP, but allow the > + * colour gains to range in the [0.0f, 15.999f] interval, which seems > + * reasonable. > + */ > + AwbAlgorithm<UQ<4, 8>> awbAlgo_; > }; > > } /* namespace ipa::soft::algorithms */ > diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp > index ace9c35dc462..ff37c718c6e4 100644 > --- a/src/ipa/simple/algorithms/ccm.cpp > +++ b/src/ipa/simple/algorithms/ccm.cpp > @@ -44,7 +44,7 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const ValueNode &tuningData) > void Ccm::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame, > IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params) > { > - const unsigned int ct = context.activeState.awb.temperatureK; > + const unsigned int ct = frameContext.awb.temperatureK; > > /* Change CCM only on bigger temperature changes. */ > if (!currentCcm_ || > diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h > index 8ccfacb46a59..0646f42d5618 100644 > --- a/src/ipa/simple/ipa_context.h > +++ b/src/ipa/simple/ipa_context.h > @@ -16,6 +16,7 @@ > #include "libcamera/internal/matrix.h" > #include "libcamera/internal/vector.h" > > +#include <libipa/awb.h> > #include <libipa/fc_queue.h> > > #include "core_ipa_interface.h" > @@ -25,6 +26,8 @@ namespace libcamera { > namespace ipa::soft { > > struct IPASessionConfiguration { > + ipa::awb::Session awb; > + > struct { > int32_t exposureMin, exposureMax; > double againMin, againMax, again10, againMinStep; > @@ -36,6 +39,8 @@ struct IPASessionConfiguration { > }; > > struct IPAActiveState { > + ipa::awb::ActiveState awb; > + > struct { > int32_t exposure; > double again; > @@ -48,11 +53,6 @@ struct IPAActiveState { > double lastGain; > } blc; > > - struct { > - RGB<float> gains; > - unsigned int temperatureK; > - } awb; > - > Matrix<float, 3, 3> combinedMatrix; > > struct { > @@ -64,6 +64,8 @@ struct IPAActiveState { > }; > > struct IPAFrameContext : public FrameContext { > + ipa::awb::FrameContext awb; > + > Matrix<float, 3, 3> ccm; > > struct { > @@ -71,8 +73,6 @@ struct IPAFrameContext : public FrameContext { > double gain; > } sensor; > > - RGB<float> gains; > - > float gamma; > std::optional<float> contrast; > std::optional<float> saturation; > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp > index d2596d32bbcd..fc171f874833 100644 > --- a/src/libcamera/software_isp/debayer_cpu.cpp > +++ b/src/libcamera/software_isp/debayer_cpu.cpp > @@ -1051,7 +1051,7 @@ void DebayerCpu::updateLookupTables(const DebayerParams ¶ms) > auto &blue = swapRedBlueGains_ ? red_ : blue_; > for (unsigned int i = 0; i < kRGBLookupSize; i++) { > /* Apply gamma after gain! */ > - const RGB<float> lutGains = (gains * i / div).min(gammaTableSize - 1); > + const RGB<double> lutGains = (gains * i / div).min(gammaTableSize - 1); > red[i] = gammaTable_[static_cast<unsigned int>(lutGains.r())]; > green[i] = gammaTable_[static_cast<unsigned int>(lutGains.g())]; > blue[i] = gammaTable_[static_cast<unsigned int>(lutGains.b())];
Milan Zamazal <mzamazal@redhat.com> writes: > Hi Jacopo, > > Jacopo Mondi <jacopo.mondi@ideasonboard.com> writes: > >> From: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >> Port the SoftISP Awb algorithm to use the new libipa implementation >> of AwbAlgorithm. >> >> The awbAlgo_ class member is initialized with the Q<2, 8> type even if >> there is no physical register representation for SoftISP. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> >> --- >> .../internal/software_isp/debayer_params.h | 2 +- >> src/ipa/simple/algorithms/awb.cpp | 112 ++++++++++++++------- >> src/ipa/simple/algorithms/awb.h | 29 ++++++ >> src/ipa/simple/algorithms/ccm.cpp | 2 +- >> src/ipa/simple/ipa_context.h | 14 +-- >> src/libcamera/software_isp/debayer_cpu.cpp | 2 +- >> 6 files changed, 117 insertions(+), 44 deletions(-) >> >> diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h >> index 6772b43bced4..b46bbd7a8273 100644 >> --- a/include/libcamera/internal/software_isp/debayer_params.h >> +++ b/include/libcamera/internal/software_isp/debayer_params.h >> @@ -24,7 +24,7 @@ struct DebayerParams { >> RGB<float> blackLevel = RGB<float>({ 0.0, 0.0, 0.0 }); >> float gamma = 1.0; >> float contrastExp = 1.0; >> - RGB<float> gains = RGB<float>({ 1.0, 1.0, 1.0 }); >> + RGB<double> gains = RGB<double>({ 1.0, 1.0, 1.0 }); >> }; >> >> } /* namespace libcamera */ >> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp >> index 05155c83d172..01323d9779c5 100644 >> --- a/src/ipa/simple/algorithms/awb.cpp >> +++ b/src/ipa/simple/algorithms/awb.cpp >> @@ -15,7 +15,6 @@ >> #include <libcamera/control_ids.h> >> >> #include "libipa/colours.h" >> -#include "simple/ipa_context.h" >> >> namespace libcamera { >> >> @@ -23,41 +22,79 @@ LOG_DEFINE_CATEGORY(IPASoftAwb) >> >> namespace ipa::soft::algorithms { >> >> +/* >> + * \todo Replace it with a proper Lux algorithm >> + */ >> +static constexpr unsigned int kDefaultLux = 500; >> + >> +class SimpleAwbStats final : public AwbStats >> +{ >> +public: >> + SimpleAwbStats() {} >> + SimpleAwbStats(const RGB<double> &rgbMeans) >> + : AwbStats(rgbMeans) >> + { >> + } >> + >> + /* Minimum mean value below which AWB can't operate. */ >> + double minColourValue() const override >> + { >> + return 0.2; >> + } >> +}; >> + >> +/** >> + * \copydoc libcamera::ipa::Algorithm::init >> + */ >> +int Awb::init(IPAContext &context, const ValueNode &tuningData) >> +{ >> + return awbAlgo_.init(tuningData, context.ctrlMap); >> +} >> + >> +/** >> + * \copydoc libcamera::ipa::Algorithm::configure >> + */ >> int Awb::configure(IPAContext &context, >> [[maybe_unused]] const IPAConfigInfo &configInfo) >> { >> - auto &gains = context.activeState.awb.gains; >> - gains = { { 1.0, 1.0, 1.0 } }; >> + return awbAlgo_.configure(context.activeState.awb, >> + context.configuration.awb); >> +} >> >> - return 0; >> +/** >> + * \copydoc libcamera::ipa::Algorithm::queueRequest >> + */ >> +void Awb::queueRequest(IPAContext &context, >> + const uint32_t frame, >> + IPAFrameContext &frameContext, >> + const ControlList &controls) >> +{ >> + awbAlgo_.queueRequest(context.activeState.awb, frame, frameContext.awb, >> + controls); >> } >> >> +/** >> + * \copydoc libcamera::ipa::Algorithm::prepare >> + */ >> void Awb::prepare(IPAContext &context, >> [[maybe_unused]] const uint32_t frame, >> IPAFrameContext &frameContext, >> DebayerParams *params) >> { >> - auto &gains = context.activeState.awb.gains; >> + awbAlgo_.prepare(context.activeState.awb, frameContext.awb); >> >> - frameContext.gains = gains; >> - params->gains = gains; >> + params->gains = frameContext.awb.gains; >> } >> >> -void Awb::process(IPAContext &context, >> - [[maybe_unused]] const uint32_t frame, >> - IPAFrameContext &frameContext, >> - const SwIspStats *stats, >> - ControlList &metadata) >> +SimpleAwbStats Awb::calculateRgbMeans(IPAContext &context, >> + const SwIspStats *stats) const >> { >> + if (!stats->valid) >> + return {}; >> + >> const SwIspStats::Histogram &histogram = stats->yHistogram; >> const uint8_t blackLevel = context.activeState.blc.level; >> >> - metadata.set(controls::ColourGains, { frameContext.gains.r(), >> - frameContext.gains.b() }); >> - >> - if (!stats->valid) >> - return; >> - >> /* >> * Black level must be subtracted to get the correct AWB ratios, they >> * would be off if they were computed from the whole brightness range >> @@ -67,30 +104,37 @@ void Awb::process(IPAContext &context, >> histogram.begin(), histogram.end(), uint64_t(0)); >> const uint64_t offset = blackLevel * nPixels; >> const uint64_t minValid = 1; >> + >> /* >> * Make sure the sums are at least minValid, while preventing unsigned >> * integer underflow. >> */ >> const RGB<uint64_t> sum = stats->sum_.max(offset + minValid) - offset; >> >> + RGB<double> rgbMeans = { { static_cast<double>(sum.r() / nPixels), >> + static_cast<double>(sum.g() / nPixels), >> + static_cast<double>(sum.b() / nPixels) } }; >> + >> /* >> - * Calculate red and blue gains for AWB. >> - * Clamp max gain at 4.0, this also avoids 0 division. >> + * \todo Determine the minimum allowed thresholds from the mean >> + * but we currently have the sum - not the mean value! > > I don't understand the todo -- aren't rgbMeans means? > > Otherwise it looks good to me. I don't know what causes the camshark > problem, it looks to me from the code that the controls should be > initialised correctly and I cannot test/debug it now. The controls look all right. The problem is in Camshark -- 0.2.4 has some problem with ColourGains and main has problem to display images: ERROR: Failed to decode image: type object 'PixelFormats' has no attribute 'find_libcamera_name' >> + * >> + * Currently set to SimpleAwbStats::minColourValue() = 0.2. >> */ >> - auto &gains = context.activeState.awb.gains; >> - gains = { { >> - sum.r() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.r(), >> - 1.0, >> - sum.b() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.b(), >> - } }; >> - >> - RGB<double> rgbGains{ { 1 / gains.r(), 1 / gains.g(), 1 / gains.b() } }; >> - context.activeState.awb.temperatureK = estimateCCT(rgbGains); >> - metadata.set(controls::ColourTemperature, context.activeState.awb.temperatureK); >> - >> - LOG(IPASoftAwb, Debug) >> - << "gain R/B: " << gains << "; temperature: " >> - << context.activeState.awb.temperatureK; >> + return SimpleAwbStats(rgbMeans); >> +} >> + >> +/** >> + * \copydoc libcamera::ipa::Algorithm::process >> + */ >> +void Awb::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, >> + IPAFrameContext &frameContext, const SwIspStats *stats, >> + ControlList &metadata) >> +{ >> + SimpleAwbStats awbStats = calculateRgbMeans(context, stats); >> + >> + awbAlgo_.process(context.activeState.awb, frameContext.awb, awbStats, >> + kDefaultLux, metadata); >> } >> >> REGISTER_IPA_ALGORITHM(Awb, "Awb") >> diff --git a/src/ipa/simple/algorithms/awb.h b/src/ipa/simple/algorithms/awb.h >> index ad993f39c180..cb36cd092e51 100644 >> --- a/src/ipa/simple/algorithms/awb.h >> +++ b/src/ipa/simple/algorithms/awb.h >> @@ -7,19 +7,37 @@ >> >> #pragma once >> >> +#include <libcamera/controls.h> >> + >> +#include "libcamera/internal/software_isp/debayer_params.h" >> +#include "libcamera/internal/value_node.h" >> + >> +#include "libipa/awb.h" >> +#include "libipa/fixedpoint.h" >> +#include "simple/ipa_context.h" >> + >> #include "algorithm.h" >> >> namespace libcamera { >> >> namespace ipa::soft::algorithms { >> >> +class SimpleAwbStats; >> + >> class Awb : public Algorithm >> { >> public: >> Awb() = default; >> ~Awb() = default; >> >> + int init(IPAContext &context, >> + const ValueNode &tuningData) override; >> int configure(IPAContext &context, const IPAConfigInfo &configInfo) override; >> + >> + void queueRequest(IPAContext &context, >> + [[maybe_unused]] const uint32_t frame, >> + IPAFrameContext &frameContext, >> + const ControlList &controls) override; >> void prepare(IPAContext &context, >> const uint32_t frame, >> IPAFrameContext &frameContext, >> @@ -29,6 +47,17 @@ public: >> IPAFrameContext &frameContext, >> const SwIspStats *stats, >> ControlList &metadata) override; >> + >> +private: >> + SimpleAwbStats calculateRgbMeans(IPAContext &context, >> + const SwIspStats *stats) const; >> + >> + /* >> + * There actually is no Q register format for SoftISP, but allow the >> + * colour gains to range in the [0.0f, 15.999f] interval, which seems >> + * reasonable. >> + */ >> + AwbAlgorithm<UQ<4, 8>> awbAlgo_; >> }; >> >> } /* namespace ipa::soft::algorithms */ >> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp >> index ace9c35dc462..ff37c718c6e4 100644 >> --- a/src/ipa/simple/algorithms/ccm.cpp >> +++ b/src/ipa/simple/algorithms/ccm.cpp >> @@ -44,7 +44,7 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const ValueNode &tuningData) >> void Ccm::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame, >> IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params) >> { >> - const unsigned int ct = context.activeState.awb.temperatureK; >> + const unsigned int ct = frameContext.awb.temperatureK; >> >> /* Change CCM only on bigger temperature changes. */ >> if (!currentCcm_ || >> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h >> index 8ccfacb46a59..0646f42d5618 100644 >> --- a/src/ipa/simple/ipa_context.h >> +++ b/src/ipa/simple/ipa_context.h >> @@ -16,6 +16,7 @@ >> #include "libcamera/internal/matrix.h" >> #include "libcamera/internal/vector.h" >> >> +#include <libipa/awb.h> >> #include <libipa/fc_queue.h> >> >> #include "core_ipa_interface.h" >> @@ -25,6 +26,8 @@ namespace libcamera { >> namespace ipa::soft { >> >> struct IPASessionConfiguration { >> + ipa::awb::Session awb; >> + >> struct { >> int32_t exposureMin, exposureMax; >> double againMin, againMax, again10, againMinStep; >> @@ -36,6 +39,8 @@ struct IPASessionConfiguration { >> }; >> >> struct IPAActiveState { >> + ipa::awb::ActiveState awb; >> + >> struct { >> int32_t exposure; >> double again; >> @@ -48,11 +53,6 @@ struct IPAActiveState { >> double lastGain; >> } blc; >> >> - struct { >> - RGB<float> gains; >> - unsigned int temperatureK; >> - } awb; >> - >> Matrix<float, 3, 3> combinedMatrix; >> >> struct { >> @@ -64,6 +64,8 @@ struct IPAActiveState { >> }; >> >> struct IPAFrameContext : public FrameContext { >> + ipa::awb::FrameContext awb; >> + >> Matrix<float, 3, 3> ccm; >> >> struct { >> @@ -71,8 +73,6 @@ struct IPAFrameContext : public FrameContext { >> double gain; >> } sensor; >> >> - RGB<float> gains; >> - >> float gamma; >> std::optional<float> contrast; >> std::optional<float> saturation; >> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp >> index d2596d32bbcd..fc171f874833 100644 >> --- a/src/libcamera/software_isp/debayer_cpu.cpp >> +++ b/src/libcamera/software_isp/debayer_cpu.cpp >> @@ -1051,7 +1051,7 @@ void DebayerCpu::updateLookupTables(const DebayerParams ¶ms) >> auto &blue = swapRedBlueGains_ ? red_ : blue_; >> for (unsigned int i = 0; i < kRGBLookupSize; i++) { >> /* Apply gamma after gain! */ >> - const RGB<float> lutGains = (gains * i / div).min(gammaTableSize - 1); >> + const RGB<double> lutGains = (gains * i / div).min(gammaTableSize - 1); >> red[i] = gammaTable_[static_cast<unsigned int>(lutGains.r())]; >> green[i] = gammaTable_[static_cast<unsigned int>(lutGains.g())]; >> blue[i] = gammaTable_[static_cast<unsigned int>(lutGains.b())];
Hi Milan On Sat, Jun 20, 2026 at 07:12:49PM +0200, Milan Zamazal wrote: > Milan Zamazal <mzamazal@redhat.com> writes: > > > Hi Jacopo, > > > > Jacopo Mondi <jacopo.mondi@ideasonboard.com> writes: > > > >> From: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> > >> Port the SoftISP Awb algorithm to use the new libipa implementation > >> of AwbAlgorithm. > >> > >> The awbAlgo_ class member is initialized with the Q<2, 8> type even if > >> there is no physical register representation for SoftISP. > >> > >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > >> --- > >> .../internal/software_isp/debayer_params.h | 2 +- > >> src/ipa/simple/algorithms/awb.cpp | 112 ++++++++++++++------- > >> src/ipa/simple/algorithms/awb.h | 29 ++++++ > >> src/ipa/simple/algorithms/ccm.cpp | 2 +- > >> src/ipa/simple/ipa_context.h | 14 +-- > >> src/libcamera/software_isp/debayer_cpu.cpp | 2 +- > >> 6 files changed, 117 insertions(+), 44 deletions(-) > >> > >> diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h > >> index 6772b43bced4..b46bbd7a8273 100644 > >> --- a/include/libcamera/internal/software_isp/debayer_params.h > >> +++ b/include/libcamera/internal/software_isp/debayer_params.h > >> @@ -24,7 +24,7 @@ struct DebayerParams { > >> RGB<float> blackLevel = RGB<float>({ 0.0, 0.0, 0.0 }); > >> float gamma = 1.0; > >> float contrastExp = 1.0; > >> - RGB<float> gains = RGB<float>({ 1.0, 1.0, 1.0 }); > >> + RGB<double> gains = RGB<double>({ 1.0, 1.0, 1.0 }); > >> }; > >> > >> } /* namespace libcamera */ > >> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp > >> index 05155c83d172..01323d9779c5 100644 > >> --- a/src/ipa/simple/algorithms/awb.cpp > >> +++ b/src/ipa/simple/algorithms/awb.cpp > >> @@ -15,7 +15,6 @@ > >> #include <libcamera/control_ids.h> > >> > >> #include "libipa/colours.h" > >> -#include "simple/ipa_context.h" > >> > >> namespace libcamera { > >> > >> @@ -23,41 +22,79 @@ LOG_DEFINE_CATEGORY(IPASoftAwb) > >> > >> namespace ipa::soft::algorithms { > >> > >> +/* > >> + * \todo Replace it with a proper Lux algorithm > >> + */ > >> +static constexpr unsigned int kDefaultLux = 500; > >> + > >> +class SimpleAwbStats final : public AwbStats > >> +{ > >> +public: > >> + SimpleAwbStats() {} > >> + SimpleAwbStats(const RGB<double> &rgbMeans) > >> + : AwbStats(rgbMeans) > >> + { > >> + } > >> + > >> + /* Minimum mean value below which AWB can't operate. */ > >> + double minColourValue() const override > >> + { > >> + return 0.2; > >> + } > >> +}; > >> + > >> +/** > >> + * \copydoc libcamera::ipa::Algorithm::init > >> + */ > >> +int Awb::init(IPAContext &context, const ValueNode &tuningData) > >> +{ > >> + return awbAlgo_.init(tuningData, context.ctrlMap); > >> +} > >> + > >> +/** > >> + * \copydoc libcamera::ipa::Algorithm::configure > >> + */ > >> int Awb::configure(IPAContext &context, > >> [[maybe_unused]] const IPAConfigInfo &configInfo) > >> { > >> - auto &gains = context.activeState.awb.gains; > >> - gains = { { 1.0, 1.0, 1.0 } }; > >> + return awbAlgo_.configure(context.activeState.awb, > >> + context.configuration.awb); > >> +} > >> > >> - return 0; > >> +/** > >> + * \copydoc libcamera::ipa::Algorithm::queueRequest > >> + */ > >> +void Awb::queueRequest(IPAContext &context, > >> + const uint32_t frame, > >> + IPAFrameContext &frameContext, > >> + const ControlList &controls) > >> +{ > >> + awbAlgo_.queueRequest(context.activeState.awb, frame, frameContext.awb, > >> + controls); > >> } > >> > >> +/** > >> + * \copydoc libcamera::ipa::Algorithm::prepare > >> + */ > >> void Awb::prepare(IPAContext &context, > >> [[maybe_unused]] const uint32_t frame, > >> IPAFrameContext &frameContext, > >> DebayerParams *params) > >> { > >> - auto &gains = context.activeState.awb.gains; > >> + awbAlgo_.prepare(context.activeState.awb, frameContext.awb); > >> > >> - frameContext.gains = gains; > >> - params->gains = gains; > >> + params->gains = frameContext.awb.gains; > >> } > >> > >> -void Awb::process(IPAContext &context, > >> - [[maybe_unused]] const uint32_t frame, > >> - IPAFrameContext &frameContext, > >> - const SwIspStats *stats, > >> - ControlList &metadata) > >> +SimpleAwbStats Awb::calculateRgbMeans(IPAContext &context, > >> + const SwIspStats *stats) const > >> { > >> + if (!stats->valid) > >> + return {}; > >> + > >> const SwIspStats::Histogram &histogram = stats->yHistogram; > >> const uint8_t blackLevel = context.activeState.blc.level; > >> > >> - metadata.set(controls::ColourGains, { frameContext.gains.r(), > >> - frameContext.gains.b() }); > >> - > >> - if (!stats->valid) > >> - return; > >> - > >> /* > >> * Black level must be subtracted to get the correct AWB ratios, they > >> * would be off if they were computed from the whole brightness range > >> @@ -67,30 +104,37 @@ void Awb::process(IPAContext &context, > >> histogram.begin(), histogram.end(), uint64_t(0)); > >> const uint64_t offset = blackLevel * nPixels; > >> const uint64_t minValid = 1; > >> + > >> /* > >> * Make sure the sums are at least minValid, while preventing unsigned > >> * integer underflow. > >> */ > >> const RGB<uint64_t> sum = stats->sum_.max(offset + minValid) - offset; > >> > >> + RGB<double> rgbMeans = { { static_cast<double>(sum.r() / nPixels), > >> + static_cast<double>(sum.g() / nPixels), > >> + static_cast<double>(sum.b() / nPixels) } }; > >> + > >> /* > >> - * Calculate red and blue gains for AWB. > >> - * Clamp max gain at 4.0, this also avoids 0 division. > >> + * \todo Determine the minimum allowed thresholds from the mean > >> + * but we currently have the sum - not the mean value! > > > > I don't understand the todo -- aren't rgbMeans means? > > > > Otherwise it looks good to me. I don't know what causes the camshark > > problem, it looks to me from the code that the controls should be > > initialised correctly and I cannot test/debug it now. > > The controls look all right. The problem is in Camshark -- 0.2.4 > has some problem with ColourGains and main has problem to display > images: > > ERROR: Failed to decode image: type object 'PixelFormats' has no attribute 'find_libcamera_name' > It might be related to your venv having an old version of pixutils ? I personally recently had to delete my venv and re-create it with up to date dependencies after a camshark upgrade. > >> + * > >> + * Currently set to SimpleAwbStats::minColourValue() = 0.2. > >> */ > >> - auto &gains = context.activeState.awb.gains; > >> - gains = { { > >> - sum.r() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.r(), > >> - 1.0, > >> - sum.b() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.b(), > >> - } }; > >> - > >> - RGB<double> rgbGains{ { 1 / gains.r(), 1 / gains.g(), 1 / gains.b() } }; > >> - context.activeState.awb.temperatureK = estimateCCT(rgbGains); > >> - metadata.set(controls::ColourTemperature, context.activeState.awb.temperatureK); > >> - > >> - LOG(IPASoftAwb, Debug) > >> - << "gain R/B: " << gains << "; temperature: " > >> - << context.activeState.awb.temperatureK; > >> + return SimpleAwbStats(rgbMeans); > >> +} > >> + > >> +/** > >> + * \copydoc libcamera::ipa::Algorithm::process > >> + */ > >> +void Awb::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > >> + IPAFrameContext &frameContext, const SwIspStats *stats, > >> + ControlList &metadata) > >> +{ > >> + SimpleAwbStats awbStats = calculateRgbMeans(context, stats); > >> + > >> + awbAlgo_.process(context.activeState.awb, frameContext.awb, awbStats, > >> + kDefaultLux, metadata); > >> } > >> > >> REGISTER_IPA_ALGORITHM(Awb, "Awb") > >> diff --git a/src/ipa/simple/algorithms/awb.h b/src/ipa/simple/algorithms/awb.h > >> index ad993f39c180..cb36cd092e51 100644 > >> --- a/src/ipa/simple/algorithms/awb.h > >> +++ b/src/ipa/simple/algorithms/awb.h > >> @@ -7,19 +7,37 @@ > >> > >> #pragma once > >> > >> +#include <libcamera/controls.h> > >> + > >> +#include "libcamera/internal/software_isp/debayer_params.h" > >> +#include "libcamera/internal/value_node.h" > >> + > >> +#include "libipa/awb.h" > >> +#include "libipa/fixedpoint.h" > >> +#include "simple/ipa_context.h" > >> + > >> #include "algorithm.h" > >> > >> namespace libcamera { > >> > >> namespace ipa::soft::algorithms { > >> > >> +class SimpleAwbStats; > >> + > >> class Awb : public Algorithm > >> { > >> public: > >> Awb() = default; > >> ~Awb() = default; > >> > >> + int init(IPAContext &context, > >> + const ValueNode &tuningData) override; > >> int configure(IPAContext &context, const IPAConfigInfo &configInfo) override; > >> + > >> + void queueRequest(IPAContext &context, > >> + [[maybe_unused]] const uint32_t frame, > >> + IPAFrameContext &frameContext, > >> + const ControlList &controls) override; > >> void prepare(IPAContext &context, > >> const uint32_t frame, > >> IPAFrameContext &frameContext, > >> @@ -29,6 +47,17 @@ public: > >> IPAFrameContext &frameContext, > >> const SwIspStats *stats, > >> ControlList &metadata) override; > >> + > >> +private: > >> + SimpleAwbStats calculateRgbMeans(IPAContext &context, > >> + const SwIspStats *stats) const; > >> + > >> + /* > >> + * There actually is no Q register format for SoftISP, but allow the > >> + * colour gains to range in the [0.0f, 15.999f] interval, which seems > >> + * reasonable. > >> + */ > >> + AwbAlgorithm<UQ<4, 8>> awbAlgo_; > >> }; > >> > >> } /* namespace ipa::soft::algorithms */ > >> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp > >> index ace9c35dc462..ff37c718c6e4 100644 > >> --- a/src/ipa/simple/algorithms/ccm.cpp > >> +++ b/src/ipa/simple/algorithms/ccm.cpp > >> @@ -44,7 +44,7 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const ValueNode &tuningData) > >> void Ccm::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame, > >> IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params) > >> { > >> - const unsigned int ct = context.activeState.awb.temperatureK; > >> + const unsigned int ct = frameContext.awb.temperatureK; > >> > >> /* Change CCM only on bigger temperature changes. */ > >> if (!currentCcm_ || > >> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h > >> index 8ccfacb46a59..0646f42d5618 100644 > >> --- a/src/ipa/simple/ipa_context.h > >> +++ b/src/ipa/simple/ipa_context.h > >> @@ -16,6 +16,7 @@ > >> #include "libcamera/internal/matrix.h" > >> #include "libcamera/internal/vector.h" > >> > >> +#include <libipa/awb.h> > >> #include <libipa/fc_queue.h> > >> > >> #include "core_ipa_interface.h" > >> @@ -25,6 +26,8 @@ namespace libcamera { > >> namespace ipa::soft { > >> > >> struct IPASessionConfiguration { > >> + ipa::awb::Session awb; > >> + > >> struct { > >> int32_t exposureMin, exposureMax; > >> double againMin, againMax, again10, againMinStep; > >> @@ -36,6 +39,8 @@ struct IPASessionConfiguration { > >> }; > >> > >> struct IPAActiveState { > >> + ipa::awb::ActiveState awb; > >> + > >> struct { > >> int32_t exposure; > >> double again; > >> @@ -48,11 +53,6 @@ struct IPAActiveState { > >> double lastGain; > >> } blc; > >> > >> - struct { > >> - RGB<float> gains; > >> - unsigned int temperatureK; > >> - } awb; > >> - > >> Matrix<float, 3, 3> combinedMatrix; > >> > >> struct { > >> @@ -64,6 +64,8 @@ struct IPAActiveState { > >> }; > >> > >> struct IPAFrameContext : public FrameContext { > >> + ipa::awb::FrameContext awb; > >> + > >> Matrix<float, 3, 3> ccm; > >> > >> struct { > >> @@ -71,8 +73,6 @@ struct IPAFrameContext : public FrameContext { > >> double gain; > >> } sensor; > >> > >> - RGB<float> gains; > >> - > >> float gamma; > >> std::optional<float> contrast; > >> std::optional<float> saturation; > >> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp > >> index d2596d32bbcd..fc171f874833 100644 > >> --- a/src/libcamera/software_isp/debayer_cpu.cpp > >> +++ b/src/libcamera/software_isp/debayer_cpu.cpp > >> @@ -1051,7 +1051,7 @@ void DebayerCpu::updateLookupTables(const DebayerParams ¶ms) > >> auto &blue = swapRedBlueGains_ ? red_ : blue_; > >> for (unsigned int i = 0; i < kRGBLookupSize; i++) { > >> /* Apply gamma after gain! */ > >> - const RGB<float> lutGains = (gains * i / div).min(gammaTableSize - 1); > >> + const RGB<double> lutGains = (gains * i / div).min(gammaTableSize - 1); > >> red[i] = gammaTable_[static_cast<unsigned int>(lutGains.r())]; > >> green[i] = gammaTable_[static_cast<unsigned int>(lutGains.g())]; > >> blue[i] = gammaTable_[static_cast<unsigned int>(lutGains.b())]; >
Hi Jacopo, Jacopo Mondi <jacopo.mondi@ideasonboard.com> writes: >> The controls look all right. The problem is in Camshark -- 0.2.4 >> has some problem with ColourGains and main has problem to display >> images: >> >> ERROR: Failed to decode image: type object 'PixelFormats' has no attribute 'find_libcamera_name' >> > > It might be related to your venv having an old version of pixutils ? > > I personally recently had to delete my venv and re-create it with up > to date dependencies after a camshark upgrade. That's it. Thanks for help! The controls work for me with the series.
diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h index 6772b43bced4..b46bbd7a8273 100644 --- a/include/libcamera/internal/software_isp/debayer_params.h +++ b/include/libcamera/internal/software_isp/debayer_params.h @@ -24,7 +24,7 @@ struct DebayerParams { RGB<float> blackLevel = RGB<float>({ 0.0, 0.0, 0.0 }); float gamma = 1.0; float contrastExp = 1.0; - RGB<float> gains = RGB<float>({ 1.0, 1.0, 1.0 }); + RGB<double> gains = RGB<double>({ 1.0, 1.0, 1.0 }); }; } /* namespace libcamera */ diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp index 05155c83d172..01323d9779c5 100644 --- a/src/ipa/simple/algorithms/awb.cpp +++ b/src/ipa/simple/algorithms/awb.cpp @@ -15,7 +15,6 @@ #include <libcamera/control_ids.h> #include "libipa/colours.h" -#include "simple/ipa_context.h" namespace libcamera { @@ -23,41 +22,79 @@ LOG_DEFINE_CATEGORY(IPASoftAwb) namespace ipa::soft::algorithms { +/* + * \todo Replace it with a proper Lux algorithm + */ +static constexpr unsigned int kDefaultLux = 500; + +class SimpleAwbStats final : public AwbStats +{ +public: + SimpleAwbStats() {} + SimpleAwbStats(const RGB<double> &rgbMeans) + : AwbStats(rgbMeans) + { + } + + /* Minimum mean value below which AWB can't operate. */ + double minColourValue() const override + { + return 0.2; + } +}; + +/** + * \copydoc libcamera::ipa::Algorithm::init + */ +int Awb::init(IPAContext &context, const ValueNode &tuningData) +{ + return awbAlgo_.init(tuningData, context.ctrlMap); +} + +/** + * \copydoc libcamera::ipa::Algorithm::configure + */ int Awb::configure(IPAContext &context, [[maybe_unused]] const IPAConfigInfo &configInfo) { - auto &gains = context.activeState.awb.gains; - gains = { { 1.0, 1.0, 1.0 } }; + return awbAlgo_.configure(context.activeState.awb, + context.configuration.awb); +} - return 0; +/** + * \copydoc libcamera::ipa::Algorithm::queueRequest + */ +void Awb::queueRequest(IPAContext &context, + const uint32_t frame, + IPAFrameContext &frameContext, + const ControlList &controls) +{ + awbAlgo_.queueRequest(context.activeState.awb, frame, frameContext.awb, + controls); } +/** + * \copydoc libcamera::ipa::Algorithm::prepare + */ void Awb::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame, IPAFrameContext &frameContext, DebayerParams *params) { - auto &gains = context.activeState.awb.gains; + awbAlgo_.prepare(context.activeState.awb, frameContext.awb); - frameContext.gains = gains; - params->gains = gains; + params->gains = frameContext.awb.gains; } -void Awb::process(IPAContext &context, - [[maybe_unused]] const uint32_t frame, - IPAFrameContext &frameContext, - const SwIspStats *stats, - ControlList &metadata) +SimpleAwbStats Awb::calculateRgbMeans(IPAContext &context, + const SwIspStats *stats) const { + if (!stats->valid) + return {}; + const SwIspStats::Histogram &histogram = stats->yHistogram; const uint8_t blackLevel = context.activeState.blc.level; - metadata.set(controls::ColourGains, { frameContext.gains.r(), - frameContext.gains.b() }); - - if (!stats->valid) - return; - /* * Black level must be subtracted to get the correct AWB ratios, they * would be off if they were computed from the whole brightness range @@ -67,30 +104,37 @@ void Awb::process(IPAContext &context, histogram.begin(), histogram.end(), uint64_t(0)); const uint64_t offset = blackLevel * nPixels; const uint64_t minValid = 1; + /* * Make sure the sums are at least minValid, while preventing unsigned * integer underflow. */ const RGB<uint64_t> sum = stats->sum_.max(offset + minValid) - offset; + RGB<double> rgbMeans = { { static_cast<double>(sum.r() / nPixels), + static_cast<double>(sum.g() / nPixels), + static_cast<double>(sum.b() / nPixels) } }; + /* - * Calculate red and blue gains for AWB. - * Clamp max gain at 4.0, this also avoids 0 division. + * \todo Determine the minimum allowed thresholds from the mean + * but we currently have the sum - not the mean value! + * + * Currently set to SimpleAwbStats::minColourValue() = 0.2. */ - auto &gains = context.activeState.awb.gains; - gains = { { - sum.r() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.r(), - 1.0, - sum.b() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.b(), - } }; - - RGB<double> rgbGains{ { 1 / gains.r(), 1 / gains.g(), 1 / gains.b() } }; - context.activeState.awb.temperatureK = estimateCCT(rgbGains); - metadata.set(controls::ColourTemperature, context.activeState.awb.temperatureK); - - LOG(IPASoftAwb, Debug) - << "gain R/B: " << gains << "; temperature: " - << context.activeState.awb.temperatureK; + return SimpleAwbStats(rgbMeans); +} + +/** + * \copydoc libcamera::ipa::Algorithm::process + */ +void Awb::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, + IPAFrameContext &frameContext, const SwIspStats *stats, + ControlList &metadata) +{ + SimpleAwbStats awbStats = calculateRgbMeans(context, stats); + + awbAlgo_.process(context.activeState.awb, frameContext.awb, awbStats, + kDefaultLux, metadata); } REGISTER_IPA_ALGORITHM(Awb, "Awb") diff --git a/src/ipa/simple/algorithms/awb.h b/src/ipa/simple/algorithms/awb.h index ad993f39c180..cb36cd092e51 100644 --- a/src/ipa/simple/algorithms/awb.h +++ b/src/ipa/simple/algorithms/awb.h @@ -7,19 +7,37 @@ #pragma once +#include <libcamera/controls.h> + +#include "libcamera/internal/software_isp/debayer_params.h" +#include "libcamera/internal/value_node.h" + +#include "libipa/awb.h" +#include "libipa/fixedpoint.h" +#include "simple/ipa_context.h" + #include "algorithm.h" namespace libcamera { namespace ipa::soft::algorithms { +class SimpleAwbStats; + class Awb : public Algorithm { public: Awb() = default; ~Awb() = default; + int init(IPAContext &context, + const ValueNode &tuningData) override; int configure(IPAContext &context, const IPAConfigInfo &configInfo) override; + + void queueRequest(IPAContext &context, + [[maybe_unused]] const uint32_t frame, + IPAFrameContext &frameContext, + const ControlList &controls) override; void prepare(IPAContext &context, const uint32_t frame, IPAFrameContext &frameContext, @@ -29,6 +47,17 @@ public: IPAFrameContext &frameContext, const SwIspStats *stats, ControlList &metadata) override; + +private: + SimpleAwbStats calculateRgbMeans(IPAContext &context, + const SwIspStats *stats) const; + + /* + * There actually is no Q register format for SoftISP, but allow the + * colour gains to range in the [0.0f, 15.999f] interval, which seems + * reasonable. + */ + AwbAlgorithm<UQ<4, 8>> awbAlgo_; }; } /* namespace ipa::soft::algorithms */ diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp index ace9c35dc462..ff37c718c6e4 100644 --- a/src/ipa/simple/algorithms/ccm.cpp +++ b/src/ipa/simple/algorithms/ccm.cpp @@ -44,7 +44,7 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const ValueNode &tuningData) void Ccm::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame, IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params) { - const unsigned int ct = context.activeState.awb.temperatureK; + const unsigned int ct = frameContext.awb.temperatureK; /* Change CCM only on bigger temperature changes. */ if (!currentCcm_ || diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h index 8ccfacb46a59..0646f42d5618 100644 --- a/src/ipa/simple/ipa_context.h +++ b/src/ipa/simple/ipa_context.h @@ -16,6 +16,7 @@ #include "libcamera/internal/matrix.h" #include "libcamera/internal/vector.h" +#include <libipa/awb.h> #include <libipa/fc_queue.h> #include "core_ipa_interface.h" @@ -25,6 +26,8 @@ namespace libcamera { namespace ipa::soft { struct IPASessionConfiguration { + ipa::awb::Session awb; + struct { int32_t exposureMin, exposureMax; double againMin, againMax, again10, againMinStep; @@ -36,6 +39,8 @@ struct IPASessionConfiguration { }; struct IPAActiveState { + ipa::awb::ActiveState awb; + struct { int32_t exposure; double again; @@ -48,11 +53,6 @@ struct IPAActiveState { double lastGain; } blc; - struct { - RGB<float> gains; - unsigned int temperatureK; - } awb; - Matrix<float, 3, 3> combinedMatrix; struct { @@ -64,6 +64,8 @@ struct IPAActiveState { }; struct IPAFrameContext : public FrameContext { + ipa::awb::FrameContext awb; + Matrix<float, 3, 3> ccm; struct { @@ -71,8 +73,6 @@ struct IPAFrameContext : public FrameContext { double gain; } sensor; - RGB<float> gains; - float gamma; std::optional<float> contrast; std::optional<float> saturation; diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp index d2596d32bbcd..fc171f874833 100644 --- a/src/libcamera/software_isp/debayer_cpu.cpp +++ b/src/libcamera/software_isp/debayer_cpu.cpp @@ -1051,7 +1051,7 @@ void DebayerCpu::updateLookupTables(const DebayerParams ¶ms) auto &blue = swapRedBlueGains_ ? red_ : blue_; for (unsigned int i = 0; i < kRGBLookupSize; i++) { /* Apply gamma after gain! */ - const RGB<float> lutGains = (gains * i / div).min(gammaTableSize - 1); + const RGB<double> lutGains = (gains * i / div).min(gammaTableSize - 1); red[i] = gammaTable_[static_cast<unsigned int>(lutGains.r())]; green[i] = gammaTable_[static_cast<unsigned int>(lutGains.g())]; blue[i] = gammaTable_[static_cast<unsigned int>(lutGains.b())];