[libcamera-devel] [PATCH v2 5/6] ipa: raspberrypi: Add a DenoiseAlgorithm class to the Controller
Naushir Patuck
naush at raspberrypi.com
Fri Jan 22 13:34:40 CET 2021
Hi David,
On Fri, 22 Jan 2021 at 12:14, David Plowman <david.plowman at 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 at 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 at 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 at 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 ¶ms) 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 at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210122/1f9f8ee4/attachment-0001.htm>
More information about the libcamera-devel
mailing list