[{"id":37779,"web_url":"https://patchwork.libcamera.org/comment/37779/","msgid":"<20260121001512.GA382676@killaraus>","date":"2026-01-21T00:15:12","subject":"Re: [PATCH v1] ipa: rkisp1: cproc: Fix brightness and contrast value\n\thandling","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Stefan,\n\nThank you for the patch.\n\nOn Fri, Jan 16, 2026 at 01:16:14PM +0100, Stefan Klug wrote:\n> By reverse engineering it was discovered that the transfer curve applied\n> by the CPROC module is\n> \n> Yout = Yin * contrast + brightness/2.\n> \n> However the most common expectation on contrast is to not change the\n> middle gray of an image. So the expected curve is:\n> \n> Yout = (Yin - 0.5) * contrast + 0.5 + brightness\n> \n> Map the user expectation to the hardware by changing the brightness\n> value written to the hardware. This also includes multiplying the\n> requested brightness value by two.\n> \n> Note: The limits of the brightness control are left as is even though\n> they can not be reached without changing the contrast. But limiting them\n> to [0.5, 0.5] would cut off parts of the combined control space of\n> contrast + brightness.\n> \n> Signed-off-by: Stefan Klug \n> \n> ---\n> \n> This patch needs to be applied on top of the quantization series:\n> https://patchwork.libcamera.org/project/libcamera/list/?series=5708\n> ---\n> src/ipa/rkisp1/algorithms/cproc.cpp | 38 +++++++++++++++++++++++------\n> src/ipa/rkisp1/ipa_context.h | 1 +\n> 2 files changed, 31 insertions(+), 8 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp\n> index 7484e4780094..ec2b264a79b9 100644\n> --- a/src/ipa/rkisp1/algorithms/cproc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/cproc.cpp\n> @@ -100,13 +100,9 @@ void ColorProcessing::queueRequest(IPAContext &context,\n> \n> \tconst auto &brightness = controls.get(controls::Brightness);\n> \tif (brightness) {\n> -\t\tBrightnessQ value = *brightness;\n> -\t\tif (cproc.brightness != value) {\n> -\t\t\tcproc.brightness = value;\n> -\t\t\tupdate = true;\n> -\t\t}\n> -\n> -\t\tLOG(RkISP1CProc, Debug) << \"Set brightness to \" << value;\n> +\t\tcproc.requestedBrightness = *brightness;\n\ncproc.requestedBrightness should be reset in\nColorProcessing::configure().\n\n> +\t\tLOG(RkISP1CProc, Debug) << \"Set brightness to \" << *brightness;\n> +\t\tupdate = true;\n\nWhy do you do this unconditionally now ?\n\n> \t}\n> \n> \tconst auto &contrast = controls.get(controls::Contrast);\n> @@ -120,6 +116,24 @@ void ColorProcessing::queueRequest(IPAContext &context,\n> \t\tLOG(RkISP1CProc, Debug) << \"Set contrast to \" << value;\n> \t}\n> \n> +\t/*\n> +\t * The CPROC module applies the transfer function\n> +\t *\n> +\t * Yout = Yin * contrast + brightness/2\n> +\t *\n> +\t * Most users expect that changing contrast doesn't change the middle\n> +\t * gray and that the brightness value is normalized to one. So they\n> +\t * expect a transfer function of\n> +\t *\n> +\t * Yout = (Yin - 0.5) * contrast + 0.5 + brightness\n> +\t *\n> +\t * To implement that with the given hardware we need to correct the\n> +\t * brightness value.\n> +\t */\n> +\tif (update) {\n> +\t\tcproc.brightness = (1 - cproc.contrast.value()) + 2 * cproc.requestedBrightness;\n\nI don't think the x2 factor is correct if you don't change the limits.\n\n> +\t}\n\nNo need for curly braces.\n\n> +\n> \tconst auto &hue = controls.get(controls::Hue);\n> \tif (hue) {\n> \t\t/* Scale the Hue from ]-90, +90] */\n> @@ -179,7 +193,15 @@ void ColorProcessing::process([[maybe_unused]] IPAContext &context,\n> \t\t\t [[maybe_unused]] const rkisp1_stat_buffer *stats,\n> \t\t\t ControlList &metadata)\n> {\n> -\tmetadata.set(controls::Brightness, frameContext.cproc.brightness.value());\n> +\t/*\n> +\t * Report the actually applied brightness value. See queueRequest() for\n> +\t * more details.\n> +\t */\n> +\tfloat actualBrightness = (frameContext.cproc.brightness.value() -\n> +\t\t\t\t (1 - frameContext.cproc.contrast.value())) /\n> +\t\t\t\t 2;\n\nCan we compute this when the controls change and cache the value instead\nof recomputing it for every frame ?\n\n> +\n> +\tmetadata.set(controls::Brightness, actualBrightness);\n> \tmetadata.set(controls::Contrast, frameContext.cproc.contrast.value());\n> \tmetadata.set(controls::Hue, frameContext.cproc.hue.value() * kHueScale);\n> \tmetadata.set(controls::Saturation, frameContext.cproc.saturation.value());\n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index 80b035044cda..309567858b64 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -122,6 +122,7 @@ struct IPAActiveState {\n> \t} ccm;\n> \n> \tstruct {\n> +\t\tfloat requestedBrightness;\n> \t\tBrightnessQ brightness;\n> \t\tContrastQ contrast;\n> \t\tHueQ hue;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 754FFC3220\n\tfor ;\n\tWed, 21 Jan 2026 00:15:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 83DB861FC4;\n\tWed, 21 Jan 2026 01:15:15 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3BB37606D5\n\tfor ;\n\tWed, 21 Jan 2026 01:15:14 +0100 (CET)","from pendragon.ideasonboard.com\n\t(2001-14ba-703d-e500--2a1.rev.dnainternet.fi\n\t[IPv6:2001:14ba:703d:e500::2a1])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 924952DD;\n\tWed, 21 Jan 2026 01:14:42 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"iERHfR4U\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1768954482;\n\tbh=nHXy2MJ+NEksUnCyh5UKhjzrtRJXuQJl/Q5j5PbLSmc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=iERHfR4Ut0f+mILSMFPTE8G099VpgYDCaSLNCAydWaav3oRREhWKjlf+0yaQZ344u\n\toqkQkcjRl0KbIXwG9an5GHQt9+zFEzMBq3SKk2lkmT3VfKe37IIrEi4mRBvVmaBcdd\n\t7tfp7w4YMMklsBUc5AtZZDtMV1MRSyCgKvGkZRXM=","Date":"Wed, 21 Jan 2026 02:15:12 +0200","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1] ipa: rkisp1: cproc: Fix brightness and contrast value\n\thandling","Message-ID":"<20260121001512.GA382676@killaraus>","References":"<20260116121658.215712-1-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20260116121658.215712-1-stefan.klug@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":37780,"web_url":"https://patchwork.libcamera.org/comment/37780/","msgid":"<176897487745.508107.12355351030826782862@localhost>","date":"2026-01-21T05:54:37","subject":"Re: [PATCH v1] ipa: rkisp1: cproc: Fix brightness and contrast value\n\thandling","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Laurent,\n\nThank you for the review. \n\nQuoting Laurent Pinchart (2026-01-21 01:15:12)\n> Hi Stefan,\n> \n> Thank you for the patch.\n> \n> On Fri, Jan 16, 2026 at 01:16:14PM +0100, Stefan Klug wrote:\n> > By reverse engineering it was discovered that the transfer curve applied\n> > by the CPROC module is\n> > \n> > Yout = Yin * contrast + brightness/2.\n> > \n> > However the most common expectation on contrast is to not change the\n> > middle gray of an image. So the expected curve is:\n> > \n> > Yout = (Yin - 0.5) * contrast + 0.5 + brightness\n> > \n> > Map the user expectation to the hardware by changing the brightness\n> > value written to the hardware. This also includes multiplying the\n> > requested brightness value by two.\n> > \n> > Note: The limits of the brightness control are left as is even though\n> > they can not be reached without changing the contrast. But limiting them\n> > to [0.5, 0.5] would cut off parts of the combined control space of\n> > contrast + brightness.\n> > \n> > Signed-off-by: Stefan Klug \n> > \n> > ---\n> > \n> > This patch needs to be applied on top of the quantization series:\n> > https://patchwork.libcamera.org/project/libcamera/list/?series=5708\n> > ---\n> > src/ipa/rkisp1/algorithms/cproc.cpp | 38 +++++++++++++++++++++++------\n> > src/ipa/rkisp1/ipa_context.h | 1 +\n> > 2 files changed, 31 insertions(+), 8 deletions(-)\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp\n> > index 7484e4780094..ec2b264a79b9 100644\n> > --- a/src/ipa/rkisp1/algorithms/cproc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/cproc.cpp\n> > @@ -100,13 +100,9 @@ void ColorProcessing::queueRequest(IPAContext &context,\n> > \n> > const auto &brightness = controls.get(controls::Brightness);\n> > if (brightness) {\n> > - BrightnessQ value = *brightness;\n> > - if (cproc.brightness != value) {\n> > - cproc.brightness = value;\n> > - update = true;\n> > - }\n> > -\n> > - LOG(RkISP1CProc, Debug) << \"Set brightness to \" << value;\n> > + cproc.requestedBrightness = *brightness;\n> \n> cproc.requestedBrightness should be reset in\n> ColorProcessing::configure().\n\nYes\n\n> \n> > + LOG(RkISP1CProc, Debug) << \"Set brightness to \" << *brightness;\n> > + update = true;\n> \n> Why do you do this unconditionally now ?\n\nIt's the same as before. Depends if we want to log the control setter,\nor the actual value applied. I thought the intention was to log the\nsetter as the log wasn't inside the \"if (cproc.brightness != value) {\".\n\n> \n> > }\n> > \n> > const auto &contrast = controls.get(controls::Contrast);\n> > @@ -120,6 +116,24 @@ void ColorProcessing::queueRequest(IPAContext &context,\n> > LOG(RkISP1CProc, Debug) << \"Set contrast to \" << value;\n> > }\n> > \n> > + /*\n> > + * The CPROC module applies the transfer function\n> > + *\n> > + * Yout = Yin * contrast + brightness/2\n> > + *\n> > + * Most users expect that changing contrast doesn't change the middle\n> > + * gray and that the brightness value is normalized to one. So they\n> > + * expect a transfer function of\n> > + *\n> > + * Yout = (Yin - 0.5) * contrast + 0.5 + brightness\n> > + *\n> > + * To implement that with the given hardware we need to correct the\n> > + * brightness value.\n> > + */\n> > + if (update) {\n> > + cproc.brightness = (1 - cproc.contrast.value()) + 2 * cproc.requestedBrightness;\n> \n> I don't think the x2 factor is correct if you don't change the limits.\n\nWhy not? You can't realize every requested value as it is dependent on\nthe contrast value. But this way we can reach the whole available space.\n\nThe first \"(1 - cproc.contrast.value())\" has a output range of [-1, 1]\nso to be able to set the hardware brightness value in the range of [-1,\n1] we need to keep the limits as is.\n\nIdeally the limits should change based on the contrast. But we don't\nhave that.\n\n> \n> > + }\n> \n> No need for curly braces.\n\nOoops again.\n\n> \n> > +\n> > const auto &hue = controls.get(controls::Hue);\n> > if (hue) {\n> > /* Scale the Hue from ]-90, +90] */\n> > @@ -179,7 +193,15 @@ void ColorProcessing::process([[maybe_unused]] IPAContext &context,\n> > [[maybe_unused]] const rkisp1_stat_buffer *stats,\n> > ControlList &metadata)\n> > {\n> > - metadata.set(controls::Brightness, frameContext.cproc.brightness.value());\n> > + /*\n> > + * Report the actually applied brightness value. See queueRequest() for\n> > + * more details.\n> > + */\n> > + float actualBrightness = (frameContext.cproc.brightness.value() -\n> > + (1 - frameContext.cproc.contrast.value())) /\n> > + 2;\n> \n> Can we compute this when the controls change and cache the value instead\n> of recomputing it for every frame ?\n\nHm, I wouldn't be worried about performance, but it could actually\nimprove the readability of the code. I'll give it a try.\n\nRegards,\nStefan\n\n> \n> > +\n> > + metadata.set(controls::Brightness, actualBrightness);\n> > metadata.set(controls::Contrast, frameContext.cproc.contrast.value());\n> > metadata.set(controls::Hue, frameContext.cproc.hue.value() * kHueScale);\n> > metadata.set(controls::Saturation, frameContext.cproc.saturation.value());\n> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > index 80b035044cda..309567858b64 100644\n> > --- a/src/ipa/rkisp1/ipa_context.h\n> > +++ b/src/ipa/rkisp1/ipa_context.h\n> > @@ -122,6 +122,7 @@ struct IPAActiveState {\n> > } ccm;\n> > \n> > struct {\n> > + float requestedBrightness;\n> > BrightnessQ brightness;\n> > ContrastQ contrast;\n> > HueQ hue;\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E707EBDCBF\n\tfor ;\n\tWed, 21 Jan 2026 05:54:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1FDC861FC4;\n\tWed, 21 Jan 2026 06:54:41 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 536DA606D5\n\tfor ;\n\tWed, 21 Jan 2026 06:54:40 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:d6b4:14c6:5912:f196])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 6FA75593;\n\tWed, 21 Jan 2026 06:54:08 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"SZK6opmK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1768974848;\n\tbh=1IMxrMMelIeLhW8VNzlqOgkD2qAlxnaP+tI6sGoWcbc=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=SZK6opmKPjddNCcOzu2boiIHgUmMnl8UQQMkZcgJIieweOwCMwoSXKtYANnXBh6Mq\n\tl76FFufHCUwxAu0u3cYel1AGYD+kqQ/k26BjZbY+79Sh7Lsb+BtY1T5mPp9slLEI/N\n\tYYcFVBJfodIWn+PbNt0f6rKo1UFbDENWtBodQydc=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20260121001512.GA382676@killaraus>","References":"<20260116121658.215712-1-stefan.klug@ideasonboard.com>\n\t<20260121001512.GA382676@killaraus>","Subject":"Re: [PATCH v1] ipa: rkisp1: cproc: Fix brightness and contrast value\n\thandling","From":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","To":"Laurent Pinchart ","Date":"Wed, 21 Jan 2026 06:54:37 +0100","Message-ID":"<176897487745.508107.12355351030826782862@localhost>","User-Agent":"alot/0.12.dev8+g2c003385c862.d20250602","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":37786,"web_url":"https://patchwork.libcamera.org/comment/37786/","msgid":"<20260121113619.GE382676@killaraus>","date":"2026-01-21T11:36:19","subject":"Re: [PATCH v1] ipa: rkisp1: cproc: Fix brightness and contrast value\n\thandling","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Jan 21, 2026 at 06:54:37AM +0100, Stefan Klug wrote:\n> Quoting Laurent Pinchart (2026-01-21 01:15:12)\n> > On Fri, Jan 16, 2026 at 01:16:14PM +0100, Stefan Klug wrote:\n> > > By reverse engineering it was discovered that the transfer curve applied\n> > > by the CPROC module is\n> > > \n> > > Yout = Yin * contrast + brightness/2.\n> > > \n> > > However the most common expectation on contrast is to not change the\n> > > middle gray of an image. So the expected curve is:\n> > > \n> > > Yout = (Yin - 0.5) * contrast + 0.5 + brightness\n> > > \n> > > Map the user expectation to the hardware by changing the brightness\n> > > value written to the hardware. This also includes multiplying the\n> > > requested brightness value by two.\n> > > \n> > > Note: The limits of the brightness control are left as is even though\n> > > they can not be reached without changing the contrast. But limiting them\n> > > to [0.5, 0.5] would cut off parts of the combined control space of\n> > > contrast + brightness.\n> > > \n> > > Signed-off-by: Stefan Klug \n> > > \n> > > ---\n> > > \n> > > This patch needs to be applied on top of the quantization series:\n> > > https://patchwork.libcamera.org/project/libcamera/list/?series=5708\n> > > ---\n> > > src/ipa/rkisp1/algorithms/cproc.cpp | 38 +++++++++++++++++++++++------\n> > > src/ipa/rkisp1/ipa_context.h | 1 +\n> > > 2 files changed, 31 insertions(+), 8 deletions(-)\n> > > \n> > > diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp\n> > > index 7484e4780094..ec2b264a79b9 100644\n> > > --- a/src/ipa/rkisp1/algorithms/cproc.cpp\n> > > +++ b/src/ipa/rkisp1/algorithms/cproc.cpp\n> > > @@ -100,13 +100,9 @@ void ColorProcessing::queueRequest(IPAContext &context,\n> > > \n> > > const auto &brightness = controls.get(controls::Brightness);\n> > > if (brightness) {\n> > > - BrightnessQ value = *brightness;\n> > > - if (cproc.brightness != value) {\n> > > - cproc.brightness = value;\n> > > - update = true;\n> > > - }\n> > > -\n> > > - LOG(RkISP1CProc, Debug) << \"Set brightness to \" << value;\n> > > + cproc.requestedBrightness = *brightness;\n> > \n> > cproc.requestedBrightness should be reset in\n> > ColorProcessing::configure().\n> \n> Yes\n> \n> > > + LOG(RkISP1CProc, Debug) << \"Set brightness to \" << *brightness;\n> > > + update = true;\n> > \n> > Why do you do this unconditionally now ?\n> \n> It's the same as before. Depends if we want to log the control setter,\n> or the actual value applied. I thought the intention was to log the\n> setter as the log wasn't inside the \"if (cproc.brightness != value) {\".\n\nI meant setting update to true, not logging.\n\n> > > }\n> > > \n> > > const auto &contrast = controls.get(controls::Contrast);\n> > > @@ -120,6 +116,24 @@ void ColorProcessing::queueRequest(IPAContext &context,\n> > > LOG(RkISP1CProc, Debug) << \"Set contrast to \" << value;\n> > > }\n> > > \n> > > + /*\n> > > + * The CPROC module applies the transfer function\n> > > + *\n> > > + * Yout = Yin * contrast + brightness/2\n\nBy the way, please record the fact that this is due to the CPROC using\n9-bit data internally after multiplying by the contrast (which is\nlimited to x2) to avoid intermediate clamping, and the brightness\nregister therefore being expressed as 9 bits.\n\n> > > + *\n> > > + * Most users expect that changing contrast doesn't change the middle\n> > > + * gray and that the brightness value is normalized to one. So they\n> > > + * expect a transfer function of\n> > > + *\n> > > + * Yout = (Yin - 0.5) * contrast + 0.5 + brightness\n\n\nYout = Yin * contrast - 0.5 * contrast + 0.5 + brightness\n\nBR = (1 - contrast) + brightness * 2\n\n> > > + *\n> > > + * To implement that with the given hardware we need to correct the\n> > > + * brightness value.\n> > > + */\n> > > + if (update) {\n> > > + cproc.brightness = (1 - cproc.contrast.value()) + 2 * cproc.requestedBrightness;\n> > \n> > I don't think the x2 factor is correct if you don't change the limits.\n> \n> Why not? You can't realize every requested value as it is dependent on\n> the contrast value. But this way we can reach the whole available space.\n> \n> The first \"(1 - cproc.contrast.value())\" has a output range of [-1, 1]\n> so to be able to set the hardware brightness value in the range of [-1,\n> 1] we need to keep the limits as is.\n\nYou're right.\n\n> Ideally the limits should change based on the contrast. But we don't\n> have that.\n> \n> > \n> > > + }\n> > \n> > No need for curly braces.\n> \n> Ooops again.\n> \n> > > +\n> > > const auto &hue = controls.get(controls::Hue);\n> > > if (hue) {\n> > > /* Scale the Hue from ]-90, +90] */\n> > > @@ -179,7 +193,15 @@ void ColorProcessing::process([[maybe_unused]] IPAContext &context,\n> > > [[maybe_unused]] const rkisp1_stat_buffer *stats,\n> > > ControlList &metadata)\n> > > {\n> > > - metadata.set(controls::Brightness, frameContext.cproc.brightness.value());\n> > > + /*\n> > > + * Report the actually applied brightness value. See queueRequest() for\n> > > + * more details.\n> > > + */\n> > > + float actualBrightness = (frameContext.cproc.brightness.value() -\n> > > + (1 - frameContext.cproc.contrast.value())) /\n> > > + 2;\n> > \n> > Can we compute this when the controls change and cache the value instead\n> > of recomputing it for every frame ?\n> \n> Hm, I wouldn't be worried about performance, but it could actually\n> improve the readability of the code. I'll give it a try.\n\nI think it would be more readable, yes.\n\n> > > +\n> > > + metadata.set(controls::Brightness, actualBrightness);\n> > > metadata.set(controls::Contrast, frameContext.cproc.contrast.value());\n> > > metadata.set(controls::Hue, frameContext.cproc.hue.value() * kHueScale);\n> > > metadata.set(controls::Saturation, frameContext.cproc.saturation.value());\n> > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > index 80b035044cda..309567858b64 100644\n> > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > @@ -122,6 +122,7 @@ struct IPAActiveState {\n> > > } ccm;\n> > > \n> > > struct {\n> > > + float requestedBrightness;\n> > > BrightnessQ brightness;\n> > > ContrastQ contrast;\n> > > HueQ hue;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3DCB9BDCBF\n\tfor ;\n\tWed, 21 Jan 2026 11:36:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3403261FC9;\n\tWed, 21 Jan 2026 12:36:23 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 988FB61F9F\n\tfor ;\n\tWed, 21 Jan 2026 12:36:21 +0100 (CET)","from pendragon.ideasonboard.com\n\t(2001-14ba-703d-e500--2a1.rev.dnainternet.fi\n\t[IPv6:2001:14ba:703d:e500::2a1])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 9A23B593;\n\tWed, 21 Jan 2026 12:35:49 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"GCTdDwBI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1768995349;\n\tbh=X/zQAUag3vMg78e0IX9HBc2QARHNmnPwAS3ss9RE2jU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GCTdDwBI5cOeBkDjVd25cn99vf+GZsHzX6la9Ws3jlvOUjzNTjIc26naOhcYiZR17\n\tyjMNOtXyp3N1p6XmltitoFya34BBfKjFmo/IXZsq6B3ssntOPNbxrVgWPWDTea24UE\n\tIuDyaY76TGZ0jpIGdKlHnFYuZM3TH+klCDSoHr8I=","Date":"Wed, 21 Jan 2026 13:36:19 +0200","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1] ipa: rkisp1: cproc: Fix brightness and contrast value\n\thandling","Message-ID":"<20260121113619.GE382676@killaraus>","References":"<20260116121658.215712-1-stefan.klug@ideasonboard.com>\n\t<20260121001512.GA382676@killaraus>\n\t<176897487745.508107.12355351030826782862@localhost>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<176897487745.508107.12355351030826782862@localhost>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":37787,"web_url":"https://patchwork.libcamera.org/comment/37787/","msgid":"<176899766848.23550.2759588137284894601@localhost>","date":"2026-01-21T12:14:28","subject":"Re: [PATCH v1] ipa: rkisp1: cproc: Fix brightness and contrast value\n\thandling","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2026-01-21 12:36:19)\n> On Wed, Jan 21, 2026 at 06:54:37AM +0100, Stefan Klug wrote:\n> > Quoting Laurent Pinchart (2026-01-21 01:15:12)\n> > > On Fri, Jan 16, 2026 at 01:16:14PM +0100, Stefan Klug wrote:\n> > > > By reverse engineering it was discovered that the transfer curve applied\n> > > > by the CPROC module is\n> > > > \n> > > > Yout = Yin * contrast + brightness/2.\n> > > > \n> > > > However the most common expectation on contrast is to not change the\n> > > > middle gray of an image. So the expected curve is:\n> > > > \n> > > > Yout = (Yin - 0.5) * contrast + 0.5 + brightness\n> > > > \n> > > > Map the user expectation to the hardware by changing the brightness\n> > > > value written to the hardware. This also includes multiplying the\n> > > > requested brightness value by two.\n> > > > \n> > > > Note: The limits of the brightness control are left as is even though\n> > > > they can not be reached without changing the contrast. But limiting them\n> > > > to [0.5, 0.5] would cut off parts of the combined control space of\n> > > > contrast + brightness.\n> > > > \n> > > > Signed-off-by: Stefan Klug \n> > > > \n> > > > ---\n> > > > \n> > > > This patch needs to be applied on top of the quantization series:\n> > > > https://patchwork.libcamera.org/project/libcamera/list/?series=5708\n> > > > ---\n> > > > src/ipa/rkisp1/algorithms/cproc.cpp | 38 +++++++++++++++++++++++------\n> > > > src/ipa/rkisp1/ipa_context.h | 1 +\n> > > > 2 files changed, 31 insertions(+), 8 deletions(-)\n> > > > \n> > > > diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp\n> > > > index 7484e4780094..ec2b264a79b9 100644\n> > > > --- a/src/ipa/rkisp1/algorithms/cproc.cpp\n> > > > +++ b/src/ipa/rkisp1/algorithms/cproc.cpp\n> > > > @@ -100,13 +100,9 @@ void ColorProcessing::queueRequest(IPAContext &context,\n> > > > \n> > > > const auto &brightness = controls.get(controls::Brightness);\n> > > > if (brightness) {\n> > > > - BrightnessQ value = *brightness;\n> > > > - if (cproc.brightness != value) {\n> > > > - cproc.brightness = value;\n> > > > - update = true;\n> > > > - }\n> > > > -\n> > > > - LOG(RkISP1CProc, Debug) << \"Set brightness to \" << value;\n> > > > + cproc.requestedBrightness = *brightness;\n> > > \n> > > cproc.requestedBrightness should be reset in\n> > > ColorProcessing::configure().\n> > \n> > Yes\n> > \n> > > > + LOG(RkISP1CProc, Debug) << \"Set brightness to \" << *brightness;\n> > > > + update = true;\n> > > \n> > > Why do you do this unconditionally now ?\n> > \n> > It's the same as before. Depends if we want to log the control setter,\n> > or the actual value applied. I thought the intention was to log the\n> > setter as the log wasn't inside the \"if (cproc.brightness != value) {\".\n> \n> I meant setting update to true, not logging.\n> \n> > > > }\n> > > > \n> > > > const auto &contrast = controls.get(controls::Contrast);\n> > > > @@ -120,6 +116,24 @@ void ColorProcessing::queueRequest(IPAContext &context,\n> > > > LOG(RkISP1CProc, Debug) << \"Set contrast to \" << value;\n> > > > }\n> > > > \n> > > > + /*\n> > > > + * The CPROC module applies the transfer function\n> > > > + *\n> > > > + * Yout = Yin * contrast + brightness/2\n> \n> By the way, please record the fact that this is due to the CPROC using\n> 9-bit data internally after multiplying by the contrast (which is\n> limited to x2) to avoid intermediate clamping, and the brightness\n> register therefore being expressed as 9 bits.\n\nThat is the assumption. CPROC also allows 10bit input data so I'm\nhesitant talking about 9 bits. What about:\n\nThe calculations in CPROC are done one bit wider than the input data to\naccount for the maximum contrast of 2x without clamping. Brightness is\napplied after and therefor with only half the numeric value.\n\n> \n> > > > + *\n> > > > + * Most users expect that changing contrast doesn't change the middle\n> > > > + * gray and that the brightness value is normalized to one. So they\n> > > > + * expect a transfer function of\n> > > > + *\n> > > > + * Yout = (Yin - 0.5) * contrast + 0.5 + brightness\n> \n> \n> Yout = Yin * contrast - 0.5 * contrast + 0.5 + brightness\n\nWhat do you mean here? Why would you prefer this notation?\n\nBest regards,\nStefan\n\n> \n> BR = (1 - contrast) + brightness * 2\n> \n> > > > + *\n> > > > + * To implement that with the given hardware we need to correct the\n> > > > + * brightness value.\n> > > > + */\n> > > > + if (update) {\n> > > > + cproc.brightness = (1 - cproc.contrast.value()) + 2 * cproc.requestedBrightness;\n> > > \n> > > I don't think the x2 factor is correct if you don't change the limits.\n> > \n> > Why not? You can't realize every requested value as it is dependent on\n> > the contrast value. But this way we can reach the whole available space.\n> > \n> > The first \"(1 - cproc.contrast.value())\" has a output range of [-1, 1]\n> > so to be able to set the hardware brightness value in the range of [-1,\n> > 1] we need to keep the limits as is.\n> \n> You're right.\n> \n> > Ideally the limits should change based on the contrast. But we don't\n> > have that.\n> > \n> > > \n> > > > + }\n> > > \n> > > No need for curly braces.\n> > \n> > Ooops again.\n> > \n> > > > +\n> > > > const auto &hue = controls.get(controls::Hue);\n> > > > if (hue) {\n> > > > /* Scale the Hue from ]-90, +90] */\n> > > > @@ -179,7 +193,15 @@ void ColorProcessing::process([[maybe_unused]] IPAContext &context,\n> > > > [[maybe_unused]] const rkisp1_stat_buffer *stats,\n> > > > ControlList &metadata)\n> > > > {\n> > > > - metadata.set(controls::Brightness, frameContext.cproc.brightness.value());\n> > > > + /*\n> > > > + * Report the actually applied brightness value. See queueRequest() for\n> > > > + * more details.\n> > > > + */\n> > > > + float actualBrightness = (frameContext.cproc.brightness.value() -\n> > > > + (1 - frameContext.cproc.contrast.value())) /\n> > > > + 2;\n> > > \n> > > Can we compute this when the controls change and cache the value instead\n> > > of recomputing it for every frame ?\n> > \n> > Hm, I wouldn't be worried about performance, but it could actually\n> > improve the readability of the code. I'll give it a try.\n> \n> I think it would be more readable, yes.\n> \n> > > > +\n> > > > + metadata.set(controls::Brightness, actualBrightness);\n> > > > metadata.set(controls::Contrast, frameContext.cproc.contrast.value());\n> > > > metadata.set(controls::Hue, frameContext.cproc.hue.value() * kHueScale);\n> > > > metadata.set(controls::Saturation, frameContext.cproc.saturation.value());\n> > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > > index 80b035044cda..309567858b64 100644\n> > > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > > @@ -122,6 +122,7 @@ struct IPAActiveState {\n> > > > } ccm;\n> > > > \n> > > > struct {\n> > > > + float requestedBrightness;\n> > > > BrightnessQ brightness;\n> > > > ContrastQ contrast;\n> > > > HueQ hue;\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6423DBDCBF\n\tfor ;\n\tWed, 21 Jan 2026 12:14:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A191561FC4;\n\tWed, 21 Jan 2026 13:14:32 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DFBF961F9F\n\tfor ;\n\tWed, 21 Jan 2026 13:14:30 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:8b90:6146:64f5:1ba3])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 05C8A741;\n\tWed, 21 Jan 2026 13:13:58 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"XTYveOrc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1768997639;\n\tbh=zIWMxqvfoodv7iBMljiZyJfYMX6ipckFVG1de3wxg4k=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=XTYveOrch5sKglievZ8JFjHxT12zutLKxFtdrgJLitwD5BfryUh0uaGzZW/GFnNQn\n\ttmDQrHQ0BervQDmhyVW6k2dpozIi0zVVIlibz3LP7jurCtQkcc0fPHXu4NKNrAi/e1\n\tFV28TPOx+XO7GLSdiBPRFKeFhs74tsdc+nbxWObw=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20260121113619.GE382676@killaraus>","References":"<20260116121658.215712-1-stefan.klug@ideasonboard.com>\n\t<20260121001512.GA382676@killaraus>\n\t<176897487745.508107.12355351030826782862@localhost>\n\t<20260121113619.GE382676@killaraus>","Subject":"Re: [PATCH v1] ipa: rkisp1: cproc: Fix brightness and contrast value\n\thandling","From":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","To":"Laurent Pinchart ","Date":"Wed, 21 Jan 2026 13:14:28 +0100","Message-ID":"<176899766848.23550.2759588137284894601@localhost>","User-Agent":"alot/0.12.dev8+g2c003385c862.d20250602","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":37812,"web_url":"https://patchwork.libcamera.org/comment/37812/","msgid":"<20260121162931.GF382676@killaraus>","date":"2026-01-21T16:29:31","subject":"Re: [PATCH v1] ipa: rkisp1: cproc: Fix brightness and contrast value\n\thandling","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Jan 21, 2026 at 01:14:28PM +0100, Stefan Klug wrote:\n> Quoting Laurent Pinchart (2026-01-21 12:36:19)\n> > On Wed, Jan 21, 2026 at 06:54:37AM +0100, Stefan Klug wrote:\n> > > Quoting Laurent Pinchart (2026-01-21 01:15:12)\n> > > > On Fri, Jan 16, 2026 at 01:16:14PM +0100, Stefan Klug wrote:\n> > > > > By reverse engineering it was discovered that the transfer curve applied\n> > > > > by the CPROC module is\n> > > > > \n> > > > > Yout = Yin * contrast + brightness/2.\n> > > > > \n> > > > > However the most common expectation on contrast is to not change the\n> > > > > middle gray of an image. So the expected curve is:\n> > > > > \n> > > > > Yout = (Yin - 0.5) * contrast + 0.5 + brightness\n> > > > > \n> > > > > Map the user expectation to the hardware by changing the brightness\n> > > > > value written to the hardware. This also includes multiplying the\n> > > > > requested brightness value by two.\n> > > > > \n> > > > > Note: The limits of the brightness control are left as is even though\n> > > > > they can not be reached without changing the contrast. But limiting them\n> > > > > to [0.5, 0.5] would cut off parts of the combined control space of\n> > > > > contrast + brightness.\n> > > > > \n> > > > > Signed-off-by: Stefan Klug \n> > > > > \n> > > > > ---\n> > > > > \n> > > > > This patch needs to be applied on top of the quantization series:\n> > > > > https://patchwork.libcamera.org/project/libcamera/list/?series=5708\n> > > > > ---\n> > > > > src/ipa/rkisp1/algorithms/cproc.cpp | 38 +++++++++++++++++++++++------\n> > > > > src/ipa/rkisp1/ipa_context.h | 1 +\n> > > > > 2 files changed, 31 insertions(+), 8 deletions(-)\n> > > > > \n> > > > > diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp\n> > > > > index 7484e4780094..ec2b264a79b9 100644\n> > > > > --- a/src/ipa/rkisp1/algorithms/cproc.cpp\n> > > > > +++ b/src/ipa/rkisp1/algorithms/cproc.cpp\n> > > > > @@ -100,13 +100,9 @@ void ColorProcessing::queueRequest(IPAContext &context,\n> > > > > \n> > > > > const auto &brightness = controls.get(controls::Brightness);\n> > > > > if (brightness) {\n> > > > > - BrightnessQ value = *brightness;\n> > > > > - if (cproc.brightness != value) {\n> > > > > - cproc.brightness = value;\n> > > > > - update = true;\n> > > > > - }\n> > > > > -\n> > > > > - LOG(RkISP1CProc, Debug) << \"Set brightness to \" << value;\n> > > > > + cproc.requestedBrightness = *brightness;\n> > > > \n> > > > cproc.requestedBrightness should be reset in\n> > > > ColorProcessing::configure().\n> > > \n> > > Yes\n> > > \n> > > > > + LOG(RkISP1CProc, Debug) << \"Set brightness to \" << *brightness;\n> > > > > + update = true;\n> > > > \n> > > > Why do you do this unconditionally now ?\n> > > \n> > > It's the same as before. Depends if we want to log the control setter,\n> > > or the actual value applied. I thought the intention was to log the\n> > > setter as the log wasn't inside the \"if (cproc.brightness != value) {\".\n> > \n> > I meant setting update to true, not logging.\n> > \n> > > > > }\n> > > > > \n> > > > > const auto &contrast = controls.get(controls::Contrast);\n> > > > > @@ -120,6 +116,24 @@ void ColorProcessing::queueRequest(IPAContext &context,\n> > > > > LOG(RkISP1CProc, Debug) << \"Set contrast to \" << value;\n> > > > > }\n> > > > > \n> > > > > + /*\n> > > > > + * The CPROC module applies the transfer function\n> > > > > + *\n> > > > > + * Yout = Yin * contrast + brightness/2\n> > \n> > By the way, please record the fact that this is due to the CPROC using\n> > 9-bit data internally after multiplying by the contrast (which is\n> > limited to x2) to avoid intermediate clamping, and the brightness\n> > register therefore being expressed as 9 bits.\n> \n> That is the assumption. CPROC also allows 10bit input data so I'm\n> hesitant talking about 9 bits. What about:\n\nIndeed, according to the i.MX8MP reference manual its input is 10 bits\n(assuming the diagram is correct). That's weird though, because you've\ntested that pixel values are increased by exactly brightness / 2 if I\nrecall correctly.\n\n> The calculations in CPROC are done one bit wider than the input data to\n> account for the maximum contrast of 2x without clamping. Brightness is\n> applied after and therefor with only half the numeric value.\n\ns/therefor/therefore/\n\nNot sure if it's totally accurate (as we're guessing some things, but I\nthink this is good enough for now.\n\n> > > > > + *\n> > > > > + * Most users expect that changing contrast doesn't change the middle\n> > > > > + * gray and that the brightness value is normalized to one. So they\n> > > > > + * expect a transfer function of\n> > > > > + *\n> > > > > + * Yout = (Yin - 0.5) * contrast + 0.5 + brightness\n> > \n> > Yout = Yin * contrast - 0.5 * contrast + 0.5 + brightness\n> \n> What do you mean here? Why would you prefer this notation?\n\nOops no that's a leftover of notes I was taking :-) Please ignore this.\n\n> > BR = (1 - contrast) + brightness * 2\n> > \n> > > > > + *\n> > > > > + * To implement that with the given hardware we need to correct the\n> > > > > + * brightness value.\n> > > > > + */\n> > > > > + if (update) {\n> > > > > + cproc.brightness = (1 - cproc.contrast.value()) + 2 * cproc.requestedBrightness;\n> > > > \n> > > > I don't think the x2 factor is correct if you don't change the limits.\n> > > \n> > > Why not? You can't realize every requested value as it is dependent on\n> > > the contrast value. But this way we can reach the whole available space.\n> > > \n> > > The first \"(1 - cproc.contrast.value())\" has a output range of [-1, 1]\n> > > so to be able to set the hardware brightness value in the range of [-1,\n> > > 1] we need to keep the limits as is.\n> > \n> > You're right.\n> > \n> > > Ideally the limits should change based on the contrast. But we don't\n> > > have that.\n> > > \n> > > > > + }\n> > > > \n> > > > No need for curly braces.\n> > > \n> > > Ooops again.\n> > > \n> > > > > +\n> > > > > const auto &hue = controls.get(controls::Hue);\n> > > > > if (hue) {\n> > > > > /* Scale the Hue from ]-90, +90] */\n> > > > > @@ -179,7 +193,15 @@ void ColorProcessing::process([[maybe_unused]] IPAContext &context,\n> > > > > [[maybe_unused]] const rkisp1_stat_buffer *stats,\n> > > > > ControlList &metadata)\n> > > > > {\n> > > > > - metadata.set(controls::Brightness, frameContext.cproc.brightness.value());\n> > > > > + /*\n> > > > > + * Report the actually applied brightness value. See queueRequest() for\n> > > > > + * more details.\n> > > > > + */\n> > > > > + float actualBrightness = (frameContext.cproc.brightness.value() -\n> > > > > + (1 - frameContext.cproc.contrast.value())) /\n> > > > > + 2;\n> > > > \n> > > > Can we compute this when the controls change and cache the value instead\n> > > > of recomputing it for every frame ?\n> > > \n> > > Hm, I wouldn't be worried about performance, but it could actually\n> > > improve the readability of the code. I'll give it a try.\n> > \n> > I think it would be more readable, yes.\n> > \n> > > > > +\n> > > > > + metadata.set(controls::Brightness, actualBrightness);\n> > > > > metadata.set(controls::Contrast, frameContext.cproc.contrast.value());\n> > > > > metadata.set(controls::Hue, frameContext.cproc.hue.value() * kHueScale);\n> > > > > metadata.set(controls::Saturation, frameContext.cproc.saturation.value());\n> > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > > > index 80b035044cda..309567858b64 100644\n> > > > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > > > @@ -122,6 +122,7 @@ struct IPAActiveState {\n> > > > > } ccm;\n> > > > > \n> > > > > struct {\n> > > > > + float requestedBrightness;\n> > > > > BrightnessQ brightness;\n> > > > > ContrastQ contrast;\n> > > > > HueQ hue;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D4C51BDCBF\n\tfor ;\n\tWed, 21 Jan 2026 16:29:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1B7BA61FBB;\n\tWed, 21 Jan 2026 17:29:34 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9101661F9F\n\tfor ;\n\tWed, 21 Jan 2026 17:29:32 +0100 (CET)","from pendragon.ideasonboard.com\n\t(2001-14ba-703d-e500--2a1.rev.dnainternet.fi\n\t[IPv6:2001:14ba:703d:e500::2a1])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 5E4FA1C6;\n\tWed, 21 Jan 2026 17:29:00 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"eR3tnqXC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1769012940;\n\tbh=HvY8kd1KYk+nL5jVKQ7I7Rw6llob46jtZTHIOAT5lUQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=eR3tnqXCfGSfHwzQhO6A87dU5b/0YhXZnLG/8SCSF9hY0OxIafT6dh3dO3FkSRv5x\n\tJl0WrfAiFElZLFRSaRnseFz/9FdL+/lUKuFKvCesoq+X5CF/8gBXE7pooLVapd2Imb\n\tM+hrOZr5iznOBZmXfNimC1tcAdZWSP56w7ue0x44=","Date":"Wed, 21 Jan 2026 18:29:31 +0200","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1] ipa: rkisp1: cproc: Fix brightness and contrast value\n\thandling","Message-ID":"<20260121162931.GF382676@killaraus>","References":"<20260116121658.215712-1-stefan.klug@ideasonboard.com>\n\t<20260121001512.GA382676@killaraus>\n\t<176897487745.508107.12355351030826782862@localhost>\n\t<20260121113619.GE382676@killaraus>\n\t<176899766848.23550.2759588137284894601@localhost>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<176899766848.23550.2759588137284894601@localhost>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":37982,"web_url":"https://patchwork.libcamera.org/comment/37982/","msgid":"<176958308059.810169.8619502228769782331@localhost>","date":"2026-01-28T06:51:20","subject":"Re: [PATCH v1] ipa: rkisp1: cproc: Fix brightness and contrast value\n\thandling","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Laurent,\n\nQuoting Laurent Pinchart (2026-01-21 17:29:31)\n> On Wed, Jan 21, 2026 at 01:14:28PM +0100, Stefan Klug wrote:\n> > Quoting Laurent Pinchart (2026-01-21 12:36:19)\n> > > On Wed, Jan 21, 2026 at 06:54:37AM +0100, Stefan Klug wrote:\n> > > > Quoting Laurent Pinchart (2026-01-21 01:15:12)\n> > > > > On Fri, Jan 16, 2026 at 01:16:14PM +0100, Stefan Klug wrote:\n> > > > > > By reverse engineering it was discovered that the transfer curve applied\n> > > > > > by the CPROC module is\n> > > > > > \n> > > > > > Yout = Yin * contrast + brightness/2.\n> > > > > > \n> > > > > > However the most common expectation on contrast is to not change the\n> > > > > > middle gray of an image. So the expected curve is:\n> > > > > > \n> > > > > > Yout = (Yin - 0.5) * contrast + 0.5 + brightness\n> > > > > > \n> > > > > > Map the user expectation to the hardware by changing the brightness\n> > > > > > value written to the hardware. This also includes multiplying the\n> > > > > > requested brightness value by two.\n> > > > > > \n> > > > > > Note: The limits of the brightness control are left as is even though\n> > > > > > they can not be reached without changing the contrast. But limiting them\n> > > > > > to [0.5, 0.5] would cut off parts of the combined control space of\n> > > > > > contrast + brightness.\n> > > > > > \n> > > > > > Signed-off-by: Stefan Klug \n> > > > > > \n> > > > > > ---\n> > > > > > \n> > > > > > This patch needs to be applied on top of the quantization series:\n> > > > > > https://patchwork.libcamera.org/project/libcamera/list/?series=5708\n> > > > > > ---\n> > > > > > src/ipa/rkisp1/algorithms/cproc.cpp | 38 +++++++++++++++++++++++------\n> > > > > > src/ipa/rkisp1/ipa_context.h | 1 +\n> > > > > > 2 files changed, 31 insertions(+), 8 deletions(-)\n> > > > > > \n> > > > > > diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp\n> > > > > > index 7484e4780094..ec2b264a79b9 100644\n> > > > > > --- a/src/ipa/rkisp1/algorithms/cproc.cpp\n> > > > > > +++ b/src/ipa/rkisp1/algorithms/cproc.cpp\n> > > > > > @@ -100,13 +100,9 @@ void ColorProcessing::queueRequest(IPAContext &context,\n> > > > > > \n> > > > > > const auto &brightness = controls.get(controls::Brightness);\n> > > > > > if (brightness) {\n> > > > > > - BrightnessQ value = *brightness;\n> > > > > > - if (cproc.brightness != value) {\n> > > > > > - cproc.brightness = value;\n> > > > > > - update = true;\n> > > > > > - }\n> > > > > > -\n> > > > > > - LOG(RkISP1CProc, Debug) << \"Set brightness to \" << value;\n> > > > > > + cproc.requestedBrightness = *brightness;\n> > > > > \n> > > > > cproc.requestedBrightness should be reset in\n> > > > > ColorProcessing::configure().\n> > > > \n> > > > Yes\n> > > > \n> > > > > > + LOG(RkISP1CProc, Debug) << \"Set brightness to \" << *brightness;\n> > > > > > + update = true;\n> > > > > \n> > > > > Why do you do this unconditionally now ?\n> > > > \n> > > > It's the same as before. Depends if we want to log the control setter,\n> > > > or the actual value applied. I thought the intention was to log the\n> > > > setter as the log wasn't inside the \"if (cproc.brightness != value) {\".\n> > > \n> > > I meant setting update to true, not logging.\n> > > \n> > > > > > }\n> > > > > > \n> > > > > > const auto &contrast = controls.get(controls::Contrast);\n> > > > > > @@ -120,6 +116,24 @@ void ColorProcessing::queueRequest(IPAContext &context,\n> > > > > > LOG(RkISP1CProc, Debug) << \"Set contrast to \" << value;\n> > > > > > }\n> > > > > > \n> > > > > > + /*\n> > > > > > + * The CPROC module applies the transfer function\n> > > > > > + *\n> > > > > > + * Yout = Yin * contrast + brightness/2\n> > > \n> > > By the way, please record the fact that this is due to the CPROC using\n> > > 9-bit data internally after multiplying by the contrast (which is\n> > > limited to x2) to avoid intermediate clamping, and the brightness\n> > > register therefore being expressed as 9 bits.\n> > \n> > That is the assumption. CPROC also allows 10bit input data so I'm\n> > hesitant talking about 9 bits. What about:\n> \n> Indeed, according to the i.MX8MP reference manual its input is 10 bits\n> (assuming the diagram is correct). That's weird though, because you've\n> tested that pixel values are increased by exactly brightness / 2 if I\n> recall correctly.\n> \n> > The calculations in CPROC are done one bit wider than the input data to\n> > account for the maximum contrast of 2x without clamping. Brightness is\n> > applied after and therefor with only half the numeric value.\n> \n> s/therefor/therefore/\n> \n> Not sure if it's totally accurate (as we're guessing some things, but I\n> think this is good enough for now.\n\nI did a bit more guessing here as I realized that above explanation\nonly made partial sense. What about the following guess:\n\nInput to the CPROC module is always 10bit data (this is known). The\nhardware shifts right by 1 bit to create headroom for the contrast\nmultiplication by a maximum of 2x. Then it applies the contrast\nmultiplication and the brightness addition in 10bit space. Then it\nshifts right by another bit to get the 8 bit output data. \n\nIn my head that kind of makes sense :-)\n\nBest regards,\nStefan\n\n> \n> > > > > > + *\n> > > > > > + * Most users expect that changing contrast doesn't change the middle\n> > > > > > + * gray and that the brightness value is normalized to one. So they\n> > > > > > + * expect a transfer function of\n> > > > > > + *\n> > > > > > + * Yout = (Yin - 0.5) * contrast + 0.5 + brightness\n> > > \n> > > Yout = Yin * contrast - 0.5 * contrast + 0.5 + brightness\n> > \n> > What do you mean here? Why would you prefer this notation?\n> \n> Oops no that's a leftover of notes I was taking :-) Please ignore this.\n> \n> > > BR = (1 - contrast) + brightness * 2\n> > > \n> > > > > > + *\n> > > > > > + * To implement that with the given hardware we need to correct the\n> > > > > > + * brightness value.\n> > > > > > + */\n> > > > > > + if (update) {\n> > > > > > + cproc.brightness = (1 - cproc.contrast.value()) + 2 * cproc.requestedBrightness;\n> > > > > \n> > > > > I don't think the x2 factor is correct if you don't change the limits.\n> > > > \n> > > > Why not? You can't realize every requested value as it is dependent on\n> > > > the contrast value. But this way we can reach the whole available space.\n> > > > \n> > > > The first \"(1 - cproc.contrast.value())\" has a output range of [-1, 1]\n> > > > so to be able to set the hardware brightness value in the range of [-1,\n> > > > 1] we need to keep the limits as is.\n> > > \n> > > You're right.\n> > > \n> > > > Ideally the limits should change based on the contrast. But we don't\n> > > > have that.\n> > > > \n> > > > > > + }\n> > > > > \n> > > > > No need for curly braces.\n> > > > \n> > > > Ooops again.\n> > > > \n> > > > > > +\n> > > > > > const auto &hue = controls.get(controls::Hue);\n> > > > > > if (hue) {\n> > > > > > /* Scale the Hue from ]-90, +90] */\n> > > > > > @@ -179,7 +193,15 @@ void ColorProcessing::process([[maybe_unused]] IPAContext &context,\n> > > > > > [[maybe_unused]] const rkisp1_stat_buffer *stats,\n> > > > > > ControlList &metadata)\n> > > > > > {\n> > > > > > - metadata.set(controls::Brightness, frameContext.cproc.brightness.value());\n> > > > > > + /*\n> > > > > > + * Report the actually applied brightness value. See queueRequest() for\n> > > > > > + * more details.\n> > > > > > + */\n> > > > > > + float actualBrightness = (frameContext.cproc.brightness.value() -\n> > > > > > + (1 - frameContext.cproc.contrast.value())) /\n> > > > > > + 2;\n> > > > > \n> > > > > Can we compute this when the controls change and cache the value instead\n> > > > > of recomputing it for every frame ?\n> > > > \n> > > > Hm, I wouldn't be worried about performance, but it could actually\n> > > > improve the readability of the code. I'll give it a try.\n> > > \n> > > I think it would be more readable, yes.\n> > > \n> > > > > > +\n> > > > > > + metadata.set(controls::Brightness, actualBrightness);\n> > > > > > metadata.set(controls::Contrast, frameContext.cproc.contrast.value());\n> > > > > > metadata.set(controls::Hue, frameContext.cproc.hue.value() * kHueScale);\n> > > > > > metadata.set(controls::Saturation, frameContext.cproc.saturation.value());\n> > > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > > > > index 80b035044cda..309567858b64 100644\n> > > > > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > > > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > > > > @@ -122,6 +122,7 @@ struct IPAActiveState {\n> > > > > > } ccm;\n> > > > > > \n> > > > > > struct {\n> > > > > > + float requestedBrightness;\n> > > > > > BrightnessQ brightness;\n> > > > > > ContrastQ contrast;\n> > > > > > HueQ hue;\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id AC347C3220\n\tfor ;\n\tWed, 28 Jan 2026 06:51:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D2A2561FC8;\n\tWed, 28 Jan 2026 07:51:24 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A330961A35\n\tfor ;\n\tWed, 28 Jan 2026 07:51:23 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:1a60:e70f:ec38:13a9])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id B7E89581;\n\tWed, 28 Jan 2026 07:50:46 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"LAnshQdV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1769583046;\n\tbh=1Q6JL7SjK7HmosqUNbEd5G5+VNr/TjCGXBZMETPEM8c=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=LAnshQdV5xVfFYA0fI+h5RQkDF41rNN9jj9noh1G7xED3EjPoL0BCMD1VeAiJHcyZ\n\tXK+dUfGKcxNBwd3i6z4qc52jiiJYhSkI3cnzidh9U1RmA5imioArFsNI0b7hmiR+2O\n\tMDWVinRx3KtiWkHTtPHhN4HrZk2OM/iQ/IPfudiY=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20260121162931.GF382676@killaraus>","References":"<20260116121658.215712-1-stefan.klug@ideasonboard.com>\n\t<20260121001512.GA382676@killaraus>\n\t<176897487745.508107.12355351030826782862@localhost>\n\t<20260121113619.GE382676@killaraus>\n\t<176899766848.23550.2759588137284894601@localhost>\n\t<20260121162931.GF382676@killaraus>","Subject":"Re: [PATCH v1] ipa: rkisp1: cproc: Fix brightness and contrast value\n\thandling","From":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","To":"Laurent Pinchart ","Date":"Wed, 28 Jan 2026 07:51:20 +0100","Message-ID":"<176958308059.810169.8619502228769782331@localhost>","User-Agent":"alot/0.12.dev8+g2c003385c862.d20250602","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]