[v6,09/18] libcamera: ipa: add Soft IPA

19766 diff mbox series

From: Andrey Konovalov <andrey.konovalov@linaro.org>

Define the Soft IPA main and event interfaces, add the Soft IPA
implementation.

The current src/ipa/meson.build assumes the IPA name to match the
pipeline name. For this reason "-Dipas=simple" is used for the
Soft IPA module.

Auto exposure/gain and AWB implementation by Dennis, Toon and Martti.

Auto exposure/gain targets a Mean Sample Value of 2.5 following
the MSV calculation algorithm from:
https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf

Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
Tested-by: Pavel Machek <pavel@ucw.cz>
Reviewed-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
Co-developed-by: Dennis Bonke <admin@dennisbonke.com>
Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
Co-developed-by: Marttico <g.martti@gmail.com>
Signed-off-by: Marttico <g.martti@gmail.com>
Co-developed-by: Toon Langendam <t.langendam@gmail.com>
Signed-off-by: Toon Langendam <t.langendam@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 Documentation/Doxyfile.in         |   1 +
 include/libcamera/ipa/meson.build |   1 +
 include/libcamera/ipa/soft.mojom  |  28 +++
 meson_options.txt                 |   2 +-
 src/ipa/simple/data/meson.build   |   9 +
 src/ipa/simple/data/soft.conf     |   3 +
 src/ipa/simple/meson.build        |  25 +++
 src/ipa/simple/soft_simple.cpp    | 326 ++++++++++++++++++++++++++++++
 8 files changed, 394 insertions(+), 1 deletion(-)
 create mode 100644 include/libcamera/ipa/soft.mojom
 create mode 100644 src/ipa/simple/data/meson.build
 create mode 100644 src/ipa/simple/data/soft.conf
 create mode 100644 src/ipa/simple/meson.build
 create mode 100644 src/ipa/simple/soft_simple.cpp

Hi Milan and Andrey,

Thank you for the patch.

On Tue, Mar 19, 2024 at 01:35:56PM +0100, Milan Zamazal wrote:
> From: Andrey Konovalov <andrey.konovalov@linaro.org>
> 
> Define the Soft IPA main and event interfaces, add the Soft IPA
> implementation.
> 
> The current src/ipa/meson.build assumes the IPA name to match the
> pipeline name. For this reason "-Dipas=simple" is used for the
> Soft IPA module.

This should be addressed, please record this as a \todo item somewhere.

> Auto exposure/gain and AWB implementation by Dennis, Toon and Martti.
> 
> Auto exposure/gain targets a Mean Sample Value of 2.5 following
> the MSV calculation algorithm from:
> https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf
> 
> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
> Tested-by: Pavel Machek <pavel@ucw.cz>
> Reviewed-by: Pavel Machek <pavel@ucw.cz>
> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> Co-developed-by: Dennis Bonke <admin@dennisbonke.com>
> Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
> Co-developed-by: Marttico <g.martti@gmail.com>
> Signed-off-by: Marttico <g.martti@gmail.com>
> Co-developed-by: Toon Langendam <t.langendam@gmail.com>
> Signed-off-by: Toon Langendam <t.langendam@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  Documentation/Doxyfile.in         |   1 +
>  include/libcamera/ipa/meson.build |   1 +
>  include/libcamera/ipa/soft.mojom  |  28 +++
>  meson_options.txt                 |   2 +-
>  src/ipa/simple/data/meson.build   |   9 +
>  src/ipa/simple/data/soft.conf     |   3 +
>  src/ipa/simple/meson.build        |  25 +++
>  src/ipa/simple/soft_simple.cpp    | 326 ++++++++++++++++++++++++++++++
>  8 files changed, 394 insertions(+), 1 deletion(-)
>  create mode 100644 include/libcamera/ipa/soft.mojom
>  create mode 100644 src/ipa/simple/data/meson.build
>  create mode 100644 src/ipa/simple/data/soft.conf
>  create mode 100644 src/ipa/simple/meson.build
>  create mode 100644 src/ipa/simple/soft_simple.cpp
> 
> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> index a86ea6c1..2be8d47b 100644
> --- a/Documentation/Doxyfile.in
> +++ b/Documentation/Doxyfile.in
> @@ -44,6 +44,7 @@ EXCLUDE                = @TOP_SRCDIR@/include/libcamera/base/span.h \
>                           @TOP_SRCDIR@/src/libcamera/pipeline/ \
>                           @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \
>                           @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \
> +                         @TOP_BUILDDIR@/include/libcamera/ipa/soft_ipa_interface.h \

Why is this needed ?

>                           @TOP_BUILDDIR@/src/libcamera/proxy/
>  
>  EXCLUDE_PATTERNS       = @TOP_BUILDDIR@/include/libcamera/ipa/*_serializer.h \
> diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build
> index f3b4881c..3352d08f 100644
> --- a/include/libcamera/ipa/meson.build
> +++ b/include/libcamera/ipa/meson.build
> @@ -65,6 +65,7 @@ pipeline_ipa_mojom_mapping = {
>      'ipu3': 'ipu3.mojom',
>      'rkisp1': 'rkisp1.mojom',
>      'rpi/vc4': 'raspberrypi.mojom',
> +    'simple': 'soft.mojom',
>      'vimc': 'vimc.mojom',
>  }
>  
> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom
> new file mode 100644
> index 00000000..c249bd75
> --- /dev/null
> +++ b/include/libcamera/ipa/soft.mojom
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +
> +/*
> + * \todo Document the interface and remove the related EXCLUDE_PATTERNS entry.

Ah that's why. It doesn't have to be done before merging, but could you
address this sooner than later ?

> + */
> +
> +module ipa.soft;
> +
> +import "include/libcamera/ipa/core.mojom";
> +
> +interface IPASoftInterface {
> +	init(libcamera.IPASettings settings,
> +	     libcamera.SharedFD fdStats,
> +	     libcamera.SharedFD fdParams,

The IPA and soft ISP run in separate threads. How do you guarantee that
the stats and params are not accessed concurrently by both ? Shouldn't
you have a pool of buffers for each, mimicking what is done with
hardware ISPs ?

> +	     libcamera.ControlInfoMap sensorCtrlInfoMap)
> +		=> (int32 ret);
> +	start() => (int32 ret);
> +	stop();
> +	configure(libcamera.ControlInfoMap sensorCtrlInfoMap)
> +		=> (int32 ret);
> +
> +	[async] processStats(libcamera.ControlList sensorControls);
> +};
> +
> +interface IPASoftEventInterface {
> +	setSensorControls(libcamera.ControlList sensorControls);
> +	setIspParams(int32 dummy);

Drop the dummy value.

> +};
> diff --git a/meson_options.txt b/meson_options.txt
> index 99dab96d..2644bef0 100644
> --- a/meson_options.txt
> +++ b/meson_options.txt
> @@ -27,7 +27,7 @@ option('gstreamer',
>  
>  option('ipas',
>          type : 'array',
> -        choices : ['ipu3', 'rkisp1', 'rpi/vc4', 'vimc'],
> +        choices : ['ipu3', 'rkisp1', 'rpi/vc4', 'simple', 'vimc'],
>          description : 'Select which IPA modules to build')
>  
>  option('lc-compliance',
> diff --git a/src/ipa/simple/data/meson.build b/src/ipa/simple/data/meson.build
> new file mode 100644
> index 00000000..33548cc6
> --- /dev/null
> +++ b/src/ipa/simple/data/meson.build
> @@ -0,0 +1,9 @@
> +# SPDX-License-Identifier: CC0-1.0
> +
> +conf_files = files([
> +    'soft.conf',
> +])
> +
> +install_data(conf_files,
> +             install_dir : ipa_data_dir / 'soft',
> +             install_tag : 'runtime')
> diff --git a/src/ipa/simple/data/soft.conf b/src/ipa/simple/data/soft.conf
> new file mode 100644
> index 00000000..0c70e7c0
> --- /dev/null
> +++ b/src/ipa/simple/data/soft.conf

We use YAML files for all IPAs, could you do the same here ? It
shouldn't be much extra work as the file is empty :-)

> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: LGPL-2.1-or-later
> +#
> +# Dummy configuration file for the soft IPA.
> diff --git a/src/ipa/simple/meson.build b/src/ipa/simple/meson.build
> new file mode 100644
> index 00000000..3e863db7
> --- /dev/null
> +++ b/src/ipa/simple/meson.build
> @@ -0,0 +1,25 @@
> +# SPDX-License-Identifier: CC0-1.0
> +
> +ipa_name = 'ipa_soft_simple'
> +
> +mod = shared_module(ipa_name,
> +                    ['soft_simple.cpp', libcamera_generated_ipa_headers],
> +                    name_prefix : '',
> +                    include_directories : [ipa_includes, libipa_includes],
> +                    dependencies : libcamera_private,
> +                    link_with : libipa,
> +                    install : true,
> +                    install_dir : ipa_install_dir)
> +
> +if ipa_sign_module
> +    custom_target(ipa_name + '.so.sign',
> +                  input : mod,
> +                  output : ipa_name + '.so.sign',
> +                  command : [ipa_sign, ipa_priv_key, '@INPUT@', '@OUTPUT@'],
> +                  install : false,
> +                  build_by_default : true)
> +endif
> +
> +subdir('data')
> +
> +ipa_names += ipa_name
> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> new file mode 100644
> index 00000000..312df4ba
> --- /dev/null
> +++ b/src/ipa/simple/soft_simple.cpp
> @@ -0,0 +1,326 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2023, Linaro Ltd
> + *
> + * soft_simple.cpp - Simple Software Image Processing Algorithm module
> + */
> +
> +#include <sys/mman.h>
> +
> +#include <libcamera/base/file.h>
> +#include <libcamera/base/log.h>
> +#include <libcamera/base/shared_fd.h>
> +
> +#include <libcamera/control_ids.h>
> +#include <libcamera/controls.h>
> +
> +#include <libcamera/ipa/ipa_interface.h>
> +#include <libcamera/ipa/ipa_module_info.h>
> +#include <libcamera/ipa/soft_ipa_interface.h>
> +
> +#include "libcamera/internal/camera_sensor.h"

Not needed.

> +#include "libcamera/internal/software_isp/debayer_params.h"
> +#include "libcamera/internal/software_isp/swisp_stats.h"
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(IPASoft)
> +
> +namespace ipa::soft {
> +
> +class IPASoftSimple : public ipa::soft::IPASoftInterface
> +{
> +public:
> +	IPASoftSimple()
> +		: params_(static_cast<DebayerParams *>(MAP_FAILED)),

Initialize this to nullptr, that's more standard, and will make the
tests in the destructor nicer. init() will have to set the pointers to
null if mmap() calls fail.

> +		  stats_(static_cast<SwIspStats *>(MAP_FAILED)), ignore_updates_(0)

s/ignore_updates_/ignoreUpdates_/

> +	{
> +	}
> +
> +	~IPASoftSimple()
> +	{
> +		if (stats_ != MAP_FAILED)
> +			munmap(stats_, sizeof(SwIspStats));
> +		if (params_ != MAP_FAILED)
> +			munmap(params_, sizeof(DebayerParams));
> +	}

No need to inline this.

> +
> +	int init(const IPASettings &settings,
> +		 const SharedFD &fdStats,
> +		 const SharedFD &fdParams,
> +		 const ControlInfoMap &sensorInfoMap) override;
> +	int configure(const ControlInfoMap &sensorInfoMap) override;
> +
> +	int start() override;
> +	void stop() override;
> +
> +	void processStats(const ControlList &sensorControls) override;
> +
> +private:
> +	void updateExposure(double exposureMSV);
> +
> +	SharedFD fdStats_;
> +	SharedFD fdParams_;
> +	DebayerParams *params_;
> +	SwIspStats *stats_;
> +
> +	int32_t exposure_min_, exposure_max_;
> +	int32_t again_min_, again_max_;
> +	int32_t again_, exposure_;
> +	unsigned int ignore_updates_;
> +};
> +
> +int IPASoftSimple::init([[maybe_unused]] const IPASettings &settings,
> +			const SharedFD &fdStats,
> +			const SharedFD &fdParams,
> +			const ControlInfoMap &sensorInfoMap)
> +{
> +	fdStats_ = fdStats;

As you never use fdStats_ and fdParams_ after this function returns,
there's no need to store a copy.

> +	if (!fdStats_.isValid()) {
> +		LOG(IPASoft, Error) << "Invalid Statistics handle";
> +		return -ENODEV;
> +	}
> +
> +	fdParams_ = fdParams;
> +	if (!fdParams_.isValid()) {
> +		LOG(IPASoft, Error) << "Invalid Parameters handle";
> +		return -ENODEV;
> +	}
> +
> +	params_ = static_cast<DebayerParams *>(mmap(nullptr, sizeof(DebayerParams),
> +						    PROT_WRITE, MAP_SHARED,
> +						    fdParams_.get(), 0));
> +	if (params_ == MAP_FAILED) {
> +		LOG(IPASoft, Error) << "Unable to map Parameters";
> +		return -errno;
> +	}

	void *mem = mmap(nullptr, sizeof(DebayerParams), PROT_WRITE, MAP_SHARED,
			 fdParams_.get(), 0));
	if (mem == MAP_FAILED) {
		LOG(IPASoft, Error) << "Unable to map Parameters";
		return -errno;
	}

	params_ = static_cast<DebayerParams *>(mem);

> +
> +	stats_ = static_cast<SwIspStats *>(mmap(nullptr, sizeof(SwIspStats),
> +						PROT_READ, MAP_SHARED,
> +						fdStats_.get(), 0));
> +	if (stats_ == MAP_FAILED) {
> +		LOG(IPASoft, Error) << "Unable to map Statistics";
> +		return -errno;
> +	}
> +
> +	if (sensorInfoMap.find(V4L2_CID_EXPOSURE) == sensorInfoMap.end()) {
> +		LOG(IPASoft, Error) << "Don't have exposure control";
> +		return -EINVAL;
> +	}
> +
> +	if (sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN) == sensorInfoMap.end()) {
> +		LOG(IPASoft, Error) << "Don't have gain control";
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +int IPASoftSimple::configure(const ControlInfoMap &sensorInfoMap)
> +{
> +	const ControlInfo &exposure_info = sensorInfoMap.find(V4L2_CID_EXPOSURE)->second;

s/exposure_info/exposureInfo/

I'll let you address the other snake_case to camelCase conversions in
the series :-)

> +	const ControlInfo &gain_info = sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN)->second;
> +
> +	exposure_min_ = exposure_info.min().get<int32_t>();
> +	exposure_max_ = exposure_info.max().get<int32_t>();
> +	if (!exposure_min_) {
> +		LOG(IPASoft, Warning) << "Minimum exposure is zero, that can't be linear";
> +		exposure_min_ = 1;
> +	}
> +
> +	again_min_ = gain_info.min().get<int32_t>();
> +	again_max_ = gain_info.max().get<int32_t>();

Add a blank line.

> +	/*
> +	 * The camera sensor gain (g) is usually not equal to the value written
> +	 * into the gain register (x). But the way how the AGC algorithm changes
> +	 * the gain value to make the total exposure closer to the optimum assumes
> +	 * that g(x) is not too far from linear function. If the minimal gain is 0,
> +	 * the g(x) is likely to be far from the linear, like g(x) = a / (b * x + c).
> +	 * To avoid unexpected changes to the gain by the AGC algorithm (abrupt near
> +	 * one edge, and very small near the other) we limit the range of the gain
> +	 * values used.
> +	 */
> +	if (!again_min_) {
> +		LOG(IPASoft, Warning) << "Minimum gain is zero, that can't be linear";
> +		again_min_ = std::min(100, again_min_ / 2 + again_max_ / 2);
> +	}

A patch further in the series addresses this by using
CameraSensorHelper. It should be squashed with this patch.

> +
> +	LOG(IPASoft, Info) << "Exposure " << exposure_min_ << "-" << exposure_max_
> +			   << ", gain " << again_min_ << "-" << again_max_;
> +
> +	return 0;
> +}
> +
> +int IPASoftSimple::start()
> +{
> +	return 0;
> +}
> +
> +void IPASoftSimple::stop()
> +{
> +}
> +
> +/*
> + * The number of bins to use for the optimal exposure calculations.
> + */
> +static constexpr unsigned int kExposureBinsCount = 5;

Missing blank line. Same below.

> +/*
> + * The exposure is optimal when the mean sample value of the histogram is
> + * in the middle of the range.
> + */
> +static constexpr float kExposureOptimal = kExposureBinsCount / 2.0;
> +/*
> + * The below value implements the hysteresis for the exposure adjustment.
> + * It is small enough to have the exposure close to the optimal, and is big
> + * enough to prevent the exposure from wobbling around the optimal value.
> + */
> +static constexpr float kExposureSatisfactory = 0.2;

Move these to the beginning of the file, just before the IPASoftSimple
definition.

> +
> +void IPASoftSimple::processStats(const ControlList &sensorControls)
> +{
> +	/*
> +	 * Calculate red and blue gains for AWB.
> +	 * Clamp max gain at 4.0, this also avoids 0 division.
> +	 */
> +	if (stats_->sumR_ <= stats_->sumG_ / 4)
> +		params_->gainR = 1024;
> +	else
> +		params_->gainR = 256 * stats_->sumG_ / stats_->sumR_;
> +
> +	if (stats_->sumB_ <= stats_->sumG_ / 4)
> +		params_->gainB = 1024;
> +	else
> +		params_->gainB = 256 * stats_->sumG_ / stats_->sumB_;
> +
> +	/* Green gain and gamma values are fixed */
> +	params_->gainG = 256;
> +	params_->gamma = 0.5;
> +
> +	setIspParams.emit(0);

Do you envision switching to the libipa/algorithm.h API at some point ?
If so, it would be nice to record it somewhere.

> +
> +	/*
> +	 * AE / AGC, use 2 frames delay to make sure that the exposure and
> +	 * the gain set have applied to the camera sensor.
> +	 */
> +	if (ignore_updates_ > 0) {
> +		--ignore_updates_;
> +		return;
> +	}

Also something that could be improved and that should be recorded
somewhere.

> +
> +	/*
> +	 * Calculate Mean Sample Value (MSV) according to formula from:
> +	 * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf
> +	 */
> +	constexpr unsigned int yHistValsPerBin =
> +		SwIspStats::kYHistogramSize / kExposureBinsCount;
> +	constexpr unsigned int yHistValsPerBinMod =
> +		SwIspStats::kYHistogramSize /
> +		(SwIspStats::kYHistogramSize % kExposureBinsCount + 1);
> +	int ExposureBins[kExposureBinsCount] = {};

s/ExposureBins/exposureBins/

> +	unsigned int denom = 0;
> +	unsigned int num = 0;
> +
> +	for (unsigned int i = 0; i < SwIspStats::kYHistogramSize; i++) {
> +		unsigned int idx = (i - (i / yHistValsPerBinMod)) / yHistValsPerBin;
> +		ExposureBins[idx] += stats_->yHistogram[i];
> +	}
> +
> +	for (unsigned int i = 0; i < kExposureBinsCount; i++) {
> +		LOG(IPASoft, Debug) << i << ": " << ExposureBins[i];
> +		denom += ExposureBins[i];
> +		num += ExposureBins[i] * (i + 1);
> +	}
> +
> +	float exposureMSV = (float)num / denom;

C++ casts.

> +
> +	/* sanity check */

s/sanity/Sanity/

> +	if (!sensorControls.contains(V4L2_CID_EXPOSURE) ||
> +	    !sensorControls.contains(V4L2_CID_ANALOGUE_GAIN)) {
> +		LOG(IPASoft, Error) << "Control(s) missing";
> +		return;
> +	}
> +
> +	ControlList ctrls(sensorControls);

You shouldn't copy the control list, but instead create one from the
ControlInfoMap that you get in init() (and that you then need to stored
in the IPA class).

> +
> +	exposure_ = ctrls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> +	again_ = ctrls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();

	exposure_ = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
	again_ = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();

> +
> +	updateExposure(exposureMSV);
> +
> +	ctrls.set(V4L2_CID_EXPOSURE, exposure_);
> +	ctrls.set(V4L2_CID_ANALOGUE_GAIN, again_);
> +
> +	ignore_updates_ = 2;
> +
> +	setSensorControls.emit(ctrls);
> +
> +	LOG(IPASoft, Debug) << "exposureMSV " << exposureMSV
> +			    << " exp " << exposure_ << " again " << again_
> +			    << " gain R/B " << params_->gainR << "/" << params_->gainB;
> +}
> +
> +void IPASoftSimple::updateExposure(double exposureMSV)
> +{
> +	/* DENOMINATOR of 10 gives ~10% increment/decrement; DENOMINATOR of 5 - about ~20% */

Line wrap, and the DENOMINATOR needs to be renamed to kExpDenominator.

> +	static constexpr uint8_t kExpDenominator = 10;
> +	static constexpr uint8_t kExpNumeratorUp = kExpDenominator + 1;
> +	static constexpr uint8_t kExpNumeratorDown = kExpDenominator - 1;
> +
> +	int next;
> +
> +	if (exposureMSV < kExposureOptimal - kExposureSatisfactory) {
> +		next = exposure_ * kExpNumeratorUp / kExpDenominator;
> +		if (next - exposure_ < 1)
> +			exposure_ += 1;
> +		else
> +			exposure_ = next;
> +		if (exposure_ >= exposure_max_) {
> +			next = again_ * kExpNumeratorUp / kExpDenominator;
> +			if (next - again_ < 1)
> +				again_ += 1;
> +			else
> +				again_ = next;
> +		}
> +	}
> +
> +	if (exposureMSV > kExposureOptimal + kExposureSatisfactory) {
> +		if (exposure_ == exposure_max_ && again_ != again_min_) {
> +			next = again_ * kExpNumeratorDown / kExpDenominator;
> +			if (again_ - next < 1)
> +				again_ -= 1;
> +			else
> +				again_ = next;
> +		} else {
> +			next = exposure_ * kExpNumeratorDown / kExpDenominator;
> +			if (exposure_ - next < 1)
> +				exposure_ -= 1;
> +			else
> +				exposure_ = next;
> +		}
> +	}
> +
> +	exposure_ = std::clamp(exposure_, exposure_min_, exposure_max_);
> +	again_ = std::clamp(again_, again_min_, again_max_);
> +}
> +
> +} /* namespace ipa::soft */
> +
> +/*
> + * External IPA module interface
> + */
> +extern "C" {
> +const struct IPAModuleInfo ipaModuleInfo = {
> +	IPA_MODULE_API_VERSION,
> +	0,
> +	"SimplePipelineHandler",
> +	"simple",
> +};
> +
> +IPAInterface *ipaCreate()
> +{
> +	return new ipa::soft::IPASoftSimple();
> +}
> +
> +} /* extern "C" */
> +
> +} /* namespace libcamera */

Hi Laurent,

Thank you for the review!

On 27.03.2024 19:38, Laurent Pinchart wrote:
> Hi Milan and Andrey,
> 
> Thank you for the patch.
> 
> On Tue, Mar 19, 2024 at 01:35:56PM +0100, Milan Zamazal wrote:
>> From: Andrey Konovalov <andrey.konovalov@linaro.org>
>>
>> Define the Soft IPA main and event interfaces, add the Soft IPA
>> implementation.
>>
>> The current src/ipa/meson.build assumes the IPA name to match the
>> pipeline name. For this reason "-Dipas=simple" is used for the
>> Soft IPA module.
> 
> This should be addressed, please record this as a \todo item somewhere.

Ack

>> Auto exposure/gain and AWB implementation by Dennis, Toon and Martti.
>>
>> Auto exposure/gain targets a Mean Sample Value of 2.5 following
>> the MSV calculation algorithm from:
>> https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf
>>
>> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
>> Tested-by: Pavel Machek <pavel@ucw.cz>
>> Reviewed-by: Pavel Machek <pavel@ucw.cz>
>> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
>> Co-developed-by: Dennis Bonke <admin@dennisbonke.com>
>> Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
>> Co-developed-by: Marttico <g.martti@gmail.com>
>> Signed-off-by: Marttico <g.martti@gmail.com>
>> Co-developed-by: Toon Langendam <t.langendam@gmail.com>
>> Signed-off-by: Toon Langendam <t.langendam@gmail.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   Documentation/Doxyfile.in         |   1 +
>>   include/libcamera/ipa/meson.build |   1 +
>>   include/libcamera/ipa/soft.mojom  |  28 +++
>>   meson_options.txt                 |   2 +-
>>   src/ipa/simple/data/meson.build   |   9 +
>>   src/ipa/simple/data/soft.conf     |   3 +
>>   src/ipa/simple/meson.build        |  25 +++
>>   src/ipa/simple/soft_simple.cpp    | 326 ++++++++++++++++++++++++++++++
>>   8 files changed, 394 insertions(+), 1 deletion(-)
>>   create mode 100644 include/libcamera/ipa/soft.mojom
>>   create mode 100644 src/ipa/simple/data/meson.build
>>   create mode 100644 src/ipa/simple/data/soft.conf
>>   create mode 100644 src/ipa/simple/meson.build
>>   create mode 100644 src/ipa/simple/soft_simple.cpp
>>
>> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
>> index a86ea6c1..2be8d47b 100644
>> --- a/Documentation/Doxyfile.in
>> +++ b/Documentation/Doxyfile.in
>> @@ -44,6 +44,7 @@ EXCLUDE                = @TOP_SRCDIR@/include/libcamera/base/span.h \
>>                            @TOP_SRCDIR@/src/libcamera/pipeline/ \
>>                            @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \
>>                            @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \
>> +                         @TOP_BUILDDIR@/include/libcamera/ipa/soft_ipa_interface.h \
> 
> Why is this needed ?
> 
>>                            @TOP_BUILDDIR@/src/libcamera/proxy/
>>   
>>   EXCLUDE_PATTERNS       = @TOP_BUILDDIR@/include/libcamera/ipa/*_serializer.h \
>> diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build
>> index f3b4881c..3352d08f 100644
>> --- a/include/libcamera/ipa/meson.build
>> +++ b/include/libcamera/ipa/meson.build
>> @@ -65,6 +65,7 @@ pipeline_ipa_mojom_mapping = {
>>       'ipu3': 'ipu3.mojom',
>>       'rkisp1': 'rkisp1.mojom',
>>       'rpi/vc4': 'raspberrypi.mojom',
>> +    'simple': 'soft.mojom',
>>       'vimc': 'vimc.mojom',
>>   }
>>   
>> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom
>> new file mode 100644
>> index 00000000..c249bd75
>> --- /dev/null
>> +++ b/include/libcamera/ipa/soft.mojom
>> @@ -0,0 +1,28 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +
>> +/*
>> + * \todo Document the interface and remove the related EXCLUDE_PATTERNS entry.
> 
> Ah that's why.

Yes, because, well... all the other IPAs were doing that...

> It doesn't have to be done before merging, but could you
> address this sooner than later ?

I can't promise a lot, but I'll look into that.

>> + */
>> +
>> +module ipa.soft;
>> +
>> +import "include/libcamera/ipa/core.mojom";
>> +
>> +interface IPASoftInterface {
>> +	init(libcamera.IPASettings settings,
>> +	     libcamera.SharedFD fdStats,
>> +	     libcamera.SharedFD fdParams,
> 
> The IPA and soft ISP run in separate threads. How do you guarantee that
> the stats and params are not accessed concurrently by both ?

I tried avoiding using a pool of buffers for stats and params, and used just a
single chunk of shared memory to pass the stats from soft ISP (its worker thread)
to IPA, and the other chunk for params.

The soft ISP accumulates the stats in its member variable, and after all the stats
for the frame are calculated, the member variable is copyed to the shared memory and
statReady signal is emitted (so using the member var implements some kind of double
buffering). This way the write to the stats shared memory happens once per frame and
is short in time right before emitting the statReady signal.
The callback executed on receiving statReady signal invokes the IPA's processStats
function. So the IPA's processStats() is called soon after the statReady is emitted.
If the IPA finish processing the stats before the next frame is processed by the soft IPS
and the stats for the next frame are written into the shared memory, there is no concurrent
acess.

This doesn't strictly guarantee that the stats are not accessed concurrently by the IPA and
soft ISP. But unless the video stream from the camera sensor overloads the system this
scheme works OK. And if the load is high the concurrent access isn't impossible, and can harm
the image quality, but doesn't lead to critical problems like invalid pointers as the stats
structure only contains numbers and an array of fixed size.

> Shouldn't
> you have a pool of buffers for each, mimicking what is done with
> hardware ISPs ?

This would definitely work, but is a more heavy wait implementation.

On the other side, the current implementation is less universal (isn't very scalable), more fragile,
and can't reliably associate the stats and the params with the given frame from the camera sensor.

So let me try to implement what you suggested ("a pool of buffers for each, mimicking what is done with
hardware ISPs").

>> +	     libcamera.ControlInfoMap sensorCtrlInfoMap)
>> +		=> (int32 ret);
>> +	start() => (int32 ret);
>> +	stop();
>> +	configure(libcamera.ControlInfoMap sensorCtrlInfoMap)
>> +		=> (int32 ret);
>> +
>> +	[async] processStats(libcamera.ControlList sensorControls);
>> +};
>> +
>> +interface IPASoftEventInterface {
>> +	setSensorControls(libcamera.ControlList sensorControls);
>> +	setIspParams(int32 dummy);
> 
> Drop the dummy value.

libcamera docs do allow signals with zero parameters.
But when I tried having zero parameters for an EventInterface function,
it didn't work for me iirc.
Let me double check.

>> +};
>> diff --git a/meson_options.txt b/meson_options.txt
>> index 99dab96d..2644bef0 100644
>> --- a/meson_options.txt
>> +++ b/meson_options.txt
>> @@ -27,7 +27,7 @@ option('gstreamer',
>>   
>>   option('ipas',
>>           type : 'array',
>> -        choices : ['ipu3', 'rkisp1', 'rpi/vc4', 'vimc'],
>> +        choices : ['ipu3', 'rkisp1', 'rpi/vc4', 'simple', 'vimc'],
>>           description : 'Select which IPA modules to build')
>>   
>>   option('lc-compliance',
>> diff --git a/src/ipa/simple/data/meson.build b/src/ipa/simple/data/meson.build
>> new file mode 100644
>> index 00000000..33548cc6
>> --- /dev/null
>> +++ b/src/ipa/simple/data/meson.build
>> @@ -0,0 +1,9 @@
>> +# SPDX-License-Identifier: CC0-1.0
>> +
>> +conf_files = files([
>> +    'soft.conf',
>> +])
>> +
>> +install_data(conf_files,
>> +             install_dir : ipa_data_dir / 'soft',
>> +             install_tag : 'runtime')
>> diff --git a/src/ipa/simple/data/soft.conf b/src/ipa/simple/data/soft.conf
>> new file mode 100644
>> index 00000000..0c70e7c0
>> --- /dev/null
>> +++ b/src/ipa/simple/data/soft.conf
> 
> We use YAML files for all IPAs, could you do the same here ? It
> shouldn't be much extra work as the file is empty :-)

OK

>> @@ -0,0 +1,3 @@
>> +# SPDX-License-Identifier: LGPL-2.1-or-later
>> +#
>> +# Dummy configuration file for the soft IPA.
>> diff --git a/src/ipa/simple/meson.build b/src/ipa/simple/meson.build
>> new file mode 100644
>> index 00000000..3e863db7
>> --- /dev/null
>> +++ b/src/ipa/simple/meson.build
>> @@ -0,0 +1,25 @@
>> +# SPDX-License-Identifier: CC0-1.0
>> +
>> +ipa_name = 'ipa_soft_simple'
>> +
>> +mod = shared_module(ipa_name,
>> +                    ['soft_simple.cpp', libcamera_generated_ipa_headers],
>> +                    name_prefix : '',
>> +                    include_directories : [ipa_includes, libipa_includes],
>> +                    dependencies : libcamera_private,
>> +                    link_with : libipa,
>> +                    install : true,
>> +                    install_dir : ipa_install_dir)
>> +
>> +if ipa_sign_module
>> +    custom_target(ipa_name + '.so.sign',
>> +                  input : mod,
>> +                  output : ipa_name + '.so.sign',
>> +                  command : [ipa_sign, ipa_priv_key, '@INPUT@', '@OUTPUT@'],
>> +                  install : false,
>> +                  build_by_default : true)
>> +endif
>> +
>> +subdir('data')
>> +
>> +ipa_names += ipa_name
>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
>> new file mode 100644
>> index 00000000..312df4ba
>> --- /dev/null
>> +++ b/src/ipa/simple/soft_simple.cpp
>> @@ -0,0 +1,326 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2023, Linaro Ltd
>> + *
>> + * soft_simple.cpp - Simple Software Image Processing Algorithm module
>> + */
>> +
>> +#include <sys/mman.h>
>> +
>> +#include <libcamera/base/file.h>
>> +#include <libcamera/base/log.h>
>> +#include <libcamera/base/shared_fd.h>
>> +
>> +#include <libcamera/control_ids.h>
>> +#include <libcamera/controls.h>
>> +
>> +#include <libcamera/ipa/ipa_interface.h>
>> +#include <libcamera/ipa/ipa_module_info.h>
>> +#include <libcamera/ipa/soft_ipa_interface.h>
>> +
>> +#include "libcamera/internal/camera_sensor.h"
> 
> Not needed.

OK

>> +#include "libcamera/internal/software_isp/debayer_params.h"
>> +#include "libcamera/internal/software_isp/swisp_stats.h"
>> +
>> +namespace libcamera {
>> +
>> +LOG_DEFINE_CATEGORY(IPASoft)
>> +
>> +namespace ipa::soft {
>> +
>> +class IPASoftSimple : public ipa::soft::IPASoftInterface
>> +{
>> +public:
>> +	IPASoftSimple()
>> +		: params_(static_cast<DebayerParams *>(MAP_FAILED)),
> 
> Initialize this to nullptr, that's more standard, and will make the
> tests in the destructor nicer. init() will have to set the pointers to
> null if mmap() calls fail.

OK

>> +		  stats_(static_cast<SwIspStats *>(MAP_FAILED)), ignore_updates_(0)
> 
> s/ignore_updates_/ignoreUpdates_/

OK

>> +	{
>> +	}
>> +
>> +	~IPASoftSimple()
>> +	{
>> +		if (stats_ != MAP_FAILED)
>> +			munmap(stats_, sizeof(SwIspStats));
>> +		if (params_ != MAP_FAILED)
>> +			munmap(params_, sizeof(DebayerParams));
>> +	}
> 
> No need to inline this.

OK

>> +
>> +	int init(const IPASettings &settings,
>> +		 const SharedFD &fdStats,
>> +		 const SharedFD &fdParams,
>> +		 const ControlInfoMap &sensorInfoMap) override;
>> +	int configure(const ControlInfoMap &sensorInfoMap) override;
>> +
>> +	int start() override;
>> +	void stop() override;
>> +
>> +	void processStats(const ControlList &sensorControls) override;
>> +
>> +private:
>> +	void updateExposure(double exposureMSV);
>> +
>> +	SharedFD fdStats_;
>> +	SharedFD fdParams_;
>> +	DebayerParams *params_;
>> +	SwIspStats *stats_;
>> +
>> +	int32_t exposure_min_, exposure_max_;
>> +	int32_t again_min_, again_max_;
>> +	int32_t again_, exposure_;
>> +	unsigned int ignore_updates_;
>> +};
>> +
>> +int IPASoftSimple::init([[maybe_unused]] const IPASettings &settings,
>> +			const SharedFD &fdStats,
>> +			const SharedFD &fdParams,
>> +			const ControlInfoMap &sensorInfoMap)
>> +{
>> +	fdStats_ = fdStats;
> 
> As you never use fdStats_ and fdParams_ after this function returns,
> there's no need to store a copy.

Good catch. OK

>> +	if (!fdStats_.isValid()) {
>> +		LOG(IPASoft, Error) << "Invalid Statistics handle";
>> +		return -ENODEV;
>> +	}
>> +
>> +	fdParams_ = fdParams;
>> +	if (!fdParams_.isValid()) {
>> +		LOG(IPASoft, Error) << "Invalid Parameters handle";
>> +		return -ENODEV;
>> +	}
>> +
>> +	params_ = static_cast<DebayerParams *>(mmap(nullptr, sizeof(DebayerParams),
>> +						    PROT_WRITE, MAP_SHARED,
>> +						    fdParams_.get(), 0));
>> +	if (params_ == MAP_FAILED) {
>> +		LOG(IPASoft, Error) << "Unable to map Parameters";
>> +		return -errno;
>> +	}
> 
> 	void *mem = mmap(nullptr, sizeof(DebayerParams), PROT_WRITE, MAP_SHARED,
> 			 fdParams_.get(), 0));
> 	if (mem == MAP_FAILED) {
> 		LOG(IPASoft, Error) << "Unable to map Parameters";
> 		return -errno;
> 	}
> 
> 	params_ = static_cast<DebayerParams *>(mem);

OK

>> +
>> +	stats_ = static_cast<SwIspStats *>(mmap(nullptr, sizeof(SwIspStats),
>> +						PROT_READ, MAP_SHARED,
>> +						fdStats_.get(), 0));
>> +	if (stats_ == MAP_FAILED) {
>> +		LOG(IPASoft, Error) << "Unable to map Statistics";
>> +		return -errno;
>> +	}
>> +
>> +	if (sensorInfoMap.find(V4L2_CID_EXPOSURE) == sensorInfoMap.end()) {
>> +		LOG(IPASoft, Error) << "Don't have exposure control";
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN) == sensorInfoMap.end()) {
>> +		LOG(IPASoft, Error) << "Don't have gain control";
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +int IPASoftSimple::configure(const ControlInfoMap &sensorInfoMap)
>> +{
>> +	const ControlInfo &exposure_info = sensorInfoMap.find(V4L2_CID_EXPOSURE)->second;
> 
> s/exposure_info/exposureInfo/
> 
> I'll let you address the other snake_case to camelCase conversions in
> the series :-)

OK

>> +	const ControlInfo &gain_info = sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN)->second;
>> +
>> +	exposure_min_ = exposure_info.min().get<int32_t>();
>> +	exposure_max_ = exposure_info.max().get<int32_t>();
>> +	if (!exposure_min_) {
>> +		LOG(IPASoft, Warning) << "Minimum exposure is zero, that can't be linear";
>> +		exposure_min_ = 1;
>> +	}
>> +
>> +	again_min_ = gain_info.min().get<int32_t>();
>> +	again_max_ = gain_info.max().get<int32_t>();
> 
> Add a blank line.

OK

>> +	/*
>> +	 * The camera sensor gain (g) is usually not equal to the value written
>> +	 * into the gain register (x). But the way how the AGC algorithm changes
>> +	 * the gain value to make the total exposure closer to the optimum assumes
>> +	 * that g(x) is not too far from linear function. If the minimal gain is 0,
>> +	 * the g(x) is likely to be far from the linear, like g(x) = a / (b * x + c).
>> +	 * To avoid unexpected changes to the gain by the AGC algorithm (abrupt near
>> +	 * one edge, and very small near the other) we limit the range of the gain
>> +	 * values used.
>> +	 */
>> +	if (!again_min_) {
>> +		LOG(IPASoft, Warning) << "Minimum gain is zero, that can't be linear";
>> +		again_min_ = std::min(100, again_min_ / 2 + again_max_ / 2);
>> +	}
> 
> A patch further in the series addresses this by using
> CameraSensorHelper. It should be squashed with this patch.

OK

>> +
>> +	LOG(IPASoft, Info) << "Exposure " << exposure_min_ << "-" << exposure_max_
>> +			   << ", gain " << again_min_ << "-" << again_max_;
>> +
>> +	return 0;
>> +}
>> +
>> +int IPASoftSimple::start()
>> +{
>> +	return 0;
>> +}
>> +
>> +void IPASoftSimple::stop()
>> +{
>> +}
>> +
>> +/*
>> + * The number of bins to use for the optimal exposure calculations.
>> + */
>> +static constexpr unsigned int kExposureBinsCount = 5;
> 
> Missing blank line. Same below.

OK

>> +/*
>> + * The exposure is optimal when the mean sample value of the histogram is
>> + * in the middle of the range.
>> + */
>> +static constexpr float kExposureOptimal = kExposureBinsCount / 2.0;
>> +/*
>> + * The below value implements the hysteresis for the exposure adjustment.
>> + * It is small enough to have the exposure close to the optimal, and is big
>> + * enough to prevent the exposure from wobbling around the optimal value.
>> + */
>> +static constexpr float kExposureSatisfactory = 0.2;
> 
> Move these to the beginning of the file, just before the IPASoftSimple
> definition.

OK

>> +
>> +void IPASoftSimple::processStats(const ControlList &sensorControls)
>> +{
>> +	/*
>> +	 * Calculate red and blue gains for AWB.
>> +	 * Clamp max gain at 4.0, this also avoids 0 division.
>> +	 */
>> +	if (stats_->sumR_ <= stats_->sumG_ / 4)
>> +		params_->gainR = 1024;
>> +	else
>> +		params_->gainR = 256 * stats_->sumG_ / stats_->sumR_;
>> +
>> +	if (stats_->sumB_ <= stats_->sumG_ / 4)
>> +		params_->gainB = 1024;
>> +	else
>> +		params_->gainB = 256 * stats_->sumG_ / stats_->sumB_;
>> +
>> +	/* Green gain and gamma values are fixed */
>> +	params_->gainG = 256;
>> +	params_->gamma = 0.5;
>> +
>> +	setIspParams.emit(0);
> 
> Do you envision switching to the libipa/algorithm.h API at some point ?
> If so, it would be nice to record it somewhere.

At some point, yes.
Will mention that.

>> +
>> +	/*
>> +	 * AE / AGC, use 2 frames delay to make sure that the exposure and
>> +	 * the gain set have applied to the camera sensor.
>> +	 */
>> +	if (ignore_updates_ > 0) {
>> +		--ignore_updates_;
>> +		return;
>> +	}
> 
> Also something that could be improved and that should be recorded
> somewhere.

OK

>> +
>> +	/*
>> +	 * Calculate Mean Sample Value (MSV) according to formula from:
>> +	 * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf
>> +	 */
>> +	constexpr unsigned int yHistValsPerBin =
>> +		SwIspStats::kYHistogramSize / kExposureBinsCount;
>> +	constexpr unsigned int yHistValsPerBinMod =
>> +		SwIspStats::kYHistogramSize /
>> +		(SwIspStats::kYHistogramSize % kExposureBinsCount + 1);
>> +	int ExposureBins[kExposureBinsCount] = {};
> 
> s/ExposureBins/exposureBins/

OK

>> +	unsigned int denom = 0;
>> +	unsigned int num = 0;
>> +
>> +	for (unsigned int i = 0; i < SwIspStats::kYHistogramSize; i++) {
>> +		unsigned int idx = (i - (i / yHistValsPerBinMod)) / yHistValsPerBin;
>> +		ExposureBins[idx] += stats_->yHistogram[i];
>> +	}
>> +
>> +	for (unsigned int i = 0; i < kExposureBinsCount; i++) {
>> +		LOG(IPASoft, Debug) << i << ": " << ExposureBins[i];
>> +		denom += ExposureBins[i];
>> +		num += ExposureBins[i] * (i + 1);
>> +	}
>> +
>> +	float exposureMSV = (float)num / denom;
> 
> C++ casts.

OK

>> +
>> +	/* sanity check */
> 
> s/sanity/Sanity/

OK

>> +	if (!sensorControls.contains(V4L2_CID_EXPOSURE) ||
>> +	    !sensorControls.contains(V4L2_CID_ANALOGUE_GAIN)) {
>> +		LOG(IPASoft, Error) << "Control(s) missing";
>> +		return;
>> +	}
>> +
>> +	ControlList ctrls(sensorControls);
> 
> You shouldn't copy the control list, but instead create one from the
> ControlInfoMap that you get in init() (and that you then need to stored
> in the IPA class).

OK, thanks

>> +
>> +	exposure_ = ctrls.get(V4L2_CID_EXPOSURE).get<int32_t>();
>> +	again_ = ctrls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
> 
> 	exposure_ = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> 	again_ = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();

OK

>> +
>> +	updateExposure(exposureMSV);
>> +
>> +	ctrls.set(V4L2_CID_EXPOSURE, exposure_);
>> +	ctrls.set(V4L2_CID_ANALOGUE_GAIN, again_);
>> +
>> +	ignore_updates_ = 2;
>> +
>> +	setSensorControls.emit(ctrls);
>> +
>> +	LOG(IPASoft, Debug) << "exposureMSV " << exposureMSV
>> +			    << " exp " << exposure_ << " again " << again_
>> +			    << " gain R/B " << params_->gainR << "/" << params_->gainB;
>> +}
>> +
>> +void IPASoftSimple::updateExposure(double exposureMSV)
>> +{
>> +	/* DENOMINATOR of 10 gives ~10% increment/decrement; DENOMINATOR of 5 - about ~20% */
> 
> Line wrap, and the DENOMINATOR needs to be renamed to kExpDenominator.

OK

Thanks for the review,
Andrei

>> +	static constexpr uint8_t kExpDenominator = 10;
>> +	static constexpr uint8_t kExpNumeratorUp = kExpDenominator + 1;
>> +	static constexpr uint8_t kExpNumeratorDown = kExpDenominator - 1;
>> +
>> +	int next;
>> +
>> +	if (exposureMSV < kExposureOptimal - kExposureSatisfactory) {
>> +		next = exposure_ * kExpNumeratorUp / kExpDenominator;
>> +		if (next - exposure_ < 1)
>> +			exposure_ += 1;
>> +		else
>> +			exposure_ = next;
>> +		if (exposure_ >= exposure_max_) {
>> +			next = again_ * kExpNumeratorUp / kExpDenominator;
>> +			if (next - again_ < 1)
>> +				again_ += 1;
>> +			else
>> +				again_ = next;
>> +		}
>> +	}
>> +
>> +	if (exposureMSV > kExposureOptimal + kExposureSatisfactory) {
>> +		if (exposure_ == exposure_max_ && again_ != again_min_) {
>> +			next = again_ * kExpNumeratorDown / kExpDenominator;
>> +			if (again_ - next < 1)
>> +				again_ -= 1;
>> +			else
>> +				again_ = next;
>> +		} else {
>> +			next = exposure_ * kExpNumeratorDown / kExpDenominator;
>> +			if (exposure_ - next < 1)
>> +				exposure_ -= 1;
>> +			else
>> +				exposure_ = next;
>> +		}
>> +	}
>> +
>> +	exposure_ = std::clamp(exposure_, exposure_min_, exposure_max_);
>> +	again_ = std::clamp(again_, again_min_, again_max_);
>> +}
>> +
>> +} /* namespace ipa::soft */
>> +
>> +/*
>> + * External IPA module interface
>> + */
>> +extern "C" {
>> +const struct IPAModuleInfo ipaModuleInfo = {
>> +	IPA_MODULE_API_VERSION,
>> +	0,
>> +	"SimplePipelineHandler",
>> +	"simple",
>> +};
>> +
>> +IPAInterface *ipaCreate()
>> +{
>> +	return new ipa::soft::IPASoftSimple();
>> +}
>> +
>> +} /* extern "C" */
>> +
>> +} /* namespace libcamera */
>

Andrei Konovalov <andrey.konovalov.ynk@gmail.com> writes:

> On 27.03.2024 19:38, Laurent Pinchart wrote:

>>> +	     libcamera.ControlInfoMap sensorCtrlInfoMap)
>>> +		=> (int32 ret);
>>> +	start() => (int32 ret);
>>> +	stop();
>>> +	configure(libcamera.ControlInfoMap sensorCtrlInfoMap)
>>> +		=> (int32 ret);
>>> +
>>> +	[async] processStats(libcamera.ControlList sensorControls);
>>> +};
>>> +
>>> +interface IPASoftEventInterface {
>>> +	setSensorControls(libcamera.ControlList sensorControls);
>>> +	setIspParams(int32 dummy);
>> Drop the dummy value.
>
> libcamera docs do allow signals with zero parameters.
> But when I tried having zero parameters for an EventInterface function,
> it didn't work for me iirc.
> Let me double check.

When the parameter is not present, the following code is generated:

  void IPAProxySoft::setIspParamsIPC(
          std::vector<uint8_t>::const_iterator data,
          size_t dataSize,
          [[maybe_unused]] const std::vector<SharedFD> &fds)
  {




          setIspParams.emit();
  }

And then the compiler complains:

  src/libcamera/proxy/soft_ipa_proxy.cpp: In member function ‘void libcamera::ipa::soft::IPAProxySoft::setIspParamsIPC(std::vector<unsigned char>::const_iterator, size_t, const std::vector<libcamera::SharedFD>&)’:
  src/libcamera/proxy/soft_ipa_proxy.cpp:416:46: error: unused parameter ‘data’ [-Werror=unused-parameter]
    416 |         std::vector<uint8_t>::const_iterator data,
        |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
  src/libcamera/proxy/soft_ipa_proxy.cpp:417:16: error: unused parameter ‘dataSize’ [-Werror=unused-parameter]
    417 |         size_t dataSize,
        |         ~~~~~~~^~~~~~~~
  cc1plus: all warnings being treated as errors

Regards,
Milan

Hi Laurent and Milan,

On 28.03.2024 01:36, Andrei Konovalov wrote:
> Hi Laurent,
> 
> Thank you for the review!
> 
> On 27.03.2024 19:38, Laurent Pinchart wrote:
>> Hi Milan and Andrey,
>>
>> Thank you for the patch.
>>
>> On Tue, Mar 19, 2024 at 01:35:56PM +0100, Milan Zamazal wrote:
>>> From: Andrey Konovalov <andrey.konovalov@linaro.org>
>>>
>>> Define the Soft IPA main and event interfaces, add the Soft IPA
>>> implementation.
>>>
>>> The current src/ipa/meson.build assumes the IPA name to match the
>>> pipeline name. For this reason "-Dipas=simple" is used for the
>>> Soft IPA module.
>>
>> This should be addressed, please record this as a \todo item somewhere.
> 
> Ack
> 
>>> Auto exposure/gain and AWB implementation by Dennis, Toon and Martti.
>>>
>>> Auto exposure/gain targets a Mean Sample Value of 2.5 following
>>> the MSV calculation algorithm from:
>>> https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf
>>>
>>> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
>>> Tested-by: Pavel Machek <pavel@ucw.cz>
>>> Reviewed-by: Pavel Machek <pavel@ucw.cz>
>>> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
>>> Co-developed-by: Dennis Bonke <admin@dennisbonke.com>
>>> Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
>>> Co-developed-by: Marttico <g.martti@gmail.com>
>>> Signed-off-by: Marttico <g.martti@gmail.com>
>>> Co-developed-by: Toon Langendam <t.langendam@gmail.com>
>>> Signed-off-by: Toon Langendam <t.langendam@gmail.com>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>>   Documentation/Doxyfile.in         |   1 +
>>>   include/libcamera/ipa/meson.build |   1 +
>>>   include/libcamera/ipa/soft.mojom  |  28 +++
>>>   meson_options.txt                 |   2 +-
>>>   src/ipa/simple/data/meson.build   |   9 +
>>>   src/ipa/simple/data/soft.conf     |   3 +
>>>   src/ipa/simple/meson.build        |  25 +++
>>>   src/ipa/simple/soft_simple.cpp    | 326 ++++++++++++++++++++++++++++++
>>>   8 files changed, 394 insertions(+), 1 deletion(-)
>>>   create mode 100644 include/libcamera/ipa/soft.mojom
>>>   create mode 100644 src/ipa/simple/data/meson.build
>>>   create mode 100644 src/ipa/simple/data/soft.conf
>>>   create mode 100644 src/ipa/simple/meson.build
>>>   create mode 100644 src/ipa/simple/soft_simple.cpp
>>>
>>> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
>>> index a86ea6c1..2be8d47b 100644
>>> --- a/Documentation/Doxyfile.in
>>> +++ b/Documentation/Doxyfile.in
>>> @@ -44,6 +44,7 @@ EXCLUDE                = @TOP_SRCDIR@/include/libcamera/base/span.h \
>>>                            @TOP_SRCDIR@/src/libcamera/pipeline/ \
>>>                            @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \
>>>                            @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \
>>> +                         @TOP_BUILDDIR@/include/libcamera/ipa/soft_ipa_interface.h \
>>
>> Why is this needed ?
>>
>>>                            @TOP_BUILDDIR@/src/libcamera/proxy/
>>>   EXCLUDE_PATTERNS       = @TOP_BUILDDIR@/include/libcamera/ipa/*_serializer.h \
>>> diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build
>>> index f3b4881c..3352d08f 100644
>>> --- a/include/libcamera/ipa/meson.build
>>> +++ b/include/libcamera/ipa/meson.build
>>> @@ -65,6 +65,7 @@ pipeline_ipa_mojom_mapping = {
>>>       'ipu3': 'ipu3.mojom',
>>>       'rkisp1': 'rkisp1.mojom',
>>>       'rpi/vc4': 'raspberrypi.mojom',
>>> +    'simple': 'soft.mojom',
>>>       'vimc': 'vimc.mojom',
>>>   }
>>> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom
>>> new file mode 100644
>>> index 00000000..c249bd75
>>> --- /dev/null
>>> +++ b/include/libcamera/ipa/soft.mojom
>>> @@ -0,0 +1,28 @@
>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>> +
>>> +/*
>>> + * \todo Document the interface and remove the related EXCLUDE_PATTERNS entry.
>>
>> Ah that's why.
> 
> Yes, because, well... all the other IPAs were doing that...
> 
>> It doesn't have to be done before merging, but could you
>> address this sooner than later ?
> 
> I can't promise a lot, but I'll look into that.
> 
>>> + */
>>> +
>>> +module ipa.soft;
>>> +
>>> +import "include/libcamera/ipa/core.mojom";
>>> +
>>> +interface IPASoftInterface {
>>> +    init(libcamera.IPASettings settings,
>>> +         libcamera.SharedFD fdStats,
>>> +         libcamera.SharedFD fdParams,
>>
>> The IPA and soft ISP run in separate threads. How do you guarantee that
>> the stats and params are not accessed concurrently by both ?
> 
> I tried avoiding using a pool of buffers for stats and params, and used just a
> single chunk of shared memory to pass the stats from soft ISP (its worker thread)
> to IPA, and the other chunk for params.
> 
> The soft ISP accumulates the stats in its member variable, and after all the stats
> for the frame are calculated, the member variable is copyed to the shared memory and
> statReady signal is emitted (so using the member var implements some kind of double
> buffering). This way the write to the stats shared memory happens once per frame and
> is short in time right before emitting the statReady signal.
> The callback executed on receiving statReady signal invokes the IPA's processStats
> function. So the IPA's processStats() is called soon after the statReady is emitted.
> If the IPA finish processing the stats before the next frame is processed by the soft IPS
> and the stats for the next frame are written into the shared memory, there is no concurrent
> acess.
> 
> This doesn't strictly guarantee that the stats are not accessed concurrently by the IPA and
> soft ISP. But unless the video stream from the camera sensor overloads the system this
> scheme works OK. And if the load is high the concurrent access isn't impossible, and can harm
> the image quality, but doesn't lead to critical problems like invalid pointers as the stats
> structure only contains numbers and an array of fixed size.
> 
>> Shouldn't
>> you have a pool of buffers for each, mimicking what is done with
>> hardware ISPs ?
> 
> This would definitely work, but is a more heavy wait implementation.
> 
> On the other side, the current implementation is less universal (isn't very scalable), more fragile,
> and can't reliably associate the stats and the params with the given frame from the camera sensor.
> 
> So let me try to implement what you suggested ("a pool of buffers for each, mimicking what is done with
> hardware ISPs").

I won't be able to do this particular part by tomorrow...
So a branch I plan to publish tomorrow - with this patch updated to address the review comments
and the [PATCH v6 18/18] "libcamera: Soft IPA: use CameraSensorHelper for analogue gain" merged
into the earlier ones - would not use the pool of buffers for the stats and the params (yet).

Thanks,
Andrei

>>> +         libcamera.ControlInfoMap sensorCtrlInfoMap)
>>> +        => (int32 ret);
>>> +    start() => (int32 ret);
>>> +    stop();
>>> +    configure(libcamera.ControlInfoMap sensorCtrlInfoMap)
>>> +        => (int32 ret);
>>> +
>>> +    [async] processStats(libcamera.ControlList sensorControls);
>>> +};
>>> +
>>> +interface IPASoftEventInterface {
>>> +    setSensorControls(libcamera.ControlList sensorControls);
>>> +    setIspParams(int32 dummy);
>>
>> Drop the dummy value.
> 
> libcamera docs do allow signals with zero parameters.
> But when I tried having zero parameters for an EventInterface function,
> it didn't work for me iirc.
> Let me double check.
> 
>>> +};
>>> diff --git a/meson_options.txt b/meson_options.txt
>>> index 99dab96d..2644bef0 100644
>>> --- a/meson_options.txt
>>> +++ b/meson_options.txt
>>> @@ -27,7 +27,7 @@ option('gstreamer',
>>>   option('ipas',
>>>           type : 'array',
>>> -        choices : ['ipu3', 'rkisp1', 'rpi/vc4', 'vimc'],
>>> +        choices : ['ipu3', 'rkisp1', 'rpi/vc4', 'simple', 'vimc'],
>>>           description : 'Select which IPA modules to build')
>>>   option('lc-compliance',
>>> diff --git a/src/ipa/simple/data/meson.build b/src/ipa/simple/data/meson.build
>>> new file mode 100644
>>> index 00000000..33548cc6
>>> --- /dev/null
>>> +++ b/src/ipa/simple/data/meson.build
>>> @@ -0,0 +1,9 @@
>>> +# SPDX-License-Identifier: CC0-1.0
>>> +
>>> +conf_files = files([
>>> +    'soft.conf',
>>> +])
>>> +
>>> +install_data(conf_files,
>>> +             install_dir : ipa_data_dir / 'soft',
>>> +             install_tag : 'runtime')
>>> diff --git a/src/ipa/simple/data/soft.conf b/src/ipa/simple/data/soft.conf
>>> new file mode 100644
>>> index 00000000..0c70e7c0
>>> --- /dev/null
>>> +++ b/src/ipa/simple/data/soft.conf
>>
>> We use YAML files for all IPAs, could you do the same here ? It
>> shouldn't be much extra work as the file is empty :-)
> 
> OK
> 
>>> @@ -0,0 +1,3 @@
>>> +# SPDX-License-Identifier: LGPL-2.1-or-later
>>> +#
>>> +# Dummy configuration file for the soft IPA.
>>> diff --git a/src/ipa/simple/meson.build b/src/ipa/simple/meson.build
>>> new file mode 100644
>>> index 00000000..3e863db7
>>> --- /dev/null
>>> +++ b/src/ipa/simple/meson.build
>>> @@ -0,0 +1,25 @@
>>> +# SPDX-License-Identifier: CC0-1.0
>>> +
>>> +ipa_name = 'ipa_soft_simple'
>>> +
>>> +mod = shared_module(ipa_name,
>>> +                    ['soft_simple.cpp', libcamera_generated_ipa_headers],
>>> +                    name_prefix : '',
>>> +                    include_directories : [ipa_includes, libipa_includes],
>>> +                    dependencies : libcamera_private,
>>> +                    link_with : libipa,
>>> +                    install : true,
>>> +                    install_dir : ipa_install_dir)
>>> +
>>> +if ipa_sign_module
>>> +    custom_target(ipa_name + '.so.sign',
>>> +                  input : mod,
>>> +                  output : ipa_name + '.so.sign',
>>> +                  command : [ipa_sign, ipa_priv_key, '@INPUT@', '@OUTPUT@'],
>>> +                  install : false,
>>> +                  build_by_default : true)
>>> +endif
>>> +
>>> +subdir('data')
>>> +
>>> +ipa_names += ipa_name
>>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
>>> new file mode 100644
>>> index 00000000..312df4ba
>>> --- /dev/null
>>> +++ b/src/ipa/simple/soft_simple.cpp
>>> @@ -0,0 +1,326 @@
>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>> +/*
>>> + * Copyright (C) 2023, Linaro Ltd
>>> + *
>>> + * soft_simple.cpp - Simple Software Image Processing Algorithm module
>>> + */
>>> +
>>> +#include <sys/mman.h>
>>> +
>>> +#include <libcamera/base/file.h>
>>> +#include <libcamera/base/log.h>
>>> +#include <libcamera/base/shared_fd.h>
>>> +
>>> +#include <libcamera/control_ids.h>
>>> +#include <libcamera/controls.h>
>>> +
>>> +#include <libcamera/ipa/ipa_interface.h>
>>> +#include <libcamera/ipa/ipa_module_info.h>
>>> +#include <libcamera/ipa/soft_ipa_interface.h>
>>> +
>>> +#include "libcamera/internal/camera_sensor.h"
>>
>> Not needed.
> 
> OK
> 
>>> +#include "libcamera/internal/software_isp/debayer_params.h"
>>> +#include "libcamera/internal/software_isp/swisp_stats.h"
>>> +
>>> +namespace libcamera {
>>> +
>>> +LOG_DEFINE_CATEGORY(IPASoft)
>>> +
>>> +namespace ipa::soft {
>>> +
>>> +class IPASoftSimple : public ipa::soft::IPASoftInterface
>>> +{
>>> +public:
>>> +    IPASoftSimple()
>>> +        : params_(static_cast<DebayerParams *>(MAP_FAILED)),
>>
>> Initialize this to nullptr, that's more standard, and will make the
>> tests in the destructor nicer. init() will have to set the pointers to
>> null if mmap() calls fail.
> 
> OK
> 
>>> +          stats_(static_cast<SwIspStats *>(MAP_FAILED)), ignore_updates_(0)
>>
>> s/ignore_updates_/ignoreUpdates_/
> 
> OK
> 
>>> +    {
>>> +    }
>>> +
>>> +    ~IPASoftSimple()
>>> +    {
>>> +        if (stats_ != MAP_FAILED)
>>> +            munmap(stats_, sizeof(SwIspStats));
>>> +        if (params_ != MAP_FAILED)
>>> +            munmap(params_, sizeof(DebayerParams));
>>> +    }
>>
>> No need to inline this.
> 
> OK
> 
>>> +
>>> +    int init(const IPASettings &settings,
>>> +         const SharedFD &fdStats,
>>> +         const SharedFD &fdParams,
>>> +         const ControlInfoMap &sensorInfoMap) override;
>>> +    int configure(const ControlInfoMap &sensorInfoMap) override;
>>> +
>>> +    int start() override;
>>> +    void stop() override;
>>> +
>>> +    void processStats(const ControlList &sensorControls) override;
>>> +
>>> +private:
>>> +    void updateExposure(double exposureMSV);
>>> +
>>> +    SharedFD fdStats_;
>>> +    SharedFD fdParams_;
>>> +    DebayerParams *params_;
>>> +    SwIspStats *stats_;
>>> +
>>> +    int32_t exposure_min_, exposure_max_;
>>> +    int32_t again_min_, again_max_;
>>> +    int32_t again_, exposure_;
>>> +    unsigned int ignore_updates_;
>>> +};
>>> +
>>> +int IPASoftSimple::init([[maybe_unused]] const IPASettings &settings,
>>> +            const SharedFD &fdStats,
>>> +            const SharedFD &fdParams,
>>> +            const ControlInfoMap &sensorInfoMap)
>>> +{
>>> +    fdStats_ = fdStats;
>>
>> As you never use fdStats_ and fdParams_ after this function returns,
>> there's no need to store a copy.
> 
> Good catch. OK
> 
>>> +    if (!fdStats_.isValid()) {
>>> +        LOG(IPASoft, Error) << "Invalid Statistics handle";
>>> +        return -ENODEV;
>>> +    }
>>> +
>>> +    fdParams_ = fdParams;
>>> +    if (!fdParams_.isValid()) {
>>> +        LOG(IPASoft, Error) << "Invalid Parameters handle";
>>> +        return -ENODEV;
>>> +    }
>>> +
>>> +    params_ = static_cast<DebayerParams *>(mmap(nullptr, sizeof(DebayerParams),
>>> +                            PROT_WRITE, MAP_SHARED,
>>> +                            fdParams_.get(), 0));
>>> +    if (params_ == MAP_FAILED) {
>>> +        LOG(IPASoft, Error) << "Unable to map Parameters";
>>> +        return -errno;
>>> +    }
>>
>>     void *mem = mmap(nullptr, sizeof(DebayerParams), PROT_WRITE, MAP_SHARED,
>>              fdParams_.get(), 0));
>>     if (mem == MAP_FAILED) {
>>         LOG(IPASoft, Error) << "Unable to map Parameters";
>>         return -errno;
>>     }
>>
>>     params_ = static_cast<DebayerParams *>(mem);
> 
> OK
> 
>>> +
>>> +    stats_ = static_cast<SwIspStats *>(mmap(nullptr, sizeof(SwIspStats),
>>> +                        PROT_READ, MAP_SHARED,
>>> +                        fdStats_.get(), 0));
>>> +    if (stats_ == MAP_FAILED) {
>>> +        LOG(IPASoft, Error) << "Unable to map Statistics";
>>> +        return -errno;
>>> +    }
>>> +
>>> +    if (sensorInfoMap.find(V4L2_CID_EXPOSURE) == sensorInfoMap.end()) {
>>> +        LOG(IPASoft, Error) << "Don't have exposure control";
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    if (sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN) == sensorInfoMap.end()) {
>>> +        LOG(IPASoft, Error) << "Don't have gain control";
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +int IPASoftSimple::configure(const ControlInfoMap &sensorInfoMap)
>>> +{
>>> +    const ControlInfo &exposure_info = sensorInfoMap.find(V4L2_CID_EXPOSURE)->second;
>>
>> s/exposure_info/exposureInfo/
>>
>> I'll let you address the other snake_case to camelCase conversions in
>> the series :-)
> 
> OK
> 
>>> +    const ControlInfo &gain_info = sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN)->second;
>>> +
>>> +    exposure_min_ = exposure_info.min().get<int32_t>();
>>> +    exposure_max_ = exposure_info.max().get<int32_t>();
>>> +    if (!exposure_min_) {
>>> +        LOG(IPASoft, Warning) << "Minimum exposure is zero, that can't be linear";
>>> +        exposure_min_ = 1;
>>> +    }
>>> +
>>> +    again_min_ = gain_info.min().get<int32_t>();
>>> +    again_max_ = gain_info.max().get<int32_t>();
>>
>> Add a blank line.
> 
> OK
> 
>>> +    /*
>>> +     * The camera sensor gain (g) is usually not equal to the value written
>>> +     * into the gain register (x). But the way how the AGC algorithm changes
>>> +     * the gain value to make the total exposure closer to the optimum assumes
>>> +     * that g(x) is not too far from linear function. If the minimal gain is 0,
>>> +     * the g(x) is likely to be far from the linear, like g(x) = a / (b * x + c).
>>> +     * To avoid unexpected changes to the gain by the AGC algorithm (abrupt near
>>> +     * one edge, and very small near the other) we limit the range of the gain
>>> +     * values used.
>>> +     */
>>> +    if (!again_min_) {
>>> +        LOG(IPASoft, Warning) << "Minimum gain is zero, that can't be linear";
>>> +        again_min_ = std::min(100, again_min_ / 2 + again_max_ / 2);
>>> +    }
>>
>> A patch further in the series addresses this by using
>> CameraSensorHelper. It should be squashed with this patch.
> 
> OK
> 
>>> +
>>> +    LOG(IPASoft, Info) << "Exposure " << exposure_min_ << "-" << exposure_max_
>>> +               << ", gain " << again_min_ << "-" << again_max_;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +int IPASoftSimple::start()
>>> +{
>>> +    return 0;
>>> +}
>>> +
>>> +void IPASoftSimple::stop()
>>> +{
>>> +}
>>> +
>>> +/*
>>> + * The number of bins to use for the optimal exposure calculations.
>>> + */
>>> +static constexpr unsigned int kExposureBinsCount = 5;
>>
>> Missing blank line. Same below.
> 
> OK
> 
>>> +/*
>>> + * The exposure is optimal when the mean sample value of the histogram is
>>> + * in the middle of the range.
>>> + */
>>> +static constexpr float kExposureOptimal = kExposureBinsCount / 2.0;
>>> +/*
>>> + * The below value implements the hysteresis for the exposure adjustment.
>>> + * It is small enough to have the exposure close to the optimal, and is big
>>> + * enough to prevent the exposure from wobbling around the optimal value.
>>> + */
>>> +static constexpr float kExposureSatisfactory = 0.2;
>>
>> Move these to the beginning of the file, just before the IPASoftSimple
>> definition.
> 
> OK
> 
>>> +
>>> +void IPASoftSimple::processStats(const ControlList &sensorControls)
>>> +{
>>> +    /*
>>> +     * Calculate red and blue gains for AWB.
>>> +     * Clamp max gain at 4.0, this also avoids 0 division.
>>> +     */
>>> +    if (stats_->sumR_ <= stats_->sumG_ / 4)
>>> +        params_->gainR = 1024;
>>> +    else
>>> +        params_->gainR = 256 * stats_->sumG_ / stats_->sumR_;
>>> +
>>> +    if (stats_->sumB_ <= stats_->sumG_ / 4)
>>> +        params_->gainB = 1024;
>>> +    else
>>> +        params_->gainB = 256 * stats_->sumG_ / stats_->sumB_;
>>> +
>>> +    /* Green gain and gamma values are fixed */
>>> +    params_->gainG = 256;
>>> +    params_->gamma = 0.5;
>>> +
>>> +    setIspParams.emit(0);
>>
>> Do you envision switching to the libipa/algorithm.h API at some point ?
>> If so, it would be nice to record it somewhere.
> 
> At some point, yes.
> Will mention that.
> 
>>> +
>>> +    /*
>>> +     * AE / AGC, use 2 frames delay to make sure that the exposure and
>>> +     * the gain set have applied to the camera sensor.
>>> +     */
>>> +    if (ignore_updates_ > 0) {
>>> +        --ignore_updates_;
>>> +        return;
>>> +    }
>>
>> Also something that could be improved and that should be recorded
>> somewhere.
> 
> OK
> 
>>> +
>>> +    /*
>>> +     * Calculate Mean Sample Value (MSV) according to formula from:
>>> +     * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf
>>> +     */
>>> +    constexpr unsigned int yHistValsPerBin =
>>> +        SwIspStats::kYHistogramSize / kExposureBinsCount;
>>> +    constexpr unsigned int yHistValsPerBinMod =
>>> +        SwIspStats::kYHistogramSize /
>>> +        (SwIspStats::kYHistogramSize % kExposureBinsCount + 1);
>>> +    int ExposureBins[kExposureBinsCount] = {};
>>
>> s/ExposureBins/exposureBins/
> 
> OK
> 
>>> +    unsigned int denom = 0;
>>> +    unsigned int num = 0;
>>> +
>>> +    for (unsigned int i = 0; i < SwIspStats::kYHistogramSize; i++) {
>>> +        unsigned int idx = (i - (i / yHistValsPerBinMod)) / yHistValsPerBin;
>>> +        ExposureBins[idx] += stats_->yHistogram[i];
>>> +    }
>>> +
>>> +    for (unsigned int i = 0; i < kExposureBinsCount; i++) {
>>> +        LOG(IPASoft, Debug) << i << ": " << ExposureBins[i];
>>> +        denom += ExposureBins[i];
>>> +        num += ExposureBins[i] * (i + 1);
>>> +    }
>>> +
>>> +    float exposureMSV = (float)num / denom;
>>
>> C++ casts.
> 
> OK
> 
>>> +
>>> +    /* sanity check */
>>
>> s/sanity/Sanity/
> 
> OK
> 
>>> +    if (!sensorControls.contains(V4L2_CID_EXPOSURE) ||
>>> +        !sensorControls.contains(V4L2_CID_ANALOGUE_GAIN)) {
>>> +        LOG(IPASoft, Error) << "Control(s) missing";
>>> +        return;
>>> +    }
>>> +
>>> +    ControlList ctrls(sensorControls);
>>
>> You shouldn't copy the control list, but instead create one from the
>> ControlInfoMap that you get in init() (and that you then need to stored
>> in the IPA class).
> 
> OK, thanks
> 
>>> +
>>> +    exposure_ = ctrls.get(V4L2_CID_EXPOSURE).get<int32_t>();
>>> +    again_ = ctrls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
>>
>>     exposure_ = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
>>     again_ = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
> 
> OK
> 
>>> +
>>> +    updateExposure(exposureMSV);
>>> +
>>> +    ctrls.set(V4L2_CID_EXPOSURE, exposure_);
>>> +    ctrls.set(V4L2_CID_ANALOGUE_GAIN, again_);
>>> +
>>> +    ignore_updates_ = 2;
>>> +
>>> +    setSensorControls.emit(ctrls);
>>> +
>>> +    LOG(IPASoft, Debug) << "exposureMSV " << exposureMSV
>>> +                << " exp " << exposure_ << " again " << again_
>>> +                << " gain R/B " << params_->gainR << "/" << params_->gainB;
>>> +}
>>> +
>>> +void IPASoftSimple::updateExposure(double exposureMSV)
>>> +{
>>> +    /* DENOMINATOR of 10 gives ~10% increment/decrement; DENOMINATOR of 5 - about ~20% */
>>
>> Line wrap, and the DENOMINATOR needs to be renamed to kExpDenominator.
> 
> OK
> 
> Thanks for the review,
> Andrei
> 
>>> +    static constexpr uint8_t kExpDenominator = 10;
>>> +    static constexpr uint8_t kExpNumeratorUp = kExpDenominator + 1;
>>> +    static constexpr uint8_t kExpNumeratorDown = kExpDenominator - 1;
>>> +
>>> +    int next;
>>> +
>>> +    if (exposureMSV < kExposureOptimal - kExposureSatisfactory) {
>>> +        next = exposure_ * kExpNumeratorUp / kExpDenominator;
>>> +        if (next - exposure_ < 1)
>>> +            exposure_ += 1;
>>> +        else
>>> +            exposure_ = next;
>>> +        if (exposure_ >= exposure_max_) {
>>> +            next = again_ * kExpNumeratorUp / kExpDenominator;
>>> +            if (next - again_ < 1)
>>> +                again_ += 1;
>>> +            else
>>> +                again_ = next;
>>> +        }
>>> +    }
>>> +
>>> +    if (exposureMSV > kExposureOptimal + kExposureSatisfactory) {
>>> +        if (exposure_ == exposure_max_ && again_ != again_min_) {
>>> +            next = again_ * kExpNumeratorDown / kExpDenominator;
>>> +            if (again_ - next < 1)
>>> +                again_ -= 1;
>>> +            else
>>> +                again_ = next;
>>> +        } else {
>>> +            next = exposure_ * kExpNumeratorDown / kExpDenominator;
>>> +            if (exposure_ - next < 1)
>>> +                exposure_ -= 1;
>>> +            else
>>> +                exposure_ = next;
>>> +        }
>>> +    }
>>> +
>>> +    exposure_ = std::clamp(exposure_, exposure_min_, exposure_max_);
>>> +    again_ = std::clamp(again_, again_min_, again_max_);
>>> +}
>>> +
>>> +} /* namespace ipa::soft */
>>> +
>>> +/*
>>> + * External IPA module interface
>>> + */
>>> +extern "C" {
>>> +const struct IPAModuleInfo ipaModuleInfo = {
>>> +    IPA_MODULE_API_VERSION,
>>> +    0,
>>> +    "SimplePipelineHandler",
>>> +    "simple",
>>> +};
>>> +
>>> +IPAInterface *ipaCreate()
>>> +{
>>> +    return new ipa::soft::IPASoftSimple();
>>> +}
>>> +
>>> +} /* extern "C" */
>>> +
>>> +} /* namespace libcamera */
>>

Hi Milan,

On Tue, Apr 02, 2024 at 01:25:31PM +0200, Milan Zamazal wrote:
> Andrei Konovalov <andrey.konovalov.ynk@gmail.com> writes:
> 
> > On 27.03.2024 19:38, Laurent Pinchart wrote:
> 
> >>> +	     libcamera.ControlInfoMap sensorCtrlInfoMap)
> >>> +		=> (int32 ret);
> >>> +	start() => (int32 ret);
> >>> +	stop();
> >>> +	configure(libcamera.ControlInfoMap sensorCtrlInfoMap)
> >>> +		=> (int32 ret);
> >>> +
> >>> +	[async] processStats(libcamera.ControlList sensorControls);
> >>> +};
> >>> +
> >>> +interface IPASoftEventInterface {
> >>> +	setSensorControls(libcamera.ControlList sensorControls);
> >>> +	setIspParams(int32 dummy);
> >>
> >> Drop the dummy value.
> >
> > libcamera docs do allow signals with zero parameters.
> > But when I tried having zero parameters for an EventInterface function,
> > it didn't work for me iirc.
> > Let me double check.
> 
> When the parameter is not present, the following code is generated:
> 
>   void IPAProxySoft::setIspParamsIPC(
>           std::vector<uint8_t>::const_iterator data,
>           size_t dataSize,
>           [[maybe_unused]] const std::vector<SharedFD> &fds)
>   {
> 
> 
> 
> 
>           setIspParams.emit();
>   }
> 
> And then the compiler complains:
> 
>   src/libcamera/proxy/soft_ipa_proxy.cpp: In member function ‘void libcamera::ipa::soft::IPAProxySoft::setIspParamsIPC(std::vector<unsigned char>::const_iterator, size_t, const std::vector<libcamera::SharedFD>&)’:
>   src/libcamera/proxy/soft_ipa_proxy.cpp:416:46: error: unused parameter ‘data’ [-Werror=unused-parameter]
>     416 |         std::vector<uint8_t>::const_iterator data,
>         |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
>   src/libcamera/proxy/soft_ipa_proxy.cpp:417:16: error: unused parameter ‘dataSize’ [-Werror=unused-parameter]
>     417 |         size_t dataSize,
>         |         ~~~~~~~^~~~~~~~
>   cc1plus: all warnings being treated as errors

Ah, while we support signals without parameters, it looks like we
haven't tested them in the IPA interface. Oops.

Paul, would you be able to help Milan with this ?

Hi Andrei,

On Tue, Apr 02, 2024 at 11:34:13PM +0300, Andrei Konovalov wrote:
> On 28.03.2024 01:36, Andrei Konovalov wrote:
> > On 27.03.2024 19:38, Laurent Pinchart wrote:
> >> On Tue, Mar 19, 2024 at 01:35:56PM +0100, Milan Zamazal wrote:
> >>> From: Andrey Konovalov <andrey.konovalov@linaro.org>
> >>>
> >>> Define the Soft IPA main and event interfaces, add the Soft IPA
> >>> implementation.
> >>>
> >>> The current src/ipa/meson.build assumes the IPA name to match the
> >>> pipeline name. For this reason "-Dipas=simple" is used for the
> >>> Soft IPA module.
> >>
> >> This should be addressed, please record this as a \todo item somewhere.
> > 
> > Ack
> > 
> >>> Auto exposure/gain and AWB implementation by Dennis, Toon and Martti.
> >>>
> >>> Auto exposure/gain targets a Mean Sample Value of 2.5 following
> >>> the MSV calculation algorithm from:
> >>> https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf
> >>>
> >>> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
> >>> Tested-by: Pavel Machek <pavel@ucw.cz>
> >>> Reviewed-by: Pavel Machek <pavel@ucw.cz>
> >>> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> >>> Co-developed-by: Dennis Bonke <admin@dennisbonke.com>
> >>> Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
> >>> Co-developed-by: Marttico <g.martti@gmail.com>
> >>> Signed-off-by: Marttico <g.martti@gmail.com>
> >>> Co-developed-by: Toon Langendam <t.langendam@gmail.com>
> >>> Signed-off-by: Toon Langendam <t.langendam@gmail.com>
> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>> ---
> >>>   Documentation/Doxyfile.in         |   1 +
> >>>   include/libcamera/ipa/meson.build |   1 +
> >>>   include/libcamera/ipa/soft.mojom  |  28 +++
> >>>   meson_options.txt                 |   2 +-
> >>>   src/ipa/simple/data/meson.build   |   9 +
> >>>   src/ipa/simple/data/soft.conf     |   3 +
> >>>   src/ipa/simple/meson.build        |  25 +++
> >>>   src/ipa/simple/soft_simple.cpp    | 326 ++++++++++++++++++++++++++++++
> >>>   8 files changed, 394 insertions(+), 1 deletion(-)
> >>>   create mode 100644 include/libcamera/ipa/soft.mojom
> >>>   create mode 100644 src/ipa/simple/data/meson.build
> >>>   create mode 100644 src/ipa/simple/data/soft.conf
> >>>   create mode 100644 src/ipa/simple/meson.build
> >>>   create mode 100644 src/ipa/simple/soft_simple.cpp
> >>>
> >>> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> >>> index a86ea6c1..2be8d47b 100644
> >>> --- a/Documentation/Doxyfile.in
> >>> +++ b/Documentation/Doxyfile.in
> >>> @@ -44,6 +44,7 @@ EXCLUDE                = @TOP_SRCDIR@/include/libcamera/base/span.h \
> >>>                            @TOP_SRCDIR@/src/libcamera/pipeline/ \
> >>>                            @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \
> >>>                            @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \
> >>> +                         @TOP_BUILDDIR@/include/libcamera/ipa/soft_ipa_interface.h \
> >>
> >> Why is this needed ?
> >>
> >>>                            @TOP_BUILDDIR@/src/libcamera/proxy/
> >>>   EXCLUDE_PATTERNS       = @TOP_BUILDDIR@/include/libcamera/ipa/*_serializer.h \
> >>> diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build
> >>> index f3b4881c..3352d08f 100644
> >>> --- a/include/libcamera/ipa/meson.build
> >>> +++ b/include/libcamera/ipa/meson.build
> >>> @@ -65,6 +65,7 @@ pipeline_ipa_mojom_mapping = {
> >>>       'ipu3': 'ipu3.mojom',
> >>>       'rkisp1': 'rkisp1.mojom',
> >>>       'rpi/vc4': 'raspberrypi.mojom',
> >>> +    'simple': 'soft.mojom',
> >>>       'vimc': 'vimc.mojom',
> >>>   }
> >>> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom
> >>> new file mode 100644
> >>> index 00000000..c249bd75
> >>> --- /dev/null
> >>> +++ b/include/libcamera/ipa/soft.mojom
> >>> @@ -0,0 +1,28 @@
> >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >>> +
> >>> +/*
> >>> + * \todo Document the interface and remove the related EXCLUDE_PATTERNS entry.
> >>
> >> Ah that's why.
> > 
> > Yes, because, well... all the other IPAs were doing that...
> > 
> >> It doesn't have to be done before merging, but could you
> >> address this sooner than later ?
> > 
> > I can't promise a lot, but I'll look into that.
> > 
> >>> + */
> >>> +
> >>> +module ipa.soft;
> >>> +
> >>> +import "include/libcamera/ipa/core.mojom";
> >>> +
> >>> +interface IPASoftInterface {
> >>> +    init(libcamera.IPASettings settings,
> >>> +         libcamera.SharedFD fdStats,
> >>> +         libcamera.SharedFD fdParams,
> >>
> >> The IPA and soft ISP run in separate threads. How do you guarantee that
> >> the stats and params are not accessed concurrently by both ?
> > 
> > I tried avoiding using a pool of buffers for stats and params, and used just a
> > single chunk of shared memory to pass the stats from soft ISP (its worker thread)
> > to IPA, and the other chunk for params.
> > 
> > The soft ISP accumulates the stats in its member variable, and after all the stats
> > for the frame are calculated, the member variable is copyed to the shared memory and
> > statReady signal is emitted (so using the member var implements some kind of double
> > buffering). This way the write to the stats shared memory happens once per frame and
> > is short in time right before emitting the statReady signal.
> > The callback executed on receiving statReady signal invokes the IPA's processStats
> > function. So the IPA's processStats() is called soon after the statReady is emitted.
> > If the IPA finish processing the stats before the next frame is processed by the soft IPS
> > and the stats for the next frame are written into the shared memory, there is no concurrent
> > acess.
> > 
> > This doesn't strictly guarantee that the stats are not accessed concurrently by the IPA and
> > soft ISP. But unless the video stream from the camera sensor overloads the system this
> > scheme works OK. And if the load is high the concurrent access isn't impossible, and can harm
> > the image quality, but doesn't lead to critical problems like invalid pointers as the stats
> > structure only contains numbers and an array of fixed size.
> >
> >> Shouldn't
> >> you have a pool of buffers for each, mimicking what is done with
> >> hardware ISPs ?
> > 
> > This would definitely work, but is a more heavy wait implementation.
> > 
> > On the other side, the current implementation is less universal (isn't very scalable), more fragile,
> > and can't reliably associate the stats and the params with the given frame from the camera sensor.
> > 
> > So let me try to implement what you suggested ("a pool of buffers for each, mimicking what is done with
> > hardware ISPs").
> 
> I won't be able to do this particular part by tomorrow...
> So a branch I plan to publish tomorrow - with this patch updated to address the review comments
> and the [PATCH v6 18/18] "libcamera: Soft IPA: use CameraSensorHelper for analogue gain" merged
> into the earlier ones - would not use the pool of buffers for the stats and the params (yet).

That's fine. While I don't like relying on race windows being small, and
on hitting the race being harmless (as it's dangerous and not a very
good design), it doesn't have to be fixed to merge this series. It
should however be recorded as a TODO item in the next version, and be
addressed on top sooner than later, before adding more features.

> >>> +         libcamera.ControlInfoMap sensorCtrlInfoMap)
> >>> +        => (int32 ret);
> >>> +    start() => (int32 ret);
> >>> +    stop();
> >>> +    configure(libcamera.ControlInfoMap sensorCtrlInfoMap)
> >>> +        => (int32 ret);
> >>> +
> >>> +    [async] processStats(libcamera.ControlList sensorControls);
> >>> +};
> >>> +
> >>> +interface IPASoftEventInterface {
> >>> +    setSensorControls(libcamera.ControlList sensorControls);
> >>> +    setIspParams(int32 dummy);
> >>
> >> Drop the dummy value.
> > 
> > libcamera docs do allow signals with zero parameters.
> > But when I tried having zero parameters for an EventInterface function,
> > it didn't work for me iirc.
> > Let me double check.

Milan replied separately on this. It looks like we've never tested that,
I think Paul will be able to help.

> >>> +};
> >>> diff --git a/meson_options.txt b/meson_options.txt
> >>> index 99dab96d..2644bef0 100644
> >>> --- a/meson_options.txt
> >>> +++ b/meson_options.txt
> >>> @@ -27,7 +27,7 @@ option('gstreamer',
> >>>   option('ipas',
> >>>           type : 'array',
> >>> -        choices : ['ipu3', 'rkisp1', 'rpi/vc4', 'vimc'],
> >>> +        choices : ['ipu3', 'rkisp1', 'rpi/vc4', 'simple', 'vimc'],
> >>>           description : 'Select which IPA modules to build')
> >>>   option('lc-compliance',
> >>> diff --git a/src/ipa/simple/data/meson.build b/src/ipa/simple/data/meson.build
> >>> new file mode 100644
> >>> index 00000000..33548cc6
> >>> --- /dev/null
> >>> +++ b/src/ipa/simple/data/meson.build
> >>> @@ -0,0 +1,9 @@
> >>> +# SPDX-License-Identifier: CC0-1.0
> >>> +
> >>> +conf_files = files([
> >>> +    'soft.conf',
> >>> +])
> >>> +
> >>> +install_data(conf_files,
> >>> +             install_dir : ipa_data_dir / 'soft',
> >>> +             install_tag : 'runtime')
> >>> diff --git a/src/ipa/simple/data/soft.conf b/src/ipa/simple/data/soft.conf
> >>> new file mode 100644
> >>> index 00000000..0c70e7c0
> >>> --- /dev/null
> >>> +++ b/src/ipa/simple/data/soft.conf
> >>
> >> We use YAML files for all IPAs, could you do the same here ? It
> >> shouldn't be much extra work as the file is empty :-)
> > 
> > OK
> > 
> >>> @@ -0,0 +1,3 @@
> >>> +# SPDX-License-Identifier: LGPL-2.1-or-later
> >>> +#
> >>> +# Dummy configuration file for the soft IPA.
> >>> diff --git a/src/ipa/simple/meson.build b/src/ipa/simple/meson.build
> >>> new file mode 100644
> >>> index 00000000..3e863db7
> >>> --- /dev/null
> >>> +++ b/src/ipa/simple/meson.build
> >>> @@ -0,0 +1,25 @@
> >>> +# SPDX-License-Identifier: CC0-1.0
> >>> +
> >>> +ipa_name = 'ipa_soft_simple'
> >>> +
> >>> +mod = shared_module(ipa_name,
> >>> +                    ['soft_simple.cpp', libcamera_generated_ipa_headers],
> >>> +                    name_prefix : '',
> >>> +                    include_directories : [ipa_includes, libipa_includes],
> >>> +                    dependencies : libcamera_private,
> >>> +                    link_with : libipa,
> >>> +                    install : true,
> >>> +                    install_dir : ipa_install_dir)
> >>> +
> >>> +if ipa_sign_module
> >>> +    custom_target(ipa_name + '.so.sign',
> >>> +                  input : mod,
> >>> +                  output : ipa_name + '.so.sign',
> >>> +                  command : [ipa_sign, ipa_priv_key, '@INPUT@', '@OUTPUT@'],
> >>> +                  install : false,
> >>> +                  build_by_default : true)
> >>> +endif
> >>> +
> >>> +subdir('data')
> >>> +
> >>> +ipa_names += ipa_name
> >>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> >>> new file mode 100644
> >>> index 00000000..312df4ba
> >>> --- /dev/null
> >>> +++ b/src/ipa/simple/soft_simple.cpp
> >>> @@ -0,0 +1,326 @@
> >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >>> +/*
> >>> + * Copyright (C) 2023, Linaro Ltd
> >>> + *
> >>> + * soft_simple.cpp - Simple Software Image Processing Algorithm module
> >>> + */
> >>> +
> >>> +#include <sys/mman.h>
> >>> +
> >>> +#include <libcamera/base/file.h>
> >>> +#include <libcamera/base/log.h>
> >>> +#include <libcamera/base/shared_fd.h>
> >>> +
> >>> +#include <libcamera/control_ids.h>
> >>> +#include <libcamera/controls.h>
> >>> +
> >>> +#include <libcamera/ipa/ipa_interface.h>
> >>> +#include <libcamera/ipa/ipa_module_info.h>
> >>> +#include <libcamera/ipa/soft_ipa_interface.h>
> >>> +
> >>> +#include "libcamera/internal/camera_sensor.h"
> >>
> >> Not needed.
> > 
> > OK
> > 
> >>> +#include "libcamera/internal/software_isp/debayer_params.h"
> >>> +#include "libcamera/internal/software_isp/swisp_stats.h"
> >>> +
> >>> +namespace libcamera {
> >>> +
> >>> +LOG_DEFINE_CATEGORY(IPASoft)
> >>> +
> >>> +namespace ipa::soft {
> >>> +
> >>> +class IPASoftSimple : public ipa::soft::IPASoftInterface
> >>> +{
> >>> +public:
> >>> +    IPASoftSimple()
> >>> +        : params_(static_cast<DebayerParams *>(MAP_FAILED)),
> >>
> >> Initialize this to nullptr, that's more standard, and will make the
> >> tests in the destructor nicer. init() will have to set the pointers to
> >> null if mmap() calls fail.
> > 
> > OK
> > 
> >>> +          stats_(static_cast<SwIspStats *>(MAP_FAILED)), ignore_updates_(0)
> >>
> >> s/ignore_updates_/ignoreUpdates_/
> > 
> > OK
> > 
> >>> +    {
> >>> +    }
> >>> +
> >>> +    ~IPASoftSimple()
> >>> +    {
> >>> +        if (stats_ != MAP_FAILED)
> >>> +            munmap(stats_, sizeof(SwIspStats));
> >>> +        if (params_ != MAP_FAILED)
> >>> +            munmap(params_, sizeof(DebayerParams));
> >>> +    }
> >>
> >> No need to inline this.
> > 
> > OK
> > 
> >>> +
> >>> +    int init(const IPASettings &settings,
> >>> +         const SharedFD &fdStats,
> >>> +         const SharedFD &fdParams,
> >>> +         const ControlInfoMap &sensorInfoMap) override;
> >>> +    int configure(const ControlInfoMap &sensorInfoMap) override;
> >>> +
> >>> +    int start() override;
> >>> +    void stop() override;
> >>> +
> >>> +    void processStats(const ControlList &sensorControls) override;
> >>> +
> >>> +private:
> >>> +    void updateExposure(double exposureMSV);
> >>> +
> >>> +    SharedFD fdStats_;
> >>> +    SharedFD fdParams_;
> >>> +    DebayerParams *params_;
> >>> +    SwIspStats *stats_;
> >>> +
> >>> +    int32_t exposure_min_, exposure_max_;
> >>> +    int32_t again_min_, again_max_;
> >>> +    int32_t again_, exposure_;
> >>> +    unsigned int ignore_updates_;
> >>> +};
> >>> +
> >>> +int IPASoftSimple::init([[maybe_unused]] const IPASettings &settings,
> >>> +            const SharedFD &fdStats,
> >>> +            const SharedFD &fdParams,
> >>> +            const ControlInfoMap &sensorInfoMap)
> >>> +{
> >>> +    fdStats_ = fdStats;
> >>
> >> As you never use fdStats_ and fdParams_ after this function returns,
> >> there's no need to store a copy.
> > 
> > Good catch. OK
> > 
> >>> +    if (!fdStats_.isValid()) {
> >>> +        LOG(IPASoft, Error) << "Invalid Statistics handle";
> >>> +        return -ENODEV;
> >>> +    }
> >>> +
> >>> +    fdParams_ = fdParams;
> >>> +    if (!fdParams_.isValid()) {
> >>> +        LOG(IPASoft, Error) << "Invalid Parameters handle";
> >>> +        return -ENODEV;
> >>> +    }
> >>> +
> >>> +    params_ = static_cast<DebayerParams *>(mmap(nullptr, sizeof(DebayerParams),
> >>> +                            PROT_WRITE, MAP_SHARED,
> >>> +                            fdParams_.get(), 0));
> >>> +    if (params_ == MAP_FAILED) {
> >>> +        LOG(IPASoft, Error) << "Unable to map Parameters";
> >>> +        return -errno;
> >>> +    }
> >>
> >>     void *mem = mmap(nullptr, sizeof(DebayerParams), PROT_WRITE, MAP_SHARED,
> >>              fdParams_.get(), 0));
> >>     if (mem == MAP_FAILED) {
> >>         LOG(IPASoft, Error) << "Unable to map Parameters";
> >>         return -errno;
> >>     }
> >>
> >>     params_ = static_cast<DebayerParams *>(mem);
> > 
> > OK
> > 
> >>> +
> >>> +    stats_ = static_cast<SwIspStats *>(mmap(nullptr, sizeof(SwIspStats),
> >>> +                        PROT_READ, MAP_SHARED,
> >>> +                        fdStats_.get(), 0));
> >>> +    if (stats_ == MAP_FAILED) {
> >>> +        LOG(IPASoft, Error) << "Unable to map Statistics";
> >>> +        return -errno;
> >>> +    }
> >>> +
> >>> +    if (sensorInfoMap.find(V4L2_CID_EXPOSURE) == sensorInfoMap.end()) {
> >>> +        LOG(IPASoft, Error) << "Don't have exposure control";
> >>> +        return -EINVAL;
> >>> +    }
> >>> +
> >>> +    if (sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN) == sensorInfoMap.end()) {
> >>> +        LOG(IPASoft, Error) << "Don't have gain control";
> >>> +        return -EINVAL;
> >>> +    }
> >>> +
> >>> +    return 0;
> >>> +}
> >>> +
> >>> +int IPASoftSimple::configure(const ControlInfoMap &sensorInfoMap)
> >>> +{
> >>> +    const ControlInfo &exposure_info = sensorInfoMap.find(V4L2_CID_EXPOSURE)->second;
> >>
> >> s/exposure_info/exposureInfo/
> >>
> >> I'll let you address the other snake_case to camelCase conversions in
> >> the series :-)
> > 
> > OK
> > 
> >>> +    const ControlInfo &gain_info = sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN)->second;
> >>> +
> >>> +    exposure_min_ = exposure_info.min().get<int32_t>();
> >>> +    exposure_max_ = exposure_info.max().get<int32_t>();
> >>> +    if (!exposure_min_) {
> >>> +        LOG(IPASoft, Warning) << "Minimum exposure is zero, that can't be linear";
> >>> +        exposure_min_ = 1;
> >>> +    }
> >>> +
> >>> +    again_min_ = gain_info.min().get<int32_t>();
> >>> +    again_max_ = gain_info.max().get<int32_t>();
> >>
> >> Add a blank line.
> > 
> > OK
> > 
> >>> +    /*
> >>> +     * The camera sensor gain (g) is usually not equal to the value written
> >>> +     * into the gain register (x). But the way how the AGC algorithm changes
> >>> +     * the gain value to make the total exposure closer to the optimum assumes
> >>> +     * that g(x) is not too far from linear function. If the minimal gain is 0,
> >>> +     * the g(x) is likely to be far from the linear, like g(x) = a / (b * x + c).
> >>> +     * To avoid unexpected changes to the gain by the AGC algorithm (abrupt near
> >>> +     * one edge, and very small near the other) we limit the range of the gain
> >>> +     * values used.
> >>> +     */
> >>> +    if (!again_min_) {
> >>> +        LOG(IPASoft, Warning) << "Minimum gain is zero, that can't be linear";
> >>> +        again_min_ = std::min(100, again_min_ / 2 + again_max_ / 2);
> >>> +    }
> >>
> >> A patch further in the series addresses this by using
> >> CameraSensorHelper. It should be squashed with this patch.
> > 
> > OK
> > 
> >>> +
> >>> +    LOG(IPASoft, Info) << "Exposure " << exposure_min_ << "-" << exposure_max_
> >>> +               << ", gain " << again_min_ << "-" << again_max_;
> >>> +
> >>> +    return 0;
> >>> +}
> >>> +
> >>> +int IPASoftSimple::start()
> >>> +{
> >>> +    return 0;
> >>> +}
> >>> +
> >>> +void IPASoftSimple::stop()
> >>> +{
> >>> +}
> >>> +
> >>> +/*
> >>> + * The number of bins to use for the optimal exposure calculations.
> >>> + */
> >>> +static constexpr unsigned int kExposureBinsCount = 5;
> >>
> >> Missing blank line. Same below.
> > 
> > OK
> > 
> >>> +/*
> >>> + * The exposure is optimal when the mean sample value of the histogram is
> >>> + * in the middle of the range.
> >>> + */
> >>> +static constexpr float kExposureOptimal = kExposureBinsCount / 2.0;
> >>> +/*
> >>> + * The below value implements the hysteresis for the exposure adjustment.
> >>> + * It is small enough to have the exposure close to the optimal, and is big
> >>> + * enough to prevent the exposure from wobbling around the optimal value.
> >>> + */
> >>> +static constexpr float kExposureSatisfactory = 0.2;
> >>
> >> Move these to the beginning of the file, just before the IPASoftSimple
> >> definition.
> > 
> > OK
> > 
> >>> +
> >>> +void IPASoftSimple::processStats(const ControlList &sensorControls)
> >>> +{
> >>> +    /*
> >>> +     * Calculate red and blue gains for AWB.
> >>> +     * Clamp max gain at 4.0, this also avoids 0 division.
> >>> +     */
> >>> +    if (stats_->sumR_ <= stats_->sumG_ / 4)
> >>> +        params_->gainR = 1024;
> >>> +    else
> >>> +        params_->gainR = 256 * stats_->sumG_ / stats_->sumR_;
> >>> +
> >>> +    if (stats_->sumB_ <= stats_->sumG_ / 4)
> >>> +        params_->gainB = 1024;
> >>> +    else
> >>> +        params_->gainB = 256 * stats_->sumG_ / stats_->sumB_;
> >>> +
> >>> +    /* Green gain and gamma values are fixed */
> >>> +    params_->gainG = 256;
> >>> +    params_->gamma = 0.5;
> >>> +
> >>> +    setIspParams.emit(0);
> >>
> >> Do you envision switching to the libipa/algorithm.h API at some point ?
> >> If so, it would be nice to record it somewhere.
> > 
> > At some point, yes.
> > Will mention that.
> > 
> >>> +
> >>> +    /*
> >>> +     * AE / AGC, use 2 frames delay to make sure that the exposure and
> >>> +     * the gain set have applied to the camera sensor.
> >>> +     */
> >>> +    if (ignore_updates_ > 0) {
> >>> +        --ignore_updates_;
> >>> +        return;
> >>> +    }
> >>
> >> Also something that could be improved and that should be recorded
> >> somewhere.
> > 
> > OK
> > 
> >>> +
> >>> +    /*
> >>> +     * Calculate Mean Sample Value (MSV) according to formula from:
> >>> +     * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf
> >>> +     */
> >>> +    constexpr unsigned int yHistValsPerBin =
> >>> +        SwIspStats::kYHistogramSize / kExposureBinsCount;
> >>> +    constexpr unsigned int yHistValsPerBinMod =
> >>> +        SwIspStats::kYHistogramSize /
> >>> +        (SwIspStats::kYHistogramSize % kExposureBinsCount + 1);
> >>> +    int ExposureBins[kExposureBinsCount] = {};
> >>
> >> s/ExposureBins/exposureBins/
> > 
> > OK
> > 
> >>> +    unsigned int denom = 0;
> >>> +    unsigned int num = 0;
> >>> +
> >>> +    for (unsigned int i = 0; i < SwIspStats::kYHistogramSize; i++) {
> >>> +        unsigned int idx = (i - (i / yHistValsPerBinMod)) / yHistValsPerBin;
> >>> +        ExposureBins[idx] += stats_->yHistogram[i];
> >>> +    }
> >>> +
> >>> +    for (unsigned int i = 0; i < kExposureBinsCount; i++) {
> >>> +        LOG(IPASoft, Debug) << i << ": " << ExposureBins[i];
> >>> +        denom += ExposureBins[i];
> >>> +        num += ExposureBins[i] * (i + 1);
> >>> +    }
> >>> +
> >>> +    float exposureMSV = (float)num / denom;
> >>
> >> C++ casts.
> > 
> > OK
> > 
> >>> +
> >>> +    /* sanity check */
> >>
> >> s/sanity/Sanity/
> > 
> > OK
> > 
> >>> +    if (!sensorControls.contains(V4L2_CID_EXPOSURE) ||
> >>> +        !sensorControls.contains(V4L2_CID_ANALOGUE_GAIN)) {
> >>> +        LOG(IPASoft, Error) << "Control(s) missing";
> >>> +        return;
> >>> +    }
> >>> +
> >>> +    ControlList ctrls(sensorControls);
> >>
> >> You shouldn't copy the control list, but instead create one from the
> >> ControlInfoMap that you get in init() (and that you then need to stored
> >> in the IPA class).
> > 
> > OK, thanks
> > 
> >>> +
> >>> +    exposure_ = ctrls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> >>> +    again_ = ctrls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
> >>
> >>     exposure_ = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> >>     again_ = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
> > 
> > OK
> > 
> >>> +
> >>> +    updateExposure(exposureMSV);
> >>> +
> >>> +    ctrls.set(V4L2_CID_EXPOSURE, exposure_);
> >>> +    ctrls.set(V4L2_CID_ANALOGUE_GAIN, again_);
> >>> +
> >>> +    ignore_updates_ = 2;
> >>> +
> >>> +    setSensorControls.emit(ctrls);
> >>> +
> >>> +    LOG(IPASoft, Debug) << "exposureMSV " << exposureMSV
> >>> +                << " exp " << exposure_ << " again " << again_
> >>> +                << " gain R/B " << params_->gainR << "/" << params_->gainB;
> >>> +}
> >>> +
> >>> +void IPASoftSimple::updateExposure(double exposureMSV)
> >>> +{
> >>> +    /* DENOMINATOR of 10 gives ~10% increment/decrement; DENOMINATOR of 5 - about ~20% */
> >>
> >> Line wrap, and the DENOMINATOR needs to be renamed to kExpDenominator.
> > 
> > OK
> > 
> > Thanks for the review,
> > Andrei
> > 
> >>> +    static constexpr uint8_t kExpDenominator = 10;
> >>> +    static constexpr uint8_t kExpNumeratorUp = kExpDenominator + 1;
> >>> +    static constexpr uint8_t kExpNumeratorDown = kExpDenominator - 1;
> >>> +
> >>> +    int next;
> >>> +
> >>> +    if (exposureMSV < kExposureOptimal - kExposureSatisfactory) {
> >>> +        next = exposure_ * kExpNumeratorUp / kExpDenominator;
> >>> +        if (next - exposure_ < 1)
> >>> +            exposure_ += 1;
> >>> +        else
> >>> +            exposure_ = next;
> >>> +        if (exposure_ >= exposure_max_) {
> >>> +            next = again_ * kExpNumeratorUp / kExpDenominator;
> >>> +            if (next - again_ < 1)
> >>> +                again_ += 1;
> >>> +            else
> >>> +                again_ = next;
> >>> +        }
> >>> +    }
> >>> +
> >>> +    if (exposureMSV > kExposureOptimal + kExposureSatisfactory) {
> >>> +        if (exposure_ == exposure_max_ && again_ != again_min_) {
> >>> +            next = again_ * kExpNumeratorDown / kExpDenominator;
> >>> +            if (again_ - next < 1)
> >>> +                again_ -= 1;
> >>> +            else
> >>> +                again_ = next;
> >>> +        } else {
> >>> +            next = exposure_ * kExpNumeratorDown / kExpDenominator;
> >>> +            if (exposure_ - next < 1)
> >>> +                exposure_ -= 1;
> >>> +            else
> >>> +                exposure_ = next;
> >>> +        }
> >>> +    }
> >>> +
> >>> +    exposure_ = std::clamp(exposure_, exposure_min_, exposure_max_);
> >>> +    again_ = std::clamp(again_, again_min_, again_max_);
> >>> +}
> >>> +
> >>> +} /* namespace ipa::soft */
> >>> +
> >>> +/*
> >>> + * External IPA module interface
> >>> + */
> >>> +extern "C" {
> >>> +const struct IPAModuleInfo ipaModuleInfo = {
> >>> +    IPA_MODULE_API_VERSION,
> >>> +    0,
> >>> +    "SimplePipelineHandler",
> >>> +    "simple",
> >>> +};
> >>> +
> >>> +IPAInterface *ipaCreate()
> >>> +{
> >>> +    return new ipa::soft::IPASoftSimple();
> >>> +}
> >>> +
> >>> +} /* extern "C" */
> >>> +
> >>> +} /* namespace libcamera */

Andrei Konovalov <andrey.konovalov.ynk@gmail.com> writes:

> Hi Laurent and Milan,
>
> On 28.03.2024 01:36, Andrei Konovalov wrote:
>> Hi Laurent,
>> Thank you for the review!
>> On 27.03.2024 19:38, Laurent Pinchart wrote:
>>> Hi Milan and Andrey,
>>>
>>> Thank you for the patch.
>>>
>>> On Tue, Mar 19, 2024 at 01:35:56PM +0100, Milan Zamazal wrote:
>>>> From: Andrey Konovalov <andrey.konovalov@linaro.org>
>>>>
>>>> Define the Soft IPA main and event interfaces, add the Soft IPA
>>>> implementation.
>>>>
>>>> The current src/ipa/meson.build assumes the IPA name to match the
>>>> pipeline name. For this reason "-Dipas=simple" is used for the
>>>> Soft IPA module.
>>>
>>> This should be addressed, please record this as a \todo item somewhere.
>> Ack
>> 
>>>> Auto exposure/gain and AWB implementation by Dennis, Toon and Martti.
>>>>
>>>> Auto exposure/gain targets a Mean Sample Value of 2.5 following
>>>> the MSV calculation algorithm from:
>>>> https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf
>>>>
>>>> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
>>>> Tested-by: Pavel Machek <pavel@ucw.cz>
>>>> Reviewed-by: Pavel Machek <pavel@ucw.cz>
>>>> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
>>>> Co-developed-by: Dennis Bonke <admin@dennisbonke.com>
>>>> Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
>>>> Co-developed-by: Marttico <g.martti@gmail.com>
>>>> Signed-off-by: Marttico <g.martti@gmail.com>
>>>> Co-developed-by: Toon Langendam <t.langendam@gmail.com>
>>>> Signed-off-by: Toon Langendam <t.langendam@gmail.com>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>   Documentation/Doxyfile.in         |   1 +
>>>>   include/libcamera/ipa/meson.build |   1 +
>>>>   include/libcamera/ipa/soft.mojom  |  28 +++
>>>>   meson_options.txt                 |   2 +-
>>>>   src/ipa/simple/data/meson.build   |   9 +
>>>>   src/ipa/simple/data/soft.conf     |   3 +
>>>>   src/ipa/simple/meson.build        |  25 +++
>>>>   src/ipa/simple/soft_simple.cpp    | 326 ++++++++++++++++++++++++++++++
>>>>   8 files changed, 394 insertions(+), 1 deletion(-)
>>>>   create mode 100644 include/libcamera/ipa/soft.mojom
>>>>   create mode 100644 src/ipa/simple/data/meson.build
>>>>   create mode 100644 src/ipa/simple/data/soft.conf
>>>>   create mode 100644 src/ipa/simple/meson.build
>>>>   create mode 100644 src/ipa/simple/soft_simple.cpp
>>>>
>>>> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
>>>> index a86ea6c1..2be8d47b 100644
>>>> --- a/Documentation/Doxyfile.in
>>>> +++ b/Documentation/Doxyfile.in
>>>> @@ -44,6 +44,7 @@ EXCLUDE                = @TOP_SRCDIR@/include/libcamera/base/span.h \
>>>>                            @TOP_SRCDIR@/src/libcamera/pipeline/ \
>>>>                            @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \
>>>>                            @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \
>>>> +                         @TOP_BUILDDIR@/include/libcamera/ipa/soft_ipa_interface.h \
>>>
>>> Why is this needed ?
>>>
>>>>                            @TOP_BUILDDIR@/src/libcamera/proxy/
>>>>   EXCLUDE_PATTERNS       = @TOP_BUILDDIR@/include/libcamera/ipa/*_serializer.h \
>>>> diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build
>>>> index f3b4881c..3352d08f 100644
>>>> --- a/include/libcamera/ipa/meson.build
>>>> +++ b/include/libcamera/ipa/meson.build
>>>> @@ -65,6 +65,7 @@ pipeline_ipa_mojom_mapping = {
>>>>       'ipu3': 'ipu3.mojom',
>>>>       'rkisp1': 'rkisp1.mojom',
>>>>       'rpi/vc4': 'raspberrypi.mojom',
>>>> +    'simple': 'soft.mojom',
>>>>       'vimc': 'vimc.mojom',
>>>>   }
>>>> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom
>>>> new file mode 100644
>>>> index 00000000..c249bd75
>>>> --- /dev/null
>>>> +++ b/include/libcamera/ipa/soft.mojom
>>>> @@ -0,0 +1,28 @@
>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>>> +
>>>> +/*
>>>> + * \todo Document the interface and remove the related EXCLUDE_PATTERNS entry.
>>>
>>> Ah that's why.
>> Yes, because, well... all the other IPAs were doing that...
>> 
>>> It doesn't have to be done before merging, but could you
>>> address this sooner than later ?
>> I can't promise a lot, but I'll look into that.
>> 
>>>> + */
>>>> +
>>>> +module ipa.soft;
>>>> +
>>>> +import "include/libcamera/ipa/core.mojom";
>>>> +
>>>> +interface IPASoftInterface {
>>>> +    init(libcamera.IPASettings settings,
>>>> +         libcamera.SharedFD fdStats,
>>>> +         libcamera.SharedFD fdParams,
>>>
>>> The IPA and soft ISP run in separate threads. How do you guarantee that
>>> the stats and params are not accessed concurrently by both ?
>> I tried avoiding using a pool of buffers for stats and params, and used just a
>> single chunk of shared memory to pass the stats from soft ISP (its worker thread)
>> to IPA, and the other chunk for params.
>> The soft ISP accumulates the stats in its member variable, and after all the stats
>> for the frame are calculated, the member variable is copyed to the shared memory and
>> statReady signal is emitted (so using the member var implements some kind of double
>> buffering). This way the write to the stats shared memory happens once per frame and
>> is short in time right before emitting the statReady signal.
>> The callback executed on receiving statReady signal invokes the IPA's processStats
>> function. So the IPA's processStats() is called soon after the statReady is emitted.
>> If the IPA finish processing the stats before the next frame is processed by the soft IPS
>> and the stats for the next frame are written into the shared memory, there is no concurrent
>> acess.
>> This doesn't strictly guarantee that the stats are not accessed concurrently by the IPA and
>> soft ISP. But unless the video stream from the camera sensor overloads the system this
>> scheme works OK. And if the load is high the concurrent access isn't impossible, and can harm
>> the image quality, but doesn't lead to critical problems like invalid pointers as the stats
>> structure only contains numbers and an array of fixed size.
>> 
>>> Shouldn't
>>> you have a pool of buffers for each, mimicking what is done with
>>> hardware ISPs ?
>> This would definitely work, but is a more heavy wait implementation.
>> On the other side, the current implementation is less universal (isn't very scalable), more fragile,
>> and can't reliably associate the stats and the params with the given frame from the camera sensor.
>> So let me try to implement what you suggested ("a pool of buffers for each, mimicking what is done with
>> hardware ISPs").
>
> I won't be able to do this particular part by tomorrow...
> So a branch I plan to publish tomorrow - with this patch updated to address the review comments
> and the [PATCH v6 18/18] "libcamera: Soft IPA: use CameraSensorHelper for analogue gain" merged
> into the earlier ones - would not use the pool of buffers for the stats and the params (yet).

Hi Andrei,

I already made those easier parts, see
https://gitlab.freedesktop.org/mzamazal/libcamera-softisp/-/commits/SoftwareISP-v10?ref_type=heads
(not ready for review yet).

Sorry for misunderstanding and duplicate work.

Regards,
Milan

> Thanks,
> Andrei
>
>>>> +         libcamera.ControlInfoMap sensorCtrlInfoMap)
>>>> +        => (int32 ret);
>>>> +    start() => (int32 ret);
>>>> +    stop();
>>>> +    configure(libcamera.ControlInfoMap sensorCtrlInfoMap)
>>>> +        => (int32 ret);
>>>> +
>>>> +    [async] processStats(libcamera.ControlList sensorControls);
>>>> +};
>>>> +
>>>> +interface IPASoftEventInterface {
>>>> +    setSensorControls(libcamera.ControlList sensorControls);
>>>> +    setIspParams(int32 dummy);
>>>
>>> Drop the dummy value.
>> libcamera docs do allow signals with zero parameters.
>> But when I tried having zero parameters for an EventInterface function,
>> it didn't work for me iirc.
>> Let me double check.
>> 
>>>> +};
>>>> diff --git a/meson_options.txt b/meson_options.txt
>>>> index 99dab96d..2644bef0 100644
>>>> --- a/meson_options.txt
>>>> +++ b/meson_options.txt
>>>> @@ -27,7 +27,7 @@ option('gstreamer',
>>>>   option('ipas',
>>>>           type : 'array',
>>>> -        choices : ['ipu3', 'rkisp1', 'rpi/vc4', 'vimc'],
>>>> +        choices : ['ipu3', 'rkisp1', 'rpi/vc4', 'simple', 'vimc'],
>>>>           description : 'Select which IPA modules to build')
>>>>   option('lc-compliance',
>>>> diff --git a/src/ipa/simple/data/meson.build b/src/ipa/simple/data/meson.build
>>>> new file mode 100644
>>>> index 00000000..33548cc6
>>>> --- /dev/null
>>>> +++ b/src/ipa/simple/data/meson.build
>>>> @@ -0,0 +1,9 @@
>>>> +# SPDX-License-Identifier: CC0-1.0
>>>> +
>>>> +conf_files = files([
>>>> +    'soft.conf',
>>>> +])
>>>> +
>>>> +install_data(conf_files,
>>>> +             install_dir : ipa_data_dir / 'soft',
>>>> +             install_tag : 'runtime')
>>>> diff --git a/src/ipa/simple/data/soft.conf b/src/ipa/simple/data/soft.conf
>>>> new file mode 100644
>>>> index 00000000..0c70e7c0
>>>> --- /dev/null
>>>> +++ b/src/ipa/simple/data/soft.conf
>>>
>>> We use YAML files for all IPAs, could you do the same here ? It
>>> shouldn't be much extra work as the file is empty :-)
>> OK
>> 
>>>> @@ -0,0 +1,3 @@
>>>> +# SPDX-License-Identifier: LGPL-2.1-or-later
>>>> +#
>>>> +# Dummy configuration file for the soft IPA.
>>>> diff --git a/src/ipa/simple/meson.build b/src/ipa/simple/meson.build
>>>> new file mode 100644
>>>> index 00000000..3e863db7
>>>> --- /dev/null
>>>> +++ b/src/ipa/simple/meson.build
>>>> @@ -0,0 +1,25 @@
>>>> +# SPDX-License-Identifier: CC0-1.0
>>>> +
>>>> +ipa_name = 'ipa_soft_simple'
>>>> +
>>>> +mod = shared_module(ipa_name,
>>>> +                    ['soft_simple.cpp', libcamera_generated_ipa_headers],
>>>> +                    name_prefix : '',
>>>> +                    include_directories : [ipa_includes, libipa_includes],
>>>> +                    dependencies : libcamera_private,
>>>> +                    link_with : libipa,
>>>> +                    install : true,
>>>> +                    install_dir : ipa_install_dir)
>>>> +
>>>> +if ipa_sign_module
>>>> +    custom_target(ipa_name + '.so.sign',
>>>> +                  input : mod,
>>>> +                  output : ipa_name + '.so.sign',
>>>> +                  command : [ipa_sign, ipa_priv_key, '@INPUT@', '@OUTPUT@'],
>>>> +                  install : false,
>>>> +                  build_by_default : true)
>>>> +endif
>>>> +
>>>> +subdir('data')
>>>> +
>>>> +ipa_names += ipa_name
>>>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
>>>> new file mode 100644
>>>> index 00000000..312df4ba
>>>> --- /dev/null
>>>> +++ b/src/ipa/simple/soft_simple.cpp
>>>> @@ -0,0 +1,326 @@
>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>>> +/*
>>>> + * Copyright (C) 2023, Linaro Ltd
>>>> + *
>>>> + * soft_simple.cpp - Simple Software Image Processing Algorithm module
>>>> + */
>>>> +
>>>> +#include <sys/mman.h>
>>>> +
>>>> +#include <libcamera/base/file.h>
>>>> +#include <libcamera/base/log.h>
>>>> +#include <libcamera/base/shared_fd.h>
>>>> +
>>>> +#include <libcamera/control_ids.h>
>>>> +#include <libcamera/controls.h>
>>>> +
>>>> +#include <libcamera/ipa/ipa_interface.h>
>>>> +#include <libcamera/ipa/ipa_module_info.h>
>>>> +#include <libcamera/ipa/soft_ipa_interface.h>
>>>> +
>>>> +#include "libcamera/internal/camera_sensor.h"
>>>
>>> Not needed.
>> OK
>> 
>>>> +#include "libcamera/internal/software_isp/debayer_params.h"
>>>> +#include "libcamera/internal/software_isp/swisp_stats.h"
>>>> +
>>>> +namespace libcamera {
>>>> +
>>>> +LOG_DEFINE_CATEGORY(IPASoft)
>>>> +
>>>> +namespace ipa::soft {
>>>> +
>>>> +class IPASoftSimple : public ipa::soft::IPASoftInterface
>>>> +{
>>>> +public:
>>>> +    IPASoftSimple()
>>>> +        : params_(static_cast<DebayerParams *>(MAP_FAILED)),
>>>
>>> Initialize this to nullptr, that's more standard, and will make the
>>> tests in the destructor nicer. init() will have to set the pointers to
>>> null if mmap() calls fail.
>> OK
>> 
>>>> +          stats_(static_cast<SwIspStats *>(MAP_FAILED)), ignore_updates_(0)
>>>
>>> s/ignore_updates_/ignoreUpdates_/
>> OK
>> 
>>>> +    {
>>>> +    }
>>>> +
>>>> +    ~IPASoftSimple()
>>>> +    {
>>>> +        if (stats_ != MAP_FAILED)
>>>> +            munmap(stats_, sizeof(SwIspStats));
>>>> +        if (params_ != MAP_FAILED)
>>>> +            munmap(params_, sizeof(DebayerParams));
>>>> +    }
>>>
>>> No need to inline this.
>> OK
>> 
>>>> +
>>>> +    int init(const IPASettings &settings,
>>>> +         const SharedFD &fdStats,
>>>> +         const SharedFD &fdParams,
>>>> +         const ControlInfoMap &sensorInfoMap) override;
>>>> +    int configure(const ControlInfoMap &sensorInfoMap) override;
>>>> +
>>>> +    int start() override;
>>>> +    void stop() override;
>>>> +
>>>> +    void processStats(const ControlList &sensorControls) override;
>>>> +
>>>> +private:
>>>> +    void updateExposure(double exposureMSV);
>>>> +
>>>> +    SharedFD fdStats_;
>>>> +    SharedFD fdParams_;
>>>> +    DebayerParams *params_;
>>>> +    SwIspStats *stats_;
>>>> +
>>>> +    int32_t exposure_min_, exposure_max_;
>>>> +    int32_t again_min_, again_max_;
>>>> +    int32_t again_, exposure_;
>>>> +    unsigned int ignore_updates_;
>>>> +};
>>>> +
>>>> +int IPASoftSimple::init([[maybe_unused]] const IPASettings &settings,
>>>> +            const SharedFD &fdStats,
>>>> +            const SharedFD &fdParams,
>>>> +            const ControlInfoMap &sensorInfoMap)
>>>> +{
>>>> +    fdStats_ = fdStats;
>>>
>>> As you never use fdStats_ and fdParams_ after this function returns,
>>> there's no need to store a copy.
>> Good catch. OK
>> 
>>>> +    if (!fdStats_.isValid()) {
>>>> +        LOG(IPASoft, Error) << "Invalid Statistics handle";
>>>> +        return -ENODEV;
>>>> +    }
>>>> +
>>>> +    fdParams_ = fdParams;
>>>> +    if (!fdParams_.isValid()) {
>>>> +        LOG(IPASoft, Error) << "Invalid Parameters handle";
>>>> +        return -ENODEV;
>>>> +    }
>>>> +
>>>> +    params_ = static_cast<DebayerParams *>(mmap(nullptr, sizeof(DebayerParams),
>>>> +                            PROT_WRITE, MAP_SHARED,
>>>> +                            fdParams_.get(), 0));
>>>> +    if (params_ == MAP_FAILED) {
>>>> +        LOG(IPASoft, Error) << "Unable to map Parameters";
>>>> +        return -errno;
>>>> +    }
>>>
>>>     void *mem = mmap(nullptr, sizeof(DebayerParams), PROT_WRITE, MAP_SHARED,
>>>              fdParams_.get(), 0));
>>>     if (mem == MAP_FAILED) {
>>>         LOG(IPASoft, Error) << "Unable to map Parameters";
>>>         return -errno;
>>>     }
>>>
>>>     params_ = static_cast<DebayerParams *>(mem);
>> OK
>> 
>>>> +
>>>> +    stats_ = static_cast<SwIspStats *>(mmap(nullptr, sizeof(SwIspStats),
>>>> +                        PROT_READ, MAP_SHARED,
>>>> +                        fdStats_.get(), 0));
>>>> +    if (stats_ == MAP_FAILED) {
>>>> +        LOG(IPASoft, Error) << "Unable to map Statistics";
>>>> +        return -errno;
>>>> +    }
>>>> +
>>>> +    if (sensorInfoMap.find(V4L2_CID_EXPOSURE) == sensorInfoMap.end()) {
>>>> +        LOG(IPASoft, Error) << "Don't have exposure control";
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    if (sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN) == sensorInfoMap.end()) {
>>>> +        LOG(IPASoft, Error) << "Don't have gain control";
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +int IPASoftSimple::configure(const ControlInfoMap &sensorInfoMap)
>>>> +{
>>>> +    const ControlInfo &exposure_info = sensorInfoMap.find(V4L2_CID_EXPOSURE)->second;
>>>
>>> s/exposure_info/exposureInfo/
>>>
>>> I'll let you address the other snake_case to camelCase conversions in
>>> the series :-)
>> OK
>> 
>>>> +    const ControlInfo &gain_info = sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN)->second;
>>>> +
>>>> +    exposure_min_ = exposure_info.min().get<int32_t>();
>>>> +    exposure_max_ = exposure_info.max().get<int32_t>();
>>>> +    if (!exposure_min_) {
>>>> +        LOG(IPASoft, Warning) << "Minimum exposure is zero, that can't be linear";
>>>> +        exposure_min_ = 1;
>>>> +    }
>>>> +
>>>> +    again_min_ = gain_info.min().get<int32_t>();
>>>> +    again_max_ = gain_info.max().get<int32_t>();
>>>
>>> Add a blank line.
>> OK
>> 
>>>> +    /*
>>>> +     * The camera sensor gain (g) is usually not equal to the value written
>>>> +     * into the gain register (x). But the way how the AGC algorithm changes
>>>> +     * the gain value to make the total exposure closer to the optimum assumes
>>>> +     * that g(x) is not too far from linear function. If the minimal gain is 0,
>>>> +     * the g(x) is likely to be far from the linear, like g(x) = a / (b * x + c).
>>>> +     * To avoid unexpected changes to the gain by the AGC algorithm (abrupt near
>>>> +     * one edge, and very small near the other) we limit the range of the gain
>>>> +     * values used.
>>>> +     */
>>>> +    if (!again_min_) {
>>>> +        LOG(IPASoft, Warning) << "Minimum gain is zero, that can't be linear";
>>>> +        again_min_ = std::min(100, again_min_ / 2 + again_max_ / 2);
>>>> +    }
>>>
>>> A patch further in the series addresses this by using
>>> CameraSensorHelper. It should be squashed with this patch.
>> OK
>> 
>>>> +
>>>> +    LOG(IPASoft, Info) << "Exposure " << exposure_min_ << "-" << exposure_max_
>>>> +               << ", gain " << again_min_ << "-" << again_max_;
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +int IPASoftSimple::start()
>>>> +{
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +void IPASoftSimple::stop()
>>>> +{
>>>> +}
>>>> +
>>>> +/*
>>>> + * The number of bins to use for the optimal exposure calculations.
>>>> + */
>>>> +static constexpr unsigned int kExposureBinsCount = 5;
>>>
>>> Missing blank line. Same below.
>> OK
>> 
>>>> +/*
>>>> + * The exposure is optimal when the mean sample value of the histogram is
>>>> + * in the middle of the range.
>>>> + */
>>>> +static constexpr float kExposureOptimal = kExposureBinsCount / 2.0;
>>>> +/*
>>>> + * The below value implements the hysteresis for the exposure adjustment.
>>>> + * It is small enough to have the exposure close to the optimal, and is big
>>>> + * enough to prevent the exposure from wobbling around the optimal value.
>>>> + */
>>>> +static constexpr float kExposureSatisfactory = 0.2;
>>>
>>> Move these to the beginning of the file, just before the IPASoftSimple
>>> definition.
>> OK
>> 
>>>> +
>>>> +void IPASoftSimple::processStats(const ControlList &sensorControls)
>>>> +{
>>>> +    /*
>>>> +     * Calculate red and blue gains for AWB.
>>>> +     * Clamp max gain at 4.0, this also avoids 0 division.
>>>> +     */
>>>> +    if (stats_->sumR_ <= stats_->sumG_ / 4)
>>>> +        params_->gainR = 1024;
>>>> +    else
>>>> +        params_->gainR = 256 * stats_->sumG_ / stats_->sumR_;
>>>> +
>>>> +    if (stats_->sumB_ <= stats_->sumG_ / 4)
>>>> +        params_->gainB = 1024;
>>>> +    else
>>>> +        params_->gainB = 256 * stats_->sumG_ / stats_->sumB_;
>>>> +
>>>> +    /* Green gain and gamma values are fixed */
>>>> +    params_->gainG = 256;
>>>> +    params_->gamma = 0.5;
>>>> +
>>>> +    setIspParams.emit(0);
>>>
>>> Do you envision switching to the libipa/algorithm.h API at some point ?
>>> If so, it would be nice to record it somewhere.
>> At some point, yes.
>> Will mention that.
>> 
>>>> +
>>>> +    /*
>>>> +     * AE / AGC, use 2 frames delay to make sure that the exposure and
>>>> +     * the gain set have applied to the camera sensor.
>>>> +     */
>>>> +    if (ignore_updates_ > 0) {
>>>> +        --ignore_updates_;
>>>> +        return;
>>>> +    }
>>>
>>> Also something that could be improved and that should be recorded
>>> somewhere.
>> OK
>> 
>>>> +
>>>> +    /*
>>>> +     * Calculate Mean Sample Value (MSV) according to formula from:
>>>> +     * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf
>>>> +     */
>>>> +    constexpr unsigned int yHistValsPerBin =
>>>> +        SwIspStats::kYHistogramSize / kExposureBinsCount;
>>>> +    constexpr unsigned int yHistValsPerBinMod =
>>>> +        SwIspStats::kYHistogramSize /
>>>> +        (SwIspStats::kYHistogramSize % kExposureBinsCount + 1);
>>>> +    int ExposureBins[kExposureBinsCount] = {};
>>>
>>> s/ExposureBins/exposureBins/
>> OK
>> 
>>>> +    unsigned int denom = 0;
>>>> +    unsigned int num = 0;
>>>> +
>>>> +    for (unsigned int i = 0; i < SwIspStats::kYHistogramSize; i++) {
>>>> +        unsigned int idx = (i - (i / yHistValsPerBinMod)) / yHistValsPerBin;
>>>> +        ExposureBins[idx] += stats_->yHistogram[i];
>>>> +    }
>>>> +
>>>> +    for (unsigned int i = 0; i < kExposureBinsCount; i++) {
>>>> +        LOG(IPASoft, Debug) << i << ": " << ExposureBins[i];
>>>> +        denom += ExposureBins[i];
>>>> +        num += ExposureBins[i] * (i + 1);
>>>> +    }
>>>> +
>>>> +    float exposureMSV = (float)num / denom;
>>>
>>> C++ casts.
>> OK
>> 
>>>> +
>>>> +    /* sanity check */
>>>
>>> s/sanity/Sanity/
>> OK
>> 
>>>> +    if (!sensorControls.contains(V4L2_CID_EXPOSURE) ||
>>>> +        !sensorControls.contains(V4L2_CID_ANALOGUE_GAIN)) {
>>>> +        LOG(IPASoft, Error) << "Control(s) missing";
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    ControlList ctrls(sensorControls);
>>>
>>> You shouldn't copy the control list, but instead create one from the
>>> ControlInfoMap that you get in init() (and that you then need to stored
>>> in the IPA class).
>> OK, thanks
>> 
>>>> +
>>>> +    exposure_ = ctrls.get(V4L2_CID_EXPOSURE).get<int32_t>();
>>>> +    again_ = ctrls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
>>>
>>>     exposure_ = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
>>>     again_ = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
>> OK
>> 
>>>> +
>>>> +    updateExposure(exposureMSV);
>>>> +
>>>> +    ctrls.set(V4L2_CID_EXPOSURE, exposure_);
>>>> +    ctrls.set(V4L2_CID_ANALOGUE_GAIN, again_);
>>>> +
>>>> +    ignore_updates_ = 2;
>>>> +
>>>> +    setSensorControls.emit(ctrls);
>>>> +
>>>> +    LOG(IPASoft, Debug) << "exposureMSV " << exposureMSV
>>>> +                << " exp " << exposure_ << " again " << again_
>>>> +                << " gain R/B " << params_->gainR << "/" << params_->gainB;
>>>> +}
>>>> +
>>>> +void IPASoftSimple::updateExposure(double exposureMSV)
>>>> +{
>>>> +    /* DENOMINATOR of 10 gives ~10% increment/decrement; DENOMINATOR of 5 - about ~20% */
>>>
>>> Line wrap, and the DENOMINATOR needs to be renamed to kExpDenominator.
>> OK
>> Thanks for the review,
>> Andrei
>> 
>>>> +    static constexpr uint8_t kExpDenominator = 10;
>>>> +    static constexpr uint8_t kExpNumeratorUp = kExpDenominator + 1;
>>>> +    static constexpr uint8_t kExpNumeratorDown = kExpDenominator - 1;
>>>> +
>>>> +    int next;
>>>> +
>>>> +    if (exposureMSV < kExposureOptimal - kExposureSatisfactory) {
>>>> +        next = exposure_ * kExpNumeratorUp / kExpDenominator;
>>>> +        if (next - exposure_ < 1)
>>>> +            exposure_ += 1;
>>>> +        else
>>>> +            exposure_ = next;
>>>> +        if (exposure_ >= exposure_max_) {
>>>> +            next = again_ * kExpNumeratorUp / kExpDenominator;
>>>> +            if (next - again_ < 1)
>>>> +                again_ += 1;
>>>> +            else
>>>> +                again_ = next;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    if (exposureMSV > kExposureOptimal + kExposureSatisfactory) {
>>>> +        if (exposure_ == exposure_max_ && again_ != again_min_) {
>>>> +            next = again_ * kExpNumeratorDown / kExpDenominator;
>>>> +            if (again_ - next < 1)
>>>> +                again_ -= 1;
>>>> +            else
>>>> +                again_ = next;
>>>> +        } else {
>>>> +            next = exposure_ * kExpNumeratorDown / kExpDenominator;
>>>> +            if (exposure_ - next < 1)
>>>> +                exposure_ -= 1;
>>>> +            else
>>>> +                exposure_ = next;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    exposure_ = std::clamp(exposure_, exposure_min_, exposure_max_);
>>>> +    again_ = std::clamp(again_, again_min_, again_max_);
>>>> +}
>>>> +
>>>> +} /* namespace ipa::soft */
>>>> +
>>>> +/*
>>>> + * External IPA module interface
>>>> + */
>>>> +extern "C" {
>>>> +const struct IPAModuleInfo ipaModuleInfo = {
>>>> +    IPA_MODULE_API_VERSION,
>>>> +    0,
>>>> +    "SimplePipelineHandler",
>>>> +    "simple",
>>>> +};
>>>> +
>>>> +IPAInterface *ipaCreate()
>>>> +{
>>>> +    return new ipa::soft::IPASoftSimple();
>>>> +}
>>>> +
>>>> +} /* extern "C" */
>>>> +
>>>> +} /* namespace libcamera */
>>>

Hi Milan,

On 03.04.2024 11:53, Milan Zamazal wrote:
> Andrei Konovalov <andrey.konovalov.ynk@gmail.com> writes:
> 
>> Hi Laurent and Milan,
>>
>> On 28.03.2024 01:36, Andrei Konovalov wrote:
>>> Hi Laurent,
>>> Thank you for the review!
>>> On 27.03.2024 19:38, Laurent Pinchart wrote:
>>>> Hi Milan and Andrey,
>>>>
>>>> Thank you for the patch.
>>>>
>>>> On Tue, Mar 19, 2024 at 01:35:56PM +0100, Milan Zamazal wrote:
>>>>> From: Andrey Konovalov <andrey.konovalov@linaro.org>
>>>>>
>>>>> Define the Soft IPA main and event interfaces, add the Soft IPA
>>>>> implementation.
>>>>>
>>>>> The current src/ipa/meson.build assumes the IPA name to match the
>>>>> pipeline name. For this reason "-Dipas=simple" is used for the
>>>>> Soft IPA module.
>>>>
>>>> This should be addressed, please record this as a \todo item somewhere.
>>> Ack
>>>
>>>>> Auto exposure/gain and AWB implementation by Dennis, Toon and Martti.
>>>>>
>>>>> Auto exposure/gain targets a Mean Sample Value of 2.5 following
>>>>> the MSV calculation algorithm from:
>>>>> https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf
>>>>>
>>>>> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
>>>>> Tested-by: Pavel Machek <pavel@ucw.cz>
>>>>> Reviewed-by: Pavel Machek <pavel@ucw.cz>
>>>>> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
>>>>> Co-developed-by: Dennis Bonke <admin@dennisbonke.com>
>>>>> Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
>>>>> Co-developed-by: Marttico <g.martti@gmail.com>
>>>>> Signed-off-by: Marttico <g.martti@gmail.com>
>>>>> Co-developed-by: Toon Langendam <t.langendam@gmail.com>
>>>>> Signed-off-by: Toon Langendam <t.langendam@gmail.com>
>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>> ---
>>>>>    Documentation/Doxyfile.in         |   1 +
>>>>>    include/libcamera/ipa/meson.build |   1 +
>>>>>    include/libcamera/ipa/soft.mojom  |  28 +++
>>>>>    meson_options.txt                 |   2 +-
>>>>>    src/ipa/simple/data/meson.build   |   9 +
>>>>>    src/ipa/simple/data/soft.conf     |   3 +
>>>>>    src/ipa/simple/meson.build        |  25 +++
>>>>>    src/ipa/simple/soft_simple.cpp    | 326 ++++++++++++++++++++++++++++++
>>>>>    8 files changed, 394 insertions(+), 1 deletion(-)
>>>>>    create mode 100644 include/libcamera/ipa/soft.mojom
>>>>>    create mode 100644 src/ipa/simple/data/meson.build
>>>>>    create mode 100644 src/ipa/simple/data/soft.conf
>>>>>    create mode 100644 src/ipa/simple/meson.build
>>>>>    create mode 100644 src/ipa/simple/soft_simple.cpp
>>>>>
>>>>> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
>>>>> index a86ea6c1..2be8d47b 100644
>>>>> --- a/Documentation/Doxyfile.in
>>>>> +++ b/Documentation/Doxyfile.in
>>>>> @@ -44,6 +44,7 @@ EXCLUDE                = @TOP_SRCDIR@/include/libcamera/base/span.h \
>>>>>                             @TOP_SRCDIR@/src/libcamera/pipeline/ \
>>>>>                             @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \
>>>>>                             @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \
>>>>> +                         @TOP_BUILDDIR@/include/libcamera/ipa/soft_ipa_interface.h \
>>>>
>>>> Why is this needed ?
>>>>
>>>>>                             @TOP_BUILDDIR@/src/libcamera/proxy/
>>>>>    EXCLUDE_PATTERNS       = @TOP_BUILDDIR@/include/libcamera/ipa/*_serializer.h \
>>>>> diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build
>>>>> index f3b4881c..3352d08f 100644
>>>>> --- a/include/libcamera/ipa/meson.build
>>>>> +++ b/include/libcamera/ipa/meson.build
>>>>> @@ -65,6 +65,7 @@ pipeline_ipa_mojom_mapping = {
>>>>>        'ipu3': 'ipu3.mojom',
>>>>>        'rkisp1': 'rkisp1.mojom',
>>>>>        'rpi/vc4': 'raspberrypi.mojom',
>>>>> +    'simple': 'soft.mojom',
>>>>>        'vimc': 'vimc.mojom',
>>>>>    }
>>>>> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom
>>>>> new file mode 100644
>>>>> index 00000000..c249bd75
>>>>> --- /dev/null
>>>>> +++ b/include/libcamera/ipa/soft.mojom
>>>>> @@ -0,0 +1,28 @@
>>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>>>> +
>>>>> +/*
>>>>> + * \todo Document the interface and remove the related EXCLUDE_PATTERNS entry.
>>>>
>>>> Ah that's why.
>>> Yes, because, well... all the other IPAs were doing that...
>>>
>>>> It doesn't have to be done before merging, but could you
>>>> address this sooner than later ?
>>> I can't promise a lot, but I'll look into that.
>>>
>>>>> + */
>>>>> +
>>>>> +module ipa.soft;
>>>>> +
>>>>> +import "include/libcamera/ipa/core.mojom";
>>>>> +
>>>>> +interface IPASoftInterface {
>>>>> +    init(libcamera.IPASettings settings,
>>>>> +         libcamera.SharedFD fdStats,
>>>>> +         libcamera.SharedFD fdParams,
>>>>
>>>> The IPA and soft ISP run in separate threads. How do you guarantee that
>>>> the stats and params are not accessed concurrently by both ?
>>> I tried avoiding using a pool of buffers for stats and params, and used just a
>>> single chunk of shared memory to pass the stats from soft ISP (its worker thread)
>>> to IPA, and the other chunk for params.
>>> The soft ISP accumulates the stats in its member variable, and after all the stats
>>> for the frame are calculated, the member variable is copyed to the shared memory and
>>> statReady signal is emitted (so using the member var implements some kind of double
>>> buffering). This way the write to the stats shared memory happens once per frame and
>>> is short in time right before emitting the statReady signal.
>>> The callback executed on receiving statReady signal invokes the IPA's processStats
>>> function. So the IPA's processStats() is called soon after the statReady is emitted.
>>> If the IPA finish processing the stats before the next frame is processed by the soft IPS
>>> and the stats for the next frame are written into the shared memory, there is no concurrent
>>> acess.
>>> This doesn't strictly guarantee that the stats are not accessed concurrently by the IPA and
>>> soft ISP. But unless the video stream from the camera sensor overloads the system this
>>> scheme works OK. And if the load is high the concurrent access isn't impossible, and can harm
>>> the image quality, but doesn't lead to critical problems like invalid pointers as the stats
>>> structure only contains numbers and an array of fixed size.
>>>
>>>> Shouldn't
>>>> you have a pool of buffers for each, mimicking what is done with
>>>> hardware ISPs ?
>>> This would definitely work, but is a more heavy wait implementation.
>>> On the other side, the current implementation is less universal (isn't very scalable), more fragile,
>>> and can't reliably associate the stats and the params with the given frame from the camera sensor.
>>> So let me try to implement what you suggested ("a pool of buffers for each, mimicking what is done with
>>> hardware ISPs").
>>
>> I won't be able to do this particular part by tomorrow...
>> So a branch I plan to publish tomorrow - with this patch updated to address the review comments
>> and the [PATCH v6 18/18] "libcamera: Soft IPA: use CameraSensorHelper for analogue gain" merged
>> into the earlier ones - would not use the pool of buffers for the stats and the params (yet).
> 
> Hi Andrei,
> 
> I already made those easier parts, see
> https://gitlab.freedesktop.org/mzamazal/libcamera-softisp/-/commits/SoftwareISP-v10?ref_type=heads
> (not ready for review yet).
> 
> Sorry for misunderstanding and duplicate work.

Actually that's fine, and you bwork on this patch is not lost.
I've reused some of your changes in the branch I've pushed:
https://gitlab.freedesktop.org/camera/libcamera-softisp/-/commits/SoftwareISP-v10-candidate-ynk1/?ref_type=heads

It is based on
https://gitlab.freedesktop.org/mzamazal/libcamera-softisp/-/commits/SoftwareISP-v10?ref_type=heads
I've only updated the commits:
   "libcamera: ipa: add Soft IPA"
   "libcamera: introduce SoftwareIsp"
and also rebased
   "libcamera: software_isp: Apply black level compensation"
as it depends on the changes to the other two.

The updated patchset still doesn't use the IPA configuration file, but it parses it and prints
the version read from the config file if debug messages are enabled for the IPASoft category.
If one runs libcamera without installing it, LIBCAMERA_IPA_CONFIG_PATH environment variable should be
set properly (LIBCAMERA_IPA_CONFIG_PATH=${the_install_dir}/share/libcamera/ipa).
I've figured out why I had problems with libcamera failing to find the IPA config file. Now this part works OK,
no hacks were needed.

As I wrote earlier, this version still doesn't have the pools of buffers for the params and the stats.
This is in my todo list, but I don't have the date for this yet.

Thanks,
Andrei

> Regards,
> Milan
> 
>> Thanks,
>> Andrei
>>
>>>>> +         libcamera.ControlInfoMap sensorCtrlInfoMap)
>>>>> +        => (int32 ret);
>>>>> +    start() => (int32 ret);
>>>>> +    stop();
>>>>> +    configure(libcamera.ControlInfoMap sensorCtrlInfoMap)
>>>>> +        => (int32 ret);
>>>>> +
>>>>> +    [async] processStats(libcamera.ControlList sensorControls);
>>>>> +};
>>>>> +
>>>>> +interface IPASoftEventInterface {
>>>>> +    setSensorControls(libcamera.ControlList sensorControls);
>>>>> +    setIspParams(int32 dummy);
>>>>
>>>> Drop the dummy value.
>>> libcamera docs do allow signals with zero parameters.
>>> But when I tried having zero parameters for an EventInterface function,
>>> it didn't work for me iirc.
>>> Let me double check.
>>>
>>>>> +};
>>>>> diff --git a/meson_options.txt b/meson_options.txt
>>>>> index 99dab96d..2644bef0 100644
>>>>> --- a/meson_options.txt
>>>>> +++ b/meson_options.txt
>>>>> @@ -27,7 +27,7 @@ option('gstreamer',
>>>>>    option('ipas',
>>>>>            type : 'array',
>>>>> -        choices : ['ipu3', 'rkisp1', 'rpi/vc4', 'vimc'],
>>>>> +        choices : ['ipu3', 'rkisp1', 'rpi/vc4', 'simple', 'vimc'],
>>>>>            description : 'Select which IPA modules to build')
>>>>>    option('lc-compliance',
>>>>> diff --git a/src/ipa/simple/data/meson.build b/src/ipa/simple/data/meson.build
>>>>> new file mode 100644
>>>>> index 00000000..33548cc6
>>>>> --- /dev/null
>>>>> +++ b/src/ipa/simple/data/meson.build
>>>>> @@ -0,0 +1,9 @@
>>>>> +# SPDX-License-Identifier: CC0-1.0
>>>>> +
>>>>> +conf_files = files([
>>>>> +    'soft.conf',
>>>>> +])
>>>>> +
>>>>> +install_data(conf_files,
>>>>> +             install_dir : ipa_data_dir / 'soft',
>>>>> +             install_tag : 'runtime')
>>>>> diff --git a/src/ipa/simple/data/soft.conf b/src/ipa/simple/data/soft.conf
>>>>> new file mode 100644
>>>>> index 00000000..0c70e7c0
>>>>> --- /dev/null
>>>>> +++ b/src/ipa/simple/data/soft.conf
>>>>
>>>> We use YAML files for all IPAs, could you do the same here ? It
>>>> shouldn't be much extra work as the file is empty :-)
>>> OK
>>>
>>>>> @@ -0,0 +1,3 @@
>>>>> +# SPDX-License-Identifier: LGPL-2.1-or-later
>>>>> +#
>>>>> +# Dummy configuration file for the soft IPA.
>>>>> diff --git a/src/ipa/simple/meson.build b/src/ipa/simple/meson.build
>>>>> new file mode 100644
>>>>> index 00000000..3e863db7
>>>>> --- /dev/null
>>>>> +++ b/src/ipa/simple/meson.build
>>>>> @@ -0,0 +1,25 @@
>>>>> +# SPDX-License-Identifier: CC0-1.0
>>>>> +
>>>>> +ipa_name = 'ipa_soft_simple'
>>>>> +
>>>>> +mod = shared_module(ipa_name,
>>>>> +                    ['soft_simple.cpp', libcamera_generated_ipa_headers],
>>>>> +                    name_prefix : '',
>>>>> +                    include_directories : [ipa_includes, libipa_includes],
>>>>> +                    dependencies : libcamera_private,
>>>>> +                    link_with : libipa,
>>>>> +                    install : true,
>>>>> +                    install_dir : ipa_install_dir)
>>>>> +
>>>>> +if ipa_sign_module
>>>>> +    custom_target(ipa_name + '.so.sign',
>>>>> +                  input : mod,
>>>>> +                  output : ipa_name + '.so.sign',
>>>>> +                  command : [ipa_sign, ipa_priv_key, '@INPUT@', '@OUTPUT@'],
>>>>> +                  install : false,
>>>>> +                  build_by_default : true)
>>>>> +endif
>>>>> +
>>>>> +subdir('data')
>>>>> +
>>>>> +ipa_names += ipa_name
>>>>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
>>>>> new file mode 100644
>>>>> index 00000000..312df4ba
>>>>> --- /dev/null
>>>>> +++ b/src/ipa/simple/soft_simple.cpp
>>>>> @@ -0,0 +1,326 @@
>>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>>>> +/*
>>>>> + * Copyright (C) 2023, Linaro Ltd
>>>>> + *
>>>>> + * soft_simple.cpp - Simple Software Image Processing Algorithm module
>>>>> + */
>>>>> +
>>>>> +#include <sys/mman.h>
>>>>> +
>>>>> +#include <libcamera/base/file.h>
>>>>> +#include <libcamera/base/log.h>
>>>>> +#include <libcamera/base/shared_fd.h>
>>>>> +
>>>>> +#include <libcamera/control_ids.h>
>>>>> +#include <libcamera/controls.h>
>>>>> +
>>>>> +#include <libcamera/ipa/ipa_interface.h>
>>>>> +#include <libcamera/ipa/ipa_module_info.h>
>>>>> +#include <libcamera/ipa/soft_ipa_interface.h>
>>>>> +
>>>>> +#include "libcamera/internal/camera_sensor.h"
>>>>
>>>> Not needed.
>>> OK
>>>
>>>>> +#include "libcamera/internal/software_isp/debayer_params.h"
>>>>> +#include "libcamera/internal/software_isp/swisp_stats.h"
>>>>> +
>>>>> +namespace libcamera {
>>>>> +
>>>>> +LOG_DEFINE_CATEGORY(IPASoft)
>>>>> +
>>>>> +namespace ipa::soft {
>>>>> +
>>>>> +class IPASoftSimple : public ipa::soft::IPASoftInterface
>>>>> +{
>>>>> +public:
>>>>> +    IPASoftSimple()
>>>>> +        : params_(static_cast<DebayerParams *>(MAP_FAILED)),
>>>>
>>>> Initialize this to nullptr, that's more standard, and will make the
>>>> tests in the destructor nicer. init() will have to set the pointers to
>>>> null if mmap() calls fail.
>>> OK
>>>
>>>>> +          stats_(static_cast<SwIspStats *>(MAP_FAILED)), ignore_updates_(0)
>>>>
>>>> s/ignore_updates_/ignoreUpdates_/
>>> OK
>>>
>>>>> +    {
>>>>> +    }
>>>>> +
>>>>> +    ~IPASoftSimple()
>>>>> +    {
>>>>> +        if (stats_ != MAP_FAILED)
>>>>> +            munmap(stats_, sizeof(SwIspStats));
>>>>> +        if (params_ != MAP_FAILED)
>>>>> +            munmap(params_, sizeof(DebayerParams));
>>>>> +    }
>>>>
>>>> No need to inline this.
>>> OK
>>>
>>>>> +
>>>>> +    int init(const IPASettings &settings,
>>>>> +         const SharedFD &fdStats,
>>>>> +         const SharedFD &fdParams,
>>>>> +         const ControlInfoMap &sensorInfoMap) override;
>>>>> +    int configure(const ControlInfoMap &sensorInfoMap) override;
>>>>> +
>>>>> +    int start() override;
>>>>> +    void stop() override;
>>>>> +
>>>>> +    void processStats(const ControlList &sensorControls) override;
>>>>> +
>>>>> +private:
>>>>> +    void updateExposure(double exposureMSV);
>>>>> +
>>>>> +    SharedFD fdStats_;
>>>>> +    SharedFD fdParams_;
>>>>> +    DebayerParams *params_;
>>>>> +    SwIspStats *stats_;
>>>>> +
>>>>> +    int32_t exposure_min_, exposure_max_;
>>>>> +    int32_t again_min_, again_max_;
>>>>> +    int32_t again_, exposure_;
>>>>> +    unsigned int ignore_updates_;
>>>>> +};
>>>>> +
>>>>> +int IPASoftSimple::init([[maybe_unused]] const IPASettings &settings,
>>>>> +            const SharedFD &fdStats,
>>>>> +            const SharedFD &fdParams,
>>>>> +            const ControlInfoMap &sensorInfoMap)
>>>>> +{
>>>>> +    fdStats_ = fdStats;
>>>>
>>>> As you never use fdStats_ and fdParams_ after this function returns,
>>>> there's no need to store a copy.
>>> Good catch. OK
>>>
>>>>> +    if (!fdStats_.isValid()) {
>>>>> +        LOG(IPASoft, Error) << "Invalid Statistics handle";
>>>>> +        return -ENODEV;
>>>>> +    }
>>>>> +
>>>>> +    fdParams_ = fdParams;
>>>>> +    if (!fdParams_.isValid()) {
>>>>> +        LOG(IPASoft, Error) << "Invalid Parameters handle";
>>>>> +        return -ENODEV;
>>>>> +    }
>>>>> +
>>>>> +    params_ = static_cast<DebayerParams *>(mmap(nullptr, sizeof(DebayerParams),
>>>>> +                            PROT_WRITE, MAP_SHARED,
>>>>> +                            fdParams_.get(), 0));
>>>>> +    if (params_ == MAP_FAILED) {
>>>>> +        LOG(IPASoft, Error) << "Unable to map Parameters";
>>>>> +        return -errno;
>>>>> +    }
>>>>
>>>>      void *mem = mmap(nullptr, sizeof(DebayerParams), PROT_WRITE, MAP_SHARED,
>>>>               fdParams_.get(), 0));
>>>>      if (mem == MAP_FAILED) {
>>>>          LOG(IPASoft, Error) << "Unable to map Parameters";
>>>>          return -errno;
>>>>      }
>>>>
>>>>      params_ = static_cast<DebayerParams *>(mem);
>>> OK
>>>
>>>>> +
>>>>> +    stats_ = static_cast<SwIspStats *>(mmap(nullptr, sizeof(SwIspStats),
>>>>> +                        PROT_READ, MAP_SHARED,
>>>>> +                        fdStats_.get(), 0));
>>>>> +    if (stats_ == MAP_FAILED) {
>>>>> +        LOG(IPASoft, Error) << "Unable to map Statistics";
>>>>> +        return -errno;
>>>>> +    }
>>>>> +
>>>>> +    if (sensorInfoMap.find(V4L2_CID_EXPOSURE) == sensorInfoMap.end()) {
>>>>> +        LOG(IPASoft, Error) << "Don't have exposure control";
>>>>> +        return -EINVAL;
>>>>> +    }
>>>>> +
>>>>> +    if (sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN) == sensorInfoMap.end()) {
>>>>> +        LOG(IPASoft, Error) << "Don't have gain control";
>>>>> +        return -EINVAL;
>>>>> +    }
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +int IPASoftSimple::configure(const ControlInfoMap &sensorInfoMap)
>>>>> +{
>>>>> +    const ControlInfo &exposure_info = sensorInfoMap.find(V4L2_CID_EXPOSURE)->second;
>>>>
>>>> s/exposure_info/exposureInfo/
>>>>
>>>> I'll let you address the other snake_case to camelCase conversions in
>>>> the series :-)
>>> OK
>>>
>>>>> +    const ControlInfo &gain_info = sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN)->second;
>>>>> +
>>>>> +    exposure_min_ = exposure_info.min().get<int32_t>();
>>>>> +    exposure_max_ = exposure_info.max().get<int32_t>();
>>>>> +    if (!exposure_min_) {
>>>>> +        LOG(IPASoft, Warning) << "Minimum exposure is zero, that can't be linear";
>>>>> +        exposure_min_ = 1;
>>>>> +    }
>>>>> +
>>>>> +    again_min_ = gain_info.min().get<int32_t>();
>>>>> +    again_max_ = gain_info.max().get<int32_t>();
>>>>
>>>> Add a blank line.
>>> OK
>>>
>>>>> +    /*
>>>>> +     * The camera sensor gain (g) is usually not equal to the value written
>>>>> +     * into the gain register (x). But the way how the AGC algorithm changes
>>>>> +     * the gain value to make the total exposure closer to the optimum assumes
>>>>> +     * that g(x) is not too far from linear function. If the minimal gain is 0,
>>>>> +     * the g(x) is likely to be far from the linear, like g(x) = a / (b * x + c).
>>>>> +     * To avoid unexpected changes to the gain by the AGC algorithm (abrupt near
>>>>> +     * one edge, and very small near the other) we limit the range of the gain
>>>>> +     * values used.
>>>>> +     */
>>>>> +    if (!again_min_) {
>>>>> +        LOG(IPASoft, Warning) << "Minimum gain is zero, that can't be linear";
>>>>> +        again_min_ = std::min(100, again_min_ / 2 + again_max_ / 2);
>>>>> +    }
>>>>
>>>> A patch further in the series addresses this by using
>>>> CameraSensorHelper. It should be squashed with this patch.
>>> OK
>>>
>>>>> +
>>>>> +    LOG(IPASoft, Info) << "Exposure " << exposure_min_ << "-" << exposure_max_
>>>>> +               << ", gain " << again_min_ << "-" << again_max_;
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +int IPASoftSimple::start()
>>>>> +{
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +void IPASoftSimple::stop()
>>>>> +{
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * The number of bins to use for the optimal exposure calculations.
>>>>> + */
>>>>> +static constexpr unsigned int kExposureBinsCount = 5;
>>>>
>>>> Missing blank line. Same below.
>>> OK
>>>
>>>>> +/*
>>>>> + * The exposure is optimal when the mean sample value of the histogram is
>>>>> + * in the middle of the range.
>>>>> + */
>>>>> +static constexpr float kExposureOptimal = kExposureBinsCount / 2.0;
>>>>> +/*
>>>>> + * The below value implements the hysteresis for the exposure adjustment.
>>>>> + * It is small enough to have the exposure close to the optimal, and is big
>>>>> + * enough to prevent the exposure from wobbling around the optimal value.
>>>>> + */
>>>>> +static constexpr float kExposureSatisfactory = 0.2;
>>>>
>>>> Move these to the beginning of the file, just before the IPASoftSimple
>>>> definition.
>>> OK
>>>
>>>>> +
>>>>> +void IPASoftSimple::processStats(const ControlList &sensorControls)
>>>>> +{
>>>>> +    /*
>>>>> +     * Calculate red and blue gains for AWB.
>>>>> +     * Clamp max gain at 4.0, this also avoids 0 division.
>>>>> +     */
>>>>> +    if (stats_->sumR_ <= stats_->sumG_ / 4)
>>>>> +        params_->gainR = 1024;
>>>>> +    else
>>>>> +        params_->gainR = 256 * stats_->sumG_ / stats_->sumR_;
>>>>> +
>>>>> +    if (stats_->sumB_ <= stats_->sumG_ / 4)
>>>>> +        params_->gainB = 1024;
>>>>> +    else
>>>>> +        params_->gainB = 256 * stats_->sumG_ / stats_->sumB_;
>>>>> +
>>>>> +    /* Green gain and gamma values are fixed */
>>>>> +    params_->gainG = 256;
>>>>> +    params_->gamma = 0.5;
>>>>> +
>>>>> +    setIspParams.emit(0);
>>>>
>>>> Do you envision switching to the libipa/algorithm.h API at some point ?
>>>> If so, it would be nice to record it somewhere.
>>> At some point, yes.
>>> Will mention that.
>>>
>>>>> +
>>>>> +    /*
>>>>> +     * AE / AGC, use 2 frames delay to make sure that the exposure and
>>>>> +     * the gain set have applied to the camera sensor.
>>>>> +     */
>>>>> +    if (ignore_updates_ > 0) {
>>>>> +        --ignore_updates_;
>>>>> +        return;
>>>>> +    }
>>>>
>>>> Also something that could be improved and that should be recorded
>>>> somewhere.
>>> OK
>>>
>>>>> +
>>>>> +    /*
>>>>> +     * Calculate Mean Sample Value (MSV) according to formula from:
>>>>> +     * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf
>>>>> +     */
>>>>> +    constexpr unsigned int yHistValsPerBin =
>>>>> +        SwIspStats::kYHistogramSize / kExposureBinsCount;
>>>>> +    constexpr unsigned int yHistValsPerBinMod =
>>>>> +        SwIspStats::kYHistogramSize /
>>>>> +        (SwIspStats::kYHistogramSize % kExposureBinsCount + 1);
>>>>> +    int ExposureBins[kExposureBinsCount] = {};
>>>>
>>>> s/ExposureBins/exposureBins/
>>> OK
>>>
>>>>> +    unsigned int denom = 0;
>>>>> +    unsigned int num = 0;
>>>>> +
>>>>> +    for (unsigned int i = 0; i < SwIspStats::kYHistogramSize; i++) {
>>>>> +        unsigned int idx = (i - (i / yHistValsPerBinMod)) / yHistValsPerBin;
>>>>> +        ExposureBins[idx] += stats_->yHistogram[i];
>>>>> +    }
>>>>> +
>>>>> +    for (unsigned int i = 0; i < kExposureBinsCount; i++) {
>>>>> +        LOG(IPASoft, Debug) << i << ": " << ExposureBins[i];
>>>>> +        denom += ExposureBins[i];
>>>>> +        num += ExposureBins[i] * (i + 1);
>>>>> +    }
>>>>> +
>>>>> +    float exposureMSV = (float)num / denom;
>>>>
>>>> C++ casts.
>>> OK
>>>
>>>>> +
>>>>> +    /* sanity check */
>>>>
>>>> s/sanity/Sanity/
>>> OK
>>>
>>>>> +    if (!sensorControls.contains(V4L2_CID_EXPOSURE) ||
>>>>> +        !sensorControls.contains(V4L2_CID_ANALOGUE_GAIN)) {
>>>>> +        LOG(IPASoft, Error) << "Control(s) missing";
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    ControlList ctrls(sensorControls);
>>>>
>>>> You shouldn't copy the control list, but instead create one from the
>>>> ControlInfoMap that you get in init() (and that you then need to stored
>>>> in the IPA class).
>>> OK, thanks
>>>
>>>>> +
>>>>> +    exposure_ = ctrls.get(V4L2_CID_EXPOSURE).get<int32_t>();
>>>>> +    again_ = ctrls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
>>>>
>>>>      exposure_ = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
>>>>      again_ = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
>>> OK
>>>
>>>>> +
>>>>> +    updateExposure(exposureMSV);
>>>>> +
>>>>> +    ctrls.set(V4L2_CID_EXPOSURE, exposure_);
>>>>> +    ctrls.set(V4L2_CID_ANALOGUE_GAIN, again_);
>>>>> +
>>>>> +    ignore_updates_ = 2;
>>>>> +
>>>>> +    setSensorControls.emit(ctrls);
>>>>> +
>>>>> +    LOG(IPASoft, Debug) << "exposureMSV " << exposureMSV
>>>>> +                << " exp " << exposure_ << " again " << again_
>>>>> +                << " gain R/B " << params_->gainR << "/" << params_->gainB;
>>>>> +}
>>>>> +
>>>>> +void IPASoftSimple::updateExposure(double exposureMSV)
>>>>> +{
>>>>> +    /* DENOMINATOR of 10 gives ~10% increment/decrement; DENOMINATOR of 5 - about ~20% */
>>>>
>>>> Line wrap, and the DENOMINATOR needs to be renamed to kExpDenominator.
>>> OK
>>> Thanks for the review,
>>> Andrei
>>>
>>>>> +    static constexpr uint8_t kExpDenominator = 10;
>>>>> +    static constexpr uint8_t kExpNumeratorUp = kExpDenominator + 1;
>>>>> +    static constexpr uint8_t kExpNumeratorDown = kExpDenominator - 1;
>>>>> +
>>>>> +    int next;
>>>>> +
>>>>> +    if (exposureMSV < kExposureOptimal - kExposureSatisfactory) {
>>>>> +        next = exposure_ * kExpNumeratorUp / kExpDenominator;
>>>>> +        if (next - exposure_ < 1)
>>>>> +            exposure_ += 1;
>>>>> +        else
>>>>> +            exposure_ = next;
>>>>> +        if (exposure_ >= exposure_max_) {
>>>>> +            next = again_ * kExpNumeratorUp / kExpDenominator;
>>>>> +            if (next - again_ < 1)
>>>>> +                again_ += 1;
>>>>> +            else
>>>>> +                again_ = next;
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>> +    if (exposureMSV > kExposureOptimal + kExposureSatisfactory) {
>>>>> +        if (exposure_ == exposure_max_ && again_ != again_min_) {
>>>>> +            next = again_ * kExpNumeratorDown / kExpDenominator;
>>>>> +            if (again_ - next < 1)
>>>>> +                again_ -= 1;
>>>>> +            else
>>>>> +                again_ = next;
>>>>> +        } else {
>>>>> +            next = exposure_ * kExpNumeratorDown / kExpDenominator;
>>>>> +            if (exposure_ - next < 1)
>>>>> +                exposure_ -= 1;
>>>>> +            else
>>>>> +                exposure_ = next;
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>> +    exposure_ = std::clamp(exposure_, exposure_min_, exposure_max_);
>>>>> +    again_ = std::clamp(again_, again_min_, again_max_);
>>>>> +}
>>>>> +
>>>>> +} /* namespace ipa::soft */
>>>>> +
>>>>> +/*
>>>>> + * External IPA module interface
>>>>> + */
>>>>> +extern "C" {
>>>>> +const struct IPAModuleInfo ipaModuleInfo = {
>>>>> +    IPA_MODULE_API_VERSION,
>>>>> +    0,
>>>>> +    "SimplePipelineHandler",
>>>>> +    "simple",
>>>>> +};
>>>>> +
>>>>> +IPAInterface *ipaCreate()
>>>>> +{
>>>>> +    return new ipa::soft::IPASoftSimple();
>>>>> +}
>>>>> +
>>>>> +} /* extern "C" */
>>>>> +
>>>>> +} /* namespace libcamera */
>>>>
>

Andrei Konovalov <andrey.konovalov.ynk@gmail.com> writes:

> I've reused some of your changes in the branch I've pushed:
> https://gitlab.freedesktop.org/camera/libcamera-softisp/-/commits/SoftwareISP-v10-candidate-ynk1/?ref_type=heads
>
> It is based on
> https://gitlab.freedesktop.org/mzamazal/libcamera-softisp/-/commits/SoftwareISP-v10?ref_type=heads
> I've only updated the commits:
>   "libcamera: ipa: add Soft IPA"
>   "libcamera: introduce SoftwareIsp"
> and also rebased
>   "libcamera: software_isp: Apply black level compensation"
> as it depends on the changes to the other two.

Hi Andrei,

nice work, thank you!  I integrated your changes into my branch (I just fixed
one trivial typo in a comment), which is now available also at
https://gitlab.freedesktop.org/camera/libcamera-softisp/-/tree/SoftwareISP-v10?ref_type=heads

I'll post v7 soon.

Regards,
Milan

> The updated patchset still doesn't use the IPA configuration file, but it parses it and prints
> the version read from the config file if debug messages are enabled for the IPASoft category.
> If one runs libcamera without installing it, LIBCAMERA_IPA_CONFIG_PATH environment variable should be
> set properly (LIBCAMERA_IPA_CONFIG_PATH=${the_install_dir}/share/libcamera/ipa).
> I've figured out why I had problems with libcamera failing to find the IPA config file. Now this part works OK,
> no hacks were needed.
>
> As I wrote earlier, this version still doesn't have the pools of buffers for the params and the stats.
> This is in my todo list, but I don't have the date for this yet.
>
> Thanks,
> Andrei