<div dir="ltr"><div dir="ltr">Hi Kieran,<div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 22 Jan 2021 at 12:07, Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@ideasonboard.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>
On 22/01/2021 09:25, Naushir Patuck wrote:<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>
I assume the assert means that it's (expected to be) guaranteed to get<br>
the algorithm, so we don't need to handle that gracefully.<br>
<br>
In which case assert is just fine ;-)<br></blockquote><div><br></div><div>Yes, for now, we guarantee that sdn should be available. David and I do need to work out mechanisms where they may eventually be absent, but we are not there yet.</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>
<br>
> +<br>
> + int32_t idx = ctrl.second.get<int32_t>();<br>
<br>
My usual screams at how I dislike std::pair ;-) But that's a rant at<br>
C++, not this patch.<br>
<br>
<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>
<br>
Is the ternary required? Isn't<br>
(denoiseStatus->mode == RPiController::DenoiseMode::Off) sufficient?<br></blockquote><div><br></div><div>I think you mean "!=" there :-)</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>
Hrm, it's assigned to a __u32, so I guess in someways it's good to be<br>
explicit.<br></blockquote><div><br></div><div>No, this is not strictly required, but I do prefer to be explicit purely as a matter of style. Hope that is ok.</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: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
<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>
<br>
-- <br>
Regards<br>
--<br>
Kieran<br>
</blockquote></div></div>