[PATCH v2 1/1] libcamera: software_isp: Add saturation control
Milan Zamazal
mzamazal at redhat.com
Tue May 13 16:46:26 CEST 2025
Hi Paul,
thank you for review.
Paul Elder <paul.elder at ideasonboard.com> writes:
> Hi Milan,
>
> Thanks for the patch.
>
> Quoting Milan Zamazal (2025-04-30 13:20:42)
>> Saturation control is added on top of the colour correction matrix. A
>> way of saturation adjustment that can be fully integrated into the
>
> s/way/method/ I think would be clearer (without having to reorganize the entire
> sentence)
>
>> 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 saturation is adjusted by converting to Y'CbCr colour space,
>> applying the saturation value on the colour axes, and converting back to
>> RGB. ITU-R BT.601 conversion is used to convert between the colour
>> spaces, for no particular reason.
>>
>> 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 at redhat.com>
>> ---
>> src/ipa/simple/algorithms/ccm.cpp | 60 +++++++++++++++++++++++++++++--
>> src/ipa/simple/algorithms/ccm.h | 11 ++++++
>> src/ipa/simple/ipa_context.h | 4 +++
>> 3 files changed, 72 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp
>> index d5ba928d..021aefc9 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,77 @@ 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)
>
> I think applySaturation might be a more appropriate name. updateSaturation
> sounds like saturation is the output value and it gets updated based on some
> input.
>
>> +{
>> + /* https://en.wikipedia.org/wiki/YCbCr#ITU-R_BT.601_conversion */
>> + const Matrix<float, 3, 3> rgb2ycbcr{
>> + { 0.256788235294, 0.504129411765, 0.0979058823529,
>> + -0.148223529412, -0.290992156863, 0.439215686275,
>> + 0.439215686275, -0.367788235294, -0.0714274509804 }
>> + };
>> + const Matrix<float, 3, 3> ycbcr2rgb{
>> + { 1.16438356164, 0, 1.59602678571,
>> + 1.16438356164, -0.391762290094, -0.812967647235,
>> + 1.16438356164, 2.01723214285, 0 }
>> + };
>> + const Matrix<float, 3, 3> saturationMatrix{
>> + { 1, 0, 0,
>> + 0, saturation, 0,
>> + 0, 0, saturation }
>> + };
>> + ccm = ycbcr2rgb * saturationMatrix * rgb2ycbcr * ccm;
>> +}
>> +
>> 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 +118,9 @@ void Ccm::process([[maybe_unused]] IPAContext &context,
>> ControlList &metadata)
>> {
>> metadata.set(controls::ColourCorrectionMatrix, frameContext.ccm.ccm.data());
>> +
>> + const auto &saturation = frameContext.saturation;
>> + metadata.set(controls::Saturation, saturation.value_or(1.0));
>> }
>>
>> 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;
>
> Is there a reason why this is double while lastSaturation_ is float? The
> Saturation control is also float.
I think it was just a mistake. I'll fix this and apply the other
improvements you suggested.
>> } 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;
>
> Same here.
>
>
> Otherwise, looks good.
>
> Thanks,
>
> Paul
>
>> };
>>
>> struct IPAContext {
>> --
>> 2.49.0
>>
More information about the libcamera-devel
mailing list