[PATCH] libcamera: controls: Handle enum values without a cast
Barnabás Pőcze
pobrn at protonmail.com
Thu Sep 26 17:07:05 CEST 2024
Hi
2024. szeptember 26., csütörtök 12:36 keltezéssel, Laurent Pinchart <laurent.pinchart at 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 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"...
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
More information about the libcamera-devel
mailing list