[v4,11/35] ipa: libipa: lsc_polynomial: Don't inline ValueNode accessor
diff mbox series

Message ID 20260708-libipa-algorithms-v4-11-f0e99f035294@ideasonboard.com
State New
Headers show
Series
  • ipa: libipa: Introduce libipa algorithms
Related show

Commit Message

Jacopo Mondi July 8, 2026, 9:24 a.m. UTC 
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

There's no need to inline the ValueNode::Accessor::get() function for
LscPolynomial. Move it to from lsc_polynomial.h to lsc_polynomial.cpp.

While at it, replace C++-style comments with C-style comments according
to the libcamera coding style.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 src/ipa/libipa/lsc_polynomial.cpp | 26 ++++++++++++++++++++++++--
 src/ipa/libipa/lsc_polynomial.h   | 24 ------------------------
 2 files changed, 24 insertions(+), 26 deletions(-)

Comments

Milan Zamazal July 8, 2026, 12:10 p.m. UTC | #1 
Hi,

Jacopo Mondi <jacopo.mondi@ideasonboard.com> writes:

> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> There's no need to inline the ValueNode::Accessor::get() function for
> LscPolynomial. Move it to from lsc_polynomial.h to lsc_polynomial.cpp.
>
> While at it, replace C++-style comments with C-style comments according
> to the libcamera coding style.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  src/ipa/libipa/lsc_polynomial.cpp | 26 ++++++++++++++++++++++++--
>  src/ipa/libipa/lsc_polynomial.h   | 24 ------------------------
>  2 files changed, 24 insertions(+), 26 deletions(-)
>
> diff --git a/src/ipa/libipa/lsc_polynomial.cpp b/src/ipa/libipa/lsc_polynomial.cpp
> index f607d86c54c3..00569c2fd160 100644
> --- a/src/ipa/libipa/lsc_polynomial.cpp
> +++ b/src/ipa/libipa/lsc_polynomial.cpp
> @@ -77,5 +77,27 @@ namespace ipa {
>   * \param size The size of the reference image
>   */
>  
> -} // namespace ipa
> -} // namespace libcamera
> +} /* namespace ipa */
> +
> +#ifndef __DOXYGEN__
> +template<>
> +std::optional<ipa::LscPolynomial>
> +ValueNode::Accessor<ipa::LscPolynomial>::get(const ValueNode &obj) const
> +{
> +	std::optional<double> cx = obj["cx"].get<double>();
> +	std::optional<double> cy = obj["cy"].get<double>();
> +	std::optional<double> k0 = obj["k0"].get<double>();
> +	std::optional<double> k1 = obj["k1"].get<double>();
> +	std::optional<double> k2 = obj["k2"].get<double>();
> +	std::optional<double> k3 = obj["k3"].get<double>();
> +	std::optional<double> k4 = obj["k4"].get<double>();
> +
> +	if (!(cx && cy && k0 && k1 && k2 && k3 && k4))
> +		LOG(LscPolynomial, Error)
> +			<< "Polynomial is missing a parameter";

This should return an empty value or do something about the missing
parameters, otherwise the error is followed by a crash in the return
statement below.

This is not a fault of this patch but it would be worth to fix, here or elsewhere.

> +
> +	return ipa::LscPolynomial(*cx, *cy, *k0, *k1, *k2, *k3, *k4);
> +}
> +#endif /* __DOXYGEN__ */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/libipa/lsc_polynomial.h b/src/ipa/libipa/lsc_polynomial.h
> index d7d9ae42e360..d52ec86946b1 100644
> --- a/src/ipa/libipa/lsc_polynomial.h
> +++ b/src/ipa/libipa/lsc_polynomial.h
> @@ -80,28 +80,4 @@ private:
>  
>  } /* namespace ipa */
>  
> -#ifndef __DOXYGEN__
> -
> -template<>
> -struct ValueNode::Accessor<ipa::LscPolynomial> {
> -	std::optional<ipa::LscPolynomial> get(const ValueNode &obj) const
> -	{
> -		std::optional<double> cx = obj["cx"].get<double>();
> -		std::optional<double> cy = obj["cy"].get<double>();
> -		std::optional<double> k0 = obj["k0"].get<double>();
> -		std::optional<double> k1 = obj["k1"].get<double>();
> -		std::optional<double> k2 = obj["k2"].get<double>();
> -		std::optional<double> k3 = obj["k3"].get<double>();
> -		std::optional<double> k4 = obj["k4"].get<double>();
> -
> -		if (!(cx && cy && k0 && k1 && k2 && k3 && k4))
> -			LOG(LscPolynomial, Error)
> -				<< "Polynomial is missing a parameter";
> -
> -		return ipa::LscPolynomial(*cx, *cy, *k0, *k1, *k2, *k3, *k4);
> -	}
> -};
> -
> -#endif
> -
>  } /* namespace libcamera */
Jacopo Mondi July 8, 2026, 1:46 p.m. UTC | #2 
Hi Milan

On Wed, Jul 08, 2026 at 02:10:27PM +0200, Milan Zamazal wrote:
> Hi,
>
> Jacopo Mondi <jacopo.mondi@ideasonboard.com> writes:
>
> > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > There's no need to inline the ValueNode::Accessor::get() function for
> > LscPolynomial. Move it to from lsc_polynomial.h to lsc_polynomial.cpp.
> >
> > While at it, replace C++-style comments with C-style comments according
> > to the libcamera coding style.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> >  src/ipa/libipa/lsc_polynomial.cpp | 26 ++++++++++++++++++++++++--
> >  src/ipa/libipa/lsc_polynomial.h   | 24 ------------------------
> >  2 files changed, 24 insertions(+), 26 deletions(-)
> >
> > diff --git a/src/ipa/libipa/lsc_polynomial.cpp b/src/ipa/libipa/lsc_polynomial.cpp
> > index f607d86c54c3..00569c2fd160 100644
> > --- a/src/ipa/libipa/lsc_polynomial.cpp
> > +++ b/src/ipa/libipa/lsc_polynomial.cpp
> > @@ -77,5 +77,27 @@ namespace ipa {
> >   * \param size The size of the reference image
> >   */
> >
> > -} // namespace ipa
> > -} // namespace libcamera
> > +} /* namespace ipa */
> > +
> > +#ifndef __DOXYGEN__
> > +template<>
> > +std::optional<ipa::LscPolynomial>
> > +ValueNode::Accessor<ipa::LscPolynomial>::get(const ValueNode &obj) const
> > +{
> > +	std::optional<double> cx = obj["cx"].get<double>();
> > +	std::optional<double> cy = obj["cy"].get<double>();
> > +	std::optional<double> k0 = obj["k0"].get<double>();
> > +	std::optional<double> k1 = obj["k1"].get<double>();
> > +	std::optional<double> k2 = obj["k2"].get<double>();
> > +	std::optional<double> k3 = obj["k3"].get<double>();
> > +	std::optional<double> k4 = obj["k4"].get<double>();
> > +
> > +	if (!(cx && cy && k0 && k1 && k2 && k3 && k4))
> > +		LOG(LscPolynomial, Error)
> > +			<< "Polynomial is missing a parameter";
>
> This should return an empty value or do something about the missing
> parameters, otherwise the error is followed by a crash in the return
> statement below.
>
> This is not a fault of this patch but it would be worth to fix, here or elsewhere.
>

Thanks for pointing this out

It should be enough to:

--- a/src/ipa/libipa/lsc_polynomial.cpp
+++ b/src/ipa/libipa/lsc_polynomial.cpp
@@ -229,9 +229,11 @@ ValueNode::Accessor<ipa::lsc::Polynomial>::get(const ValueNode &obj) const
        std::optional<double> k3 = obj["k3"].get<double>();
        std::optional<double> k4 = obj["k4"].get<double>();

-       if (!(cx && cy && k0 && k1 && k2 && k3 && k4))
+       if (!(cx && cy && k0 && k1 && k2 && k3 && k4)) {
                LOG(LscPolynomial, Error)
                        << "Polynomial is missing a parameter";
+               return std::nullopt;
+       }

        return ipa::lsc::Polynomial(*cx, *cy, *k0, *k1, *k2, *k3, *k4);
 }

The caller already checks

        auto polynomial = set[k.c_str()].get<lsc::Polynomial>();
        if (!polynomial) {
                LOG(LscPolynomial, Error)
                        << "Missing polynomial for component "
                        << k;
                return -EINVAL;
        }

Which should guarantee parsing fails if a polynomial component is
missing.

I'll add a patch to the series

Thanks
   j

> > +
> > +	return ipa::LscPolynomial(*cx, *cy, *k0, *k1, *k2, *k3, *k4);
> > +}
> > +#endif /* __DOXYGEN__ */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/libipa/lsc_polynomial.h b/src/ipa/libipa/lsc_polynomial.h
> > index d7d9ae42e360..d52ec86946b1 100644
> > --- a/src/ipa/libipa/lsc_polynomial.h
> > +++ b/src/ipa/libipa/lsc_polynomial.h
> > @@ -80,28 +80,4 @@ private:
> >
> >  } /* namespace ipa */
> >
> > -#ifndef __DOXYGEN__
> > -
> > -template<>
> > -struct ValueNode::Accessor<ipa::LscPolynomial> {
> > -	std::optional<ipa::LscPolynomial> get(const ValueNode &obj) const
> > -	{
> > -		std::optional<double> cx = obj["cx"].get<double>();
> > -		std::optional<double> cy = obj["cy"].get<double>();
> > -		std::optional<double> k0 = obj["k0"].get<double>();
> > -		std::optional<double> k1 = obj["k1"].get<double>();
> > -		std::optional<double> k2 = obj["k2"].get<double>();
> > -		std::optional<double> k3 = obj["k3"].get<double>();
> > -		std::optional<double> k4 = obj["k4"].get<double>();
> > -
> > -		if (!(cx && cy && k0 && k1 && k2 && k3 && k4))
> > -			LOG(LscPolynomial, Error)
> > -				<< "Polynomial is missing a parameter";
> > -
> > -		return ipa::LscPolynomial(*cx, *cy, *k0, *k1, *k2, *k3, *k4);
> > -	}
> > -};
> > -
> > -#endif
> > -
> >  } /* namespace libcamera */
>

Patch
diff mbox series 

diff --git a/src/ipa/libipa/lsc_polynomial.cpp b/src/ipa/libipa/lsc_polynomial.cpp
index f607d86c54c3..00569c2fd160 100644
--- a/src/ipa/libipa/lsc_polynomial.cpp
+++ b/src/ipa/libipa/lsc_polynomial.cpp
@@ -77,5 +77,27 @@  namespace ipa {
  * \param size The size of the reference image
  */
 
-} // namespace ipa
-} // namespace libcamera
+} /* namespace ipa */
+
+#ifndef __DOXYGEN__
+template<>
+std::optional<ipa::LscPolynomial>
+ValueNode::Accessor<ipa::LscPolynomial>::get(const ValueNode &obj) const
+{
+	std::optional<double> cx = obj["cx"].get<double>();
+	std::optional<double> cy = obj["cy"].get<double>();
+	std::optional<double> k0 = obj["k0"].get<double>();
+	std::optional<double> k1 = obj["k1"].get<double>();
+	std::optional<double> k2 = obj["k2"].get<double>();
+	std::optional<double> k3 = obj["k3"].get<double>();
+	std::optional<double> k4 = obj["k4"].get<double>();
+
+	if (!(cx && cy && k0 && k1 && k2 && k3 && k4))
+		LOG(LscPolynomial, Error)
+			<< "Polynomial is missing a parameter";
+
+	return ipa::LscPolynomial(*cx, *cy, *k0, *k1, *k2, *k3, *k4);
+}
+#endif /* __DOXYGEN__ */
+
+} /* namespace libcamera */
diff --git a/src/ipa/libipa/lsc_polynomial.h b/src/ipa/libipa/lsc_polynomial.h
index d7d9ae42e360..d52ec86946b1 100644
--- a/src/ipa/libipa/lsc_polynomial.h
+++ b/src/ipa/libipa/lsc_polynomial.h
@@ -80,28 +80,4 @@  private:
 
 } /* namespace ipa */
 
-#ifndef __DOXYGEN__
-
-template<>
-struct ValueNode::Accessor<ipa::LscPolynomial> {
-	std::optional<ipa::LscPolynomial> get(const ValueNode &obj) const
-	{
-		std::optional<double> cx = obj["cx"].get<double>();
-		std::optional<double> cy = obj["cy"].get<double>();
-		std::optional<double> k0 = obj["k0"].get<double>();
-		std::optional<double> k1 = obj["k1"].get<double>();
-		std::optional<double> k2 = obj["k2"].get<double>();
-		std::optional<double> k3 = obj["k3"].get<double>();
-		std::optional<double> k4 = obj["k4"].get<double>();
-
-		if (!(cx && cy && k0 && k1 && k2 && k3 && k4))
-			LOG(LscPolynomial, Error)
-				<< "Polynomial is missing a parameter";
-
-		return ipa::LscPolynomial(*cx, *cy, *k0, *k1, *k2, *k3, *k4);
-	}
-};
-
-#endif
-
 } /* namespace libcamera */