| Message ID | 20250407085802.16269-1-mzamazal@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting Milan Zamazal (2025-04-07 09:58:02) > Saturation control is added on top of the colour correction matrix. A > way of saturation adjustment that can be fully integrated into the > colour correction matrix is used. The control is available only if Ccm > algorithm is enabled. > > The control uses 0.0-2.0 value range, with 1.0 being unmodified > saturation, 0.0 full desaturation and 2.0 quite saturated. > > The matrix for saturation adjustment is taken from > https://www.graficaobscura.com/matrix/index.html. The colour correction > matrix is applied before gamma and the given matrix is suitable for such > a case. Alternatively, the transformation used in libcamera rpi ccm.cpp > could be used. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/ipa/simple/algorithms/ccm.cpp | 57 +++++++++++++++++++++++++++++-- > src/ipa/simple/algorithms/ccm.h | 11 ++++++ > src/ipa/simple/ipa_context.h | 4 +++ > 3 files changed, 69 insertions(+), 3 deletions(-) > > diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp > index d5ba928d..2700a247 100644 > --- a/src/ipa/simple/algorithms/ccm.cpp > +++ b/src/ipa/simple/algorithms/ccm.cpp > @@ -3,7 +3,7 @@ > * Copyright (C) 2024, Ideas On Board > * Copyright (C) 2024-2025, Red Hat Inc. > * > - * Color correction matrix > + * Color correction matrix + saturation > */ > > #include "ccm.h" > @@ -13,6 +13,8 @@ > > #include <libcamera/control_ids.h> > > +#include "libcamera/internal/matrix.h" > + > namespace { > > constexpr unsigned int kTemperatureThreshold = 100; > @@ -35,28 +37,73 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData > } > > context.ccmEnabled = true; > + context.ctrlMap[&controls::Saturation] = ControlInfo(0.0f, 2.0f, 1.0f); > + > + return 0; > +} > + > +int Ccm::configure(IPAContext &context, > + [[maybe_unused]] const IPAConfigInfo &configInfo) > +{ > + context.activeState.knobs.saturation = std::optional<double>(); > > return 0; > } > > +void Ccm::queueRequest(typename Module::Context &context, > + [[maybe_unused]] const uint32_t frame, > + [[maybe_unused]] typename Module::FrameContext &frameContext, > + const ControlList &controls) > +{ > + const auto &saturation = controls.get(controls::Saturation); > + if (saturation.has_value()) { > + context.activeState.knobs.saturation = saturation; > + LOG(IPASoftCcm, Debug) << "Setting saturation to " << saturation.value(); > + } > +} > + > +void Ccm::updateSaturation(Matrix<float, 3, 3> &ccm, float saturation) > +{ > + /* > + * See https://www.graficaobscura.com/matrix/index.html. Looks like it would be easy to add a brightness control too after this. > + * This is applied before gamma thus a matrix for linear RGB must be used. > + * The saturation range is 0..2, with 1 being an unchanged saturation and 0 > + * no saturation (monochrome). > + */ > + constexpr float r = 0.3086; > + constexpr float g = 0.6094; > + constexpr float b = 0.0820; it would be interesting to dig into where these numbers are derived from exactly ... https://dsp.stackexchange.com/questions/27599/how-is-the-luminance-vector-calculated-for-these-linear-matrix-transforms seems to report others have looked, but I don't see a direct answer yet (but I won't dig deep) ... And the referenced article is quite helpful. As this is the 'luminance' vector - I wonder if writing it as RGB<float> luminance = { 0.3086, 0.6094, 0.0820 }; makes sense... > + const float s1 = 1.0 - saturation; > + ccm = ccm * Matrix<float, 3, 3>{ { s1 * r + saturation, s1 * g, s1 * b, > + s1 * r, s1 * g + saturation, s1 * b, > + s1 * r, s1 * g, s1 * b + saturation } }; but that would become much more terse ... so I'm not sure it's worth it here? Or would it then be more explicitly readable like the applySaturation in RPi? > +} > + > void Ccm::prepare(IPAContext &context, const uint32_t frame, > IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params) > { > + auto &saturation = context.activeState.knobs.saturation; > + > const unsigned int ct = context.activeState.awb.temperatureK; > > - /* Change CCM only on bigger temperature changes. */ > + /* Change CCM only on saturation or bigger temperature changes. */ > if (frame > 0 && > - utils::abs_diff(ct, lastCt_) < kTemperatureThreshold) { > + utils::abs_diff(ct, lastCt_) < kTemperatureThreshold && > + saturation == lastSaturation_) { > frameContext.ccm.ccm = context.activeState.ccm.ccm; > context.activeState.ccm.changed = false; > return; > } > > lastCt_ = ct; > + lastSaturation_ = saturation; > Matrix<float, 3, 3> ccm = ccm_.getInterpolated(ct); > + if (saturation) > + updateSaturation(ccm, saturation.value()); > > context.activeState.ccm.ccm = ccm; > frameContext.ccm.ccm = ccm; > + frameContext.saturation = saturation; > context.activeState.ccm.changed = true; > } > > @@ -67,6 +114,10 @@ void Ccm::process([[maybe_unused]] IPAContext &context, > ControlList &metadata) > { > metadata.set(controls::ColourCorrectionMatrix, frameContext.ccm.ccm.data()); > + > + const auto &saturation = frameContext.saturation; > + if (saturation) > + metadata.set(controls::Saturation, saturation.value()); This matches what we currently do for Contrast, but I wonder if we should report a metadata of '1.0' for both Contrast and Saturation if they aren't set ... as that's what the 'state' is/should be ? Anyway, I wouldn't block this patch on that - as it would be on top for both controls... Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Though, while it's functional - looking at the resulting image in camshark, I think I would expect a saturation of 0.0 to be 'greyscale' - with all pixels having roughly the same r=g=b - and I don't think that's the case ... It looks like there's definitely a bias against green. It looks like R=B, but G is always slightly less by at least ~20% ? ... leaving the image looking slightly tinted purple. That could be because I'm running an untuned/identity matrix for CCM perhaps? or is it because white balance needs to be taken into account ? But G = (RB-20%) doesn't sound like white balance to me at the moment... > } > > REGISTER_IPA_ALGORITHM(Ccm, "Ccm") > diff --git a/src/ipa/simple/algorithms/ccm.h b/src/ipa/simple/algorithms/ccm.h > index f4e2b85b..2c5d2170 100644 > --- a/src/ipa/simple/algorithms/ccm.h > +++ b/src/ipa/simple/algorithms/ccm.h > @@ -7,6 +7,8 @@ > > #pragma once > > +#include <optional> > + > #include "libcamera/internal/matrix.h" > > #include <libipa/interpolator.h> > @@ -24,6 +26,12 @@ public: > ~Ccm() = default; > > int init(IPAContext &context, const YamlObject &tuningData) override; > + int configure(IPAContext &context, > + const IPAConfigInfo &configInfo) override; > + void queueRequest(typename Module::Context &context, > + const uint32_t frame, > + typename Module::FrameContext &frameContext, > + const ControlList &controls) override; > void prepare(IPAContext &context, > const uint32_t frame, > IPAFrameContext &frameContext, > @@ -34,7 +42,10 @@ public: > ControlList &metadata) override; > > private: > + void updateSaturation(Matrix<float, 3, 3> &ccm, float saturation); > + > unsigned int lastCt_; > + std::optional<float> lastSaturation_; > Interpolator<Matrix<float, 3, 3>> ccm_; > }; > > diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h > index 88cc6c35..56792981 100644 > --- a/src/ipa/simple/ipa_context.h > +++ b/src/ipa/simple/ipa_context.h > @@ -63,6 +63,7 @@ struct IPAActiveState { > struct { > /* 0..2 range, 1.0 = normal */ > std::optional<double> contrast; > + std::optional<double> saturation; > } knobs; > }; > > @@ -75,11 +76,14 @@ struct IPAFrameContext : public FrameContext { > int32_t exposure; > double gain; > } sensor; > + > struct { > double red; > double blue; > } gains; > + > std::optional<double> contrast; > + std::optional<double> saturation; > }; > > struct IPAContext { > -- > 2.49.0 >
Hi Kieran, thank you for review. Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > Quoting Milan Zamazal (2025-04-07 09:58:02) >> Saturation control is added on top of the colour correction matrix. A >> way of saturation adjustment that can be fully integrated into the > >> colour correction matrix is used. The control is available only if Ccm >> algorithm is enabled. >> >> The control uses 0.0-2.0 value range, with 1.0 being unmodified >> saturation, 0.0 full desaturation and 2.0 quite saturated. >> >> The matrix for saturation adjustment is taken from >> https://www.graficaobscura.com/matrix/index.html. The colour correction >> matrix is applied before gamma and the given matrix is suitable for such >> a case. Alternatively, the transformation used in libcamera rpi ccm.cpp >> could be used. >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> src/ipa/simple/algorithms/ccm.cpp | 57 +++++++++++++++++++++++++++++-- >> src/ipa/simple/algorithms/ccm.h | 11 ++++++ >> src/ipa/simple/ipa_context.h | 4 +++ >> 3 files changed, 69 insertions(+), 3 deletions(-) >> >> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp >> index d5ba928d..2700a247 100644 >> --- a/src/ipa/simple/algorithms/ccm.cpp >> +++ b/src/ipa/simple/algorithms/ccm.cpp >> @@ -3,7 +3,7 @@ >> * Copyright (C) 2024, Ideas On Board >> * Copyright (C) 2024-2025, Red Hat Inc. >> * >> - * Color correction matrix >> + * Color correction matrix + saturation >> */ >> >> #include "ccm.h" >> @@ -13,6 +13,8 @@ >> >> #include <libcamera/control_ids.h> >> >> +#include "libcamera/internal/matrix.h" >> + >> namespace { >> >> constexpr unsigned int kTemperatureThreshold = 100; >> @@ -35,28 +37,73 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData >> } >> >> context.ccmEnabled = true; >> + context.ctrlMap[&controls::Saturation] = ControlInfo(0.0f, 2.0f, 1.0f); >> + >> + return 0; >> +} >> + >> +int Ccm::configure(IPAContext &context, >> + [[maybe_unused]] const IPAConfigInfo &configInfo) >> +{ >> + context.activeState.knobs.saturation = std::optional<double>(); >> >> return 0; >> } >> >> +void Ccm::queueRequest(typename Module::Context &context, >> + [[maybe_unused]] const uint32_t frame, >> + [[maybe_unused]] typename Module::FrameContext &frameContext, >> + const ControlList &controls) >> +{ >> + const auto &saturation = controls.get(controls::Saturation); >> + if (saturation.has_value()) { >> + context.activeState.knobs.saturation = saturation; >> + LOG(IPASoftCcm, Debug) << "Setting saturation to " << saturation.value(); >> + } >> +} >> + >> +void Ccm::updateSaturation(Matrix<float, 3, 3> &ccm, float saturation) >> +{ >> + /* >> + * See https://www.graficaobscura.com/matrix/index.html. > > Looks like it would be easy to add a brightness control too after this. Yes, when CCM is used, it's easy to add brightness. And when CCM is not used, it's easy too, applying it when making the LUT values -- this is important for CPU ISP when CCM is not used (having to use CCM just to adjust brightness would be overkill). >> + * This is applied before gamma thus a matrix for linear RGB must be used. >> + * The saturation range is 0..2, with 1 being an unchanged saturation and 0 >> + * no saturation (monochrome). >> + */ >> + constexpr float r = 0.3086; >> + constexpr float g = 0.6094; >> + constexpr float b = 0.0820; > > it would be interesting to dig into where these numbers are derived > from exactly ... I guess some educated magic, similarly to other luminance related conversions. > https://dsp.stackexchange.com/questions/27599/how-is-the-luminance-vector-calculated-for-these-linear-matrix-transforms > seems to report others have looked, but I don't see a direct answer yet > (but I won't dig deep) ... > > And the referenced article is quite helpful. > > As this is the 'luminance' vector - I wonder if writing it as > RGB<float> luminance = { 0.3086, 0.6094, 0.0820 }; > > makes sense... > >> + const float s1 = 1.0 - saturation; >> + ccm = ccm * Matrix<float, 3, 3>{ { s1 * r + saturation, s1 * g, s1 * b, >> + s1 * r, s1 * g + saturation, s1 * b, >> + s1 * r, s1 * g, s1 * b + saturation } }; > > but that would become much more terse ... so I'm not sure it's worth it > here? Or would it then be more explicitly readable like the > applySaturation in RPi? I'll try and let's see. >> +} >> + >> void Ccm::prepare(IPAContext &context, const uint32_t frame, >> IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params) >> { >> + auto &saturation = context.activeState.knobs.saturation; >> + >> const unsigned int ct = context.activeState.awb.temperatureK; >> >> - /* Change CCM only on bigger temperature changes. */ >> + /* Change CCM only on saturation or bigger temperature changes. */ >> if (frame > 0 && >> - utils::abs_diff(ct, lastCt_) < kTemperatureThreshold) { >> + utils::abs_diff(ct, lastCt_) < kTemperatureThreshold && >> + saturation == lastSaturation_) { >> frameContext.ccm.ccm = context.activeState.ccm.ccm; >> context.activeState.ccm.changed = false; >> return; >> } >> >> lastCt_ = ct; >> + lastSaturation_ = saturation; >> Matrix<float, 3, 3> ccm = ccm_.getInterpolated(ct); >> + if (saturation) >> + updateSaturation(ccm, saturation.value()); >> >> context.activeState.ccm.ccm = ccm; >> frameContext.ccm.ccm = ccm; >> + frameContext.saturation = saturation; >> context.activeState.ccm.changed = true; >> } >> >> @@ -67,6 +114,10 @@ void Ccm::process([[maybe_unused]] IPAContext &context, >> ControlList &metadata) >> { >> metadata.set(controls::ColourCorrectionMatrix, frameContext.ccm.ccm.data()); >> + >> + const auto &saturation = frameContext.saturation; >> + if (saturation) >> + metadata.set(controls::Saturation, saturation.value()); > > This matches what we currently do for Contrast, but I wonder if we > should report a metadata of '1.0' for both Contrast and Saturation if > they aren't set ... as that's what the 'state' is/should be ? I don't have an opinion on this, both of "no saturation request -> no metadata" and "no saturation request -> the corresponding default value in metadata" make sense to me. > Anyway, I wouldn't block this patch on that - as it would be on top for > both controls... > > Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Though, while it's functional - looking at the resulting image in > camshark, I think I would expect a saturation of 0.0 to be 'greyscale' - > with all pixels having roughly the same r=g=b - and I don't think that's > the case ... It looks like there's definitely a bias against green. It > looks like R=B, but G is always slightly less by at least ~20% ? ... > leaving the image looking slightly tinted purple. Oh, now, under daylight, I get a prominent purple tint too. > That could be because I'm running an untuned/identity matrix for CCM > perhaps? or is it because white balance needs to be taken into account ? No, it's a math mistake I did in CCM patches. White balance must be applied before CCM. Let C be the CCM, W the AWB gains CCM-like matrix, and P the pixel RGB vector. I thought "let's apply C after W" and computed (W * C) * P. Which is wrong, to apply C after W means C * (W * P) = (C * W) * P. The last matrix applied must be the first one in the multiplication. When I swap the multiplication in src/ipa/simple/algorithms/lut.cpp from auto ccm = gainCcm * context.activeState.ccm.ccm; to auto ccm = context.activeState.ccm.ccm * gainCcm; it gets desaturated as it should be (there cannot be any colour cast when desaturation is applied correctly, regardless of sensor calibration). Maybe this will also explain and improve the colour casts I previously experienced when experimenting with my sensor CCMs from other pipelines. > But G = (RB-20%) doesn't sound like white balance to me at the moment... > >> } >> >> REGISTER_IPA_ALGORITHM(Ccm, "Ccm") >> diff --git a/src/ipa/simple/algorithms/ccm.h b/src/ipa/simple/algorithms/ccm.h >> index f4e2b85b..2c5d2170 100644 >> --- a/src/ipa/simple/algorithms/ccm.h >> +++ b/src/ipa/simple/algorithms/ccm.h >> @@ -7,6 +7,8 @@ >> >> #pragma once >> >> +#include <optional> >> + >> #include "libcamera/internal/matrix.h" >> >> #include <libipa/interpolator.h> >> @@ -24,6 +26,12 @@ public: >> ~Ccm() = default; >> >> int init(IPAContext &context, const YamlObject &tuningData) override; >> + int configure(IPAContext &context, >> + const IPAConfigInfo &configInfo) override; >> + void queueRequest(typename Module::Context &context, >> + const uint32_t frame, >> + typename Module::FrameContext &frameContext, >> + const ControlList &controls) override; >> void prepare(IPAContext &context, >> const uint32_t frame, >> IPAFrameContext &frameContext, >> @@ -34,7 +42,10 @@ public: >> ControlList &metadata) override; >> >> private: >> + void updateSaturation(Matrix<float, 3, 3> &ccm, float saturation); >> + >> unsigned int lastCt_; >> + std::optional<float> lastSaturation_; >> Interpolator<Matrix<float, 3, 3>> ccm_; >> }; >> >> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h >> index 88cc6c35..56792981 100644 >> --- a/src/ipa/simple/ipa_context.h >> +++ b/src/ipa/simple/ipa_context.h >> @@ -63,6 +63,7 @@ struct IPAActiveState { >> struct { >> /* 0..2 range, 1.0 = normal */ >> std::optional<double> contrast; >> + std::optional<double> saturation; >> } knobs; >> }; >> >> @@ -75,11 +76,14 @@ struct IPAFrameContext : public FrameContext { >> int32_t exposure; >> double gain; >> } sensor; >> + >> struct { >> double red; >> double blue; >> } gains; >> + >> std::optional<double> contrast; >> + std::optional<double> saturation; >> }; >> >> struct IPAContext { >> -- >> 2.49.0 >>
Quoting Milan Zamazal (2025-04-14 14:15:38) > Hi Kieran, > > thank you for review. > > Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > > > Quoting Milan Zamazal (2025-04-07 09:58:02) > >> Saturation control is added on top of the colour correction matrix. A > >> way of saturation adjustment that can be fully integrated into the > > > >> colour correction matrix is used. The control is available only if Ccm > >> algorithm is enabled. > >> > >> The control uses 0.0-2.0 value range, with 1.0 being unmodified > >> saturation, 0.0 full desaturation and 2.0 quite saturated. > >> > >> The matrix for saturation adjustment is taken from > >> https://www.graficaobscura.com/matrix/index.html. The colour correction > >> matrix is applied before gamma and the given matrix is suitable for such > >> a case. Alternatively, the transformation used in libcamera rpi ccm.cpp > >> could be used. > >> > >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > >> --- > >> src/ipa/simple/algorithms/ccm.cpp | 57 +++++++++++++++++++++++++++++-- > >> src/ipa/simple/algorithms/ccm.h | 11 ++++++ > >> src/ipa/simple/ipa_context.h | 4 +++ > >> 3 files changed, 69 insertions(+), 3 deletions(-) > >> > >> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp > >> index d5ba928d..2700a247 100644 > >> --- a/src/ipa/simple/algorithms/ccm.cpp > >> +++ b/src/ipa/simple/algorithms/ccm.cpp > >> @@ -3,7 +3,7 @@ > >> * Copyright (C) 2024, Ideas On Board > >> * Copyright (C) 2024-2025, Red Hat Inc. > >> * > >> - * Color correction matrix > >> + * Color correction matrix + saturation > >> */ > >> > >> #include "ccm.h" > >> @@ -13,6 +13,8 @@ > >> > >> #include <libcamera/control_ids.h> > >> > >> +#include "libcamera/internal/matrix.h" > >> + > >> namespace { > >> > >> constexpr unsigned int kTemperatureThreshold = 100; > >> @@ -35,28 +37,73 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData > >> } > >> > >> context.ccmEnabled = true; > >> + context.ctrlMap[&controls::Saturation] = ControlInfo(0.0f, 2.0f, 1.0f); > >> + > >> + return 0; > >> +} > >> + > >> +int Ccm::configure(IPAContext &context, > >> + [[maybe_unused]] const IPAConfigInfo &configInfo) > >> +{ > >> + context.activeState.knobs.saturation = std::optional<double>(); > >> > >> return 0; > >> } > >> > >> +void Ccm::queueRequest(typename Module::Context &context, > >> + [[maybe_unused]] const uint32_t frame, > >> + [[maybe_unused]] typename Module::FrameContext &frameContext, > >> + const ControlList &controls) > >> +{ > >> + const auto &saturation = controls.get(controls::Saturation); > >> + if (saturation.has_value()) { > >> + context.activeState.knobs.saturation = saturation; > >> + LOG(IPASoftCcm, Debug) << "Setting saturation to " << saturation.value(); > >> + } > >> +} > >> + > >> +void Ccm::updateSaturation(Matrix<float, 3, 3> &ccm, float saturation) > >> +{ > >> + /* > >> + * See https://www.graficaobscura.com/matrix/index.html. > > > > Looks like it would be easy to add a brightness control too after this. > > Yes, when CCM is used, it's easy to add brightness. And when CCM is not > used, it's easy too, applying it when making the LUT values -- this is > important for CPU ISP when CCM is not used (having to use CCM just to > adjust brightness would be overkill). Aha, then indeed lets just apply it directly to the LUT (later) :-) > > >> + * This is applied before gamma thus a matrix for linear RGB must be used. > >> + * The saturation range is 0..2, with 1 being an unchanged saturation and 0 > >> + * no saturation (monochrome). > >> + */ > >> + constexpr float r = 0.3086; > >> + constexpr float g = 0.6094; > >> + constexpr float b = 0.0820; > > > > it would be interesting to dig into where these numbers are derived > > from exactly ... > > I guess some educated magic, similarly to other luminance related > conversions. > > > https://dsp.stackexchange.com/questions/27599/how-is-the-luminance-vector-calculated-for-these-linear-matrix-transforms > > seems to report others have looked, but I don't see a direct answer yet > > (but I won't dig deep) ... > > > > And the referenced article is quite helpful. > > > > As this is the 'luminance' vector - I wonder if writing it as > > RGB<float> luminance = { 0.3086, 0.6094, 0.0820 }; > > > > makes sense... > > > >> + const float s1 = 1.0 - saturation; > >> + ccm = ccm * Matrix<float, 3, 3>{ { s1 * r + saturation, s1 * g, s1 * b, > >> + s1 * r, s1 * g + saturation, s1 * b, > >> + s1 * r, s1 * g, s1 * b + saturation } }; > > > > but that would become much more terse ... so I'm not sure it's worth it > > here? Or would it then be more explicitly readable like the > > applySaturation in RPi? > > I'll try and let's see. > > >> +} > >> + > >> void Ccm::prepare(IPAContext &context, const uint32_t frame, > >> IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params) > >> { > >> + auto &saturation = context.activeState.knobs.saturation; > >> + > >> const unsigned int ct = context.activeState.awb.temperatureK; > >> > >> - /* Change CCM only on bigger temperature changes. */ > >> + /* Change CCM only on saturation or bigger temperature changes. */ > >> if (frame > 0 && > >> - utils::abs_diff(ct, lastCt_) < kTemperatureThreshold) { > >> + utils::abs_diff(ct, lastCt_) < kTemperatureThreshold && > >> + saturation == lastSaturation_) { > >> frameContext.ccm.ccm = context.activeState.ccm.ccm; > >> context.activeState.ccm.changed = false; > >> return; > >> } > >> > >> lastCt_ = ct; > >> + lastSaturation_ = saturation; > >> Matrix<float, 3, 3> ccm = ccm_.getInterpolated(ct); > >> + if (saturation) > >> + updateSaturation(ccm, saturation.value()); > >> > >> context.activeState.ccm.ccm = ccm; > >> frameContext.ccm.ccm = ccm; > >> + frameContext.saturation = saturation; > >> context.activeState.ccm.changed = true; > >> } > >> > >> @@ -67,6 +114,10 @@ void Ccm::process([[maybe_unused]] IPAContext &context, > >> ControlList &metadata) > >> { > >> metadata.set(controls::ColourCorrectionMatrix, frameContext.ccm.ccm.data()); > >> + > >> + const auto &saturation = frameContext.saturation; > >> + if (saturation) > >> + metadata.set(controls::Saturation, saturation.value()); > > > > This matches what we currently do for Contrast, but I wonder if we > > should report a metadata of '1.0' for both Contrast and Saturation if > > they aren't set ... as that's what the 'state' is/should be ? > > I don't have an opinion on this, both of "no saturation request -> no > metadata" and "no saturation request -> the corresponding default value > in metadata" make sense to me. > > > Anyway, I wouldn't block this patch on that - as it would be on top for > > both controls... > > > > Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > Though, while it's functional - looking at the resulting image in > > camshark, I think I would expect a saturation of 0.0 to be 'greyscale' - > > with all pixels having roughly the same r=g=b - and I don't think that's > > the case ... It looks like there's definitely a bias against green. It > > looks like R=B, but G is always slightly less by at least ~20% ? ... > > leaving the image looking slightly tinted purple. > > Oh, now, under daylight, I get a prominent purple tint too. > > > That could be because I'm running an untuned/identity matrix for CCM > > perhaps? or is it because white balance needs to be taken into account ? > > No, it's a math mistake I did in CCM patches. White balance must be > applied before CCM. Let C be the CCM, W the AWB gains CCM-like matrix, > and P the pixel RGB vector. I thought "let's apply C after W" and > computed (W * C) * P. Which is wrong, to apply C after W means > C * (W * P) = (C * W) * P. The last matrix applied must be the first > one in the multiplication. When I swap the multiplication in > src/ipa/simple/algorithms/lut.cpp from > > auto ccm = gainCcm * context.activeState.ccm.ccm; > > to > > auto ccm = context.activeState.ccm.ccm * gainCcm; > > it gets desaturated as it should be (there cannot be any colour cast > when desaturation is applied correctly, regardless of sensor > calibration). Maybe this will also explain and improve the colour casts > I previously experienced when experimenting with my sensor CCMs from > other pipelines. > > > But G = (RB-20%) doesn't sound like white balance to me at the moment... aha, so maybe it is ;-) Great, well if that's explained ... we have a path forwards. Does it impact this patch much? Would it help to merge this first? or fix the CCM then rebase this one ? I think the colour tint is the only thing stopping me from adding a tag, and if that's clarified, then for Saturation: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> If you respin with the calculation split out in Ccm::updateSaturation() you can still collect the tag I think. (perhaps there's a new series with the above change and the color fix on the way?) -- Kieran > > > >> } > >> > >> REGISTER_IPA_ALGORITHM(Ccm, "Ccm") > >> diff --git a/src/ipa/simple/algorithms/ccm.h b/src/ipa/simple/algorithms/ccm.h > >> index f4e2b85b..2c5d2170 100644 > >> --- a/src/ipa/simple/algorithms/ccm.h > >> +++ b/src/ipa/simple/algorithms/ccm.h > >> @@ -7,6 +7,8 @@ > >> > >> #pragma once > >> > >> +#include <optional> > >> + > >> #include "libcamera/internal/matrix.h" > >> > >> #include <libipa/interpolator.h> > >> @@ -24,6 +26,12 @@ public: > >> ~Ccm() = default; > >> > >> int init(IPAContext &context, const YamlObject &tuningData) override; > >> + int configure(IPAContext &context, > >> + const IPAConfigInfo &configInfo) override; > >> + void queueRequest(typename Module::Context &context, > >> + const uint32_t frame, > >> + typename Module::FrameContext &frameContext, > >> + const ControlList &controls) override; > >> void prepare(IPAContext &context, > >> const uint32_t frame, > >> IPAFrameContext &frameContext, > >> @@ -34,7 +42,10 @@ public: > >> ControlList &metadata) override; > >> > >> private: > >> + void updateSaturation(Matrix<float, 3, 3> &ccm, float saturation); > >> + > >> unsigned int lastCt_; > >> + std::optional<float> lastSaturation_; > >> Interpolator<Matrix<float, 3, 3>> ccm_; > >> }; > >> > >> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h > >> index 88cc6c35..56792981 100644 > >> --- a/src/ipa/simple/ipa_context.h > >> +++ b/src/ipa/simple/ipa_context.h > >> @@ -63,6 +63,7 @@ struct IPAActiveState { > >> struct { > >> /* 0..2 range, 1.0 = normal */ > >> std::optional<double> contrast; > >> + std::optional<double> saturation; > >> } knobs; > >> }; > >> > >> @@ -75,11 +76,14 @@ struct IPAFrameContext : public FrameContext { > >> int32_t exposure; > >> double gain; > >> } sensor; > >> + > >> struct { > >> double red; > >> double blue; > >> } gains; > >> + > >> std::optional<double> contrast; > >> + std::optional<double> saturation; > >> }; > >> > >> struct IPAContext { > >> -- > >> 2.49.0 > >> >
Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > Quoting Milan Zamazal (2025-04-14 14:15:38) >> Hi Kieran, >> > >> thank you for review. >> >> Kieran Bingham <kieran.bingham@ideasonboard.com> writes: >> >> > Quoting Milan Zamazal (2025-04-07 09:58:02) >> >> Saturation control is added on top of the colour correction matrix. A >> >> way of saturation adjustment that can be fully integrated into the >> > >> >> colour correction matrix is used. The control is available only if Ccm >> >> algorithm is enabled. >> >> >> >> The control uses 0.0-2.0 value range, with 1.0 being unmodified >> >> saturation, 0.0 full desaturation and 2.0 quite saturated. >> >> >> >> The matrix for saturation adjustment is taken from >> >> https://www.graficaobscura.com/matrix/index.html. The colour correction >> >> matrix is applied before gamma and the given matrix is suitable for such >> >> a case. Alternatively, the transformation used in libcamera rpi ccm.cpp >> >> could be used. >> >> >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> >> --- >> >> src/ipa/simple/algorithms/ccm.cpp | 57 +++++++++++++++++++++++++++++-- >> >> src/ipa/simple/algorithms/ccm.h | 11 ++++++ >> >> src/ipa/simple/ipa_context.h | 4 +++ >> >> 3 files changed, 69 insertions(+), 3 deletions(-) >> >> >> >> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp >> >> index d5ba928d..2700a247 100644 >> >> --- a/src/ipa/simple/algorithms/ccm.cpp >> >> +++ b/src/ipa/simple/algorithms/ccm.cpp >> >> @@ -3,7 +3,7 @@ >> >> * Copyright (C) 2024, Ideas On Board >> >> * Copyright (C) 2024-2025, Red Hat Inc. >> >> * >> >> - * Color correction matrix >> >> + * Color correction matrix + saturation >> >> */ >> >> >> >> #include "ccm.h" >> >> @@ -13,6 +13,8 @@ >> >> >> >> #include <libcamera/control_ids.h> >> >> >> >> +#include "libcamera/internal/matrix.h" >> >> + >> >> namespace { >> >> >> >> constexpr unsigned int kTemperatureThreshold = 100; >> >> @@ -35,28 +37,73 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData >> >> } >> >> >> >> context.ccmEnabled = true; >> >> + context.ctrlMap[&controls::Saturation] = ControlInfo(0.0f, 2.0f, 1.0f); >> >> + >> >> + return 0; >> >> +} >> >> + >> >> +int Ccm::configure(IPAContext &context, >> >> + [[maybe_unused]] const IPAConfigInfo &configInfo) >> >> +{ >> >> + context.activeState.knobs.saturation = std::optional<double>(); >> >> >> >> return 0; >> >> } >> >> >> >> +void Ccm::queueRequest(typename Module::Context &context, >> >> + [[maybe_unused]] const uint32_t frame, >> >> + [[maybe_unused]] typename Module::FrameContext &frameContext, >> >> + const ControlList &controls) >> >> +{ >> >> + const auto &saturation = controls.get(controls::Saturation); >> >> + if (saturation.has_value()) { >> >> + context.activeState.knobs.saturation = saturation; >> >> + LOG(IPASoftCcm, Debug) << "Setting saturation to " << saturation.value(); >> >> + } >> >> +} >> >> + >> >> +void Ccm::updateSaturation(Matrix<float, 3, 3> &ccm, float saturation) >> >> +{ >> >> + /* >> >> + * See https://www.graficaobscura.com/matrix/index.html. >> > >> > Looks like it would be easy to add a brightness control too after this. >> >> Yes, when CCM is used, it's easy to add brightness. And when CCM is not >> used, it's easy too, applying it when making the LUT values -- this is >> important for CPU ISP when CCM is not used (having to use CCM just to >> adjust brightness would be overkill). > > Aha, then indeed lets just apply it directly to the LUT (later) :-) > >> >> >> + * This is applied before gamma thus a matrix for linear RGB must be used. >> >> + * The saturation range is 0..2, with 1 being an unchanged saturation and 0 >> >> + * no saturation (monochrome). >> >> + */ >> >> + constexpr float r = 0.3086; >> >> + constexpr float g = 0.6094; >> >> + constexpr float b = 0.0820; >> > >> > it would be interesting to dig into where these numbers are derived >> > from exactly ... >> >> I guess some educated magic, similarly to other luminance related >> conversions. >> >> > https://dsp.stackexchange.com/questions/27599/how-is-the-luminance-vector-calculated-for-these-linear-matrix-transforms >> > seems to report others have looked, but I don't see a direct answer yet >> > (but I won't dig deep) ... >> > >> > And the referenced article is quite helpful. >> > >> > As this is the 'luminance' vector - I wonder if writing it as >> > RGB<float> luminance = { 0.3086, 0.6094, 0.0820 }; >> > >> > makes sense... >> > >> >> + const float s1 = 1.0 - saturation; >> >> + ccm = ccm * Matrix<float, 3, 3>{ { s1 * r + saturation, s1 * g, s1 * b, >> >> + s1 * r, s1 * g + saturation, s1 * b, >> >> + s1 * r, s1 * g, s1 * b + saturation } }; This multiplication should be also performed in the reverse order. >> > >> > but that would become much more terse ... so I'm not sure it's worth it >> > here? Or would it then be more explicitly readable like the >> > applySaturation in RPi? >> >> I'll try and let's see. >> >> >> +} >> >> + >> >> void Ccm::prepare(IPAContext &context, const uint32_t frame, >> >> IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params) >> >> { >> >> + auto &saturation = context.activeState.knobs.saturation; >> >> + >> >> const unsigned int ct = context.activeState.awb.temperatureK; >> >> >> >> - /* Change CCM only on bigger temperature changes. */ >> >> + /* Change CCM only on saturation or bigger temperature changes. */ >> >> if (frame > 0 && >> >> - utils::abs_diff(ct, lastCt_) < kTemperatureThreshold) { >> >> + utils::abs_diff(ct, lastCt_) < kTemperatureThreshold && >> >> + saturation == lastSaturation_) { >> >> frameContext.ccm.ccm = context.activeState.ccm.ccm; >> >> context.activeState.ccm.changed = false; >> >> return; >> >> } >> >> >> >> lastCt_ = ct; >> >> + lastSaturation_ = saturation; >> >> Matrix<float, 3, 3> ccm = ccm_.getInterpolated(ct); >> >> + if (saturation) >> >> + updateSaturation(ccm, saturation.value()); >> >> >> >> context.activeState.ccm.ccm = ccm; >> >> frameContext.ccm.ccm = ccm; >> >> + frameContext.saturation = saturation; >> >> context.activeState.ccm.changed = true; >> >> } >> >> >> >> @@ -67,6 +114,10 @@ void Ccm::process([[maybe_unused]] IPAContext &context, >> >> ControlList &metadata) >> >> { >> >> metadata.set(controls::ColourCorrectionMatrix, frameContext.ccm.ccm.data()); >> >> + >> >> + const auto &saturation = frameContext.saturation; >> >> + if (saturation) >> >> + metadata.set(controls::Saturation, saturation.value()); >> > >> > This matches what we currently do for Contrast, but I wonder if we >> > should report a metadata of '1.0' for both Contrast and Saturation if >> > they aren't set ... as that's what the 'state' is/should be ? >> >> I don't have an opinion on this, both of "no saturation request -> no >> metadata" and "no saturation request -> the corresponding default value >> in metadata" make sense to me. >> >> > Anyway, I wouldn't block this patch on that - as it would be on top for >> > both controls... >> > >> > Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> > >> > Though, while it's functional - looking at the resulting image in >> > camshark, I think I would expect a saturation of 0.0 to be 'greyscale' - >> > with all pixels having roughly the same r=g=b - and I don't think that's >> > the case ... It looks like there's definitely a bias against green. It >> > looks like R=B, but G is always slightly less by at least ~20% ? ... >> > leaving the image looking slightly tinted purple. >> >> Oh, now, under daylight, I get a prominent purple tint too. >> >> > That could be because I'm running an untuned/identity matrix for CCM >> > perhaps? or is it because white balance needs to be taken into account ? >> >> No, it's a math mistake I did in CCM patches. White balance must be >> applied before CCM. Let C be the CCM, W the AWB gains CCM-like matrix, >> and P the pixel RGB vector. I thought "let's apply C after W" and >> computed (W * C) * P. Which is wrong, to apply C after W means >> C * (W * P) = (C * W) * P. The last matrix applied must be the first >> one in the multiplication. When I swap the multiplication in >> src/ipa/simple/algorithms/lut.cpp from >> >> auto ccm = gainCcm * context.activeState.ccm.ccm; >> >> to >> >> auto ccm = context.activeState.ccm.ccm * gainCcm; >> >> it gets desaturated as it should be (there cannot be any colour cast >> when desaturation is applied correctly, regardless of sensor >> calibration). Maybe this will also explain and improve the colour casts >> I previously experienced when experimenting with my sensor CCMs from >> other pipelines. >> >> > But G = (RB-20%) doesn't sound like white balance to me at the moment... > > aha, so maybe it is ;-) > > Great, well if that's explained ... we have a path forwards. Does it > impact this patch much? Would it help to merge this first? or fix the > CCM then rebase this one ? The latter. I'll post the CCM fix separately. The saturation patch needs a multiplication order fix too and I'd like to simplify updateSaturation() a bit; I'll post v2. Then there is a question what to do about saturation + contrast. I believe contrast should be applied between sensor-CCM and saturation but this would be too expensive on a CPU. > I think the colour tint is the only thing stopping me from adding a tag, > and if that's clarified, then for Saturation: > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > If you respin with the calculation split out in Ccm::updateSaturation() > you can still collect the tag I think. (perhaps there's a new series > with the above change and the color fix on the way?) > > -- > Kieran > >> > >> >> } >> >> >> >> REGISTER_IPA_ALGORITHM(Ccm, "Ccm") >> >> diff --git a/src/ipa/simple/algorithms/ccm.h b/src/ipa/simple/algorithms/ccm.h >> >> index f4e2b85b..2c5d2170 100644 >> >> --- a/src/ipa/simple/algorithms/ccm.h >> >> +++ b/src/ipa/simple/algorithms/ccm.h >> >> @@ -7,6 +7,8 @@ >> >> >> >> #pragma once >> >> >> >> +#include <optional> >> >> + >> >> #include "libcamera/internal/matrix.h" >> >> >> >> #include <libipa/interpolator.h> >> >> @@ -24,6 +26,12 @@ public: >> >> ~Ccm() = default; >> >> >> >> int init(IPAContext &context, const YamlObject &tuningData) override; >> >> + int configure(IPAContext &context, >> >> + const IPAConfigInfo &configInfo) override; >> >> + void queueRequest(typename Module::Context &context, >> >> + const uint32_t frame, >> >> + typename Module::FrameContext &frameContext, >> >> + const ControlList &controls) override; >> >> void prepare(IPAContext &context, >> >> const uint32_t frame, >> >> IPAFrameContext &frameContext, >> >> @@ -34,7 +42,10 @@ public: >> >> ControlList &metadata) override; >> >> >> >> private: >> >> + void updateSaturation(Matrix<float, 3, 3> &ccm, float saturation); >> >> + >> >> unsigned int lastCt_; >> >> + std::optional<float> lastSaturation_; >> >> Interpolator<Matrix<float, 3, 3>> ccm_; >> >> }; >> >> >> >> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h >> >> index 88cc6c35..56792981 100644 >> >> --- a/src/ipa/simple/ipa_context.h >> >> +++ b/src/ipa/simple/ipa_context.h >> >> @@ -63,6 +63,7 @@ struct IPAActiveState { >> >> struct { >> >> /* 0..2 range, 1.0 = normal */ >> >> std::optional<double> contrast; >> >> + std::optional<double> saturation; >> >> } knobs; >> >> }; >> >> >> >> @@ -75,11 +76,14 @@ struct IPAFrameContext : public FrameContext { >> >> int32_t exposure; >> >> double gain; >> >> } sensor; >> >> + >> >> struct { >> >> double red; >> >> double blue; >> >> } gains; >> >> + >> >> std::optional<double> contrast; >> >> + std::optional<double> saturation; >> >> }; >> >> >> >> struct IPAContext { >> >> -- >> >> 2.49.0 >> >> >>
On Mon, Apr 14, 2025 at 04:49:58PM +0200, Milan Zamazal wrote: > Kieran Bingham writes: > > Quoting Milan Zamazal (2025-04-14 14:15:38) > >> Kieran Bingham writes: > >> > Quoting Milan Zamazal (2025-04-07 09:58:02) > >> >> Saturation control is added on top of the colour correction matrix. A > >> >> way of saturation adjustment that can be fully integrated into the > >> >> colour correction matrix is used. The control is available only if Ccm > >> >> algorithm is enabled. > >> >> > >> >> The control uses 0.0-2.0 value range, with 1.0 being unmodified > >> >> saturation, 0.0 full desaturation and 2.0 quite saturated. > >> >> > >> >> The matrix for saturation adjustment is taken from > >> >> https://www.graficaobscura.com/matrix/index.html. The colour correction > >> >> matrix is applied before gamma and the given matrix is suitable for such > >> >> a case. Alternatively, the transformation used in libcamera rpi ccm.cpp > >> >> could be used. > >> >> > >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > >> >> --- > >> >> src/ipa/simple/algorithms/ccm.cpp | 57 +++++++++++++++++++++++++++++-- > >> >> src/ipa/simple/algorithms/ccm.h | 11 ++++++ > >> >> src/ipa/simple/ipa_context.h | 4 +++ > >> >> 3 files changed, 69 insertions(+), 3 deletions(-) > >> >> > >> >> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp > >> >> index d5ba928d..2700a247 100644 > >> >> --- a/src/ipa/simple/algorithms/ccm.cpp > >> >> +++ b/src/ipa/simple/algorithms/ccm.cpp > >> >> @@ -3,7 +3,7 @@ > >> >> * Copyright (C) 2024, Ideas On Board > >> >> * Copyright (C) 2024-2025, Red Hat Inc. > >> >> * > >> >> - * Color correction matrix > >> >> + * Color correction matrix + saturation > >> >> */ > >> >> > >> >> #include "ccm.h" > >> >> @@ -13,6 +13,8 @@ > >> >> > >> >> #include <libcamera/control_ids.h> > >> >> > >> >> +#include "libcamera/internal/matrix.h" > >> >> + > >> >> namespace { > >> >> > >> >> constexpr unsigned int kTemperatureThreshold = 100; > >> >> @@ -35,28 +37,73 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData > >> >> } > >> >> > >> >> context.ccmEnabled = true; > >> >> + context.ctrlMap[&controls::Saturation] = ControlInfo(0.0f, 2.0f, 1.0f); > >> >> + > >> >> + return 0; > >> >> +} > >> >> + > >> >> +int Ccm::configure(IPAContext &context, > >> >> + [[maybe_unused]] const IPAConfigInfo &configInfo) > >> >> +{ > >> >> + context.activeState.knobs.saturation = std::optional<double>(); > >> >> > >> >> return 0; > >> >> } > >> >> > >> >> +void Ccm::queueRequest(typename Module::Context &context, > >> >> + [[maybe_unused]] const uint32_t frame, > >> >> + [[maybe_unused]] typename Module::FrameContext &frameContext, > >> >> + const ControlList &controls) > >> >> +{ > >> >> + const auto &saturation = controls.get(controls::Saturation); > >> >> + if (saturation.has_value()) { > >> >> + context.activeState.knobs.saturation = saturation; > >> >> + LOG(IPASoftCcm, Debug) << "Setting saturation to " << saturation.value(); > >> >> + } > >> >> +} > >> >> + > >> >> +void Ccm::updateSaturation(Matrix<float, 3, 3> &ccm, float saturation) > >> >> +{ > >> >> + /* > >> >> + * See https://www.graficaobscura.com/matrix/index.html. > >> > > >> > Looks like it would be easy to add a brightness control too after this. > >> > >> Yes, when CCM is used, it's easy to add brightness. And when CCM is not > >> used, it's easy too, applying it when making the LUT values -- this is > >> important for CPU ISP when CCM is not used (having to use CCM just to > >> adjust brightness would be overkill). > > > > Aha, then indeed lets just apply it directly to the LUT (later) :-) > > > >> >> + * This is applied before gamma thus a matrix for linear RGB must be used. > >> >> + * The saturation range is 0..2, with 1 being an unchanged saturation and 0 > >> >> + * no saturation (monochrome). > >> >> + */ > >> >> + constexpr float r = 0.3086; > >> >> + constexpr float g = 0.6094; > >> >> + constexpr float b = 0.0820; > >> > > >> > it would be interesting to dig into where these numbers are derived > >> > from exactly ... > >> > >> I guess some educated magic, similarly to other luminance related > >> conversions. There are multiple ways to apply saturation to an RGB value: - Convert to HSV, apply a factor to the saturation, and convert back to RGB. This is rarely (if at all) used in hardware as far as I know. Software implementations are computationally expensive and can't be expressed by a simple matrix multiplication. - Convert to Y'CbCr, apply a factor to the Cb and Cr components, and convert back to RGB. This method is more commonly used in ISPs. As they often output YUV data, the conversion to Y'CbCr is already present in the pipeline. Note that the saturation is applied in non-linear space in this case. Assuming a CSC matrix that converts from RGB to YUV, a saturation factor 'sat' can be applied on RGB data by multiplying the RGB vector by [ 1.0 0.0 0.0 ] CSC^-1 * [ 0.0 sat 0.0 ] * CSC [ 0.0 0.0 sat ] - Desaturating the image completely by replacing the R, G and B components by a luminance value, and interpolating linearly between the saturated and non-saturated pixel values. I'm also not aware of this method being commonly used in hardware. Software implementations are less costly thatn HSV-based saturation as they can be expressed by a matrix multiplication. This is what https://www.graficaobscura.com/matrix/index.html does. Can we express the math below more explicitly, based on either the Y'CbCr method or the interpolation method ? Magic values should also be explained. > >> > https://dsp.stackexchange.com/questions/27599/how-is-the-luminance-vector-calculated-for-these-linear-matrix-transforms > >> > seems to report others have looked, but I don't see a direct answer yet > >> > (but I won't dig deep) ... > >> > > >> > And the referenced article is quite helpful. > >> > > >> > As this is the 'luminance' vector - I wonder if writing it as > >> > RGB<float> luminance = { 0.3086, 0.6094, 0.0820 }; > >> > > >> > makes sense... > >> > > >> >> + const float s1 = 1.0 - saturation; > >> >> + ccm = ccm * Matrix<float, 3, 3>{ { s1 * r + saturation, s1 * g, s1 * b, > >> >> + s1 * r, s1 * g + saturation, s1 * b, > >> >> + s1 * r, s1 * g, s1 * b + saturation } }; > > This multiplication should be also performed in the reverse order. > > >> > > >> > but that would become much more terse ... so I'm not sure it's worth it > >> > here? Or would it then be more explicitly readable like the > >> > applySaturation in RPi? > >> > >> I'll try and let's see. > >> > >> >> +} > >> >> + > >> >> void Ccm::prepare(IPAContext &context, const uint32_t frame, > >> >> IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params) > >> >> { > >> >> + auto &saturation = context.activeState.knobs.saturation; > >> >> + > >> >> const unsigned int ct = context.activeState.awb.temperatureK; > >> >> > >> >> - /* Change CCM only on bigger temperature changes. */ > >> >> + /* Change CCM only on saturation or bigger temperature changes. */ > >> >> if (frame > 0 && > >> >> - utils::abs_diff(ct, lastCt_) < kTemperatureThreshold) { > >> >> + utils::abs_diff(ct, lastCt_) < kTemperatureThreshold && > >> >> + saturation == lastSaturation_) { > >> >> frameContext.ccm.ccm = context.activeState.ccm.ccm; > >> >> context.activeState.ccm.changed = false; > >> >> return; > >> >> } > >> >> > >> >> lastCt_ = ct; > >> >> + lastSaturation_ = saturation; > >> >> Matrix<float, 3, 3> ccm = ccm_.getInterpolated(ct); > >> >> + if (saturation) > >> >> + updateSaturation(ccm, saturation.value()); > >> >> > >> >> context.activeState.ccm.ccm = ccm; > >> >> frameContext.ccm.ccm = ccm; > >> >> + frameContext.saturation = saturation; > >> >> context.activeState.ccm.changed = true; > >> >> } > >> >> > >> >> @@ -67,6 +114,10 @@ void Ccm::process([[maybe_unused]] IPAContext &context, > >> >> ControlList &metadata) > >> >> { > >> >> metadata.set(controls::ColourCorrectionMatrix, frameContext.ccm.ccm.data()); > >> >> + > >> >> + const auto &saturation = frameContext.saturation; > >> >> + if (saturation) > >> >> + metadata.set(controls::Saturation, saturation.value()); > >> > > >> > This matches what we currently do for Contrast, but I wonder if we > >> > should report a metadata of '1.0' for both Contrast and Saturation if > >> > they aren't set ... as that's what the 'state' is/should be ? > >> > >> I don't have an opinion on this, both of "no saturation request -> no > >> metadata" and "no saturation request -> the corresponding default value > >> in metadata" make sense to me. As controls persist, we should keep reporting the value. Every frame should be entirely described by its metadata. > >> > Anyway, I wouldn't block this patch on that - as it would be on top for > >> > both controls... > >> > > >> > Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> > > >> > Though, while it's functional - looking at the resulting image in > >> > camshark, I think I would expect a saturation of 0.0 to be 'greyscale' - > >> > with all pixels having roughly the same r=g=b - and I don't think that's > >> > the case ... It looks like there's definitely a bias against green. It > >> > looks like R=B, but G is always slightly less by at least ~20% ? ... > >> > leaving the image looking slightly tinted purple. > >> > >> Oh, now, under daylight, I get a prominent purple tint too. > >> > >> > That could be because I'm running an untuned/identity matrix for CCM > >> > perhaps? or is it because white balance needs to be taken into account ? > >> > >> No, it's a math mistake I did in CCM patches. White balance must be > >> applied before CCM. Let C be the CCM, W the AWB gains CCM-like matrix, > >> and P the pixel RGB vector. I thought "let's apply C after W" and > >> computed (W * C) * P. Which is wrong, to apply C after W means > >> C * (W * P) = (C * W) * P. The last matrix applied must be the first > >> one in the multiplication. When I swap the multiplication in > >> src/ipa/simple/algorithms/lut.cpp from > >> > >> auto ccm = gainCcm * context.activeState.ccm.ccm; > >> > >> to > >> > >> auto ccm = context.activeState.ccm.ccm * gainCcm; > >> > >> it gets desaturated as it should be (there cannot be any colour cast > >> when desaturation is applied correctly, regardless of sensor > >> calibration). Maybe this will also explain and improve the colour casts > >> I previously experienced when experimenting with my sensor CCMs from > >> other pipelines. > >> > >> > But G = (RB-20%) doesn't sound like white balance to me at the moment... > > > > aha, so maybe it is ;-) > > > > Great, well if that's explained ... we have a path forwards. Does it > > impact this patch much? Would it help to merge this first? or fix the > > CCM then rebase this one ? > > The latter. I'll post the CCM fix separately. The saturation patch > needs a multiplication order fix too and I'd like to simplify > updateSaturation() a bit; I'll post v2. Agreed, fixing the CCM first is better. > Then there is a question what to do about saturation + contrast. I > believe contrast should be applied between sensor-CCM and saturation but > this would be too expensive on a CPU. Contrast is typically applied at the end of the processing pipeline, in non-linear YUV space, at the same time as saturation. Once we will have a GPU-accelerated implementation of the software ISP, applying multiple matrix multiplications in different stages of the pipeline would become much cheaper. I know this may be a controversial opinion, but I think it will be important to have a CPU-based implementation that matches the same processing pipeline as the GPU-accelerated implementation, even if it gets costly for the CPU. > > I think the colour tint is the only thing stopping me from adding a tag, > > and if that's clarified, then for Saturation: > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > If you respin with the calculation split out in Ccm::updateSaturation() > > you can still collect the tag I think. (perhaps there's a new series > > with the above change and the color fix on the way?) > > > >> >> } > >> >> > >> >> REGISTER_IPA_ALGORITHM(Ccm, "Ccm") > >> >> diff --git a/src/ipa/simple/algorithms/ccm.h b/src/ipa/simple/algorithms/ccm.h > >> >> index f4e2b85b..2c5d2170 100644 > >> >> --- a/src/ipa/simple/algorithms/ccm.h > >> >> +++ b/src/ipa/simple/algorithms/ccm.h > >> >> @@ -7,6 +7,8 @@ > >> >> > >> >> #pragma once > >> >> > >> >> +#include <optional> > >> >> + > >> >> #include "libcamera/internal/matrix.h" > >> >> > >> >> #include <libipa/interpolator.h> > >> >> @@ -24,6 +26,12 @@ public: > >> >> ~Ccm() = default; > >> >> > >> >> int init(IPAContext &context, const YamlObject &tuningData) override; > >> >> + int configure(IPAContext &context, > >> >> + const IPAConfigInfo &configInfo) override; > >> >> + void queueRequest(typename Module::Context &context, > >> >> + const uint32_t frame, > >> >> + typename Module::FrameContext &frameContext, > >> >> + const ControlList &controls) override; > >> >> void prepare(IPAContext &context, > >> >> const uint32_t frame, > >> >> IPAFrameContext &frameContext, > >> >> @@ -34,7 +42,10 @@ public: > >> >> ControlList &metadata) override; > >> >> > >> >> private: > >> >> + void updateSaturation(Matrix<float, 3, 3> &ccm, float saturation); > >> >> + > >> >> unsigned int lastCt_; > >> >> + std::optional<float> lastSaturation_; > >> >> Interpolator<Matrix<float, 3, 3>> ccm_; > >> >> }; > >> >> > >> >> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h > >> >> index 88cc6c35..56792981 100644 > >> >> --- a/src/ipa/simple/ipa_context.h > >> >> +++ b/src/ipa/simple/ipa_context.h > >> >> @@ -63,6 +63,7 @@ struct IPAActiveState { > >> >> struct { > >> >> /* 0..2 range, 1.0 = normal */ > >> >> std::optional<double> contrast; > >> >> + std::optional<double> saturation; > >> >> } knobs; > >> >> }; > >> >> > >> >> @@ -75,11 +76,14 @@ struct IPAFrameContext : public FrameContext { > >> >> int32_t exposure; > >> >> double gain; > >> >> } sensor; > >> >> + > >> >> struct { > >> >> double red; > >> >> double blue; > >> >> } gains; > >> >> + > >> >> std::optional<double> contrast; > >> >> + std::optional<double> saturation; > >> >> }; > >> >> > >> >> struct IPAContext {
diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp index d5ba928d..2700a247 100644 --- a/src/ipa/simple/algorithms/ccm.cpp +++ b/src/ipa/simple/algorithms/ccm.cpp @@ -3,7 +3,7 @@ * Copyright (C) 2024, Ideas On Board * Copyright (C) 2024-2025, Red Hat Inc. * - * Color correction matrix + * Color correction matrix + saturation */ #include "ccm.h" @@ -13,6 +13,8 @@ #include <libcamera/control_ids.h> +#include "libcamera/internal/matrix.h" + namespace { constexpr unsigned int kTemperatureThreshold = 100; @@ -35,28 +37,73 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData } context.ccmEnabled = true; + context.ctrlMap[&controls::Saturation] = ControlInfo(0.0f, 2.0f, 1.0f); + + return 0; +} + +int Ccm::configure(IPAContext &context, + [[maybe_unused]] const IPAConfigInfo &configInfo) +{ + context.activeState.knobs.saturation = std::optional<double>(); return 0; } +void Ccm::queueRequest(typename Module::Context &context, + [[maybe_unused]] const uint32_t frame, + [[maybe_unused]] typename Module::FrameContext &frameContext, + const ControlList &controls) +{ + const auto &saturation = controls.get(controls::Saturation); + if (saturation.has_value()) { + context.activeState.knobs.saturation = saturation; + LOG(IPASoftCcm, Debug) << "Setting saturation to " << saturation.value(); + } +} + +void Ccm::updateSaturation(Matrix<float, 3, 3> &ccm, float saturation) +{ + /* + * See https://www.graficaobscura.com/matrix/index.html. + * This is applied before gamma thus a matrix for linear RGB must be used. + * The saturation range is 0..2, with 1 being an unchanged saturation and 0 + * no saturation (monochrome). + */ + constexpr float r = 0.3086; + constexpr float g = 0.6094; + constexpr float b = 0.0820; + const float s1 = 1.0 - saturation; + ccm = ccm * Matrix<float, 3, 3>{ { s1 * r + saturation, s1 * g, s1 * b, + s1 * r, s1 * g + saturation, s1 * b, + s1 * r, s1 * g, s1 * b + saturation } }; +} + void Ccm::prepare(IPAContext &context, const uint32_t frame, IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params) { + auto &saturation = context.activeState.knobs.saturation; + const unsigned int ct = context.activeState.awb.temperatureK; - /* Change CCM only on bigger temperature changes. */ + /* Change CCM only on saturation or bigger temperature changes. */ if (frame > 0 && - utils::abs_diff(ct, lastCt_) < kTemperatureThreshold) { + utils::abs_diff(ct, lastCt_) < kTemperatureThreshold && + saturation == lastSaturation_) { frameContext.ccm.ccm = context.activeState.ccm.ccm; context.activeState.ccm.changed = false; return; } lastCt_ = ct; + lastSaturation_ = saturation; Matrix<float, 3, 3> ccm = ccm_.getInterpolated(ct); + if (saturation) + updateSaturation(ccm, saturation.value()); context.activeState.ccm.ccm = ccm; frameContext.ccm.ccm = ccm; + frameContext.saturation = saturation; context.activeState.ccm.changed = true; } @@ -67,6 +114,10 @@ void Ccm::process([[maybe_unused]] IPAContext &context, ControlList &metadata) { metadata.set(controls::ColourCorrectionMatrix, frameContext.ccm.ccm.data()); + + const auto &saturation = frameContext.saturation; + if (saturation) + metadata.set(controls::Saturation, saturation.value()); } REGISTER_IPA_ALGORITHM(Ccm, "Ccm") diff --git a/src/ipa/simple/algorithms/ccm.h b/src/ipa/simple/algorithms/ccm.h index f4e2b85b..2c5d2170 100644 --- a/src/ipa/simple/algorithms/ccm.h +++ b/src/ipa/simple/algorithms/ccm.h @@ -7,6 +7,8 @@ #pragma once +#include <optional> + #include "libcamera/internal/matrix.h" #include <libipa/interpolator.h> @@ -24,6 +26,12 @@ public: ~Ccm() = default; int init(IPAContext &context, const YamlObject &tuningData) override; + int configure(IPAContext &context, + const IPAConfigInfo &configInfo) override; + void queueRequest(typename Module::Context &context, + const uint32_t frame, + typename Module::FrameContext &frameContext, + const ControlList &controls) override; void prepare(IPAContext &context, const uint32_t frame, IPAFrameContext &frameContext, @@ -34,7 +42,10 @@ public: ControlList &metadata) override; private: + void updateSaturation(Matrix<float, 3, 3> &ccm, float saturation); + unsigned int lastCt_; + std::optional<float> lastSaturation_; Interpolator<Matrix<float, 3, 3>> ccm_; }; diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h index 88cc6c35..56792981 100644 --- a/src/ipa/simple/ipa_context.h +++ b/src/ipa/simple/ipa_context.h @@ -63,6 +63,7 @@ struct IPAActiveState { struct { /* 0..2 range, 1.0 = normal */ std::optional<double> contrast; + std::optional<double> saturation; } knobs; }; @@ -75,11 +76,14 @@ struct IPAFrameContext : public FrameContext { int32_t exposure; double gain; } sensor; + struct { double red; double blue; } gains; + std::optional<double> contrast; + std::optional<double> saturation; }; struct IPAContext {
Saturation control is added on top of the colour correction matrix. A way of saturation adjustment that can be fully integrated into the colour correction matrix is used. The control is available only if Ccm algorithm is enabled. The control uses 0.0-2.0 value range, with 1.0 being unmodified saturation, 0.0 full desaturation and 2.0 quite saturated. The matrix for saturation adjustment is taken from https://www.graficaobscura.com/matrix/index.html. The colour correction matrix is applied before gamma and the given matrix is suitable for such a case. Alternatively, the transformation used in libcamera rpi ccm.cpp could be used. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- src/ipa/simple/algorithms/ccm.cpp | 57 +++++++++++++++++++++++++++++-- src/ipa/simple/algorithms/ccm.h | 11 ++++++ src/ipa/simple/ipa_context.h | 4 +++ 3 files changed, 69 insertions(+), 3 deletions(-)