[libcamera-devel] [PATCH 5/5] ipa: raspberrypi: Handle control::NoiseReductionMode in the controller
David Plowman
david.plowman at raspberrypi.com
Wed Jan 20 12:16:01 CET 2021
Hi Naush
On Wed, 20 Jan 2021 at 10:58, Naushir Patuck <naush at raspberrypi.com> wrote:
>
> Hi David,
>
>
> On Wed, 20 Jan 2021 at 10:29, David Plowman <david.plowman at raspberrypi.com> wrote:
>>
>> Hi Naush
>>
>> Thanks for this patch (and the previous one, namely 4/5). I'm afraid
>> I'd quite like to have just a small meta-discussion about this one
>> first. Only a small one, though!
>>
>> Reading these patches it seems like an application setting
>> NoiseReductionModeOff will still get regular spatial denoise, just not
>> colour denoise. Is that right? Under such circumstances I think it
>> would be clearer to rename DenoiseMode as ColourDenoiseMode.
>>
>> Having said that, if we're going to support NoiseReductionModeOff,
>> then we should perhaps make it disable all denoising? I suspect folks
>> would find it "surprising" if it didn't do that. As such, maybe
>> DenoiseMode would remain as DenoiseMode (not ColourDenoiseMode) and
>> we'd have 4 values, namely:
>>
>> DenoiseMode::Off - really everything off
>> DenoiseMode::ColourOff - colour denoise off but regular spatial denoise on
>> DenoiseMode::ColorFast - with fast colour denoise
>> DenoiseMode::ColorHighQuality - with slow colour denoise
>>
>> (Setting the denoise strength to zero will disable spatial denoise.)
>>
>> What do you think?
>
>
> Yes, I think this is a reasonable thing to do. I will make the appropriate changes in the next revisions.
> To disable SDN, would it be better to set the "enabled" field in the bcm2835_isp_denoise struct to 0 over setting the strength to 0?
Whichever you prefer, I don't think it should matter. Famous last words!! :)
Best regards
David
>
> Regards,
> Naush
>
>
>>
>>
>> On Wed, 20 Jan 2021 at 08:35, Naushir Patuck <naush at 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 at 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 8af3d603a3ff..91041fa68930 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 "dpc_status.h"
>> > #include "focus_status.h"
>> > #include "geq_status.h"
>> > @@ -553,7 +554,8 @@ bool IPARPi::validateIspControls()
>> > V4L2_CID_USER_BCM2835_ISP_DENOISE,
>> > V4L2_CID_USER_BCM2835_ISP_SHARPEN,
>> > V4L2_CID_USER_BCM2835_ISP_DPC,
>> > - V4L2_CID_USER_BCM2835_ISP_LENS_SHADING
>> > + V4L2_CID_USER_BCM2835_ISP_LENS_SHADING,
>> > + V4L2_CID_USER_BCM2835_ISP_CDN,
>> > };
>> >
>> > for (auto c : ctrls) {
>> > @@ -603,6 +605,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::Fast },
>> > + { controls::draft::NoiseReductionModeHighQuality, RPiController::DenoiseMode::HighQuality },
>> > + { controls::draft::NoiseReductionModeMinimal, RPiController::DenoiseMode::Fast },
>>
>> If we go with a revised definition of DenoiseMode, maybe this one
>> would be DenoiseMode::ColourOff?
>>
>> Anyway, thanks for all this work. Best regards
>> David
>>
>> > + { controls::draft::NoiseReductionModeZSL, RPiController::DenoiseMode::HighQuality },
>> > +};
>> > +
>> > void IPARPi::queueRequest(const ControlList &controls)
>> > {
>> > /* Clear the return metadata buffer. */
>> > @@ -806,6 +816,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()
>> > @@ -1052,9 +1078,28 @@ void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ct
>> > 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::Fast:
>> > + cdn.enabled = 1;
>> > + cdn.mode = CDN_MODE_FAST;
>> > + break;
>> > + case RPiController::DenoiseMode::HighQuality:
>> > + 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 at lists.libcamera.org
>> > https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list