[{"id":28665,"web_url":"https://patchwork.libcamera.org/comment/28665/","msgid":"","date":"2024-02-14T18:15:46","subject":"Re: [PATCH v3 09/16] libcamera: ipa: add Soft IPA","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"Hi\n\n\n2024. február 14., szerda 18:01 keltezéssel, Hans de Goede írta:\n\n> From: Andrey Konovalov \n> \n> Define the Soft IPA main and event interfaces, add the Soft IPA\n> implementation.\n> \n> The current src/ipa/meson.build assumes the IPA name to match the\n> pipeline name. For this reason \"-Dipas=simple\" is used for the\n> Soft IPA module.\n> \n> Auto exposure/gain and AWB implementation by Dennis, Toon and Martti.\n> \n> Auto exposure/gain targets a Mean Sample Value of 2.5 following\n> the MSV calculation algorithm from:\n> https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf\n> \n> Tested-by: Bryan O'Donoghue # sc8280xp Lenovo x13s\n> Tested-by: Pavel Machek \n> Reviewed-by: Pavel Machek \n> Signed-off-by: Andrey Konovalov \n> Co-developed-by: Dennis Bonke \n> Signed-off-by: Dennis Bonke \n> Co-developed-by: Marttico \n> Signed-off-by: Marttico \n> Co-developed-by: Toon Langendam \n> Signed-off-by: Toon Langendam \n> Signed-off-by: Hans de Goede \n> ---\n> Changes in v3:\n> - Use new SwIspStats::kYHistogramSize constexpr and adjust\n> the auto-exposure/-gain code so that it can deal with\n> that having a different value then 16 (modify the loop\n> to divide the histogram in 5 bins to not have hardcoded\n> values)\n> - Rename a few foo_bar symbols to fooBar\n> ---\n> Documentation/Doxyfile.in | 1 +\n> include/libcamera/ipa/meson.build | 1 +\n> include/libcamera/ipa/soft.mojom | 28 +++\n> meson_options.txt | 2 +-\n> src/ipa/simple/data/meson.build | 9 +\n> src/ipa/simple/data/soft.conf | 3 +\n> src/ipa/simple/meson.build | 25 +++\n> src/ipa/simple/soft_simple.cpp | 308 ++++++++++++++++++++++++++++++\n> 8 files changed, 376 insertions(+), 1 deletion(-)\n> create mode 100644 include/libcamera/ipa/soft.mojom\n> create mode 100644 src/ipa/simple/data/meson.build\n> create mode 100644 src/ipa/simple/data/soft.conf\n> create mode 100644 src/ipa/simple/meson.build\n> create mode 100644 src/ipa/simple/soft_simple.cpp\n> \n> [...]\n> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n> new file mode 100644\n> index 00000000..f7ac02f9\n> --- /dev/null\n> +++ b/src/ipa/simple/soft_simple.cpp\n> @@ -0,0 +1,308 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2023, Linaro Ltd\n> + *\n> + * soft_simple.cpp - Simple Software Image Processing Algorithm module\n> + */\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \"libcamera/internal/camera_sensor.h\"\n> +#include \"libcamera/internal/software_isp/debayer_params.h\"\n> +#include \"libcamera/internal/software_isp/swisp_stats.h\"\n> +\n> +#define EXPOSURE_OPTIMAL_VALUE 2.5\n> +#define EXPOSURE_SATISFACTORY_OFFSET 0.2\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(IPASoft)\n> +\n> +namespace ipa::soft {\n> +\n> +class IPASoftSimple : public ipa::soft::IPASoftInterface\n> +{\n> +public:\n> +\tIPASoftSimple()\n> +\t\t: ignore_updates_(0)\n> +\t{\n> +\t}\n> +\n> +\t~IPASoftSimple()\n> +\t{\n> +\t\tif (stats_)\n\nif (stats_ != MAP_FAILED)\n\n\n> +\t\t\tmunmap(stats_, sizeof(SwIspStats));\n> +\t\tif (params_)\n> +\t\t\tmunmap(params_, sizeof(DebayerParams));\n> +\t}\n> +\n> +\tint init(const IPASettings &settings,\n> +\t\t const SharedFD &fdStats,\n> +\t\t const SharedFD &fdParams,\n> +\t\t const ControlInfoMap &sensorInfoMap) override;\n> +\tint configure(const ControlInfoMap &sensorInfoMap) override;\n> +\n> +\tint start() override;\n> +\tvoid stop() override;\n> +\n> +\tvoid processStats(const ControlList &sensorControls) override;\n> +\n> +private:\n> +\tvoid updateExposure(double exposureMSV);\n> +\n> +\tSharedFD fdStats_;\n> +\tSharedFD fdParams_;\n> +\tDebayerParams *params_;\n> +\tSwIspStats *stats_;\n\nThe above two should be initialized to MAP_FAILED.\n\n\n> +\tint exposure_min_, exposure_max_;\n> +\tint again_min_, again_max_;\n> +\tint again_, exposure_;\n> +\tint ignore_updates_;\n> +};\n> +\n> +int IPASoftSimple::init([[maybe_unused]] const IPASettings &settings,\n> +\t\t\tconst SharedFD &fdStats,\n> +\t\t\tconst SharedFD &fdParams,\n> +\t\t\tconst ControlInfoMap &sensorInfoMap)\n> +{\n> +\tfdStats_ = std::move(fdStats);\n\nstd::move(fdStats) has the type `const SharedFD&&`, so `fdStats_ = ...`\nwill result in a call to the copy assignment operator `operator=(const SharedFD&)`.\nSo `std::move()` does not really do anything here, and can be removed.\nAlternatively, if possible, you can change the argument type to be just `SharedFD`,\nand then moving makes sense (preferably in the member initializer list).\n\n\n> +\tif (!fdStats_.isValid()) {\n> +\t\tLOG(IPASoft, Error) << \"Invalid Statistics handle\";\n> +\t\treturn -ENODEV;\n> +\t}\n> +\n> +\tfdParams_ = std::move(fdParams);\n> +\tif (!fdParams_.isValid()) {\n> +\t\tLOG(IPASoft, Error) << \"Invalid Parameters handle\";\n> +\t\treturn -ENODEV;\n> +\t}\n> +\n> +\tparams_ = static_cast(mmap(nullptr, sizeof(DebayerParams),\n> +\t\t\t\t\t\t PROT_WRITE, MAP_SHARED,\n> +\t\t\t\t\t\t fdParams_.get(), 0));\n> +\tif (!params_) {\n\nmmap returns MAP_FAILED on failure.\n\n\n> +\t\tLOG(IPASoft, Error) << \"Unable to map Parameters\";\n> +\t\treturn -ENODEV;\n\nMaybe using `errno` would be better?\n\n\n> +\t}\n> +\n> +\tstats_ = static_cast(mmap(nullptr, sizeof(SwIspStats),\n> +\t\t\t\t\t\tPROT_READ, MAP_SHARED,\n> +\t\t\t\t\t\tfdStats_.get(), 0));\n> +\tif (!stats_) {\n> +\t\tLOG(IPASoft, Error) << \"Unable to map Statistics\";\n> +\t\treturn -ENODEV;\n> +\t}\n> +\n> +\tif (sensorInfoMap.find(V4L2_CID_EXPOSURE) == sensorInfoMap.end()) {\n> +\t\tLOG(IPASoft, Error) << \"Don't have exposure control\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tif (sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN) == sensorInfoMap.end()) {\n> +\t\tLOG(IPASoft, Error) << \"Don't have gain control\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> [...]","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3971EBDE17\n\tfor ;\n\tWed, 14 Feb 2024 18:16:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9383962801;\n\tWed, 14 Feb 2024 19:16:07 +0100 (CET)","from mail-4322.protonmail.ch (mail-4322.protonmail.ch\n\t[185.70.43.22])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3588761CB2\n\tfor ;\n\tWed, 14 Feb 2024 19:16:06 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"lcbOWkBR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1707934564; x=1708193764;\n\tbh=XpVGhFfJjVIjzPTj7mMawoAc1ZhckU2CwEPejodAuYs=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector;\n\tb=lcbOWkBRKOCaJFASJgodfqX1FzYDfJ08tDPr1aWIXtrFmdJSEqCZ3Qs2S+sfolglP\n\trBtqRxNxr03g+/PbpYRUwxhuRgnuJ1fwKJv952DOx3DXgQmZePkIwDSJuj8svcUMRo\n\tGhVgvRBcf1v27ZcMrSNMN+GZaWXKuUFqXhjCDJrF9y0S/hq2O2YcX471CxRv6uHkmF\n\tJkIfnZDpCNRCgFXjU0w3Ki8uJA6vAFRS3wfvyzWY/gK7iJGoaaEv0y1QuY/yyn4lnB\n\tGwATclIKbrtE0AUOlKl+8VZLMOWjyERSiJXsnS85qxKkXPAyQpxoWL11F1H0Vt2dp5\n\tl5LiO7oIdWi5Q==","Date":"Wed, 14 Feb 2024 18:15:46 +0000","To":"Hans de Goede ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Subject":"Re: [PATCH v3 09/16] libcamera: ipa: add Soft IPA","Message-ID":"","In-Reply-To":"<20240214170122.60754-10-hdegoede@redhat.com>","References":"<20240214170122.60754-1-hdegoede@redhat.com>\n\t<20240214170122.60754-10-hdegoede@redhat.com>","Feedback-ID":"20568564:user:proton","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Maxime Ripard , Marttico ,\n\tToon Langendam ,\n\tlibcamera-devel@lists.libcamera.org, Pavel Machek ,\n\tBryan O'Donoghue , \n\tDennis Bonke ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28675,"web_url":"https://patchwork.libcamera.org/comment/28675/","msgid":"<875xypd704.fsf@redhat.com>","date":"2024-02-15T16:31:07","subject":"Re: [PATCH v3 09/16] libcamera: ipa: add Soft IPA","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hans de Goede writes:\n\n> From: Andrey Konovalov \n>\n> Define the Soft IPA main and event interfaces, add the Soft IPA\n> implementation.\n>\n> The current src/ipa/meson.build assumes the IPA name to match the\n> pipeline name. For this reason \"-Dipas=simple\" is used for the\n> Soft IPA module.\n>\n> Auto exposure/gain and AWB implementation by Dennis, Toon and Martti.\n>\n> Auto exposure/gain targets a Mean Sample Value of 2.5 following\n> the MSV calculation algorithm from:\n> https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf\n>\n> Tested-by: Bryan O'Donoghue # sc8280xp Lenovo x13s\n> Tested-by: Pavel Machek \n> Reviewed-by: Pavel Machek \n> Signed-off-by: Andrey Konovalov \n> Co-developed-by: Dennis Bonke \n> Signed-off-by: Dennis Bonke \n> Co-developed-by: Marttico \n> Signed-off-by: Marttico \n> Co-developed-by: Toon Langendam \n> Signed-off-by: Toon Langendam \n> Signed-off-by: Hans de Goede \n> ---\n> Changes in v3:\n> - Use new SwIspStats::kYHistogramSize constexpr and adjust\n> the auto-exposure/-gain code so that it can deal with\n> that having a different value then 16 (modify the loop\n> to divide the histogram in 5 bins to not have hardcoded\n> values)\n> - Rename a few foo_bar symbols to fooBar\n> ---\n> Documentation/Doxyfile.in | 1 +\n> include/libcamera/ipa/meson.build | 1 +\n> include/libcamera/ipa/soft.mojom | 28 +++\n> meson_options.txt | 2 +-\n> src/ipa/simple/data/meson.build | 9 +\n> src/ipa/simple/data/soft.conf | 3 +\n> src/ipa/simple/meson.build | 25 +++\n> src/ipa/simple/soft_simple.cpp | 308 ++++++++++++++++++++++++++++++\n> 8 files changed, 376 insertions(+), 1 deletion(-)\n> create mode 100644 include/libcamera/ipa/soft.mojom\n> create mode 100644 src/ipa/simple/data/meson.build\n> create mode 100644 src/ipa/simple/data/soft.conf\n> create mode 100644 src/ipa/simple/meson.build\n> create mode 100644 src/ipa/simple/soft_simple.cpp\n>\n> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> index a86ea6c1..2be8d47b 100644\n> --- a/Documentation/Doxyfile.in\n> +++ b/Documentation/Doxyfile.in\n> @@ -44,6 +44,7 @@ EXCLUDE = @TOP_SRCDIR@/include/libcamera/base/span.h \\\n> @TOP_SRCDIR@/src/libcamera/pipeline/ \\\n> @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \\\n> @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \\\n> + @TOP_BUILDDIR@/include/libcamera/ipa/soft_ipa_interface.h \\\n> @TOP_BUILDDIR@/src/libcamera/proxy/\n> \n> EXCLUDE_PATTERNS = @TOP_BUILDDIR@/include/libcamera/ipa/*_serializer.h \\\n> diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build\n> index f3b4881c..3352d08f 100644\n> --- a/include/libcamera/ipa/meson.build\n> +++ b/include/libcamera/ipa/meson.build\n> @@ -65,6 +65,7 @@ pipeline_ipa_mojom_mapping = {\n> 'ipu3': 'ipu3.mojom',\n> 'rkisp1': 'rkisp1.mojom',\n> 'rpi/vc4': 'raspberrypi.mojom',\n> + 'simple': 'soft.mojom',\n> 'vimc': 'vimc.mojom',\n> }\n> \n> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom\n> new file mode 100644\n> index 00000000..c249bd75\n> --- /dev/null\n> +++ b/include/libcamera/ipa/soft.mojom\n> @@ -0,0 +1,28 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +\n> +/*\n> + * \\todo Document the interface and remove the related EXCLUDE_PATTERNS entry.\n> + */\n> +\n> +module ipa.soft;\n> +\n> +import \"include/libcamera/ipa/core.mojom\";\n> +\n> +interface IPASoftInterface {\n> +\tinit(libcamera.IPASettings settings,\n> +\t libcamera.SharedFD fdStats,\n> +\t libcamera.SharedFD fdParams,\n> +\t libcamera.ControlInfoMap sensorCtrlInfoMap)\n> +\t\t=> (int32 ret);\n> +\tstart() => (int32 ret);\n> +\tstop();\n> +\tconfigure(libcamera.ControlInfoMap sensorCtrlInfoMap)\n> +\t\t=> (int32 ret);\n> +\n> +\t[async] processStats(libcamera.ControlList sensorControls);\n> +};\n> +\n> +interface IPASoftEventInterface {\n> +\tsetSensorControls(libcamera.ControlList sensorControls);\n> +\tsetIspParams(int32 dummy);\n> +};\n> diff --git a/meson_options.txt b/meson_options.txt\n> index 5fdc7be8..94372e47 100644\n> --- a/meson_options.txt\n> +++ b/meson_options.txt\n> @@ -27,7 +27,7 @@ option('gstreamer',\n> \n> option('ipas',\n> type : 'array',\n> - choices : ['ipu3', 'rkisp1', 'rpi/vc4', 'vimc'],\n> + choices : ['ipu3', 'rkisp1', 'rpi/vc4', 'simple', 'vimc'],\n> description : 'Select which IPA modules to build')\n> \n> option('lc-compliance',\n> diff --git a/src/ipa/simple/data/meson.build b/src/ipa/simple/data/meson.build\n> new file mode 100644\n> index 00000000..33548cc6\n> --- /dev/null\n> +++ b/src/ipa/simple/data/meson.build\n> @@ -0,0 +1,9 @@\n> +# SPDX-License-Identifier: CC0-1.0\n> +\n> +conf_files = files([\n> + 'soft.conf',\n> +])\n> +\n> +install_data(conf_files,\n> + install_dir : ipa_data_dir / 'soft',\n> + install_tag : 'runtime')\n> diff --git a/src/ipa/simple/data/soft.conf b/src/ipa/simple/data/soft.conf\n> new file mode 100644\n> index 00000000..0c70e7c0\n> --- /dev/null\n> +++ b/src/ipa/simple/data/soft.conf\n> @@ -0,0 +1,3 @@\n> +# SPDX-License-Identifier: LGPL-2.1-or-later\n> +#\n> +# Dummy configuration file for the soft IPA.\n> diff --git a/src/ipa/simple/meson.build b/src/ipa/simple/meson.build\n> new file mode 100644\n> index 00000000..3e863db7\n> --- /dev/null\n> +++ b/src/ipa/simple/meson.build\n> @@ -0,0 +1,25 @@\n> +# SPDX-License-Identifier: CC0-1.0\n> +\n> +ipa_name = 'ipa_soft_simple'\n> +\n> +mod = shared_module(ipa_name,\n> + ['soft_simple.cpp', libcamera_generated_ipa_headers],\n> + name_prefix : '',\n> + include_directories : [ipa_includes, libipa_includes],\n> + dependencies : libcamera_private,\n> + link_with : libipa,\n> + install : true,\n> + install_dir : ipa_install_dir)\n> +\n> +if ipa_sign_module\n> + custom_target(ipa_name + '.so.sign',\n> + input : mod,\n> + output : ipa_name + '.so.sign',\n> + command : [ipa_sign, ipa_priv_key, '@INPUT@', '@OUTPUT@'],\n> + install : false,\n> + build_by_default : true)\n> +endif\n> +\n> +subdir('data')\n> +\n> +ipa_names += ipa_name\n> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n> new file mode 100644\n> index 00000000..f7ac02f9\n> --- /dev/null\n> +++ b/src/ipa/simple/soft_simple.cpp\n> @@ -0,0 +1,308 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2023, Linaro Ltd\n> + *\n> + * soft_simple.cpp - Simple Software Image Processing Algorithm module\n> + */\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \"libcamera/internal/camera_sensor.h\"\n> +#include \"libcamera/internal/software_isp/debayer_params.h\"\n> +#include \"libcamera/internal/software_isp/swisp_stats.h\"\n> +\n> +#define EXPOSURE_OPTIMAL_VALUE 2.5\n> +#define EXPOSURE_SATISFACTORY_OFFSET 0.2\n\nDo both the values come from the cited paper or do they deserve some\nexplanation?\n\nAnd maybe constexpr rather than #define?\n\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(IPASoft)\n> +\n> +namespace ipa::soft {\n> +\n> +class IPASoftSimple : public ipa::soft::IPASoftInterface\n> +{\n> +public:\n> +\tIPASoftSimple()\n> +\t\t: ignore_updates_(0)\n> +\t{\n> +\t}\n> +\n> +\t~IPASoftSimple()\n> +\t{\n> +\t\tif (stats_)\n> +\t\t\tmunmap(stats_, sizeof(SwIspStats));\n> +\t\tif (params_)\n> +\t\t\tmunmap(params_, sizeof(DebayerParams));\n> +\t}\n> +\n> +\tint init(const IPASettings &settings,\n> +\t\t const SharedFD &fdStats,\n> +\t\t const SharedFD &fdParams,\n> +\t\t const ControlInfoMap &sensorInfoMap) override;\n> +\tint configure(const ControlInfoMap &sensorInfoMap) override;\n> +\n> +\tint start() override;\n> +\tvoid stop() override;\n> +\n> +\tvoid processStats(const ControlList &sensorControls) override;\n> +\n> +private:\n> +\tvoid updateExposure(double exposureMSV);\n> +\n> +\tSharedFD fdStats_;\n> +\tSharedFD fdParams_;\n> +\tDebayerParams *params_;\n> +\tSwIspStats *stats_;\n> +\tint exposure_min_, exposure_max_;\n> +\tint again_min_, again_max_;\n> +\tint again_, exposure_;\n> +\tint ignore_updates_;\n> +};\n> +\n> +int IPASoftSimple::init([[maybe_unused]] const IPASettings &settings,\n> +\t\t\tconst SharedFD &fdStats,\n> +\t\t\tconst SharedFD &fdParams,\n> +\t\t\tconst ControlInfoMap &sensorInfoMap)\n> +{\n> +\tfdStats_ = std::move(fdStats);\n> +\tif (!fdStats_.isValid()) {\n> +\t\tLOG(IPASoft, Error) << \"Invalid Statistics handle\";\n> +\t\treturn -ENODEV;\n> +\t}\n> +\n> +\tfdParams_ = std::move(fdParams);\n> +\tif (!fdParams_.isValid()) {\n> +\t\tLOG(IPASoft, Error) << \"Invalid Parameters handle\";\n> +\t\treturn -ENODEV;\n> +\t}\n> +\n> +\tparams_ = static_cast(mmap(nullptr, sizeof(DebayerParams),\n> +\t\t\t\t\t\t PROT_WRITE, MAP_SHARED,\n> +\t\t\t\t\t\t fdParams_.get(), 0));\n> +\tif (!params_) {\n> +\t\tLOG(IPASoft, Error) << \"Unable to map Parameters\";\n> +\t\treturn -ENODEV;\n> +\t}\n> +\n> +\tstats_ = static_cast(mmap(nullptr, sizeof(SwIspStats),\n> +\t\t\t\t\t\tPROT_READ, MAP_SHARED,\n> +\t\t\t\t\t\tfdStats_.get(), 0));\n> +\tif (!stats_) {\n> +\t\tLOG(IPASoft, Error) << \"Unable to map Statistics\";\n> +\t\treturn -ENODEV;\n> +\t}\n> +\n> +\tif (sensorInfoMap.find(V4L2_CID_EXPOSURE) == sensorInfoMap.end()) {\n> +\t\tLOG(IPASoft, Error) << \"Don't have exposure control\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tif (sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN) == sensorInfoMap.end()) {\n> +\t\tLOG(IPASoft, Error) << \"Don't have gain control\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +int IPASoftSimple::configure(const ControlInfoMap &sensorInfoMap)\n> +{\n> +\tconst ControlInfo &exposure_info = sensorInfoMap.find(V4L2_CID_EXPOSURE)->second;\n> +\tconst ControlInfo &gain_info = sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN)->second;\n> +\n> +\texposure_min_ = exposure_info.min().get();\n> +\tif (!exposure_min_) {\n> +\t\tLOG(IPASoft, Warning) << \"Minimum exposure is zero, that can't be linear\";\n> +\t\texposure_min_ = 1;\n> +\t}\n> +\texposure_max_ = exposure_info.max().get();\n> +\tagain_min_ = gain_info.min().get();\n> +\tif (!again_min_) {\n> +\t\tLOG(IPASoft, Warning) << \"Minimum gain is zero, that can't be linear\";\n> +\t\tagain_min_ = 100;\n> +\t}\n\nI wonder what \"can't be linear\" means here exactly. \n\n> +\tagain_max_ = gain_info.max().get();\n\nCan it happen that again_max_ is lower than the artificially set again_min_ =\n100 above?\n\n> +\n> +\tLOG(IPASoft, Info) << \"Exposure \" << exposure_min_ << \"-\" << exposure_max_\n> +\t\t\t << \", gain \" << again_min_ << \"-\" << again_max_;\n> +\n> +\treturn 0;\n> +}\n> +\n> +int IPASoftSimple::start()\n> +{\n> +\treturn 0;\n> +}\n> +\n> +void IPASoftSimple::stop()\n> +{\n> +}\n> +\n> +void IPASoftSimple::processStats(const ControlList &sensorControls)\n> +{\n> +\t/*\n> +\t * Calculate red and blue gains for AWB.\n> +\t * Clamp max gain at 4.0, this also avoids 0 division.\n> +\t */\n> +\tif (stats_->sumR_ <= stats_->sumG_ / 4)\n> +\t\tparams_->gainR = 1024;\n> +\telse\n> +\t\tparams_->gainR = 256 * stats_->sumG_ / stats_->sumR_;\n> +\n> +\tif (stats_->sumB_ <= stats_->sumG_ / 4)\n> +\t\tparams_->gainB = 1024;\n> +\telse\n> +\t\tparams_->gainB = 256 * stats_->sumG_ / stats_->sumB_;\n> +\n> +\t/* Green gain and gamma values are fixed */\n> +\tparams_->gainG = 256;\n> +\tparams_->gamma = 0.5;\n> +\n> +\tsetIspParams.emit(0);\n> +\n> +\t/*\n> +\t * AE / AGC, use 2 frames delay to make sure that the exposure and\n> +\t * the gain set have applied to the camera sensor.\n> +\t */\n> +\tif (ignore_updates_ > 0) {\n> +\t\t--ignore_updates_;\n> +\t\treturn;\n> +\t}\n> +\n> +\t/*\n> +\t * Calculate Mean Sample Value (MSV) according to formula from:\n> +\t * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf\n> +\t */\n> +\tconstexpr unsigned int ExposureBinsCount = 5;\n> +\tconstexpr unsigned int yHistValsPerBin =\n> +\t\tSwIspStats::kYHistogramSize / ExposureBinsCount;\n> +\tconstexpr unsigned int yHistValsPerBinMod =\n> +\t\tSwIspStats::kYHistogramSize /\n> +\t\t(SwIspStats::kYHistogramSize % ExposureBinsCount + 1);\n> +\tint ExposureBins[ExposureBinsCount] = {};\n> +\tunsigned int denom = 0;\n> +\tunsigned int num = 0;\n> +\n> +\tfor (unsigned int i = 0; i < SwIspStats::kYHistogramSize; i++) {\n> +\t\tunsigned int idx = (i - (i / yHistValsPerBinMod)) / yHistValsPerBin;\n> +\t\tExposureBins[idx] += stats_->yHistogram[i];\n> +\t}\n> +\n> +\tfor (unsigned int i = 0; i < ExposureBinsCount; i++) {\n> +\t\tLOG(IPASoft, Debug) << i << \": \" << ExposureBins[i];\n> +\t\tdenom += ExposureBins[i];\n> +\t\tnum += ExposureBins[i] * (i + 1);\n> +\t}\n> +\n> +\tfloat exposureMSV = (float)num / denom;\n> +\n> +\t/* sanity check */\n> +\tif (!sensorControls.contains(V4L2_CID_EXPOSURE) ||\n> +\t !sensorControls.contains(V4L2_CID_ANALOGUE_GAIN)) {\n> +\t\tLOG(IPASoft, Error) << \"Control(s) missing\";\n> +\t\treturn;\n> +\t}\n> +\n> +\tControlList ctrls(sensorControls);\n> +\n> +\texposure_ = ctrls.get(V4L2_CID_EXPOSURE).get();\n> +\tagain_ = ctrls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> +\n> +\tupdateExposure(exposureMSV);\n> +\n> +\tctrls.set(V4L2_CID_EXPOSURE, exposure_);\n> +\tctrls.set(V4L2_CID_ANALOGUE_GAIN, again_);\n> +\n> +\tignore_updates_ = 2;\n> +\n> +\tsetSensorControls.emit(ctrls);\n> +\n> +\tLOG(IPASoft, Debug) << \"exposureMSV \" << exposureMSV\n> +\t\t\t << \" exp \" << exposure_ << \" again \" << again_\n> +\t\t\t << \" gain R/B \" << params_->gainR << \"/\" << params_->gainB;\n> +}\n> +\n> +/* DENOMINATOR of 10 gives ~10% increment/decrement; DENOMINATOR of 5 - about ~20% */\n> +#define DENOMINATOR 10\n> +#define UP_NUMERATOR (DENOMINATOR + 1)\n> +#define DOWN_NUMERATOR (DENOMINATOR - 1)\n> +\n> +void IPASoftSimple::updateExposure(double exposureMSV)\n> +{\n> +\tint next;\n> +\n> +\tif (exposureMSV < EXPOSURE_OPTIMAL_VALUE - EXPOSURE_SATISFACTORY_OFFSET) {\n> +\t\tnext = exposure_ * UP_NUMERATOR / DENOMINATOR;\n> +\t\tif (next - exposure_ < 1)\n> +\t\t\texposure_ += 1;\n> +\t\telse\n> +\t\t\texposure_ = next;\n> +\t\tif (exposure_ >= exposure_max_) {\n> +\t\t\tnext = again_ * UP_NUMERATOR / DENOMINATOR;\n> +\t\t\tif (next - again_ < 1)\n> +\t\t\t\tagain_ += 1;\n> +\t\t\telse\n> +\t\t\t\tagain_ = next;\n> +\t\t}\n> +\t}\n> +\n> +\tif (exposureMSV > EXPOSURE_OPTIMAL_VALUE + EXPOSURE_SATISFACTORY_OFFSET) {\n> +\t\tif (exposure_ == exposure_max_ && again_ != again_min_) {\n> +\t\t\tnext = again_ * DOWN_NUMERATOR / DENOMINATOR;\n> +\t\t\tif (again_ - next < 1)\n> +\t\t\t\tagain_ -= 1;\n> +\t\t\telse\n> +\t\t\t\tagain_ = next;\n> +\t\t} else {\n> +\t\t\tnext = exposure_ * DOWN_NUMERATOR / DENOMINATOR;\n> +\t\t\tif (exposure_ - next < 1)\n> +\t\t\t\texposure_ -= 1;\n> +\t\t\telse\n> +\t\t\t\texposure_ = next;\n> +\t\t}\n> +\t}\n> +\n> +\tif (exposure_ > exposure_max_)\n> +\t\texposure_ = exposure_max_;\n> +\telse if (exposure_ < exposure_min_)\n> +\t\texposure_ = exposure_min_;\n> +\n> +\tif (again_ > again_max_)\n> +\t\tagain_ = again_max_;\n> +\telse if (again_ < again_min_)\n> +\t\tagain_ = again_min_;\n> +}\n> +\n> +} /* namespace ipa::soft */\n> +\n> +/*\n> + * External IPA module interface\n> + */\n> +extern \"C\" {\n> +const struct IPAModuleInfo ipaModuleInfo = {\n> +\tIPA_MODULE_API_VERSION,\n> +\t0,\n> +\t\"SimplePipelineHandler\",\n> +\t\"simple\",\n> +};\n> +\n> +IPAInterface *ipaCreate()\n> +{\n> +\treturn new ipa::soft::IPASoftSimple();\n> +}\n> +\n> +} /* extern \"C\" */\n> +\n> +} /* namespace libcamera */","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 74BB4BDE17\n\tfor ;\n\tThu, 15 Feb 2024 16:31:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DF89862801;\n\tThu, 15 Feb 2024 17:31:16 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C002F61CB0\n\tfor ;\n\tThu, 15 Feb 2024 17:31:14 +0100 (CET)","from mail-wr1-f71.google.com (mail-wr1-f71.google.com\n\t[209.85.221.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-583-R_eMP608NiSmtErQLVnkTA-1; Thu, 15 Feb 2024 11:31:11 -0500","by mail-wr1-f71.google.com with SMTP id\n\tffacd0b85a97d-33b8837355dso568047f8f.0\n\tfor ;\n\tThu, 15 Feb 2024 08:31:10 -0800 (PST)","from nuthatch (nat-pool-brq-t.redhat.com. [213.175.37.10])\n\tby smtp.gmail.com with ESMTPSA id\n\tv5-20020a5d59c5000000b0033b1277e95dsm2380442wry.77.2024.02.15.08.31.08\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 15 Feb 2024 08:31:08 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"PKfIijy+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1708014673;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=rtd+K71myA4QOi1Kv1MGN66YqoF+XqcSbmd7uoCr6Uc=;\n\tb=PKfIijy+V055RvMhgI/UbSQVqTGhHe9C8gEXUEQmxmPebPd8qHhWtAXxJbH/4F6Wj7ls4i\n\tMxHPhJDlE9nCfwhbHQcKaCz3A6Qk/0NGMYzkbLX0yMuG022DPR4XMuo4vQtzikw5oWEPcK\n\tiAWMYnpzgxFfGtaGWniCidxbTZk09tY=","X-MC-Unique":"R_eMP608NiSmtErQLVnkTA-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1708014670; x=1708619470;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=rtd+K71myA4QOi1Kv1MGN66YqoF+XqcSbmd7uoCr6Uc=;\n\tb=Lh7ZEUb0a7VJGtMz8KQZiqCQyDCJe4kCJB0iKiusZFt2eknoo5YCaQHTvnDIUPN+uR\n\tZ9dS1ptmMBMUi7jgxhGeviz/b40unDqEKS5YdRazE1AlHt2Az9T6YE28aataP8ehhXNi\n\t+5QIcClQcLizlOfYA6TNZOZGGX6HtI1aOkKJuSdB0zyHD6HNQTY1wqva+SmQ6qHnuojO\n\t++TG1p7knOalFV4kQu4j5rEyFqL6mlzFqesd1YwTggvJhzF3KfySKNWSpikP3H1CYS9x\n\tmhD/4FojKA0sSLnum4hEolFcDxKzXx17OL6l/m/C7ERoFt48KjPSK0KDfh70tuayWifp\n\tHXOA==","X-Gm-Message-State":"AOJu0Ywxc4u46rvLX1t8pUcpPc0sKj1Ued0uv4GAWoulcJsARSWmcdp/\n\tjZ+sTyf1qr0wbvQksz+PGKArGo68WUvLvT1wcsGYb5Ba8/Q77m93qZp80/apL6Qg9ld/XWOvDI6\n\tOEdwXuUpQKDfMD6GaOo5BLOcs7KgA3xWIt50IIhYemw+QABNodr0srrlsws1lZgEY0USAkOk=","X-Received":["by 2002:adf:f44a:0:b0:33b:50e5:8a78 with SMTP id\n\tf10-20020adff44a000000b0033b50e58a78mr1602280wrp.22.1708014669863; \n\tThu, 15 Feb 2024 08:31:09 -0800 (PST)","by 2002:adf:f44a:0:b0:33b:50e5:8a78 with SMTP id\n\tf10-20020adff44a000000b0033b50e58a78mr1602264wrp.22.1708014669466; \n\tThu, 15 Feb 2024 08:31:09 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IHPvibJTk7zb49u4CKnz8z0Nkxt+FqMa2HCnCoEnql0B+ltfiATUhqndner81iY28KoJRo3sA==","From":"Milan Zamazal ","To":"Hans de Goede ","Subject":"Re: [PATCH v3 09/16] libcamera: ipa: add Soft IPA","In-Reply-To":"<20240214170122.60754-10-hdegoede@redhat.com> (Hans de Goede's\n\tmessage of \"Wed, 14 Feb 2024 18:01:13 +0100\")","References":"<20240214170122.60754-1-hdegoede@redhat.com>\n\t<20240214170122.60754-10-hdegoede@redhat.com>","Date":"Thu, 15 Feb 2024 17:31:07 +0100","Message-ID":"<875xypd704.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Maxime Ripard , Marttico ,\n\tToon Langendam ,\n\tlibcamera-devel@lists.libcamera.org, Pavel Machek ,\n\tBryan O'Donoghue , \n\tDennis Bonke ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28677,"web_url":"https://patchwork.libcamera.org/comment/28677/","msgid":"<170801745454.1011926.3111478956976259688@ping.linuxembedded.co.uk>","date":"2024-02-15T17:17:34","subject":"Re: [PATCH v3 09/16] libcamera: ipa: add Soft IPA","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Milan Zamazal (2024-02-15 16:31:07)\n> Hans de Goede writes:\n> \n> > From: Andrey Konovalov \n> >\n> > Define the Soft IPA main and event interfaces, add the Soft IPA\n> > implementation.\n> >\n> > The current src/ipa/meson.build assumes the IPA name to match the\n> > pipeline name. For this reason \"-Dipas=simple\" is used for the\n> > Soft IPA module.\n> >\n> > Auto exposure/gain and AWB implementation by Dennis, Toon and Martti.\n> >\n> > Auto exposure/gain targets a Mean Sample Value of 2.5 following\n> > the MSV calculation algorithm from:\n> > https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf\n> >\n> > Tested-by: Bryan O'Donoghue # sc8280xp Lenovo x13s\n> > Tested-by: Pavel Machek \n> > Reviewed-by: Pavel Machek \n> > Signed-off-by: Andrey Konovalov \n> > Co-developed-by: Dennis Bonke \n> > Signed-off-by: Dennis Bonke \n> > Co-developed-by: Marttico \n> > Signed-off-by: Marttico \n> > Co-developed-by: Toon Langendam \n> > Signed-off-by: Toon Langendam \n> > Signed-off-by: Hans de Goede \n\nDefinitely a team effort ;-) I like it! It's a big project after all.\n\n\n\n> > +{\n> > + const ControlInfo &exposure_info = sensorInfoMap.find(V4L2_CID_EXPOSURE)->second;\n> > + const ControlInfo &gain_info = sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN)->second;\n> > +\n> > + exposure_min_ = exposure_info.min().get();\n> > + if (!exposure_min_) {\n> > + LOG(IPASoft, Warning) << \"Minimum exposure is zero, that can't be linear\";\n\nIndeed, these should be going through the CameraSensorHelper class to\nconvert and manage gains.\n\nAt some level/point you will need to know what the gain codes are as a\nmultiplier. The CameraSensor helpers are expected to provide that sensor\nspecific calculation for you.\n\n\n\n--\nKieran","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id DACA1BDE17\n\tfor ;\n\tThu, 15 Feb 2024 17:17:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 49F5162805;\n\tThu, 15 Feb 2024 18:17:40 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0FDB761CB0\n\tfor ;\n\tThu, 15 Feb 2024 18:17:38 +0100 (CET)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E2F489CA;\n\tThu, 15 Feb 2024 18:17:33 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"RBsoKN/1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1708017454;\n\tbh=pSCig4qvoQQh04ExJiR8tHMsaTYad2ryKTI67LS8XWc=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=RBsoKN/1w7nLJPfyh77RxtFP5HHzqGVFNCVa0ZOT4Om+AOVm/lpzfVn8eBNDElcKh\n\tVTCumTop8pK6GxnET/hvqXUbOOGs1qdO/hfXJrpD1n3L2wG0RvcJGc/wpzGBoj9tXz\n\togSUowORGUDY5j1qHESxbfozJl31Z9+2rxRsv+s0=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<875xypd704.fsf@redhat.com>","References":"<20240214170122.60754-1-hdegoede@redhat.com>\n\t<20240214170122.60754-10-hdegoede@redhat.com>\n\t<875xypd704.fsf@redhat.com>","Subject":"Re: [PATCH v3 09/16] libcamera: ipa: add Soft IPA","From":"Kieran Bingham ","To":"Hans de Goede , Milan Zamazal ","Date":"Thu, 15 Feb 2024 17:17:34 +0000","Message-ID":"<170801745454.1011926.3111478956976259688@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Maxime Ripard , Marttico ,\n\tToon Langendam ,\n\tlibcamera-devel@lists.libcamera.org, Pavel Machek ,\n\tBryan O'Donoghue , \n\tDennis Bonke ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28702,"web_url":"https://patchwork.libcamera.org/comment/28702/","msgid":"<170843431013.1011926.9612134639156491265@ping.linuxembedded.co.uk>","date":"2024-02-20T13:05:10","subject":"Re: [PATCH v3 09/16] libcamera: ipa: add Soft IPA","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Hans de Goede (2024-02-14 17:01:13)\n> From: Andrey Konovalov \n> \n> Define the Soft IPA main and event interfaces, add the Soft IPA\n> implementation.\n> \n> The current src/ipa/meson.build assumes the IPA name to match the\n> pipeline name. For this reason \"-Dipas=simple\" is used for the\n> Soft IPA module.\n> \n> Auto exposure/gain and AWB implementation by Dennis, Toon and Martti.\n> \n> Auto exposure/gain targets a Mean Sample Value of 2.5 following\n> the MSV calculation algorithm from:\n> https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf\n> \n> Tested-by: Bryan O'Donoghue # sc8280xp Lenovo x13s\n> Tested-by: Pavel Machek \n> Reviewed-by: Pavel Machek \n> Signed-off-by: Andrey Konovalov \n> Co-developed-by: Dennis Bonke \n> Signed-off-by: Dennis Bonke \n> Co-developed-by: Marttico \n> Signed-off-by: Marttico \n> Co-developed-by: Toon Langendam \n> Signed-off-by: Toon Langendam \n> Signed-off-by: Hans de Goede \n> ---\n> Changes in v3:\n> - Use new SwIspStats::kYHistogramSize constexpr and adjust\n> the auto-exposure/-gain code so that it can deal with\n> that having a different value then 16 (modify the loop\n> to divide the histogram in 5 bins to not have hardcoded\n> values)\n> - Rename a few foo_bar symbols to fooBar\n> ---\n> Documentation/Doxyfile.in | 1 +\n> include/libcamera/ipa/meson.build | 1 +\n> include/libcamera/ipa/soft.mojom | 28 +++\n> meson_options.txt | 2 +-\n> src/ipa/simple/data/meson.build | 9 +\n> src/ipa/simple/data/soft.conf | 3 +\n> src/ipa/simple/meson.build | 25 +++\n> src/ipa/simple/soft_simple.cpp | 308 ++++++++++++++++++++++++++++++\n> 8 files changed, 376 insertions(+), 1 deletion(-)\n> create mode 100644 include/libcamera/ipa/soft.mojom\n> create mode 100644 src/ipa/simple/data/meson.build\n> create mode 100644 src/ipa/simple/data/soft.conf\n> create mode 100644 src/ipa/simple/meson.build\n> create mode 100644 src/ipa/simple/soft_simple.cpp\n> \n> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> index a86ea6c1..2be8d47b 100644\n> --- a/Documentation/Doxyfile.in\n> +++ b/Documentation/Doxyfile.in\n> @@ -44,6 +44,7 @@ EXCLUDE = @TOP_SRCDIR@/include/libcamera/base/span.h \\\n> @TOP_SRCDIR@/src/libcamera/pipeline/ \\\n> @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \\\n> @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \\\n> + @TOP_BUILDDIR@/include/libcamera/ipa/soft_ipa_interface.h \\\n> @TOP_BUILDDIR@/src/libcamera/proxy/\n> \n> EXCLUDE_PATTERNS = @TOP_BUILDDIR@/include/libcamera/ipa/*_serializer.h \\\n> diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build\n> index f3b4881c..3352d08f 100644\n> --- a/include/libcamera/ipa/meson.build\n> +++ b/include/libcamera/ipa/meson.build\n> @@ -65,6 +65,7 @@ pipeline_ipa_mojom_mapping = {\n> 'ipu3': 'ipu3.mojom',\n> 'rkisp1': 'rkisp1.mojom',\n> 'rpi/vc4': 'raspberrypi.mojom',\n> + 'simple': 'soft.mojom',\n> 'vimc': 'vimc.mojom',\n> }\n> \n> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom\n> new file mode 100644\n> index 00000000..c249bd75\n> --- /dev/null\n> +++ b/include/libcamera/ipa/soft.mojom\n> @@ -0,0 +1,28 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +\n> +/*\n> + * \\todo Document the interface and remove the related EXCLUDE_PATTERNS entry.\n> + */\n> +\n> +module ipa.soft;\n> +\n> +import \"include/libcamera/ipa/core.mojom\";\n> +\n> +interface IPASoftInterface {\n> + init(libcamera.IPASettings settings,\n> + libcamera.SharedFD fdStats,\n> + libcamera.SharedFD fdParams,\n> + libcamera.ControlInfoMap sensorCtrlInfoMap)\n> + => (int32 ret);\n> + start() => (int32 ret);\n> + stop();\n> + configure(libcamera.ControlInfoMap sensorCtrlInfoMap)\n> + => (int32 ret);\n> +\n> + [async] processStats(libcamera.ControlList sensorControls);\n> +};\n> +\n> +interface IPASoftEventInterface {\n> + setSensorControls(libcamera.ControlList sensorControls);\n> + setIspParams(int32 dummy);\n> +};\n> diff --git a/meson_options.txt b/meson_options.txt\n> index 5fdc7be8..94372e47 100644\n> --- a/meson_options.txt\n> +++ b/meson_options.txt\n> @@ -27,7 +27,7 @@ option('gstreamer',\n> \n> option('ipas',\n> type : 'array',\n> - choices : ['ipu3', 'rkisp1', 'rpi/vc4', 'vimc'],\n> + choices : ['ipu3', 'rkisp1', 'rpi/vc4', 'simple', 'vimc'],\n> description : 'Select which IPA modules to build')\n> \n> option('lc-compliance',\n> diff --git a/src/ipa/simple/data/meson.build b/src/ipa/simple/data/meson.build\n> new file mode 100644\n> index 00000000..33548cc6\n> --- /dev/null\n> +++ b/src/ipa/simple/data/meson.build\n> @@ -0,0 +1,9 @@\n> +# SPDX-License-Identifier: CC0-1.0\n> +\n> +conf_files = files([\n> + 'soft.conf',\n> +])\n> +\n> +install_data(conf_files,\n> + install_dir : ipa_data_dir / 'soft',\n> + install_tag : 'runtime')\n> diff --git a/src/ipa/simple/data/soft.conf b/src/ipa/simple/data/soft.conf\n> new file mode 100644\n> index 00000000..0c70e7c0\n> --- /dev/null\n> +++ b/src/ipa/simple/data/soft.conf\n> @@ -0,0 +1,3 @@\n> +# SPDX-License-Identifier: LGPL-2.1-or-later\n> +#\n> +# Dummy configuration file for the soft IPA.\n> diff --git a/src/ipa/simple/meson.build b/src/ipa/simple/meson.build\n> new file mode 100644\n> index 00000000..3e863db7\n> --- /dev/null\n> +++ b/src/ipa/simple/meson.build\n> @@ -0,0 +1,25 @@\n> +# SPDX-License-Identifier: CC0-1.0\n> +\n> +ipa_name = 'ipa_soft_simple'\n> +\n> +mod = shared_module(ipa_name,\n> + ['soft_simple.cpp', libcamera_generated_ipa_headers],\n> + name_prefix : '',\n> + include_directories : [ipa_includes, libipa_includes],\n> + dependencies : libcamera_private,\n> + link_with : libipa,\n> + install : true,\n> + install_dir : ipa_install_dir)\n> +\n> +if ipa_sign_module\n> + custom_target(ipa_name + '.so.sign',\n> + input : mod,\n> + output : ipa_name + '.so.sign',\n> + command : [ipa_sign, ipa_priv_key, '@INPUT@', '@OUTPUT@'],\n> + install : false,\n> + build_by_default : true)\n> +endif\n> +\n> +subdir('data')\n> +\n> +ipa_names += ipa_name\n> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n> new file mode 100644\n> index 00000000..f7ac02f9\n> --- /dev/null\n> +++ b/src/ipa/simple/soft_simple.cpp\n> @@ -0,0 +1,308 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2023, Linaro Ltd\n> + *\n> + * soft_simple.cpp - Simple Software Image Processing Algorithm module\n> + */\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \"libcamera/internal/camera_sensor.h\"\n> +#include \"libcamera/internal/software_isp/debayer_params.h\"\n> +#include \"libcamera/internal/software_isp/swisp_stats.h\"\n> +\n> +#define EXPOSURE_OPTIMAL_VALUE 2.5\n> +#define EXPOSURE_SATISFACTORY_OFFSET 0.2\n\nPerhaps:\n\nstatic constexpr float kExposureOptimal = 2.5;\nstatic constexpr float kExposureSatisfactory = 0.2;\n\nBut can those constants have some more comments or documentation about\n/why/ they are optimal and satisfactory? Where do they come from? what\nare the ranges? Anything applicable to say about them?\n\n\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(IPASoft)\n> +\n> +namespace ipa::soft {\n> +\n> +class IPASoftSimple : public ipa::soft::IPASoftInterface\n> +{\n> +public:\n> + IPASoftSimple()\n> + : ignore_updates_(0)\n> + {\n> + }\n> +\n> + ~IPASoftSimple()\n> + {\n> + if (stats_)\n> + munmap(stats_, sizeof(SwIspStats));\n> + if (params_)\n> + munmap(params_, sizeof(DebayerParams));\n> + }\n> +\n> + int init(const IPASettings &settings,\n> + const SharedFD &fdStats,\n> + const SharedFD &fdParams,\n> + const ControlInfoMap &sensorInfoMap) override;\n> + int configure(const ControlInfoMap &sensorInfoMap) override;\n> +\n> + int start() override;\n> + void stop() override;\n> +\n> + void processStats(const ControlList &sensorControls) override;\n> +\n> +private:\n> + void updateExposure(double exposureMSV);\n> +\n> + SharedFD fdStats_;\n> + SharedFD fdParams_;\n> + DebayerParams *params_;\n> + SwIspStats *stats_;\n\nI'd throw a blank line between class instances and the following PODs\n\n> + int exposure_min_, exposure_max_;\n> + int again_min_, again_max_;\n> + int again_, exposure_;\n> + int ignore_updates_;\n\nShould those by unsigned? Can they ever be 0 or less than zero?\n\n> +};\n> +\n> +int IPASoftSimple::init([[maybe_unused]] const IPASettings &settings,\n> + const SharedFD &fdStats,\n> + const SharedFD &fdParams,\n> + const ControlInfoMap &sensorInfoMap)\n> +{\n> + fdStats_ = std::move(fdStats);\n> + if (!fdStats_.isValid()) {\n> + LOG(IPASoft, Error) << \"Invalid Statistics handle\";\n> + return -ENODEV;\n> + }\n> +\n> + fdParams_ = std::move(fdParams);\n> + if (!fdParams_.isValid()) {\n> + LOG(IPASoft, Error) << \"Invalid Parameters handle\";\n> + return -ENODEV;\n> + }\n> +\n> + params_ = static_cast(mmap(nullptr, sizeof(DebayerParams),\n> + PROT_WRITE, MAP_SHARED,\n> + fdParams_.get(), 0));\n\nWe'd usually use MappedFrameBuffer() here so that the mappings are\nhandled by RAII here\n\n> + if (!params_) {\n> + LOG(IPASoft, Error) << \"Unable to map Parameters\";\n> + return -ENODEV;\n> + }\n> +\n> + stats_ = static_cast(mmap(nullptr, sizeof(SwIspStats),\n> + PROT_READ, MAP_SHARED,\n> + fdStats_.get(), 0));\n> + if (!stats_) {\n> + LOG(IPASoft, Error) << \"Unable to map Statistics\";\n> + return -ENODEV;\n> + }\n\nSame here.\n\n\n> +\n> + if (sensorInfoMap.find(V4L2_CID_EXPOSURE) == sensorInfoMap.end()) {\n> + LOG(IPASoft, Error) << \"Don't have exposure control\";\n> + return -EINVAL;\n> + }\n> +\n> + if (sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN) == sensorInfoMap.end()) {\n> + LOG(IPASoft, Error) << \"Don't have gain control\";\n> + return -EINVAL;\n> + }\n> +\n\nIn the future I would expect AEGC to be broken out to it's own modular\n'Algorithm' (see rkisp1 for the createAlgorithms()\n\n\nWe're also lacking any set up or registration for control handling. I\nguess for now there may be none, but do you plan to have controls? I'm\nfine with those being added later.\n\n\n\n> + return 0;\n> +}\n> +\n> +int IPASoftSimple::configure(const ControlInfoMap &sensorInfoMap)\n> +{\n> + const ControlInfo &exposure_info = sensorInfoMap.find(V4L2_CID_EXPOSURE)->second;\n> + const ControlInfo &gain_info = sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN)->second;\n> +\n> + exposure_min_ = exposure_info.min().get();\n> + if (!exposure_min_) {\n> + LOG(IPASoft, Warning) << \"Minimum exposure is zero, that can't be linear\";\n> + exposure_min_ = 1;\n> + }\n> + exposure_max_ = exposure_info.max().get();\n> + again_min_ = gain_info.min().get();\n> + if (!again_min_) {\n> + LOG(IPASoft, Warning) << \"Minimum gain is zero, that can't be linear\";\n> + again_min_ = 100;\n> + }\n\nThis is where you should almost certainly be looking at the\nCameraSensorHelpers. We expect the gain model to be abstracted by each\nsensor through the helpers.\n\nIPA algorithms should operate on a linear model, and the\nCameraSensorHelpers will convert any logarithmic gain codes to linear on\na per-sensor basis, as well as impose any sensor specific limits.\n\n\n> + again_max_ = gain_info.max().get();\n> +\n> + LOG(IPASoft, Info) << \"Exposure \" << exposure_min_ << \"-\" << exposure_max_\n> + << \", gain \" << again_min_ << \"-\" << again_max_;\n> +\n> + return 0;\n> +}\n> +\n> +int IPASoftSimple::start()\n> +{\n> + return 0;\n> +}\n> +\n> +void IPASoftSimple::stop()\n> +{\n> +}\n> +\n> +void IPASoftSimple::processStats(const ControlList &sensorControls)\n> +{\n> + /*\n> + * Calculate red and blue gains for AWB.\n> + * Clamp max gain at 4.0, this also avoids 0 division.\n> + */\n> + if (stats_->sumR_ <= stats_->sumG_ / 4)\n> + params_->gainR = 1024;\n> + else\n> + params_->gainR = 256 * stats_->sumG_ / stats_->sumR_;\n> +\n> + if (stats_->sumB_ <= stats_->sumG_ / 4)\n> + params_->gainB = 1024;\n> + else\n> + params_->gainB = 256 * stats_->sumG_ / stats_->sumB_;\n> +\n> + /* Green gain and gamma values are fixed */\n> + params_->gainG = 256;\n> + params_->gamma = 0.5;\n> +\n> + setIspParams.emit(0);\n> +\n> + /*\n> + * AE / AGC, use 2 frames delay to make sure that the exposure and\n> + * the gain set have applied to the camera sensor.\n> + */\n\nUsing known sensor specific delays defined in the CameraSensorHelper and\nDelayedControls should be considered here in the future. But as a first\nimplementation for CPU only, where doing /less/ work is helpful - this\nis fine for me.\n\n\n> + if (ignore_updates_ > 0) {\n> + --ignore_updates_;\n> + return;\n> + }\n> +\n> + /*\n> + * Calculate Mean Sample Value (MSV) according to formula from:\n> + * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf\n> + */\n> + constexpr unsigned int ExposureBinsCount = 5;\n> + constexpr unsigned int yHistValsPerBin =\n> + SwIspStats::kYHistogramSize / ExposureBinsCount;\n> + constexpr unsigned int yHistValsPerBinMod =\n> + SwIspStats::kYHistogramSize /\n> + (SwIspStats::kYHistogramSize % ExposureBinsCount + 1);\n> + int ExposureBins[ExposureBinsCount] = {};\n> + unsigned int denom = 0;\n> + unsigned int num = 0;\n> +\n> + for (unsigned int i = 0; i < SwIspStats::kYHistogramSize; i++) {\n> + unsigned int idx = (i - (i / yHistValsPerBinMod)) / yHistValsPerBin;\n> + ExposureBins[idx] += stats_->yHistogram[i];\n> + }\n> +\n> + for (unsigned int i = 0; i < ExposureBinsCount; i++) {\n> + LOG(IPASoft, Debug) << i << \": \" << ExposureBins[i];\n> + denom += ExposureBins[i];\n> + num += ExposureBins[i] * (i + 1);\n> + }\n> +\n> + float exposureMSV = (float)num / denom;\n> +\n> + /* sanity check */\n> + if (!sensorControls.contains(V4L2_CID_EXPOSURE) ||\n> + !sensorControls.contains(V4L2_CID_ANALOGUE_GAIN)) {\n> + LOG(IPASoft, Error) << \"Control(s) missing\";\n> + return;\n> + }\n> +\n> + ControlList ctrls(sensorControls);\n> +\n> + exposure_ = ctrls.get(V4L2_CID_EXPOSURE).get();\n> + again_ = ctrls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> +\n> + updateExposure(exposureMSV);\n> +\n> + ctrls.set(V4L2_CID_EXPOSURE, exposure_);\n> + ctrls.set(V4L2_CID_ANALOGUE_GAIN, again_);\n> +\n> + ignore_updates_ = 2;\n> +\n> + setSensorControls.emit(ctrls);\n> +\n> + LOG(IPASoft, Debug) << \"exposureMSV \" << exposureMSV\n> + << \" exp \" << exposure_ << \" again \" << again_\n> + << \" gain R/B \" << params_->gainR << \"/\" << params_->gainB;\n> +}\n> +\n> +/* DENOMINATOR of 10 gives ~10% increment/decrement; DENOMINATOR of 5 - about ~20% */\n> +#define DENOMINATOR 10\n> +#define UP_NUMERATOR (DENOMINATOR + 1)\n> +#define DOWN_NUMERATOR (DENOMINATOR - 1)\n\nOur C++ code style for constants would be more like:\n\nstatic constexpr uint8_t kExpDenominator = 10;\nstatic constexpr uint8_t kExpNumeratorUp = kExpDenominator + 1;\nstatic constexpr uint8_t kExpNumeratorDown = kExpDenominator - 1;\n\n> +\n> +void IPASoftSimple::updateExposure(double exposureMSV)\n> +{\n> + int next;\n> +\n> + if (exposureMSV < EXPOSURE_OPTIMAL_VALUE - EXPOSURE_SATISFACTORY_OFFSET) {\n> + next = exposure_ * UP_NUMERATOR / DENOMINATOR;\n> + if (next - exposure_ < 1)\n> + exposure_ += 1;\n> + else\n> + exposure_ = next;\n> + if (exposure_ >= exposure_max_) {\n> + next = again_ * UP_NUMERATOR / DENOMINATOR;\n> + if (next - again_ < 1)\n> + again_ += 1;\n> + else\n> + again_ = next;\n> + }\n> + }\n> +\n> + if (exposureMSV > EXPOSURE_OPTIMAL_VALUE + EXPOSURE_SATISFACTORY_OFFSET) {\n> + if (exposure_ == exposure_max_ && again_ != again_min_) {\n> + next = again_ * DOWN_NUMERATOR / DENOMINATOR;\n> + if (again_ - next < 1)\n> + again_ -= 1;\n> + else\n> + again_ = next;\n> + } else {\n> + next = exposure_ * DOWN_NUMERATOR / DENOMINATOR;\n> + if (exposure_ - next < 1)\n> + exposure_ -= 1;\n> + else\n> + exposure_ = next;\n> + }\n> + }\n> +\n> + if (exposure_ > exposure_max_)\n> + exposure_ = exposure_max_;\n> + else if (exposure_ < exposure_min_)\n> + exposure_ = exposure_min_;\n> +\n> + if (again_ > again_max_)\n> + again_ = again_max_;\n> + else if (again_ < again_min_)\n> + again_ = again_min_;\n\nHow about\n\texposure_ = std::clamp(exposure_, exposure_min_, exposure_max_);\n \tagain_ = std::clamp(again_, again_min_, again_max_);\n\n?\n\n\n> +}\n> +\n> +} /* namespace ipa::soft */\n> +\n> +/*\n> + * External IPA module interface\n> + */\n> +extern \"C\" {\n> +const struct IPAModuleInfo ipaModuleInfo = {\n> + IPA_MODULE_API_VERSION,\n> + 0,\n> + \"SimplePipelineHandler\",\n> + \"simple\",\n> +};\n> +\n> +IPAInterface *ipaCreate()\n> +{\n> + return new ipa::soft::IPASoftSimple();\n> +}\n> +\n> +} /* extern \"C\" */\n> +\n> +} /* namespace libcamera */\n> -- \n> 2.43.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1DAD4BF415\n\tfor ;\n\tTue, 20 Feb 2024 13:05:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 78EBC61CAA;\n\tTue, 20 Feb 2024 14:05:15 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 861DA61CA3\n\tfor ;\n\tTue, 20 Feb 2024 14:05:13 +0100 (CET)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 18D1E899;\n\tTue, 20 Feb 2024 14:05:06 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"rnTdqNpJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1708434306;\n\tbh=s9H6L9x/7j41zmowJ/6b0B3Pm6M6EXkQ5EG0Oi0YCiU=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=rnTdqNpJdee2/nnsV9rR6A+HChgrg7FQShUIz1vMcxwJM+BcBKoAoW1pLCV8EFiRz\n\tasffI3kJEdQCKWE/K/G4qMFyX7dv0glqhqa+frsA3YcyklHGQnZA711jNKPx4dDFzl\n\tDmCsEAc1GBSbFiGJu78JsP7N30hX9GlM2IfgF/j8=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240214170122.60754-10-hdegoede@redhat.com>","References":"<20240214170122.60754-1-hdegoede@redhat.com>\n\t<20240214170122.60754-10-hdegoede@redhat.com>","Subject":"Re: [PATCH v3 09/16] libcamera: ipa: add Soft IPA","From":"Kieran Bingham ","To":"Hans de Goede , libcamera-devel@lists.libcamera.org","Date":"Tue, 20 Feb 2024 13:05:10 +0000","Message-ID":"<170843431013.1011926.9612134639156491265@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Maxime Ripard , Marttico ,\n\tToon Langendam , Pavel Machek , \n\tBryan O'Donoghue ,\n\tDennis Bonke ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28703,"web_url":"https://patchwork.libcamera.org/comment/28703/","msgid":"<6c9fe0c7-b97d-4d8c-bd29-25bd36a3a6ff@gmail.com>","date":"2024-02-20T15:23:02","subject":"Re: [PATCH v3 09/16] libcamera: ipa: add Soft IPA","submitter":{"id":179,"url":"https://patchwork.libcamera.org/api/people/179/","name":"Andrei Konovalov","email":"andrey.konovalov.ynk@gmail.com"},"content":"On 20.02.2024 16:05, Kieran Bingham wrote:\n> Quoting Hans de Goede (2024-02-14 17:01:13)\n>> From: Andrey Konovalov \n>>\n>> Define the Soft IPA main and event interfaces, add the Soft IPA\n>> implementation.\n>>\n>> The current src/ipa/meson.build assumes the IPA name to match the\n>> pipeline name. For this reason \"-Dipas=simple\" is used for the\n>> Soft IPA module.\n>>\n>> Auto exposure/gain and AWB implementation by Dennis, Toon and Martti.\n>>\n>> Auto exposure/gain targets a Mean Sample Value of 2.5 following\n>> the MSV calculation algorithm from:\n>> https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf\n>>\n>> Tested-by: Bryan O'Donoghue # sc8280xp Lenovo x13s\n>> Tested-by: Pavel Machek \n>> Reviewed-by: Pavel Machek \n>> Signed-off-by: Andrey Konovalov \n>> Co-developed-by: Dennis Bonke \n>> Signed-off-by: Dennis Bonke \n>> Co-developed-by: Marttico \n>> Signed-off-by: Marttico \n>> Co-developed-by: Toon Langendam \n>> Signed-off-by: Toon Langendam \n>> Signed-off-by: Hans de Goede \n>> ---\n>> Changes in v3:\n>> - Use new SwIspStats::kYHistogramSize constexpr and adjust\n>> the auto-exposure/-gain code so that it can deal with\n>> that having a different value then 16 (modify the loop\n>> to divide the histogram in 5 bins to not have hardcoded\n>> values)\n>> - Rename a few foo_bar symbols to fooBar\n>> ---\n>> Documentation/Doxyfile.in | 1 +\n>> include/libcamera/ipa/meson.build | 1 +\n>> include/libcamera/ipa/soft.mojom | 28 +++\n>> meson_options.txt | 2 +-\n>> src/ipa/simple/data/meson.build | 9 +\n>> src/ipa/simple/data/soft.conf | 3 +\n>> src/ipa/simple/meson.build | 25 +++\n>> src/ipa/simple/soft_simple.cpp | 308 ++++++++++++++++++++++++++++++\n>> 8 files changed, 376 insertions(+), 1 deletion(-)\n>> create mode 100644 include/libcamera/ipa/soft.mojom\n>> create mode 100644 src/ipa/simple/data/meson.build\n>> create mode 100644 src/ipa/simple/data/soft.conf\n>> create mode 100644 src/ipa/simple/meson.build\n>> create mode 100644 src/ipa/simple/soft_simple.cpp\n>>\n>> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n>> index a86ea6c1..2be8d47b 100644\n>> --- a/Documentation/Doxyfile.in\n>> +++ b/Documentation/Doxyfile.in\n>> @@ -44,6 +44,7 @@ EXCLUDE = @TOP_SRCDIR@/include/libcamera/base/span.h \\\n>> @TOP_SRCDIR@/src/libcamera/pipeline/ \\\n>> @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \\\n>> @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \\\n>> + @TOP_BUILDDIR@/include/libcamera/ipa/soft_ipa_interface.h \\\n>> @TOP_BUILDDIR@/src/libcamera/proxy/\n>> \n>> EXCLUDE_PATTERNS = @TOP_BUILDDIR@/include/libcamera/ipa/*_serializer.h \\\n>> diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build\n>> index f3b4881c..3352d08f 100644\n>> --- a/include/libcamera/ipa/meson.build\n>> +++ b/include/libcamera/ipa/meson.build\n>> @@ -65,6 +65,7 @@ pipeline_ipa_mojom_mapping = {\n>> 'ipu3': 'ipu3.mojom',\n>> 'rkisp1': 'rkisp1.mojom',\n>> 'rpi/vc4': 'raspberrypi.mojom',\n>> + 'simple': 'soft.mojom',\n>> 'vimc': 'vimc.mojom',\n>> }\n>> \n>> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom\n>> new file mode 100644\n>> index 00000000..c249bd75\n>> --- /dev/null\n>> +++ b/include/libcamera/ipa/soft.mojom\n>> @@ -0,0 +1,28 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +\n>> +/*\n>> + * \\todo Document the interface and remove the related EXCLUDE_PATTERNS entry.\n>> + */\n>> +\n>> +module ipa.soft;\n>> +\n>> +import \"include/libcamera/ipa/core.mojom\";\n>> +\n>> +interface IPASoftInterface {\n>> + init(libcamera.IPASettings settings,\n>> + libcamera.SharedFD fdStats,\n>> + libcamera.SharedFD fdParams,\n>> + libcamera.ControlInfoMap sensorCtrlInfoMap)\n>> + => (int32 ret);\n>> + start() => (int32 ret);\n>> + stop();\n>> + configure(libcamera.ControlInfoMap sensorCtrlInfoMap)\n>> + => (int32 ret);\n>> +\n>> + [async] processStats(libcamera.ControlList sensorControls);\n>> +};\n>> +\n>> +interface IPASoftEventInterface {\n>> + setSensorControls(libcamera.ControlList sensorControls);\n>> + setIspParams(int32 dummy);\n>> +};\n>> diff --git a/meson_options.txt b/meson_options.txt\n>> index 5fdc7be8..94372e47 100644\n>> --- a/meson_options.txt\n>> +++ b/meson_options.txt\n>> @@ -27,7 +27,7 @@ option('gstreamer',\n>> \n>> option('ipas',\n>> type : 'array',\n>> - choices : ['ipu3', 'rkisp1', 'rpi/vc4', 'vimc'],\n>> + choices : ['ipu3', 'rkisp1', 'rpi/vc4', 'simple', 'vimc'],\n>> description : 'Select which IPA modules to build')\n>> \n>> option('lc-compliance',\n>> diff --git a/src/ipa/simple/data/meson.build b/src/ipa/simple/data/meson.build\n>> new file mode 100644\n>> index 00000000..33548cc6\n>> --- /dev/null\n>> +++ b/src/ipa/simple/data/meson.build\n>> @@ -0,0 +1,9 @@\n>> +# SPDX-License-Identifier: CC0-1.0\n>> +\n>> +conf_files = files([\n>> + 'soft.conf',\n>> +])\n>> +\n>> +install_data(conf_files,\n>> + install_dir : ipa_data_dir / 'soft',\n>> + install_tag : 'runtime')\n>> diff --git a/src/ipa/simple/data/soft.conf b/src/ipa/simple/data/soft.conf\n>> new file mode 100644\n>> index 00000000..0c70e7c0\n>> --- /dev/null\n>> +++ b/src/ipa/simple/data/soft.conf\n>> @@ -0,0 +1,3 @@\n>> +# SPDX-License-Identifier: LGPL-2.1-or-later\n>> +#\n>> +# Dummy configuration file for the soft IPA.\n>> diff --git a/src/ipa/simple/meson.build b/src/ipa/simple/meson.build\n>> new file mode 100644\n>> index 00000000..3e863db7\n>> --- /dev/null\n>> +++ b/src/ipa/simple/meson.build\n>> @@ -0,0 +1,25 @@\n>> +# SPDX-License-Identifier: CC0-1.0\n>> +\n>> +ipa_name = 'ipa_soft_simple'\n>> +\n>> +mod = shared_module(ipa_name,\n>> + ['soft_simple.cpp', libcamera_generated_ipa_headers],\n>> + name_prefix : '',\n>> + include_directories : [ipa_includes, libipa_includes],\n>> + dependencies : libcamera_private,\n>> + link_with : libipa,\n>> + install : true,\n>> + install_dir : ipa_install_dir)\n>> +\n>> +if ipa_sign_module\n>> + custom_target(ipa_name + '.so.sign',\n>> + input : mod,\n>> + output : ipa_name + '.so.sign',\n>> + command : [ipa_sign, ipa_priv_key, '@INPUT@', '@OUTPUT@'],\n>> + install : false,\n>> + build_by_default : true)\n>> +endif\n>> +\n>> +subdir('data')\n>> +\n>> +ipa_names += ipa_name\n>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n>> new file mode 100644\n>> index 00000000..f7ac02f9\n>> --- /dev/null\n>> +++ b/src/ipa/simple/soft_simple.cpp\n>> @@ -0,0 +1,308 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2023, Linaro Ltd\n>> + *\n>> + * soft_simple.cpp - Simple Software Image Processing Algorithm module\n>> + */\n>> +\n>> +#include \n>> +\n>> +#include \n>> +#include \n>> +#include \n>> +\n>> +#include \n>> +#include \n>> +\n>> +#include \n>> +#include \n>> +#include \n>> +\n>> +#include \"libcamera/internal/camera_sensor.h\"\n>> +#include \"libcamera/internal/software_isp/debayer_params.h\"\n>> +#include \"libcamera/internal/software_isp/swisp_stats.h\"\n>> +\n>> +#define EXPOSURE_OPTIMAL_VALUE 2.5\n>> +#define EXPOSURE_SATISFACTORY_OFFSET 0.2\n> \n> Perhaps:\n> \n> static constexpr float kExposureOptimal = 2.5;\n> static constexpr float kExposureSatisfactory = 0.2;\n> \n> But can those constants have some more comments or documentation about\n> /why/ they are optimal and satisfactory? Where do they come from? what\n> are the ranges? Anything applicable to say about them?\n\nI second the oppinion that some more comments here would be useful.\n\nTo my understanding, kExposureOptimal = ExposureBinsCount / 2.0;\n(That is, the mean sample value of the histogram should be in the middle of the range.)\n\nAnd kExposureSatisfactory is the hesteresys to prevent the exposure from wobbling around\nthe optimal exposure. Some empirically selected value small enough to have the exposure\nclose to the optimal, and big enough to prevent the jitter.\n\nPlease correct me if I am wrong.\n\nStill here is what is not quite clear to me, and the paper84final.pdf doesn't seem to\nexplain:\n\nSwIspStats::kYHistogramSize is 16. For AE 5 bins are used.\nWhat is the advantage of reassembling the 16 bins from SwIspStats into 5\n(vs e.g. setting ExposureBinsCount to SwIspStats::kYHistogramSize and using 16 bins) ?\n\n>> +\n>> +namespace libcamera {\n>> +\n>> +LOG_DEFINE_CATEGORY(IPASoft)\n>> +\n>> +namespace ipa::soft {\n>> +\n>> +class IPASoftSimple : public ipa::soft::IPASoftInterface\n>> +{\n>> +public:\n>> + IPASoftSimple()\n>> + : ignore_updates_(0)\n>> + {\n>> + }\n>> +\n>> + ~IPASoftSimple()\n>> + {\n>> + if (stats_)\n>> + munmap(stats_, sizeof(SwIspStats));\n>> + if (params_)\n>> + munmap(params_, sizeof(DebayerParams));\n>> + }\n>> +\n>> + int init(const IPASettings &settings,\n>> + const SharedFD &fdStats,\n>> + const SharedFD &fdParams,\n>> + const ControlInfoMap &sensorInfoMap) override;\n>> + int configure(const ControlInfoMap &sensorInfoMap) override;\n>> +\n>> + int start() override;\n>> + void stop() override;\n>> +\n>> + void processStats(const ControlList &sensorControls) override;\n>> +\n>> +private:\n>> + void updateExposure(double exposureMSV);\n>> +\n>> + SharedFD fdStats_;\n>> + SharedFD fdParams_;\n>> + DebayerParams *params_;\n>> + SwIspStats *stats_;\n> \n> I'd throw a blank line between class instances and the following PODs\n> \n>> + int exposure_min_, exposure_max_;\n>> + int again_min_, again_max_;\n>> + int again_, exposure_;\n>> + int ignore_updates_;\n> \n> Should those by unsigned? Can they ever be 0 or less than zero?\n\nunsigned would be fine\n\n>> +};\n>> +\n>> +int IPASoftSimple::init([[maybe_unused]] const IPASettings &settings,\n>> + const SharedFD &fdStats,\n>> + const SharedFD &fdParams,\n>> + const ControlInfoMap &sensorInfoMap)\n>> +{\n>> + fdStats_ = std::move(fdStats);\n>> + if (!fdStats_.isValid()) {\n>> + LOG(IPASoft, Error) << \"Invalid Statistics handle\";\n>> + return -ENODEV;\n>> + }\n>> +\n>> + fdParams_ = std::move(fdParams);\n>> + if (!fdParams_.isValid()) {\n>> + LOG(IPASoft, Error) << \"Invalid Parameters handle\";\n>> + return -ENODEV;\n>> + }\n>> +\n>> + params_ = static_cast(mmap(nullptr, sizeof(DebayerParams),\n>> + PROT_WRITE, MAP_SHARED,\n>> + fdParams_.get(), 0));\n> \n> We'd usually use MappedFrameBuffer() here so that the mappings are\n> handled by RAII here\n\nOK, will fix that.\n\n>> + if (!params_) {\n>> + LOG(IPASoft, Error) << \"Unable to map Parameters\";\n>> + return -ENODEV;\n>> + }\n>> +\n>> + stats_ = static_cast(mmap(nullptr, sizeof(SwIspStats),\n>> + PROT_READ, MAP_SHARED,\n>> + fdStats_.get(), 0));\n>> + if (!stats_) {\n>> + LOG(IPASoft, Error) << \"Unable to map Statistics\";\n>> + return -ENODEV;\n>> + }\n> \n> Same here.\n\nAck\n\n>> +\n>> + if (sensorInfoMap.find(V4L2_CID_EXPOSURE) == sensorInfoMap.end()) {\n>> + LOG(IPASoft, Error) << \"Don't have exposure control\";\n>> + return -EINVAL;\n>> + }\n>> +\n>> + if (sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN) == sensorInfoMap.end()) {\n>> + LOG(IPASoft, Error) << \"Don't have gain control\";\n>> + return -EINVAL;\n>> + }\n>> +\n> \n> In the future I would expect AEGC to be broken out to it's own modular\n> 'Algorithm' (see rkisp1 for the createAlgorithms()\n\nThanks, I'll take a look.\n\n> We're also lacking any set up or registration for control handling. I\n> guess for now there may be none, but do you plan to have controls? I'm\n> fine with those being added later.\n\nI'd stick to what we currently have (none), and revisit this later.\n\n>> + return 0;\n>> +}\n>> +\n>> +int IPASoftSimple::configure(const ControlInfoMap &sensorInfoMap)\n>> +{\n>> + const ControlInfo &exposure_info = sensorInfoMap.find(V4L2_CID_EXPOSURE)->second;\n>> + const ControlInfo &gain_info = sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN)->second;\n>> +\n>> + exposure_min_ = exposure_info.min().get();\n>> + if (!exposure_min_) {\n>> + LOG(IPASoft, Warning) << \"Minimum exposure is zero, that can't be linear\";\n>> + exposure_min_ = 1;\n>> + }\n>> + exposure_max_ = exposure_info.max().get();\n>> + again_min_ = gain_info.min().get();\n>> + if (!again_min_) {\n>> + LOG(IPASoft, Warning) << \"Minimum gain is zero, that can't be linear\";\n>> + again_min_ = 100;\n>> + }\n> \n> This is where you should almost certainly be looking at the\n> CameraSensorHelpers. We expect the gain model to be abstracted by each\n> sensor through the helpers.\n> \n> IPA algorithms should operate on a linear model, and the\n> CameraSensorHelpers will convert any logarithmic gain codes to linear on\n> a per-sensor basis, as well as impose any sensor specific limits.\n\nI am going to draft a patch to use the CameraSensorHelpers before the end of this week\n(hopefully sooner).\n\n>> + again_max_ = gain_info.max().get();\n>> +\n>> + LOG(IPASoft, Info) << \"Exposure \" << exposure_min_ << \"-\" << exposure_max_\n>> + << \", gain \" << again_min_ << \"-\" << again_max_;\n>> +\n>> + return 0;\n>> +}\n>> +\n>> +int IPASoftSimple::start()\n>> +{\n>> + return 0;\n>> +}\n>> +\n>> +void IPASoftSimple::stop()\n>> +{\n>> +}\n>> +\n>> +void IPASoftSimple::processStats(const ControlList &sensorControls)\n>> +{\n>> + /*\n>> + * Calculate red and blue gains for AWB.\n>> + * Clamp max gain at 4.0, this also avoids 0 division.\n>> + */\n>> + if (stats_->sumR_ <= stats_->sumG_ / 4)\n>> + params_->gainR = 1024;\n>> + else\n>> + params_->gainR = 256 * stats_->sumG_ / stats_->sumR_;\n>> +\n>> + if (stats_->sumB_ <= stats_->sumG_ / 4)\n>> + params_->gainB = 1024;\n>> + else\n>> + params_->gainB = 256 * stats_->sumG_ / stats_->sumB_;\n>> +\n>> + /* Green gain and gamma values are fixed */\n>> + params_->gainG = 256;\n>> + params_->gamma = 0.5;\n>> +\n>> + setIspParams.emit(0);\n>> +\n>> + /*\n>> + * AE / AGC, use 2 frames delay to make sure that the exposure and\n>> + * the gain set have applied to the camera sensor.\n>> + */\n> \n> Using known sensor specific delays defined in the CameraSensorHelper and\n> DelayedControls should be considered here in the future. But as a first\n> implementation for CPU only, where doing /less/ work is helpful - this\n> is fine for me.\n\nAgreed.\n\n>> + if (ignore_updates_ > 0) {\n>> + --ignore_updates_;\n>> + return;\n>> + }\n>> +\n>> + /*\n>> + * Calculate Mean Sample Value (MSV) according to formula from:\n>> + * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf\n>> + */\n>> + constexpr unsigned int ExposureBinsCount = 5;\n>> + constexpr unsigned int yHistValsPerBin =\n>> + SwIspStats::kYHistogramSize / ExposureBinsCount;\n>> + constexpr unsigned int yHistValsPerBinMod =\n>> + SwIspStats::kYHistogramSize /\n>> + (SwIspStats::kYHistogramSize % ExposureBinsCount + 1);\n>> + int ExposureBins[ExposureBinsCount] = {};\n>> + unsigned int denom = 0;\n>> + unsigned int num = 0;\n>> +\n>> + for (unsigned int i = 0; i < SwIspStats::kYHistogramSize; i++) {\n>> + unsigned int idx = (i - (i / yHistValsPerBinMod)) / yHistValsPerBin;\n>> + ExposureBins[idx] += stats_->yHistogram[i];\n>> + }\n>> +\n>> + for (unsigned int i = 0; i < ExposureBinsCount; i++) {\n>> + LOG(IPASoft, Debug) << i << \": \" << ExposureBins[i];\n>> + denom += ExposureBins[i];\n>> + num += ExposureBins[i] * (i + 1);\n>> + }\n>> +\n>> + float exposureMSV = (float)num / denom;\n>> +\n>> + /* sanity check */\n>> + if (!sensorControls.contains(V4L2_CID_EXPOSURE) ||\n>> + !sensorControls.contains(V4L2_CID_ANALOGUE_GAIN)) {\n>> + LOG(IPASoft, Error) << \"Control(s) missing\";\n>> + return;\n>> + }\n>> +\n>> + ControlList ctrls(sensorControls);\n>> +\n>> + exposure_ = ctrls.get(V4L2_CID_EXPOSURE).get();\n>> + again_ = ctrls.get(V4L2_CID_ANALOGUE_GAIN).get();\n>> +\n>> + updateExposure(exposureMSV);\n>> +\n>> + ctrls.set(V4L2_CID_EXPOSURE, exposure_);\n>> + ctrls.set(V4L2_CID_ANALOGUE_GAIN, again_);\n>> +\n>> + ignore_updates_ = 2;\n>> +\n>> + setSensorControls.emit(ctrls);\n>> +\n>> + LOG(IPASoft, Debug) << \"exposureMSV \" << exposureMSV\n>> + << \" exp \" << exposure_ << \" again \" << again_\n>> + << \" gain R/B \" << params_->gainR << \"/\" << params_->gainB;\n>> +}\n>> +\n>> +/* DENOMINATOR of 10 gives ~10% increment/decrement; DENOMINATOR of 5 - about ~20% */\n>> +#define DENOMINATOR 10\n>> +#define UP_NUMERATOR (DENOMINATOR + 1)\n>> +#define DOWN_NUMERATOR (DENOMINATOR - 1)\n> \n> Our C++ code style for constants would be more like:\n> \n> static constexpr uint8_t kExpDenominator = 10;\n> static constexpr uint8_t kExpNumeratorUp = kExpDenominator + 1;\n> static constexpr uint8_t kExpNumeratorDown = kExpDenominator - 1;\n\nThis was written by me, and I'll update this part to use static constexpr.\n\n>> +\n>> +void IPASoftSimple::updateExposure(double exposureMSV)\n>> +{\n>> + int next;\n>> +\n>> + if (exposureMSV < EXPOSURE_OPTIMAL_VALUE - EXPOSURE_SATISFACTORY_OFFSET) {\n>> + next = exposure_ * UP_NUMERATOR / DENOMINATOR;\n>> + if (next - exposure_ < 1)\n>> + exposure_ += 1;\n>> + else\n>> + exposure_ = next;\n>> + if (exposure_ >= exposure_max_) {\n>> + next = again_ * UP_NUMERATOR / DENOMINATOR;\n>> + if (next - again_ < 1)\n>> + again_ += 1;\n>> + else\n>> + again_ = next;\n>> + }\n>> + }\n>> +\n>> + if (exposureMSV > EXPOSURE_OPTIMAL_VALUE + EXPOSURE_SATISFACTORY_OFFSET) {\n>> + if (exposure_ == exposure_max_ && again_ != again_min_) {\n>> + next = again_ * DOWN_NUMERATOR / DENOMINATOR;\n>> + if (again_ - next < 1)\n>> + again_ -= 1;\n>> + else\n>> + again_ = next;\n>> + } else {\n>> + next = exposure_ * DOWN_NUMERATOR / DENOMINATOR;\n>> + if (exposure_ - next < 1)\n>> + exposure_ -= 1;\n>> + else\n>> + exposure_ = next;\n>> + }\n>> + }\n>> +\n>> + if (exposure_ > exposure_max_)\n>> + exposure_ = exposure_max_;\n>> + else if (exposure_ < exposure_min_)\n>> + exposure_ = exposure_min_;\n>> +\n>> + if (again_ > again_max_)\n>> + again_ = again_max_;\n>> + else if (again_ < again_min_)\n>> + again_ = again_min_;\n> \n> How about\n> \texposure_ = std::clamp(exposure_, exposure_min_, exposure_max_);\n> \tagain_ = std::clamp(again_, again_min_, again_max_);\n> \n> ?\n\nOK, will fix.\n\nThanks,\nAndrei\n\n>> +}\n>> +\n>> +} /* namespace ipa::soft */\n>> +\n>> +/*\n>> + * External IPA module interface\n>> + */\n>> +extern \"C\" {\n>> +const struct IPAModuleInfo ipaModuleInfo = {\n>> + IPA_MODULE_API_VERSION,\n>> + 0,\n>> + \"SimplePipelineHandler\",\n>> + \"simple\",\n>> +};\n>> +\n>> +IPAInterface *ipaCreate()\n>> +{\n>> + return new ipa::soft::IPASoftSimple();\n>> +}\n>> +\n>> +} /* extern \"C\" */\n>> +\n>> +} /* namespace libcamera */\n>> -- \n>> 2.43.0\n>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id CBFDBC3257\n\tfor ;\n\tTue, 20 Feb 2024 15:23:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E07EB62807;\n\tTue, 20 Feb 2024 16:23:06 +0100 (CET)","from mail-lj1-x234.google.com (mail-lj1-x234.google.com\n\t[IPv6:2a00:1450:4864:20::234])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DE37B61CA3\n\tfor ;\n\tTue, 20 Feb 2024 16:23:04 +0100 (CET)","by mail-lj1-x234.google.com with SMTP id\n\t38308e7fff4ca-2d1094b549cso81496241fa.3\n\tfor ;\n\tTue, 20 Feb 2024 07:23:04 -0800 (PST)","from [192.168.118.26] ([87.116.160.233])\n\tby smtp.gmail.com with ESMTPSA id\n\tn6-20020a05600c3b8600b004126a6ee498sm4541033wms.12.2024.02.20.07.23.02\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tTue, 20 Feb 2024 07:23:03 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"JTdEtXKn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=gmail.com; s=20230601; t=1708442584; x=1709047384;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:in-reply-to:from:references:cc:to\n\t:content-language:subject:user-agent:mime-version:date:message-id\n\t:from:to:cc:subject:date:message-id:reply-to;\n\tbh=4yhl4n9JeEzKyfwG9gb1RWsoOBS/1+bSZ8Ffy35lHIc=;\n\tb=JTdEtXKnumJANfYN8GxIGDNGWD9zpitojNZ7OK5rvanoquxNv3eMWpbpyYvPd5TTWm\n\tQ4TvrDrl1k3yHZruPKVo4nal03F0oqrw2MNEJCsGP0tREF7DK8FChlcm0jD9QxU69nwW\n\t40SXJh+Ks2EAXi5hvhIPpmlFYd3sXPuM8xTvsG5Z/LFthRgBVoFybiVYh6Wm1osw28eZ\n\tps5/VhthrFewu6b+s3nVmhrLaBsA1jsgnNRsnLsABDHyTrdPWhSs1TlmtJtvH/zU2ASV\n\tBGh7IVpx2ySSsRgQ/0ugoy24UEyGQn5+sh4RiJT9hzDd21LIvv18qgdl5QfTmxhEVQbd\n\tMugw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1708442584; x=1709047384;\n\th=content-transfer-encoding:in-reply-to:from:references:cc:to\n\t:content-language:subject:user-agent:mime-version:date:message-id\n\t:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;\n\tbh=4yhl4n9JeEzKyfwG9gb1RWsoOBS/1+bSZ8Ffy35lHIc=;\n\tb=RbHpvBpmY0Ummu7I4D3Ks/3eSesyMlBLqfNrtPOmlEr3XEEJFnk+4M8ybeMqYgpNrZ\n\tbDEprd6ZrlTvOc2qSNrD0WWyC/Idwn//B2aUMIPltzzMiZ3HzviMw3YhOLXqTj+x9prw\n\tu6FL7TsSb+/jklEFyvAFCU5XL/IEzNbti3yGohKXAcl8weECoUyr+OkyNW8eLs0UQe1e\n\tuIlGWkMS069PYG/CuVW/T0hZpcbNjdMTK/3iWRkh7VPbETKOZEATngHOqGwerAgHq3jC\n\tr7580CryQikVNI99Q40G70q5s9of2gdPjWEqVJsmP1AvYjUt8Ce7z0neFThaXbRF8Ixq\n\tR/0w==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCWT9LO4S9zp5gvyPHfw7DBS+z7FULLP4b958b7viW+dpV4906iQaulnAcq0O4L+Ro49+H06hDLyvO2mAXxbFu12muMfAMjQFCf5S9mWwIfYA2bTBA==","X-Gm-Message-State":"AOJu0YygQoQYI+ifO1PRnsFH1dB449SLhwON0wN9o3JfeQhnFe10VjfM\n\taELnVqdtNuw5ynaG33HWW1N1dpKMp3tBpUwieIJilO7Wf/s3fpG1","X-Google-Smtp-Source":"AGHT+IFeadFltKIED49oM/tj/WdWcpsxcCpeICczBsdVDXNTf0VUKXfNwzRSFQHFWdZP0qYoy4Z8pg==","X-Received":"by 2002:a2e:a545:0:b0:2d2:3dd7:813d with SMTP id\n\te5-20020a2ea545000000b002d23dd7813dmr4088224ljn.17.1708442583616; \n\tTue, 20 Feb 2024 07:23:03 -0800 (PST)","Message-ID":"<6c9fe0c7-b97d-4d8c-bd29-25bd36a3a6ff@gmail.com>","Date":"Tue, 20 Feb 2024 18:23:02 +0300","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v3 09/16] libcamera: ipa: add Soft IPA","Content-Language":"en-US","To":"Kieran Bingham ,\n\tHans de Goede , libcamera-devel@lists.libcamera.org","References":"<20240214170122.60754-1-hdegoede@redhat.com>\n\t<20240214170122.60754-10-hdegoede@redhat.com>\n\t<170843431013.1011926.9612134639156491265@ping.linuxembedded.co.uk>","From":"Andrei Konovalov ","In-Reply-To":"<170843431013.1011926.9612134639156491265@ping.linuxembedded.co.uk>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Maxime Ripard , Marttico ,\n\tToon Langendam , Pavel Machek , \n\tBryan O'Donoghue ,\n\tDennis Bonke ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28704,"web_url":"https://patchwork.libcamera.org/comment/28704/","msgid":"<170844439906.1011926.2188078205048491174@ping.linuxembedded.co.uk>","date":"2024-02-20T15:53:19","subject":"Re: [PATCH v3 09/16] libcamera: ipa: add Soft IPA","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Andrei Konovalov (2024-02-20 15:23:02)\n> On 20.02.2024 16:05, Kieran Bingham wrote:\n> > Quoting Hans de Goede (2024-02-14 17:01:13)\n> >> From: Andrey Konovalov \n> >>\n> >> Define the Soft IPA main and event interfaces, add the Soft IPA\n> >> implementation.\n> >>\n> >> The current src/ipa/meson.build assumes the IPA name to match the\n> >> pipeline name. For this reason \"-Dipas=simple\" is used for the\n> >> Soft IPA module.\n> >>\n> >> Auto exposure/gain and AWB implementation by Dennis, Toon and Martti.\n> >>\n> >> Auto exposure/gain targets a Mean Sample Value of 2.5 following\n> >> the MSV calculation algorithm from:\n> >> https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf\n> >>\n> >> Tested-by: Bryan O'Donoghue # sc8280xp Lenovo x13s\n> >> Tested-by: Pavel Machek \n> >> Reviewed-by: Pavel Machek \n> >> Signed-off-by: Andrey Konovalov \n> >> Co-developed-by: Dennis Bonke \n> >> Signed-off-by: Dennis Bonke \n> >> Co-developed-by: Marttico \n> >> Signed-off-by: Marttico \n> >> Co-developed-by: Toon Langendam \n> >> Signed-off-by: Toon Langendam \n> >> Signed-off-by: Hans de Goede \n> >> ---\n> >> Changes in v3:\n> >> - Use new SwIspStats::kYHistogramSize constexpr and adjust\n> >> the auto-exposure/-gain code so that it can deal with\n> >> that having a different value then 16 (modify the loop\n> >> to divide the histogram in 5 bins to not have hardcoded\n> >> values)\n> >> - Rename a few foo_bar symbols to fooBar\n> >> ---\n> >> Documentation/Doxyfile.in | 1 +\n> >> include/libcamera/ipa/meson.build | 1 +\n> >> include/libcamera/ipa/soft.mojom | 28 +++\n> >> meson_options.txt | 2 +-\n> >> src/ipa/simple/data/meson.build | 9 +\n> >> src/ipa/simple/data/soft.conf | 3 +\n> >> src/ipa/simple/meson.build | 25 +++\n> >> src/ipa/simple/soft_simple.cpp | 308 ++++++++++++++++++++++++++++++\n> >> 8 files changed, 376 insertions(+), 1 deletion(-)\n> >> create mode 100644 include/libcamera/ipa/soft.mojom\n> >> create mode 100644 src/ipa/simple/data/meson.build\n> >> create mode 100644 src/ipa/simple/data/soft.conf\n> >> create mode 100644 src/ipa/simple/meson.build\n> >> create mode 100644 src/ipa/simple/soft_simple.cpp\n> >>\n> >> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> >> index a86ea6c1..2be8d47b 100644\n> >> --- a/Documentation/Doxyfile.in\n> >> +++ b/Documentation/Doxyfile.in\n> >> @@ -44,6 +44,7 @@ EXCLUDE = @TOP_SRCDIR@/include/libcamera/base/span.h \\\n> >> @TOP_SRCDIR@/src/libcamera/pipeline/ \\\n> >> @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \\\n> >> @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \\\n> >> + @TOP_BUILDDIR@/include/libcamera/ipa/soft_ipa_interface.h \\\n> >> @TOP_BUILDDIR@/src/libcamera/proxy/\n> >> \n> >> EXCLUDE_PATTERNS = @TOP_BUILDDIR@/include/libcamera/ipa/*_serializer.h \\\n> >> diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build\n> >> index f3b4881c..3352d08f 100644\n> >> --- a/include/libcamera/ipa/meson.build\n> >> +++ b/include/libcamera/ipa/meson.build\n> >> @@ -65,6 +65,7 @@ pipeline_ipa_mojom_mapping = {\n> >> 'ipu3': 'ipu3.mojom',\n> >> 'rkisp1': 'rkisp1.mojom',\n> >> 'rpi/vc4': 'raspberrypi.mojom',\n> >> + 'simple': 'soft.mojom',\n> >> 'vimc': 'vimc.mojom',\n> >> }\n> >> \n> >> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom\n> >> new file mode 100644\n> >> index 00000000..c249bd75\n> >> --- /dev/null\n> >> +++ b/include/libcamera/ipa/soft.mojom\n> >> @@ -0,0 +1,28 @@\n> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> +\n> >> +/*\n> >> + * \\todo Document the interface and remove the related EXCLUDE_PATTERNS entry.\n> >> + */\n> >> +\n> >> +module ipa.soft;\n> >> +\n> >> +import \"include/libcamera/ipa/core.mojom\";\n> >> +\n> >> +interface IPASoftInterface {\n> >> + init(libcamera.IPASettings settings,\n> >> + libcamera.SharedFD fdStats,\n> >> + libcamera.SharedFD fdParams,\n> >> + libcamera.ControlInfoMap sensorCtrlInfoMap)\n> >> + => (int32 ret);\n> >> + start() => (int32 ret);\n> >> + stop();\n> >> + configure(libcamera.ControlInfoMap sensorCtrlInfoMap)\n> >> + => (int32 ret);\n> >> +\n> >> + [async] processStats(libcamera.ControlList sensorControls);\n> >> +};\n> >> +\n> >> +interface IPASoftEventInterface {\n> >> + setSensorControls(libcamera.ControlList sensorControls);\n> >> + setIspParams(int32 dummy);\n> >> +};\n> >> diff --git a/meson_options.txt b/meson_options.txt\n> >> index 5fdc7be8..94372e47 100644\n> >> --- a/meson_options.txt\n> >> +++ b/meson_options.txt\n> >> @@ -27,7 +27,7 @@ option('gstreamer',\n> >> \n> >> option('ipas',\n> >> type : 'array',\n> >> - choices : ['ipu3', 'rkisp1', 'rpi/vc4', 'vimc'],\n> >> + choices : ['ipu3', 'rkisp1', 'rpi/vc4', 'simple', 'vimc'],\n> >> description : 'Select which IPA modules to build')\n> >> \n> >> option('lc-compliance',\n> >> diff --git a/src/ipa/simple/data/meson.build b/src/ipa/simple/data/meson.build\n> >> new file mode 100644\n> >> index 00000000..33548cc6\n> >> --- /dev/null\n> >> +++ b/src/ipa/simple/data/meson.build\n> >> @@ -0,0 +1,9 @@\n> >> +# SPDX-License-Identifier: CC0-1.0\n> >> +\n> >> +conf_files = files([\n> >> + 'soft.conf',\n> >> +])\n> >> +\n> >> +install_data(conf_files,\n> >> + install_dir : ipa_data_dir / 'soft',\n> >> + install_tag : 'runtime')\n> >> diff --git a/src/ipa/simple/data/soft.conf b/src/ipa/simple/data/soft.conf\n> >> new file mode 100644\n> >> index 00000000..0c70e7c0\n> >> --- /dev/null\n> >> +++ b/src/ipa/simple/data/soft.conf\n> >> @@ -0,0 +1,3 @@\n> >> +# SPDX-License-Identifier: LGPL-2.1-or-later\n> >> +#\n> >> +# Dummy configuration file for the soft IPA.\n> >> diff --git a/src/ipa/simple/meson.build b/src/ipa/simple/meson.build\n> >> new file mode 100644\n> >> index 00000000..3e863db7\n> >> --- /dev/null\n> >> +++ b/src/ipa/simple/meson.build\n> >> @@ -0,0 +1,25 @@\n> >> +# SPDX-License-Identifier: CC0-1.0\n> >> +\n> >> +ipa_name = 'ipa_soft_simple'\n> >> +\n> >> +mod = shared_module(ipa_name,\n> >> + ['soft_simple.cpp', libcamera_generated_ipa_headers],\n> >> + name_prefix : '',\n> >> + include_directories : [ipa_includes, libipa_includes],\n> >> + dependencies : libcamera_private,\n> >> + link_with : libipa,\n> >> + install : true,\n> >> + install_dir : ipa_install_dir)\n> >> +\n> >> +if ipa_sign_module\n> >> + custom_target(ipa_name + '.so.sign',\n> >> + input : mod,\n> >> + output : ipa_name + '.so.sign',\n> >> + command : [ipa_sign, ipa_priv_key, '@INPUT@', '@OUTPUT@'],\n> >> + install : false,\n> >> + build_by_default : true)\n> >> +endif\n> >> +\n> >> +subdir('data')\n> >> +\n> >> +ipa_names += ipa_name\n> >> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n> >> new file mode 100644\n> >> index 00000000..f7ac02f9\n> >> --- /dev/null\n> >> +++ b/src/ipa/simple/soft_simple.cpp\n> >> @@ -0,0 +1,308 @@\n> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> +/*\n> >> + * Copyright (C) 2023, Linaro Ltd\n> >> + *\n> >> + * soft_simple.cpp - Simple Software Image Processing Algorithm module\n> >> + */\n> >> +\n> >> +#include \n> >> +\n> >> +#include \n> >> +#include \n> >> +#include \n> >> +\n> >> +#include \n> >> +#include \n> >> +\n> >> +#include \n> >> +#include \n> >> +#include \n> >> +\n> >> +#include \"libcamera/internal/camera_sensor.h\"\n> >> +#include \"libcamera/internal/software_isp/debayer_params.h\"\n> >> +#include \"libcamera/internal/software_isp/swisp_stats.h\"\n> >> +\n> >> +#define EXPOSURE_OPTIMAL_VALUE 2.5\n> >> +#define EXPOSURE_SATISFACTORY_OFFSET 0.2\n> > \n> > Perhaps:\n> > \n> > static constexpr float kExposureOptimal = 2.5;\n> > static constexpr float kExposureSatisfactory = 0.2;\n> > \n> > But can those constants have some more comments or documentation about\n> > /why/ they are optimal and satisfactory? Where do they come from? what\n> > are the ranges? Anything applicable to say about them?\n> \n> I second the oppinion that some more comments here would be useful.\n> \n> To my understanding, kExposureOptimal = ExposureBinsCount / 2.0;\n> (That is, the mean sample value of the histogram should be in the middle of the range.)\n\nYes, even better - if they are the result of other parameters,\nexpressing that is far more helpful to know that things will change if\nwe add more bins, or ... that they'll automatically adapt...\n\n> And kExposureSatisfactory is the hesteresys to prevent the exposure from wobbling around\n> the optimal exposure. Some empirically selected value small enough to have the exposure\n> close to the optimal, and big enough to prevent the jitter.\n\nSounds feasible to me - but wasn't clear just from seeing two numbers,\nso comments welcome ;-)\n\n> Please correct me if I am wrong.\n> \n> Still here is what is not quite clear to me, and the paper84final.pdf doesn't seem to\n> explain:\n> \n> SwIspStats::kYHistogramSize is 16. For AE 5 bins are used.\n> What is the advantage of reassembling the 16 bins from SwIspStats into 5\n> (vs e.g. setting ExposureBinsCount to SwIspStats::kYHistogramSize and using 16 bins) ?\n> \n> >> +\n> >> +namespace libcamera {\n> >> +\n> >> +LOG_DEFINE_CATEGORY(IPASoft)\n> >> +\n> >> +namespace ipa::soft {\n> >> +\n> >> +class IPASoftSimple : public ipa::soft::IPASoftInterface\n> >> +{\n> >> +public:\n> >> + IPASoftSimple()\n> >> + : ignore_updates_(0)\n> >> + {\n> >> + }\n> >> +\n> >> + ~IPASoftSimple()\n> >> + {\n> >> + if (stats_)\n> >> + munmap(stats_, sizeof(SwIspStats));\n> >> + if (params_)\n> >> + munmap(params_, sizeof(DebayerParams));\n> >> + }\n> >> +\n> >> + int init(const IPASettings &settings,\n> >> + const SharedFD &fdStats,\n> >> + const SharedFD &fdParams,\n> >> + const ControlInfoMap &sensorInfoMap) override;\n> >> + int configure(const ControlInfoMap &sensorInfoMap) override;\n> >> +\n> >> + int start() override;\n> >> + void stop() override;\n> >> +\n> >> + void processStats(const ControlList &sensorControls) override;\n> >> +\n> >> +private:\n> >> + void updateExposure(double exposureMSV);\n> >> +\n> >> + SharedFD fdStats_;\n> >> + SharedFD fdParams_;\n> >> + DebayerParams *params_;\n> >> + SwIspStats *stats_;\n> > \n> > I'd throw a blank line between class instances and the following PODs\n> > \n> >> + int exposure_min_, exposure_max_;\n> >> + int again_min_, again_max_;\n> >> + int again_, exposure_;\n> >> + int ignore_updates_;\n> > \n> > Should those by unsigned? Can they ever be 0 or less than zero?\n> \n> unsigned would be fine\n> \n> >> +};\n> >> +\n> >> +int IPASoftSimple::init([[maybe_unused]] const IPASettings &settings,\n> >> + const SharedFD &fdStats,\n> >> + const SharedFD &fdParams,\n> >> + const ControlInfoMap &sensorInfoMap)\n> >> +{\n> >> + fdStats_ = std::move(fdStats);\n> >> + if (!fdStats_.isValid()) {\n> >> + LOG(IPASoft, Error) << \"Invalid Statistics handle\";\n> >> + return -ENODEV;\n> >> + }\n> >> +\n> >> + fdParams_ = std::move(fdParams);\n> >> + if (!fdParams_.isValid()) {\n> >> + LOG(IPASoft, Error) << \"Invalid Parameters handle\";\n> >> + return -ENODEV;\n> >> + }\n> >> +\n> >> + params_ = static_cast(mmap(nullptr, sizeof(DebayerParams),\n> >> + PROT_WRITE, MAP_SHARED,\n> >> + fdParams_.get(), 0));\n> > \n> > We'd usually use MappedFrameBuffer() here so that the mappings are\n> > handled by RAII here\n> \n> OK, will fix that.\n> \n> >> + if (!params_) {\n> >> + LOG(IPASoft, Error) << \"Unable to map Parameters\";\n> >> + return -ENODEV;\n> >> + }\n> >> +\n> >> + stats_ = static_cast(mmap(nullptr, sizeof(SwIspStats),\n> >> + PROT_READ, MAP_SHARED,\n> >> + fdStats_.get(), 0));\n> >> + if (!stats_) {\n> >> + LOG(IPASoft, Error) << \"Unable to map Statistics\";\n> >> + return -ENODEV;\n> >> + }\n> > \n> > Same here.\n> \n> Ack\n> \n> >> +\n> >> + if (sensorInfoMap.find(V4L2_CID_EXPOSURE) == sensorInfoMap.end()) {\n> >> + LOG(IPASoft, Error) << \"Don't have exposure control\";\n> >> + return -EINVAL;\n> >> + }\n> >> +\n> >> + if (sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN) == sensorInfoMap.end()) {\n> >> + LOG(IPASoft, Error) << \"Don't have gain control\";\n> >> + return -EINVAL;\n> >> + }\n> >> +\n> > \n> > In the future I would expect AEGC to be broken out to it's own modular\n> > 'Algorithm' (see rkisp1 for the createAlgorithms()\n> \n> Thanks, I'll take a look.\n\nIf it makes sense and helps starting to make things modular then it's\ndefinitely a nice to have .... but as we don't really have a lot of\nalgorithms here to break out - I don't mind if it's done later. It's\njust helpful to be aware of the design goals of the ipas.\n\n\n> > We're also lacking any set up or registration for control handling. I\n> > guess for now there may be none, but do you plan to have controls? I'm\n> > fine with those being added later.\n> \n> I'd stick to what we currently have (none), and revisit this later.\n\nAck. That's one part that I think breaking out algorithms to components\nwill help though as each algorithm can/should then be responsible for\nregistering what controls it will support and handling them.\n\n\n> \n> >> + return 0;\n> >> +}\n> >> +\n> >> +int IPASoftSimple::configure(const ControlInfoMap &sensorInfoMap)\n> >> +{\n> >> + const ControlInfo &exposure_info = sensorInfoMap.find(V4L2_CID_EXPOSURE)->second;\n> >> + const ControlInfo &gain_info = sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN)->second;\n> >> +\n> >> + exposure_min_ = exposure_info.min().get();\n> >> + if (!exposure_min_) {\n> >> + LOG(IPASoft, Warning) << \"Minimum exposure is zero, that can't be linear\";\n> >> + exposure_min_ = 1;\n> >> + }\n> >> + exposure_max_ = exposure_info.max().get();\n> >> + again_min_ = gain_info.min().get();\n> >> + if (!again_min_) {\n> >> + LOG(IPASoft, Warning) << \"Minimum gain is zero, that can't be linear\";\n> >> + again_min_ = 100;\n> >> + }\n> > \n> > This is where you should almost certainly be looking at the\n> > CameraSensorHelpers. We expect the gain model to be abstracted by each\n> > sensor through the helpers.\n> > \n> > IPA algorithms should operate on a linear model, and the\n> > CameraSensorHelpers will convert any logarithmic gain codes to linear on\n> > a per-sensor basis, as well as impose any sensor specific limits.\n> \n> I am going to draft a patch to use the CameraSensorHelpers before the end of this week\n> (hopefully sooner).\n\nI hope this part shouldn't be too complicated. They're already used in\nother IPAs. The idea is to factor out the common sensor specific\nparts away from the IPA.\n\n\n--\nKieran\n\n\n> \n> >> + again_max_ = gain_info.max().get();\n> >> +\n> >> + LOG(IPASoft, Info) << \"Exposure \" << exposure_min_ << \"-\" << exposure_max_\n> >> + << \", gain \" << again_min_ << \"-\" << again_max_;\n> >> +\n> >> + return 0;\n> >> +}\n> >> +\n> >> +int IPASoftSimple::start()\n> >> +{\n> >> + return 0;\n> >> +}\n> >> +\n> >> +void IPASoftSimple::stop()\n> >> +{\n> >> +}\n> >> +\n> >> +void IPASoftSimple::processStats(const ControlList &sensorControls)\n> >> +{\n> >> + /*\n> >> + * Calculate red and blue gains for AWB.\n> >> + * Clamp max gain at 4.0, this also avoids 0 division.\n> >> + */\n> >> + if (stats_->sumR_ <= stats_->sumG_ / 4)\n> >> + params_->gainR = 1024;\n> >> + else\n> >> + params_->gainR = 256 * stats_->sumG_ / stats_->sumR_;\n> >> +\n> >> + if (stats_->sumB_ <= stats_->sumG_ / 4)\n> >> + params_->gainB = 1024;\n> >> + else\n> >> + params_->gainB = 256 * stats_->sumG_ / stats_->sumB_;\n> >> +\n> >> + /* Green gain and gamma values are fixed */\n> >> + params_->gainG = 256;\n> >> + params_->gamma = 0.5;\n> >> +\n> >> + setIspParams.emit(0);\n> >> +\n> >> + /*\n> >> + * AE / AGC, use 2 frames delay to make sure that the exposure and\n> >> + * the gain set have applied to the camera sensor.\n> >> + */\n> > \n> > Using known sensor specific delays defined in the CameraSensorHelper and\n> > DelayedControls should be considered here in the future. But as a first\n> > implementation for CPU only, where doing /less/ work is helpful - this\n> > is fine for me.\n> \n> Agreed.\n> \n> >> + if (ignore_updates_ > 0) {\n> >> + --ignore_updates_;\n> >> + return;\n> >> + }\n> >> +\n> >> + /*\n> >> + * Calculate Mean Sample Value (MSV) according to formula from:\n> >> + * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf\n> >> + */\n> >> + constexpr unsigned int ExposureBinsCount = 5;\n> >> + constexpr unsigned int yHistValsPerBin =\n> >> + SwIspStats::kYHistogramSize / ExposureBinsCount;\n> >> + constexpr unsigned int yHistValsPerBinMod =\n> >> + SwIspStats::kYHistogramSize /\n> >> + (SwIspStats::kYHistogramSize % ExposureBinsCount + 1);\n> >> + int ExposureBins[ExposureBinsCount] = {};\n> >> + unsigned int denom = 0;\n> >> + unsigned int num = 0;\n> >> +\n> >> + for (unsigned int i = 0; i < SwIspStats::kYHistogramSize; i++) {\n> >> + unsigned int idx = (i - (i / yHistValsPerBinMod)) / yHistValsPerBin;\n> >> + ExposureBins[idx] += stats_->yHistogram[i];\n> >> + }\n> >> +\n> >> + for (unsigned int i = 0; i < ExposureBinsCount; i++) {\n> >> + LOG(IPASoft, Debug) << i << \": \" << ExposureBins[i];\n> >> + denom += ExposureBins[i];\n> >> + num += ExposureBins[i] * (i + 1);\n> >> + }\n> >> +\n> >> + float exposureMSV = (float)num / denom;\n> >> +\n> >> + /* sanity check */\n> >> + if (!sensorControls.contains(V4L2_CID_EXPOSURE) ||\n> >> + !sensorControls.contains(V4L2_CID_ANALOGUE_GAIN)) {\n> >> + LOG(IPASoft, Error) << \"Control(s) missing\";\n> >> + return;\n> >> + }\n> >> +\n> >> + ControlList ctrls(sensorControls);\n> >> +\n> >> + exposure_ = ctrls.get(V4L2_CID_EXPOSURE).get();\n> >> + again_ = ctrls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> >> +\n> >> + updateExposure(exposureMSV);\n> >> +\n> >> + ctrls.set(V4L2_CID_EXPOSURE, exposure_);\n> >> + ctrls.set(V4L2_CID_ANALOGUE_GAIN, again_);\n> >> +\n> >> + ignore_updates_ = 2;\n> >> +\n> >> + setSensorControls.emit(ctrls);\n> >> +\n> >> + LOG(IPASoft, Debug) << \"exposureMSV \" << exposureMSV\n> >> + << \" exp \" << exposure_ << \" again \" << again_\n> >> + << \" gain R/B \" << params_->gainR << \"/\" << params_->gainB;\n> >> +}\n> >> +\n> >> +/* DENOMINATOR of 10 gives ~10% increment/decrement; DENOMINATOR of 5 - about ~20% */\n> >> +#define DENOMINATOR 10\n> >> +#define UP_NUMERATOR (DENOMINATOR + 1)\n> >> +#define DOWN_NUMERATOR (DENOMINATOR - 1)\n> > \n> > Our C++ code style for constants would be more like:\n> > \n> > static constexpr uint8_t kExpDenominator = 10;\n> > static constexpr uint8_t kExpNumeratorUp = kExpDenominator + 1;\n> > static constexpr uint8_t kExpNumeratorDown = kExpDenominator - 1;\n> \n> This was written by me, and I'll update this part to use static constexpr.\n> \n> >> +\n> >> +void IPASoftSimple::updateExposure(double exposureMSV)\n> >> +{\n> >> + int next;\n> >> +\n> >> + if (exposureMSV < EXPOSURE_OPTIMAL_VALUE - EXPOSURE_SATISFACTORY_OFFSET) {\n> >> + next = exposure_ * UP_NUMERATOR / DENOMINATOR;\n> >> + if (next - exposure_ < 1)\n> >> + exposure_ += 1;\n> >> + else\n> >> + exposure_ = next;\n> >> + if (exposure_ >= exposure_max_) {\n> >> + next = again_ * UP_NUMERATOR / DENOMINATOR;\n> >> + if (next - again_ < 1)\n> >> + again_ += 1;\n> >> + else\n> >> + again_ = next;\n> >> + }\n> >> + }\n> >> +\n> >> + if (exposureMSV > EXPOSURE_OPTIMAL_VALUE + EXPOSURE_SATISFACTORY_OFFSET) {\n> >> + if (exposure_ == exposure_max_ && again_ != again_min_) {\n> >> + next = again_ * DOWN_NUMERATOR / DENOMINATOR;\n> >> + if (again_ - next < 1)\n> >> + again_ -= 1;\n> >> + else\n> >> + again_ = next;\n> >> + } else {\n> >> + next = exposure_ * DOWN_NUMERATOR / DENOMINATOR;\n> >> + if (exposure_ - next < 1)\n> >> + exposure_ -= 1;\n> >> + else\n> >> + exposure_ = next;\n> >> + }\n> >> + }\n> >> +\n> >> + if (exposure_ > exposure_max_)\n> >> + exposure_ = exposure_max_;\n> >> + else if (exposure_ < exposure_min_)\n> >> + exposure_ = exposure_min_;\n> >> +\n> >> + if (again_ > again_max_)\n> >> + again_ = again_max_;\n> >> + else if (again_ < again_min_)\n> >> + again_ = again_min_;\n> > \n> > How about\n> > exposure_ = std::clamp(exposure_, exposure_min_, exposure_max_);\n> > again_ = std::clamp(again_, again_min_, again_max_);\n> > \n> > ?\n> \n> OK, will fix.\n> \n> Thanks,\n> Andrei\n> \n> >> +}\n> >> +\n> >> +} /* namespace ipa::soft */\n> >> +\n> >> +/*\n> >> + * External IPA module interface\n> >> + */\n> >> +extern \"C\" {\n> >> +const struct IPAModuleInfo ipaModuleInfo = {\n> >> + IPA_MODULE_API_VERSION,\n> >> + 0,\n> >> + \"SimplePipelineHandler\",\n> >> + \"simple\",\n> >> +};\n> >> +\n> >> +IPAInterface *ipaCreate()\n> >> +{\n> >> + return new ipa::soft::IPASoftSimple();\n> >> +}\n> >> +\n> >> +} /* extern \"C\" */\n> >> +\n> >> +} /* namespace libcamera */\n> >> -- \n> >> 2.43.0\n> >>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 15047BF415\n\tfor ;\n\tTue, 20 Feb 2024 15:53:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3D92C61CAA;\n\tTue, 20 Feb 2024 16:53:24 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7641461CA3\n\tfor ;\n\tTue, 20 Feb 2024 16:53:22 +0100 (CET)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EA3B8899;\n\tTue, 20 Feb 2024 16:53:14 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"do7k+haE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1708444395;\n\tbh=p1Y0UT3VRI/suUyGeUggMCKUkU1GdbQ8dzqMnRNXmAg=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=do7k+haEj8+qMwWw+bjNs2Qm//lWFAY5clT8XkJ5POFT5eq2ujz1tG2Oy0VQNbNUh\n\tOPptBbG3YXxLlJRrZ6TnGm7yDFbsqFHCqN5gzCR34mJwZi4PyVauf7kG0IBjHt073D\n\tEvAJmaTVcm9MMDry/1TSQKThUVHZN40fOaIehIgk=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<6c9fe0c7-b97d-4d8c-bd29-25bd36a3a6ff@gmail.com>","References":"<20240214170122.60754-1-hdegoede@redhat.com>\n\t<20240214170122.60754-10-hdegoede@redhat.com>\n\t<170843431013.1011926.9612134639156491265@ping.linuxembedded.co.uk>\n\t<6c9fe0c7-b97d-4d8c-bd29-25bd36a3a6ff@gmail.com>","Subject":"Re: [PATCH v3 09/16] libcamera: ipa: add Soft IPA","From":"Kieran Bingham ","To":"Andrei Konovalov ,\n\tHans de Goede , libcamera-devel@lists.libcamera.org","Date":"Tue, 20 Feb 2024 15:53:19 +0000","Message-ID":"<170844439906.1011926.2188078205048491174@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Maxime Ripard , Marttico ,\n\tToon Langendam , Pavel Machek , \n\tBryan O'Donoghue ,\n\tDennis Bonke ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28705,"web_url":"https://patchwork.libcamera.org/comment/28705/","msgid":"<450c67ab-5a42-469d-9c12-3bbaf7db34cd@gmail.com>","date":"2024-02-20T20:28:32","subject":"Re: [PATCH v3 09/16] libcamera: ipa: add Soft IPA","submitter":{"id":179,"url":"https://patchwork.libcamera.org/api/people/179/","name":"Andrei Konovalov","email":"andrey.konovalov.ynk@gmail.com"},"content":"Hi Barnabás,\n\nOn 14.02.2024 21:15, Barnabás Pőcze wrote:\n> Hi\n> \n> \n> 2024. február 14., szerda 18:01 keltezéssel, Hans de Goede írta:\n> \n>> From: Andrey Konovalov \n>>\n>> Define the Soft IPA main and event interfaces, add the Soft IPA\n>> implementation.\n>>\n>> The current src/ipa/meson.build assumes the IPA name to match the\n>> pipeline name. For this reason \"-Dipas=simple\" is used for the\n>> Soft IPA module.\n>>\n>> Auto exposure/gain and AWB implementation by Dennis, Toon and Martti.\n>>\n>> Auto exposure/gain targets a Mean Sample Value of 2.5 following\n>> the MSV calculation algorithm from:\n>> https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf\n>>\n>> Tested-by: Bryan O'Donoghue # sc8280xp Lenovo x13s\n>> Tested-by: Pavel Machek \n>> Reviewed-by: Pavel Machek \n>> Signed-off-by: Andrey Konovalov \n>> Co-developed-by: Dennis Bonke \n>> Signed-off-by: Dennis Bonke \n>> Co-developed-by: Marttico \n>> Signed-off-by: Marttico \n>> Co-developed-by: Toon Langendam \n>> Signed-off-by: Toon Langendam \n>> Signed-off-by: Hans de Goede \n>> ---\n>> Changes in v3:\n>> - Use new SwIspStats::kYHistogramSize constexpr and adjust\n>> the auto-exposure/-gain code so that it can deal with\n>> that having a different value then 16 (modify the loop\n>> to divide the histogram in 5 bins to not have hardcoded\n>> values)\n>> - Rename a few foo_bar symbols to fooBar\n>> ---\n>> Documentation/Doxyfile.in | 1 +\n>> include/libcamera/ipa/meson.build | 1 +\n>> include/libcamera/ipa/soft.mojom | 28 +++\n>> meson_options.txt | 2 +-\n>> src/ipa/simple/data/meson.build | 9 +\n>> src/ipa/simple/data/soft.conf | 3 +\n>> src/ipa/simple/meson.build | 25 +++\n>> src/ipa/simple/soft_simple.cpp | 308 ++++++++++++++++++++++++++++++\n>> 8 files changed, 376 insertions(+), 1 deletion(-)\n>> create mode 100644 include/libcamera/ipa/soft.mojom\n>> create mode 100644 src/ipa/simple/data/meson.build\n>> create mode 100644 src/ipa/simple/data/soft.conf\n>> create mode 100644 src/ipa/simple/meson.build\n>> create mode 100644 src/ipa/simple/soft_simple.cpp\n>>\n>> [...]\n>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n>> new file mode 100644\n>> index 00000000..f7ac02f9\n>> --- /dev/null\n>> +++ b/src/ipa/simple/soft_simple.cpp\n>> @@ -0,0 +1,308 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2023, Linaro Ltd\n>> + *\n>> + * soft_simple.cpp - Simple Software Image Processing Algorithm module\n>> + */\n>> +\n>> +#include \n>> +\n>> +#include \n>> +#include \n>> +#include \n>> +\n>> +#include \n>> +#include \n>> +\n>> +#include \n>> +#include \n>> +#include \n>> +\n>> +#include \"libcamera/internal/camera_sensor.h\"\n>> +#include \"libcamera/internal/software_isp/debayer_params.h\"\n>> +#include \"libcamera/internal/software_isp/swisp_stats.h\"\n>> +\n>> +#define EXPOSURE_OPTIMAL_VALUE 2.5\n>> +#define EXPOSURE_SATISFACTORY_OFFSET 0.2\n>> +\n>> +namespace libcamera {\n>> +\n>> +LOG_DEFINE_CATEGORY(IPASoft)\n>> +\n>> +namespace ipa::soft {\n>> +\n>> +class IPASoftSimple : public ipa::soft::IPASoftInterface\n>> +{\n>> +public:\n>> +\tIPASoftSimple()\n>> +\t\t: ignore_updates_(0)\n>> +\t{\n>> +\t}\n>> +\n>> +\t~IPASoftSimple()\n>> +\t{\n>> +\t\tif (stats_)\n> \n> if (stats_ != MAP_FAILED)\n> \n> \n>> +\t\t\tmunmap(stats_, sizeof(SwIspStats));\n>> +\t\tif (params_)\n>> +\t\t\tmunmap(params_, sizeof(DebayerParams));\n>> +\t}\n>> +\n>> +\tint init(const IPASettings &settings,\n>> +\t\t const SharedFD &fdStats,\n>> +\t\t const SharedFD &fdParams,\n>> +\t\t const ControlInfoMap &sensorInfoMap) override;\n>> +\tint configure(const ControlInfoMap &sensorInfoMap) override;\n>> +\n>> +\tint start() override;\n>> +\tvoid stop() override;\n>> +\n>> +\tvoid processStats(const ControlList &sensorControls) override;\n>> +\n>> +private:\n>> +\tvoid updateExposure(double exposureMSV);\n>> +\n>> +\tSharedFD fdStats_;\n>> +\tSharedFD fdParams_;\n>> +\tDebayerParams *params_;\n>> +\tSwIspStats *stats_;\n> \n> The above two should be initialized to MAP_FAILED.\n> \n> \n>> +\tint exposure_min_, exposure_max_;\n>> +\tint again_min_, again_max_;\n>> +\tint again_, exposure_;\n>> +\tint ignore_updates_;\n>> +};\n>> +\n>> +int IPASoftSimple::init([[maybe_unused]] const IPASettings &settings,\n>> +\t\t\tconst SharedFD &fdStats,\n>> +\t\t\tconst SharedFD &fdParams,\n>> +\t\t\tconst ControlInfoMap &sensorInfoMap)\n>> +{\n>> +\tfdStats_ = std::move(fdStats);\n> \n> std::move(fdStats) has the type `const SharedFD&&`, so `fdStats_ = ...`\n> will result in a call to the copy assignment operator `operator=(const SharedFD&)`.\n> So `std::move()` does not really do anything here, and can be removed.\n> Alternatively, if possible, you can change the argument type to be just `SharedFD`,\n> and then moving makes sense (preferably in the member initializer list).\n\nGood catch\n\n>> +\tif (!fdStats_.isValid()) {\n>> +\t\tLOG(IPASoft, Error) << \"Invalid Statistics handle\";\n>> +\t\treturn -ENODEV;\n>> +\t}\n>> +\n>> +\tfdParams_ = std::move(fdParams);\n>> +\tif (!fdParams_.isValid()) {\n>> +\t\tLOG(IPASoft, Error) << \"Invalid Parameters handle\";\n>> +\t\treturn -ENODEV;\n>> +\t}\n>> +\n>> +\tparams_ = static_cast(mmap(nullptr, sizeof(DebayerParams),\n>> +\t\t\t\t\t\t PROT_WRITE, MAP_SHARED,\n>> +\t\t\t\t\t\t fdParams_.get(), 0));\n>> +\tif (!params_) {\n> \n> mmap returns MAP_FAILED on failure.\n\nRight. I'll fix this and the related parts in the next version of the patch set.\n\n>> +\t\tLOG(IPASoft, Error) << \"Unable to map Parameters\";\n>> +\t\treturn -ENODEV;\n> \n> Maybe using `errno` would be better?\n\nYes, sounds reasonable.\n\nThanks,\nAndrei\n\n>> +\t}\n>> +\n>> +\tstats_ = static_cast(mmap(nullptr, sizeof(SwIspStats),\n>> +\t\t\t\t\t\tPROT_READ, MAP_SHARED,\n>> +\t\t\t\t\t\tfdStats_.get(), 0));\n>> +\tif (!stats_) {\n>> +\t\tLOG(IPASoft, Error) << \"Unable to map Statistics\";\n>> +\t\treturn -ENODEV;\n>> +\t}\n>> +\n>> +\tif (sensorInfoMap.find(V4L2_CID_EXPOSURE) == sensorInfoMap.end()) {\n>> +\t\tLOG(IPASoft, Error) << \"Don't have exposure control\";\n>> +\t\treturn -EINVAL;\n>> +\t}\n>> +\n>> +\tif (sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN) == sensorInfoMap.end()) {\n>> +\t\tLOG(IPASoft, Error) << \"Don't have gain control\";\n>> +\t\treturn -EINVAL;\n>> +\t}\n>> +\n>> +\treturn 0;\n>> +}\n>> [...]","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 2039AC3257\n\tfor ;\n\tTue, 20 Feb 2024 20:28:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1B3C862807;\n\tTue, 20 Feb 2024 21:28:36 +0100 (CET)","from mail-wm1-x336.google.com (mail-wm1-x336.google.com\n\t[IPv6:2a00:1450:4864:20::336])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DE93661CA3\n\tfor ;\n\tTue, 20 Feb 2024 21:28:34 +0100 (CET)","by mail-wm1-x336.google.com with SMTP id\n\t5b1f17b1804b1-412698ac6f9so12512125e9.0\n\tfor ;\n\tTue, 20 Feb 2024 12:28:34 -0800 (PST)","from [192.168.118.26] ([87.116.160.233])\n\tby smtp.gmail.com with ESMTPSA id\n\t26-20020a05600c025a00b0041273fc463csm143629wmj.17.2024.02.20.12.28.33\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tTue, 20 Feb 2024 12:28:33 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"KfJUZtga\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=gmail.com; s=20230601; t=1708460914; x=1709065714;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:in-reply-to:from:references:cc:to\n\t:content-language:subject:user-agent:mime-version:date:message-id\n\t:from:to:cc:subject:date:message-id:reply-to;\n\tbh=MTDWtlp1h1W3qa54xl12FZMtEHK0QqO+7465aiXDFHA=;\n\tb=KfJUZtgaBvOZIY+gay+Hzdp64Nhj90JMZW4tQqfC5T6exeHcrTP1LzqJybA0TxfH8J\n\taS3MFAhDY27BY6FSH3rCUIhYp+1hpuwnlfxirRJwTKkMj/PnjPt17q2xhPWI3BBY7+j0\n\tV/wKwK2LeWyTfUPmBGSJWtvdTFn2k2o7vG+Kwt0B8wEVKJCHI6wQgijbS6JC61NUI+dw\n\t5RaPx29IwVJ/Vwbz6aAvMQLH8sdC/ulXecHQBCfUy7hBm9/8ITyKPKiEPEpKi4RyLjdm\n\t5vGNjKydutv9kDYZF6UXmOfz/ZpiYkOgxV++aWQQrUMSsGWQqUq9XrmggAD4RxpzUcUS\n\tVc6w==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1708460914; x=1709065714;\n\th=content-transfer-encoding:in-reply-to:from:references:cc:to\n\t:content-language:subject:user-agent:mime-version:date:message-id\n\t:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;\n\tbh=MTDWtlp1h1W3qa54xl12FZMtEHK0QqO+7465aiXDFHA=;\n\tb=DB5f5rAQe4RTyT/VREnqu4YYmYR8NpOaWGA1u+lpI99+TrU7MNABSDzibS3Rx99CEf\n\tNwfOktk3QUCZ70cc0+8uZ50jZCmEfQUBllK50u4aQZ0mU530VsO0kL7/lnPjOt3gWaQI\n\tcMuPE5rJHlivtYeE9DyCs0LhYHiIaByXML+0f+h54RhqzsSXP81GntKoG1rlrvsQzmn6\n\tTQqu02Gv0vGp5DX77VsnNZJsAPePuFNSamqz6RLLkqZ2UBoriEJUUFn3AebWkyGtuC3o\n\ty+mlasciz1nKj+/R3cLqeHox9HLd1XiaKC1g4sIwOiREbMmQJJw0+cG+gnREtbxKrA6S\n\teW3w==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCVudmUjN/w3//a8EdvHkSG+ucxJO0NpPAorgG00cHZYqCLsYd/s+0DXlvwsjJdC52ignftGxNpwyrZnts6u42WZE3UY5FpbWMVn/6HcNGL20EDlwQ==","X-Gm-Message-State":"AOJu0Yz170Jmuov0s+zXyn5bBcjZCPohrKqlAzZdgcpw6uIg5oIwm/A8\n\tUaijkxGq3oYmtkNy6bOOpi1Y6iYpUhYdNNKsplz8hNsmBSpn81Y6","X-Google-Smtp-Source":"AGHT+IF22VAcxGPFGmvILZ+70ZiWQdrujq32ST+3TqRHkeRwb31rp730dMTuAg48RBPOIyHRUpudVQ==","X-Received":"by 2002:a05:600c:4f91:b0:412:5f08:b508 with SMTP id\n\tn17-20020a05600c4f9100b004125f08b508mr8398694wmq.9.1708460914127; \n\tTue, 20 Feb 2024 12:28:34 -0800 (PST)","Message-ID":"<450c67ab-5a42-469d-9c12-3bbaf7db34cd@gmail.com>","Date":"Tue, 20 Feb 2024 23:28:32 +0300","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v3 09/16] libcamera: ipa: add Soft IPA","Content-Language":"en-US","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tHans de Goede ","References":"<20240214170122.60754-1-hdegoede@redhat.com>\n\t<20240214170122.60754-10-hdegoede@redhat.com>\n\t","From":"Andrei Konovalov ","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Maxime Ripard , Marttico ,\n\tToon Langendam ,\n\tlibcamera-devel@lists.libcamera.org, Pavel Machek ,\n\tBryan O'Donoghue , \n\tDennis Bonke ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]