[{"id":32862,"web_url":"https://patchwork.libcamera.org/comment/32862/","msgid":"<20241218030709.GA23470@pendragon.ideasonboard.com>","date":"2024-12-18T03:07:09","subject":"Re: [PATCH v2 1/2] 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":"Hi Paul,\n\nThank you for the patch.\n\nOn Mon, Dec 16, 2024 at 06:49:32PM +0900, Paul Elder wrote:\n> Add a Lux helper to libipa that does the estimation of the lux level\n> given gain, exposure, 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> ---\n> Changes in v2:\n> - improve documentation\n> - add binSize member variable and corresponding setter\n> - remove aperture\n> - split gain into analogue and digital\n> - change tuning file names into camel case\n> ---\n> src/ipa/libipa/lux.cpp | 183 +++++++++++++++++++++++++++++++++++++\n> src/ipa/libipa/lux.h | 48 ++++++++++\n> src/ipa/libipa/meson.build | 2 +\n> 3 files changed, 233 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 000000000000..55b771705ab7\n> --- /dev/null\n> +++ b/src/ipa/libipa/lux.cpp\n> @@ -0,0 +1,183 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2019, Raspberry Pi Ltd\n> + * Copyright (C) 2024, Paul Elder \n> + *\n> + * 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. It can be used to adjust\n> + * the target Y value in AGC or for Bayesian AWB estimation.\n\nThat sounds more like a commit message. For the documentation, you can\nwrite\n\n * Estimating the lux level of an image is a common operation that can for\n * instance be used to adjust the target Y value in AGC or for Bayesian AWB\n * estimation.\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> + * IPAs that wish to use lux esimation should create a Lux algorithm module\n> + * that lightly wraps this module by providing the platform-specific luminance\n> + * histogram. The Lux entry in the tuning file must then precede the algorithms\n> + * that depend on the estimated lux value.\n> + */\n> +\n> +/**\n> + * \\var Lux::binSize_\n> + * \\brief The maximum count of each bin\n> + *\n> + * This must be populated via setBinSize() before estimateLux() is used.\n> + */\n> +\n> +/**\n> + * \\var Lux::referenceExposureTime_\n> + * \\brief The exposure time of the reference image, in microseconds.\n\ns/\\.$//g\n\nSame below.\n\n> + */\n> +\n> +/**\n> + * \\var Lux::referenceAnalogueGain_\n> + * \\brief The analogue gain of the reference image.\n> + */\n> +\n> +/**\n> + * \\var Lux::referenceDigitalGain_\n> + * \\brief The analogue gain of the reference image.\n> + */\n> +\n> +/**\n> + * \\var Lux::referenceY_\n> + * \\brief The measured luminance of the reference image, out of the bin size.\n> + *\n> + * \\sa binSize_\n> + */\n> +\n> +/**\n> + * \\var Lux::referenceLux_\n> + * \\brief The estimated lux level of the reference image.\n> + */\n> +\n> +/**\n> + * \\brief Configure the bin size of the Lux helper module\n> + * \\param[in] binSize The maximum count of each bin\n> + *\n> + * This must be configured befure estimateLux() is called, as it is\n> + * platform-specific.\n> + */\n> +void Lux::setBinSize(unsigned int binSize)\n\nIn patch 2/2 this function is called at init() time with a compile-time\nconstant. Do you expect use cases where the value wouldn't be known at\ncompile time ? If not you could pass it to the constructor instead.\n\nIf the number of bins could vary, I'd rename this function to init(), to\nmore clearly indicate it is meant as a one-time initialization.\n\n> +{\n> +\tbinSize_ = binSize;\n> +}\n> +\n> +/**\n> + * \\brief Parse tuning data\n> + * \\param[in] tuningData The YamlObject representing the tuning data\n> + *\n> + * This function parses yaml tuning data for the common Lux module. It requires\n> + * reference exposure time, analogue gain, digital gain, and lux values.\n> + *\n> + * \\code{.unparsed}\n> + * algorithms:\n> + * - Lux:\n> + * referenceExposureTime: 10000\n> + * referenceAnalogueGain: 4.0\n> + * referenceDigitalGain: 1.0\n> + * referenceY: 12000\n\nYou're missing referenceLux.\n\n> + * \\endcode\n> + *\n> + * \\return 0 on success or a negative error code\n> + */\n> +int Lux::readYaml(const YamlObject &tuningData)\n> +{\n> +\tauto value = tuningData[\"referenceExposureTime\"].get();\n> +\tif (!value) {\n> +\t\tLOG(Lux, Error) << \"Missing tuning parameter: 'referenceExposureTime'\";\n\nIf you wrote\n\n\t\tLOG(Lux, Error) << \"Missing tuning parameter: \" << \"referenceExposureTime\";\n\nI think the compiler could do string de-duplication. Same below.\n\n> +\t\treturn -EINVAL;\n> +\t}\n> +\treferenceExposureTime_ = *value * 1.0us;\n> +\n> +\tvalue = tuningData[\"referenceAnalogueGain\"].get();\n> +\tif (!value) {\n> +\t\tLOG(Lux, Error) << \"Missing tuning parameter: 'referenceAnalogueGain'\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\treferenceAnalogueGain_ = *value;\n> +\n> +\tvalue = tuningData[\"referenceDigitalGain\"].get();\n> +\tif (!value) {\n> +\t\tLOG(Lux, Error) << \"Missing tuning parameter: 'referenceDigitalGain'\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\treferenceDigitalGain_ = *value;\n> +\n> +\tvalue = tuningData[\"referenceY\"].get();\n> +\tif (!value) {\n> +\t\tLOG(Lux, Error) << \"Missing tuning parameter: 'referenceY'\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\treferenceY_ = *value;\n> +\n> +\tvalue = tuningData[\"referenceLux\"].get();\n> +\tif (!value) {\n> +\t\tLOG(Lux, Error) << \"Missing tuning parameter: 'referenceLux'\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\treferenceLux_ = *value;\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Estimate lux given runtime values\n> + * \\param[in] exposureTime Exposure time applied to the frame\n> + * \\param[in] aGain Analogue gain applied to the frame\n> + * \\param[in] dGain Digital gain applied to the frame\n> + * \\param[in] yHist Histogram from the ISP statistics\n> + *\n> + * Estimate the lux given the exposure time, gain, and histogram.\n> + *\n> + * The bin size must be configured via setBinSize() before this function can be\n> + * called.\n> + *\n> + * \\return Estimated lux value\n> + */\n> +double Lux::estimateLux(utils::Duration exposureTime,\n> +\t\t\tdouble aGain, double dGain,\n> +\t\t\tconst Histogram &yHist) const\n> +{\n> +\tdouble currentY = yHist.interQuantileMean(0, 1);\n> +\tdouble exposureTimeRatio = referenceExposureTime_ / exposureTime;\n> +\tdouble aGainRatio = referenceAnalogueGain_ / aGain;\n> +\tdouble dGainRatio = referenceDigitalGain_ / dGain;\n> +\tdouble yRatio = currentY * (binSize_ / yHist.bins()) / referenceY_;\n> +\n> +\tdouble estimatedLux = exposureTimeRatio * aGainRatio * dGainRatio *\n> +\t\t\t yRatio * referenceLux_;\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 000000000000..3e6dcc5c3413\n> --- /dev/null\n> +++ b/src/ipa/libipa/lux.h\n> @@ -0,0 +1,48 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2019, Raspberry Pi Ltd\n> + * Copyright (C) 2024, Paul Elder \n> + *\n> + * Helper class that implements lux estimation\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +#include \n> +#include \n\nNone of these seem to be needed in the header file.\n\n> +\n> +#include \n> +\n> +#include \"libcamera/internal/yaml_parser.h\"\n\nThis can be replaced by a forward declaration of the YamlObject class.\n\n> +\n> +#include \"histogram.h\"\n\nSame here for the Histogram class.\n\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +\n> +class Lux\n> +{\n> +public:\n> +\tLux() = default;\n> +\t~Lux() = default;\n\nNo need to declare the default constructor and destructor if they're\ndefaulted and the class has no other user-declared constructor.\n\n> +\n> +\tvoid setBinSize(unsigned int binSize);\n> +\tint readYaml(const YamlObject &tuningData);\n\ns/readYaml/parseTuningData/\n\nReviewed-by: Laurent Pinchart \n\n> +\tdouble estimateLux(utils::Duration exposureTime,\n> +\t\t\t double aGain, double dGain,\n> +\t\t\t const Histogram &yHist) const;\n> +\n> +private:\n> +\tunsigned int binSize_;\n> +\tutils::Duration referenceExposureTime_;\n> +\tdouble referenceAnalogueGain_;\n> +\tdouble referenceDigitalGain_;\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 93ae25da5e83..189ec0311bd3 100644\n> --- a/src/ipa/libipa/meson.build\n> +++ b/src/ipa/libipa/meson.build\n> @@ -10,6 +10,7 @@ libipa_headers = files([\n> 'histogram.h',\n> 'interpolator.h',\n> 'lsc_polynomial.h',\n> + 'lux.h',\n> 'module.h',\n> 'pwl.h',\n> 'vector.h',\n> @@ -25,6 +26,7 @@ libipa_sources = files([\n> 'histogram.cpp',\n> 'interpolator.cpp',\n> 'lsc_polynomial.cpp',\n> + 'lux.cpp',\n> 'module.cpp',\n> 'pwl.cpp',\n> 'vector.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 6A1D9C3304\n\tfor ;\n\tWed, 18 Dec 2024 03:07:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2F9AD68077;\n\tWed, 18 Dec 2024 04:07:14 +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 F100F61898\n\tfor ;\n\tWed, 18 Dec 2024 04:07:11 +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 111D3514;\n\tWed, 18 Dec 2024 04:06:34 +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=\"dPfJuCZN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1734491194;\n\tbh=BHCtBPRwX0PKNX2YMj8IB1ILpwy2GwjK3dx2tOuTdwc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dPfJuCZNigldKxy8AnrsKFMnnMsvYSZvHi7jgeakYVTpMigiclxA/d9XqjkGaY/IT\n\tfSsz9BWeniZZbbj5ENytNWc/eLbwAjC4YJqviaHvQDjiSJ/o5VUp+WhTefQBVg3JsJ\n\tE577c7CQyyW/aMbJD3NUjHDQRFJZ3IcnaDSQMJ0E=","Date":"Wed, 18 Dec 2024 05:07:09 +0200","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org, stefan.klug@ideasonboard.com","Subject":"Re: [PATCH v2 1/2] ipa: libipa: Add Lux helper","Message-ID":"<20241218030709.GA23470@pendragon.ideasonboard.com>","References":"<20241216094933.198027-1-paul.elder@ideasonboard.com>\n\t<20241216094933.198027-2-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241216094933.198027-2-paul.elder@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\" "}}]