libcamera: controls: Handle enum values without a cast
diff mbox series

Message ID 20240926002427.27703-1-laurent.pinchart@ideasonboard.com
State Accepted
Commit e6da224926b0d18dc4ad814843f2561e4e16a027
Headers show
Series
  • libcamera: controls: Handle enum values without a cast
Related show

Commit Message

Laurent Pinchart Sept. 26, 2024, 12:24 a.m. UTC 
When constructing a ControlValue from an enum value, an explicit cast to
int32_t is needed as we use int32_t as the underlying type for all
enumerated controls. This makes users of ControlValue more complex. To
simplify them, specialize the control_type template for enum types, to
support construction of ControlValue directly without a cast.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/libcamera/controls.h         | 6 +++++-
 src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-
 2 files changed, 6 insertions(+), 2 deletions(-)


base-commit: f2088eb91fd6477b152233b9031cb115ca1ae824

Comments

Jacopo Mondi Sept. 26, 2024, 9:13 a.m. UTC | #1 
Hi Laurent

On Thu, Sep 26, 2024 at 03:24:27AM GMT, Laurent Pinchart wrote:
> When constructing a ControlValue from an enum value, an explicit cast to
> int32_t is needed as we use int32_t as the underlying type for all
> enumerated controls. This makes users of ControlValue more complex. To
> simplify them, specialize the control_type template for enum types, to
> support construction of ControlValue directly without a cast.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

brilliant, less things to type in!

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

Thanks
  j

> ---
>  include/libcamera/controls.h         | 6 +++++-
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-
>  2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index 25f67ed948a3..c5131870d402 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -39,7 +39,7 @@ enum ControlType {
>
>  namespace details {
>
> -template<typename T>
> +template<typename T, typename = std::void_t<>>
>  struct control_type {
>  };
>
> @@ -102,6 +102,10 @@ struct control_type<Span<T, N>> : public control_type<std::remove_cv_t<T>> {
>  	static constexpr std::size_t size = N;
>  };
>
> +template<typename T>
> +struct control_type<T, std::enable_if_t<std::is_enum_v<T>>> : public control_type<int32_t> {
> +};
> +
>  } /* namespace details */
>
>  class ControlValue
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 430aa902eec8..29d3c6c194c1 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -958,7 +958,7 @@ int PipelineHandlerIPU3::updateControls(IPU3CameraData *data)
>  		values.reserve(testPatternModes.size());
>
>  		for (auto pattern : testPatternModes)
> -			values.emplace_back(static_cast<int32_t>(pattern));
> +			values.emplace_back(pattern);
>
>  		controls[&controls::draft::TestPatternMode] = ControlInfo(values);
>  	}
>
> base-commit: f2088eb91fd6477b152233b9031cb115ca1ae824
> --
> Regards,
>
> Laurent Pinchart
>
Kieran Bingham Sept. 26, 2024, 10:28 a.m. UTC | #2 
Quoting Jacopo Mondi (2024-09-26 10:13:44)
> Hi Laurent
> 
> On Thu, Sep 26, 2024 at 03:24:27AM GMT, Laurent Pinchart wrote:
> > When constructing a ControlValue from an enum value, an explicit cast to
> > int32_t is needed as we use int32_t as the underlying type for all
> > enumerated controls. This makes users of ControlValue more complex. To
> > simplify them, specialize the control_type template for enum types, to
> > support construction of ControlValue directly without a cast.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> brilliant, less things to type in!
> 
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
> Thanks
>   j
> 
> > ---
> >  include/libcamera/controls.h         | 6 +++++-
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-
> >  2 files changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > index 25f67ed948a3..c5131870d402 100644
> > --- a/include/libcamera/controls.h
> > +++ b/include/libcamera/controls.h
> > @@ -39,7 +39,7 @@ enum ControlType {
> >
> >  namespace details {
> >
> > -template<typename T>
> > +template<typename T, typename = std::void_t<>>
> >  struct control_type {
> >  };
> >
> > @@ -102,6 +102,10 @@ struct control_type<Span<T, N>> : public control_type<std::remove_cv_t<T>> {
> >       static constexpr std::size_t size = N;
> >  };
> >
> > +template<typename T>
> > +struct control_type<T, std::enable_if_t<std::is_enum_v<T>>> : public control_type<int32_t> {
> > +};
> > +

I have no idea how you get this black magic to work ... :-)

But if it works and is reducing boilerplate casts which impact readability
then \o/

So I think this makes a new templated control_type of the given enum
type, which is derived from an int32_t ...

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

> >  } /* namespace details */
> >
> >  class ControlValue
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 430aa902eec8..29d3c6c194c1 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -958,7 +958,7 @@ int PipelineHandlerIPU3::updateControls(IPU3CameraData *data)
> >               values.reserve(testPatternModes.size());
> >
> >               for (auto pattern : testPatternModes)
> > -                     values.emplace_back(static_cast<int32_t>(pattern));
> > +                     values.emplace_back(pattern);
> >
> >               controls[&controls::draft::TestPatternMode] = ControlInfo(values);
> >       }
> >
> > base-commit: f2088eb91fd6477b152233b9031cb115ca1ae824
> > --
> > Regards,
> >
> > Laurent Pinchart
> >
Laurent Pinchart Sept. 26, 2024, 10:36 a.m. UTC | #3 
On Thu, Sep 26, 2024 at 11:28:41AM +0100, Kieran Bingham wrote:
> Quoting Jacopo Mondi (2024-09-26 10:13:44)
> > On Thu, Sep 26, 2024 at 03:24:27AM GMT, Laurent Pinchart wrote:
> > > When constructing a ControlValue from an enum value, an explicit cast to
> > > int32_t is needed as we use int32_t as the underlying type for all
> > > enumerated controls. This makes users of ControlValue more complex. To
> > > simplify them, specialize the control_type template for enum types, to
> > > support construction of ControlValue directly without a cast.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > brilliant, less things to type in!
> > 
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > 
> > Thanks
> >   j
> > 
> > > ---
> > >  include/libcamera/controls.h         | 6 +++++-
> > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-
> > >  2 files changed, 6 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > > index 25f67ed948a3..c5131870d402 100644
> > > --- a/include/libcamera/controls.h
> > > +++ b/include/libcamera/controls.h
> > > @@ -39,7 +39,7 @@ enum ControlType {
> > >
> > >  namespace details {
> > >
> > > -template<typename T>
> > > +template<typename T, typename = std::void_t<>>
> > >  struct control_type {
> > >  };
> > >
> > > @@ -102,6 +102,10 @@ struct control_type<Span<T, N>> : public control_type<std::remove_cv_t<T>> {
> > >       static constexpr std::size_t size = N;
> > >  };
> > >
> > > +template<typename T>
> > > +struct control_type<T, std::enable_if_t<std::is_enum_v<T>>> : public control_type<int32_t> {
> > > +};
> > > +
> 
> I have no idea how you get this black magic to work ... :-)

Sometimes I'm not sure myself :-)

> But if it works and is reducing boilerplate casts which impact readability
> then \o/
> 
> So I think this makes a new templated control_type of the given enum
> type, which is derived from an int32_t ...

It introduces a new specialization for the control_type structure
template, which is enabled only when T is a enumeration type. The
specialization inherits from control_type<int32_t> to make it explicit
that we're storing the value as an int32_t. I could have copied

	static constexpr ControlType value = ControlTypeInteger32;
	static constexpr std::size_t size = 0;

from struct control_type<int32_t> instead, but inheriting expresses the
goal better I think.

I had to add a second template parameter to the base template, as I
needed a place to introduce the std::enable_if_t<> in the template
specialization. Writing

template<typename T, std::enable_if_t<std::is_enum_v<T>>> * = nullptr>
struct control_type<T> : public control_type<int32_t> {
};

is not valid, as it's not a specialization of the base template.

As the second template parameter of the base template has a default
value, none of the other template specialization need to be modified.

> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > >  } /* namespace details */
> > >
> > >  class ControlValue
> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > index 430aa902eec8..29d3c6c194c1 100644
> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > @@ -958,7 +958,7 @@ int PipelineHandlerIPU3::updateControls(IPU3CameraData *data)
> > >               values.reserve(testPatternModes.size());
> > >
> > >               for (auto pattern : testPatternModes)
> > > -                     values.emplace_back(static_cast<int32_t>(pattern));
> > > +                     values.emplace_back(pattern);
> > >
> > >               controls[&controls::draft::TestPatternMode] = ControlInfo(values);
> > >       }
> > >
> > > base-commit: f2088eb91fd6477b152233b9031cb115ca1ae824
Barnabás Pőcze Sept. 26, 2024, 3:07 p.m. UTC | #4 
Hi


2024. szeptember 26., csütörtök 12:36 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta:

> On Thu, Sep 26, 2024 at 11:28:41AM +0100, Kieran Bingham wrote:
> > Quoting Jacopo Mondi (2024-09-26 10:13:44)
> > > On Thu, Sep 26, 2024 at 03:24:27AM GMT, Laurent Pinchart wrote:
> > > > When constructing a ControlValue from an enum value, an explicit cast to
> > > > int32_t is needed as we use int32_t as the underlying type for all
> > > > enumerated controls. This makes users of ControlValue more complex. To
> > > > simplify them, specialize the control_type template for enum types, to
> > > > support construction of ControlValue directly without a cast.
> > > >
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >
> > > brilliant, less things to type in!
> > >
> > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > >
> > > Thanks
> > >   j
> > >
> > > > ---
> > > >  include/libcamera/controls.h         | 6 +++++-
> > > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-
> > > >  2 files changed, 6 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > > > index 25f67ed948a3..c5131870d402 100644
> > > > --- a/include/libcamera/controls.h
> > > > +++ b/include/libcamera/controls.h
> > > > @@ -39,7 +39,7 @@ enum ControlType {
> > > >
> > > >  namespace details {
> > > >
> > > > -template<typename T>
> > > > +template<typename T, typename = std::void_t<>>
> > > >  struct control_type {
> > > >  };
> > > >
> > > > @@ -102,6 +102,10 @@ struct control_type<Span<T, N>> : public control_type<std::remove_cv_t<T>> {
> > > >       static constexpr std::size_t size = N;
> > > >  };
> > > >
> > > > +template<typename T>
> > > > +struct control_type<T, std::enable_if_t<std::is_enum_v<T>>> : public control_type<int32_t> {
> > > > +};
> > > > +
> >
> > I have no idea how you get this black magic to work ... :-)
> 
> Sometimes I'm not sure myself :-)

In C++20 one could just say something along the lines of

template<typename T> requires (std::is_enum_v<T>)
struct control_type<T> : control_type<int32_t> { };

A lot less of SFINAE "magic"...

But more importantly, consider

  enum class thing : std::uint16_t { a };
  ControlValue { thing::a };

One would expect this to work, but it won't because the assertion in

  void ControlValue::set(ControlType type, bool isArray, const void *data,
  		       std::size_t numElements, std::size_t elementSize)
  {
  	ASSERT(elementSize == ControlValueSize[type]);

aborts the process since `elementSize == sizeof(std::uint16_t) != sizeof(std::int32_t)`.

This affects every enum type whose underlying type is not the same size as `std::int32_t`.
This can be addressed by restricting the specialization even more with something like

  std::is_enum_v<T> && sizeof(T) == sizeof(int32_t)

Or properly supporting every enum type. The patch below shows a quick prototype
trying to do that, note that it uses C++20 for brevity but equivalent results can
be achieved with sufficient SFINAE usage. Also note, that it still does not
properly handle arrays of enums.


diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
index 25f67ed9..db3919d5 100644
--- a/include/libcamera/controls.h
+++ b/include/libcamera/controls.h
@@ -53,30 +54,34 @@ template<>
 struct control_type<bool> {
 	static constexpr ControlType value = ControlTypeBool;
 	static constexpr std::size_t size = 0;
+	using value_type = bool;
 };
 
 template<>
 struct control_type<uint8_t> {
 	static constexpr ControlType value = ControlTypeByte;
-	static constexpr std::size_t size = 0;
+	using value_type = uint8_t;
 };
 
 template<>
 struct control_type<int32_t> {
 	static constexpr ControlType value = ControlTypeInteger32;
 	static constexpr std::size_t size = 0;
+	using value_type = int32_t;
 };
 
 template<>
 struct control_type<int64_t> {
 	static constexpr ControlType value = ControlTypeInteger64;
 	static constexpr std::size_t size = 0;
+	using value_type = int64_t;
 };
 
 template<>
 struct control_type<float> {
 	static constexpr ControlType value = ControlTypeFloat;
 	static constexpr std::size_t size = 0;
+	using value_type = float;
 };
 
 template<>
@@ -89,12 +94,14 @@ template<>
 struct control_type<Rectangle> {
 	static constexpr ControlType value = ControlTypeRectangle;
 	static constexpr std::size_t size = 0;
+	using value_type = Rectangle;
 };
 
 template<>
 struct control_type<Size> {
 	static constexpr ControlType value = ControlTypeSize;
 	static constexpr std::size_t size = 0;
+	using value_type = Size;
 };
 
 template<typename T, std::size_t N>
@@ -102,38 +109,24 @@ struct control_type<Span<T, N>> : public control_type<std::remove_cv_t<T>> {
 	static constexpr std::size_t size = N;
 };
 
+template<typename T>
+	requires (std::is_enum_v<T> && sizeof(T) <= sizeof(int32_t))
+struct control_type<T> : control_type<int32_t> {
+	static constexpr auto convert(T x) { return static_cast<int32_t>(x); }
+};
+
+template<typename T>
+	requires (std::is_enum_v<T> && sizeof(int32_t) < sizeof(T) && sizeof(T) <= sizeof(int64_t))
+struct control_type<T> : control_type<int64_t> {
+	static constexpr auto convert(T x) { return static_cast<int64_t>(x); }
+};
+
 } /* namespace details */
 
 class ControlValue
 {
 public:
 	ControlValue();
-
-#ifndef __DOXYGEN__
-	template<typename T, std::enable_if_t<!details::is_span<T>::value &&
-					      details::control_type<T>::value &&
-					      !std::is_same<std::string, std::remove_cv_t<T>>::value,
-					      std::nullptr_t> = nullptr>
-	ControlValue(const T &value)
-		: type_(ControlTypeNone), numElements_(0)
-	{
-		set(details::control_type<std::remove_cv_t<T>>::value, false,
-		    &value, 1, sizeof(T));
-	}
-
-	template<typename T, std::enable_if_t<details::is_span<T>::value ||
-					      std::is_same<std::string, std::remove_cv_t<T>>::value,
-					      std::nullptr_t> = nullptr>
-#else
-	template<typename T>
-#endif
-	ControlValue(const T &value)
-		: type_(ControlTypeNone), numElements_(0)
-	{
-		set(details::control_type<std::remove_cv_t<T>>::value, true,
-		    value.data(), value.size(), sizeof(typename T::value_type));
-	}
-
 	~ControlValue();
 
 	ControlValue(const ControlValue &other);
@@ -182,28 +175,46 @@ public:
 		return T{ value, numElements_ };
 	}
 
-#ifndef __DOXYGEN__
-	template<typename T, std::enable_if_t<!details::is_span<T>::value &&
-					      !std::is_same<std::string, std::remove_cv_t<T>>::value,
-					      std::nullptr_t> = nullptr>
+	template<typename T>
+		requires (requires { details::control_type<std::remove_cv_t<T>>::value; }
+			  && !details::is_span<std::remove_cv_t<T>>::value
+			  && !std::is_same_v<std::string, std::remove_cv_t<T>>)
 	void set(const T &value)
 	{
-		set(details::control_type<std::remove_cv_t<T>>::value, false,
-		    reinterpret_cast<const void *>(&value), 1, sizeof(T));
+		using CT = details::control_type<std::remove_cv_t<T>>;
+
+		if constexpr (requires {
+			typename CT::value_type;
+			requires !std::is_same_v<typename CT::value_type, std::remove_cv_t<T>>;
+		}) {
+			typename CT::value_type cvalue = CT::convert(value);
+
+			set(CT::value, false,
+			    reinterpret_cast<const void *>(&cvalue), 1, sizeof(cvalue));
+		}
+		else {
+			set(CT::value, false,
+			    reinterpret_cast<const void *>(&value), 1, sizeof(value));
+		}
 	}
 
-	template<typename T, std::enable_if_t<details::is_span<T>::value ||
-					      std::is_same<std::string, std::remove_cv_t<T>>::value,
-					      std::nullptr_t> = nullptr>
-#else
 	template<typename T>
-#endif
+		requires (requires { details::control_type<std::remove_cv_t<T>>::value; }
+			  && (details::is_span<T>::value || std::is_same_v<std::string, std::remove_cv_t<T>>))
 	void set(const T &value)
 	{
 		set(details::control_type<std::remove_cv_t<T>>::value, true,
 		    value.data(), value.size(), sizeof(typename T::value_type));
 	}
 
+	template<typename T>
+	ControlValue(const T &value)
+		requires (requires { set(value); })
+		: type_(ControlTypeNone), numElements_(0)
+	{
+		set(value);
+	}
+
 	void reserve(ControlType type, bool isArray = false,
 		     std::size_t numElements = 1);
 


> [...]


Regards,
Barnabás Pőcze
Laurent Pinchart Sept. 26, 2024, 11:58 p.m. UTC | #5 
Hi Barnabás,

On Thu, Sep 26, 2024 at 03:07:05PM +0000, Barnabás Pőcze wrote:
> 2024. szeptember 26., csütörtök 12:36 keltezéssel, Laurent Pinchart írta:
> > On Thu, Sep 26, 2024 at 11:28:41AM +0100, Kieran Bingham wrote:
> > > Quoting Jacopo Mondi (2024-09-26 10:13:44)
> > > > On Thu, Sep 26, 2024 at 03:24:27AM GMT, Laurent Pinchart wrote:
> > > > > When constructing a ControlValue from an enum value, an explicit cast to
> > > > > int32_t is needed as we use int32_t as the underlying type for all
> > > > > enumerated controls. This makes users of ControlValue more complex. To
> > > > > simplify them, specialize the control_type template for enum types, to
> > > > > support construction of ControlValue directly without a cast.
> > > > >
> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > >
> > > > brilliant, less things to type in!
> > > >
> > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > >
> > > > Thanks
> > > >   j
> > > >
> > > > > ---
> > > > >  include/libcamera/controls.h         | 6 +++++-
> > > > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-
> > > > >  2 files changed, 6 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > > > > index 25f67ed948a3..c5131870d402 100644
> > > > > --- a/include/libcamera/controls.h
> > > > > +++ b/include/libcamera/controls.h
> > > > > @@ -39,7 +39,7 @@ enum ControlType {
> > > > >
> > > > >  namespace details {
> > > > >
> > > > > -template<typename T>
> > > > > +template<typename T, typename = std::void_t<>>
> > > > >  struct control_type {
> > > > >  };
> > > > >
> > > > > @@ -102,6 +102,10 @@ struct control_type<Span<T, N>> : public control_type<std::remove_cv_t<T>> {
> > > > >       static constexpr std::size_t size = N;
> > > > >  };
> > > > >
> > > > > +template<typename T>
> > > > > +struct control_type<T, std::enable_if_t<std::is_enum_v<T>>> : public control_type<int32_t> {
> > > > > +};
> > > > > +
> > >
> > > I have no idea how you get this black magic to work ... :-)
> > 
> > Sometimes I'm not sure myself :-)
> 
> In C++20 one could just say something along the lines of
> 
> template<typename T> requires (std::is_enum_v<T>)
> struct control_type<T> : control_type<int32_t> { };
> 
> A lot less of SFINAE "magic"...

There are quite a few C++20 features I'd love to use in libcamera. We
could finally replace Span with std::span :-)

> But more importantly, consider
> 
>   enum class thing : std::uint16_t { a };
>   ControlValue { thing::a };
> 
> One would expect this to work, but it won't because the assertion in
> 
>   void ControlValue::set(ControlType type, bool isArray, const void *data,
>   		       std::size_t numElements, std::size_t elementSize)
>   {
>   	ASSERT(elementSize == ControlValueSize[type]);
> 
> aborts the process since `elementSize == sizeof(std::uint16_t) != sizeof(std::int32_t)`.
> 
> This affects every enum type whose underlying type is not the same size as `std::int32_t`.
> This can be addressed by restricting the specialization even more with something like
> 
>   std::is_enum_v<T> && sizeof(T) == sizeof(int32_t)
> 
> Or properly supporting every enum type. The patch below shows a quick prototype
> trying to do that, note that it uses C++20 for brevity but equivalent results can
> be achieved with sufficient SFINAE usage. Also note, that it still does not
> properly handle arrays of enums.

Couldn't it be done simply by inheriting from
control_type<std::underlying_type_t<T>> ? I've considered that, but
given that we have standardized enums to use int32_t as the underlying
type of libcamera controls, I decided not to go that way.

> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index 25f67ed9..db3919d5 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -53,30 +54,34 @@ template<>
>  struct control_type<bool> {
>  	static constexpr ControlType value = ControlTypeBool;
>  	static constexpr std::size_t size = 0;
> +	using value_type = bool;
>  };
>  
>  template<>
>  struct control_type<uint8_t> {
>  	static constexpr ControlType value = ControlTypeByte;
> -	static constexpr std::size_t size = 0;
> +	using value_type = uint8_t;
>  };
>  
>  template<>
>  struct control_type<int32_t> {
>  	static constexpr ControlType value = ControlTypeInteger32;
>  	static constexpr std::size_t size = 0;
> +	using value_type = int32_t;
>  };
>  
>  template<>
>  struct control_type<int64_t> {
>  	static constexpr ControlType value = ControlTypeInteger64;
>  	static constexpr std::size_t size = 0;
> +	using value_type = int64_t;
>  };
>  
>  template<>
>  struct control_type<float> {
>  	static constexpr ControlType value = ControlTypeFloat;
>  	static constexpr std::size_t size = 0;
> +	using value_type = float;
>  };
>  
>  template<>
> @@ -89,12 +94,14 @@ template<>
>  struct control_type<Rectangle> {
>  	static constexpr ControlType value = ControlTypeRectangle;
>  	static constexpr std::size_t size = 0;
> +	using value_type = Rectangle;
>  };
>  
>  template<>
>  struct control_type<Size> {
>  	static constexpr ControlType value = ControlTypeSize;
>  	static constexpr std::size_t size = 0;
> +	using value_type = Size;
>  };
>  
>  template<typename T, std::size_t N>
> @@ -102,38 +109,24 @@ struct control_type<Span<T, N>> : public control_type<std::remove_cv_t<T>> {
>  	static constexpr std::size_t size = N;
>  };
>  
> +template<typename T>
> +	requires (std::is_enum_v<T> && sizeof(T) <= sizeof(int32_t))
> +struct control_type<T> : control_type<int32_t> {
> +	static constexpr auto convert(T x) { return static_cast<int32_t>(x); }
> +};
> +
> +template<typename T>
> +	requires (std::is_enum_v<T> && sizeof(int32_t) < sizeof(T) && sizeof(T) <= sizeof(int64_t))
> +struct control_type<T> : control_type<int64_t> {
> +	static constexpr auto convert(T x) { return static_cast<int64_t>(x); }
> +};
> +
>  } /* namespace details */
>  
>  class ControlValue
>  {
>  public:
>  	ControlValue();
> -
> -#ifndef __DOXYGEN__
> -	template<typename T, std::enable_if_t<!details::is_span<T>::value &&
> -					      details::control_type<T>::value &&
> -					      !std::is_same<std::string, std::remove_cv_t<T>>::value,
> -					      std::nullptr_t> = nullptr>
> -	ControlValue(const T &value)
> -		: type_(ControlTypeNone), numElements_(0)
> -	{
> -		set(details::control_type<std::remove_cv_t<T>>::value, false,
> -		    &value, 1, sizeof(T));
> -	}
> -
> -	template<typename T, std::enable_if_t<details::is_span<T>::value ||
> -					      std::is_same<std::string, std::remove_cv_t<T>>::value,
> -					      std::nullptr_t> = nullptr>
> -#else
> -	template<typename T>
> -#endif
> -	ControlValue(const T &value)
> -		: type_(ControlTypeNone), numElements_(0)
> -	{
> -		set(details::control_type<std::remove_cv_t<T>>::value, true,
> -		    value.data(), value.size(), sizeof(typename T::value_type));
> -	}
> -
>  	~ControlValue();
>  
>  	ControlValue(const ControlValue &other);
> @@ -182,28 +175,46 @@ public:
>  		return T{ value, numElements_ };
>  	}
>  
> -#ifndef __DOXYGEN__
> -	template<typename T, std::enable_if_t<!details::is_span<T>::value &&
> -					      !std::is_same<std::string, std::remove_cv_t<T>>::value,
> -					      std::nullptr_t> = nullptr>
> +	template<typename T>
> +		requires (requires { details::control_type<std::remove_cv_t<T>>::value; }
> +			  && !details::is_span<std::remove_cv_t<T>>::value
> +			  && !std::is_same_v<std::string, std::remove_cv_t<T>>)
>  	void set(const T &value)
>  	{
> -		set(details::control_type<std::remove_cv_t<T>>::value, false,
> -		    reinterpret_cast<const void *>(&value), 1, sizeof(T));
> +		using CT = details::control_type<std::remove_cv_t<T>>;
> +
> +		if constexpr (requires {
> +			typename CT::value_type;
> +			requires !std::is_same_v<typename CT::value_type, std::remove_cv_t<T>>;
> +		}) {
> +			typename CT::value_type cvalue = CT::convert(value);
> +
> +			set(CT::value, false,
> +			    reinterpret_cast<const void *>(&cvalue), 1, sizeof(cvalue));
> +		}
> +		else {
> +			set(CT::value, false,
> +			    reinterpret_cast<const void *>(&value), 1, sizeof(value));
> +		}
>  	}
>  
> -	template<typename T, std::enable_if_t<details::is_span<T>::value ||
> -					      std::is_same<std::string, std::remove_cv_t<T>>::value,
> -					      std::nullptr_t> = nullptr>
> -#else
>  	template<typename T>
> -#endif
> +		requires (requires { details::control_type<std::remove_cv_t<T>>::value; }
> +			  && (details::is_span<T>::value || std::is_same_v<std::string, std::remove_cv_t<T>>))
>  	void set(const T &value)
>  	{
>  		set(details::control_type<std::remove_cv_t<T>>::value, true,
>  		    value.data(), value.size(), sizeof(typename T::value_type));
>  	}
>  
> +	template<typename T>
> +	ControlValue(const T &value)
> +		requires (requires { set(value); })
> +		: type_(ControlTypeNone), numElements_(0)
> +	{
> +		set(value);
> +	}
> +
>  	void reserve(ControlType type, bool isArray = false,
>  		     std::size_t numElements = 1);
>  
> > [...]
Barnabás Pőcze Sept. 27, 2024, 10:27 a.m. UTC | #6 
2024. szeptember 27., péntek 1:58 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta:


> Hi Barnabás,
> 
> On Thu, Sep 26, 2024 at 03:07:05PM +0000, Barnabás Pőcze wrote:
> > 2024. szeptember 26., csütörtök 12:36 keltezéssel, Laurent Pinchart írta:
> > > On Thu, Sep 26, 2024 at 11:28:41AM +0100, Kieran Bingham wrote:
> > > > Quoting Jacopo Mondi (2024-09-26 10:13:44)
> > > > > On Thu, Sep 26, 2024 at 03:24:27AM GMT, Laurent Pinchart wrote:
> > > > > > When constructing a ControlValue from an enum value, an explicit cast to
> > > > > > int32_t is needed as we use int32_t as the underlying type for all
> > > > > > enumerated controls. This makes users of ControlValue more complex. To
> > > > > > simplify them, specialize the control_type template for enum types, to
> > > > > > support construction of ControlValue directly without a cast.
> > > > > >
> > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > >
> > > > > brilliant, less things to type in!
> > > > >
> > > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > >
> > > > > Thanks
> > > > >   j
> > > > >
> > > > > > ---
> > > > > >  include/libcamera/controls.h         | 6 +++++-
> > > > > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-
> > > > > >  2 files changed, 6 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > > > > > index 25f67ed948a3..c5131870d402 100644
> > > > > > --- a/include/libcamera/controls.h
> > > > > > +++ b/include/libcamera/controls.h
> > > > > > @@ -39,7 +39,7 @@ enum ControlType {
> > > > > >
> > > > > >  namespace details {
> > > > > >
> > > > > > -template<typename T>
> > > > > > +template<typename T, typename = std::void_t<>>
> > > > > >  struct control_type {
> > > > > >  };
> > > > > >
> > > > > > @@ -102,6 +102,10 @@ struct control_type<Span<T, N>> : public control_type<std::remove_cv_t<T>> {
> > > > > >       static constexpr std::size_t size = N;
> > > > > >  };
> > > > > >
> > > > > > +template<typename T>
> > > > > > +struct control_type<T, std::enable_if_t<std::is_enum_v<T>>> : public control_type<int32_t> {
> > > > > > +};
> > > > > > +
> > > >
> > > > I have no idea how you get this black magic to work ... :-)
> > >
> > > Sometimes I'm not sure myself :-)
> >
> > In C++20 one could just say something along the lines of
> >
> > template<typename T> requires (std::is_enum_v<T>)
> > struct control_type<T> : control_type<int32_t> { };
> >
> > A lot less of SFINAE "magic"...
> 
> There are quite a few C++20 features I'd love to use in libcamera. We
> could finally replace Span with std::span :-)
> 
> > But more importantly, consider
> >
> >   enum class thing : std::uint16_t { a };
> >   ControlValue { thing::a };
> >
> > One would expect this to work, but it won't because the assertion in
> >
> >   void ControlValue::set(ControlType type, bool isArray, const void *data,
> >   		       std::size_t numElements, std::size_t elementSize)
> >   {
> >   	ASSERT(elementSize == ControlValueSize[type]);
> >
> > aborts the process since `elementSize == sizeof(std::uint16_t) != sizeof(std::int32_t)`.
> >
> > This affects every enum type whose underlying type is not the same size as `std::int32_t`.
> > This can be addressed by restricting the specialization even more with something like
> >
> >   std::is_enum_v<T> && sizeof(T) == sizeof(int32_t)
> >
> > Or properly supporting every enum type. The patch below shows a quick prototype
> > trying to do that, note that it uses C++20 for brevity but equivalent results can
> > be achieved with sufficient SFINAE usage. Also note, that it still does not
> > properly handle arrays of enums.
> 
> Couldn't it be done simply by inheriting from
> control_type<std::underlying_type_t<T>> ? I've considered that, but
> given that we have standardized enums to use int32_t as the underlying
> type of libcamera controls, I decided not to go that way.

I guess that should work. But only for enums which have int{32,64}_t as their
underlying type since those are the only two integer types supported. I would
imagine uint{32,64}_t are also prime candidates for enums. But I suppose it
shouldn't be too difficult to add (all) other fixed-width integer types as well.


Regards,
Barnabás Pőcze


> 
> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > index 25f67ed9..db3919d5 100644
> > --- a/include/libcamera/controls.h
> > +++ b/include/libcamera/controls.h
> > @@ -53,30 +54,34 @@ template<>
> >  struct control_type<bool> {
> >  	static constexpr ControlType value = ControlTypeBool;
> >  	static constexpr std::size_t size = 0;
> > +	using value_type = bool;
> >  };
> >
> >  template<>
> >  struct control_type<uint8_t> {
> >  	static constexpr ControlType value = ControlTypeByte;
> > -	static constexpr std::size_t size = 0;
> > +	using value_type = uint8_t;
> >  };
> >
> >  template<>
> >  struct control_type<int32_t> {
> >  	static constexpr ControlType value = ControlTypeInteger32;
> >  	static constexpr std::size_t size = 0;
> > +	using value_type = int32_t;
> >  };
> >
> >  template<>
> >  struct control_type<int64_t> {
> >  	static constexpr ControlType value = ControlTypeInteger64;
> >  	static constexpr std::size_t size = 0;
> > +	using value_type = int64_t;
> >  };
> >
> >  template<>
> >  struct control_type<float> {
> >  	static constexpr ControlType value = ControlTypeFloat;
> >  	static constexpr std::size_t size = 0;
> > +	using value_type = float;
> >  };
> >
> >  template<>
> > @@ -89,12 +94,14 @@ template<>
> >  struct control_type<Rectangle> {
> >  	static constexpr ControlType value = ControlTypeRectangle;
> >  	static constexpr std::size_t size = 0;
> > +	using value_type = Rectangle;
> >  };
> >
> >  template<>
> >  struct control_type<Size> {
> >  	static constexpr ControlType value = ControlTypeSize;
> >  	static constexpr std::size_t size = 0;
> > +	using value_type = Size;
> >  };
> >
> >  template<typename T, std::size_t N>
> > @@ -102,38 +109,24 @@ struct control_type<Span<T, N>> : public control_type<std::remove_cv_t<T>> {
> >  	static constexpr std::size_t size = N;
> >  };
> >
> > +template<typename T>
> > +	requires (std::is_enum_v<T> && sizeof(T) <= sizeof(int32_t))
> > +struct control_type<T> : control_type<int32_t> {
> > +	static constexpr auto convert(T x) { return static_cast<int32_t>(x); }
> > +};
> > +
> > +template<typename T>
> > +	requires (std::is_enum_v<T> && sizeof(int32_t) < sizeof(T) && sizeof(T) <= sizeof(int64_t))
> > +struct control_type<T> : control_type<int64_t> {
> > +	static constexpr auto convert(T x) { return static_cast<int64_t>(x); }
> > +};
> > +
> >  } /* namespace details */
> >
> >  class ControlValue
> >  {
> >  public:
> >  	ControlValue();
> > -
> > -#ifndef __DOXYGEN__
> > -	template<typename T, std::enable_if_t<!details::is_span<T>::value &&
> > -					      details::control_type<T>::value &&
> > -					      !std::is_same<std::string, std::remove_cv_t<T>>::value,
> > -					      std::nullptr_t> = nullptr>
> > -	ControlValue(const T &value)
> > -		: type_(ControlTypeNone), numElements_(0)
> > -	{
> > -		set(details::control_type<std::remove_cv_t<T>>::value, false,
> > -		    &value, 1, sizeof(T));
> > -	}
> > -
> > -	template<typename T, std::enable_if_t<details::is_span<T>::value ||
> > -					      std::is_same<std::string, std::remove_cv_t<T>>::value,
> > -					      std::nullptr_t> = nullptr>
> > -#else
> > -	template<typename T>
> > -#endif
> > -	ControlValue(const T &value)
> > -		: type_(ControlTypeNone), numElements_(0)
> > -	{
> > -		set(details::control_type<std::remove_cv_t<T>>::value, true,
> > -		    value.data(), value.size(), sizeof(typename T::value_type));
> > -	}
> > -
> >  	~ControlValue();
> >
> >  	ControlValue(const ControlValue &other);
> > @@ -182,28 +175,46 @@ public:
> >  		return T{ value, numElements_ };
> >  	}
> >
> > -#ifndef __DOXYGEN__
> > -	template<typename T, std::enable_if_t<!details::is_span<T>::value &&
> > -					      !std::is_same<std::string, std::remove_cv_t<T>>::value,
> > -					      std::nullptr_t> = nullptr>
> > +	template<typename T>
> > +		requires (requires { details::control_type<std::remove_cv_t<T>>::value; }
> > +			  && !details::is_span<std::remove_cv_t<T>>::value
> > +			  && !std::is_same_v<std::string, std::remove_cv_t<T>>)
> >  	void set(const T &value)
> >  	{
> > -		set(details::control_type<std::remove_cv_t<T>>::value, false,
> > -		    reinterpret_cast<const void *>(&value), 1, sizeof(T));
> > +		using CT = details::control_type<std::remove_cv_t<T>>;
> > +
> > +		if constexpr (requires {
> > +			typename CT::value_type;
> > +			requires !std::is_same_v<typename CT::value_type, std::remove_cv_t<T>>;
> > +		}) {
> > +			typename CT::value_type cvalue = CT::convert(value);
> > +
> > +			set(CT::value, false,
> > +			    reinterpret_cast<const void *>(&cvalue), 1, sizeof(cvalue));
> > +		}
> > +		else {
> > +			set(CT::value, false,
> > +			    reinterpret_cast<const void *>(&value), 1, sizeof(value));
> > +		}
> >  	}
> >
> > -	template<typename T, std::enable_if_t<details::is_span<T>::value ||
> > -					      std::is_same<std::string, std::remove_cv_t<T>>::value,
> > -					      std::nullptr_t> = nullptr>
> > -#else
> >  	template<typename T>
> > -#endif
> > +		requires (requires { details::control_type<std::remove_cv_t<T>>::value; }
> > +			  && (details::is_span<T>::value || std::is_same_v<std::string, std::remove_cv_t<T>>))
> >  	void set(const T &value)
> >  	{
> >  		set(details::control_type<std::remove_cv_t<T>>::value, true,
> >  		    value.data(), value.size(), sizeof(typename T::value_type));
> >  	}
> >
> > +	template<typename T>
> > +	ControlValue(const T &value)
> > +		requires (requires { set(value); })
> > +		: type_(ControlTypeNone), numElements_(0)
> > +	{
> > +		set(value);
> > +	}
> > +
> >  	void reserve(ControlType type, bool isArray = false,
> >  		     std::size_t numElements = 1);
> >
> > > [...]
> 
> --
> Regards,
> 
> Laurent Pinchart
>

Patch
diff mbox series 

diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
index 25f67ed948a3..c5131870d402 100644
--- a/include/libcamera/controls.h
+++ b/include/libcamera/controls.h
@@ -39,7 +39,7 @@  enum ControlType {
 
 namespace details {
 
-template<typename T>
+template<typename T, typename = std::void_t<>>
 struct control_type {
 };
 
@@ -102,6 +102,10 @@  struct control_type<Span<T, N>> : public control_type<std::remove_cv_t<T>> {
 	static constexpr std::size_t size = N;
 };
 
+template<typename T>
+struct control_type<T, std::enable_if_t<std::is_enum_v<T>>> : public control_type<int32_t> {
+};
+
 } /* namespace details */
 
 class ControlValue
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index 430aa902eec8..29d3c6c194c1 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -958,7 +958,7 @@  int PipelineHandlerIPU3::updateControls(IPU3CameraData *data)
 		values.reserve(testPatternModes.size());
 
 		for (auto pattern : testPatternModes)
-			values.emplace_back(static_cast<int32_t>(pattern));
+			values.emplace_back(pattern);
 
 		controls[&controls::draft::TestPatternMode] = ControlInfo(values);
 	}