| Message ID | 20220523092435.475510-6-fsylvestre@baylibre.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Florian, Thank you for the patch. On Mon, May 23, 2022 at 11:24:35AM +0200, Florian Sylvestre via libcamera-devel wrote: > Get default values for Black Level Correction algorithm form Yaml > configuration file. s/form Yaml configuration/from YAML tuning/ > Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com> > --- > src/ipa/rkisp1/algorithms/blc.cpp | 46 ++++++++++++++++++++++++++++--- > src/ipa/rkisp1/algorithms/blc.h | 9 ++++++ > 2 files changed, 51 insertions(+), 4 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp > index 0c5948ff..177bc22e 100644 > --- a/src/ipa/rkisp1/algorithms/blc.cpp > +++ b/src/ipa/rkisp1/algorithms/blc.cpp > @@ -7,6 +7,10 @@ > > #include "blc.h" > > +#include <libcamera/base/log.h> > + > +#include <libcamera/internal/yaml_parser.h> For internal headers we use "" instead of <>. > + > /** > * \file blc.h > */ > @@ -29,6 +33,40 @@ namespace ipa::rkisp1::algorithms { > * isn't currently supported. > */ > > +LOG_DEFINE_CATEGORY(RkISP1Blc) > + > +/** > + * \copydoc libcamera::ipa::Algorithm::init > + */ > +int BlackLevelCorrection::init(IPAContext &context, > + const YamlObject *params) > +{ > + if (context.frameContext.frameCount > 0) > + return -EBUSY; > + > + /* Parse property "BlackLevelCorrection" */ > + if (!params->contains("BlackLevelCorrection")) > + return -EINVAL; > + > + const YamlObject &blcObject = (*params)["BlackLevelCorrection"]; > + > + if (!blcObject.isDictionary()) > + return -EINVAL; > + > + blackLevelRed_ = blcObject["R"].get<int32_t>(256); Can the value be negative ? If not, let go to uint32_t. Actually, you'll have uint16_t support when rebasing on top of the YAML parser improvements, so you can use that. > + blackLevelGreenR_ = blcObject["Gr"].get<int32_t>(256); > + blackLevelGreenB_ = blcObject["Gb"].get<int32_t>(256); > + blackLevelBlue_ = blcObject["B"].get<int32_t>(256); > + > + LOG(RkISP1Blc, Debug) > + << " Read black levels red " << blackLevelRed_ > + << " green (red) " << blackLevelGreenR_ > + << " green (blue) " << blackLevelGreenB_ > + << " blue " << blackLevelBlue_; << " Read black levels red " << blackLevelRed_ << ", green (red) " << blackLevelGreenR_ << ", green (blue) " << blackLevelGreenB_ << ", blue " << blackLevelBlue_; > + > + return 0; > +} > + > /** > * \copydoc libcamera::ipa::Algorithm::prepare > */ > @@ -42,10 +80,10 @@ void BlackLevelCorrection::prepare(IPAContext &context, > * \todo Use a configuration file for it ? > */ > params->others.bls_config.enable_auto = 0; > - params->others.bls_config.fixed_val.r = 256; > - params->others.bls_config.fixed_val.gr = 256; > - params->others.bls_config.fixed_val.gb = 256; > - params->others.bls_config.fixed_val.b = 256; > + params->others.bls_config.fixed_val.r = blackLevelRed_; > + params->others.bls_config.fixed_val.gr = blackLevelGreenR_; > + params->others.bls_config.fixed_val.gb = blackLevelGreenB_; > + params->others.bls_config.fixed_val.b = blackLevelBlue_; > > params->module_en_update |= RKISP1_CIF_ISP_MODULE_BLS; > params->module_ens |= RKISP1_CIF_ISP_MODULE_BLS; > diff --git a/src/ipa/rkisp1/algorithms/blc.h b/src/ipa/rkisp1/algorithms/blc.h > index 69874d8f..2f6e2d82 100644 > --- a/src/ipa/rkisp1/algorithms/blc.h > +++ b/src/ipa/rkisp1/algorithms/blc.h > @@ -13,6 +13,8 @@ > > namespace libcamera { > > +class YamlObject; You can drop this as it's alread forward-declared by algorithm.h; > + > struct IPACameraSensorInfo; > > namespace ipa::rkisp1::algorithms { > @@ -23,7 +25,14 @@ public: > BlackLevelCorrection() = default; > ~BlackLevelCorrection() = default; > > + int init(IPAContext &context, const YamlObject *params) override; > void prepare(IPAContext &context, rkisp1_params_cfg *params) override; > + > +private: > + int16_t blackLevelRed_; > + int16_t blackLevelGreenR_; > + int16_t blackLevelGreenB_; > + int16_t blackLevelBlue_; If the values can't be negative let's use uint16_t here. > }; > > } /* namespace ipa::rkisp1::algorithms */
>> + const YamlObject &blcObject = (*params)["BlackLevelCorrection"]; >> + >> + if (!blcObject.isDictionary()) >> + return -EINVAL; >> + >> + blackLevelRed_ = blcObject["R"].get<int32_t>(256); >Can the value be negative ? If not, let go to uint32_t. Actually, you'll >have uint16_t support when rebasing on top of the YAML parser >improvements, so you can use that. The values can be negatives. FYI: Extracted from .h comments: * struct rkisp1_cif_isp_bls_fixed_val - BLS fixed subtraction values * * The values will be subtracted from the sensor * values. Therefore a negative value means addition instead of subtraction! I have seen the [RFC] on modifications for Yaml Parser to support int16, but since it's 'only' at [RFC] state, should we not provide this version and update it when the support of int16 will be available? Regards, Florian Sylvestre Le jeu. 26 mai 2022 à 13:04, Laurent Pinchart < laurent.pinchart@ideasonboard.com> a écrit : > Hi Florian, > > Thank you for the patch. > > On Mon, May 23, 2022 at 11:24:35AM +0200, Florian Sylvestre via > libcamera-devel wrote: > > Get default values for Black Level Correction algorithm form Yaml > > configuration file. > > s/form Yaml configuration/from YAML tuning/ > > > Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com> > > --- > > src/ipa/rkisp1/algorithms/blc.cpp | 46 ++++++++++++++++++++++++++++--- > > src/ipa/rkisp1/algorithms/blc.h | 9 ++++++ > > 2 files changed, 51 insertions(+), 4 deletions(-) > > > > diff --git a/src/ipa/rkisp1/algorithms/blc.cpp > b/src/ipa/rkisp1/algorithms/blc.cpp > > index 0c5948ff..177bc22e 100644 > > --- a/src/ipa/rkisp1/algorithms/blc.cpp > > +++ b/src/ipa/rkisp1/algorithms/blc.cpp > > @@ -7,6 +7,10 @@ > > > > #include "blc.h" > > > > +#include <libcamera/base/log.h> > > + > > +#include <libcamera/internal/yaml_parser.h> > > For internal headers we use "" instead of <>. > > > + > > /** > > * \file blc.h > > */ > > @@ -29,6 +33,40 @@ namespace ipa::rkisp1::algorithms { > > * isn't currently supported. > > */ > > > > +LOG_DEFINE_CATEGORY(RkISP1Blc) > > + > > +/** > > + * \copydoc libcamera::ipa::Algorithm::init > > + */ > > +int BlackLevelCorrection::init(IPAContext &context, > > + const YamlObject *params) > > +{ > > + if (context.frameContext.frameCount > 0) > > + return -EBUSY; > > + > > + /* Parse property "BlackLevelCorrection" */ > > + if (!params->contains("BlackLevelCorrection")) > > + return -EINVAL; > > + > > + const YamlObject &blcObject = (*params)["BlackLevelCorrection"]; > > + > > + if (!blcObject.isDictionary()) > > + return -EINVAL; > > + > > + blackLevelRed_ = blcObject["R"].get<int32_t>(256); > > Can the value be negative ? If not, let go to uint32_t. Actually, you'll > have uint16_t support when rebasing on top of the YAML parser > improvements, so you can use that. > > > + blackLevelGreenR_ = blcObject["Gr"].get<int32_t>(256); > > + blackLevelGreenB_ = blcObject["Gb"].get<int32_t>(256); > > + blackLevelBlue_ = blcObject["B"].get<int32_t>(256); > > + > > + LOG(RkISP1Blc, Debug) > > + << " Read black levels red " << blackLevelRed_ > > + << " green (red) " << blackLevelGreenR_ > > + << " green (blue) " << blackLevelGreenB_ > > + << " blue " << blackLevelBlue_; > > << " Read black levels red " << blackLevelRed_ > << ", green (red) " << blackLevelGreenR_ > << ", green (blue) " << blackLevelGreenB_ > << ", blue " << blackLevelBlue_; > > > + > > + return 0; > > +} > > + > > /** > > * \copydoc libcamera::ipa::Algorithm::prepare > > */ > > @@ -42,10 +80,10 @@ void BlackLevelCorrection::prepare(IPAContext > &context, > > * \todo Use a configuration file for it ? > > */ > > params->others.bls_config.enable_auto = 0; > > - params->others.bls_config.fixed_val.r = 256; > > - params->others.bls_config.fixed_val.gr = 256; > > - params->others.bls_config.fixed_val.gb = 256; > > - params->others.bls_config.fixed_val.b = 256; > > + params->others.bls_config.fixed_val.r = blackLevelRed_; > > + params->others.bls_config.fixed_val.gr = blackLevelGreenR_; > > + params->others.bls_config.fixed_val.gb = blackLevelGreenB_; > > + params->others.bls_config.fixed_val.b = blackLevelBlue_; > > > > params->module_en_update |= RKISP1_CIF_ISP_MODULE_BLS; > > params->module_ens |= RKISP1_CIF_ISP_MODULE_BLS; > > diff --git a/src/ipa/rkisp1/algorithms/blc.h > b/src/ipa/rkisp1/algorithms/blc.h > > index 69874d8f..2f6e2d82 100644 > > --- a/src/ipa/rkisp1/algorithms/blc.h > > +++ b/src/ipa/rkisp1/algorithms/blc.h > > @@ -13,6 +13,8 @@ > > > > namespace libcamera { > > > > +class YamlObject; > > You can drop this as it's alread forward-declared by algorithm.h; > > > + > > struct IPACameraSensorInfo; > > > > namespace ipa::rkisp1::algorithms { > > @@ -23,7 +25,14 @@ public: > > BlackLevelCorrection() = default; > > ~BlackLevelCorrection() = default; > > > > + int init(IPAContext &context, const YamlObject *params) override; > > void prepare(IPAContext &context, rkisp1_params_cfg *params) > override; > > + > > +private: > > + int16_t blackLevelRed_; > > + int16_t blackLevelGreenR_; > > + int16_t blackLevelGreenB_; > > + int16_t blackLevelBlue_; > > If the values can't be negative let's use uint16_t here. > > > }; > > > > } /* namespace ipa::rkisp1::algorithms */ > > -- > Regards, > > Laurent Pinchart >
Hi Florian, On Wed, Jun 01, 2022 at 10:19:44AM +0200, Florian Sylvestre wrote: > >> + const YamlObject &blcObject = (*params)["BlackLevelCorrection"]; > >> + > >> + if (!blcObject.isDictionary()) > >> + return -EINVAL; > >> + > >> + blackLevelRed_ = blcObject["R"].get<int32_t>(256); > > > > Can the value be negative ? If not, let go to uint32_t. Actually, you'll > > have uint16_t support when rebasing on top of the YAML parser > > improvements, so you can use that. > > The values can be negatives. > FYI: Extracted from .h comments: > * struct rkisp1_cif_isp_bls_fixed_val - BLS fixed subtraction values > * > * The values will be subtracted from the sensor > * values. Therefore a negative value means addition instead of subtraction! I don't expect negative values to be very useful, but that's fine. > I have seen the [RFC] on modifications for Yaml Parser to support int16, > but since it's 'only' at [RFC] state, should we not provide this version and > update it when the support of int16 will be available? You can rebase on top of the RFC version (v2 available from https://gitlab.com/ideasonboard/nxp/libcamera/-/commits/pinchartl/yaml/) as it should get merged very soon. > Le jeu. 26 mai 2022 à 13:04, Laurent Pinchart a écrit : > > On Mon, May 23, 2022 at 11:24:35AM +0200, Florian Sylvestre wrote: > > > Get default values for Black Level Correction algorithm form Yaml > > > configuration file. > > > > s/form Yaml configuration/from YAML tuning/ > > > > > Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com> > > > --- > > > src/ipa/rkisp1/algorithms/blc.cpp | 46 ++++++++++++++++++++++++++++--- > > > src/ipa/rkisp1/algorithms/blc.h | 9 ++++++ > > > 2 files changed, 51 insertions(+), 4 deletions(-) > > > > > > diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp > > > index 0c5948ff..177bc22e 100644 > > > --- a/src/ipa/rkisp1/algorithms/blc.cpp > > > +++ b/src/ipa/rkisp1/algorithms/blc.cpp > > > @@ -7,6 +7,10 @@ > > > > > > #include "blc.h" > > > > > > +#include <libcamera/base/log.h> > > > + > > > +#include <libcamera/internal/yaml_parser.h> > > > > For internal headers we use "" instead of <>. > > > > > + > > > /** > > > * \file blc.h > > > */ > > > @@ -29,6 +33,40 @@ namespace ipa::rkisp1::algorithms { > > > * isn't currently supported. > > > */ > > > > > > +LOG_DEFINE_CATEGORY(RkISP1Blc) > > > + > > > +/** > > > + * \copydoc libcamera::ipa::Algorithm::init > > > + */ > > > +int BlackLevelCorrection::init(IPAContext &context, > > > + const YamlObject *params) > > > +{ > > > + if (context.frameContext.frameCount > 0) > > > + return -EBUSY; > > > + > > > + /* Parse property "BlackLevelCorrection" */ > > > + if (!params->contains("BlackLevelCorrection")) > > > + return -EINVAL; > > > + > > > + const YamlObject &blcObject = (*params)["BlackLevelCorrection"]; > > > + > > > + if (!blcObject.isDictionary()) > > > + return -EINVAL; > > > + > > > + blackLevelRed_ = blcObject["R"].get<int32_t>(256); > > > > Can the value be negative ? If not, let go to uint32_t. Actually, you'll > > have uint16_t support when rebasing on top of the YAML parser > > improvements, so you can use that. > > > > > + blackLevelGreenR_ = blcObject["Gr"].get<int32_t>(256); > > > + blackLevelGreenB_ = blcObject["Gb"].get<int32_t>(256); > > > + blackLevelBlue_ = blcObject["B"].get<int32_t>(256); > > > + > > > + LOG(RkISP1Blc, Debug) > > > + << " Read black levels red " << blackLevelRed_ > > > + << " green (red) " << blackLevelGreenR_ > > > + << " green (blue) " << blackLevelGreenB_ > > > + << " blue " << blackLevelBlue_; > > > > << " Read black levels red " << blackLevelRed_ > > << ", green (red) " << blackLevelGreenR_ > > << ", green (blue) " << blackLevelGreenB_ > > << ", blue " << blackLevelBlue_; > > > > > + > > > + return 0; > > > +} > > > + > > > /** > > > * \copydoc libcamera::ipa::Algorithm::prepare > > > */ > > > @@ -42,10 +80,10 @@ void BlackLevelCorrection::prepare(IPAContext &context, > > > * \todo Use a configuration file for it ? > > > */ > > > params->others.bls_config.enable_auto = 0; > > > - params->others.bls_config.fixed_val.r = 256; > > > - params->others.bls_config.fixed_val.gr = 256; > > > - params->others.bls_config.fixed_val.gb = 256; > > > - params->others.bls_config.fixed_val.b = 256; > > > + params->others.bls_config.fixed_val.r = blackLevelRed_; > > > + params->others.bls_config.fixed_val.gr = blackLevelGreenR_; > > > + params->others.bls_config.fixed_val.gb = blackLevelGreenB_; > > > + params->others.bls_config.fixed_val.b = blackLevelBlue_; > > > > > > params->module_en_update |= RKISP1_CIF_ISP_MODULE_BLS; > > > params->module_ens |= RKISP1_CIF_ISP_MODULE_BLS; > > > diff --git a/src/ipa/rkisp1/algorithms/blc.h b/src/ipa/rkisp1/algorithms/blc.h > > > index 69874d8f..2f6e2d82 100644 > > > --- a/src/ipa/rkisp1/algorithms/blc.h > > > +++ b/src/ipa/rkisp1/algorithms/blc.h > > > @@ -13,6 +13,8 @@ > > > > > > namespace libcamera { > > > > > > +class YamlObject; > > > > You can drop this as it's alread forward-declared by algorithm.h; > > > > > + > > > struct IPACameraSensorInfo; > > > > > > namespace ipa::rkisp1::algorithms { > > > @@ -23,7 +25,14 @@ public: > > > BlackLevelCorrection() = default; > > > ~BlackLevelCorrection() = default; > > > > > > + int init(IPAContext &context, const YamlObject *params) override; > > > void prepare(IPAContext &context, rkisp1_params_cfg *params) override; > > > + > > > +private: > > > + int16_t blackLevelRed_; > > > + int16_t blackLevelGreenR_; > > > + int16_t blackLevelGreenB_; > > > + int16_t blackLevelBlue_; > > > > If the values can't be negative let's use uint16_t here. > > > > > }; > > > > > > } /* namespace ipa::rkisp1::algorithms */
diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp index 0c5948ff..177bc22e 100644 --- a/src/ipa/rkisp1/algorithms/blc.cpp +++ b/src/ipa/rkisp1/algorithms/blc.cpp @@ -7,6 +7,10 @@ #include "blc.h" +#include <libcamera/base/log.h> + +#include <libcamera/internal/yaml_parser.h> + /** * \file blc.h */ @@ -29,6 +33,40 @@ namespace ipa::rkisp1::algorithms { * isn't currently supported. */ +LOG_DEFINE_CATEGORY(RkISP1Blc) + +/** + * \copydoc libcamera::ipa::Algorithm::init + */ +int BlackLevelCorrection::init(IPAContext &context, + const YamlObject *params) +{ + if (context.frameContext.frameCount > 0) + return -EBUSY; + + /* Parse property "BlackLevelCorrection" */ + if (!params->contains("BlackLevelCorrection")) + return -EINVAL; + + const YamlObject &blcObject = (*params)["BlackLevelCorrection"]; + + if (!blcObject.isDictionary()) + return -EINVAL; + + blackLevelRed_ = blcObject["R"].get<int32_t>(256); + blackLevelGreenR_ = blcObject["Gr"].get<int32_t>(256); + blackLevelGreenB_ = blcObject["Gb"].get<int32_t>(256); + blackLevelBlue_ = blcObject["B"].get<int32_t>(256); + + LOG(RkISP1Blc, Debug) + << " Read black levels red " << blackLevelRed_ + << " green (red) " << blackLevelGreenR_ + << " green (blue) " << blackLevelGreenB_ + << " blue " << blackLevelBlue_; + + return 0; +} + /** * \copydoc libcamera::ipa::Algorithm::prepare */ @@ -42,10 +80,10 @@ void BlackLevelCorrection::prepare(IPAContext &context, * \todo Use a configuration file for it ? */ params->others.bls_config.enable_auto = 0; - params->others.bls_config.fixed_val.r = 256; - params->others.bls_config.fixed_val.gr = 256; - params->others.bls_config.fixed_val.gb = 256; - params->others.bls_config.fixed_val.b = 256; + params->others.bls_config.fixed_val.r = blackLevelRed_; + params->others.bls_config.fixed_val.gr = blackLevelGreenR_; + params->others.bls_config.fixed_val.gb = blackLevelGreenB_; + params->others.bls_config.fixed_val.b = blackLevelBlue_; params->module_en_update |= RKISP1_CIF_ISP_MODULE_BLS; params->module_ens |= RKISP1_CIF_ISP_MODULE_BLS; diff --git a/src/ipa/rkisp1/algorithms/blc.h b/src/ipa/rkisp1/algorithms/blc.h index 69874d8f..2f6e2d82 100644 --- a/src/ipa/rkisp1/algorithms/blc.h +++ b/src/ipa/rkisp1/algorithms/blc.h @@ -13,6 +13,8 @@ namespace libcamera { +class YamlObject; + struct IPACameraSensorInfo; namespace ipa::rkisp1::algorithms { @@ -23,7 +25,14 @@ public: BlackLevelCorrection() = default; ~BlackLevelCorrection() = default; + int init(IPAContext &context, const YamlObject *params) override; void prepare(IPAContext &context, rkisp1_params_cfg *params) override; + +private: + int16_t blackLevelRed_; + int16_t blackLevelGreenR_; + int16_t blackLevelGreenB_; + int16_t blackLevelBlue_; }; } /* namespace ipa::rkisp1::algorithms */
Get default values for Black Level Correction algorithm form Yaml configuration file. Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com> --- src/ipa/rkisp1/algorithms/blc.cpp | 46 ++++++++++++++++++++++++++++--- src/ipa/rkisp1/algorithms/blc.h | 9 ++++++ 2 files changed, 51 insertions(+), 4 deletions(-)