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

[Naushir Patuck]

Hi Laurent,

Thank you for your review feedback.

On Tue, 26 Jan 2021 at 22:52, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> Hi Naush,
>
> Thank you for the patch.
>
> On Tue, Jan 26, 2021 at 04:24:12PM +0000, Naushir Patuck 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>
> > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.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 220cf174aa4f..8f80bb80c129 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 "denoise_status.h"
> >  #include "dpc_status.h"
> >  #include "focus_status.h"
> > @@ -565,6 +566,7 @@ bool IPARPi::validateIspControls()
> >               V4L2_CID_USER_BCM2835_ISP_SHARPEN,
> >               V4L2_CID_USER_BCM2835_ISP_DPC,
> >               V4L2_CID_USER_BCM2835_ISP_LENS_SHADING,
> > +             V4L2_CID_USER_BCM2835_ISP_CDN,
> >       };
> >
> >       for (auto c : ctrls) {
> > @@ -614,6 +616,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::ColourFast },
> > +     { controls::draft::NoiseReductionModeHighQuality,
> RPiController::DenoiseMode::ColourHighQuality },
> > +     { controls::draft::NoiseReductionModeMinimal,
> RPiController::DenoiseMode::ColourOff },
> > +     { controls::draft::NoiseReductionModeZSL,
> RPiController::DenoiseMode::ColourHighQuality },
> > +};
> > +
> >  void IPARPi::queueRequest(const ControlList &controls)
> >  {
> >       /* Clear the return metadata buffer. */
> > @@ -836,6 +846,22 @@ void IPARPi::queueRequest(const ControlList
> &controls)
> >                       break;
> >               }
> >
> > +             case controls::NOISE_REDUCTION_MODE: {
>
> I may be missing something, but you don't seem to add the control to the
> ControlInfoMap in include/libcamera/ipa/raspberrypi.h, so it won't be
> exposed to applications.
>

This was on purpose - although my thinking was a bit misguided when I wrote
the original patch.  I will add this in now.


>
> > +                     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));
>
>                         auto mode = DenoiseModeTable.find(idx);
>                         if (mode != DenoiseModeTable.end()) {
>                                 sdn->SetMode(mode->second);
>
> will avoid the double lookup.
>

Ack.


>
> > +
>  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()
> > @@ -1087,16 +1113,35 @@ void IPARPi::applyDenoise(const struct
> DenoiseStatus *denoiseStatus, ControlList
> >  {
> >       bcm2835_isp_denoise denoise;
> >
> > -     denoise.enabled = 1;
> > +     denoise.enabled = denoiseStatus->mode ==
> RPiController::DenoiseMode::Off ? 0 : 1;
>
> You could write
>
>         denoise.enabled = denoiseStatus->mode !=
> RPiController::DenoiseMode::Off;
>

Ack


>
> >       denoise.constant = denoiseStatus->noise_constant;
> >       denoise.slope.num = 1000 * denoiseStatus->noise_slope;
> >       denoise.slope.den = 1000;
> >       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::ColourFast:
> > +             cdn.enabled = 1;
> > +             cdn.mode = CDN_MODE_FAST;
> > +             break;
> > +     case RPiController::DenoiseMode::ColourHighQuality:
> > +             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)
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210127/ca3b8a6e/attachment-0001.htm>