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

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

Commit Message

Naushir Patuck Jan. 26, 2021, 4:24 p.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>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
Reviewed-by: Kieran Bingham <kieran.bingham@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

Comments

Laurent Pinchart Jan. 26, 2021, 10:47 p.m. UTC | #1 
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@raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@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.)

> +
> +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
Laurent Pinchart Jan. 26, 2021, 10:54 p.m. UTC | #2 
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@raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@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
Naushir Patuck Jan. 27, 2021, 8:41 a.m. UTC | #3 
Hi Laurent,

On Tue, 26 Jan 2021 at 22:54, Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> 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@raspberrypi.com>
> > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > > Reviewed-by: Kieran Bingham <kieran.bingham@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.
>

Yes, you are right.  My first implementation did have this as an enum
class.  I opted to change because we have to cast out to an int later on.
But I do prefer enum class, so I will revert back.


> >
> > (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 ?
>

High quality denoise is just what it says on the tin.  It analyses and
filters the output image.  However, this is slow in processing due to the
complex filtering operations, and really only wants to be used for a stills
image capture.  Throughput numbers are based on clock speed, but typically
you would be talking about a factor of 10 slower than the typical pixel
processing rate.

Fast colour denoise analyses a 1/4 resolution image and filters out the
full resolution output image.  This gives us the faster throughput we need
when running viewfinder or video recording. Additionally, the filtering is
less complex than the high quality mode, so is faster yet.  Again, throughput
numbers are based on clock speed, but you would only see a typical under 2x
slowdown over the typical pixel processing rate.


>
> > > +
> > > +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 ?
>

This is meant to be a C interoparable header file.  But perhaps that does
not matter so much, I will have to discuss this with David.  If I switch to
enum class for DenoiseMode (as above), this will have to remain integer
based, or is there an alternative?

Regards,
Naush


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

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..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