[libcamera-devel] [PATCH v2] libcamera: v4l2_device: Simplify usage of getControls()
Niklas Söderlund
niklas.soderlund at ragnatech.se
Tue Apr 28 04:14:32 CEST 2020
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
>
> 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
> * 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,
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
> *
> - * 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.
> *
> - * \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)
> {
> - 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.
> + */
> + 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));
> +
> + 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
More information about the libcamera-devel
mailing list