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