[libcamera-devel,02/10] libcamera: v4l2_controls: Move V4L2ControlId out of V4L2ControlInfo
diff mbox series

Message ID 20191013232755.3292-3-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • Merge V4L2ControlInfoMap and ControlInfoMap
Related show

Commit Message

Laurent Pinchart Oct. 13, 2019, 11:27 p.m. UTC 
From: Jacopo Mondi <jacopo@jmondi.org>

In order to reconcile the libcamera and V4L2 control info maps, we need
to move the V4L2ControlId embedded in V4L2ControlInfo map out of the
class. Store the V4L2ControlId instances in the V4L2Device that creates
them, and only reference them from V4L2ControlInfo.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/include/v4l2_controls.h | 7 ++++---
 src/libcamera/include/v4l2_device.h   | 4 +++-
 src/libcamera/v4l2_controls.cpp       | 6 ++++--
 src/libcamera/v4l2_device.cpp         | 3 ++-
 4 files changed, 13 insertions(+), 7 deletions(-)

Comments

Niklas Söderlund Oct. 15, 2019, 12:12 a.m. UTC | #1 
Hi Jacopo, Laurent,

Thanks for your patch.

On 2019-10-14 02:27:48 +0300, Laurent Pinchart wrote:
> From: Jacopo Mondi <jacopo@jmondi.org>
> 
> In order to reconcile the libcamera and V4L2 control info maps, we need
> to move the V4L2ControlId embedded in V4L2ControlInfo map out of the
> class. Store the V4L2ControlId instances in the V4L2Device that creates
> them, and only reference them from V4L2ControlInfo.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  src/libcamera/include/v4l2_controls.h | 7 ++++---
>  src/libcamera/include/v4l2_device.h   | 4 +++-
>  src/libcamera/v4l2_controls.cpp       | 6 ++++--
>  src/libcamera/v4l2_device.cpp         | 3 ++-
>  4 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h
> index 133f5262febf..4d7ac1a133c7 100644
> --- a/src/libcamera/include/v4l2_controls.h
> +++ b/src/libcamera/include/v4l2_controls.h
> @@ -28,13 +28,14 @@ public:
>  class V4L2ControlInfo
>  {
>  public:
> -	V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl);
> +	V4L2ControlInfo(const V4L2ControlId &id,
> +			const struct v4l2_query_ext_ctrl &ctrl);
>  
> -	const ControlId &id() const { return id_; }
> +	const ControlId &id() const { return *id_; }
>  	const ControlRange &range() const { return range_; }
>  
>  private:
> -	V4L2ControlId id_;
> +	const V4L2ControlId *id_;
>  	ControlRange range_;
>  };
>  
> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> index daa762d58d2b..5a5b85827f23 100644
> --- a/src/libcamera/include/v4l2_device.h
> +++ b/src/libcamera/include/v4l2_device.h
> @@ -8,7 +8,8 @@
>  #define __LIBCAMERA_V4L2_DEVICE_H__
>  
>  #include <map>
> -#include <string>
> +#include <memory>
> +#include <vector>
>  
>  #include <linux/videodev2.h>
>  
> @@ -48,6 +49,7 @@ private:
>  			    const struct v4l2_ext_control *v4l2Ctrls,
>  			    unsigned int count);
>  
> +	std::vector<std::unique_ptr<V4L2ControlId>> controlIds_;
>  	V4L2ControlInfoMap controls_;
>  	std::string deviceNode_;
>  	int fd_;
> diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> index dcf31b7a8f26..12c4fb271ba5 100644
> --- a/src/libcamera/v4l2_controls.cpp
> +++ b/src/libcamera/v4l2_controls.cpp
> @@ -122,10 +122,12 @@ V4L2ControlId::V4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl)
>  
>  /**
>   * \brief Construct a V4L2ControlInfo from a struct v4l2_query_ext_ctrl
> + * \param[in] id The V4L2 control ID
>   * \param[in] ctrl The struct v4l2_query_ext_ctrl as returned by the kernel
>   */
> -V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
> -	: id_(ctrl)
> +V4L2ControlInfo::V4L2ControlInfo(const V4L2ControlId &id,
> +				 const struct v4l2_query_ext_ctrl &ctrl)
> +	: id_(&id)
>  {
>  	if (ctrl.type == V4L2_CTRL_TYPE_INTEGER64)
>  		range_ = ControlRange(static_cast<int64_t>(ctrl.minimum),
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 54cc214ecce9..144a60b4fe93 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -386,9 +386,10 @@ void V4L2Device::listControls()
>  			continue;
>  		}
>  
> +		controlIds_.emplace_back(utils::make_unique<V4L2ControlId>(ctrl));
>  		ctrls.emplace(std::piecewise_construct,
>  			      std::forward_as_tuple(ctrl.id),
> -			      std::forward_as_tuple(ctrl));
> +			      std::forward_as_tuple(*controlIds_.back().get(), ctrl));
>  	}
>  
>  	controls_ = std::move(ctrls);
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Jacopo Mondi Oct. 15, 2019, 2:51 p.m. UTC | #2 
Hi Laurent,

On Mon, Oct 14, 2019 at 02:27:48AM +0300, Laurent Pinchart wrote:
> From: Jacopo Mondi <jacopo@jmondi.org>
>
> In order to reconcile the libcamera and V4L2 control info maps, we need
> to move the V4L2ControlId embedded in V4L2ControlInfo map out of the
> class. Store the V4L2ControlId instances in the V4L2Device that creates
> them, and only reference them from V4L2ControlInfo.
>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Much better now that ControlId are stored as unique_ptrs.
Great!

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> ---
>  src/libcamera/include/v4l2_controls.h | 7 ++++---
>  src/libcamera/include/v4l2_device.h   | 4 +++-
>  src/libcamera/v4l2_controls.cpp       | 6 ++++--
>  src/libcamera/v4l2_device.cpp         | 3 ++-
>  4 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h
> index 133f5262febf..4d7ac1a133c7 100644
> --- a/src/libcamera/include/v4l2_controls.h
> +++ b/src/libcamera/include/v4l2_controls.h
> @@ -28,13 +28,14 @@ public:
>  class V4L2ControlInfo
>  {
>  public:
> -	V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl);
> +	V4L2ControlInfo(const V4L2ControlId &id,
> +			const struct v4l2_query_ext_ctrl &ctrl);
>
> -	const ControlId &id() const { return id_; }
> +	const ControlId &id() const { return *id_; }
>  	const ControlRange &range() const { return range_; }
>
>  private:
> -	V4L2ControlId id_;
> +	const V4L2ControlId *id_;
>  	ControlRange range_;
>  };
>
> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> index daa762d58d2b..5a5b85827f23 100644
> --- a/src/libcamera/include/v4l2_device.h
> +++ b/src/libcamera/include/v4l2_device.h
> @@ -8,7 +8,8 @@
>  #define __LIBCAMERA_V4L2_DEVICE_H__
>
>  #include <map>
> -#include <string>
> +#include <memory>
> +#include <vector>
>
>  #include <linux/videodev2.h>
>
> @@ -48,6 +49,7 @@ private:
>  			    const struct v4l2_ext_control *v4l2Ctrls,
>  			    unsigned int count);
>
> +	std::vector<std::unique_ptr<V4L2ControlId>> controlIds_;
>  	V4L2ControlInfoMap controls_;
>  	std::string deviceNode_;
>  	int fd_;
> diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> index dcf31b7a8f26..12c4fb271ba5 100644
> --- a/src/libcamera/v4l2_controls.cpp
> +++ b/src/libcamera/v4l2_controls.cpp
> @@ -122,10 +122,12 @@ V4L2ControlId::V4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl)
>
>  /**
>   * \brief Construct a V4L2ControlInfo from a struct v4l2_query_ext_ctrl
> + * \param[in] id The V4L2 control ID
>   * \param[in] ctrl The struct v4l2_query_ext_ctrl as returned by the kernel
>   */
> -V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
> -	: id_(ctrl)
> +V4L2ControlInfo::V4L2ControlInfo(const V4L2ControlId &id,
> +				 const struct v4l2_query_ext_ctrl &ctrl)
> +	: id_(&id)
>  {
>  	if (ctrl.type == V4L2_CTRL_TYPE_INTEGER64)
>  		range_ = ControlRange(static_cast<int64_t>(ctrl.minimum),
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 54cc214ecce9..144a60b4fe93 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -386,9 +386,10 @@ void V4L2Device::listControls()
>  			continue;
>  		}
>
> +		controlIds_.emplace_back(utils::make_unique<V4L2ControlId>(ctrl));
>  		ctrls.emplace(std::piecewise_construct,
>  			      std::forward_as_tuple(ctrl.id),
> -			      std::forward_as_tuple(ctrl));
> +			      std::forward_as_tuple(*controlIds_.back().get(), ctrl));
>  	}
>
>  	controls_ = std::move(ctrls);
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series 

diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h
index 133f5262febf..4d7ac1a133c7 100644
--- a/src/libcamera/include/v4l2_controls.h
+++ b/src/libcamera/include/v4l2_controls.h
@@ -28,13 +28,14 @@  public:
 class V4L2ControlInfo
 {
 public:
-	V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl);
+	V4L2ControlInfo(const V4L2ControlId &id,
+			const struct v4l2_query_ext_ctrl &ctrl);
 
-	const ControlId &id() const { return id_; }
+	const ControlId &id() const { return *id_; }
 	const ControlRange &range() const { return range_; }
 
 private:
-	V4L2ControlId id_;
+	const V4L2ControlId *id_;
 	ControlRange range_;
 };
 
diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
index daa762d58d2b..5a5b85827f23 100644
--- a/src/libcamera/include/v4l2_device.h
+++ b/src/libcamera/include/v4l2_device.h
@@ -8,7 +8,8 @@ 
 #define __LIBCAMERA_V4L2_DEVICE_H__
 
 #include <map>
-#include <string>
+#include <memory>
+#include <vector>
 
 #include <linux/videodev2.h>
 
@@ -48,6 +49,7 @@  private:
 			    const struct v4l2_ext_control *v4l2Ctrls,
 			    unsigned int count);
 
+	std::vector<std::unique_ptr<V4L2ControlId>> controlIds_;
 	V4L2ControlInfoMap controls_;
 	std::string deviceNode_;
 	int fd_;
diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
index dcf31b7a8f26..12c4fb271ba5 100644
--- a/src/libcamera/v4l2_controls.cpp
+++ b/src/libcamera/v4l2_controls.cpp
@@ -122,10 +122,12 @@  V4L2ControlId::V4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl)
 
 /**
  * \brief Construct a V4L2ControlInfo from a struct v4l2_query_ext_ctrl
+ * \param[in] id The V4L2 control ID
  * \param[in] ctrl The struct v4l2_query_ext_ctrl as returned by the kernel
  */
-V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
-	: id_(ctrl)
+V4L2ControlInfo::V4L2ControlInfo(const V4L2ControlId &id,
+				 const struct v4l2_query_ext_ctrl &ctrl)
+	: id_(&id)
 {
 	if (ctrl.type == V4L2_CTRL_TYPE_INTEGER64)
 		range_ = ControlRange(static_cast<int64_t>(ctrl.minimum),
diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
index 54cc214ecce9..144a60b4fe93 100644
--- a/src/libcamera/v4l2_device.cpp
+++ b/src/libcamera/v4l2_device.cpp
@@ -386,9 +386,10 @@  void V4L2Device::listControls()
 			continue;
 		}
 
+		controlIds_.emplace_back(utils::make_unique<V4L2ControlId>(ctrl));
 		ctrls.emplace(std::piecewise_construct,
 			      std::forward_as_tuple(ctrl.id),
-			      std::forward_as_tuple(ctrl));
+			      std::forward_as_tuple(*controlIds_.back().get(), ctrl));
 	}
 
 	controls_ = std::move(ctrls);