[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
>
Milan Zamazal Nov. 13, 2025, 2:44 p.m. UTC | #2 
Hi Kieran,

Kieran Bingham <kieran.bingham@ideasonboard.com> writes:

> 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?

Well, LUT is really not optional, without it, at least CPU ISP cannot
operate at all.  Maybe the table constructions should be moved out of
the algorithms to debayering.

>>         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 ?

Yes, it would be useful, will try.

>>         context.configuration.gamma = 0.5;
>> -       context.activeState.knobs.contrast = std::optional<double>();
>>         updateGammaTable(context);
>
> I guess LUT could now be called 'gamma.cpp' ?

I think it would be better to set up the gamma value in adjust.cpp.
Then perhaps the rest of lut.cpp could be moved out of ipa.  Let's see
in v2.

>>  
>>         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);