[v2] ipa: softisp: adjust: Read default contrast from tuning

28310 diff mbox series

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 contrast 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.

Add focused tests for the neutral fallback, a tuned default, a request
override, and invalid tuning.

Signed-off-by: James Alexander <opensource@inspiredexperts.com>
---
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 |  27 +++++-
 src/ipa/softisp/algorithms/adjust.h   |   3 +
 test/ipa/meson.build                  |   1 +
 test/ipa/softisp/adjust.cpp           | 125 ++++++++++++++++++++++++++
 test/ipa/softisp/meson.build          |  17 ++++
 5 files changed, 169 insertions(+), 4 deletions(-)
 create mode 100644 test/ipa/softisp/adjust.cpp
 create mode 100644 test/ipa/softisp/meson.build

Hi James,

thank you for the patch.

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 contrast 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.
>
> Add focused tests for the neutral fallback, a tuned default, a request
> override, and invalid tuning.
>
> Signed-off-by: James Alexander <opensource@inspiredexperts.com>
> ---
> 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 |  27 +++++-
>  src/ipa/softisp/algorithms/adjust.h   |   3 +
>  test/ipa/meson.build                  |   1 +
>  test/ipa/softisp/adjust.cpp           | 125 ++++++++++++++++++++++++++
>  test/ipa/softisp/meson.build          |  17 ++++
>  5 files changed, 169 insertions(+), 4 deletions(-)
>  create mode 100644 test/ipa/softisp/adjust.cpp
>  create mode 100644 test/ipa/softisp/meson.build
>
> diff --git a/src/ipa/softisp/algorithms/adjust.cpp b/src/ipa/softisp/algorithms/adjust.cpp
> index 56e2cf0..8672033 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["contrast"];

It may be worth to think a bit more about the node name, in case we'd
like to add more.  Maybe "defaultContrast" or something like

  contrast:
    default: VALUE

would be more future-proof?

Otherwise it looks good to me.

Reviewed-by: Milan Zamazal <mzamazal@redhat.com>

> +	const std::optional<float> contrast = contrastNode.get<float>();
> +	if (contrastNode && !contrast) {
> +		LOG(IPASoftIspAdjust, Error) << "Failed to parse contrast";
> +		return -EINVAL;
> +	}
> +
> +	defaultContrast_ = contrast.value_or(kDefaultContrast);
> +	if (defaultContrast_ < kMinContrast || defaultContrast_ > kMaxContrast) {
> +		LOG(IPASoftIspAdjust, Error)
> +			<< "Contrast 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;
> diff --git a/src/ipa/softisp/algorithms/adjust.h b/src/ipa/softisp/algorithms/adjust.h
> index 1acf7cd..726e24c 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/test/ipa/meson.build b/test/ipa/meson.build
> index ceed15b..f249091 100644
> --- a/test/ipa/meson.build
> +++ b/test/ipa/meson.build
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: CC0-1.0
>  
>  subdir('libipa')
> +subdir('softisp')
>  
>  ipa_test = [
>      {'name': 'ipa_module_test', 'sources': ['ipa_module_test.cpp']},
> diff --git a/test/ipa/softisp/adjust.cpp b/test/ipa/softisp/adjust.cpp
> new file mode 100644
> index 0000000..24e862b
> --- /dev/null
> +++ b/test/ipa/softisp/adjust.cpp
> @@ -0,0 +1,125 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2026, James Alexander
> + *
> + * Soft ISP image adjustment algorithm tests
> + */
> +
> +#include <cmath>
> +#include <errno.h>
> +#include <iostream>
> +#include <memory>
> +
> +#include <libcamera/control_ids.h>
> +
> +#include "libcamera/internal/value_node.h"
> +
> +#include "algorithms/adjust.h"
> +
> +#include "test.h"
> +
> +using namespace libcamera;
> +using namespace libcamera::ipa::softisp;
> +using namespace libcamera::ipa::softisp::algorithms;
> +
> +namespace {
> +
> +constexpr float kEpsilon = 0.0001f;
> +
> +bool closeEnough(float lhs, float rhs)
> +{
> +	return std::abs(lhs - rhs) < kEpsilon;
> +}
> +
> +} /* namespace */
> +
> +class AdjustTest : public Test
> +{
> +protected:
> +	int testDefaults()
> +	{
> +		IPAContext context(1);
> +		ValueNode tuning;
> +		Adjust adjust;
> +
> +		if (adjust.init(context, tuning))
> +			return TestFail;
> +
> +		IPAConfigInfo configInfo{};
> +		if (adjust.configure(context, configInfo))
> +			return TestFail;
> +
> +		const float contrast = context.ctrlMap.at(&controls::Contrast).def().get<float>();
> +		if (!closeEnough(contrast, 1.0f) ||
> +		    !context.activeState.knobs.contrast ||
> +		    !closeEnough(*context.activeState.knobs.contrast, 1.0f)) {
> +			std::cerr << "Default contrast was not preserved" << std::endl;
> +			return TestFail;
> +		}
> +
> +		return TestPass;
> +	}
> +
> +	int testTunedDefaultAndControl()
> +	{
> +		IPAContext context(1);
> +		ValueNode tuning;
> +		tuning.add("contrast", std::make_unique<ValueNode>(1.15f));
> +		Adjust adjust;
> +
> +		if (adjust.init(context, tuning))
> +			return TestFail;
> +
> +		IPAConfigInfo configInfo{};
> +		if (adjust.configure(context, configInfo))
> +			return TestFail;
> +
> +		const float contrast = context.ctrlMap.at(&controls::Contrast).def().get<float>();
> +		if (!closeEnough(contrast, 1.15f) ||
> +		    !context.activeState.knobs.contrast ||
> +		    !closeEnough(*context.activeState.knobs.contrast, 1.15f)) {
> +			std::cerr << "Tuned contrast default was not applied" << std::endl;
> +			return TestFail;
> +		}
> +
> +		ControlList controlsList(controls::controls);
> +		controlsList.set(controls::Contrast, 0.9f);
> +		IPAFrameContext frameContext{};
> +		adjust.queueRequest(context, 0, frameContext, controlsList);
> +
> +		if (!context.activeState.knobs.contrast ||
> +		    !closeEnough(*context.activeState.knobs.contrast, 0.9f)) {
> +			std::cerr << "Request control did not override tuned contrast" << std::endl;
> +			return TestFail;
> +		}
> +
> +		return TestPass;
> +	}
> +
> +	int testInvalidTuning()
> +	{
> +		IPAContext context(1);
> +		ValueNode tuning;
> +		tuning.add("contrast", std::make_unique<ValueNode>(3.0f));
> +		Adjust adjust;
> +
> +		if (adjust.init(context, tuning) != -EINVAL) {
> +			std::cerr << "Out-of-range contrast tuning was accepted" << std::endl;
> +			return TestFail;
> +		}
> +
> +		return TestPass;
> +	}
> +
> +	int run() override
> +	{
> +		if (testDefaults() != TestPass ||
> +		    testTunedDefaultAndControl() != TestPass ||
> +		    testInvalidTuning() != TestPass)
> +			return TestFail;
> +
> +		return TestPass;
> +	}
> +};
> +
> +TEST_REGISTER(AdjustTest)
> diff --git a/test/ipa/softisp/meson.build b/test/ipa/softisp/meson.build
> new file mode 100644
> index 0000000..78ad612
> --- /dev/null
> +++ b/test/ipa/softisp/meson.build
> @@ -0,0 +1,17 @@
> +# SPDX-License-Identifier: CC0-1.0
> +
> +softisp_adjust_test = executable(
> +    'softisp_adjust',
> +    [
> +        'adjust.cpp',
> +        '../../../src/ipa/softisp/algorithms/adjust.cpp',
> +    ],
> +    dependencies : [libcamera_private, libipa_dep],
> +    link_with : [test_libraries],
> +    include_directories : [
> +        test_includes_internal,
> +        include_directories('../../../src/ipa/softisp'),
> +    ],
> +)
> +
> +test('softisp_adjust', softisp_adjust_test, suite : 'ipa')

Hi

2026. 09. 16. 21:27 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
> default.
> 
> Read an optional contrast 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.
> 
> Add focused tests for the neutral fallback, a tuned default, a request
> override, and invalid tuning.
> 
> Signed-off-by: James Alexander <opensource@inspiredexperts.com>
> ---
> 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 |  27 +++++-
>   src/ipa/softisp/algorithms/adjust.h   |   3 +
>   test/ipa/meson.build                  |   1 +
>   test/ipa/softisp/adjust.cpp           | 125 ++++++++++++++++++++++++++
>   test/ipa/softisp/meson.build          |  17 ++++
>   5 files changed, 169 insertions(+), 4 deletions(-)
>   create mode 100644 test/ipa/softisp/adjust.cpp
>   create mode 100644 test/ipa/softisp/meson.build
> 
> diff --git a/src/ipa/softisp/algorithms/adjust.cpp b/src/ipa/softisp/algorithms/adjust.cpp
> index 56e2cf0..8672033 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["contrast"];
> +	const std::optional<float> contrast = contrastNode.get<float>();
> +	if (contrastNode && !contrast) {
> +		LOG(IPASoftIspAdjust, Error) << "Failed to parse contrast";
> +		return -EINVAL;
> +	}
> +
> +	defaultContrast_ = contrast.value_or(kDefaultContrast);
> +	if (defaultContrast_ < kMinContrast || defaultContrast_ > kMaxContrast) {
> +		LOG(IPASoftIspAdjust, Error)
> +			<< "Contrast 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_;

I think now it would make sense to potentially make `frameContext.contrast`
and `context.activeState.knobs.contrast` just simple `float`s. There doesn't
seem to be a need for an optional.


>   	context.activeState.knobs.saturation = std::optional<float>();
>   
>   	return 0;
> diff --git a/src/ipa/softisp/algorithms/adjust.h b/src/ipa/softisp/algorithms/adjust.h
> index 1acf7cd..726e24c 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/test/ipa/meson.build b/test/ipa/meson.build
> index ceed15b..f249091 100644
> --- a/test/ipa/meson.build
> +++ b/test/ipa/meson.build
> @@ -1,6 +1,7 @@
>   # SPDX-License-Identifier: CC0-1.0
>   
>   subdir('libipa')
> +subdir('softisp')
>   
>   ipa_test = [
>       {'name': 'ipa_module_test', 'sources': ['ipa_module_test.cpp']},
> diff --git a/test/ipa/softisp/adjust.cpp b/test/ipa/softisp/adjust.cpp
> new file mode 100644
> index 0000000..24e862b
> --- /dev/null
> +++ b/test/ipa/softisp/adjust.cpp

I'm not sure about the test. It looks a bit ad-hoc.
I think this needs more consideration wrt. if/how ipa
algorithms should be tested.


> @@ -0,0 +1,125 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2026, James Alexander
> + *
> + * Soft ISP image adjustment algorithm tests
> + */
> +
> +#include <cmath>
> +#include <errno.h>
> +#include <iostream>
> +#include <memory>
> +
> +#include <libcamera/control_ids.h>
> +
> +#include "libcamera/internal/value_node.h"
> +
> +#include "algorithms/adjust.h"
> +
> +#include "test.h"
> +
> +using namespace libcamera;
> +using namespace libcamera::ipa::softisp;
> +using namespace libcamera::ipa::softisp::algorithms;
> +
> +namespace {
> +
> +constexpr float kEpsilon = 0.0001f;
> +
> +bool closeEnough(float lhs, float rhs)
> +{
> +	return std::abs(lhs - rhs) < kEpsilon;
> +}
> +
> +} /* namespace */
> +
> +class AdjustTest : public Test
> +{
> +protected:
> +	int testDefaults()
> +	{
> +		IPAContext context(1);
> +		ValueNode tuning;
> +		Adjust adjust;
> +
> +		if (adjust.init(context, tuning))
> +			return TestFail;
> +
> +		IPAConfigInfo configInfo{};
> +		if (adjust.configure(context, configInfo))
> +			return TestFail;
> +
> +		const float contrast = context.ctrlMap.at(&controls::Contrast).def().get<float>();
> +		if (!closeEnough(contrast, 1.0f) ||
> +		    !context.activeState.knobs.contrast ||
> +		    !closeEnough(*context.activeState.knobs.contrast, 1.0f)) {
> +			std::cerr << "Default contrast was not preserved" << std::endl;
> +			return TestFail;
> +		}
> +
> +		return TestPass;
> +	}
> +
> +	int testTunedDefaultAndControl()
> +	{
> +		IPAContext context(1);
> +		ValueNode tuning;
> +		tuning.add("contrast", std::make_unique<ValueNode>(1.15f));
> +		Adjust adjust;
> +
> +		if (adjust.init(context, tuning))
> +			return TestFail;
> +
> +		IPAConfigInfo configInfo{};
> +		if (adjust.configure(context, configInfo))
> +			return TestFail;
> +
> +		const float contrast = context.ctrlMap.at(&controls::Contrast).def().get<float>();
> +		if (!closeEnough(contrast, 1.15f) ||
> +		    !context.activeState.knobs.contrast ||
> +		    !closeEnough(*context.activeState.knobs.contrast, 1.15f)) {
> +			std::cerr << "Tuned contrast default was not applied" << std::endl;
> +			return TestFail;
> +		}
> +
> +		ControlList controlsList(controls::controls);
> +		controlsList.set(controls::Contrast, 0.9f);
> +		IPAFrameContext frameContext{};
> +		adjust.queueRequest(context, 0, frameContext, controlsList);
> +
> +		if (!context.activeState.knobs.contrast ||
> +		    !closeEnough(*context.activeState.knobs.contrast, 0.9f)) {
> +			std::cerr << "Request control did not override tuned contrast" << std::endl;
> +			return TestFail;
> +		}
> +
> +		return TestPass;
> +	}
> +
> +	int testInvalidTuning()
> +	{
> +		IPAContext context(1);
> +		ValueNode tuning;
> +		tuning.add("contrast", std::make_unique<ValueNode>(3.0f));
> +		Adjust adjust;
> +
> +		if (adjust.init(context, tuning) != -EINVAL) {
> +			std::cerr << "Out-of-range contrast tuning was accepted" << std::endl;
> +			return TestFail;
> +		}
> +
> +		return TestPass;
> +	}
> +
> +	int run() override
> +	{
> +		if (testDefaults() != TestPass ||
> +		    testTunedDefaultAndControl() != TestPass ||
> +		    testInvalidTuning() != TestPass)
> +			return TestFail;
> +
> +		return TestPass;
> +	}
> +};
> +
> +TEST_REGISTER(AdjustTest)
> diff --git a/test/ipa/softisp/meson.build b/test/ipa/softisp/meson.build
> new file mode 100644
> index 0000000..78ad612
> --- /dev/null
> +++ b/test/ipa/softisp/meson.build
> @@ -0,0 +1,17 @@
> +# SPDX-License-Identifier: CC0-1.0
> +
> +softisp_adjust_test = executable(
> +    'softisp_adjust',
> +    [
> +        'adjust.cpp',
> +        '../../../src/ipa/softisp/algorithms/adjust.cpp',
> +    ],
> +    dependencies : [libcamera_private, libipa_dep],
> +    link_with : [test_libraries],
> +    include_directories : [
> +        test_includes_internal,
> +        include_directories('../../../src/ipa/softisp'),
> +    ],
> +)
> +
> +test('softisp_adjust', softisp_adjust_test, suite : 'ipa')