[PATCH v2 3/4] libcamera: controls: Add support for querying direction information
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Dec 4 17:38:05 CET 2024
On Wed, Dec 04, 2024 at 11:46:39AM +0000, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2024-11-27 13:55:12)
> > Hi Paul,
> >
> > Thank you for the patch.
> >
> > On Wed, Nov 27, 2024 at 05:50:16PM +0900, Paul Elder wrote:
> > > Add support to ControlId for querying direction information. This allows
> > > applications to query whether a ControlId is meant for being set in
> > > controls or to be returned in metadata or both. This also has a side
> > > effect of properly encoding this information, as previously it was only
> > > mentioned losely and inconsistently in the control id definition.
> > >
> > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > >
> > > ---
> > > Changes in v2:
> > > - simplify code
> > > ---
> > > include/libcamera/controls.h | 20 ++++++++++++++-
> > > src/libcamera/control_ids.cpp.in | 4 +--
> > > src/libcamera/controls.cpp | 43 ++++++++++++++++++++++++++++++--
> > > 3 files changed, 62 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > > index 3cfe2de5973c..74a566a05db4 100644
> > > --- a/include/libcamera/controls.h
> > > +++ b/include/libcamera/controls.h
> > > @@ -17,6 +17,7 @@
> > > #include <vector>
> > >
> > > #include <libcamera/base/class.h>
> > > +#include <libcamera/base/flags.h>
> > > #include <libcamera/base/span.h>
> > >
> > > #include <libcamera/geometry.h>
> > > @@ -235,8 +236,18 @@ private:
> > > class ControlId
> > > {
> > > public:
> > > + enum class Direction {
> > > + In = (1 << 0),
> > > + Out = (1 << 1),
> > > + };
> > > +
> > > + using DirectionFlags = Flags<Direction>;
> > > +
> > > ControlId(unsigned int id, const std::string &name, const std::string &vendor,
> > > ControlType type, std::size_t size = 0,
> > > + DirectionFlags direction =
> > > + static_cast<DirectionFlags>(Direction::In) |
> > > + static_cast<DirectionFlags>(Direction::Out),
> >
> > I think I would still prefer not having a default value here, as all
> > controls should have a direction. This will require setting the
> > direction explicitly when creating ControlId instances for V4L2 controls
> > or in the control serializer, but I think that's better, as it will
> > force us to look at how we handle those instead of forgetting about them
> > because they don't appear in this patch.
>
> Does that mean we'll be able to use this to convey 'ReadOnly' controls
> from V4L2 ?
>
> I assume we can construct controls from V4L2 accordingly with this now?
> and superseed
> https://patchwork.libcamera.org/project/libcamera/list/?series=4064 and
> https://patchwork.libcamera.org/project/libcamera/list/?series=3654 with
> this implementation?
We may need a few adjustements, but I think it's worth checking. I like
when new APIs have a positive unexpected impact on other parts of
libcamera, it feels clean and right :-)
> > > const std::map<std::string, int32_t> &enumStrMap = {});
> > >
> > > unsigned int id() const { return id_; }
> > > @@ -245,6 +256,8 @@ public:
> > > ControlType type() const { return type_; }
> > > bool isArray() const { return size_ > 0; }
> > > std::size_t size() const { return size_; }
> > > + bool isInput() const { return !!(direction_ & Direction::In); }
> > > + bool isOutput() const { return !!(direction_ & Direction::Out); }
> > > const std::map<int32_t, std::string> &enumerators() const { return reverseMap_; }
> > >
> > > private:
> > > @@ -255,10 +268,13 @@ private:
> > > std::string vendor_;
> > > ControlType type_;
> > > std::size_t size_;
> > > + DirectionFlags direction_;
> > > std::map<std::string, int32_t> enumStrMap_;
> > > std::map<int32_t, std::string> reverseMap_;
> > > };
> > >
> > > +LIBCAMERA_FLAGS_ENABLE_OPERATORS(ControlId::Direction)
> > > +
> > > static inline bool operator==(unsigned int lhs, const ControlId &rhs)
> > > {
> > > return lhs == rhs.id();
> > > @@ -286,9 +302,11 @@ public:
> > > using type = T;
> > >
> > > Control(unsigned int id, const char *name, const char *vendor,
> > > + ControlId::DirectionFlags direction = ControlId::Direction::In
> > > + | ControlId::Direction::Out,
> > > const std::map<std::string, int32_t> &enumStrMap = {})
> > > : ControlId(id, name, vendor, details::control_type<std::remove_cv_t<T>>::value,
> > > - details::control_type<std::remove_cv_t<T>>::size, enumStrMap)
> > > + details::control_type<std::remove_cv_t<T>>::size, direction, enumStrMap)
> > > {
> > > }
> > >
> > > diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in
> > > index afe9e2c96610..65668d486dbc 100644
> > > --- a/src/libcamera/control_ids.cpp.in
> > > +++ b/src/libcamera/control_ids.cpp.in
> > > @@ -89,9 +89,9 @@ extern const std::map<std::string, {{ctrl.type}}> {{ctrl.name}}NameValueMap = {
> > > { "{{enum.name}}", {{enum.name}} },
> > > {%- endfor %}
> > > };
> > > -extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, "{{ctrl.name}}", "{{vendor}}", {{ctrl.name}}NameValueMap);
> > > +extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, "{{ctrl.name}}", "{{vendor}}", {{ctrl.direction}}, {{ctrl.name}}NameValueMap);
> > > {% else -%}
> > > -extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, "{{ctrl.name}}", "{{vendor}}");
> > > +extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, "{{ctrl.name}}", "{{vendor}}", {{ctrl.direction}});
> > > {% endif -%}
> > > {%- endfor %}
> > >
> > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > > index 2efecf0fc52b..f221a7e621ab 100644
> > > --- a/src/libcamera/controls.cpp
> > > +++ b/src/libcamera/controls.cpp
> > > @@ -397,14 +397,15 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen
> > > * \param[in] vendor The vendor name
> > > * \param[in] type The control data type
> > > * \param[in] size The size of the array control, or 0 if scalar control
> > > + * \param[in] direction The direction of the control, if it can be used in Controls or Metadata
> > > * \param[in] enumStrMap The map from enum names to values (optional)
> > > */
> > > ControlId::ControlId(unsigned int id, const std::string &name,
> > > const std::string &vendor, ControlType type,
> > > - std::size_t size,
> > > + std::size_t size, DirectionFlags direction,
> > > const std::map<std::string, int32_t> &enumStrMap)
> > > : id_(id), name_(name), vendor_(vendor), type_(type), size_(size),
> > > - enumStrMap_(enumStrMap)
> > > + direction_(direction), enumStrMap_(enumStrMap)
> > > {
> > > for (const auto &pair : enumStrMap_)
> > > reverseMap_[pair.second] = pair.first;
> > > @@ -440,6 +441,26 @@ ControlId::ControlId(unsigned int id, const std::string &name,
> > > * \return True if the control is an array control, false otherwise
> > > */
> > >
> > > +/**
> > > + * \fn bool ControlId::isInput() const
> > > + * \brief Determine if the control is available to be used as an input control
> > > + *
> > > + * Controls can be used either as input as a control, or as output in metadata.
> >
> > Maybe "as input in controls" to match the "in metadata" ?
> >
> > > + * This function checks if the control is allowed to be used as the former.
> > > + *
> > > + * \return True if the control can be used as an input control, false otherwise
> > > + */
> > > +
> > > +/**
> > > + * \fn bool ControlId::isOutput() const
> > > + * \brief Determine if the control is available to be used in output metadata
> > > + *
> > > + * Controls can be used either as input as a control, or as output in metadata.
> > > + * This function checks if the control is allowed to be used as the latter.
> > > + *
> > > + * \return True if the control can be returned in output metadata, false otherwise
> > > + */
> > > +
> > > /**
> > > * \fn std::size_t ControlId::size() const
> > > * \brief Retrieve the size of the control if it is an array control
> > > @@ -471,6 +492,22 @@ ControlId::ControlId(unsigned int id, const std::string &name,
> > > * \return True if \a lhs.id() is equal to \a rhs, false otherwise
> > > */
> > >
> > > +/**
> > > + * \enum ControlId::Direction
> > > + * \brief The direction that a control of the ControlId is capable of being passed from/to
> >
> > This sounds a bit weird.
> >
> > > + *
> > > + * \var ControlId::Direction::In
> > > + * \brief The control can be passed as input in controls
> > > + *
> > > + * \var ControlId::Direction::Out
> > > + * \brief The control can be returned as output in metadata
> > > + */
> > > +
> > > +/**
> > > + * \typedef ControlId::DirectionFlags
> > > + * \brief A wrapper for ControlId::Direction so that it can be used as flags
> > > + */
> > > +
> > > /**
> > > * \class Control
> > > * \brief Describe a control and its intrinsic properties
> > > @@ -504,6 +541,8 @@ ControlId::ControlId(unsigned int id, const std::string &name,
> > > * \param[in] id The control numerical ID
> > > * \param[in] name The control name
> > > * \param[in] vendor The vendor name
> > > + * \param[in] direction The direction of the control, if it can be used in
> > > + * Controls or Metadata
> > > * \param[in] enumStrMap The map from enum names to values (optional)
> > > *
> > > * The control data type is automatically deduced from the template type T.
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list