[libcamera-devel] [PATCH 04/10] libcamera: v4l2_controls: Index V4L2ControlInfoMap by ControlId *
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Oct 15 17:31:18 CEST 2019
Hi Jacopo,
On Tue, Oct 15, 2019 at 05:16:06PM +0200, Jacopo Mondi wrote:
> On Mon, Oct 14, 2019 at 02:27:50AM +0300, Laurent Pinchart wrote:
> > To bring the libcamera and V4L2 control info maps closer, index the
> > latter by ControlId * like the former. As V4L2ControlInfoMap is widely
> > indexed by V4L2 numerical IDs, add accessors based on numerical IDs.
> >
> > This allows complete removal of the ControId pointer from the
> > V4L2ControlInfo, as the ControId is accessible as the key when iterating
> > over the map. A handful of users have to be modified to adapt to the
> > change.
> >
> > The controlInfo argument from V4L2Device::updateControls() can also be
> > removed as it itsn't used anymore.
>
> Great! I had this in my early serialization work and I really wanted
> to see the v4l2_device control handling simplified
>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > src/libcamera/include/v4l2_controls.h | 40 +++++++++------
> > src/libcamera/include/v4l2_device.h | 1 -
> > src/libcamera/pipeline/uvcvideo.cpp | 2 +-
> > src/libcamera/pipeline/vimc.cpp | 2 +-
> > src/libcamera/v4l2_controls.cpp | 71 +++++++++++++++++++++------
> > src/libcamera/v4l2_device.cpp | 25 +++-------
> > 6 files changed, 92 insertions(+), 49 deletions(-)
> >
> > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h
> > index 4d7ac1a133c7..ca7217501256 100644
> > --- a/src/libcamera/include/v4l2_controls.h
> > +++ b/src/libcamera/include/v4l2_controls.h
> > @@ -28,31 +28,41 @@ public:
> > class V4L2ControlInfo
> > {
> > public:
> > - V4L2ControlInfo(const V4L2ControlId &id,
> > - const struct v4l2_query_ext_ctrl &ctrl);
> > + V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl);
> >
> > - const ControlId &id() const { return *id_; }
> > const ControlRange &range() const { return range_; }
> >
> > private:
> > - const V4L2ControlId *id_;
> > ControlRange range_;
> > };
> >
> > -class V4L2ControlInfoMap : private std::map<unsigned int, V4L2ControlInfo>
> > +class V4L2ControlInfoMap : private std::map<const ControlId *, V4L2ControlInfo>
> > {
> > public:
> > - V4L2ControlInfoMap &operator=(std::map<unsigned int, V4L2ControlInfo> &&info);
> > + V4L2ControlInfoMap &operator=(std::map<const ControlId *, 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;
> > + using std::map<const ControlId *, V4L2ControlInfo>::key_type;
> > + using std::map<const ControlId *, V4L2ControlInfo>::mapped_type;
> > + using std::map<const ControlId *, V4L2ControlInfo>::value_type;
> > + using std::map<const ControlId *, V4L2ControlInfo>::size_type;
> > + using std::map<const ControlId *, V4L2ControlInfo>::iterator;
> > + using std::map<const ControlId *, V4L2ControlInfo>::const_iterator;
> > +
> > + using std::map<const ControlId *, V4L2ControlInfo>::begin;
> > + using std::map<const ControlId *, V4L2ControlInfo>::cbegin;
> > + using std::map<const ControlId *, V4L2ControlInfo>::end;
> > + using std::map<const ControlId *, V4L2ControlInfo>::cend;
> > + using std::map<const ControlId *, V4L2ControlInfo>::at;
> > + using std::map<const ControlId *, V4L2ControlInfo>::empty;
> > + using std::map<const ControlId *, V4L2ControlInfo>::size;
> > + using std::map<const ControlId *, V4L2ControlInfo>::count;
> > + using std::map<const ControlId *, V4L2ControlInfo>::find;
> > +
> > + mapped_type &at(unsigned int key);
> > + const mapped_type &at(unsigned int key) const;
> > + size_type count(unsigned int key) const;
> > + iterator find(unsigned int key);
> > + const_iterator find(unsigned int key) const;
> >
> > const ControlIdMap &idmap() const { return idmap_; }
> >
> > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> > index 5a5b85827f23..f30b1c2cde34 100644
> > --- a/src/libcamera/include/v4l2_device.h
> > +++ b/src/libcamera/include/v4l2_device.h
> > @@ -45,7 +45,6 @@ protected:
> > private:
> > void listControls();
> > void updateControls(ControlList *ctrls,
> > - const V4L2ControlInfo **controlInfo,
> > const struct v4l2_ext_control *v4l2Ctrls,
> > unsigned int count);
> >
> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> > index 547ad5ca4e55..4d76b5fd9347 100644
> > --- a/src/libcamera/pipeline/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo.cpp
> > @@ -340,7 +340,7 @@ int UVCCameraData::init(MediaEntity *entity)
> > const V4L2ControlInfo &info = ctrl.second;
> > const ControlId *id;
> >
> > - switch (info.id().id()) {
> > + switch (ctrl.first->id()) {
> > case V4L2_CID_BRIGHTNESS:
> > id = &controls::Brightness;
> > break;
> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> > index 53d00360eb6e..78c0fe5ae509 100644
> > --- a/src/libcamera/pipeline/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc.cpp
> > @@ -416,7 +416,7 @@ int VimcCameraData::init(MediaDevice *media)
> > const V4L2ControlInfo &info = ctrl.second;
> > const ControlId *id;
> >
> > - switch (info.id().id()) {
> > + switch (ctrl.first->id()) {
> > case V4L2_CID_BRIGHTNESS:
> > id = &controls::Brightness;
> > break;
> > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> > index 12c4fb271ba5..9a5e4830db91 100644
> > --- a/src/libcamera/v4l2_controls.cpp
> > +++ b/src/libcamera/v4l2_controls.cpp
> > @@ -122,12 +122,9 @@ 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 V4L2ControlId &id,
> > - const struct v4l2_query_ext_ctrl &ctrl)
> > - : id_(&id)
> > +V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
> > {
> > if (ctrl.type == V4L2_CTRL_TYPE_INTEGER64)
> > range_ = ControlRange(static_cast<int64_t>(ctrl.minimum),
> > @@ -137,12 +134,6 @@ V4L2ControlInfo::V4L2ControlInfo(const V4L2ControlId &id,
> > static_cast<int32_t>(ctrl.maximum));
> > }
> >
> > -/**
> > - * \fn V4L2ControlInfo::id()
> > - * \brief Retrieve the control ID
> > - * \return The V4L2 control ID
> > - */
> > -
> > /**
> > * \fn V4L2ControlInfo::range()
> > * \brief Retrieve the control value range
> > @@ -151,7 +142,7 @@ V4L2ControlInfo::V4L2ControlInfo(const V4L2ControlId &id,
> >
> > /**
> > * \class V4L2ControlInfoMap
> > - * \brief A map of control ID to V4L2ControlInfo
> > + * \brief A map of controlID to V4L2ControlInfo
> > */
> >
> > /**
> > @@ -165,17 +156,69 @@ V4L2ControlInfo::V4L2ControlInfo(const V4L2ControlId &id,
> > *
> > * \return The populated V4L2ControlInfoMap
> > */
> > -V4L2ControlInfoMap &V4L2ControlInfoMap::operator=(std::map<unsigned int, V4L2ControlInfo> &&info)
> > +V4L2ControlInfoMap &V4L2ControlInfoMap::operator=(std::map<const ControlId *, V4L2ControlInfo> &&info)
> > {
> > - std::map<unsigned int, V4L2ControlInfo>::operator=(std::move(info));
> > + std::map<const ControlId *, V4L2ControlInfo>::operator=(std::move(info));
> >
> > idmap_.clear();
> > for (const auto &ctrl : *this)
> > - idmap_[ctrl.first] = &ctrl.second.id();
> > + idmap_[ctrl.first->id()] = ctrl.first;
> >
> > return *this;
> > }
> >
> > +/**
> > + * \brief Access specified element by numerical ID
> > + * \param[in] id The numerical ID
> > + * \return A reference to the element whose ID is equal to \a id
> > + */
> > +V4L2ControlInfoMap::mapped_type &V4L2ControlInfoMap::at(unsigned int id)
> > +{
> > + return at(idmap_.at(id));
> > +}
> > +
> > +/**
> > + * \brief Access specified element by numerical ID
> > + * \param[in] id The numerical ID
> > + * \return A const reference to the element whose ID is equal to \a id
> > + */
> > +const V4L2ControlInfoMap::mapped_type &V4L2ControlInfoMap::at(unsigned int id) const
> > +{
> > + return at(idmap_.at(id));
> > +}
> > +
> > +/**
> > + * \brief Count the number of elements matching a numerical ID
> > + * \param[in] id The numerical ID
> > + * \return The number of elements matching the numerical \a id
> > + */
> > +V4L2ControlInfoMap::size_type V4L2ControlInfoMap::count(unsigned int id) const
> > +{
> > + return count(idmap_.at(id));
> > +}
>
> Can we have multiple elements indexed by the same id ?
That would require a V4L2 device to expose multiple controls with the
same ID. Highly unlikely given that the kernel API won't support this
:-)
> > +
> > +/**
> > + * \brief Find the element matching a numerical ID
> > + * \param[in] id The numerical ID
> > + * \return An iterator pointing to the element matching the numerical \a id, or
> > + * end() if no such element exists
> > + */
> > +V4L2ControlInfoMap::iterator V4L2ControlInfoMap::find(unsigned int id)
> > +{
> > + return find(idmap_.at(id));
> > +}
> > +
> > +/**
> > + * \brief Find the element matching a numerical ID
> > + * \param[in] id The numerical ID
> > + * \return A const iterator pointing to the element matching the numerical
> > + * \a id, or end() if no such element exists
> > + */
> > +V4L2ControlInfoMap::const_iterator V4L2ControlInfoMap::find(unsigned int id) const
> > +{
> > + return find(idmap_.at(id));
> > +}
> > +
> > /**
> > * \fn const ControlIdMap &V4L2ControlInfoMap::idmap() const
> > * \brief Retrieve the ControlId map
> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > index 144a60b4fe93..4bb7d5950f3a 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -171,7 +171,6 @@ int V4L2Device::getControls(ControlList *ctrls)
> > if (count == 0)
> > return 0;
> >
> > - const V4L2ControlInfo *controlInfo[count];
> > struct v4l2_ext_control v4l2Ctrls[count];
> > memset(v4l2Ctrls, 0, sizeof(v4l2Ctrls));
> >
> > @@ -185,10 +184,7 @@ int V4L2Device::getControls(ControlList *ctrls)
> > return -EINVAL;
> > }
> >
> > - const V4L2ControlInfo *info = &iter->second;
> > - controlInfo[i] = info;
> > v4l2Ctrls[i].id = id->id();
> > -
> > i++;
> > }
> >
> > @@ -215,7 +211,7 @@ int V4L2Device::getControls(ControlList *ctrls)
> > ret = errorIdx;
> > }
> >
> > - updateControls(ctrls, controlInfo, v4l2Ctrls, count);
> > + updateControls(ctrls, v4l2Ctrls, count);
> >
> > return ret;
> > }
> > @@ -249,7 +245,6 @@ int V4L2Device::setControls(ControlList *ctrls)
> > if (count == 0)
> > return 0;
> >
> > - const V4L2ControlInfo *controlInfo[count];
> > struct v4l2_ext_control v4l2Ctrls[count];
> > memset(v4l2Ctrls, 0, sizeof(v4l2Ctrls));
> >
> > @@ -263,13 +258,11 @@ int V4L2Device::setControls(ControlList *ctrls)
> > return -EINVAL;
> > }
> >
> > - const V4L2ControlInfo *info = &iter->second;
> > - controlInfo[i] = info;
> > v4l2Ctrls[i].id = id->id();
> >
> > /* Set the v4l2_ext_control value for the write operation. */
> > const ControlValue &value = ctrl.second;
> > - switch (info->id().type()) {
> > + switch (id->type()) {
> > case ControlTypeInteger64:
> > v4l2Ctrls[i].value64 = value.get<int64_t>();
> > break;
> > @@ -308,7 +301,7 @@ int V4L2Device::setControls(ControlList *ctrls)
> > ret = errorIdx;
> > }
> >
> > - updateControls(ctrls, controlInfo, v4l2Ctrls, count);
> > + updateControls(ctrls, v4l2Ctrls, count);
> >
> > return ret;
> > }
> > @@ -349,7 +342,7 @@ int V4L2Device::ioctl(unsigned long request, void *argp)
> > */
> > void V4L2Device::listControls()
> > {
> > - std::map<unsigned int, V4L2ControlInfo> ctrls;
> > + std::map<const ControlId *, V4L2ControlInfo> ctrls;
> > struct v4l2_query_ext_ctrl ctrl = {};
> >
> > /* \todo Add support for menu and compound controls. */
> > @@ -388,8 +381,8 @@ void V4L2Device::listControls()
> >
> > controlIds_.emplace_back(utils::make_unique<V4L2ControlId>(ctrl));
>
> Not in this patch but I just noticed: doesn't this create a
> V4L2ControlId while we should instead forward the argument to its
> constructor ? Well, I think the unique_ptr<> here plays a role, and I
> cannot suggest a way around it...
controlIds_ store instance of std::unique_ptr<>, so it doesn't construct
V4L2ControlId instances. This should, as far as I can tell, create a
single V4L2ControlId. The fact that ControlId copies are now impossible
is also a hint that no copy is taking place.
> Anyway, the change is good and desirable:
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
> > ctrls.emplace(std::piecewise_construct,
> > - std::forward_as_tuple(ctrl.id),
> > - std::forward_as_tuple(*controlIds_.back().get(), ctrl));
> > + std::forward_as_tuple(controlIds_.back().get()),
> > + std::forward_as_tuple(ctrl));
> > }
> >
> > controls_ = std::move(ctrls);
> > @@ -399,12 +392,10 @@ void V4L2Device::listControls()
> > * \brief Update the value of the first \a count V4L2 controls in \a ctrls using
> > * values in \a v4l2Ctrls
> > * \param[inout] ctrls List of V4L2 controls to update
> > - * \param[in] controlInfo List of V4L2 control information
> > * \param[in] v4l2Ctrls List of V4L2 extended controls as returned by the driver
> > * \param[in] count The number of controls to update
> > */
> > void V4L2Device::updateControls(ControlList *ctrls,
> > - const V4L2ControlInfo **controlInfo,
> > const struct v4l2_ext_control *v4l2Ctrls,
> > unsigned int count)
> > {
> > @@ -414,10 +405,10 @@ void V4L2Device::updateControls(ControlList *ctrls,
> > break;
> >
> > const struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];
> > - const V4L2ControlInfo *info = controlInfo[i];
> > + const ControlId *id = ctrl.first;
> > ControlValue &value = ctrl.second;
> >
> > - switch (info->id().type()) {
> > + switch (id->type()) {
> > case ControlTypeInteger64:
> > value.set<int64_t>(v4l2Ctrl->value64);
> > break;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list