[libcamera-devel,v3,6/6] ipa: raspberrypi: Handle control::NoiseReductionMode in the controller
diff mbox series

Message ID 20210122131451.726584-6-naush@raspberrypi.com
State Superseded
Headers show
Series
  • [libcamera-devel,v3,1/6] pipeline: raspberrypi: Refactor stream configuration routine
Related show

Commit Message

Naushir Patuck Jan. 22, 2021, 1:14 p.m. UTC 
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@raspberrypi.com>
---
 src/ipa/raspberrypi/raspberrypi.cpp | 47 ++++++++++++++++++++++++++++-
 1 file changed, 46 insertions(+), 1 deletion(-)

Comments

Kieran Bingham Jan. 22, 2021, 1:25 p.m. UTC | #1 
On 22/01/2021 13:14, 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@raspberrypi.com>

This was on v2, and I htnik it still stands

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> ---
>  src/ipa/raspberrypi/raspberrypi.cpp | 47 ++++++++++++++++++++++++++++-
>  1 file changed, 46 insertions(+), 1 deletion(-)
> 
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 220cf174aa4f..8f80bb80c129 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,22 @@ 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>();
> +			if (DenoiseModeTable.count(idx)) {
> +				sdn->SetMode(DenoiseModeTable.at(idx));
> +				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()
> @@ -1087,16 +1113,35 @@ void IPARPi::applyDenoise(const struct DenoiseStatus *denoiseStatus, ControlList
>  {
>  	bcm2835_isp_denoise denoise;
>  
> -	denoise.enabled = 1;
> +	denoise.enabled = denoiseStatus->mode == RPiController::DenoiseMode::Off ? 0 : 1;
>  	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 (denoiseStatus->mode) {
> +	case RPiController::DenoiseMode::ColourFast:
> +		cdn.enabled = 1;
> +		cdn.mode = CDN_MODE_FAST;
> +		break;
> +	case RPiController::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)
>
Naushir Patuck Jan. 22, 2021, 1:28 p.m. UTC | #2 
On Fri, 22 Jan 2021 at 13:25, Kieran Bingham <
kieran.bingham@ideasonboard.com> wrote:

> On 22/01/2021 13:14, 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@raspberrypi.com>
>
> This was on v2, and I htnik it still stands
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>

Oops, my bad.  I thought I added your tag.  David also had a tag on this
commit:

> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>

I blame my fingers and git!



>
> > ---
> >  src/ipa/raspberrypi/raspberrypi.cpp | 47 ++++++++++++++++++++++++++++-
> >  1 file changed, 46 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 220cf174aa4f..8f80bb80c129 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,22 @@ 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>();
> > +                     if (DenoiseModeTable.count(idx)) {
> > +                             sdn->SetMode(DenoiseModeTable.at(idx));
> > +
>  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()
> > @@ -1087,16 +1113,35 @@ void IPARPi::applyDenoise(const struct
> DenoiseStatus *denoiseStatus, ControlList
> >  {
> >       bcm2835_isp_denoise denoise;
> >
> > -     denoise.enabled = 1;
> > +     denoise.enabled = denoiseStatus->mode ==
> RPiController::DenoiseMode::Off ? 0 : 1;
> >       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 (denoiseStatus->mode) {
> > +     case RPiController::DenoiseMode::ColourFast:
> > +             cdn.enabled = 1;
> > +             cdn.mode = CDN_MODE_FAST;
> > +             break;
> > +     case RPiController::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
> --
> Kieran
>

Patch
diff mbox series 

diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
index 220cf174aa4f..8f80bb80c129 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,22 @@  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>();
+			if (DenoiseModeTable.count(idx)) {
+				sdn->SetMode(DenoiseModeTable.at(idx));
+				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()
@@ -1087,16 +1113,35 @@  void IPARPi::applyDenoise(const struct DenoiseStatus *denoiseStatus, ControlList
 {
 	bcm2835_isp_denoise denoise;
 
-	denoise.enabled = 1;
+	denoise.enabled = denoiseStatus->mode == RPiController::DenoiseMode::Off ? 0 : 1;
 	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 (denoiseStatus->mode) {
+	case RPiController::DenoiseMode::ColourFast:
+		cdn.enabled = 1;
+		cdn.mode = CDN_MODE_FAST;
+		break;
+	case RPiController::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)