[libcamera-devel] [PATCH 04/10] libcamera: v4l2_controls: Index V4L2ControlInfoMap by ControlId *
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Oct 15 20:08:27 CEST 2019
Hi Jacopo,
On Tue, Oct 15, 2019 at 07:37:27PM +0200, Jacopo Mondi wrote:
> On Tue, Oct 15, 2019 at 06:31:18PM +0300, Laurent Pinchart wrote:
> >>> +/**
> >>> + * \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
> > :-)
>
> That's why I asked :) What does a count() operation does then if not
> making sure the control is present or not ?
On a std::map(), count() always returns 0 or 1.
> >>> +
> >>> +/**
> >>> + * \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