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

[David Plowman]

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 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