| Message ID | 6f68162366f6bcb48dea57054c5733d5@inspiredexperts.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
James Alexander <opensource@inspiredexperts.com> writes: > SoftISP tuning files can enable the Adjust algorithm, but they cannot > select the default Contrast control value. This forces applications to > supply the control on every request when a sensor needs a non-neutral > default. > > Read an optional defaultContrast value from the Adjust tuning section, > validate it against the advertised control range, and use it as both the > control default and initial state. Tuning files that omit the value > retain > the existing neutral default, and per-request controls remain > authoritative. > > Since contrast now always has an initial value, make its active and > per-frame > state non-optional and remove the fallback paths. > > Signed-off-by: James Alexander <opensource@inspiredexperts.com> Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > --- > Changes in v3: > - Rename the tuning key to defaultContrast. > - Make the active and per-frame contrast state non-optional. > - Drop the standalone Adjust test pending a common IPA algorithm test > structure. > > Tested with a simple-pipeline/softisp build and all seven existing IPA > tests > on x86_64. > > Changes in v2: > - Rebase the change from the simple IPA onto the current softisp Adjust > algorithm. > - Limit the tuning option to contrast. Gamma is being moved to the > common > libipa GammaAlgorithm, and saturation remains tied to CCM behavior. > - Reject malformed and out-of-range values instead of clamping them. > - Add focused tests for fallback, tuning, request override, and > validation. > > src/ipa/softisp/algorithms/adjust.cpp | 33 > ++++++++++++++++++++++++++------- > src/ipa/softisp/algorithms/adjust.h | 3 +++ > src/ipa/softisp/ipa_context.h | 4 ++-- > 3 files changed, 31 insertions(+), 9 deletions(-) > > diff --git a/src/ipa/softisp/algorithms/adjust.cpp > b/src/ipa/softisp/algorithms/adjust.cpp > index 56e2cf0e8..6e6010bc4 100644 > --- a/src/ipa/softisp/algorithms/adjust.cpp > +++ b/src/ipa/softisp/algorithms/adjust.cpp > @@ -8,6 +8,8 @@ > > #include "adjust.h" > > +#include <errno.h> > + > #include <libcamera/base/log.h> > #include <libcamera/base/utils.h> > > @@ -19,17 +21,34 @@ namespace libcamera { > > namespace ipa::softisp::algorithms { > > -constexpr float kDefaultContrast = 1.0f; > constexpr float kDefaultSaturation = 1.0f; > > +constexpr float kMinContrast = 0.0f; > +constexpr float kMaxContrast = 2.0f; > + > LOG_DEFINE_CATEGORY(IPASoftIspAdjust) > > -int Adjust::init(IPAContext &context, [[maybe_unused]] const ValueNode > &tuningData) > +int Adjust::init(IPAContext &context, const ValueNode &tuningData) > { > + const ValueNode &contrastNode = tuningData["defaultContrast"]; > + const std::optional<float> contrast = contrastNode.get<float>(); > + if (contrastNode && !contrast) { > + LOG(IPASoftIspAdjust, Error) << "Failed to parse defaultContrast"; > + return -EINVAL; > + } > + > + defaultContrast_ = contrast.value_or(kDefaultContrast); > + if (defaultContrast_ < kMinContrast || defaultContrast_ > > kMaxContrast) { > + LOG(IPASoftIspAdjust, Error) > + << "defaultContrast must be in the range [" << kMinContrast > + << ", " << kMaxContrast << "]"; > + return -EINVAL; > + } > + > context.ctrlMap[&controls::Gamma] = > ControlInfo(0.1f, 10.0f, kDefaultGamma); > context.ctrlMap[&controls::Contrast] = > - ControlInfo(0.0f, 2.0f, kDefaultContrast); > + ControlInfo(kMinContrast, kMaxContrast, defaultContrast_); > if (context.ccmEnabled) > context.ctrlMap[&controls::Saturation] = > ControlInfo(0.0f, 2.0f, kDefaultSaturation); > @@ -40,7 +59,7 @@ int Adjust::configure(IPAContext &context, > [[maybe_unused]] const IPAConfigInfo &configInfo) > { > context.activeState.knobs.gamma = kDefaultGamma; > - context.activeState.knobs.contrast = std::optional<float>(); > + context.activeState.knobs.contrast = defaultContrast_; > context.activeState.knobs.saturation = std::optional<float>(); > > return 0; > @@ -59,7 +78,7 @@ void Adjust::queueRequest(typename Module::Context > &context, > > const auto &contrast = controls.get(controls::Contrast); > if (contrast.has_value()) { > - context.activeState.knobs.contrast = contrast; > + context.activeState.knobs.contrast = contrast.value(); > LOG(IPASoftIspAdjust, Debug) << "Setting contrast to " << > contrast.value(); > } > > @@ -107,7 +126,7 @@ void Adjust::prepare(IPAContext &context, > } > > params->gamma = 1.0 / context.activeState.knobs.gamma; > - const float contrast = > context.activeState.knobs.contrast.value_or(kDefaultContrast); > + const float contrast = context.activeState.knobs.contrast; > params->contrastExp = tan(std::clamp(contrast * M_PI_4, 0.0, M_PI_2 - > 0.00001)); > } > > @@ -121,7 +140,7 @@ void Adjust::process([[maybe_unused]] IPAContext > &context, > metadata.set(controls::Gamma, gamma); > > const auto &contrast = frameContext.contrast; > - metadata.set(controls::Contrast, contrast.value_or(kDefaultContrast)); > + metadata.set(controls::Contrast, contrast); > > const auto &saturation = frameContext.saturation; > metadata.set(controls::Saturation, > saturation.value_or(kDefaultSaturation)); > diff --git a/src/ipa/softisp/algorithms/adjust.h > b/src/ipa/softisp/algorithms/adjust.h > index 1acf7cdf1..726e24cfc 100644 > --- a/src/ipa/softisp/algorithms/adjust.h > +++ b/src/ipa/softisp/algorithms/adjust.h > @@ -18,6 +18,7 @@ namespace libcamera { > namespace ipa::softisp::algorithms { > > constexpr float kDefaultGamma = 2.2f; > +constexpr float kDefaultContrast = 1.0f; > > class Adjust : public Algorithm > { > @@ -43,6 +44,8 @@ public: > > private: > void applySaturation(Matrix<float, 3, 3> &ccm, float saturation); > + > + float defaultContrast_ = kDefaultContrast; > }; > > } /* namespace ipa::softisp::algorithms */ > diff --git a/src/ipa/softisp/ipa_context.h > b/src/ipa/softisp/ipa_context.h > index 397e6c99b..6c1a517d1 100644 > --- a/src/ipa/softisp/ipa_context.h > +++ b/src/ipa/softisp/ipa_context.h > @@ -52,7 +52,7 @@ struct IPAActiveState { > struct { > float gamma; > /* 0..2 range, 1.0 = normal */ > - std::optional<float> contrast; > + float contrast; > std::optional<float> saturation; > } knobs; > }; > @@ -68,7 +68,7 @@ struct IPAFrameContext : public FrameContext { > } sensor; > > float gamma; > - std::optional<float> contrast; > + float contrast; > std::optional<float> saturation; > };
2026. 09. 27. 1:12 keltezéssel, James Alexander írta: > SoftISP tuning files can enable the Adjust algorithm, but they cannot > select the default Contrast control value. This forces applications to > supply the control on every request when a sensor needs a non-neutral The desired contrast only needs to be provided once, not with every request. > default. > > Read an optional defaultContrast value from the Adjust tuning section, > validate it against the advertised control range, and use it as both the > control default and initial state. Tuning files that omit the value > retain > the existing neutral default, and per-request controls remain > authoritative. > > Since contrast now always has an initial value, make its active and > per-frame > state non-optional and remove the fallback paths. > > Signed-off-by: James Alexander <opensource@inspiredexperts.com> > --- > Changes in v3: > - Rename the tuning key to defaultContrast. > - Make the active and per-frame contrast state non-optional. > - Drop the standalone Adjust test pending a common IPA algorithm test > structure. > > Tested with a simple-pipeline/softisp build and all seven existing IPA > tests > on x86_64. > > Changes in v2: > - Rebase the change from the simple IPA onto the current softisp Adjust > algorithm. > - Limit the tuning option to contrast. Gamma is being moved to the > common > libipa GammaAlgorithm, and saturation remains tied to CCM behavior. > - Reject malformed and out-of-range values instead of clamping them. > - Add focused tests for fallback, tuning, request override, and > validation. > You're missing a `---` here. > src/ipa/softisp/algorithms/adjust.cpp | 33 > ++++++++++++++++++++++++++------- > src/ipa/softisp/algorithms/adjust.h | 3 +++ > src/ipa/softisp/ipa_context.h | 4 ++-- > 3 files changed, 31 insertions(+), 9 deletions(-) > > diff --git a/src/ipa/softisp/algorithms/adjust.cpp > b/src/ipa/softisp/algorithms/adjust.cpp Is there a stray new line before the "b"? Indeed, there are multiple unnecessary newlines in the email that make the patch inapplicable. > index 56e2cf0e8..6e6010bc4 100644 > --- a/src/ipa/softisp/algorithms/adjust.cpp > +++ b/src/ipa/softisp/algorithms/adjust.cpp > @@ -8,6 +8,8 @@ > > #include "adjust.h" > > +#include <errno.h> > + > #include <libcamera/base/log.h> > #include <libcamera/base/utils.h> > > @@ -19,17 +21,34 @@ namespace libcamera { > > namespace ipa::softisp::algorithms { > > -constexpr float kDefaultContrast = 1.0f; > constexpr float kDefaultSaturation = 1.0f; > > +constexpr float kMinContrast = 0.0f; > +constexpr float kMaxContrast = 2.0f; > + > LOG_DEFINE_CATEGORY(IPASoftIspAdjust) > > -int Adjust::init(IPAContext &context, [[maybe_unused]] const ValueNode > &tuningData) > +int Adjust::init(IPAContext &context, const ValueNode &tuningData) > { > + const ValueNode &contrastNode = tuningData["defaultContrast"]; I see the key was changed due to a previous review comment. I just want to note e.g. libipa/gamma.cpp, that is somewhat similar, and that uses the simple "gamma" key for its default value. I feel like we should go with "contrast" here. If there is a need for setting the min/max/etc. of the control, the the value can simply be a dictionary. E.g. contrast: 1.0 # 1.0 is the default ... contrast: default: 1.0 min: ... max: ... This is even relatively easy to in a backwards compatible manner. So I cannot really a see a drawback with going for contrast: <default value> for now. > + const std::optional<float> contrast = contrastNode.get<float>(); > + if (contrastNode && !contrast) { > + LOG(IPASoftIspAdjust, Error) << "Failed to parse defaultContrast"; > + return -EINVAL; > + } > + > + defaultContrast_ = contrast.value_or(kDefaultContrast); > + if (defaultContrast_ < kMinContrast || defaultContrast_ > > kMaxContrast) { > + LOG(IPASoftIspAdjust, Error) > + << "defaultContrast must be in the range [" << kMinContrast > + << ", " << kMaxContrast << "]"; > + return -EINVAL; > + } > + > context.ctrlMap[&controls::Gamma] = > ControlInfo(0.1f, 10.0f, kDefaultGamma); > context.ctrlMap[&controls::Contrast] = > - ControlInfo(0.0f, 2.0f, kDefaultContrast); > + ControlInfo(kMinContrast, kMaxContrast, defaultContrast_); > if (context.ccmEnabled) > context.ctrlMap[&controls::Saturation] = > ControlInfo(0.0f, 2.0f, kDefaultSaturation); > @@ -40,7 +59,7 @@ int Adjust::configure(IPAContext &context, > [[maybe_unused]] const IPAConfigInfo &configInfo) > { > context.activeState.knobs.gamma = kDefaultGamma; > - context.activeState.knobs.contrast = std::optional<float>(); > + context.activeState.knobs.contrast = defaultContrast_; > context.activeState.knobs.saturation = std::optional<float>(); > > return 0; > @@ -59,7 +78,7 @@ void Adjust::queueRequest(typename Module::Context > &context, > > const auto &contrast = controls.get(controls::Contrast); > if (contrast.has_value()) { > - context.activeState.knobs.contrast = contrast; > + context.activeState.knobs.contrast = contrast.value(); > LOG(IPASoftIspAdjust, Debug) << "Setting contrast to " << > contrast.value(); > } > > @@ -107,7 +126,7 @@ void Adjust::prepare(IPAContext &context, > } > > params->gamma = 1.0 / context.activeState.knobs.gamma; > - const float contrast = > context.activeState.knobs.contrast.value_or(kDefaultContrast); > + const float contrast = context.activeState.knobs.contrast; > params->contrastExp = tan(std::clamp(contrast * M_PI_4, 0.0, M_PI_2 - > 0.00001)); > } > > @@ -121,7 +140,7 @@ void Adjust::process([[maybe_unused]] IPAContext > &context, > metadata.set(controls::Gamma, gamma); > > const auto &contrast = frameContext.contrast; > - metadata.set(controls::Contrast, contrast.value_or(kDefaultContrast)); > + metadata.set(controls::Contrast, contrast); metadat.set(controls::Contrast, frameContext.contrast); > > const auto &saturation = frameContext.saturation; > metadata.set(controls::Saturation, > saturation.value_or(kDefaultSaturation)); > diff --git a/src/ipa/softisp/algorithms/adjust.h > b/src/ipa/softisp/algorithms/adjust.h > index 1acf7cdf1..726e24cfc 100644 > --- a/src/ipa/softisp/algorithms/adjust.h > +++ b/src/ipa/softisp/algorithms/adjust.h > @@ -18,6 +18,7 @@ namespace libcamera { > namespace ipa::softisp::algorithms { > > constexpr float kDefaultGamma = 2.2f; > +constexpr float kDefaultContrast = 1.0f; > > class Adjust : public Algorithm > { > @@ -43,6 +44,8 @@ public: > > private: > void applySaturation(Matrix<float, 3, 3> &ccm, float saturation); > + > + float defaultContrast_ = kDefaultContrast; > }; > > } /* namespace ipa::softisp::algorithms */ > diff --git a/src/ipa/softisp/ipa_context.h > b/src/ipa/softisp/ipa_context.h > index 397e6c99b..6c1a517d1 100644 > --- a/src/ipa/softisp/ipa_context.h > +++ b/src/ipa/softisp/ipa_context.h > @@ -52,7 +52,7 @@ struct IPAActiveState { > struct { > float gamma; > /* 0..2 range, 1.0 = normal */ > - std::optional<float> contrast; > + float contrast; > std::optional<float> saturation; > } knobs; > }; > @@ -68,7 +68,7 @@ struct IPAFrameContext : public FrameContext { > } sensor; > > float gamma; > - std::optional<float> contrast; > + float contrast; > std::optional<float> saturation; > }; > > -- > 2.53.0
diff --git a/src/ipa/softisp/algorithms/adjust.cpp b/src/ipa/softisp/algorithms/adjust.cpp index 56e2cf0e8..6e6010bc4 100644 --- a/src/ipa/softisp/algorithms/adjust.cpp +++ b/src/ipa/softisp/algorithms/adjust.cpp @@ -8,6 +8,8 @@ #include "adjust.h" +#include <errno.h> + #include <libcamera/base/log.h> #include <libcamera/base/utils.h> @@ -19,17 +21,34 @@ namespace libcamera { namespace ipa::softisp::algorithms { -constexpr float kDefaultContrast = 1.0f; constexpr float kDefaultSaturation = 1.0f; +constexpr float kMinContrast = 0.0f; +constexpr float kMaxContrast = 2.0f; + LOG_DEFINE_CATEGORY(IPASoftIspAdjust) -int Adjust::init(IPAContext &context, [[maybe_unused]] const ValueNode &tuningData) +int Adjust::init(IPAContext &context, const ValueNode &tuningData) { + const ValueNode &contrastNode = tuningData["defaultContrast"]; + const std::optional<float> contrast = contrastNode.get<float>(); + if (contrastNode && !contrast) { + LOG(IPASoftIspAdjust, Error) << "Failed to parse defaultContrast"; + return -EINVAL; + } + + defaultContrast_ = contrast.value_or(kDefaultContrast); + if (defaultContrast_ < kMinContrast || defaultContrast_ > kMaxContrast) { + LOG(IPASoftIspAdjust, Error) + << "defaultContrast must be in the range [" << kMinContrast + << ", " << kMaxContrast << "]"; + return -EINVAL; + } + context.ctrlMap[&controls::Gamma] = ControlInfo(0.1f, 10.0f, kDefaultGamma); context.ctrlMap[&controls::Contrast] = - ControlInfo(0.0f, 2.0f, kDefaultContrast); + ControlInfo(kMinContrast, kMaxContrast, defaultContrast_); if (context.ccmEnabled) context.ctrlMap[&controls::Saturation] = ControlInfo(0.0f, 2.0f, kDefaultSaturation); @@ -40,7 +59,7 @@ int Adjust::configure(IPAContext &context, [[maybe_unused]] const IPAConfigInfo &configInfo) { context.activeState.knobs.gamma = kDefaultGamma; - context.activeState.knobs.contrast = std::optional<float>(); + context.activeState.knobs.contrast = defaultContrast_; context.activeState.knobs.saturation = std::optional<float>(); return 0; @@ -59,7 +78,7 @@ void Adjust::queueRequest(typename Module::Context &context, const auto &contrast = controls.get(controls::Contrast); if (contrast.has_value()) { - context.activeState.knobs.contrast = contrast; + context.activeState.knobs.contrast = contrast.value(); LOG(IPASoftIspAdjust, Debug) << "Setting contrast to " << contrast.value(); } @@ -107,7 +126,7 @@ void Adjust::prepare(IPAContext &context, } params->gamma = 1.0 / context.activeState.knobs.gamma; - const float contrast = context.activeState.knobs.contrast.value_or(kDefaultContrast); + const float contrast = context.activeState.knobs.contrast; params->contrastExp = tan(std::clamp(contrast * M_PI_4, 0.0, M_PI_2 - 0.00001)); } @@ -121,7 +140,7 @@ void Adjust::process([[maybe_unused]] IPAContext &context, metadata.set(controls::Gamma, gamma); const auto &contrast = frameContext.contrast; - metadata.set(controls::Contrast, contrast.value_or(kDefaultContrast)); + metadata.set(controls::Contrast, contrast); const auto &saturation = frameContext.saturation; metadata.set(controls::Saturation, saturation.value_or(kDefaultSaturation)); diff --git a/src/ipa/softisp/algorithms/adjust.h b/src/ipa/softisp/algorithms/adjust.h index 1acf7cdf1..726e24cfc 100644 --- a/src/ipa/softisp/algorithms/adjust.h +++ b/src/ipa/softisp/algorithms/adjust.h @@ -18,6 +18,7 @@ namespace libcamera { namespace ipa::softisp::algorithms { constexpr float kDefaultGamma = 2.2f; +constexpr float kDefaultContrast = 1.0f; class Adjust : public Algorithm { @@ -43,6 +44,8 @@ public: private: void applySaturation(Matrix<float, 3, 3> &ccm, float saturation); + + float defaultContrast_ = kDefaultContrast; }; } /* namespace ipa::softisp::algorithms */ diff --git a/src/ipa/softisp/ipa_context.h b/src/ipa/softisp/ipa_context.h index 397e6c99b..6c1a517d1 100644 --- a/src/ipa/softisp/ipa_context.h +++ b/src/ipa/softisp/ipa_context.h @@ -52,7 +52,7 @@ struct IPAActiveState { struct { float gamma; /* 0..2 range, 1.0 = normal */ - std::optional<float> contrast; + float contrast; std::optional<float> saturation; } knobs; }; @@ -68,7 +68,7 @@ struct IPAFrameContext : public FrameContext { } sensor; float gamma; - std::optional<float> contrast; + float contrast; std::optional<float> saturation; };
SoftISP tuning files can enable the Adjust algorithm, but they cannot select the default Contrast control value. This forces applications to supply the control on every request when a sensor needs a non-neutral default. Read an optional defaultContrast value from the Adjust tuning section, validate it against the advertised control range, and use it as both the control default and initial state. Tuning files that omit the value retain the existing neutral default, and per-request controls remain authoritative. Since contrast now always has an initial value, make its active and per-frame state non-optional and remove the fallback paths. Signed-off-by: James Alexander <opensource@inspiredexperts.com> --- Changes in v3: - Rename the tuning key to defaultContrast. - Make the active and per-frame contrast state non-optional. - Drop the standalone Adjust test pending a common IPA algorithm test structure. Tested with a simple-pipeline/softisp build and all seven existing IPA tests on x86_64. Changes in v2: - Rebase the change from the simple IPA onto the current softisp Adjust algorithm. - Limit the tuning option to contrast. Gamma is being moved to the common libipa GammaAlgorithm, and saturation remains tied to CCM behavior. - Reject malformed and out-of-range values instead of clamping them. - Add focused tests for fallback, tuning, request override, and validation. src/ipa/softisp/algorithms/adjust.cpp | 33 ++++++++++++++++++++++++++------- src/ipa/softisp/algorithms/adjust.h | 3 +++ src/ipa/softisp/ipa_context.h | 4 ++-- 3 files changed, 31 insertions(+), 9 deletions(-)