[libcamera-devel] [PATCH 3/3] ipa: rkisp1: Add support of De-noise Pre-Filter control
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Aug 5 03:11:06 CEST 2022
Hi Florian,
Thank you for the patch.
On Thu, Aug 04, 2022 at 04:12:28PM +0200, Florian Sylvestre via libcamera-devel wrote:
> The De-noise pre-filter algorithm is a bilateral filter which combines a range
I would write "de-noise" without a capital D, and probably "denoise"
actually. Same below where applicable.
> filter and a domain filter. The De-noise pre-filter is applied before
> demosaicing.
>
> Signed-off-by: Florian Sylvestre <fsylvestre at baylibre.com>
> ---
> src/ipa/rkisp1/algorithms/dpf.cpp | 279 ++++++++++++++++++++++++++
> src/ipa/rkisp1/algorithms/dpf.h | 36 ++++
> src/ipa/rkisp1/algorithms/meson.build | 1 +
> src/ipa/rkisp1/data/ov5640.yaml | 11 +
> src/ipa/rkisp1/ipa_context.cpp | 11 +
> src/ipa/rkisp1/ipa_context.h | 5 +
> 6 files changed, 343 insertions(+)
> create mode 100644 src/ipa/rkisp1/algorithms/dpf.cpp
> create mode 100644 src/ipa/rkisp1/algorithms/dpf.h
>
> diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp
> new file mode 100644
> index 00000000..d99bb728
> --- /dev/null
> +++ b/src/ipa/rkisp1/algorithms/dpf.cpp
> @@ -0,0 +1,279 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021-2022, Ideas On Board
> + *
> + * dpf.cpp - RkISP1 De-noise Pre-Filter control
> + */
> +
> +#include "dpf.h"
> +
> +#include <cmath>
> +
> +#include <libcamera/base/log.h>
> +
> +#include <libcamera/control_ids.h>
> +#include "linux/rkisp1-config.h"
> +
> +/**
> + * \file dpf.h
> + */
> +
> +namespace libcamera {
> +
> +namespace ipa::rkisp1::algorithms {
> +
> +/**
> + * \class Dpf
> + * \brief RkISP1 De-noise Pre-Filter control
> + *
> + * The De-noise pre-filter algorithm is a bilateral filter which combines a
> + * range filter and a domain filter. The De-noise pre-filter is applied before
> + * demosaicing.
> + */
> +
> +LOG_DEFINE_CATEGORY(RkISP1Dpf)
> +
> +Dpf::Dpf()
> + : initialized_(false), config_({}), strength_config_({})
> +{
> +}
> +
> +/**
> + * \copydoc libcamera::ipa::Algorithm::init
> + */
> +int Dpf::init([[maybe_unused]] IPAContext &context,
> + const YamlObject &tuningData)
> +{
> + std::vector<double> values;
> +
> + /*
> + * The domain kernel is configured with a 9x9 kernel for the green
> + * pixels, and a 13x9 or 9x9 kernel for red and blue pixels.
> + */
> + const YamlObject &dFObject = tuningData["DomainFilter"];
> + if (!dFObject.isDictionary()) {
> + LOG(RkISP1Dpf, Error)
> + << "Issue while parsing 'DomainFilter' in tuning "
> + << "file: entry must be a dictionary";
> + return -EINVAL;
> + }
I think you could drop the error handling here. If dFObject isn't
correct, then the check on dFObject["g"] below will fail, and that
should be good enough.
> +
> + /*
> + * For the green component, we have the 9x9 kernel specified
> + * as 6 coefficients:
> + * Y
> + * ^
> + * 4 | 6 5 4 5 6
> + * 3 | 5 3 3 5
> + * 2 | 5 3 2 3 5
> + * 1 | 3 1 1 3
> + * 0 - 4 2 0 2 4
> + * -1 | 3 1 1 3
> + * -2 | 5 3 2 3 5
> + * -3 | 5 3 3 5
> + * -4 | 6 5 4 5 6
> + * +---------|--------> X
> + * -4....-1 0 1 2 3 4
> + */
> + values = dFObject["g"].getList<double>().value_or(utils::defopt);
> + if (values.size() != RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS) {
> + LOG(RkISP1Dpf, Error)
> + << "Invalid 'DomainFilter:g': expected "
> + << RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS
> + << " elements, got " << values.size();
> + return -EINVAL;
> + }
> +
> + for (unsigned int i = 0; i < values.size(); ++i) {
> + config_.g_flt.spatial_coeff[i] = std::round(values[i] * 16);
> + }
No need for curly braces.
I wonder if we should store the values as integers in the config file ?
That would simplify the code here, you could then even write it as
std::copy_n(values.begin(), values.size(),
std::begin(config_.g_flt.spatial_coeff));
> +
> + config_.g_flt.gr_enable = true;
> + config_.g_flt.gb_enable = true;
> +
> + /*
> + * For the red and blue components, we have the 13x9 kernel specified
> + * as 6 coefficients:
> + *
> + * Y
> + * ^
> + * 4 | 6 5 4 3 4 5 6
> + * |
> + * 2 | 5 4 2 1 2 4 5
> + * |
> + * 0 - 5 3 1 0 1 3 5
> + * |
> + * -2 | 5 4 2 1 2 4 5
> + * |
> + * -4 | 6 5 4 3 4 5 6
> + * +-------------|------------> X
> + * -6 -4 -2 0 2 4 6
> + *
> + * For a 9x9 kernel, columns -6 and -6 are dropped, so coefficient
"-6 and 6"
> + * number 6 is not used.
> + */
> + values = dFObject["rb"].getList<double>().value_or(utils::defopt);
> + if (values.size() != RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS &&
> + values.size() != RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS - 1) {
> + LOG(RkISP1Dpf, Error)
> + << "Invalid 'DomainFilter:rb': expected "
> + << RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS - 1
> + << " or " << RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS
> + << " elements, got " << values.size();
> + return -EINVAL;
> + }
> +
> + config_.rb_flt.fltsize = values.size() == RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS
> + ? RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_13x9
> + : RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_9x9;
I wonder if there would ever be a use case for switching between two
different filter sizes at runtime. Not something we need to bother with
for now, this looks nice.
> +
> + for (unsigned int i = 0; i < values.size(); ++i) {
> + config_.rb_flt.spatial_coeff[i] = std::round(values[i] * 16);
> + }
Same as above.
> +
> + config_.rb_flt.r_enable = true;
> + config_.rb_flt.b_enable = true;
> +
> + /*
> + * The range kernel is configured with a noise level lookup table (NLL)
> + * which stores a piecewise linear function that characterizes the
> + * sensor noise profile as a noise level function curve (NLF).
> + */
> + const YamlObject &rFObject = tuningData["RangeFilter"];
The data in the tuning file describe the noise level function, so I'd
name the element "NoiseLevelFunction" or something similar.
> + if (!rFObject.isDictionary()) {
> + LOG(RkISP1Dpf, Error)
> + << "Issue while parsing 'RangeFilter' in tuning "
> + << "file: entry must be a dictionary";
> + return -EINVAL;
> + }
Same as above for the error check.
> +
> + values = rFObject["coeff"].getList<double>().value_or(utils::defopt);
> + if (values.size() != RKISP1_CIF_ISP_DPF_MAX_NLF_COEFFS) {
> + LOG(RkISP1Dpf, Error)
> + << "Invalid 'RangeFilter:coeff': expected "
> + << RKISP1_CIF_ISP_DPF_MAX_NLF_COEFFS
> + << " elements, got " << values.size();
> + return -EINVAL;
> + }
> +
> + for (unsigned int i = 0; i < values.size(); ++i) {
> + config_.nll.coeff[i] = std::round(values[i] * 1024);
> + }
> +
> + std::string scaleMode = rFObject["scale-mode"].get<std::string>("");
> + if (scaleMode != "linear" && scaleMode != "logarithmic") {
> + LOG(RkISP1Dpf, Error)
> + << "Invalid 'RangeFilter:scale-mode': expected "
> + << "'linear' or 'logarithmic' value, got "
> + << scaleMode;
> + return -EINVAL;
> + }
> +
> + config_.nll.scale_mode = scaleMode == "linear"
> + ? RKISP1_CIF_ISP_NLL_SCALE_LINEAR
> + : RKISP1_CIF_ISP_NLL_SCALE_LOGARITHMIC;
You could avoid comparing to "linear" twice:
if (scaleMode == "linear") {
config_.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LINEAR;
} else if (scaleMode == "logarithmic") {
config_.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LOGARITHMIC;
} else {
LOG(RkISP1Dpf, Error)
<< "Invalid 'RangeFilter:scale-mode': expected "
<< "'linear' or 'logarithmic' value, got "
<< scaleMode;
return -EINVAL;
}
(I wish a switch/case on a string would be possible)
> +
> + const YamlObject &fSObject = tuningData["FilterStrength"];
> + if (!fSObject.isDictionary()) {
> + LOG(RkISP1Dpf, Error)
> + << "Issue while parsing 'FilterStrength' in tuning "
> + << "file: entry must be a dictionary";
> + return -EINVAL;
> + }
> +
> + strength_config_.r = fSObject["r"].get<uint16_t>(64);
> + strength_config_.g = fSObject["g"].get<uint16_t>(64);
> + strength_config_.b = fSObject["b"].get<uint16_t>(64);
> +
> + initialized_ = true;
> +
> + return 0;
> +}
> +
> +/**
> + * \copydoc libcamera::ipa::Algorithm::queueRequest
> + */
> +void Dpf::queueRequest(IPAContext &context,
> + [[maybe_unused]] const uint32_t frame,
> + const ControlList &controls)
> +{
> + auto &dpf = context.frameContext.dpf;
> +
> + const auto &denoise = controls.get(controls::draft::NoiseReductionMode);
> + if (denoise) {
> + LOG(RkISP1Dpf, Debug) << "Set denoise to " << *denoise;
> +
> + switch (*denoise) {
> + case controls::draft::NoiseReductionModeOff:
> + dpf.denoise = false;
> + dpf.updateParams = true;
> + break;
> + case controls::draft::NoiseReductionModeMinimal:
> + case controls::draft::NoiseReductionModeHighQuality:
> + case controls::draft::NoiseReductionModeFast:
> + dpf.denoise = true;
> + dpf.updateParams = true;
> + break;
> + default:
> + LOG(RkISP1Dpf, Error)
> + << "Unsupported denoise value "
> + << *denoise;
> + }
> + }
> +}
> +
> +/**
> + * \copydoc libcamera::ipa::Algorithm::prepare
> + */
> +void Dpf::prepare(IPAContext &context, rkisp1_params_cfg *params)
> +{
> + auto &dpf = context.frameContext.dpf;
> + auto &awb = context.configuration.awb;
> + auto &lsc = context.configuration.lsc;
> + auto &dpf_config = params->others.dpf_config;
> + auto &dpf_strength_config = params->others.dpf_strength_config;
> +
> + if ((context.frameContext.frameCount > 0) && (!dpf.updateParams))
No need for the inner parentheses.
> + return;
> +
> + if (!initialized_)
> + return;
Move this to the very beginning, before the variables.
> +
> + /*
> + * We must copy the static configuration each time we modify a
> + * parameter in the config because the strucure is zeroed between each
> + * call.
> + */
> + dpf_config = config_;
> + dpf_strength_config = strength_config_;
> +
> + if (dpf.updateParams) {
> + if (!dpf.denoise || (!awb.enabled && !lsc.enabled)) {
> + dpf_config.gain.mode =
> + RKISP1_CIF_ISP_DPF_GAIN_USAGE_DISABLED;
> + } else if (awb.enabled && lsc.enabled) {
> + dpf_config.gain.mode =
> + RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_LSC_GAINS;
> + } else if (awb.enabled) {
> + dpf_config.gain.mode =
> + RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_GAINS;
> + } else { /* lsc.enabled only */
> + dpf_config.gain.mode =
> + RKISP1_CIF_ISP_DPF_GAIN_USAGE_LSC_GAINS;
> + }
> + }
> +
> + dpf.updateParams = false;
> +
> + uint32_t flags = RKISP1_CIF_ISP_MODULE_DPF |
> + RKISP1_CIF_ISP_MODULE_DPF_STRENGTH;
> + params->module_en_update |= flags;
> + params->module_ens |= flags;
> + params->module_cfg_update |= flags;
I don't think this is right. When denoising is disabled, you should
clear RKISP1_CIF_ISP_MODULE_DPF in module_ens to disable it.
The module_cfg_update field indicates which ISP configuration parameters
need to be changed, so it should only be set for the very first frame. I
think the following should do.
if (context.frameContext.frameCount == 0) {
dpf_config = config_;
dpf_strength_config = strength_config_;
if (awb.enabled && lsc.enabled) {
dpf_config.gain.mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_LSC_GAINS;
else if (awb.enabled)
dpf_config.gain.mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_GAINS;
else if (lsc.enabled)
dpf_config.gain.mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_LSC_GAINS;
else
dpf_config.gain.mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_DISABLED;
params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_DPF
| RKISP1_CIF_ISP_MODULE_DPF_STRENGTH;
}
if (dpf.updateParams) {
params->module_en_update |= RKISP1_CIF_ISP_MODULE_DPF;
if (dpf.denoise)
params->module_ens |= RKISP1_CIF_ISP_MODULE_DPF;
dpf.updateParams = false;
}
> +}
> +
> +REGISTER_IPA_ALGORITHM(Dpf, "Dpf")
> +
> +} /* namespace ipa::rkisp1::algorithms */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h
> new file mode 100644
> index 00000000..c8b8419a
> --- /dev/null
> +++ b/src/ipa/rkisp1/algorithms/dpf.h
> @@ -0,0 +1,36 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021-2022, Ideas On Board
> + *
> + * dpf.h - RkISP1 De-noise Pre-Filter control
> + */
> +
> +#pragma once
> +
> +#include <sys/types.h>
> +
> +#include "algorithm.h"
> +
> +namespace libcamera {
> +
> +namespace ipa::rkisp1::algorithms {
> +
> +class Dpf : public Algorithm
> +{
> +public:
> + Dpf();
> + ~Dpf() = default;
> +
> + int init(IPAContext &context, const YamlObject &tuningData) override;
> + void queueRequest(IPAContext &context, const uint32_t frame,
> + const ControlList &controls) override;
> + void prepare(IPAContext &context, rkisp1_params_cfg *params) override;
> +
> +private:
> + bool initialized_;
> + struct rkisp1_cif_isp_dpf_config config_;
> + struct rkisp1_cif_isp_dpf_strength_config strength_config_;
strengthConfig_
> +};
> +
> +} /* namespace ipa::rkisp1::algorithms */
> +} /* namespace libcamera */
> diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build
> index e48974b4..e1492dd5 100644
> --- a/src/ipa/rkisp1/algorithms/meson.build
> +++ b/src/ipa/rkisp1/algorithms/meson.build
> @@ -5,6 +5,7 @@ rkisp1_ipa_algorithms = files([
> 'awb.cpp',
> 'blc.cpp',
> 'cproc.cpp',
> + 'dpf.cpp',
> 'dpcc.cpp',
You nearly got the alphabetical order right :-)
> 'filter.cpp',
> 'gsl.cpp',
> diff --git a/src/ipa/rkisp1/data/ov5640.yaml b/src/ipa/rkisp1/data/ov5640.yaml
> index 93d7d1e7..69c6ad97 100644
> --- a/src/ipa/rkisp1/data/ov5640.yaml
> +++ b/src/ipa/rkisp1/data/ov5640.yaml
> @@ -11,6 +11,17 @@ algorithms:
> Gb: 256
> B: 256
> - ColorProcessing:
> + - Dpf:
> + DomainFilter:
> + g: [ 1.0, 1.0, 1.0, 1.0, 1.0, 1.0 ]
> + rb: [ 1.0, 1.0, 1.0, 1.0, 1.0, 1.0 ]
> + RangeFilter:
> + coeff: [ 0.999, 0.999, 0.999, 0.999, 0.999, 0.999, 0.999, 0.999, 0.999, 0.999, 0.999, 0.999, 0.999, 0.999, 0.999, 0.999, 0.999 ]
> + scale-mode: "linear"
> + FilterStrength:
> + r: 64
> + g: 64
> + b: 64
It won't make a difference for now, but I'd move this after LSC as the
DPF depends on the LSC gains. Actually, I think it would make sense to
sort the algorithms according to the hardware order, with exceptions
where needed.
> - GammaSensorLinearization:
> x-intervals: [ 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2 ]
> y:
> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> index c90bf992..3b92e95e 100644
> --- a/src/ipa/rkisp1/ipa_context.cpp
> +++ b/src/ipa/rkisp1/ipa_context.cpp
> @@ -165,6 +165,17 @@ namespace libcamera::ipa::rkisp1 {
> * \brief Indicates if ISP parameters need to be updated
> */
>
> +/**
> + * \var IPAFrameContext::dpf
> + * \brief Context for the De-noise Pre-Filter algorithm
> + *
> + * \var IPAFrameContext::dpf.denoise
> + * \brief Indicates if de-noise is activated
> + *
> + * \var IPAFrameContext::dpf.updateParams
> + * \brief Indicates if ISP parameters need to be updated
> + */
> +
> /**
> * \var IPAFrameContext::filter
> * \brief Context for the Filter algorithm
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index 0cd6aadb..3a743ac3 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -69,6 +69,11 @@ struct IPAFrameContext {
> bool updateParams;
> } cproc;
>
> + struct {
> + bool denoise;
> + bool updateParams;
> + } dpf;
> +
> struct {
> uint8_t denoise;
> uint8_t sharpness;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list