<div dir="ltr"><div dir="ltr">Hi David,<div><br></div><div>Thank you for your review.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 22 Jan 2021 at 11:59, David Plowman <<a href="mailto:david.plowman@raspberrypi.com">david.plowman@raspberrypi.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>
Thanks for this patch. It all looks good to me now. Only one small<br>
question, do we need to be quite sure that rpi-update is sufficiently<br>
up-to-date before merging this set? (wouldn't want to create a<br>
headache for people who are just trying our libcamera apps for the<br>
first time!)<br></blockquote><div><br></div><div>I've already confirmed that the rpi-update firmware does have the necessary changes for colour denoise to work.  So all should be good.</div><div><br></div><div>Regards,</div><div>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: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
<br>
Best regards<br>
David<br>
<br>
On Fri, 22 Jan 2021 at 09:25, Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>> wrote:<br>
><br>
> The application provided noise reduction mode gets passed into the<br>
> denoise controller.  The denoise controller in turn returns the mode to<br>
> the IPA which now sets up the colour denoise processing appropriately.<br>
><br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> ---<br>
>  src/ipa/raspberrypi/raspberrypi.cpp | 47 ++++++++++++++++++++++++++++-<br>
>  1 file changed, 46 insertions(+), 1 deletion(-)<br>
><br>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> index 220cf174aa4f..8f80bb80c129 100644<br>
> --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> @@ -43,6 +43,7 @@<br>
>  #include "contrast_algorithm.hpp"<br>
>  #include "contrast_status.h"<br>
>  #include "controller.hpp"<br>
> +#include "denoise_algorithm.hpp"<br>
>  #include "denoise_status.h"<br>
>  #include "dpc_status.h"<br>
>  #include "focus_status.h"<br>
> @@ -565,6 +566,7 @@ bool IPARPi::validateIspControls()<br>
>  Â  Â  Â  Â  Â  Â  Â  Â V4L2_CID_USER_BCM2835_ISP_SHARPEN,<br>
>  Â  Â  Â  Â  Â  Â  Â  Â V4L2_CID_USER_BCM2835_ISP_DPC,<br>
>  Â  Â  Â  Â  Â  Â  Â  Â V4L2_CID_USER_BCM2835_ISP_LENS_SHADING,<br>
> +  Â  Â  Â  Â  Â  Â  Â V4L2_CID_USER_BCM2835_ISP_CDN,<br>
>  Â  Â  Â  Â };<br>
><br>
>  Â  Â  Â  Â for (auto c : ctrls) {<br>
> @@ -614,6 +616,14 @@ static const std::map<int32_t, std::string> AwbModeTable = {<br>
>  Â  Â  Â  Â { controls::AwbCustom, "custom" },<br>
>  };<br>
><br>
> +static const std::map<int32_t, RPiController::DenoiseMode> DenoiseModeTable = {<br>
> +  Â  Â  Â { controls::draft::NoiseReductionModeOff, RPiController::DenoiseMode::Off },<br>
> +  Â  Â  Â { controls::draft::NoiseReductionModeFast, RPiController::DenoiseMode::ColourFast },<br>
> +  Â  Â  Â { controls::draft::NoiseReductionModeHighQuality, RPiController::DenoiseMode::ColourHighQuality },<br>
> +  Â  Â  Â { controls::draft::NoiseReductionModeMinimal, RPiController::DenoiseMode::ColourOff },<br>
> +  Â  Â  Â { controls::draft::NoiseReductionModeZSL, RPiController::DenoiseMode::ColourHighQuality },<br>
> +};<br>
> +<br>
>  void IPARPi::queueRequest(const ControlList &controls)<br>
>  {<br>
>  Â  Â  Â  Â /* Clear the return metadata buffer. */<br>
> @@ -836,6 +846,22 @@ void IPARPi::queueRequest(const ControlList &controls)<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â break;<br>
>  Â  Â  Â  Â  Â  Â  Â  Â }<br>
><br>
> +  Â  Â  Â  Â  Â  Â  Â case controls::NOISE_REDUCTION_MODE: {<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â RPiController::DenoiseAlgorithm *sdn = dynamic_cast<RPiController::DenoiseAlgorithm *>(<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â controller_.GetAlgorithm("SDN"));<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â ASSERT(sdn);<br>
> +<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â int32_t idx = ctrl.second.get<int32_t>();<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â if (DenoiseModeTable.count(idx)) {<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â sdn->SetMode(DenoiseModeTable.at(idx));<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â libcameraMetadata_.set(controls::draft::NoiseReductionMode, idx);<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â } else {<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â LOG(IPARPI, Error) << "Noise reduction mode " << idx<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  << " not recognised";<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â }<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â break;<br>
> +  Â  Â  Â  Â  Â  Â  Â }<br>
> +<br>
>  Â  Â  Â  Â  Â  Â  Â  Â default:<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â LOG(IPARPI, Warning)<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â << "Ctrl " << controls::<a href="http://controls.at" rel="noreferrer" target="_blank">controls.at</a>(ctrl.first)->name()<br>
> @@ -1087,16 +1113,35 @@ void IPARPi::applyDenoise(const struct DenoiseStatus *denoiseStatus, ControlList<br>
>  {<br>
>  Â  Â  Â  Â bcm2835_isp_denoise denoise;<br>
><br>
> -  Â  Â  Â denoise.enabled = 1;<br>
> +  Â  Â  Â denoise.enabled = denoiseStatus->mode == RPiController::DenoiseMode::Off ? 0 : 1;<br>
>  Â  Â  Â  Â denoise.constant = denoiseStatus->noise_constant;<br>
>  Â  Â  Â  Â denoise.slope.num = 1000 * denoiseStatus->noise_slope;<br>
>  Â  Â  Â  Â denoise.slope.den = 1000;<br>
>  Â  Â  Â  Â denoise.strength.num = 1000 * denoiseStatus->strength;<br>
>  Â  Â  Â  Â denoise.strength.den = 1000;<br>
><br>
> +  Â  Â  Â /* Set the CDN mode to match the SDN operating mode. */<br>
> +  Â  Â  Â bcm2835_isp_cdn cdn;<br>
> +  Â  Â  Â switch (denoiseStatus->mode) {<br>
> +  Â  Â  Â case RPiController::DenoiseMode::ColourFast:<br>
> +  Â  Â  Â  Â  Â  Â  Â cdn.enabled = 1;<br>
> +  Â  Â  Â  Â  Â  Â  Â cdn.mode = CDN_MODE_FAST;<br>
> +  Â  Â  Â  Â  Â  Â  Â break;<br>
> +  Â  Â  Â case RPiController::DenoiseMode::ColourHighQuality:<br>
> +  Â  Â  Â  Â  Â  Â  Â cdn.enabled = 1;<br>
> +  Â  Â  Â  Â  Â  Â  Â cdn.mode = CDN_MODE_HIGH_QUALITY;<br>
> +  Â  Â  Â  Â  Â  Â  Â break;<br>
> +  Â  Â  Â default:<br>
> +  Â  Â  Â  Â  Â  Â  Â cdn.enabled = 0;<br>
> +  Â  Â  Â }<br>
> +<br>
>  Â  Â  Â  Â ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&denoise),<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â sizeof(denoise) });<br>
>  Â  Â  Â  Â ctrls.set(V4L2_CID_USER_BCM2835_ISP_DENOISE, c);<br>
> +<br>
> +  Â  Â  Â c = ControlValue(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&cdn),<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â sizeof(cdn) });<br>
> +  Â  Â  Â ctrls.set(V4L2_CID_USER_BCM2835_ISP_CDN, c);<br>
>  }<br>
><br>
>  void IPARPi::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls)<br>
> --<br>
> 2.25.1<br>
><br>
> _______________________________________________<br>
> libcamera-devel mailing list<br>
> <a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
> <a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
</blockquote></div></div>