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

[David Plowman]

HI Kieran

Thanks for the reply.

On Tue, 4 Jan 2022 at 10:33, Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> Quoting David Plowman (2022-01-04 08:27:49)
> > 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
>
> Is 'Minimal' different to 'Fast' somehow?

Yes, in Raspberry PI world "fast" means to use the "fast software
colour denoise algorithm" whereas "minimal" means "no software colour
denoise at all". (There's also a "high quality" but slower version of
software colour denoise.)

I think we need to keep "minimal" as an option, but as you say, ZSL
doesn't bother us for the moment.

Thanks
David

>
> > "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.
>
> In this series, ZSL is defined as being used when reprocessing, so it
> will only apply NoiseReduction on low resolution streams (view finders)
> assuming that a RAW stream will also be provided which will be de-noised
> on only later processed frames.
>
> As I understand it, this patch is directly removing the listing that ZSL
> is available, and listing direct matches of what is exposed by the RPi
> IPA modes?
>
>
> >
> > 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
> > > >