From patchwork Tue Jun 16 14:28:55 2026 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stefan Klug X-Patchwork-Id: 26907 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id 6C6CAC3261 for ; Tue, 16 Jun 2026 14:29:08 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 8AF5C626DE; Tue, 16 Jun 2026 16:29:07 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="nJUUBdho"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 50822623CC for ; Tue, 16 Jun 2026 16:29:06 +0200 (CEST) Received: from ideasonboard.com (unknown [IPv6:2a00:6020:448c:6c00:f9ec:e24d:190b:e780]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 6B3C12EC; Tue, 16 Jun 2026 16:28:32 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1781620112; bh=n6Dr8NMyAjvZnpn6NftDTkNyhqduRaXb1yx+p/UxGls=; h=From:To:Cc:Subject:Date:From; b=nJUUBdhouQRSOTP61b1331cMNvZrob8yiSr9OwLK0F55/KfipWd84ZW+KubYZFoge NvP0E9b2ggcMALxF4WsRD+q2l7QSaI9+Xwvn9nHH6l8+rE4+CvfiCQngDYjfOLhd4E ntNN7hpkdNZkQIQmqJyma4mIevJZ1qCqeU7yuw0s= From: Stefan Klug To: libcamera-devel@lists.libcamera.org Cc: Stefan Klug , Kieran Bingham , Laurent Pinchart Subject: [PATCH v3] ipa: rkisp1: cproc: Fix brightness and contrast value handling Date: Tue, 16 Jun 2026 16:28:55 +0200 Message-ID: <20260616142900.438522-1-stefan.klug@ideasonboard.com> X-Mailer: git-send-email 2.53.0 MIME-Version: 1.0 X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" 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 Reviewed-by: Kieran Bingham Reviewed-by: Laurent Pinchart --- 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(-) 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;