[PATCH] libcamera: controls: Handle enum values without a cast
Barnabás Pőcze
pobrn at protonmail.com
Fri Sep 27 12:27:01 CEST 2024
2024. szeptember 27., péntek 1:58 keltezéssel, Laurent Pinchart <laurent.pinchart at 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 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.
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
>
More information about the libcamera-devel
mailing list