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

Paul Elder paul.elder at ideasonboard.com
Mon Dec 9 12:00:41 CET 2024


On Fri, Dec 06, 2024 at 12:59:09PM +0000, Kieran Bingham wrote:
> Quoting Paul Elder (2024-11-29 09:19:15)
> > 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 v3:
> > - make direction parameter to ControlId and Control consutrctors
> >   mandatory
> >   - this entails expanding the control serializer/deserializer for
> >     V4L2Device and ControlSerializer to properly handle the direction
> >   - also the constructor parameters have to be reordered, compared to
> >     the last vesrion at least
> > 
> > Changes in v2:
> > - simplify code
> > ---
> >  include/libcamera/controls.h         | 19 ++++++++++--
> >  include/libcamera/ipa/ipa_controls.h |  3 +-
> >  src/libcamera/control_ids.cpp.in     |  4 +--
> >  src/libcamera/control_serializer.cpp | 11 ++++++-
> >  src/libcamera/controls.cpp           | 45 ++++++++++++++++++++++++++--
> >  src/libcamera/v4l2_device.cpp        | 10 ++++++-
> >  6 files changed, 82 insertions(+), 10 deletions(-)
> > 
> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > index 3cfe2de5973c..13bb914badbf 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,14 +236,24 @@ private:
> >  class ControlId
> >  {
> >  public:
> > +       enum class Direction {
> > +               In = (1 << 0),
> > +               Out = (1 << 1),
> > +       };
> 
> I'm curious that I think the previous patch is using the flags that are
> only just defined here ...
> 
> > +
> > +       using DirectionFlags = Flags<Direction>;
> > +
> >         ControlId(unsigned int id, const std::string &name, const std::string &vendor,
> > -                 ControlType type, std::size_t size = 0,
> > +                 ControlType type, DirectionFlags direction,
> > +                 std::size_t size = 0,
> >                   const std::map<std::string, int32_t> &enumStrMap = {});
> >  
> >         unsigned int id() const { return id_; }
> >         const std::string &name() const { return name_; }
> >         const std::string &vendor() const { return vendor_; }
> >         ControlType type() const { return type_; }
> > +       bool isInput() const { return !!(direction_ & Direction::In); }
> > +       bool isOutput() const { return !!(direction_ & Direction::Out); }
> >         bool isArray() const { return size_ > 0; }
> >         std::size_t size() const { return size_; }
> >         const std::map<int32_t, std::string> &enumerators() const { return reverseMap_; }
> > @@ -254,11 +265,14 @@ private:
> >         std::string name_;
> >         std::string vendor_;
> >         ControlType type_;
> > +       DirectionFlags direction_;
> >         std::size_t size_;
> >         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 +300,10 @@ public:
> >         using type = T;
> >  
> >         Control(unsigned int id, const char *name, const char *vendor,
> > +               ControlId::DirectionFlags direction,
> >                 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)
> > +                           direction, details::control_type<std::remove_cv_t<T>>::size, enumStrMap)
> >         {
> >         }
> >  
> > diff --git a/include/libcamera/ipa/ipa_controls.h b/include/libcamera/ipa/ipa_controls.h
> > index 5fd13394fcef..980668c86bcc 100644
> > --- a/include/libcamera/ipa/ipa_controls.h
> > +++ b/include/libcamera/ipa/ipa_controls.h
> > @@ -46,7 +46,8 @@ struct ipa_control_info_entry {
> >         uint32_t id;
> >         uint32_t type;
> >         uint32_t offset;
> > -       uint32_t padding[1];
> > +       uint8_t direction;
> > +       uint8_t padding[3];
> 
> What's the padding for here? Alignment ? or something else?
> Should we 'add more' while we're in an ABI break ? or is it needed at
> all?

I assumed it was for alignment.


Paul

> 
> >  };
> >  
> >  #ifdef __cplusplus
> > 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/control_serializer.cpp b/src/libcamera/control_serializer.cpp
> > index 0a5e8220a0ff..0804c178f0d4 100644
> > --- a/src/libcamera/control_serializer.cpp
> > +++ b/src/libcamera/control_serializer.cpp
> > @@ -281,6 +281,9 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,
> >                 entry.id = id->id();
> >                 entry.type = id->type();
> >                 entry.offset = values.offset();
> > +               entry.direction =
> > +                       (id->isInput() ? static_cast<uint8_t>(ControlId::Direction::In) : 0) |
> > +                       (id->isOutput() ? static_cast<uint8_t>(ControlId::Direction::Out) : 0);
> 
> Is this necessary ? Can't the raw value just be serialized? (oh or
> perhaps the raw value can't be accessed in which case it probably is
> needed ... unless we add an id->direction(); which is probably better
> than re-encoding the same thing ...)
> 
> >                 entries.write(&entry);
> >  
> >                 store(info, values);
> > @@ -493,12 +496,18 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &
> >  
> >                 /* If we're using a local id map, populate it. */
> >                 if (localIdMap) {
> > +                       ControlId::DirectionFlags flags;
> > +                       if (entry->direction & static_cast<uint8_t>(ControlId::Direction::In))
> > +                               flags |= ControlId::Direction::In;
> > +                       if (entry->direction & static_cast<uint8_t>(ControlId::Direction::Out))
> > +                               flags |= ControlId::Direction::Out;
> 
> 
> and here - can't we just do a direct usage? or is there an issue with
> serialising 'Flags' ?
> 
> >                         /**
> >                          * \todo Find a way to preserve the control name for
> >                          * debugging purpose.
> >                          */
> >                         controlIds_.emplace_back(std::make_unique<ControlId>(entry->id,
> > -                                                                            "", "local", type));
> > +                                                                            "", "local", type,
> > +                                                                            flags));
> >                         (*localIdMap)[entry->id] = controlIds_.back().get();
> >                 }
> >  
> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > index 2efecf0fc52b..23b71fac8e84 100644
> > --- a/src/libcamera/controls.cpp
> > +++ b/src/libcamera/controls.cpp
> > @@ -396,15 +396,16 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen
> >   * \param[in] name The control name
> >   * \param[in] vendor The vendor name
> >   * \param[in] type The control data type
> > + * \param[in] direction The direction of the control, if it can be used in Controls or Metadata
> >   * \param[in] size The size of the array control, or 0 if scalar control
> >   * \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,
> > +                    DirectionFlags direction, std::size_t size,
> >                      const std::map<std::string, int32_t> &enumStrMap)
> > -       : id_(id), name_(name), vendor_(vendor), type_(type), size_(size),
> > -         enumStrMap_(enumStrMap)
> > +       : id_(id), name_(name), vendor_(vendor), type_(type),
> > +         direction_(direction), size_(size), enumStrMap_(enumStrMap)
> >  {
> >         for (const auto &pair : enumStrMap_)
> >                 reverseMap_[pair.second] = pair.first;
> > @@ -434,6 +435,26 @@ ControlId::ControlId(unsigned int id, const std::string &name,
> >   * \return The control data type
> >   */
> >  
> > +/**
> > + * \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 in controls, 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 in controls, 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 bool ControlId::isArray() const
> >   * \brief Determine if the control 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 the control is capable of being passed from/to
> > + *
> > + * \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.
> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > index 7d21cf15fec1..4634ccc392b6 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -520,7 +520,15 @@ std::unique_ptr<ControlId> V4L2Device::v4l2ControlId(const v4l2_query_ext_ctrl &
> >         const std::string name(static_cast<const char *>(ctrl.name), len);
> >         const ControlType type = v4l2CtrlType(ctrl.type);
> >  
> > -       return std::make_unique<ControlId>(ctrl.id, name, "v4l2", type);
> > +       ControlId::DirectionFlags flags;
> 
> I expect it's fine, but the above is uninitialised,
> 
> > +       if (ctrl.flags & V4L2_CTRL_FLAG_READ_ONLY)
> > +               flags |= ControlId::Direction::Out;
> 
> So you shouldn't be 'or'ing in here, just assign directly.
> 
> Same for the two below.
> 
> > +       else if (ctrl.flags & V4L2_CTRL_FLAG_WRITE_ONLY)
> > +               flags |= ControlId::Direction::In;
> > +       else
> > +               flags |= ControlId::Direction::In | ControlId::Direction::Out;
> > +
> > +       return std::make_unique<ControlId>(ctrl.id, name, "v4l2", type, flags);
> >  }
> >  
> >  /**
> > -- 
> > 2.39.2
> >


More information about the libcamera-devel mailing list