[libcamera-devel] [PATCH v5 6/6] ipa: raspberrypi: Handle control::NoiseReductionMode in the controller

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Fri, Jan 29, 2021 at 03:11:54PM +0000, Naushir Patuck wrote:
> The application provided noise reduction mode gets passed into the
> denoise controller. The denoise controller in turn returns the mode to
> the IPA which now sets up the colour denoise processing appropriately.
> 
> 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>

> ---
>  include/libcamera/ipa/raspberrypi.h |  1 +
>  src/ipa/raspberrypi/raspberrypi.cpp | 51 ++++++++++++++++++++++++++++-
>  2 files changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> index 1de36039cee0..3be79fc5ed73 100644
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ b/include/libcamera/ipa/raspberrypi.h
> @@ -66,6 +66,7 @@ static const ControlInfoMap Controls = {
>  	{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
>  	{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
>  	{ &controls::FrameDurations, ControlInfo(1000, 1000000000) },
> +	{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },
>  };
>  
>  } /* namespace RPi */
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 220cf174aa4f..862c52bde082 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -43,6 +43,7 @@
>  #include "contrast_algorithm.hpp"
>  #include "contrast_status.h"
>  #include "controller.hpp"
> +#include "denoise_algorithm.hpp"
>  #include "denoise_status.h"
>  #include "dpc_status.h"
>  #include "focus_status.h"
> @@ -565,6 +566,7 @@ bool IPARPi::validateIspControls()
>  		V4L2_CID_USER_BCM2835_ISP_SHARPEN,
>  		V4L2_CID_USER_BCM2835_ISP_DPC,
>  		V4L2_CID_USER_BCM2835_ISP_LENS_SHADING,
> +		V4L2_CID_USER_BCM2835_ISP_CDN,
>  	};
>  
>  	for (auto c : ctrls) {
> @@ -614,6 +616,14 @@ static const std::map<int32_t, std::string> AwbModeTable = {
>  	{ controls::AwbCustom, "custom" },
>  };
>  
> +static const std::map<int32_t, RPiController::DenoiseMode> DenoiseModeTable = {
> +	{ controls::draft::NoiseReductionModeOff, RPiController::DenoiseMode::Off },
> +	{ controls::draft::NoiseReductionModeFast, RPiController::DenoiseMode::ColourFast },
> +	{ controls::draft::NoiseReductionModeHighQuality, RPiController::DenoiseMode::ColourHighQuality },
> +	{ controls::draft::NoiseReductionModeMinimal, RPiController::DenoiseMode::ColourOff },
> +	{ controls::draft::NoiseReductionModeZSL, RPiController::DenoiseMode::ColourHighQuality },
> +};
> +
>  void IPARPi::queueRequest(const ControlList &controls)
>  {
>  	/* Clear the return metadata buffer. */
> @@ -836,6 +846,23 @@ void IPARPi::queueRequest(const ControlList &controls)
>  			break;
>  		}
>  
> +		case controls::NOISE_REDUCTION_MODE: {
> +			RPiController::DenoiseAlgorithm *sdn = dynamic_cast<RPiController::DenoiseAlgorithm *>(
> +				controller_.GetAlgorithm("SDN"));
> +			ASSERT(sdn);
> +
> +			int32_t idx = ctrl.second.get<int32_t>();
> +			auto mode = DenoiseModeTable.find(idx);
> +			if (mode != DenoiseModeTable.end()) {
> +				sdn->SetMode(mode->second);
> +				libcameraMetadata_.set(controls::draft::NoiseReductionMode, idx);
> +			} else {
> +				LOG(IPARPI, Error) << "Noise reduction mode " << idx
> +						   << " not recognised";
> +			}
> +			break;
> +		}
> +
>  		default:
>  			LOG(IPARPI, Warning)
>  				<< "Ctrl " << controls::controls.at(ctrl.first)->name()
> @@ -1085,18 +1112,40 @@ void IPARPi::applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls)
>  
>  void IPARPi::applyDenoise(const struct DenoiseStatus *denoiseStatus, ControlList &ctrls)
>  {
> +	using RPiController::DenoiseMode;
> +
>  	bcm2835_isp_denoise denoise;
> +	DenoiseMode mode = static_cast<DenoiseMode>(denoiseStatus->mode);
>  
> -	denoise.enabled = 1;
> +	denoise.enabled = mode != DenoiseMode::Off;
>  	denoise.constant = denoiseStatus->noise_constant;
>  	denoise.slope.num = 1000 * denoiseStatus->noise_slope;
>  	denoise.slope.den = 1000;
>  	denoise.strength.num = 1000 * denoiseStatus->strength;
>  	denoise.strength.den = 1000;
>  
> +	/* Set the CDN mode to match the SDN operating mode. */
> +	bcm2835_isp_cdn cdn;
> +	switch (mode) {
> +	case DenoiseMode::ColourFast:
> +		cdn.enabled = 1;
> +		cdn.mode = CDN_MODE_FAST;
> +		break;
> +	case DenoiseMode::ColourHighQuality:
> +		cdn.enabled = 1;
> +		cdn.mode = CDN_MODE_HIGH_QUALITY;
> +		break;
> +	default:
> +		cdn.enabled = 0;
> +	}
> +
>  	ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&denoise),
>  					    sizeof(denoise) });
>  	ctrls.set(V4L2_CID_USER_BCM2835_ISP_DENOISE, c);
> +
> +	c = ControlValue(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&cdn),
> +					      sizeof(cdn) });
> +	ctrls.set(V4L2_CID_USER_BCM2835_ISP_CDN, c);
>  }
>  
>  void IPARPi::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls)

-- 
Regards,

Laurent Pinchart