<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 20 Jan 2021 at 11:16, 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>
On Wed, 20 Jan 2021 at 10:58, Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>> wrote:<br>
><br>
> Hi David,<br>
><br>
><br>
> On Wed, 20 Jan 2021 at 10:29, David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>> wrote:<br>
>><br>
>> 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>
>><br>
>> (Setting the denoise strength to zero will disable spatial denoise.)<br>
>><br>
>> What do you think?<br>
><br>
><br>
> Yes, I think this is a reasonable thing to do. I will make the appropriate changes in the next revisions.<br>
> 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?<br>
<br>
Whichever you prefer, I don't think it should matter. Famous last words!! :)<br></blockquote><div><br></div><div>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?</div><div><br></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>
Best regards<br>
David<br>
<br>
><br>
> Regards,<br>
> Naush<br>
><br>
><br>
>><br>
>><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>