[libcamera-devel] [PATCH v6 4/6] ipa: raspberrypi: Rename SdnStatus to DenoiseStatus

[Naushir Patuck]

Hi Laurent,


On Tue, 9 Feb 2021 at 00:37, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> Hi Naush,
>
> Thank you for the patch.
>
> On Mon, Feb 08, 2021 at 03:07:36PM +0000, Naushir Patuck wrote:
> > This change is in anticipation of the addition of a DenoiseAlgorithm
> > base class which the SDN class will derive from. We want to match the
> > metadata object name with the base class algorithm name.
> >
> > This renames:
> > - SdnStatus metadata object to DenoiseStatus
> > - "sdn.status" metadata string key to "denoise.status"
> > - sdn_status.h header file to denoise_status.h
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  .../controller/{sdn_status.h => denoise_status.h}         | 8 ++++----
> >  src/ipa/raspberrypi/controller/rpi/sdn.cpp                | 4 ++--
> >  src/ipa/raspberrypi/raspberrypi.cpp                       | 8 ++++----
> >  3 files changed, 10 insertions(+), 10 deletions(-)
> >  rename src/ipa/raspberrypi/controller/{sdn_status.h =>
> denoise_status.h} (50%)
> >
> > diff --git a/src/ipa/raspberrypi/controller/sdn_status.h
> b/src/ipa/raspberrypi/controller/denoise_status.h
> > similarity index 50%
> > rename from src/ipa/raspberrypi/controller/sdn_status.h
> > rename to src/ipa/raspberrypi/controller/denoise_status.h
> > index 871e0b62af1f..94f5df200ffd 100644
> > --- a/src/ipa/raspberrypi/controller/sdn_status.h
> > +++ b/src/ipa/raspberrypi/controller/denoise_status.h
> > @@ -1,18 +1,18 @@
> >  /* SPDX-License-Identifier: BSD-2-Clause */
> >  /*
> > - * Copyright (C) 2019, Raspberry Pi (Trading) Limited
> > + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Limited
> >   *
> > - * sdn_status.h - SDN (spatial denoise) control algorithm status
> > + * denoise_status.h - Denoise control algorithm status
> >   */
> >  #pragma once
> >
> > -// This stores the parameters required for Spatial Denoise (SDN).
> > +// This stores the parameters required for Denoise (SDN).
>
> Should we drop "(SDN)" ?
>
> >
> >  #ifdef __cplusplus
> >  extern "C" {
> >  #endif
> >
> > -struct SdnStatus {
> > +struct DenoiseStatus {
> >       double noise_constant;
> >       double noise_slope;
> >       double strength;
> > diff --git a/src/ipa/raspberrypi/controller/rpi/sdn.cpp
> b/src/ipa/raspberrypi/controller/rpi/sdn.cpp
> > index 0fad25504345..ef84a2dcbf86 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/sdn.cpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/sdn.cpp
> > @@ -7,8 +7,8 @@
> >
> >  #include "libcamera/internal/log.h"
> >
> > +#include "../denoise_status.h"
> >  #include "../noise_status.h"
> > -#include "../sdn_status.h"
> >
> >  #include "sdn.hpp"
> >
> > @@ -49,7 +49,7 @@ void Sdn::Prepare(Metadata *image_metadata)
> >       LOG(RPiSdn, Debug)
> >               << "Noise profile: constant " <<
> noise_status.noise_constant
> >               << " slope " << noise_status.noise_slope;
> > -     struct SdnStatus status;
> > +     struct DenoiseStatus status;
> >       status.noise_constant = noise_status.noise_constant * deviation_;
> >       status.noise_slope = noise_status.noise_slope * deviation_;
> >       status.strength = strength_;
>
> v5 used to rename the metadata entry here:
>
> -       image_metadata->Set("sdn.status", status);
> +       image_metadata->Set("denoise.status", status);
>
> Isn't it still needed ? I can add this back when applying.
>

Yes, it is!  Thank you for catching that.
I'm sorry, this was finger trouble on my part.  I did have this fixed
locally, but evidently it was staged and I didn't commit it before sending
:-(

Regards,
Naush



>
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > index ff14cfc4b706..e76688a7b323 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -43,13 +43,13 @@
> >  #include "contrast_algorithm.hpp"
> >  #include "contrast_status.h"
> >  #include "controller.hpp"
> > +#include "denoise_status.h"
> >  #include "dpc_status.h"
> >  #include "focus_status.h"
> >  #include "geq_status.h"
> >  #include "lux_status.h"
> >  #include "metadata.hpp"
> >  #include "noise_status.h"
> > -#include "sdn_status.h"
> >  #include "sharpen_algorithm.hpp"
> >  #include "sharpen_status.h"
> >
> > @@ -110,7 +110,7 @@ private:
> >       void applyBlackLevel(const struct BlackLevelStatus
> *blackLevelStatus, ControlList &ctrls);
> >       void applyGamma(const struct ContrastStatus *contrastStatus,
> ControlList &ctrls);
> >       void applyGEQ(const struct GeqStatus *geqStatus, ControlList
> &ctrls);
> > -     void applyDenoise(const struct SdnStatus *denoiseStatus,
> ControlList &ctrls);
> > +     void applyDenoise(const struct DenoiseStatus *denoiseStatus,
> ControlList &ctrls);
> >       void applySharpen(const struct SharpenStatus *sharpenStatus,
> ControlList &ctrls);
> >       void applyDPC(const struct DpcStatus *dpcStatus, ControlList
> &ctrls);
> >       void applyLS(const struct AlscStatus *lsStatus, ControlList
> &ctrls);
> > @@ -952,7 +952,7 @@ void IPARPi::prepareISP(unsigned int bufferId)
> >               if (geqStatus)
> >                       applyGEQ(geqStatus, ctrls);
> >
> > -             SdnStatus *denoiseStatus =
> rpiMetadata_.GetLocked<SdnStatus>("sdn.status");
> > +             DenoiseStatus *denoiseStatus =
> rpiMetadata_.GetLocked<DenoiseStatus>("denoise.status");
> >               if (denoiseStatus)
> >                       applyDenoise(denoiseStatus, ctrls);
> >
> > @@ -1171,7 +1171,7 @@ void IPARPi::applyGEQ(const struct GeqStatus
> *geqStatus, ControlList &ctrls)
> >       ctrls.set(V4L2_CID_USER_BCM2835_ISP_GEQ, c);
> >  }
> >
> > -void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus,
> ControlList &ctrls)
> > +void IPARPi::applyDenoise(const struct DenoiseStatus *denoiseStatus,
> ControlList &ctrls)
> >  {
> >       bcm2835_isp_denoise denoise;
> >
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210209/e856b0c1/attachment.htm>