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

[Laurent Pinchart]

On Wed, Jan 27, 2021 at 12:47:06AM +0200, Laurent Pinchart wrote:
> Hi Naush,
> 
> Thank you for the patch.
> 
> On Tue, Jan 26, 2021 at 04:24:11PM +0000, 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>
> > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.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
> > @@ -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 };
> 
> Should we consider using enum class here ? Adding an unqualified 'Off'
> identifier to the namespace seems quite prone to namespace clashes.
> 
> (Additionally we usually split enums with one entry per line, but the
> RPi IPA doesn't use the same coding style, so it's up to you.)

Another question, what's the difference between ColourFast and
ColourHighQuality ? How fast is fast, or how slow is high quality ?

> > +
> > +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;
> 
> Should this be of type DenoiseMode ?
> 
> >  };
> >  
> >  #ifdef __cplusplus
> > diff --git a/src/ipa/raspberrypi/controller/rpi/sdn.cpp b/src/ipa/raspberrypi/controller/rpi/sdn.cpp
> > index d8c1521a6633..ecc7845eda4c 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::ColourOff)
> >  {
> >  }
> >  
> > @@ -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,

Laurent Pinchart