[v3,09/16] libcamera: ipa: add Soft IPA

19492 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>
---
Changes in v3:
- Use new SwIspStats::kYHistogramSize constexpr and adjust
  the auto-exposure/-gain code so that it can deal with
  that having a different value then 16 (modify the loop
  to divide the histogram in 5 bins to not have hardcoded
  values)
- Rename a few foo_bar symbols to fooBar
---
 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    | 308 ++++++++++++++++++++++++++++++
 8 files changed, 376 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


2024. február 14., szerda 18:01 keltezéssel, Hans de Goede <hdegoede@redhat.com> írta:

> 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>
> ---
> Changes in v3:
> - Use new SwIspStats::kYHistogramSize constexpr and adjust
>   the auto-exposure/-gain code so that it can deal with
>   that having a different value then 16 (modify the loop
>   to divide the histogram in 5 bins to not have hardcoded
>   values)
> - Rename a few foo_bar symbols to fooBar
> ---
>  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    | 308 ++++++++++++++++++++++++++++++
>  8 files changed, 376 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/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> new file mode 100644
> index 00000000..f7ac02f9
> --- /dev/null
> +++ b/src/ipa/simple/soft_simple.cpp
> @@ -0,0 +1,308 @@
> +/* 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"
> +#include "libcamera/internal/software_isp/debayer_params.h"
> +#include "libcamera/internal/software_isp/swisp_stats.h"
> +
> +#define EXPOSURE_OPTIMAL_VALUE 2.5
> +#define EXPOSURE_SATISFACTORY_OFFSET 0.2
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(IPASoft)
> +
> +namespace ipa::soft {
> +
> +class IPASoftSimple : public ipa::soft::IPASoftInterface
> +{
> +public:
> +	IPASoftSimple()
> +		: ignore_updates_(0)
> +	{
> +	}
> +
> +	~IPASoftSimple()
> +	{
> +		if (stats_)

if (stats_ != MAP_FAILED)


> +			munmap(stats_, sizeof(SwIspStats));
> +		if (params_)
> +			munmap(params_, sizeof(DebayerParams));
> +	}
> +
> +	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_;

The above two should be initialized to MAP_FAILED.


> +	int exposure_min_, exposure_max_;
> +	int again_min_, again_max_;
> +	int again_, exposure_;
> +	int ignore_updates_;
> +};
> +
> +int IPASoftSimple::init([[maybe_unused]] const IPASettings &settings,
> +			const SharedFD &fdStats,
> +			const SharedFD &fdParams,
> +			const ControlInfoMap &sensorInfoMap)
> +{
> +	fdStats_ = std::move(fdStats);

std::move(fdStats) has the type `const SharedFD&&`, so `fdStats_ = ...`
will result in a call to the copy assignment operator `operator=(const SharedFD&)`.
So `std::move()` does not really do anything here, and can be removed.
Alternatively, if possible, you can change the argument type to be just `SharedFD`,
and then moving makes sense (preferably in the member initializer list).


> +	if (!fdStats_.isValid()) {
> +		LOG(IPASoft, Error) << "Invalid Statistics handle";
> +		return -ENODEV;
> +	}
> +
> +	fdParams_ = std::move(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_) {

mmap returns MAP_FAILED on failure.


> +		LOG(IPASoft, Error) << "Unable to map Parameters";
> +		return -ENODEV;

Maybe using `errno` would be better?


> +	}
> +
> +	stats_ = static_cast<SwIspStats *>(mmap(nullptr, sizeof(SwIspStats),
> +						PROT_READ, MAP_SHARED,
> +						fdStats_.get(), 0));
> +	if (!stats_) {
> +		LOG(IPASoft, Error) << "Unable to map Statistics";
> +		return -ENODEV;
> +	}
> +
> +	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;
> +}
> [...]

Hans de Goede <hdegoede@redhat.com> writes:

> 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>
> ---
> Changes in v3:
> - Use new SwIspStats::kYHistogramSize constexpr and adjust
>   the auto-exposure/-gain code so that it can deal with
>   that having a different value then 16 (modify the loop
>   to divide the histogram in 5 bins to not have hardcoded
>   values)
> - Rename a few foo_bar symbols to fooBar
> ---
>  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    | 308 ++++++++++++++++++++++++++++++
>  8 files changed, 376 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 \
>                           @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.
> + */
> +
> +module ipa.soft;
> +
> +import "include/libcamera/ipa/core.mojom";
> +
> +interface IPASoftInterface {
> +	init(libcamera.IPASettings settings,
> +	     libcamera.SharedFD fdStats,
> +	     libcamera.SharedFD fdParams,
> +	     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);
> +};
> diff --git a/meson_options.txt b/meson_options.txt
> index 5fdc7be8..94372e47 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
> @@ -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..f7ac02f9
> --- /dev/null
> +++ b/src/ipa/simple/soft_simple.cpp
> @@ -0,0 +1,308 @@
> +/* 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"
> +#include "libcamera/internal/software_isp/debayer_params.h"
> +#include "libcamera/internal/software_isp/swisp_stats.h"
> +
> +#define EXPOSURE_OPTIMAL_VALUE 2.5
> +#define EXPOSURE_SATISFACTORY_OFFSET 0.2

Do both the values come from the cited paper or do they deserve some
explanation?

And maybe constexpr rather than #define?

> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(IPASoft)
> +
> +namespace ipa::soft {
> +
> +class IPASoftSimple : public ipa::soft::IPASoftInterface
> +{
> +public:
> +	IPASoftSimple()
> +		: ignore_updates_(0)
> +	{
> +	}
> +
> +	~IPASoftSimple()
> +	{
> +		if (stats_)
> +			munmap(stats_, sizeof(SwIspStats));
> +		if (params_)
> +			munmap(params_, sizeof(DebayerParams));
> +	}
> +
> +	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_;
> +	int exposure_min_, exposure_max_;
> +	int again_min_, again_max_;
> +	int again_, exposure_;
> +	int ignore_updates_;
> +};
> +
> +int IPASoftSimple::init([[maybe_unused]] const IPASettings &settings,
> +			const SharedFD &fdStats,
> +			const SharedFD &fdParams,
> +			const ControlInfoMap &sensorInfoMap)
> +{
> +	fdStats_ = std::move(fdStats);
> +	if (!fdStats_.isValid()) {
> +		LOG(IPASoft, Error) << "Invalid Statistics handle";
> +		return -ENODEV;
> +	}
> +
> +	fdParams_ = std::move(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_) {
> +		LOG(IPASoft, Error) << "Unable to map Parameters";
> +		return -ENODEV;
> +	}
> +
> +	stats_ = static_cast<SwIspStats *>(mmap(nullptr, sizeof(SwIspStats),
> +						PROT_READ, MAP_SHARED,
> +						fdStats_.get(), 0));
> +	if (!stats_) {
> +		LOG(IPASoft, Error) << "Unable to map Statistics";
> +		return -ENODEV;
> +	}
> +
> +	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;
> +	const ControlInfo &gain_info = sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN)->second;
> +
> +	exposure_min_ = exposure_info.min().get<int>();
> +	if (!exposure_min_) {
> +		LOG(IPASoft, Warning) << "Minimum exposure is zero, that can't be linear";
> +		exposure_min_ = 1;
> +	}
> +	exposure_max_ = exposure_info.max().get<int>();
> +	again_min_ = gain_info.min().get<int>();
> +	if (!again_min_) {
> +		LOG(IPASoft, Warning) << "Minimum gain is zero, that can't be linear";
> +		again_min_ = 100;
> +	}

I wonder what "can't be linear" means here exactly. 

> +	again_max_ = gain_info.max().get<int>();

Can it happen that again_max_ is lower than the artificially set again_min_ =
100 above?

> +
> +	LOG(IPASoft, Info) << "Exposure " << exposure_min_ << "-" << exposure_max_
> +			   << ", gain " << again_min_ << "-" << again_max_;
> +
> +	return 0;
> +}
> +
> +int IPASoftSimple::start()
> +{
> +	return 0;
> +}
> +
> +void IPASoftSimple::stop()
> +{
> +}
> +
> +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);
> +
> +	/*
> +	 * 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;
> +	}
> +
> +	/*
> +	 * Calculate Mean Sample Value (MSV) according to formula from:
> +	 * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf
> +	 */
> +	constexpr unsigned int ExposureBinsCount = 5;
> +	constexpr unsigned int yHistValsPerBin =
> +		SwIspStats::kYHistogramSize / ExposureBinsCount;
> +	constexpr unsigned int yHistValsPerBinMod =
> +		SwIspStats::kYHistogramSize /
> +		(SwIspStats::kYHistogramSize % ExposureBinsCount + 1);
> +	int ExposureBins[ExposureBinsCount] = {};
> +	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 < ExposureBinsCount; i++) {
> +		LOG(IPASoft, Debug) << i << ": " << ExposureBins[i];
> +		denom += ExposureBins[i];
> +		num += ExposureBins[i] * (i + 1);
> +	}
> +
> +	float exposureMSV = (float)num / denom;
> +
> +	/* sanity check */
> +	if (!sensorControls.contains(V4L2_CID_EXPOSURE) ||
> +	    !sensorControls.contains(V4L2_CID_ANALOGUE_GAIN)) {
> +		LOG(IPASoft, Error) << "Control(s) missing";
> +		return;
> +	}
> +
> +	ControlList ctrls(sensorControls);
> +
> +	exposure_ = ctrls.get(V4L2_CID_EXPOSURE).get<int>();
> +	again_ = ctrls.get(V4L2_CID_ANALOGUE_GAIN).get<int>();
> +
> +	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;
> +}
> +
> +/* DENOMINATOR of 10 gives ~10% increment/decrement; DENOMINATOR of 5 - about ~20% */
> +#define DENOMINATOR 10
> +#define UP_NUMERATOR (DENOMINATOR + 1)
> +#define DOWN_NUMERATOR (DENOMINATOR - 1)
> +
> +void IPASoftSimple::updateExposure(double exposureMSV)
> +{
> +	int next;
> +
> +	if (exposureMSV < EXPOSURE_OPTIMAL_VALUE - EXPOSURE_SATISFACTORY_OFFSET) {
> +		next = exposure_ * UP_NUMERATOR / DENOMINATOR;
> +		if (next - exposure_ < 1)
> +			exposure_ += 1;
> +		else
> +			exposure_ = next;
> +		if (exposure_ >= exposure_max_) {
> +			next = again_ * UP_NUMERATOR / DENOMINATOR;
> +			if (next - again_ < 1)
> +				again_ += 1;
> +			else
> +				again_ = next;
> +		}
> +	}
> +
> +	if (exposureMSV > EXPOSURE_OPTIMAL_VALUE + EXPOSURE_SATISFACTORY_OFFSET) {
> +		if (exposure_ == exposure_max_ && again_ != again_min_) {
> +			next = again_ * DOWN_NUMERATOR / DENOMINATOR;
> +			if (again_ - next < 1)
> +				again_ -= 1;
> +			else
> +				again_ = next;
> +		} else {
> +			next = exposure_ * DOWN_NUMERATOR / DENOMINATOR;
> +			if (exposure_ - next < 1)
> +				exposure_ -= 1;
> +			else
> +				exposure_ = next;
> +		}
> +	}
> +
> +	if (exposure_ > exposure_max_)
> +		exposure_ = exposure_max_;
> +	else if (exposure_ < exposure_min_)
> +		exposure_ = exposure_min_;
> +
> +	if (again_ > again_max_)
> +		again_ = again_max_;
> +	else if (again_ < again_min_)
> +		again_ = again_min_;
> +}
> +
> +} /* 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 */

Quoting Milan Zamazal (2024-02-15 16:31:07)
> Hans de Goede <hdegoede@redhat.com> writes:
> 
> > 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>

Definitely a team effort ;-) I like it! It's a big project after all.

<snip>

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

Indeed, these should be going through the CameraSensorHelper class to
convert and manage gains.

At some level/point you will need to know what the gain codes are as a
multiplier. The CameraSensor helpers are expected to provide that sensor
specific calculation for you.



--
Kieran

Quoting Hans de Goede (2024-02-14 17:01:13)
> 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>
> ---
> Changes in v3:
> - Use new SwIspStats::kYHistogramSize constexpr and adjust
>   the auto-exposure/-gain code so that it can deal with
>   that having a different value then 16 (modify the loop
>   to divide the histogram in 5 bins to not have hardcoded
>   values)
> - Rename a few foo_bar symbols to fooBar
> ---
>  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    | 308 ++++++++++++++++++++++++++++++
>  8 files changed, 376 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 \
>                           @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.
> + */
> +
> +module ipa.soft;
> +
> +import "include/libcamera/ipa/core.mojom";
> +
> +interface IPASoftInterface {
> +       init(libcamera.IPASettings settings,
> +            libcamera.SharedFD fdStats,
> +            libcamera.SharedFD fdParams,
> +            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);
> +};
> diff --git a/meson_options.txt b/meson_options.txt
> index 5fdc7be8..94372e47 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
> @@ -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..f7ac02f9
> --- /dev/null
> +++ b/src/ipa/simple/soft_simple.cpp
> @@ -0,0 +1,308 @@
> +/* 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"
> +#include "libcamera/internal/software_isp/debayer_params.h"
> +#include "libcamera/internal/software_isp/swisp_stats.h"
> +
> +#define EXPOSURE_OPTIMAL_VALUE 2.5
> +#define EXPOSURE_SATISFACTORY_OFFSET 0.2

Perhaps:

static constexpr float kExposureOptimal = 2.5;
static constexpr float kExposureSatisfactory = 0.2;

But can those constants have some more comments or documentation about
/why/ they are optimal and satisfactory? Where do they come from? what
are the ranges? Anything applicable to say about them?


> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(IPASoft)
> +
> +namespace ipa::soft {
> +
> +class IPASoftSimple : public ipa::soft::IPASoftInterface
> +{
> +public:
> +       IPASoftSimple()
> +               : ignore_updates_(0)
> +       {
> +       }
> +
> +       ~IPASoftSimple()
> +       {
> +               if (stats_)
> +                       munmap(stats_, sizeof(SwIspStats));
> +               if (params_)
> +                       munmap(params_, sizeof(DebayerParams));
> +       }
> +
> +       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_;

I'd throw a blank line between class instances and the following PODs

> +       int exposure_min_, exposure_max_;
> +       int again_min_, again_max_;
> +       int again_, exposure_;
> +       int ignore_updates_;

Should those by unsigned? Can they ever be 0 or less than zero?

> +};
> +
> +int IPASoftSimple::init([[maybe_unused]] const IPASettings &settings,
> +                       const SharedFD &fdStats,
> +                       const SharedFD &fdParams,
> +                       const ControlInfoMap &sensorInfoMap)
> +{
> +       fdStats_ = std::move(fdStats);
> +       if (!fdStats_.isValid()) {
> +               LOG(IPASoft, Error) << "Invalid Statistics handle";
> +               return -ENODEV;
> +       }
> +
> +       fdParams_ = std::move(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));

We'd usually use MappedFrameBuffer() here so that the mappings are
handled by RAII here

> +       if (!params_) {
> +               LOG(IPASoft, Error) << "Unable to map Parameters";
> +               return -ENODEV;
> +       }
> +
> +       stats_ = static_cast<SwIspStats *>(mmap(nullptr, sizeof(SwIspStats),
> +                                               PROT_READ, MAP_SHARED,
> +                                               fdStats_.get(), 0));
> +       if (!stats_) {
> +               LOG(IPASoft, Error) << "Unable to map Statistics";
> +               return -ENODEV;
> +       }

Same here.


> +
> +       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;
> +       }
> +

In the future I would expect AEGC to be broken out to it's own modular
'Algorithm' (see rkisp1 for the createAlgorithms()


We're also lacking any set up or registration for control handling. I
guess for now there may be none, but do you plan to have controls? I'm
fine with those being added later.



> +       return 0;
> +}
> +
> +int IPASoftSimple::configure(const ControlInfoMap &sensorInfoMap)
> +{
> +       const ControlInfo &exposure_info = sensorInfoMap.find(V4L2_CID_EXPOSURE)->second;
> +       const ControlInfo &gain_info = sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN)->second;
> +
> +       exposure_min_ = exposure_info.min().get<int>();
> +       if (!exposure_min_) {
> +               LOG(IPASoft, Warning) << "Minimum exposure is zero, that can't be linear";
> +               exposure_min_ = 1;
> +       }
> +       exposure_max_ = exposure_info.max().get<int>();
> +       again_min_ = gain_info.min().get<int>();
> +       if (!again_min_) {
> +               LOG(IPASoft, Warning) << "Minimum gain is zero, that can't be linear";
> +               again_min_ = 100;
> +       }

This is where you should almost certainly be looking at the
CameraSensorHelpers. We expect the gain model to be abstracted by each
sensor through the helpers.

IPA algorithms should operate on a linear model, and the
CameraSensorHelpers will convert any logarithmic gain codes to linear on
a per-sensor basis, as well as impose any sensor specific limits.


> +       again_max_ = gain_info.max().get<int>();
> +
> +       LOG(IPASoft, Info) << "Exposure " << exposure_min_ << "-" << exposure_max_
> +                          << ", gain " << again_min_ << "-" << again_max_;
> +
> +       return 0;
> +}
> +
> +int IPASoftSimple::start()
> +{
> +       return 0;
> +}
> +
> +void IPASoftSimple::stop()
> +{
> +}
> +
> +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);
> +
> +       /*
> +        * AE / AGC, use 2 frames delay to make sure that the exposure and
> +        * the gain set have applied to the camera sensor.
> +        */

Using known sensor specific delays defined in the CameraSensorHelper and
DelayedControls should be considered here in the future. But as a first
implementation for CPU only, where doing /less/ work is helpful - this
is fine for me.


> +       if (ignore_updates_ > 0) {
> +               --ignore_updates_;
> +               return;
> +       }
> +
> +       /*
> +        * Calculate Mean Sample Value (MSV) according to formula from:
> +        * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf
> +        */
> +       constexpr unsigned int ExposureBinsCount = 5;
> +       constexpr unsigned int yHistValsPerBin =
> +               SwIspStats::kYHistogramSize / ExposureBinsCount;
> +       constexpr unsigned int yHistValsPerBinMod =
> +               SwIspStats::kYHistogramSize /
> +               (SwIspStats::kYHistogramSize % ExposureBinsCount + 1);
> +       int ExposureBins[ExposureBinsCount] = {};
> +       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 < ExposureBinsCount; i++) {
> +               LOG(IPASoft, Debug) << i << ": " << ExposureBins[i];
> +               denom += ExposureBins[i];
> +               num += ExposureBins[i] * (i + 1);
> +       }
> +
> +       float exposureMSV = (float)num / denom;
> +
> +       /* sanity check */
> +       if (!sensorControls.contains(V4L2_CID_EXPOSURE) ||
> +           !sensorControls.contains(V4L2_CID_ANALOGUE_GAIN)) {
> +               LOG(IPASoft, Error) << "Control(s) missing";
> +               return;
> +       }
> +
> +       ControlList ctrls(sensorControls);
> +
> +       exposure_ = ctrls.get(V4L2_CID_EXPOSURE).get<int>();
> +       again_ = ctrls.get(V4L2_CID_ANALOGUE_GAIN).get<int>();
> +
> +       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;
> +}
> +
> +/* DENOMINATOR of 10 gives ~10% increment/decrement; DENOMINATOR of 5 - about ~20% */
> +#define DENOMINATOR 10
> +#define UP_NUMERATOR (DENOMINATOR + 1)
> +#define DOWN_NUMERATOR (DENOMINATOR - 1)

Our C++ code style for constants would be more like:

static constexpr uint8_t kExpDenominator = 10;
static constexpr uint8_t kExpNumeratorUp = kExpDenominator + 1;
static constexpr uint8_t kExpNumeratorDown = kExpDenominator - 1;

> +
> +void IPASoftSimple::updateExposure(double exposureMSV)
> +{
> +       int next;
> +
> +       if (exposureMSV < EXPOSURE_OPTIMAL_VALUE - EXPOSURE_SATISFACTORY_OFFSET) {
> +               next = exposure_ * UP_NUMERATOR / DENOMINATOR;
> +               if (next - exposure_ < 1)
> +                       exposure_ += 1;
> +               else
> +                       exposure_ = next;
> +               if (exposure_ >= exposure_max_) {
> +                       next = again_ * UP_NUMERATOR / DENOMINATOR;
> +                       if (next - again_ < 1)
> +                               again_ += 1;
> +                       else
> +                               again_ = next;
> +               }
> +       }
> +
> +       if (exposureMSV > EXPOSURE_OPTIMAL_VALUE + EXPOSURE_SATISFACTORY_OFFSET) {
> +               if (exposure_ == exposure_max_ && again_ != again_min_) {
> +                       next = again_ * DOWN_NUMERATOR / DENOMINATOR;
> +                       if (again_ - next < 1)
> +                               again_ -= 1;
> +                       else
> +                               again_ = next;
> +               } else {
> +                       next = exposure_ * DOWN_NUMERATOR / DENOMINATOR;
> +                       if (exposure_ - next < 1)
> +                               exposure_ -= 1;
> +                       else
> +                               exposure_ = next;
> +               }
> +       }
> +
> +       if (exposure_ > exposure_max_)
> +               exposure_ = exposure_max_;
> +       else if (exposure_ < exposure_min_)
> +               exposure_ = exposure_min_;
> +
> +       if (again_ > again_max_)
> +               again_ = again_max_;
> +       else if (again_ < again_min_)
> +               again_ = again_min_;

How about
	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 */
> -- 
> 2.43.0
>

On 20.02.2024 16:05, Kieran Bingham wrote:
> Quoting Hans de Goede (2024-02-14 17:01:13)
>> 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>
>> ---
>> Changes in v3:
>> - Use new SwIspStats::kYHistogramSize constexpr and adjust
>>    the auto-exposure/-gain code so that it can deal with
>>    that having a different value then 16 (modify the loop
>>    to divide the histogram in 5 bins to not have hardcoded
>>    values)
>> - Rename a few foo_bar symbols to fooBar
>> ---
>>   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    | 308 ++++++++++++++++++++++++++++++
>>   8 files changed, 376 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 \
>>                            @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.
>> + */
>> +
>> +module ipa.soft;
>> +
>> +import "include/libcamera/ipa/core.mojom";
>> +
>> +interface IPASoftInterface {
>> +       init(libcamera.IPASettings settings,
>> +            libcamera.SharedFD fdStats,
>> +            libcamera.SharedFD fdParams,
>> +            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);
>> +};
>> diff --git a/meson_options.txt b/meson_options.txt
>> index 5fdc7be8..94372e47 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
>> @@ -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..f7ac02f9
>> --- /dev/null
>> +++ b/src/ipa/simple/soft_simple.cpp
>> @@ -0,0 +1,308 @@
>> +/* 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"
>> +#include "libcamera/internal/software_isp/debayer_params.h"
>> +#include "libcamera/internal/software_isp/swisp_stats.h"
>> +
>> +#define EXPOSURE_OPTIMAL_VALUE 2.5
>> +#define EXPOSURE_SATISFACTORY_OFFSET 0.2
> 
> Perhaps:
> 
> static constexpr float kExposureOptimal = 2.5;
> static constexpr float kExposureSatisfactory = 0.2;
> 
> But can those constants have some more comments or documentation about
> /why/ they are optimal and satisfactory? Where do they come from? what
> are the ranges? Anything applicable to say about them?

I second the oppinion that some more comments here would be useful.

To my understanding, kExposureOptimal = ExposureBinsCount / 2.0;
(That is, the mean sample value of the histogram should be in the middle of the range.)

And kExposureSatisfactory is the hesteresys to prevent the exposure from wobbling around
the optimal exposure. Some empirically selected value small enough to have the exposure
close to the optimal, and big enough to prevent the jitter.

Please correct me if I am wrong.

Still here is what is not quite clear to me, and the paper84final.pdf doesn't seem to
explain:

SwIspStats::kYHistogramSize is 16. For AE 5 bins are used.
What is the advantage of reassembling the 16 bins from SwIspStats into 5
(vs e.g. setting ExposureBinsCount to SwIspStats::kYHistogramSize and using 16 bins) ?

>> +
>> +namespace libcamera {
>> +
>> +LOG_DEFINE_CATEGORY(IPASoft)
>> +
>> +namespace ipa::soft {
>> +
>> +class IPASoftSimple : public ipa::soft::IPASoftInterface
>> +{
>> +public:
>> +       IPASoftSimple()
>> +               : ignore_updates_(0)
>> +       {
>> +       }
>> +
>> +       ~IPASoftSimple()
>> +       {
>> +               if (stats_)
>> +                       munmap(stats_, sizeof(SwIspStats));
>> +               if (params_)
>> +                       munmap(params_, sizeof(DebayerParams));
>> +       }
>> +
>> +       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_;
> 
> I'd throw a blank line between class instances and the following PODs
> 
>> +       int exposure_min_, exposure_max_;
>> +       int again_min_, again_max_;
>> +       int again_, exposure_;
>> +       int ignore_updates_;
> 
> Should those by unsigned? Can they ever be 0 or less than zero?

unsigned would be fine

>> +};
>> +
>> +int IPASoftSimple::init([[maybe_unused]] const IPASettings &settings,
>> +                       const SharedFD &fdStats,
>> +                       const SharedFD &fdParams,
>> +                       const ControlInfoMap &sensorInfoMap)
>> +{
>> +       fdStats_ = std::move(fdStats);
>> +       if (!fdStats_.isValid()) {
>> +               LOG(IPASoft, Error) << "Invalid Statistics handle";
>> +               return -ENODEV;
>> +       }
>> +
>> +       fdParams_ = std::move(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));
> 
> We'd usually use MappedFrameBuffer() here so that the mappings are
> handled by RAII here

OK, will fix that.

>> +       if (!params_) {
>> +               LOG(IPASoft, Error) << "Unable to map Parameters";
>> +               return -ENODEV;
>> +       }
>> +
>> +       stats_ = static_cast<SwIspStats *>(mmap(nullptr, sizeof(SwIspStats),
>> +                                               PROT_READ, MAP_SHARED,
>> +                                               fdStats_.get(), 0));
>> +       if (!stats_) {
>> +               LOG(IPASoft, Error) << "Unable to map Statistics";
>> +               return -ENODEV;
>> +       }
> 
> Same here.

Ack

>> +
>> +       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;
>> +       }
>> +
> 
> In the future I would expect AEGC to be broken out to it's own modular
> 'Algorithm' (see rkisp1 for the createAlgorithms()

Thanks, I'll take a look.

> We're also lacking any set up or registration for control handling. I
> guess for now there may be none, but do you plan to have controls? I'm
> fine with those being added later.

I'd stick to what we currently have (none), and revisit this later.

>> +       return 0;
>> +}
>> +
>> +int IPASoftSimple::configure(const ControlInfoMap &sensorInfoMap)
>> +{
>> +       const ControlInfo &exposure_info = sensorInfoMap.find(V4L2_CID_EXPOSURE)->second;
>> +       const ControlInfo &gain_info = sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN)->second;
>> +
>> +       exposure_min_ = exposure_info.min().get<int>();
>> +       if (!exposure_min_) {
>> +               LOG(IPASoft, Warning) << "Minimum exposure is zero, that can't be linear";
>> +               exposure_min_ = 1;
>> +       }
>> +       exposure_max_ = exposure_info.max().get<int>();
>> +       again_min_ = gain_info.min().get<int>();
>> +       if (!again_min_) {
>> +               LOG(IPASoft, Warning) << "Minimum gain is zero, that can't be linear";
>> +               again_min_ = 100;
>> +       }
> 
> This is where you should almost certainly be looking at the
> CameraSensorHelpers. We expect the gain model to be abstracted by each
> sensor through the helpers.
> 
> IPA algorithms should operate on a linear model, and the
> CameraSensorHelpers will convert any logarithmic gain codes to linear on
> a per-sensor basis, as well as impose any sensor specific limits.

I am going to draft a patch to use the CameraSensorHelpers before the end of this week
(hopefully sooner).

>> +       again_max_ = gain_info.max().get<int>();
>> +
>> +       LOG(IPASoft, Info) << "Exposure " << exposure_min_ << "-" << exposure_max_
>> +                          << ", gain " << again_min_ << "-" << again_max_;
>> +
>> +       return 0;
>> +}
>> +
>> +int IPASoftSimple::start()
>> +{
>> +       return 0;
>> +}
>> +
>> +void IPASoftSimple::stop()
>> +{
>> +}
>> +
>> +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);
>> +
>> +       /*
>> +        * AE / AGC, use 2 frames delay to make sure that the exposure and
>> +        * the gain set have applied to the camera sensor.
>> +        */
> 
> Using known sensor specific delays defined in the CameraSensorHelper and
> DelayedControls should be considered here in the future. But as a first
> implementation for CPU only, where doing /less/ work is helpful - this
> is fine for me.

Agreed.

>> +       if (ignore_updates_ > 0) {
>> +               --ignore_updates_;
>> +               return;
>> +       }
>> +
>> +       /*
>> +        * Calculate Mean Sample Value (MSV) according to formula from:
>> +        * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf
>> +        */
>> +       constexpr unsigned int ExposureBinsCount = 5;
>> +       constexpr unsigned int yHistValsPerBin =
>> +               SwIspStats::kYHistogramSize / ExposureBinsCount;
>> +       constexpr unsigned int yHistValsPerBinMod =
>> +               SwIspStats::kYHistogramSize /
>> +               (SwIspStats::kYHistogramSize % ExposureBinsCount + 1);
>> +       int ExposureBins[ExposureBinsCount] = {};
>> +       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 < ExposureBinsCount; i++) {
>> +               LOG(IPASoft, Debug) << i << ": " << ExposureBins[i];
>> +               denom += ExposureBins[i];
>> +               num += ExposureBins[i] * (i + 1);
>> +       }
>> +
>> +       float exposureMSV = (float)num / denom;
>> +
>> +       /* sanity check */
>> +       if (!sensorControls.contains(V4L2_CID_EXPOSURE) ||
>> +           !sensorControls.contains(V4L2_CID_ANALOGUE_GAIN)) {
>> +               LOG(IPASoft, Error) << "Control(s) missing";
>> +               return;
>> +       }
>> +
>> +       ControlList ctrls(sensorControls);
>> +
>> +       exposure_ = ctrls.get(V4L2_CID_EXPOSURE).get<int>();
>> +       again_ = ctrls.get(V4L2_CID_ANALOGUE_GAIN).get<int>();
>> +
>> +       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;
>> +}
>> +
>> +/* DENOMINATOR of 10 gives ~10% increment/decrement; DENOMINATOR of 5 - about ~20% */
>> +#define DENOMINATOR 10
>> +#define UP_NUMERATOR (DENOMINATOR + 1)
>> +#define DOWN_NUMERATOR (DENOMINATOR - 1)
> 
> Our C++ code style for constants would be more like:
> 
> static constexpr uint8_t kExpDenominator = 10;
> static constexpr uint8_t kExpNumeratorUp = kExpDenominator + 1;
> static constexpr uint8_t kExpNumeratorDown = kExpDenominator - 1;

This was written by me, and I'll update this part to use static constexpr.

>> +
>> +void IPASoftSimple::updateExposure(double exposureMSV)
>> +{
>> +       int next;
>> +
>> +       if (exposureMSV < EXPOSURE_OPTIMAL_VALUE - EXPOSURE_SATISFACTORY_OFFSET) {
>> +               next = exposure_ * UP_NUMERATOR / DENOMINATOR;
>> +               if (next - exposure_ < 1)
>> +                       exposure_ += 1;
>> +               else
>> +                       exposure_ = next;
>> +               if (exposure_ >= exposure_max_) {
>> +                       next = again_ * UP_NUMERATOR / DENOMINATOR;
>> +                       if (next - again_ < 1)
>> +                               again_ += 1;
>> +                       else
>> +                               again_ = next;
>> +               }
>> +       }
>> +
>> +       if (exposureMSV > EXPOSURE_OPTIMAL_VALUE + EXPOSURE_SATISFACTORY_OFFSET) {
>> +               if (exposure_ == exposure_max_ && again_ != again_min_) {
>> +                       next = again_ * DOWN_NUMERATOR / DENOMINATOR;
>> +                       if (again_ - next < 1)
>> +                               again_ -= 1;
>> +                       else
>> +                               again_ = next;
>> +               } else {
>> +                       next = exposure_ * DOWN_NUMERATOR / DENOMINATOR;
>> +                       if (exposure_ - next < 1)
>> +                               exposure_ -= 1;
>> +                       else
>> +                               exposure_ = next;
>> +               }
>> +       }
>> +
>> +       if (exposure_ > exposure_max_)
>> +               exposure_ = exposure_max_;
>> +       else if (exposure_ < exposure_min_)
>> +               exposure_ = exposure_min_;
>> +
>> +       if (again_ > again_max_)
>> +               again_ = again_max_;
>> +       else if (again_ < again_min_)
>> +               again_ = again_min_;
> 
> How about
> 	exposure_ = std::clamp(exposure_, exposure_min_, exposure_max_);
>   	again_ = std::clamp(again_, again_min_, again_max_);
> 
> ?

OK, will fix.

Thanks,
Andrei

>> +}
>> +
>> +} /* 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 */
>> -- 
>> 2.43.0
>>

Quoting Andrei Konovalov (2024-02-20 15:23:02)
> On 20.02.2024 16:05, Kieran Bingham wrote:
> > Quoting Hans de Goede (2024-02-14 17:01:13)
> >> 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>
> >> ---
> >> Changes in v3:
> >> - Use new SwIspStats::kYHistogramSize constexpr and adjust
> >>    the auto-exposure/-gain code so that it can deal with
> >>    that having a different value then 16 (modify the loop
> >>    to divide the histogram in 5 bins to not have hardcoded
> >>    values)
> >> - Rename a few foo_bar symbols to fooBar
> >> ---
> >>   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    | 308 ++++++++++++++++++++++++++++++
> >>   8 files changed, 376 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 \
> >>                            @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.
> >> + */
> >> +
> >> +module ipa.soft;
> >> +
> >> +import "include/libcamera/ipa/core.mojom";
> >> +
> >> +interface IPASoftInterface {
> >> +       init(libcamera.IPASettings settings,
> >> +            libcamera.SharedFD fdStats,
> >> +            libcamera.SharedFD fdParams,
> >> +            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);
> >> +};
> >> diff --git a/meson_options.txt b/meson_options.txt
> >> index 5fdc7be8..94372e47 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
> >> @@ -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..f7ac02f9
> >> --- /dev/null
> >> +++ b/src/ipa/simple/soft_simple.cpp
> >> @@ -0,0 +1,308 @@
> >> +/* 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"
> >> +#include "libcamera/internal/software_isp/debayer_params.h"
> >> +#include "libcamera/internal/software_isp/swisp_stats.h"
> >> +
> >> +#define EXPOSURE_OPTIMAL_VALUE 2.5
> >> +#define EXPOSURE_SATISFACTORY_OFFSET 0.2
> > 
> > Perhaps:
> > 
> > static constexpr float kExposureOptimal = 2.5;
> > static constexpr float kExposureSatisfactory = 0.2;
> > 
> > But can those constants have some more comments or documentation about
> > /why/ they are optimal and satisfactory? Where do they come from? what
> > are the ranges? Anything applicable to say about them?
> 
> I second the oppinion that some more comments here would be useful.
> 
> To my understanding, kExposureOptimal = ExposureBinsCount / 2.0;
> (That is, the mean sample value of the histogram should be in the middle of the range.)

Yes, even better - if they are the result of other parameters,
expressing that is far more helpful to know that things will change if
we add more bins, or ... that they'll automatically adapt...

> And kExposureSatisfactory is the hesteresys to prevent the exposure from wobbling around
> the optimal exposure. Some empirically selected value small enough to have the exposure
> close to the optimal, and big enough to prevent the jitter.

Sounds feasible to me - but wasn't clear just from seeing two numbers,
so comments welcome ;-)

> Please correct me if I am wrong.
> 
> Still here is what is not quite clear to me, and the paper84final.pdf doesn't seem to
> explain:
> 
> SwIspStats::kYHistogramSize is 16. For AE 5 bins are used.
> What is the advantage of reassembling the 16 bins from SwIspStats into 5
> (vs e.g. setting ExposureBinsCount to SwIspStats::kYHistogramSize and using 16 bins) ?
> 
> >> +
> >> +namespace libcamera {
> >> +
> >> +LOG_DEFINE_CATEGORY(IPASoft)
> >> +
> >> +namespace ipa::soft {
> >> +
> >> +class IPASoftSimple : public ipa::soft::IPASoftInterface
> >> +{
> >> +public:
> >> +       IPASoftSimple()
> >> +               : ignore_updates_(0)
> >> +       {
> >> +       }
> >> +
> >> +       ~IPASoftSimple()
> >> +       {
> >> +               if (stats_)
> >> +                       munmap(stats_, sizeof(SwIspStats));
> >> +               if (params_)
> >> +                       munmap(params_, sizeof(DebayerParams));
> >> +       }
> >> +
> >> +       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_;
> > 
> > I'd throw a blank line between class instances and the following PODs
> > 
> >> +       int exposure_min_, exposure_max_;
> >> +       int again_min_, again_max_;
> >> +       int again_, exposure_;
> >> +       int ignore_updates_;
> > 
> > Should those by unsigned? Can they ever be 0 or less than zero?
> 
> unsigned would be fine
> 
> >> +};
> >> +
> >> +int IPASoftSimple::init([[maybe_unused]] const IPASettings &settings,
> >> +                       const SharedFD &fdStats,
> >> +                       const SharedFD &fdParams,
> >> +                       const ControlInfoMap &sensorInfoMap)
> >> +{
> >> +       fdStats_ = std::move(fdStats);
> >> +       if (!fdStats_.isValid()) {
> >> +               LOG(IPASoft, Error) << "Invalid Statistics handle";
> >> +               return -ENODEV;
> >> +       }
> >> +
> >> +       fdParams_ = std::move(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));
> > 
> > We'd usually use MappedFrameBuffer() here so that the mappings are
> > handled by RAII here
> 
> OK, will fix that.
> 
> >> +       if (!params_) {
> >> +               LOG(IPASoft, Error) << "Unable to map Parameters";
> >> +               return -ENODEV;
> >> +       }
> >> +
> >> +       stats_ = static_cast<SwIspStats *>(mmap(nullptr, sizeof(SwIspStats),
> >> +                                               PROT_READ, MAP_SHARED,
> >> +                                               fdStats_.get(), 0));
> >> +       if (!stats_) {
> >> +               LOG(IPASoft, Error) << "Unable to map Statistics";
> >> +               return -ENODEV;
> >> +       }
> > 
> > Same here.
> 
> Ack
> 
> >> +
> >> +       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;
> >> +       }
> >> +
> > 
> > In the future I would expect AEGC to be broken out to it's own modular
> > 'Algorithm' (see rkisp1 for the createAlgorithms()
> 
> Thanks, I'll take a look.

If it makes sense and helps starting to make things modular then it's
definitely a nice to have .... but as we don't really have a lot of
algorithms here to break out - I don't mind if it's done later. It's
just helpful to be aware of the design goals of the ipas.


> > We're also lacking any set up or registration for control handling. I
> > guess for now there may be none, but do you plan to have controls? I'm
> > fine with those being added later.
> 
> I'd stick to what we currently have (none), and revisit this later.

Ack. That's one part that I think breaking out algorithms to components
will help though as each algorithm can/should then be responsible for
registering what controls it will support and handling them.


> 
> >> +       return 0;
> >> +}
> >> +
> >> +int IPASoftSimple::configure(const ControlInfoMap &sensorInfoMap)
> >> +{
> >> +       const ControlInfo &exposure_info = sensorInfoMap.find(V4L2_CID_EXPOSURE)->second;
> >> +       const ControlInfo &gain_info = sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN)->second;
> >> +
> >> +       exposure_min_ = exposure_info.min().get<int>();
> >> +       if (!exposure_min_) {
> >> +               LOG(IPASoft, Warning) << "Minimum exposure is zero, that can't be linear";
> >> +               exposure_min_ = 1;
> >> +       }
> >> +       exposure_max_ = exposure_info.max().get<int>();
> >> +       again_min_ = gain_info.min().get<int>();
> >> +       if (!again_min_) {
> >> +               LOG(IPASoft, Warning) << "Minimum gain is zero, that can't be linear";
> >> +               again_min_ = 100;
> >> +       }
> > 
> > This is where you should almost certainly be looking at the
> > CameraSensorHelpers. We expect the gain model to be abstracted by each
> > sensor through the helpers.
> > 
> > IPA algorithms should operate on a linear model, and the
> > CameraSensorHelpers will convert any logarithmic gain codes to linear on
> > a per-sensor basis, as well as impose any sensor specific limits.
> 
> I am going to draft a patch to use the CameraSensorHelpers before the end of this week
> (hopefully sooner).

I hope this part shouldn't be too complicated. They're already used in
other IPAs. The idea is to factor out the common sensor specific
parts away from the IPA.


--
Kieran


> 
> >> +       again_max_ = gain_info.max().get<int>();
> >> +
> >> +       LOG(IPASoft, Info) << "Exposure " << exposure_min_ << "-" << exposure_max_
> >> +                          << ", gain " << again_min_ << "-" << again_max_;
> >> +
> >> +       return 0;
> >> +}
> >> +
> >> +int IPASoftSimple::start()
> >> +{
> >> +       return 0;
> >> +}
> >> +
> >> +void IPASoftSimple::stop()
> >> +{
> >> +}
> >> +
> >> +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);
> >> +
> >> +       /*
> >> +        * AE / AGC, use 2 frames delay to make sure that the exposure and
> >> +        * the gain set have applied to the camera sensor.
> >> +        */
> > 
> > Using known sensor specific delays defined in the CameraSensorHelper and
> > DelayedControls should be considered here in the future. But as a first
> > implementation for CPU only, where doing /less/ work is helpful - this
> > is fine for me.
> 
> Agreed.
> 
> >> +       if (ignore_updates_ > 0) {
> >> +               --ignore_updates_;
> >> +               return;
> >> +       }
> >> +
> >> +       /*
> >> +        * Calculate Mean Sample Value (MSV) according to formula from:
> >> +        * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf
> >> +        */
> >> +       constexpr unsigned int ExposureBinsCount = 5;
> >> +       constexpr unsigned int yHistValsPerBin =
> >> +               SwIspStats::kYHistogramSize / ExposureBinsCount;
> >> +       constexpr unsigned int yHistValsPerBinMod =
> >> +               SwIspStats::kYHistogramSize /
> >> +               (SwIspStats::kYHistogramSize % ExposureBinsCount + 1);
> >> +       int ExposureBins[ExposureBinsCount] = {};
> >> +       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 < ExposureBinsCount; i++) {
> >> +               LOG(IPASoft, Debug) << i << ": " << ExposureBins[i];
> >> +               denom += ExposureBins[i];
> >> +               num += ExposureBins[i] * (i + 1);
> >> +       }
> >> +
> >> +       float exposureMSV = (float)num / denom;
> >> +
> >> +       /* sanity check */
> >> +       if (!sensorControls.contains(V4L2_CID_EXPOSURE) ||
> >> +           !sensorControls.contains(V4L2_CID_ANALOGUE_GAIN)) {
> >> +               LOG(IPASoft, Error) << "Control(s) missing";
> >> +               return;
> >> +       }
> >> +
> >> +       ControlList ctrls(sensorControls);
> >> +
> >> +       exposure_ = ctrls.get(V4L2_CID_EXPOSURE).get<int>();
> >> +       again_ = ctrls.get(V4L2_CID_ANALOGUE_GAIN).get<int>();
> >> +
> >> +       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;
> >> +}
> >> +
> >> +/* DENOMINATOR of 10 gives ~10% increment/decrement; DENOMINATOR of 5 - about ~20% */
> >> +#define DENOMINATOR 10
> >> +#define UP_NUMERATOR (DENOMINATOR + 1)
> >> +#define DOWN_NUMERATOR (DENOMINATOR - 1)
> > 
> > Our C++ code style for constants would be more like:
> > 
> > static constexpr uint8_t kExpDenominator = 10;
> > static constexpr uint8_t kExpNumeratorUp = kExpDenominator + 1;
> > static constexpr uint8_t kExpNumeratorDown = kExpDenominator - 1;
> 
> This was written by me, and I'll update this part to use static constexpr.
> 
> >> +
> >> +void IPASoftSimple::updateExposure(double exposureMSV)
> >> +{
> >> +       int next;
> >> +
> >> +       if (exposureMSV < EXPOSURE_OPTIMAL_VALUE - EXPOSURE_SATISFACTORY_OFFSET) {
> >> +               next = exposure_ * UP_NUMERATOR / DENOMINATOR;
> >> +               if (next - exposure_ < 1)
> >> +                       exposure_ += 1;
> >> +               else
> >> +                       exposure_ = next;
> >> +               if (exposure_ >= exposure_max_) {
> >> +                       next = again_ * UP_NUMERATOR / DENOMINATOR;
> >> +                       if (next - again_ < 1)
> >> +                               again_ += 1;
> >> +                       else
> >> +                               again_ = next;
> >> +               }
> >> +       }
> >> +
> >> +       if (exposureMSV > EXPOSURE_OPTIMAL_VALUE + EXPOSURE_SATISFACTORY_OFFSET) {
> >> +               if (exposure_ == exposure_max_ && again_ != again_min_) {
> >> +                       next = again_ * DOWN_NUMERATOR / DENOMINATOR;
> >> +                       if (again_ - next < 1)
> >> +                               again_ -= 1;
> >> +                       else
> >> +                               again_ = next;
> >> +               } else {
> >> +                       next = exposure_ * DOWN_NUMERATOR / DENOMINATOR;
> >> +                       if (exposure_ - next < 1)
> >> +                               exposure_ -= 1;
> >> +                       else
> >> +                               exposure_ = next;
> >> +               }
> >> +       }
> >> +
> >> +       if (exposure_ > exposure_max_)
> >> +               exposure_ = exposure_max_;
> >> +       else if (exposure_ < exposure_min_)
> >> +               exposure_ = exposure_min_;
> >> +
> >> +       if (again_ > again_max_)
> >> +               again_ = again_max_;
> >> +       else if (again_ < again_min_)
> >> +               again_ = again_min_;
> > 
> > How about
> >       exposure_ = std::clamp(exposure_, exposure_min_, exposure_max_);
> >       again_ = std::clamp(again_, again_min_, again_max_);
> > 
> > ?
> 
> OK, will fix.
> 
> Thanks,
> Andrei
> 
> >> +}
> >> +
> >> +} /* 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 */
> >> -- 
> >> 2.43.0
> >>

Hi Barnabás,

On 14.02.2024 21:15, Barnabás Pőcze wrote:
> Hi
> 
> 
> 2024. február 14., szerda 18:01 keltezéssel, Hans de Goede <hdegoede@redhat.com> írta:
> 
>> 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>
>> ---
>> Changes in v3:
>> - Use new SwIspStats::kYHistogramSize constexpr and adjust
>>    the auto-exposure/-gain code so that it can deal with
>>    that having a different value then 16 (modify the loop
>>    to divide the histogram in 5 bins to not have hardcoded
>>    values)
>> - Rename a few foo_bar symbols to fooBar
>> ---
>>   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    | 308 ++++++++++++++++++++++++++++++
>>   8 files changed, 376 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/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
>> new file mode 100644
>> index 00000000..f7ac02f9
>> --- /dev/null
>> +++ b/src/ipa/simple/soft_simple.cpp
>> @@ -0,0 +1,308 @@
>> +/* 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"
>> +#include "libcamera/internal/software_isp/debayer_params.h"
>> +#include "libcamera/internal/software_isp/swisp_stats.h"
>> +
>> +#define EXPOSURE_OPTIMAL_VALUE 2.5
>> +#define EXPOSURE_SATISFACTORY_OFFSET 0.2
>> +
>> +namespace libcamera {
>> +
>> +LOG_DEFINE_CATEGORY(IPASoft)
>> +
>> +namespace ipa::soft {
>> +
>> +class IPASoftSimple : public ipa::soft::IPASoftInterface
>> +{
>> +public:
>> +	IPASoftSimple()
>> +		: ignore_updates_(0)
>> +	{
>> +	}
>> +
>> +	~IPASoftSimple()
>> +	{
>> +		if (stats_)
> 
> if (stats_ != MAP_FAILED)
> 
> 
>> +			munmap(stats_, sizeof(SwIspStats));
>> +		if (params_)
>> +			munmap(params_, sizeof(DebayerParams));
>> +	}
>> +
>> +	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_;
> 
> The above two should be initialized to MAP_FAILED.
> 
> 
>> +	int exposure_min_, exposure_max_;
>> +	int again_min_, again_max_;
>> +	int again_, exposure_;
>> +	int ignore_updates_;
>> +};
>> +
>> +int IPASoftSimple::init([[maybe_unused]] const IPASettings &settings,
>> +			const SharedFD &fdStats,
>> +			const SharedFD &fdParams,
>> +			const ControlInfoMap &sensorInfoMap)
>> +{
>> +	fdStats_ = std::move(fdStats);
> 
> std::move(fdStats) has the type `const SharedFD&&`, so `fdStats_ = ...`
> will result in a call to the copy assignment operator `operator=(const SharedFD&)`.
> So `std::move()` does not really do anything here, and can be removed.
> Alternatively, if possible, you can change the argument type to be just `SharedFD`,
> and then moving makes sense (preferably in the member initializer list).

Good catch

>> +	if (!fdStats_.isValid()) {
>> +		LOG(IPASoft, Error) << "Invalid Statistics handle";
>> +		return -ENODEV;
>> +	}
>> +
>> +	fdParams_ = std::move(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_) {
> 
> mmap returns MAP_FAILED on failure.

Right. I'll fix this and the related parts in the next version of the patch set.

>> +		LOG(IPASoft, Error) << "Unable to map Parameters";
>> +		return -ENODEV;
> 
> Maybe using `errno` would be better?

Yes, sounds reasonable.

Thanks,
Andrei

>> +	}
>> +
>> +	stats_ = static_cast<SwIspStats *>(mmap(nullptr, sizeof(SwIspStats),
>> +						PROT_READ, MAP_SHARED,
>> +						fdStats_.get(), 0));
>> +	if (!stats_) {
>> +		LOG(IPASoft, Error) << "Unable to map Statistics";
>> +		return -ENODEV;
>> +	}
>> +
>> +	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;
>> +}
>> [...]