[PATCH 1/3] libcamera: controls: Add vendor information to ControlId

[Paul Elder]

On Thu, Oct 10, 2024 at 11:43:04PM +0300, Laurent Pinchart wrote:
> Hi Paul,
> 
> Thank you for the patch.
> 
> On Thu, Oct 10, 2024 at 05:47:17PM +0900, Paul Elder wrote:
> > Add vendor/namespace information to ControlId, so that the vendor can be
> > queried from it. This is expected to be used by applications either
> > simply to display the vendor or for it to be used for grouping in a
> > UI.
> > 
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > ---
> >  include/libcamera/controls.h         |  8 ++++++--
> >  src/libcamera/control_ids.cpp.in     |  4 ++--
> >  src/libcamera/control_serializer.cpp |  2 +-
> >  src/libcamera/controls.cpp           | 16 +++++++++++++---
> >  src/libcamera/v4l2_device.cpp        |  2 +-
> >  5 files changed, 23 insertions(+), 9 deletions(-)
> > 
> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > index ca60bbacad17..07750b76dfb8 100644
> > --- a/include/libcamera/controls.h
> > +++ b/include/libcamera/controls.h
> > @@ -235,12 +235,14 @@ class ControlId
> >  {
> >  public:
> >  	ControlId(unsigned int id, const std::string &name, ControlType type,
> > +		  const std::string &vendor,
> >  		  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_; }
> >  	ControlType type() const { return type_; }
> > +	const std::string &vendor() const { return vendor_; }
> 
> Nitpicking, I'd group the function along with name() (same for vendor_
> and name_, and the .cpp file).
> 
> >  	bool isArray() const { return size_ > 0; }
> >  	std::size_t size() const { return size_; }
> >  	const std::map<int32_t, std::string> &enumerators() const { return reverseMap_; }
> > @@ -251,6 +253,7 @@ private:
> >  	unsigned int id_;
> >  	std::string name_;
> >  	ControlType type_;
> > +	std::string vendor_;
> >  	std::size_t size_;
> >  	std::map<std::string, int32_t> enumStrMap_;
> >  	std::map<int32_t, std::string> reverseMap_;
> > @@ -282,9 +285,10 @@ class Control : public ControlId
> >  public:
> >  	using type = T;
> >  
> > -	Control(unsigned int id, const char *name, const std::map<std::string, int32_t> &enumStrMap = {})
> > +	Control(unsigned int id, const char *name, const char *vendor,
> > +		const std::map<std::string, int32_t> &enumStrMap = {})
> >  		: ControlId(id, name, details::control_type<std::remove_cv_t<T>>::value,
> > -			    details::control_type<std::remove_cv_t<T>>::size, enumStrMap)
> > +			    vendor, details::control_type<std::remove_cv_t<T>>::size, enumStrMap)
> >  	{
> >  	}
> >  
> > diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in
> > index 3a20493119bb..afe9e2c96610 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}}", {{ctrl.name}}NameValueMap);
> > +extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, "{{ctrl.name}}", "{{vendor}}", {{ctrl.name}}NameValueMap);
> >  {% else -%}
> > -extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, "{{ctrl.name}}");
> > +extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, "{{ctrl.name}}", "{{vendor}}");
> >  {% endif -%}
> >  {%- endfor %}
> >  
> > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp
> > index 52fd714fb4bd..0da65fdc0fad 100644
> > --- a/src/libcamera/control_serializer.cpp
> > +++ b/src/libcamera/control_serializer.cpp
> > @@ -498,7 +498,7 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &
> >  			 * debugging purpose.
> >  			 */
> >  			controlIds_.emplace_back(std::make_unique<ControlId>(entry->id,
> > -									     "", type));
> > +									     "", type, "local"));
> 
> The local ID map is used for V4L2 controls only, should we use "v4l2" as
> the vendor name, like we do in V4L2Device::v4l2ControlId() ? Or could
> the ID map be used for something else later ? We don't have a control
> name here, so I suppose it's no big deal if the vendor name is
> meaningless. I'm fine keeping it as-is.

Oh, I thought that in theory technically you could have a completely
separate control space (not that you could in practice).


Paul

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> >  			(*localIdMap)[entry->id] = controlIds_.back().get();
> >  		}
> >  
> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > index 62185d643ecd..7b55fecbc032 100644
> > --- a/src/libcamera/controls.cpp
> > +++ b/src/libcamera/controls.cpp
> > @@ -396,11 +396,14 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen
> >   * \param[in] name The control name
> >   * \param[in] type The control data type
> >   * \param[in] size The size of the array control, or 0 if scalar control
> > + * \param[in] vendor The vendor name
> >   * \param[in] enumStrMap The map from enum names to values (optional)
> >   */
> >  ControlId::ControlId(unsigned int id, const std::string &name, ControlType type,
> > -		     std::size_t size, const std::map<std::string, int32_t> &enumStrMap)
> > -	: id_(id), name_(name), type_(type), size_(size), enumStrMap_(enumStrMap)
> > +		     const std::string &vendor, std::size_t size,
> > +		     const std::map<std::string, int32_t> &enumStrMap)
> > +	: id_(id), name_(name), type_(type), vendor_(vendor), size_(size),
> > +	  enumStrMap_(enumStrMap)
> >  {
> >  	for (const auto &pair : enumStrMap_)
> >  		reverseMap_[pair.second] = pair.first;
> > @@ -430,6 +433,12 @@ ControlId::ControlId(unsigned int id, const std::string &name, ControlType type,
> >   * \return True if the control is an array control, false otherwise
> >   */
> >  
> > +/**
> > + * \fn const std::string &ControlId::vendor() const
> > + * \brief Retrieve the vendor name
> > + * \return The vendor name, as a string
> > + */
> > +
> >  /**
> >   * \fn std::size_t ControlId::size() const
> >   * \brief Retrieve the size of the control if it is an array control
> > @@ -489,10 +498,11 @@ ControlId::ControlId(unsigned int id, const std::string &name, ControlType type,
> >   */
> >  
> >  /**
> > - * \fn Control::Control(unsigned int id, const char *name)
> > + * \fn Control::Control(unsigned int id, const char *name, const char *vendor)
> >   * \brief Construct a Control instance
> >   * \param[in] id The control numerical ID
> >   * \param[in] name The control name
> > + * \param[in] vendor The vendor name
> >   * \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 68add4f2e642..7aa2b92ab8e7 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -520,7 +520,7 @@ 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, type);
> > +	return std::make_unique<ControlId>(ctrl.id, name, type, "v4l2");
> >  }
> >  
> >  /**
> 
> -- 
> Regards,
> 
> Laurent Pinchart