[PATCH 09/11] ipa: libipa: pwl: Specialize YamlObject getter

[Laurent Pinchart]

On Thu, Jun 13, 2024 at 12:36:54PM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2024-06-13 02:39:42)
> > Implement a specialization of the YamlObject::Getter structure to
> > support deserializing ipa::Pwl objects from YAML data.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  src/ipa/libipa/pwl.cpp | 35 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 35 insertions(+)
> > 
> > diff --git a/src/ipa/libipa/pwl.cpp b/src/ipa/libipa/pwl.cpp
> > index cf864fbb3889..3c639645fa3d 100644
> > --- a/src/ipa/libipa/pwl.cpp
> > +++ b/src/ipa/libipa/pwl.cpp
> > @@ -459,4 +459,39 @@ std::string Pwl::toString() const
> >  
> >  } /* namespace ipa */
> >  
> > +#ifndef __DOXYGEN__
> > +/*
> > + * The YAML data shall be a list of numerical values with an even number of
> > + * elements. They are parsed in pairs into x and y points in the piecewise
> > + * linear function, and added in order. x must be monotonically increasing.
> > + */
> 
> I love this text. But I'm not sure it's where users will see it ;-(

Users of libcamera definitely won't. From a user point of view, this
needs to be translate to a combination of YAML schema and documentation
for the tuning file formats.

>From an IPA module developer point of view, this comment will not appear
in generated documentation either. It occurred to me when writing the
patches, and I'm still not sure if I'm (very slightly) concerned by that
or not. I don't think it will catch anyone by surprise, and I expect
most users to copy code or get inspiration from existing IPA modules.

> > +template<>
> > +std::optional<ipa::Pwl>
> > +YamlObject::Getter<ipa::Pwl>::get(const YamlObject &obj) const
> > +{
> > +       if (!obj.size() || obj.size() % 2)
> > +               return std::nullopt;
> > +
> > +       ipa::Pwl pwl;
> > +
> > +       const auto &list = obj.asList();
> > +
> > +       for (auto it = list.begin(); it != list.end(); it++) {
> > +               auto x = it->get<double>();
> > +               if (!x)
> > +                       return std::nullopt;
> > +               auto y = (++it)->get<double>();
> > +               if (!y)
> 
> I assume that's safe...
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> > +                       return std::nullopt;
> > +
> > +               pwl.append(*x, *y);
> > +       }
> > +
> > +       if (pwl.size() != obj.size() / 2)
> 
> Now I see the value of size...
> 
> > +               return std::nullopt;
> > +
> > +       return pwl;
> > +}
> > +#endif /* __DOXYGEN__ */
> > +
> >  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart