[libcamera-devel] [PATCH 5/5] ipa: raspberrypi: Handle control::NoiseReductionMode in the controller

[Naushir Patuck]

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?

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/9ba74c86/attachment-0001.htm>