[PATCH v6 09/18] libcamera: ipa: add Soft IPA
Milan Zamazal
mzamazal at redhat.com
Wed Apr 3 10:53:45 CEST 2024
Andrei Konovalov <andrey.konovalov.ynk at gmail.com> writes:
> Hi Laurent and Milan,
>
> On 28.03.2024 01:36, Andrei Konovalov wrote:
>> Hi Laurent,
>> Thank you for the review!
>> On 27.03.2024 19:38, Laurent Pinchart wrote:
>>> Hi Milan and Andrey,
>>>
>>> Thank you for the patch.
>>>
>>> On Tue, Mar 19, 2024 at 01:35:56PM +0100, Milan Zamazal wrote:
>>>> From: Andrey Konovalov <andrey.konovalov 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.
>> Ack
>>
>>>> Auto exposure/gain and AWB implementation by Dennis, Toon and Martti.
>>>>
>>>> Auto exposure/gain targets a Mean Sample Value of 2.5 following
>>>> the MSV calculation algorithm from:
>>>> https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf
>>>>
>>>> Tested-by: Bryan O'Donoghue <bryan.odonoghue 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.
>> Yes, because, well... all the other IPAs were doing that...
>>
>>> It doesn't have to be done before merging, but could you
>>> address this sooner than later ?
>> I can't promise a lot, but I'll look into that.
>>
>>>> + */
>>>> +
>>>> +module ipa.soft;
>>>> +
>>>> +import "include/libcamera/ipa/core.mojom";
>>>> +
>>>> +interface IPASoftInterface {
>>>> + init(libcamera.IPASettings settings,
>>>> + libcamera.SharedFD fdStats,
>>>> + libcamera.SharedFD fdParams,
>>>
>>> The IPA and soft ISP run in separate threads. How do you guarantee that
>>> the stats and params are not accessed concurrently by both ?
>> I tried avoiding using a pool of buffers for stats and params, and used just a
>> single chunk of shared memory to pass the stats from soft ISP (its worker thread)
>> to IPA, and the other chunk for params.
>> The soft ISP accumulates the stats in its member variable, and after all the stats
>> for the frame are calculated, the member variable is copyed to the shared memory and
>> statReady signal is emitted (so using the member var implements some kind of double
>> buffering). This way the write to the stats shared memory happens once per frame and
>> is short in time right before emitting the statReady signal.
>> The callback executed on receiving statReady signal invokes the IPA's processStats
>> function. So the IPA's processStats() is called soon after the statReady is emitted.
>> If the IPA finish processing the stats before the next frame is processed by the soft IPS
>> and the stats for the next frame are written into the shared memory, there is no concurrent
>> acess.
>> This doesn't strictly guarantee that the stats are not accessed concurrently by the IPA and
>> soft ISP. But unless the video stream from the camera sensor overloads the system this
>> scheme works OK. And if the load is high the concurrent access isn't impossible, and can harm
>> the image quality, but doesn't lead to critical problems like invalid pointers as the stats
>> structure only contains numbers and an array of fixed size.
>>
>>> Shouldn't
>>> you have a pool of buffers for each, mimicking what is done with
>>> hardware ISPs ?
>> This would definitely work, but is a more heavy wait implementation.
>> On the other side, the current implementation is less universal (isn't very scalable), more fragile,
>> and can't reliably associate the stats and the params with the given frame from the camera sensor.
>> So let me try to implement what you suggested ("a pool of buffers for each, mimicking what is done with
>> hardware ISPs").
>
> I won't be able to do this particular part by tomorrow...
> So a branch I plan to publish tomorrow - with this patch updated to address the review comments
> and the [PATCH v6 18/18] "libcamera: Soft IPA: use CameraSensorHelper for analogue gain" merged
> into the earlier ones - would not use the pool of buffers for the stats and the params (yet).
Hi Andrei,
I already made those easier parts, see
https://gitlab.freedesktop.org/mzamazal/libcamera-softisp/-/commits/SoftwareISP-v10?ref_type=heads
(not ready for review yet).
Sorry for misunderstanding and duplicate work.
Regards,
Milan
> Thanks,
> Andrei
>
>>>> + libcamera.ControlInfoMap sensorCtrlInfoMap)
>>>> + => (int32 ret);
>>>> + start() => (int32 ret);
>>>> + stop();
>>>> + configure(libcamera.ControlInfoMap sensorCtrlInfoMap)
>>>> + => (int32 ret);
>>>> +
>>>> + [async] processStats(libcamera.ControlList sensorControls);
>>>> +};
>>>> +
>>>> +interface IPASoftEventInterface {
>>>> + setSensorControls(libcamera.ControlList sensorControls);
>>>> + setIspParams(int32 dummy);
>>>
>>> Drop the dummy value.
>> libcamera docs do allow signals with zero parameters.
>> But when I tried having zero parameters for an EventInterface function,
>> it didn't work for me iirc.
>> Let me double check.
>>
>>>> +};
>>>> diff --git a/meson_options.txt b/meson_options.txt
>>>> index 99dab96d..2644bef0 100644
>>>> --- a/meson_options.txt
>>>> +++ b/meson_options.txt
>>>> @@ -27,7 +27,7 @@ option('gstreamer',
>>>> option('ipas',
>>>> type : 'array',
>>>> - choices : ['ipu3', 'rkisp1', 'rpi/vc4', 'vimc'],
>>>> + choices : ['ipu3', 'rkisp1', 'rpi/vc4', 'simple', 'vimc'],
>>>> description : 'Select which IPA modules to build')
>>>> option('lc-compliance',
>>>> diff --git a/src/ipa/simple/data/meson.build b/src/ipa/simple/data/meson.build
>>>> new file mode 100644
>>>> index 00000000..33548cc6
>>>> --- /dev/null
>>>> +++ b/src/ipa/simple/data/meson.build
>>>> @@ -0,0 +1,9 @@
>>>> +# SPDX-License-Identifier: CC0-1.0
>>>> +
>>>> +conf_files = files([
>>>> + 'soft.conf',
>>>> +])
>>>> +
>>>> +install_data(conf_files,
>>>> + install_dir : ipa_data_dir / 'soft',
>>>> + install_tag : 'runtime')
>>>> diff --git a/src/ipa/simple/data/soft.conf b/src/ipa/simple/data/soft.conf
>>>> new file mode 100644
>>>> index 00000000..0c70e7c0
>>>> --- /dev/null
>>>> +++ b/src/ipa/simple/data/soft.conf
>>>
>>> We use YAML files for all IPAs, could you do the same here ? It
>>> shouldn't be much extra work as the file is empty :-)
>> OK
>>
>>>> @@ -0,0 +1,3 @@
>>>> +# SPDX-License-Identifier: LGPL-2.1-or-later
>>>> +#
>>>> +# Dummy configuration file for the soft IPA.
>>>> diff --git a/src/ipa/simple/meson.build b/src/ipa/simple/meson.build
>>>> new file mode 100644
>>>> index 00000000..3e863db7
>>>> --- /dev/null
>>>> +++ b/src/ipa/simple/meson.build
>>>> @@ -0,0 +1,25 @@
>>>> +# SPDX-License-Identifier: CC0-1.0
>>>> +
>>>> +ipa_name = 'ipa_soft_simple'
>>>> +
>>>> +mod = shared_module(ipa_name,
>>>> + ['soft_simple.cpp', libcamera_generated_ipa_headers],
>>>> + name_prefix : '',
>>>> + include_directories : [ipa_includes, libipa_includes],
>>>> + dependencies : libcamera_private,
>>>> + link_with : libipa,
>>>> + install : true,
>>>> + install_dir : ipa_install_dir)
>>>> +
>>>> +if ipa_sign_module
>>>> + custom_target(ipa_name + '.so.sign',
>>>> + input : mod,
>>>> + output : ipa_name + '.so.sign',
>>>> + command : [ipa_sign, ipa_priv_key, '@INPUT@', '@OUTPUT@'],
>>>> + install : false,
>>>> + build_by_default : true)
>>>> +endif
>>>> +
>>>> +subdir('data')
>>>> +
>>>> +ipa_names += ipa_name
>>>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
>>>> new file mode 100644
>>>> index 00000000..312df4ba
>>>> --- /dev/null
>>>> +++ b/src/ipa/simple/soft_simple.cpp
>>>> @@ -0,0 +1,326 @@
>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>>> +/*
>>>> + * Copyright (C) 2023, Linaro Ltd
>>>> + *
>>>> + * soft_simple.cpp - Simple Software Image Processing Algorithm module
>>>> + */
>>>> +
>>>> +#include <sys/mman.h>
>>>> +
>>>> +#include <libcamera/base/file.h>
>>>> +#include <libcamera/base/log.h>
>>>> +#include <libcamera/base/shared_fd.h>
>>>> +
>>>> +#include <libcamera/control_ids.h>
>>>> +#include <libcamera/controls.h>
>>>> +
>>>> +#include <libcamera/ipa/ipa_interface.h>
>>>> +#include <libcamera/ipa/ipa_module_info.h>
>>>> +#include <libcamera/ipa/soft_ipa_interface.h>
>>>> +
>>>> +#include "libcamera/internal/camera_sensor.h"
>>>
>>> Not needed.
>> OK
>>
>>>> +#include "libcamera/internal/software_isp/debayer_params.h"
>>>> +#include "libcamera/internal/software_isp/swisp_stats.h"
>>>> +
>>>> +namespace libcamera {
>>>> +
>>>> +LOG_DEFINE_CATEGORY(IPASoft)
>>>> +
>>>> +namespace ipa::soft {
>>>> +
>>>> +class IPASoftSimple : public ipa::soft::IPASoftInterface
>>>> +{
>>>> +public:
>>>> + IPASoftSimple()
>>>> + : params_(static_cast<DebayerParams *>(MAP_FAILED)),
>>>
>>> Initialize this to nullptr, that's more standard, and will make the
>>> tests in the destructor nicer. init() will have to set the pointers to
>>> null if mmap() calls fail.
>> OK
>>
>>>> + stats_(static_cast<SwIspStats *>(MAP_FAILED)), ignore_updates_(0)
>>>
>>> s/ignore_updates_/ignoreUpdates_/
>> OK
>>
>>>> + {
>>>> + }
>>>> +
>>>> + ~IPASoftSimple()
>>>> + {
>>>> + if (stats_ != MAP_FAILED)
>>>> + munmap(stats_, sizeof(SwIspStats));
>>>> + if (params_ != MAP_FAILED)
>>>> + munmap(params_, sizeof(DebayerParams));
>>>> + }
>>>
>>> No need to inline this.
>> OK
>>
>>>> +
>>>> + int init(const IPASettings &settings,
>>>> + const SharedFD &fdStats,
>>>> + const SharedFD &fdParams,
>>>> + const ControlInfoMap &sensorInfoMap) override;
>>>> + int configure(const ControlInfoMap &sensorInfoMap) override;
>>>> +
>>>> + int start() override;
>>>> + void stop() override;
>>>> +
>>>> + void processStats(const ControlList &sensorControls) override;
>>>> +
>>>> +private:
>>>> + void updateExposure(double exposureMSV);
>>>> +
>>>> + SharedFD fdStats_;
>>>> + SharedFD fdParams_;
>>>> + DebayerParams *params_;
>>>> + SwIspStats *stats_;
>>>> +
>>>> + int32_t exposure_min_, exposure_max_;
>>>> + int32_t again_min_, again_max_;
>>>> + int32_t again_, exposure_;
>>>> + unsigned int ignore_updates_;
>>>> +};
>>>> +
>>>> +int IPASoftSimple::init([[maybe_unused]] const IPASettings &settings,
>>>> + const SharedFD &fdStats,
>>>> + const SharedFD &fdParams,
>>>> + const ControlInfoMap &sensorInfoMap)
>>>> +{
>>>> + fdStats_ = fdStats;
>>>
>>> As you never use fdStats_ and fdParams_ after this function returns,
>>> there's no need to store a copy.
>> Good catch. OK
>>
>>>> + if (!fdStats_.isValid()) {
>>>> + LOG(IPASoft, Error) << "Invalid Statistics handle";
>>>> + return -ENODEV;
>>>> + }
>>>> +
>>>> + fdParams_ = fdParams;
>>>> + if (!fdParams_.isValid()) {
>>>> + LOG(IPASoft, Error) << "Invalid Parameters handle";
>>>> + return -ENODEV;
>>>> + }
>>>> +
>>>> + params_ = static_cast<DebayerParams *>(mmap(nullptr, sizeof(DebayerParams),
>>>> + PROT_WRITE, MAP_SHARED,
>>>> + fdParams_.get(), 0));
>>>> + if (params_ == MAP_FAILED) {
>>>> + LOG(IPASoft, Error) << "Unable to map Parameters";
>>>> + return -errno;
>>>> + }
>>>
>>> void *mem = mmap(nullptr, sizeof(DebayerParams), PROT_WRITE, MAP_SHARED,
>>> fdParams_.get(), 0));
>>> if (mem == MAP_FAILED) {
>>> LOG(IPASoft, Error) << "Unable to map Parameters";
>>> return -errno;
>>> }
>>>
>>> params_ = static_cast<DebayerParams *>(mem);
>> OK
>>
>>>> +
>>>> + stats_ = static_cast<SwIspStats *>(mmap(nullptr, sizeof(SwIspStats),
>>>> + PROT_READ, MAP_SHARED,
>>>> + fdStats_.get(), 0));
>>>> + if (stats_ == MAP_FAILED) {
>>>> + LOG(IPASoft, Error) << "Unable to map Statistics";
>>>> + return -errno;
>>>> + }
>>>> +
>>>> + if (sensorInfoMap.find(V4L2_CID_EXPOSURE) == sensorInfoMap.end()) {
>>>> + LOG(IPASoft, Error) << "Don't have exposure control";
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + if (sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN) == sensorInfoMap.end()) {
>>>> + LOG(IPASoft, Error) << "Don't have gain control";
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +int IPASoftSimple::configure(const ControlInfoMap &sensorInfoMap)
>>>> +{
>>>> + const ControlInfo &exposure_info = sensorInfoMap.find(V4L2_CID_EXPOSURE)->second;
>>>
>>> s/exposure_info/exposureInfo/
>>>
>>> I'll let you address the other snake_case to camelCase conversions in
>>> the series :-)
>> OK
>>
>>>> + const ControlInfo &gain_info = sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN)->second;
>>>> +
>>>> + exposure_min_ = exposure_info.min().get<int32_t>();
>>>> + exposure_max_ = exposure_info.max().get<int32_t>();
>>>> + if (!exposure_min_) {
>>>> + LOG(IPASoft, Warning) << "Minimum exposure is zero, that can't be linear";
>>>> + exposure_min_ = 1;
>>>> + }
>>>> +
>>>> + again_min_ = gain_info.min().get<int32_t>();
>>>> + again_max_ = gain_info.max().get<int32_t>();
>>>
>>> Add a blank line.
>> OK
>>
>>>> + /*
>>>> + * The camera sensor gain (g) is usually not equal to the value written
>>>> + * into the gain register (x). But the way how the AGC algorithm changes
>>>> + * the gain value to make the total exposure closer to the optimum assumes
>>>> + * that g(x) is not too far from linear function. If the minimal gain is 0,
>>>> + * the g(x) is likely to be far from the linear, like g(x) = a / (b * x + c).
>>>> + * To avoid unexpected changes to the gain by the AGC algorithm (abrupt near
>>>> + * one edge, and very small near the other) we limit the range of the gain
>>>> + * values used.
>>>> + */
>>>> + if (!again_min_) {
>>>> + LOG(IPASoft, Warning) << "Minimum gain is zero, that can't be linear";
>>>> + again_min_ = std::min(100, again_min_ / 2 + again_max_ / 2);
>>>> + }
>>>
>>> A patch further in the series addresses this by using
>>> CameraSensorHelper. It should be squashed with this patch.
>> OK
>>
>>>> +
>>>> + LOG(IPASoft, Info) << "Exposure " << exposure_min_ << "-" << exposure_max_
>>>> + << ", gain " << again_min_ << "-" << again_max_;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +int IPASoftSimple::start()
>>>> +{
>>>> + return 0;
>>>> +}
>>>> +
>>>> +void IPASoftSimple::stop()
>>>> +{
>>>> +}
>>>> +
>>>> +/*
>>>> + * The number of bins to use for the optimal exposure calculations.
>>>> + */
>>>> +static constexpr unsigned int kExposureBinsCount = 5;
>>>
>>> Missing blank line. Same below.
>> OK
>>
>>>> +/*
>>>> + * The exposure is optimal when the mean sample value of the histogram is
>>>> + * in the middle of the range.
>>>> + */
>>>> +static constexpr float kExposureOptimal = kExposureBinsCount / 2.0;
>>>> +/*
>>>> + * The below value implements the hysteresis for the exposure adjustment.
>>>> + * It is small enough to have the exposure close to the optimal, and is big
>>>> + * enough to prevent the exposure from wobbling around the optimal value.
>>>> + */
>>>> +static constexpr float kExposureSatisfactory = 0.2;
>>>
>>> Move these to the beginning of the file, just before the IPASoftSimple
>>> definition.
>> OK
>>
>>>> +
>>>> +void IPASoftSimple::processStats(const ControlList &sensorControls)
>>>> +{
>>>> + /*
>>>> + * Calculate red and blue gains for AWB.
>>>> + * Clamp max gain at 4.0, this also avoids 0 division.
>>>> + */
>>>> + if (stats_->sumR_ <= stats_->sumG_ / 4)
>>>> + params_->gainR = 1024;
>>>> + else
>>>> + params_->gainR = 256 * stats_->sumG_ / stats_->sumR_;
>>>> +
>>>> + if (stats_->sumB_ <= stats_->sumG_ / 4)
>>>> + params_->gainB = 1024;
>>>> + else
>>>> + params_->gainB = 256 * stats_->sumG_ / stats_->sumB_;
>>>> +
>>>> + /* Green gain and gamma values are fixed */
>>>> + params_->gainG = 256;
>>>> + params_->gamma = 0.5;
>>>> +
>>>> + setIspParams.emit(0);
>>>
>>> Do you envision switching to the libipa/algorithm.h API at some point ?
>>> If so, it would be nice to record it somewhere.
>> At some point, yes.
>> Will mention that.
>>
>>>> +
>>>> + /*
>>>> + * AE / AGC, use 2 frames delay to make sure that the exposure and
>>>> + * the gain set have applied to the camera sensor.
>>>> + */
>>>> + if (ignore_updates_ > 0) {
>>>> + --ignore_updates_;
>>>> + return;
>>>> + }
>>>
>>> Also something that could be improved and that should be recorded
>>> somewhere.
>> OK
>>
>>>> +
>>>> + /*
>>>> + * Calculate Mean Sample Value (MSV) according to formula from:
>>>> + * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf
>>>> + */
>>>> + constexpr unsigned int yHistValsPerBin =
>>>> + SwIspStats::kYHistogramSize / kExposureBinsCount;
>>>> + constexpr unsigned int yHistValsPerBinMod =
>>>> + SwIspStats::kYHistogramSize /
>>>> + (SwIspStats::kYHistogramSize % kExposureBinsCount + 1);
>>>> + int ExposureBins[kExposureBinsCount] = {};
>>>
>>> s/ExposureBins/exposureBins/
>> OK
>>
>>>> + unsigned int denom = 0;
>>>> + unsigned int num = 0;
>>>> +
>>>> + for (unsigned int i = 0; i < SwIspStats::kYHistogramSize; i++) {
>>>> + unsigned int idx = (i - (i / yHistValsPerBinMod)) / yHistValsPerBin;
>>>> + ExposureBins[idx] += stats_->yHistogram[i];
>>>> + }
>>>> +
>>>> + for (unsigned int i = 0; i < kExposureBinsCount; i++) {
>>>> + LOG(IPASoft, Debug) << i << ": " << ExposureBins[i];
>>>> + denom += ExposureBins[i];
>>>> + num += ExposureBins[i] * (i + 1);
>>>> + }
>>>> +
>>>> + float exposureMSV = (float)num / denom;
>>>
>>> C++ casts.
>> OK
>>
>>>> +
>>>> + /* sanity check */
>>>
>>> s/sanity/Sanity/
>> OK
>>
>>>> + if (!sensorControls.contains(V4L2_CID_EXPOSURE) ||
>>>> + !sensorControls.contains(V4L2_CID_ANALOGUE_GAIN)) {
>>>> + LOG(IPASoft, Error) << "Control(s) missing";
>>>> + return;
>>>> + }
>>>> +
>>>> + ControlList ctrls(sensorControls);
>>>
>>> You shouldn't copy the control list, but instead create one from the
>>> ControlInfoMap that you get in init() (and that you then need to stored
>>> in the IPA class).
>> OK, thanks
>>
>>>> +
>>>> + exposure_ = ctrls.get(V4L2_CID_EXPOSURE).get<int32_t>();
>>>> + again_ = ctrls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
>>>
>>> exposure_ = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
>>> again_ = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
>> OK
>>
>>>> +
>>>> + updateExposure(exposureMSV);
>>>> +
>>>> + ctrls.set(V4L2_CID_EXPOSURE, exposure_);
>>>> + ctrls.set(V4L2_CID_ANALOGUE_GAIN, again_);
>>>> +
>>>> + ignore_updates_ = 2;
>>>> +
>>>> + setSensorControls.emit(ctrls);
>>>> +
>>>> + LOG(IPASoft, Debug) << "exposureMSV " << exposureMSV
>>>> + << " exp " << exposure_ << " again " << again_
>>>> + << " gain R/B " << params_->gainR << "/" << params_->gainB;
>>>> +}
>>>> +
>>>> +void IPASoftSimple::updateExposure(double exposureMSV)
>>>> +{
>>>> + /* DENOMINATOR of 10 gives ~10% increment/decrement; DENOMINATOR of 5 - about ~20% */
>>>
>>> Line wrap, and the DENOMINATOR needs to be renamed to kExpDenominator.
>> OK
>> Thanks for the review,
>> Andrei
>>
>>>> + static constexpr uint8_t kExpDenominator = 10;
>>>> + static constexpr uint8_t kExpNumeratorUp = kExpDenominator + 1;
>>>> + static constexpr uint8_t kExpNumeratorDown = kExpDenominator - 1;
>>>> +
>>>> + int next;
>>>> +
>>>> + if (exposureMSV < kExposureOptimal - kExposureSatisfactory) {
>>>> + next = exposure_ * kExpNumeratorUp / kExpDenominator;
>>>> + if (next - exposure_ < 1)
>>>> + exposure_ += 1;
>>>> + else
>>>> + exposure_ = next;
>>>> + if (exposure_ >= exposure_max_) {
>>>> + next = again_ * kExpNumeratorUp / kExpDenominator;
>>>> + if (next - again_ < 1)
>>>> + again_ += 1;
>>>> + else
>>>> + again_ = next;
>>>> + }
>>>> + }
>>>> +
>>>> + if (exposureMSV > kExposureOptimal + kExposureSatisfactory) {
>>>> + if (exposure_ == exposure_max_ && again_ != again_min_) {
>>>> + next = again_ * kExpNumeratorDown / kExpDenominator;
>>>> + if (again_ - next < 1)
>>>> + again_ -= 1;
>>>> + else
>>>> + again_ = next;
>>>> + } else {
>>>> + next = exposure_ * kExpNumeratorDown / kExpDenominator;
>>>> + if (exposure_ - next < 1)
>>>> + exposure_ -= 1;
>>>> + else
>>>> + exposure_ = next;
>>>> + }
>>>> + }
>>>> +
>>>> + exposure_ = std::clamp(exposure_, exposure_min_, exposure_max_);
>>>> + again_ = std::clamp(again_, again_min_, again_max_);
>>>> +}
>>>> +
>>>> +} /* namespace ipa::soft */
>>>> +
>>>> +/*
>>>> + * External IPA module interface
>>>> + */
>>>> +extern "C" {
>>>> +const struct IPAModuleInfo ipaModuleInfo = {
>>>> + IPA_MODULE_API_VERSION,
>>>> + 0,
>>>> + "SimplePipelineHandler",
>>>> + "simple",
>>>> +};
>>>> +
>>>> +IPAInterface *ipaCreate()
>>>> +{
>>>> + return new ipa::soft::IPASoftSimple();
>>>> +}
>>>> +
>>>> +} /* extern "C" */
>>>> +
>>>> +} /* namespace libcamera */
>>>
More information about the libcamera-devel
mailing list