| Message ID | 20220704152318.221213-4-fsylvestre@baylibre.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Florian Sylvestre via libcamera-devel (2022-07-04 16:23:17) > During the demosaicing step, rkisp1 ISP is processing denoising and sharpness > control. > Add demosaicing algorithm with denoise and sharpness values based on user > controls. > > Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com> > --- > src/ipa/rkisp1/algorithms/demosaicing.cpp | 208 ++++++++++++++++++++++ > src/ipa/rkisp1/algorithms/demosaicing.h | 37 ++++ > src/ipa/rkisp1/algorithms/meson.build | 1 + > src/ipa/rkisp1/data/ov5640.yaml | 1 + > src/ipa/rkisp1/rkisp1.cpp | 1 + > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 + > 6 files changed, 256 insertions(+) > create mode 100644 src/ipa/rkisp1/algorithms/demosaicing.cpp > create mode 100644 src/ipa/rkisp1/algorithms/demosaicing.h > > diff --git a/src/ipa/rkisp1/algorithms/demosaicing.cpp b/src/ipa/rkisp1/algorithms/demosaicing.cpp > new file mode 100644 > index 00000000..2597735e > --- /dev/null > +++ b/src/ipa/rkisp1/algorithms/demosaicing.cpp > @@ -0,0 +1,208 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2021-2022, Ideas On Board > + * > + * lsc.cpp - RkISP1 Defect Pixel Cluster Correction control * demosaicing.cpp - ... here perhaps. > + */ > + > +#include "demosaicing.h" > + > +#include <libcamera/base/log.h> > + > +#include <libcamera/control_ids.h> > + > +#include "libcamera/internal/yaml_parser.h" > + > +/** > + * \file demosaicing.h > + */ > + > +#define ISP_FILT_LUM_WEIGHT_DEFAULT 0x00022040 > +#define ISP_FILT_MODE_DEFAULT 0x000004f2 > + > +/* Default values : should be updated with automatic control implementation. */ > +#define SHARPNESS_DEFAULT 2 > +#define DENOISE_DEFAULT 2 > + > +namespace libcamera { > + > +namespace ipa::rkisp1::algorithms { > + > +/** > + * \class Demosaicing > + * \brief RkISP1 Demosaicing control > + * > + * The Demosaicing algorithm is responsible to reconstruct a full color image > + * from the sensor raw data. During the demosaicing step, rkisp1 ISP is > + * processing denoising and sharpness control. Only some subtle suggestion for reword - but it's up to you. It might not be accurate: * The Demosaicing algorithm is responsible for reconstructing a full * color image from the sensor raw data. During the demosaicing step, The * RKISP1 will additionally process denoise and sharpness controls. > + * > + * /todo In current version the denoise and sharpness control is based on user > + * controls. In a future version it should be controlled automatically by the > + * algorithm. > + */ > + > +LOG_DEFINE_CATEGORY(RkISP1Demosaicing) > + > +Demosaicing::Demosaicing() > + : userSharpness_(SHARPNESS_DEFAULT), userDenoise_(DENOISE_DEFAULT), > + updateUserControls_(true) > +{ > +} > + > +/** > + * \copydoc libcamera::ipa::Algorithm::queueRequest > + */ > +void Demosaicing::queueRequest([[maybe_unused]] IPAContext &context, > + [[maybe_unused]] const uint32_t frame, > + const ControlList &controls) > +{ > + for (auto const &ctrl : controls) { > + if (ctrl.first == controls::SHARPNESS) { I wonder if switch (ctrl.first) { case controls::SHARPNESS: ... break; would be cleaner than split if/elses. > + userSharpness_ = ctrl.second.get<float>(); In preparation for supporting 'per frame controls' (once you've added passing the FrameContext reference into queueRequest), I think you could already put userSharpness_ into IPAFrameContext.demosaic.sharpness (Add a demosaic structure in [0]) [0] https://git.libcamera.org/libcamera/libcamera.git/tree/src/ipa/rkisp1/ipa_context.h#n43 Same for denoise control too of course. > + updateUserControls_ = true; > + > + LOG(RkISP1Demosaicing, Debug) > + << "Set sharpness to: " > + << int(userSharpness_); > + } else if (ctrl.first == controls::NOISE_REDUCTION_MODE) { > + int32_t denoiseControl = ctrl.second.get<int32_t>(); > + > + switch (denoiseControl) { > + case controls::draft::NoiseReductionModeOff: > + userDenoise_ = 0; Are these 'values' or indexes into the arrays below? Could they be enums? > + break; > + case controls::draft::NoiseReductionModeMinimal: > + userDenoise_ = 1; > + break; > + case controls::draft::NoiseReductionModeHighQuality: > + case controls::draft::NoiseReductionModeFast: > + userDenoise_ = 3; > + break; > + default: > + LOG(RkISP1Demosaicing, Error) > + << "Unsupported denoise value: " > + << int(denoiseControl); > + continue; > + } > + > + updateUserControls_ = true; > + > + LOG(RkISP1Demosaicing, Debug) > + << "Set denoise to: " > + << int(userDenoise_); > + } > + } > +} > + > +/** > + * \copydoc libcamera::ipa::Algorithm::prepare > + */ > +void Demosaicing::prepare([[maybe_unused]] IPAContext &context, > + rkisp1_params_cfg *params) > +{ > + /* Check if the algorithm configuration has been updated. */ > + if (!updateUserControls_) > + return; > + > + updateUserControls_ = false; > + > + static constexpr uint16_t filt_fac_sh0[] = { > + 0x04, 0x07, 0x0A, 0x0C, 0x10, 0x14, 0x1A, 0x1E, 0x24, 0x2A, 0x30 > + }; > + > + static constexpr uint16_t filt_fac_sh1[] = { > + 0x04, 0x08, 0x0C, 0x10, 0x16, 0x1B, 0x20, 0x26, 0x2C, 0x30, 0x3F > + }; > + > + static constexpr uint16_t filt_fac_mid[] = { > + 0x04, 0x06, 0x08, 0x0A, 0x0C, 0x10, 0x13, 0x17, 0x1D, 0x22, 0x28 > + }; > + > + static constexpr uint16_t filt_fac_bl0[] = { > + 0x02, 0x02, 0x04, 0x06, 0x08, 0x0A, 0x0C, 0x10, 0x15, 0x1A, 0x24 > + }; > + > + static constexpr uint16_t filt_fac_bl1[] = { > + 0x00, 0x00, 0x00, 0x02, 0x04, 0x04, 0x06, 0x08, 0x0D, 0x14, 0x20 > + }; > + > + static constexpr uint16_t filt_thresh_sh0[] = { > + 0, 18, 26, 36, 41, 75, 90, 120, 170, 250, 1023 > + }; > + > + static constexpr uint16_t filt_thresh_sh1[] = { > + 0, 33, 44, 51, 67, 100, 120, 150, 200, 300, 1023 > + }; > + > + static constexpr uint16_t filt_thresh_bl0[] = { > + 0, 8, 13, 23, 26, 50, 60, 80, 140, 180, 1023 > + }; > + > + static constexpr uint16_t filt_thresh_bl1[] = { > + 0, 2, 5, 10, 15, 20, 26, 51, 100, 150, 1023 > + }; > + > + static constexpr uint16_t stage1_select[] = { > + 6, 6, 4, 4, 3, 3, 2, 2, 2, 2, 2 > + }; > + > + static constexpr uint16_t filt_chr_v_mode[] = { > + 1, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3 > + }; > + > + static constexpr uint16_t filt_chr_h_mode[] = { > + 0, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3 > + }; As far as I can tell, these sets are only used by either the sharpness or denoise. I'd be tempted to pull each one to it's own structure - and define the full state of each level as a contained structure. But I can see some merit in keeping the levels for each parameter in an array like this so the levels can be seen too across each setting ... I'm torn :( > + > + params->others.flt_config.fac_sh0 = filt_fac_sh0[userSharpness_]; > + params->others.flt_config.fac_sh1 = filt_fac_sh1[userSharpness_]; > + params->others.flt_config.fac_mid = filt_fac_mid[userSharpness_]; > + params->others.flt_config.fac_bl0 = filt_fac_bl0[userSharpness_]; > + params->others.flt_config.fac_bl1 = filt_fac_bl1[userSharpness_]; I don't believe this is safe currently. I don't think we have any specific clamping of the control values to ensure they are within range - so accepting a user input value as an index to an array could access memory out of bounds. This likely needs to be clamped - (or we need to make sure that controls are enforced to be clamped within their ranges too). > + > + params->others.flt_config.lum_weight = ISP_FILT_LUM_WEIGHT_DEFAULT; > + params->others.flt_config.mode = ISP_FILT_MODE_DEFAULT; > + params->others.flt_config.thresh_sh0 = filt_thresh_sh0[userDenoise_]; > + params->others.flt_config.thresh_sh1 = filt_thresh_sh1[userDenoise_]; > + params->others.flt_config.thresh_bl0 = filt_thresh_bl0[userDenoise_]; > + params->others.flt_config.thresh_bl1 = filt_thresh_bl1[userDenoise_]; > + params->others.flt_config.grn_stage1 = stage1_select[userDenoise_]; > + params->others.flt_config.chr_v_mode = filt_chr_v_mode[userDenoise_]; > + params->others.flt_config.chr_h_mode = filt_chr_h_mode[userDenoise_]; > + > + if (userDenoise_ == 9) { > + if (userSharpness_ > 3) > + params->others.flt_config.grn_stage1 = 2; > + else > + params->others.flt_config.grn_stage1 = 1; > + } else if (userDenoise_ == 10) { > + if (userSharpness_ > 5) > + params->others.flt_config.grn_stage1 = 2; > + else if (userSharpness_ > 3) > + params->others.flt_config.grn_stage1 = 1; > + else > + params->others.flt_config.grn_stage1 = 0; > + } else if (userDenoise_ > 7) { > + if (userSharpness_ > 7) { > + params->others.flt_config.fac_bl0 = > + params->others.flt_config.fac_bl0 / 2; > + params->others.flt_config.fac_bl1 = > + params->others.flt_config.fac_bl1 / 4; > + } else if (userSharpness_ > 4) { > + params->others.flt_config.fac_bl0 = > + params->others.flt_config.fac_bl0 * 3 / 4; > + params->others.flt_config.fac_bl1 = > + params->others.flt_config.fac_bl1 / 2; > + } > + } I only ever see userDenoise_ set to either 0, 1, or 3 ... Did I miss something going on elsewhere? Can it be set to 10? Is this logic taken from some sample code somewhere? Or a reference manual perhaps? > + > + params->module_en_update |= RKISP1_CIF_ISP_MODULE_FLT; > + params->module_ens |= RKISP1_CIF_ISP_MODULE_FLT; > + params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_FLT; > +} > + > +REGISTER_IPA_ALGORITHM(Demosaicing, "Demosaicing") > + > +} /* namespace ipa::rkisp1::algorithms */ > + > +} /* namespace libcamera */ > diff --git a/src/ipa/rkisp1/algorithms/demosaicing.h b/src/ipa/rkisp1/algorithms/demosaicing.h > new file mode 100644 > index 00000000..6d05b0e4 > --- /dev/null > +++ b/src/ipa/rkisp1/algorithms/demosaicing.h > @@ -0,0 +1,37 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2021-2022, Ideas On Board > + * > + * dpcc.h - RkISP1 Demosaicing control Maybe we shouldn't be specifying the filename in the comments ... > + */ > + > +#pragma once > + > +#include <sys/types.h> > +#include <linux/rkisp1-config.h> > + > +#include "algorithm.h" > + > +namespace libcamera { > + > +struct IPACameraSensorInfo; > + > +namespace ipa::rkisp1::algorithms { > + > +class Demosaicing : public Algorithm > +{ > +public: > + Demosaicing(); > + ~Demosaicing() = default; > + > + void queueRequest(IPAContext &context, const uint32_t frame, > + const ControlList &controls) override; > + void prepare(IPAContext &context, rkisp1_params_cfg *params) override; > +private: > + uint32_t userSharpness_; > + uint32_t userDenoise_; > + bool updateUserControls_; > +}; > + > +} /* namespace ipa::rkisp1::algorithms */ > +} /* namespace libcamera */ > diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build > index 87007493..7e078b0d 100644 > --- a/src/ipa/rkisp1/algorithms/meson.build > +++ b/src/ipa/rkisp1/algorithms/meson.build > @@ -4,6 +4,7 @@ rkisp1_ipa_algorithms = files([ > 'agc.cpp', > 'awb.cpp', > 'blc.cpp', > + 'demosaicing.cpp', > 'dpcc.cpp', > 'gsl.cpp', > 'lsc.cpp', > diff --git a/src/ipa/rkisp1/data/ov5640.yaml b/src/ipa/rkisp1/data/ov5640.yaml > index 51228218..4ae0ffc0 100644 > --- a/src/ipa/rkisp1/data/ov5640.yaml > +++ b/src/ipa/rkisp1/data/ov5640.yaml > @@ -157,4 +157,5 @@ algorithms: > rnd-offsets: > green: 2 > red-blue: 2 > + - Demosaicing: > ... > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > index 9b0d675c..85a75070 100644 > --- a/src/ipa/rkisp1/rkisp1.cpp > +++ b/src/ipa/rkisp1/rkisp1.cpp > @@ -31,6 +31,7 @@ > #include "algorithms/algorithm.h" > #include "algorithms/awb.h" > #include "algorithms/blc.h" > +#include "algorithms/demosaicing.h" > #include "algorithms/dpcc.h" > #include "algorithms/gsl.h" > #include "algorithms/lsc.h" > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 72689c88..3d0075ee 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -953,6 +953,14 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor) > std::make_unique<RkISP1CameraData>(this, &mainPath_, &selfPath_); > > ControlInfoMap::Map ctrls; > + ctrls.emplace(std::piecewise_construct, > + std::forward_as_tuple(&controls::Sharpness), > + std::forward_as_tuple(0.0f, 10.0f, 1.0f)); > + > + ctrls.emplace(std::piecewise_construct, > + std::forward_as_tuple(&controls::draft::NoiseReductionMode), > + std::forward_as_tuple(controls::draft::NoiseReductionModeValues)); > + I think these should be added from the IPA - but perhaps that's also missing some plumbing? > ctrls.emplace(std::piecewise_construct, > std::forward_as_tuple(&controls::AeEnable), > std::forward_as_tuple(false, true)); > -- > 2.34.1 >
Hi Kieran, On Fri, Jul 15, 2022 at 03:46:11PM +0100, Kieran Bingham via libcamera-devel wrote: > Quoting Florian Sylvestre via libcamera-devel (2022-07-04 16:23:17) > > During the demosaicing step, rkisp1 ISP is processing denoising and sharpness > > control. > > Add demosaicing algorithm with denoise and sharpness values based on user > > controls. > > > > Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com> > > --- > > src/ipa/rkisp1/algorithms/demosaicing.cpp | 208 ++++++++++++++++++++++ > > src/ipa/rkisp1/algorithms/demosaicing.h | 37 ++++ > > src/ipa/rkisp1/algorithms/meson.build | 1 + > > src/ipa/rkisp1/data/ov5640.yaml | 1 + > > src/ipa/rkisp1/rkisp1.cpp | 1 + > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 + > > 6 files changed, 256 insertions(+) > > create mode 100644 src/ipa/rkisp1/algorithms/demosaicing.cpp > > create mode 100644 src/ipa/rkisp1/algorithms/demosaicing.h > > > > diff --git a/src/ipa/rkisp1/algorithms/demosaicing.cpp b/src/ipa/rkisp1/algorithms/demosaicing.cpp > > new file mode 100644 > > index 00000000..2597735e > > --- /dev/null > > +++ b/src/ipa/rkisp1/algorithms/demosaicing.cpp > > @@ -0,0 +1,208 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2021-2022, Ideas On Board > > + * > > + * lsc.cpp - RkISP1 Defect Pixel Cluster Correction control > > * demosaicing.cpp - ... here perhaps. > > > + */ > > + > > +#include "demosaicing.h" > > + > > +#include <libcamera/base/log.h> > > + > > +#include <libcamera/control_ids.h> > > + > > +#include "libcamera/internal/yaml_parser.h" > > + > > +/** > > + * \file demosaicing.h > > + */ > > + > > +#define ISP_FILT_LUM_WEIGHT_DEFAULT 0x00022040 > > +#define ISP_FILT_MODE_DEFAULT 0x000004f2 > > + > > +/* Default values : should be updated with automatic control implementation. */ > > +#define SHARPNESS_DEFAULT 2 > > +#define DENOISE_DEFAULT 2 > > + > > +namespace libcamera { > > + > > +namespace ipa::rkisp1::algorithms { > > + > > +/** > > + * \class Demosaicing > > + * \brief RkISP1 Demosaicing control > > + * > > + * The Demosaicing algorithm is responsible to reconstruct a full color image > > + * from the sensor raw data. During the demosaicing step, rkisp1 ISP is > > + * processing denoising and sharpness control. > > Only some subtle suggestion for reword - but it's up to you. It might > not be accurate: > > * The Demosaicing algorithm is responsible for reconstructing a full > * color image from the sensor raw data. During the demosaicing step, The > * RKISP1 will additionally process denoise and sharpness controls. > > > + * > > + * /todo In current version the denoise and sharpness control is based on user > > + * controls. In a future version it should be controlled automatically by the > > + * algorithm. > > + */ > > + > > +LOG_DEFINE_CATEGORY(RkISP1Demosaicing) > > + > > +Demosaicing::Demosaicing() > > + : userSharpness_(SHARPNESS_DEFAULT), userDenoise_(DENOISE_DEFAULT), > > + updateUserControls_(true) > > +{ > > +} > > + > > +/** > > + * \copydoc libcamera::ipa::Algorithm::queueRequest > > + */ > > +void Demosaicing::queueRequest([[maybe_unused]] IPAContext &context, > > + [[maybe_unused]] const uint32_t frame, > > + const ControlList &controls) > > +{ > > + for (auto const &ctrl : controls) { > > + if (ctrl.first == controls::SHARPNESS) { > > I wonder if > switch (ctrl.first) { > > case controls::SHARPNESS: > ... > break; > > would be cleaner than split if/elses. > > > + userSharpness_ = ctrl.second.get<float>(); > > In preparation for supporting 'per frame controls' (once you've added > passing the FrameContext reference into queueRequest), I think you could > already put userSharpness_ into IPAFrameContext.demosaic.sharpness > > (Add a demosaic structure in [0]) > [0] https://git.libcamera.org/libcamera/libcamera.git/tree/src/ipa/rkisp1/ipa_context.h#n43 > > Same for denoise control too of course. > > > + updateUserControls_ = true; > > + > > + LOG(RkISP1Demosaicing, Debug) > > + << "Set sharpness to: " > > + << int(userSharpness_); > > + } else if (ctrl.first == controls::NOISE_REDUCTION_MODE) { > > + int32_t denoiseControl = ctrl.second.get<int32_t>(); > > + > > + switch (denoiseControl) { > > + case controls::draft::NoiseReductionModeOff: > > + userDenoise_ = 0; > > Are these 'values' or indexes into the arrays below? Could they be > enums? Those are denoising levels, and they index the arrays below indeed. They're not enumerated values though, they're really levels. > > + break; > > + case controls::draft::NoiseReductionModeMinimal: > > + userDenoise_ = 1; > > + break; > > + case controls::draft::NoiseReductionModeHighQuality: > > + case controls::draft::NoiseReductionModeFast: > > + userDenoise_ = 3; > > + break; > > + default: > > + LOG(RkISP1Demosaicing, Error) > > + << "Unsupported denoise value: " > > + << int(denoiseControl); > > + continue; > > + } > > + > > + updateUserControls_ = true; > > + > > + LOG(RkISP1Demosaicing, Debug) > > + << "Set denoise to: " > > + << int(userDenoise_); > > + } > > + } > > +} > > + > > +/** > > + * \copydoc libcamera::ipa::Algorithm::prepare > > + */ > > +void Demosaicing::prepare([[maybe_unused]] IPAContext &context, > > + rkisp1_params_cfg *params) > > +{ > > + /* Check if the algorithm configuration has been updated. */ > > + if (!updateUserControls_) > > + return; > > + > > + updateUserControls_ = false; > > + > > + static constexpr uint16_t filt_fac_sh0[] = { > > + 0x04, 0x07, 0x0A, 0x0C, 0x10, 0x14, 0x1A, 0x1E, 0x24, 0x2A, 0x30 > > + }; > > + > > + static constexpr uint16_t filt_fac_sh1[] = { > > + 0x04, 0x08, 0x0C, 0x10, 0x16, 0x1B, 0x20, 0x26, 0x2C, 0x30, 0x3F > > + }; > > + > > + static constexpr uint16_t filt_fac_mid[] = { > > + 0x04, 0x06, 0x08, 0x0A, 0x0C, 0x10, 0x13, 0x17, 0x1D, 0x22, 0x28 > > + }; > > + > > + static constexpr uint16_t filt_fac_bl0[] = { > > + 0x02, 0x02, 0x04, 0x06, 0x08, 0x0A, 0x0C, 0x10, 0x15, 0x1A, 0x24 > > + }; > > + > > + static constexpr uint16_t filt_fac_bl1[] = { > > + 0x00, 0x00, 0x00, 0x02, 0x04, 0x04, 0x06, 0x08, 0x0D, 0x14, 0x20 > > + }; > > + > > + static constexpr uint16_t filt_thresh_sh0[] = { > > + 0, 18, 26, 36, 41, 75, 90, 120, 170, 250, 1023 > > + }; > > + > > + static constexpr uint16_t filt_thresh_sh1[] = { > > + 0, 33, 44, 51, 67, 100, 120, 150, 200, 300, 1023 > > + }; > > + > > + static constexpr uint16_t filt_thresh_bl0[] = { > > + 0, 8, 13, 23, 26, 50, 60, 80, 140, 180, 1023 > > + }; > > + > > + static constexpr uint16_t filt_thresh_bl1[] = { > > + 0, 2, 5, 10, 15, 20, 26, 51, 100, 150, 1023 > > + }; > > + > > + static constexpr uint16_t stage1_select[] = { > > + 6, 6, 4, 4, 3, 3, 2, 2, 2, 2, 2 > > + }; > > + > > + static constexpr uint16_t filt_chr_v_mode[] = { > > + 1, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3 > > + }; > > + > > + static constexpr uint16_t filt_chr_h_mode[] = { > > + 0, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3 > > + }; > > As far as I can tell, these sets are only used by either the sharpness > or denoise. > > I'd be tempted to pull each one to it's own structure - and define the > full state of each level as a contained structure. > > But I can see some merit in keeping the levels for each parameter in an > array like this so the levels can be seen too across each setting ... > I'm torn :( > > > + > > + params->others.flt_config.fac_sh0 = filt_fac_sh0[userSharpness_]; > > + params->others.flt_config.fac_sh1 = filt_fac_sh1[userSharpness_]; > > + params->others.flt_config.fac_mid = filt_fac_mid[userSharpness_]; > > + params->others.flt_config.fac_bl0 = filt_fac_bl0[userSharpness_]; > > + params->others.flt_config.fac_bl1 = filt_fac_bl1[userSharpness_]; > > I don't believe this is safe currently. > > I don't think we have any specific clamping of the control values to > ensure they are within range - so accepting a user input value as an > index to an array could access memory out of bounds. > > This likely needs to be clamped - (or we need to make sure that controls > are enforced to be clamped within their ranges too). > > > + > > + params->others.flt_config.lum_weight = ISP_FILT_LUM_WEIGHT_DEFAULT; > > + params->others.flt_config.mode = ISP_FILT_MODE_DEFAULT; > > + params->others.flt_config.thresh_sh0 = filt_thresh_sh0[userDenoise_]; > > + params->others.flt_config.thresh_sh1 = filt_thresh_sh1[userDenoise_]; > > + params->others.flt_config.thresh_bl0 = filt_thresh_bl0[userDenoise_]; > > + params->others.flt_config.thresh_bl1 = filt_thresh_bl1[userDenoise_]; > > + params->others.flt_config.grn_stage1 = stage1_select[userDenoise_]; > > + params->others.flt_config.chr_v_mode = filt_chr_v_mode[userDenoise_]; > > + params->others.flt_config.chr_h_mode = filt_chr_h_mode[userDenoise_]; > > + > > + if (userDenoise_ == 9) { > > + if (userSharpness_ > 3) > > + params->others.flt_config.grn_stage1 = 2; > > + else > > + params->others.flt_config.grn_stage1 = 1; > > + } else if (userDenoise_ == 10) { > > + if (userSharpness_ > 5) > > + params->others.flt_config.grn_stage1 = 2; > > + else if (userSharpness_ > 3) > > + params->others.flt_config.grn_stage1 = 1; > > + else > > + params->others.flt_config.grn_stage1 = 0; > > + } else if (userDenoise_ > 7) { > > + if (userSharpness_ > 7) { > > + params->others.flt_config.fac_bl0 = > > + params->others.flt_config.fac_bl0 / 2; > > + params->others.flt_config.fac_bl1 = > > + params->others.flt_config.fac_bl1 / 4; > > + } else if (userSharpness_ > 4) { > > + params->others.flt_config.fac_bl0 = > > + params->others.flt_config.fac_bl0 * 3 / 4; > > + params->others.flt_config.fac_bl1 = > > + params->others.flt_config.fac_bl1 / 2; > > + } > > + } > > I only ever see userDenoise_ set to either 0, 1, or 3 ... Did I miss > something going on elsewhere? Can it be set to 10? The denoising level is currently hardcoded, the next step will be to set it dynamically based on sensor noise characterization (depending on the exposure time and analog gain). It could thus reach higher values. > Is this logic taken from some sample code somewhere? Or a reference > manual perhaps? Somewhere not public I'm afraid. > > + > > + params->module_en_update |= RKISP1_CIF_ISP_MODULE_FLT; > > + params->module_ens |= RKISP1_CIF_ISP_MODULE_FLT; > > + params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_FLT; > > +} > > + > > +REGISTER_IPA_ALGORITHM(Demosaicing, "Demosaicing") > > + > > +} /* namespace ipa::rkisp1::algorithms */ > > + > > +} /* namespace libcamera */ > > diff --git a/src/ipa/rkisp1/algorithms/demosaicing.h b/src/ipa/rkisp1/algorithms/demosaicing.h > > new file mode 100644 > > index 00000000..6d05b0e4 > > --- /dev/null > > +++ b/src/ipa/rkisp1/algorithms/demosaicing.h > > @@ -0,0 +1,37 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2021-2022, Ideas On Board > > + * > > + * dpcc.h - RkISP1 Demosaicing control > > Maybe we shouldn't be specifying the filename in the comments ... I've been thinking about this too :-) > > + */ > > + > > +#pragma once > > + > > +#include <sys/types.h> > > +#include <linux/rkisp1-config.h> > > + > > +#include "algorithm.h" > > + > > +namespace libcamera { > > + > > +struct IPACameraSensorInfo; > > + > > +namespace ipa::rkisp1::algorithms { > > + > > +class Demosaicing : public Algorithm > > +{ > > +public: > > + Demosaicing(); > > + ~Demosaicing() = default; > > + > > + void queueRequest(IPAContext &context, const uint32_t frame, > > + const ControlList &controls) override; > > + void prepare(IPAContext &context, rkisp1_params_cfg *params) override; > > +private: > > + uint32_t userSharpness_; > > + uint32_t userDenoise_; > > + bool updateUserControls_; > > +}; > > + > > +} /* namespace ipa::rkisp1::algorithms */ > > +} /* namespace libcamera */ > > diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build > > index 87007493..7e078b0d 100644 > > --- a/src/ipa/rkisp1/algorithms/meson.build > > +++ b/src/ipa/rkisp1/algorithms/meson.build > > @@ -4,6 +4,7 @@ rkisp1_ipa_algorithms = files([ > > 'agc.cpp', > > 'awb.cpp', > > 'blc.cpp', > > + 'demosaicing.cpp', > > 'dpcc.cpp', > > 'gsl.cpp', > > 'lsc.cpp', > > diff --git a/src/ipa/rkisp1/data/ov5640.yaml b/src/ipa/rkisp1/data/ov5640.yaml > > index 51228218..4ae0ffc0 100644 > > --- a/src/ipa/rkisp1/data/ov5640.yaml > > +++ b/src/ipa/rkisp1/data/ov5640.yaml > > @@ -157,4 +157,5 @@ algorithms: > > rnd-offsets: > > green: 2 > > red-blue: 2 > > + - Demosaicing: > > ... > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > index 9b0d675c..85a75070 100644 > > --- a/src/ipa/rkisp1/rkisp1.cpp > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > @@ -31,6 +31,7 @@ > > #include "algorithms/algorithm.h" > > #include "algorithms/awb.h" > > #include "algorithms/blc.h" > > +#include "algorithms/demosaicing.h" > > #include "algorithms/dpcc.h" > > #include "algorithms/gsl.h" > > #include "algorithms/lsc.h" > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > index 72689c88..3d0075ee 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > @@ -953,6 +953,14 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor) > > std::make_unique<RkISP1CameraData>(this, &mainPath_, &selfPath_); > > > > ControlInfoMap::Map ctrls; > > + ctrls.emplace(std::piecewise_construct, > > + std::forward_as_tuple(&controls::Sharpness), > > + std::forward_as_tuple(0.0f, 10.0f, 1.0f)); > > + > > + ctrls.emplace(std::piecewise_construct, > > + std::forward_as_tuple(&controls::draft::NoiseReductionMode), > > + std::forward_as_tuple(controls::draft::NoiseReductionModeValues)); > > + > > I think these should be added from the IPA - but perhaps that's > also missing some plumbing? I'll work on that. > > ctrls.emplace(std::piecewise_construct, > > std::forward_as_tuple(&controls::AeEnable), > > std::forward_as_tuple(false, true));
diff --git a/src/ipa/rkisp1/algorithms/demosaicing.cpp b/src/ipa/rkisp1/algorithms/demosaicing.cpp new file mode 100644 index 00000000..2597735e --- /dev/null +++ b/src/ipa/rkisp1/algorithms/demosaicing.cpp @@ -0,0 +1,208 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2021-2022, Ideas On Board + * + * lsc.cpp - RkISP1 Defect Pixel Cluster Correction control + */ + +#include "demosaicing.h" + +#include <libcamera/base/log.h> + +#include <libcamera/control_ids.h> + +#include "libcamera/internal/yaml_parser.h" + +/** + * \file demosaicing.h + */ + +#define ISP_FILT_LUM_WEIGHT_DEFAULT 0x00022040 +#define ISP_FILT_MODE_DEFAULT 0x000004f2 + +/* Default values : should be updated with automatic control implementation. */ +#define SHARPNESS_DEFAULT 2 +#define DENOISE_DEFAULT 2 + +namespace libcamera { + +namespace ipa::rkisp1::algorithms { + +/** + * \class Demosaicing + * \brief RkISP1 Demosaicing control + * + * The Demosaicing algorithm is responsible to reconstruct a full color image + * from the sensor raw data. During the demosaicing step, rkisp1 ISP is + * processing denoising and sharpness control. + * + * /todo In current version the denoise and sharpness control is based on user + * controls. In a future version it should be controlled automatically by the + * algorithm. + */ + +LOG_DEFINE_CATEGORY(RkISP1Demosaicing) + +Demosaicing::Demosaicing() + : userSharpness_(SHARPNESS_DEFAULT), userDenoise_(DENOISE_DEFAULT), + updateUserControls_(true) +{ +} + +/** + * \copydoc libcamera::ipa::Algorithm::queueRequest + */ +void Demosaicing::queueRequest([[maybe_unused]] IPAContext &context, + [[maybe_unused]] const uint32_t frame, + const ControlList &controls) +{ + for (auto const &ctrl : controls) { + if (ctrl.first == controls::SHARPNESS) { + userSharpness_ = ctrl.second.get<float>(); + updateUserControls_ = true; + + LOG(RkISP1Demosaicing, Debug) + << "Set sharpness to: " + << int(userSharpness_); + } else if (ctrl.first == controls::NOISE_REDUCTION_MODE) { + int32_t denoiseControl = ctrl.second.get<int32_t>(); + + switch (denoiseControl) { + case controls::draft::NoiseReductionModeOff: + userDenoise_ = 0; + break; + case controls::draft::NoiseReductionModeMinimal: + userDenoise_ = 1; + break; + case controls::draft::NoiseReductionModeHighQuality: + case controls::draft::NoiseReductionModeFast: + userDenoise_ = 3; + break; + default: + LOG(RkISP1Demosaicing, Error) + << "Unsupported denoise value: " + << int(denoiseControl); + continue; + } + + updateUserControls_ = true; + + LOG(RkISP1Demosaicing, Debug) + << "Set denoise to: " + << int(userDenoise_); + } + } +} + +/** + * \copydoc libcamera::ipa::Algorithm::prepare + */ +void Demosaicing::prepare([[maybe_unused]] IPAContext &context, + rkisp1_params_cfg *params) +{ + /* Check if the algorithm configuration has been updated. */ + if (!updateUserControls_) + return; + + updateUserControls_ = false; + + static constexpr uint16_t filt_fac_sh0[] = { + 0x04, 0x07, 0x0A, 0x0C, 0x10, 0x14, 0x1A, 0x1E, 0x24, 0x2A, 0x30 + }; + + static constexpr uint16_t filt_fac_sh1[] = { + 0x04, 0x08, 0x0C, 0x10, 0x16, 0x1B, 0x20, 0x26, 0x2C, 0x30, 0x3F + }; + + static constexpr uint16_t filt_fac_mid[] = { + 0x04, 0x06, 0x08, 0x0A, 0x0C, 0x10, 0x13, 0x17, 0x1D, 0x22, 0x28 + }; + + static constexpr uint16_t filt_fac_bl0[] = { + 0x02, 0x02, 0x04, 0x06, 0x08, 0x0A, 0x0C, 0x10, 0x15, 0x1A, 0x24 + }; + + static constexpr uint16_t filt_fac_bl1[] = { + 0x00, 0x00, 0x00, 0x02, 0x04, 0x04, 0x06, 0x08, 0x0D, 0x14, 0x20 + }; + + static constexpr uint16_t filt_thresh_sh0[] = { + 0, 18, 26, 36, 41, 75, 90, 120, 170, 250, 1023 + }; + + static constexpr uint16_t filt_thresh_sh1[] = { + 0, 33, 44, 51, 67, 100, 120, 150, 200, 300, 1023 + }; + + static constexpr uint16_t filt_thresh_bl0[] = { + 0, 8, 13, 23, 26, 50, 60, 80, 140, 180, 1023 + }; + + static constexpr uint16_t filt_thresh_bl1[] = { + 0, 2, 5, 10, 15, 20, 26, 51, 100, 150, 1023 + }; + + static constexpr uint16_t stage1_select[] = { + 6, 6, 4, 4, 3, 3, 2, 2, 2, 2, 2 + }; + + static constexpr uint16_t filt_chr_v_mode[] = { + 1, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3 + }; + + static constexpr uint16_t filt_chr_h_mode[] = { + 0, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3 + }; + + params->others.flt_config.fac_sh0 = filt_fac_sh0[userSharpness_]; + params->others.flt_config.fac_sh1 = filt_fac_sh1[userSharpness_]; + params->others.flt_config.fac_mid = filt_fac_mid[userSharpness_]; + params->others.flt_config.fac_bl0 = filt_fac_bl0[userSharpness_]; + params->others.flt_config.fac_bl1 = filt_fac_bl1[userSharpness_]; + + params->others.flt_config.lum_weight = ISP_FILT_LUM_WEIGHT_DEFAULT; + params->others.flt_config.mode = ISP_FILT_MODE_DEFAULT; + params->others.flt_config.thresh_sh0 = filt_thresh_sh0[userDenoise_]; + params->others.flt_config.thresh_sh1 = filt_thresh_sh1[userDenoise_]; + params->others.flt_config.thresh_bl0 = filt_thresh_bl0[userDenoise_]; + params->others.flt_config.thresh_bl1 = filt_thresh_bl1[userDenoise_]; + params->others.flt_config.grn_stage1 = stage1_select[userDenoise_]; + params->others.flt_config.chr_v_mode = filt_chr_v_mode[userDenoise_]; + params->others.flt_config.chr_h_mode = filt_chr_h_mode[userDenoise_]; + + if (userDenoise_ == 9) { + if (userSharpness_ > 3) + params->others.flt_config.grn_stage1 = 2; + else + params->others.flt_config.grn_stage1 = 1; + } else if (userDenoise_ == 10) { + if (userSharpness_ > 5) + params->others.flt_config.grn_stage1 = 2; + else if (userSharpness_ > 3) + params->others.flt_config.grn_stage1 = 1; + else + params->others.flt_config.grn_stage1 = 0; + } else if (userDenoise_ > 7) { + if (userSharpness_ > 7) { + params->others.flt_config.fac_bl0 = + params->others.flt_config.fac_bl0 / 2; + params->others.flt_config.fac_bl1 = + params->others.flt_config.fac_bl1 / 4; + } else if (userSharpness_ > 4) { + params->others.flt_config.fac_bl0 = + params->others.flt_config.fac_bl0 * 3 / 4; + params->others.flt_config.fac_bl1 = + params->others.flt_config.fac_bl1 / 2; + } + } + + params->module_en_update |= RKISP1_CIF_ISP_MODULE_FLT; + params->module_ens |= RKISP1_CIF_ISP_MODULE_FLT; + params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_FLT; +} + +REGISTER_IPA_ALGORITHM(Demosaicing, "Demosaicing") + +} /* namespace ipa::rkisp1::algorithms */ + +} /* namespace libcamera */ diff --git a/src/ipa/rkisp1/algorithms/demosaicing.h b/src/ipa/rkisp1/algorithms/demosaicing.h new file mode 100644 index 00000000..6d05b0e4 --- /dev/null +++ b/src/ipa/rkisp1/algorithms/demosaicing.h @@ -0,0 +1,37 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2021-2022, Ideas On Board + * + * dpcc.h - RkISP1 Demosaicing control + */ + +#pragma once + +#include <sys/types.h> +#include <linux/rkisp1-config.h> + +#include "algorithm.h" + +namespace libcamera { + +struct IPACameraSensorInfo; + +namespace ipa::rkisp1::algorithms { + +class Demosaicing : public Algorithm +{ +public: + Demosaicing(); + ~Demosaicing() = default; + + void queueRequest(IPAContext &context, const uint32_t frame, + const ControlList &controls) override; + void prepare(IPAContext &context, rkisp1_params_cfg *params) override; +private: + uint32_t userSharpness_; + uint32_t userDenoise_; + bool updateUserControls_; +}; + +} /* namespace ipa::rkisp1::algorithms */ +} /* namespace libcamera */ diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build index 87007493..7e078b0d 100644 --- a/src/ipa/rkisp1/algorithms/meson.build +++ b/src/ipa/rkisp1/algorithms/meson.build @@ -4,6 +4,7 @@ rkisp1_ipa_algorithms = files([ 'agc.cpp', 'awb.cpp', 'blc.cpp', + 'demosaicing.cpp', 'dpcc.cpp', 'gsl.cpp', 'lsc.cpp', diff --git a/src/ipa/rkisp1/data/ov5640.yaml b/src/ipa/rkisp1/data/ov5640.yaml index 51228218..4ae0ffc0 100644 --- a/src/ipa/rkisp1/data/ov5640.yaml +++ b/src/ipa/rkisp1/data/ov5640.yaml @@ -157,4 +157,5 @@ algorithms: rnd-offsets: green: 2 red-blue: 2 + - Demosaicing: ... diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp index 9b0d675c..85a75070 100644 --- a/src/ipa/rkisp1/rkisp1.cpp +++ b/src/ipa/rkisp1/rkisp1.cpp @@ -31,6 +31,7 @@ #include "algorithms/algorithm.h" #include "algorithms/awb.h" #include "algorithms/blc.h" +#include "algorithms/demosaicing.h" #include "algorithms/dpcc.h" #include "algorithms/gsl.h" #include "algorithms/lsc.h" diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index 72689c88..3d0075ee 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -953,6 +953,14 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor) std::make_unique<RkISP1CameraData>(this, &mainPath_, &selfPath_); ControlInfoMap::Map ctrls; + ctrls.emplace(std::piecewise_construct, + std::forward_as_tuple(&controls::Sharpness), + std::forward_as_tuple(0.0f, 10.0f, 1.0f)); + + ctrls.emplace(std::piecewise_construct, + std::forward_as_tuple(&controls::draft::NoiseReductionMode), + std::forward_as_tuple(controls::draft::NoiseReductionModeValues)); + ctrls.emplace(std::piecewise_construct, std::forward_as_tuple(&controls::AeEnable), std::forward_as_tuple(false, true));
During the demosaicing step, rkisp1 ISP is processing denoising and sharpness control. Add demosaicing algorithm with denoise and sharpness values based on user controls. Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com> --- src/ipa/rkisp1/algorithms/demosaicing.cpp | 208 ++++++++++++++++++++++ src/ipa/rkisp1/algorithms/demosaicing.h | 37 ++++ src/ipa/rkisp1/algorithms/meson.build | 1 + src/ipa/rkisp1/data/ov5640.yaml | 1 + src/ipa/rkisp1/rkisp1.cpp | 1 + src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 + 6 files changed, 256 insertions(+) create mode 100644 src/ipa/rkisp1/algorithms/demosaicing.cpp create mode 100644 src/ipa/rkisp1/algorithms/demosaicing.h