<div dir="ltr"><div dir="ltr">Hi David,<div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 20 Jan 2021 at 10:29, David Plowman <<a href="mailto:david.plowman@raspberrypi.com">david.plowman@raspberrypi.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush<br>
<br>
Thanks for this patch (and the previous one, namely 4/5). I'm afraid<br>
I'd quite like to have just a small meta-discussion about this one<br>
first. Only a small one, though!<br>
<br>
Reading these patches it seems like an application setting<br>
NoiseReductionModeOff will still get regular spatial denoise, just not<br>
colour denoise. Is that right? Under such circumstances I think it<br>
would be clearer to rename DenoiseMode as ColourDenoiseMode.<br>
<br>
Having said that, if we're going to support NoiseReductionModeOff,<br>
then we should perhaps make it disable all denoising? I suspect folks<br>
would find it "surprising" if it didn't do that. As such, maybe<br>
DenoiseMode would remain as DenoiseMode (not ColourDenoiseMode) and<br>
we'd have 4 values, namely:<br>
<br>
DenoiseMode::Off - really everything off<br>
DenoiseMode::ColourOff - colour denoise off but regular spatial denoise on<br>
DenoiseMode::ColorFast - with fast colour denoise<br>
DenoiseMode::ColorHighQuality - with slow colour denoise<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">(Setting the denoise strength to zero will disable spatial denoise.)<br>
<br>
What do you think?<br></blockquote><div><br></div><div><div>Yes, I think this is a reasonable thing to do. I will make the appropriate changes in the next revisions.</div><div>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?</div></div><div><br></div><div>Regards,</div><div>Naush</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
On Wed, 20 Jan 2021 at 08:35, Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>> wrote:<br>
><br>
> The application provided noise reduction mode gets passed into the<br>
> denoise controller. The denoise controller in turn returns the mode to<br>
> the IPA which now sets up the colour denoise processing appropriately.<br>
><br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> ---<br>
> src/ipa/raspberrypi/raspberrypi.cpp | 47 ++++++++++++++++++++++++++++-<br>
> 1 file changed, 46 insertions(+), 1 deletion(-)<br>
><br>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> index 8af3d603a3ff..91041fa68930 100644<br>
> --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> @@ -43,6 +43,7 @@<br>
> #include "contrast_algorithm.hpp"<br>
> #include "contrast_status.h"<br>
> #include "controller.hpp"<br>
> +#include "denoise_algorithm.hpp"<br>
> #include "dpc_status.h"<br>
> #include "focus_status.h"<br>
> #include "geq_status.h"<br>
> @@ -553,7 +554,8 @@ bool IPARPi::validateIspControls()<br>
> V4L2_CID_USER_BCM2835_ISP_DENOISE,<br>
> V4L2_CID_USER_BCM2835_ISP_SHARPEN,<br>
> V4L2_CID_USER_BCM2835_ISP_DPC,<br>
> - V4L2_CID_USER_BCM2835_ISP_LENS_SHADING<br>
> + V4L2_CID_USER_BCM2835_ISP_LENS_SHADING,<br>
> + V4L2_CID_USER_BCM2835_ISP_CDN,<br>
> };<br>
><br>
> for (auto c : ctrls) {<br>
> @@ -603,6 +605,14 @@ static const std::map<int32_t, std::string> AwbModeTable = {<br>
> { controls::AwbCustom, "custom" },<br>
> };<br>
><br>
> +static const std::map<int32_t, RPiController::DenoiseMode> DenoiseModeTable = {<br>
> + { controls::draft::NoiseReductionModeOff, RPiController::DenoiseMode::Off },<br>
> + { controls::draft::NoiseReductionModeFast, RPiController::DenoiseMode::Fast },<br>
> + { controls::draft::NoiseReductionModeHighQuality, RPiController::DenoiseMode::HighQuality },<br>
> + { controls::draft::NoiseReductionModeMinimal, RPiController::DenoiseMode::Fast },<br>
<br>
If we go with a revised definition of DenoiseMode, maybe this one<br>
would be DenoiseMode::ColourOff?<br>
<br>
Anyway, thanks for all this work. Best regards<br>
David<br>
<br>
> + { controls::draft::NoiseReductionModeZSL, RPiController::DenoiseMode::HighQuality },<br>
> +};<br>
> +<br>
> void IPARPi::queueRequest(const ControlList &controls)<br>
> {<br>
> /* Clear the return metadata buffer. */<br>
> @@ -806,6 +816,22 @@ void IPARPi::queueRequest(const ControlList &controls)<br>
> break;<br>
> }<br>
><br>
> + case controls::NOISE_REDUCTION_MODE: {<br>
> + RPiController::DenoiseAlgorithm *sdn = dynamic_cast<RPiController::DenoiseAlgorithm *>(<br>
> + controller_.GetAlgorithm("SDN"));<br>
> + ASSERT(sdn);<br>
> +<br>
> + int32_t idx = ctrl.second.get<int32_t>();<br>
> + if (DenoiseModeTable.count(idx)) {<br>
> + sdn->SetMode(DenoiseModeTable.at(idx));<br>
> + libcameraMetadata_.set(controls::draft::NoiseReductionMode, idx);<br>
> + } else {<br>
> + LOG(IPARPI, Error) << "Noise reduction mode " << idx<br>
> + << " not recognised";<br>
> + }<br>
> + break;<br>
> + }<br>
> +<br>
> default:<br>
> LOG(IPARPI, Warning)<br>
> << "Ctrl " << controls::<a href="http://controls.at" rel="noreferrer" target="_blank">controls.at</a>(ctrl.first)->name()<br>
> @@ -1052,9 +1078,28 @@ void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ct<br>
> denoise.strength.num = 1000 * denoiseStatus->strength;<br>
> denoise.strength.den = 1000;<br>
><br>
> + /* Set the CDN mode to match the SDN operating mode. */<br>
> + bcm2835_isp_cdn cdn;<br>
> + switch (denoiseStatus->mode) {<br>
> + case RPiController::DenoiseMode::Fast:<br>
> + cdn.enabled = 1;<br>
> + cdn.mode = CDN_MODE_FAST;<br>
> + break;<br>
> + case RPiController::DenoiseMode::HighQuality:<br>
> + cdn.enabled = 1;<br>
> + cdn.mode = CDN_MODE_HIGH_QUALITY;<br>
> + break;<br>
> + default:<br>
> + cdn.enabled = 0;<br>
> + }<br>
> +<br>
> ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&denoise),<br>
> sizeof(denoise) });<br>
> ctrls.set(V4L2_CID_USER_BCM2835_ISP_DENOISE, c);<br>
> +<br>
> + c = ControlValue(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&cdn),<br>
> + sizeof(cdn) });<br>
> + ctrls.set(V4L2_CID_USER_BCM2835_ISP_CDN, c);<br>
> }<br>
><br>
> void IPARPi::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls)<br>
> --<br>
> 2.25.1<br>
><br>
> _______________________________________________<br>
> libcamera-devel mailing list<br>
> <a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
> <a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
</blockquote></div></div>