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

[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 at 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 at ideasonboard.com>
> 
> > +                       vector[i++] = *value;
> > +               }
> > +
> > +               return vector;
> > +       }
> > +};
> >  #endif /* __DOXYGEN__ */
> >  
> >  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart