[libcamera-devel] [PATCH v2 12/14] libcamera: v4l2_controls: Turn V4L2ControlInfoMap into a class

[Jacopo Mondi]

Hi Laurent,

On Sat, Oct 12, 2019 at 09:44:05PM +0300, Laurent Pinchart wrote:
> In preparation for extending V4L2ControlInfoMap with control idmap
> support, turn it into a real class. Make it inherit from std::map<>
> instead of wrapping it to keep the API simple.
>
> V4L2ControlInfoMap is meant to be constructed with a set of control
> info, and never modified afterwards. To ensure this, inherit from
> std::map<> with private access specifier, and explicitly expose the
> std::map<> methods that do not allow insertion or removal of elements. A
> custom move assignment operator is implemented to allow efficient
> construction of the V4L2ControlInfoMap.

Thanks for having clarified my design concerns.

>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/libcamera/include/v4l2_controls.h | 16 +++++++++++++++-
>  src/libcamera/v4l2_controls.cpp       | 17 ++++++++++++++++-
>  src/libcamera/v4l2_device.cpp         |  9 ++++++---
>  3 files changed, 37 insertions(+), 5 deletions(-)
>
> diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h
> index 949ca21cb863..8990e755a385 100644
> --- a/src/libcamera/include/v4l2_controls.h
> +++ b/src/libcamera/include/v4l2_controls.h
> @@ -43,7 +43,21 @@ private:
>  	ControlRange range_;
>  };
>
> -using V4L2ControlInfoMap = std::map<unsigned int, V4L2ControlInfo>;
> +class V4L2ControlInfoMap : private std::map<unsigned int, V4L2ControlInfo>
> +{
> +public:
> +	V4L2ControlInfoMap &operator=(std::map<unsigned int, V4L2ControlInfo> &&info);
> +
> +	using std::map<unsigned int, V4L2ControlInfo>::begin;
> +	using std::map<unsigned int, V4L2ControlInfo>::cbegin;
> +	using std::map<unsigned int, V4L2ControlInfo>::end;
> +	using std::map<unsigned int, V4L2ControlInfo>::cend;
> +	using std::map<unsigned int, V4L2ControlInfo>::at;
> +	using std::map<unsigned int, V4L2ControlInfo>::empty;
> +	using std::map<unsigned int, V4L2ControlInfo>::size;
> +	using std::map<unsigned int, V4L2ControlInfo>::count;
> +	using std::map<unsigned int, V4L2ControlInfo>::find;
> +};
>
>  class V4L2Control
>  {
> diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> index 3f5f3ff10880..438e8d8bdb99 100644
> --- a/src/libcamera/v4l2_controls.cpp
> +++ b/src/libcamera/v4l2_controls.cpp
> @@ -156,10 +156,25 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
>   */
>
>  /**
> - * \typedef V4L2ControlInfoMap
> + * \class V4L2ControlInfoMap
>   * \brief A map of control ID to V4L2ControlInfo
>   */
>
> +/**
> + * \brief Move assignment operator from plain map

Missing parameter documentation

> + *
> + * Populate the map by replacing its contents with those of \a info using move
> + * semantics. This is the only supported

incomplete comment

> + *
> + * \return *this

What about a more lenght description ?
        \return The populated V4L2ControlInfoMap

> + */
> +V4L2ControlInfoMap &V4L2ControlInfoMap::operator=(std::map<unsigned int, V4L2ControlInfo> &&info)
> +{
> +	std::map<unsigned int, V4L2ControlInfo>::operator=(std::move(info));
> +
> +	return *this;
> +}
> +
>  /**
>   * \class V4L2Control
>   * \brief A V4L2 control value
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 8c5998435020..1f755f0f3ef6 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -340,6 +340,7 @@ int V4L2Device::ioctl(unsigned long request, void *argp)
>   */
>  void V4L2Device::listControls()
>  {
> +	std::map<unsigned int, V4L2ControlInfo> ctrls;
>  	struct v4l2_query_ext_ctrl ctrl = {};
>
>  	/* \todo Add support for menu and compound controls. */
> @@ -368,10 +369,12 @@ void V4L2Device::listControls()
>  			continue;
>  		}
>
> -		controls_.emplace(std::piecewise_construct,
> -				  std::forward_as_tuple(ctrl.id),
> -				  std::forward_as_tuple(ctrl));
> +		ctrls.emplace(std::piecewise_construct,
> +			      std::forward_as_tuple(ctrl.id),
> +			      std::forward_as_tuple(ctrl));
>  	}
> +
> +	controls_ = std::move(ctrls);

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

Thanks
  j

>  }
>
>  /*
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20191013/1fa32b07/attachment.sig>