From patchwork Thu Apr 4 08:46:47 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Milan Zamazal X-Patchwork-Id: 19830 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id C4F03C32CE for ; Thu, 4 Apr 2024 08:47:50 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 51F246338A; Thu, 4 Apr 2024 10:47:50 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.b="TgEnTONT"; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 077556338F for ; Thu, 4 Apr 2024 10:47:47 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1712220466; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=qbpxnGBdpSyqNezYdl+6Vod+n+Xxvs94M+E0KM43dAM=; b=TgEnTONTtDA1XI7DUEKLhTl4kqHnASF4ZdKk8ahpt2Nf52Pvxq6DqpOfgSuPDX+auNYmx4 KFramhSUipX9u5QG9Cylocn7a46zYmfKbS0oJL/APlA2bScR3MNOggTpITat3mZfKXKrUU ssugZQHUQvMguBIlbFUmI5NIdekVJ+g= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-79-wk8TaXLUPce7smQ4usoPCw-1; Thu, 04 Apr 2024 04:47:43 -0400 X-MC-Unique: wk8TaXLUPce7smQ4usoPCw-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 98F50101A562; Thu, 4 Apr 2024 08:47:42 +0000 (UTC) Received: from nuthatch.brq.redhat.com (unknown [10.43.17.39]) by smtp.corp.redhat.com (Postfix) with ESMTP id E426E8173; Thu, 4 Apr 2024 08:47:40 +0000 (UTC) From: Milan Zamazal To: libcamera-devel@lists.libcamera.org Cc: Andrey Konovalov , Andrei Konovalov , Bryan O'Donoghue , Maxime Ripard , Milan Zamazal , Pavel Machek , Hans de Goede , Kieran Bingham , Laurent Pinchart , Dennis Bonke , Marttico , Toon Langendam Subject: [PATCH v7 10/18] libcamera: ipa: add Soft IPA Date: Thu, 4 Apr 2024 10:46:47 +0200 Message-ID: <20240404084657.353464-11-mzamazal@redhat.com> In-Reply-To: <20240404084657.353464-1-mzamazal@redhat.com> References: <20240404084657.353464-1-mzamazal@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.5 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" From: Andrey Konovalov 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 Use CameraSensorHelper to convert the analogue gain code read from the camera sensor into real analogue gain value. In the future this makes it possible to use faster AE/AGC algorithm. Right now the CameraSensorHelper lets us use the full range of analogue gain values. If there is no CameraSensorHelper for the camera sensor in use, a warning log message is printed. Tested-by: Bryan O'Donoghue # sc8280xp Lenovo x13s Tested-by: Pavel Machek Reviewed-by: Pavel Machek Signed-off-by: Andrey Konovalov Co-developed-by: Dennis Bonke Signed-off-by: Dennis Bonke Co-developed-by: Marttico Signed-off-by: Marttico Co-developed-by: Toon Langendam Signed-off-by: Toon Langendam Signed-off-by: Hans de Goede --- Documentation/Doxyfile.in | 1 + include/libcamera/ipa/meson.build | 1 + include/libcamera/ipa/soft.mojom | 28 ++ meson_options.txt | 2 +- src/ipa/meson.build | 3 + src/ipa/simple/data/meson.build | 10 + src/ipa/simple/data/uncalibrated.yaml | 5 + src/ipa/simple/meson.build | 25 ++ src/ipa/simple/soft_simple.cpp | 393 ++++++++++++++++++++++++++ src/libcamera/software_isp/TODO | 83 ++++++ 10 files changed, 550 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/uncalibrated.yaml 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 7c4f6d3a..c61eb555 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/meson.build b/src/ipa/meson.build index 48793e07..0ad4631d 100644 --- a/src/ipa/meson.build +++ b/src/ipa/meson.build @@ -41,6 +41,9 @@ ipa_names = [] subdirs = [] foreach pipeline : pipelines + # The current implementation expects the IPA module name to match the + # pipeline name. + # \todo Make the IPA naming scheme more flexible. if not ipa_modules.contains(pipeline) continue endif diff --git a/src/ipa/simple/data/meson.build b/src/ipa/simple/data/meson.build new file mode 100644 index 00000000..92795ee4 --- /dev/null +++ b/src/ipa/simple/data/meson.build @@ -0,0 +1,10 @@ +# SPDX-License-Identifier: CC0-1.0 + +conf_files = files([ + 'uncalibrated.yaml', +]) + +# The install_dir must match the name from the IPAModuleInfo +install_data(conf_files, + install_dir : ipa_data_dir / 'simple', + install_tag : 'runtime') diff --git a/src/ipa/simple/data/uncalibrated.yaml b/src/ipa/simple/data/uncalibrated.yaml new file mode 100644 index 00000000..ff981a1a --- /dev/null +++ b/src/ipa/simple/data/uncalibrated.yaml @@ -0,0 +1,5 @@ +# SPDX-License-Identifier: CC0-1.0 +%YAML 1.1 +--- +version: 1 +... 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..196b9c57 --- /dev/null +++ b/src/ipa/simple/soft_simple.cpp @@ -0,0 +1,393 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2023, Linaro Ltd + * + * soft_simple.cpp - Simple Software Image Processing Algorithm module + */ + +#include + +#include + +#include +#include +#include + +#include +#include + +#include +#include +#include + +#include "libcamera/internal/software_isp/debayer_params.h" +#include "libcamera/internal/software_isp/swisp_stats.h" +#include "libcamera/internal/yaml_parser.h" + +#include "libipa/camera_sensor_helper.h" + +namespace libcamera { + +LOG_DEFINE_CATEGORY(IPASoft) + +namespace ipa::soft { + +/* + * The number of bins to use for the optimal exposure calculations. + */ +static constexpr unsigned int kExposureBinsCount = 5; + +/* + * 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; + +class IPASoftSimple : public ipa::soft::IPASoftInterface +{ +public: + IPASoftSimple() + : params_(nullptr), stats_(nullptr), ignoreUpdates_(0) + { + } + + ~IPASoftSimple(); + + 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); + + DebayerParams *params_; + SwIspStats *stats_; + std::unique_ptr camHelper_; + ControlInfoMap sensorInfoMap_; + + int32_t exposureMin_, exposureMax_; + int32_t exposure_; + double againMin_, againMax_, againMinStep_; + double again_; + unsigned int ignoreUpdates_; +}; + +IPASoftSimple::~IPASoftSimple() +{ + if (stats_) + munmap(stats_, sizeof(SwIspStats)); + if (params_) + munmap(params_, sizeof(DebayerParams)); +} + +int IPASoftSimple::init(const IPASettings &settings, + const SharedFD &fdStats, + const SharedFD &fdParams, + const ControlInfoMap &sensorInfoMap) +{ + camHelper_ = CameraSensorHelperFactoryBase::create(settings.sensorModel); + if (!camHelper_) { + LOG(IPASoft, Warning) + << "Failed to create camera sensor helper for " + << settings.sensorModel; + } + + /* Load the tuning data file */ + File file(settings.configurationFile); + if (!file.open(File::OpenModeFlag::ReadOnly)) { + int ret = file.error(); + LOG(IPASoft, Error) + << "Failed to open configuration file " + << settings.configurationFile << ": " << strerror(-ret); + return ret; + } + + std::unique_ptr data = YamlParser::parse(file); + if (!data) + return -EINVAL; + + /* \todo Use the IPA configuration file for real. */ + unsigned int version = (*data)["version"].get(0); + LOG(IPASoft, Debug) << "Tuning file version " << version; + + params_ = nullptr; + stats_ = nullptr; + + if (!fdStats.isValid()) { + LOG(IPASoft, Error) << "Invalid Statistics handle"; + return -ENODEV; + } + + if (!fdParams.isValid()) { + LOG(IPASoft, Error) << "Invalid Parameters handle"; + return -ENODEV; + } + + { + 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(mem); + } + + { + void *mem = mmap(nullptr, sizeof(SwIspStats), PROT_READ, + MAP_SHARED, fdStats.get(), 0); + if (mem == MAP_FAILED) { + LOG(IPASoft, Error) << "Unable to map Statistics"; + return -errno; + } + + stats_ = static_cast(mem); + } + + /* + * Check if the sensor driver supports the controls required by the + * Soft IPA. + * Don't save the min and max control values yet, as e.g. the limits + * for V4L2_CID_EXPOSURE depend on the configured sensor resolution. + */ + 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) +{ + sensorInfoMap_ = sensorInfoMap; + + const ControlInfo &exposureInfo = sensorInfoMap_.find(V4L2_CID_EXPOSURE)->second; + const ControlInfo &gainInfo = sensorInfoMap_.find(V4L2_CID_ANALOGUE_GAIN)->second; + + exposureMin_ = exposureInfo.min().get(); + exposureMax_ = exposureInfo.max().get(); + if (!exposureMin_) { + LOG(IPASoft, Warning) << "Minimum exposure is zero, that can't be linear"; + exposureMin_ = 1; + } + + int32_t againMin = gainInfo.min().get(); + int32_t againMax = gainInfo.max().get(); + + if (camHelper_) { + againMin_ = camHelper_->gain(againMin); + againMax_ = camHelper_->gain(againMax); + againMinStep_ = (againMax_ - againMin_) / 100.0; + } else { + /* + * 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. + */ + againMax_ = againMax; + if (!againMin) { + LOG(IPASoft, Warning) + << "Minimum gain is zero, that can't be linear"; + againMin_ = std::min(100, againMin / 2 + againMax / 2); + } + againMinStep_ = 1.0; + } + + LOG(IPASoft, Info) << "Exposure " << exposureMin_ << "-" << exposureMax_ + << ", gain " << againMin_ << "-" << againMax_ + << " (" << againMinStep_ << ")"; + + 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); + + /* \todo Switch to the libipa/algorithm.h API someday. */ + + /* + * AE / AGC, use 2 frames delay to make sure that the exposure and + * the gain set have applied to the camera sensor. + * \todo This could be handled better with DelayedControls. + */ + if (ignoreUpdates_ > 0) { + --ignoreUpdates_; + return; + } + + /* + * 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] = {}; + 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 = static_cast(num) / denom; + + /* Sanity check */ + if (!sensorControls.contains(V4L2_CID_EXPOSURE) || + !sensorControls.contains(V4L2_CID_ANALOGUE_GAIN)) { + LOG(IPASoft, Error) << "Control(s) missing"; + return; + } + + exposure_ = sensorControls.get(V4L2_CID_EXPOSURE).get(); + int32_t again = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get(); + again_ = camHelper_ ? camHelper_->gain(again) : again; + + updateExposure(exposureMSV); + + ControlList ctrls(sensorInfoMap_); + + ctrls.set(V4L2_CID_EXPOSURE, exposure_); + ctrls.set(V4L2_CID_ANALOGUE_GAIN, + static_cast(camHelper_ ? camHelper_->gainCode(again_) : again_)); + + ignoreUpdates_ = 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) +{ + /* + * kExpDenominator of 10 gives ~10% increment/decrement; + * kExpDenominator of 5 - about ~20% + */ + static constexpr uint8_t kExpDenominator = 10; + static constexpr uint8_t kExpNumeratorUp = kExpDenominator + 1; + static constexpr uint8_t kExpNumeratorDown = kExpDenominator - 1; + + double next; + + if (exposureMSV < kExposureOptimal - kExposureSatisfactory) { + next = exposure_ * kExpNumeratorUp / kExpDenominator; + if (next - exposure_ < 1) + exposure_ += 1; + else + exposure_ = next; + if (exposure_ >= exposureMax_) { + next = again_ * kExpNumeratorUp / kExpDenominator; + if (next - again_ < againMinStep_) + again_ += againMinStep_; + else + again_ = next; + } + } + + if (exposureMSV > kExposureOptimal + kExposureSatisfactory) { + if (exposure_ == exposureMax_ && again_ > againMin_) { + next = again_ * kExpNumeratorDown / kExpDenominator; + if (again_ - next < againMinStep_) + again_ -= againMinStep_; + else + again_ = next; + } else { + next = exposure_ * kExpNumeratorDown / kExpDenominator; + if (exposure_ - next < 1) + exposure_ -= 1; + else + exposure_ = next; + } + } + + exposure_ = std::clamp(exposure_, exposureMin_, exposureMax_); + again_ = std::clamp(again_, againMin_, againMax_); +} + +} /* 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 */ diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO index 29be5386..ae0af25b 100644 --- a/src/libcamera/software_isp/TODO +++ b/src/libcamera/software_isp/TODO @@ -173,3 +173,86 @@ the need for performances and the need for a maintainable architecture. > I think this falls under the lets wait until we have a GPU > based SoftISP MVP/POC and then do some refactoring to see which > bits should go where. + +--- + +8. Decouple pipeline and IPA naming + +> 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. + +--- + +9. Doxyfile cleanup + +>> 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 ? + +--- + +10. Switch to libipa/algorithm.h API in processStats + +>> void IPASoftSimple::processStats(const ControlList &sensorControls) +>> +> Do you envision switching to the libipa/algorithm.h API at some point ? + +At some point, yes. + +--- + +11. Improve handling the sensor controls which take effect with a delay + +> void IPASoftSimple::processStats(const ControlList &sensorControls) +> { +> ... +> /* +> * 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; +> } + +This could be handled better with DelayedControls.