[PATCH] libcamera: controls: Handle enum values without a cast
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Sep 27 01:58:50 CEST 2024
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 at ideasonboard.com>
> > > >
> > > > brilliant, less things to type in!
> > > >
> > > > Reviewed-by: Jacopo Mondi <jacopo.mondi at 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);
>
> > [...]
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list