[libcamera-devel] [PATCH v2] libcamera: v4l2_device: Simplify usage of getControls()
Jacopo Mondi
jacopo at jmondi.org
Tue Apr 28 14:04:46 CEST 2020
Hi Laurent,
On Tue, Apr 28, 2020 at 04:14:32AM +0200, Niklas Söderlund wrote:
> Hi Laurent,
>
> Thanks four your work.
>
> On 2020-04-28 04:21:22 +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>
> > ---
> > Changes since v1:
> >
> > - Fill the ControlList and v4l2_ext_control arrays separately
> >
Sorry, it should have been caught by review :/
> > The patch has no dependency on "[PATCH 1/2] libcamera: controls: Return
> > ControlValue reference from ControlList::set()" anymore.
> > ---
> > src/libcamera/camera_sensor.cpp | 23 ++++-------
> > src/libcamera/include/camera_sensor.h | 2 +-
> > src/libcamera/include/v4l2_device.h | 2 +-
> > src/libcamera/v4l2_device.cpp | 59 +++++++++++++++------------
> > test/v4l2_videodevice/controls.cpp | 20 +++++----
> > 5 files changed, 55 insertions(+), 51 deletions(-)
> >
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index 140186b79f6f..55a9fbd71e69 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -323,30 +323,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
While at it, could you drop "compound controls" from documentation, as
they're now supported ?
I did the the same for the v4l2 device but forgot camera sensor...
> > * 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
>
> Here and bellow, would it make sens to move the 'an empty list is
> returned on error' information from above to the \return statement?
> Whit or without this addressed,
or to match our canonical style:
", or an empty list on error"
>
> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
>
> > */
> > -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 7dc4ebc79051..e68185370eb2 100644
> > --- a/src/libcamera/include/camera_sensor.h
> > +++ b/src/libcamera/include/camera_sensor.h
> > @@ -44,7 +44,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 c134266e5fd7..d009fc47c2ef 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -147,46 +147,52 @@ 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
list of control's'
> > *
> > - * 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), or if any other error occurs
> > * during validation of the requested controls, no control is read and this
> > - * method returns -EINVAL.
> > + * 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.
Isn't this a repetition of the above statement ?
> > *
> > - * \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
same comment as above
> > */
> > -int V4L2Device::getControls(ControlList *ctrls)
> > +ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)
> > {
> > - unsigned int count = ctrls->size();
> > + unsigned int count = ids.size();
> > if (count == 0)
> > - return 0;
> > + return {};
> >
> > - struct v4l2_ext_control v4l2Ctrls[count];
> > - memset(v4l2Ctrls, 0, sizeof(v4l2Ctrls));
> > + ControlList ctrls{ controls_ };
> >
> > - unsigned int i = 0;
> > - for (auto &ctrl : *ctrls) {
> > - unsigned int id = ctrl.first;
> > + /*
> > + * Start by filling the ControlList. This can't be combined with filling
> > + * v4l2Ctrls, as updateControls() relies on both containers having the
> > + * same order, and the control list is based on a map, which is not
> > + * sorted by insertion order.
> > + */
Ah, this was nasty to debug I assume...
> > + 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 {};
> > }
> >
> > + ctrls.set(id, {});
> > + }
> > +
> > + struct v4l2_ext_control v4l2Ctrls[count];
> > + memset(v4l2Ctrls, 0, sizeof(v4l2Ctrls));
Can we initialize an array with {} and save memset ? I know this was
there already, so it's fine if you want to keep it
Anyway, minors apart this looks good!
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
Side note: we now have a getControls() which works on ids and a
setControls() which work on lists. Would that make sense to provide
a setControls(id, T value) ? Would this be abused by lazy implementers
which instead of grouping controls in a list would call multiple times
this new function ? Just wondering and not on this patch anyway...
Thanks
j
> > +
> > + unsigned int i = 0;
> > + for (auto &ctrl : ctrls) {
> > + unsigned int id = ctrl.first;
> > const struct v4l2_query_ext_ctrl &info = controlInfo_[id];
> > - ControlValue &value = ctrl.second;
> >
> > if (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {
> > ControlType type;
> > @@ -200,9 +206,10 @@ int V4L2Device::getControls(ControlList *ctrls)
> > LOG(V4L2, Error)
> > << "Unsupported payload control type "
> > << info.type;
> > - return -EINVAL;
> > + return {};
> > }
> >
> > + ControlValue &value = ctrl.second;
> > value.reserve(type, true, info.elems);
> > Span<uint8_t> data = value.data();
> >
> > @@ -227,7 +234,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 +244,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
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list