[libcamera-devel,3/9] libcamera: controls: Store control name in ControlId
diff mbox series

Message ID 20191007224642.6597-4-laurent.pinchart@ideasonboard.com
State Superseded
Headers show
Series
  • Use ControlList for both libcamera and V4L2 controls
Related show

Commit Message

Laurent Pinchart Oct. 7, 2019, 10:46 p.m. UTC 
The ControlId class stores a pointer to the control name. This works
fine for statically-defined controls, but requires code that allocates
controls dynamically (for instance based on control discovery on a V4L2
device) to keep a list of control names in separate storage. To ease
usage of dynamically allocated controls, store a copy of the control
name string in the ControlId class.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/libcamera/controls.h | 6 +++---
 src/libcamera/controls.cpp   | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

Comments

Jacopo Mondi Oct. 9, 2019, 8:18 a.m. UTC | #1 
Hi Laurent,

On Tue, Oct 08, 2019 at 01:46:36AM +0300, Laurent Pinchart wrote:
> The ControlId class stores a pointer to the control name. This works
> fine for statically-defined controls, but requires code that allocates
> controls dynamically (for instance based on control discovery on a V4L2
> device) to keep a list of control names in separate storage. To ease
> usage of dynamically allocated controls, store a copy of the control
> name string in the ControlId class.
>

I would go as far as removing this from the ControlId, as it will need
to be serialized and it is used for debug purposes only.

In case we want to retrieve it for debug, we can use the controls
global map you have reintroduced in patch 1

If you like best to keep it:
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  include/libcamera/controls.h | 6 +++---
>  src/libcamera/controls.cpp   | 2 +-
>  2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index 49ff1a6f747f..a5a6a135ec16 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -54,11 +54,11 @@ class ControlId
>  {
>  public:
>  	unsigned int id() const { return id_; }
> -	const char *name() const { return name_; }
> +	const std::string &name() const { return name_; }
>  	ControlType type() const { return type_; }
>
>  protected:
> -	ControlId(unsigned int id, const char *name, ControlType type)
> +	ControlId(unsigned int id, const std::string &name, ControlType type)
>  		: id_(id), name_(name), type_(type)
>  	{
>  	}
> @@ -68,7 +68,7 @@ private:
>  	ControlId &operator=(const ControlId &) = delete;
>
>  	unsigned int id_;
> -	const char *name_;
> +	std::string name_;
>  	ControlType type_;
>  };
>
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> index f862a09adef9..e528dd80a2a7 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -205,7 +205,7 @@ std::string ControlValue::toString() const
>   */
>
>  /**
> - * \fn ControlId::ControlId(unsigned int id, const char *name, ControlType type)
> + * \fn ControlId::ControlId(unsigned int id, const std::string &name, ControlType type)
>   * \brief Construct a ControlId instance
>   * \param[in] id The control numerical ID
>   * \param[in] name The control name
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Oct. 9, 2019, 8:23 a.m. UTC | #2 
Hi Jacopo,

On Wed, Oct 09, 2019 at 10:18:22AM +0200, Jacopo Mondi wrote:
> On Tue, Oct 08, 2019 at 01:46:36AM +0300, Laurent Pinchart wrote:
> > The ControlId class stores a pointer to the control name. This works
> > fine for statically-defined controls, but requires code that allocates
> > controls dynamically (for instance based on control discovery on a V4L2
> > device) to keep a list of control names in separate storage. To ease
> > usage of dynamically allocated controls, store a copy of the control
> > name string in the ControlId class.
> 
> I would go as far as removing this from the ControlId, as it will need
> to be serialized and it is used for debug purposes only.
> 
> In case we want to retrieve it for debug, we can use the controls
> global map you have reintroduced in patch 1

But that map stores the name in ControlId :-) Where would you store it ?

> If you like best to keep it:
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  include/libcamera/controls.h | 6 +++---
> >  src/libcamera/controls.cpp   | 2 +-
> >  2 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > index 49ff1a6f747f..a5a6a135ec16 100644
> > --- a/include/libcamera/controls.h
> > +++ b/include/libcamera/controls.h
> > @@ -54,11 +54,11 @@ class ControlId
> >  {
> >  public:
> >  	unsigned int id() const { return id_; }
> > -	const char *name() const { return name_; }
> > +	const std::string &name() const { return name_; }
> >  	ControlType type() const { return type_; }
> >
> >  protected:
> > -	ControlId(unsigned int id, const char *name, ControlType type)
> > +	ControlId(unsigned int id, const std::string &name, ControlType type)
> >  		: id_(id), name_(name), type_(type)
> >  	{
> >  	}
> > @@ -68,7 +68,7 @@ private:
> >  	ControlId &operator=(const ControlId &) = delete;
> >
> >  	unsigned int id_;
> > -	const char *name_;
> > +	std::string name_;
> >  	ControlType type_;
> >  };
> >
> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > index f862a09adef9..e528dd80a2a7 100644
> > --- a/src/libcamera/controls.cpp
> > +++ b/src/libcamera/controls.cpp
> > @@ -205,7 +205,7 @@ std::string ControlValue::toString() const
> >   */
> >
> >  /**
> > - * \fn ControlId::ControlId(unsigned int id, const char *name, ControlType type)
> > + * \fn ControlId::ControlId(unsigned int id, const std::string &name, ControlType type)
> >   * \brief Construct a ControlId instance
> >   * \param[in] id The control numerical ID
> >   * \param[in] name The control name
Jacopo Mondi Oct. 9, 2019, 8:38 a.m. UTC | #3 
Hi Laurent,

On Wed, Oct 09, 2019 at 11:23:10AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Wed, Oct 09, 2019 at 10:18:22AM +0200, Jacopo Mondi wrote:
> > On Tue, Oct 08, 2019 at 01:46:36AM +0300, Laurent Pinchart wrote:
> > > The ControlId class stores a pointer to the control name. This works
> > > fine for statically-defined controls, but requires code that allocates
> > > controls dynamically (for instance based on control discovery on a V4L2
> > > device) to keep a list of control names in separate storage. To ease
> > > usage of dynamically allocated controls, store a copy of the control
> > > name string in the ControlId class.
> >
> > I would go as far as removing this from the ControlId, as it will need
> > to be serialized and it is used for debug purposes only.
> >
> > In case we want to retrieve it for debug, we can use the controls
> > global map you have reintroduced in patch 1
>
> But that map stores the name in ControlId :-) Where would you store it ?
>

A derived class only used there?

Anyway, scratch this, too complex now and not really something that
concerns this series.

Please take my tag in.

Thanks
  j

> > If you like best to keep it:
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  include/libcamera/controls.h | 6 +++---
> > >  src/libcamera/controls.cpp   | 2 +-
> > >  2 files changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > > index 49ff1a6f747f..a5a6a135ec16 100644
> > > --- a/include/libcamera/controls.h
> > > +++ b/include/libcamera/controls.h
> > > @@ -54,11 +54,11 @@ class ControlId
> > >  {
> > >  public:
> > >  	unsigned int id() const { return id_; }
> > > -	const char *name() const { return name_; }
> > > +	const std::string &name() const { return name_; }
> > >  	ControlType type() const { return type_; }
> > >
> > >  protected:
> > > -	ControlId(unsigned int id, const char *name, ControlType type)
> > > +	ControlId(unsigned int id, const std::string &name, ControlType type)
> > >  		: id_(id), name_(name), type_(type)
> > >  	{
> > >  	}
> > > @@ -68,7 +68,7 @@ private:
> > >  	ControlId &operator=(const ControlId &) = delete;
> > >
> > >  	unsigned int id_;
> > > -	const char *name_;
> > > +	std::string name_;
> > >  	ControlType type_;
> > >  };
> > >
> > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > > index f862a09adef9..e528dd80a2a7 100644
> > > --- a/src/libcamera/controls.cpp
> > > +++ b/src/libcamera/controls.cpp
> > > @@ -205,7 +205,7 @@ std::string ControlValue::toString() const
> > >   */
> > >
> > >  /**
> > > - * \fn ControlId::ControlId(unsigned int id, const char *name, ControlType type)
> > > + * \fn ControlId::ControlId(unsigned int id, const std::string &name, ControlType type)
> > >   * \brief Construct a ControlId instance
> > >   * \param[in] id The control numerical ID
> > >   * \param[in] name The control name
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
index 49ff1a6f747f..a5a6a135ec16 100644
--- a/include/libcamera/controls.h
+++ b/include/libcamera/controls.h
@@ -54,11 +54,11 @@  class ControlId
 {
 public:
 	unsigned int id() const { return id_; }
-	const char *name() const { return name_; }
+	const std::string &name() const { return name_; }
 	ControlType type() const { return type_; }
 
 protected:
-	ControlId(unsigned int id, const char *name, ControlType type)
+	ControlId(unsigned int id, const std::string &name, ControlType type)
 		: id_(id), name_(name), type_(type)
 	{
 	}
@@ -68,7 +68,7 @@  private:
 	ControlId &operator=(const ControlId &) = delete;
 
 	unsigned int id_;
-	const char *name_;
+	std::string name_;
 	ControlType type_;
 };
 
diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
index f862a09adef9..e528dd80a2a7 100644
--- a/src/libcamera/controls.cpp
+++ b/src/libcamera/controls.cpp
@@ -205,7 +205,7 @@  std::string ControlValue::toString() const
  */
 
 /**
- * \fn ControlId::ControlId(unsigned int id, const char *name, ControlType type)
+ * \fn ControlId::ControlId(unsigned int id, const std::string &name, ControlType type)
  * \brief Construct a ControlId instance
  * \param[in] id The control numerical ID
  * \param[in] name The control name