| Message ID | 20210122092537.708375-7-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush Thanks for this patch. It all looks good to me now. Only one small question, do we need to be quite sure that rpi-update is sufficiently up-to-date before merging this set? (wouldn't want to create a headache for people who are just trying our libcamera apps for the first time!) Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Best regards David On Fri, 22 Jan 2021 at 09:25, Naushir Patuck <naush@raspberrypi.com> 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> > --- > 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) > -- > 2.25.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi David, Thank you for your review. On Fri, 22 Jan 2021 at 11:59, David Plowman <david.plowman@raspberrypi.com> wrote: > Hi Naush > > Thanks for this patch. It all looks good to me now. Only one small > question, do we need to be quite sure that rpi-update is sufficiently > up-to-date before merging this set? (wouldn't want to create a > headache for people who are just trying our libcamera apps for the > first time!) > I've already confirmed that the rpi-update firmware does have the necessary changes for colour denoise to work. So all should be good. Regards, Naush > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > Best regards > David > > On Fri, 22 Jan 2021 at 09:25, Naushir Patuck <naush@raspberrypi.com> > 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> > > --- > > 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) > > -- > > 2.25.1 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel >
Hi Naush, On 22/01/2021 09:25, 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> > --- > 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); I assume the assert means that it's (expected to be) guaranteed to get the algorithm, so we don't need to handle that gracefully. In which case assert is just fine ;-) > + > + int32_t idx = ctrl.second.get<int32_t>(); My usual screams at how I dislike std::pair ;-) But that's a rant at C++, not this patch. > + 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; Is the ternary required? Isn't (denoiseStatus->mode == RPiController::DenoiseMode::Off) sufficient? Hrm, it's assigned to a __u32, so I guess in someways it's good to be explicit. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > 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) >
Hi Kieran, On Fri, 22 Jan 2021 at 12:07, Kieran Bingham < kieran.bingham@ideasonboard.com> wrote: > Hi Naush, > > On 22/01/2021 09:25, 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> > > --- > > 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); > > I assume the assert means that it's (expected to be) guaranteed to get > the algorithm, so we don't need to handle that gracefully. > > In which case assert is just fine ;-) > 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. > > > > + > > + int32_t idx = ctrl.second.get<int32_t>(); > > My usual screams at how I dislike std::pair ;-) But that's a rant at > C++, not this patch. > > > > + 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; > > Is the ternary required? Isn't > (denoiseStatus->mode == RPiController::DenoiseMode::Off) sufficient? > I think you mean "!=" there :-) > > Hrm, it's assigned to a __u32, so I guess in someways it's good to be > explicit. > No, this is not strictly required, but I do prefer to be explicit purely as a matter of style. Hope that is ok. Regards, Naush > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > 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 >
On 22/01/2021 13:14, Naushir Patuck wrote: > Hi Kieran, > > > On Fri, 22 Jan 2021 at 12:07, Kieran Bingham > <kieran.bingham@ideasonboard.com > <mailto:kieran.bingham@ideasonboard.com>> wrote: > > Hi Naush, > > On 22/01/2021 09:25, 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 > <mailto:naush@raspberrypi.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); > > I assume the assert means that it's (expected to be) guaranteed to get > the algorithm, so we don't need to handle that gracefully. > > In which case assert is just fine ;-) > > > 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. > > > > > > + > > + int32_t idx = ctrl.second.get<int32_t>(); > > My usual screams at how I dislike std::pair ;-) But that's a rant at > C++, not this patch. > > > > + 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 > <http://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; > > Is the ternary required? Isn't > (denoiseStatus->mode == RPiController::DenoiseMode::Off) sufficient? > > > I think you mean "!=" there :-) Aha - Just testing ;-) hehe > > > > Hrm, it's assigned to a __u32, so I guess in someways it's good to be > explicit. > > > No, this is not strictly required, but I do prefer to be explicit purely > as a matter of style. Hope that is ok. That's fine with me. Particularly as it's not storing in a boolean. > > Regards, > Naush > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com > <mailto:kieran.bingham@ideasonboard.com>> > > > 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 >
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)
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(-)