[{"id":13455,"web_url":"https://patchwork.libcamera.org/comment/13455/","msgid":"<20201023210025.GG5979@pendragon.ideasonboard.com>","date":"2020-10-23T21:00:25","subject":"Re: [libcamera-devel] [PATCH v4 03/12] libcamera: controls:\n\tConstruct from valid values","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Fri, Oct 23, 2020 at 07:11:07PM +0200, Jacopo Mondi wrote:\n> Add two constructors to the ControlInfo class that allows creating\n> a class instance from the list of the control valid values with\n> an optional default one.\n> \n> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> ---\n>  include/libcamera/controls.h |  5 ++++\n>  src/libcamera/controls.cpp   | 47 ++++++++++++++++++++++++++++++++++++\n>  2 files changed, 52 insertions(+)\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 80944efc133a..0ae4fd002726 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -268,10 +268,14 @@ public:\n>  \texplicit ControlInfo(const ControlValue &min = 0,\n>  \t\t\t     const ControlValue &max = 0,\n>  \t\t\t     const ControlValue &def = 0);\n> +\texplicit ControlInfo(const ControlValue *values, unsigned int numValues,\n> +\t\t\t     const ControlValue &def);\n> +\texplicit ControlInfo(const ControlValue *values, unsigned int numValues);\n\nWe could use a single constructor, with def = {}. The implementation can\ntest def.isNone() to decide if the default should be taken from the\nfirst value in the values, or from def.\n\n>  \n>  \tconst ControlValue &min() const { return min_; }\n>  \tconst ControlValue &max() const { return max_; }\n>  \tconst ControlValue &def() const { return def_; }\n> +\tconst std::vector<ControlValue> &values() const { return values_; }\n\nYou need to include <vector> at the top of the file for this\n(compilation fails with clang-8 + libc++ for me otherwise).\n\n>  \n>  \tstd::string toString() const;\n>  \n> @@ -289,6 +293,7 @@ private:\n>  \tControlValue min_;\n>  \tControlValue max_;\n>  \tControlValue def_;\n> +\tstd::vector<ControlValue> values_;\n>  };\n>  \n>  using ControlIdMap = std::unordered_map<unsigned int, const ControlId *>;\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index dca782667d88..2a2f04a598b6 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -491,6 +491,42 @@ ControlInfo::ControlInfo(const ControlValue &min,\n>  {\n>  }\n>  \n> +/**\n> + * \\brief Construct a ControlInfo from the list of values values and a default\n\ns/values values/valid values/\n\n> + * \\param[in] values The control valid values\n> + * \\param[in] numValues The number or valid values\n\ns/number of/number of/\n\n> + * \\param[in] def The control default value\n> + *\n> + * Construct a ControlInfo from a list of valid values. The ControlInfo\n> + * minimum and maximum values are assigned to the first and last members of\n> + * the values list respectively.\n> + */\n> +ControlInfo::ControlInfo(const ControlValue *values, unsigned int numValues,\n> +\t\t\t const ControlValue &def)\n> +\t: def_(def)\n> +{\n> +\tmin_ = values[0];\n> +\tmax_ = values[numValues - 1];\n> +\n\nMaybe\n\n\tvalues_.reserve(numValues);\n\nhere ?\n\n> +\tfor (unsigned int i = 0; i < numValues; ++i)\n> +\t\tvalues_.push_back(values[i]);\n> +}\n\nI was puzzled by your earlier report regarding usage of Span here so I\nhad a look. Would the following make sense ?\n\ndiff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\nindex 011c5867045b..3d7f4b0dc1a7 100644\n--- a/include/libcamera/controls.h\n+++ b/include/libcamera/controls.h\n@@ -269,9 +269,8 @@ public:\n \texplicit ControlInfo(const ControlValue &min = 0,\n \t\t\t     const ControlValue &max = 0,\n \t\t\t     const ControlValue &def = 0);\n-\texplicit ControlInfo(const ControlValue *values, unsigned int numValues,\n-\t\t\t     const ControlValue &def);\n-\texplicit ControlInfo(const ControlValue *values, unsigned int numValues);\n+\texplicit ControlInfo(Span<const ControlValue> values,\n+\t\t\t     const ControlValue &def = {});\n\n \tconst ControlValue &min() const { return min_; }\n \tconst ControlValue &max() const { return max_; }\ndiff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\nindex 1d3827404966..78ccb90eae2d 100644\n--- a/include/libcamera/ipa/raspberrypi.h\n+++ b/include/libcamera/ipa/raspberrypi.h\n@@ -50,13 +50,13 @@ static const ControlInfoMap Controls = {\n \t{ &controls::AeEnable, ControlInfo(false, true) },\n \t{ &controls::ExposureTime, ControlInfo(0, 999999) },\n \t{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n-\t{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues.data(), controls::AeMeteringModeValues.size()) },\n-\t{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues.data(), controls::AeConstraintModeValues.size()) },\n-\t{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues.data(), controls::AeExposureModeValues.size()) },\n+\t{ &controls::AeMeteringMode, ControlInfo(Span<const ControlValue>(controls::AeMeteringModeValues)) },\n+\t{ &controls::AeConstraintMode, ControlInfo(Span<const ControlValue>(controls::AeConstraintModeValues)) },\n+\t{ &controls::AeExposureMode, ControlInfo(Span<const ControlValue>(controls::AeExposureModeValues)) },\n \t{ &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },\n \t{ &controls::AwbEnable, ControlInfo(false, true) },\n \t{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n-\t{ &controls::AwbMode, ControlInfo(controls::AwbModeValues.data(), controls::AwbModeValues.size()) },\n+\t{ &controls::AwbMode, ControlInfo(Span<const ControlValue>(controls::AwbModeValues)) },\n \t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n \t{ &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n \t{ &controls::Saturation, ControlInfo(0.0f, 32.0f) },\ndiff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\nindex 2a2f04a598b6..aad461121651 100644\n--- a/src/libcamera/controls.cpp\n+++ b/src/libcamera/controls.cpp\n@@ -491,40 +491,26 @@ ControlInfo::ControlInfo(const ControlValue &min,\n {\n }\n\n-/**\n- * \\brief Construct a ControlInfo from the list of values values and a default\n- * \\param[in] values The control valid values\n- * \\param[in] numValues The number or valid values\n- * \\param[in] def The control default value\n- *\n- * Construct a ControlInfo from a list of valid values. The ControlInfo\n- * minimum and maximum values are assigned to the first and last members of\n- * the values list respectively.\n- */\n-ControlInfo::ControlInfo(const ControlValue *values, unsigned int numValues,\n-\t\t\t const ControlValue &def)\n-\t: def_(def)\n-{\n-\tmin_ = values[0];\n-\tmax_ = values[numValues - 1];\n-\n-\tfor (unsigned int i = 0; i < numValues; ++i)\n-\t\tvalues_.push_back(values[i]);\n-}\n-\n /**\n  * \\brief Construct a ControlInfo from the list of valid values\n  * \\param[in] values The control valid values\n- * \\param[in] numValues The number or valid values\n+ * \\param[in] def The control default value\n  *\n  * Construct a ControlInfo from a list of valid values. The ControlInfo\n- * minimum and maximum values are assigned to the first and last members of\n- * the values list respectively. The ControlInfo default value is set to be\n- * equal to the minimum one.\n+ * minimum and maximum values are set to the first and last members of the\n+ * values list respectively. The default value is set to \\a def if provided, or\n+ * to the minimum value otherwise.\n  */\n-ControlInfo::ControlInfo(const ControlValue *values, unsigned int numValues)\n-\t: ControlInfo(values, numValues, values[0])\n+ControlInfo::ControlInfo(Span<const ControlValue> values,\n+\t\t\t const ControlValue &def)\n {\n+\tmin_ = values.front();\n+\tmax_ = values.back();\n+\tdef_ = !def.isNone() ? def : values.front();\n+\n+\tvalues_.reserve(values.size());\n+\tfor (const ControlValue &value : values)\n+\t\tvalues_.push_back(value);\n }\n\n /**\n\nI like the ControlInfo::ControlInfo() constructor better, but I'm not\ntoo fond of the explicit Span constructor in\ninclude/libcamera/ipa/raspberrypi.h. Without that, relying on the Span\nimplicit constructor, the compiler complains:\n\nIn file included from ../../src/ipa/raspberrypi/raspberrypi.cpp:21:\n../../include/libcamera/ipa/raspberrypi.h:53:31: error: call to constructor of 'libcamera::ControlInfo' is ambiguous\n        { &controls::AeMeteringMode, ControlInfo{controls::AeMeteringModeValues} },\n                                     ^          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n../../include/libcamera/controls.h:269:11: note: candidate constructor\n        explicit ControlInfo(const ControlValue &min = 0,\n                 ^\n../../include/libcamera/controls.h:272:11: note: candidate constructor\n        explicit ControlInfo(Span<const ControlValue> values,\n\n(that's why clang, gcc is more cryptic)\n\nInvestigating a bit more, it can be fixed with\n\ndiff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\nindex 011c5867045b..3b7f3347761e 100644\n--- a/include/libcamera/controls.h\n+++ b/include/libcamera/controls.h\n@@ -97,6 +97,7 @@ public:\n\n #ifndef __DOXYGEN__\n \ttemplate<typename T, typename std::enable_if_t<!details::is_span<T>::value &&\n+\t\t\t\t\t\t       details::control_type<T>::value &&\n \t\t\t\t\t\t       !std::is_same<std::string, std::remove_cv_t<T>>::value,\n \t\t\t\t\t\t       std::nullptr_t> = nullptr>\n \tControlValue(const T &value)\n\nI'll submit this change as a patch as I think it makes sense on its own,\ndo you think the above changes could be integrated in the next version\nof this series ?\n\n> +\n> +/**\n> + * \\brief Construct a ControlInfo from the list of valid values\n> + * \\param[in] values The control valid values\n> + * \\param[in] numValues The number or valid values\n> + *\n> + * Construct a ControlInfo from a list of valid values. The ControlInfo\n> + * minimum and maximum values are assigned to the first and last members of\n> + * the values list respectively. The ControlInfo default value is set to be\n> + * equal to the minimum one.\n> + */\n> +ControlInfo::ControlInfo(const ControlValue *values, unsigned int numValues)\n> +\t: ControlInfo(values, numValues, values[0])\n> +{\n> +}\n> +\n>  /**\n>   * \\fn ControlInfo::min()\n>   * \\brief Retrieve the minimum value of the control\n> @@ -519,6 +555,17 @@ ControlInfo::ControlInfo(const ControlValue &min,\n>   * \\return A ControlValue with the default value for the control\n>   */\n>  \n> +/**\n> + * \\fn ControlInfo::values()\n> + * \\brief Retrieve the list of valid values\n> + *\n> + * For controls that support a pre-defined number of values, the enumeration of\n> + * those is reported through a vector of ControlValue instances accessible with\n> + * this method.\n> + *\n> + * \\return A vector of ControlValue representing the control valid values\n> + */\n> +\n>  /**\n>   * \\brief Provide a string representation of the ControlInfo\n>   */","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 2FCD4C3D3C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 23 Oct 2020 21:01:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BD10A61417;\n\tFri, 23 Oct 2020 23:01:13 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 37C87603C1\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 23 Oct 2020 23:01:12 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AA504B26;\n\tFri, 23 Oct 2020 23:01:11 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"bavB/Quv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1603486871;\n\tbh=h3TgpQ0tN2Odu7NjABnEsQODQN0RBhvBsOos0ginpFE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bavB/QuvWm/FaiJBfo7DgrLKVBv0tMhtatDgaeBweijrSHf1N59iuYNEhyOT+eFOg\n\t6k/V+cyIpKOW9WCnU/ZORYGjW5dXmfT0jyLzQCQwRPg6OTS3uf2G7jL9KeSMAi00sJ\n\tE7rJMtGO5NnsO93UiCYMVzDV8Ubmd2HOYoJcO5LE=","Date":"Sat, 24 Oct 2020 00:00:25 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<20201023210025.GG5979@pendragon.ideasonboard.com>","References":"<20201023171116.24899-1-jacopo@jmondi.org>\n\t<20201023171116.24899-4-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201023171116.24899-4-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v4 03/12] libcamera: controls:\n\tConstruct from valid values","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]