[{"id":14694,"web_url":"https://patchwork.libcamera.org/comment/14694/","msgid":"","date":"2021-01-22T11:49:35","subject":"Re: [libcamera-devel] [PATCH v2 5/6] ipa: raspberrypi: Add a\n\tDenoiseAlgorithm class to the Controller","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Naush\n\nThanks for this patch. Don't think I have anything to add on this one!\n\nReviewed-by: David Plowman \n\nBest regards\nDavid\n\nOn Fri, 22 Jan 2021 at 09:25, Naushir Patuck wrote:\n>\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> ---\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> +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 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>\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..f85a09afb380 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::ColourFast)\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 \" << 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 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> 2.25.1\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 EC6EFC0F2A\n\tfor ;\n\tFri, 22 Jan 2021 11:49:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 71E6468245;\n\tFri, 22 Jan 2021 12:49:49 +0100 (CET)","from mail-oi1-x22f.google.com (mail-oi1-x22f.google.com\n\t[IPv6:2607:f8b0:4864:20::22f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CC03F681D8\n\tfor ;\n\tFri, 22 Jan 2021 12:49:47 +0100 (CET)","by mail-oi1-x22f.google.com with SMTP id w8so5700959oie.2\n\tfor ;\n\tFri, 22 Jan 2021 03:49:47 -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=\"sgtfCLqD\"; 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=AjGXWWbUQi1MnONu6eqdb56cbs4vzyZY4Ukryo6C7Ss=;\n\tb=sgtfCLqD5C3CqsI77oravXkAfHcrAJeokysVVm8BMmo1F5R7XPcyNThXwAeH3sS1LN\n\tSG/wz0PHBFdh8oB7pR+LNoGUuRWBZwYXx8l43kjGTTRa+iLQhVwV7wb9290zSZh5IoE1\n\t8G89fPiQ2SBoI5PurTktspXZfAEnV8cO8Ts/LQYJBpcoeOhiq1NBz1hIAUBtlUyJ0bni\n\th72yokdu6Unxlqzi6/f27jdoFcZIUVqzzZ/37J6MB0jgirHEdqzDE8PBt5iyY3EJ9Fc0\n\tjxsY8lO1RAQuWG8l4OOm+djiHf/zCV0pAq+KcGB84FEUMrbv8Pr/uDp1HkWsboHNW/NR\n\tBYXg==","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=AjGXWWbUQi1MnONu6eqdb56cbs4vzyZY4Ukryo6C7Ss=;\n\tb=TSbGWnPRSquKlqAfN+t07DFrh5VxlR7TyO3BnPn0iPQ6LSxTPDvYHpBOz8y7Vk5rwr\n\tFeaULNpN2YXhylnCmq1wXvpWbuEnJ6zF6QIYTZHfFJtLjkRxoyn8/+arzTyLccz7TO+M\n\tSrGBVWUDqoOTJlxQXSZSOEsaEkKTLHsXvB8qlhHKFkc0rolSOomZmCEFQFxAFGNfbQXf\n\t9i79pwuMjk67mvCZOT+f2/KHyfc3gd8J3PQx7KW0jfyLxkl/IU1fTMTXO1gYyYP7zaB3\n\tbNkxYFZesnLV8XO52TK6TDgKnMH5iQiE7PCJKQyjRZVei87MHZp+Bz5s5JpsDLSBOdX/\n\tOUlw==","X-Gm-Message-State":"AOAM532o9CI102VgL5uC2GZ+1pJy2r39ku5Iasm3GZeT6PV7nrNIin+k\n\tamSH4fmOluc7MVvbW5oVpN2Np3TkMWEZACCWLS3AQa54zMUGVQ==","X-Google-Smtp-Source":"ABdhPJz5K5z5uBYWGNcw0Hb1bBY11edUfTYQhIKKjb9Hua/LGUGcYGusZOD2R/rgE9T/sVwqJip+xZeJ/GwXRIqFJaI=","X-Received":"by 2002:aca:4d93:: with SMTP id\n\ta141mr2862542oib.107.1611316186541; \n\tFri, 22 Jan 2021 03:49:46 -0800 (PST)","MIME-Version":"1.0","References":"<20210122092537.708375-1-naush@raspberrypi.com>\n\t<20210122092537.708375-6-naush@raspberrypi.com>","In-Reply-To":"<20210122092537.708375-6-naush@raspberrypi.com>","From":"David Plowman ","Date":"Fri, 22 Jan 2021 11:49:35 +0000","Message-ID":"","To":"Naushir Patuck ","Subject":"Re: [libcamera-devel] [PATCH v2 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":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14695,"web_url":"https://patchwork.libcamera.org/comment/14695/","msgid":"","date":"2021-01-22T11:50:27","subject":"Re: [libcamera-devel] [PATCH v2 5/6] ipa: raspberrypi: Add a\n\tDenoiseAlgorithm class to the Controller","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Naush,\n\nOn 22/01/2021 09:25, 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> ---\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\nThere's a bit of a mix of .h and .hpp within src/ipa/raspberrypi/ but I\ndon't think that matters to much here.\n\nOtherwise, nothing controversial.\n\nReviewed-by: Kieran Bingham \n\n\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> +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> \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..f85a09afb380 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::ColourFast)\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\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 1D29AC0F2A\n\tfor ;\n\tFri, 22 Jan 2021 11:50:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E0BCF68246;\n\tFri, 22 Jan 2021 12:50:31 +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 169BD681D8\n\tfor ;\n\tFri, 22 Jan 2021 12:50:30 +0100 (CET)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 793AF4FB;\n\tFri, 22 Jan 2021 12:50:29 +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=\"fYogB3Jq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1611316229;\n\tbh=/dBsVm1Sf/ycNU408XB/PdYra7QLW04Z+h5jSi/Fv2M=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=fYogB3JqsuyVoB0cXUYs+qDraTvahRy+XfTzfbNRrtwYVt20sZ7KaidN0lMjSjr7u\n\tkzfx02zGxJM4nbq/RsSX7/IvOKwKXvuvqv+d7gDofkL3n4q8tWLBBp9mrTFwCUz5/M\n\tPxNrURQPaIxzRN5cIfjC4JCNkSgMKo+0aBHKJNUs=","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210122092537.708375-1-naush@raspberrypi.com>\n\t<20210122092537.708375-6-naush@raspberrypi.com>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Fri, 22 Jan 2021 11:50:27 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20210122092537.708375-6-naush@raspberrypi.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v2 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","Reply-To":"kieran.bingham@ideasonboard.com","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14703,"web_url":"https://patchwork.libcamera.org/comment/14703/","msgid":"","date":"2021-01-22T12:14:24","subject":"Re: [libcamera-devel] [PATCH v2 5/6] ipa: raspberrypi: Add a\n\tDenoiseAlgorithm class to the Controller","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi again Naush\n\nI know I okayed this but maybe I can raise just one more little thing? (sorry!)\n\nOn Fri, 22 Jan 2021 at 11:50, Kieran Bingham\n wrote:\n>\n> Hi Naush,\n>\n> On 22/01/2021 09:25, 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> > ---\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>\n> There's a bit of a mix of .h and .hpp within src/ipa/raspberrypi/ but I\n> don't think that matters to much here.\n>\n> Otherwise, nothing controversial.\n>\n> Reviewed-by: Kieran Bingham \n>\n>\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> > +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 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> >\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..f85a09afb380 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::ColourFast)\n\nCould we maybe default this to ColourOff? This would mean that, when\nthis patch goes in, it will make no difference to the behaviour of the\nlibcamera-apps. Otherwise I'm slightly nervous that behaviour will\nchange unexpectedly for people who are just trying those apps for the\nfirst time, until we update our apps to set this explicitly.\n\nActually I think it's a better default as folks will get the\nframerates the camera modes promise, and would have to make an active\nchoice if they're prepared to trade that for better colour denoise.\n\nOn the related subject of updating the libcamera-apps once this patch\nset it in, will you do that? :)\n\nThanks!\nDavid\n\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 \" << 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 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> --\n> Regards\n> --\n> Kieran\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 1954EC0F2A\n\tfor ;\n\tFri, 22 Jan 2021 12:14:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 841DB68260;\n\tFri, 22 Jan 2021 13:14:38 +0100 (CET)","from mail-ot1-x336.google.com (mail-ot1-x336.google.com\n\t[IPv6:2607:f8b0:4864:20::336])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D929C6824C\n\tfor ;\n\tFri, 22 Jan 2021 13:14:36 +0100 (CET)","by mail-ot1-x336.google.com with SMTP id a109so4873709otc.1\n\tfor ;\n\tFri, 22 Jan 2021 04:14:36 -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=\"M/n+8pFw\"; 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=a/Imz5cNz7b1dHVrOV5lOk0tcOFLZZsaUjyPhF806Sc=;\n\tb=M/n+8pFwb3W2HmNjXLIvybxEkQfKhkBH3DkEW9XRRpxVflT68T5zFxTeqGWrxKYG+s\n\tfXUN7cE7PATlOPB/+CXv5Ik0U8L6B83uoupx0gAijqm+TFAcwO/bcI8KouEJiQ4p7JpT\n\txQTKnpckPAnUETarDEObz1M5xmOfv4p1EPWkc1FUAvAJDXoOxYPPjGPVogmt8avNAZy5\n\tdFy6Abp5x1ZyX+bXfsIyTbFgNCDAzaeEGcKTaIhxuMj5wHCxYzrl2TkBA21O2W0wyJF0\n\tMKsfMaV5Hes8nuV2lXDz5FukL1HFJXVG9iwiTvB4orTRtHwnIIoTjLysro+N8T1rdKXs\n\tVYqg==","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=a/Imz5cNz7b1dHVrOV5lOk0tcOFLZZsaUjyPhF806Sc=;\n\tb=p2xi0vOlP6zJETstscyb+2itReP8NJvBMrZgL0kfXmAzqZEDLTd7dvPwVLRa38HMEK\n\t/7ROl+fXk+hB/QAqThF2UW9zr3mqy29sGaU+SHd9OPyiwxQ6uABfhgUmyqmFxBLUIIDj\n\t3OthZqwyIN9THbY54Z15yytqUE18/V46AUUpGYldHlj/WSAjUWNGysbp1/G56bBG7AxH\n\tDKlem/bDsm/XIilBqwaqp9YNccNkuRny051gT4/RoX1VaR2ZaqotR5bCs+U49PVAbo6W\n\tRtvTKaaNNEHzAUKMXR/7NsokQ+rGlmaztf5AmtpGdm7SXHU1KRZlO5zRkTzm733MsEMQ\n\tBIbQ==","X-Gm-Message-State":"AOAM531/5w9Y5mqqvAh4zMMmx3+ZIJ3i6CCXpOb2ul4Deexd1xspwtje\n\ttYa7YD8BItGQ7rtWZL4erMArJ6W5bjVMigRlY3E6YA==","X-Google-Smtp-Source":"ABdhPJwuDwJFa6M/EyEvy0KwVNaa6q2gV/se7bi6XiTEuVHCdY4uu/nls4vzMoEYAJCjejkvA4lA0LXPIPrB1/XWkME=","X-Received":"by 2002:a9d:6852:: with SMTP id\n\tc18mr3139884oto.166.1611317675704; \n\tFri, 22 Jan 2021 04:14:35 -0800 (PST)","MIME-Version":"1.0","References":"<20210122092537.708375-1-naush@raspberrypi.com>\n\t<20210122092537.708375-6-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Fri, 22 Jan 2021 12:14:24 +0000","Message-ID":"","To":"Kieran Bingham ","Subject":"Re: [libcamera-devel] [PATCH v2 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":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14705,"web_url":"https://patchwork.libcamera.org/comment/14705/","msgid":"","date":"2021-01-22T12:34:40","subject":"Re: [libcamera-devel] [PATCH v2 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 David,\n\nOn Fri, 22 Jan 2021 at 12:14, David Plowman \nwrote:\n\n> Hi again Naush\n>\n> I know I okayed this but maybe I can raise just one more little thing?\n> (sorry!)\n>\n> On Fri, 22 Jan 2021 at 11:50, Kieran Bingham\n> wrote:\n> >\n> > Hi Naush,\n> >\n> > On 22/01/2021 09:25, 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> > > ---\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> >\n> > There's a bit of a mix of .h and .hpp within src/ipa/raspberrypi/ but I\n> > don't think that matters to much here.\n> >\n> > Otherwise, nothing controversial.\n> >\n> > Reviewed-by: Kieran Bingham \n> >\n> >\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> > > +class DenoiseAlgorithm : public Algorithm\n> > > +{\n> > > +public:\n> > > + DenoiseAlgorithm(Controller *controller) : Algorithm(controller)\n> {}\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> > >\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..f85a09afb380 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::ColourFast)\n>\n> Could we maybe default this to ColourOff? This would mean that, when\n> this patch goes in, it will make no difference to the behaviour of the\n> libcamera-apps. Otherwise I'm slightly nervous that behaviour will\n> change unexpectedly for people who are just trying those apps for the\n> first time, until we update our apps to set this explicitly.\n>\n> Actually I think it's a better default as folks will get the\n> framerates the camera modes promise, and would have to make an active\n> choice if they're prepared to trade that for better colour denoise.\n>\n\nSure, that makes sense, I'll prepare a change for that.\n\n\n> On the related subject of updating the libcamera-apps once this patch\n> set it in, will you do that? :)\n>\n\nSure, I can have a look at that soon-ish.\n\nRegards,\nNaush\n\n\n\n>\n> Thanks!\n> David\n>\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> > --\n> > Regards\n> > --\n> > Kieran\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\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 D87BAC0F2A\n\tfor ;\n\tFri, 22 Jan 2021 12:34:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4973B68269;\n\tFri, 22 Jan 2021 13:34:59 +0100 (CET)","from mail-lj1-x234.google.com (mail-lj1-x234.google.com\n\t[IPv6:2a00:1450:4864:20::234])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D792F6824C\n\tfor ;\n\tFri, 22 Jan 2021 13:34:57 +0100 (CET)","by mail-lj1-x234.google.com with SMTP id e7so6302701ljg.10\n\tfor ;\n\tFri, 22 Jan 2021 04:34:57 -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=\"bGSnNnk/\"; 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=bMqHG2Rxig4ncmFsng+PyyDtorDOcSWmAl5poTNKKGA=;\n\tb=bGSnNnk/MMuxD0S2k3P917iaKNt8W2bjRCzlqa9HmhW4mI+ZSi48QIlvL0qGUMHOZQ\n\tHNS75YLia3MuSBYgWPo3Pn6Rc/DA1oUV+jtYEtT+ltlKMRv6kefGLT71BjY9HRq5jGRg\n\tV5SCS7aPCvnDNf6VJ4r92musi873/WhaAq109bk0FznJZdBNpQnc9jI44xWm2ltbWOU9\n\t9dnz6XjMoXi4hSUW7RsB88TwCNYyu3bIZM7b+lxsECZVxxDA6zThYdGRJ9UyAV11C2RM\n\t9DhI36XCwV45b+dC1NOS43H1gg7tD2nY7c0YPhsEJfFKHIGBFc589dw//BgOq1sPujnI\n\thpmg==","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=bMqHG2Rxig4ncmFsng+PyyDtorDOcSWmAl5poTNKKGA=;\n\tb=V/56axeS9Ktq2l1wqRCY6Z6IBePlrR8GYi255kN2Sn9YNlGwddHlZCBi1jvYFcaPU3\n\txM0EGq5bpnh+WdASuRs/Zb3mlU04+6VWEAg3x/lu2B2rRtqvvxfoRI4LhQX2hpbg4CMg\n\tEMbvqB4NDLs6i1W+w93FE/X8ijFiE+ai+smjAQ9NLVupBhemCXGaDjFaOPZ2VjJhFN8J\n\tsQuR6J4GpxObD+8V8sCb6nfYZZpvrCkUnG3qQo23s92AQa/dLm+Mr6oYjUpxfukGr4mW\n\tV4aG8TYW25tasf5hKb86bR/ALdY2Amw1A0NwBPFUmubt4I01ZzGRLCctFgYXRTblJCWu\n\tfKMg==","X-Gm-Message-State":"AOAM5316+H3NMNWCCymA7tu0phtMRuKIi8kVfAxU99Av2J08VVVfoT/F\n\tGSGn6sWaLMq6KN7+QPmwaZFqx+mdFX1+O3QhEy07qg==","X-Google-Smtp-Source":"ABdhPJwuCGpiQBKgWpOF4zbV4kKNbO3z1pglCw0kXDi9m2l8xBLTV/aWh3yaKtks1cilEfA8C52cnpEUZtjkPVGcyjI=","X-Received":"by 2002:a2e:7605:: with SMTP id r5mr373061ljc.299.1611318897239; \n\tFri, 22 Jan 2021 04:34:57 -0800 (PST)","MIME-Version":"1.0","References":"<20210122092537.708375-1-naush@raspberrypi.com>\n\t<20210122092537.708375-6-naush@raspberrypi.com>\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Fri, 22 Jan 2021 12:34:40 +0000","Message-ID":"","To":"David Plowman ","Subject":"Re: [libcamera-devel] [PATCH v2 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=\"===============7027757518001928046==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14706,"web_url":"https://patchwork.libcamera.org/comment/14706/","msgid":"","date":"2021-01-22T12:46:07","subject":"Re: [libcamera-devel] [PATCH v2 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 Kieran,\n\nThank you for your review feedback.\n\nOn Fri, 22 Jan 2021 at 11:50, Kieran Bingham <\nkieran.bingham@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> On 22/01/2021 09:25, 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> > ---\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\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>\n> There's a bit of a mix of .h and .hpp within src/ipa/raspberrypi/ but I\n> don't think that matters to much here.\n>\n\nSadly, yes there is. Our controller code uses .hpp throughout, and this\nfile really ought to follow. We do have a non-trivial task of reformatting\nall our controller code to better match with the libcamera formatting\nstyle, so we ought to add renaming .hpp to .h to that list.\n\nRegards,\nNaush\n\n\n>\n> Otherwise, nothing controversial.\n>\n> Reviewed-by: Kieran Bingham \n>\n>\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> > +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> >\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..f85a09afb380 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::ColourFast)\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> --\n> Regards\n> --\n> Kieran\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 5510EBD808\n\tfor ;\n\tFri, 22 Jan 2021 12:46:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DD1FC6825B;\n\tFri, 22 Jan 2021 13:46:27 +0100 (CET)","from mail-lf1-x129.google.com (mail-lf1-x129.google.com\n\t[IPv6:2a00:1450:4864:20::129])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7E6B46825B\n\tfor ;\n\tFri, 22 Jan 2021 13:46:26 +0100 (CET)","by mail-lf1-x129.google.com with SMTP id m22so7345833lfg.5\n\tfor ;\n\tFri, 22 Jan 2021 04:46:26 -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=\"GZiwoISL\"; 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=nySBF/XZzNeClbOdDrJjX9+C4TtbEkQddnKMyeTjsOs=;\n\tb=GZiwoISLJ3pZ6AqrRV1wg+5sGr4FryyWv6TC6XgHmtOgmkx5dFjD9Q4DJkOg36NW6y\n\tSIKlsGtAJputc10+2M9lINkmexAT40LFZKyqsdlkqfQxRxSOnAKkRCePnAKrQX3HPKtu\n\tYR6mxlAvnBuDsZ6WMC/y4P/8UnBzIEvKhxOnHOvmuysdC0ffwqcICfQa9RtFmdRl3q2L\n\tfol6LcjJuoS34AvYYZl4BhRaQanfLM/6OX4mBMlOebFQbwaQ5Q7BC8M93JMqOzrDYaSY\n\tZT35e9WKQxzFVc2R6NeQrHSAd9az5cLq/HVG3moYJl13y+b/noHAG0gT1Bgo2ygDGrf3\n\tu5kQ==","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=nySBF/XZzNeClbOdDrJjX9+C4TtbEkQddnKMyeTjsOs=;\n\tb=LRxlMvBZzpthpO7xXlGRJpy4NAaUwCEtAjzth6nVOgTiipwSYLhxShK+2aWoGTlQr1\n\tWVUv8aES7IpVdvRDOqwzG/dItYULoGw8egx/zcFXe+spzJAZuZZLU190iw72qyB+hOXy\n\t3wYJbQN8LsxcReiUAsH+Ts6toygDFK/IZv8qFr9V1aXvhJP2ALlYHmLvfCh5DRpIqMLB\n\to3uw6HPuR21mnETznZApJPZbEE2MVf4RZi3fIOmryU3EcpF0DcocGDAqyR54Pr8jdJ0t\n\tTP+QdJonwEGke7TqdPKiQN4v8EGi85QWBoy/PD1A21sWkRHcPLDfoEGnF9y1R1W8Uw3e\n\tJuiA==","X-Gm-Message-State":"AOAM5332JRZo6oTZG5JK52GkKHTriucoDL/44ian+hToamNhxXZA+hcU\n\tlktDz3Dyzomdt3GY7IbmvRFPzc6Gk5OWTabgARjNiZ8mPRc=","X-Google-Smtp-Source":"ABdhPJzf/L+woV1vuDBpPKg//kDkC3E6vp6Q50ap/kGlCGg7uitPHsqqCPglaUI6/OY2flLh3ZE2wS8us0z8DxvYHJU=","X-Received":"by 2002:a19:8805:: with SMTP id k5mr822754lfd.567.1611319584416; \n\tFri, 22 Jan 2021 04:46:24 -0800 (PST)","MIME-Version":"1.0","References":"<20210122092537.708375-1-naush@raspberrypi.com>\n\t<20210122092537.708375-6-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Fri, 22 Jan 2021 12:46:07 +0000","Message-ID":"","To":"Kieran Bingham ","Subject":"Re: [libcamera-devel] [PATCH v2 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=\"===============4635141108340189794==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]