[libcamera-devel] [PATCH v2] libcamera: v4l2_device: Simplify usage of getControls()
Jacopo Mondi
jacopo at jmondi.org
Tue Apr 28 14:31:32 CEST 2020
On Tue, Apr 28, 2020 at 02:04:46PM +0200, Jacopo Mondi wrote:
> 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
>
No we can't :)
../src/libcamera/v4l2_device.cpp:189:36: error: variable-sized object may not be initialized
struct v4l2_ext_control v4l2Ctrls[count]{};
^~~~~
Ignore this comment then!
> 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
> _______________________________________________
> 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