| Message ID | 20240613013944.23344-10-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
On Thu, Jun 13, 2024 at 04:39:42AM +0300, Laurent Pinchart wrote: > Implement a specialization of the YamlObject::Getter structure to > support deserializing ipa::Pwl objects from YAML data. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Paul Elder <paul.elder@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. > + */ > +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) > + return std::nullopt; > + > + pwl.append(*x, *y); > + } > + > + if (pwl.size() != obj.size() / 2) > + return std::nullopt; > + > + return pwl; > +} > +#endif /* __DOXYGEN__ */ > + > } /* namespace libcamera */
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@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 ;-( > +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@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 >
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@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@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 */
Quoting Laurent Pinchart (2024-06-13 12:59:42) > 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@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. I meant users of the function... but yes I think it's fine for now. -- Kieran
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. + */ +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) + return std::nullopt; + + pwl.append(*x, *y); + } + + if (pwl.size() != obj.size() / 2) + return std::nullopt; + + return pwl; +} +#endif /* __DOXYGEN__ */ + } /* namespace libcamera */
Implement a specialization of the YamlObject::Getter structure to support deserializing ipa::Pwl objects from YAML data. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/ipa/libipa/pwl.cpp | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)