[{"id":15070,"web_url":"https://patchwork.libcamera.org/comment/15070/","msgid":"","date":"2021-02-09T00:37:15","subject":"Re: [libcamera-devel] [PATCH v6 4/6] ipa: raspberrypi: Rename\n\tSdnStatus to DenoiseStatus","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 Mon, Feb 08, 2021 at 03:07:36PM +0000, Naushir Patuck wrote:\n> This change is in anticipation of the addition of a DenoiseAlgorithm\n> base class which the SDN class will derive from. We want to match the\n> metadata object name with the base class algorithm name.\n> \n> This renames:\n> - SdnStatus metadata object to DenoiseStatus\n> - \"sdn.status\" metadata string key to \"denoise.status\"\n> - sdn_status.h header file to denoise_status.h\n> \n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n> Reviewed-by: Kieran Bingham \n> Reviewed-by: Laurent Pinchart \n> ---\n> .../controller/{sdn_status.h => denoise_status.h} | 8 ++++----\n> src/ipa/raspberrypi/controller/rpi/sdn.cpp | 4 ++--\n> src/ipa/raspberrypi/raspberrypi.cpp | 8 ++++----\n> 3 files changed, 10 insertions(+), 10 deletions(-)\n> rename src/ipa/raspberrypi/controller/{sdn_status.h => denoise_status.h} (50%)\n> \n> diff --git a/src/ipa/raspberrypi/controller/sdn_status.h b/src/ipa/raspberrypi/controller/denoise_status.h\n> similarity index 50%\n> rename from src/ipa/raspberrypi/controller/sdn_status.h\n> rename to src/ipa/raspberrypi/controller/denoise_status.h\n> index 871e0b62af1f..94f5df200ffd 100644\n> --- a/src/ipa/raspberrypi/controller/sdn_status.h\n> +++ b/src/ipa/raspberrypi/controller/denoise_status.h\n> @@ -1,18 +1,18 @@\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_status.h - SDN (spatial denoise) control algorithm status\n> + * denoise_status.h - Denoise control algorithm status\n> */\n> #pragma once\n> \n> -// This stores the parameters required for Spatial Denoise (SDN).\n> +// This stores the parameters required for Denoise (SDN).\n\nShould we drop \"(SDN)\" ?\n\n> \n> #ifdef __cplusplus\n> extern \"C\" {\n> #endif\n> \n> -struct SdnStatus {\n> +struct DenoiseStatus {\n> \tdouble noise_constant;\n> \tdouble noise_slope;\n> \tdouble strength;\n> diff --git a/src/ipa/raspberrypi/controller/rpi/sdn.cpp b/src/ipa/raspberrypi/controller/rpi/sdn.cpp\n> index 0fad25504345..ef84a2dcbf86 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/sdn.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/sdn.cpp\n> @@ -7,8 +7,8 @@\n> \n> #include \"libcamera/internal/log.h\"\n> \n> +#include \"../denoise_status.h\"\n> #include \"../noise_status.h\"\n> -#include \"../sdn_status.h\"\n> \n> #include \"sdn.hpp\"\n> \n> @@ -49,7 +49,7 @@ void Sdn::Prepare(Metadata *image_metadata)\n> \tLOG(RPiSdn, Debug)\n> \t\t<< \"Noise profile: constant \" << noise_status.noise_constant\n> \t\t<< \" slope \" << noise_status.noise_slope;\n> -\tstruct SdnStatus status;\n> +\tstruct DenoiseStatus status;\n> \tstatus.noise_constant = noise_status.noise_constant * deviation_;\n> \tstatus.noise_slope = noise_status.noise_slope * deviation_;\n> \tstatus.strength = strength_;\n\nv5 used to rename the metadata entry here:\n\n-\timage_metadata->Set(\"sdn.status\", status);\n+\timage_metadata->Set(\"denoise.status\", status);\n\nIsn't it still needed ? I can add this back when applying.\n\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index ff14cfc4b706..e76688a7b323 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -43,13 +43,13 @@\n> #include \"contrast_algorithm.hpp\"\n> #include \"contrast_status.h\"\n> #include \"controller.hpp\"\n> +#include \"denoise_status.h\"\n> #include \"dpc_status.h\"\n> #include \"focus_status.h\"\n> #include \"geq_status.h\"\n> #include \"lux_status.h\"\n> #include \"metadata.hpp\"\n> #include \"noise_status.h\"\n> -#include \"sdn_status.h\"\n> #include \"sharpen_algorithm.hpp\"\n> #include \"sharpen_status.h\"\n> \n> @@ -110,7 +110,7 @@ private:\n> \tvoid applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls);\n> \tvoid applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls);\n> \tvoid applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls);\n> -\tvoid applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ctrls);\n> +\tvoid applyDenoise(const struct DenoiseStatus *denoiseStatus, ControlList &ctrls);\n> \tvoid applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls);\n> \tvoid applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls);\n> \tvoid applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls);\n> @@ -952,7 +952,7 @@ void IPARPi::prepareISP(unsigned int bufferId)\n> \t\tif (geqStatus)\n> \t\t\tapplyGEQ(geqStatus, ctrls);\n> \n> -\t\tSdnStatus *denoiseStatus = rpiMetadata_.GetLocked(\"sdn.status\");\n> +\t\tDenoiseStatus *denoiseStatus = rpiMetadata_.GetLocked(\"denoise.status\");\n> \t\tif (denoiseStatus)\n> \t\t\tapplyDenoise(denoiseStatus, ctrls);\n> \n> @@ -1171,7 +1171,7 @@ void IPARPi::applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls)\n> \tctrls.set(V4L2_CID_USER_BCM2835_ISP_GEQ, c);\n> }\n> \n> -void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ctrls)\n> +void IPARPi::applyDenoise(const struct DenoiseStatus *denoiseStatus, ControlList &ctrls)\n> {\n> \tbcm2835_isp_denoise denoise;\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 EF3B3BD160\n\tfor ;\n\tTue, 9 Feb 2021 00:37:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 77D646160C;\n\tTue, 9 Feb 2021 01:37: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 4D847602FF\n\tfor ;\n\tTue, 9 Feb 2021 01:37:40 +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 AAB08583;\n\tTue, 9 Feb 2021 01:37:39 +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=\"SPa7KnGC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1612831059;\n\tbh=qc/DXyENMY18vsdxsGwYCL3UyVUoL0OytxOJzAVOmwA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SPa7KnGCGEgDa6Qo1qm3nJzqI0+syvi4T3pLP18Qg4P9BF2Ew+U8t1j6qvPE9eZrn\n\trTLz0M2NnkrCP51GfCw99QBupZA2zimHo7rxQnaMN3dTXFskLldMgh/HmVZNey/Zle\n\trb1cHp0o3V5NDeVCo+QMHzZLS51p++Dn/WFuZaMY=","Date":"Tue, 9 Feb 2021 02:37:15 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210208150738.2293520-1-naush@raspberrypi.com>\n\t<20210208150738.2293520-5-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210208150738.2293520-5-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v6 4/6] ipa: raspberrypi: Rename\n\tSdnStatus to DenoiseStatus","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":15077,"web_url":"https://patchwork.libcamera.org/comment/15077/","msgid":"","date":"2021-02-09T07:30:06","subject":"Re: [libcamera-devel] [PATCH v6 4/6] ipa: raspberrypi: Rename\n\tSdnStatus to DenoiseStatus","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\n\nOn Tue, 9 Feb 2021 at 00:37, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Mon, Feb 08, 2021 at 03:07:36PM +0000, Naushir Patuck wrote:\n> > This change is in anticipation of the addition of a DenoiseAlgorithm\n> > base class which the SDN class will derive from. We want to match the\n> > metadata object name with the base class algorithm name.\n> >\n> > This renames:\n> > - SdnStatus metadata object to DenoiseStatus\n> > - \"sdn.status\" metadata string key to \"denoise.status\"\n> > - sdn_status.h header file to denoise_status.h\n> >\n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: David Plowman \n> > Reviewed-by: Kieran Bingham \n> > Reviewed-by: Laurent Pinchart \n> > ---\n> > .../controller/{sdn_status.h => denoise_status.h} | 8 ++++----\n> > src/ipa/raspberrypi/controller/rpi/sdn.cpp | 4 ++--\n> > src/ipa/raspberrypi/raspberrypi.cpp | 8 ++++----\n> > 3 files changed, 10 insertions(+), 10 deletions(-)\n> > rename src/ipa/raspberrypi/controller/{sdn_status.h =>\n> denoise_status.h} (50%)\n> >\n> > diff --git a/src/ipa/raspberrypi/controller/sdn_status.h\n> b/src/ipa/raspberrypi/controller/denoise_status.h\n> > similarity index 50%\n> > rename from src/ipa/raspberrypi/controller/sdn_status.h\n> > rename to src/ipa/raspberrypi/controller/denoise_status.h\n> > index 871e0b62af1f..94f5df200ffd 100644\n> > --- a/src/ipa/raspberrypi/controller/sdn_status.h\n> > +++ b/src/ipa/raspberrypi/controller/denoise_status.h\n> > @@ -1,18 +1,18 @@\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_status.h - SDN (spatial denoise) control algorithm status\n> > + * denoise_status.h - Denoise control algorithm status\n> > */\n> > #pragma once\n> >\n> > -// This stores the parameters required for Spatial Denoise (SDN).\n> > +// This stores the parameters required for Denoise (SDN).\n>\n> Should we drop \"(SDN)\" ?\n>\n> >\n> > #ifdef __cplusplus\n> > extern \"C\" {\n> > #endif\n> >\n> > -struct SdnStatus {\n> > +struct DenoiseStatus {\n> > double noise_constant;\n> > double noise_slope;\n> > double strength;\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/sdn.cpp\n> b/src/ipa/raspberrypi/controller/rpi/sdn.cpp\n> > index 0fad25504345..ef84a2dcbf86 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/sdn.cpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/sdn.cpp\n> > @@ -7,8 +7,8 @@\n> >\n> > #include \"libcamera/internal/log.h\"\n> >\n> > +#include \"../denoise_status.h\"\n> > #include \"../noise_status.h\"\n> > -#include \"../sdn_status.h\"\n> >\n> > #include \"sdn.hpp\"\n> >\n> > @@ -49,7 +49,7 @@ void Sdn::Prepare(Metadata *image_metadata)\n> > LOG(RPiSdn, Debug)\n> > << \"Noise profile: constant \" <<\n> noise_status.noise_constant\n> > << \" slope \" << noise_status.noise_slope;\n> > - struct SdnStatus status;\n> > + struct DenoiseStatus status;\n> > status.noise_constant = noise_status.noise_constant * deviation_;\n> > status.noise_slope = noise_status.noise_slope * deviation_;\n> > status.strength = strength_;\n>\n> v5 used to rename the metadata entry here:\n>\n> - image_metadata->Set(\"sdn.status\", status);\n> + image_metadata->Set(\"denoise.status\", status);\n>\n> Isn't it still needed ? I can add this back when applying.\n>\n\nYes, it is! Thank you for catching that.\nI'm sorry, this was finger trouble on my part. I did have this fixed\nlocally, but evidently it was staged and I didn't commit it before sending\n:-(\n\nRegards,\nNaush\n\n\n\n>\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index ff14cfc4b706..e76688a7b323 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -43,13 +43,13 @@\n> > #include \"contrast_algorithm.hpp\"\n> > #include \"contrast_status.h\"\n> > #include \"controller.hpp\"\n> > +#include \"denoise_status.h\"\n> > #include \"dpc_status.h\"\n> > #include \"focus_status.h\"\n> > #include \"geq_status.h\"\n> > #include \"lux_status.h\"\n> > #include \"metadata.hpp\"\n> > #include \"noise_status.h\"\n> > -#include \"sdn_status.h\"\n> > #include \"sharpen_algorithm.hpp\"\n> > #include \"sharpen_status.h\"\n> >\n> > @@ -110,7 +110,7 @@ private:\n> > void applyBlackLevel(const struct BlackLevelStatus\n> *blackLevelStatus, ControlList &ctrls);\n> > void applyGamma(const struct ContrastStatus *contrastStatus,\n> ControlList &ctrls);\n> > void applyGEQ(const struct GeqStatus *geqStatus, ControlList\n> &ctrls);\n> > - void applyDenoise(const struct SdnStatus *denoiseStatus,\n> ControlList &ctrls);\n> > + void applyDenoise(const struct DenoiseStatus *denoiseStatus,\n> ControlList &ctrls);\n> > void applySharpen(const struct SharpenStatus *sharpenStatus,\n> ControlList &ctrls);\n> > void applyDPC(const struct DpcStatus *dpcStatus, ControlList\n> &ctrls);\n> > void applyLS(const struct AlscStatus *lsStatus, ControlList\n> &ctrls);\n> > @@ -952,7 +952,7 @@ void IPARPi::prepareISP(unsigned int bufferId)\n> > if (geqStatus)\n> > applyGEQ(geqStatus, ctrls);\n> >\n> > - SdnStatus *denoiseStatus =\n> rpiMetadata_.GetLocked(\"sdn.status\");\n> > + DenoiseStatus *denoiseStatus =\n> rpiMetadata_.GetLocked(\"denoise.status\");\n> > if (denoiseStatus)\n> > applyDenoise(denoiseStatus, ctrls);\n> >\n> > @@ -1171,7 +1171,7 @@ void IPARPi::applyGEQ(const struct GeqStatus\n> *geqStatus, ControlList &ctrls)\n> > ctrls.set(V4L2_CID_USER_BCM2835_ISP_GEQ, c);\n> > }\n> >\n> > -void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus,\n> ControlList &ctrls)\n> > +void IPARPi::applyDenoise(const struct DenoiseStatus *denoiseStatus,\n> ControlList &ctrls)\n> > {\n> > bcm2835_isp_denoise denoise;\n> >\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 95F33BD162\n\tfor ;\n\tTue, 9 Feb 2021 07:30:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 043B361613;\n\tTue, 9 Feb 2021 08:30:27 +0100 (CET)","from mail-lf1-x131.google.com (mail-lf1-x131.google.com\n\t[IPv6:2a00:1450:4864:20::131])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4C98B613F3\n\tfor ;\n\tTue, 9 Feb 2021 08:30:25 +0100 (CET)","by mail-lf1-x131.google.com with SMTP id p21so26821069lfu.11\n\tfor ;\n\tMon, 08 Feb 2021 23:30:25 -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=\"jxFKHFr2\"; 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=kCp1CwZyOHBtaGgXdSt/326Ws3AXAtUHbHgIujC1YvM=;\n\tb=jxFKHFr2NbkfF5h1tQuIgslP5Ffav+WrANs1p8kSejZp+0QfvlVaISe1jxbHDU9LZy\n\tF1HpCfXzguUwcYYBH9SF3f0ybm3pixKj+kGIiMYR2tvBXjLGbgSh5jPa8O+qUHh0aSGT\n\tQpPdO9EcsZcm71X7872ztPLQrJS8AH61Vav5PleWz0lw2cnYQ4BYn+7KBdA+0lSkJkHm\n\ttTiNOtjIxAT6pbPhXS10sT7ID8/1CNjwygqnNXTWwdkaVJBBdEuxts1aJ0CDPQMpt6OU\n\t+gyQGKwS49JZisVQC5n79CmeXlC2C7/iYIEOjRPkk1ugi+kDYUPF93KgrGVlgWFFehJo\n\t5/1Q==","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=kCp1CwZyOHBtaGgXdSt/326Ws3AXAtUHbHgIujC1YvM=;\n\tb=QDblkvzChQ2yOEgAEMQf74NbybO7imVw8TlWMizGt0knRtdeWVXd9qImu/l4OplryI\n\tIicD/rvD3IQYDzAvr2jg2ekeYVWv8R1SlMBtZ5Ra/0x9KE70C9GEUhBZcgiSaU1z/wyQ\n\tsQvdwIfpVfDZ7bxeAYv4q1ffSOUsoYdNuvSl/xj5otbfqPxeCOWJbdeJGjSj6Hzn5J+W\n\tgWIevMppG74dL3AqMf6N8lnELVsKN0S54OsGxtV5LkWJ0CBorwrBgcKggStATixVPo4Z\n\tIyhHeerEyIqfvuyj7a9ssO/y+h//n7GqvBltlo/g7t8f4oTDthImwcjlD58d1Veg3OSA\n\tizQg==","X-Gm-Message-State":"AOAM532rIEN3qdQ7pfO0NyNOU2dCtiTSHW/pRx4WGmKoND8Tt5E9PXtf\n\tC6Fa78At2tF9aKP8b7dIs2cxHHQwcSt/0eK4uF0BZ8oe1GBgiA==","X-Google-Smtp-Source":"ABdhPJySUGlLihZCXEM4OTQGZV33nHLANZjPPpiCZqy3BB4QAsrMTwKqD7usPo1UChfyeHZ8LzN/rPin0lmqHU1WhBo=","X-Received":"by 2002:a19:ee09:: with SMTP id\n\tg9mr12972492lfb.272.1612855824527; \n\tMon, 08 Feb 2021 23:30:24 -0800 (PST)","MIME-Version":"1.0","References":"<20210208150738.2293520-1-naush@raspberrypi.com>\n\t<20210208150738.2293520-5-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Tue, 9 Feb 2021 07:30:06 +0000","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v6 4/6] ipa: raspberrypi: Rename\n\tSdnStatus to DenoiseStatus","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=\"===============1503439354939983158==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]