[PATCH 3/4] libcamera: controls: Add support for querying direction information
Paul Elder
paul.elder at ideasonboard.com
Wed Nov 27 09:40:43 CET 2024
Hi Laurent,
Thank you for the review.
On Tue, Nov 26, 2024 at 01:19:10AM +0200, Laurent Pinchart wrote:
> Hi Paul,
>
> Thank you for the patch.
>
> On Tue, Nov 26, 2024 at 12:30:02AM +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>
> > ---
> > include/libcamera/controls.h | 27 +++++++++++++++++++-
> > src/libcamera/control_ids.cpp.in | 4 +--
> > src/libcamera/controls.cpp | 42 ++++++++++++++++++++++++++++++--
> > 3 files changed, 68 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > index 3cfe2de5973c..cd338ac0d653 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,
> > + const DirectionFlags &direction =
> > + static_cast<DirectionFlags>(Direction::In) |
> > + static_cast<DirectionFlags>(Direction::Out),
>
> I think we could make this argument explicit, without a default value,
> as all controls have a direction.
It was causing problems in generating v4l2 controls in V4L2Device, and
in ControlSerializer, so I'll leave it in.
>
> > const std::map<std::string, int32_t> &enumStrMap = {});
> >
> > unsigned int id() const { return id_; }
> > @@ -245,6 +256,16 @@ public:
> > ControlType type() const { return type_; }
> > bool isArray() const { return size_ > 0; }
> > std::size_t size() const { return size_; }
> > + bool isInput() const
>
> With the changes proposed in the cover letter this now holds on a single
> line:
>
> bool isInput() const { return !!(direction_ & Direction::In); }
>
> > + {
> > + return static_cast<bool>(
> > + direction_ & static_cast<DirectionFlags>(Direction::In));
> > + }
> > + bool isOutput() const
>
> Same here.
>
> > + {
> > + return static_cast<bool>(
> > + direction_ & static_cast<DirectionFlags>(Direction::Out));
> > + }
> > const std::map<int32_t, std::string> &enumerators() const { return reverseMap_; }
> >
> > private:
> > @@ -255,6 +276,7 @@ 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_;
> > };
> > @@ -286,9 +308,12 @@ public:
> > using type = T;
> >
> > Control(unsigned int id, const char *name, const char *vendor,
> > + const ControlId::DirectionFlags &direction =
> > + static_cast<ControlId::DirectionFlags>(ControlId::Direction::In) |
> > + static_cast<ControlId::DirectionFlags>(ControlId::Direction::Out),
>
> Do we want to pass this as an argument to the constructor, or as a
> template argument ? The latter would allow handling the direction at
> compile time (not sure what the use case would be though).
I think I'll keep it as regular parameters.
>
> > 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..30eb17e7f064 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
>
> Line wrap ?
>
> > * \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, const 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.
> > + * 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.
>
> "Controls can be used [...] as a control"
I'm not sure I understand the change that you want here. I think this is
more explicit and informative. It also appeases all the bikeshedders
that want an explicit mention somewhere and we don't have any mention of
controls in the application developer guide so this might be the second
best place to put it.
>
> :S I don't have a better suggestion for now that wouldn't involve a
> tree-wide rename, so I'm OK with this.
>
> > + * 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
> > + *
> > + * \var ControlId::Direction::In
> > + * \brief The control can be passed in controls as input
> > + *
> > + * \var ControlId::Direction::Out
> > + * \brief The control can be returned in output as metadata
>
> The two definitions are not consistent: "in controls as input" vs. "in
> output as metadata".
Oops, right.
Thanks,
Paul
>
> > + */
> > +
> > +/**
> > + * \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,7 @@ 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.
More information about the libcamera-devel
mailing list