[v5,09/16] ipa: rkisp1: cproc: Provide a Hue control
diff mbox series

Message ID 20260114173918.1744023-10-kieran.bingham@ideasonboard.com
State Superseded
Headers show
Series
  • libipa: Introduce a Quantized type
Related show

Commit Message

Kieran Bingham Jan. 14, 2026, 5:39 p.m. UTC 
The RKISP1 supports a configurable Hue as part of the colour processing
unit (cproc).

Implement the new control converting to the hardware scale accordingly
and report the applied control in the completed request metadata.

This is implemented as a phase shift of the chrominance values between
-90 and +87.188 degrees according to the datasheet however the type
itself would imply that this is a range between -90 and 89.2969.

Moreover, the hardware applies the inverse phase shift to the operation
expected and documented by libcamera, so we apply a negative scale when
converting the libcamera control to the Q<1,7> type, resulting in a
range of -89.2969 to +90.0 degrees control.

Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

---

v5:
- Use Q<1, 7> directly and handle scaling in cproc.cpp
- Use full string of quantized in debug log
- Invert/Negate the hardware hue direction

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/ipa/rkisp1/algorithms/cproc.cpp | 28 ++++++++++++++++++++++++++++
 src/ipa/rkisp1/ipa_context.h        |  3 +++
 2 files changed, 31 insertions(+)

Comments

Barnabás Pőcze Jan. 15, 2026, 3:11 p.m. UTC | #1 
2026. 01. 14. 18:39 keltezéssel, Kieran Bingham írta:
> The RKISP1 supports a configurable Hue as part of the colour processing
> unit (cproc).
> 
> Implement the new control converting to the hardware scale accordingly
> and report the applied control in the completed request metadata.
> 
> This is implemented as a phase shift of the chrominance values between
> -90 and +87.188 degrees according to the datasheet however the type
> itself would imply that this is a range between -90 and 89.2969.
> 
> Moreover, the hardware applies the inverse phase shift to the operation
> expected and documented by libcamera, so we apply a negative scale when
> converting the libcamera control to the Q<1,7> type, resulting in a
> range of -89.2969 to +90.0 degrees control.
> 
> Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> ---

A quick test with camshark seems ok.

Tested-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>


> 
> v5:
> - Use Q<1, 7> directly and handle scaling in cproc.cpp
> - Use full string of quantized in debug log
> - Invert/Negate the hardware hue direction
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>   src/ipa/rkisp1/algorithms/cproc.cpp | 28 ++++++++++++++++++++++++++++
>   src/ipa/rkisp1/ipa_context.h        |  3 +++
>   2 files changed, 31 insertions(+)
> 
> diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp
> index e9e2b5444bc9..7484e4780094 100644
> --- a/src/ipa/rkisp1/algorithms/cproc.cpp
> +++ b/src/ipa/rkisp1/algorithms/cproc.cpp
> @@ -37,8 +37,15 @@ namespace {
>   
>   constexpr float kDefaultBrightness = 0.0f;
>   constexpr float kDefaultContrast = 1.0f;
> +constexpr float kDefaultHue = 0.0f;
>   constexpr float kDefaultSaturation = 1.0f;
>   
> +/*
> + * The Hue scale is negated as the hardware performs the opposite phase shift
> + * to what is expected and defined from the libcamera Hue control value.
> + */
> +constexpr float kHueScale = -90.0f;
> +
>   } /* namespace */
>   
>   /**
> @@ -53,6 +60,11 @@ int ColorProcessing::init(IPAContext &context,
>   	cmap[&controls::Contrast] = ControlInfo(0.0f, 1.993f, kDefaultContrast);
>   	cmap[&controls::Saturation] = ControlInfo(0.0f, 1.993f, kDefaultSaturation);
>   
> +	/* Hue adjustment is negated by kHueScale, min/max are swapped */
> +	cmap[&controls::Hue] = ControlInfo(HueQ::TraitsType::max * kHueScale,
> +					   HueQ::TraitsType::min * kHueScale,
> +					   kDefaultHue);
> +
>   	return 0;
>   }
>   
> @@ -66,6 +78,7 @@ int ColorProcessing::configure(IPAContext &context,
>   
>   	cproc.brightness = BrightnessQ(kDefaultBrightness);
>   	cproc.contrast = ContrastQ(kDefaultContrast);
> +	cproc.hue = HueQ(kDefaultHue);
>   	cproc.saturation = SaturationQ(kDefaultSaturation);
>   
>   	return 0;
> @@ -107,6 +120,18 @@ void ColorProcessing::queueRequest(IPAContext &context,
>   		LOG(RkISP1CProc, Debug) << "Set contrast to " << value;
>   	}
>   
> +	const auto &hue = controls.get(controls::Hue);
> +	if (hue) {
> +		/* Scale the Hue from ]-90, +90] */
> +		HueQ value = *hue / kHueScale;
> +		if (cproc.hue != value) {
> +			cproc.hue = value;
> +			update = true;
> +		}
> +
> +		LOG(RkISP1CProc, Debug) << "Set hue to " << value;
> +	}
> +
>   	const auto saturation = controls.get(controls::Saturation);
>   	if (saturation) {
>   		SaturationQ value = *saturation;
> @@ -120,6 +145,7 @@ void ColorProcessing::queueRequest(IPAContext &context,
>   
>   	frameContext.cproc.brightness = cproc.brightness;
>   	frameContext.cproc.contrast = cproc.contrast;
> +	frameContext.cproc.hue = cproc.hue;
>   	frameContext.cproc.saturation = cproc.saturation;
>   	frameContext.cproc.update = update;
>   }
> @@ -140,6 +166,7 @@ void ColorProcessing::prepare([[maybe_unused]] IPAContext &context,
>   	config.setEnabled(true);
>   	config->brightness = frameContext.cproc.brightness.quantized();
>   	config->contrast = frameContext.cproc.contrast.quantized();
> +	config->hue = frameContext.cproc.hue.quantized();
>   	config->sat = frameContext.cproc.saturation.quantized();
>   }
>   
> @@ -154,6 +181,7 @@ void ColorProcessing::process([[maybe_unused]] IPAContext &context,
>   {
>   	metadata.set(controls::Brightness, frameContext.cproc.brightness.value());
>   	metadata.set(controls::Contrast, frameContext.cproc.contrast.value());
> +	metadata.set(controls::Hue, frameContext.cproc.hue.value() * kHueScale);
>   	metadata.set(controls::Saturation, frameContext.cproc.saturation.value());
>   }
>   
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index cdb9a17b5adc..80b035044cda 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -36,6 +36,7 @@ namespace ipa::rkisp1 {
>   /* Fixed point types used by CPROC */
>   using BrightnessQ = Q<1, 7>;
>   using ContrastQ = UQ<1, 7>;
> +using HueQ = Q<1, 7>;
>   using SaturationQ = UQ<1, 7>;
>   
>   struct IPAHwSettings {
> @@ -123,6 +124,7 @@ struct IPAActiveState {
>   	struct {
>   		BrightnessQ brightness;
>   		ContrastQ contrast;
> +		HueQ hue;
>   		SaturationQ saturation;
>   	} cproc;
>   
> @@ -181,6 +183,7 @@ struct IPAFrameContext : public FrameContext {
>   	struct {
>   		BrightnessQ brightness;
>   		ContrastQ contrast;
> +		HueQ hue;
>   		SaturationQ saturation;
>   
>   		bool update;
Barnabás Pőcze Jan. 15, 2026, 3:37 p.m. UTC | #2 
2026. 01. 15. 16:11 keltezéssel, Barnabás Pőcze írta:
> 2026. 01. 14. 18:39 keltezéssel, Kieran Bingham írta:
>> The RKISP1 supports a configurable Hue as part of the colour processing
>> unit (cproc).
>>
>> Implement the new control converting to the hardware scale accordingly
>> and report the applied control in the completed request metadata.
>>
>> This is implemented as a phase shift of the chrominance values between
>> -90 and +87.188 degrees according to the datasheet however the type
>> itself would imply that this is a range between -90 and 89.2969.
>>
>> Moreover, the hardware applies the inverse phase shift to the operation
>> expected and documented by libcamera, so we apply a negative scale when
>> converting the libcamera control to the Q<1,7> type, resulting in a
>> range of -89.2969 to +90.0 degrees control.
>>
>> Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>
>> ---
> 
> A quick test with camshark seems ok.
> 
> Tested-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> 
> 
>>
>> v5:
>> - Use Q<1, 7> directly and handle scaling in cproc.cpp
>> - Use full string of quantized in debug log
>> - Invert/Negate the hardware hue direction
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>   src/ipa/rkisp1/algorithms/cproc.cpp | 28 ++++++++++++++++++++++++++++
>>   src/ipa/rkisp1/ipa_context.h        |  3 +++
>>   2 files changed, 31 insertions(+)
>>
>> diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp
>> index e9e2b5444bc9..7484e4780094 100644
>> --- a/src/ipa/rkisp1/algorithms/cproc.cpp
>> +++ b/src/ipa/rkisp1/algorithms/cproc.cpp
>> @@ -37,8 +37,15 @@ namespace {
>>   constexpr float kDefaultBrightness = 0.0f;
>>   constexpr float kDefaultContrast = 1.0f;
>> +constexpr float kDefaultHue = 0.0f;
>>   constexpr float kDefaultSaturation = 1.0f;
>> +/*
>> + * The Hue scale is negated as the hardware performs the opposite phase shift
>> + * to what is expected and defined from the libcamera Hue control value.
>> + */
>> +constexpr float kHueScale = -90.0f;
>> +
>>   } /* namespace */
>>   /**
>> @@ -53,6 +60,11 @@ int ColorProcessing::init(IPAContext &context,
>>       cmap[&controls::Contrast] = ControlInfo(0.0f, 1.993f, kDefaultContrast);
>>       cmap[&controls::Saturation] = ControlInfo(0.0f, 1.993f, kDefaultSaturation);
>> +    /* Hue adjustment is negated by kHueScale, min/max are swapped */
>> +    cmap[&controls::Hue] = ControlInfo(HueQ::TraitsType::max * kHueScale,
>> +                       HueQ::TraitsType::min * kHueScale,
>> +                       kDefaultHue);
>> +
>>       return 0;
>>   }
>> @@ -66,6 +78,7 @@ int ColorProcessing::configure(IPAContext &context,
>>       cproc.brightness = BrightnessQ(kDefaultBrightness);
>>       cproc.contrast = ContrastQ(kDefaultContrast);
>> +    cproc.hue = HueQ(kDefaultHue);
>>       cproc.saturation = SaturationQ(kDefaultSaturation);
>>       return 0;
>> @@ -107,6 +120,18 @@ void ColorProcessing::queueRequest(IPAContext &context,
>>           LOG(RkISP1CProc, Debug) << "Set contrast to " << value;
>>       }
>> +    const auto &hue = controls.get(controls::Hue);
>> +    if (hue) {
>> +        /* Scale the Hue from ]-90, +90] */
>> +        HueQ value = *hue / kHueScale;
>> +        if (cproc.hue != value) {
>> +            cproc.hue = value;
>> +            update = true;
>> +        }
>> +
>> +        LOG(RkISP1CProc, Debug) << "Set hue to " << value;
>> +    }
>> +
>>       const auto saturation = controls.get(controls::Saturation);
>>       if (saturation) {
>>           SaturationQ value = *saturation;
>> @@ -120,6 +145,7 @@ void ColorProcessing::queueRequest(IPAContext &context,
>>       frameContext.cproc.brightness = cproc.brightness;
>>       frameContext.cproc.contrast = cproc.contrast;
>> +    frameContext.cproc.hue = cproc.hue;
>>       frameContext.cproc.saturation = cproc.saturation;
>>       frameContext.cproc.update = update;
>>   }
>> @@ -140,6 +166,7 @@ void ColorProcessing::prepare([[maybe_unused]] IPAContext &context,
>>       config.setEnabled(true);
>>       config->brightness = frameContext.cproc.brightness.quantized();
>>       config->contrast = frameContext.cproc.contrast.quantized();
>> +    config->hue = frameContext.cproc.hue.quantized();
>>       config->sat = frameContext.cproc.saturation.quantized();
>>   }
>> @@ -154,6 +181,7 @@ void ColorProcessing::process([[maybe_unused]] IPAContext &context,
>>   {
>>       metadata.set(controls::Brightness, frameContext.cproc.brightness.value());
>>       metadata.set(controls::Contrast, frameContext.cproc.contrast.value());
>> +    metadata.set(controls::Hue, frameContext.cproc.hue.value() * kHueScale);

I suppose there is one thing that might be somewhat annoying: if it is set to 0,
then the metadata returns -0.


>>       metadata.set(controls::Saturation, frameContext.cproc.saturation.value());
>>   }
>> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
>> index cdb9a17b5adc..80b035044cda 100644
>> --- a/src/ipa/rkisp1/ipa_context.h
>> +++ b/src/ipa/rkisp1/ipa_context.h
>> @@ -36,6 +36,7 @@ namespace ipa::rkisp1 {
>>   /* Fixed point types used by CPROC */
>>   using BrightnessQ = Q<1, 7>;
>>   using ContrastQ = UQ<1, 7>;
>> +using HueQ = Q<1, 7>;
>>   using SaturationQ = UQ<1, 7>;
>>   struct IPAHwSettings {
>> @@ -123,6 +124,7 @@ struct IPAActiveState {
>>       struct {
>>           BrightnessQ brightness;
>>           ContrastQ contrast;
>> +        HueQ hue;
>>           SaturationQ saturation;
>>       } cproc;
>> @@ -181,6 +183,7 @@ struct IPAFrameContext : public FrameContext {
>>       struct {
>>           BrightnessQ brightness;
>>           ContrastQ contrast;
>> +        HueQ hue;
>>           SaturationQ saturation;
>>           bool update;
>

Patch
diff mbox series 

diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp
index e9e2b5444bc9..7484e4780094 100644
--- a/src/ipa/rkisp1/algorithms/cproc.cpp
+++ b/src/ipa/rkisp1/algorithms/cproc.cpp
@@ -37,8 +37,15 @@  namespace {
 
 constexpr float kDefaultBrightness = 0.0f;
 constexpr float kDefaultContrast = 1.0f;
+constexpr float kDefaultHue = 0.0f;
 constexpr float kDefaultSaturation = 1.0f;
 
+/*
+ * The Hue scale is negated as the hardware performs the opposite phase shift
+ * to what is expected and defined from the libcamera Hue control value.
+ */
+constexpr float kHueScale = -90.0f;
+
 } /* namespace */
 
 /**
@@ -53,6 +60,11 @@  int ColorProcessing::init(IPAContext &context,
 	cmap[&controls::Contrast] = ControlInfo(0.0f, 1.993f, kDefaultContrast);
 	cmap[&controls::Saturation] = ControlInfo(0.0f, 1.993f, kDefaultSaturation);
 
+	/* Hue adjustment is negated by kHueScale, min/max are swapped */
+	cmap[&controls::Hue] = ControlInfo(HueQ::TraitsType::max * kHueScale,
+					   HueQ::TraitsType::min * kHueScale,
+					   kDefaultHue);
+
 	return 0;
 }
 
@@ -66,6 +78,7 @@  int ColorProcessing::configure(IPAContext &context,
 
 	cproc.brightness = BrightnessQ(kDefaultBrightness);
 	cproc.contrast = ContrastQ(kDefaultContrast);
+	cproc.hue = HueQ(kDefaultHue);
 	cproc.saturation = SaturationQ(kDefaultSaturation);
 
 	return 0;
@@ -107,6 +120,18 @@  void ColorProcessing::queueRequest(IPAContext &context,
 		LOG(RkISP1CProc, Debug) << "Set contrast to " << value;
 	}
 
+	const auto &hue = controls.get(controls::Hue);
+	if (hue) {
+		/* Scale the Hue from ]-90, +90] */
+		HueQ value = *hue / kHueScale;
+		if (cproc.hue != value) {
+			cproc.hue = value;
+			update = true;
+		}
+
+		LOG(RkISP1CProc, Debug) << "Set hue to " << value;
+	}
+
 	const auto saturation = controls.get(controls::Saturation);
 	if (saturation) {
 		SaturationQ value = *saturation;
@@ -120,6 +145,7 @@  void ColorProcessing::queueRequest(IPAContext &context,
 
 	frameContext.cproc.brightness = cproc.brightness;
 	frameContext.cproc.contrast = cproc.contrast;
+	frameContext.cproc.hue = cproc.hue;
 	frameContext.cproc.saturation = cproc.saturation;
 	frameContext.cproc.update = update;
 }
@@ -140,6 +166,7 @@  void ColorProcessing::prepare([[maybe_unused]] IPAContext &context,
 	config.setEnabled(true);
 	config->brightness = frameContext.cproc.brightness.quantized();
 	config->contrast = frameContext.cproc.contrast.quantized();
+	config->hue = frameContext.cproc.hue.quantized();
 	config->sat = frameContext.cproc.saturation.quantized();
 }
 
@@ -154,6 +181,7 @@  void ColorProcessing::process([[maybe_unused]] IPAContext &context,
 {
 	metadata.set(controls::Brightness, frameContext.cproc.brightness.value());
 	metadata.set(controls::Contrast, frameContext.cproc.contrast.value());
+	metadata.set(controls::Hue, frameContext.cproc.hue.value() * kHueScale);
 	metadata.set(controls::Saturation, frameContext.cproc.saturation.value());
 }
 
diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
index cdb9a17b5adc..80b035044cda 100644
--- a/src/ipa/rkisp1/ipa_context.h
+++ b/src/ipa/rkisp1/ipa_context.h
@@ -36,6 +36,7 @@  namespace ipa::rkisp1 {
 /* Fixed point types used by CPROC */
 using BrightnessQ = Q<1, 7>;
 using ContrastQ = UQ<1, 7>;
+using HueQ = Q<1, 7>;
 using SaturationQ = UQ<1, 7>;
 
 struct IPAHwSettings {
@@ -123,6 +124,7 @@  struct IPAActiveState {
 	struct {
 		BrightnessQ brightness;
 		ContrastQ contrast;
+		HueQ hue;
 		SaturationQ saturation;
 	} cproc;
 
@@ -181,6 +183,7 @@  struct IPAFrameContext : public FrameContext {
 	struct {
 		BrightnessQ brightness;
 		ContrastQ contrast;
+		HueQ hue;
 		SaturationQ saturation;
 
 		bool update;