[libcamera-devel] [PATCH 04/10] libcamera: v4l2_controls: Index V4L2ControlInfoMap by ControlId *
Jacopo Mondi
jacopo at jmondi.org
Tue Oct 15 17:16:06 CEST 2019
Hi Laurent,
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 ?
> +
> +/**
> + * \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...
Anyway, the change is good and desirable:
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
Thanks
j
> 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
>
> _______________________________________________
> 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/20191015/6c6f942d/attachment.sig>
More information about the libcamera-devel
mailing list