[{"id":34626,"web_url":"https://patchwork.libcamera.org/comment/34626/","msgid":"<20250624183659.GB20757@pendragon.ideasonboard.com>","date":"2025-06-24T18:36:59","subject":"Re: [PATCH 1/3] ipa: rkisp1: cproc: Report metadata","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Tue, Jun 24, 2025 at 06:12:21PM +0100, Kieran Bingham wrote:\n> From: \"van Veen, Stephan\" \n> \n> Presently the colour processing component exposes controls for\n> brightness, saturation, and contrast to the applications which are\n> handled and processed accordingly on the ISP.\n> \n> The implementation lacks reporting the values that are set back to the\n> application.\n> \n> Provide the inverse conversion functions to convert the values applied\n> and report them in the completed request metadata.\n> \n> Signed-off-by: van Veen, Stephan \n> Signed-off-by: Zenker, Sebastian \n> [Kieran: Refactor commit message, fix checkstyle warnings]\n> Signed-off-by: Kieran Bingham \n> ---\n> src/ipa/rkisp1/algorithms/cproc.cpp | 22 ++++++++++++++++++++++\n> src/ipa/rkisp1/algorithms/cproc.h | 4 ++++\n> 2 files changed, 26 insertions(+)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp\n> index d1fff6990d37..9213ee49b374 100644\n> --- a/src/ipa/rkisp1/algorithms/cproc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/cproc.cpp\n> @@ -44,11 +44,21 @@ int convertBrightness(const float v)\n> \treturn std::clamp(std::lround(v * 128), -128, 127);\n> }\n> \n> +float convertBrightness(const int v)\n> +{\n> +\treturn static_cast(v) / 128.0f;\n> +}\n> +\n> int convertContrastOrSaturation(const float v)\n> {\n> \treturn std::clamp(std::lround(v * 128), 0, 255);\n> }\n> \n> +float convertContrastOrSaturation(const int v)\n> +{\n> +\treturn static_cast(v) / 128.0f;\n> +}\n> +\n> } /* namespace */\n> \n> /**\n> @@ -153,6 +163,18 @@ void ColorProcessing::prepare([[maybe_unused]] IPAContext &context,\n> \tconfig->sat = frameContext.cproc.saturation;\n> }\n> \n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::process\n> + */\n> +void ColorProcessing::process([[maybe_unused]] IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> +\t\t\t IPAFrameContext &frameContext, [[maybe_unused]] const rkisp1_stat_buffer *stats,\n> +\t\t\t ControlList &metadata)\n> +{\n> +\tmetadata.set(controls::Brightness, convertBrightness(frameContext.cproc.brightness));\n> +\tmetadata.set(controls::Contrast, convertContrastOrSaturation(frameContext.cproc.contrast));\n> +\tmetadata.set(controls::Saturation, convertContrastOrSaturation(frameContext.cproc.saturation));\n\nInstead of converting this for every frame, could we store the floating\npoint value in the frame context, and only convert them to hardware\nconfiguration when .update is set ?\n\n> +}\n> +\n> REGISTER_IPA_ALGORITHM(ColorProcessing, \"ColorProcessing\")\n> \n> } /* namespace ipa::rkisp1::algorithms */\n> diff --git a/src/ipa/rkisp1/algorithms/cproc.h b/src/ipa/rkisp1/algorithms/cproc.h\n> index fd38fd17e8bb..9b589ebd4ad7 100644\n> --- a/src/ipa/rkisp1/algorithms/cproc.h\n> +++ b/src/ipa/rkisp1/algorithms/cproc.h\n> @@ -30,6 +30,10 @@ public:\n> \tvoid prepare(IPAContext &context, const uint32_t frame,\n> \t\t IPAFrameContext &frameContext,\n> \t\t RkISP1Params *params) override;\n> +\tvoid process(IPAContext &context, const uint32_t frame,\n> +\t\t IPAFrameContext &frameContext,\n> +\t\t const rkisp1_stat_buffer *stats,\n> +\t\t ControlList &metadata) override;\n> };\n> \n> } /* namespace ipa::rkisp1::algorithms */","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 B0A62BDE6B\n\tfor ;\n\tTue, 24 Jun 2025 18:37:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8FB5068DDD;\n\tTue, 24 Jun 2025 20:37:22 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 85C7461534\n\tfor ;\n\tTue, 24 Jun 2025 20:37:20 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 7EF5A134A; \n\tTue, 24 Jun 2025 20:37:02 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"SnlIF2yu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1750790222;\n\tbh=qD9/ArWKjzEdAGqgnfzOdkOltdNsMtLIK3XqsHJSt7U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SnlIF2yu0goyEKj2+xsrmgqIQwqQTXydMeMww5ukox0jvzwjfR1ID9BKKyWyGfWn0\n\t10BBHkwYdfbbsUxaQfFnNBbbwMlCS81kHHrpXv6CYcf5AhX+TH7UuCeFZCrRuMaOec\n\tXa4ysMQBifCyygQm9hsAGz04xYgZvEAgdPKuZuJw=","Date":"Tue, 24 Jun 2025 21:36:59 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"libcamera devel ,\n\t\"van Veen, Stephan\" ,\n\t\"Zenker, Sebastian\" ","Subject":"Re: [PATCH 1/3] ipa: rkisp1: cproc: Report metadata","Message-ID":"<20250624183659.GB20757@pendragon.ideasonboard.com>","References":"<20250624171223.2181226-1-kieran.bingham@ideasonboard.com>\n\t<20250624171223.2181226-2-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250624171223.2181226-2-kieran.bingham@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":34812,"web_url":"https://patchwork.libcamera.org/comment/34812/","msgid":"<175191129385.3123004.9230415131881002775@ping.linuxembedded.co.uk>","date":"2025-07-07T18:01:33","subject":"Re: [PATCH 1/3] ipa: rkisp1: cproc: Report metadata","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2025-06-24 19:36:59)\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Tue, Jun 24, 2025 at 06:12:21PM +0100, Kieran Bingham wrote:\n> > From: \"van Veen, Stephan\" \n> > \n> > Presently the colour processing component exposes controls for\n> > brightness, saturation, and contrast to the applications which are\n> > handled and processed accordingly on the ISP.\n> > \n> > The implementation lacks reporting the values that are set back to the\n> > application.\n> > \n> > Provide the inverse conversion functions to convert the values applied\n> > and report them in the completed request metadata.\n> > \n> > Signed-off-by: van Veen, Stephan \n> > Signed-off-by: Zenker, Sebastian \n> > [Kieran: Refactor commit message, fix checkstyle warnings]\n> > Signed-off-by: Kieran Bingham \n> > ---\n> > src/ipa/rkisp1/algorithms/cproc.cpp | 22 ++++++++++++++++++++++\n> > src/ipa/rkisp1/algorithms/cproc.h | 4 ++++\n> > 2 files changed, 26 insertions(+)\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp\n> > index d1fff6990d37..9213ee49b374 100644\n> > --- a/src/ipa/rkisp1/algorithms/cproc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/cproc.cpp\n> > @@ -44,11 +44,21 @@ int convertBrightness(const float v)\n> > return std::clamp(std::lround(v * 128), -128, 127);\n> > }\n> > \n> > +float convertBrightness(const int v)\n> > +{\n> > + return static_cast(v) / 128.0f;\n> > +}\n> > +\n> > int convertContrastOrSaturation(const float v)\n> > {\n> > return std::clamp(std::lround(v * 128), 0, 255);\n> > }\n> > \n> > +float convertContrastOrSaturation(const int v)\n> > +{\n> > + return static_cast(v) / 128.0f;\n> > +}\n> > +\n> > } /* namespace */\n> > \n> > /**\n> > @@ -153,6 +163,18 @@ void ColorProcessing::prepare([[maybe_unused]] IPAContext &context,\n> > config->sat = frameContext.cproc.saturation;\n> > }\n> > \n> > +/**\n> > + * \\copydoc libcamera::ipa::Algorithm::process\n> > + */\n> > +void ColorProcessing::process([[maybe_unused]] IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > + IPAFrameContext &frameContext, [[maybe_unused]] const rkisp1_stat_buffer *stats,\n> > + ControlList &metadata)\n> > +{\n> > + metadata.set(controls::Brightness, convertBrightness(frameContext.cproc.brightness));\n> > + metadata.set(controls::Contrast, convertContrastOrSaturation(frameContext.cproc.contrast));\n> > + metadata.set(controls::Saturation, convertContrastOrSaturation(frameContext.cproc.saturation));\n> \n> Instead of converting this for every frame, could we store the floating\n> point value in the frame context, and only convert them to hardware\n> configuration when .update is set ?\n\nWould you prefer to see just the float stored in the context, perhaps\nquantised to what the hardware will set? Or would you prefer I extend\nthe framecontext to store both the float and integer values to avoid\nmultiple round trips of conversions ?\n\n--\nKieran\n\n\n> \n> > +}\n> > +\n> > REGISTER_IPA_ALGORITHM(ColorProcessing, \"ColorProcessing\")\n> > \n> > } /* namespace ipa::rkisp1::algorithms */\n> > diff --git a/src/ipa/rkisp1/algorithms/cproc.h b/src/ipa/rkisp1/algorithms/cproc.h\n> > index fd38fd17e8bb..9b589ebd4ad7 100644\n> > --- a/src/ipa/rkisp1/algorithms/cproc.h\n> > +++ b/src/ipa/rkisp1/algorithms/cproc.h\n> > @@ -30,6 +30,10 @@ public:\n> > void prepare(IPAContext &context, const uint32_t frame,\n> > IPAFrameContext &frameContext,\n> > RkISP1Params *params) override;\n> > + void process(IPAContext &context, const uint32_t frame,\n> > + IPAFrameContext &frameContext,\n> > + const rkisp1_stat_buffer *stats,\n> > + ControlList &metadata) override;\n> > };\n> > \n> > } /* namespace ipa::rkisp1::algorithms */\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 17DBEC0DA4\n\tfor ;\n\tMon, 7 Jul 2025 18:01:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D153668EBF;\n\tMon, 7 Jul 2025 20:01:38 +0200 (CEST)","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 6791E6151E\n\tfor ;\n\tMon, 7 Jul 2025 20:01:37 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 077F57E4;\n\tMon, 7 Jul 2025 20:01:10 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"W/nGkogO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1751911270;\n\tbh=V97Rw/TTIgJVETOkjMMsjPEZ4qXFHDzPKBHsu1yUtDY=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=W/nGkogOI5i5WJ+4hVsfCwbgaB2YZPqsBn6/WJerGfVReX+wyI6sm6ZjYcxns94mv\n\tnO1lTXK+UrQQVBYQKEU4C06VktQxdQ65D/KiHhp+ODdrJCxRlI674ZKPvsqUUjCpPE\n\tvoPT/06higzluy3aF74J22ylcxb1Z5mF/RWq451c=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250624183659.GB20757@pendragon.ideasonboard.com>","References":"<20250624171223.2181226-1-kieran.bingham@ideasonboard.com>\n\t<20250624171223.2181226-2-kieran.bingham@ideasonboard.com>\n\t<20250624183659.GB20757@pendragon.ideasonboard.com>","Subject":"Re: [PATCH 1/3] ipa: rkisp1: cproc: Report metadata","From":"Kieran Bingham ","Cc":"libcamera devel , \"van Veen,\n\tStephan\" , \"Zenker,\n\tSebastian\" ","To":"Laurent Pinchart ","Date":"Mon, 07 Jul 2025 19:01:33 +0100","Message-ID":"<175191129385.3123004.9230415131881002775@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","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\" "}}]