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

[Laurent Pinchart]

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.

> 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