[PATCH v3 09/16] libcamera: ipa: add Soft IPA
Andrei Konovalov
andrey.konovalov.ynk at gmail.com
Tue Feb 20 16:23:02 CET 2024
On 20.02.2024 16:05, Kieran Bingham wrote:
> Quoting Hans de Goede (2024-02-14 17:01:13)
>> 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.
>>
>> 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>
>> ---
>> 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
>>
More information about the libcamera-devel
mailing list