[libcamera-devel,4/4] ipa: rkisp1: Add support of ColorProcessing control

16534 diff mbox series

Add ColorProcessing algorithm that is in charge to manage brightness, contrast
and saturation controls.
These controls are currently based on user controls.

Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>
---
 .../rkisp1/algorithms/color_processing.cpp    | 101 ++++++++++++++++++
 src/ipa/rkisp1/algorithms/color_processing.h  |  38 +++++++
 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      |  12 +++
 6 files changed, 154 insertions(+)
 create mode 100644 src/ipa/rkisp1/algorithms/color_processing.cpp
 create mode 100644 src/ipa/rkisp1/algorithms/color_processing.h

Hi Florian,

Thank you for the patch.

On Mon, Jul 04, 2022 at 05:23:18PM +0200, Florian Sylvestre via libcamera-devel wrote:
> Add ColorProcessing algorithm that is in charge to manage brightness, contrast
> and saturation controls.
> These controls are currently based on user controls.
> 
> Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>
> ---
>  .../rkisp1/algorithms/color_processing.cpp    | 101 ++++++++++++++++++
>  src/ipa/rkisp1/algorithms/color_processing.h  |  38 +++++++
>  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      |  12 +++
>  6 files changed, 154 insertions(+)
>  create mode 100644 src/ipa/rkisp1/algorithms/color_processing.cpp
>  create mode 100644 src/ipa/rkisp1/algorithms/color_processing.h
> 
> diff --git a/src/ipa/rkisp1/algorithms/color_processing.cpp b/src/ipa/rkisp1/algorithms/color_processing.cpp
> new file mode 100644
> index 00000000..a7f2a470
> --- /dev/null
> +++ b/src/ipa/rkisp1/algorithms/color_processing.cpp
> @@ -0,0 +1,101 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021-2022, Ideas On Board
> + *
> + * lsc.cpp - RkISP1 Defect Pixel Cluster Correction control

lsc.cpp ? :-)

> + */
> +
> +#include "color_processing.h"
> +
> +#include <libcamera/base/log.h>
> +
> +#include <libcamera/control_ids.h>
> +
> +#include "libcamera/internal/yaml_parser.h"
> +#include "linux/rkisp1-config.h"

I think you can drop this header.

> +
> +/**
> + * \file color-processing.h
> + */
> +
> +namespace libcamera {
> +
> +namespace ipa::rkisp1::algorithms {
> +
> +/**
> + * \class ColorProcessing
> + * \brief RkISP1 Color Processing control
> + *
> + * The ColorProcessing algorithm is responsible to apply brightness, contrast
> + * and saturation corrections.
> + * The coefficient applied for brightness, contrast and saturation are directly
> + * provided by user controls.

 * The ColorProcessing algorithm is responsible for applying brightness, contrast
 * and saturation corrections. The values are directly provided through requests
 * by the corresponding controls.

> + */
> +
> +LOG_DEFINE_CATEGORY(RkISP1ColorProcessing)
> +
> +ColorProcessing::ColorProcessing()
> +	: userBrightness_(0), userContrast_(0),
> +	  userSaturation_(0), updateUserControls_(true)
> +{
> +}
> +
> +/**
> + * \copydoc libcamera::ipa::Algorithm::queueRequest
> + */
> +void ColorProcessing::queueRequest([[maybe_unused]] IPAContext &context,
> +				   [[maybe_unused]] const uint32_t frame,
> +				   const ControlList &controls)
> +{
> +	for (auto const &ctrl : controls) {
> +		if (ctrl.first == controls::BRIGHTNESS) {
> +			userBrightness_ = ctrl.second.get<float>();
> +			updateUserControls_ = true;
> +
> +			LOG(RkISP1ColorProcessing, Debug)
> +				<< "Set brightness to: "

s/to: /to /

(same below)

> +				<< int(userBrightness_);
> +		} else if (ctrl.first == controls::CONTRAST) {
> +			userContrast_ = ctrl.second.get<float>();

You need to scale the contrast and saturation values, the controls are
in the range [0.0, 1.992] and the register values are expressed as 8-bit
integers.

> +			updateUserControls_ = true;
> +
> +			LOG(RkISP1ColorProcessing, Debug)
> +				<< "Set contrast to: "
> +				<< int(userContrast_);
> +		} else if (ctrl.first == controls::SATURATION) {
> +			userSaturation_ = ctrl.second.get<float>();
> +			updateUserControls_ = true;
> +
> +			LOG(RkISP1ColorProcessing, Debug)
> +				<< "Set saturation to: "
> +				<< int(userSaturation_);
> +		}
> +	}

No need to loop over the control list. With the usage of std::optional
for ControlList that we're about to merge, this can be written

	const auto &brightness = controls.get(controls::Brightness);
	if (brightness) {
		userBrightness_ = *brightness;
		updateUserControls_ = true;

		LOG(RkISP1ColorProcessing, Debug)
			<< "Set brightness to: "
			<< int(userBrightness_);
	}

and similarly for the contract and saturation controls.

> +}
> +
> +/**
> + * \copydoc libcamera::ipa::Algorithm::prepare
> + */
> +void ColorProcessing::prepare([[maybe_unused]] IPAContext &context,
> +			      rkisp1_params_cfg *params)
> +{
> +	/* Check if the algorithm configuration has been updated. */
> +	if (!updateUserControls_)
> +		return;
> +
> +	updateUserControls_ = false;
> +
> +	params->others.cproc_config.brightness = userBrightness_;
> +	params->others.cproc_config.contrast = userContrast_;
> +	params->others.cproc_config.sat = userSaturation_;
> +
> +	params->module_en_update |= RKISP1_CIF_ISP_MODULE_CPROC;
> +	params->module_ens |= RKISP1_CIF_ISP_MODULE_CPROC;
> +	params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_CPROC;
> +}
> +
> +REGISTER_IPA_ALGORITHM(ColorProcessing, "ColorProcessing")
> +
> +} /* namespace ipa::rkisp1::algorithms */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/rkisp1/algorithms/color_processing.h b/src/ipa/rkisp1/algorithms/color_processing.h
> new file mode 100644
> index 00000000..bdb40bd1
> --- /dev/null
> +++ b/src/ipa/rkisp1/algorithms/color_processing.h
> @@ -0,0 +1,38 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021-2022, Ideas On Board
> + *
> + * dpcc.h - RkISP1 Color Processing control

dpcc.h ? :-)

> + */
> +
> +#pragma once
> +
> +#include <sys/types.h>
> +#include <linux/rkisp1-config.h>

You can drop this.

> +
> +#include "algorithm.h"
> +
> +namespace libcamera {
> +
> +struct IPACameraSensorInfo;

You can drop this.

> +
> +namespace ipa::rkisp1::algorithms {
> +
> +class ColorProcessing : public Algorithm
> +{
> +public:
> +	ColorProcessing();
> +	~ColorProcessing() = default;
> +
> +	void queueRequest(IPAContext &context, const uint32_t frame,
> +			  const ControlList &controls) override;
> +	void prepare(IPAContext &context, rkisp1_params_cfg *params) override;

Missing blank line.

> +private:
> +	uint32_t userBrightness_;
> +	uint32_t userContrast_;
> +	uint32_t userSaturation_;
> +	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 7e078b0d..7f9c3899 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',
> +    'color_processing.cpp',

We could call this cproc.cpp to match the name used by the rkisp1.

>      'demosaicing.cpp',
>      'dpcc.cpp',
>      'gsl.cpp',
> diff --git a/src/ipa/rkisp1/data/ov5640.yaml b/src/ipa/rkisp1/data/ov5640.yaml
> index 4ae0ffc0..331f9d2d 100644
> --- a/src/ipa/rkisp1/data/ov5640.yaml
> +++ b/src/ipa/rkisp1/data/ov5640.yaml
> @@ -10,6 +10,7 @@ algorithms:
>        Gr: 256
>        Gb: 256
>        B:  256
> +  - ColorProcessing:
>    - 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/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 85a75070..d380c59b 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/color_processing.h"
>  #include "algorithms/demosaicing.h"
>  #include "algorithms/dpcc.h"
>  #include "algorithms/gsl.h"
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 3d0075ee..4e82ab39 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -957,6 +957,18 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>  		      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::Brightness),
> +		      std::forward_as_tuple(-128.0f, 127.0f));

Let's make this [-1.0, 1.0] and scale the value in the IPA.

> +
> +	ctrls.emplace(std::piecewise_construct,
> +		      std::forward_as_tuple(&controls::Contrast),
> +		      std::forward_as_tuple(0.0f, 1.992f));
> +
> +	ctrls.emplace(std::piecewise_construct,
> +		      std::forward_as_tuple(&controls::Saturation),
> +		      std::forward_as_tuple(0.0f, 1.992f));
> +
>  	ctrls.emplace(std::piecewise_construct,
>  		      std::forward_as_tuple(&controls::draft::NoiseReductionMode),
>  		      std::forward_as_tuple(controls::draft::NoiseReductionModeValues));