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

[Kieran Bingham]

On 22/01/2021 13:14, Naushir Patuck wrote:
> Hi Kieran,
> 
> 
> On Fri, 22 Jan 2021 at 12:07, Kieran Bingham
> <kieran.bingham at ideasonboard.com
> <mailto:kieran.bingham at ideasonboard.com>> wrote:
> 
>     Hi Naush,
> 
>     On 22/01/2021 09:25, 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
>     <mailto: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 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: {
>     > +                     RPiController::DenoiseAlgorithm *sdn =
>     dynamic_cast<RPiController::DenoiseAlgorithm *>(
>     > +                             controller_.GetAlgorithm("SDN"));
>     > +                     ASSERT(sdn);
> 
>     I assume the assert means that it's (expected to be) guaranteed to get
>     the algorithm, so we don't need to handle that gracefully.
> 
>     In which case assert is just fine ;-)
> 
> 
> Yes, for now, we guarantee that sdn should be available.  David and I do
> need to work out mechanisms where they may eventually be absent, but we
> are not there yet.
>  
> 
> 
> 
>     > +
>     > +                     int32_t idx = ctrl.second.get<int32_t>();
> 
>     My usual screams at how I dislike std::pair ;-) But that's a rant at
>     C++, not this patch.
> 
> 
>     > +                     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
>     <http://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;
> 
>     Is the ternary required? Isn't
>      (denoiseStatus->mode == RPiController::DenoiseMode::Off) sufficient?
> 
> 
> I think you mean "!=" there :-)

Aha - Just testing ;-) hehe

>  
> 
> 
>     Hrm, it's assigned to a __u32, so I guess in someways it's good to be
>     explicit.
> 
> 
> No, this is not strictly required, but I do prefer to be explicit purely
> as a matter of style.  Hope that is ok.

That's fine with me. Particularly as it's not storing in a boolean.

> 
> Regards,
> Naush
> 
>  
> 
> 
>     Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com
>     <mailto:kieran.bingham at ideasonboard.com>>
> 
>     >       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
>     --
>     Kieran
> 

-- 
Regards
--
Kieran