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

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

Commit Message

Kieran Bingham Jan. 21, 2026, 5:37 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>
Tested-by: Barnabás Pőcze <barnabas.pocze@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

Stefan Klug Jan. 23, 2026, 2:14 p.m. UTC | #1 
Hi Kieran,

Thank you for the patch.

Quoting Kieran Bingham (2026-01-21 18:37:28)
> 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>
> Tested-by: Barnabás Pőcze <barnabas.pocze@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(+)
> 
> 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;

Here the Quantized type really makes things way easier...

> +               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 just realize that you now carry the kHueScale all over. This is
basically a similar problem like mapping float gains to sensor gain
values. So in theory you could create specialized quantized traits that
include the kHueScale. So maybe at some point we have a generalized
ScaledFixedPointQTraits... :-)

Wait this is interesting I need to write it...

template<float S, typename T>
struct ScaledQTraits {
        using QuantizedType = typename T::QuantizedType;
	static constexpr QuantizedType qMin = T::qMin;
	static constexpr QuantizedType qMax = T::qMax;

	static constexpr float toFloat(QuantizedType q)
	{
		return T::toFloat(q) * S;
	}

	static constexpr float min = toFloat(qMin);
	static constexpr float max = toFloat(qMax);

	static QuantizedType fromFloat(float v)
	{
		return T::fromFloat(v / S);
	}
};

using HueQ = Quantized<ScaledQTraits<-90.0f, FixedPointQTraits<1, 7, uint8_t> >>;



... but requires C++-20 to compile due to the float template arg.

Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>

Cheers,
Stefan

>         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;
> -- 
> 2.52.0
>
Kieran Bingham Jan. 23, 2026, 3:24 p.m. UTC | #2 
Quoting Stefan Klug (2026-01-23 14:14:32)
> Hi Kieran,
> 
> Thank you for the patch.
> 
> Quoting Kieran Bingham (2026-01-21 18:37:28)
> > 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>
> > Tested-by: Barnabás Pőcze <barnabas.pocze@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(+)
> > 
> > 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;
> 
> Here the Quantized type really makes things way easier...
> 
> > +               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 just realize that you now carry the kHueScale all over. This is
> basically a similar problem like mapping float gains to sensor gain
> values. So in theory you could create specialized quantized traits that
> include the kHueScale. So maybe at some point we have a generalized
> ScaledFixedPointQTraits... :-)
> 
> Wait this is interesting I need to write it...
> 
> template<float S, typename T>
> struct ScaledQTraits {
>         using QuantizedType = typename T::QuantizedType;
>         static constexpr QuantizedType qMin = T::qMin;
>         static constexpr QuantizedType qMax = T::qMax;
> 
>         static constexpr float toFloat(QuantizedType q)
>         {
>                 return T::toFloat(q) * S;
>         }
> 
>         static constexpr float min = toFloat(qMin);
>         static constexpr float max = toFloat(qMax);
> 
>         static QuantizedType fromFloat(float v)
>         {
>                 return T::fromFloat(v / S);
>         }
> };
> 
> using HueQ = Quantized<ScaledQTraits<-90.0f, FixedPointQTraits<1, 7, uint8_t> >>;
> 

Yes, I implemented this already (without float scales) - but Laurent
asked me to remove it:

[v4,10/21] ipa: libipa: fixedpoint: Provide a ScaledFixedPoint type
 - https://patchwork.libcamera.org/patch/25040/
[v4,11/21] test: libipa: Provide ScaledFixedPoint tests
 - https://patchwork.libcamera.org/patch/25041/

using HueQ = Quantized<ScaledFixedPointQTraits<Q1_7::TraitsType, 90>>;
 - https://patchwork.libcamera.org/patch/25045/


> ... but requires C++-20 to compile due to the float template arg.

Perhaps a future optimisation after we move to C++20 if there's other
users of scaled types.


> Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>

Thanks

Kieran

> 
> Cheers,
> Stefan
> 
> >         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;
> > -- 
> > 2.52.0
> >

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;