[libcamera-devel] [PATCH v2 5/6] ipa: raspberrypi: Add a DenoiseAlgorithm class to the Controller

[Naushir Patuck]

Hi Kieran,

Thank you for your review feedback.

On Fri, 22 Jan 2021 at 11:50, Kieran Bingham <
kieran.bingham at ideasonboard.com> wrote:

> Hi Naush,
>
> On 22/01/2021 09:25, Naushir Patuck wrote:
> > This denoise algorithm class will be used to pass in the user requested
> > denoise operating mode to the controller.  The existing Denoise
> > controller will derive from this new DenoiseAlgorithm class.
> >
> > Add a denoise mode field in the denoise status metadata object for the
> > IPA to use when configuring the ISP.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> >  .../controller/denoise_algorithm.hpp          | 23 +++++++++++++++++++
> >  .../raspberrypi/controller/denoise_status.h   |  1 +
> >  src/ipa/raspberrypi/controller/rpi/sdn.cpp    | 11 +++++++--
> >  src/ipa/raspberrypi/controller/rpi/sdn.hpp    |  5 +++-
> >  4 files changed, 37 insertions(+), 3 deletions(-)
> >  create mode 100644 src/ipa/raspberrypi/controller/denoise_algorithm.hpp
> >
> > diff --git a/src/ipa/raspberrypi/controller/denoise_algorithm.hpp
> b/src/ipa/raspberrypi/controller/denoise_algorithm.hpp
> > new file mode 100644
> > index 000000000000..df1f35cc9e5f
> > --- /dev/null
> > +++ b/src/ipa/raspberrypi/controller/denoise_algorithm.hpp
>
> There's a bit of a mix of .h and .hpp within src/ipa/raspberrypi/ but I
> don't think that matters to much here.
>

Sadly, yes there is.  Our controller code uses .hpp throughout, and this
file really ought to follow.  We do have a non-trivial task of reformatting
all our controller code to better match with the libcamera formatting
style, so we ought to add renaming .hpp to .h to that list.

Regards,
Naush


>
> Otherwise, nothing controversial.
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
>
> > @@ -0,0 +1,23 @@
> > +/* SPDX-License-Identifier: BSD-2-Clause */
> > +/*
> > + * Copyright (C) 2021, Raspberry Pi (Trading) Limited
> > + *
> > + * denoise.hpp - Denoise control algorithm interface
> > + */
> > +#pragma once
> > +
> > +#include "algorithm.hpp"
> > +
> > +namespace RPiController {
> > +
> > +enum DenoiseMode { Off, ColourOff, ColourFast, ColourHighQuality };
> > +
> > +class DenoiseAlgorithm : public Algorithm
> > +{
> > +public:
> > +     DenoiseAlgorithm(Controller *controller) : Algorithm(controller) {}
> > +     // A Denoise algorithm must provide the following:
> > +     virtual void SetMode(DenoiseMode mode) = 0;
> > +};
> > +
> > +} // namespace RPiController
> > diff --git a/src/ipa/raspberrypi/controller/denoise_status.h
> b/src/ipa/raspberrypi/controller/denoise_status.h
> > index 06d7cfb91ae8..6064cc2c192e 100644
> > --- a/src/ipa/raspberrypi/controller/denoise_status.h
> > +++ b/src/ipa/raspberrypi/controller/denoise_status.h
> > @@ -16,6 +16,7 @@ struct DenoiseStatus {
> >       double noise_constant;
> >       double noise_slope;
> >       double strength;
> > +     unsigned int mode;
> >  };
> >
> >  #ifdef __cplusplus
> > diff --git a/src/ipa/raspberrypi/controller/rpi/sdn.cpp
> b/src/ipa/raspberrypi/controller/rpi/sdn.cpp
> > index d8c1521a6633..f85a09afb380 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/sdn.cpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/sdn.cpp
> > @@ -1,6 +1,6 @@
> >  /* SPDX-License-Identifier: BSD-2-Clause */
> >  /*
> > - * Copyright (C) 2019, Raspberry Pi (Trading) Limited
> > + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Limited
> >   *
> >   * sdn.cpp - SDN (spatial denoise) control algorithm
> >   */
> > @@ -18,7 +18,7 @@ using namespace RPiController;
> >  #define NAME "rpi.sdn"
> >
> >  Sdn::Sdn(Controller *controller)
> > -     : Algorithm(controller)
> > +     : DenoiseAlgorithm(controller), mode_(DenoiseMode::ColourFast)
> >  {
> >  }
> >
> > @@ -48,6 +48,7 @@ void Sdn::Prepare(Metadata *image_metadata)
> >       status.noise_constant = noise_status.noise_constant * deviation_;
> >       status.noise_slope = noise_status.noise_slope * deviation_;
> >       status.strength = strength_;
> > +     status.mode = mode_;
> >       image_metadata->Set("denoise.status", status);
> >       RPI_LOG("Sdn: programmed constant " << status.noise_constant
> >                                           << " slope " <<
> status.noise_slope
> > @@ -55,6 +56,12 @@ void Sdn::Prepare(Metadata *image_metadata)
> >                                           << status.strength);
> >  }
> >
> > +void Sdn::SetMode(DenoiseMode mode)
> > +{
> > +     // We only distinguish between off and all other modes.
> > +     mode_ = mode;
> > +}
> > +
> >  // Register algorithm with the system.
> >  static Algorithm *Create(Controller *controller)
> >  {
> > diff --git a/src/ipa/raspberrypi/controller/rpi/sdn.hpp
> b/src/ipa/raspberrypi/controller/rpi/sdn.hpp
> > index 486c000d7b77..2371ce04163f 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/sdn.hpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/sdn.hpp
> > @@ -7,12 +7,13 @@
> >  #pragma once
> >
> >  #include "../algorithm.hpp"
> > +#include "../denoise_algorithm.hpp"
> >
> >  namespace RPiController {
> >
> >  // Algorithm to calculate correct spatial denoise (SDN) settings.
> >
> > -class Sdn : public Algorithm
> > +class Sdn : public DenoiseAlgorithm
> >  {
> >  public:
> >       Sdn(Controller *controller = NULL);
> > @@ -20,10 +21,12 @@ public:
> >       void Read(boost::property_tree::ptree const &params) override;
> >       void Initialise() override;
> >       void Prepare(Metadata *image_metadata) override;
> > +     void SetMode(DenoiseMode mode) override;
> >
> >  private:
> >       double deviation_;
> >       double strength_;
> > +     DenoiseMode mode_;
> >  };
> >
> >  } // namespace RPiController
> >
>
> --
> Regards
> --
> Kieran
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210122/521762d6/attachment.htm>