[PATCH 3/4] libcamera: controls: Add support for querying direction information

Laurent Pinchart laurent.pinchart at ideasonboard.com
Wed Nov 27 09:53:50 CET 2024


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 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.

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.

-- 
Regards,

Laurent Pinchart


More information about the libcamera-devel mailing list