[{"id":14629,"web_url":"https://patchwork.libcamera.org/comment/14629/","msgid":"","date":"2021-01-20T10:29:32","subject":"Re: [libcamera-devel] [PATCH 5/5] ipa: raspberrypi: Handle\n\tcontrol::NoiseReductionMode in 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 (and the previous one, namely 4/5). I'm afraid\nI'd quite like to have just a small meta-discussion about this one\nfirst. Only a small one, though!\n\nReading these patches it seems like an application setting\nNoiseReductionModeOff will still get regular spatial denoise, just not\ncolour denoise. Is that right? Under such circumstances I think it\nwould be clearer to rename DenoiseMode as ColourDenoiseMode.\n\nHaving said that, if we're going to support NoiseReductionModeOff,\nthen we should perhaps make it disable all denoising? I suspect folks\nwould find it \"surprising\" if it didn't do that. As such, maybe\nDenoiseMode would remain as DenoiseMode (not ColourDenoiseMode) and\nwe'd have 4 values, namely:\n\nDenoiseMode::Off - really everything off\nDenoiseMode::ColourOff - colour denoise off but regular spatial denoise on\nDenoiseMode::ColorFast - with fast colour denoise\nDenoiseMode::ColorHighQuality - with slow colour denoise\n\n(Setting the denoise strength to zero will disable spatial denoise.)\n\nWhat do you think?\n\nOn Wed, 20 Jan 2021 at 08:35, Naushir Patuck wrote:\n>\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> ---\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 8af3d603a3ff..91041fa68930 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 \"dpc_status.h\"\n> #include \"focus_status.h\"\n> #include \"geq_status.h\"\n> @@ -553,7 +554,8 @@ bool IPARPi::validateIspControls()\n> V4L2_CID_USER_BCM2835_ISP_DENOISE,\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_LENS_SHADING,\n> + V4L2_CID_USER_BCM2835_ISP_CDN,\n> };\n>\n> for (auto c : ctrls) {\n> @@ -603,6 +605,14 @@ static const std::map AwbModeTable = {\n> { controls::AwbCustom, \"custom\" },\n> };\n>\n> +static const std::map DenoiseModeTable = {\n> + { controls::draft::NoiseReductionModeOff, RPiController::DenoiseMode::Off },\n> + { controls::draft::NoiseReductionModeFast, RPiController::DenoiseMode::Fast },\n> + { controls::draft::NoiseReductionModeHighQuality, RPiController::DenoiseMode::HighQuality },\n> + { controls::draft::NoiseReductionModeMinimal, RPiController::DenoiseMode::Fast },\n\nIf we go with a revised definition of DenoiseMode, maybe this one\nwould be DenoiseMode::ColourOff?\n\nAnyway, thanks for all this work. Best regards\nDavid\n\n> + { controls::draft::NoiseReductionModeZSL, RPiController::DenoiseMode::HighQuality },\n> +};\n> +\n> void IPARPi::queueRequest(const ControlList &controls)\n> {\n> /* Clear the return metadata buffer. */\n> @@ -806,6 +816,22 @@ void IPARPi::queueRequest(const ControlList &controls)\n> break;\n> }\n>\n> + case controls::NOISE_REDUCTION_MODE: {\n> + RPiController::DenoiseAlgorithm *sdn = 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> + libcameraMetadata_.set(controls::draft::NoiseReductionMode, idx);\n> + } else {\n> + LOG(IPARPI, Error) << \"Noise reduction mode \" << idx\n> + << \" not recognised\";\n> + }\n> + break;\n> + }\n> +\n> default:\n> LOG(IPARPI, Warning)\n> << \"Ctrl \" << controls::controls.at(ctrl.first)->name()\n> @@ -1052,9 +1078,28 @@ void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ct\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::Fast:\n> + cdn.enabled = 1;\n> + cdn.mode = CDN_MODE_FAST;\n> + break;\n> + case RPiController::DenoiseMode::HighQuality:\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, ControlList &ctrls)\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 D7B47C0F2A\n\tfor ;\n\tWed, 20 Jan 2021 10:29:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9DB3B68196;\n\tWed, 20 Jan 2021 11:29:46 +0100 (CET)","from mail-oi1-x22e.google.com (mail-oi1-x22e.google.com\n\t[IPv6:2607:f8b0:4864:20::22e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EE2E16817D\n\tfor ;\n\tWed, 20 Jan 2021 11:29:44 +0100 (CET)","by mail-oi1-x22e.google.com with SMTP id x71so8699394oia.9\n\tfor ;\n\tWed, 20 Jan 2021 02:29:44 -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=\"NZrxU/66\"; 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=PC8qfzXm6AK15So1xjgz0lLHU8D4cPxft1CRXKemN4E=;\n\tb=NZrxU/66/goROEPYqfOPu7bXDMSxygqM6sj3t1oGP8J+lb37z1cFA+ikC0PySiPm0Y\n\tZ5bNK0wsaSQZYx2966w9S5Q12FUss3AXvTnO/0X6KE4r1+17dSX8hJT6yCVwOd9ZikGr\n\tjJ+DNjcdLRN30Ux1YCkIabQ9UaJLNZNSVd5X0H+mhKPjIKbMXWm9Ho+s/z+hYtc2CYq3\n\tY2pdMnrSgJnT9RXX0/FSA7xNz75Y0Lyicyf7FTgfYc/+7BnLxvL5AlA4f0SgzN0AW5NL\n\th8JVnqWvThUdDZvGbt32l3DdAeYehaTwmIJH4khFrDMZLgxeIfwtPRZ2pEoz7HCUssyR\n\tYIPw==","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=PC8qfzXm6AK15So1xjgz0lLHU8D4cPxft1CRXKemN4E=;\n\tb=tDaM0XHXSJ2ImJ4xS5JO1fbM+YwIpVyGQuaFupDUz/JhIGzhHQaF8r9+xwZyQJgie8\n\t0FQvcVRGuVi4ZB3thqxUNXu34sJvo9pptrjbUdtv3gKwsW46cUGGvMBpJNgIBAlBFmP/\n\tJRYpcNcwRpFBO9vCVT33fMHY7E+eowI1u4DH+Y1gE0R4mg65cwk1koEnkqUZZlm09zJh\n\txHswcqsgEApKTr+VjpEC6/H5lBQNiParXbE9yUP5w9CxFLKJ6/DDgwj9ajCBjWui7peT\n\tAhCY+Sg96WsKxYKqsrqzGiYuxxy2mfUVWnG0Op/HDc4aYjvplHC25YPE7/WrZxJlijpv\n\tdUCw==","X-Gm-Message-State":"AOAM532aEBPKAQr6vwlDUAxgdF7V9cT6Jh86NXpd9c9gCtcjr/2M28gs\n\t7gDNbzLhZCUchZxPLtPDeEkGnyOgoQM5gmX8LNQS1g==","X-Google-Smtp-Source":"ABdhPJzon5y7vDGo6hI+E06AbYLwUw4RbGDl8b9i26m0Z5Wy/REBs56F7+QRYYfm8daDGNnocgdNnsDsydDI0R1TU/U=","X-Received":"by 2002:a05:6808:8cb:: with SMTP id\n\tk11mr2400951oij.22.1611138583751; \n\tWed, 20 Jan 2021 02:29:43 -0800 (PST)","MIME-Version":"1.0","References":"<20210120083449.642418-1-naush@raspberrypi.com>\n\t<20210120083449.642418-6-naush@raspberrypi.com>","In-Reply-To":"<20210120083449.642418-6-naush@raspberrypi.com>","From":"David Plowman ","Date":"Wed, 20 Jan 2021 10:29:32 +0000","Message-ID":"","To":"Naushir Patuck ","Subject":"Re: [libcamera-devel] [PATCH 5/5] 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":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14630,"web_url":"https://patchwork.libcamera.org/comment/14630/","msgid":"","date":"2021-01-20T10:58:12","subject":"Re: [libcamera-devel] [PATCH 5/5] 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 David,\n\n\nOn Wed, 20 Jan 2021 at 10:29, David Plowman \nwrote:\n\n> Hi Naush\n>\n> Thanks for this patch (and the previous one, namely 4/5). I'm afraid\n> I'd quite like to have just a small meta-discussion about this one\n> first. Only a small one, though!\n>\n> Reading these patches it seems like an application setting\n> NoiseReductionModeOff will still get regular spatial denoise, just not\n> colour denoise. Is that right? Under such circumstances I think it\n> would be clearer to rename DenoiseMode as ColourDenoiseMode.\n>\n> Having said that, if we're going to support NoiseReductionModeOff,\n> then we should perhaps make it disable all denoising? I suspect folks\n> would find it \"surprising\" if it didn't do that. As such, maybe\n> DenoiseMode would remain as DenoiseMode (not ColourDenoiseMode) and\n> we'd have 4 values, namely:\n>\n> DenoiseMode::Off - really everything off\n> DenoiseMode::ColourOff - colour denoise off but regular spatial denoise on\n> DenoiseMode::ColorFast - with fast colour denoise\n> DenoiseMode::ColorHighQuality - with slow colour denoise\n>\n(Setting the denoise strength to zero will disable spatial denoise.)\n>\n> What do you think?\n>\n\nYes, I think this is a reasonable thing to do. I will make the appropriate\nchanges in the next revisions.\nTo disable SDN, would it be better to set the \"enabled\" field in\nthe bcm2835_isp_denoise struct to 0 over setting the strength to 0?\n\nRegards,\nNaush\n\n\n\n>\n> On Wed, 20 Jan 2021 at 08:35, Naushir Patuck \n> wrote:\n> >\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> > ---\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 8af3d603a3ff..91041fa68930 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 \"dpc_status.h\"\n> > #include \"focus_status.h\"\n> > #include \"geq_status.h\"\n> > @@ -553,7 +554,8 @@ bool IPARPi::validateIspControls()\n> > V4L2_CID_USER_BCM2835_ISP_DENOISE,\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_LENS_SHADING,\n> > + V4L2_CID_USER_BCM2835_ISP_CDN,\n> > };\n> >\n> > for (auto c : ctrls) {\n> > @@ -603,6 +605,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::Fast },\n> > + { controls::draft::NoiseReductionModeHighQuality,\n> RPiController::DenoiseMode::HighQuality },\n> > + { controls::draft::NoiseReductionModeMinimal,\n> RPiController::DenoiseMode::Fast },\n>\n> If we go with a revised definition of DenoiseMode, maybe this one\n> would be DenoiseMode::ColourOff?\n>\n> Anyway, thanks for all this work. Best regards\n> David\n>\n> > + { controls::draft::NoiseReductionModeZSL,\n> RPiController::DenoiseMode::HighQuality },\n> > +};\n> > +\n> > void IPARPi::queueRequest(const ControlList &controls)\n> > {\n> > /* Clear the return metadata buffer. */\n> > @@ -806,6 +816,22 @@ void IPARPi::queueRequest(const ControlList\n> &controls)\n> > break;\n> > }\n> >\n> > + case controls::NOISE_REDUCTION_MODE: {\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> 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> > @@ -1052,9 +1078,28 @@ void IPARPi::applyDenoise(const struct SdnStatus\n> *denoiseStatus, ControlList &ct\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::Fast:\n> > + cdn.enabled = 1;\n> > + cdn.mode = CDN_MODE_FAST;\n> > + break;\n> > + case RPiController::DenoiseMode::HighQuality:\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> > 2.25.1\n> >\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 2601FC0F2A\n\tfor ;\n\tWed, 20 Jan 2021 10:58:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 854136819F;\n\tWed, 20 Jan 2021 11:58:30 +0100 (CET)","from mail-lj1-x233.google.com (mail-lj1-x233.google.com\n\t[IPv6:2a00:1450:4864:20::233])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 004D76817D\n\tfor ;\n\tWed, 20 Jan 2021 11:58:28 +0100 (CET)","by mail-lj1-x233.google.com with SMTP id p13so25597996ljg.2\n\tfor ;\n\tWed, 20 Jan 2021 02:58:28 -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=\"X4/zLGtQ\"; 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=pujPSsz9LTM3TidfwxoSbRVoEoQBJ/TDfrUV0cxXbg0=;\n\tb=X4/zLGtQKy7hb0GqVVooyCfm05K72T4T3Xg1Gk58EaSE5lnAKx60ucwGFRbVFHFTxa\n\tc3ldjaDyDo7HLw4VsrugviNsplZk3Th4S4LJHAY8BNo6gBEGq1ncfXUaTCYV23WBLj43\n\tZWfvQMKGPrvP99NvSxgmRUrKNQ8A2/1X5fu9Z4G9VUuHXYlTV5UJZrrxTHQTG8kENy6f\n\tlLRyzyhPrjVBUuG87QlDT4JG7zae5Pk4bXxGX44/Mg5gioh9E4tampzJLX1IFoWyJ6Xg\n\tTOvxJPNDM+eeAopHKwE+HOCN+5lG4zvx1f85gVxJn557SjoWzomCVPkvNk2SwzUSL9Oc\n\twatg==","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=pujPSsz9LTM3TidfwxoSbRVoEoQBJ/TDfrUV0cxXbg0=;\n\tb=PKs1ZJ/pvz8hTZZQJ3bID3V2JVSPMneekNp3DPNgX1BXPsoK1D6JiWrhFHPlExknCI\n\tgrlk1wNWIZlxH4Iso9okleA1BrlOTmPC7woQKwzRDhaD8nTfJTyGAil7wiCJdOX3ww7h\n\tdatoxTraO9k36XBrt5wiWxYji71GaDaUzcRWLq3JjUvsTuSwsp4AbdxUY2YCEM0s5LoW\n\t6Yry9NrrPf7ZcgmSBBSZsHGMSto9u/nAI4h6V6r3N0/b3roCH8nrrowr7MpTiDqBKa2F\n\td7WIDPr8QnsOxkdCyFY6brIRt3d4ytrdPw8+5/+M9ExPbp6P4KniQtWgipLF1KCjjfaZ\n\tU7ow==","X-Gm-Message-State":"AOAM53125lMEv4iAmPF5a5fk9x69CteGAtfJcB4fViWfKF97QivdgBcC\n\tdKBpYLjWHiavbD3uA7heOwzbVGDwBOFaflJ0uP0hBNgE0HayjQ==","X-Google-Smtp-Source":"ABdhPJw/UIy0A/ifDahpKPSw9J8lbHmaI7Dz6FVqlrvOJxEoHXrgERlVbD6LHkx324sMbddm3P1d08vNyNDSI/9yZoM=","X-Received":"by 2002:a05:651c:2108:: with SMTP id\n\ta8mr4033159ljq.329.1611140308411; \n\tWed, 20 Jan 2021 02:58:28 -0800 (PST)","MIME-Version":"1.0","References":"<20210120083449.642418-1-naush@raspberrypi.com>\n\t<20210120083449.642418-6-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Wed, 20 Jan 2021 10:58:12 +0000","Message-ID":"","To":"David Plowman ","Subject":"Re: [libcamera-devel] [PATCH 5/5] 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=\"===============1424225994538203456==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14632,"web_url":"https://patchwork.libcamera.org/comment/14632/","msgid":"","date":"2021-01-20T11:16:01","subject":"Re: [libcamera-devel] [PATCH 5/5] ipa: raspberrypi: Handle\n\tcontrol::NoiseReductionMode in 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\nOn Wed, 20 Jan 2021 at 10:58, Naushir Patuck wrote:\n>\n> Hi David,\n>\n>\n> On Wed, 20 Jan 2021 at 10:29, David Plowman wrote:\n>>\n>> Hi Naush\n>>\n>> Thanks for this patch (and the previous one, namely 4/5). I'm afraid\n>> I'd quite like to have just a small meta-discussion about this one\n>> first. Only a small one, though!\n>>\n>> Reading these patches it seems like an application setting\n>> NoiseReductionModeOff will still get regular spatial denoise, just not\n>> colour denoise. Is that right? Under such circumstances I think it\n>> would be clearer to rename DenoiseMode as ColourDenoiseMode.\n>>\n>> Having said that, if we're going to support NoiseReductionModeOff,\n>> then we should perhaps make it disable all denoising? I suspect folks\n>> would find it \"surprising\" if it didn't do that. As such, maybe\n>> DenoiseMode would remain as DenoiseMode (not ColourDenoiseMode) and\n>> we'd have 4 values, namely:\n>>\n>> DenoiseMode::Off - really everything off\n>> DenoiseMode::ColourOff - colour denoise off but regular spatial denoise on\n>> DenoiseMode::ColorFast - with fast colour denoise\n>> DenoiseMode::ColorHighQuality - with slow colour denoise\n>>\n>> (Setting the denoise strength to zero will disable spatial denoise.)\n>>\n>> What do you think?\n>\n>\n> Yes, I think this is a reasonable thing to do. I will make the appropriate changes in the next revisions.\n> To disable SDN, would it be better to set the \"enabled\" field in the bcm2835_isp_denoise struct to 0 over setting the strength to 0?\n\nWhichever you prefer, I don't think it should matter. Famous last words!! :)\n\nBest regards\nDavid\n\n>\n> Regards,\n> Naush\n>\n>\n>>\n>>\n>> On Wed, 20 Jan 2021 at 08:35, Naushir Patuck wrote:\n>> >\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>> > ---\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 8af3d603a3ff..91041fa68930 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 \"dpc_status.h\"\n>> > #include \"focus_status.h\"\n>> > #include \"geq_status.h\"\n>> > @@ -553,7 +554,8 @@ bool IPARPi::validateIspControls()\n>> > V4L2_CID_USER_BCM2835_ISP_DENOISE,\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_LENS_SHADING,\n>> > + V4L2_CID_USER_BCM2835_ISP_CDN,\n>> > };\n>> >\n>> > for (auto c : ctrls) {\n>> > @@ -603,6 +605,14 @@ static const std::map AwbModeTable = {\n>> > { controls::AwbCustom, \"custom\" },\n>> > };\n>> >\n>> > +static const std::map DenoiseModeTable = {\n>> > + { controls::draft::NoiseReductionModeOff, RPiController::DenoiseMode::Off },\n>> > + { controls::draft::NoiseReductionModeFast, RPiController::DenoiseMode::Fast },\n>> > + { controls::draft::NoiseReductionModeHighQuality, RPiController::DenoiseMode::HighQuality },\n>> > + { controls::draft::NoiseReductionModeMinimal, RPiController::DenoiseMode::Fast },\n>>\n>> If we go with a revised definition of DenoiseMode, maybe this one\n>> would be DenoiseMode::ColourOff?\n>>\n>> Anyway, thanks for all this work. Best regards\n>> David\n>>\n>> > + { controls::draft::NoiseReductionModeZSL, RPiController::DenoiseMode::HighQuality },\n>> > +};\n>> > +\n>> > void IPARPi::queueRequest(const ControlList &controls)\n>> > {\n>> > /* Clear the return metadata buffer. */\n>> > @@ -806,6 +816,22 @@ void IPARPi::queueRequest(const ControlList &controls)\n>> > break;\n>> > }\n>> >\n>> > + case controls::NOISE_REDUCTION_MODE: {\n>> > + RPiController::DenoiseAlgorithm *sdn = 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>> > + libcameraMetadata_.set(controls::draft::NoiseReductionMode, idx);\n>> > + } else {\n>> > + LOG(IPARPI, Error) << \"Noise reduction mode \" << idx\n>> > + << \" not recognised\";\n>> > + }\n>> > + break;\n>> > + }\n>> > +\n>> > default:\n>> > LOG(IPARPI, Warning)\n>> > << \"Ctrl \" << controls::controls.at(ctrl.first)->name()\n>> > @@ -1052,9 +1078,28 @@ void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ct\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::Fast:\n>> > + cdn.enabled = 1;\n>> > + cdn.mode = CDN_MODE_FAST;\n>> > + break;\n>> > + case RPiController::DenoiseMode::HighQuality:\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, ControlList &ctrls)\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 A52FBBD808\n\tfor ;\n\tWed, 20 Jan 2021 11:16:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 209156819D;\n\tWed, 20 Jan 2021 12:16:15 +0100 (CET)","from mail-oi1-x234.google.com (mail-oi1-x234.google.com\n\t[IPv6:2607:f8b0:4864:20::234])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 463706811D\n\tfor ;\n\tWed, 20 Jan 2021 12:16:13 +0100 (CET)","by mail-oi1-x234.google.com with SMTP id x71so8815228oia.9\n\tfor ;\n\tWed, 20 Jan 2021 03:16:13 -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=\"X3G15VLa\"; 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=/DpKayvxBONCnKR1tRi+hZq0IGVk6b0mvi0UkamNT1Y=;\n\tb=X3G15VLav1JZJD3oQgXvrLmGJ/HZIWlzLtUe6RhlPp+dA3zeM9z1HuhlpIAqQTI6hd\n\taoQgSeJDSqg8yepmt7bpUOprhDgXEfwgb2GvZjjkYyxvfvtf+/2UN+ZGhNxewzn90RnL\n\t5BWpdf5iVHUh35AU+SsJTjToc5oGPICi4PtpGVaBsype1uvf0GWikMsCbAWwEUeVvVlr\n\tsnJlnz8RAQqo1Ssa5oJuqrTePsVP8VFYwFYxuCc1lJcFSs42vI0FEwOv0sAfVzz4gDSG\n\tWb1cyXsbNwj3Ll5DrZIXwZ4B7bqOc0QemvGzSeMm5vACKUMprDgeeefRRJ9fIDUmyjrG\n\tIfqg==","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=/DpKayvxBONCnKR1tRi+hZq0IGVk6b0mvi0UkamNT1Y=;\n\tb=Z4lHGo+LJYda64k7DgViukOUcIorLG8ZG0+21GJzc8926phclqDDomKoZPcLMsuwzq\n\tdIFni+FI4VKG901KAL4Wn320GCpxAWFShlDKmu9bu+QU4sv3VGNvFRkBZ9q6mz+QhsG/\n\tRNn5QcU+/qOEKQVJtvRd58pot3YAqC1dpsGNeneFUXWyw6r9t/8Mj5csmxsACtJ/I0jZ\n\tk7Vmw42Pc61h0nDj48GJaoMWfE0zVSkoILwjYvW6GK4cwO5aCDGGW9zsUZTE6biRW8D7\n\t3bV3TzqMWjHjNNHxLpLDwtyDuoj4C+3qJBH2XqZI1m5xer5U5f64uLlK1nOZ110KKFEh\n\t511A==","X-Gm-Message-State":"AOAM532gpRZwAYROfPBV59A/gy6QhItcRtfPt0PzJ5cQvRd2rNSL/aT+\n\tKUHIB0Z8RrbQBqrsJ24ohNhS0zO5Xfv0qj1PLvGb7vTC2ShjlQ==","X-Google-Smtp-Source":"ABdhPJzVGb1WHUcq9PIsvcPlY6uQlq0sjDq9qQr2/79KvhoKsrXoTaDzLtfzc1TRpE7R5waV5JuZRR8qixLdec0/Jco=","X-Received":"by 2002:aca:55c6:: with SMTP id\n\tj189mr2412897oib.107.1611141372028; \n\tWed, 20 Jan 2021 03:16:12 -0800 (PST)","MIME-Version":"1.0","References":"<20210120083449.642418-1-naush@raspberrypi.com>\n\t<20210120083449.642418-6-naush@raspberrypi.com>\n\t\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Wed, 20 Jan 2021 11:16:01 +0000","Message-ID":"","To":"Naushir Patuck ","Subject":"Re: [libcamera-devel] [PATCH 5/5] 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":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14633,"web_url":"https://patchwork.libcamera.org/comment/14633/","msgid":"","date":"2021-01-20T11:20:58","subject":"Re: [libcamera-devel] [PATCH 5/5] 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":"On Wed, 20 Jan 2021 at 11:16, David Plowman \nwrote:\n\n> Hi Naush\n>\n> On Wed, 20 Jan 2021 at 10:58, Naushir Patuck \n> wrote:\n> >\n> > Hi David,\n> >\n> >\n> > On Wed, 20 Jan 2021 at 10:29, David Plowman <\n> david.plowman@raspberrypi.com> wrote:\n> >>\n> >> Hi Naush\n> >>\n> >> Thanks for this patch (and the previous one, namely 4/5). I'm afraid\n> >> I'd quite like to have just a small meta-discussion about this one\n> >> first. Only a small one, though!\n> >>\n> >> Reading these patches it seems like an application setting\n> >> NoiseReductionModeOff will still get regular spatial denoise, just not\n> >> colour denoise. Is that right? Under such circumstances I think it\n> >> would be clearer to rename DenoiseMode as ColourDenoiseMode.\n> >>\n> >> Having said that, if we're going to support NoiseReductionModeOff,\n> >> then we should perhaps make it disable all denoising? I suspect folks\n> >> would find it \"surprising\" if it didn't do that. As such, maybe\n> >> DenoiseMode would remain as DenoiseMode (not ColourDenoiseMode) and\n> >> we'd have 4 values, namely:\n> >>\n> >> DenoiseMode::Off - really everything off\n> >> DenoiseMode::ColourOff - colour denoise off but regular spatial denoise\n> on\n> >> DenoiseMode::ColorFast - with fast colour denoise\n> >> DenoiseMode::ColorHighQuality - with slow colour denoise\n> >>\n> >> (Setting the denoise strength to zero will disable spatial denoise.)\n> >>\n> >> What do you think?\n> >\n> >\n> > Yes, I think this is a reasonable thing to do. I will make the\n> appropriate changes in the next revisions.\n> > To disable SDN, would it be better to set the \"enabled\" field in the\n> bcm2835_isp_denoise struct to 0 over setting the strength to 0?\n>\n> Whichever you prefer, I don't think it should matter. Famous last words!!\n> :)\n>\n\nI will go with \"enable\", seems more logical. One other thing that we might\nwant to address now is the Controller metadata structure for denoise is\ncalled \"struct SdnStatus\". Now that it passed CDN state though it, perhaps\nI should rename this to denoiseStatus. What do you think?\n\nNaush\n\n\n\n>\n> Best regards\n> David\n>\n> >\n> > Regards,\n> > Naush\n> >\n> >\n> >>\n> >>\n> >> On Wed, 20 Jan 2021 at 08:35, Naushir Patuck \n> wrote:\n> >> >\n> >> > The application provided noise reduction mode gets passed into the\n> >> > denoise controller. The denoise controller in turn returns the mode\n> to\n> >> > the IPA which now sets up the colour denoise processing appropriately.\n> >> >\n> >> > Signed-off-by: Naushir Patuck \n> >> > ---\n> >> > src/ipa/raspberrypi/raspberrypi.cpp | 47\n> ++++++++++++++++++++++++++++-\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 8af3d603a3ff..91041fa68930 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 \"dpc_status.h\"\n> >> > #include \"focus_status.h\"\n> >> > #include \"geq_status.h\"\n> >> > @@ -553,7 +554,8 @@ bool IPARPi::validateIspControls()\n> >> > V4L2_CID_USER_BCM2835_ISP_DENOISE,\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_LENS_SHADING,\n> >> > + V4L2_CID_USER_BCM2835_ISP_CDN,\n> >> > };\n> >> >\n> >> > for (auto c : ctrls) {\n> >> > @@ -603,6 +605,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::Fast },\n> >> > + { controls::draft::NoiseReductionModeHighQuality,\n> RPiController::DenoiseMode::HighQuality },\n> >> > + { controls::draft::NoiseReductionModeMinimal,\n> RPiController::DenoiseMode::Fast },\n> >>\n> >> If we go with a revised definition of DenoiseMode, maybe this one\n> >> would be DenoiseMode::ColourOff?\n> >>\n> >> Anyway, thanks for all this work. Best regards\n> >> David\n> >>\n> >> > + { controls::draft::NoiseReductionModeZSL,\n> RPiController::DenoiseMode::HighQuality },\n> >> > +};\n> >> > +\n> >> > void IPARPi::queueRequest(const ControlList &controls)\n> >> > {\n> >> > /* Clear the return metadata buffer. */\n> >> > @@ -806,6 +816,22 @@ void IPARPi::queueRequest(const ControlList\n> &controls)\n> >> > break;\n> >> > }\n> >> >\n> >> > + case controls::NOISE_REDUCTION_MODE: {\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> >> > +\n> sdn->SetMode(DenoiseModeTable.at(idx));\n> >> > +\n> libcameraMetadata_.set(controls::draft::NoiseReductionMode, idx);\n> >> > + } else {\n> >> > + LOG(IPARPI, Error) << \"Noise\n> reduction mode \" << idx\n> >> > + << \" not\n> recognised\";\n> >> > + }\n> >> > + break;\n> >> > + }\n> >> > +\n> >> > default:\n> >> > LOG(IPARPI, Warning)\n> >> > << \"Ctrl \" << controls::controls.at\n> (ctrl.first)->name()\n> >> > @@ -1052,9 +1078,28 @@ void IPARPi::applyDenoise(const struct\n> SdnStatus *denoiseStatus, ControlList &ct\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::Fast:\n> >> > + cdn.enabled = 1;\n> >> > + cdn.mode = CDN_MODE_FAST;\n> >> > + break;\n> >> > + case RPiController::DenoiseMode::HighQuality:\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{\n> 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> >> > 2.25.1\n> >> >\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 60DB3C0F2A\n\tfor ;\n\tWed, 20 Jan 2021 11:21:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D7B68681AA;\n\tWed, 20 Jan 2021 12:21:16 +0100 (CET)","from mail-lf1-x12a.google.com (mail-lf1-x12a.google.com\n\t[IPv6:2a00:1450:4864:20::12a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8F6A36811D\n\tfor ;\n\tWed, 20 Jan 2021 12:21:15 +0100 (CET)","by mail-lf1-x12a.google.com with SMTP id b26so33641322lff.9\n\tfor ;\n\tWed, 20 Jan 2021 03:21:15 -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=\"eA5pg5Lw\"; 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=cFfyZfvlLktD2epxDVMVy8MBIdow33Y6f/dnaulWuAA=;\n\tb=eA5pg5Lwquf5lVvZeKwo9lFW8logbF/pCIvJhcKDAw/k2gIu//LvB/f9IdPIvWCb4s\n\tVcE0p+fPZqD/YFu6+6u2FVhyYCZ/pbtp5v6/dmwvRveoKTW1O5ByEd7qi1OAVyfNPCB7\n\txxp53dXCHI5ND6cSC4iboL/9jIfhLvPcyLYSFOLEqAxPxTjI29NUenVq1RYeRmipYTLF\n\tObVq2iqhjcKnFck2xPQwTNC57rh3sF937KGjq5h+w0Ee8N35Sos97V7siY4adG0dgESp\n\t4SbFHZCEuUakYDQygNiBsuDAoMwbZ5yzK3gIPJDSYXF0ns1gkY20jULHvEIHZ8LQ2pXt\n\tMkGA==","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=cFfyZfvlLktD2epxDVMVy8MBIdow33Y6f/dnaulWuAA=;\n\tb=umTfT5c5Tvqrvl0pSvVYGRNkKSMro0c7/Ee591ScHk/LoJzK1/E1YBLMirYJNHp6A7\n\tp57DTYVj9TVIRS30fKIyNQlDRYCaCDhKgn4Q0CZ4IAYfZ4XyMD4IuCmKGDNHJpdu7cwX\n\tIMNDkM2REkYahRKa1ZiNERNj+0JM+P18dyeHw29SDiPYBTalyNycW8pzJmxfEiWWchU7\n\thTbHvl25VXB+cyars6l/9P+DZxP95iCjpgMlQbr+b+6tFq3u4GPBu0WBB9Y/bCMBh0M6\n\t4DDRvG9YItFBn3TeVHC3xUarJuzPMUl0sgtFFBZveOznmoBqJ/6qeuksMCnk5LatHI/7\n\tPmmA==","X-Gm-Message-State":"AOAM530KmNwA7Sid+geVJ3Nlt/V3lb0/ZW+XJrnnDEk9xU8ipvr47WeU\n\tgAKFLTeON948Zrv1OoQV7hORpRpIVbkvHV/xM9YpNw==","X-Google-Smtp-Source":"ABdhPJy6HSJ3TZsgtLrsWfAKZk+B+vkayP3Bl4qJYBq5btiU4SI8TfdOEs3kPFiQfKR6t2rIaYYSk7S03MmYgPskQdY=","X-Received":"by 2002:a19:ee09:: with SMTP id g9mr2000444lfb.272.1611141674948;\n\tWed, 20 Jan 2021 03:21:14 -0800 (PST)","MIME-Version":"1.0","References":"<20210120083449.642418-1-naush@raspberrypi.com>\n\t<20210120083449.642418-6-naush@raspberrypi.com>\n\t\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Wed, 20 Jan 2021 11:20:58 +0000","Message-ID":"","To":"David Plowman ","Subject":"Re: [libcamera-devel] [PATCH 5/5] 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=\"===============6559339080502173038==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14634,"web_url":"https://patchwork.libcamera.org/comment/14634/","msgid":"","date":"2021-01-20T11:32:48","subject":"Re: [libcamera-devel] [PATCH 5/5] ipa: raspberrypi: Handle\n\tcontrol::NoiseReductionMode in 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\nYes, I agree. The XxxStatus normally matches the derived XxxAlgorithm,\nso DenoiseStatus would be better.\n\nDavid\n\nOn Wed, 20 Jan 2021 at 11:21, Naushir Patuck wrote:\n>\n>\n>\n> On Wed, 20 Jan 2021 at 11:16, David Plowman wrote:\n>>\n>> Hi Naush\n>>\n>> On Wed, 20 Jan 2021 at 10:58, Naushir Patuck wrote:\n>> >\n>> > Hi David,\n>> >\n>> >\n>> > On Wed, 20 Jan 2021 at 10:29, David Plowman wrote:\n>> >>\n>> >> Hi Naush\n>> >>\n>> >> Thanks for this patch (and the previous one, namely 4/5). I'm afraid\n>> >> I'd quite like to have just a small meta-discussion about this one\n>> >> first. Only a small one, though!\n>> >>\n>> >> Reading these patches it seems like an application setting\n>> >> NoiseReductionModeOff will still get regular spatial denoise, just not\n>> >> colour denoise. Is that right? Under such circumstances I think it\n>> >> would be clearer to rename DenoiseMode as ColourDenoiseMode.\n>> >>\n>> >> Having said that, if we're going to support NoiseReductionModeOff,\n>> >> then we should perhaps make it disable all denoising? I suspect folks\n>> >> would find it \"surprising\" if it didn't do that. As such, maybe\n>> >> DenoiseMode would remain as DenoiseMode (not ColourDenoiseMode) and\n>> >> we'd have 4 values, namely:\n>> >>\n>> >> DenoiseMode::Off - really everything off\n>> >> DenoiseMode::ColourOff - colour denoise off but regular spatial denoise on\n>> >> DenoiseMode::ColorFast - with fast colour denoise\n>> >> DenoiseMode::ColorHighQuality - with slow colour denoise\n>> >>\n>> >> (Setting the denoise strength to zero will disable spatial denoise.)\n>> >>\n>> >> What do you think?\n>> >\n>> >\n>> > Yes, I think this is a reasonable thing to do. I will make the appropriate changes in the next revisions.\n>> > To disable SDN, would it be better to set the \"enabled\" field in the bcm2835_isp_denoise struct to 0 over setting the strength to 0?\n>>\n>> Whichever you prefer, I don't think it should matter. Famous last words!! :)\n>\n>\n> I will go with \"enable\", seems more logical. One other thing that we might want to address now is the Controller metadata structure for denoise is called \"struct SdnStatus\". Now that it passed CDN state though it, perhaps I should rename this to denoiseStatus. What do you think?\n>\n> Naush\n>\n>\n>>\n>>\n>> Best regards\n>> David\n>>\n>> >\n>> > Regards,\n>> > Naush\n>> >\n>> >\n>> >>\n>> >>\n>> >> On Wed, 20 Jan 2021 at 08:35, Naushir Patuck wrote:\n>> >> >\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>> >> > ---\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 8af3d603a3ff..91041fa68930 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 \"dpc_status.h\"\n>> >> > #include \"focus_status.h\"\n>> >> > #include \"geq_status.h\"\n>> >> > @@ -553,7 +554,8 @@ bool IPARPi::validateIspControls()\n>> >> > V4L2_CID_USER_BCM2835_ISP_DENOISE,\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_LENS_SHADING,\n>> >> > + V4L2_CID_USER_BCM2835_ISP_CDN,\n>> >> > };\n>> >> >\n>> >> > for (auto c : ctrls) {\n>> >> > @@ -603,6 +605,14 @@ static const std::map AwbModeTable = {\n>> >> > { controls::AwbCustom, \"custom\" },\n>> >> > };\n>> >> >\n>> >> > +static const std::map DenoiseModeTable = {\n>> >> > + { controls::draft::NoiseReductionModeOff, RPiController::DenoiseMode::Off },\n>> >> > + { controls::draft::NoiseReductionModeFast, RPiController::DenoiseMode::Fast },\n>> >> > + { controls::draft::NoiseReductionModeHighQuality, RPiController::DenoiseMode::HighQuality },\n>> >> > + { controls::draft::NoiseReductionModeMinimal, RPiController::DenoiseMode::Fast },\n>> >>\n>> >> If we go with a revised definition of DenoiseMode, maybe this one\n>> >> would be DenoiseMode::ColourOff?\n>> >>\n>> >> Anyway, thanks for all this work. Best regards\n>> >> David\n>> >>\n>> >> > + { controls::draft::NoiseReductionModeZSL, RPiController::DenoiseMode::HighQuality },\n>> >> > +};\n>> >> > +\n>> >> > void IPARPi::queueRequest(const ControlList &controls)\n>> >> > {\n>> >> > /* Clear the return metadata buffer. */\n>> >> > @@ -806,6 +816,22 @@ void IPARPi::queueRequest(const ControlList &controls)\n>> >> > break;\n>> >> > }\n>> >> >\n>> >> > + case controls::NOISE_REDUCTION_MODE: {\n>> >> > + RPiController::DenoiseAlgorithm *sdn = 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>> >> > + libcameraMetadata_.set(controls::draft::NoiseReductionMode, idx);\n>> >> > + } else {\n>> >> > + LOG(IPARPI, Error) << \"Noise reduction mode \" << idx\n>> >> > + << \" not recognised\";\n>> >> > + }\n>> >> > + break;\n>> >> > + }\n>> >> > +\n>> >> > default:\n>> >> > LOG(IPARPI, Warning)\n>> >> > << \"Ctrl \" << controls::controls.at(ctrl.first)->name()\n>> >> > @@ -1052,9 +1078,28 @@ void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ct\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::Fast:\n>> >> > + cdn.enabled = 1;\n>> >> > + cdn.mode = CDN_MODE_FAST;\n>> >> > + break;\n>> >> > + case RPiController::DenoiseMode::HighQuality:\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, ControlList &ctrls)\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 B79B7BD808\n\tfor ;\n\tWed, 20 Jan 2021 11:33:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 42870681AA;\n\tWed, 20 Jan 2021 12:33:02 +0100 (CET)","from mail-ot1-x331.google.com (mail-ot1-x331.google.com\n\t[IPv6:2607:f8b0:4864:20::331])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A3F3D6819D\n\tfor ;\n\tWed, 20 Jan 2021 12:33:00 +0100 (CET)","by mail-ot1-x331.google.com with SMTP id 36so11535194otp.2\n\tfor ;\n\tWed, 20 Jan 2021 03:33:00 -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=\"HypA+EFM\"; 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:content-transfer-encoding;\n\tbh=VsuHGmX28i/3B696IRgCeHDQXGEXLPA1yf3TxGwFy20=;\n\tb=HypA+EFMQKpP9SHUR0tN4FnKGNfuDQnXN3KxFtdJ5YZTOatx8PNYYqIbESSa+wYgr9\n\t7XdyNyH6vxhiBrUEyIN2/YKHZnMWaSKhP+9iJPjbiJh5GoLRKvlVtFitLvWtOQD8zUdb\n\tdvdWA6eeNPItNeQ+crpXmpXKrvCI+7u1h2bHr9YuSSCeHEI3bRr36wWHigJbIR5mLhAv\n\txkyJVscsjvXBBNboArUBgJ5hslsid9hFB0FR1YbNrcCLu9AvenMVWKO6FsnykJ3ENS7Z\n\tPhUoGKqs8ygGd1F4ZSt8zcaNUXOeMDYkfHtj2gd7+6Uyyh70dGC0QO1JRP78BrjMPbE1\n\tnrUg==","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:content-transfer-encoding;\n\tbh=VsuHGmX28i/3B696IRgCeHDQXGEXLPA1yf3TxGwFy20=;\n\tb=cN205p1+GJvyyhcQ7Yd2Qa4GXXhfEbeJex59uGTG25BKbWpu0FYHN11yaGrE+ZH1tR\n\t6j/QwGMn2xZjnEhSYti4G22w7+3kl3bhYwEdw/5pvVjtSA8Fy70QZELLdhjSMG8R4Im8\n\tpwjszWsPk0ZaFc/d7L9XmmWoDEu7Msdy4TOp9jszxGTyCiaK/rE+3bEjfg9Hfg5cZruz\n\thOzK0GADXz8+MGe/h56G2Z8PXdIkv2cvx8xVwO2TJmZYUhCL0RYThrOvX1fUs0J87kFl\n\tbh4f06e4wOahxP69U0wa+7gSE8PgXfX1R6JpGah/O+/17E6I7uy+FQWqIaCsG4GkArTk\n\t7gFg==","X-Gm-Message-State":"AOAM533t9iRirSIiXbHPfvDK6xBeUb85vwmvkdjnluP0t2X+vQ40v3TC\n\tAicezFAYp+F+67DZa91TFMFOEx9CPQdzDBTfTgmbFA==","X-Google-Smtp-Source":"ABdhPJy/49BH6hw7Jc7Z4uRal2fjn9AmzN57S/fROQbTWzyasRf2RvON8kbIAa4KBqkSy2oGTKvIgFf8U9IDAM6QkFQ=","X-Received":"by 2002:a05:6830:1e7b:: with SMTP id\n\tm27mr4242875otr.317.1611142379493; \n\tWed, 20 Jan 2021 03:32:59 -0800 (PST)","MIME-Version":"1.0","References":"<20210120083449.642418-1-naush@raspberrypi.com>\n\t<20210120083449.642418-6-naush@raspberrypi.com>\n\t\n\t\n\t\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Wed, 20 Jan 2021 11:32:48 +0000","Message-ID":"","To":"Naushir Patuck ","Subject":"Re: [libcamera-devel] [PATCH 5/5] 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":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]