[{"id":35398,"web_url":"https://patchwork.libcamera.org/comment/35398/","msgid":"","date":"2025-08-14T07:44:01","subject":"Re: [PATCH] ipa: rpi:: denoise: Implement TDN back-off for CDN\n\tdeviation","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi David,\n\nThank you for the patch.\n\nOn Tue, 12 Aug 2025 at 14:39, David Plowman\n wrote:\n>\n> The CDN (colour denoise) deviation gets gradually reduced frame by\n> frame as TDN (temporal denoise) comes in and has more effect. CDN is\n> more harmful to image detail than TDN, so ramping it down in favour of\n> TDN is beneficial.\n>\n> The tuning file parameters are chosen so that existing tuning files\n> don't have to be updated and will carry on working \"mostly like they\n> did\" (apart from the new back-off).\n>\n> Signed-off-by: David Plowman \n> ---\n> src/ipa/rpi/controller/rpi/denoise.cpp | 22 +++++++++++++++++++---\n> src/ipa/rpi/controller/rpi/denoise.h | 4 +++-\n> 2 files changed, 22 insertions(+), 4 deletions(-)\n>\n> diff --git a/src/ipa/rpi/controller/rpi/denoise.cpp b/src/ipa/rpi/controller/rpi/denoise.cpp\n> index ba851658..d9bbeddd 100644\n> --- a/src/ipa/rpi/controller/rpi/denoise.cpp\n> +++ b/src/ipa/rpi/controller/rpi/denoise.cpp\n> @@ -37,7 +37,17 @@ int DenoiseConfig::read(const libcamera::YamlObject ¶ms)\n> cdnEnable = params.contains(\"cdn\");\n> if (cdnEnable) {\n> auto &cdnParams = params[\"cdn\"];\n> - cdnDeviation = cdnParams[\"deviation\"].get(120);\n> + cdnDeviationNoTdn = cdnParams[\"deviation_no_tdn\"].get(150);\n> + /*\n> + * For backwards compatibility with existing tuning files, interpret \"deviation\"\n> + * as giving the \"no TDN\" deviation, where present.\n> + */\n> + cdnDeviationNoTdn = cdnParams[\"deviation\"].get(cdnDeviationNoTdn);\n\nDo we want an if-else clause so that if both \"deviation_no_tdn\" and\n\"deviation\" are set in the file, we stick to the former?\n\nWith or without that change:\n\nReviewed-by: Naushir Patuck \n\n\n> + /*\n> + * A third the value of the no TDN deviation is about right for with-TDN, if\n> + * the user hasn't specified otherwise.\n> + */\n> + cdnDeviationWithTdn = cdnParams[\"deviation_with_tdn\"].get(cdnDeviationNoTdn / 3);\n> cdnStrength = cdnParams[\"strength\"].get(0.2);\n> }\n>\n> @@ -48,13 +58,14 @@ int DenoiseConfig::read(const libcamera::YamlObject ¶ms)\n> tdnThreshold = tdnParams[\"threshold\"].get(0.75);\n> } else if (sdnEnable) {\n> /*\n> - * If SDN is enabled but TDN isn't, overwrite all the SDN settings\n> + * If SDN is enabled but TDN isn't, overwrite all the SDN/CDN settings\n> * with the \"no TDN\" versions. This makes it easier to enable or\n> * disable TDN in the tuning file without editing all the other\n> * parameters.\n> */\n> sdnDeviation = sdnDeviation2 = sdnDeviationNoTdn;\n> sdnStrength = sdnStrengthNoTdn;\n> + cdnDeviationWithTdn = cdnDeviationNoTdn;\n> }\n>\n> return 0;\n> @@ -107,6 +118,7 @@ void Denoise::switchMode([[maybe_unused]] CameraMode const &cameraMode,\n> currentSdnDeviation_ = currentConfig_->sdnDeviationNoTdn;\n> currentSdnStrength_ = currentConfig_->sdnStrengthNoTdn;\n> currentSdnDeviation2_ = currentConfig_->sdnDeviationNoTdn;\n> + currentCdnDeviation_ = currentConfig_->cdnDeviationNoTdn;\n> }\n>\n> void Denoise::prepare(Metadata *imageMetadata)\n> @@ -159,8 +171,12 @@ void Denoise::prepare(Metadata *imageMetadata)\n>\n> if (currentConfig_->cdnEnable && mode_ != DenoiseMode::ColourOff) {\n> struct CdnStatus cdn;\n> - cdn.threshold = currentConfig_->cdnDeviation * noiseStatus.noiseSlope + noiseStatus.noiseConstant;\n> + cdn.threshold = currentCdnDeviation_ * noiseStatus.noiseSlope + noiseStatus.noiseConstant;\n> cdn.strength = currentConfig_->cdnStrength;\n> + /* For the next frame, we back off the CDN deviation as TDN ramps up. */\n> + double f = currentConfig_->sdnTdnBackoff;\n> + currentCdnDeviation_ = f * currentCdnDeviation_ + (1 - f) * currentConfig_->cdnDeviationWithTdn;\n> +\n> imageMetadata->set(\"cdn.status\", cdn);\n> LOG(RPiDenoise, Debug)\n> << \"programmed cdn threshold \" << cdn.threshold\n> diff --git a/src/ipa/rpi/controller/rpi/denoise.h b/src/ipa/rpi/controller/rpi/denoise.h\n> index 79946c97..e23a2e8f 100644\n> --- a/src/ipa/rpi/controller/rpi/denoise.h\n> +++ b/src/ipa/rpi/controller/rpi/denoise.h\n> @@ -23,7 +23,8 @@ struct DenoiseConfig {\n> double sdnDeviationNoTdn;\n> double sdnStrengthNoTdn;\n> double sdnTdnBackoff;\n> - double cdnDeviation;\n> + double cdnDeviationNoTdn;\n> + double cdnDeviationWithTdn;\n> double cdnStrength;\n> double tdnDeviation;\n> double tdnThreshold;\n> @@ -54,6 +55,7 @@ private:\n> double currentSdnDeviation_;\n> double currentSdnStrength_;\n> double currentSdnDeviation2_;\n> + double currentCdnDeviation_;\n> };\n>\n> } // namespace RPiController\n> --\n> 2.39.5\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 93130BEFBE\n\tfor ;\n\tThu, 14 Aug 2025 07:44:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DB0936923F;\n\tThu, 14 Aug 2025 09:44:37 +0200 (CEST)","from mail-vk1-xa34.google.com (mail-vk1-xa34.google.com\n\t[IPv6:2607:f8b0:4864:20::a34])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4854E61436\n\tfor ;\n\tThu, 14 Aug 2025 09:44:35 +0200 (CEST)","by mail-vk1-xa34.google.com with SMTP id\n\t71dfb90a1353d-53b175da35bso13709e0c.3\n\tfor ;\n\tThu, 14 Aug 2025 00:44:35 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"sl45C81f\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1755157474; x=1755762274;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=I00U6mBJgu1JbZ8XUdQd48jdSqqT99HjDrbjgKY8vtg=;\n\tb=sl45C81fY8GPnZjoKm27MQiLtgbMxnKjpRpQeHhpyYd780QLHIfXf0rp2OOlKCKL5P\n\tjJhMj1ENQ09rsgewCgtnbarJ1abokk4gAKbj1ehWVjiORXxHvaUJ/q0+23SzQF2EnYLP\n\tsTMFMzE5iMM/VY5b6GYCAdk+JUHQBqXXjiRxeXKnWRLsJhlgJfll9LLmnxt0+1VKfDNY\n\tTVprecnkUfb4RXPDZHuVglQN3otBP77GNeKZJX0g07QIiLwEBf0dEjCCEEwEJmO+nhaz\n\tbHt6tcF1ruZ4ZrjGDIBq3Myi1fnJtbuzaf6pPY4KOBCagxd4qjSwSNLKw7+Gf88f6mD0\n\t/bwg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1755157474; x=1755762274;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=I00U6mBJgu1JbZ8XUdQd48jdSqqT99HjDrbjgKY8vtg=;\n\tb=XK5RL6BvoFbcoAggcbqsxEaf0UCM1/oa7Awsc61ulqptNVSU3aVtCIoUW2IV93TGs5\n\t6eTcXIG1Hb+w1FHD2SwrLq9dTZbwOuS6Qfkahs2G8j67xAfL5wFPoI6j2W1Lcp9c3Ge+\n\tcDMY8SPhCO3P+03HsXRCbPKaYmSJZekp4nNz6k3ha4B2ab0wV/k95IpgQ078PNEFAS0P\n\trbBD/5dxWaEEFJfd98zpNGozKyQblVtYFT1oQpotMPl2sJNIrvquexCJHcgPbRiuaEs4\n\t6CCaYMSdUyO+1fU6PfdeGgDbNKTBv18Jg0gy4hN8ehXPzQ4fZCXry1yI81xS/dyKvC3m\n\ti3Ug==","X-Gm-Message-State":"AOJu0YxRdzw0xCNctTiaj2Qey75H5RGvCrCszJvy7jTOj9zKCbhTMkEx\n\tkzbXfpS6VMaUXAMkkG6sqwlLWGg8kGi/z+wJidNHVK56ycPhJI1k6DVdfi7SSIqCatyEolvfAM+\n\tlp7k+J0UlX8HLQHdT2GNIPnzx1vI/nVLTuhsDyhZiPQ==","X-Gm-Gg":"ASbGnctvYBWJx6XGtlcwe10eFy93Nc7WCAUfaVPSv6T6AmMAiKmutozeoaq3wY9oncG\n\tRvYtIPC+hWhJPciEF8486gqftNuNMNpqfo/nTtPvIMkWUk7POlOh4+M/OcZV5Y8vgd3p+C62oKo\n\t3DVmDr2b0b7objzs4Qv0tmTaslxU1ghFDmgLCVUS+x4AMkkjnQ/7Kluog9JozWQcgEuOBJ1hYv2\n\tvyWoshaevtIP3tIUrJ2J0WfM1IHEW4Ofz0YG/I=","X-Google-Smtp-Source":"AGHT+IEimTNlPhtpDvXwSI20y9HIFUPYAEAVo+JuF0s6ci+UQavy7iU4HtF/uA1ZOJuEyrUJFd9mLozyogSaib2Jzy4=","X-Received":"by 2002:a05:6122:8492:b0:53b:104c:8ef with SMTP id\n\t71dfb90a1353d-53b18b6ed33mr90361e0c.2.1755157473903; Thu, 14 Aug 2025\n\t00:44:33 -0700 (PDT)","MIME-Version":"1.0","References":"<20250812133856.3981-1-david.plowman@raspberrypi.com>","In-Reply-To":"<20250812133856.3981-1-david.plowman@raspberrypi.com>","From":"Naushir Patuck ","Date":"Thu, 14 Aug 2025 08:44:01 +0100","X-Gm-Features":"Ac12FXyYfoq7HV4kEobt02JBpC80JUH7HAvm4vxjI9wNhK9ALRIDqW7nwS0zbuA","Message-ID":"","Subject":"Re: [PATCH] ipa: rpi:: denoise: Implement TDN back-off for CDN\n\tdeviation","To":"David Plowman ","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":35421,"web_url":"https://patchwork.libcamera.org/comment/35421/","msgid":"","date":"2025-08-15T06:59:59","subject":"Re: [PATCH] ipa: rpi:: denoise: Implement TDN back-off for CDN\n\tdeviation","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 the review.\n\nOn Thu, 14 Aug 2025 at 08:44, Naushir Patuck wrote:\n>\n> Hi David,\n>\n> Thank you for the patch.\n>\n> On Tue, 12 Aug 2025 at 14:39, David Plowman\n> wrote:\n> >\n> > The CDN (colour denoise) deviation gets gradually reduced frame by\n> > frame as TDN (temporal denoise) comes in and has more effect. CDN is\n> > more harmful to image detail than TDN, so ramping it down in favour of\n> > TDN is beneficial.\n> >\n> > The tuning file parameters are chosen so that existing tuning files\n> > don't have to be updated and will carry on working \"mostly like they\n> > did\" (apart from the new back-off).\n> >\n> > Signed-off-by: David Plowman \n> > ---\n> > src/ipa/rpi/controller/rpi/denoise.cpp | 22 +++++++++++++++++++---\n> > src/ipa/rpi/controller/rpi/denoise.h | 4 +++-\n> > 2 files changed, 22 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/src/ipa/rpi/controller/rpi/denoise.cpp b/src/ipa/rpi/controller/rpi/denoise.cpp\n> > index ba851658..d9bbeddd 100644\n> > --- a/src/ipa/rpi/controller/rpi/denoise.cpp\n> > +++ b/src/ipa/rpi/controller/rpi/denoise.cpp\n> > @@ -37,7 +37,17 @@ int DenoiseConfig::read(const libcamera::YamlObject ¶ms)\n> > cdnEnable = params.contains(\"cdn\");\n> > if (cdnEnable) {\n> > auto &cdnParams = params[\"cdn\"];\n> > - cdnDeviation = cdnParams[\"deviation\"].get(120);\n> > + cdnDeviationNoTdn = cdnParams[\"deviation_no_tdn\"].get(150);\n> > + /*\n> > + * For backwards compatibility with existing tuning files, interpret \"deviation\"\n> > + * as giving the \"no TDN\" deviation, where present.\n> > + */\n> > + cdnDeviationNoTdn = cdnParams[\"deviation\"].get(cdnDeviationNoTdn);\n>\n> Do we want an if-else clause so that if both \"deviation_no_tdn\" and\n> \"deviation\" are set in the file, we stick to the former?\n\nNot sure, really - don't feel strongly. Or I suppose swapping the\norder like this would do it too:\n\n + /*\n + * For backwards compatibility with existing tuning\nfiles, interpret \"deviation\"\n + * as giving the \"no TDN\" deviation, where present.\n + */\n + cdnDeviationNoTdn = cdnParams[\"deviation\"].get(150);\n + cdnDeviationNoTdn =\ncdnParams[\"deviation_no_tdn\"].get(cdnDeviationNoTdn);\n\nDavid\n\n>\n> With or without that change:\n>\n> Reviewed-by: Naushir Patuck \n>\n>\n> > + /*\n> > + * A third the value of the no TDN deviation is about right for with-TDN, if\n> > + * the user hasn't specified otherwise.\n> > + */\n> > + cdnDeviationWithTdn = cdnParams[\"deviation_with_tdn\"].get(cdnDeviationNoTdn / 3);\n> > cdnStrength = cdnParams[\"strength\"].get(0.2);\n> > }\n> >\n> > @@ -48,13 +58,14 @@ int DenoiseConfig::read(const libcamera::YamlObject ¶ms)\n> > tdnThreshold = tdnParams[\"threshold\"].get(0.75);\n> > } else if (sdnEnable) {\n> > /*\n> > - * If SDN is enabled but TDN isn't, overwrite all the SDN settings\n> > + * If SDN is enabled but TDN isn't, overwrite all the SDN/CDN settings\n> > * with the \"no TDN\" versions. This makes it easier to enable or\n> > * disable TDN in the tuning file without editing all the other\n> > * parameters.\n> > */\n> > sdnDeviation = sdnDeviation2 = sdnDeviationNoTdn;\n> > sdnStrength = sdnStrengthNoTdn;\n> > + cdnDeviationWithTdn = cdnDeviationNoTdn;\n> > }\n> >\n> > return 0;\n> > @@ -107,6 +118,7 @@ void Denoise::switchMode([[maybe_unused]] CameraMode const &cameraMode,\n> > currentSdnDeviation_ = currentConfig_->sdnDeviationNoTdn;\n> > currentSdnStrength_ = currentConfig_->sdnStrengthNoTdn;\n> > currentSdnDeviation2_ = currentConfig_->sdnDeviationNoTdn;\n> > + currentCdnDeviation_ = currentConfig_->cdnDeviationNoTdn;\n> > }\n> >\n> > void Denoise::prepare(Metadata *imageMetadata)\n> > @@ -159,8 +171,12 @@ void Denoise::prepare(Metadata *imageMetadata)\n> >\n> > if (currentConfig_->cdnEnable && mode_ != DenoiseMode::ColourOff) {\n> > struct CdnStatus cdn;\n> > - cdn.threshold = currentConfig_->cdnDeviation * noiseStatus.noiseSlope + noiseStatus.noiseConstant;\n> > + cdn.threshold = currentCdnDeviation_ * noiseStatus.noiseSlope + noiseStatus.noiseConstant;\n> > cdn.strength = currentConfig_->cdnStrength;\n> > + /* For the next frame, we back off the CDN deviation as TDN ramps up. */\n> > + double f = currentConfig_->sdnTdnBackoff;\n> > + currentCdnDeviation_ = f * currentCdnDeviation_ + (1 - f) * currentConfig_->cdnDeviationWithTdn;\n> > +\n> > imageMetadata->set(\"cdn.status\", cdn);\n> > LOG(RPiDenoise, Debug)\n> > << \"programmed cdn threshold \" << cdn.threshold\n> > diff --git a/src/ipa/rpi/controller/rpi/denoise.h b/src/ipa/rpi/controller/rpi/denoise.h\n> > index 79946c97..e23a2e8f 100644\n> > --- a/src/ipa/rpi/controller/rpi/denoise.h\n> > +++ b/src/ipa/rpi/controller/rpi/denoise.h\n> > @@ -23,7 +23,8 @@ struct DenoiseConfig {\n> > double sdnDeviationNoTdn;\n> > double sdnStrengthNoTdn;\n> > double sdnTdnBackoff;\n> > - double cdnDeviation;\n> > + double cdnDeviationNoTdn;\n> > + double cdnDeviationWithTdn;\n> > double cdnStrength;\n> > double tdnDeviation;\n> > double tdnThreshold;\n> > @@ -54,6 +55,7 @@ private:\n> > double currentSdnDeviation_;\n> > double currentSdnStrength_;\n> > double currentSdnDeviation2_;\n> > + double currentCdnDeviation_;\n> > };\n> >\n> > } // namespace RPiController\n> > --\n> > 2.39.5\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 04D98BEFBE\n\tfor ;\n\tFri, 15 Aug 2025 07:00:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 65EE269257;\n\tFri, 15 Aug 2025 09:00:14 +0200 (CEST)","from mail-qt1-x832.google.com (mail-qt1-x832.google.com\n\t[IPv6:2607:f8b0:4864:20::832])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 49E0B69244\n\tfor ;\n\tFri, 15 Aug 2025 09:00:12 +0200 (CEST)","by mail-qt1-x832.google.com with SMTP id\n\td75a77b69052e-4b109bbea05so25976711cf.2\n\tfor ;\n\tFri, 15 Aug 2025 00:00:12 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"Bk2rmP9E\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1755241211; x=1755846011;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=RBdgSbWKkXTbeYyCc+9IkCKRqm76xSf3IiGZfFx9TVA=;\n\tb=Bk2rmP9EG92AiKQ3rc8+SHecT7llVqTuogBHrGb/YsQnpbHBd6/C2Cz2G97Bd1b8Fj\n\t/vm/rw4afzH4YQ2RtkiQaKqHgsexEFznIebUsIPq1S9JOhYzdppX6bU8Mjie2HfzbozK\n\tqkcZ3E2FeR8ma3FTXTbloxkpkCyKxHLJFQGypIbvLNwSircog4AbpJBJ6HyozoubiFg7\n\tcCbfcnu3upbw3piZRVaAtrsNErG4Zd41D/ph0GYxT093CsDY7BCZkcvwILZS6+DlruR2\n\tUz6oLb/Zt0LRghNJB5xp5+ICkiQ9UeiYDbb7DcpYNamAcpNcb4BlB1GKYTKVHmP480zh\n\th03w==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1755241211; x=1755846011;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=RBdgSbWKkXTbeYyCc+9IkCKRqm76xSf3IiGZfFx9TVA=;\n\tb=geu5xdc3uTxIdBq9PXE13IDrZx+if1Gbbq2L2mxy7ub5DIx2tW1TEIz84Ebwmt2CXG\n\tIEUt6T/qNDyQ+bvYY/RBHA3oDyAtkTIKrKIJXo25Frbk70cuqdi7pMC4hoHxrtO3r1Of\n\tdM1VYkSgswTeRw5ogxi0Eh/FsKZ09cmtre5Yq7JuMBciwLkz69uLZv8ChuK99bqABaeN\n\theTO3UD7fsXuwk32kz+906/cqwLDUCtBNCEX2r/13j1bQbtMmIBEL1CKLgYHl+Euz8xB\n\tHWyVx07NM9RwxcmKyIDFQP1AP1PqG8pJWd2TcPIDMAKLsGM1b06pUD1jkD6SNQA5XAHp\n\tXv4w==","X-Gm-Message-State":"AOJu0Yy/IvRmjolwbMY+1tH1CqVbYxzNSI/VYQsgWzsjjGnBSORTIH+C\n\tgfuctWDxq8DdOZG6d/BjmejWnYDhNXAabjXHdP4vRQpQiTWuxCKoApR+xNjtoJrPk5JJLw9Ds/F\n\tfCYYMpY5O380b4Wn/JBl2bMmBKBBigu4V6xX0VIZ/eA==","X-Gm-Gg":"ASbGnctjc5n7E5LjQb8pBVgUK9LoicbBixrbZRa4onkaBgZu5n+iige7R+WPNAX4lSx\n\tDp7YO57iFLJn5I1AptqY8t4EqIX+T9ul/55RBP20bojBTE4gdu/zpml/xe38cLZo16LfwCqYtqI\n\t9EUYQoC4CnX7ffMuGFVbVmTMa0eFf0SHtyE9VK9MeFRlD7rxGauvrrD76OUc4hqdfqYOrkZN9F3\n\tP99Kj2EEWklOqFEHkxF04aTsAR7SSFGIKd7","X-Google-Smtp-Source":"AGHT+IGRe+q0ocW03/3o7UdsHCNLfeuMZq66/oApL7oSLjjCn8fxbWT4ugwlBVQnSsJ336JNMQKU1ZAa3rdrlfB11yo=","X-Received":"by 2002:a05:622a:1189:b0:4b0:677f:db03 with SMTP id\n\td75a77b69052e-4b11e1b0d4fmr10049231cf.15.1755241210937;\n\tFri, 15 Aug 2025 00:00:10 -0700 (PDT)","MIME-Version":"1.0","References":"<20250812133856.3981-1-david.plowman@raspberrypi.com>\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Fri, 15 Aug 2025 07:59:59 +0100","X-Gm-Features":"Ac12FXxNFSSntYSAxOa6pYYQSBtTQ4n070M020ttKxia1sGFNnm_aQ4Yr9ktD88","Message-ID":"","Subject":"Re: [PATCH] ipa: rpi:: denoise: Implement TDN back-off for CDN\n\tdeviation","To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":35423,"web_url":"https://patchwork.libcamera.org/comment/35423/","msgid":"","date":"2025-08-15T07:11:45","subject":"Re: [PATCH] ipa: rpi:: denoise: Implement TDN back-off for CDN\n\tdeviation","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"On Fri, 15 Aug 2025 at 08:00, David Plowman\n wrote:\n>\n> Hi Naush\n>\n> Thanks for the review.\n>\n> On Thu, 14 Aug 2025 at 08:44, Naushir Patuck wrote:\n> >\n> > Hi David,\n> >\n> > Thank you for the patch.\n> >\n> > On Tue, 12 Aug 2025 at 14:39, David Plowman\n> > wrote:\n> > >\n> > > The CDN (colour denoise) deviation gets gradually reduced frame by\n> > > frame as TDN (temporal denoise) comes in and has more effect. CDN is\n> > > more harmful to image detail than TDN, so ramping it down in favour of\n> > > TDN is beneficial.\n> > >\n> > > The tuning file parameters are chosen so that existing tuning files\n> > > don't have to be updated and will carry on working \"mostly like they\n> > > did\" (apart from the new back-off).\n> > >\n> > > Signed-off-by: David Plowman \n> > > ---\n> > > src/ipa/rpi/controller/rpi/denoise.cpp | 22 +++++++++++++++++++---\n> > > src/ipa/rpi/controller/rpi/denoise.h | 4 +++-\n> > > 2 files changed, 22 insertions(+), 4 deletions(-)\n> > >\n> > > diff --git a/src/ipa/rpi/controller/rpi/denoise.cpp b/src/ipa/rpi/controller/rpi/denoise.cpp\n> > > index ba851658..d9bbeddd 100644\n> > > --- a/src/ipa/rpi/controller/rpi/denoise.cpp\n> > > +++ b/src/ipa/rpi/controller/rpi/denoise.cpp\n> > > @@ -37,7 +37,17 @@ int DenoiseConfig::read(const libcamera::YamlObject ¶ms)\n> > > cdnEnable = params.contains(\"cdn\");\n> > > if (cdnEnable) {\n> > > auto &cdnParams = params[\"cdn\"];\n> > > - cdnDeviation = cdnParams[\"deviation\"].get(120);\n> > > + cdnDeviationNoTdn = cdnParams[\"deviation_no_tdn\"].get(150);\n> > > + /*\n> > > + * For backwards compatibility with existing tuning files, interpret \"deviation\"\n> > > + * as giving the \"no TDN\" deviation, where present.\n> > > + */\n> > > + cdnDeviationNoTdn = cdnParams[\"deviation\"].get(cdnDeviationNoTdn);\n> >\n> > Do we want an if-else clause so that if both \"deviation_no_tdn\" and\n> > \"deviation\" are set in the file, we stick to the former?\n>\n> Not sure, really - don't feel strongly. Or I suppose swapping the\n> order like this would do it too:\n>\n> + /*\n> + * For backwards compatibility with existing tuning\n> files, interpret \"deviation\"\n> + * as giving the \"no TDN\" deviation, where present.\n> + */\n> + cdnDeviationNoTdn = cdnParams[\"deviation\"].get(150);\n> + cdnDeviationNoTdn =\n> cdnParams[\"deviation_no_tdn\"].get(cdnDeviationNoTdn);\n\nYup that would do!\n\nReviewed-by: Naushir Patuck \n\n>\n> David\n>\n> >\n> > With or without that change:\n> >\n> > Reviewed-by: Naushir Patuck \n> >\n> >\n> > > + /*\n> > > + * A third the value of the no TDN deviation is about right for with-TDN, if\n> > > + * the user hasn't specified otherwise.\n> > > + */\n> > > + cdnDeviationWithTdn = cdnParams[\"deviation_with_tdn\"].get(cdnDeviationNoTdn / 3);\n> > > cdnStrength = cdnParams[\"strength\"].get(0.2);\n> > > }\n> > >\n> > > @@ -48,13 +58,14 @@ int DenoiseConfig::read(const libcamera::YamlObject ¶ms)\n> > > tdnThreshold = tdnParams[\"threshold\"].get(0.75);\n> > > } else if (sdnEnable) {\n> > > /*\n> > > - * If SDN is enabled but TDN isn't, overwrite all the SDN settings\n> > > + * If SDN is enabled but TDN isn't, overwrite all the SDN/CDN settings\n> > > * with the \"no TDN\" versions. This makes it easier to enable or\n> > > * disable TDN in the tuning file without editing all the other\n> > > * parameters.\n> > > */\n> > > sdnDeviation = sdnDeviation2 = sdnDeviationNoTdn;\n> > > sdnStrength = sdnStrengthNoTdn;\n> > > + cdnDeviationWithTdn = cdnDeviationNoTdn;\n> > > }\n> > >\n> > > return 0;\n> > > @@ -107,6 +118,7 @@ void Denoise::switchMode([[maybe_unused]] CameraMode const &cameraMode,\n> > > currentSdnDeviation_ = currentConfig_->sdnDeviationNoTdn;\n> > > currentSdnStrength_ = currentConfig_->sdnStrengthNoTdn;\n> > > currentSdnDeviation2_ = currentConfig_->sdnDeviationNoTdn;\n> > > + currentCdnDeviation_ = currentConfig_->cdnDeviationNoTdn;\n> > > }\n> > >\n> > > void Denoise::prepare(Metadata *imageMetadata)\n> > > @@ -159,8 +171,12 @@ void Denoise::prepare(Metadata *imageMetadata)\n> > >\n> > > if (currentConfig_->cdnEnable && mode_ != DenoiseMode::ColourOff) {\n> > > struct CdnStatus cdn;\n> > > - cdn.threshold = currentConfig_->cdnDeviation * noiseStatus.noiseSlope + noiseStatus.noiseConstant;\n> > > + cdn.threshold = currentCdnDeviation_ * noiseStatus.noiseSlope + noiseStatus.noiseConstant;\n> > > cdn.strength = currentConfig_->cdnStrength;\n> > > + /* For the next frame, we back off the CDN deviation as TDN ramps up. */\n> > > + double f = currentConfig_->sdnTdnBackoff;\n> > > + currentCdnDeviation_ = f * currentCdnDeviation_ + (1 - f) * currentConfig_->cdnDeviationWithTdn;\n> > > +\n> > > imageMetadata->set(\"cdn.status\", cdn);\n> > > LOG(RPiDenoise, Debug)\n> > > << \"programmed cdn threshold \" << cdn.threshold\n> > > diff --git a/src/ipa/rpi/controller/rpi/denoise.h b/src/ipa/rpi/controller/rpi/denoise.h\n> > > index 79946c97..e23a2e8f 100644\n> > > --- a/src/ipa/rpi/controller/rpi/denoise.h\n> > > +++ b/src/ipa/rpi/controller/rpi/denoise.h\n> > > @@ -23,7 +23,8 @@ struct DenoiseConfig {\n> > > double sdnDeviationNoTdn;\n> > > double sdnStrengthNoTdn;\n> > > double sdnTdnBackoff;\n> > > - double cdnDeviation;\n> > > + double cdnDeviationNoTdn;\n> > > + double cdnDeviationWithTdn;\n> > > double cdnStrength;\n> > > double tdnDeviation;\n> > > double tdnThreshold;\n> > > @@ -54,6 +55,7 @@ private:\n> > > double currentSdnDeviation_;\n> > > double currentSdnStrength_;\n> > > double currentSdnDeviation2_;\n> > > + double currentCdnDeviation_;\n> > > };\n> > >\n> > > } // namespace RPiController\n> > > --\n> > > 2.39.5\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 708B8BEFBE\n\tfor ;\n\tFri, 15 Aug 2025 07:12:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6181069259;\n\tFri, 15 Aug 2025 09:12:23 +0200 (CEST)","from mail-vk1-xa29.google.com (mail-vk1-xa29.google.com\n\t[IPv6:2607:f8b0:4864:20::a29])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6956E69244\n\tfor ;\n\tFri, 15 Aug 2025 09:12:21 +0200 (CEST)","by mail-vk1-xa29.google.com with SMTP id\n\t71dfb90a1353d-53b175da35bso33953e0c.3\n\tfor ;\n\tFri, 15 Aug 2025 00:12:21 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"oj99XpUu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1755241940; x=1755846740;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=PIEHxtqiBGM1PTglEVrjcvhASY8LzgkywXrHGqfDZ3E=;\n\tb=oj99XpUuodmJJLuQCH3aBVNASqWG93Rf4FqrcEYAlBNafvhalBU0kMGmDA7nMIawj/\n\tY0hjpRfnuZVfZGoDK++TgDcvIWU8l4FWprE7ty6HkKYiI1aZ2KkYvxdEV6p6ls8IWiXk\n\t1g0HXktwRRkJuHce1vx+YqhD8yEOjtxA16U8FxfwauXkvR4Nd5ZwaS0XqZaiV4ZhhF24\n\todKsgqz5dMoW5YkMm9OlyyOVs3ob1KWdv1K7MesxqhxnCc2APHSzUJhfR0cG8bGf7oow\n\too08AtOLkQvVpzysTEWeCsA6OwuQO+SUHtya0NdIEhmMSf1ovPCciOvImfaKE7IHsfRe\n\tTRVg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1755241940; x=1755846740;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=PIEHxtqiBGM1PTglEVrjcvhASY8LzgkywXrHGqfDZ3E=;\n\tb=iRgH8iIDoGtzjVrcV90id+RVj8z3rl0Rm4RNCEAr6Y+/EPLDAwXTIJ1roc0K1t1sp7\n\tpvlEQ2e9CPiyE8egDMTRWKJFHBR14RVfrFLkuOzLF5tWwt2viGqaocBqhAn2lc0l2U1I\n\t+nZzQ9vfb/Moguz/WN3vXoQ96srSrZCh3li15nhFjwh/86DFwLpy8cdIh22i9vakqNR9\n\tXWV+rCrG8R5IVoomJuri0wIRkhpQc+iBdZcncHZvCpvNfXnOkFjjT+WK4DtLItnujkv8\n\tjrrN3D47Ig8mCL2QGm7bghb2Uk6Ja+s34mfJoMb/w9xE54Z0Uc62lAVeSj7lRkRl7hCi\n\tz4Jg==","X-Gm-Message-State":"AOJu0YzSp8lsz2iJc/U/3aVhJMr0WfrTI0Sqjlt5fyRULvoR5YHCQuIl\n\tZqgJlD2n3w7g5TN1iRuRSHLNdHFoBU1IK1WDe2UYCEH692mCY/s/ARG8vqGJKv8VwNetvwGPG60\n\taNWVkJE3yLBxGgDSU8CcNqAqoRbxHRlY/iJchqb81rw==","X-Gm-Gg":"ASbGncvZ9YqRGvQeZRv3GFyVnQr3WlkEnBXi5/NbvwQsJmMGUIk+/alsVs0yN3S/CQ1\n\tvi63cK8MsVdRVJQZtnaXegmV154aFchYmUHHqBQzU7dZIzjBY26B0A13BKuPJvAnthMs8o2vIZ7\n\tglg23FfoTT1ML8WXQ59qZQDcdHgz+veYQu7ht1R+zcutcWYq9P9KCarRlNEpCcKLZi91lVCQ0qy\n\t0H4GdTJfb/rqLoHhIrqgdaxJC8td1lrO566BPY=","X-Google-Smtp-Source":"AGHT+IE3z74jdhP/HcF5dl7IJcsfAOaRkwpkeeaumaCrpUk1SP5Z8yXoBi18cMufbQ1fhgOvUz6jrD5YUUYbUarBcrM=","X-Received":"by 2002:a05:6122:312a:b0:53b:104c:8ef with SMTP id\n\t71dfb90a1353d-53b2b884b44mr63472e0c.2.1755241940030; Fri, 15 Aug 2025\n\t00:12:20 -0700 (PDT)","MIME-Version":"1.0","References":"<20250812133856.3981-1-david.plowman@raspberrypi.com>\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Fri, 15 Aug 2025 08:11:45 +0100","X-Gm-Features":"Ac12FXxSe4NMb-wt9BS62_WOkp2HTJtkXzJ7_he-ZwCwl8IfrWQNpXujW7dJYVU","Message-ID":"","Subject":"Re: [PATCH] ipa: rpi:: denoise: Implement TDN back-off for CDN\n\tdeviation","To":"David Plowman ","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]