[v2] ipa: rkisp1: cproc: Fix brightness and contrast value handling

25906 diff mbox series

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>

---

This patch needs to be applied on top of the quantization series:
https://patchwork.libcamera.org/project/libcamera/list/?series=5708

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 | 58 ++++++++++++++++++++++-------
 src/ipa/rkisp1/ipa_context.cpp      | 17 +++++++--
 src/ipa/rkisp1/ipa_context.h        |  3 ++
 3 files changed, 61 insertions(+), 17 deletions(-)

Quoting Stefan Klug (2026-01-21 13:39:07)
> 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

did you mean [-0.5, 0.5] ? 

Measuring the limits on my board with camshark metadata results:

 | Contrast | Brightness-min | Brightness-max |
 |----------|----------------|----------------|
 |   1.99   |     -0.0039    |    0.98828     |
 |   1.00   |     -0.5       |    0.49609     |
 |   0.00   |     -1.0       |   -0.00390     |

But I think that's correct from my understanding so far, just some
floating point variance which I'm not concerned about so:


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

> contrast + brightness.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> 
> ---
> 
> This patch needs to be applied on top of the quantization series:
> https://patchwork.libcamera.org/project/libcamera/list/?series=5708
> 
> 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 | 58 ++++++++++++++++++++++-------
>  src/ipa/rkisp1/ipa_context.cpp      | 17 +++++++--
>  src/ipa/rkisp1/ipa_context.h        |  3 ++
>  3 files changed, 61 insertions(+), 17 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp
> index 7484e4780094..e0f6ac9816f7 100644
> --- a/src/ipa/rkisp1/algorithms/cproc.cpp
> +++ b/src/ipa/rkisp1/algorithms/cproc.cpp
> @@ -46,6 +46,33 @@ 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 + brightness
> +        *
> +        * To implement above formula with the given hardware we need adjust the
> +        * hardware brightness value depending on the contrast.
> +        */
> +       cproc.brightness = (1 - cproc.contrast.value()) + 2 * cproc.requestedBrightness;
> +       cproc.actualBrightness = (cproc.brightness.value() - (1 - cproc.contrast.value())) / 2;
> +}
> +
>  } /* namespace */
>  
>  /**
> @@ -76,8 +103,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 +127,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 +138,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 +175,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 +211,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 80b035044cda..72ec64532dd8 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -122,6 +122,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;
> -- 
> 2.51.0
>