[{"id":24324,"web_url":"https://patchwork.libcamera.org/comment/24324/","msgid":"","date":"2022-08-03T09:27:52","subject":"Re: [libcamera-devel] [PATCH] libcamera: yaml_parser: Drop\n\tstd::optional<> from getList()","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 Wed, Aug 03, 2022 at 11:15:02AM +0200, Jacopo Mondi wrote:\n> The YamlParser::getList() function returns an\n> std::optional>.\n> \n> The obvious (and only) gain in using std::optional<> is to be able to\n> check the validity of the returned value with the dereference operator.\n> \n> However, usage of std::vector as the std::optional<> template\n> argument causes issues with the usage of utils::defopt with gcc8, as the\n> it fails to construct a vector with defopt:\n> \n> /usr/include/c++/8/optional:1267:8: error: call of overloaded\n> ‘vector(const libcamera::utils::details::defopt_t&)’ is ambiguous\n> : static_cast<_Tp>(std::forward<_Up>(__u))\n> \n> As the added value of std::optional<> is debatable when used in getList(),\n> drop it and return an std::vector directly. The few characters more\n> that have to be typed to check the validity of the return value\n> (value.empty() in place of just !value) are compensated by the shorter\n> assignment statement (value = getList() in place of value =\n> getList.value_or(utils::defopt))\n\nThat doesn't allow differentiating between an empty list (which may be\nvalid) and a list that can't be read correctly. Even if that's not\nneeded in the handful of callers we have now, I'd like to keep this\npossible in the API.\n\n> Reported-by: https://buildbot.libcamera.org/#/builders/6/builds/436\n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/internal/yaml_parser.h | 2 +-\n> src/ipa/raspberrypi/controller/rpi/agc.cpp | 10 +++++-----\n> src/ipa/rkisp1/algorithms/gsl.cpp | 8 ++++----\n> src/ipa/rkisp1/algorithms/lsc.cpp | 4 ++--\n> src/libcamera/yaml_parser.cpp | 18 +++++++++---------\n> test/yaml-parser.cpp | 2 +-\n> 6 files changed, 22 insertions(+), 22 deletions(-)\n> \n> diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h\n> index 5ba777d364fa..6d04c556f9b1 100644\n> --- a/include/libcamera/internal/yaml_parser.h\n> +++ b/include/libcamera/internal/yaml_parser.h\n> @@ -197,7 +197,7 @@ public:\n> #else\n> \ttemplate\n> #endif\n> -\tstd::optional> getList() const;\n> +\tstd::vector getList() const;\n> \n> \tDictAdapter asDict() const { return DictAdapter{ list_ }; }\n> \tListAdapter asList() const { return ListAdapter{ list_ }; }\n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> index bd54a639d637..bb15d3d84186 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> @@ -74,16 +74,16 @@ readMeteringModes(std::map &metering_modes,\n> \n> int AgcExposureMode::read(const libcamera::YamlObject ¶ms)\n> {\n> -\tauto value = params[\"shutter\"].getList();\n> -\tif (!value)\n> +\tstd::vector value = params[\"shutter\"].getList();\n> +\tif (value.empty())\n> \t\treturn -EINVAL;\n> -\tstd::transform(value->begin(), value->end(), std::back_inserter(shutter),\n> +\tstd::transform(value.begin(), value.end(), std::back_inserter(shutter),\n> \t\t [](double v) { return v * 1us; });\n> \n> \tvalue = params[\"gain\"].getList();\n> -\tif (!value)\n> +\tif (value.empty())\n> \t\treturn -EINVAL;\n> -\tgain = std::move(*value);\n> +\tgain = std::move(value);\n> \n> \tif (shutter.size() < 2 || gain.size() < 2) {\n> \t\tLOG(RPiAgc, Error)\n> diff --git a/src/ipa/rkisp1/algorithms/gsl.cpp b/src/ipa/rkisp1/algorithms/gsl.cpp\n> index 2fd1a23d3a9b..0cff1b77b058 100644\n> --- a/src/ipa/rkisp1/algorithms/gsl.cpp\n> +++ b/src/ipa/rkisp1/algorithms/gsl.cpp\n> @@ -60,7 +60,7 @@ int GammaSensorLinearization::init([[maybe_unused]] IPAContext &context,\n> \t\t\t\t const YamlObject &tuningData)\n> {\n> \tstd::vector xIntervals =\n> -\t\ttuningData[\"x-intervals\"].getList().value_or(utils::defopt);\n> +\t\ttuningData[\"x-intervals\"].getList();\n> \tif (xIntervals.size() != kDegammaXIntervals) {\n> \t\tLOG(RkISP1Gsl, Error)\n> \t\t\t<< \"Invalid 'x' coordinates: expected \"\n> @@ -84,7 +84,7 @@ int GammaSensorLinearization::init([[maybe_unused]] IPAContext &context,\n> \t\treturn -EINVAL;\n> \t}\n> \n> -\tcurveYr_ = yObject[\"red\"].getList().value_or(utils::defopt);\n> +\tcurveYr_ = yObject[\"red\"].getList();\n> \tif (curveYr_.size() != RKISP1_CIF_ISP_DEGAMMA_CURVE_SIZE) {\n> \t\tLOG(RkISP1Gsl, Error)\n> \t\t\t<< \"Invalid 'y:red' coordinates: expected \"\n> @@ -93,7 +93,7 @@ int GammaSensorLinearization::init([[maybe_unused]] IPAContext &context,\n> \t\treturn -EINVAL;\n> \t}\n> \n> -\tcurveYg_ = yObject[\"green\"].getList().value_or(utils::defopt);\n> +\tcurveYg_ = yObject[\"green\"].getList();\n> \tif (curveYg_.size() != RKISP1_CIF_ISP_DEGAMMA_CURVE_SIZE) {\n> \t\tLOG(RkISP1Gsl, Error)\n> \t\t\t<< \"Invalid 'y:green' coordinates: expected \"\n> @@ -102,7 +102,7 @@ int GammaSensorLinearization::init([[maybe_unused]] IPAContext &context,\n> \t\treturn -EINVAL;\n> \t}\n> \n> -\tcurveYb_ = yObject[\"blue\"].getList().value_or(utils::defopt);\n> +\tcurveYb_ = yObject[\"blue\"].getList();\n> \tif (curveYb_.size() != RKISP1_CIF_ISP_DEGAMMA_CURVE_SIZE) {\n> \t\tLOG(RkISP1Gsl, Error)\n> \t\t\t<< \"Invalid 'y:blue' coordinates: expected \"\n> diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp\n> index 05c8c0dab5c8..d1a4332c9089 100644\n> --- a/src/ipa/rkisp1/algorithms/lsc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/lsc.cpp\n> @@ -43,7 +43,7 @@ static std::vector parseSizes(const YamlObject &tuningData,\n> \t\t\t\t const char *prop)\n> {\n> \tstd::vector sizes =\n> -\t\ttuningData[prop].getList().value_or(utils::defopt);\n> +\t\ttuningData[prop].getList();\n> \tif (sizes.size() != RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE) {\n> \t\tLOG(RkISP1Lsc, Error)\n> \t\t\t<< \"Invalid '\" << prop << \"' values: expected \"\n> @@ -76,7 +76,7 @@ static std::vector parseTable(const YamlObject &tuningData,\n> \t\tRKISP1_CIF_ISP_LSC_SAMPLES_MAX * RKISP1_CIF_ISP_LSC_SAMPLES_MAX;\n> \n> \tstd::vector table =\n> -\t\ttuningData[prop].getList().value_or(utils::defopt);\n> +\t\ttuningData[prop].getList();\n> \tif (table.size() != kLscNumSamples) {\n> \t\tLOG(RkISP1Lsc, Error)\n> \t\t\t<< \"Invalid '\" << prop << \"' values: expected \"\n> diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp\n> index 84cb57d6de83..d422e0811fa7 100644\n> --- a/src/libcamera/yaml_parser.cpp\n> +++ b/src/libcamera/yaml_parser.cpp\n> @@ -316,7 +316,7 @@ template \t\t std::is_same_v ||\n> \t\t std::is_same_v ||\n> \t\t std::is_same_v> *>\n> -std::optional> YamlObject::getList() const\n> +std::vector YamlObject::getList() const\n> {\n> \tif (type_ != Type::List)\n> \t\treturn {};\n> @@ -334,14 +334,14 @@ std::optional> YamlObject::getList() const\n> \treturn values;\n> }\n> \n> -template std::optional> YamlObject::getList() const;\n> -template std::optional> YamlObject::getList() const;\n> -template std::optional> YamlObject::getList() const;\n> -template std::optional> YamlObject::getList() const;\n> -template std::optional> YamlObject::getList() const;\n> -template std::optional> YamlObject::getList() const;\n> -template std::optional> YamlObject::getList() const;\n> -template std::optional> YamlObject::getList() const;\n> +template std::vector YamlObject::getList() const;\n> +template std::vector YamlObject::getList() const;\n> +template std::vector YamlObject::getList() const;\n> +template std::vector YamlObject::getList() const;\n> +template std::vector YamlObject::getList() const;\n> +template std::vector YamlObject::getList() const;\n> +template std::vector YamlObject::getList() const;\n> +template std::vector YamlObject::getList() const;\n> \n> #endif /* __DOXYGEN__ */\n> \n> diff --git a/test/yaml-parser.cpp b/test/yaml-parser.cpp\n> index 93ba88b8bcd5..e25944e7f6b8 100644\n> --- a/test/yaml-parser.cpp\n> +++ b/test/yaml-parser.cpp\n> @@ -530,7 +530,7 @@ protected:\n> \t\t}\n> \n> \t\tconst auto &values = firstElement.getList();\n> -\t\tif (!values || values->size() != 2 || (*values)[0] != 1 || (*values)[1] != 2) {\n> +\t\tif (values.empty() || values.size() != 2 || values[0] != 1 || values[1] != 2) {\n> \t\t\tcerr << \"getList() failed to return correct vector\" << std::endl;\n> \t\t\treturn TestFail;\n> \t\t}","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 3BFF4BE173\n\tfor ;\n\tWed, 3 Aug 2022 09:28:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B532463315;\n\tWed, 3 Aug 2022 11:27: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 1FCFE603EF\n\tfor ;\n\tWed, 3 Aug 2022 11:27:59 +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 594858B;\n\tWed, 3 Aug 2022 11:27:58 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659518879;\n\tbh=fOEgld2b7T90f28mffWxY7y3kxDXFuppsPt9e5AkmBI=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=Mw9/EOfeN9oO2q+Gn6SjTQ/e55yBr8Pa2mOb2kTr7+IpZAILHyyfO1SAlhYyrzfwR\n\tr/BbnHfxSRueABwp7EoM1Q0hvl2Z9JX3W4LEaQzYt30qV0yH25sAmhtBv8uJLlbEWh\n\tmz+8ubKznH2SUusaEjzSQ2+wf341QwX0Qg1FAIzSq88LJU+DaJmHGOcFMQVaF+A5JN\n\td5wmDGSH1tqa6EUzQDgtRqcfsYTGcNdyGPfjCXeOSfEt8l3h3MT0PCrprvoM94XbF0\n\tdMjrRQPLS6/ft31NmVhjB7Gq7f9Vejf/6J3nATzEX9UytT6y1dfLXW+vkJHoja2bl1\n\tPzKbe0xNB6CrQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1659518878;\n\tbh=fOEgld2b7T90f28mffWxY7y3kxDXFuppsPt9e5AkmBI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jyZMXcxW/5r9VJBUmnhrHllCtc+wFI7l3lUI8SrpQWnyPSfoDBZ8vtGjNLB1ymmbo\n\tUVDdftxYz4rajs42o61044TMeRZHDFgz9u3XxGmeEDZqft/nV9y8QGf7bbsJTj4fWP\n\tdnvqwfSIg9iBy6YnA6k/NX9KEMkE35qFpQcx8GXo="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"jyZMXcxW\"; dkim-atps=neutral","Date":"Wed, 3 Aug 2022 12:27:52 +0300","To":"Jacopo Mondi ","Message-ID":"","References":"<20220803091502.17280-1-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20220803091502.17280-1-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: yaml_parser: Drop\n\tstd::optional<> from getList()","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24325,"web_url":"https://patchwork.libcamera.org/comment/24325/","msgid":"<20220803094257.rncvg3h5hh5hworx@uno.localdomain>","date":"2022-08-03T09:42:57","subject":"Re: [libcamera-devel] [PATCH] libcamera: yaml_parser: Drop\n\tstd::optional<> from getList()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"On Wed, Aug 03, 2022 at 12:27:52PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Wed, Aug 03, 2022 at 11:15:02AM +0200, Jacopo Mondi wrote:\n> > The YamlParser::getList() function returns an\n> > std::optional>.\n> >\n> > The obvious (and only) gain in using std::optional<> is to be able to\n> > check the validity of the returned value with the dereference operator.\n> >\n> > However, usage of std::vector as the std::optional<> template\n> > argument causes issues with the usage of utils::defopt with gcc8, as the\n> > it fails to construct a vector with defopt:\n> >\n> > /usr/include/c++/8/optional:1267:8: error: call of overloaded\n> > ‘vector(const libcamera::utils::details::defopt_t&)’ is ambiguous\n> > : static_cast<_Tp>(std::forward<_Up>(__u))\n> >\n> > As the added value of std::optional<> is debatable when used in getList(),\n> > drop it and return an std::vector directly. The few characters more\n> > that have to be typed to check the validity of the return value\n> > (value.empty() in place of just !value) are compensated by the shorter\n> > assignment statement (value = getList() in place of value =\n> > getList.value_or(utils::defopt))\n>\n> That doesn't allow differentiating between an empty list (which may be\n> valid) and a list that can't be read correctly. Even if that's not\n> needed in the handful of callers we have now, I'd like to keep this\n> possible in the API.\n>\n\nDidn't the fact that value_or() was used made it impossible to detect\nthat already ?\n\nAnd by the way, in case of errors parsing the YAML file the current\nimplementation returns {}, which is actually an empty list and not\nnullopt.\n\n> > Reported-by: https://buildbot.libcamera.org/#/builders/6/builds/436\n\nThis however should be addressed in case std::optional<> has to be\nkept in the interface.\n\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/internal/yaml_parser.h | 2 +-\n> > src/ipa/raspberrypi/controller/rpi/agc.cpp | 10 +++++-----\n> > src/ipa/rkisp1/algorithms/gsl.cpp | 8 ++++----\n> > src/ipa/rkisp1/algorithms/lsc.cpp | 4 ++--\n> > src/libcamera/yaml_parser.cpp | 18 +++++++++---------\n> > test/yaml-parser.cpp | 2 +-\n> > 6 files changed, 22 insertions(+), 22 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h\n> > index 5ba777d364fa..6d04c556f9b1 100644\n> > --- a/include/libcamera/internal/yaml_parser.h\n> > +++ b/include/libcamera/internal/yaml_parser.h\n> > @@ -197,7 +197,7 @@ public:\n> > #else\n> > \ttemplate\n> > #endif\n> > -\tstd::optional> getList() const;\n> > +\tstd::vector getList() const;\n> >\n> > \tDictAdapter asDict() const { return DictAdapter{ list_ }; }\n> > \tListAdapter asList() const { return ListAdapter{ list_ }; }\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > index bd54a639d637..bb15d3d84186 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > @@ -74,16 +74,16 @@ readMeteringModes(std::map &metering_modes,\n> >\n> > int AgcExposureMode::read(const libcamera::YamlObject ¶ms)\n> > {\n> > -\tauto value = params[\"shutter\"].getList();\n> > -\tif (!value)\n> > +\tstd::vector value = params[\"shutter\"].getList();\n> > +\tif (value.empty())\n> > \t\treturn -EINVAL;\n> > -\tstd::transform(value->begin(), value->end(), std::back_inserter(shutter),\n> > +\tstd::transform(value.begin(), value.end(), std::back_inserter(shutter),\n> > \t\t [](double v) { return v * 1us; });\n> >\n> > \tvalue = params[\"gain\"].getList();\n> > -\tif (!value)\n> > +\tif (value.empty())\n> > \t\treturn -EINVAL;\n> > -\tgain = std::move(*value);\n> > +\tgain = std::move(value);\n> >\n> > \tif (shutter.size() < 2 || gain.size() < 2) {\n> > \t\tLOG(RPiAgc, Error)\n> > diff --git a/src/ipa/rkisp1/algorithms/gsl.cpp b/src/ipa/rkisp1/algorithms/gsl.cpp\n> > index 2fd1a23d3a9b..0cff1b77b058 100644\n> > --- a/src/ipa/rkisp1/algorithms/gsl.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/gsl.cpp\n> > @@ -60,7 +60,7 @@ int GammaSensorLinearization::init([[maybe_unused]] IPAContext &context,\n> > \t\t\t\t const YamlObject &tuningData)\n> > {\n> > \tstd::vector xIntervals =\n> > -\t\ttuningData[\"x-intervals\"].getList().value_or(utils::defopt);\n> > +\t\ttuningData[\"x-intervals\"].getList();\n> > \tif (xIntervals.size() != kDegammaXIntervals) {\n> > \t\tLOG(RkISP1Gsl, Error)\n> > \t\t\t<< \"Invalid 'x' coordinates: expected \"\n> > @@ -84,7 +84,7 @@ int GammaSensorLinearization::init([[maybe_unused]] IPAContext &context,\n> > \t\treturn -EINVAL;\n> > \t}\n> >\n> > -\tcurveYr_ = yObject[\"red\"].getList().value_or(utils::defopt);\n> > +\tcurveYr_ = yObject[\"red\"].getList();\n> > \tif (curveYr_.size() != RKISP1_CIF_ISP_DEGAMMA_CURVE_SIZE) {\n> > \t\tLOG(RkISP1Gsl, Error)\n> > \t\t\t<< \"Invalid 'y:red' coordinates: expected \"\n> > @@ -93,7 +93,7 @@ int GammaSensorLinearization::init([[maybe_unused]] IPAContext &context,\n> > \t\treturn -EINVAL;\n> > \t}\n> >\n> > -\tcurveYg_ = yObject[\"green\"].getList().value_or(utils::defopt);\n> > +\tcurveYg_ = yObject[\"green\"].getList();\n> > \tif (curveYg_.size() != RKISP1_CIF_ISP_DEGAMMA_CURVE_SIZE) {\n> > \t\tLOG(RkISP1Gsl, Error)\n> > \t\t\t<< \"Invalid 'y:green' coordinates: expected \"\n> > @@ -102,7 +102,7 @@ int GammaSensorLinearization::init([[maybe_unused]] IPAContext &context,\n> > \t\treturn -EINVAL;\n> > \t}\n> >\n> > -\tcurveYb_ = yObject[\"blue\"].getList().value_or(utils::defopt);\n> > +\tcurveYb_ = yObject[\"blue\"].getList();\n> > \tif (curveYb_.size() != RKISP1_CIF_ISP_DEGAMMA_CURVE_SIZE) {\n> > \t\tLOG(RkISP1Gsl, Error)\n> > \t\t\t<< \"Invalid 'y:blue' coordinates: expected \"\n> > diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp\n> > index 05c8c0dab5c8..d1a4332c9089 100644\n> > --- a/src/ipa/rkisp1/algorithms/lsc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/lsc.cpp\n> > @@ -43,7 +43,7 @@ static std::vector parseSizes(const YamlObject &tuningData,\n> > \t\t\t\t const char *prop)\n> > {\n> > \tstd::vector sizes =\n> > -\t\ttuningData[prop].getList().value_or(utils::defopt);\n> > +\t\ttuningData[prop].getList();\n> > \tif (sizes.size() != RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE) {\n> > \t\tLOG(RkISP1Lsc, Error)\n> > \t\t\t<< \"Invalid '\" << prop << \"' values: expected \"\n> > @@ -76,7 +76,7 @@ static std::vector parseTable(const YamlObject &tuningData,\n> > \t\tRKISP1_CIF_ISP_LSC_SAMPLES_MAX * RKISP1_CIF_ISP_LSC_SAMPLES_MAX;\n> >\n> > \tstd::vector table =\n> > -\t\ttuningData[prop].getList().value_or(utils::defopt);\n> > +\t\ttuningData[prop].getList();\n> > \tif (table.size() != kLscNumSamples) {\n> > \t\tLOG(RkISP1Lsc, Error)\n> > \t\t\t<< \"Invalid '\" << prop << \"' values: expected \"\n> > diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp\n> > index 84cb57d6de83..d422e0811fa7 100644\n> > --- a/src/libcamera/yaml_parser.cpp\n> > +++ b/src/libcamera/yaml_parser.cpp\n> > @@ -316,7 +316,7 @@ template > \t\t std::is_same_v ||\n> > \t\t std::is_same_v ||\n> > \t\t std::is_same_v> *>\n> > -std::optional> YamlObject::getList() const\n> > +std::vector YamlObject::getList() const\n> > {\n> > \tif (type_ != Type::List)\n> > \t\treturn {};\n> > @@ -334,14 +334,14 @@ std::optional> YamlObject::getList() const\n> > \treturn values;\n> > }\n> >\n> > -template std::optional> YamlObject::getList() const;\n> > -template std::optional> YamlObject::getList() const;\n> > -template std::optional> YamlObject::getList() const;\n> > -template std::optional> YamlObject::getList() const;\n> > -template std::optional> YamlObject::getList() const;\n> > -template std::optional> YamlObject::getList() const;\n> > -template std::optional> YamlObject::getList() const;\n> > -template std::optional> YamlObject::getList() const;\n> > +template std::vector YamlObject::getList() const;\n> > +template std::vector YamlObject::getList() const;\n> > +template std::vector YamlObject::getList() const;\n> > +template std::vector YamlObject::getList() const;\n> > +template std::vector YamlObject::getList() const;\n> > +template std::vector YamlObject::getList() const;\n> > +template std::vector YamlObject::getList() const;\n> > +template std::vector YamlObject::getList() const;\n> >\n> > #endif /* __DOXYGEN__ */\n> >\n> > diff --git a/test/yaml-parser.cpp b/test/yaml-parser.cpp\n> > index 93ba88b8bcd5..e25944e7f6b8 100644\n> > --- a/test/yaml-parser.cpp\n> > +++ b/test/yaml-parser.cpp\n> > @@ -530,7 +530,7 @@ protected:\n> > \t\t}\n> >\n> > \t\tconst auto &values = firstElement.getList();\n> > -\t\tif (!values || values->size() != 2 || (*values)[0] != 1 || (*values)[1] != 2) {\n> > +\t\tif (values.empty() || values.size() != 2 || values[0] != 1 || values[1] != 2) {\n> > \t\t\tcerr << \"getList() failed to return correct vector\" << std::endl;\n> > \t\t\treturn TestFail;\n> > \t\t}\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 D2C39C3272\n\tfor ;\n\tWed, 3 Aug 2022 09:43:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3419B63315;\n\tWed, 3 Aug 2022 11:43:02 +0200 (CEST)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::227])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C17D9603EF\n\tfor ;\n\tWed, 3 Aug 2022 11:43:00 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id E652A20003;\n\tWed, 3 Aug 2022 09:42:58 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659519782;\n\tbh=QoEZUyQ3iz2FPpGE8lnWoN2Wqu1q7FBmwtoCmA4OpZQ=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=2wy1BJM5e4UIi/7in+NguNhpqQunNue6on5NSCw2fKZcF8QzST/NWV7GJ3MTK0wPN\n\t3XaqpU1LxMMabnaY0rAvaQtDWT1XVp3UONynj+Ahlmaky5kYEnBQNmtcm0WQfRYLpc\n\ts9S6nD4jok2Fq0imKzrp5c8yEs9zVs7keNfF4C2P0rT0PKJM/uayprRG8JESbpoagc\n\tuzeOeozs6hliOYbqWwGteZW0meFxe+ISHJFzpxEpn0B+avhecLID//EJ5/H1DwhFxS\n\tbJi+nnIO+MnsEBeEznSTwGa1wKqemT9hgtNHzTxs6Fi9eJqxmP2OkY17dvNdC+O8+s\n\tfgVMVyt9FGcyw==","Date":"Wed, 3 Aug 2022 11:42:57 +0200","To":"Laurent Pinchart ","Message-ID":"<20220803094257.rncvg3h5hh5hworx@uno.localdomain>","References":"<20220803091502.17280-1-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: yaml_parser: Drop\n\tstd::optional<> from getList()","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24328,"web_url":"https://patchwork.libcamera.org/comment/24328/","msgid":"","date":"2022-08-03T10:21:56","subject":"Re: [libcamera-devel] [PATCH] libcamera: yaml_parser: Drop\n\tstd::optional<> from getList()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Aug 03, 2022 at 11:42:57AM +0200, Jacopo Mondi wrote:\n> On Wed, Aug 03, 2022 at 12:27:52PM +0300, Laurent Pinchart wrote:\n> > On Wed, Aug 03, 2022 at 11:15:02AM +0200, Jacopo Mondi wrote:\n> > > The YamlParser::getList() function returns an\n> > > std::optional>.\n> > >\n> > > The obvious (and only) gain in using std::optional<> is to be able to\n> > > check the validity of the returned value with the dereference operator.\n> > >\n> > > However, usage of std::vector as the std::optional<> template\n> > > argument causes issues with the usage of utils::defopt with gcc8, as the\n> > > it fails to construct a vector with defopt:\n> > >\n> > > /usr/include/c++/8/optional:1267:8: error: call of overloaded\n> > > ‘vector(const libcamera::utils::details::defopt_t&)’ is ambiguous\n> > > : static_cast<_Tp>(std::forward<_Up>(__u))\n> > >\n> > > As the added value of std::optional<> is debatable when used in getList(),\n> > > drop it and return an std::vector directly. The few characters more\n> > > that have to be typed to check the validity of the return value\n> > > (value.empty() in place of just !value) are compensated by the shorter\n> > > assignment statement (value = getList() in place of value =\n> > > getList.value_or(utils::defopt))\n> >\n> > That doesn't allow differentiating between an empty list (which may be\n> > valid) and a list that can't be read correctly. Even if that's not\n> > needed in the handful of callers we have now, I'd like to keep this\n> > possible in the API.\n> \n> Didn't the fact that value_or() was used made it impossible to detect\n> that already ?\n\nThe existing callers expect a list of a fixed size, so they use\nvalue_or() and check the size of the vector. A caller that would need to\ndifferentiate the twp would do\n\nvoid doSomethingWithList(const std::vector &values)\n{\n\t...\n}\n\n{\n\t...\n\n\tstd::optional> value = yaml.getList();\n\tif (!value)\n\t\t/* Error handling */\n\n\tdoSomethingWithList(*value);\n\n\t...\n}\n\n> And by the way, in case of errors parsing the YAML file the current\n> implementation returns {}, which is actually an empty list and not\n> nullopt.\n\nThat's wrong indeed, my bad. Would you like to send a patch to fix that,\nor should I ?\n\n> > > Reported-by: https://buildbot.libcamera.org/#/builders/6/builds/436\n> \n> This however should be addressed in case std::optional<> has to be\n> kept in the interface.\n\nYes, I agree.\n\nCan you reproduce the issue locally ?\n\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > include/libcamera/internal/yaml_parser.h | 2 +-\n> > > src/ipa/raspberrypi/controller/rpi/agc.cpp | 10 +++++-----\n> > > src/ipa/rkisp1/algorithms/gsl.cpp | 8 ++++----\n> > > src/ipa/rkisp1/algorithms/lsc.cpp | 4 ++--\n> > > src/libcamera/yaml_parser.cpp | 18 +++++++++---------\n> > > test/yaml-parser.cpp | 2 +-\n> > > 6 files changed, 22 insertions(+), 22 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h\n> > > index 5ba777d364fa..6d04c556f9b1 100644\n> > > --- a/include/libcamera/internal/yaml_parser.h\n> > > +++ b/include/libcamera/internal/yaml_parser.h\n> > > @@ -197,7 +197,7 @@ public:\n> > > #else\n> > > \ttemplate\n> > > #endif\n> > > -\tstd::optional> getList() const;\n> > > +\tstd::vector getList() const;\n> > >\n> > > \tDictAdapter asDict() const { return DictAdapter{ list_ }; }\n> > > \tListAdapter asList() const { return ListAdapter{ list_ }; }\n> > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > > index bd54a639d637..bb15d3d84186 100644\n> > > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > > @@ -74,16 +74,16 @@ readMeteringModes(std::map &metering_modes,\n> > >\n> > > int AgcExposureMode::read(const libcamera::YamlObject ¶ms)\n> > > {\n> > > -\tauto value = params[\"shutter\"].getList();\n> > > -\tif (!value)\n> > > +\tstd::vector value = params[\"shutter\"].getList();\n> > > +\tif (value.empty())\n> > > \t\treturn -EINVAL;\n> > > -\tstd::transform(value->begin(), value->end(), std::back_inserter(shutter),\n> > > +\tstd::transform(value.begin(), value.end(), std::back_inserter(shutter),\n> > > \t\t [](double v) { return v * 1us; });\n> > >\n> > > \tvalue = params[\"gain\"].getList();\n> > > -\tif (!value)\n> > > +\tif (value.empty())\n> > > \t\treturn -EINVAL;\n> > > -\tgain = std::move(*value);\n> > > +\tgain = std::move(value);\n> > >\n> > > \tif (shutter.size() < 2 || gain.size() < 2) {\n> > > \t\tLOG(RPiAgc, Error)\n> > > diff --git a/src/ipa/rkisp1/algorithms/gsl.cpp b/src/ipa/rkisp1/algorithms/gsl.cpp\n> > > index 2fd1a23d3a9b..0cff1b77b058 100644\n> > > --- a/src/ipa/rkisp1/algorithms/gsl.cpp\n> > > +++ b/src/ipa/rkisp1/algorithms/gsl.cpp\n> > > @@ -60,7 +60,7 @@ int GammaSensorLinearization::init([[maybe_unused]] IPAContext &context,\n> > > \t\t\t\t const YamlObject &tuningData)\n> > > {\n> > > \tstd::vector xIntervals =\n> > > -\t\ttuningData[\"x-intervals\"].getList().value_or(utils::defopt);\n> > > +\t\ttuningData[\"x-intervals\"].getList();\n> > > \tif (xIntervals.size() != kDegammaXIntervals) {\n> > > \t\tLOG(RkISP1Gsl, Error)\n> > > \t\t\t<< \"Invalid 'x' coordinates: expected \"\n> > > @@ -84,7 +84,7 @@ int GammaSensorLinearization::init([[maybe_unused]] IPAContext &context,\n> > > \t\treturn -EINVAL;\n> > > \t}\n> > >\n> > > -\tcurveYr_ = yObject[\"red\"].getList().value_or(utils::defopt);\n> > > +\tcurveYr_ = yObject[\"red\"].getList();\n> > > \tif (curveYr_.size() != RKISP1_CIF_ISP_DEGAMMA_CURVE_SIZE) {\n> > > \t\tLOG(RkISP1Gsl, Error)\n> > > \t\t\t<< \"Invalid 'y:red' coordinates: expected \"\n> > > @@ -93,7 +93,7 @@ int GammaSensorLinearization::init([[maybe_unused]] IPAContext &context,\n> > > \t\treturn -EINVAL;\n> > > \t}\n> > >\n> > > -\tcurveYg_ = yObject[\"green\"].getList().value_or(utils::defopt);\n> > > +\tcurveYg_ = yObject[\"green\"].getList();\n> > > \tif (curveYg_.size() != RKISP1_CIF_ISP_DEGAMMA_CURVE_SIZE) {\n> > > \t\tLOG(RkISP1Gsl, Error)\n> > > \t\t\t<< \"Invalid 'y:green' coordinates: expected \"\n> > > @@ -102,7 +102,7 @@ int GammaSensorLinearization::init([[maybe_unused]] IPAContext &context,\n> > > \t\treturn -EINVAL;\n> > > \t}\n> > >\n> > > -\tcurveYb_ = yObject[\"blue\"].getList().value_or(utils::defopt);\n> > > +\tcurveYb_ = yObject[\"blue\"].getList();\n> > > \tif (curveYb_.size() != RKISP1_CIF_ISP_DEGAMMA_CURVE_SIZE) {\n> > > \t\tLOG(RkISP1Gsl, Error)\n> > > \t\t\t<< \"Invalid 'y:blue' coordinates: expected \"\n> > > diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp\n> > > index 05c8c0dab5c8..d1a4332c9089 100644\n> > > --- a/src/ipa/rkisp1/algorithms/lsc.cpp\n> > > +++ b/src/ipa/rkisp1/algorithms/lsc.cpp\n> > > @@ -43,7 +43,7 @@ static std::vector parseSizes(const YamlObject &tuningData,\n> > > \t\t\t\t const char *prop)\n> > > {\n> > > \tstd::vector sizes =\n> > > -\t\ttuningData[prop].getList().value_or(utils::defopt);\n> > > +\t\ttuningData[prop].getList();\n> > > \tif (sizes.size() != RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE) {\n> > > \t\tLOG(RkISP1Lsc, Error)\n> > > \t\t\t<< \"Invalid '\" << prop << \"' values: expected \"\n> > > @@ -76,7 +76,7 @@ static std::vector parseTable(const YamlObject &tuningData,\n> > > \t\tRKISP1_CIF_ISP_LSC_SAMPLES_MAX * RKISP1_CIF_ISP_LSC_SAMPLES_MAX;\n> > >\n> > > \tstd::vector table =\n> > > -\t\ttuningData[prop].getList().value_or(utils::defopt);\n> > > +\t\ttuningData[prop].getList();\n> > > \tif (table.size() != kLscNumSamples) {\n> > > \t\tLOG(RkISP1Lsc, Error)\n> > > \t\t\t<< \"Invalid '\" << prop << \"' values: expected \"\n> > > diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp\n> > > index 84cb57d6de83..d422e0811fa7 100644\n> > > --- a/src/libcamera/yaml_parser.cpp\n> > > +++ b/src/libcamera/yaml_parser.cpp\n> > > @@ -316,7 +316,7 @@ template > > \t\t std::is_same_v ||\n> > > \t\t std::is_same_v ||\n> > > \t\t std::is_same_v> *>\n> > > -std::optional> YamlObject::getList() const\n> > > +std::vector YamlObject::getList() const\n> > > {\n> > > \tif (type_ != Type::List)\n> > > \t\treturn {};\n> > > @@ -334,14 +334,14 @@ std::optional> YamlObject::getList() const\n> > > \treturn values;\n> > > }\n> > >\n> > > -template std::optional> YamlObject::getList() const;\n> > > -template std::optional> YamlObject::getList() const;\n> > > -template std::optional> YamlObject::getList() const;\n> > > -template std::optional> YamlObject::getList() const;\n> > > -template std::optional> YamlObject::getList() const;\n> > > -template std::optional> YamlObject::getList() const;\n> > > -template std::optional> YamlObject::getList() const;\n> > > -template std::optional> YamlObject::getList() const;\n> > > +template std::vector YamlObject::getList() const;\n> > > +template std::vector YamlObject::getList() const;\n> > > +template std::vector YamlObject::getList() const;\n> > > +template std::vector YamlObject::getList() const;\n> > > +template std::vector YamlObject::getList() const;\n> > > +template std::vector YamlObject::getList() const;\n> > > +template std::vector YamlObject::getList() const;\n> > > +template std::vector YamlObject::getList() const;\n> > >\n> > > #endif /* __DOXYGEN__ */\n> > >\n> > > diff --git a/test/yaml-parser.cpp b/test/yaml-parser.cpp\n> > > index 93ba88b8bcd5..e25944e7f6b8 100644\n> > > --- a/test/yaml-parser.cpp\n> > > +++ b/test/yaml-parser.cpp\n> > > @@ -530,7 +530,7 @@ protected:\n> > > \t\t}\n> > >\n> > > \t\tconst auto &values = firstElement.getList();\n> > > -\t\tif (!values || values->size() != 2 || (*values)[0] != 1 || (*values)[1] != 2) {\n> > > +\t\tif (values.empty() || values.size() != 2 || values[0] != 1 || values[1] != 2) {\n> > > \t\t\tcerr << \"getList() failed to return correct vector\" << std::endl;\n> > > \t\t\treturn TestFail;\n> > > \t\t}","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 48DB5C3272\n\tfor ;\n\tWed, 3 Aug 2022 10:22:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A1A9461FAE;\n\tWed, 3 Aug 2022 12:22:04 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BC53A603E6\n\tfor ;\n\tWed, 3 Aug 2022 12:22:02 +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 103588B;\n\tWed, 3 Aug 2022 12:22:02 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659522124;\n\tbh=nP2RCAw82NJTwRtIbfAGiHp+144TwzsI9H8VA9f7Q9I=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=rc622D6QTI7iHep5J7Z72mnKdEw1S3ZcflafI0me99tS8OtSdTduAznMtCqXxN6uS\n\tTnBNw1e76T8zRT7IRZ9eZCnNv8VL1S7uy7M68d6m915M/O6uumZoTCQzVC8DEoaIiq\n\tRIs28Uby1CV98ppRPLFbXBy26eygFhALNtJqDI99R85EF+VJMEcezm7pQI5M7o+b6c\n\tRxnCufllM4sgwEP82eqW1ZZhJZZDk+P51YkJvbEBEH+NjnKzsx8fFJwhetrrmmHonC\n\tqwmxBrdaJCiDNxo7yhEL3XAm7Z1iwVJjFFYMvM2CGN/mDWbZQ42gOkOSovVXzXmCXP\n\tk8LOceZSdyvyA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1659522122;\n\tbh=nP2RCAw82NJTwRtIbfAGiHp+144TwzsI9H8VA9f7Q9I=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fmhYnSo/H8la+N9w2MtnlCRnbshnihRsTVjL0eruZmmwczmh+zUGs7EUGZ6mW00gI\n\tuI6hJKYcZBJNk1dsmf0GpOQX8DOmbHE/c6kWEYJjJ+UvJA+Hqg6TIM/8ofoaXLUe0Z\n\tHx8yFFmvGZ6Y2qfjpnKaJfxVtv5jqTboAzaOXPFA="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"fmhYnSo/\"; dkim-atps=neutral","Date":"Wed, 3 Aug 2022 13:21:56 +0300","To":"Jacopo Mondi ","Message-ID":"","References":"<20220803091502.17280-1-jacopo@jmondi.org>\n\t\n\t<20220803094257.rncvg3h5hh5hworx@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20220803094257.rncvg3h5hh5hworx@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: yaml_parser: Drop\n\tstd::optional<> from getList()","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]