[03/11] ipa: libipa: vector: Specialize YamlObject getter

20275 diff mbox series

Implement a specialization of the YamlObject::Getter structure to
support deserializing ipa::Vector objects from YAML data.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/ipa/libipa/vector.cpp | 17 +++++++++++++++++
 src/ipa/libipa/vector.h   | 25 +++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

On Thu, Jun 13, 2024 at 04:39:36AM +0300, Laurent Pinchart wrote:
> Implement a specialization of the YamlObject::Getter structure to
> support deserializing ipa::Vector objects from YAML data.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  src/ipa/libipa/vector.cpp | 17 +++++++++++++++++
>  src/ipa/libipa/vector.h   | 25 +++++++++++++++++++++++++
>  2 files changed, 42 insertions(+)
> 
> diff --git a/src/ipa/libipa/vector.cpp b/src/ipa/libipa/vector.cpp
> index 5de4ae48b419..4e987d82fa70 100644
> --- a/src/ipa/libipa/vector.cpp
> +++ b/src/ipa/libipa/vector.cpp
> @@ -148,6 +148,23 @@ namespace ipa {
>   * \return True if the two vectors are not equal, false otherwise
>   */
>  
> +#ifndef __DOXYGEN__
> +bool vectorValidateYaml(const YamlObject &obj, unsigned int size)
> +{
> +	if (!obj.isList())
> +		return false;
> +
> +	if (obj.size() != size) {
> +		LOG(Vector, Error)
> +			<< "Wrong number of values in YAML vector: expected "
> +			<< size << ", got " << obj.size();
> +		return false;
> +	}
> +
> +	return true;
> +}
> +#endif /* __DOXYGEN__ */
> +
>  } /* namespace ipa */
>  
>  } /* namespace libcamera */
> diff --git a/src/ipa/libipa/vector.h b/src/ipa/libipa/vector.h
> index 7c444363d4bb..4b2fe581ecc2 100644
> --- a/src/ipa/libipa/vector.h
> +++ b/src/ipa/libipa/vector.h
> @@ -180,6 +180,10 @@ bool operator!=(const Vector<T, Rows> &lhs, const Vector<T, Rows> &rhs)
>  	return !(lhs == rhs);
>  }
>  
> +#ifndef __DOXYGEN__
> +bool vectorValidateYaml(const YamlObject &obj, unsigned int size);
> +#endif /* __DOXYGEN__ */
> +
>  } /* namespace ipa */
>  
>  #ifndef __DOXYGEN__
> @@ -195,6 +199,27 @@ std::ostream &operator<<(std::ostream &out, const ipa::Vector<T, Rows> &v)
>  
>  	return out;
>  }
> +
> +template<typename T, unsigned int Rows>
> +struct YamlObject::Getter<ipa::Vector<T, Rows>> {
> +	std::optional<ipa::Vector<T, Rows>> get(const YamlObject &obj) const
> +	{
> +		if (!ipa::vectorValidateYaml(obj, Rows))
> +			return std::nullopt;
> +
> +		ipa::Vector<T, Rows> vector;
> +
> +		unsigned int i = 0;
> +		for (const YamlObject &entry : obj.asList()) {
> +			const auto value = entry.get<T>();
> +			if (!value)
> +				return std::nullopt;
> +			vector[i++] = *value;
> +		}
> +
> +		return vector;
> +	}
> +};
>  #endif /* __DOXYGEN__ */
>  
>  } /* namespace libcamera */

Quoting Laurent Pinchart (2024-06-13 02:39:36)
> Implement a specialization of the YamlObject::Getter structure to
> support deserializing ipa::Vector objects from YAML data.

Diving to the how / why the preceeding patches are used before those
onese themselves :D

> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/ipa/libipa/vector.cpp | 17 +++++++++++++++++
>  src/ipa/libipa/vector.h   | 25 +++++++++++++++++++++++++
>  2 files changed, 42 insertions(+)
> 
> diff --git a/src/ipa/libipa/vector.cpp b/src/ipa/libipa/vector.cpp
> index 5de4ae48b419..4e987d82fa70 100644
> --- a/src/ipa/libipa/vector.cpp
> +++ b/src/ipa/libipa/vector.cpp
> @@ -148,6 +148,23 @@ namespace ipa {
>   * \return True if the two vectors are not equal, false otherwise
>   */
>  
> +#ifndef __DOXYGEN__
> +bool vectorValidateYaml(const YamlObject &obj, unsigned int size)
> +{
> +       if (!obj.isList())
> +               return false;
> +
> +       if (obj.size() != size) {
> +               LOG(Vector, Error)
> +                       << "Wrong number of values in YAML vector: expected "
> +                       << size << ", got " << obj.size();
> +               return false;
> +       }
> +
> +       return true;
> +}
> +#endif /* __DOXYGEN__ */
> +
>  } /* namespace ipa */
>  
>  } /* namespace libcamera */
> diff --git a/src/ipa/libipa/vector.h b/src/ipa/libipa/vector.h
> index 7c444363d4bb..4b2fe581ecc2 100644
> --- a/src/ipa/libipa/vector.h
> +++ b/src/ipa/libipa/vector.h
> @@ -180,6 +180,10 @@ bool operator!=(const Vector<T, Rows> &lhs, const Vector<T, Rows> &rhs)
>         return !(lhs == rhs);
>  }
>  
> +#ifndef __DOXYGEN__
> +bool vectorValidateYaml(const YamlObject &obj, unsigned int size);
> +#endif /* __DOXYGEN__ */
> +
>  } /* namespace ipa */
>  
>  #ifndef __DOXYGEN__
> @@ -195,6 +199,27 @@ std::ostream &operator<<(std::ostream &out, const ipa::Vector<T, Rows> &v)
>  
>         return out;
>  }
> +
> +template<typename T, unsigned int Rows>
> +struct YamlObject::Getter<ipa::Vector<T, Rows>> {
> +       std::optional<ipa::Vector<T, Rows>> get(const YamlObject &obj) const

Ok - so the design is to keep the gets that mix the two types
(YamlObject, Vector) outside of both of those classes, by implementing a
Getter which yaml_parser.h std::optional<T> get() will be able to map to
through it's "return Getter<T>{}.get(*this);"

Sounds good to me if it's working.

> +       {
> +               if (!ipa::vectorValidateYaml(obj, Rows))
> +                       return std::nullopt;
> +
> +               ipa::Vector<T, Rows> vector;
> +
> +               unsigned int i = 0;
> +               for (const YamlObject &entry : obj.asList()) {
> +                       const auto value = entry.get<T>();
> +                       if (!value)
> +                               return std::nullopt;

Should we be LOGging an error here? This /shouldn't/ happen - I think it
can only happen if someone puts the wrong type into the Yaml object? But
that would be easier to identify/debug with a print... which should be
fairly low cost to add in here right?

Of course there's no error prints on the base type getters either - but
as this is a specialisation - maybe it's more 'custom' and could warrant
more verbosity?

Anyway, either way:


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> +                       vector[i++] = *value;
> +               }
> +
> +               return vector;
> +       }
> +};
>  #endif /* __DOXYGEN__ */
>  
>  } /* namespace libcamera */
> -- 
> Regards,
> 
> Laurent Pinchart
>

Hi Kieran,

On Thu, Jun 13, 2024 at 09:14:03AM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2024-06-13 02:39:36)
> > Implement a specialization of the YamlObject::Getter structure to
> > support deserializing ipa::Vector objects from YAML data.
> 
> Diving to the how / why the preceeding patches are used before those
> onese themselves :D

Good idea :-) That's the most important part.

> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  src/ipa/libipa/vector.cpp | 17 +++++++++++++++++
> >  src/ipa/libipa/vector.h   | 25 +++++++++++++++++++++++++
> >  2 files changed, 42 insertions(+)
> > 
> > diff --git a/src/ipa/libipa/vector.cpp b/src/ipa/libipa/vector.cpp
> > index 5de4ae48b419..4e987d82fa70 100644
> > --- a/src/ipa/libipa/vector.cpp
> > +++ b/src/ipa/libipa/vector.cpp
> > @@ -148,6 +148,23 @@ namespace ipa {
> >   * \return True if the two vectors are not equal, false otherwise
> >   */
> >  
> > +#ifndef __DOXYGEN__
> > +bool vectorValidateYaml(const YamlObject &obj, unsigned int size)
> > +{
> > +       if (!obj.isList())
> > +               return false;
> > +
> > +       if (obj.size() != size) {
> > +               LOG(Vector, Error)
> > +                       << "Wrong number of values in YAML vector: expected "
> > +                       << size << ", got " << obj.size();
> > +               return false;
> > +       }
> > +
> > +       return true;
> > +}
> > +#endif /* __DOXYGEN__ */
> > +
> >  } /* namespace ipa */
> >  
> >  } /* namespace libcamera */
> > diff --git a/src/ipa/libipa/vector.h b/src/ipa/libipa/vector.h
> > index 7c444363d4bb..4b2fe581ecc2 100644
> > --- a/src/ipa/libipa/vector.h
> > +++ b/src/ipa/libipa/vector.h
> > @@ -180,6 +180,10 @@ bool operator!=(const Vector<T, Rows> &lhs, const Vector<T, Rows> &rhs)
> >         return !(lhs == rhs);
> >  }
> >  
> > +#ifndef __DOXYGEN__
> > +bool vectorValidateYaml(const YamlObject &obj, unsigned int size);
> > +#endif /* __DOXYGEN__ */
> > +
> >  } /* namespace ipa */
> >  
> >  #ifndef __DOXYGEN__
> > @@ -195,6 +199,27 @@ std::ostream &operator<<(std::ostream &out, const ipa::Vector<T, Rows> &v)
> >  
> >         return out;
> >  }
> > +
> > +template<typename T, unsigned int Rows>
> > +struct YamlObject::Getter<ipa::Vector<T, Rows>> {
> > +       std::optional<ipa::Vector<T, Rows>> get(const YamlObject &obj) const
> 
> Ok - so the design is to keep the gets that mix the two types
> (YamlObject, Vector) outside of both of those classes, by implementing a
> Getter which yaml_parser.h std::optional<T> get() will be able to map to
> through it's "return Getter<T>{}.get(*this);"

Yes that's the idea. If the type you want to deserialize is a type
template, you need to do it as in this patch, with a specialization of
the Getter structure. If the type is not a type template, then you can
do as in patch 09/11, skipping the structure and just write

template<>
std::optional<ipa::Pwl>
YamlObject::Getter<ipa::Pwl>::get(const YamlObject &obj) const
{
	...
}

It's the same mechanism in both cases, with a small syntax shortcut for
non-template types.

For the users, nothing changes, it's YamlObject::get<T>() in all cases.

> Sounds good to me if it's working.

As far as I can tell it is :-)

> > +       {
> > +               if (!ipa::vectorValidateYaml(obj, Rows))
> > +                       return std::nullopt;
> > +
> > +               ipa::Vector<T, Rows> vector;
> > +
> > +               unsigned int i = 0;
> > +               for (const YamlObject &entry : obj.asList()) {
> > +                       const auto value = entry.get<T>();
> > +                       if (!value)
> > +                               return std::nullopt;
> 
> Should we be LOGging an error here? This /shouldn't/ happen - I think it
> can only happen if someone puts the wrong type into the Yaml object? But
> that would be easier to identify/debug with a print... which should be
> fairly low cost to add in here right?
> 
> Of course there's no error prints on the base type getters either - but
> as this is a specialisation - maybe it's more 'custom' and could warrant
> more verbosity?

I've avoided a print here, as this function is defined inline, so I
wanted to avoid code bloat. Any message we would print here would only
be able to tell that there was an issue reading *a* vector. I think a
message in the caller would be better, to indicate what failed.

> Anyway, either way:
> 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > +                       vector[i++] = *value;
> > +               }
> > +
> > +               return vector;
> > +       }
> > +};
> >  #endif /* __DOXYGEN__ */
> >  
> >  } /* namespace libcamera */