[libcamera-devel] [PATCH 2/2] libcamera: v4l2_device: Simplify usage of getControls()
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Apr 26 19:26:56 CEST 2020
Hi Jacopo,
On Sun, Apr 26, 2020 at 04:43:58PM +0200, Jacopo Mondi wrote:
> On Sun, Apr 26, 2020 at 02:16:23AM +0300, Laurent Pinchart wrote:
> > The V4L2Device::getControls() function takes a ControlList that needs to
> > be pre-populated with dummy entries for the controls that need to be
> > read. This is a cumbersome API, especially when reading a single
> > control. Make it nicer by passing the list of V4L2 controls as a vector
> > of control IDs, and returning a ControlList.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > src/libcamera/camera_sensor.cpp | 23 ++++++---------
> > src/libcamera/include/camera_sensor.h | 2 +-
>
> While I welcome this for the v4l2 device part, I would leave
> CameraSensor out, as before defining what kind of controls the class
> should accepta (v4l2 control IDs or libcamera Control<> instances) we
> should probably have a bit more use cases...
Sure, but the CameraSensor class uses V4L2Device::getControls(), so it
has to be modified :-) I could keep the existing
CameraSensor::getControls() API and reimplement it on top of the new
V4L2Device::getControls(), but that's unnecessary churn in my opinion as
there's no user at the moment. The other option is do drop
CameraSensor::getControls() completely for now. I can do that, but isn't
it simpler to keep it until we decide what to do with it ?
> > src/libcamera/include/v4l2_device.h | 2 +-
> > src/libcamera/v4l2_device.cpp | 42 ++++++++++++---------------
> > test/v4l2_videodevice/controls.cpp | 20 +++++++------
> > 5 files changed, 41 insertions(+), 48 deletions(-)
> >
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index 2219a4307436..ce585cab1a4f 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -299,30 +299,25 @@ const ControlInfoMap &CameraSensor::controls() const
> >
> > /**
> > * \brief Read controls from the sensor
> > - * \param[inout] ctrls The list of controls to read
> > + * \param[in] ids The list of control to read, specified by their ID
> > *
> > - * This method reads the value of all controls contained in \a ctrls, and stores
> > - * their values in the corresponding \a ctrls entry.
> > + * This method reads the value of all controls contained in \a ids, and returns
> > + * their values as a ControlList.
> > *
> > - * If any control in \a ctrls is not supported by the device, is disabled (i.e.
> > + * If any control in \a ids is not supported by the device, is disabled (i.e.
> > * has the V4L2_CTRL_FLAG_DISABLED flag set), is a compound control, or if any
> > * other error occurs during validation of the requested controls, no control is
> > - * read and this method returns -EINVAL.
> > + * read and this method returns an empty control list.
> > *
> > - * If an error occurs while reading the controls, the index of the first control
> > - * that couldn't be read is returned. The value of all controls below that index
> > - * are updated in \a ctrls, while the value of all the other controls are not
> > - * changed.
> > + * If an error occurs while reading the controls, an empty list is returned.
> > *
> > * \sa V4L2Device::getControls()
> > *
> > - * \return 0 on success or an error code otherwise
> > - * \retval -EINVAL One of the control is not supported or not accessible
> > - * \retval i The index of the control that failed
> > + * \return The control values in a ControlList
> > */
> > -int CameraSensor::getControls(ControlList *ctrls)
> > +ControlList CameraSensor::getControls(const std::vector<uint32_t> &ids)
> > {
> > - return subdev_->getControls(ctrls);
> > + return subdev_->getControls(ids);
> > }
> >
> > /**
> > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h
> > index 99cff98128dc..5277f7f7fe87 100644
> > --- a/src/libcamera/include/camera_sensor.h
> > +++ b/src/libcamera/include/camera_sensor.h
> > @@ -43,7 +43,7 @@ public:
> > int setFormat(V4L2SubdeviceFormat *format);
> >
> > const ControlInfoMap &controls() const;
> > - int getControls(ControlList *ctrls);
> > + ControlList getControls(const std::vector<uint32_t> &ids);
> > int setControls(ControlList *ctrls);
> >
> > const ControlList &properties() const { return properties_; }
> > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> > index ce8edd98a01d..e604a40df4c9 100644
> > --- a/src/libcamera/include/v4l2_device.h
> > +++ b/src/libcamera/include/v4l2_device.h
> > @@ -26,7 +26,7 @@ public:
> >
> > const ControlInfoMap &controls() const { return controls_; }
> >
> > - int getControls(ControlList *ctrls);
> > + ControlList getControls(const std::vector<uint32_t> &ids);
> > int setControls(ControlList *ctrls);
> >
> > const std::string &deviceNode() const { return deviceNode_; }
> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > index 03e305165096..989ed05c9692 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -147,46 +147,42 @@ void V4L2Device::close()
> >
> > /**
> > * \brief Read controls from the device
> > - * \param[inout] ctrls The list of controls to read
> > + * \param[in] ids The list of control to read, specified by their ID
> > *
> > - * This method reads the value of all controls contained in \a ctrls, and stores
> > - * their values in the corresponding \a ctrls entry.
> > + * This method reads the value of all controls contained in \a ids, and returns
> > + * their values as a ControlList.
> > *
> > - * If any control in \a ctrls is not supported by the device, is disabled (i.e.
> > + * If any control in \a ids is not supported by the device, is disabled (i.e.
> > * has the V4L2_CTRL_FLAG_DISABLED flag set), is a compound control, or if any
> > * other error occurs during validation of the requested controls, no control is
>
> I'm about to merge the documentation update that removes compound
> controls from the list of unsupported controls, so please rebase the
> next version.
Sure, no problem.
> > - * read and this method returns -EINVAL.
> > + * read and this method returns an empty control list.
> > *
> > - * If an error occurs while reading the controls, the index of the first control
> > - * that couldn't be read is returned. The value of all controls below that index
> > - * are updated in \a ctrls, while the value of all the other controls are not
> > - * changed.
> > + * If an error occurs while reading the controls, an empty list is returned.
> > *
> > - * \return 0 on success or an error code otherwise
> > - * \retval -EINVAL One of the control is not supported or not accessible
> > - * \retval i The index of the control that failed
> > + * \return The control values in a ControlList
> > */
> > -int V4L2Device::getControls(ControlList *ctrls)
> > +ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)
>
> So we're copying back a ControlList now, thoughts on efficiency ?
With return valued optimization we get copy ellision, so it should be
fine. RVO is mandatory since C++-17 and optional before, and in practice
both g++ and clang++ implement it for C++-14.
> > {
> > - unsigned int count = ctrls->size();
> > + unsigned int count = ids.size();
> > if (count == 0)
> > - return 0;
> > + return {};
> > +
> > + ControlList ctrls{ controls_ };
> >
> > struct v4l2_ext_control v4l2Ctrls[count];
> > memset(v4l2Ctrls, 0, sizeof(v4l2Ctrls));
> >
> > unsigned int i = 0;
> > - for (auto &ctrl : *ctrls) {
> > - unsigned int id = ctrl.first;
> > + for (uint32_t id : ids) {
> > const auto iter = controls_.find(id);
> > if (iter == controls_.end()) {
> > LOG(V4L2, Error)
> > << "Control " << utils::hex(id) << " not found";
> > - return -EINVAL;
> > + return {};
> > }
> >
> > const struct v4l2_query_ext_ctrl &info = controlInfo_[id];
> > - ControlValue &value = ctrl.second;
> > + ControlValue &value = ctrls.set(id, {});
> >
> > if (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {
> > ControlType type;
> > @@ -200,7 +196,7 @@ int V4L2Device::getControls(ControlList *ctrls)
> > LOG(V4L2, Error)
> > << "Unsupported payload control type "
> > << info.type;
> > - return -EINVAL;
> > + return {};
> > }
> >
> > value.reserve(type, true, info.elems);
> > @@ -227,7 +223,7 @@ int V4L2Device::getControls(ControlList *ctrls)
> > if (errorIdx == 0 || errorIdx >= count) {
> > LOG(V4L2, Error) << "Unable to read controls: "
> > << strerror(-ret);
> > - return -EINVAL;
> > + return {};
> > }
> >
> > /* A specific control failed. */
> > @@ -237,9 +233,9 @@ int V4L2Device::getControls(ControlList *ctrls)
> > ret = errorIdx;
> > }
> >
> > - updateControls(ctrls, v4l2Ctrls, count);
> > + updateControls(&ctrls, v4l2Ctrls, count);
> >
> > - return ret;
> > + return ctrls;
> > }
> >
> > /**
> > diff --git a/test/v4l2_videodevice/controls.cpp b/test/v4l2_videodevice/controls.cpp
> > index da9e0111e221..347af2112f1a 100644
> > --- a/test/v4l2_videodevice/controls.cpp
> > +++ b/test/v4l2_videodevice/controls.cpp
> > @@ -57,18 +57,20 @@ protected:
> > const ControlInfo &u8 = infoMap.find(VIVID_CID_U8_4D_ARRAY)->second;
> >
> > /* Test getting controls. */
> > - ControlList ctrls(infoMap);
> > - ctrls.set(V4L2_CID_BRIGHTNESS, -1);
> > - ctrls.set(V4L2_CID_CONTRAST, -1);
> > - ctrls.set(V4L2_CID_SATURATION, -1);
> > - ctrls.set(VIVID_CID_U8_4D_ARRAY, 0);
> > -
> > - int ret = capture_->getControls(&ctrls);
> > - if (ret) {
> > + ControlList ctrls = capture_->getControls({ V4L2_CID_BRIGHTNESS,
> > + V4L2_CID_CONTRAST,
> > + V4L2_CID_SATURATION,
> > + VIVID_CID_U8_4D_ARRAY });
> > + if (ctrls.empty()) {
> > cerr << "Failed to get controls" << endl;
> > return TestFail;
> > }
> >
> > + if (ctrls.infoMap() != &infoMap) {
> > + cerr << "Incorrect infoMap for retrieved controls" << endl;
> > + return TestFail;
> > + }
> > +
> > if (ctrls.get(V4L2_CID_BRIGHTNESS).get<int32_t>() == -1 ||
> > ctrls.get(V4L2_CID_CONTRAST).get<int32_t>() == -1 ||
> > ctrls.get(V4L2_CID_SATURATION).get<int32_t>() == -1) {
> > @@ -97,7 +99,7 @@ protected:
> > std::fill(u8Values.begin(), u8Values.end(), u8.min().get<uint8_t>());
> > ctrls.set(VIVID_CID_U8_4D_ARRAY, Span<const uint8_t>(u8Values));
> >
> > - ret = capture_->setControls(&ctrls);
> > + int ret = capture_->setControls(&ctrls);
> > if (ret) {
> > cerr << "Failed to set controls" << endl;
> > return TestFail;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list