[libcamera-devel] [PATCH 5/5] ipa: raspberrypi: Handle control::NoiseReductionMode in the controller
Naushir Patuck
naush at raspberrypi.com
Wed Jan 20 12:20:58 CET 2021
On Wed, 20 Jan 2021 at 11:16, David Plowman <david.plowman at raspberrypi.com>
wrote:
> Hi Naush
>
> On Wed, 20 Jan 2021 at 10:58, Naushir Patuck <naush at raspberrypi.com>
> wrote:
> >
> > Hi David,
> >
> >
> > On Wed, 20 Jan 2021 at 10:29, David Plowman <
> david.plowman at 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 at 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 at 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 at lists.libcamera.org
> >> > https://lists.libcamera.org/listinfo/libcamera-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210120/61cb295e/attachment-0001.htm>
More information about the libcamera-devel
mailing list