| Message ID | 20260116121658.215712-1-stefan.klug@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Stefan, Thank you for the patch. On Fri, Jan 16, 2026 at 01:16:14PM +0100, Stefan Klug wrote: > 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 > --- > src/ipa/rkisp1/algorithms/cproc.cpp | 38 +++++++++++++++++++++++------ > src/ipa/rkisp1/ipa_context.h | 1 + > 2 files changed, 31 insertions(+), 8 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp > index 7484e4780094..ec2b264a79b9 100644 > --- a/src/ipa/rkisp1/algorithms/cproc.cpp > +++ b/src/ipa/rkisp1/algorithms/cproc.cpp > @@ -100,13 +100,9 @@ void ColorProcessing::queueRequest(IPAContext &context, > > 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; > + cproc.requestedBrightness = *brightness; cproc.requestedBrightness should be reset in ColorProcessing::configure(). > + LOG(RkISP1CProc, Debug) << "Set brightness to " << *brightness; > + update = true; Why do you do this unconditionally now ? > } > > const auto &contrast = controls.get(controls::Contrast); > @@ -120,6 +116,24 @@ void ColorProcessing::queueRequest(IPAContext &context, > LOG(RkISP1CProc, Debug) << "Set contrast to " << value; > } > > + /* > + * The CPROC module applies the transfer function > + * > + * Yout = Yin * contrast + brightness/2 > + * > + * 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 that with the given hardware we need to correct the > + * brightness value. > + */ > + if (update) { > + cproc.brightness = (1 - cproc.contrast.value()) + 2 * cproc.requestedBrightness; I don't think the x2 factor is correct if you don't change the limits. > + } No need for curly braces. > + > const auto &hue = controls.get(controls::Hue); > if (hue) { > /* Scale the Hue from ]-90, +90] */ > @@ -179,7 +193,15 @@ void ColorProcessing::process([[maybe_unused]] IPAContext &context, > [[maybe_unused]] const rkisp1_stat_buffer *stats, > ControlList &metadata) > { > - metadata.set(controls::Brightness, frameContext.cproc.brightness.value()); > + /* > + * Report the actually applied brightness value. See queueRequest() for > + * more details. > + */ > + float actualBrightness = (frameContext.cproc.brightness.value() - > + (1 - frameContext.cproc.contrast.value())) / > + 2; Can we compute this when the controls change and cache the value instead of recomputing it for every frame ? > + > + metadata.set(controls::Brightness, 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.h b/src/ipa/rkisp1/ipa_context.h > index 80b035044cda..309567858b64 100644 > --- a/src/ipa/rkisp1/ipa_context.h > +++ b/src/ipa/rkisp1/ipa_context.h > @@ -122,6 +122,7 @@ struct IPAActiveState { > } ccm; > > struct { > + float requestedBrightness; > BrightnessQ brightness; > ContrastQ contrast; > HueQ hue;
Hi Laurent, Thank you for the review. Quoting Laurent Pinchart (2026-01-21 01:15:12) > Hi Stefan, > > Thank you for the patch. > > On Fri, Jan 16, 2026 at 01:16:14PM +0100, Stefan Klug wrote: > > 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 > > --- > > src/ipa/rkisp1/algorithms/cproc.cpp | 38 +++++++++++++++++++++++------ > > src/ipa/rkisp1/ipa_context.h | 1 + > > 2 files changed, 31 insertions(+), 8 deletions(-) > > > > diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp > > index 7484e4780094..ec2b264a79b9 100644 > > --- a/src/ipa/rkisp1/algorithms/cproc.cpp > > +++ b/src/ipa/rkisp1/algorithms/cproc.cpp > > @@ -100,13 +100,9 @@ void ColorProcessing::queueRequest(IPAContext &context, > > > > 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; > > + cproc.requestedBrightness = *brightness; > > cproc.requestedBrightness should be reset in > ColorProcessing::configure(). Yes > > > + LOG(RkISP1CProc, Debug) << "Set brightness to " << *brightness; > > + update = true; > > Why do you do this unconditionally now ? It's the same as before. Depends if we want to log the control setter, or the actual value applied. I thought the intention was to log the setter as the log wasn't inside the "if (cproc.brightness != value) {". > > > } > > > > const auto &contrast = controls.get(controls::Contrast); > > @@ -120,6 +116,24 @@ void ColorProcessing::queueRequest(IPAContext &context, > > LOG(RkISP1CProc, Debug) << "Set contrast to " << value; > > } > > > > + /* > > + * The CPROC module applies the transfer function > > + * > > + * Yout = Yin * contrast + brightness/2 > > + * > > + * 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 that with the given hardware we need to correct the > > + * brightness value. > > + */ > > + if (update) { > > + cproc.brightness = (1 - cproc.contrast.value()) + 2 * cproc.requestedBrightness; > > I don't think the x2 factor is correct if you don't change the limits. Why not? You can't realize every requested value as it is dependent on the contrast value. But this way we can reach the whole available space. The first "(1 - cproc.contrast.value())" has a output range of [-1, 1] so to be able to set the hardware brightness value in the range of [-1, 1] we need to keep the limits as is. Ideally the limits should change based on the contrast. But we don't have that. > > > + } > > No need for curly braces. Ooops again. > > > + > > const auto &hue = controls.get(controls::Hue); > > if (hue) { > > /* Scale the Hue from ]-90, +90] */ > > @@ -179,7 +193,15 @@ void ColorProcessing::process([[maybe_unused]] IPAContext &context, > > [[maybe_unused]] const rkisp1_stat_buffer *stats, > > ControlList &metadata) > > { > > - metadata.set(controls::Brightness, frameContext.cproc.brightness.value()); > > + /* > > + * Report the actually applied brightness value. See queueRequest() for > > + * more details. > > + */ > > + float actualBrightness = (frameContext.cproc.brightness.value() - > > + (1 - frameContext.cproc.contrast.value())) / > > + 2; > > Can we compute this when the controls change and cache the value instead > of recomputing it for every frame ? Hm, I wouldn't be worried about performance, but it could actually improve the readability of the code. I'll give it a try. Regards, Stefan > > > + > > + metadata.set(controls::Brightness, 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.h b/src/ipa/rkisp1/ipa_context.h > > index 80b035044cda..309567858b64 100644 > > --- a/src/ipa/rkisp1/ipa_context.h > > +++ b/src/ipa/rkisp1/ipa_context.h > > @@ -122,6 +122,7 @@ struct IPAActiveState { > > } ccm; > > > > struct { > > + float requestedBrightness; > > BrightnessQ brightness; > > ContrastQ contrast; > > HueQ hue; > > -- > Regards, > > Laurent Pinchart
On Wed, Jan 21, 2026 at 06:54:37AM +0100, Stefan Klug wrote: > Quoting Laurent Pinchart (2026-01-21 01:15:12) > > On Fri, Jan 16, 2026 at 01:16:14PM +0100, Stefan Klug wrote: > > > 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 > > > --- > > > src/ipa/rkisp1/algorithms/cproc.cpp | 38 +++++++++++++++++++++++------ > > > src/ipa/rkisp1/ipa_context.h | 1 + > > > 2 files changed, 31 insertions(+), 8 deletions(-) > > > > > > diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp > > > index 7484e4780094..ec2b264a79b9 100644 > > > --- a/src/ipa/rkisp1/algorithms/cproc.cpp > > > +++ b/src/ipa/rkisp1/algorithms/cproc.cpp > > > @@ -100,13 +100,9 @@ void ColorProcessing::queueRequest(IPAContext &context, > > > > > > 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; > > > + cproc.requestedBrightness = *brightness; > > > > cproc.requestedBrightness should be reset in > > ColorProcessing::configure(). > > Yes > > > > + LOG(RkISP1CProc, Debug) << "Set brightness to " << *brightness; > > > + update = true; > > > > Why do you do this unconditionally now ? > > It's the same as before. Depends if we want to log the control setter, > or the actual value applied. I thought the intention was to log the > setter as the log wasn't inside the "if (cproc.brightness != value) {". I meant setting update to true, not logging. > > > } > > > > > > const auto &contrast = controls.get(controls::Contrast); > > > @@ -120,6 +116,24 @@ void ColorProcessing::queueRequest(IPAContext &context, > > > LOG(RkISP1CProc, Debug) << "Set contrast to " << value; > > > } > > > > > > + /* > > > + * The CPROC module applies the transfer function > > > + * > > > + * Yout = Yin * contrast + brightness/2 By the way, please record the fact that this is due to the CPROC using 9-bit data internally after multiplying by the contrast (which is limited to x2) to avoid intermediate clamping, and the brightness register therefore being expressed as 9 bits. > > > + * > > > + * 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 BR = (1 - contrast) + brightness * 2 > > > + * > > > + * To implement that with the given hardware we need to correct the > > > + * brightness value. > > > + */ > > > + if (update) { > > > + cproc.brightness = (1 - cproc.contrast.value()) + 2 * cproc.requestedBrightness; > > > > I don't think the x2 factor is correct if you don't change the limits. > > Why not? You can't realize every requested value as it is dependent on > the contrast value. But this way we can reach the whole available space. > > The first "(1 - cproc.contrast.value())" has a output range of [-1, 1] > so to be able to set the hardware brightness value in the range of [-1, > 1] we need to keep the limits as is. You're right. > Ideally the limits should change based on the contrast. But we don't > have that. > > > > > > + } > > > > No need for curly braces. > > Ooops again. > > > > + > > > const auto &hue = controls.get(controls::Hue); > > > if (hue) { > > > /* Scale the Hue from ]-90, +90] */ > > > @@ -179,7 +193,15 @@ void ColorProcessing::process([[maybe_unused]] IPAContext &context, > > > [[maybe_unused]] const rkisp1_stat_buffer *stats, > > > ControlList &metadata) > > > { > > > - metadata.set(controls::Brightness, frameContext.cproc.brightness.value()); > > > + /* > > > + * Report the actually applied brightness value. See queueRequest() for > > > + * more details. > > > + */ > > > + float actualBrightness = (frameContext.cproc.brightness.value() - > > > + (1 - frameContext.cproc.contrast.value())) / > > > + 2; > > > > Can we compute this when the controls change and cache the value instead > > of recomputing it for every frame ? > > Hm, I wouldn't be worried about performance, but it could actually > improve the readability of the code. I'll give it a try. I think it would be more readable, yes. > > > + > > > + metadata.set(controls::Brightness, 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.h b/src/ipa/rkisp1/ipa_context.h > > > index 80b035044cda..309567858b64 100644 > > > --- a/src/ipa/rkisp1/ipa_context.h > > > +++ b/src/ipa/rkisp1/ipa_context.h > > > @@ -122,6 +122,7 @@ struct IPAActiveState { > > > } ccm; > > > > > > struct { > > > + float requestedBrightness; > > > BrightnessQ brightness; > > > ContrastQ contrast; > > > HueQ hue;
Quoting Laurent Pinchart (2026-01-21 12:36:19) > On Wed, Jan 21, 2026 at 06:54:37AM +0100, Stefan Klug wrote: > > Quoting Laurent Pinchart (2026-01-21 01:15:12) > > > On Fri, Jan 16, 2026 at 01:16:14PM +0100, Stefan Klug wrote: > > > > 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 > > > > --- > > > > src/ipa/rkisp1/algorithms/cproc.cpp | 38 +++++++++++++++++++++++------ > > > > src/ipa/rkisp1/ipa_context.h | 1 + > > > > 2 files changed, 31 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp > > > > index 7484e4780094..ec2b264a79b9 100644 > > > > --- a/src/ipa/rkisp1/algorithms/cproc.cpp > > > > +++ b/src/ipa/rkisp1/algorithms/cproc.cpp > > > > @@ -100,13 +100,9 @@ void ColorProcessing::queueRequest(IPAContext &context, > > > > > > > > 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; > > > > + cproc.requestedBrightness = *brightness; > > > > > > cproc.requestedBrightness should be reset in > > > ColorProcessing::configure(). > > > > Yes > > > > > > + LOG(RkISP1CProc, Debug) << "Set brightness to " << *brightness; > > > > + update = true; > > > > > > Why do you do this unconditionally now ? > > > > It's the same as before. Depends if we want to log the control setter, > > or the actual value applied. I thought the intention was to log the > > setter as the log wasn't inside the "if (cproc.brightness != value) {". > > I meant setting update to true, not logging. > > > > > } > > > > > > > > const auto &contrast = controls.get(controls::Contrast); > > > > @@ -120,6 +116,24 @@ void ColorProcessing::queueRequest(IPAContext &context, > > > > LOG(RkISP1CProc, Debug) << "Set contrast to " << value; > > > > } > > > > > > > > + /* > > > > + * The CPROC module applies the transfer function > > > > + * > > > > + * Yout = Yin * contrast + brightness/2 > > By the way, please record the fact that this is due to the CPROC using > 9-bit data internally after multiplying by the contrast (which is > limited to x2) to avoid intermediate clamping, and the brightness > register therefore being expressed as 9 bits. That is the assumption. CPROC also allows 10bit input data so I'm hesitant talking about 9 bits. What about: The calculations in CPROC are done one bit wider than the input data to account for the maximum contrast of 2x without clamping. Brightness is applied after and therefor with only half the numeric 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 What do you mean here? Why would you prefer this notation? Best regards, Stefan > > BR = (1 - contrast) + brightness * 2 > > > > > + * > > > > + * To implement that with the given hardware we need to correct the > > > > + * brightness value. > > > > + */ > > > > + if (update) { > > > > + cproc.brightness = (1 - cproc.contrast.value()) + 2 * cproc.requestedBrightness; > > > > > > I don't think the x2 factor is correct if you don't change the limits. > > > > Why not? You can't realize every requested value as it is dependent on > > the contrast value. But this way we can reach the whole available space. > > > > The first "(1 - cproc.contrast.value())" has a output range of [-1, 1] > > so to be able to set the hardware brightness value in the range of [-1, > > 1] we need to keep the limits as is. > > You're right. > > > Ideally the limits should change based on the contrast. But we don't > > have that. > > > > > > > > > + } > > > > > > No need for curly braces. > > > > Ooops again. > > > > > > + > > > > const auto &hue = controls.get(controls::Hue); > > > > if (hue) { > > > > /* Scale the Hue from ]-90, +90] */ > > > > @@ -179,7 +193,15 @@ void ColorProcessing::process([[maybe_unused]] IPAContext &context, > > > > [[maybe_unused]] const rkisp1_stat_buffer *stats, > > > > ControlList &metadata) > > > > { > > > > - metadata.set(controls::Brightness, frameContext.cproc.brightness.value()); > > > > + /* > > > > + * Report the actually applied brightness value. See queueRequest() for > > > > + * more details. > > > > + */ > > > > + float actualBrightness = (frameContext.cproc.brightness.value() - > > > > + (1 - frameContext.cproc.contrast.value())) / > > > > + 2; > > > > > > Can we compute this when the controls change and cache the value instead > > > of recomputing it for every frame ? > > > > Hm, I wouldn't be worried about performance, but it could actually > > improve the readability of the code. I'll give it a try. > > I think it would be more readable, yes. > > > > > + > > > > + metadata.set(controls::Brightness, 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.h b/src/ipa/rkisp1/ipa_context.h > > > > index 80b035044cda..309567858b64 100644 > > > > --- a/src/ipa/rkisp1/ipa_context.h > > > > +++ b/src/ipa/rkisp1/ipa_context.h > > > > @@ -122,6 +122,7 @@ struct IPAActiveState { > > > > } ccm; > > > > > > > > struct { > > > > + float requestedBrightness; > > > > BrightnessQ brightness; > > > > ContrastQ contrast; > > > > HueQ hue; > > -- > Regards, > > Laurent Pinchart
On Wed, Jan 21, 2026 at 01:14:28PM +0100, Stefan Klug wrote: > Quoting Laurent Pinchart (2026-01-21 12:36:19) > > On Wed, Jan 21, 2026 at 06:54:37AM +0100, Stefan Klug wrote: > > > Quoting Laurent Pinchart (2026-01-21 01:15:12) > > > > On Fri, Jan 16, 2026 at 01:16:14PM +0100, Stefan Klug wrote: > > > > > 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 > > > > > --- > > > > > src/ipa/rkisp1/algorithms/cproc.cpp | 38 +++++++++++++++++++++++------ > > > > > src/ipa/rkisp1/ipa_context.h | 1 + > > > > > 2 files changed, 31 insertions(+), 8 deletions(-) > > > > > > > > > > diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp > > > > > index 7484e4780094..ec2b264a79b9 100644 > > > > > --- a/src/ipa/rkisp1/algorithms/cproc.cpp > > > > > +++ b/src/ipa/rkisp1/algorithms/cproc.cpp > > > > > @@ -100,13 +100,9 @@ void ColorProcessing::queueRequest(IPAContext &context, > > > > > > > > > > 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; > > > > > + cproc.requestedBrightness = *brightness; > > > > > > > > cproc.requestedBrightness should be reset in > > > > ColorProcessing::configure(). > > > > > > Yes > > > > > > > > + LOG(RkISP1CProc, Debug) << "Set brightness to " << *brightness; > > > > > + update = true; > > > > > > > > Why do you do this unconditionally now ? > > > > > > It's the same as before. Depends if we want to log the control setter, > > > or the actual value applied. I thought the intention was to log the > > > setter as the log wasn't inside the "if (cproc.brightness != value) {". > > > > I meant setting update to true, not logging. > > > > > > > } > > > > > > > > > > const auto &contrast = controls.get(controls::Contrast); > > > > > @@ -120,6 +116,24 @@ void ColorProcessing::queueRequest(IPAContext &context, > > > > > LOG(RkISP1CProc, Debug) << "Set contrast to " << value; > > > > > } > > > > > > > > > > + /* > > > > > + * The CPROC module applies the transfer function > > > > > + * > > > > > + * Yout = Yin * contrast + brightness/2 > > > > By the way, please record the fact that this is due to the CPROC using > > 9-bit data internally after multiplying by the contrast (which is > > limited to x2) to avoid intermediate clamping, and the brightness > > register therefore being expressed as 9 bits. > > That is the assumption. CPROC also allows 10bit input data so I'm > hesitant talking about 9 bits. What about: Indeed, according to the i.MX8MP reference manual its input is 10 bits (assuming the diagram is correct). That's weird though, because you've tested that pixel values are increased by exactly brightness / 2 if I recall correctly. > The calculations in CPROC are done one bit wider than the input data to > account for the maximum contrast of 2x without clamping. Brightness is > applied after and therefor with only half the numeric value. s/therefor/therefore/ Not sure if it's totally accurate (as we're guessing some things, but I think this is good enough for now. > > > > > + * > > > > > + * 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 > > What do you mean here? Why would you prefer this notation? Oops no that's a leftover of notes I was taking :-) Please ignore this. > > BR = (1 - contrast) + brightness * 2 > > > > > > > + * > > > > > + * To implement that with the given hardware we need to correct the > > > > > + * brightness value. > > > > > + */ > > > > > + if (update) { > > > > > + cproc.brightness = (1 - cproc.contrast.value()) + 2 * cproc.requestedBrightness; > > > > > > > > I don't think the x2 factor is correct if you don't change the limits. > > > > > > Why not? You can't realize every requested value as it is dependent on > > > the contrast value. But this way we can reach the whole available space. > > > > > > The first "(1 - cproc.contrast.value())" has a output range of [-1, 1] > > > so to be able to set the hardware brightness value in the range of [-1, > > > 1] we need to keep the limits as is. > > > > You're right. > > > > > Ideally the limits should change based on the contrast. But we don't > > > have that. > > > > > > > > + } > > > > > > > > No need for curly braces. > > > > > > Ooops again. > > > > > > > > + > > > > > const auto &hue = controls.get(controls::Hue); > > > > > if (hue) { > > > > > /* Scale the Hue from ]-90, +90] */ > > > > > @@ -179,7 +193,15 @@ void ColorProcessing::process([[maybe_unused]] IPAContext &context, > > > > > [[maybe_unused]] const rkisp1_stat_buffer *stats, > > > > > ControlList &metadata) > > > > > { > > > > > - metadata.set(controls::Brightness, frameContext.cproc.brightness.value()); > > > > > + /* > > > > > + * Report the actually applied brightness value. See queueRequest() for > > > > > + * more details. > > > > > + */ > > > > > + float actualBrightness = (frameContext.cproc.brightness.value() - > > > > > + (1 - frameContext.cproc.contrast.value())) / > > > > > + 2; > > > > > > > > Can we compute this when the controls change and cache the value instead > > > > of recomputing it for every frame ? > > > > > > Hm, I wouldn't be worried about performance, but it could actually > > > improve the readability of the code. I'll give it a try. > > > > I think it would be more readable, yes. > > > > > > > + > > > > > + metadata.set(controls::Brightness, 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.h b/src/ipa/rkisp1/ipa_context.h > > > > > index 80b035044cda..309567858b64 100644 > > > > > --- a/src/ipa/rkisp1/ipa_context.h > > > > > +++ b/src/ipa/rkisp1/ipa_context.h > > > > > @@ -122,6 +122,7 @@ struct IPAActiveState { > > > > > } ccm; > > > > > > > > > > struct { > > > > > + float requestedBrightness; > > > > > BrightnessQ brightness; > > > > > ContrastQ contrast; > > > > > HueQ hue;
Hi Laurent, Quoting Laurent Pinchart (2026-01-21 17:29:31) > On Wed, Jan 21, 2026 at 01:14:28PM +0100, Stefan Klug wrote: > > Quoting Laurent Pinchart (2026-01-21 12:36:19) > > > On Wed, Jan 21, 2026 at 06:54:37AM +0100, Stefan Klug wrote: > > > > Quoting Laurent Pinchart (2026-01-21 01:15:12) > > > > > On Fri, Jan 16, 2026 at 01:16:14PM +0100, Stefan Klug wrote: > > > > > > 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 > > > > > > --- > > > > > > src/ipa/rkisp1/algorithms/cproc.cpp | 38 +++++++++++++++++++++++------ > > > > > > src/ipa/rkisp1/ipa_context.h | 1 + > > > > > > 2 files changed, 31 insertions(+), 8 deletions(-) > > > > > > > > > > > > diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp > > > > > > index 7484e4780094..ec2b264a79b9 100644 > > > > > > --- a/src/ipa/rkisp1/algorithms/cproc.cpp > > > > > > +++ b/src/ipa/rkisp1/algorithms/cproc.cpp > > > > > > @@ -100,13 +100,9 @@ void ColorProcessing::queueRequest(IPAContext &context, > > > > > > > > > > > > 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; > > > > > > + cproc.requestedBrightness = *brightness; > > > > > > > > > > cproc.requestedBrightness should be reset in > > > > > ColorProcessing::configure(). > > > > > > > > Yes > > > > > > > > > > + LOG(RkISP1CProc, Debug) << "Set brightness to " << *brightness; > > > > > > + update = true; > > > > > > > > > > Why do you do this unconditionally now ? > > > > > > > > It's the same as before. Depends if we want to log the control setter, > > > > or the actual value applied. I thought the intention was to log the > > > > setter as the log wasn't inside the "if (cproc.brightness != value) {". > > > > > > I meant setting update to true, not logging. > > > > > > > > > } > > > > > > > > > > > > const auto &contrast = controls.get(controls::Contrast); > > > > > > @@ -120,6 +116,24 @@ void ColorProcessing::queueRequest(IPAContext &context, > > > > > > LOG(RkISP1CProc, Debug) << "Set contrast to " << value; > > > > > > } > > > > > > > > > > > > + /* > > > > > > + * The CPROC module applies the transfer function > > > > > > + * > > > > > > + * Yout = Yin * contrast + brightness/2 > > > > > > By the way, please record the fact that this is due to the CPROC using > > > 9-bit data internally after multiplying by the contrast (which is > > > limited to x2) to avoid intermediate clamping, and the brightness > > > register therefore being expressed as 9 bits. > > > > That is the assumption. CPROC also allows 10bit input data so I'm > > hesitant talking about 9 bits. What about: > > Indeed, according to the i.MX8MP reference manual its input is 10 bits > (assuming the diagram is correct). That's weird though, because you've > tested that pixel values are increased by exactly brightness / 2 if I > recall correctly. > > > The calculations in CPROC are done one bit wider than the input data to > > account for the maximum contrast of 2x without clamping. Brightness is > > applied after and therefor with only half the numeric value. > > s/therefor/therefore/ > > Not sure if it's totally accurate (as we're guessing some things, but I > think this is good enough for now. I did a bit more guessing here as I realized that above explanation only made partial sense. What about the following guess: Input to the CPROC module is always 10bit data (this is known). The hardware shifts right by 1 bit to create headroom for the contrast multiplication by a maximum of 2x. Then it applies the contrast multiplication and the brightness addition in 10bit space. Then it shifts right by another bit to get the 8 bit output data. In my head that kind of makes sense :-) Best regards, Stefan > > > > > > > + * > > > > > > + * 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 > > > > What do you mean here? Why would you prefer this notation? > > Oops no that's a leftover of notes I was taking :-) Please ignore this. > > > > BR = (1 - contrast) + brightness * 2 > > > > > > > > > + * > > > > > > + * To implement that with the given hardware we need to correct the > > > > > > + * brightness value. > > > > > > + */ > > > > > > + if (update) { > > > > > > + cproc.brightness = (1 - cproc.contrast.value()) + 2 * cproc.requestedBrightness; > > > > > > > > > > I don't think the x2 factor is correct if you don't change the limits. > > > > > > > > Why not? You can't realize every requested value as it is dependent on > > > > the contrast value. But this way we can reach the whole available space. > > > > > > > > The first "(1 - cproc.contrast.value())" has a output range of [-1, 1] > > > > so to be able to set the hardware brightness value in the range of [-1, > > > > 1] we need to keep the limits as is. > > > > > > You're right. > > > > > > > Ideally the limits should change based on the contrast. But we don't > > > > have that. > > > > > > > > > > + } > > > > > > > > > > No need for curly braces. > > > > > > > > Ooops again. > > > > > > > > > > + > > > > > > const auto &hue = controls.get(controls::Hue); > > > > > > if (hue) { > > > > > > /* Scale the Hue from ]-90, +90] */ > > > > > > @@ -179,7 +193,15 @@ void ColorProcessing::process([[maybe_unused]] IPAContext &context, > > > > > > [[maybe_unused]] const rkisp1_stat_buffer *stats, > > > > > > ControlList &metadata) > > > > > > { > > > > > > - metadata.set(controls::Brightness, frameContext.cproc.brightness.value()); > > > > > > + /* > > > > > > + * Report the actually applied brightness value. See queueRequest() for > > > > > > + * more details. > > > > > > + */ > > > > > > + float actualBrightness = (frameContext.cproc.brightness.value() - > > > > > > + (1 - frameContext.cproc.contrast.value())) / > > > > > > + 2; > > > > > > > > > > Can we compute this when the controls change and cache the value instead > > > > > of recomputing it for every frame ? > > > > > > > > Hm, I wouldn't be worried about performance, but it could actually > > > > improve the readability of the code. I'll give it a try. > > > > > > I think it would be more readable, yes. > > > > > > > > > + > > > > > > + metadata.set(controls::Brightness, 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.h b/src/ipa/rkisp1/ipa_context.h > > > > > > index 80b035044cda..309567858b64 100644 > > > > > > --- a/src/ipa/rkisp1/ipa_context.h > > > > > > +++ b/src/ipa/rkisp1/ipa_context.h > > > > > > @@ -122,6 +122,7 @@ struct IPAActiveState { > > > > > > } ccm; > > > > > > > > > > > > struct { > > > > > > + float requestedBrightness; > > > > > > BrightnessQ brightness; > > > > > > ContrastQ contrast; > > > > > > HueQ hue; > > -- > Regards, > > Laurent Pinchart
diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp index 7484e4780094..ec2b264a79b9 100644 --- a/src/ipa/rkisp1/algorithms/cproc.cpp +++ b/src/ipa/rkisp1/algorithms/cproc.cpp @@ -100,13 +100,9 @@ void ColorProcessing::queueRequest(IPAContext &context, 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; + cproc.requestedBrightness = *brightness; + LOG(RkISP1CProc, Debug) << "Set brightness to " << *brightness; + update = true; } const auto &contrast = controls.get(controls::Contrast); @@ -120,6 +116,24 @@ void ColorProcessing::queueRequest(IPAContext &context, LOG(RkISP1CProc, Debug) << "Set contrast to " << value; } + /* + * The CPROC module applies the transfer function + * + * Yout = Yin * contrast + brightness/2 + * + * 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 that with the given hardware we need to correct the + * brightness value. + */ + if (update) { + cproc.brightness = (1 - cproc.contrast.value()) + 2 * cproc.requestedBrightness; + } + const auto &hue = controls.get(controls::Hue); if (hue) { /* Scale the Hue from ]-90, +90] */ @@ -179,7 +193,15 @@ void ColorProcessing::process([[maybe_unused]] IPAContext &context, [[maybe_unused]] const rkisp1_stat_buffer *stats, ControlList &metadata) { - metadata.set(controls::Brightness, frameContext.cproc.brightness.value()); + /* + * Report the actually applied brightness value. See queueRequest() for + * more details. + */ + float actualBrightness = (frameContext.cproc.brightness.value() - + (1 - frameContext.cproc.contrast.value())) / + 2; + + metadata.set(controls::Brightness, 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.h b/src/ipa/rkisp1/ipa_context.h index 80b035044cda..309567858b64 100644 --- a/src/ipa/rkisp1/ipa_context.h +++ b/src/ipa/rkisp1/ipa_context.h @@ -122,6 +122,7 @@ struct IPAActiveState { } ccm; struct { + float requestedBrightness; BrightnessQ brightness; ContrastQ contrast; HueQ hue;
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 --- src/ipa/rkisp1/algorithms/cproc.cpp | 38 +++++++++++++++++++++++------ src/ipa/rkisp1/ipa_context.h | 1 + 2 files changed, 31 insertions(+), 8 deletions(-)