[{"id":14795,"web_url":"https://patchwork.libcamera.org/comment/14795/","msgid":"","date":"2021-01-26T22:47:05","subject":"Re: [libcamera-devel] [PATCH v4 5/6] ipa: raspberrypi: Add a\n\tDenoiseAlgorithm class to the Controller","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Tue, Jan 26, 2021 at 04:24:11PM +0000, Naushir Patuck wrote:\n> This denoise algorithm class will be used to pass in the user requested\n> denoise operating mode to the controller. The existing Denoise\n> controller will derive from this new DenoiseAlgorithm class.\n> \n> Add a denoise mode field in the denoise status metadata object for the\n> IPA to use when configuring the ISP.\n> \n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n> Reviewed-by: Kieran Bingham \n> ---\n> .../controller/denoise_algorithm.hpp | 23 +++++++++++++++++++\n> .../raspberrypi/controller/denoise_status.h | 1 +\n> src/ipa/raspberrypi/controller/rpi/sdn.cpp | 11 +++++++--\n> src/ipa/raspberrypi/controller/rpi/sdn.hpp | 5 +++-\n> 4 files changed, 37 insertions(+), 3 deletions(-)\n> create mode 100644 src/ipa/raspberrypi/controller/denoise_algorithm.hpp\n> \n> diff --git a/src/ipa/raspberrypi/controller/denoise_algorithm.hpp b/src/ipa/raspberrypi/controller/denoise_algorithm.hpp\n> new file mode 100644\n> index 000000000000..df1f35cc9e5f\n> --- /dev/null\n> +++ b/src/ipa/raspberrypi/controller/denoise_algorithm.hpp\n> @@ -0,0 +1,23 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2021, Raspberry Pi (Trading) Limited\n> + *\n> + * denoise.hpp - Denoise control algorithm interface\n> + */\n> +#pragma once\n> +\n> +#include \"algorithm.hpp\"\n> +\n> +namespace RPiController {\n> +\n> +enum DenoiseMode { Off, ColourOff, ColourFast, ColourHighQuality };\n\nShould we consider using enum class here ? Adding an unqualified 'Off'\nidentifier to the namespace seems quite prone to namespace clashes.\n\n(Additionally we usually split enums with one entry per line, but the\nRPi IPA doesn't use the same coding style, so it's up to you.)\n\n> +\n> +class DenoiseAlgorithm : public Algorithm\n> +{\n> +public:\n> +\tDenoiseAlgorithm(Controller *controller) : Algorithm(controller) {}\n> +\t// A Denoise algorithm must provide the following:\n> +\tvirtual void SetMode(DenoiseMode mode) = 0;\n> +};\n> +\n> +} // namespace RPiController\n> diff --git a/src/ipa/raspberrypi/controller/denoise_status.h b/src/ipa/raspberrypi/controller/denoise_status.h\n> index 06d7cfb91ae8..6064cc2c192e 100644\n> --- a/src/ipa/raspberrypi/controller/denoise_status.h\n> +++ b/src/ipa/raspberrypi/controller/denoise_status.h\n> @@ -16,6 +16,7 @@ struct DenoiseStatus {\n> \tdouble noise_constant;\n> \tdouble noise_slope;\n> \tdouble strength;\n> +\tunsigned int mode;\n\nShould this be of type DenoiseMode ?\n\n> };\n> \n> #ifdef __cplusplus\n> diff --git a/src/ipa/raspberrypi/controller/rpi/sdn.cpp b/src/ipa/raspberrypi/controller/rpi/sdn.cpp\n> index d8c1521a6633..ecc7845eda4c 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/sdn.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/sdn.cpp\n> @@ -1,6 +1,6 @@\n> /* SPDX-License-Identifier: BSD-2-Clause */\n> /*\n> - * Copyright (C) 2019, Raspberry Pi (Trading) Limited\n> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Limited\n> *\n> * sdn.cpp - SDN (spatial denoise) control algorithm\n> */\n> @@ -18,7 +18,7 @@ using namespace RPiController;\n> #define NAME \"rpi.sdn\"\n> \n> Sdn::Sdn(Controller *controller)\n> -\t: Algorithm(controller)\n> +\t: DenoiseAlgorithm(controller), mode_(DenoiseMode::ColourOff)\n> {\n> }\n> \n> @@ -48,6 +48,7 @@ void Sdn::Prepare(Metadata *image_metadata)\n> \tstatus.noise_constant = noise_status.noise_constant * deviation_;\n> \tstatus.noise_slope = noise_status.noise_slope * deviation_;\n> \tstatus.strength = strength_;\n> +\tstatus.mode = mode_;\n> \timage_metadata->Set(\"denoise.status\", status);\n> \tRPI_LOG(\"Sdn: programmed constant \" << status.noise_constant\n> \t\t\t\t\t << \" slope \" << status.noise_slope\n> @@ -55,6 +56,12 @@ void Sdn::Prepare(Metadata *image_metadata)\n> \t\t\t\t\t << status.strength);\n> }\n> \n> +void Sdn::SetMode(DenoiseMode mode)\n> +{\n> +\t// We only distinguish between off and all other modes.\n> +\tmode_ = mode;\n> +}\n> +\n> // Register algorithm with the system.\n> static Algorithm *Create(Controller *controller)\n> {\n> diff --git a/src/ipa/raspberrypi/controller/rpi/sdn.hpp b/src/ipa/raspberrypi/controller/rpi/sdn.hpp\n> index 486c000d7b77..2371ce04163f 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/sdn.hpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/sdn.hpp\n> @@ -7,12 +7,13 @@\n> #pragma once\n> \n> #include \"../algorithm.hpp\"\n> +#include \"../denoise_algorithm.hpp\"\n> \n> namespace RPiController {\n> \n> // Algorithm to calculate correct spatial denoise (SDN) settings.\n> \n> -class Sdn : public Algorithm\n> +class Sdn : public DenoiseAlgorithm\n> {\n> public:\n> \tSdn(Controller *controller = NULL);\n> @@ -20,10 +21,12 @@ public:\n> \tvoid Read(boost::property_tree::ptree const ¶ms) override;\n> \tvoid Initialise() override;\n> \tvoid Prepare(Metadata *image_metadata) override;\n> +\tvoid SetMode(DenoiseMode mode) override;\n> \n> private:\n> \tdouble deviation_;\n> \tdouble strength_;\n> +\tDenoiseMode mode_;\n> };\n> \n> } // namespace RPiController","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 3E152BD808\n\tfor ;\n\tTue, 26 Jan 2021 22:47:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 90D0E68329;\n\tTue, 26 Jan 2021 23:47:28 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B47B76831A\n\tfor ;\n\tTue, 26 Jan 2021 23:47:25 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DADE82C1;\n\tTue, 26 Jan 2021 23:47:24 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"lbAKK+dP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1611701245;\n\tbh=xJ+WIro642O8k3qxeiNbbq95VnhtrTLMh56BcjJMVOU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lbAKK+dPIsWKuDT6UZakvn24pwv4+vwHC59NJ5yLOWExQuyQRzF/+AJuUX7tg02fs\n\tCxATWiiiANNO7lptLc7NRRXV2ZPA4GRgOGkuXE3+i1WetNXkzjuhLl5nW4EFxseGuM\n\tPT7fymqA8jSJ0AFd44Izm5x1W4NqSV2Z1kT5v5og=","Date":"Wed, 27 Jan 2021 00:47:05 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210126162412.824033-1-naush@raspberrypi.com>\n\t<20210126162412.824033-6-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210126162412.824033-6-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v4 5/6] ipa: raspberrypi: Add a\n\tDenoiseAlgorithm class to the Controller","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14798,"web_url":"https://patchwork.libcamera.org/comment/14798/","msgid":"","date":"2021-01-26T22:54:34","subject":"Re: [libcamera-devel] [PATCH v4 5/6] ipa: raspberrypi: Add a\n\tDenoiseAlgorithm class to the Controller","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Jan 27, 2021 at 12:47:06AM +0200, Laurent Pinchart wrote:\n> Hi Naush,\n> \n> Thank you for the patch.\n> \n> On Tue, Jan 26, 2021 at 04:24:11PM +0000, Naushir Patuck wrote:\n> > This denoise algorithm class will be used to pass in the user requested\n> > denoise operating mode to the controller. The existing Denoise\n> > controller will derive from this new DenoiseAlgorithm class.\n> > \n> > Add a denoise mode field in the denoise status metadata object for the\n> > IPA to use when configuring the ISP.\n> > \n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: David Plowman \n> > Reviewed-by: Kieran Bingham \n> > ---\n> > .../controller/denoise_algorithm.hpp | 23 +++++++++++++++++++\n> > .../raspberrypi/controller/denoise_status.h | 1 +\n> > src/ipa/raspberrypi/controller/rpi/sdn.cpp | 11 +++++++--\n> > src/ipa/raspberrypi/controller/rpi/sdn.hpp | 5 +++-\n> > 4 files changed, 37 insertions(+), 3 deletions(-)\n> > create mode 100644 src/ipa/raspberrypi/controller/denoise_algorithm.hpp\n> > \n> > diff --git a/src/ipa/raspberrypi/controller/denoise_algorithm.hpp b/src/ipa/raspberrypi/controller/denoise_algorithm.hpp\n> > new file mode 100644\n> > index 000000000000..df1f35cc9e5f\n> > --- /dev/null\n> > +++ b/src/ipa/raspberrypi/controller/denoise_algorithm.hpp\n> > @@ -0,0 +1,23 @@\n> > +/* SPDX-License-Identifier: BSD-2-Clause */\n> > +/*\n> > + * Copyright (C) 2021, Raspberry Pi (Trading) Limited\n> > + *\n> > + * denoise.hpp - Denoise control algorithm interface\n> > + */\n> > +#pragma once\n> > +\n> > +#include \"algorithm.hpp\"\n> > +\n> > +namespace RPiController {\n> > +\n> > +enum DenoiseMode { Off, ColourOff, ColourFast, ColourHighQuality };\n> \n> Should we consider using enum class here ? Adding an unqualified 'Off'\n> identifier to the namespace seems quite prone to namespace clashes.\n> \n> (Additionally we usually split enums with one entry per line, but the\n> RPi IPA doesn't use the same coding style, so it's up to you.)\n\nAnother question, what's the difference between ColourFast and\nColourHighQuality ? How fast is fast, or how slow is high quality ?\n\n> > +\n> > +class DenoiseAlgorithm : public Algorithm\n> > +{\n> > +public:\n> > +\tDenoiseAlgorithm(Controller *controller) : Algorithm(controller) {}\n> > +\t// A Denoise algorithm must provide the following:\n> > +\tvirtual void SetMode(DenoiseMode mode) = 0;\n> > +};\n> > +\n> > +} // namespace RPiController\n> > diff --git a/src/ipa/raspberrypi/controller/denoise_status.h b/src/ipa/raspberrypi/controller/denoise_status.h\n> > index 06d7cfb91ae8..6064cc2c192e 100644\n> > --- a/src/ipa/raspberrypi/controller/denoise_status.h\n> > +++ b/src/ipa/raspberrypi/controller/denoise_status.h\n> > @@ -16,6 +16,7 @@ struct DenoiseStatus {\n> > \tdouble noise_constant;\n> > \tdouble noise_slope;\n> > \tdouble strength;\n> > +\tunsigned int mode;\n> \n> Should this be of type DenoiseMode ?\n> \n> > };\n> > \n> > #ifdef __cplusplus\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/sdn.cpp b/src/ipa/raspberrypi/controller/rpi/sdn.cpp\n> > index d8c1521a6633..ecc7845eda4c 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/sdn.cpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/sdn.cpp\n> > @@ -1,6 +1,6 @@\n> > /* SPDX-License-Identifier: BSD-2-Clause */\n> > /*\n> > - * Copyright (C) 2019, Raspberry Pi (Trading) Limited\n> > + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Limited\n> > *\n> > * sdn.cpp - SDN (spatial denoise) control algorithm\n> > */\n> > @@ -18,7 +18,7 @@ using namespace RPiController;\n> > #define NAME \"rpi.sdn\"\n> > \n> > Sdn::Sdn(Controller *controller)\n> > -\t: Algorithm(controller)\n> > +\t: DenoiseAlgorithm(controller), mode_(DenoiseMode::ColourOff)\n> > {\n> > }\n> > \n> > @@ -48,6 +48,7 @@ void Sdn::Prepare(Metadata *image_metadata)\n> > \tstatus.noise_constant = noise_status.noise_constant * deviation_;\n> > \tstatus.noise_slope = noise_status.noise_slope * deviation_;\n> > \tstatus.strength = strength_;\n> > +\tstatus.mode = mode_;\n> > \timage_metadata->Set(\"denoise.status\", status);\n> > \tRPI_LOG(\"Sdn: programmed constant \" << status.noise_constant\n> > \t\t\t\t\t << \" slope \" << status.noise_slope\n> > @@ -55,6 +56,12 @@ void Sdn::Prepare(Metadata *image_metadata)\n> > \t\t\t\t\t << status.strength);\n> > }\n> > \n> > +void Sdn::SetMode(DenoiseMode mode)\n> > +{\n> > +\t// We only distinguish between off and all other modes.\n> > +\tmode_ = mode;\n> > +}\n> > +\n> > // Register algorithm with the system.\n> > static Algorithm *Create(Controller *controller)\n> > {\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/sdn.hpp b/src/ipa/raspberrypi/controller/rpi/sdn.hpp\n> > index 486c000d7b77..2371ce04163f 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/sdn.hpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/sdn.hpp\n> > @@ -7,12 +7,13 @@\n> > #pragma once\n> > \n> > #include \"../algorithm.hpp\"\n> > +#include \"../denoise_algorithm.hpp\"\n> > \n> > namespace RPiController {\n> > \n> > // Algorithm to calculate correct spatial denoise (SDN) settings.\n> > \n> > -class Sdn : public Algorithm\n> > +class Sdn : public DenoiseAlgorithm\n> > {\n> > public:\n> > \tSdn(Controller *controller = NULL);\n> > @@ -20,10 +21,12 @@ public:\n> > \tvoid Read(boost::property_tree::ptree const ¶ms) override;\n> > \tvoid Initialise() override;\n> > \tvoid Prepare(Metadata *image_metadata) override;\n> > +\tvoid SetMode(DenoiseMode mode) override;\n> > \n> > private:\n> > \tdouble deviation_;\n> > \tdouble strength_;\n> > +\tDenoiseMode mode_;\n> > };\n> > \n> > } // namespace RPiController","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 D2974BD808\n\tfor ;\n\tTue, 26 Jan 2021 22:54:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A11886832B;\n\tTue, 26 Jan 2021 23:54:55 +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 EE0DA6831A\n\tfor ;\n\tTue, 26 Jan 2021 23:54:53 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7C6382C1;\n\tTue, 26 Jan 2021 23:54:53 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"jPbpYnCx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1611701693;\n\tbh=IBZhBdQLvRjkx2LnuIFvvt7EBgwRmuMeWuU7iu/C1IY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jPbpYnCxGdt2M1aq5rKDEtWfEt30RtKcxkUygY4ZPa0/ytZCrEmD61tECoE/z8yWr\n\tB6cEvj8bOmCwBdYe2F3otK0HEoJG2dLNHZBurZLmaz6SqCXnO4YZDpRrElVrTVzqau\n\tPVD2YDqlz1tJ/EcABmuvd6zl6gix1yPQJWi1//vw=","Date":"Wed, 27 Jan 2021 00:54:34 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210126162412.824033-1-naush@raspberrypi.com>\n\t<20210126162412.824033-6-naush@raspberrypi.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 5/6] ipa: raspberrypi: Add a\n\tDenoiseAlgorithm class to the Controller","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14815,"web_url":"https://patchwork.libcamera.org/comment/14815/","msgid":"","date":"2021-01-27T08:41:14","subject":"Re: [libcamera-devel] [PATCH v4 5/6] ipa: raspberrypi: Add a\n\tDenoiseAlgorithm class to the Controller","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Tue, 26 Jan 2021 at 22:54, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> On Wed, Jan 27, 2021 at 12:47:06AM +0200, Laurent Pinchart wrote:\n> > Hi Naush,\n> >\n> > Thank you for the patch.\n> >\n> > On Tue, Jan 26, 2021 at 04:24:11PM +0000, Naushir Patuck wrote:\n> > > This denoise algorithm class will be used to pass in the user requested\n> > > denoise operating mode to the controller. The existing Denoise\n> > > controller will derive from this new DenoiseAlgorithm class.\n> > >\n> > > Add a denoise mode field in the denoise status metadata object for the\n> > > IPA to use when configuring the ISP.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > Reviewed-by: David Plowman \n> > > Reviewed-by: Kieran Bingham \n> > > ---\n> > > .../controller/denoise_algorithm.hpp | 23 +++++++++++++++++++\n> > > .../raspberrypi/controller/denoise_status.h | 1 +\n> > > src/ipa/raspberrypi/controller/rpi/sdn.cpp | 11 +++++++--\n> > > src/ipa/raspberrypi/controller/rpi/sdn.hpp | 5 +++-\n> > > 4 files changed, 37 insertions(+), 3 deletions(-)\n> > > create mode 100644\n> src/ipa/raspberrypi/controller/denoise_algorithm.hpp\n> > >\n> > > diff --git a/src/ipa/raspberrypi/controller/denoise_algorithm.hpp\n> b/src/ipa/raspberrypi/controller/denoise_algorithm.hpp\n> > > new file mode 100644\n> > > index 000000000000..df1f35cc9e5f\n> > > --- /dev/null\n> > > +++ b/src/ipa/raspberrypi/controller/denoise_algorithm.hpp\n> > > @@ -0,0 +1,23 @@\n> > > +/* SPDX-License-Identifier: BSD-2-Clause */\n> > > +/*\n> > > + * Copyright (C) 2021, Raspberry Pi (Trading) Limited\n> > > + *\n> > > + * denoise.hpp - Denoise control algorithm interface\n> > > + */\n> > > +#pragma once\n> > > +\n> > > +#include \"algorithm.hpp\"\n> > > +\n> > > +namespace RPiController {\n> > > +\n> > > +enum DenoiseMode { Off, ColourOff, ColourFast, ColourHighQuality };\n> >\n> > Should we consider using enum class here ? Adding an unqualified 'Off'\n> > identifier to the namespace seems quite prone to namespace clashes.\n>\n\nYes, you are right. My first implementation did have this as an enum\nclass. I opted to change because we have to cast out to an int later on.\nBut I do prefer enum class, so I will revert back.\n\n\n> >\n> > (Additionally we usually split enums with one entry per line, but the\n> > RPi IPA doesn't use the same coding style, so it's up to you.)\n>\n> Another question, what's the difference between ColourFast and\n> ColourHighQuality ? How fast is fast, or how slow is high quality ?\n>\n\nHigh quality denoise is just what it says on the tin. It analyses and\nfilters the output image. However, this is slow in processing due to the\ncomplex filtering operations, and really only wants to be used for a stills\nimage capture. Throughput numbers are based on clock speed, but typically\nyou would be talking about a factor of 10 slower than the typical pixel\nprocessing rate.\n\nFast colour denoise analyses a 1/4 resolution image and filters out the\nfull resolution output image. This gives us the faster throughput we need\nwhen running viewfinder or video recording. Additionally, the filtering is\nless complex than the high quality mode, so is faster yet. Again, throughput\nnumbers are based on clock speed, but you would only see a typical under 2x\nslowdown over the typical pixel processing rate.\n\n\n>\n> > > +\n> > > +class DenoiseAlgorithm : public Algorithm\n> > > +{\n> > > +public:\n> > > + DenoiseAlgorithm(Controller *controller) : Algorithm(controller) {}\n> > > + // A Denoise algorithm must provide the following:\n> > > + virtual void SetMode(DenoiseMode mode) = 0;\n> > > +};\n> > > +\n> > > +} // namespace RPiController\n> > > diff --git a/src/ipa/raspberrypi/controller/denoise_status.h\n> b/src/ipa/raspberrypi/controller/denoise_status.h\n> > > index 06d7cfb91ae8..6064cc2c192e 100644\n> > > --- a/src/ipa/raspberrypi/controller/denoise_status.h\n> > > +++ b/src/ipa/raspberrypi/controller/denoise_status.h\n> > > @@ -16,6 +16,7 @@ struct DenoiseStatus {\n> > > double noise_constant;\n> > > double noise_slope;\n> > > double strength;\n> > > + unsigned int mode;\n> >\n> > Should this be of type DenoiseMode ?\n>\n\nThis is meant to be a C interoparable header file. But perhaps that does\nnot matter so much, I will have to discuss this with David. If I switch to\nenum class for DenoiseMode (as above), this will have to remain integer\nbased, or is there an alternative?\n\nRegards,\nNaush\n\n\n> >\n> > > };\n> > >\n> > > #ifdef __cplusplus\n> > > diff --git a/src/ipa/raspberrypi/controller/rpi/sdn.cpp\n> b/src/ipa/raspberrypi/controller/rpi/sdn.cpp\n> > > index d8c1521a6633..ecc7845eda4c 100644\n> > > --- a/src/ipa/raspberrypi/controller/rpi/sdn.cpp\n> > > +++ b/src/ipa/raspberrypi/controller/rpi/sdn.cpp\n> > > @@ -1,6 +1,6 @@\n> > > /* SPDX-License-Identifier: BSD-2-Clause */\n> > > /*\n> > > - * Copyright (C) 2019, Raspberry Pi (Trading) Limited\n> > > + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Limited\n> > > *\n> > > * sdn.cpp - SDN (spatial denoise) control algorithm\n> > > */\n> > > @@ -18,7 +18,7 @@ using namespace RPiController;\n> > > #define NAME \"rpi.sdn\"\n> > >\n> > > Sdn::Sdn(Controller *controller)\n> > > - : Algorithm(controller)\n> > > + : DenoiseAlgorithm(controller), mode_(DenoiseMode::ColourOff)\n> > > {\n> > > }\n> > >\n> > > @@ -48,6 +48,7 @@ void Sdn::Prepare(Metadata *image_metadata)\n> > > status.noise_constant = noise_status.noise_constant * deviation_;\n> > > status.noise_slope = noise_status.noise_slope * deviation_;\n> > > status.strength = strength_;\n> > > + status.mode = mode_;\n> > > image_metadata->Set(\"denoise.status\", status);\n> > > RPI_LOG(\"Sdn: programmed constant \" << status.noise_constant\n> > > << \" slope \" <<\n> status.noise_slope\n> > > @@ -55,6 +56,12 @@ void Sdn::Prepare(Metadata *image_metadata)\n> > > << status.strength);\n> > > }\n> > >\n> > > +void Sdn::SetMode(DenoiseMode mode)\n> > > +{\n> > > + // We only distinguish between off and all other modes.\n> > > + mode_ = mode;\n> > > +}\n> > > +\n> > > // Register algorithm with the system.\n> > > static Algorithm *Create(Controller *controller)\n> > > {\n> > > diff --git a/src/ipa/raspberrypi/controller/rpi/sdn.hpp\n> b/src/ipa/raspberrypi/controller/rpi/sdn.hpp\n> > > index 486c000d7b77..2371ce04163f 100644\n> > > --- a/src/ipa/raspberrypi/controller/rpi/sdn.hpp\n> > > +++ b/src/ipa/raspberrypi/controller/rpi/sdn.hpp\n> > > @@ -7,12 +7,13 @@\n> > > #pragma once\n> > >\n> > > #include \"../algorithm.hpp\"\n> > > +#include \"../denoise_algorithm.hpp\"\n> > >\n> > > namespace RPiController {\n> > >\n> > > // Algorithm to calculate correct spatial denoise (SDN) settings.\n> > >\n> > > -class Sdn : public Algorithm\n> > > +class Sdn : public DenoiseAlgorithm\n> > > {\n> > > public:\n> > > Sdn(Controller *controller = NULL);\n> > > @@ -20,10 +21,12 @@ public:\n> > > void Read(boost::property_tree::ptree const ¶ms) override;\n> > > void Initialise() override;\n> > > void Prepare(Metadata *image_metadata) override;\n> > > + void SetMode(DenoiseMode mode) override;\n> > >\n> > > private:\n> > > double deviation_;\n> > > double strength_;\n> > > + DenoiseMode mode_;\n> > > };\n> > >\n> > > } // namespace RPiController\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","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 63F54BD808\n\tfor ;\n\tWed, 27 Jan 2021 08:41:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B85276834E;\n\tWed, 27 Jan 2021 09:41:32 +0100 (CET)","from mail-lj1-x230.google.com (mail-lj1-x230.google.com\n\t[IPv6:2a00:1450:4864:20::230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8D7E06833C\n\tfor ;\n\tWed, 27 Jan 2021 09:41:31 +0100 (CET)","by mail-lj1-x230.google.com with SMTP id s18so1115320ljg.7\n\tfor ;\n\tWed, 27 Jan 2021 00:41:31 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"jg3m3nvV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=+07SCazjXMJxE644q2D3IzIoucNnmC0LGzFSdnUdkv8=;\n\tb=jg3m3nvVc0aXrOGR1J0z8CH++igh9et2fHlnjyXZA8ig8+qFctK0tZ552T/AWB1kJ2\n\tRMwSs6dc1z/ElbXVhbAklPXLKVZ+hBT4TcDiFo5esEJlBTimsWp4GfK9j8LoXt4qBeB9\n\tYMJcnw7qqaKzOvw4xpo5ZBcXY+i6Raz8gDYx+wR430u/d2BbOs1lfcGJnnYHUq0jntnJ\n\txWFthcK/u3aOO+8nWHO7qlmwnFoqFIBURjAqDmXFLatAfPxI2rbiWp+CMez392Exdz83\n\ta8QOGzycphL1LiD9xls5Xi8wyyQBrex5LZS2EiHZAd+QKR2+jEk0bRhPte6VTGyxmwlL\n\tG2pg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=+07SCazjXMJxE644q2D3IzIoucNnmC0LGzFSdnUdkv8=;\n\tb=CXtSFMDgcpN2jQrb9a6X+HDaBZl34bmjiURb1sc86yH9kWDvvOE53i1cWYyCtCTwLf\n\tLS8WLy+tc33gPkqiGsQtZOP6ivCJsYxgrblEwa7HHjWOzUAECBsv7OrdB2ERFDzvvsJs\n\tX/u2CpwQXc4kOMRLqkDM2tregYkJq9PpHSdlx7R+CnlaSM1L5tX0Hd5PALhnw8nir3sn\n\tjiWE4SfrWh9cchHpRCEoqSlB7TpQrvMJT+3fmxUSQMJIJ2PxFKE7uvCweRuyIsdC5zV+\n\tOXIk24DeDf38btnAF0alxrS2v/Wcg+RybXtTs+aMZqDkxLY/hiHOPidoBiPibKHZs7uU\n\tv1+A==","X-Gm-Message-State":"AOAM533UudFPrfWkxQte531gKCf7ItWnR4WYnwqsOpEnVsGoXkEBttlm\n\t8hGWMlDZum579yR1VEUHtzVau3HpNEgPleziuTCxHvIrArFRkJdD","X-Google-Smtp-Source":"ABdhPJwL9AcYRYtx6itUBy7dPxjaODp1oO6YJ4o0AIdkb9tbs+Yh0rGt6c+g6Z2/rUyHIzviM62vrktHIvGF11tyXl8=","X-Received":"by 2002:a05:651c:2108:: with SMTP id\n\ta8mr4969181ljq.329.1611736890921; \n\tWed, 27 Jan 2021 00:41:30 -0800 (PST)","MIME-Version":"1.0","References":"<20210126162412.824033-1-naush@raspberrypi.com>\n\t<20210126162412.824033-6-naush@raspberrypi.com>\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Wed, 27 Jan 2021 08:41:14 +0000","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v4 5/6] ipa: raspberrypi: Add a\n\tDenoiseAlgorithm class to the Controller","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","Cc":"libcamera devel ","Content-Type":"multipart/mixed;\n\tboundary=\"===============5803132825986295123==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]