[{"id":29098,"web_url":"https://patchwork.libcamera.org/comment/29098/","msgid":"<20240327163808.GG4721@pendragon.ideasonboard.com>","date":"2024-03-27T16:38:08","subject":"Re: [PATCH v6 09/18] libcamera: ipa: add Soft IPA","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Milan and Andrey,\n\nThank you for the patch.\n\nOn Tue, Mar 19, 2024 at 01:35:56PM +0100, Milan Zamazal wrote:\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\nThis should be addressed, please record this as a \\todo item somewhere.\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> 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 | 326 ++++++++++++++++++++++++++++++\n> 8 files changed, 394 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\nWhy is this needed ?\n\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\nAh that's why. It doesn't have to be done before merging, but could you\naddress this sooner than later ?\n\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\nThe IPA and soft ISP run in separate threads. How do you guarantee that\nthe stats and params are not accessed concurrently by both ? Shouldn't\nyou have a pool of buffers for each, mimicking what is done with\nhardware ISPs ?\n\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\nDrop the dummy value.\n\n> +};\n> diff --git a/meson_options.txt b/meson_options.txt\n> index 99dab96d..2644bef0 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\nWe use YAML files for all IPAs, could you do the same here ? It\nshouldn't be much extra work as the file is empty :-)\n\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..312df4ba\n> --- /dev/null\n> +++ b/src/ipa/simple/soft_simple.cpp\n> @@ -0,0 +1,326 @@\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\nNot needed.\n\n> +#include \"libcamera/internal/software_isp/debayer_params.h\"\n> +#include \"libcamera/internal/software_isp/swisp_stats.h\"\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: params_(static_cast(MAP_FAILED)),\n\nInitialize this to nullptr, that's more standard, and will make the\ntests in the destructor nicer. init() will have to set the pointers to\nnull if mmap() calls fail.\n\n> +\t\t stats_(static_cast(MAP_FAILED)), ignore_updates_(0)\n\ns/ignore_updates_/ignoreUpdates_/\n\n> +\t{\n> +\t}\n> +\n> +\t~IPASoftSimple()\n> +\t{\n> +\t\tif (stats_ != MAP_FAILED)\n> +\t\t\tmunmap(stats_, sizeof(SwIspStats));\n> +\t\tif (params_ != MAP_FAILED)\n> +\t\t\tmunmap(params_, sizeof(DebayerParams));\n> +\t}\n\nNo need to inline this.\n\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> +\tint32_t exposure_min_, exposure_max_;\n> +\tint32_t again_min_, again_max_;\n> +\tint32_t again_, exposure_;\n> +\tunsigned int 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_ = fdStats;\n\nAs you never use fdStats_ and fdParams_ after this function returns,\nthere's no need to store a copy.\n\n> +\tif (!fdStats_.isValid()) {\n> +\t\tLOG(IPASoft, Error) << \"Invalid Statistics handle\";\n> +\t\treturn -ENODEV;\n> +\t}\n> +\n> +\tfdParams_ = 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_ == MAP_FAILED) {\n> +\t\tLOG(IPASoft, Error) << \"Unable to map Parameters\";\n> +\t\treturn -errno;\n> +\t}\n\n\tvoid *mem = mmap(nullptr, sizeof(DebayerParams), PROT_WRITE, MAP_SHARED,\n\t\t\t fdParams_.get(), 0));\n\tif (mem == MAP_FAILED) {\n\t\tLOG(IPASoft, Error) << \"Unable to map Parameters\";\n\t\treturn -errno;\n\t}\n\n\tparams_ = static_cast(mem);\n\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_ == MAP_FAILED) {\n> +\t\tLOG(IPASoft, Error) << \"Unable to map Statistics\";\n> +\t\treturn -errno;\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\ns/exposure_info/exposureInfo/\n\nI'll let you address the other snake_case to camelCase conversions in\nthe series :-)\n\n> +\tconst ControlInfo &gain_info = sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN)->second;\n> +\n> +\texposure_min_ = exposure_info.min().get();\n> +\texposure_max_ = exposure_info.max().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> +\n> +\tagain_min_ = gain_info.min().get();\n> +\tagain_max_ = gain_info.max().get();\n\nAdd a blank line.\n\n> +\t/*\n> +\t * The camera sensor gain (g) is usually not equal to the value written\n> +\t * into the gain register (x). But the way how the AGC algorithm changes\n> +\t * the gain value to make the total exposure closer to the optimum assumes\n> +\t * that g(x) is not too far from linear function. If the minimal gain is 0,\n> +\t * the g(x) is likely to be far from the linear, like g(x) = a / (b * x + c).\n> +\t * To avoid unexpected changes to the gain by the AGC algorithm (abrupt near\n> +\t * one edge, and very small near the other) we limit the range of the gain\n> +\t * values used.\n> +\t */\n> +\tif (!again_min_) {\n> +\t\tLOG(IPASoft, Warning) << \"Minimum gain is zero, that can't be linear\";\n> +\t\tagain_min_ = std::min(100, again_min_ / 2 + again_max_ / 2);\n> +\t}\n\nA patch further in the series addresses this by using\nCameraSensorHelper. It should be squashed with this patch.\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> +/*\n> + * The number of bins to use for the optimal exposure calculations.\n> + */\n> +static constexpr unsigned int kExposureBinsCount = 5;\n\nMissing blank line. Same below.\n\n> +/*\n> + * The exposure is optimal when the mean sample value of the histogram is\n> + * in the middle of the range.\n> + */\n> +static constexpr float kExposureOptimal = kExposureBinsCount / 2.0;\n> +/*\n> + * The below value implements the hysteresis for the exposure adjustment.\n> + * It is small enough to have the exposure close to the optimal, and is big\n> + * enough to prevent the exposure from wobbling around the optimal value.\n> + */\n> +static constexpr float kExposureSatisfactory = 0.2;\n\nMove these to the beginning of the file, just before the IPASoftSimple\ndefinition.\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\nDo you envision switching to the libipa/algorithm.h API at some point ?\nIf so, it would be nice to record it somewhere.\n\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\nAlso something that could be improved and that should be recorded\nsomewhere.\n\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 yHistValsPerBin =\n> +\t\tSwIspStats::kYHistogramSize / kExposureBinsCount;\n> +\tconstexpr unsigned int yHistValsPerBinMod =\n> +\t\tSwIspStats::kYHistogramSize /\n> +\t\t(SwIspStats::kYHistogramSize % kExposureBinsCount + 1);\n> +\tint ExposureBins[kExposureBinsCount] = {};\n\ns/ExposureBins/exposureBins/\n\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 < kExposureBinsCount; 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\nC++ casts.\n\n> +\n> +\t/* sanity check */\n\ns/sanity/Sanity/\n\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\nYou shouldn't copy the control list, but instead create one from the\nControlInfoMap that you get in init() (and that you then need to stored\nin the IPA class).\n\n> +\n> +\texposure_ = ctrls.get(V4L2_CID_EXPOSURE).get();\n> +\tagain_ = ctrls.get(V4L2_CID_ANALOGUE_GAIN).get();\n\n\texposure_ = sensorControls.get(V4L2_CID_EXPOSURE).get();\n\tagain_ = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n\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> +void IPASoftSimple::updateExposure(double exposureMSV)\n> +{\n> +\t/* DENOMINATOR of 10 gives ~10% increment/decrement; DENOMINATOR of 5 - about ~20% */\n\nLine wrap, and the DENOMINATOR needs to be renamed to kExpDenominator.\n\n> +\tstatic constexpr uint8_t kExpDenominator = 10;\n> +\tstatic constexpr uint8_t kExpNumeratorUp = kExpDenominator + 1;\n> +\tstatic constexpr uint8_t kExpNumeratorDown = kExpDenominator - 1;\n> +\n> +\tint next;\n> +\n> +\tif (exposureMSV < kExposureOptimal - kExposureSatisfactory) {\n> +\t\tnext = exposure_ * kExpNumeratorUp / kExpDenominator;\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_ * kExpNumeratorUp / kExpDenominator;\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 > kExposureOptimal + kExposureSatisfactory) {\n> +\t\tif (exposure_ == exposure_max_ && again_ != again_min_) {\n> +\t\t\tnext = again_ * kExpNumeratorDown / kExpDenominator;\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_ * kExpNumeratorDown / kExpDenominator;\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> +\texposure_ = std::clamp(exposure_, exposure_min_, exposure_max_);\n> +\tagain_ = std::clamp(again_, again_min_, again_max_);\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 B6B71C0DA4\n\tfor ;\n\tWed, 27 Mar 2024 16:38:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B6A5E63331;\n\tWed, 27 Mar 2024 17:38:18 +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 06EB161C35\n\tfor ;\n\tWed, 27 Mar 2024 17:38:17 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 07E3513AC;\n\tWed, 27 Mar 2024 17:37:43 +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=\"JzfSgGw7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711557464;\n\tbh=FkWPbigR+RmYQOJzTa3VjnarIvGeL1XbgGoMjb0HHNE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=JzfSgGw7INJt4pyZAH+vT0RBBQ13r+F2636RpFwlfcrgM7NR/oN7XrVRDa0LrCJR1\n\txcR7ksir80ymluJl9IaQgwmcB45QZ6SEL25UphXPSa/lfyMZc/SylOqu2VzQIzKMCv\n\tPSbbDHL65zzppNYswiF60BCi1Z8v+GR/nX+iZktQ=","Date":"Wed, 27 Mar 2024 18:38:08 +0200","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org,\n\tAndrey Konovalov ,\n\tBryan O'Donoghue ,\n\tMaxime Ripard , Pavel Machek ,\n\tHans de Goede ,\n\tKieran Bingham ,\n\tDennis Bonke , Marttico , \n\tToon Langendam ","Subject":"Re: [PATCH v6 09/18] libcamera: ipa: add Soft IPA","Message-ID":"<20240327163808.GG4721@pendragon.ideasonboard.com>","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-10-mzamazal@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240319123622.675599-10-mzamazal@redhat.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29116,"web_url":"https://patchwork.libcamera.org/comment/29116/","msgid":"<46848c9c-5bea-472e-bcb7-efc24479fb45@gmail.com>","date":"2024-03-27T22:36:43","subject":"Re: [PATCH v6 09/18] 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 Laurent,\n\nThank you for the review!\n\nOn 27.03.2024 19:38, Laurent Pinchart wrote:\n> Hi Milan and Andrey,\n> \n> Thank you for the patch.\n> \n> On Tue, Mar 19, 2024 at 01:35:56PM +0100, Milan Zamazal wrote:\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> This should be addressed, please record this as a \\todo item somewhere.\n\nAck\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>> 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 | 326 ++++++++++++++++++++++++++++++\n>> 8 files changed, 394 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> \n> Why is this needed ?\n> \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> Ah that's why.\n\nYes, because, well... all the other IPAs were doing that...\n\n> It doesn't have to be done before merging, but could you\n> address this sooner than later ?\n\nI can't promise a lot, but I'll look into that.\n\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> \n> The IPA and soft ISP run in separate threads. How do you guarantee that\n> the stats and params are not accessed concurrently by both ?\n\nI tried avoiding using a pool of buffers for stats and params, and used just a\nsingle chunk of shared memory to pass the stats from soft ISP (its worker thread)\nto IPA, and the other chunk for params.\n\nThe soft ISP accumulates the stats in its member variable, and after all the stats\nfor the frame are calculated, the member variable is copyed to the shared memory and\nstatReady signal is emitted (so using the member var implements some kind of double\nbuffering). This way the write to the stats shared memory happens once per frame and\nis short in time right before emitting the statReady signal.\nThe callback executed on receiving statReady signal invokes the IPA's processStats\nfunction. So the IPA's processStats() is called soon after the statReady is emitted.\nIf the IPA finish processing the stats before the next frame is processed by the soft IPS\nand the stats for the next frame are written into the shared memory, there is no concurrent\nacess.\n\nThis doesn't strictly guarantee that the stats are not accessed concurrently by the IPA and\nsoft ISP. But unless the video stream from the camera sensor overloads the system this\nscheme works OK. And if the load is high the concurrent access isn't impossible, and can harm\nthe image quality, but doesn't lead to critical problems like invalid pointers as the stats\nstructure only contains numbers and an array of fixed size.\n\n> Shouldn't\n> you have a pool of buffers for each, mimicking what is done with\n> hardware ISPs ?\n\nThis would definitely work, but is a more heavy wait implementation.\n\nOn the other side, the current implementation is less universal (isn't very scalable), more fragile,\nand can't reliably associate the stats and the params with the given frame from the camera sensor.\n\nSo let me try to implement what you suggested (\"a pool of buffers for each, mimicking what is done with\nhardware ISPs\").\n\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> Drop the dummy value.\n\nlibcamera docs do allow signals with zero parameters.\nBut when I tried having zero parameters for an EventInterface function,\nit didn't work for me iirc.\nLet me double check.\n\n>> +};\n>> diff --git a/meson_options.txt b/meson_options.txt\n>> index 99dab96d..2644bef0 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> \n> We use YAML files for all IPAs, could you do the same here ? It\n> shouldn't be much extra work as the file is empty :-)\n\nOK\n\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..312df4ba\n>> --- /dev/null\n>> +++ b/src/ipa/simple/soft_simple.cpp\n>> @@ -0,0 +1,326 @@\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> \n> Not needed.\n\nOK\n\n>> +#include \"libcamera/internal/software_isp/debayer_params.h\"\n>> +#include \"libcamera/internal/software_isp/swisp_stats.h\"\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: params_(static_cast(MAP_FAILED)),\n> \n> Initialize this to nullptr, that's more standard, and will make the\n> tests in the destructor nicer. init() will have to set the pointers to\n> null if mmap() calls fail.\n\nOK\n\n>> +\t\t stats_(static_cast(MAP_FAILED)), ignore_updates_(0)\n> \n> s/ignore_updates_/ignoreUpdates_/\n\nOK\n\n>> +\t{\n>> +\t}\n>> +\n>> +\t~IPASoftSimple()\n>> +\t{\n>> +\t\tif (stats_ != MAP_FAILED)\n>> +\t\t\tmunmap(stats_, sizeof(SwIspStats));\n>> +\t\tif (params_ != MAP_FAILED)\n>> +\t\t\tmunmap(params_, sizeof(DebayerParams));\n>> +\t}\n> \n> No need to inline this.\n\nOK\n\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>> +\tint32_t exposure_min_, exposure_max_;\n>> +\tint32_t again_min_, again_max_;\n>> +\tint32_t again_, exposure_;\n>> +\tunsigned int 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_ = fdStats;\n> \n> As you never use fdStats_ and fdParams_ after this function returns,\n> there's no need to store a copy.\n\nGood catch. OK\n\n>> +\tif (!fdStats_.isValid()) {\n>> +\t\tLOG(IPASoft, Error) << \"Invalid Statistics handle\";\n>> +\t\treturn -ENODEV;\n>> +\t}\n>> +\n>> +\tfdParams_ = 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_ == MAP_FAILED) {\n>> +\t\tLOG(IPASoft, Error) << \"Unable to map Parameters\";\n>> +\t\treturn -errno;\n>> +\t}\n> \n> \tvoid *mem = mmap(nullptr, sizeof(DebayerParams), PROT_WRITE, MAP_SHARED,\n> \t\t\t fdParams_.get(), 0));\n> \tif (mem == MAP_FAILED) {\n> \t\tLOG(IPASoft, Error) << \"Unable to map Parameters\";\n> \t\treturn -errno;\n> \t}\n> \n> \tparams_ = static_cast(mem);\n\nOK\n\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_ == MAP_FAILED) {\n>> +\t\tLOG(IPASoft, Error) << \"Unable to map Statistics\";\n>> +\t\treturn -errno;\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> \n> s/exposure_info/exposureInfo/\n> \n> I'll let you address the other snake_case to camelCase conversions in\n> the series :-)\n\nOK\n\n>> +\tconst ControlInfo &gain_info = sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN)->second;\n>> +\n>> +\texposure_min_ = exposure_info.min().get();\n>> +\texposure_max_ = exposure_info.max().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>> +\n>> +\tagain_min_ = gain_info.min().get();\n>> +\tagain_max_ = gain_info.max().get();\n> \n> Add a blank line.\n\nOK\n\n>> +\t/*\n>> +\t * The camera sensor gain (g) is usually not equal to the value written\n>> +\t * into the gain register (x). But the way how the AGC algorithm changes\n>> +\t * the gain value to make the total exposure closer to the optimum assumes\n>> +\t * that g(x) is not too far from linear function. If the minimal gain is 0,\n>> +\t * the g(x) is likely to be far from the linear, like g(x) = a / (b * x + c).\n>> +\t * To avoid unexpected changes to the gain by the AGC algorithm (abrupt near\n>> +\t * one edge, and very small near the other) we limit the range of the gain\n>> +\t * values used.\n>> +\t */\n>> +\tif (!again_min_) {\n>> +\t\tLOG(IPASoft, Warning) << \"Minimum gain is zero, that can't be linear\";\n>> +\t\tagain_min_ = std::min(100, again_min_ / 2 + again_max_ / 2);\n>> +\t}\n> \n> A patch further in the series addresses this by using\n> CameraSensorHelper. It should be squashed with this patch.\n\nOK\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>> +/*\n>> + * The number of bins to use for the optimal exposure calculations.\n>> + */\n>> +static constexpr unsigned int kExposureBinsCount = 5;\n> \n> Missing blank line. Same below.\n\nOK\n\n>> +/*\n>> + * The exposure is optimal when the mean sample value of the histogram is\n>> + * in the middle of the range.\n>> + */\n>> +static constexpr float kExposureOptimal = kExposureBinsCount / 2.0;\n>> +/*\n>> + * The below value implements the hysteresis for the exposure adjustment.\n>> + * It is small enough to have the exposure close to the optimal, and is big\n>> + * enough to prevent the exposure from wobbling around the optimal value.\n>> + */\n>> +static constexpr float kExposureSatisfactory = 0.2;\n> \n> Move these to the beginning of the file, just before the IPASoftSimple\n> definition.\n\nOK\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> Do you envision switching to the libipa/algorithm.h API at some point ?\n> If so, it would be nice to record it somewhere.\n\nAt some point, yes.\nWill mention that.\n\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> Also something that could be improved and that should be recorded\n> somewhere.\n\nOK\n\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 yHistValsPerBin =\n>> +\t\tSwIspStats::kYHistogramSize / kExposureBinsCount;\n>> +\tconstexpr unsigned int yHistValsPerBinMod =\n>> +\t\tSwIspStats::kYHistogramSize /\n>> +\t\t(SwIspStats::kYHistogramSize % kExposureBinsCount + 1);\n>> +\tint ExposureBins[kExposureBinsCount] = {};\n> \n> s/ExposureBins/exposureBins/\n\nOK\n\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 < kExposureBinsCount; 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> C++ casts.\n\nOK\n\n>> +\n>> +\t/* sanity check */\n> \n> s/sanity/Sanity/\n\nOK\n\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> You shouldn't copy the control list, but instead create one from the\n> ControlInfoMap that you get in init() (and that you then need to stored\n> in the IPA class).\n\nOK, thanks\n\n>> +\n>> +\texposure_ = ctrls.get(V4L2_CID_EXPOSURE).get();\n>> +\tagain_ = ctrls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> \n> \texposure_ = sensorControls.get(V4L2_CID_EXPOSURE).get();\n> \tagain_ = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n\nOK\n\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>> +void IPASoftSimple::updateExposure(double exposureMSV)\n>> +{\n>> +\t/* DENOMINATOR of 10 gives ~10% increment/decrement; DENOMINATOR of 5 - about ~20% */\n> \n> Line wrap, and the DENOMINATOR needs to be renamed to kExpDenominator.\n\nOK\n\nThanks for the review,\nAndrei\n\n>> +\tstatic constexpr uint8_t kExpDenominator = 10;\n>> +\tstatic constexpr uint8_t kExpNumeratorUp = kExpDenominator + 1;\n>> +\tstatic constexpr uint8_t kExpNumeratorDown = kExpDenominator - 1;\n>> +\n>> +\tint next;\n>> +\n>> +\tif (exposureMSV < kExposureOptimal - kExposureSatisfactory) {\n>> +\t\tnext = exposure_ * kExpNumeratorUp / kExpDenominator;\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_ * kExpNumeratorUp / kExpDenominator;\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 > kExposureOptimal + kExposureSatisfactory) {\n>> +\t\tif (exposure_ == exposure_max_ && again_ != again_min_) {\n>> +\t\t\tnext = again_ * kExpNumeratorDown / kExpDenominator;\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_ * kExpNumeratorDown / kExpDenominator;\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>> +\texposure_ = std::clamp(exposure_, exposure_min_, exposure_max_);\n>> +\tagain_ = std::clamp(again_, again_min_, again_max_);\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 */\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 5675FBD160\n\tfor ;\n\tWed, 27 Mar 2024 22:36:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 10A1163331;\n\tWed, 27 Mar 2024 23:36:48 +0100 (CET)","from mail-ej1-x630.google.com (mail-ej1-x630.google.com\n\t[IPv6:2a00:1450:4864:20::630])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AC0A661C35\n\tfor ;\n\tWed, 27 Mar 2024 23:36:45 +0100 (CET)","by mail-ej1-x630.google.com with SMTP id\n\ta640c23a62f3a-a4702457ccbso39913366b.3\n\tfor ;\n\tWed, 27 Mar 2024 15:36:45 -0700 (PDT)","from [192.168.118.26] ([87.116.165.87])\n\tby smtp.gmail.com with ESMTPSA id\n\tgw4-20020a170906f14400b00a46d786365esm29018ejb.94.2024.03.27.15.36.43\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tWed, 27 Mar 2024 15:36:44 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"MsY28qDJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=gmail.com; s=20230601; t=1711579005; x=1712183805;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:in-reply-to:from:content-language\n\t:references:cc:to:subject:user-agent:mime-version:date:message-id\n\t:from:to:cc:subject:date:message-id:reply-to;\n\tbh=lnjUAQD2a0E1QocGAmEMOyl6YNHRUJMX63ZtDTLpnjs=;\n\tb=MsY28qDJw0Y2qowhVVlMVX2DuGpBgtHLMtnxtqlsX9U7DXGkXqMYppZ9MDn0SOE/sk\n\tYn1jUB1RSU8brwO3YfsfdKunbkXuteInKvri4uujnBZ8ESH2xOCGeJBnn/0WbnO+uHZ3\n\tSKr6D9CXHGjC+ASr5aILVpVAsndWNtbo5JR+GX/M1mw3vI4FBY9Q6aBSBKIU+viTDK6r\n\tu+L4veoj1iIMSqfI0Gqlqxhh7gnd+KivYMF7uJpbu1S8b/WX8AZbu5cqQaZbxWr8L/ad\n\t5/YQH6zEZAnEqFt+cwc06Xa56WWieryOx/oJ3c6udiU8wang8iaoN/R4ndY8WgCZ/u0D\n\tz+bQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1711579005; x=1712183805;\n\th=content-transfer-encoding:in-reply-to:from:content-language\n\t:references:cc:to: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=lnjUAQD2a0E1QocGAmEMOyl6YNHRUJMX63ZtDTLpnjs=;\n\tb=pkpeZbtN5XtzR9R4qO7aUlrAnC4wc7G+4mrsWb8TToLXgGvxzhgSFlx2wuds3aSu8j\n\tnI/pqbbfu61Kq8/idJJKOmBV6y1Xsn1wspC01115XeHp85b+aebu8Fukyls889bOAg9e\n\tWop/MzJpuYkBv8F38OyBl3jvO3/1tN4QBz/eoAgseQnDFG46GNqYmEzI2TUFquc1IIGD\n\tgJXcEkwy4GklhLVNZH0MAkahamhhgceeMKAIM+7TE3FfFvFhz6MxVK9kuvDkXYRMS2ak\n\tW8QtXT3ZVWlInGE+TW1gMOsBBpNSzi8zxCaPEYDvfWr5dya+4c1FWm4StC+PZ0+1WnlJ\n\tBSzA==","X-Gm-Message-State":"AOJu0YyedXbNifq0pwdH0BXWOzmVR/vgIKF5OuDVWSbvz+xphNvePZQG\n\t2wCkcQAaawKslPoRm7pcM74OWUkw+j/YH/RMjsyIAu0+BdBeluaZ","X-Google-Smtp-Source":"AGHT+IGXQm8bFleM+KUHU53l+cg5zQ9IOEwUdtJuiD2OL2QrkMROt8VIRsCqUQNggs/NyydIdagppQ==","X-Received":"by 2002:a17:906:b204:b0:a4e:df0:9d8 with SMTP id\n\tp4-20020a170906b20400b00a4e0df009d8mr521913ejz.48.1711579004700; \n\tWed, 27 Mar 2024 15:36:44 -0700 (PDT)","Message-ID":"<46848c9c-5bea-472e-bcb7-efc24479fb45@gmail.com>","Date":"Thu, 28 Mar 2024 01:36:43 +0300","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v6 09/18] libcamera: ipa: add Soft IPA","To":"Laurent Pinchart ,\n\tMilan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org,\n\tBryan O'Donoghue ,\n\tMaxime Ripard , Pavel Machek ,\n\tHans de Goede ,\n\tKieran Bingham ,\n\tDennis Bonke , Marttico , \n\tToon Langendam ","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-10-mzamazal@redhat.com>\n\t<20240327163808.GG4721@pendragon.ideasonboard.com>","Content-Language":"en-US","From":"Andrei Konovalov ","In-Reply-To":"<20240327163808.GG4721@pendragon.ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29128,"web_url":"https://patchwork.libcamera.org/comment/29128/","msgid":"<87sf04huqc.fsf@redhat.com>","date":"2024-04-02T11:25:31","subject":"Re: [PATCH v6 09/18] libcamera: ipa: add Soft IPA","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Andrei Konovalov writes:\n\n> On 27.03.2024 19:38, Laurent Pinchart wrote:\n\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>> Drop the dummy value.\n>\n> libcamera docs do allow signals with zero parameters.\n> But when I tried having zero parameters for an EventInterface function,\n> it didn't work for me iirc.\n> Let me double check.\n\nWhen the parameter is not present, the following code is generated:\n\n void IPAProxySoft::setIspParamsIPC(\n std::vector::const_iterator data,\n size_t dataSize,\n [[maybe_unused]] const std::vector &fds)\n {\n\n\n\n\n setIspParams.emit();\n }\n\nAnd then the compiler complains:\n\n src/libcamera/proxy/soft_ipa_proxy.cpp: In member function ‘void libcamera::ipa::soft::IPAProxySoft::setIspParamsIPC(std::vector::const_iterator, size_t, const std::vector&)’:\n src/libcamera/proxy/soft_ipa_proxy.cpp:416:46: error: unused parameter ‘data’ [-Werror=unused-parameter]\n 416 | std::vector::const_iterator data,\n | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~\n src/libcamera/proxy/soft_ipa_proxy.cpp:417:16: error: unused parameter ‘dataSize’ [-Werror=unused-parameter]\n 417 | size_t dataSize,\n | ~~~~~~~^~~~~~~~\n cc1plus: all warnings being treated as errors\n\nRegards,\nMilan","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 704DEBD16B\n\tfor ;\n\tTue, 2 Apr 2024 11:25:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 639B06333B;\n\tTue, 2 Apr 2024 13:25:40 +0200 (CEST)","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 A86A661C33\n\tfor ;\n\tTue, 2 Apr 2024 13:25:37 +0200 (CEST)","from mail-ed1-f71.google.com (mail-ed1-f71.google.com\n\t[209.85.208.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-551-N9aTICl5OKG7mFDVE_vwAQ-1; Tue, 02 Apr 2024 07:25:35 -0400","by mail-ed1-f71.google.com with SMTP id\n\t4fb4d7f45d1cf-56c079c74d1so2242537a12.2\n\tfor ;\n\tTue, 02 Apr 2024 04:25:34 -0700 (PDT)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\ter13-20020a056402448d00b0056c0a3d91easm6588498edb.12.2024.04.02.04.25.32\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 02 Apr 2024 04:25:32 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"UDd/e9lu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1712057136;\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\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=yQYaDQXHpg5OYD1I2S8inVg0vVwU10AaALHdg798FXg=;\n\tb=UDd/e9ludWe8Cj/SxfKgRbYjA1DooW+a+5Idn2eP75LUjcb/lj43AenbLixl2ANmyYKgR7\n\t6Gu+WZ7g5S9bhQLHW+pfP5c1iTaHAYU0UzX75qZRQNFT3QPUJ6oLLnKkVhI9jVVdVRBXX/\n\thBmjMqTTFaOtPmVEGYokEhracIVXSBE=","X-MC-Unique":"N9aTICl5OKG7mFDVE_vwAQ-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1712057134; x=1712661934;\n\th=content-transfer-encoding:mime-version:user-agent:message-id:date\n\t:references:in-reply-to:subject:cc:to:from:x-gm-message-state:from\n\t:to:cc:subject:date:message-id:reply-to;\n\tbh=ZzSQV61e7Q2A/tdMOLnMpEInlPHKiBM1PciAKfjLezo=;\n\tb=kA5WHf6Q4GnOZjkXS8Cw3Y3KlG3/OstxZvmoUTN0N51RGYlWpgBU5BfCue54NWzwbo\n\t/cu0eHnTJ/QFd/8DLU/zLkWZP3fAmHt/AoHzEPMnZl5xuW+MYJ3qjHuV64sbEX2jAxnf\n\t9aAfhCgXRA/sWgpf9eVlA9jIi3np9/h/y94sJQzc7caneAuTunPw9GlqNeQUGeEofAOY\n\tfJ/pbMoagIXjDqkdQze2Xr14T5HLsB4GAM54qjn2OO9fQ8L8WI7YhloMYijeN8Z84lw3\n\t5R/QW9CBiyCXpVb2KtB7MVBPRXuKl3eQcEzv6RTMcUAH9st+PjXlZaPY7woyUlN9S8NC\n\tbiRA==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCXTYSQb2M6z5jqma2nJZYTKbGdUnHMVOw9Mk6U4asm7gqkKxeKqIMOwm/uykD/zc34vhXiaLTenlHSR71UVwFwY9bzHKQmDPcPxl+m7re9F97Kvng==","X-Gm-Message-State":"AOJu0YztItTinurd/qEfP2awAN8Wjjg0faJPm7XJ8L4GgtlIWeBtqzSt\n\tFirnl/DVXO7v4k80ID7J80DHnkWisuB63caTYvSLMD42ubbk+BUh6do4AQvmva5CTbHbqFz9jH2\n\tO5cfBjXoADy1PgI5EuO+/3x7bCyDntuBGbUkaTawsRzJ2Pkk2uI+evKVoqkPgGci8vDZyE8U=","X-Received":["by 2002:a50:9faa:0:b0:56c:295e:2c01 with SMTP id\n\tc39-20020a509faa000000b0056c295e2c01mr7487669edf.15.1712057133980; \n\tTue, 02 Apr 2024 04:25:33 -0700 (PDT)","by 2002:a50:9faa:0:b0:56c:295e:2c01 with SMTP id\n\tc39-20020a509faa000000b0056c295e2c01mr7487644edf.15.1712057133562; \n\tTue, 02 Apr 2024 04:25:33 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IHrzDRD5X/dfI6Pp+vc/u3JKqT1fEdp4eKNJuDrlZsBb0+kLIh5/DINk2//UXwb7X32ZOU6fw==","From":"Milan Zamazal ","To":"Andrei Konovalov ","Cc":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org, Bryan O'Donoghue\n\t, Maxime Ripard , Pavel\n\tMachek , Hans de Goede , Kieran\n\tBingham , Dennis Bonke\n\t, Marttico , Toon Langendam\n\t","Subject":"Re: [PATCH v6 09/18] libcamera: ipa: add Soft IPA","In-Reply-To":"<46848c9c-5bea-472e-bcb7-efc24479fb45@gmail.com> (Andrei\n\tKonovalov's message of \"Thu, 28 Mar 2024 01:36:43 +0300\")","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-10-mzamazal@redhat.com>\n\t<20240327163808.GG4721@pendragon.ideasonboard.com>\n\t<46848c9c-5bea-472e-bcb7-efc24479fb45@gmail.com>","Date":"Tue, 02 Apr 2024 13:25:31 +0200","Message-ID":"<87sf04huqc.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; 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29139,"web_url":"https://patchwork.libcamera.org/comment/29139/","msgid":"","date":"2024-04-02T20:34:13","subject":"Re: [PATCH v6 09/18] 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 Laurent and Milan,\n\nOn 28.03.2024 01:36, Andrei Konovalov wrote:\n> Hi Laurent,\n> \n> Thank you for the review!\n> \n> On 27.03.2024 19:38, Laurent Pinchart wrote:\n>> Hi Milan and Andrey,\n>>\n>> Thank you for the patch.\n>>\n>> On Tue, Mar 19, 2024 at 01:35:56PM +0100, Milan Zamazal wrote:\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>> This should be addressed, please record this as a \\todo item somewhere.\n> \n> Ack\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>>>   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    | 326 ++++++++++++++++++++++++++++++\n>>>   8 files changed, 394 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>>\n>> Why is this needed ?\n>>\n>>>                            @TOP_BUILDDIR@/src/libcamera/proxy/\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>>> 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>> Ah that's why.\n> \n> Yes, because, well... all the other IPAs were doing that...\n> \n>> It doesn't have to be done before merging, but could you\n>> address this sooner than later ?\n> \n> I can't promise a lot, but I'll look into that.\n> \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>>\n>> The IPA and soft ISP run in separate threads. How do you guarantee that\n>> the stats and params are not accessed concurrently by both ?\n> \n> I tried avoiding using a pool of buffers for stats and params, and used just a\n> single chunk of shared memory to pass the stats from soft ISP (its worker thread)\n> to IPA, and the other chunk for params.\n> \n> The soft ISP accumulates the stats in its member variable, and after all the stats\n> for the frame are calculated, the member variable is copyed to the shared memory and\n> statReady signal is emitted (so using the member var implements some kind of double\n> buffering). This way the write to the stats shared memory happens once per frame and\n> is short in time right before emitting the statReady signal.\n> The callback executed on receiving statReady signal invokes the IPA's processStats\n> function. So the IPA's processStats() is called soon after the statReady is emitted.\n> If the IPA finish processing the stats before the next frame is processed by the soft IPS\n> and the stats for the next frame are written into the shared memory, there is no concurrent\n> acess.\n> \n> This doesn't strictly guarantee that the stats are not accessed concurrently by the IPA and\n> soft ISP. But unless the video stream from the camera sensor overloads the system this\n> scheme works OK. And if the load is high the concurrent access isn't impossible, and can harm\n> the image quality, but doesn't lead to critical problems like invalid pointers as the stats\n> structure only contains numbers and an array of fixed size.\n> \n>> Shouldn't\n>> you have a pool of buffers for each, mimicking what is done with\n>> hardware ISPs ?\n> \n> This would definitely work, but is a more heavy wait implementation.\n> \n> On the other side, the current implementation is less universal (isn't very scalable), more fragile,\n> and can't reliably associate the stats and the params with the given frame from the camera sensor.\n> \n> So let me try to implement what you suggested (\"a pool of buffers for each, mimicking what is done with\n> hardware ISPs\").\n\nI won't be able to do this particular part by tomorrow...\nSo a branch I plan to publish tomorrow - with this patch updated to address the review comments\nand the [PATCH v6 18/18] \"libcamera: Soft IPA: use CameraSensorHelper for analogue gain\" merged\ninto the earlier ones - would not use the pool of buffers for the stats and the params (yet).\n\nThanks,\nAndrei\n\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>> Drop the dummy value.\n> \n> libcamera docs do allow signals with zero parameters.\n> But when I tried having zero parameters for an EventInterface function,\n> it didn't work for me iirc.\n> Let me double check.\n> \n>>> +};\n>>> diff --git a/meson_options.txt b/meson_options.txt\n>>> index 99dab96d..2644bef0 100644\n>>> --- a/meson_options.txt\n>>> +++ b/meson_options.txt\n>>> @@ -27,7 +27,7 @@ option('gstreamer',\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>>>   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>>\n>> We use YAML files for all IPAs, could you do the same here ? It\n>> shouldn't be much extra work as the file is empty :-)\n> \n> OK\n> \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..312df4ba\n>>> --- /dev/null\n>>> +++ b/src/ipa/simple/soft_simple.cpp\n>>> @@ -0,0 +1,326 @@\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>>\n>> Not needed.\n> \n> OK\n> \n>>> +#include \"libcamera/internal/software_isp/debayer_params.h\"\n>>> +#include \"libcamera/internal/software_isp/swisp_stats.h\"\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>>> +        : params_(static_cast(MAP_FAILED)),\n>>\n>> Initialize this to nullptr, that's more standard, and will make the\n>> tests in the destructor nicer. init() will have to set the pointers to\n>> null if mmap() calls fail.\n> \n> OK\n> \n>>> +          stats_(static_cast(MAP_FAILED)), ignore_updates_(0)\n>>\n>> s/ignore_updates_/ignoreUpdates_/\n> \n> OK\n> \n>>> +    {\n>>> +    }\n>>> +\n>>> +    ~IPASoftSimple()\n>>> +    {\n>>> +        if (stats_ != MAP_FAILED)\n>>> +            munmap(stats_, sizeof(SwIspStats));\n>>> +        if (params_ != MAP_FAILED)\n>>> +            munmap(params_, sizeof(DebayerParams));\n>>> +    }\n>>\n>> No need to inline this.\n> \n> OK\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>>> +    int32_t exposure_min_, exposure_max_;\n>>> +    int32_t again_min_, again_max_;\n>>> +    int32_t again_, exposure_;\n>>> +    unsigned int ignore_updates_;\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_ = fdStats;\n>>\n>> As you never use fdStats_ and fdParams_ after this function returns,\n>> there's no need to store a copy.\n> \n> Good catch. OK\n> \n>>> +    if (!fdStats_.isValid()) {\n>>> +        LOG(IPASoft, Error) << \"Invalid Statistics handle\";\n>>> +        return -ENODEV;\n>>> +    }\n>>> +\n>>> +    fdParams_ = 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>>> +    if (params_ == MAP_FAILED) {\n>>> +        LOG(IPASoft, Error) << \"Unable to map Parameters\";\n>>> +        return -errno;\n>>> +    }\n>>\n>>     void *mem = mmap(nullptr, sizeof(DebayerParams), PROT_WRITE, MAP_SHARED,\n>>              fdParams_.get(), 0));\n>>     if (mem == MAP_FAILED) {\n>>         LOG(IPASoft, Error) << \"Unable to map Parameters\";\n>>         return -errno;\n>>     }\n>>\n>>     params_ = static_cast(mem);\n> \n> OK\n> \n>>> +\n>>> +    stats_ = static_cast(mmap(nullptr, sizeof(SwIspStats),\n>>> +                        PROT_READ, MAP_SHARED,\n>>> +                        fdStats_.get(), 0));\n>>> +    if (stats_ == MAP_FAILED) {\n>>> +        LOG(IPASoft, Error) << \"Unable to map Statistics\";\n>>> +        return -errno;\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>>> +    return 0;\n>>> +}\n>>> +\n>>> +int IPASoftSimple::configure(const ControlInfoMap &sensorInfoMap)\n>>> +{\n>>> +    const ControlInfo &exposure_info = sensorInfoMap.find(V4L2_CID_EXPOSURE)->second;\n>>\n>> s/exposure_info/exposureInfo/\n>>\n>> I'll let you address the other snake_case to camelCase conversions in\n>> the series :-)\n> \n> OK\n> \n>>> +    const ControlInfo &gain_info = sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN)->second;\n>>> +\n>>> +    exposure_min_ = exposure_info.min().get();\n>>> +    exposure_max_ = exposure_info.max().get();\n>>> +    if (!exposure_min_) {\n>>> +        LOG(IPASoft, Warning) << \"Minimum exposure is zero, that can't be linear\";\n>>> +        exposure_min_ = 1;\n>>> +    }\n>>> +\n>>> +    again_min_ = gain_info.min().get();\n>>> +    again_max_ = gain_info.max().get();\n>>\n>> Add a blank line.\n> \n> OK\n> \n>>> +    /*\n>>> +     * The camera sensor gain (g) is usually not equal to the value written\n>>> +     * into the gain register (x). But the way how the AGC algorithm changes\n>>> +     * the gain value to make the total exposure closer to the optimum assumes\n>>> +     * that g(x) is not too far from linear function. If the minimal gain is 0,\n>>> +     * the g(x) is likely to be far from the linear, like g(x) = a / (b * x + c).\n>>> +     * To avoid unexpected changes to the gain by the AGC algorithm (abrupt near\n>>> +     * one edge, and very small near the other) we limit the range of the gain\n>>> +     * values used.\n>>> +     */\n>>> +    if (!again_min_) {\n>>> +        LOG(IPASoft, Warning) << \"Minimum gain is zero, that can't be linear\";\n>>> +        again_min_ = std::min(100, again_min_ / 2 + again_max_ / 2);\n>>> +    }\n>>\n>> A patch further in the series addresses this by using\n>> CameraSensorHelper. It should be squashed with this patch.\n> \n> OK\n> \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>>> +/*\n>>> + * The number of bins to use for the optimal exposure calculations.\n>>> + */\n>>> +static constexpr unsigned int kExposureBinsCount = 5;\n>>\n>> Missing blank line. Same below.\n> \n> OK\n> \n>>> +/*\n>>> + * The exposure is optimal when the mean sample value of the histogram is\n>>> + * in the middle of the range.\n>>> + */\n>>> +static constexpr float kExposureOptimal = kExposureBinsCount / 2.0;\n>>> +/*\n>>> + * The below value implements the hysteresis for the exposure adjustment.\n>>> + * It is small enough to have the exposure close to the optimal, and is big\n>>> + * enough to prevent the exposure from wobbling around the optimal value.\n>>> + */\n>>> +static constexpr float kExposureSatisfactory = 0.2;\n>>\n>> Move these to the beginning of the file, just before the IPASoftSimple\n>> definition.\n> \n> OK\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>> Do you envision switching to the libipa/algorithm.h API at some point ?\n>> If so, it would be nice to record it somewhere.\n> \n> At some point, yes.\n> Will mention that.\n> \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>>> +    if (ignore_updates_ > 0) {\n>>> +        --ignore_updates_;\n>>> +        return;\n>>> +    }\n>>\n>> Also something that could be improved and that should be recorded\n>> somewhere.\n> \n> OK\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 yHistValsPerBin =\n>>> +        SwIspStats::kYHistogramSize / kExposureBinsCount;\n>>> +    constexpr unsigned int yHistValsPerBinMod =\n>>> +        SwIspStats::kYHistogramSize /\n>>> +        (SwIspStats::kYHistogramSize % kExposureBinsCount + 1);\n>>> +    int ExposureBins[kExposureBinsCount] = {};\n>>\n>> s/ExposureBins/exposureBins/\n> \n> OK\n> \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 < kExposureBinsCount; 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>> C++ casts.\n> \n> OK\n> \n>>> +\n>>> +    /* sanity check */\n>>\n>> s/sanity/Sanity/\n> \n> OK\n> \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>> You shouldn't copy the control list, but instead create one from the\n>> ControlInfoMap that you get in init() (and that you then need to stored\n>> in the IPA class).\n> \n> OK, thanks\n> \n>>> +\n>>> +    exposure_ = ctrls.get(V4L2_CID_EXPOSURE).get();\n>>> +    again_ = ctrls.get(V4L2_CID_ANALOGUE_GAIN).get();\n>>\n>>     exposure_ = sensorControls.get(V4L2_CID_EXPOSURE).get();\n>>     again_ = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> \n> OK\n> \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>>> +void IPASoftSimple::updateExposure(double exposureMSV)\n>>> +{\n>>> +    /* DENOMINATOR of 10 gives ~10% increment/decrement; DENOMINATOR of 5 - about ~20% */\n>>\n>> Line wrap, and the DENOMINATOR needs to be renamed to kExpDenominator.\n> \n> OK\n> \n> Thanks for the review,\n> Andrei\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>>> +    int next;\n>>> +\n>>> +    if (exposureMSV < kExposureOptimal - kExposureSatisfactory) {\n>>> +        next = exposure_ * kExpNumeratorUp / kExpDenominator;\n>>> +        if (next - exposure_ < 1)\n>>> +            exposure_ += 1;\n>>> +        else\n>>> +            exposure_ = next;\n>>> +        if (exposure_ >= exposure_max_) {\n>>> +            next = again_ * kExpNumeratorUp / kExpDenominator;\n>>> +            if (next - again_ < 1)\n>>> +                again_ += 1;\n>>> +            else\n>>> +                again_ = next;\n>>> +        }\n>>> +    }\n>>> +\n>>> +    if (exposureMSV > kExposureOptimal + kExposureSatisfactory) {\n>>> +        if (exposure_ == exposure_max_ && again_ != again_min_) {\n>>> +            next = again_ * kExpNumeratorDown / kExpDenominator;\n>>> +            if (again_ - next < 1)\n>>> +                again_ -= 1;\n>>> +            else\n>>> +                again_ = next;\n>>> +        } else {\n>>> +            next = exposure_ * kExpNumeratorDown / kExpDenominator;\n>>> +            if (exposure_ - next < 1)\n>>> +                exposure_ -= 1;\n>>> +            else\n>>> +                exposure_ = next;\n>>> +        }\n>>> +    }\n>>> +\n>>> +    exposure_ = std::clamp(exposure_, exposure_min_, exposure_max_);\n>>> +    again_ = std::clamp(again_, again_min_, again_max_);\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>>","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 CECEFC0DA4\n\tfor ;\n\tTue, 2 Apr 2024 20:34:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C1A2163360;\n\tTue, 2 Apr 2024 22:34:17 +0200 (CEST)","from mail-ed1-x534.google.com (mail-ed1-x534.google.com\n\t[IPv6:2a00:1450:4864:20::534])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1FC776308D\n\tfor ;\n\tTue, 2 Apr 2024 22:34:16 +0200 (CEST)","by mail-ed1-x534.google.com with SMTP id\n\t4fb4d7f45d1cf-56bc5a3aeb9so7201852a12.3\n\tfor ;\n\tTue, 02 Apr 2024 13:34:16 -0700 (PDT)","from [192.168.118.26] ([87.116.167.129])\n\tby smtp.gmail.com with ESMTPSA id\n\tk23-20020a170906579700b00a4a33ca6497sm6858222ejq.204.2024.04.02.13.34.14\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tTue, 02 Apr 2024 13:34:14 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"Fcqtmrok\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=gmail.com; s=20230601; t=1712090055; x=1712694855;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:in-reply-to:content-language:references\n\t:cc:to:from:subject:user-agent:mime-version:date:message-id:from:to\n\t:cc:subject:date:message-id:reply-to;\n\tbh=3nuPnwVmoH5Y0ThWYUlXSp65OxjNK5mZ6zP9tAYUD4g=;\n\tb=FcqtmrokLcc8bhvW/g8GkeBWWymM2or1Pn4040B6xp74E0KpOR2ia/vD7FcJsIIZrB\n\tW0B3YF+iXKQ6RM33JD0FYKi0nTfff3fOe3Fipbh666XzeC6iCZE+EwS+ThpKZF5YgilC\n\tOm86KOLeh+UZRrKGWqKExAEgXPaFiiaisYxaDx9Y5hxpUTnnf2JzGi3avcKuosDDPxws\n\tA6m27U9EVzFM7XonPXZEuDFj1KLHlPW0IiZmpLZ8cXBSMqw/5sdvdfT4+6KX7XMSl/Hu\n\tGhMYUE+34bWV7CrqMSwaM9PHBIOrcRMgBQnk7HvhKF/h7BMraODR3MToNIQJCKYrCHxY\n\tJriQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1712090055; x=1712694855;\n\th=content-transfer-encoding:in-reply-to:content-language:references\n\t:cc:to:from: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=3nuPnwVmoH5Y0ThWYUlXSp65OxjNK5mZ6zP9tAYUD4g=;\n\tb=rUn1cb/DnxGMq3/ww2PNRbh2SfEkCn//vh7Loo+4uQC6ro5feV/yz4i7ZYCZ9EPKZ8\n\t0IAGznyhTscKF6XAx3ORt1ka6+LDGs0RjHpSYrtX2M597iyGqqBOiEUJ70VBk/PEzeQf\n\tZbn4SUcK0x9CDmb79IavckGgJ+jp2POQRR/QBrzM5AyBVQakCzHSsOdDNAlzX1pDkgwN\n\t72yd0qc/9FlhevAe7vTq7WtBqH1pT6ohl8NjHGMsul433CqYsMRPfoxmG+tHzNnu5u9t\n\toceolxqbF5ahQiOc68Ohes65vs4OtZPOllG4gDXtuSQA09pmalDqTd6V7nT1NV8oGZ7F\n\toeCQ==","X-Gm-Message-State":"AOJu0YxT35uHn23e5GjfSUiuZilK4D4mxYUH2/jJ+M6NUJa94FXE8x+r\n\ttLavbgxx6gRURFFuKQIXDXNj42+GmXMjsu8YZjPvbqJ6dIUOpkxa","X-Google-Smtp-Source":"AGHT+IHwHzF/HjaIR6wZgtZCAcQfjkXhaRVY503tw+GeieWsLbjnjLU8PBlKzvjjCh00z1v+NMRqkw==","X-Received":"by 2002:a17:907:86ab:b0:a4e:5bad:b6f3 with SMTP id\n\tqa43-20020a17090786ab00b00a4e5badb6f3mr6710852ejc.10.1712090055112; \n\tTue, 02 Apr 2024 13:34:15 -0700 (PDT)","Message-ID":"","Date":"Tue, 2 Apr 2024 23:34:13 +0300","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v6 09/18] libcamera: ipa: add Soft IPA","From":"Andrei Konovalov ","To":"Laurent Pinchart ,\n\tMilan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org,\n\tBryan O'Donoghue ,\n\tMaxime Ripard , Pavel Machek ,\n\tHans de Goede ,\n\tKieran Bingham ,\n\tDennis Bonke , Marttico , \n\tToon Langendam ","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-10-mzamazal@redhat.com>\n\t<20240327163808.GG4721@pendragon.ideasonboard.com>\n\t<46848c9c-5bea-472e-bcb7-efc24479fb45@gmail.com>","Content-Language":"en-US","In-Reply-To":"<46848c9c-5bea-472e-bcb7-efc24479fb45@gmail.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29140,"web_url":"https://patchwork.libcamera.org/comment/29140/","msgid":"<20240402203956.GA16740@pendragon.ideasonboard.com>","date":"2024-04-02T20:39:56","subject":"Re: [PATCH v6 09/18] libcamera: ipa: add Soft IPA","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Milan,\n\nOn Tue, Apr 02, 2024 at 01:25:31PM +0200, Milan Zamazal wrote:\n> Andrei Konovalov writes:\n> \n> > On 27.03.2024 19:38, Laurent Pinchart wrote:\n> \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> >> Drop the dummy value.\n> >\n> > libcamera docs do allow signals with zero parameters.\n> > But when I tried having zero parameters for an EventInterface function,\n> > it didn't work for me iirc.\n> > Let me double check.\n> \n> When the parameter is not present, the following code is generated:\n> \n> void IPAProxySoft::setIspParamsIPC(\n> std::vector::const_iterator data,\n> size_t dataSize,\n> [[maybe_unused]] const std::vector &fds)\n> {\n> \n> \n> \n> \n> setIspParams.emit();\n> }\n> \n> And then the compiler complains:\n> \n> src/libcamera/proxy/soft_ipa_proxy.cpp: In member function ‘void libcamera::ipa::soft::IPAProxySoft::setIspParamsIPC(std::vector::const_iterator, size_t, const std::vector&)’:\n> src/libcamera/proxy/soft_ipa_proxy.cpp:416:46: error: unused parameter ‘data’ [-Werror=unused-parameter]\n> 416 | std::vector::const_iterator data,\n> | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~\n> src/libcamera/proxy/soft_ipa_proxy.cpp:417:16: error: unused parameter ‘dataSize’ [-Werror=unused-parameter]\n> 417 | size_t dataSize,\n> | ~~~~~~~^~~~~~~~\n> cc1plus: all warnings being treated as errors\n\nAh, while we support signals without parameters, it looks like we\nhaven't tested them in the IPA interface. Oops.\n\nPaul, would you be able to help Milan with this ?","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 D16A7BD16B\n\tfor ;\n\tTue, 2 Apr 2024 20:40:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0BABD63360;\n\tTue, 2 Apr 2024 22:40:08 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 54AA86308D\n\tfor ;\n\tTue, 2 Apr 2024 22:40:07 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D3C121A2;\n\tTue, 2 Apr 2024 22:39:29 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"MPagfZZj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1712090370;\n\tbh=rTyhAhMr1eIY98pFPLgSRtBd1vMTrB1Lo5WaPNSe1bk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=MPagfZZjMbqMXiZexQvrXOKVSgXzTcwF0hvxQNoNv00hB9SsIb2C70KuEuAm04mTx\n\tfY9VYIp0KfRdZ8C0IvbzIgKstbcsrSKqTfLqjOUkOI+ClTGFeo85RsKlYEhAFlVU8r\n\toR/MI2P2yg7Xt7zbVzoP7AKKXAS3tImeSI58HGEo=","Date":"Tue, 2 Apr 2024 23:39:56 +0300","From":"Laurent Pinchart ","To":"Milan Zamazal ,\n\tPaul Elder ","Cc":"Andrei Konovalov ,\n\tlibcamera-devel@lists.libcamera.org,\n\tBryan O'Donoghue ,\n\tMaxime Ripard , Pavel Machek ,\n\tHans de Goede ,\n\tKieran Bingham ,\n\tDennis Bonke , Marttico , \n\tToon Langendam ","Subject":"Re: [PATCH v6 09/18] libcamera: ipa: add Soft IPA","Message-ID":"<20240402203956.GA16740@pendragon.ideasonboard.com>","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-10-mzamazal@redhat.com>\n\t<20240327163808.GG4721@pendragon.ideasonboard.com>\n\t<46848c9c-5bea-472e-bcb7-efc24479fb45@gmail.com>\n\t<87sf04huqc.fsf@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<87sf04huqc.fsf@redhat.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29141,"web_url":"https://patchwork.libcamera.org/comment/29141/","msgid":"<20240402204813.GB16740@pendragon.ideasonboard.com>","date":"2024-04-02T20:48:13","subject":"Re: [PATCH v6 09/18] libcamera: ipa: add Soft IPA","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Andrei,\n\nOn Tue, Apr 02, 2024 at 11:34:13PM +0300, Andrei Konovalov wrote:\n> On 28.03.2024 01:36, Andrei Konovalov wrote:\n> > On 27.03.2024 19:38, Laurent Pinchart wrote:\n> >> On Tue, Mar 19, 2024 at 01:35:56PM +0100, Milan Zamazal wrote:\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> >> This should be addressed, please record this as a \\todo item somewhere.\n> > \n> > Ack\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> >>>   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    | 326 ++++++++++++++++++++++++++++++\n> >>>   8 files changed, 394 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> >>\n> >> Why is this needed ?\n> >>\n> >>>                            @TOP_BUILDDIR@/src/libcamera/proxy/\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> >>> 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> >> Ah that's why.\n> > \n> > Yes, because, well... all the other IPAs were doing that...\n> > \n> >> It doesn't have to be done before merging, but could you\n> >> address this sooner than later ?\n> > \n> > I can't promise a lot, but I'll look into that.\n> > \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> >>\n> >> The IPA and soft ISP run in separate threads. How do you guarantee that\n> >> the stats and params are not accessed concurrently by both ?\n> > \n> > I tried avoiding using a pool of buffers for stats and params, and used just a\n> > single chunk of shared memory to pass the stats from soft ISP (its worker thread)\n> > to IPA, and the other chunk for params.\n> > \n> > The soft ISP accumulates the stats in its member variable, and after all the stats\n> > for the frame are calculated, the member variable is copyed to the shared memory and\n> > statReady signal is emitted (so using the member var implements some kind of double\n> > buffering). This way the write to the stats shared memory happens once per frame and\n> > is short in time right before emitting the statReady signal.\n> > The callback executed on receiving statReady signal invokes the IPA's processStats\n> > function. So the IPA's processStats() is called soon after the statReady is emitted.\n> > If the IPA finish processing the stats before the next frame is processed by the soft IPS\n> > and the stats for the next frame are written into the shared memory, there is no concurrent\n> > acess.\n> > \n> > This doesn't strictly guarantee that the stats are not accessed concurrently by the IPA and\n> > soft ISP. But unless the video stream from the camera sensor overloads the system this\n> > scheme works OK. And if the load is high the concurrent access isn't impossible, and can harm\n> > the image quality, but doesn't lead to critical problems like invalid pointers as the stats\n> > structure only contains numbers and an array of fixed size.\n> >\n> >> Shouldn't\n> >> you have a pool of buffers for each, mimicking what is done with\n> >> hardware ISPs ?\n> > \n> > This would definitely work, but is a more heavy wait implementation.\n> > \n> > On the other side, the current implementation is less universal (isn't very scalable), more fragile,\n> > and can't reliably associate the stats and the params with the given frame from the camera sensor.\n> > \n> > So let me try to implement what you suggested (\"a pool of buffers for each, mimicking what is done with\n> > hardware ISPs\").\n> \n> I won't be able to do this particular part by tomorrow...\n> So a branch I plan to publish tomorrow - with this patch updated to address the review comments\n> and the [PATCH v6 18/18] \"libcamera: Soft IPA: use CameraSensorHelper for analogue gain\" merged\n> into the earlier ones - would not use the pool of buffers for the stats and the params (yet).\n\nThat's fine. While I don't like relying on race windows being small, and\non hitting the race being harmless (as it's dangerous and not a very\ngood design), it doesn't have to be fixed to merge this series. It\nshould however be recorded as a TODO item in the next version, and be\naddressed on top sooner than later, before adding more features.\n\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> >> Drop the dummy value.\n> > \n> > libcamera docs do allow signals with zero parameters.\n> > But when I tried having zero parameters for an EventInterface function,\n> > it didn't work for me iirc.\n> > Let me double check.\n\nMilan replied separately on this. It looks like we've never tested that,\nI think Paul will be able to help.\n\n> >>> +};\n> >>> diff --git a/meson_options.txt b/meson_options.txt\n> >>> index 99dab96d..2644bef0 100644\n> >>> --- a/meson_options.txt\n> >>> +++ b/meson_options.txt\n> >>> @@ -27,7 +27,7 @@ option('gstreamer',\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> >>>   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> >>\n> >> We use YAML files for all IPAs, could you do the same here ? It\n> >> shouldn't be much extra work as the file is empty :-)\n> > \n> > OK\n> > \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..312df4ba\n> >>> --- /dev/null\n> >>> +++ b/src/ipa/simple/soft_simple.cpp\n> >>> @@ -0,0 +1,326 @@\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> >>\n> >> Not needed.\n> > \n> > OK\n> > \n> >>> +#include \"libcamera/internal/software_isp/debayer_params.h\"\n> >>> +#include \"libcamera/internal/software_isp/swisp_stats.h\"\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> >>> +        : params_(static_cast(MAP_FAILED)),\n> >>\n> >> Initialize this to nullptr, that's more standard, and will make the\n> >> tests in the destructor nicer. init() will have to set the pointers to\n> >> null if mmap() calls fail.\n> > \n> > OK\n> > \n> >>> +          stats_(static_cast(MAP_FAILED)), ignore_updates_(0)\n> >>\n> >> s/ignore_updates_/ignoreUpdates_/\n> > \n> > OK\n> > \n> >>> +    {\n> >>> +    }\n> >>> +\n> >>> +    ~IPASoftSimple()\n> >>> +    {\n> >>> +        if (stats_ != MAP_FAILED)\n> >>> +            munmap(stats_, sizeof(SwIspStats));\n> >>> +        if (params_ != MAP_FAILED)\n> >>> +            munmap(params_, sizeof(DebayerParams));\n> >>> +    }\n> >>\n> >> No need to inline this.\n> > \n> > OK\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> >>> +    int32_t exposure_min_, exposure_max_;\n> >>> +    int32_t again_min_, again_max_;\n> >>> +    int32_t again_, exposure_;\n> >>> +    unsigned int ignore_updates_;\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_ = fdStats;\n> >>\n> >> As you never use fdStats_ and fdParams_ after this function returns,\n> >> there's no need to store a copy.\n> > \n> > Good catch. OK\n> > \n> >>> +    if (!fdStats_.isValid()) {\n> >>> +        LOG(IPASoft, Error) << \"Invalid Statistics handle\";\n> >>> +        return -ENODEV;\n> >>> +    }\n> >>> +\n> >>> +    fdParams_ = 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> >>> +    if (params_ == MAP_FAILED) {\n> >>> +        LOG(IPASoft, Error) << \"Unable to map Parameters\";\n> >>> +        return -errno;\n> >>> +    }\n> >>\n> >>     void *mem = mmap(nullptr, sizeof(DebayerParams), PROT_WRITE, MAP_SHARED,\n> >>              fdParams_.get(), 0));\n> >>     if (mem == MAP_FAILED) {\n> >>         LOG(IPASoft, Error) << \"Unable to map Parameters\";\n> >>         return -errno;\n> >>     }\n> >>\n> >>     params_ = static_cast(mem);\n> > \n> > OK\n> > \n> >>> +\n> >>> +    stats_ = static_cast(mmap(nullptr, sizeof(SwIspStats),\n> >>> +                        PROT_READ, MAP_SHARED,\n> >>> +                        fdStats_.get(), 0));\n> >>> +    if (stats_ == MAP_FAILED) {\n> >>> +        LOG(IPASoft, Error) << \"Unable to map Statistics\";\n> >>> +        return -errno;\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> >>> +    return 0;\n> >>> +}\n> >>> +\n> >>> +int IPASoftSimple::configure(const ControlInfoMap &sensorInfoMap)\n> >>> +{\n> >>> +    const ControlInfo &exposure_info = sensorInfoMap.find(V4L2_CID_EXPOSURE)->second;\n> >>\n> >> s/exposure_info/exposureInfo/\n> >>\n> >> I'll let you address the other snake_case to camelCase conversions in\n> >> the series :-)\n> > \n> > OK\n> > \n> >>> +    const ControlInfo &gain_info = sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN)->second;\n> >>> +\n> >>> +    exposure_min_ = exposure_info.min().get();\n> >>> +    exposure_max_ = exposure_info.max().get();\n> >>> +    if (!exposure_min_) {\n> >>> +        LOG(IPASoft, Warning) << \"Minimum exposure is zero, that can't be linear\";\n> >>> +        exposure_min_ = 1;\n> >>> +    }\n> >>> +\n> >>> +    again_min_ = gain_info.min().get();\n> >>> +    again_max_ = gain_info.max().get();\n> >>\n> >> Add a blank line.\n> > \n> > OK\n> > \n> >>> +    /*\n> >>> +     * The camera sensor gain (g) is usually not equal to the value written\n> >>> +     * into the gain register (x). But the way how the AGC algorithm changes\n> >>> +     * the gain value to make the total exposure closer to the optimum assumes\n> >>> +     * that g(x) is not too far from linear function. If the minimal gain is 0,\n> >>> +     * the g(x) is likely to be far from the linear, like g(x) = a / (b * x + c).\n> >>> +     * To avoid unexpected changes to the gain by the AGC algorithm (abrupt near\n> >>> +     * one edge, and very small near the other) we limit the range of the gain\n> >>> +     * values used.\n> >>> +     */\n> >>> +    if (!again_min_) {\n> >>> +        LOG(IPASoft, Warning) << \"Minimum gain is zero, that can't be linear\";\n> >>> +        again_min_ = std::min(100, again_min_ / 2 + again_max_ / 2);\n> >>> +    }\n> >>\n> >> A patch further in the series addresses this by using\n> >> CameraSensorHelper. It should be squashed with this patch.\n> > \n> > OK\n> > \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> >>> +/*\n> >>> + * The number of bins to use for the optimal exposure calculations.\n> >>> + */\n> >>> +static constexpr unsigned int kExposureBinsCount = 5;\n> >>\n> >> Missing blank line. Same below.\n> > \n> > OK\n> > \n> >>> +/*\n> >>> + * The exposure is optimal when the mean sample value of the histogram is\n> >>> + * in the middle of the range.\n> >>> + */\n> >>> +static constexpr float kExposureOptimal = kExposureBinsCount / 2.0;\n> >>> +/*\n> >>> + * The below value implements the hysteresis for the exposure adjustment.\n> >>> + * It is small enough to have the exposure close to the optimal, and is big\n> >>> + * enough to prevent the exposure from wobbling around the optimal value.\n> >>> + */\n> >>> +static constexpr float kExposureSatisfactory = 0.2;\n> >>\n> >> Move these to the beginning of the file, just before the IPASoftSimple\n> >> definition.\n> > \n> > OK\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> >> Do you envision switching to the libipa/algorithm.h API at some point ?\n> >> If so, it would be nice to record it somewhere.\n> > \n> > At some point, yes.\n> > Will mention that.\n> > \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> >>> +    if (ignore_updates_ > 0) {\n> >>> +        --ignore_updates_;\n> >>> +        return;\n> >>> +    }\n> >>\n> >> Also something that could be improved and that should be recorded\n> >> somewhere.\n> > \n> > OK\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 yHistValsPerBin =\n> >>> +        SwIspStats::kYHistogramSize / kExposureBinsCount;\n> >>> +    constexpr unsigned int yHistValsPerBinMod =\n> >>> +        SwIspStats::kYHistogramSize /\n> >>> +        (SwIspStats::kYHistogramSize % kExposureBinsCount + 1);\n> >>> +    int ExposureBins[kExposureBinsCount] = {};\n> >>\n> >> s/ExposureBins/exposureBins/\n> > \n> > OK\n> > \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 < kExposureBinsCount; 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> >> C++ casts.\n> > \n> > OK\n> > \n> >>> +\n> >>> +    /* sanity check */\n> >>\n> >> s/sanity/Sanity/\n> > \n> > OK\n> > \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> >> You shouldn't copy the control list, but instead create one from the\n> >> ControlInfoMap that you get in init() (and that you then need to stored\n> >> in the IPA class).\n> > \n> > OK, thanks\n> > \n> >>> +\n> >>> +    exposure_ = ctrls.get(V4L2_CID_EXPOSURE).get();\n> >>> +    again_ = ctrls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> >>\n> >>     exposure_ = sensorControls.get(V4L2_CID_EXPOSURE).get();\n> >>     again_ = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> > \n> > OK\n> > \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> >>> +void IPASoftSimple::updateExposure(double exposureMSV)\n> >>> +{\n> >>> +    /* DENOMINATOR of 10 gives ~10% increment/decrement; DENOMINATOR of 5 - about ~20% */\n> >>\n> >> Line wrap, and the DENOMINATOR needs to be renamed to kExpDenominator.\n> > \n> > OK\n> > \n> > Thanks for the review,\n> > Andrei\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> >>> +    int next;\n> >>> +\n> >>> +    if (exposureMSV < kExposureOptimal - kExposureSatisfactory) {\n> >>> +        next = exposure_ * kExpNumeratorUp / kExpDenominator;\n> >>> +        if (next - exposure_ < 1)\n> >>> +            exposure_ += 1;\n> >>> +        else\n> >>> +            exposure_ = next;\n> >>> +        if (exposure_ >= exposure_max_) {\n> >>> +            next = again_ * kExpNumeratorUp / kExpDenominator;\n> >>> +            if (next - again_ < 1)\n> >>> +                again_ += 1;\n> >>> +            else\n> >>> +                again_ = next;\n> >>> +        }\n> >>> +    }\n> >>> +\n> >>> +    if (exposureMSV > kExposureOptimal + kExposureSatisfactory) {\n> >>> +        if (exposure_ == exposure_max_ && again_ != again_min_) {\n> >>> +            next = again_ * kExpNumeratorDown / kExpDenominator;\n> >>> +            if (again_ - next < 1)\n> >>> +                again_ -= 1;\n> >>> +            else\n> >>> +                again_ = next;\n> >>> +        } else {\n> >>> +            next = exposure_ * kExpNumeratorDown / kExpDenominator;\n> >>> +            if (exposure_ - next < 1)\n> >>> +                exposure_ -= 1;\n> >>> +            else\n> >>> +                exposure_ = next;\n> >>> +        }\n> >>> +    }\n> >>> +\n> >>> +    exposure_ = std::clamp(exposure_, exposure_min_, exposure_max_);\n> >>> +    again_ = std::clamp(again_, again_min_, again_max_);\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 */","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 0D84BC0DA4\n\tfor ;\n\tTue, 2 Apr 2024 20:48:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 37FA263369;\n\tTue, 2 Apr 2024 22:48:26 +0200 (CEST)","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 8F0CF63334\n\tfor ;\n\tTue, 2 Apr 2024 22:48:24 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 31E1C1A2;\n\tTue, 2 Apr 2024 22:47:47 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"OT9c37CS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1712090867;\n\tbh=1pQWXe94M4U8mRlOpbL5rqvVh4WGkxVcqJ/FH2gvMZY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=OT9c37CSQV30NAISzOn5bg+QIe188DxisjkbYpGmSe204yzk4C0uDu/evGFYsa7/i\n\tsQBhlKPtpZxpNeJiMBbbB+4bykR1Y38ZaIYm7ly4v5gh3OmG4p1CjXQ1MR6KTC2RbS\n\tldl+v6hlNljw2SbaFTF9zqMaovwMuflE4j5J6Mos=","Date":"Tue, 2 Apr 2024 23:48:13 +0300","From":"Laurent Pinchart ","To":"Andrei Konovalov ","Cc":"Milan Zamazal , libcamera-devel@lists.libcamera.org,\n\tBryan O'Donoghue ,\n\tMaxime Ripard , Pavel Machek ,\n\tHans de Goede ,\n\tKieran Bingham ,\n\tDennis Bonke , Marttico , \n\tToon Langendam ","Subject":"Re: [PATCH v6 09/18] libcamera: ipa: add Soft IPA","Message-ID":"<20240402204813.GB16740@pendragon.ideasonboard.com>","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-10-mzamazal@redhat.com>\n\t<20240327163808.GG4721@pendragon.ideasonboard.com>\n\t<46848c9c-5bea-472e-bcb7-efc24479fb45@gmail.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29145,"web_url":"https://patchwork.libcamera.org/comment/29145/","msgid":"<871q7mg73a.fsf@redhat.com>","date":"2024-04-03T08:53:45","subject":"Re: [PATCH v6 09/18] libcamera: ipa: add Soft IPA","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Andrei Konovalov writes:\n\n> Hi Laurent and Milan,\n>\n> On 28.03.2024 01:36, Andrei Konovalov wrote:\n>> Hi Laurent,\n>> Thank you for the review!\n>> On 27.03.2024 19:38, Laurent Pinchart wrote:\n>>> Hi Milan and Andrey,\n>>>\n>>> Thank you for the patch.\n>>>\n>>> On Tue, Mar 19, 2024 at 01:35:56PM +0100, Milan Zamazal wrote:\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>>> This should be addressed, please record this as a \\todo item somewhere.\n>> Ack\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>>>>   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    | 326 ++++++++++++++++++++++++++++++\n>>>>   8 files changed, 394 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>>>\n>>> Why is this needed ?\n>>>\n>>>>                            @TOP_BUILDDIR@/src/libcamera/proxy/\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>>>> 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>>> Ah that's why.\n>> Yes, because, well... all the other IPAs were doing that...\n>> \n>>> It doesn't have to be done before merging, but could you\n>>> address this sooner than later ?\n>> I can't promise a lot, but I'll look into that.\n>> \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>>>\n>>> The IPA and soft ISP run in separate threads. How do you guarantee that\n>>> the stats and params are not accessed concurrently by both ?\n>> I tried avoiding using a pool of buffers for stats and params, and used just a\n>> single chunk of shared memory to pass the stats from soft ISP (its worker thread)\n>> to IPA, and the other chunk for params.\n>> The soft ISP accumulates the stats in its member variable, and after all the stats\n>> for the frame are calculated, the member variable is copyed to the shared memory and\n>> statReady signal is emitted (so using the member var implements some kind of double\n>> buffering). This way the write to the stats shared memory happens once per frame and\n>> is short in time right before emitting the statReady signal.\n>> The callback executed on receiving statReady signal invokes the IPA's processStats\n>> function. So the IPA's processStats() is called soon after the statReady is emitted.\n>> If the IPA finish processing the stats before the next frame is processed by the soft IPS\n>> and the stats for the next frame are written into the shared memory, there is no concurrent\n>> acess.\n>> This doesn't strictly guarantee that the stats are not accessed concurrently by the IPA and\n>> soft ISP. But unless the video stream from the camera sensor overloads the system this\n>> scheme works OK. And if the load is high the concurrent access isn't impossible, and can harm\n>> the image quality, but doesn't lead to critical problems like invalid pointers as the stats\n>> structure only contains numbers and an array of fixed size.\n>> \n>>> Shouldn't\n>>> you have a pool of buffers for each, mimicking what is done with\n>>> hardware ISPs ?\n>> This would definitely work, but is a more heavy wait implementation.\n>> On the other side, the current implementation is less universal (isn't very scalable), more fragile,\n>> and can't reliably associate the stats and the params with the given frame from the camera sensor.\n>> So let me try to implement what you suggested (\"a pool of buffers for each, mimicking what is done with\n>> hardware ISPs\").\n>\n> I won't be able to do this particular part by tomorrow...\n> So a branch I plan to publish tomorrow - with this patch updated to address the review comments\n> and the [PATCH v6 18/18] \"libcamera: Soft IPA: use CameraSensorHelper for analogue gain\" merged\n> into the earlier ones - would not use the pool of buffers for the stats and the params (yet).\n\nHi Andrei,\n\nI already made those easier parts, see\nhttps://gitlab.freedesktop.org/mzamazal/libcamera-softisp/-/commits/SoftwareISP-v10?ref_type=heads\n(not ready for review yet).\n\nSorry for misunderstanding and duplicate work.\n\nRegards,\nMilan\n\n> Thanks,\n> Andrei\n>\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>>> Drop the dummy value.\n>> libcamera docs do allow signals with zero parameters.\n>> But when I tried having zero parameters for an EventInterface function,\n>> it didn't work for me iirc.\n>> Let me double check.\n>> \n>>>> +};\n>>>> diff --git a/meson_options.txt b/meson_options.txt\n>>>> index 99dab96d..2644bef0 100644\n>>>> --- a/meson_options.txt\n>>>> +++ b/meson_options.txt\n>>>> @@ -27,7 +27,7 @@ option('gstreamer',\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>>>>   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>>>\n>>> We use YAML files for all IPAs, could you do the same here ? It\n>>> shouldn't be much extra work as the file is empty :-)\n>> OK\n>> \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..312df4ba\n>>>> --- /dev/null\n>>>> +++ b/src/ipa/simple/soft_simple.cpp\n>>>> @@ -0,0 +1,326 @@\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>>>\n>>> Not needed.\n>> OK\n>> \n>>>> +#include \"libcamera/internal/software_isp/debayer_params.h\"\n>>>> +#include \"libcamera/internal/software_isp/swisp_stats.h\"\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>>>> +        : params_(static_cast(MAP_FAILED)),\n>>>\n>>> Initialize this to nullptr, that's more standard, and will make the\n>>> tests in the destructor nicer. init() will have to set the pointers to\n>>> null if mmap() calls fail.\n>> OK\n>> \n>>>> +          stats_(static_cast(MAP_FAILED)), ignore_updates_(0)\n>>>\n>>> s/ignore_updates_/ignoreUpdates_/\n>> OK\n>> \n>>>> +    {\n>>>> +    }\n>>>> +\n>>>> +    ~IPASoftSimple()\n>>>> +    {\n>>>> +        if (stats_ != MAP_FAILED)\n>>>> +            munmap(stats_, sizeof(SwIspStats));\n>>>> +        if (params_ != MAP_FAILED)\n>>>> +            munmap(params_, sizeof(DebayerParams));\n>>>> +    }\n>>>\n>>> No need to inline this.\n>> OK\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>>>> +    int32_t exposure_min_, exposure_max_;\n>>>> +    int32_t again_min_, again_max_;\n>>>> +    int32_t again_, exposure_;\n>>>> +    unsigned int ignore_updates_;\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_ = fdStats;\n>>>\n>>> As you never use fdStats_ and fdParams_ after this function returns,\n>>> there's no need to store a copy.\n>> Good catch. OK\n>> \n>>>> +    if (!fdStats_.isValid()) {\n>>>> +        LOG(IPASoft, Error) << \"Invalid Statistics handle\";\n>>>> +        return -ENODEV;\n>>>> +    }\n>>>> +\n>>>> +    fdParams_ = 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>>>> +    if (params_ == MAP_FAILED) {\n>>>> +        LOG(IPASoft, Error) << \"Unable to map Parameters\";\n>>>> +        return -errno;\n>>>> +    }\n>>>\n>>>     void *mem = mmap(nullptr, sizeof(DebayerParams), PROT_WRITE, MAP_SHARED,\n>>>              fdParams_.get(), 0));\n>>>     if (mem == MAP_FAILED) {\n>>>         LOG(IPASoft, Error) << \"Unable to map Parameters\";\n>>>         return -errno;\n>>>     }\n>>>\n>>>     params_ = static_cast(mem);\n>> OK\n>> \n>>>> +\n>>>> +    stats_ = static_cast(mmap(nullptr, sizeof(SwIspStats),\n>>>> +                        PROT_READ, MAP_SHARED,\n>>>> +                        fdStats_.get(), 0));\n>>>> +    if (stats_ == MAP_FAILED) {\n>>>> +        LOG(IPASoft, Error) << \"Unable to map Statistics\";\n>>>> +        return -errno;\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>>>> +    return 0;\n>>>> +}\n>>>> +\n>>>> +int IPASoftSimple::configure(const ControlInfoMap &sensorInfoMap)\n>>>> +{\n>>>> +    const ControlInfo &exposure_info = sensorInfoMap.find(V4L2_CID_EXPOSURE)->second;\n>>>\n>>> s/exposure_info/exposureInfo/\n>>>\n>>> I'll let you address the other snake_case to camelCase conversions in\n>>> the series :-)\n>> OK\n>> \n>>>> +    const ControlInfo &gain_info = sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN)->second;\n>>>> +\n>>>> +    exposure_min_ = exposure_info.min().get();\n>>>> +    exposure_max_ = exposure_info.max().get();\n>>>> +    if (!exposure_min_) {\n>>>> +        LOG(IPASoft, Warning) << \"Minimum exposure is zero, that can't be linear\";\n>>>> +        exposure_min_ = 1;\n>>>> +    }\n>>>> +\n>>>> +    again_min_ = gain_info.min().get();\n>>>> +    again_max_ = gain_info.max().get();\n>>>\n>>> Add a blank line.\n>> OK\n>> \n>>>> +    /*\n>>>> +     * The camera sensor gain (g) is usually not equal to the value written\n>>>> +     * into the gain register (x). But the way how the AGC algorithm changes\n>>>> +     * the gain value to make the total exposure closer to the optimum assumes\n>>>> +     * that g(x) is not too far from linear function. If the minimal gain is 0,\n>>>> +     * the g(x) is likely to be far from the linear, like g(x) = a / (b * x + c).\n>>>> +     * To avoid unexpected changes to the gain by the AGC algorithm (abrupt near\n>>>> +     * one edge, and very small near the other) we limit the range of the gain\n>>>> +     * values used.\n>>>> +     */\n>>>> +    if (!again_min_) {\n>>>> +        LOG(IPASoft, Warning) << \"Minimum gain is zero, that can't be linear\";\n>>>> +        again_min_ = std::min(100, again_min_ / 2 + again_max_ / 2);\n>>>> +    }\n>>>\n>>> A patch further in the series addresses this by using\n>>> CameraSensorHelper. It should be squashed with this patch.\n>> OK\n>> \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>>>> +/*\n>>>> + * The number of bins to use for the optimal exposure calculations.\n>>>> + */\n>>>> +static constexpr unsigned int kExposureBinsCount = 5;\n>>>\n>>> Missing blank line. Same below.\n>> OK\n>> \n>>>> +/*\n>>>> + * The exposure is optimal when the mean sample value of the histogram is\n>>>> + * in the middle of the range.\n>>>> + */\n>>>> +static constexpr float kExposureOptimal = kExposureBinsCount / 2.0;\n>>>> +/*\n>>>> + * The below value implements the hysteresis for the exposure adjustment.\n>>>> + * It is small enough to have the exposure close to the optimal, and is big\n>>>> + * enough to prevent the exposure from wobbling around the optimal value.\n>>>> + */\n>>>> +static constexpr float kExposureSatisfactory = 0.2;\n>>>\n>>> Move these to the beginning of the file, just before the IPASoftSimple\n>>> definition.\n>> OK\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>>> Do you envision switching to the libipa/algorithm.h API at some point ?\n>>> If so, it would be nice to record it somewhere.\n>> At some point, yes.\n>> Will mention that.\n>> \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>>>> +    if (ignore_updates_ > 0) {\n>>>> +        --ignore_updates_;\n>>>> +        return;\n>>>> +    }\n>>>\n>>> Also something that could be improved and that should be recorded\n>>> somewhere.\n>> OK\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 yHistValsPerBin =\n>>>> +        SwIspStats::kYHistogramSize / kExposureBinsCount;\n>>>> +    constexpr unsigned int yHistValsPerBinMod =\n>>>> +        SwIspStats::kYHistogramSize /\n>>>> +        (SwIspStats::kYHistogramSize % kExposureBinsCount + 1);\n>>>> +    int ExposureBins[kExposureBinsCount] = {};\n>>>\n>>> s/ExposureBins/exposureBins/\n>> OK\n>> \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 < kExposureBinsCount; 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>>> C++ casts.\n>> OK\n>> \n>>>> +\n>>>> +    /* sanity check */\n>>>\n>>> s/sanity/Sanity/\n>> OK\n>> \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>>> You shouldn't copy the control list, but instead create one from the\n>>> ControlInfoMap that you get in init() (and that you then need to stored\n>>> in the IPA class).\n>> OK, thanks\n>> \n>>>> +\n>>>> +    exposure_ = ctrls.get(V4L2_CID_EXPOSURE).get();\n>>>> +    again_ = ctrls.get(V4L2_CID_ANALOGUE_GAIN).get();\n>>>\n>>>     exposure_ = sensorControls.get(V4L2_CID_EXPOSURE).get();\n>>>     again_ = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n>> OK\n>> \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>>>> +void IPASoftSimple::updateExposure(double exposureMSV)\n>>>> +{\n>>>> +    /* DENOMINATOR of 10 gives ~10% increment/decrement; DENOMINATOR of 5 - about ~20% */\n>>>\n>>> Line wrap, and the DENOMINATOR needs to be renamed to kExpDenominator.\n>> OK\n>> Thanks for the review,\n>> Andrei\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>>>> +    int next;\n>>>> +\n>>>> +    if (exposureMSV < kExposureOptimal - kExposureSatisfactory) {\n>>>> +        next = exposure_ * kExpNumeratorUp / kExpDenominator;\n>>>> +        if (next - exposure_ < 1)\n>>>> +            exposure_ += 1;\n>>>> +        else\n>>>> +            exposure_ = next;\n>>>> +        if (exposure_ >= exposure_max_) {\n>>>> +            next = again_ * kExpNumeratorUp / kExpDenominator;\n>>>> +            if (next - again_ < 1)\n>>>> +                again_ += 1;\n>>>> +            else\n>>>> +                again_ = next;\n>>>> +        }\n>>>> +    }\n>>>> +\n>>>> +    if (exposureMSV > kExposureOptimal + kExposureSatisfactory) {\n>>>> +        if (exposure_ == exposure_max_ && again_ != again_min_) {\n>>>> +            next = again_ * kExpNumeratorDown / kExpDenominator;\n>>>> +            if (again_ - next < 1)\n>>>> +                again_ -= 1;\n>>>> +            else\n>>>> +                again_ = next;\n>>>> +        } else {\n>>>> +            next = exposure_ * kExpNumeratorDown / kExpDenominator;\n>>>> +            if (exposure_ - next < 1)\n>>>> +                exposure_ -= 1;\n>>>> +            else\n>>>> +                exposure_ = next;\n>>>> +        }\n>>>> +    }\n>>>> +\n>>>> +    exposure_ = std::clamp(exposure_, exposure_min_, exposure_max_);\n>>>> +    again_ = std::clamp(again_, again_min_, again_max_);\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>>>","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 60DA0C0DA4\n\tfor ;\n\tWed, 3 Apr 2024 08:53:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 54F7763370;\n\tWed, 3 Apr 2024 10:53:56 +0200 (CEST)","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 EFC1E61C39\n\tfor ;\n\tWed, 3 Apr 2024 10:53:53 +0200 (CEST)","from mail-ej1-f69.google.com (mail-ej1-f69.google.com\n\t[209.85.218.69]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-159-0YA4KQG_PJaCHWzwuwKcAA-1; Wed, 03 Apr 2024 04:53:49 -0400","by mail-ej1-f69.google.com with SMTP id\n\ta640c23a62f3a-a4e9b682fadso51565466b.2\n\tfor ;\n\tWed, 03 Apr 2024 01:53:48 -0700 (PDT)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\tpw14-20020a17090720ae00b00a4e35cc42c7sm6364891ejb.170.2024.04.03.01.53.45\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 03 Apr 2024 01:53:46 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"Ff87A9zi\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1712134432;\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\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=eacSabR+9IFH+V3TpieevStclN6Ej4suC4JVBnXQ8pc=;\n\tb=Ff87A9ziUfWbXqoejrBQhMhm2RMpNR4w4XZICW9W+fJS93y4TTLLilUwTkVr+0hksrd+wQ\n\t+Nc+bqZXX2jjlZQcISkMCJkNrWSmI4Gq9DJzWcxbQT7W1c+AVSWr1sAYXsUNp0Khw4ClDc\n\t7/R3OJQFur90KKqQ7xm5+pLNyo6Zlms=","X-MC-Unique":"0YA4KQG_PJaCHWzwuwKcAA-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1712134428; x=1712739228;\n\th=content-transfer-encoding:mime-version:user-agent:message-id:date\n\t:references:in-reply-to:subject:cc:to:from:x-gm-message-state:from\n\t:to:cc:subject:date:message-id:reply-to;\n\tbh=XT7EmLE+UiAqpUBfl5S6BsHtgP4eRIMzvFrfjB6QjJk=;\n\tb=Khp13k2vtiuFZqruB0WrbphnutYZU9bWN+tXKgp3wkGhvN7SQPYOI8bGFtMakduJ3v\n\tmxlcODzNs7NqZRL99GZXlbp+l+kl9xMx/vT05tbGrPd6eo8vU5eDM8+eJDiNmHQOb0W/\n\tHCXM7qiSRzvaUX6qzZgGze/oZW2jXQPGNRJ23V3aHpK//3OLVFTjGrrm6a3BysKOrtHn\n\tTjfiOinPbRUZiRlm4+FjBNfG2u4UtfzIi1WpiEm+6hreL5iDftOgdh0Laz+DvR2DMiIx\n\t/TB++ZVWVqcHQIdichQUsPiXCqAsWFIFbJO8pUw5I5BnabeNBHuzvy3tkijyw/KcS3mN\n\tTMyQ==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCXJvLlFSFyh6yhfj3TtUbnmIB7OpQ/hzHUnQ0X6WVAmDfj9XV2x6MLOS9UFEUDhkIZLmVC7eQdS4WK2PTD/55QXpcWZWaWjTQDLr+8KzjbyMI8Iuw==","X-Gm-Message-State":"AOJu0Yyqay4GGXeUrsK5TEzPiXkRnetnWjS4LRaEwW33KSSle/23+fWv\n\tFec0Zs6PvmYbDnPXFoWeAjbDs8Fh96UlkLfYATqzRPfqBJPIKTjHBaF2oWlSkKkEXEhAtoppWh3\n\tbAugaxtVf1msS0k6PokT68F7u18IEi1Rd/ocLF/9VenDxbZeQT2nneRLJk+NhhL+1c4eEWxw=","X-Received":["by 2002:a17:906:b78e:b0:a4e:8f62:487d with SMTP id\n\tdt14-20020a170906b78e00b00a4e8f62487dmr1212875ejb.28.1712134427604; \n\tWed, 03 Apr 2024 01:53:47 -0700 (PDT)","by 2002:a17:906:b78e:b0:a4e:8f62:487d with SMTP id\n\tdt14-20020a170906b78e00b00a4e8f62487dmr1212857ejb.28.1712134427095; \n\tWed, 03 Apr 2024 01:53:47 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IFcT0BLyZvI6S+gNkk/223DlqoRt3WYu6wzUQvoYo07U7v3Re6CMR0btOIg853pDxxNMfsh6A==","From":"Milan Zamazal ","To":"Andrei Konovalov ","Cc":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org, Bryan O'Donoghue\n\t, Maxime Ripard , Pavel\n\tMachek , Hans de Goede , Kieran\n\tBingham , Dennis Bonke\n\t, Marttico , Toon Langendam\n\t","Subject":"Re: [PATCH v6 09/18] libcamera: ipa: add Soft IPA","In-Reply-To":" (Andrei\n\tKonovalov's message of \"Tue, 2 Apr 2024 23:34:13 +0300\")","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-10-mzamazal@redhat.com>\n\t<20240327163808.GG4721@pendragon.ideasonboard.com>\n\t<46848c9c-5bea-472e-bcb7-efc24479fb45@gmail.com>\n\t","Date":"Wed, 03 Apr 2024 10:53:45 +0200","Message-ID":"<871q7mg73a.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; 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29149,"web_url":"https://patchwork.libcamera.org/comment/29149/","msgid":"","date":"2024-04-03T19:06:25","subject":"Re: [PATCH v6 09/18] 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 Milan,\n\nOn 03.04.2024 11:53, Milan Zamazal wrote:\n> Andrei Konovalov writes:\n> \n>> Hi Laurent and Milan,\n>>\n>> On 28.03.2024 01:36, Andrei Konovalov wrote:\n>>> Hi Laurent,\n>>> Thank you for the review!\n>>> On 27.03.2024 19:38, Laurent Pinchart wrote:\n>>>> Hi Milan and Andrey,\n>>>>\n>>>> Thank you for the patch.\n>>>>\n>>>> On Tue, Mar 19, 2024 at 01:35:56PM +0100, Milan Zamazal wrote:\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>>>> This should be addressed, please record this as a \\todo item somewhere.\n>>> Ack\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>>>>>   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    | 326 ++++++++++++++++++++++++++++++\n>>>>>   8 files changed, 394 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>>>>\n>>>> Why is this needed ?\n>>>>\n>>>>>                            @TOP_BUILDDIR@/src/libcamera/proxy/\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>>>>> 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>>>> Ah that's why.\n>>> Yes, because, well... all the other IPAs were doing that...\n>>>\n>>>> It doesn't have to be done before merging, but could you\n>>>> address this sooner than later ?\n>>> I can't promise a lot, but I'll look into that.\n>>>\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>>>>\n>>>> The IPA and soft ISP run in separate threads. How do you guarantee that\n>>>> the stats and params are not accessed concurrently by both ?\n>>> I tried avoiding using a pool of buffers for stats and params, and used just a\n>>> single chunk of shared memory to pass the stats from soft ISP (its worker thread)\n>>> to IPA, and the other chunk for params.\n>>> The soft ISP accumulates the stats in its member variable, and after all the stats\n>>> for the frame are calculated, the member variable is copyed to the shared memory and\n>>> statReady signal is emitted (so using the member var implements some kind of double\n>>> buffering). This way the write to the stats shared memory happens once per frame and\n>>> is short in time right before emitting the statReady signal.\n>>> The callback executed on receiving statReady signal invokes the IPA's processStats\n>>> function. So the IPA's processStats() is called soon after the statReady is emitted.\n>>> If the IPA finish processing the stats before the next frame is processed by the soft IPS\n>>> and the stats for the next frame are written into the shared memory, there is no concurrent\n>>> acess.\n>>> This doesn't strictly guarantee that the stats are not accessed concurrently by the IPA and\n>>> soft ISP. But unless the video stream from the camera sensor overloads the system this\n>>> scheme works OK. And if the load is high the concurrent access isn't impossible, and can harm\n>>> the image quality, but doesn't lead to critical problems like invalid pointers as the stats\n>>> structure only contains numbers and an array of fixed size.\n>>>\n>>>> Shouldn't\n>>>> you have a pool of buffers for each, mimicking what is done with\n>>>> hardware ISPs ?\n>>> This would definitely work, but is a more heavy wait implementation.\n>>> On the other side, the current implementation is less universal (isn't very scalable), more fragile,\n>>> and can't reliably associate the stats and the params with the given frame from the camera sensor.\n>>> So let me try to implement what you suggested (\"a pool of buffers for each, mimicking what is done with\n>>> hardware ISPs\").\n>>\n>> I won't be able to do this particular part by tomorrow...\n>> So a branch I plan to publish tomorrow - with this patch updated to address the review comments\n>> and the [PATCH v6 18/18] \"libcamera: Soft IPA: use CameraSensorHelper for analogue gain\" merged\n>> into the earlier ones - would not use the pool of buffers for the stats and the params (yet).\n> \n> Hi Andrei,\n> \n> I already made those easier parts, see\n> https://gitlab.freedesktop.org/mzamazal/libcamera-softisp/-/commits/SoftwareISP-v10?ref_type=heads\n> (not ready for review yet).\n> \n> Sorry for misunderstanding and duplicate work.\n\nActually that's fine, and you bwork on this patch is not lost.\nI've reused some of your changes in the branch I've pushed:\nhttps://gitlab.freedesktop.org/camera/libcamera-softisp/-/commits/SoftwareISP-v10-candidate-ynk1/?ref_type=heads\n\nIt is based on\nhttps://gitlab.freedesktop.org/mzamazal/libcamera-softisp/-/commits/SoftwareISP-v10?ref_type=heads\nI've only updated the commits:\n \"libcamera: ipa: add Soft IPA\"\n \"libcamera: introduce SoftwareIsp\"\nand also rebased\n \"libcamera: software_isp: Apply black level compensation\"\nas it depends on the changes to the other two.\n\nThe updated patchset still doesn't use the IPA configuration file, but it parses it and prints\nthe version read from the config file if debug messages are enabled for the IPASoft category.\nIf one runs libcamera without installing it, LIBCAMERA_IPA_CONFIG_PATH environment variable should be\nset properly (LIBCAMERA_IPA_CONFIG_PATH=${the_install_dir}/share/libcamera/ipa).\nI've figured out why I had problems with libcamera failing to find the IPA config file. Now this part works OK,\nno hacks were needed.\n\nAs I wrote earlier, this version still doesn't have the pools of buffers for the params and the stats.\nThis is in my todo list, but I don't have the date for this yet.\n\nThanks,\nAndrei\n\n> Regards,\n> Milan\n> \n>> Thanks,\n>> Andrei\n>>\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>>>> Drop the dummy value.\n>>> libcamera docs do allow signals with zero parameters.\n>>> But when I tried having zero parameters for an EventInterface function,\n>>> it didn't work for me iirc.\n>>> Let me double check.\n>>>\n>>>>> +};\n>>>>> diff --git a/meson_options.txt b/meson_options.txt\n>>>>> index 99dab96d..2644bef0 100644\n>>>>> --- a/meson_options.txt\n>>>>> +++ b/meson_options.txt\n>>>>> @@ -27,7 +27,7 @@ option('gstreamer',\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>>>>>   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>>>>\n>>>> We use YAML files for all IPAs, could you do the same here ? It\n>>>> shouldn't be much extra work as the file is empty :-)\n>>> OK\n>>>\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..312df4ba\n>>>>> --- /dev/null\n>>>>> +++ b/src/ipa/simple/soft_simple.cpp\n>>>>> @@ -0,0 +1,326 @@\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>>>>\n>>>> Not needed.\n>>> OK\n>>>\n>>>>> +#include \"libcamera/internal/software_isp/debayer_params.h\"\n>>>>> +#include \"libcamera/internal/software_isp/swisp_stats.h\"\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>>>>> +        : params_(static_cast(MAP_FAILED)),\n>>>>\n>>>> Initialize this to nullptr, that's more standard, and will make the\n>>>> tests in the destructor nicer. init() will have to set the pointers to\n>>>> null if mmap() calls fail.\n>>> OK\n>>>\n>>>>> +          stats_(static_cast(MAP_FAILED)), ignore_updates_(0)\n>>>>\n>>>> s/ignore_updates_/ignoreUpdates_/\n>>> OK\n>>>\n>>>>> +    {\n>>>>> +    }\n>>>>> +\n>>>>> +    ~IPASoftSimple()\n>>>>> +    {\n>>>>> +        if (stats_ != MAP_FAILED)\n>>>>> +            munmap(stats_, sizeof(SwIspStats));\n>>>>> +        if (params_ != MAP_FAILED)\n>>>>> +            munmap(params_, sizeof(DebayerParams));\n>>>>> +    }\n>>>>\n>>>> No need to inline this.\n>>> OK\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>>>>> +    int32_t exposure_min_, exposure_max_;\n>>>>> +    int32_t again_min_, again_max_;\n>>>>> +    int32_t again_, exposure_;\n>>>>> +    unsigned int ignore_updates_;\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_ = fdStats;\n>>>>\n>>>> As you never use fdStats_ and fdParams_ after this function returns,\n>>>> there's no need to store a copy.\n>>> Good catch. OK\n>>>\n>>>>> +    if (!fdStats_.isValid()) {\n>>>>> +        LOG(IPASoft, Error) << \"Invalid Statistics handle\";\n>>>>> +        return -ENODEV;\n>>>>> +    }\n>>>>> +\n>>>>> +    fdParams_ = 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>>>>> +    if (params_ == MAP_FAILED) {\n>>>>> +        LOG(IPASoft, Error) << \"Unable to map Parameters\";\n>>>>> +        return -errno;\n>>>>> +    }\n>>>>\n>>>>     void *mem = mmap(nullptr, sizeof(DebayerParams), PROT_WRITE, MAP_SHARED,\n>>>>              fdParams_.get(), 0));\n>>>>     if (mem == MAP_FAILED) {\n>>>>         LOG(IPASoft, Error) << \"Unable to map Parameters\";\n>>>>         return -errno;\n>>>>     }\n>>>>\n>>>>     params_ = static_cast(mem);\n>>> OK\n>>>\n>>>>> +\n>>>>> +    stats_ = static_cast(mmap(nullptr, sizeof(SwIspStats),\n>>>>> +                        PROT_READ, MAP_SHARED,\n>>>>> +                        fdStats_.get(), 0));\n>>>>> +    if (stats_ == MAP_FAILED) {\n>>>>> +        LOG(IPASoft, Error) << \"Unable to map Statistics\";\n>>>>> +        return -errno;\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>>>>> +    return 0;\n>>>>> +}\n>>>>> +\n>>>>> +int IPASoftSimple::configure(const ControlInfoMap &sensorInfoMap)\n>>>>> +{\n>>>>> +    const ControlInfo &exposure_info = sensorInfoMap.find(V4L2_CID_EXPOSURE)->second;\n>>>>\n>>>> s/exposure_info/exposureInfo/\n>>>>\n>>>> I'll let you address the other snake_case to camelCase conversions in\n>>>> the series :-)\n>>> OK\n>>>\n>>>>> +    const ControlInfo &gain_info = sensorInfoMap.find(V4L2_CID_ANALOGUE_GAIN)->second;\n>>>>> +\n>>>>> +    exposure_min_ = exposure_info.min().get();\n>>>>> +    exposure_max_ = exposure_info.max().get();\n>>>>> +    if (!exposure_min_) {\n>>>>> +        LOG(IPASoft, Warning) << \"Minimum exposure is zero, that can't be linear\";\n>>>>> +        exposure_min_ = 1;\n>>>>> +    }\n>>>>> +\n>>>>> +    again_min_ = gain_info.min().get();\n>>>>> +    again_max_ = gain_info.max().get();\n>>>>\n>>>> Add a blank line.\n>>> OK\n>>>\n>>>>> +    /*\n>>>>> +     * The camera sensor gain (g) is usually not equal to the value written\n>>>>> +     * into the gain register (x). But the way how the AGC algorithm changes\n>>>>> +     * the gain value to make the total exposure closer to the optimum assumes\n>>>>> +     * that g(x) is not too far from linear function. If the minimal gain is 0,\n>>>>> +     * the g(x) is likely to be far from the linear, like g(x) = a / (b * x + c).\n>>>>> +     * To avoid unexpected changes to the gain by the AGC algorithm (abrupt near\n>>>>> +     * one edge, and very small near the other) we limit the range of the gain\n>>>>> +     * values used.\n>>>>> +     */\n>>>>> +    if (!again_min_) {\n>>>>> +        LOG(IPASoft, Warning) << \"Minimum gain is zero, that can't be linear\";\n>>>>> +        again_min_ = std::min(100, again_min_ / 2 + again_max_ / 2);\n>>>>> +    }\n>>>>\n>>>> A patch further in the series addresses this by using\n>>>> CameraSensorHelper. It should be squashed with this patch.\n>>> OK\n>>>\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>>>>> +/*\n>>>>> + * The number of bins to use for the optimal exposure calculations.\n>>>>> + */\n>>>>> +static constexpr unsigned int kExposureBinsCount = 5;\n>>>>\n>>>> Missing blank line. Same below.\n>>> OK\n>>>\n>>>>> +/*\n>>>>> + * The exposure is optimal when the mean sample value of the histogram is\n>>>>> + * in the middle of the range.\n>>>>> + */\n>>>>> +static constexpr float kExposureOptimal = kExposureBinsCount / 2.0;\n>>>>> +/*\n>>>>> + * The below value implements the hysteresis for the exposure adjustment.\n>>>>> + * It is small enough to have the exposure close to the optimal, and is big\n>>>>> + * enough to prevent the exposure from wobbling around the optimal value.\n>>>>> + */\n>>>>> +static constexpr float kExposureSatisfactory = 0.2;\n>>>>\n>>>> Move these to the beginning of the file, just before the IPASoftSimple\n>>>> definition.\n>>> OK\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>>>> Do you envision switching to the libipa/algorithm.h API at some point ?\n>>>> If so, it would be nice to record it somewhere.\n>>> At some point, yes.\n>>> Will mention that.\n>>>\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>>>>> +    if (ignore_updates_ > 0) {\n>>>>> +        --ignore_updates_;\n>>>>> +        return;\n>>>>> +    }\n>>>>\n>>>> Also something that could be improved and that should be recorded\n>>>> somewhere.\n>>> OK\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 yHistValsPerBin =\n>>>>> +        SwIspStats::kYHistogramSize / kExposureBinsCount;\n>>>>> +    constexpr unsigned int yHistValsPerBinMod =\n>>>>> +        SwIspStats::kYHistogramSize /\n>>>>> +        (SwIspStats::kYHistogramSize % kExposureBinsCount + 1);\n>>>>> +    int ExposureBins[kExposureBinsCount] = {};\n>>>>\n>>>> s/ExposureBins/exposureBins/\n>>> OK\n>>>\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 < kExposureBinsCount; 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>>>> C++ casts.\n>>> OK\n>>>\n>>>>> +\n>>>>> +    /* sanity check */\n>>>>\n>>>> s/sanity/Sanity/\n>>> OK\n>>>\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>>>> You shouldn't copy the control list, but instead create one from the\n>>>> ControlInfoMap that you get in init() (and that you then need to stored\n>>>> in the IPA class).\n>>> OK, thanks\n>>>\n>>>>> +\n>>>>> +    exposure_ = ctrls.get(V4L2_CID_EXPOSURE).get();\n>>>>> +    again_ = ctrls.get(V4L2_CID_ANALOGUE_GAIN).get();\n>>>>\n>>>>     exposure_ = sensorControls.get(V4L2_CID_EXPOSURE).get();\n>>>>     again_ = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n>>> OK\n>>>\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>>>>> +void IPASoftSimple::updateExposure(double exposureMSV)\n>>>>> +{\n>>>>> +    /* DENOMINATOR of 10 gives ~10% increment/decrement; DENOMINATOR of 5 - about ~20% */\n>>>>\n>>>> Line wrap, and the DENOMINATOR needs to be renamed to kExpDenominator.\n>>> OK\n>>> Thanks for the review,\n>>> Andrei\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>>>>> +    int next;\n>>>>> +\n>>>>> +    if (exposureMSV < kExposureOptimal - kExposureSatisfactory) {\n>>>>> +        next = exposure_ * kExpNumeratorUp / kExpDenominator;\n>>>>> +        if (next - exposure_ < 1)\n>>>>> +            exposure_ += 1;\n>>>>> +        else\n>>>>> +            exposure_ = next;\n>>>>> +        if (exposure_ >= exposure_max_) {\n>>>>> +            next = again_ * kExpNumeratorUp / kExpDenominator;\n>>>>> +            if (next - again_ < 1)\n>>>>> +                again_ += 1;\n>>>>> +            else\n>>>>> +                again_ = next;\n>>>>> +        }\n>>>>> +    }\n>>>>> +\n>>>>> +    if (exposureMSV > kExposureOptimal + kExposureSatisfactory) {\n>>>>> +        if (exposure_ == exposure_max_ && again_ != again_min_) {\n>>>>> +            next = again_ * kExpNumeratorDown / kExpDenominator;\n>>>>> +            if (again_ - next < 1)\n>>>>> +                again_ -= 1;\n>>>>> +            else\n>>>>> +                again_ = next;\n>>>>> +        } else {\n>>>>> +            next = exposure_ * kExpNumeratorDown / kExpDenominator;\n>>>>> +            if (exposure_ - next < 1)\n>>>>> +                exposure_ -= 1;\n>>>>> +            else\n>>>>> +                exposure_ = next;\n>>>>> +        }\n>>>>> +    }\n>>>>> +\n>>>>> +    exposure_ = std::clamp(exposure_, exposure_min_, exposure_max_);\n>>>>> +    again_ = std::clamp(again_, again_min_, again_max_);\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>","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 EB8D5C0DA4\n\tfor ;\n\tWed, 3 Apr 2024 19:06:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DF2B763375;\n\tWed, 3 Apr 2024 21:06:29 +0200 (CEST)","from mail-ej1-x636.google.com (mail-ej1-x636.google.com\n\t[IPv6:2a00:1450:4864:20::636])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 761C762CA0\n\tfor ;\n\tWed, 3 Apr 2024 21:06:28 +0200 (CEST)","by mail-ej1-x636.google.com with SMTP id\n\ta640c23a62f3a-a466fc8fcccso27540166b.1\n\tfor ;\n\tWed, 03 Apr 2024 12:06:28 -0700 (PDT)","from [192.168.118.26] ([87.116.166.149])\n\tby smtp.gmail.com with ESMTPSA id\n\td6-20020a1709063ec600b00a474c3c2f9dsm8001230ejj.38.2024.04.03.12.06.26\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tWed, 03 Apr 2024 12:06:27 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"N8a4z1LQ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=gmail.com; s=20230601; t=1712171188; x=1712775988;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:in-reply-to:from:content-language\n\t:references:cc:to:subject:user-agent:mime-version:date:message-id\n\t:from:to:cc:subject:date:message-id:reply-to;\n\tbh=Zk8oOWkVC4INwkHZwxxmv9vhUHyWq/5HxHpIDzuubgE=;\n\tb=N8a4z1LQd/L4+XPN0Hsg+xM1YElJbDPylhcNYp2gdKSpOfsPqlsxER7I6f1GxzCg44\n\tV1UHONfp6wvUlSCx0FOXWXvu09T672fSKk6jwtTxOfuahhep4rZHLSfft/5WBsN6nUws\n\tp9rTCRwsLCT+fmK3WajyR+TjmSbhxqDP0q8K3AUuAHofaJ0BjwqicAvW3CUQoWnO7K9s\n\ti0LAXEqZascVAicG9wkOMDwlfgoFrUY89dEZ8ghJ3x2olMLfPKsXo+B/J4oMe5xACH5F\n\tl4AApPXh0c6XoIRyjMVBOQyrF5CVot8bTy6UJb/v53ANZ74JtKvR3P7WUJgcJ7TzM75h\n\tGQ/Q==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1712171188; x=1712775988;\n\th=content-transfer-encoding:in-reply-to:from:content-language\n\t:references:cc:to: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=Zk8oOWkVC4INwkHZwxxmv9vhUHyWq/5HxHpIDzuubgE=;\n\tb=G0uuRLUWtAKH+MmyX+EZnVbYmsOGyb2T0lFM5gu1XaFjdkaBKHd8F2C4n+7Bm7NzKP\n\tJgaE2nWE7+qiBVMrMCB7Tj+YqQJoMCMYmVgP+EuszXiI0GT4JT91dn6wzud11a6vND8A\n\tRVSqn6OQhkBrUqhRTACa6I+iR0PIlWONMPMsXaM5MoL3pKT4rIilNVzQpVpX5EWteYVv\n\teTFKxcYSNTV8VRkkppxUWh0Jtv98csatS0/Z/wckLc5tSfgQ+7LuUvYWFVyArNjMmupc\n\tkElHCNruobPmB0+klih4aU27/sI39wUQyxAmYNjXeZdOS0ZFVLm5TeJDTMVZ0BelONm2\n\tVoqQ==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCVfpVDTZ0hbQGsVjSgg24hvAvudkvCnkTQ8cYrRrdaK0kBOqni1rftw78iD1lP9gh1uG6bt/zgIS18g8N9hKRHJ+cskegaxqf2LHEPLsTA8nwaULg==","X-Gm-Message-State":"AOJu0Yx/5ob1f76+WrGewxcSK4eUTMcsWOMrw4asAwBgpYZ16Et8Cy2A\n\tf8nbyzwxHmnNc9s55mRi0pwXd1d/TqumiReTv4BxvUWQQHOjJyKz","X-Google-Smtp-Source":"AGHT+IGf/qU1VMkSSstaZ9hzkkHGCS3MN7J1W/bQtvSFuTZPwDquangFoCwaGS5oG9GbQLMqCkKU0g==","X-Received":"by 2002:a17:907:7dab:b0:a46:636d:ef23 with SMTP id\n\toz43-20020a1709077dab00b00a46636def23mr165619ejc.54.1712171187441; \n\tWed, 03 Apr 2024 12:06:27 -0700 (PDT)","Message-ID":"","Date":"Wed, 3 Apr 2024 22:06:25 +0300","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v6 09/18] libcamera: ipa: add Soft IPA","To":"Milan Zamazal ","Cc":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org,\n\tBryan O'Donoghue ,\n\tMaxime Ripard , Pavel Machek ,\n\tHans de Goede ,\n\tKieran Bingham ,\n\tDennis Bonke , Marttico , \n\tToon Langendam ","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-10-mzamazal@redhat.com>\n\t<20240327163808.GG4721@pendragon.ideasonboard.com>\n\t<46848c9c-5bea-472e-bcb7-efc24479fb45@gmail.com>\n\t\n\t<871q7mg73a.fsf@redhat.com>","Content-Language":"en-US","From":"Andrei Konovalov ","In-Reply-To":"<871q7mg73a.fsf@redhat.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29150,"web_url":"https://patchwork.libcamera.org/comment/29150/","msgid":"<87y19t36s3.fsf@redhat.com>","date":"2024-04-04T07:51:08","subject":"Re: [PATCH v6 09/18] libcamera: ipa: add Soft IPA","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Andrei Konovalov writes:\n\n> I've reused some of your changes in the branch I've pushed:\n> https://gitlab.freedesktop.org/camera/libcamera-softisp/-/commits/SoftwareISP-v10-candidate-ynk1/?ref_type=heads\n>\n> It is based on\n> https://gitlab.freedesktop.org/mzamazal/libcamera-softisp/-/commits/SoftwareISP-v10?ref_type=heads\n> I've only updated the commits:\n> \"libcamera: ipa: add Soft IPA\"\n> \"libcamera: introduce SoftwareIsp\"\n> and also rebased\n> \"libcamera: software_isp: Apply black level compensation\"\n> as it depends on the changes to the other two.\n\nHi Andrei,\n\nnice work, thank you! I integrated your changes into my branch (I just fixed\none trivial typo in a comment), which is now available also at\nhttps://gitlab.freedesktop.org/camera/libcamera-softisp/-/tree/SoftwareISP-v10?ref_type=heads\n\nI'll post v7 soon.\n\nRegards,\nMilan\n\n> The updated patchset still doesn't use the IPA configuration file, but it parses it and prints\n> the version read from the config file if debug messages are enabled for the IPASoft category.\n> If one runs libcamera without installing it, LIBCAMERA_IPA_CONFIG_PATH environment variable should be\n> set properly (LIBCAMERA_IPA_CONFIG_PATH=${the_install_dir}/share/libcamera/ipa).\n> I've figured out why I had problems with libcamera failing to find the IPA config file. Now this part works OK,\n> no hacks were needed.\n>\n> As I wrote earlier, this version still doesn't have the pools of buffers for the params and the stats.\n> This is in my todo list, but I don't have the date for this yet.\n>\n> Thanks,\n> Andrei","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 06B86BD16B\n\tfor ;\n\tThu, 4 Apr 2024 07:51:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D0F8961C2B;\n\tThu, 4 Apr 2024 09:51:16 +0200 (CEST)","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 1A64161C2B\n\tfor ;\n\tThu, 4 Apr 2024 09:51:13 +0200 (CEST)","from mail-wr1-f72.google.com (mail-wr1-f72.google.com\n\t[209.85.221.72]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-445-n9pNBdPhPbSLJpEU1-Pkdw-1; Thu, 04 Apr 2024 03:51:11 -0400","by mail-wr1-f72.google.com with SMTP id\n\tffacd0b85a97d-34370ba4105so394739f8f.3\n\tfor ;\n\tThu, 04 Apr 2024 00:51:11 -0700 (PDT)","from nuthatch (nat-pool-brq-t.redhat.com. [213.175.37.10])\n\tby smtp.gmail.com with ESMTPSA id\n\tdw11-20020a0560000dcb00b00341c7129e28sm19281285wrb.91.2024.04.04.00.51.08\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 04 Apr 2024 00:51:09 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"A9/pmYF6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1712217073;\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=laPvvrGL1EuTjGJvAG+fFsK7ctmgVCRSTYCpyPSeJjA=;\n\tb=A9/pmYF6Hybb0hXy40ZPyZrTPGqI9y1v+UaAhuhi9QNxDIBZ4A3F/JLwCJxrhZS3FXPfBv\n\tm04OKASEs71uweynDsl4HoPT+ecj3twmX4AxDAX7C5nOoVtz0+TP9FXGFqJVh5UkKFz2vt\n\tXAzYu+PStEAhQOI9ZGrt6NQBhVt92Js=","X-MC-Unique":"n9pNBdPhPbSLJpEU1-Pkdw-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1712217070; x=1712821870;\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=laPvvrGL1EuTjGJvAG+fFsK7ctmgVCRSTYCpyPSeJjA=;\n\tb=vtd/yh3rThgE86NVM9vVdsB9cFKFL39bii6VLRhUF+nh5zCHPcggbKvmVkcUl500Po\n\t8Nvt+Tv0V/kJn3cb/7ZK1C1pUKPGW37LDT5kx/Fde0ayYYGyqeSeq+B/nPOnW8y7DKmW\n\tS0SJ0RbWir/+Td7btoyHLKjD/ysF0PIHdR8FBFeqTv1vWLWUEQcwXDBAKvYnyqCURNFG\n\tW7aYvI8z5eDADoteNX9/E4cp2jzAcIAKY65xY6OzBYw8Eem63YUg+yCq6/gPeUuoX2AN\n\tpXQmMSciUIW1UmWSWBBGG4cMnMGXWOvmuJuDYYJjaEYe7m8Tp5i5WphXB+Q8SAOtPNyo\n\tBALg==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCWvkwVnwjiE6pkJw4/sRebeeRJ/fLChVyY/xc15hrDWte/IYpYIlxwW59wYZN/97qJ8deERIrA31gc9OzLswuUevAbiGE9WJxSTiHV6RRs8LH5fVg==","X-Gm-Message-State":"AOJu0YxEqpju1KYYoDG4wfsTdwLhEpSRjh/x+5dtNrJOvwJdrouPQyLy\n\tVt8SlHU4aQ/TC/M0fCXA3hnPQKoHQq9V2Uhm6tnRrikco8u88q9Ikp5/rivnTlUlKIsDHRPj7Ky\n\t2Ly3aLatdqPDMd5PFdo8TLN3IDXG20Bg3iB6wed5WSI0skgE4S5+H8+RxmcD7IWHV9r8uTBo=","X-Received":["by 2002:a5d:6950:0:b0:343:6187:ff56 with SMTP id\n\tr16-20020a5d6950000000b003436187ff56mr1319341wrw.56.1712217070344; \n\tThu, 04 Apr 2024 00:51:10 -0700 (PDT)","by 2002:a5d:6950:0:b0:343:6187:ff56 with SMTP id\n\tr16-20020a5d6950000000b003436187ff56mr1319321wrw.56.1712217069833; \n\tThu, 04 Apr 2024 00:51:09 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IERp54OXShW6iNUaBATP/gp0yiGim0uKQWXfDJrUI+s5Uwzl3Fvevc6pjFvFV0pIRCuWr4xTQ==","From":"Milan Zamazal ","To":"Andrei Konovalov ","Cc":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org, Bryan O'Donoghue\n\t, Maxime Ripard , Pavel\n\tMachek , Hans de Goede , Kieran\n\tBingham , Dennis Bonke\n\t, Marttico , Toon Langendam\n\t","Subject":"Re: [PATCH v6 09/18] libcamera: ipa: add Soft IPA","In-Reply-To":" (Andrei\n\tKonovalov's message of \"Wed, 3 Apr 2024 22:06:25 +0300\")","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-10-mzamazal@redhat.com>\n\t<20240327163808.GG4721@pendragon.ideasonboard.com>\n\t<46848c9c-5bea-472e-bcb7-efc24479fb45@gmail.com>\n\t\n\t<871q7mg73a.fsf@redhat.com>\n\t","Date":"Thu, 04 Apr 2024 09:51:08 +0200","Message-ID":"<87y19t36s3.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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]