| Message ID | 20200425231623.6505-2-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@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 >
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@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;
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 - * 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) { - 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;
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@ideasonboard.com> --- 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 | 42 ++++++++++++--------------- test/v4l2_videodevice/controls.cpp | 20 +++++++------ 5 files changed, 41 insertions(+), 48 deletions(-)