[libcamera-devel] [PATCH 2/2] libcamera: v4l2_device: Simplify usage of getControls()
Jacopo Mondi
jacopo at jmondi.org
Sun Apr 26 16:43:58 CEST 2020
Hi Laurent,
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...
> 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.
> - * 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 ?
> {
> - 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