<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:46, 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 change is in anticipation of the addition of a DenoiseAlgorithm<br>
> base class which the SDN class will derive from. We want to match the<br>
> metadata object name with the base class algorithm name.<br>
> <br>
> This renames:<br>
> - SdnStatus metadata object to DenoiseStatus<br>
> - "sdn.status" metadata string key to "denoise.status"<br>
> - sdn_status.h header file to denoise_status.h<br>
> <br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> ---<br>
>  .../controller/{sdn_status.h => denoise_status.h}         | 6 +++---<br>
>  src/ipa/raspberrypi/controller/rpi/sdn.cpp                | 6 +++---<br>
>  src/ipa/raspberrypi/raspberrypi.cpp                       | 8 ++++----<br>
>  3 files changed, 10 insertions(+), 10 deletions(-)<br>
>  rename src/ipa/raspberrypi/controller/{sdn_status.h => denoise_status.h} (65%)<br>
> <br>
> diff --git a/src/ipa/raspberrypi/controller/sdn_status.h b/src/ipa/raspberrypi/controller/denoise_status.h<br>
> similarity index 65%<br>
> rename from src/ipa/raspberrypi/controller/sdn_status.h<br>
> rename to src/ipa/raspberrypi/controller/denoise_status.h<br>
> index 871e0b62af1f..06d7cfb91ae8 100644<br>
> --- a/src/ipa/raspberrypi/controller/sdn_status.h<br>
> +++ b/src/ipa/raspberrypi/controller/denoise_status.h<br>
> @@ -1,8 +1,8 @@<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_status.h - SDN (spatial denoise) control algorithm status<br>
> + * denoise_status.h - Spatial Denoise control algorithm status<br>
>   */<br>
>  #pragma once<br>
>  <br>
> @@ -12,7 +12,7 @@<br>
>  extern "C" {<br>
>  #endif<br>
>  <br>
> -struct SdnStatus {<br>
> +struct DenoiseStatus {<br>
>       double noise_constant;<br>
>       double noise_slope;<br>
>       double strength;<br>
> diff --git a/src/ipa/raspberrypi/controller/rpi/sdn.cpp b/src/ipa/raspberrypi/controller/rpi/sdn.cpp<br>
<br>
This is just the status rename then, so I guess no need to do a full<br>
rename of all SDN->denoise?<br>
<br>
Anyway, it's just a query.<br></blockquote><div><br></div><div>No, we do not want a wholesale rename.  David and I did discuss this separately.  The goal is to mach the name "Denoise" with the "DenoiseAlgorithm" class in the next patch set.  SDN as a controller is currently the only derived denoise controller, but that may not be the case in the future, if that makes sense?</div><div><br></div><div>Regards,<br>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>
Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
<br>
<br>
> index aa82830b02b4..d8c1521a6633 100644<br>
> --- a/src/ipa/raspberrypi/controller/rpi/sdn.cpp<br>
> +++ b/src/ipa/raspberrypi/controller/rpi/sdn.cpp<br>
> @@ -5,8 +5,8 @@<br>
>   * sdn.cpp - SDN (spatial denoise) control algorithm<br>
>   */<br>
>  <br>
> +#include "../denoise_status.h"<br>
>  #include "../noise_status.h"<br>
> -#include "../sdn_status.h"<br>
>  <br>
>  #include "sdn.hpp"<br>
>  <br>
> @@ -44,11 +44,11 @@ void Sdn::Prepare(Metadata *image_metadata)<br>
>       RPI_LOG("Noise profile: constant " << noise_status.noise_constant<br>
>                                          << " slope "<br>
>                                          << noise_status.noise_slope);<br>
> -     struct SdnStatus status;<br>
> +     struct DenoiseStatus status;<br>
>       status.noise_constant = noise_status.noise_constant * deviation_;<br>
>       status.noise_slope = noise_status.noise_slope * deviation_;<br>
>       status.strength = strength_;<br>
> -     image_metadata->Set("sdn.status", status);<br>
> +     image_metadata->Set("denoise.status", status);<br>
>       RPI_LOG("Sdn: programmed constant " << status.noise_constant<br>
>                                           << " slope " << status.noise_slope<br>
>                                           << " strength "<br>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> index 5ccc7a6551f5..220cf174aa4f 100644<br>
> --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> @@ -43,13 +43,13 @@<br>
>  #include "contrast_algorithm.hpp"<br>
>  #include "contrast_status.h"<br>
>  #include "controller.hpp"<br>
> +#include "denoise_status.h"<br>
>  #include "dpc_status.h"<br>
>  #include "focus_status.h"<br>
>  #include "geq_status.h"<br>
>  #include "lux_status.h"<br>
>  #include "metadata.hpp"<br>
>  #include "noise_status.h"<br>
> -#include "sdn_status.h"<br>
>  #include "sharpen_algorithm.hpp"<br>
>  #include "sharpen_status.h"<br>
>  <br>
> @@ -109,7 +109,7 @@ private:<br>
>       void applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls);<br>
>       void applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls);<br>
>       void applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls);<br>
> -     void applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ctrls);<br>
> +     void applyDenoise(const struct DenoiseStatus *denoiseStatus, ControlList &ctrls);<br>
>       void applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls);<br>
>       void applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls);<br>
>       void applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls);<br>
> @@ -899,7 +899,7 @@ void IPARPi::prepareISP(unsigned int bufferId)<br>
>               if (geqStatus)<br>
>                       applyGEQ(geqStatus, ctrls);<br>
>  <br>
> -             SdnStatus *denoiseStatus = rpiMetadata_.GetLocked<SdnStatus>("sdn.status");<br>
> +             DenoiseStatus *denoiseStatus = rpiMetadata_.GetLocked<DenoiseStatus>("denoise.status");<br>
>               if (denoiseStatus)<br>
>                       applyDenoise(denoiseStatus, ctrls);<br>
>  <br>
> @@ -1083,7 +1083,7 @@ void IPARPi::applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls)<br>
>       ctrls.set(V4L2_CID_USER_BCM2835_ISP_GEQ, c);<br>
>  }<br>
>  <br>
> -void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ctrls)<br>
> +void IPARPi::applyDenoise(const struct DenoiseStatus *denoiseStatus, ControlList &ctrls)<br>
>  {<br>
>       bcm2835_isp_denoise denoise;<br>
>  <br>
> <br>
<br>
-- <br>
Regards<br>
--<br>
Kieran<br>
</blockquote></div></div>