| Message ID | 20241125153003.3309066-4-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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. > 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). > 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" :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". > + */ > + > +/** > + * \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.
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@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.
On Wed, Nov 27, 2024 at 05:40:43PM +0900, Paul Elder wrote: > On Tue, Nov 26, 2024 at 01:19:10AM +0200, Laurent Pinchart wrote: > > 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@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. Both could easily pass {} for the parameter if they don't know the direction. For the control serializer that may actually be an issue, if it can't set a direction, it means we will have controls within libcamera where the isInput() and isOutput() functions will be unreliable, possibly leading to difficult to debug issues. > > > 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. Fine with me until we have a use case. > > > 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 I'm not proposing any change. I find the wording weird, saying that "controls can be used as a control". That's nothing new, it's due to the control vs. metadata naming issue that we should solve at some point. > 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. > > > > + */ > > > + > > > +/** > > > + * \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.
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), 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 + { + return static_cast<bool>( + direction_ & static_cast<DirectionFlags>(Direction::In)); + } + bool isOutput() const + { + 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), 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 * \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. + * 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 + */ + +/** + * \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.
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@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(-)