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

Message ID 20210126162412.824033-7-naush@raspberrypi.com
State Superseded
Headers show
Series
  • Raspberry Pi: Colour denoise
Related show

Commit Message

Naushir Patuck Jan. 26, 2021, 4:24 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>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/ipa/raspberrypi/raspberrypi.cpp | 47 ++++++++++++++++++++++++++++-
 1 file changed, 46 insertions(+), 1 deletion(-)

Comments

Laurent Pinchart Jan. 26, 2021, 10:52 p.m. UTC | #1 
Hi Naush,

Thank you for the patch.

On Tue, Jan 26, 2021 at 04:24:12PM +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@raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> 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: {

I may be missing something, but you don't seem to add the control to the
ControlInfoMap in include/libcamera/ipa/raspberrypi.h, so it won't be
exposed to applications.

> +			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));

			auto mode = DenoiseModeTable.find(idx);
			if (mode != DenoiseModeTable.end()) {
				sdn->SetMode(mode->second);

will avoid the double lookup.

> +				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;

You could write

	denoise.enabled = denoiseStatus->mode != RPiController::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 (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. 27, 2021, 8:09 a.m. UTC | #2 
Hi Laurent,

Thank you for your review feedback.

On Tue, 26 Jan 2021 at 22:52, Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> Hi Naush,
>
> Thank you for the patch.
>
> On Tue, Jan 26, 2021 at 04:24:12PM +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@raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > 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: {
>
> I may be missing something, but you don't seem to add the control to the
> ControlInfoMap in include/libcamera/ipa/raspberrypi.h, so it won't be
> exposed to applications.
>

This was on purpose - although my thinking was a bit misguided when I wrote
the original patch.  I will add this in now.


>
> > +                     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));
>
>                         auto mode = DenoiseModeTable.find(idx);
>                         if (mode != DenoiseModeTable.end()) {
>                                 sdn->SetMode(mode->second);
>
> will avoid the double lookup.
>

Ack.


>
> > +
>  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;
>
> You could write
>
>         denoise.enabled = denoiseStatus->mode !=
> RPiController::DenoiseMode::Off;
>

Ack


>
> >       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,
>
> Laurent Pinchart
>

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)