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