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

[Laurent Pinchart]

Hi Jacopo,

On Sun, Oct 13, 2019 at 04:28:40PM +0200, Jacopo Mondi wrote:
> 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.

You're welcome.

> > 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

Will fix.

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

"This is the only supported way to populate a V4L2ControlInfoMap."

> > + *
> > + * \return *this
> 
> What about a more lenght description ?
>         \return The populated V4L2ControlInfoMap

I modelled the comment based on the C++ STL documentation, but I'll
change it.

> > + */
> > +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>
> 
> >  }
> >
> >  /*

-- 
Regards,

Laurent Pinchart