[libcamera-devel] [libcamera-devel 5/5] ipa: rkisp1: add support of Black Level Correction default values

[Laurent Pinchart]

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 at 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