[libcamera-devel] [PATCH] libcamera: yaml_parser: Drop std::optional<> from getList()
Jacopo Mondi
jacopo at jmondi.org
Wed Aug 3 11:42:57 CEST 2022
On Wed, Aug 03, 2022 at 12:27:52PM +0300, Laurent Pinchart wrote:
> 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.
>
Didn't the fact that value_or() was used made it impossible to detect
that already ?
And by the way, in case of errors parsing the YAML file the current
implementation returns {}, which is actually an empty list and not
nullopt.
> > Reported-by: https://buildbot.libcamera.org/#/builders/6/builds/436
This however should be addressed in case std::optional<> has to be
kept in the interface.
> > 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