[libcamera-devel] [PATCH] libcamera: yaml_parser: Drop std::optional<> from getList()
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Aug 3 11:27:52 CEST 2022
Hi Jacopo,
Thank you for the patch.
On Wed, Aug 03, 2022 at 11:15:02AM +0200, Jacopo Mondi wrote:
> The YamlParser::getList<T>() function returns an
> std::optional<std::vector<T>>.
>
> The obvious (and only) gain in using std::optional<> is to be able to
> check the validity of the returned value with the dereference operator.
>
> However, usage of std::vector<T> as the std::optional<> template
> argument causes issues with the usage of utils::defopt with gcc8, as the
> it fails to construct a vector with defopt:
>
> /usr/include/c++/8/optional:1267:8: error: call of overloaded
> ‘vector(const libcamera::utils::details::defopt_t&)’ is ambiguous
> : static_cast<_Tp>(std::forward<_Up>(__u))
>
> As the added value of std::optional<> is debatable when used in getList(),
> drop it and return an std::vector<T> directly. The few characters more
> that have to be typed to check the validity of the return value
> (value.empty() in place of just !value) are compensated by the shorter
> assignment statement (value = getList<T>() in place of value =
> getList<T>.value_or(utils::defopt))
That doesn't allow differentiating between an empty list (which may be
valid) and a list that can't be read correctly. Even if that's not
needed in the handful of callers we have now, I'd like to keep this
possible in the API.
> Reported-by: https://buildbot.libcamera.org/#/builders/6/builds/436
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> include/libcamera/internal/yaml_parser.h | 2 +-
> src/ipa/raspberrypi/controller/rpi/agc.cpp | 10 +++++-----
> src/ipa/rkisp1/algorithms/gsl.cpp | 8 ++++----
> src/ipa/rkisp1/algorithms/lsc.cpp | 4 ++--
> src/libcamera/yaml_parser.cpp | 18 +++++++++---------
> test/yaml-parser.cpp | 2 +-
> 6 files changed, 22 insertions(+), 22 deletions(-)
>
> diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h
> index 5ba777d364fa..6d04c556f9b1 100644
> --- a/include/libcamera/internal/yaml_parser.h
> +++ b/include/libcamera/internal/yaml_parser.h
> @@ -197,7 +197,7 @@ public:
> #else
> template<typename T>
> #endif
> - std::optional<std::vector<T>> getList() const;
> + std::vector<T> getList() const;
>
> DictAdapter asDict() const { return DictAdapter{ list_ }; }
> ListAdapter asList() const { return ListAdapter{ list_ }; }
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> index bd54a639d637..bb15d3d84186 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> @@ -74,16 +74,16 @@ readMeteringModes(std::map<std::string, AgcMeteringMode> &metering_modes,
>
> int AgcExposureMode::read(const libcamera::YamlObject ¶ms)
> {
> - auto value = params["shutter"].getList<double>();
> - if (!value)
> + std::vector<double> value = params["shutter"].getList<double>();
> + if (value.empty())
> return -EINVAL;
> - std::transform(value->begin(), value->end(), std::back_inserter(shutter),
> + std::transform(value.begin(), value.end(), std::back_inserter(shutter),
> [](double v) { return v * 1us; });
>
> value = params["gain"].getList<double>();
> - if (!value)
> + if (value.empty())
> return -EINVAL;
> - gain = std::move(*value);
> + gain = std::move(value);
>
> if (shutter.size() < 2 || gain.size() < 2) {
> LOG(RPiAgc, Error)
> diff --git a/src/ipa/rkisp1/algorithms/gsl.cpp b/src/ipa/rkisp1/algorithms/gsl.cpp
> index 2fd1a23d3a9b..0cff1b77b058 100644
> --- a/src/ipa/rkisp1/algorithms/gsl.cpp
> +++ b/src/ipa/rkisp1/algorithms/gsl.cpp
> @@ -60,7 +60,7 @@ int GammaSensorLinearization::init([[maybe_unused]] IPAContext &context,
> const YamlObject &tuningData)
> {
> std::vector<uint16_t> xIntervals =
> - tuningData["x-intervals"].getList<uint16_t>().value_or(utils::defopt);
> + tuningData["x-intervals"].getList<uint16_t>();
> if (xIntervals.size() != kDegammaXIntervals) {
> LOG(RkISP1Gsl, Error)
> << "Invalid 'x' coordinates: expected "
> @@ -84,7 +84,7 @@ int GammaSensorLinearization::init([[maybe_unused]] IPAContext &context,
> return -EINVAL;
> }
>
> - curveYr_ = yObject["red"].getList<uint16_t>().value_or(utils::defopt);
> + curveYr_ = yObject["red"].getList<uint16_t>();
> if (curveYr_.size() != RKISP1_CIF_ISP_DEGAMMA_CURVE_SIZE) {
> LOG(RkISP1Gsl, Error)
> << "Invalid 'y:red' coordinates: expected "
> @@ -93,7 +93,7 @@ int GammaSensorLinearization::init([[maybe_unused]] IPAContext &context,
> return -EINVAL;
> }
>
> - curveYg_ = yObject["green"].getList<uint16_t>().value_or(utils::defopt);
> + curveYg_ = yObject["green"].getList<uint16_t>();
> if (curveYg_.size() != RKISP1_CIF_ISP_DEGAMMA_CURVE_SIZE) {
> LOG(RkISP1Gsl, Error)
> << "Invalid 'y:green' coordinates: expected "
> @@ -102,7 +102,7 @@ int GammaSensorLinearization::init([[maybe_unused]] IPAContext &context,
> return -EINVAL;
> }
>
> - curveYb_ = yObject["blue"].getList<uint16_t>().value_or(utils::defopt);
> + curveYb_ = yObject["blue"].getList<uint16_t>();
> if (curveYb_.size() != RKISP1_CIF_ISP_DEGAMMA_CURVE_SIZE) {
> LOG(RkISP1Gsl, Error)
> << "Invalid 'y:blue' coordinates: expected "
> diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp
> index 05c8c0dab5c8..d1a4332c9089 100644
> --- a/src/ipa/rkisp1/algorithms/lsc.cpp
> +++ b/src/ipa/rkisp1/algorithms/lsc.cpp
> @@ -43,7 +43,7 @@ static std::vector<double> parseSizes(const YamlObject &tuningData,
> const char *prop)
> {
> std::vector<double> sizes =
> - tuningData[prop].getList<double>().value_or(utils::defopt);
> + tuningData[prop].getList<double>();
> if (sizes.size() != RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE) {
> LOG(RkISP1Lsc, Error)
> << "Invalid '" << prop << "' values: expected "
> @@ -76,7 +76,7 @@ static std::vector<uint16_t> parseTable(const YamlObject &tuningData,
> RKISP1_CIF_ISP_LSC_SAMPLES_MAX * RKISP1_CIF_ISP_LSC_SAMPLES_MAX;
>
> std::vector<uint16_t> table =
> - tuningData[prop].getList<uint16_t>().value_or(utils::defopt);
> + tuningData[prop].getList<uint16_t>();
> if (table.size() != kLscNumSamples) {
> LOG(RkISP1Lsc, Error)
> << "Invalid '" << prop << "' values: expected "
> diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
> index 84cb57d6de83..d422e0811fa7 100644
> --- a/src/libcamera/yaml_parser.cpp
> +++ b/src/libcamera/yaml_parser.cpp
> @@ -316,7 +316,7 @@ template<typename T,
> std::is_same_v<uint32_t, T> ||
> std::is_same_v<std::string, T> ||
> std::is_same_v<Size, T>> *>
> -std::optional<std::vector<T>> YamlObject::getList() const
> +std::vector<T> YamlObject::getList() const
> {
> if (type_ != Type::List)
> return {};
> @@ -334,14 +334,14 @@ std::optional<std::vector<T>> YamlObject::getList() const
> return values;
> }
>
> -template std::optional<std::vector<bool>> YamlObject::getList<bool>() const;
> -template std::optional<std::vector<double>> YamlObject::getList<double>() const;
> -template std::optional<std::vector<int16_t>> YamlObject::getList<int16_t>() const;
> -template std::optional<std::vector<uint16_t>> YamlObject::getList<uint16_t>() const;
> -template std::optional<std::vector<int32_t>> YamlObject::getList<int32_t>() const;
> -template std::optional<std::vector<uint32_t>> YamlObject::getList<uint32_t>() const;
> -template std::optional<std::vector<std::string>> YamlObject::getList<std::string>() const;
> -template std::optional<std::vector<Size>> YamlObject::getList<Size>() const;
> +template std::vector<bool> YamlObject::getList<bool>() const;
> +template std::vector<double> YamlObject::getList<double>() const;
> +template std::vector<int16_t> YamlObject::getList<int16_t>() const;
> +template std::vector<uint16_t> YamlObject::getList<uint16_t>() const;
> +template std::vector<int32_t> YamlObject::getList<int32_t>() const;
> +template std::vector<uint32_t> YamlObject::getList<uint32_t>() const;
> +template std::vector<std::string> YamlObject::getList<std::string>() const;
> +template std::vector<Size> YamlObject::getList<Size>() const;
>
> #endif /* __DOXYGEN__ */
>
> diff --git a/test/yaml-parser.cpp b/test/yaml-parser.cpp
> index 93ba88b8bcd5..e25944e7f6b8 100644
> --- a/test/yaml-parser.cpp
> +++ b/test/yaml-parser.cpp
> @@ -530,7 +530,7 @@ protected:
> }
>
> const auto &values = firstElement.getList<uint16_t>();
> - if (!values || values->size() != 2 || (*values)[0] != 1 || (*values)[1] != 2) {
> + if (values.empty() || values.size() != 2 || values[0] != 1 || values[1] != 2) {
> cerr << "getList() failed to return correct vector" << std::endl;
> return TestFail;
> }
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list