<div dir="ltr"><div dir="ltr">Hi Kieran,<div><br></div><div>Thank you for your review feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 22 Jan 2021 at 11:50, Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush,<br>
<br>
On 22/01/2021 09:25, Naushir Patuck wrote:<br>
> This denoise algorithm class will be used to pass in the user requested<br>
> denoise operating mode to the controller. The existing Denoise<br>
> controller will derive from this new DenoiseAlgorithm class.<br>
> <br>
> Add a denoise mode field in the denoise status metadata object for the<br>
> IPA to use when configuring the ISP.<br>
> <br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> ---<br>
> .../controller/denoise_algorithm.hpp | 23 +++++++++++++++++++<br>
> .../raspberrypi/controller/denoise_status.h | 1 +<br>
> src/ipa/raspberrypi/controller/rpi/sdn.cpp | 11 +++++++--<br>
> src/ipa/raspberrypi/controller/rpi/sdn.hpp | 5 +++-<br>
> 4 files changed, 37 insertions(+), 3 deletions(-)<br>
> create mode 100644 src/ipa/raspberrypi/controller/denoise_algorithm.hpp<br>
> <br>
> diff --git a/src/ipa/raspberrypi/controller/denoise_algorithm.hpp b/src/ipa/raspberrypi/controller/denoise_algorithm.hpp<br>
> new file mode 100644<br>
> index 000000000000..df1f35cc9e5f<br>
> --- /dev/null<br>
> +++ b/src/ipa/raspberrypi/controller/denoise_algorithm.hpp<br>
<br>
There's a bit of a mix of .h and .hpp within src/ipa/raspberrypi/ but I<br>
don't think that matters to much here.<br></blockquote><div><br></div><div>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.</div><div><br></div><div>Regards,</div><div>Naush</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Otherwise, nothing controversial.<br>
<br>
Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
<br>
<br>
> @@ -0,0 +1,23 @@<br>
> +/* SPDX-License-Identifier: BSD-2-Clause */<br>
> +/*<br>
> + * Copyright (C) 2021, Raspberry Pi (Trading) Limited<br>
> + *<br>
> + * denoise.hpp - Denoise control algorithm interface<br>
> + */<br>
> +#pragma once<br>
> +<br>
> +#include "algorithm.hpp"<br>
> +<br>
> +namespace RPiController {<br>
> +<br>
> +enum DenoiseMode { Off, ColourOff, ColourFast, ColourHighQuality };<br>
> +<br>
> +class DenoiseAlgorithm : public Algorithm<br>
> +{<br>
> +public:<br>
> + DenoiseAlgorithm(Controller *controller) : Algorithm(controller) {}<br>
> + // A Denoise algorithm must provide the following:<br>
> + virtual void SetMode(DenoiseMode mode) = 0;<br>
> +};<br>
> +<br>
> +} // namespace RPiController<br>
> diff --git a/src/ipa/raspberrypi/controller/denoise_status.h b/src/ipa/raspberrypi/controller/denoise_status.h<br>
> index 06d7cfb91ae8..6064cc2c192e 100644<br>
> --- a/src/ipa/raspberrypi/controller/denoise_status.h<br>
> +++ b/src/ipa/raspberrypi/controller/denoise_status.h<br>
> @@ -16,6 +16,7 @@ struct DenoiseStatus {<br>
> double noise_constant;<br>
> double noise_slope;<br>
> double strength;<br>
> + unsigned int mode;<br>
> };<br>
> <br>
> #ifdef __cplusplus<br>
> diff --git a/src/ipa/raspberrypi/controller/rpi/sdn.cpp b/src/ipa/raspberrypi/controller/rpi/sdn.cpp<br>
> index d8c1521a6633..f85a09afb380 100644<br>
> --- a/src/ipa/raspberrypi/controller/rpi/sdn.cpp<br>
> +++ b/src/ipa/raspberrypi/controller/rpi/sdn.cpp<br>
> @@ -1,6 +1,6 @@<br>
> /* SPDX-License-Identifier: BSD-2-Clause */<br>
> /*<br>
> - * Copyright (C) 2019, Raspberry Pi (Trading) Limited<br>
> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Limited<br>
> *<br>
> * sdn.cpp - SDN (spatial denoise) control algorithm<br>
> */<br>
> @@ -18,7 +18,7 @@ using namespace RPiController;<br>
> #define NAME "rpi.sdn"<br>
> <br>
> Sdn::Sdn(Controller *controller)<br>
> - : Algorithm(controller)<br>
> + : DenoiseAlgorithm(controller), mode_(DenoiseMode::ColourFast)<br>
> {<br>
> }<br>
> <br>
> @@ -48,6 +48,7 @@ void Sdn::Prepare(Metadata *image_metadata)<br>
> status.noise_constant = noise_status.noise_constant * deviation_;<br>
> status.noise_slope = noise_status.noise_slope * deviation_;<br>
> status.strength = strength_;<br>
> + status.mode = mode_;<br>
> image_metadata->Set("denoise.status", status);<br>
> RPI_LOG("Sdn: programmed constant " << status.noise_constant<br>
> << " slope " << status.noise_slope<br>
> @@ -55,6 +56,12 @@ void Sdn::Prepare(Metadata *image_metadata)<br>
> << status.strength);<br>
> }<br>
> <br>
> +void Sdn::SetMode(DenoiseMode mode)<br>
> +{<br>
> + // We only distinguish between off and all other modes.<br>
> + mode_ = mode;<br>
> +}<br>
> +<br>
> // Register algorithm with the system.<br>
> static Algorithm *Create(Controller *controller)<br>
> {<br>
> diff --git a/src/ipa/raspberrypi/controller/rpi/sdn.hpp b/src/ipa/raspberrypi/controller/rpi/sdn.hpp<br>
> index 486c000d7b77..2371ce04163f 100644<br>
> --- a/src/ipa/raspberrypi/controller/rpi/sdn.hpp<br>
> +++ b/src/ipa/raspberrypi/controller/rpi/sdn.hpp<br>
> @@ -7,12 +7,13 @@<br>
> #pragma once<br>
> <br>
> #include "../algorithm.hpp"<br>
> +#include "../denoise_algorithm.hpp"<br>
> <br>
> namespace RPiController {<br>
> <br>
> // Algorithm to calculate correct spatial denoise (SDN) settings.<br>
> <br>
> -class Sdn : public Algorithm<br>
> +class Sdn : public DenoiseAlgorithm<br>
> {<br>
> public:<br>
> Sdn(Controller *controller = NULL);<br>
> @@ -20,10 +21,12 @@ public:<br>
> void Read(boost::property_tree::ptree const ¶ms) override;<br>
> void Initialise() override;<br>
> void Prepare(Metadata *image_metadata) override;<br>
> + void SetMode(DenoiseMode mode) override;<br>
> <br>
> private:<br>
> double deviation_;<br>
> double strength_;<br>
> + DenoiseMode mode_;<br>
> };<br>
> <br>
> } // namespace RPiController<br>
> <br>
<br>
-- <br>
Regards<br>
--<br>
Kieran<br>
</blockquote></div></div>