[libcamera-devel] [PATCH v2 2/3] ipa: raspberrypi: Clean up NoiseReductionMode values

[David Plowman]

Hi Paul, Kieran

Thank you for this patch!

On Mon, 3 Jan 2022 at 23:21, Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> Quoting Paul Elder (2021-12-21 05:10:22)
> > Remove the NoiseReductionMode values that the raspberrypi IPA does not
> > support. The ControlInfo values that the IPA reports will be used for
> > capability detection, so values that it does not support shall be
> > removed.
> >
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> >
> > ---
> > No change in v2:
> > - constexpr did not work
> > ---
> >  include/libcamera/ipa/raspberrypi.h | 8 +++++++-
> >  src/ipa/raspberrypi/raspberrypi.cpp | 2 --
> >  2 files changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> > index 548bfba0..593139c5 100644
> > --- a/include/libcamera/ipa/raspberrypi.h
> > +++ b/include/libcamera/ipa/raspberrypi.h
> > @@ -18,6 +18,12 @@ namespace libcamera {
> >
> >  namespace RPi {
> >
> > +const std::array<const ControlValue, 3> RPiNoiseReductionModeValues = {
> > +       static_cast<int32_t>(controls::NoiseReductionModeOff),
> > +       static_cast<int32_t>(controls::NoiseReductionModeFast),
> > +       static_cast<int32_t>(controls::NoiseReductionModeHighQuality),
> > +};

Just one thing, didn't we support "NoiseReductionModeMinimal" and
"NoiseReductionMode ZSL" too? (Though the former is more important to
us.) So I'm thinking those need to be in the list here as well.

Best regards
David

> > +
> >  /*
> >   * List of controls handled by the Raspberry Pi IPA
> >   *
> > @@ -46,7 +52,7 @@ static const ControlInfoMap Controls({
> >                 { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
> >                 { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
> >                 { &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },
> > -               { &controls::NoiseReductionMode, ControlInfo(controls::NoiseReductionModeValues) }
> > +               { &controls::NoiseReductionMode, ControlInfo(RPiNoiseReductionModeValues) }
>
> Aha, I didn't realise we could liimit the exposed values from controls
> enums like this.
>
> I'll have to keep an eye on how things check to ensure the control value
> is supported, but it makes sense that it should only report the modes it
> supports.
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> >         }, controls::controls);
> >
> >  } /* namespace RPi */
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 3f497be1..e0685c69 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -618,8 +618,6 @@ static const std::map<int32_t, RPiController::DenoiseMode> DenoiseModeTable = {
> >         { controls::NoiseReductionModeOff, RPiController::DenoiseMode::Off },
> >         { controls::NoiseReductionModeFast, RPiController::DenoiseMode::ColourFast },
> >         { controls::NoiseReductionModeHighQuality, RPiController::DenoiseMode::ColourHighQuality },
> > -       { controls::NoiseReductionModeRaw, RPiController::DenoiseMode::ColourOff },
> > -       { controls::NoiseReductionModeZSL, RPiController::DenoiseMode::ColourHighQuality },
> >  };
> >
> >  void IPARPi::queueRequest(const ControlList &controls)
> > --
> > 2.27.0
> >