[libcamera-devel,v2,5/6] ipa: raspberrypi: Add a DenoiseAlgorithm class to the Controller
diff mbox series

Message ID 20210122092537.708375-6-naush@raspberrypi.com
State Superseded
Headers show
Series
  • Raspberry Pi: Colour denoise
Related show

Commit Message

Naushir Patuck Jan. 22, 2021, 9:25 a.m. UTC 
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@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

Comments

David Plowman Jan. 22, 2021, 11:49 a.m. UTC | #1 
Hi Naush

Thanks for this patch. Don't think I have anything to add on this one!

Reviewed-by: David Plowman <david.plowman@raspberrypi.com>

Best regards
David

On Fri, 22 Jan 2021 at 09:25, Naushir Patuck <naush@raspberrypi.com> 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@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
> @@ -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
> --
> 2.25.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Kieran Bingham Jan. 22, 2021, 11:50 a.m. UTC | #2 
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@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.

Otherwise, nothing controversial.

Reviewed-by: Kieran Bingham <kieran.bingham@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
>
David Plowman Jan. 22, 2021, 12:14 p.m. UTC | #3 
Hi again Naush

I know I okayed this but maybe I can raise just one more little thing? (sorry!)

On Fri, 22 Jan 2021 at 11:50, Kieran Bingham
<kieran.bingham@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@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.
>
> Otherwise, nothing controversial.
>
> Reviewed-by: Kieran Bingham <kieran.bingham@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)

Could we maybe default this to ColourOff? This would mean that, when
this patch goes in, it will make no difference to the behaviour of the
libcamera-apps. Otherwise I'm slightly nervous that behaviour will
change unexpectedly for people who are just trying those apps for the
first time, until we update our apps to set this explicitly.

Actually I think it's a better default as folks will get the
framerates the camera modes promise, and would have to make an active
choice if they're prepared to trade that for better colour denoise.

On the related subject of updating the libcamera-apps once this patch
set it in, will you do that? :)

Thanks!
David

> >  {
> >  }
> >
> > @@ -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
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Naushir Patuck Jan. 22, 2021, 12:34 p.m. UTC | #4 
Hi David,

On Fri, 22 Jan 2021 at 12:14, David Plowman <david.plowman@raspberrypi.com>
wrote:

> Hi again Naush
>
> I know I okayed this but maybe I can raise just one more little thing?
> (sorry!)
>
> On Fri, 22 Jan 2021 at 11:50, Kieran Bingham
> <kieran.bingham@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@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.
> >
> > Otherwise, nothing controversial.
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham@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)
>
> Could we maybe default this to ColourOff? This would mean that, when
> this patch goes in, it will make no difference to the behaviour of the
> libcamera-apps. Otherwise I'm slightly nervous that behaviour will
> change unexpectedly for people who are just trying those apps for the
> first time, until we update our apps to set this explicitly.
>
> Actually I think it's a better default as folks will get the
> framerates the camera modes promise, and would have to make an active
> choice if they're prepared to trade that for better colour denoise.
>

Sure, that makes sense, I'll prepare a change for that.


> On the related subject of updating the libcamera-apps once this patch
> set it in, will you do that? :)
>

Sure, I can have a look at that soon-ish.

Regards,
Naush



>
> Thanks!
> David
>
> > >  {
> > >  }
> > >
> > > @@ -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
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
Naushir Patuck Jan. 22, 2021, 12:46 p.m. UTC | #5 
Hi Kieran,

Thank you for your review feedback.

On Fri, 22 Jan 2021 at 11:50, Kieran Bingham <
kieran.bingham@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@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@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
>

Patch
diff mbox series 

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 };
+
+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