[{"id":29211,"web_url":"https://patchwork.libcamera.org/comment/29211/","msgid":"<171291923475.2108906.6960245983511821570@ping.linuxembedded.co.uk>","date":"2024-04-12T10:53:54","subject":"Re: [PATCH 1/3] ipa: libipa: Add Lux helper","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Paul,\n\nQuoting Paul Elder (2024-04-12 10:16:58)\n> Add a Lux helper to libipa that does the estimation of the lux level\n> given gain, exposure, aperture, and luminance histogram. The helper also\n> handles reading the reference values from the tuning file. These are\n> expected to be common operations of lux algorithm modules in IPAs, and\n> is modeled/copied from Raspberry Pi.\n> \n> Signed-off-by: Paul Elder \n> ---\n> src/ipa/libipa/lux.cpp | 119 +++++++++++++++++++++++++++++++++++++\n> src/ipa/libipa/lux.h | 45 ++++++++++++++\n> src/ipa/libipa/meson.build | 2 +\n> 3 files changed, 166 insertions(+)\n> create mode 100644 src/ipa/libipa/lux.cpp\n> create mode 100644 src/ipa/libipa/lux.h\n> \n> diff --git a/src/ipa/libipa/lux.cpp b/src/ipa/libipa/lux.cpp\n> new file mode 100644\n> index 00000000..756cd3c4\n> --- /dev/null\n> +++ b/src/ipa/libipa/lux.cpp\n> @@ -0,0 +1,119 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2019, Raspberry Pi Ltd\n> + * Copyright (C) 2024, Paul Elder \n> + *\n> + * lux.h - Helper class that implements lux estimation\n> + */\n> +#include \"lux.h\"\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \"libcamera/internal/yaml_parser.h\"\n> +\n> +#include \"histogram.h\"\n> +\n> +/**\n> + * \\file lux.h\n> + * \\brief Helper class that implements lux estimation\n> + *\n> + * As estimating the lux level of an image is expected to be a common\n> + * operation, it is implemented in a helper in libipa.\n\nI think the brief should explain what it is and how to use it (or in\nthis case what it uses?) - not where it lives.\n\n> + */\n> +\n> +namespace libcamera {\n> +\n> +using namespace std::literals::chrono_literals;\n> +\n> +LOG_DEFINE_CATEGORY(Lux)\n> +\n> +namespace ipa {\n> +\n> +/**\n> + * \\class Lux\n> + * \\brief Class that implements lux estimation\n> + */\n> +\n> +/**\n> + * \\var Lux::referenceExposureTime_\n> + * \\brief The exposure time of the reference image, in microseconds.\n> + */\n> +\n> +/**\n> + * \\var Lux::referenceGain_\n> + * \\brief The analogue gain of the reference image.\n> + */\n> +\n> +/**\n> + * \\var Lux::referenceAperture_\n> + * \\brief The aperture of the reference image in units of 1/f.\n> + */\n> +\n> +/**\n> + * \\var Lux::referenceY_\n> + * \\brief The measured luminance of the reference image, out of 65536.\n\nWhy 65536? Would it be better/more explicit to say that it's a 16 bit\nvalue? Except of course that is BIT(16) so it's a 16-bit-value plus one?\nIs 65536 a permitted value?\n\n> + */\n> +\n> +/**\n> + * \\var Lux::referenceLux_\n> + * \\brief The estimated lux level of the reference image.\n> + */\n> +\n> +int Lux::readYaml(const YamlObject &tuningData)\n> +{\n> + auto value = tuningData[\"reference_exposure_time\"].get();\n> + if (!value) {\n> + LOG(Lux, Error) << \"Missing tuning parameter: 'reference_exposure_time'\";\n> + return -EINVAL;\n> + }\n> + referenceExposureTime_ = *value * 1.0us;\n> +\n> + value = tuningData[\"reference_gain\"].get();\n> + if (!value) {\n> + LOG(Lux, Error) << \"Missing tuning parameter: 'reference_gain'\";\n> + return -EINVAL;\n> + }\n> + referenceGain_ = *value;\n> +\n> + referenceAperture_ = tuningData[\"reference_aperture\"].get(1.0);\n> +\n> + value = tuningData[\"reference_Y\"].get();\n> + if (!value) {\n> + LOG(Lux, Error) << \"Missing tuning parameter: 'reference_Y'\";\n> + return -EINVAL;\n> + }\n> + referenceY_ = *value;\n> +\n> + value = tuningData[\"reference_lux\"].get();\n> + if (!value) {\n> + LOG(Lux, Error) << \"Missing tuning parameter: 'reference_lux'\";\n> + return -EINVAL;\n> + }\n> + referenceLux_ = *value;\n> +\n> + return 0;\n> +}\n> +\n> +double Lux::process(double gain, utils::Duration exposureTime, double aperture,\n> + const Histogram &yHist) const\n\nAs this isn't an algorithm itself, it's a helper I wonder if this\nfunction should be named to match what it /does/. I.e.\n double Lux::estimate() ?\n\n> +{\n> + double currentY = yHist.interQuantileMean(0, 1);\n> + double gainRatio = referenceGain_ / gain;\n> + double exposureTimeRatio = referenceExposureTime_ / exposureTime;\n> + double apertureRatio = referenceAperture_ / aperture;\n> + double yRatio = currentY * (65536 / yHist.bins()) / referenceY_;\n> +\n> + double estimatedLux = exposureTimeRatio * gainRatio *\n> + apertureRatio * apertureRatio *\n> + yRatio * referenceLux_;\n\nDoes the tuning process only generate a single reference point? I guess\nit's expected to be closely linear here? Or maybe it's close enough as\nan estimate anyway.\n\n\n\n> +\n> + LOG(Lux, Debug) << \"Estimated lux \" << estimatedLux;\n> + return estimatedLux;\n> +}\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/lux.h b/src/ipa/libipa/lux.h\n> new file mode 100644\n> index 00000000..6bc9cf9f\n> --- /dev/null\n> +++ b/src/ipa/libipa/lux.h\n> @@ -0,0 +1,45 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2019, Raspberry Pi Ltd\n> + * Copyright (C) 2024, Paul Elder \n> + *\n> + * lux.h - Helper class that implements lux estimation\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \"libcamera/internal/yaml_parser.h\"\n> +\n> +#include \"histogram.h\"\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +\n> +class Lux\n> +{\n> +public:\n> + Lux() = default;\n> + ~Lux() = default;\n> +\n> + int readYaml(const YamlObject &tuningData);\n> + double process(double gain, utils::Duration exposureTime,\n> + double aperture, const Histogram &yHist) const;\n> +\n> +private:\n> + utils::Duration referenceExposureTime_;\n> + double referenceGain_;\n> + double referenceAperture_;\n> + double referenceY_;\n> + double referenceLux_;\n> +};\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> index 0796982e..b6d58900 100644\n> --- a/src/ipa/libipa/meson.build\n> +++ b/src/ipa/libipa/meson.build\n> @@ -8,6 +8,7 @@ libipa_headers = files([\n> 'exposure_mode_helper.h',\n> 'fc_queue.h',\n> 'histogram.h',\n> + 'lux.h',\n> 'matrix.h',\n> 'module.h',\n> 'pwl.h',\n> @@ -21,6 +22,7 @@ libipa_sources = files([\n> 'exposure_mode_helper.cpp',\n> 'fc_queue.cpp',\n> 'histogram.cpp',\n> + 'lux.cpp',\n> 'matrix.cpp',\n> 'module.cpp',\n> 'pwl.cpp'\n> -- \n> 2.39.2\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 529CBBE08B\n\tfor ;\n\tFri, 12 Apr 2024 10:54:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6FB7861BAE;\n\tFri, 12 Apr 2024 12:53:59 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7C58C61BAE\n\tfor ;\n\tFri, 12 Apr 2024 12:53:57 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D742FA68;\n\tFri, 12 Apr 2024 12:53:13 +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=\"CSFmIce8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1712919193;\n\tbh=qUiD4nhx8NcOM4RAcArQOyGNHqy+ALfbNgzZ4NLKBNo=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=CSFmIce8m58gxYGn8RD25iPCXxAhUUK90iOJhFW0VnwMbGOZ7VlwGqcDJlhMMbj9z\n\tA/b3XbBY1V88X/vO3IRw428hEwO2um7dzmXE55l0rg+eQtD8CMwurYOb2eHzUQoyps\n\tsrT2oJ9akD8Sy1vb1sWkkf5F6+HiIu0rE5OxJ8Fg=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240412091700.1817754-2-paul.elder@ideasonboard.com>","References":"<20240412091700.1817754-1-paul.elder@ideasonboard.com>\n\t<20240412091700.1817754-2-paul.elder@ideasonboard.com>","Subject":"Re: [PATCH 1/3] ipa: libipa: Add Lux helper","From":"Kieran Bingham ","Cc":"Paul Elder ","To":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 12 Apr 2024 11:53:54 +0100","Message-ID":"<171291923475.2108906.6960245983511821570@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29587,"web_url":"https://patchwork.libcamera.org/comment/29587/","msgid":"<71d2a5d3-6d01-4a87-b399-eed90506edd8@ideasonboard.com>","date":"2024-05-21T10:11:24","subject":"Re: [PATCH 1/3] ipa: libipa: Add Lux helper","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hi Paul - thanks for the patches\n\nOn 12/04/2024 10:16, Paul Elder wrote:\n> Add a Lux helper to libipa that does the estimation of the lux level\n> given gain, exposure, aperture, and luminance histogram. The helper also\n> handles reading the reference values from the tuning file. These are\n> expected to be common operations of lux algorithm modules in IPAs, and\n> is modeled/copied from Raspberry Pi.\n>\n> Signed-off-by: Paul Elder \n> ---\n> src/ipa/libipa/lux.cpp | 119 +++++++++++++++++++++++++++++++++++++\n> src/ipa/libipa/lux.h | 45 ++++++++++++++\n> src/ipa/libipa/meson.build | 2 +\n> 3 files changed, 166 insertions(+)\n> create mode 100644 src/ipa/libipa/lux.cpp\n> create mode 100644 src/ipa/libipa/lux.h\n>\n> diff --git a/src/ipa/libipa/lux.cpp b/src/ipa/libipa/lux.cpp\n> new file mode 100644\n> index 00000000..756cd3c4\n> --- /dev/null\n> +++ b/src/ipa/libipa/lux.cpp\n> @@ -0,0 +1,119 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2019, Raspberry Pi Ltd\n> + * Copyright (C) 2024, Paul Elder \n> + *\n> + * lux.h - Helper class that implements lux estimation\n> + */\nNo filename please\n> +#include \"lux.h\"\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \"libcamera/internal/yaml_parser.h\"\n> +\n> +#include \"histogram.h\"\n> +\n> +/**\n> + * \\file lux.h\n> + * \\brief Helper class that implements lux estimation\n> + *\n> + * As estimating the lux level of an image is expected to be a common\n> + * operation, it is implemented in a helper in libipa.\n> + */\n> +\n> +namespace libcamera {\n> +\n> +using namespace std::literals::chrono_literals;\n> +\n> +LOG_DEFINE_CATEGORY(Lux)\n> +\n> +namespace ipa {\n> +\n> +/**\n> + * \\class Lux\n> + * \\brief Class that implements lux estimation\n> + */\n> +\n> +/**\n> + * \\var Lux::referenceExposureTime_\n> + * \\brief The exposure time of the reference image, in microseconds.\n> + */\n> +\n> +/**\n> + * \\var Lux::referenceGain_\n> + * \\brief The analogue gain of the reference image.\n> + */\nAnalogue gain only? What if there was digital gain applied when the reference was collected?\n> +\n> +/**\n> + * \\var Lux::referenceAperture_\n> + * \\brief The aperture of the reference image in units of 1/f.\n> + */\n> +\n> +/**\n> + * \\var Lux::referenceY_\n> + * \\brief The measured luminance of the reference image, out of 65536.\n\n\n65536? Or 65535?\n\n> + */\n> +\n> +/**\n> + * \\var Lux::referenceLux_\n> + * \\brief The estimated lux level of the reference image.\n> + */\n> +\n> +int Lux::readYaml(const YamlObject &tuningData)\n\n\nThis is a public function, so I think it should have a doxygen comment?\n\n> +{\n> +\tauto value = tuningData[\"reference_exposure_time\"].get();\n> +\tif (!value) {\n> +\t\tLOG(Lux, Error) << \"Missing tuning parameter: 'reference_exposure_time'\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\treferenceExposureTime_ = *value * 1.0us;\n\n\nLaurent suggested switching to integration time everywhere, as exposure time tends to get shortened \nto \"exposure\".\n\n> +\n> +\tvalue = tuningData[\"reference_gain\"].get();\n> +\tif (!value) {\n> +\t\tLOG(Lux, Error) << \"Missing tuning parameter: 'reference_gain'\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\treferenceGain_ = *value;\n> +\n> +\treferenceAperture_ = tuningData[\"reference_aperture\"].get(1.0);\n> +\n> +\tvalue = tuningData[\"reference_Y\"].get();\n> +\tif (!value) {\n> +\t\tLOG(Lux, Error) << \"Missing tuning parameter: 'reference_Y'\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\treferenceY_ = *value;\n> +\n> +\tvalue = tuningData[\"reference_lux\"].get();\n> +\tif (!value) {\n> +\t\tLOG(Lux, Error) << \"Missing tuning parameter: 'reference_lux'\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\treferenceLux_ = *value;\n> +\n> +\treturn 0;\n> +}\n> +\n> +double Lux::process(double gain, utils::Duration exposureTime, double aperture,\n> +\t\t const Histogram &yHist) const\nWe've no constraints on the name here, so I think \"estimateLux\" would be better. Also needs a \ndoxygen comment I think?\n> +{\n> +\tdouble currentY = yHist.interQuantileMean(0, 1);\n> +\tdouble gainRatio = referenceGain_ / gain;\n> +\tdouble exposureTimeRatio = referenceExposureTime_ / exposureTime;\n> +\tdouble apertureRatio = referenceAperture_ / aperture;\n> +\tdouble yRatio = currentY * (65536 / yHist.bins()) / referenceY_;\n> +\n> +\tdouble estimatedLux = exposureTimeRatio * gainRatio *\n> +\t\t\t apertureRatio * apertureRatio *\n> +\t\t\t yRatio * referenceLux_;\n\n\napertureRatio is in here twice; is that accidental? Do you know the background to the formula? It's \na bit strange, in my view...our assumption from the mean luminance AGC is of a linear relationship \nbetween \"total exposure\" (comprising integration time multiplied by gain) and the average of the \nyHist, but that's not really reflected here.\n\n> +\n> +\tLOG(Lux, Debug) << \"Estimated lux \" << estimatedLux;\n> +\treturn estimatedLux;\n> +}\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/lux.h b/src/ipa/libipa/lux.h\n> new file mode 100644\n> index 00000000..6bc9cf9f\n> --- /dev/null\n> +++ b/src/ipa/libipa/lux.h\n> @@ -0,0 +1,45 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2019, Raspberry Pi Ltd\n> + * Copyright (C) 2024, Paul Elder \n> + *\n> + * lux.h - Helper class that implements lux estimation\n> + */\nNo file name please\n> +\n> +#pragma once\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \"libcamera/internal/yaml_parser.h\"\n> +\n> +#include \"histogram.h\"\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +\n> +class Lux\n> +{\n> +public:\n> +\tLux() = default;\n> +\t~Lux() = default;\n> +\n> +\tint readYaml(const YamlObject &tuningData);\n> +\tdouble process(double gain, utils::Duration exposureTime,\n> +\t\t double aperture, const Histogram &yHist) const;\n> +\n> +private:\n> +\tutils::Duration referenceExposureTime_;\n> +\tdouble referenceGain_;\n> +\tdouble referenceAperture_;\n> +\tdouble referenceY_;\n> +\tdouble referenceLux_;\n> +};\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> index 0796982e..b6d58900 100644\n> --- a/src/ipa/libipa/meson.build\n> +++ b/src/ipa/libipa/meson.build\n> @@ -8,6 +8,7 @@ libipa_headers = files([\n> 'exposure_mode_helper.h',\n> 'fc_queue.h',\n> 'histogram.h',\n> + 'lux.h',\n> 'matrix.h',\n> 'module.h',\n> 'pwl.h',\n> @@ -21,6 +22,7 @@ libipa_sources = files([\n> 'exposure_mode_helper.cpp',\n> 'fc_queue.cpp',\n> 'histogram.cpp',\n> + 'lux.cpp',\n> 'matrix.cpp',\n> 'module.cpp',\n> 'pwl.cpp'","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 4A0B5BDE6B\n\tfor ;\n\tTue, 21 May 2024 10:11:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4EFC861A58;\n\tTue, 21 May 2024 12:11:30 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E975D61A58\n\tfor ;\n\tTue, 21 May 2024 12:11:27 +0200 (CEST)","from [192.168.0.43]\n\t(cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net [86.13.91.161])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B794ECE3\n\tfor ;\n\tTue, 21 May 2024 12:11:15 +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=\"OJSQd+q+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1716286275;\n\tbh=ou7BwQdtz2fc79jtNHXiRiDN6xYwk72bJ5fOSqpQpE0=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=OJSQd+q+fcGj+f/7LyTHXC53QugGUsXfPFSQHS9w3bR2BdCT2GCExxvER/+Bx+1oW\n\tqJ2Xgl/kiXWlZqIqlTKRvesuWT5fLh0DSn7GdOQPyWBt136iGSVqJQ2Lb/pohQe+Fg\n\tV4RFl2S0gzPlngxy6jPsHUOJ2sa+IgYTLZgc/JN8=","Message-ID":"<71d2a5d3-6d01-4a87-b399-eed90506edd8@ideasonboard.com>","Date":"Tue, 21 May 2024 11:11:24 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 1/3] ipa: libipa: Add Lux helper","To":"libcamera-devel@lists.libcamera.org","References":"<20240412091700.1817754-1-paul.elder@ideasonboard.com>\n\t<20240412091700.1817754-2-paul.elder@ideasonboard.com>","Content-Language":"en-US","From":"Dan Scally ","Autocrypt":"addr=dan.scally@ideasonboard.com; keydata=\n\txsFNBGLydlEBEADa5O2s0AbUguprfvXOQun/0a8y2Vk6BqkQALgeD6KnXSWwaoCULp18etYW\n\tB31bfgrdphXQ5kUQibB0ADK8DERB4wrzrUb5CMxLBFE7mQty+v5NsP0OFNK9XTaAOcmD+Ove\n\teIjYvqurAaro91jrRVrS1gBRxIFqyPgNvwwL+alMZhn3/2jU2uvBmuRrgnc/e9cHKiuT3Dtq\n\tMHGPKL2m+plk+7tjMoQFfexoQ1JKugHAjxAhJfrkXh6uS6rc01bYCyo7ybzg53m1HLFJdNGX\n\tsUKR+dQpBs3SY4s66tc1sREJqdYyTsSZf80HjIeJjU/hRunRo4NjRIJwhvnK1GyjOvvuCKVU\n\tRWpY8dNjNu5OeAfdrlvFJOxIE9M8JuYCQTMULqd1NuzbpFMjc9524U3Cngs589T7qUMPb1H1\n\tNTA81LmtJ6Y+IV5/kiTUANflpzBwhu18Ok7kGyCq2a2jsOcVmk8gZNs04gyjuj8JziYwwLbf\n\tvzABwpFVcS8aR+nHIZV1HtOzyw8CsL8OySc3K9y+Y0NRpziMRvutrppzgyMb9V+N31mK9Mxl\n\t1YkgaTl4ciNWpdfUe0yxH03OCuHi3922qhPLF4XX5LN+NaVw5Xz2o3eeWklXdouxwV7QlN33\n\tu4+u2FWzKxDqO6WLQGjxPE0mVB4Gh5Pa1Vb0ct9Ctg0qElvtGQARAQABzShEYW4gU2NhbGx5\n\tIDxkYW4uc2NhbGx5QGlkZWFzb25ib2FyZC5jb20+wsGNBBMBCAA3FiEEsdtt8OWP7+8SNfQe\n\tkiQuh/L+GMQFAmLydlIFCQWjmoACGwMECwkIBwUVCAkKCwUWAgMBAAAKCRCSJC6H8v4YxDI2\n\tEAC2Gz0iyaXJkPInyshrREEWbo0CA6v5KKf3I/HlMPqkZ48bmGoYm4mEQGFWZJAT3K4ir8bg\n\tcEfs9V54gpbrZvdwS4abXbUK4WjKwEs8HK3XJv1WXUN2bsz5oEJWZUImh9gD3naiLLI9QMMm\n\tw/aZkT+NbN5/2KvChRWhdcha7+2Te4foOY66nIM+pw2FZM6zIkInLLUik2zXOhaZtqdeJZQi\n\tHSPU9xu7TRYN4cvdZAnSpG7gQqmLm5/uGZN1/sB3kHTustQtSXKMaIcD/DMNI3JN/t+RJVS7\n\tc0Jh/ThzTmhHyhxx3DRnDIy7kwMI4CFvmhkVC2uNs9kWsj1DuX5kt8513mvfw2OcX9UnNKmZ\n\tnhNCuF6DxVrL8wjOPuIpiEj3V+K7DFF1Cxw1/yrLs8dYdYh8T8vCY2CHBMsqpESROnTazboh\n\tAiQ2xMN1cyXtX11Qwqm5U3sykpLbx2BcmUUUEAKNsM//Zn81QXKG8vOx0ZdMfnzsCaCzt8f6\n\t9dcDBBI3tJ0BI9ByiocqUoL6759LM8qm18x3FYlxvuOs4wSGPfRVaA4yh0pgI+ModVC2Pu3y\n\tejE/IxeatGqJHh6Y+iJzskdi27uFkRixl7YJZvPJAbEn7kzSi98u/5ReEA8Qhc8KO/B7wprj\n\txjNMZNYd0Eth8+WkixHYj752NT5qshKJXcyUU87BTQRi8nZSARAAx0BJayh1Fhwbf4zoY56x\n\txHEpT6DwdTAYAetd3yiKClLVJadYxOpuqyWa1bdfQWPb+h4MeXbWw/53PBgn7gI2EA7ebIRC\n\tPJJhAIkeym7hHZoxqDQTGDJjxFEL11qF+U3rhWiL2Zt0Pl+zFq0eWYYVNiXjsIS4FI2+4m16\n\ttPbDWZFJnSZ828VGtRDQdhXfx3zyVX21lVx1bX4/OZvIET7sVUufkE4hrbqrrufre7wsjD1t\n\t8MQKSapVrr1RltpzPpScdoxknOSBRwOvpp57pJJe5A0L7+WxJ+vQoQXj0j+5tmIWOAV1qBQp\n\thyoyUk9JpPfntk2EKnZHWaApFp5TcL6c5LhUvV7F6XwOjGPuGlZQCWXee9dr7zym8iR3irWT\n\t+49bIh5PMlqSLXJDYbuyFQHFxoiNdVvvf7etvGfqFYVMPVjipqfEQ38ST2nkzx+KBICz7uwj\n\tJwLBdTXzGFKHQNckGMl7F5QdO/35An/QcxBnHVMXqaSd12tkJmoRVWduwuuoFfkTY5mUV3uX\n\txGj3iVCK4V+ezOYA7c2YolfRCNMTza6vcK/P4tDjjsyBBZrCCzhBvd4VVsnnlZhVaIxoky4K\n\taL+AP+zcQrUZmXmgZjXOLryGnsaeoVrIFyrU6ly90s1y3KLoPsDaTBMtnOdwxPmo1xisH8oL\n\ta/VRgpFBfojLPxMAEQEAAcLBfAQYAQgAJhYhBLHbbfDlj+/vEjX0HpIkLofy/hjEBQJi8nZT\n\tBQkFo5qAAhsMAAoJEJIkLofy/hjEXPcQAMIPNqiWiz/HKu9W4QIf1OMUpKn3YkVIj3p3gvfM\n\tRes4fGX94Ji599uLNrPoxKyaytC4R6BTxVriTJjWK8mbo9jZIRM4vkwkZZ2bu98EweSucxbp\n\tvjESsvMXGgxniqV/RQ/3T7LABYRoIUutARYq58p5HwSP0frF0fdFHYdTa2g7MYZl1ur2JzOC\n\tFHRpGadlNzKDE3fEdoMobxHB3Lm6FDml5GyBAA8+dQYVI0oDwJ3gpZPZ0J5Vx9RbqXe8RDuR\n\tdu90hvCJkq7/tzSQ0GeD3BwXb9/R/A4dVXhaDd91Q1qQXidI+2jwhx8iqiYxbT+DoAUkQRQy\n\txBtoCM1CxH7u45URUgD//fxYr3D4B1SlonA6vdaEdHZOGwECnDpTxecENMbz/Bx7qfrmd901\n\tD+N9SjIwrbVhhSyUXYnSUb8F+9g2RDY42Sk7GcYxIeON4VzKqWM7hpkXZ47pkK0YodO+dRKM\n\tyMcoUWrTK0Uz6UzUGKoJVbxmSW/EJLEGoI5p3NWxWtScEVv8mO49gqQdrRIOheZycDmHnItt\n\t9Qjv00uFhEwv2YfiyGk6iGF2W40s2pH2t6oeuGgmiZ7g6d0MEK8Ql/4zPItvr1c1rpwpXUC1\n\tu1kQWgtnNjFHX3KiYdqjcZeRBiry1X0zY+4Y24wUU0KsEewJwjhmCKAsju1RpdlPg2kC","In-Reply-To":"<20240412091700.1817754-2-paul.elder@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":29963,"web_url":"https://patchwork.libcamera.org/comment/29963/","msgid":"<20240616225630.GD28126@pendragon.ideasonboard.com>","date":"2024-06-16T22:56:30","subject":"Re: [PATCH 1/3] ipa: libipa: Add Lux helper","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, May 21, 2024 at 11:11:24AM +0100, Daniel Scally wrote:\n> Hi Paul - thanks for the patches\n> \n> On 12/04/2024 10:16, Paul Elder wrote:\n> > Add a Lux helper to libipa that does the estimation of the lux level\n> > given gain, exposure, aperture, and luminance histogram. The helper also\n> > handles reading the reference values from the tuning file. These are\n> > expected to be common operations of lux algorithm modules in IPAs, and\n> > is modeled/copied from Raspberry Pi.\n> >\n> > Signed-off-by: Paul Elder \n> > ---\n> > src/ipa/libipa/lux.cpp | 119 +++++++++++++++++++++++++++++++++++++\n> > src/ipa/libipa/lux.h | 45 ++++++++++++++\n> > src/ipa/libipa/meson.build | 2 +\n> > 3 files changed, 166 insertions(+)\n> > create mode 100644 src/ipa/libipa/lux.cpp\n> > create mode 100644 src/ipa/libipa/lux.h\n> >\n> > diff --git a/src/ipa/libipa/lux.cpp b/src/ipa/libipa/lux.cpp\n> > new file mode 100644\n> > index 00000000..756cd3c4\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/lux.cpp\n> > @@ -0,0 +1,119 @@\n> > +/* SPDX-License-Identifier: BSD-2-Clause */\n> > +/*\n> > + * Copyright (C) 2019, Raspberry Pi Ltd\n> > + * Copyright (C) 2024, Paul Elder \n> > + *\n> > + * lux.h - Helper class that implements lux estimation\n> > + */\n> No filename please\n> > +#include \"lux.h\"\n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"libcamera/internal/yaml_parser.h\"\n> > +\n> > +#include \"histogram.h\"\n> > +\n> > +/**\n> > + * \\file lux.h\n> > + * \\brief Helper class that implements lux estimation\n> > + *\n> > + * As estimating the lux level of an image is expected to be a common\n> > + * operation, it is implemented in a helper in libipa.\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +using namespace std::literals::chrono_literals;\n> > +\n> > +LOG_DEFINE_CATEGORY(Lux)\n> > +\n> > +namespace ipa {\n> > +\n> > +/**\n> > + * \\class Lux\n> > + * \\brief Class that implements lux estimation\n> > + */\n> > +\n> > +/**\n> > + * \\var Lux::referenceExposureTime_\n> > + * \\brief The exposure time of the reference image, in microseconds.\n> > + */\n> > +\n> > +/**\n> > + * \\var Lux::referenceGain_\n> > + * \\brief The analogue gain of the reference image.\n> > + */\n> \n> Analogue gain only? What if there was digital gain applied when the reference was collected?\n> \n> > +\n> > +/**\n> > + * \\var Lux::referenceAperture_\n> > + * \\brief The aperture of the reference image in units of 1/f.\n> > + */\n> > +\n> > +/**\n> > + * \\var Lux::referenceY_\n> > + * \\brief The measured luminance of the reference image, out of 65536.\n> \n> 65536? Or 65535?\n> \n> > + */\n> > +\n> > +/**\n> > + * \\var Lux::referenceLux_\n> > + * \\brief The estimated lux level of the reference image.\n> > + */\n> > +\n> > +int Lux::readYaml(const YamlObject &tuningData)\n> \n> This is a public function, so I think it should have a doxygen comment?\n> \n> > +{\n> > +\tauto value = tuningData[\"reference_exposure_time\"].get();\n> > +\tif (!value) {\n> > +\t\tLOG(Lux, Error) << \"Missing tuning parameter: 'reference_exposure_time'\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\treferenceExposureTime_ = *value * 1.0us;\n> \n> Laurent suggested switching to integration time everywhere, as exposure time tends to get shortened \n> to \"exposure\".\n\nI think we need a tree-wide patch for this. The naming in this patch can\nbe kept if it gets merged first.\n\n> > +\n> > +\tvalue = tuningData[\"reference_gain\"].get();\n> > +\tif (!value) {\n> > +\t\tLOG(Lux, Error) << \"Missing tuning parameter: 'reference_gain'\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\treferenceGain_ = *value;\n> > +\n> > +\treferenceAperture_ = tuningData[\"reference_aperture\"].get(1.0);\n> > +\n> > +\tvalue = tuningData[\"reference_Y\"].get();\n> > +\tif (!value) {\n> > +\t\tLOG(Lux, Error) << \"Missing tuning parameter: 'reference_Y'\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\treferenceY_ = *value;\n> > +\n> > +\tvalue = tuningData[\"reference_lux\"].get();\n> > +\tif (!value) {\n> > +\t\tLOG(Lux, Error) << \"Missing tuning parameter: 'reference_lux'\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\treferenceLux_ = *value;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +double Lux::process(double gain, utils::Duration exposureTime, double aperture,\n> > +\t\t const Histogram &yHist) const\n>\n> We've no constraints on the name here, so I think \"estimateLux\" would be better. Also needs a \n> doxygen comment I think?\n>\n> > +{\n> > +\tdouble currentY = yHist.interQuantileMean(0, 1);\n> > +\tdouble gainRatio = referenceGain_ / gain;\n> > +\tdouble exposureTimeRatio = referenceExposureTime_ / exposureTime;\n> > +\tdouble apertureRatio = referenceAperture_ / aperture;\n> > +\tdouble yRatio = currentY * (65536 / yHist.bins()) / referenceY_;\n> > +\n> > +\tdouble estimatedLux = exposureTimeRatio * gainRatio *\n> > +\t\t\t apertureRatio * apertureRatio *\n> > +\t\t\t yRatio * referenceLux_;\n> \n> apertureRatio is in here twice; is that accidental?\n\nThe aperture is expressed as a number proportional to the diameter of\nthe entrance pupil, while the exposure if proportional to the area of\nthe pupil. This is worth a comment.\n\nWe don't support aperture control though, so maybe the simplest option\nwould be to drop it.\n\n> Do you know the background to the formula? It's \n> a bit strange, in my view...our assumption from the mean luminance AGC is of a linear relationship \n> between \"total exposure\" (comprising integration time multiplied by gain) and the average of the \n> yHist, but that's not really reflected here.\n> \n> > +\n> > +\tLOG(Lux, Debug) << \"Estimated lux \" << estimatedLux;\n> > +\treturn estimatedLux;\n> > +}\n> > +\n> > +} /* namespace ipa */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/libipa/lux.h b/src/ipa/libipa/lux.h\n> > new file mode 100644\n> > index 00000000..6bc9cf9f\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/lux.h\n> > @@ -0,0 +1,45 @@\n> > +/* SPDX-License-Identifier: BSD-2-Clause */\n> > +/*\n> > + * Copyright (C) 2019, Raspberry Pi Ltd\n> > + * Copyright (C) 2024, Paul Elder \n> > + *\n> > + * lux.h - Helper class that implements lux estimation\n> > + */\n> \n> No file name please\n> \n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"libcamera/internal/yaml_parser.h\"\n> > +\n> > +#include \"histogram.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace ipa {\n> > +\n> > +class Lux\n> > +{\n> > +public:\n> > +\tLux() = default;\n> > +\t~Lux() = default;\n> > +\n> > +\tint readYaml(const YamlObject &tuningData);\n> > +\tdouble process(double gain, utils::Duration exposureTime,\n> > +\t\t double aperture, const Histogram &yHist) const;\n> > +\n> > +private:\n> > +\tutils::Duration referenceExposureTime_;\n> > +\tdouble referenceGain_;\n> > +\tdouble referenceAperture_;\n> > +\tdouble referenceY_;\n> > +\tdouble referenceLux_;\n> > +};\n> > +\n> > +} /* namespace ipa */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> > index 0796982e..b6d58900 100644\n> > --- a/src/ipa/libipa/meson.build\n> > +++ b/src/ipa/libipa/meson.build\n> > @@ -8,6 +8,7 @@ libipa_headers = files([\n> > 'exposure_mode_helper.h',\n> > 'fc_queue.h',\n> > 'histogram.h',\n> > + 'lux.h',\n> > 'matrix.h',\n> > 'module.h',\n> > 'pwl.h',\n> > @@ -21,6 +22,7 @@ libipa_sources = files([\n> > 'exposure_mode_helper.cpp',\n> > 'fc_queue.cpp',\n> > 'histogram.cpp',\n> > + 'lux.cpp',\n> > 'matrix.cpp',\n> > 'module.cpp',\n> > 'pwl.cpp'","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 908C0BD87C\n\tfor ;\n\tSun, 16 Jun 2024 22:56:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B7A3B6548D;\n\tMon, 17 Jun 2024 00:56:53 +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 16ACB65458\n\tfor ;\n\tMon, 17 Jun 2024 00:56:52 +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 603059C1;\n\tMon, 17 Jun 2024 00:56:35 +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=\"fUXyiSa5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718578595;\n\tbh=zMtL/HzW9Pawg5XVZNsaSJjLOHa9au4fvqcDAOLe9bM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fUXyiSa5sNgrS0T/px4+MpfBTtLVHui21frAZhKY4UlIIR0IpWSzRg6hOyp3ekA8p\n\tpoYyTEuEtwg1xqoSDOQuZ1yJTkpM6RAk90uyyABiRAjmKeasnOavYYLx3tzSIOsdG9\n\tmINygLXVbgPReinbLFnvTvw9VHTyBa1n28U3GHIE=","Date":"Mon, 17 Jun 2024 01:56:30 +0300","From":"Laurent Pinchart ","To":"Dan Scally ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 1/3] ipa: libipa: Add Lux helper","Message-ID":"<20240616225630.GD28126@pendragon.ideasonboard.com>","References":"<20240412091700.1817754-1-paul.elder@ideasonboard.com>\n\t<20240412091700.1817754-2-paul.elder@ideasonboard.com>\n\t<71d2a5d3-6d01-4a87-b399-eed90506edd8@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<71d2a5d3-6d01-4a87-b399-eed90506edd8@ideasonboard.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\" "}}]