[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
>

On Thu, Jan 22, 2026 at 12:51:37PM +0000, Kieran Bingham wrote:
> 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

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

b = - contrast + 1.0 + 2 * brightness

> > +        *
> > +        * To implement above formula with the given hardware we need adjust the

s/above formula/the above formula/ (or just "the formula")

> > +        * hardware brightness value depending on the contrast.
> > +        */
> > +       cproc.brightness = (1 - cproc.contrast.value()) + 2 * cproc.requestedBrightness;

You can drop the parentheses.

> > +       cproc.actualBrightness = (cproc.brightness.value() - (1 - cproc.contrast.value())) / 2;

And here too ?

	cproc.actualBrightness = (cproc.brightness.value() - 1 + cproc.contrast.value()) / 2;

What's not immediately apparent from the code is that the last line is
there to clamp the requested value. Explaining that in the comment would
be nice. Maybe something like this:

	 * 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 * contract + 0.5 - 0.5 * contrast + user brightness
	 *      = Yin * contract + (1.0 - contrast + 2 * user brightness) / 2
	 */
	cproc.brightness = 1.0 - 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.0 + cproc.contrast.value()) / 2;

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > +}
> > +
> >  } /* 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;