[libcamera-devel] libcamera: yaml_parser: Drop std::optional<> from getList()
diff mbox series

Message ID 20220803091502.17280-1-jacopo@jmondi.org
State Superseded, archived
Headers show
Series
  • [libcamera-devel] libcamera: yaml_parser: Drop std::optional<> from getList()
Related show

Commit Message

Jacopo Mondi Aug. 3, 2022, 9:15 a.m. UTC 
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))

Reported-by: https://buildbot.libcamera.org/#/builders/6/builds/436
Signed-off-by: Jacopo Mondi <jacopo@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(-)

--
2.37.1

Comments

Laurent Pinchart Aug. 3, 2022, 9:27 a.m. UTC | #1 
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@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 &params)
>  {
> -	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;
>  		}
Jacopo Mondi Aug. 3, 2022, 9:42 a.m. UTC | #2 
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@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 &params)
> >  {
> > -	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
Laurent Pinchart Aug. 3, 2022, 10:21 a.m. UTC | #3 
Hi Jacopo,

On Wed, Aug 03, 2022 at 11:42:57AM +0200, Jacopo Mondi wrote:
> On Wed, Aug 03, 2022 at 12:27:52PM +0300, Laurent Pinchart wrote:
> > 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 ?

The existing callers expect a list of a fixed size, so they use
value_or() and check the size of the vector. A caller that would need to
differentiate the twp would do

void doSomethingWithList(const std::vector<uint32_t> &values)
{
	...
}

{
	...

	std::optional<std::vector<uint32_t>> value = yaml.getList<uint32_t>();
	if (!value)
		/* Error handling */

	doSomethingWithList(*value);

	...
}

> 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.

That's wrong indeed, my bad. Would you like to send a patch to fix that,
or should I ?

> > > 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.

Yes, I agree.

Can you reproduce the issue locally ?

> > > Signed-off-by: Jacopo Mondi <jacopo@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 &params)
> > >  {
> > > -	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;
> > >  		}

Patch
diff mbox series 

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 &params)
 {
-	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;
 		}