| Message ID | 20200428012122.2496-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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@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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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@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@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@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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@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@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@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > -- > > Regards, > > Niklas Söderlund > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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 */ -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;
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> --- 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(-)