[RFC,7/7] libcamera: ipa: simple: Move contrast settings to adjust.cpp
diff mbox series

Message ID 20251112082715.17823-8-mzamazal@redhat.com
State New
Headers show
Series
  • Simple pipeline IPA cleanup
Related show

Commit Message

Milan Zamazal Nov. 12, 2025, 8:27 a.m. UTC 
Let's move the contrast settings from lut.cpp to adjust.cpp, where they
belong, similarly to saturation.

Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 src/ipa/simple/algorithms/adjust.cpp | 14 +++++++++++
 src/ipa/simple/algorithms/lut.cpp    | 35 +---------------------------
 src/ipa/simple/algorithms/lut.h      | 11 ---------
 3 files changed, 15 insertions(+), 45 deletions(-)

Comments

Kieran Bingham Nov. 12, 2025, 10:21 a.m. UTC | #1 
Quoting Milan Zamazal (2025-11-12 08:27:15)
> Let's move the contrast settings from lut.cpp to adjust.cpp, where they
> belong, similarly to saturation.
> 
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>  src/ipa/simple/algorithms/adjust.cpp | 14 +++++++++++
>  src/ipa/simple/algorithms/lut.cpp    | 35 +---------------------------
>  src/ipa/simple/algorithms/lut.h      | 11 ---------
>  3 files changed, 15 insertions(+), 45 deletions(-)
> 
> diff --git a/src/ipa/simple/algorithms/adjust.cpp b/src/ipa/simple/algorithms/adjust.cpp
> index 282b3ccb0..b9a9fc710 100644
> --- a/src/ipa/simple/algorithms/adjust.cpp
> +++ b/src/ipa/simple/algorithms/adjust.cpp
> @@ -23,6 +23,7 @@ LOG_DEFINE_CATEGORY(IPASoftAdjust)
>  
>  int Adjust::init(IPAContext &context, [[maybe_unused]] const YamlObject &tuningData)
>  {


> +       context.ctrlMap[&controls::Contrast] = ControlInfo(0.0f, 2.0f, 1.0f);

Shouldn't this be dependent upon LUT/Gamma being enabled?

>         if (context.ccmEnabled)

For both of these I think we have a ciruclar dependency though :-(

To be able to know if they are enabled/available ccm/lut(gamma) would
have to be before adjust to set it before Adjust::init().

But then if ccm/lut is in front of the chain, then the processing in
adjust won't have happened in time for the values to be applied?



>                 context.ctrlMap[&controls::Saturation] = ControlInfo(0.0f, 2.0f, 1.0f);
>         return 0;
> @@ -31,6 +32,7 @@ int Adjust::init(IPAContext &context, [[maybe_unused]] const YamlObject &tuningD
>  int Adjust::configure(IPAContext &context,
>                       [[maybe_unused]] const IPAConfigInfo &configInfo)
>  {
> +       context.activeState.knobs.contrast = std::optional<double>();
>         context.activeState.knobs.saturation = std::optional<double>();
>         context.activeState.correctionMatrix =
>                 Matrix<float, 3, 3>{ { 1, 0, 0, 0, 1, 0, 0, 0, 1 } };
> @@ -43,6 +45,12 @@ void Adjust::queueRequest(typename Module::Context &context,
>                           [[maybe_unused]] typename Module::FrameContext &frameContext,
>                           const ControlList &controls)
>  {
> +       const auto &contrast = controls.get(controls::Contrast);
> +       if (contrast.has_value()) {
> +               context.activeState.knobs.contrast = contrast;
> +               LOG(IPASoftAdjust, Debug) << "Setting contrast to " << contrast.value();
> +       }
> +
>         const auto &saturation = controls.get(controls::Saturation);
>         if (saturation.has_value()) {
>                 context.activeState.knobs.saturation = saturation;
> @@ -77,6 +85,8 @@ void Adjust::prepare(IPAContext &context,
>                      IPAFrameContext &frameContext,
>                      [[maybe_unused]] DebayerParams *params)
>  {
> +       frameContext.contrast = context.activeState.knobs.contrast;
> +
>         if (!context.ccmEnabled)
>                 return;
>  
> @@ -100,6 +110,10 @@ void Adjust::process([[maybe_unused]] IPAContext &context,
>                      [[maybe_unused]] const SwIspStats *stats,
>                      ControlList &metadata)
>  {
> +       const auto &contrast = frameContext.contrast;
> +       if (contrast)
> +               metadata.set(controls::Contrast, contrast.value());
> +
>         const auto &saturation = frameContext.saturation;
>         metadata.set(controls::Saturation, saturation.value_or(1.0));
>  }
> diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp
> index 8751bb31c..876cd5cb3 100644
> --- a/src/ipa/simple/algorithms/lut.cpp
> +++ b/src/ipa/simple/algorithms/lut.cpp
> @@ -24,36 +24,16 @@ LOG_DEFINE_CATEGORY(IPASoftLut)
>  
>  namespace ipa::soft::algorithms {
>  
> -int Lut::init(IPAContext &context,
> -             [[maybe_unused]] const YamlObject &tuningData)
> -{
> -       context.ctrlMap[&controls::Contrast] = ControlInfo(0.0f, 2.0f, 1.0f);
> -       return 0;
> -}
> -
>  int Lut::configure(IPAContext &context,
>                    [[maybe_unused]] const IPAConfigInfo &configInfo)
>  {
>         /* Gamma value is fixed */

Seems easy to give this a control now ?

>         context.configuration.gamma = 0.5;
> -       context.activeState.knobs.contrast = std::optional<double>();
>         updateGammaTable(context);

I guess LUT could now be called 'gamma.cpp' ?

>  
>         return 0;
>  }
>  
> -void Lut::queueRequest(typename Module::Context &context,
> -                      [[maybe_unused]] const uint32_t frame,
> -                      [[maybe_unused]] typename Module::FrameContext &frameContext,
> -                      const ControlList &controls)
> -{
> -       const auto &contrast = controls.get(controls::Contrast);
> -       if (contrast.has_value()) {
> -               context.activeState.knobs.contrast = contrast;
> -               LOG(IPASoftLut, Debug) << "Setting contrast to " << contrast.value();
> -       }
> -}
> -
>  void Lut::updateGammaTable(IPAContext &context)
>  {
>         auto &gammaTable = context.activeState.gamma.gammaTable;
> @@ -87,11 +67,9 @@ int16_t Lut::matrixValue(unsigned int i, float ccm) const
>  
>  void Lut::prepare(IPAContext &context,
>                   [[maybe_unused]] const uint32_t frame,
> -                 IPAFrameContext &frameContext,
> +                 [[maybe_unused]] IPAFrameContext &frameContext,
>                   DebayerParams *params)
>  {
> -       frameContext.contrast = context.activeState.knobs.contrast;
> -
>         /*
>          * Update the gamma table if needed. This means if black level changes
>          * and since the black level gets updated only if a lower value is
> @@ -143,17 +121,6 @@ void Lut::prepare(IPAContext &context,
>         }
>  }
>  
> -void Lut::process([[maybe_unused]] IPAContext &context,
> -                 [[maybe_unused]] const uint32_t frame,
> -                 [[maybe_unused]] IPAFrameContext &frameContext,
> -                 [[maybe_unused]] const SwIspStats *stats,
> -                 ControlList &metadata)
> -{
> -       const auto &contrast = frameContext.contrast;
> -       if (contrast)
> -               metadata.set(controls::Contrast, contrast.value());
> -}
> -
>  REGISTER_IPA_ALGORITHM(Lut, "Lut")
>  
>  } /* namespace ipa::soft::algorithms */
> diff --git a/src/ipa/simple/algorithms/lut.h b/src/ipa/simple/algorithms/lut.h
> index 0eafd0695..ad16d1e8e 100644
> --- a/src/ipa/simple/algorithms/lut.h
> +++ b/src/ipa/simple/algorithms/lut.h
> @@ -19,22 +19,11 @@ public:
>         Lut() = default;
>         ~Lut() = 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,
>                      DebayerParams *params) override;
> -       void process(IPAContext &context,
> -                    const uint32_t frame,
> -                    IPAFrameContext &frameContext,
> -                    const SwIspStats *stats,
> -                    ControlList &metadata) override;
>  
>  private:
>         void updateGammaTable(IPAContext &context);
> -- 
> 2.51.1
>

Patch
diff mbox series 

diff --git a/src/ipa/simple/algorithms/adjust.cpp b/src/ipa/simple/algorithms/adjust.cpp
index 282b3ccb0..b9a9fc710 100644
--- a/src/ipa/simple/algorithms/adjust.cpp
+++ b/src/ipa/simple/algorithms/adjust.cpp
@@ -23,6 +23,7 @@  LOG_DEFINE_CATEGORY(IPASoftAdjust)
 
 int Adjust::init(IPAContext &context, [[maybe_unused]] const YamlObject &tuningData)
 {
+	context.ctrlMap[&controls::Contrast] = ControlInfo(0.0f, 2.0f, 1.0f);
 	if (context.ccmEnabled)
 		context.ctrlMap[&controls::Saturation] = ControlInfo(0.0f, 2.0f, 1.0f);
 	return 0;
@@ -31,6 +32,7 @@  int Adjust::init(IPAContext &context, [[maybe_unused]] const YamlObject &tuningD
 int Adjust::configure(IPAContext &context,
 		      [[maybe_unused]] const IPAConfigInfo &configInfo)
 {
+	context.activeState.knobs.contrast = std::optional<double>();
 	context.activeState.knobs.saturation = std::optional<double>();
 	context.activeState.correctionMatrix =
 		Matrix<float, 3, 3>{ { 1, 0, 0, 0, 1, 0, 0, 0, 1 } };
@@ -43,6 +45,12 @@  void Adjust::queueRequest(typename Module::Context &context,
 			  [[maybe_unused]] typename Module::FrameContext &frameContext,
 			  const ControlList &controls)
 {
+	const auto &contrast = controls.get(controls::Contrast);
+	if (contrast.has_value()) {
+		context.activeState.knobs.contrast = contrast;
+		LOG(IPASoftAdjust, Debug) << "Setting contrast to " << contrast.value();
+	}
+
 	const auto &saturation = controls.get(controls::Saturation);
 	if (saturation.has_value()) {
 		context.activeState.knobs.saturation = saturation;
@@ -77,6 +85,8 @@  void Adjust::prepare(IPAContext &context,
 		     IPAFrameContext &frameContext,
 		     [[maybe_unused]] DebayerParams *params)
 {
+	frameContext.contrast = context.activeState.knobs.contrast;
+
 	if (!context.ccmEnabled)
 		return;
 
@@ -100,6 +110,10 @@  void Adjust::process([[maybe_unused]] IPAContext &context,
 		     [[maybe_unused]] const SwIspStats *stats,
 		     ControlList &metadata)
 {
+	const auto &contrast = frameContext.contrast;
+	if (contrast)
+		metadata.set(controls::Contrast, contrast.value());
+
 	const auto &saturation = frameContext.saturation;
 	metadata.set(controls::Saturation, saturation.value_or(1.0));
 }
diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp
index 8751bb31c..876cd5cb3 100644
--- a/src/ipa/simple/algorithms/lut.cpp
+++ b/src/ipa/simple/algorithms/lut.cpp
@@ -24,36 +24,16 @@  LOG_DEFINE_CATEGORY(IPASoftLut)
 
 namespace ipa::soft::algorithms {
 
-int Lut::init(IPAContext &context,
-	      [[maybe_unused]] const YamlObject &tuningData)
-{
-	context.ctrlMap[&controls::Contrast] = ControlInfo(0.0f, 2.0f, 1.0f);
-	return 0;
-}
-
 int Lut::configure(IPAContext &context,
 		   [[maybe_unused]] const IPAConfigInfo &configInfo)
 {
 	/* Gamma value is fixed */
 	context.configuration.gamma = 0.5;
-	context.activeState.knobs.contrast = std::optional<double>();
 	updateGammaTable(context);
 
 	return 0;
 }
 
-void Lut::queueRequest(typename Module::Context &context,
-		       [[maybe_unused]] const uint32_t frame,
-		       [[maybe_unused]] typename Module::FrameContext &frameContext,
-		       const ControlList &controls)
-{
-	const auto &contrast = controls.get(controls::Contrast);
-	if (contrast.has_value()) {
-		context.activeState.knobs.contrast = contrast;
-		LOG(IPASoftLut, Debug) << "Setting contrast to " << contrast.value();
-	}
-}
-
 void Lut::updateGammaTable(IPAContext &context)
 {
 	auto &gammaTable = context.activeState.gamma.gammaTable;
@@ -87,11 +67,9 @@  int16_t Lut::matrixValue(unsigned int i, float ccm) const
 
 void Lut::prepare(IPAContext &context,
 		  [[maybe_unused]] const uint32_t frame,
-		  IPAFrameContext &frameContext,
+		  [[maybe_unused]] IPAFrameContext &frameContext,
 		  DebayerParams *params)
 {
-	frameContext.contrast = context.activeState.knobs.contrast;
-
 	/*
 	 * Update the gamma table if needed. This means if black level changes
 	 * and since the black level gets updated only if a lower value is
@@ -143,17 +121,6 @@  void Lut::prepare(IPAContext &context,
 	}
 }
 
-void Lut::process([[maybe_unused]] IPAContext &context,
-		  [[maybe_unused]] const uint32_t frame,
-		  [[maybe_unused]] IPAFrameContext &frameContext,
-		  [[maybe_unused]] const SwIspStats *stats,
-		  ControlList &metadata)
-{
-	const auto &contrast = frameContext.contrast;
-	if (contrast)
-		metadata.set(controls::Contrast, contrast.value());
-}
-
 REGISTER_IPA_ALGORITHM(Lut, "Lut")
 
 } /* namespace ipa::soft::algorithms */
diff --git a/src/ipa/simple/algorithms/lut.h b/src/ipa/simple/algorithms/lut.h
index 0eafd0695..ad16d1e8e 100644
--- a/src/ipa/simple/algorithms/lut.h
+++ b/src/ipa/simple/algorithms/lut.h
@@ -19,22 +19,11 @@  public:
 	Lut() = default;
 	~Lut() = 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,
 		     DebayerParams *params) override;
-	void process(IPAContext &context,
-		     const uint32_t frame,
-		     IPAFrameContext &frameContext,
-		     const SwIspStats *stats,
-		     ControlList &metadata) override;
 
 private:
 	void updateGammaTable(IPAContext &context);