[1/3] libcamera: controls: Add vendor information to ControlId
diff mbox series

Message ID 20241010084719.712485-2-paul.elder@ideasonboard.com
State Accepted
Headers show
Series
  • libcamera: controls: Add namespace to ControlId
Related show

Commit Message

Paul Elder Oct. 10, 2024, 8:47 a.m. UTC 
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@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(-)

Comments

Laurent Pinchart Oct. 10, 2024, 8:43 p.m. UTC | #1 
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@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.

Reviewed-by: Laurent Pinchart <laurent.pinchart@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");
>  }
>  
>  /**
Paul Elder Oct. 16, 2024, 11:21 a.m. UTC | #2 
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@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@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

Patch
diff mbox series 

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_; }
 	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"));
 			(*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");
 }
 
 /**