<div dir="ltr"><div dir="ltr">Hi David,<div><br></div><div>On Fri, 22 Jan 2021 at 12:14, David Plowman <<a href="mailto:david.plowman@raspberrypi.com">david.plowman@raspberrypi.com</a>> wrote:<br></div></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi again Naush<br>
<br>
I know I okayed this but maybe I can raise just one more little thing? (sorry!)<br>
<br>
On Fri, 22 Jan 2021 at 11:50, Kieran Bingham<br>
<<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>> wrote:<br>
><br>
> 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>
><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>
Could we maybe default this to ColourOff? This would mean that, when<br>
this patch goes in, it will make no difference to the behaviour of the<br>
libcamera-apps. Otherwise I'm slightly nervous that behaviour will<br>
change unexpectedly for people who are just trying those apps for the<br>
first time, until we update our apps to set this explicitly.<br>
<br>
Actually I think it's a better default as folks will get the<br>
framerates the camera modes promise, and would have to make an active<br>
choice if they're prepared to trade that for better colour denoise.<br></blockquote><div><br></div><div>Sure, that makes sense, I'll prepare a change for that. </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">
On the related subject of updating the libcamera-apps once this patch<br>
set it in, will you do that? :)<br></blockquote><div><br></div><div>Sure, I can have a look at that soon-ish.</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></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>
Thanks!<br>
David<br>
<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>
> _______________________________________________<br>
> libcamera-devel mailing list<br>
> <a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
> <a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
</blockquote></div></div>