[v4,12/21] ipa: rkisp1: cproc: Convert to use Quantized types
diff mbox series

Message ID 20251114005428.90024-13-kieran.bingham@ideasonboard.com
State New
Headers show
Series
  • libipa: Introduce a Quantized type
Related show

Commit Message

Kieran Bingham Nov. 14, 2025, 12:54 a.m. UTC 
Convert the Contrast and Saturuation helper functions to a Quantizer
type to support maintaining the data.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

---
v3:
- Don't add <algorithm> into ipa_context.h anymore

 src/ipa/rkisp1/algorithms/cproc.cpp | 36 +++++++++++------------------
 src/ipa/rkisp1/ipa_context.h        | 19 ++++++++++-----
 2 files changed, 27 insertions(+), 28 deletions(-)

Comments

Barnabás Pőcze Nov. 14, 2025, 6:08 p.m. UTC | #1 
2025. 11. 14. 1:54 keltezéssel, Kieran Bingham írta:
> Convert the Contrast and Saturuation helper functions to a Quantizer
> type to support maintaining the data.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> ---
> v3:
> - Don't add <algorithm> into ipa_context.h anymore
> 
>   src/ipa/rkisp1/algorithms/cproc.cpp | 36 +++++++++++------------------
>   src/ipa/rkisp1/ipa_context.h        | 19 ++++++++++-----
>   2 files changed, 27 insertions(+), 28 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp
> index d1fff6990d37..4637a824af03 100644
> --- a/src/ipa/rkisp1/algorithms/cproc.cpp
> +++ b/src/ipa/rkisp1/algorithms/cproc.cpp
> @@ -14,6 +14,8 @@
>   
>   #include <libcamera/control_ids.h>
>   
> +#include "libipa/quantized.h"
> +
>   /**
>    * \file cproc.h
>    */
> @@ -39,16 +41,6 @@ constexpr float kDefaultBrightness = 0.0f;
>   constexpr float kDefaultContrast = 1.0f;
>   constexpr float kDefaultSaturation = 1.0f;
>   
> -int convertBrightness(const float v)
> -{
> -	return std::clamp<int>(std::lround(v * 128), -128, 127);
> -}
> -
> -int convertContrastOrSaturation(const float v)
> -{
> -	return std::clamp<int>(std::lround(v * 128), 0, 255);
> -}
> -
>   } /* namespace */
>   
>   /**
> @@ -74,9 +66,9 @@ int ColorProcessing::configure(IPAContext &context,
>   {
>   	auto &cproc = context.activeState.cproc;
>   
> -	cproc.brightness = convertBrightness(kDefaultBrightness);
> -	cproc.contrast = convertContrastOrSaturation(kDefaultContrast);
> -	cproc.saturation = convertContrastOrSaturation(kDefaultSaturation);
> +	cproc.brightness = BrightnessQ(kDefaultBrightness);
> +	cproc.contrast = ContrastQ(kDefaultContrast);
> +	cproc.saturation = SaturationQ(kDefaultSaturation);
>   
>   	return 0;
>   }
> @@ -97,35 +89,35 @@ void ColorProcessing::queueRequest(IPAContext &context,
>   
>   	const auto &brightness = controls.get(controls::Brightness);
>   	if (brightness) {
> -		int value = convertBrightness(*brightness);
> +		BrightnessQ value = *brightness;
>   		if (cproc.brightness != value) {
>   			cproc.brightness = value;
>   			update = true;
>   		}
>   
> -		LOG(RkISP1CProc, Debug) << "Set brightness to " << value;
> +		LOG(RkISP1CProc, Debug) << "Set brightness to " << value.value();

If you add an `operator<<` to `Quantized<>`, then I think it makes sense to do
just `<< value` to see the underlying integer value as well in these debug messages.


>   	}
>   
>   	const auto &contrast = controls.get(controls::Contrast);
>   	if (contrast) {
> -		int value = convertContrastOrSaturation(*contrast);
> +		ContrastQ value = *contrast;
>   		if (cproc.contrast != value) {
>   			cproc.contrast = value;
>   			update = true;
>   		}
>   
> -		LOG(RkISP1CProc, Debug) << "Set contrast to " << value;
> +		LOG(RkISP1CProc, Debug) << "Set contrast to " << value.value();
>   	}
>   
>   	const auto saturation = controls.get(controls::Saturation);
>   	if (saturation) {
> -		int value = convertContrastOrSaturation(*saturation);
> +		SaturationQ value = *saturation;
>   		if (cproc.saturation != value) {
>   			cproc.saturation = value;
>   			update = true;
>   		}
>   
> -		LOG(RkISP1CProc, Debug) << "Set saturation to " << value;
> +		LOG(RkISP1CProc, Debug) << "Set saturation to " << value.value();
>   	}
>   
>   	frameContext.cproc.brightness = cproc.brightness;
> @@ -148,9 +140,9 @@ void ColorProcessing::prepare([[maybe_unused]] IPAContext &context,
>   
>   	auto config = params->block<BlockType::Cproc>();
>   	config.setEnabled(true);
> -	config->brightness = frameContext.cproc.brightness;
> -	config->contrast = frameContext.cproc.contrast;
> -	config->sat = frameContext.cproc.saturation;
> +	config->brightness = frameContext.cproc.brightness.quantized();
> +	config->contrast = frameContext.cproc.contrast.quantized();
> +	config->sat = frameContext.cproc.saturation.quantized();
>   }
>   
>   REGISTER_IPA_ALGORITHM(ColorProcessing, "ColorProcessing")
> [...]
Isaac Scott Nov. 18, 2025, 12:40 p.m. UTC | #2 
Hi Kieran,

Thank you for the patch!

Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com>

Quoting Kieran Bingham (2025-11-14 00:54:16)
> Convert the Contrast and Saturuation helper functions to a Quantizer
> type to support maintaining the data.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> ---
> v3:
> - Don't add <algorithm> into ipa_context.h anymore
> 
>  src/ipa/rkisp1/algorithms/cproc.cpp | 36 +++++++++++------------------
>  src/ipa/rkisp1/ipa_context.h        | 19 ++++++++++-----
>  2 files changed, 27 insertions(+), 28 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp
> index d1fff6990d37..4637a824af03 100644
> --- a/src/ipa/rkisp1/algorithms/cproc.cpp
> +++ b/src/ipa/rkisp1/algorithms/cproc.cpp
> @@ -14,6 +14,8 @@
>  
>  #include <libcamera/control_ids.h>
>  
> +#include "libipa/quantized.h"
> +
>  /**
>   * \file cproc.h
>   */
> @@ -39,16 +41,6 @@ constexpr float kDefaultBrightness = 0.0f;
>  constexpr float kDefaultContrast = 1.0f;
>  constexpr float kDefaultSaturation = 1.0f;
>  
> -int convertBrightness(const float v)
> -{
> -       return std::clamp<int>(std::lround(v * 128), -128, 127);
> -}
> -
> -int convertContrastOrSaturation(const float v)
> -{
> -       return std::clamp<int>(std::lround(v * 128), 0, 255);
> -}
> -
>  } /* namespace */
>  
>  /**
> @@ -74,9 +66,9 @@ int ColorProcessing::configure(IPAContext &context,
>  {
>         auto &cproc = context.activeState.cproc;
>  
> -       cproc.brightness = convertBrightness(kDefaultBrightness);
> -       cproc.contrast = convertContrastOrSaturation(kDefaultContrast);
> -       cproc.saturation = convertContrastOrSaturation(kDefaultSaturation);
> +       cproc.brightness = BrightnessQ(kDefaultBrightness);
> +       cproc.contrast = ContrastQ(kDefaultContrast);
> +       cproc.saturation = SaturationQ(kDefaultSaturation);
>  
>         return 0;
>  }
> @@ -97,35 +89,35 @@ void ColorProcessing::queueRequest(IPAContext &context,
>  
>         const auto &brightness = controls.get(controls::Brightness);
>         if (brightness) {
> -               int value = convertBrightness(*brightness);
> +               BrightnessQ value = *brightness;
>                 if (cproc.brightness != value) {
>                         cproc.brightness = value;
>                         update = true;
>                 }
>  
> -               LOG(RkISP1CProc, Debug) << "Set brightness to " << value;
> +               LOG(RkISP1CProc, Debug) << "Set brightness to " << value.value();
>         }
>  
>         const auto &contrast = controls.get(controls::Contrast);
>         if (contrast) {
> -               int value = convertContrastOrSaturation(*contrast);
> +               ContrastQ value = *contrast;
>                 if (cproc.contrast != value) {
>                         cproc.contrast = value;
>                         update = true;
>                 }
>  
> -               LOG(RkISP1CProc, Debug) << "Set contrast to " << value;
> +               LOG(RkISP1CProc, Debug) << "Set contrast to " << value.value();
>         }
>  
>         const auto saturation = controls.get(controls::Saturation);
>         if (saturation) {
> -               int value = convertContrastOrSaturation(*saturation);
> +               SaturationQ value = *saturation;
>                 if (cproc.saturation != value) {
>                         cproc.saturation = value;
>                         update = true;
>                 }
>  
> -               LOG(RkISP1CProc, Debug) << "Set saturation to " << value;
> +               LOG(RkISP1CProc, Debug) << "Set saturation to " << value.value();
>         }
>  
>         frameContext.cproc.brightness = cproc.brightness;
> @@ -148,9 +140,9 @@ void ColorProcessing::prepare([[maybe_unused]] IPAContext &context,
>  
>         auto config = params->block<BlockType::Cproc>();
>         config.setEnabled(true);
> -       config->brightness = frameContext.cproc.brightness;
> -       config->contrast = frameContext.cproc.contrast;
> -       config->sat = frameContext.cproc.saturation;
> +       config->brightness = frameContext.cproc.brightness.quantized();
> +       config->contrast = frameContext.cproc.contrast.quantized();
> +       config->sat = frameContext.cproc.saturation.quantized();
>  }
>  
>  REGISTER_IPA_ALGORITHM(ColorProcessing, "ColorProcessing")
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index f85a130d9c23..651558bdf42d 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -27,11 +27,17 @@
>  #include <libipa/camera_sensor_helper.h>
>  #include <libipa/fc_queue.h>
>  #include "libipa/agc_mean_luminance.h"
> +#include "libipa/fixedpoint.h"
>  
>  namespace libcamera {
>  
>  namespace ipa::rkisp1 {
>  
> +/* Fixed point types used by CPROC */
> +using BrightnessQ = Q1_7;
> +using ContrastQ = UQ1_7;
> +using SaturationQ = UQ1_7;
> +
>  struct IPAHwSettings {
>         unsigned int numAeCells;
>         unsigned int numHistogramBins;
> @@ -115,9 +121,9 @@ struct IPAActiveState {
>         } ccm;
>  
>         struct {
> -               int8_t brightness;
> -               uint8_t contrast;
> -               uint8_t saturation;
> +               BrightnessQ brightness;
> +               ContrastQ contrast;
> +               SaturationQ saturation;
>         } cproc;
>  
>         struct {
> @@ -169,9 +175,10 @@ struct IPAFrameContext : public FrameContext {
>         } awb;
>  
>         struct {
> -               int8_t brightness;
> -               uint8_t contrast;
> -               uint8_t saturation;
> +               BrightnessQ brightness;
> +               ContrastQ contrast;
> +               SaturationQ saturation;
> +
>                 bool update;
>         } cproc;
>  
> -- 
> 2.51.1
>
Laurent Pinchart Nov. 19, 2025, 4:30 a.m. UTC | #3 
On Fri, Nov 14, 2025 at 12:54:16AM +0000, Kieran Bingham wrote:
> Convert the Contrast and Saturuation helper functions to a Quantizer

s/Saturuation/Saturation/

You forgot brighness.

> type to support maintaining the data.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> ---
> v3:
> - Don't add <algorithm> into ipa_context.h anymore
> 
>  src/ipa/rkisp1/algorithms/cproc.cpp | 36 +++++++++++------------------
>  src/ipa/rkisp1/ipa_context.h        | 19 ++++++++++-----
>  2 files changed, 27 insertions(+), 28 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp
> index d1fff6990d37..4637a824af03 100644
> --- a/src/ipa/rkisp1/algorithms/cproc.cpp
> +++ b/src/ipa/rkisp1/algorithms/cproc.cpp
> @@ -14,6 +14,8 @@
>  
>  #include <libcamera/control_ids.h>
>  
> +#include "libipa/quantized.h"
> +
>  /**
>   * \file cproc.h
>   */
> @@ -39,16 +41,6 @@ constexpr float kDefaultBrightness = 0.0f;
>  constexpr float kDefaultContrast = 1.0f;
>  constexpr float kDefaultSaturation = 1.0f;
>  
> -int convertBrightness(const float v)
> -{
> -	return std::clamp<int>(std::lround(v * 128), -128, 127);
> -}
> -
> -int convertContrastOrSaturation(const float v)
> -{
> -	return std::clamp<int>(std::lround(v * 128), 0, 255);
> -}
> -
>  } /* namespace */
>  
>  /**
> @@ -74,9 +66,9 @@ int ColorProcessing::configure(IPAContext &context,
>  {
>  	auto &cproc = context.activeState.cproc;
>  
> -	cproc.brightness = convertBrightness(kDefaultBrightness);
> -	cproc.contrast = convertContrastOrSaturation(kDefaultContrast);
> -	cproc.saturation = convertContrastOrSaturation(kDefaultSaturation);
> +	cproc.brightness = BrightnessQ(kDefaultBrightness);
> +	cproc.contrast = ContrastQ(kDefaultContrast);
> +	cproc.saturation = SaturationQ(kDefaultSaturation);
>  
>  	return 0;
>  }
> @@ -97,35 +89,35 @@ void ColorProcessing::queueRequest(IPAContext &context,
>  
>  	const auto &brightness = controls.get(controls::Brightness);
>  	if (brightness) {
> -		int value = convertBrightness(*brightness);
> +		BrightnessQ value = *brightness;
>  		if (cproc.brightness != value) {
>  			cproc.brightness = value;
>  			update = true;
>  		}
>  
> -		LOG(RkISP1CProc, Debug) << "Set brightness to " << value;
> +		LOG(RkISP1CProc, Debug) << "Set brightness to " << value.value();

An operator<<() would be nice.

>  	}
>  
>  	const auto &contrast = controls.get(controls::Contrast);
>  	if (contrast) {
> -		int value = convertContrastOrSaturation(*contrast);
> +		ContrastQ value = *contrast;
>  		if (cproc.contrast != value) {
>  			cproc.contrast = value;
>  			update = true;
>  		}
>  
> -		LOG(RkISP1CProc, Debug) << "Set contrast to " << value;
> +		LOG(RkISP1CProc, Debug) << "Set contrast to " << value.value();
>  	}
>  
>  	const auto saturation = controls.get(controls::Saturation);
>  	if (saturation) {
> -		int value = convertContrastOrSaturation(*saturation);
> +		SaturationQ value = *saturation;
>  		if (cproc.saturation != value) {
>  			cproc.saturation = value;
>  			update = true;
>  		}
>  
> -		LOG(RkISP1CProc, Debug) << "Set saturation to " << value;
> +		LOG(RkISP1CProc, Debug) << "Set saturation to " << value.value();
>  	}
>  
>  	frameContext.cproc.brightness = cproc.brightness;
> @@ -148,9 +140,9 @@ void ColorProcessing::prepare([[maybe_unused]] IPAContext &context,
>  
>  	auto config = params->block<BlockType::Cproc>();
>  	config.setEnabled(true);
> -	config->brightness = frameContext.cproc.brightness;
> -	config->contrast = frameContext.cproc.contrast;
> -	config->sat = frameContext.cproc.saturation;
> +	config->brightness = frameContext.cproc.brightness.quantized();
> +	config->contrast = frameContext.cproc.contrast.quantized();
> +	config->sat = frameContext.cproc.saturation.quantized();
>  }
>  
>  REGISTER_IPA_ALGORITHM(ColorProcessing, "ColorProcessing")
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index f85a130d9c23..651558bdf42d 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -27,11 +27,17 @@
>  #include <libipa/camera_sensor_helper.h>
>  #include <libipa/fc_queue.h>
>  #include "libipa/agc_mean_luminance.h"
> +#include "libipa/fixedpoint.h"

While at it, could you unify the include style ?

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  
>  namespace libcamera {
>  
>  namespace ipa::rkisp1 {
>  
> +/* Fixed point types used by CPROC */
> +using BrightnessQ = Q1_7;
> +using ContrastQ = UQ1_7;
> +using SaturationQ = UQ1_7;
> +
>  struct IPAHwSettings {
>  	unsigned int numAeCells;
>  	unsigned int numHistogramBins;
> @@ -115,9 +121,9 @@ struct IPAActiveState {
>  	} ccm;
>  
>  	struct {
> -		int8_t brightness;
> -		uint8_t contrast;
> -		uint8_t saturation;
> +		BrightnessQ brightness;
> +		ContrastQ contrast;
> +		SaturationQ saturation;
>  	} cproc;
>  
>  	struct {
> @@ -169,9 +175,10 @@ struct IPAFrameContext : public FrameContext {
>  	} awb;
>  
>  	struct {
> -		int8_t brightness;
> -		uint8_t contrast;
> -		uint8_t saturation;
> +		BrightnessQ brightness;
> +		ContrastQ contrast;
> +		SaturationQ saturation;
> +
>  		bool update;
>  	} cproc;
>

Patch
diff mbox series 

diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp
index d1fff6990d37..4637a824af03 100644
--- a/src/ipa/rkisp1/algorithms/cproc.cpp
+++ b/src/ipa/rkisp1/algorithms/cproc.cpp
@@ -14,6 +14,8 @@ 
 
 #include <libcamera/control_ids.h>
 
+#include "libipa/quantized.h"
+
 /**
  * \file cproc.h
  */
@@ -39,16 +41,6 @@  constexpr float kDefaultBrightness = 0.0f;
 constexpr float kDefaultContrast = 1.0f;
 constexpr float kDefaultSaturation = 1.0f;
 
-int convertBrightness(const float v)
-{
-	return std::clamp<int>(std::lround(v * 128), -128, 127);
-}
-
-int convertContrastOrSaturation(const float v)
-{
-	return std::clamp<int>(std::lround(v * 128), 0, 255);
-}
-
 } /* namespace */
 
 /**
@@ -74,9 +66,9 @@  int ColorProcessing::configure(IPAContext &context,
 {
 	auto &cproc = context.activeState.cproc;
 
-	cproc.brightness = convertBrightness(kDefaultBrightness);
-	cproc.contrast = convertContrastOrSaturation(kDefaultContrast);
-	cproc.saturation = convertContrastOrSaturation(kDefaultSaturation);
+	cproc.brightness = BrightnessQ(kDefaultBrightness);
+	cproc.contrast = ContrastQ(kDefaultContrast);
+	cproc.saturation = SaturationQ(kDefaultSaturation);
 
 	return 0;
 }
@@ -97,35 +89,35 @@  void ColorProcessing::queueRequest(IPAContext &context,
 
 	const auto &brightness = controls.get(controls::Brightness);
 	if (brightness) {
-		int value = convertBrightness(*brightness);
+		BrightnessQ value = *brightness;
 		if (cproc.brightness != value) {
 			cproc.brightness = value;
 			update = true;
 		}
 
-		LOG(RkISP1CProc, Debug) << "Set brightness to " << value;
+		LOG(RkISP1CProc, Debug) << "Set brightness to " << value.value();
 	}
 
 	const auto &contrast = controls.get(controls::Contrast);
 	if (contrast) {
-		int value = convertContrastOrSaturation(*contrast);
+		ContrastQ value = *contrast;
 		if (cproc.contrast != value) {
 			cproc.contrast = value;
 			update = true;
 		}
 
-		LOG(RkISP1CProc, Debug) << "Set contrast to " << value;
+		LOG(RkISP1CProc, Debug) << "Set contrast to " << value.value();
 	}
 
 	const auto saturation = controls.get(controls::Saturation);
 	if (saturation) {
-		int value = convertContrastOrSaturation(*saturation);
+		SaturationQ value = *saturation;
 		if (cproc.saturation != value) {
 			cproc.saturation = value;
 			update = true;
 		}
 
-		LOG(RkISP1CProc, Debug) << "Set saturation to " << value;
+		LOG(RkISP1CProc, Debug) << "Set saturation to " << value.value();
 	}
 
 	frameContext.cproc.brightness = cproc.brightness;
@@ -148,9 +140,9 @@  void ColorProcessing::prepare([[maybe_unused]] IPAContext &context,
 
 	auto config = params->block<BlockType::Cproc>();
 	config.setEnabled(true);
-	config->brightness = frameContext.cproc.brightness;
-	config->contrast = frameContext.cproc.contrast;
-	config->sat = frameContext.cproc.saturation;
+	config->brightness = frameContext.cproc.brightness.quantized();
+	config->contrast = frameContext.cproc.contrast.quantized();
+	config->sat = frameContext.cproc.saturation.quantized();
 }
 
 REGISTER_IPA_ALGORITHM(ColorProcessing, "ColorProcessing")
diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
index f85a130d9c23..651558bdf42d 100644
--- a/src/ipa/rkisp1/ipa_context.h
+++ b/src/ipa/rkisp1/ipa_context.h
@@ -27,11 +27,17 @@ 
 #include <libipa/camera_sensor_helper.h>
 #include <libipa/fc_queue.h>
 #include "libipa/agc_mean_luminance.h"
+#include "libipa/fixedpoint.h"
 
 namespace libcamera {
 
 namespace ipa::rkisp1 {
 
+/* Fixed point types used by CPROC */
+using BrightnessQ = Q1_7;
+using ContrastQ = UQ1_7;
+using SaturationQ = UQ1_7;
+
 struct IPAHwSettings {
 	unsigned int numAeCells;
 	unsigned int numHistogramBins;
@@ -115,9 +121,9 @@  struct IPAActiveState {
 	} ccm;
 
 	struct {
-		int8_t brightness;
-		uint8_t contrast;
-		uint8_t saturation;
+		BrightnessQ brightness;
+		ContrastQ contrast;
+		SaturationQ saturation;
 	} cproc;
 
 	struct {
@@ -169,9 +175,10 @@  struct IPAFrameContext : public FrameContext {
 	} awb;
 
 	struct {
-		int8_t brightness;
-		uint8_t contrast;
-		uint8_t saturation;
+		BrightnessQ brightness;
+		ContrastQ contrast;
+		SaturationQ saturation;
+
 		bool update;
 	} cproc;