libcamera: yaml_parser: Add support for float types
diff mbox series

Message ID 20240621120318.25851-1-laurent.pinchart@ideasonboard.com
State Accepted
Commit 3d7e50fd715dee709da9e2a8c9ccd4892f4d5f75
Headers show
Series
  • libcamera: yaml_parser: Add support for float types
Related show

Commit Message

Laurent Pinchart June 21, 2024, 12:03 p.m. UTC 
The YamlObject::get<T>() function template has a specialization for
double but not for float. When used in an IPA module, the issue is
caught at module load time only, when dynamic links are resolved,
causing errors such as

Failed to open IPA module shared object: /usr/lib/libcamera/ipa_rkisp1.so: undefined symbol: _ZNK9libcamera10YamlObject6GetterIfE3getERK_

Fix it by adding a float specialization. The alternative would be to use
double only in IPA modules, but the lack of enforcement at compile time
makes this dangerous.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Jacopo, I think this may fix the issue you've experienced. Could you
test the patch ?
---
 include/libcamera/internal/yaml_parser.h | 1 +
 src/libcamera/yaml_parser.cpp            | 9 +++++++++
 2 files changed, 10 insertions(+)

Comments

Jacopo Mondi June 21, 2024, 1:13 p.m. UTC | #1 
Hi Laurent

On Fri, Jun 21, 2024 at 03:03:18PM GMT, Laurent Pinchart wrote:
> The YamlObject::get<T>() function template has a specialization for
> double but not for float. When used in an IPA module, the issue is
> caught at module load time only, when dynamic links are resolved,
> causing errors such as
>
> Failed to open IPA module shared object: /usr/lib/libcamera/ipa_rkisp1.so: undefined symbol: _ZNK9libcamera10YamlObject6GetterIfE3getERK_
>
> Fix it by adding a float specialization. The alternative would be to use
> double only in IPA modules, but the lack of enforcement at compile time
> makes this dangerous.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Jacopo, I think this may fix the issue you've experienced. Could you
> test the patch ?

Thank you! indeed it does!

Tested-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j
> ---
>  include/libcamera/internal/yaml_parser.h | 1 +
>  src/libcamera/yaml_parser.cpp            | 9 +++++++++
>  2 files changed, 10 insertions(+)
>
> diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h
> index 06a41146ad01..e38a2df9ae1d 100644
> --- a/include/libcamera/internal/yaml_parser.h
> +++ b/include/libcamera/internal/yaml_parser.h
> @@ -177,6 +177,7 @@ public:
>  	template<typename T,
>  		 std::enable_if_t<
>  			 std::is_same_v<bool, T> ||
> +			 std::is_same_v<float, T> ||
>  			 std::is_same_v<double, T> ||
>  			 std::is_same_v<int8_t, T> ||
>  			 std::is_same_v<uint8_t, T> ||
> diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
> index 56670ba7a584..025006bcdcdd 100644
> --- a/src/libcamera/yaml_parser.cpp
> +++ b/src/libcamera/yaml_parser.cpp
> @@ -278,6 +278,13 @@ YamlObject::Getter<uint32_t>::get(const YamlObject &obj) const
>  	return value;
>  }
>
> +template<>
> +std::optional<float>
> +YamlObject::Getter<float>::get(const YamlObject &obj) const
> +{
> +	return obj.get<double>();
> +}
> +
>  template<>
>  std::optional<double>
>  YamlObject::Getter<double>::get(const YamlObject &obj) const
> @@ -349,6 +356,7 @@ YamlObject::Getter<Size>::get(const YamlObject &obj) const
>  template<typename T,
>  	 std::enable_if_t<
>  		 std::is_same_v<bool, T> ||
> +		 std::is_same_v<float, T> ||
>  		 std::is_same_v<double, T> ||
>  		 std::is_same_v<int8_t, T> ||
>  		 std::is_same_v<uint8_t, T> ||
> @@ -377,6 +385,7 @@ std::optional<std::vector<T>> YamlObject::getList() const
>  }
>
>  template std::optional<std::vector<bool>> YamlObject::getList<bool>() const;
> +template std::optional<std::vector<float>> YamlObject::getList<float>() const;
>  template std::optional<std::vector<double>> YamlObject::getList<double>() const;
>  template std::optional<std::vector<int8_t>> YamlObject::getList<int8_t>() const;
>  template std::optional<std::vector<uint8_t>> YamlObject::getList<uint8_t>() const;
> --
> Regards,
>
> Laurent Pinchart
>
Stefan Klug June 21, 2024, 2:52 p.m. UTC | #2 
Hi Laurent,

On Fri, Jun 21, 2024 at 03:03:18PM +0300, Laurent Pinchart wrote:
> The YamlObject::get<T>() function template has a specialization for
> double but not for float. When used in an IPA module, the issue is
> caught at module load time only, when dynamic links are resolved,
> causing errors such as
> 
> Failed to open IPA module shared object: /usr/lib/libcamera/ipa_rkisp1.so: undefined symbol: _ZNK9libcamera10YamlObject6GetterIfE3getERK_
> 
> Fix it by adding a float specialization. The alternative would be to use
> double only in IPA modules, but the lack of enforcement at compile time
> makes this dangerous.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> 

Cheers,
Stefam

> ---
> Jacopo, I think this may fix the issue you've experienced. Could you
> test the patch ?
> ---
>  include/libcamera/internal/yaml_parser.h | 1 +
>  src/libcamera/yaml_parser.cpp            | 9 +++++++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h
> index 06a41146ad01..e38a2df9ae1d 100644
> --- a/include/libcamera/internal/yaml_parser.h
> +++ b/include/libcamera/internal/yaml_parser.h
> @@ -177,6 +177,7 @@ public:
>  	template<typename T,
>  		 std::enable_if_t<
>  			 std::is_same_v<bool, T> ||
> +			 std::is_same_v<float, T> ||
>  			 std::is_same_v<double, T> ||
>  			 std::is_same_v<int8_t, T> ||
>  			 std::is_same_v<uint8_t, T> ||
> diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
> index 56670ba7a584..025006bcdcdd 100644
> --- a/src/libcamera/yaml_parser.cpp
> +++ b/src/libcamera/yaml_parser.cpp
> @@ -278,6 +278,13 @@ YamlObject::Getter<uint32_t>::get(const YamlObject &obj) const
>  	return value;
>  }
>  
> +template<>
> +std::optional<float>
> +YamlObject::Getter<float>::get(const YamlObject &obj) const
> +{
> +	return obj.get<double>();
> +}
> +
>  template<>
>  std::optional<double>
>  YamlObject::Getter<double>::get(const YamlObject &obj) const
> @@ -349,6 +356,7 @@ YamlObject::Getter<Size>::get(const YamlObject &obj) const
>  template<typename T,
>  	 std::enable_if_t<
>  		 std::is_same_v<bool, T> ||
> +		 std::is_same_v<float, T> ||
>  		 std::is_same_v<double, T> ||
>  		 std::is_same_v<int8_t, T> ||
>  		 std::is_same_v<uint8_t, T> ||
> @@ -377,6 +385,7 @@ std::optional<std::vector<T>> YamlObject::getList() const
>  }
>  
>  template std::optional<std::vector<bool>> YamlObject::getList<bool>() const;
> +template std::optional<std::vector<float>> YamlObject::getList<float>() const;
>  template std::optional<std::vector<double>> YamlObject::getList<double>() const;
>  template std::optional<std::vector<int8_t>> YamlObject::getList<int8_t>() const;
>  template std::optional<std::vector<uint8_t>> YamlObject::getList<uint8_t>() const;
> -- 
> Regards,
> 
> Laurent Pinchart
>
Kieran Bingham June 24, 2024, 9:55 p.m. UTC | #3 
Quoting Laurent Pinchart (2024-06-21 13:03:18)
> The YamlObject::get<T>() function template has a specialization for
> double but not for float. When used in an IPA module, the issue is
> caught at module load time only, when dynamic links are resolved,
> causing errors such as
> 
> Failed to open IPA module shared object: /usr/lib/libcamera/ipa_rkisp1.so: undefined symbol: _ZNK9libcamera10YamlObject6GetterIfE3getERK_
> 
> Fix it by adding a float specialization. The alternative would be to use
> double only in IPA modules, but the lack of enforcement at compile time
> makes this dangerous.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Jacopo, I think this may fix the issue you've experienced. Could you
> test the patch ?
> ---
>  include/libcamera/internal/yaml_parser.h | 1 +
>  src/libcamera/yaml_parser.cpp            | 9 +++++++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h
> index 06a41146ad01..e38a2df9ae1d 100644
> --- a/include/libcamera/internal/yaml_parser.h
> +++ b/include/libcamera/internal/yaml_parser.h
> @@ -177,6 +177,7 @@ public:
>         template<typename T,
>                  std::enable_if_t<
>                          std::is_same_v<bool, T> ||
> +                        std::is_same_v<float, T> ||
>                          std::is_same_v<double, T> ||
>                          std::is_same_v<int8_t, T> ||
>                          std::is_same_v<uint8_t, T> ||
> diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
> index 56670ba7a584..025006bcdcdd 100644
> --- a/src/libcamera/yaml_parser.cpp
> +++ b/src/libcamera/yaml_parser.cpp
> @@ -278,6 +278,13 @@ YamlObject::Getter<uint32_t>::get(const YamlObject &obj) const
>         return value;
>  }
>  
> +template<>
> +std::optional<float>
> +YamlObject::Getter<float>::get(const YamlObject &obj) const
> +{
> +       return obj.get<double>();

I guess there's not a lot of distinction in parsing a value from Yaml as
either a float or a double... and the return/template type will
determine what the call site requires.

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

> +}
> +
>  template<>
>  std::optional<double>
>  YamlObject::Getter<double>::get(const YamlObject &obj) const
> @@ -349,6 +356,7 @@ YamlObject::Getter<Size>::get(const YamlObject &obj) const
>  template<typename T,
>          std::enable_if_t<
>                  std::is_same_v<bool, T> ||
> +                std::is_same_v<float, T> ||
>                  std::is_same_v<double, T> ||
>                  std::is_same_v<int8_t, T> ||
>                  std::is_same_v<uint8_t, T> ||
> @@ -377,6 +385,7 @@ std::optional<std::vector<T>> YamlObject::getList() const
>  }
>  
>  template std::optional<std::vector<bool>> YamlObject::getList<bool>() const;
> +template std::optional<std::vector<float>> YamlObject::getList<float>() const;
>  template std::optional<std::vector<double>> YamlObject::getList<double>() const;
>  template std::optional<std::vector<int8_t>> YamlObject::getList<int8_t>() const;
>  template std::optional<std::vector<uint8_t>> YamlObject::getList<uint8_t>() const;
> -- 
> Regards,
> 
> Laurent Pinchart
>

Patch
diff mbox series 

diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h
index 06a41146ad01..e38a2df9ae1d 100644
--- a/include/libcamera/internal/yaml_parser.h
+++ b/include/libcamera/internal/yaml_parser.h
@@ -177,6 +177,7 @@  public:
 	template<typename T,
 		 std::enable_if_t<
 			 std::is_same_v<bool, T> ||
+			 std::is_same_v<float, T> ||
 			 std::is_same_v<double, T> ||
 			 std::is_same_v<int8_t, T> ||
 			 std::is_same_v<uint8_t, T> ||
diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
index 56670ba7a584..025006bcdcdd 100644
--- a/src/libcamera/yaml_parser.cpp
+++ b/src/libcamera/yaml_parser.cpp
@@ -278,6 +278,13 @@  YamlObject::Getter<uint32_t>::get(const YamlObject &obj) const
 	return value;
 }
 
+template<>
+std::optional<float>
+YamlObject::Getter<float>::get(const YamlObject &obj) const
+{
+	return obj.get<double>();
+}
+
 template<>
 std::optional<double>
 YamlObject::Getter<double>::get(const YamlObject &obj) const
@@ -349,6 +356,7 @@  YamlObject::Getter<Size>::get(const YamlObject &obj) const
 template<typename T,
 	 std::enable_if_t<
 		 std::is_same_v<bool, T> ||
+		 std::is_same_v<float, T> ||
 		 std::is_same_v<double, T> ||
 		 std::is_same_v<int8_t, T> ||
 		 std::is_same_v<uint8_t, T> ||
@@ -377,6 +385,7 @@  std::optional<std::vector<T>> YamlObject::getList() const
 }
 
 template std::optional<std::vector<bool>> YamlObject::getList<bool>() const;
+template std::optional<std::vector<float>> YamlObject::getList<float>() const;
 template std::optional<std::vector<double>> YamlObject::getList<double>() const;
 template std::optional<std::vector<int8_t>> YamlObject::getList<int8_t>() const;
 template std::optional<std::vector<uint8_t>> YamlObject::getList<uint8_t>() const;