[1/3] ipa: simple: adjust: Read defaults from tuning
diff mbox series

Message ID 20260816204259.2845517-2-opensource@inspiredexperts.com
State New
Headers show
Series
  • ipa: simple: Add sensor-specific image and exposure tuning
Related show

Commit Message

James Alexander Aug. 16, 2026, 8:42 p.m. UTC 
Simple IPA tuning files can enable Adjust, but they cannot choose the
starting gamma, contrast or saturation. That leaves sensor-specific tuning
unable to set useful defaults without application controls.

Read optional values from the Adjust section, clamp them to the existing
control ranges and use them as both control defaults and initial state. Existing
tuning files keep the current defaults.

Build-tested against libcamera base b8910c9a4961 and hardware-tested as part of
the complete patch set on the target HP Spectre.

Signed-off-by: James Alexander <opensource@inspiredexperts.com>
---

Comments

Jacopo Mondi Aug. 18, 2026, 10:26 a.m. UTC | #1 
Hi James,

On Sun, Aug 16, 2026 at 02:42:57PM -0600, James Alexander wrote:
> Simple IPA tuning files can enable Adjust, but they cannot choose the
> starting gamma, contrast or saturation. That leaves sensor-specific tuning
> unable to set useful defaults without application controls.
>
> Read optional values from the Adjust section, clamp them to the existing
> control ranges and use them as both control defaults and initial state. Existing
> tuning files keep the current defaults.
>
> Build-tested against libcamera base b8910c9a4961 and hardware-tested as part of
> the complete patch set on the target HP Spectre.
>
> Signed-off-by: James Alexander <opensource@inspiredexperts.com>

I think this is not too different from what an HW ISP does: you
specify a gamma value in tuning file, and eventually override it
with controls at run-time.

This makes sense to me:
Acked-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

But I would like someone that knows about softisp to give a look here.

Thanks
   j

> ---
> diff --git a/src/ipa/simple/algorithms/adjust.cpp b/src/ipa/simple/algorithms/adjust.cpp
> index 8bf39c4..be29127 100644
> --- a/src/ipa/simple/algorithms/adjust.cpp
> +++ b/src/ipa/simple/algorithms/adjust.cpp
> @@ -24,24 +24,38 @@ constexpr float kDefaultSaturation = 1.0f;
>
>  LOG_DEFINE_CATEGORY(IPASoftAdjust)
>
> -int Adjust::init(IPAContext &context, [[maybe_unused]] const ValueNode &tuningData)
> +int Adjust::init(IPAContext &context, const ValueNode &tuningData)
>  {
> +	auto gamma = tuningData["gamma"].get<float>();
> +	if (gamma.has_value())
> +		defaultGamma_ = std::clamp(gamma.value(), 0.1f, 10.0f);
> +
> +	auto contrast = tuningData["contrast"].get<float>();
> +	if (contrast.has_value())
> +		defaultContrast_ = std::clamp(contrast.value(), 0.0f, 2.0f);
> +
> +	auto saturation = tuningData["saturation"].get<float>();
> +	if (saturation.has_value())
> +		defaultSaturation_ = std::clamp(saturation.value(), 0.0f, 2.0f);
> +
>  	context.ctrlMap[&controls::Gamma] =
> -		ControlInfo(0.1f, 10.0f, kDefaultGamma);
> +		ControlInfo(0.1f, 10.0f, defaultGamma_);
>  	context.ctrlMap[&controls::Contrast] =
> -		ControlInfo(0.0f, 2.0f, kDefaultContrast);
> +		ControlInfo(0.0f, 2.0f, defaultContrast_);
>  	if (context.ccmEnabled)
>  		context.ctrlMap[&controls::Saturation] =
> -			ControlInfo(0.0f, 2.0f, kDefaultSaturation);
> +			ControlInfo(0.0f, 2.0f, defaultSaturation_);
>  	return 0;
>  }
>
>  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.gamma = defaultGamma_;
> +	context.activeState.knobs.contrast = defaultContrast_;
>  	context.activeState.knobs.saturation = std::optional<float>();
> +	if (context.ccmEnabled)
> +		context.activeState.knobs.saturation = defaultSaturation_;
>
>  	return 0;
>  }
> diff --git a/src/ipa/simple/algorithms/adjust.h b/src/ipa/simple/algorithms/adjust.h
> index 49c1f26..1968b7c 100644
> --- a/src/ipa/simple/algorithms/adjust.h
> +++ b/src/ipa/simple/algorithms/adjust.h
> @@ -7,6 +7,8 @@
>
>  #pragma once
>
> +#include <optional>
> +
>  #include "libcamera/internal/matrix.h"
>
>  #include <libipa/interpolator.h>
> @@ -43,6 +45,10 @@ public:
>
>  private:
>  	void applySaturation(Matrix<float, 3, 3> &ccm, float saturation);
> +
> +	float defaultGamma_ = kDefaultGamma;
> +	float defaultContrast_ = 1.0f;
> +	float defaultSaturation_ = 1.0f;
>  };
>
>  } /* namespace ipa::soft::algorithms */
> --
> 2.46.0
Kieran Bingham Aug. 20, 2026, 9:39 a.m. UTC | #2 
Quoting Jacopo Mondi (2026-08-18 11:26:42)
> Hi James,
> 
> On Sun, Aug 16, 2026 at 02:42:57PM -0600, James Alexander wrote:
> > Simple IPA tuning files can enable Adjust, but they cannot choose the
> > starting gamma, contrast or saturation. That leaves sensor-specific tuning
> > unable to set useful defaults without application controls.
> >
> > Read optional values from the Adjust section, clamp them to the existing
> > control ranges and use them as both control defaults and initial state. Existing
> > tuning files keep the current defaults.
> >
> > Build-tested against libcamera base b8910c9a4961 and hardware-tested as part of
> > the complete patch set on the target HP Spectre.
> >
> > Signed-off-by: James Alexander <opensource@inspiredexperts.com>
> 
> I think this is not too different from what an HW ISP does: you
> specify a gamma value in tuning file, and eventually override it
> with controls at run-time.
> 
> This makes sense to me:
> Acked-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
> But I would like someone that knows about softisp to give a look here.

Should this also convert to the common implementation thought to ensure
consistent behaviour?

Dan has done the IPU3:
 - https://patchwork.libcamera.org/patch/27069/


> 
> Thanks
>    j
> 
> > ---
> > diff --git a/src/ipa/simple/algorithms/adjust.cpp b/src/ipa/simple/algorithms/adjust.cpp
> > index 8bf39c4..be29127 100644
> > --- a/src/ipa/simple/algorithms/adjust.cpp
> > +++ b/src/ipa/simple/algorithms/adjust.cpp
> > @@ -24,24 +24,38 @@ constexpr float kDefaultSaturation = 1.0f;
> >
> >  LOG_DEFINE_CATEGORY(IPASoftAdjust)
> >
> > -int Adjust::init(IPAContext &context, [[maybe_unused]] const ValueNode &tuningData)
> > +int Adjust::init(IPAContext &context, const ValueNode &tuningData)
> >  {
> > +     auto gamma = tuningData["gamma"].get<float>();
> > +     if (gamma.has_value())
> > +             defaultGamma_ = std::clamp(gamma.value(), 0.1f, 10.0f);
> > +
> > +     auto contrast = tuningData["contrast"].get<float>();
> > +     if (contrast.has_value())
> > +             defaultContrast_ = std::clamp(contrast.value(), 0.0f, 2.0f);
> > +
> > +     auto saturation = tuningData["saturation"].get<float>();
> > +     if (saturation.has_value())
> > +             defaultSaturation_ = std::clamp(saturation.value(), 0.0f, 2.0f);
> > +
> >       context.ctrlMap[&controls::Gamma] =
> > -             ControlInfo(0.1f, 10.0f, kDefaultGamma);
> > +             ControlInfo(0.1f, 10.0f, defaultGamma_);
> >       context.ctrlMap[&controls::Contrast] =
> > -             ControlInfo(0.0f, 2.0f, kDefaultContrast);
> > +             ControlInfo(0.0f, 2.0f, defaultContrast_);
> >       if (context.ccmEnabled)
> >               context.ctrlMap[&controls::Saturation] =
> > -                     ControlInfo(0.0f, 2.0f, kDefaultSaturation);
> > +                     ControlInfo(0.0f, 2.0f, defaultSaturation_);
> >       return 0;
> >  }
> >
> >  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.gamma = defaultGamma_;
> > +     context.activeState.knobs.contrast = defaultContrast_;
> >       context.activeState.knobs.saturation = std::optional<float>();
> > +     if (context.ccmEnabled)
> > +             context.activeState.knobs.saturation = defaultSaturation_;
> >
> >       return 0;
> >  }
> > diff --git a/src/ipa/simple/algorithms/adjust.h b/src/ipa/simple/algorithms/adjust.h
> > index 49c1f26..1968b7c 100644
> > --- a/src/ipa/simple/algorithms/adjust.h
> > +++ b/src/ipa/simple/algorithms/adjust.h
> > @@ -7,6 +7,8 @@
> >
> >  #pragma once
> >
> > +#include <optional>
> > +
> >  #include "libcamera/internal/matrix.h"
> >
> >  #include <libipa/interpolator.h>
> > @@ -43,6 +45,10 @@ public:
> >
> >  private:
> >       void applySaturation(Matrix<float, 3, 3> &ccm, float saturation);
> > +
> > +     float defaultGamma_ = kDefaultGamma;
> > +     float defaultContrast_ = 1.0f;
> > +     float defaultSaturation_ = 1.0f;
> >  };
> >
> >  } /* namespace ipa::soft::algorithms */
> > --
> > 2.46.0

Patch
diff mbox series 

diff --git a/src/ipa/simple/algorithms/adjust.cpp b/src/ipa/simple/algorithms/adjust.cpp
index 8bf39c4..be29127 100644
--- a/src/ipa/simple/algorithms/adjust.cpp
+++ b/src/ipa/simple/algorithms/adjust.cpp
@@ -24,24 +24,38 @@  constexpr float kDefaultSaturation = 1.0f;
 
 LOG_DEFINE_CATEGORY(IPASoftAdjust)
 
-int Adjust::init(IPAContext &context, [[maybe_unused]] const ValueNode &tuningData)
+int Adjust::init(IPAContext &context, const ValueNode &tuningData)
 {
+	auto gamma = tuningData["gamma"].get<float>();
+	if (gamma.has_value())
+		defaultGamma_ = std::clamp(gamma.value(), 0.1f, 10.0f);
+
+	auto contrast = tuningData["contrast"].get<float>();
+	if (contrast.has_value())
+		defaultContrast_ = std::clamp(contrast.value(), 0.0f, 2.0f);
+
+	auto saturation = tuningData["saturation"].get<float>();
+	if (saturation.has_value())
+		defaultSaturation_ = std::clamp(saturation.value(), 0.0f, 2.0f);
+
 	context.ctrlMap[&controls::Gamma] =
-		ControlInfo(0.1f, 10.0f, kDefaultGamma);
+		ControlInfo(0.1f, 10.0f, defaultGamma_);
 	context.ctrlMap[&controls::Contrast] =
-		ControlInfo(0.0f, 2.0f, kDefaultContrast);
+		ControlInfo(0.0f, 2.0f, defaultContrast_);
 	if (context.ccmEnabled)
 		context.ctrlMap[&controls::Saturation] =
-			ControlInfo(0.0f, 2.0f, kDefaultSaturation);
+			ControlInfo(0.0f, 2.0f, defaultSaturation_);
 	return 0;
 }
 
 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.gamma = defaultGamma_;
+	context.activeState.knobs.contrast = defaultContrast_;
 	context.activeState.knobs.saturation = std::optional<float>();
+	if (context.ccmEnabled)
+		context.activeState.knobs.saturation = defaultSaturation_;
 
 	return 0;
 }
diff --git a/src/ipa/simple/algorithms/adjust.h b/src/ipa/simple/algorithms/adjust.h
index 49c1f26..1968b7c 100644
--- a/src/ipa/simple/algorithms/adjust.h
+++ b/src/ipa/simple/algorithms/adjust.h
@@ -7,6 +7,8 @@ 
 
 #pragma once
 
+#include <optional>
+
 #include "libcamera/internal/matrix.h"
 
 #include <libipa/interpolator.h>
@@ -43,6 +45,10 @@  public:
 
 private:
 	void applySaturation(Matrix<float, 3, 3> &ccm, float saturation);
+
+	float defaultGamma_ = kDefaultGamma;
+	float defaultContrast_ = 1.0f;
+	float defaultSaturation_ = 1.0f;
 };
 
 } /* namespace ipa::soft::algorithms */