[v3] ipa: rkisp1: cproc: Fix brightness and contrast value handling
diff mbox series

Message ID 20260616142900.438522-1-stefan.klug@ideasonboard.com
State Accepted
Headers show
Series
  • [v3] ipa: rkisp1: cproc: Fix brightness and contrast value handling
Related show

Commit Message

Stefan Klug June 16, 2026, 2:28 p.m. UTC 
By reverse engineering it was discovered that the transfer curve applied
by the CPROC module is

Yout = Yin * contrast + brightness/2.

However the most common expectation on contrast is to not change the
middle gray of an image. So the expected curve is:

Yout = (Yin - 0.5) * contrast + 0.5 + brightness

Map the user expectation to the hardware by changing the brightness
value written to the hardware. This also includes multiplying the
requested brightness value by two.

Note: The limits of the brightness control are left as is even though
they can not be reached without changing the contrast. But limiting them
to [-0.5, 0.5] would cut off parts of the combined control space of
contrast + brightness.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

---

Changes in v3:
- Collected tags
- Fixed sign in commit message
- Improved code comments and dropped unnecessary parenthesis as
  suggested by Laurent.

Changes in v2:
- Moved calculation of brightness into helper function, to be able to
  reuse it inside configure()
- Moved calculation of actualBrightness from process() to the helper
  function
- Added documentation for the new context variables
- Removed unnecessary curly braces
- Improved comment
---
 src/ipa/rkisp1/algorithms/cproc.cpp | 68 +++++++++++++++++++++++------
 src/ipa/rkisp1/ipa_context.cpp      | 17 ++++++--
 src/ipa/rkisp1/ipa_context.h        |  3 ++
 3 files changed, 71 insertions(+), 17 deletions(-)

Patch
diff mbox series 

diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp
index 35ad2d7e0d9d..fae7510de07b 100644
--- a/src/ipa/rkisp1/algorithms/cproc.cpp
+++ b/src/ipa/rkisp1/algorithms/cproc.cpp
@@ -46,6 +46,43 @@  constexpr float kDefaultSaturation = 1.0f;
  */
 constexpr float kHueScale = -90.0f;
 
+void applyRequestedBrightness(IPAActiveState &activeState)
+{
+	auto &cproc = activeState.cproc;
+
+	/*
+	 * The CPROC module applies the transfer function
+	 *
+	 * Yout = Yin * contrast + brightness/2
+	 *
+	 * The calculations are done one bit wider than the input data to
+	 * account for the maximum contrast of 2x without clamping. Brightness
+	 * is applied after and therefore affects the output only with half the
+	 * value.
+	 *
+	 * Most users expect that changing contrast doesn't change the middle
+	 * gray and that the brightness value is normalized to one. So they
+	 * expect a transfer function of
+	 *
+	 * Yout = (Yin - 0.5) * contrast + 0.5 + user brightness
+	 *
+	 * This can be achieved by computing the hardware brightness value from
+	 * the user brightness and contrast:
+	 *
+	 * Yout = Yin * contrast + 0.5 - 0.5 * contrast + user brightness
+	 *      = Yin * contrast + (1 - contrast + 2 * user brightness) / 2
+	 */
+	cproc.brightness = 1 - cproc.contrast.value() + 2 * cproc.requestedBrightness;
+
+	/*
+	 * The cproc.brightness value is clamped to the hardware limits by the
+	 * Quantized class. This effectively clamps the user brightness to
+	 * limits that now depend on the contrast. Calculate the actual user
+	 * brightness to report in metadata by inverting the formula.
+	 */
+	cproc.actualBrightness = (cproc.brightness.value() - 1 + cproc.contrast.value()) / 2;
+}
+
 } /* namespace */
 
 /**
@@ -76,8 +113,10 @@  int ColorProcessing::configure(IPAContext &context,
 {
 	auto &cproc = context.activeState.cproc;
 
-	cproc.brightness = BrightnessQ(kDefaultBrightness);
+	cproc.requestedBrightness = kDefaultBrightness;
 	cproc.contrast = ContrastQ(kDefaultContrast);
+	applyRequestedBrightness(context.activeState);
+
 	cproc.hue = HueQ(kDefaultHue);
 	cproc.saturation = SaturationQ(kDefaultSaturation);
 
@@ -98,17 +137,6 @@  void ColorProcessing::queueRequest(IPAContext &context,
 	if (frame == 0)
 		update = true;
 
-	const auto &brightness = controls.get(controls::Brightness);
-	if (brightness) {
-		BrightnessQ value = *brightness;
-		if (cproc.brightness != value) {
-			cproc.brightness = value;
-			update = true;
-		}
-
-		LOG(RkISP1CProc, Debug) << "Set brightness to " << value;
-	}
-
 	const auto &contrast = controls.get(controls::Contrast);
 	if (contrast) {
 		ContrastQ value = *contrast;
@@ -120,6 +148,19 @@  void ColorProcessing::queueRequest(IPAContext &context,
 		LOG(RkISP1CProc, Debug) << "Set contrast to " << value;
 	}
 
+	const auto &brightness = controls.get(controls::Brightness);
+	if (brightness)
+		cproc.requestedBrightness = *brightness;
+
+	if (update || brightness) {
+		BrightnessQ old = cproc.brightness;
+		applyRequestedBrightness(context.activeState);
+		if (cproc.brightness != old)
+			update = true;
+
+		LOG(RkISP1CProc, Debug) << "Set brightness to " << cproc.actualBrightness;
+	}
+
 	const auto &hue = controls.get(controls::Hue);
 	if (hue) {
 		/* Scale the Hue from ]-90, +90] */
@@ -144,6 +185,7 @@  void ColorProcessing::queueRequest(IPAContext &context,
 	}
 
 	frameContext.cproc.brightness = cproc.brightness;
+	frameContext.cproc.actualBrightness = cproc.actualBrightness;
 	frameContext.cproc.contrast = cproc.contrast;
 	frameContext.cproc.hue = cproc.hue;
 	frameContext.cproc.saturation = cproc.saturation;
@@ -179,7 +221,7 @@  void ColorProcessing::process([[maybe_unused]] IPAContext &context,
 			      [[maybe_unused]] const rkisp1_stat_buffer *stats,
 			      ControlList &metadata)
 {
-	metadata.set(controls::Brightness, frameContext.cproc.brightness.value());
+	metadata.set(controls::Brightness, frameContext.cproc.actualBrightness);
 	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.cpp b/src/ipa/rkisp1/ipa_context.cpp
index 15cb0afe9fe8..f9d0aff058bc 100644
--- a/src/ipa/rkisp1/ipa_context.cpp
+++ b/src/ipa/rkisp1/ipa_context.cpp
@@ -232,9 +232,15 @@  namespace libcamera::ipa::rkisp1 {
 /**
  * \var IPAActiveState::cproc
  * \brief State for the Color Processing algorithm
+  *
+ * \var IPAActiveState::cproc.requestedBrightness
+ * \brief Brightness level requested by the user
+  *
+ * \var IPAActiveState::cproc.actualBrightness
+ * \brief Brightness level after contrast compensation
  *
- * \struct IPAActiveState::cproc.brightness
- * \brief Brightness level
+ * \var IPAActiveState::cproc.brightness
+ * \brief Hardware brightness level
  *
  * \var IPAActiveState::cproc.contrast
  * \brief Contrast level
@@ -400,8 +406,11 @@  namespace libcamera::ipa::rkisp1 {
  * \var IPAFrameContext::cproc
  * \brief Color Processing parameters for this frame
  *
- * \struct IPAFrameContext::cproc.brightness
- * \brief Brightness level
+ * \var IPAFrameContext::cproc.actualBrightness
+ * \brief Brightness level after contrast compensation
+ *
+ * \var IPAFrameContext::cproc.brightness
+ * \brief Hardware brightness level
  *
  * \var IPAFrameContext::cproc.contrast
  * \brief Contrast level
diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
index e61391bb1510..e5f70d934d67 100644
--- a/src/ipa/rkisp1/ipa_context.h
+++ b/src/ipa/rkisp1/ipa_context.h
@@ -118,6 +118,8 @@  struct IPAActiveState {
 	} ccm;
 
 	struct {
+		float requestedBrightness;
+		float actualBrightness;
 		BrightnessQ brightness;
 		ContrastQ contrast;
 		HueQ hue;
@@ -181,6 +183,7 @@  struct IPAFrameContext : public FrameContext {
 	} awb;
 
 	struct {
+		float actualBrightness;
 		BrightnessQ brightness;
 		ContrastQ contrast;
 		HueQ hue;