| Message ID | 20191012184407.31684-13-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@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@jmondi.org> Thanks j > } > > /* > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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@jmondi.org> > > > } > > > > /*
Hi Laurent, Thanks for your work. On 2019-10-12 21:44:05 +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. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > 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 > + * > + * Populate the map by replacing its contents with those of \a info using move > + * semantics. This is the only supported > + * > + * \return *this > + */ > +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); > } > > /* > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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 + * + * Populate the map by replacing its contents with those of \a info using move + * semantics. This is the only supported + * + * \return *this + */ +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); } /*
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. Signed-off-by: Laurent Pinchart <laurent.pinchart@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(-)