[{"id":14796,"web_url":"https://patchwork.libcamera.org/comment/14796/","msgid":"","date":"2021-01-26T22:52:08","subject":"Re: [libcamera-devel] [PATCH v4 6/6] ipa: raspberrypi: Handle\n\tcontrol::NoiseReductionMode in 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:12PM +0000, Naushir Patuck wrote:\n> The application provided noise reduction mode gets passed into the\n> denoise controller. The denoise controller in turn returns the mode to\n> the IPA which now sets up the colour denoise processing appropriately.\n> \n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n> Reviewed-by: Kieran Bingham \n> ---\n> src/ipa/raspberrypi/raspberrypi.cpp | 47 ++++++++++++++++++++++++++++-\n> 1 file changed, 46 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 220cf174aa4f..8f80bb80c129 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -43,6 +43,7 @@\n> #include \"contrast_algorithm.hpp\"\n> #include \"contrast_status.h\"\n> #include \"controller.hpp\"\n> +#include \"denoise_algorithm.hpp\"\n> #include \"denoise_status.h\"\n> #include \"dpc_status.h\"\n> #include \"focus_status.h\"\n> @@ -565,6 +566,7 @@ bool IPARPi::validateIspControls()\n> \t\tV4L2_CID_USER_BCM2835_ISP_SHARPEN,\n> \t\tV4L2_CID_USER_BCM2835_ISP_DPC,\n> \t\tV4L2_CID_USER_BCM2835_ISP_LENS_SHADING,\n> +\t\tV4L2_CID_USER_BCM2835_ISP_CDN,\n> \t};\n> \n> \tfor (auto c : ctrls) {\n> @@ -614,6 +616,14 @@ static const std::map AwbModeTable = {\n> \t{ controls::AwbCustom, \"custom\" },\n> };\n> \n> +static const std::map DenoiseModeTable = {\n> +\t{ controls::draft::NoiseReductionModeOff, RPiController::DenoiseMode::Off },\n> +\t{ controls::draft::NoiseReductionModeFast, RPiController::DenoiseMode::ColourFast },\n> +\t{ controls::draft::NoiseReductionModeHighQuality, RPiController::DenoiseMode::ColourHighQuality },\n> +\t{ controls::draft::NoiseReductionModeMinimal, RPiController::DenoiseMode::ColourOff },\n> +\t{ controls::draft::NoiseReductionModeZSL, RPiController::DenoiseMode::ColourHighQuality },\n> +};\n> +\n> void IPARPi::queueRequest(const ControlList &controls)\n> {\n> \t/* Clear the return metadata buffer. */\n> @@ -836,6 +846,22 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \t\t\tbreak;\n> \t\t}\n> \n> +\t\tcase controls::NOISE_REDUCTION_MODE: {\n\nI may be missing something, but you don't seem to add the control to the\nControlInfoMap in include/libcamera/ipa/raspberrypi.h, so it won't be\nexposed to applications.\n\n> +\t\t\tRPiController::DenoiseAlgorithm *sdn = dynamic_cast(\n> +\t\t\t\tcontroller_.GetAlgorithm(\"SDN\"));\n> +\t\t\tASSERT(sdn);\n> +\n> +\t\t\tint32_t idx = ctrl.second.get();\n> +\t\t\tif (DenoiseModeTable.count(idx)) {\n> +\t\t\t\tsdn->SetMode(DenoiseModeTable.at(idx));\n\n\t\t\tauto mode = DenoiseModeTable.find(idx);\n\t\t\tif (mode != DenoiseModeTable.end()) {\n\t\t\t\tsdn->SetMode(mode->second);\n\nwill avoid the double lookup.\n\n> +\t\t\t\tlibcameraMetadata_.set(controls::draft::NoiseReductionMode, idx);\n> +\t\t\t} else {\n> +\t\t\t\tLOG(IPARPI, Error) << \"Noise reduction mode \" << idx\n> +\t\t\t\t\t\t << \" not recognised\";\n> +\t\t\t}\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> \t\tdefault:\n> \t\t\tLOG(IPARPI, Warning)\n> \t\t\t\t<< \"Ctrl \" << controls::controls.at(ctrl.first)->name()\n> @@ -1087,16 +1113,35 @@ void IPARPi::applyDenoise(const struct DenoiseStatus *denoiseStatus, ControlList\n> {\n> \tbcm2835_isp_denoise denoise;\n> \n> -\tdenoise.enabled = 1;\n> +\tdenoise.enabled = denoiseStatus->mode == RPiController::DenoiseMode::Off ? 0 : 1;\n\nYou could write\n\n\tdenoise.enabled = denoiseStatus->mode != RPiController::DenoiseMode::Off;\n\n> \tdenoise.constant = denoiseStatus->noise_constant;\n> \tdenoise.slope.num = 1000 * denoiseStatus->noise_slope;\n> \tdenoise.slope.den = 1000;\n> \tdenoise.strength.num = 1000 * denoiseStatus->strength;\n> \tdenoise.strength.den = 1000;\n> \n> +\t/* Set the CDN mode to match the SDN operating mode. */\n> +\tbcm2835_isp_cdn cdn;\n> +\tswitch (denoiseStatus->mode) {\n> +\tcase RPiController::DenoiseMode::ColourFast:\n> +\t\tcdn.enabled = 1;\n> +\t\tcdn.mode = CDN_MODE_FAST;\n> +\t\tbreak;\n> +\tcase RPiController::DenoiseMode::ColourHighQuality:\n> +\t\tcdn.enabled = 1;\n> +\t\tcdn.mode = CDN_MODE_HIGH_QUALITY;\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tcdn.enabled = 0;\n> +\t}\n> +\n> \tControlValue c(Span{ reinterpret_cast(&denoise),\n> \t\t\t\t\t sizeof(denoise) });\n> \tctrls.set(V4L2_CID_USER_BCM2835_ISP_DENOISE, c);\n> +\n> +\tc = ControlValue(Span{ reinterpret_cast(&cdn),\n> +\t\t\t\t\t sizeof(cdn) });\n> +\tctrls.set(V4L2_CID_USER_BCM2835_ISP_CDN, c);\n> }\n> \n> void IPARPi::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls)","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 D47B6C0F2B\n\tfor ;\n\tTue, 26 Jan 2021 22:52:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 63D7E68329;\n\tTue, 26 Jan 2021 23:52:29 +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 5A1456831A\n\tfor ;\n\tTue, 26 Jan 2021 23:52:28 +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 BE6432C1;\n\tTue, 26 Jan 2021 23:52:27 +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=\"HPlhyUjz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1611701547;\n\tbh=OXhRJeioosaqtsBssVe9QkdiMTIdSFSfvlKxIxZ/mpo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HPlhyUjzM2ytJcnaT/8CmWkozA0mblSW76dvEDvrZVFViEMzDmAF96ZMwkCMnl/m6\n\tj7Xrj9AyOTjlzvlZ4EEeV8/AienP5esZRWycY2+7JVVGJ/kQdlwG3DhrMFyYnUYDCV\n\tstgh1jUArHhAqx342XXWMdFF9yg3voWBCdAHbKfA=","Date":"Wed, 27 Jan 2021 00:52:08 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210126162412.824033-1-naush@raspberrypi.com>\n\t<20210126162412.824033-7-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210126162412.824033-7-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v4 6/6] ipa: raspberrypi: Handle\n\tcontrol::NoiseReductionMode in 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":14813,"web_url":"https://patchwork.libcamera.org/comment/14813/","msgid":"","date":"2021-01-27T08:09:55","subject":"Re: [libcamera-devel] [PATCH v4 6/6] ipa: raspberrypi: Handle\n\tcontrol::NoiseReductionMode in the controller","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nThank you for your review feedback.\n\nOn Tue, 26 Jan 2021 at 22:52, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Tue, Jan 26, 2021 at 04:24:12PM +0000, Naushir Patuck wrote:\n> > The application provided noise reduction mode gets passed into the\n> > denoise controller. The denoise controller in turn returns the mode to\n> > the IPA which now sets up the colour denoise processing appropriately.\n> >\n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: David Plowman \n> > Reviewed-by: Kieran Bingham \n> > ---\n> > src/ipa/raspberrypi/raspberrypi.cpp | 47 ++++++++++++++++++++++++++++-\n> > 1 file changed, 46 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 220cf174aa4f..8f80bb80c129 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -43,6 +43,7 @@\n> > #include \"contrast_algorithm.hpp\"\n> > #include \"contrast_status.h\"\n> > #include \"controller.hpp\"\n> > +#include \"denoise_algorithm.hpp\"\n> > #include \"denoise_status.h\"\n> > #include \"dpc_status.h\"\n> > #include \"focus_status.h\"\n> > @@ -565,6 +566,7 @@ bool IPARPi::validateIspControls()\n> > V4L2_CID_USER_BCM2835_ISP_SHARPEN,\n> > V4L2_CID_USER_BCM2835_ISP_DPC,\n> > V4L2_CID_USER_BCM2835_ISP_LENS_SHADING,\n> > + V4L2_CID_USER_BCM2835_ISP_CDN,\n> > };\n> >\n> > for (auto c : ctrls) {\n> > @@ -614,6 +616,14 @@ static const std::map\n> AwbModeTable = {\n> > { controls::AwbCustom, \"custom\" },\n> > };\n> >\n> > +static const std::map\n> DenoiseModeTable = {\n> > + { controls::draft::NoiseReductionModeOff,\n> RPiController::DenoiseMode::Off },\n> > + { controls::draft::NoiseReductionModeFast,\n> RPiController::DenoiseMode::ColourFast },\n> > + { controls::draft::NoiseReductionModeHighQuality,\n> RPiController::DenoiseMode::ColourHighQuality },\n> > + { controls::draft::NoiseReductionModeMinimal,\n> RPiController::DenoiseMode::ColourOff },\n> > + { controls::draft::NoiseReductionModeZSL,\n> RPiController::DenoiseMode::ColourHighQuality },\n> > +};\n> > +\n> > void IPARPi::queueRequest(const ControlList &controls)\n> > {\n> > /* Clear the return metadata buffer. */\n> > @@ -836,6 +846,22 @@ void IPARPi::queueRequest(const ControlList\n> &controls)\n> > break;\n> > }\n> >\n> > + case controls::NOISE_REDUCTION_MODE: {\n>\n> I may be missing something, but you don't seem to add the control to the\n> ControlInfoMap in include/libcamera/ipa/raspberrypi.h, so it won't be\n> exposed to applications.\n>\n\nThis was on purpose - although my thinking was a bit misguided when I wrote\nthe original patch. I will add this in now.\n\n\n>\n> > + RPiController::DenoiseAlgorithm *sdn =\n> dynamic_cast(\n> > + controller_.GetAlgorithm(\"SDN\"));\n> > + ASSERT(sdn);\n> > +\n> > + int32_t idx = ctrl.second.get();\n> > + if (DenoiseModeTable.count(idx)) {\n> > + sdn->SetMode(DenoiseModeTable.at(idx));\n>\n> auto mode = DenoiseModeTable.find(idx);\n> if (mode != DenoiseModeTable.end()) {\n> sdn->SetMode(mode->second);\n>\n> will avoid the double lookup.\n>\n\nAck.\n\n\n>\n> > +\n> libcameraMetadata_.set(controls::draft::NoiseReductionMode, idx);\n> > + } else {\n> > + LOG(IPARPI, Error) << \"Noise reduction\n> mode \" << idx\n> > + << \" not recognised\";\n> > + }\n> > + break;\n> > + }\n> > +\n> > default:\n> > LOG(IPARPI, Warning)\n> > << \"Ctrl \" << controls::controls.at\n> (ctrl.first)->name()\n> > @@ -1087,16 +1113,35 @@ void IPARPi::applyDenoise(const struct\n> DenoiseStatus *denoiseStatus, ControlList\n> > {\n> > bcm2835_isp_denoise denoise;\n> >\n> > - denoise.enabled = 1;\n> > + denoise.enabled = denoiseStatus->mode ==\n> RPiController::DenoiseMode::Off ? 0 : 1;\n>\n> You could write\n>\n> denoise.enabled = denoiseStatus->mode !=\n> RPiController::DenoiseMode::Off;\n>\n\nAck\n\n\n>\n> > denoise.constant = denoiseStatus->noise_constant;\n> > denoise.slope.num = 1000 * denoiseStatus->noise_slope;\n> > denoise.slope.den = 1000;\n> > denoise.strength.num = 1000 * denoiseStatus->strength;\n> > denoise.strength.den = 1000;\n> >\n> > + /* Set the CDN mode to match the SDN operating mode. */\n> > + bcm2835_isp_cdn cdn;\n> > + switch (denoiseStatus->mode) {\n> > + case RPiController::DenoiseMode::ColourFast:\n> > + cdn.enabled = 1;\n> > + cdn.mode = CDN_MODE_FAST;\n> > + break;\n> > + case RPiController::DenoiseMode::ColourHighQuality:\n> > + cdn.enabled = 1;\n> > + cdn.mode = CDN_MODE_HIGH_QUALITY;\n> > + break;\n> > + default:\n> > + cdn.enabled = 0;\n> > + }\n> > +\n> > ControlValue c(Span{ reinterpret_cast *>(&denoise),\n> > sizeof(denoise) });\n> > ctrls.set(V4L2_CID_USER_BCM2835_ISP_DENOISE, c);\n> > +\n> > + c = ControlValue(Span{ reinterpret_cast *>(&cdn),\n> > + sizeof(cdn) });\n> > + ctrls.set(V4L2_CID_USER_BCM2835_ISP_CDN, c);\n> > }\n> >\n> > void IPARPi::applySharpen(const struct SharpenStatus *sharpenStatus,\n> ControlList &ctrls)\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 C327ABD808\n\tfor ;\n\tWed, 27 Jan 2021 08:10:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2241968346;\n\tWed, 27 Jan 2021 09:10:14 +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 10B456030C\n\tfor ;\n\tWed, 27 Jan 2021 09:10:11 +0100 (CET)","by mail-lf1-x131.google.com with SMTP id a8so1399662lfi.8\n\tfor ;\n\tWed, 27 Jan 2021 00:10:11 -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=\"mfDDYqtx\"; 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=kVpMSE30WmEtvZAEcbvyP9affgMKdG0HgewPsrgVU4w=;\n\tb=mfDDYqtxFOW6dTeD9xFu852TNIC2VscnmGSbUCMxuj1LaRbq8B5w+0V+7VEZMLl0XS\n\tslQoXtWd8MXC38ZSnj2nr9ZjRtkeryWGvxmLUoU5H8aIkxEK2DqtodkO9Vrx/qZRjezh\n\tp6wPP63fNqH5XQD6cpwqi7GQWbV0saoBNJ8e1TO3yNn5yw9Cs6uvGSygqGbse5wfvAFl\n\tyw2enFgaqgXLDlYZmOnKJ27Zei00UXnavROvL5VB0t0hYar1i9Utqut3uv23VpRp5s/M\n\tKii56A24m6bjbnsZp83nzSTxhujkqbIWx/nAcJeVkxz6dP9qOJxYw+YoKm2EgB8W6nd3\n\tihew==","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=kVpMSE30WmEtvZAEcbvyP9affgMKdG0HgewPsrgVU4w=;\n\tb=sRSIUprvSALWmqwjATew3HwI2uwpjaej0ENwDnNqEWZg3xpzzHqKuu4/LDCzh8YcZk\n\t9zjsbtiayrX8kq9GlBh5Ib6AyrR8AiAdc1iMFaZAz6+vGQ3crm6glOZ73QiRLbbBmf/P\n\tI1M5UDIFJFDd+a2518U4szjS6Zz6bCWLlsLBFZ7jbD/N9VcYNtdcoDZ69XzsRQ2oVp5j\n\tu2eAx6ngZdLNUe3cj1duDWa7cAjH9Z0qQsAWw6Ybb+wZlgdPl/24qo4/CmDYPVJ76+YD\n\tP2YV0WewpTjoVCYHQgCJFco+f+cCLnl7DfKarrssVTdzAmLIFFW9Wdg4DrUUrJgiqzLW\n\tcLxg==","X-Gm-Message-State":"AOAM532OepZ2cE30Tjlk4URvBcU20zVR8khwGfyk1lQAxmRK8jN3/DyS\n\ttcUFHqaqFDoCMJ1d9m9P59bWlG63/PNXSrV74iDwx4DeXuzhsg==","X-Google-Smtp-Source":"ABdhPJwqg40E+y4Pr3V27CVsunLY0W5s7XAh78fu3p2PQvgvKOJFJrPO0AgfbZP/2fJqBWnE9waFiyf3yzATlZuKa20=","X-Received":"by 2002:ac2:5f5b:: with SMTP id 27mr4474831lfz.375.1611735011203;\n\tWed, 27 Jan 2021 00:10:11 -0800 (PST)","MIME-Version":"1.0","References":"<20210126162412.824033-1-naush@raspberrypi.com>\n\t<20210126162412.824033-7-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Wed, 27 Jan 2021 08:09:55 +0000","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v4 6/6] ipa: raspberrypi: Handle\n\tcontrol::NoiseReductionMode in 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=\"===============5775523683279083984==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]