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

Laurent Pinchart laurent.pinchart at ideasonboard.com
Wed Aug 3 12:21:56 CEST 2022 

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

More information about the libcamera-devel mailing list