[PATCH v6 09/18] libcamera: ipa: add Soft IPA
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Mar 27 17:38:08 CET 2024
Hi Milan and Andrey,
Thank you for the patch.
On Tue, Mar 19, 2024 at 01:35:56PM +0100, Milan Zamazal wrote:
> From: Andrey Konovalov <andrey.konovalov at linaro.org>
>
> Define the Soft IPA main and event interfaces, add the Soft IPA
> implementation.
>
> The current src/ipa/meson.build assumes the IPA name to match the
> pipeline name. For this reason "-Dipas=simple" is used for the
> Soft IPA module.
This should be addressed, please record this as a \todo item somewhere.
> Auto exposure/gain and AWB implementation by Dennis, Toon and Martti.
>
> Auto exposure/gain targets a Mean Sample Value of 2.5 following
> the MSV calculation algorithm from:
> https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf
>
> Tested-by: Bryan O'Donoghue <bryan.odonoghue at linaro.org> # sc8280xp Lenovo x13s
> Tested-by: Pavel Machek <pavel at ucw.cz>
> Reviewed-by: Pavel Machek <pavel at ucw.cz>
> Signed-off-by: Andrey Konovalov <andrey.konovalov at linaro.org>
> Co-developed-by: Dennis Bonke <admin at dennisbonke.com>
> Signed-off-by: Dennis Bonke <admin at dennisbonke.com>
> Co-developed-by: Marttico <g.martti at gmail.com>
> Signed-off-by: Marttico <g.martti at gmail.com>
> Co-developed-by: Toon Langendam <t.langendam at gmail.com>
> Signed-off-by: Toon Langendam <t.langendam at gmail.com>
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> ---
> Documentation/Doxyfile.in | 1 +
> include/libcamera/ipa/meson.build | 1 +
> include/libcamera/ipa/soft.mojom | 28 +++
> meson_options.txt | 2 +-
> src/ipa/simple/data/meson.build | 9 +
> src/ipa/simple/data/soft.conf | 3 +
> src/ipa/simple/meson.build | 25 +++
> src/ipa/simple/soft_simple.cpp | 326 ++++++++++++++++++++++++++++++
> 8 files changed, 394 insertions(+), 1 deletion(-)
> create mode 100644 include/libcamera/ipa/soft.mojom
> create mode 100644 src/ipa/simple/data/meson.build
> create mode 100644 src/ipa/simple/data/soft.conf
> create mode 100644 src/ipa/simple/meson.build
> create mode 100644 src/ipa/simple/soft_simple.cpp
>
> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> index a86ea6c1..2be8d47b 100644
> --- a/Documentation/Doxyfile.in
> +++ b/Documentation/Doxyfile.in
> @@ -44,6 +44,7 @@ EXCLUDE = @TOP_SRCDIR@/include/libcamera/base/span.h \
> @TOP_SRCDIR@/src/libcamera/pipeline/ \
> @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \
> @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \
> + @TOP_BUILDDIR@/include/libcamera/ipa/soft_ipa_interface.h \
Why is this needed ?
> @TOP_BUILDDIR@/src/libcamera/proxy/
>
> EXCLUDE_PATTERNS = @TOP_BUILDDIR@/include/libcamera/ipa/*_serializer.h \
> diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build
> index f3b4881c..3352d08f 100644
> --- a/include/libcamera/ipa/meson.build
> +++ b/include/libcamera/ipa/meson.build
> @@ -65,6 +65,7 @@ pipeline_ipa_mojom_mapping = {
> 'ipu3': 'ipu3.mojom',
> 'rkisp1': 'rkisp1.mojom',
> 'rpi/vc4': 'raspberrypi.mojom',
> + 'simple': 'soft.mojom',
> 'vimc': 'vimc.mojom',
> }
>
> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom
> new file mode 100644
> index 00000000..c249bd75
> --- /dev/null
> +++ b/include/libcamera/ipa/soft.mojom
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +
> +/*
> + * \todo Document the interface and remove the related EXCLUDE_PATTERNS entry.
Ah that's why. It doesn't have to be done before merging, but could you
address this sooner than later ?
> + */
> +
> +module ipa.soft;
> +
> +import "include/libcamera/ipa/core.mojom";
> +
> +interface IPASoftInterface {
> + init(libcamera.IPASettings settings,
> + libcamera.SharedFD fdStats,
> + libcamera.SharedFD fdParams,
The IPA and soft ISP run in separate threads. How do you guarantee that
the stats and params are not accessed concurrently by both ? Shouldn't
you have a pool of buffers for each, mimicking what is done with
hardware ISPs ?
> + libcamera.ControlInfoMap sensorCtrlInfoMap)
> + => (int32 ret);
> + start() => (int32 ret);
> + stop();
> + configure(libcamera.ControlInfoMap sensorCtrlInfoMap)
> + => (int32 ret);
> +
> + [async] processStats(libcamera.ControlList sensorControls);
> +};
> +
> +interface IPASoftEventInterface {
> + setSensorControls(libcamera.ControlList sensorControls);
> + setIspParams(int32 dummy);
Drop the dummy value.
> +};
> diff --git a/meson_options.txt b/meson_options.txt
> index 99dab96d..2644bef0 100644
> --- a/meson_options.txt
> +++ b/meson_options.txt
> @@ -27,7 +27,7 @@ option('gstreamer',
>
> option('ipas',
> type : 'array',
> - choices : ['ipu3', 'rkisp1', 'rpi/vc4', 'vimc'],
> + choices : ['ipu3', 'rkisp1', 'rpi/vc4', 'simple', 'vimc'],
> description : 'Select which IPA modules to build')
>
> option('lc-compliance',
> diff --git a/src/ipa/simple/data/meson.build b/src/ipa/simple/data/meson.build
> new file mode 100644
> index 00000000..33548cc6
> --- /dev/null
> +++ b/src/ipa/simple/data/meson.build
> @@ -0,0 +1,9 @@
> +# SPDX-License-Identifier: CC0-1.0
> +
> +conf_files = files([
> + 'soft.conf',
> +])
> +
> +install_data(conf_files,
> + install_dir : ipa_data_dir / 'soft',
> + install_tag : 'runtime')
> diff --git a/src/ipa/simple/data/soft.conf b/src/ipa/simple/data/soft.conf
> new file mode 100644
> index 00000000..0c70e7c0
> --- /dev/null
> +++ b/src/ipa/simple/data/soft.conf
We use YAML files for all IPAs, could you do the same here ? It
shouldn't be much extra work as the file is empty :-)
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: LGPL-2.1-or-later
> +#
> +# Dummy configuration file for the soft IPA.
> diff --git a/src/ipa/simple/meson.build b/src/ipa/simple/meson.build
> new file mode 100644
> index 00000000..3e863db7
> --- /dev/null
> +++ b/src/ipa/simple/meson.build
> @@ -0,0 +1,25 @@
> +# SPDX-License-Identifier: CC0-1.0
> +
> +ipa_name = 'ipa_soft_simple'
> +
> +mod = shared_module(ipa_name,
> + ['soft_simple.cpp', libcamera_generated_ipa_headers],
> + name_prefix : '',
> + include_directories : [ipa_includes, libipa_includes],
> + dependencies : libcamera_private,
> + link_with : libipa,
> + install : true,
> + install_dir : ipa_install_dir)
> +
> +if ipa_sign_module
> + custom_target(ipa_name + '.so.sign',
> + input : mod,
> + output : ipa_name + '.so.sign',
> + command : [ipa_sign, ipa_priv_key, '@INPUT@', '@OUTPUT@'],
> + install : false,
> + build_by_default : true)
> +endif
> +
> +subdir('data')
> +
> +ipa_names += ipa_name
> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> new file mode 100644
> index 00000000..312df4ba
> --- /dev/null
> +++ b/src/ipa/simple/soft_simple.cpp
> @@ -0,0 +1,326 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2023, Linaro Ltd
> + *
> + * soft_simple.cpp - Simple Software Image Processing Algorithm module
> + */
> +
> +#include <sys/mman.h>
> +
> +#include <libcamera/base/file.h>
> +#include <libcamera/base/log.h>
> +#include <libcamera/base/shared_fd.h>
> +
> +#include <libcamera/control_ids.h>
> +#include <libcamera/controls.h>
> +
> +#include <libcamera/ipa/ipa_interface.h>
> +#include <libcamera/ipa/ipa_module_info.h>
> +#include <libcamera/ipa/soft_ipa_interface.h>
> +
> +#include "libcamera/internal/camera_sensor.h"
Not needed.
> +#include "libcamera/internal/software_isp/debayer_params.h"
> +#include "libcamera/internal/software_isp/swisp_stats.h"
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(IPASoft)
> +
> +namespace ipa::soft {
> +
> +class IPASoftSimple : public ipa::soft::IPASoftInterface
> +{
> +public:
> + IPASoftSimple()
> + : params_(static_cast<DebayerParams *>(MAP_FAILED)),
Initialize this to nullptr, that's more standard, and will make the
tests in the destructor nicer. init() will have to set the pointers to
null if mmap() calls fail.
> + stats_(static_cast<SwIspStats *>(MAP_FAILED)), ignore_updates_(0)
s/ignore_updates_/ignoreUpdates_/
> + {
> + }
> +
> + ~IPASoftSimple()
> + {
> + if (stats_ != MAP_FAILED)
> + munmap(stats_, sizeof(SwIspStats));
> + if (params_ != MAP_FAILED)
> + munmap(params_, sizeof(DebayerParams));
> + }
No need to inline this.
> +
> + int init(const IPASettings &settings,
> + const SharedFD &fdStats,
> + const SharedFD &fdParams,
> + const ControlInfoMap &sensorInfoMap) override;
> + int configure(const ControlInfoMap &sensorInfoMap) override;
> +
> + int start() override;
> + void stop() override;
> +
> + void processStats(const ControlList &sensorControls) override;
> +
> +private:
> + void updateExposure(double exposureMSV);
> +
> + SharedFD fdStats_;
> + SharedFD fdParams_;
> + DebayerParams *params_;
> + SwIspStats *stats_;
> +
> + int32_t exposure_min_, exposure_max_;
> + int32_t again_min_, again_max_;
> + int32_t again_, exposure_;
> + unsigned int ignore_updates_;
> +};
> +
> +int IPASoftSimple::init([[maybe_unused]] const IPASettings &settings,
> + const SharedFD &fdStats,
> + const SharedFD &fdParams,
> + const ControlInfoMap &sensorInfoMap)
> +{
> + fdStats_ = fdStats;
As you never use fdStats_ and fdParams_ after this function returns,
there's no need to store a copy.
> + if (!fdStats_.isValid()) {
> + LOG(IPASoft, Error) << "Invalid Statistics handle";
> + return -ENODEV;
> + }
> +
> + fdParams_ = fdParams;
> + if (!fdParams_.isValid()) {
> + LOG(IPASoft, Error) << "Invalid Parameters handle";
> + return -ENODEV;
> + }
> +
> + params_ = static_cast<DebayerParams *>(mmap(nullptr, sizeof(DebayerParams),
> + PROT_WRITE, MAP_SHARED,
> + fdParams_.get(), 0));
> + if (params_ == MAP_FAILED) {
> + LOG(IPASoft, Error) << "Unable to map Parameters";
> + return -errno;
> + }
void *mem = mmap(nullptr, sizeof(DebayerParams), PROT_WRITE, MAP_SHARED,
fdParams_.get(), 0));
if (mem == MAP_FAILED) {
LOG(IPASoft, Error) << "Unable to map Parameters";
return -errno;
}
params_ = static_cast<DebayerParams *>(mem);
> +
> + stats_ = static_cast<SwIspStats *>(mmap(nullptr, sizeof(SwIspStats),
> + PROT_READ, MAP_SHARED,
> + fdStats_.get(), 0));
> + if (stats_ == MAP_FAILED) {
> + LOG(IPASoft, Error) << "Unable to map Statistics";
> + return -errno;
> + }
> +
> + if (sensorInfoMap.find(V4L2_CID_EXPOSURE) == sensorInfoMap.end()) {
> + LOG(IPASoft, Error) << "Don't have exposure control";
> + return -EINVAL;
> + }
> +
> + if (sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN) == sensorInfoMap.end()) {
> + LOG(IPASoft, Error) << "Don't have gain control";
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +int IPASoftSimple::configure(const ControlInfoMap &sensorInfoMap)
> +{
> + const ControlInfo &exposure_info = sensorInfoMap.find(V4L2_CID_EXPOSURE)->second;
s/exposure_info/exposureInfo/
I'll let you address the other snake_case to camelCase conversions in
the series :-)
> + const ControlInfo &gain_info = sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN)->second;
> +
> + exposure_min_ = exposure_info.min().get<int32_t>();
> + exposure_max_ = exposure_info.max().get<int32_t>();
> + if (!exposure_min_) {
> + LOG(IPASoft, Warning) << "Minimum exposure is zero, that can't be linear";
> + exposure_min_ = 1;
> + }
> +
> + again_min_ = gain_info.min().get<int32_t>();
> + again_max_ = gain_info.max().get<int32_t>();
Add a blank line.
> + /*
> + * The camera sensor gain (g) is usually not equal to the value written
> + * into the gain register (x). But the way how the AGC algorithm changes
> + * the gain value to make the total exposure closer to the optimum assumes
> + * that g(x) is not too far from linear function. If the minimal gain is 0,
> + * the g(x) is likely to be far from the linear, like g(x) = a / (b * x + c).
> + * To avoid unexpected changes to the gain by the AGC algorithm (abrupt near
> + * one edge, and very small near the other) we limit the range of the gain
> + * values used.
> + */
> + if (!again_min_) {
> + LOG(IPASoft, Warning) << "Minimum gain is zero, that can't be linear";
> + again_min_ = std::min(100, again_min_ / 2 + again_max_ / 2);
> + }
A patch further in the series addresses this by using
CameraSensorHelper. It should be squashed with this patch.
> +
> + LOG(IPASoft, Info) << "Exposure " << exposure_min_ << "-" << exposure_max_
> + << ", gain " << again_min_ << "-" << again_max_;
> +
> + return 0;
> +}
> +
> +int IPASoftSimple::start()
> +{
> + return 0;
> +}
> +
> +void IPASoftSimple::stop()
> +{
> +}
> +
> +/*
> + * The number of bins to use for the optimal exposure calculations.
> + */
> +static constexpr unsigned int kExposureBinsCount = 5;
Missing blank line. Same below.
> +/*
> + * The exposure is optimal when the mean sample value of the histogram is
> + * in the middle of the range.
> + */
> +static constexpr float kExposureOptimal = kExposureBinsCount / 2.0;
> +/*
> + * The below value implements the hysteresis for the exposure adjustment.
> + * It is small enough to have the exposure close to the optimal, and is big
> + * enough to prevent the exposure from wobbling around the optimal value.
> + */
> +static constexpr float kExposureSatisfactory = 0.2;
Move these to the beginning of the file, just before the IPASoftSimple
definition.
> +
> +void IPASoftSimple::processStats(const ControlList &sensorControls)
> +{
> + /*
> + * Calculate red and blue gains for AWB.
> + * Clamp max gain at 4.0, this also avoids 0 division.
> + */
> + if (stats_->sumR_ <= stats_->sumG_ / 4)
> + params_->gainR = 1024;
> + else
> + params_->gainR = 256 * stats_->sumG_ / stats_->sumR_;
> +
> + if (stats_->sumB_ <= stats_->sumG_ / 4)
> + params_->gainB = 1024;
> + else
> + params_->gainB = 256 * stats_->sumG_ / stats_->sumB_;
> +
> + /* Green gain and gamma values are fixed */
> + params_->gainG = 256;
> + params_->gamma = 0.5;
> +
> + setIspParams.emit(0);
Do you envision switching to the libipa/algorithm.h API at some point ?
If so, it would be nice to record it somewhere.
> +
> + /*
> + * AE / AGC, use 2 frames delay to make sure that the exposure and
> + * the gain set have applied to the camera sensor.
> + */
> + if (ignore_updates_ > 0) {
> + --ignore_updates_;
> + return;
> + }
Also something that could be improved and that should be recorded
somewhere.
> +
> + /*
> + * Calculate Mean Sample Value (MSV) according to formula from:
> + * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf
> + */
> + constexpr unsigned int yHistValsPerBin =
> + SwIspStats::kYHistogramSize / kExposureBinsCount;
> + constexpr unsigned int yHistValsPerBinMod =
> + SwIspStats::kYHistogramSize /
> + (SwIspStats::kYHistogramSize % kExposureBinsCount + 1);
> + int ExposureBins[kExposureBinsCount] = {};
s/ExposureBins/exposureBins/
> + unsigned int denom = 0;
> + unsigned int num = 0;
> +
> + for (unsigned int i = 0; i < SwIspStats::kYHistogramSize; i++) {
> + unsigned int idx = (i - (i / yHistValsPerBinMod)) / yHistValsPerBin;
> + ExposureBins[idx] += stats_->yHistogram[i];
> + }
> +
> + for (unsigned int i = 0; i < kExposureBinsCount; i++) {
> + LOG(IPASoft, Debug) << i << ": " << ExposureBins[i];
> + denom += ExposureBins[i];
> + num += ExposureBins[i] * (i + 1);
> + }
> +
> + float exposureMSV = (float)num / denom;
C++ casts.
> +
> + /* sanity check */
s/sanity/Sanity/
> + if (!sensorControls.contains(V4L2_CID_EXPOSURE) ||
> + !sensorControls.contains(V4L2_CID_ANALOGUE_GAIN)) {
> + LOG(IPASoft, Error) << "Control(s) missing";
> + return;
> + }
> +
> + ControlList ctrls(sensorControls);
You shouldn't copy the control list, but instead create one from the
ControlInfoMap that you get in init() (and that you then need to stored
in the IPA class).
> +
> + exposure_ = ctrls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> + again_ = ctrls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
exposure_ = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
again_ = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
> +
> + updateExposure(exposureMSV);
> +
> + ctrls.set(V4L2_CID_EXPOSURE, exposure_);
> + ctrls.set(V4L2_CID_ANALOGUE_GAIN, again_);
> +
> + ignore_updates_ = 2;
> +
> + setSensorControls.emit(ctrls);
> +
> + LOG(IPASoft, Debug) << "exposureMSV " << exposureMSV
> + << " exp " << exposure_ << " again " << again_
> + << " gain R/B " << params_->gainR << "/" << params_->gainB;
> +}
> +
> +void IPASoftSimple::updateExposure(double exposureMSV)
> +{
> + /* DENOMINATOR of 10 gives ~10% increment/decrement; DENOMINATOR of 5 - about ~20% */
Line wrap, and the DENOMINATOR needs to be renamed to kExpDenominator.
> + static constexpr uint8_t kExpDenominator = 10;
> + static constexpr uint8_t kExpNumeratorUp = kExpDenominator + 1;
> + static constexpr uint8_t kExpNumeratorDown = kExpDenominator - 1;
> +
> + int next;
> +
> + if (exposureMSV < kExposureOptimal - kExposureSatisfactory) {
> + next = exposure_ * kExpNumeratorUp / kExpDenominator;
> + if (next - exposure_ < 1)
> + exposure_ += 1;
> + else
> + exposure_ = next;
> + if (exposure_ >= exposure_max_) {
> + next = again_ * kExpNumeratorUp / kExpDenominator;
> + if (next - again_ < 1)
> + again_ += 1;
> + else
> + again_ = next;
> + }
> + }
> +
> + if (exposureMSV > kExposureOptimal + kExposureSatisfactory) {
> + if (exposure_ == exposure_max_ && again_ != again_min_) {
> + next = again_ * kExpNumeratorDown / kExpDenominator;
> + if (again_ - next < 1)
> + again_ -= 1;
> + else
> + again_ = next;
> + } else {
> + next = exposure_ * kExpNumeratorDown / kExpDenominator;
> + if (exposure_ - next < 1)
> + exposure_ -= 1;
> + else
> + exposure_ = next;
> + }
> + }
> +
> + exposure_ = std::clamp(exposure_, exposure_min_, exposure_max_);
> + again_ = std::clamp(again_, again_min_, again_max_);
> +}
> +
> +} /* namespace ipa::soft */
> +
> +/*
> + * External IPA module interface
> + */
> +extern "C" {
> +const struct IPAModuleInfo ipaModuleInfo = {
> + IPA_MODULE_API_VERSION,
> + 0,
> + "SimplePipelineHandler",
> + "simple",
> +};
> +
> +IPAInterface *ipaCreate()
> +{
> + return new ipa::soft::IPASoftSimple();
> +}
> +
> +} /* extern "C" */
> +
> +} /* namespace libcamera */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list