[libcamera-devel] [PATCH 04/10] libcamera: v4l2_controls: Index V4L2ControlInfoMap by ControlId *
Jacopo Mondi
jacopo at jmondi.org
Tue Oct 15 19:37:27 CEST 2019
Hi Laurent,
On Tue, Oct 15, 2019 at 06:31:18PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> > > +/**
> > > + * \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 ?
Thanks
j
> > > +
> > > +/**
> > > + * \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
-------------- 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/07c98741/attachment.sig>
More information about the libcamera-devel
mailing list