| Message ID | 20190630233817.10130-4-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On 01/07/2019 00:38, Laurent Pinchart wrote: > Add a new controls() method to the V4L2Device class to retrieve the map > of all supported controls. This is needed in order to dynamically query > the supported controls, for instance for drivers that support different > sets of controls depending on the device model. > > To make the API easier to use, create a type alias for the control ID to > ControlInfo and use it. > > Remove the getControlInfo() method that is not used externally, as it > can now be replaced by accessing the full list of controls. Maybe it's the C-dev in me but I kinda prefer the lookup, with a null return check over a lookup with open coding the iter != controls_.end(); Functionally the same I guess, I think it's just more ingrained in my head, but the .end() check is more 'STL' so it's certainly not a problem really. > Update the CameraSensor API accordingly. Small comment on the map type chosen, but I think either std::map is probably fine too. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/camera_sensor.cpp | 10 ++++------ > src/libcamera/include/camera_sensor.h | 5 ++--- > src/libcamera/include/v4l2_controls.h | 6 ++++-- > src/libcamera/include/v4l2_device.h | 9 ++++----- > src/libcamera/v4l2_controls.cpp | 5 +++++ > src/libcamera/v4l2_device.cpp | 25 +++++++++---------------- > 6 files changed, 28 insertions(+), 32 deletions(-) > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > index 471c3e2e6e47..14f631e8ecf7 100644 > --- a/src/libcamera/camera_sensor.cpp > +++ b/src/libcamera/camera_sensor.cpp > @@ -248,14 +248,12 @@ int CameraSensor::setFormat(V4L2SubdeviceFormat *format) > } > > /** > - * \brief Retrieve information about a control > - * \param[in] id The control ID > - * \return A pointer to the V4L2ControlInfo for control \a id, or nullptr > - * if the sensor doesn't have such a control > + * \brief Retrieve the supported V4L2 controls > + * \return A map of the V4L2 controls supported by the sensor > */ > -const V4L2ControlInfo *CameraSensor::getControlInfo(unsigned int id) const > +const V4L2ControlInfoMap &CameraSensor::controls() const > { > - return subdev_->getControlInfo(id); > + return subdev_->controls(); > } > > /** > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h > index b42e7b8e1343..fe033fb374c1 100644 > --- a/src/libcamera/include/camera_sensor.h > +++ b/src/libcamera/include/camera_sensor.h > @@ -13,12 +13,11 @@ > #include <libcamera/geometry.h> > > #include "log.h" > +#include "v4l2_controls.h" > > namespace libcamera { > > class MediaEntity; > -class V4L2ControlInfo; > -class V4L2ControlList; > class V4L2Subdevice; > > struct V4L2SubdeviceFormat; > @@ -43,7 +42,7 @@ public: > const Size &size) const; > int setFormat(V4L2SubdeviceFormat *format); > > - const V4L2ControlInfo *getControlInfo(unsigned int id) const; > + const V4L2ControlInfoMap &controls() const; > int getControls(V4L2ControlList *ctrls); > int setControls(V4L2ControlList *ctrls); > > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h > index 0047efab11fa..10b726504951 100644 > --- a/src/libcamera/include/v4l2_controls.h > +++ b/src/libcamera/include/v4l2_controls.h > @@ -8,11 +8,11 @@ > #ifndef __LIBCAMERA_V4L2_CONTROLS_H__ > #define __LIBCAMERA_V4L2_CONTROLS_H__ > > +#include <map> > +#include <stdint.h> > #include <string> > #include <vector> > > -#include <stdint.h> > - > #include <linux/v4l2-controls.h> > #include <linux/videodev2.h> > > @@ -41,6 +41,8 @@ private: > int64_t max_; > }; > > +using V4L2ControlInfoMap = std::map<unsigned int, V4L2ControlInfo>; Does V4L2ControlInfoMap need to be ordered? (I don't think we do at this part do we?) std::unordered_map could be faster if we don't need to order the map. I think at the scale of controls we'll have it probably won't make much difference... We could do some measurements perhaps on a map of 1000 elements vs an unordered_map of 1000 elements, but I think that's the approx max scale that we'll contain right? As the key is an unsigned int, I think the hasher will function out of the box? > + > class V4L2Control > { > public: > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h > index 24a0134a2cba..e7e9829cb05f 100644 > --- a/src/libcamera/include/v4l2_device.h > +++ b/src/libcamera/include/v4l2_device.h > @@ -13,19 +13,18 @@ > #include <linux/videodev2.h> > > #include "log.h" > +#include "v4l2_controls.h" > > namespace libcamera { > > -class V4L2ControlInfo; > -class V4L2ControlList; > - > class V4L2Device : protected Loggable > { > public: > void close(); > bool isOpen() const { return fd_ != -1; } > > - const V4L2ControlInfo *getControlInfo(unsigned int id) const; > + const V4L2ControlInfoMap &controls() const { return controls_; } > + > int getControls(V4L2ControlList *ctrls); > int setControls(V4L2ControlList *ctrls); > > @@ -48,7 +47,7 @@ private: > const struct v4l2_ext_control *v4l2Ctrls, > unsigned int count); > > - std::map<unsigned int, V4L2ControlInfo> controls_; > + V4L2ControlInfoMap controls_; > std::string deviceNode_; > int fd_; > }; > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp > index af017bce48ba..84258d9954d0 100644 > --- a/src/libcamera/v4l2_controls.cpp > +++ b/src/libcamera/v4l2_controls.cpp > @@ -114,6 +114,11 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl) > * \return The V4L2 control maximum value > */ > > +/** > + * \typedef V4L2ControlInfoMap > + * \brief A map of control ID to V4L2ControlInfo > + */ > + > /** > * \class V4L2Control > * \brief A V4L2 control value > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index 06939dead705..59fc98cefd4e 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -111,19 +111,10 @@ void V4L2Device::close() > */ > > /** > - * \brief Retrieve information about a control > - * \param[in] id The control ID > - * \return A pointer to the V4L2ControlInfo for control \a id, or nullptr > - * if the device doesn't have such a control > + * \fn V4L2Device::controls() > + * \brief Retrieve the supported V4L2 controls > + * \return A map of the V4L2 controls supported by the device > */ > -const V4L2ControlInfo *V4L2Device::getControlInfo(unsigned int id) const > -{ > - auto it = controls_.find(id); > - if (it == controls_.end()) > - return nullptr; > - > - return &it->second; > -} > > /** > * \brief Read controls from the device > @@ -158,13 +149,14 @@ int V4L2Device::getControls(V4L2ControlList *ctrls) > > for (unsigned int i = 0; i < count; ++i) { > const V4L2Control *ctrl = ctrls->getByIndex(i); > - const V4L2ControlInfo *info = getControlInfo(ctrl->id()); > - if (!info) { > + const auto iter = controls_.find(ctrl->id()); > + if (iter == controls_.end()) { > LOG(V4L2, Error) > << "Control '" << ctrl->id() << "' not found"; > return -EINVAL; > } > > + const V4L2ControlInfo *info = &iter->second; > controlInfo[i] = info; > v4l2Ctrls[i].id = info->id(); > } > @@ -232,13 +224,14 @@ int V4L2Device::setControls(V4L2ControlList *ctrls) > > for (unsigned int i = 0; i < count; ++i) { > const V4L2Control *ctrl = ctrls->getByIndex(i); > - const V4L2ControlInfo *info = getControlInfo(ctrl->id()); > - if (!info) { > + const auto iter = controls_.find(ctrl->id()); > + if (iter == controls_.end()) { > LOG(V4L2, Error) > << "Control '" << ctrl->id() << "' not found"; > return -EINVAL; > } > > + const V4L2ControlInfo *info = &iter->second; > controlInfo[i] = info; > v4l2Ctrls[i].id = info->id(); > >
Hi Kieran, On Mon, Jul 01, 2019 at 09:03:09AM +0100, Kieran Bingham wrote: > On 01/07/2019 00:38, Laurent Pinchart wrote: > > Add a new controls() method to the V4L2Device class to retrieve the map > > of all supported controls. This is needed in order to dynamically query > > the supported controls, for instance for drivers that support different > > sets of controls depending on the device model. > > > > To make the API easier to use, create a type alias for the control ID to > > ControlInfo and use it. > > > > Remove the getControlInfo() method that is not used externally, as it > > can now be replaced by accessing the full list of controls. > > Maybe it's the C-dev in me but I kinda prefer the lookup, with a null > return check over a lookup with open coding the iter != controls_.end(); > > Functionally the same I guess, I think it's just more ingrained in my > head, but the .end() check is more 'STL' so it's certainly not a problem > really. I agree, so if we find this is too much hasle to use, I'm fine adding back a method to look up a single control (or I wonder if we could add a helper method to make look up easier on std::map). > > Update the CameraSensor API accordingly. > > Small comment on the map type chosen, but I think either std::map is > probably fine too. > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/libcamera/camera_sensor.cpp | 10 ++++------ > > src/libcamera/include/camera_sensor.h | 5 ++--- > > src/libcamera/include/v4l2_controls.h | 6 ++++-- > > src/libcamera/include/v4l2_device.h | 9 ++++----- > > src/libcamera/v4l2_controls.cpp | 5 +++++ > > src/libcamera/v4l2_device.cpp | 25 +++++++++---------------- > > 6 files changed, 28 insertions(+), 32 deletions(-) > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > > index 471c3e2e6e47..14f631e8ecf7 100644 > > --- a/src/libcamera/camera_sensor.cpp > > +++ b/src/libcamera/camera_sensor.cpp > > @@ -248,14 +248,12 @@ int CameraSensor::setFormat(V4L2SubdeviceFormat *format) > > } > > > > /** > > - * \brief Retrieve information about a control > > - * \param[in] id The control ID > > - * \return A pointer to the V4L2ControlInfo for control \a id, or nullptr > > - * if the sensor doesn't have such a control > > + * \brief Retrieve the supported V4L2 controls > > + * \return A map of the V4L2 controls supported by the sensor > > */ > > -const V4L2ControlInfo *CameraSensor::getControlInfo(unsigned int id) const > > +const V4L2ControlInfoMap &CameraSensor::controls() const > > { > > - return subdev_->getControlInfo(id); > > + return subdev_->controls(); > > } > > > > /** > > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h > > index b42e7b8e1343..fe033fb374c1 100644 > > --- a/src/libcamera/include/camera_sensor.h > > +++ b/src/libcamera/include/camera_sensor.h > > @@ -13,12 +13,11 @@ > > #include <libcamera/geometry.h> > > > > #include "log.h" > > +#include "v4l2_controls.h" > > > > namespace libcamera { > > > > class MediaEntity; > > -class V4L2ControlInfo; > > -class V4L2ControlList; > > class V4L2Subdevice; > > > > struct V4L2SubdeviceFormat; > > @@ -43,7 +42,7 @@ public: > > const Size &size) const; > > int setFormat(V4L2SubdeviceFormat *format); > > > > - const V4L2ControlInfo *getControlInfo(unsigned int id) const; > > + const V4L2ControlInfoMap &controls() const; > > int getControls(V4L2ControlList *ctrls); > > int setControls(V4L2ControlList *ctrls); > > > > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h > > index 0047efab11fa..10b726504951 100644 > > --- a/src/libcamera/include/v4l2_controls.h > > +++ b/src/libcamera/include/v4l2_controls.h > > @@ -8,11 +8,11 @@ > > #ifndef __LIBCAMERA_V4L2_CONTROLS_H__ > > #define __LIBCAMERA_V4L2_CONTROLS_H__ > > > > +#include <map> > > +#include <stdint.h> > > #include <string> > > #include <vector> > > > > -#include <stdint.h> > > - > > #include <linux/v4l2-controls.h> > > #include <linux/videodev2.h> > > > > @@ -41,6 +41,8 @@ private: > > int64_t max_; > > }; > > > > +using V4L2ControlInfoMap = std::map<unsigned int, V4L2ControlInfo>; > > Does V4L2ControlInfoMap need to be ordered? > (I don't think we do at this part do we?) > > std::unordered_map could be faster if we don't need to order the map. > I think at the scale of controls we'll have it probably won't make much > difference... > > We could do some measurements perhaps on a map of 1000 elements vs an > unordered_map of 1000 elements, but I think that's the approx max scale > that we'll contain right? I think we'll be one, if not two orders of magnitude smaller. Would you like to perform measurements ? :-) > As the key is an unsigned int, I think the hasher will function out of > the box? Correct. > > + > > class V4L2Control > > { > > public: > > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h > > index 24a0134a2cba..e7e9829cb05f 100644 > > --- a/src/libcamera/include/v4l2_device.h > > +++ b/src/libcamera/include/v4l2_device.h > > @@ -13,19 +13,18 @@ > > #include <linux/videodev2.h> > > > > #include "log.h" > > +#include "v4l2_controls.h" > > > > namespace libcamera { > > > > -class V4L2ControlInfo; > > -class V4L2ControlList; > > - > > class V4L2Device : protected Loggable > > { > > public: > > void close(); > > bool isOpen() const { return fd_ != -1; } > > > > - const V4L2ControlInfo *getControlInfo(unsigned int id) const; > > + const V4L2ControlInfoMap &controls() const { return controls_; } > > + > > int getControls(V4L2ControlList *ctrls); > > int setControls(V4L2ControlList *ctrls); > > > > @@ -48,7 +47,7 @@ private: > > const struct v4l2_ext_control *v4l2Ctrls, > > unsigned int count); > > > > - std::map<unsigned int, V4L2ControlInfo> controls_; > > + V4L2ControlInfoMap controls_; > > std::string deviceNode_; > > int fd_; > > }; > > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp > > index af017bce48ba..84258d9954d0 100644 > > --- a/src/libcamera/v4l2_controls.cpp > > +++ b/src/libcamera/v4l2_controls.cpp > > @@ -114,6 +114,11 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl) > > * \return The V4L2 control maximum value > > */ > > > > +/** > > + * \typedef V4L2ControlInfoMap > > + * \brief A map of control ID to V4L2ControlInfo > > + */ > > + > > /** > > * \class V4L2Control > > * \brief A V4L2 control value > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > index 06939dead705..59fc98cefd4e 100644 > > --- a/src/libcamera/v4l2_device.cpp > > +++ b/src/libcamera/v4l2_device.cpp > > @@ -111,19 +111,10 @@ void V4L2Device::close() > > */ > > > > /** > > - * \brief Retrieve information about a control > > - * \param[in] id The control ID > > - * \return A pointer to the V4L2ControlInfo for control \a id, or nullptr > > - * if the device doesn't have such a control > > + * \fn V4L2Device::controls() > > + * \brief Retrieve the supported V4L2 controls > > + * \return A map of the V4L2 controls supported by the device > > */ > > -const V4L2ControlInfo *V4L2Device::getControlInfo(unsigned int id) const > > -{ > > - auto it = controls_.find(id); > > - if (it == controls_.end()) > > - return nullptr; > > - > > - return &it->second; > > -} > > > > /** > > * \brief Read controls from the device > > @@ -158,13 +149,14 @@ int V4L2Device::getControls(V4L2ControlList *ctrls) > > > > for (unsigned int i = 0; i < count; ++i) { > > const V4L2Control *ctrl = ctrls->getByIndex(i); > > - const V4L2ControlInfo *info = getControlInfo(ctrl->id()); > > - if (!info) { > > + const auto iter = controls_.find(ctrl->id()); > > + if (iter == controls_.end()) { > > LOG(V4L2, Error) > > << "Control '" << ctrl->id() << "' not found"; > > return -EINVAL; > > } > > > > + const V4L2ControlInfo *info = &iter->second; > > controlInfo[i] = info; > > v4l2Ctrls[i].id = info->id(); > > } > > @@ -232,13 +224,14 @@ int V4L2Device::setControls(V4L2ControlList *ctrls) > > > > for (unsigned int i = 0; i < count; ++i) { > > const V4L2Control *ctrl = ctrls->getByIndex(i); > > - const V4L2ControlInfo *info = getControlInfo(ctrl->id()); > > - if (!info) { > > + const auto iter = controls_.find(ctrl->id()); > > + if (iter == controls_.end()) { > > LOG(V4L2, Error) > > << "Control '" << ctrl->id() << "' not found"; > > return -EINVAL; > > } > > > > + const V4L2ControlInfo *info = &iter->second; > > controlInfo[i] = info; > > v4l2Ctrls[i].id = info->id(); > >
Hi Laurent, On Mon, Jul 01, 2019 at 02:38:06AM +0300, Laurent Pinchart wrote: > Add a new controls() method to the V4L2Device class to retrieve the map > of all supported controls. This is needed in order to dynamically query > the supported controls, for instance for drivers that support different > sets of controls depending on the device model. > > To make the API easier to use, create a type alias for the control ID to > ControlInfo and use it. > > Remove the getControlInfo() method that is not used externally, as it > can now be replaced by accessing the full list of controls. > > Update the CameraSensor API accordingly. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/camera_sensor.cpp | 10 ++++------ > src/libcamera/include/camera_sensor.h | 5 ++--- > src/libcamera/include/v4l2_controls.h | 6 ++++-- > src/libcamera/include/v4l2_device.h | 9 ++++----- > src/libcamera/v4l2_controls.cpp | 5 +++++ > src/libcamera/v4l2_device.cpp | 25 +++++++++---------------- > 6 files changed, 28 insertions(+), 32 deletions(-) > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > index 471c3e2e6e47..14f631e8ecf7 100644 > --- a/src/libcamera/camera_sensor.cpp > +++ b/src/libcamera/camera_sensor.cpp > @@ -248,14 +248,12 @@ int CameraSensor::setFormat(V4L2SubdeviceFormat *format) > } > > /** > - * \brief Retrieve information about a control > - * \param[in] id The control ID > - * \return A pointer to the V4L2ControlInfo for control \a id, or nullptr > - * if the sensor doesn't have such a control > + * \brief Retrieve the supported V4L2 controls > + * \return A map of the V4L2 controls supported by the sensor > */ > -const V4L2ControlInfo *CameraSensor::getControlInfo(unsigned int id) const > +const V4L2ControlInfoMap &CameraSensor::controls() const > { > - return subdev_->getControlInfo(id); > + return subdev_->controls(); > } > > /** > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h > index b42e7b8e1343..fe033fb374c1 100644 > --- a/src/libcamera/include/camera_sensor.h > +++ b/src/libcamera/include/camera_sensor.h > @@ -13,12 +13,11 @@ > #include <libcamera/geometry.h> > > #include "log.h" > +#include "v4l2_controls.h" > > namespace libcamera { > > class MediaEntity; > -class V4L2ControlInfo; > -class V4L2ControlList; > class V4L2Subdevice; > > struct V4L2SubdeviceFormat; > @@ -43,7 +42,7 @@ public: > const Size &size) const; > int setFormat(V4L2SubdeviceFormat *format); > > - const V4L2ControlInfo *getControlInfo(unsigned int id) const; > + const V4L2ControlInfoMap &controls() const; > int getControls(V4L2ControlList *ctrls); > int setControls(V4L2ControlList *ctrls); > > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h > index 0047efab11fa..10b726504951 100644 > --- a/src/libcamera/include/v4l2_controls.h > +++ b/src/libcamera/include/v4l2_controls.h > @@ -8,11 +8,11 @@ > #ifndef __LIBCAMERA_V4L2_CONTROLS_H__ > #define __LIBCAMERA_V4L2_CONTROLS_H__ > > +#include <map> > +#include <stdint.h> Nit: C and C++ includes mixed? > #include <string> > #include <vector> > > -#include <stdint.h> > - > #include <linux/v4l2-controls.h> > #include <linux/videodev2.h> > > @@ -41,6 +41,8 @@ private: > int64_t max_; > }; > > +using V4L2ControlInfoMap = std::map<unsigned int, V4L2ControlInfo>; > + > class V4L2Control > { > public: > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h > index 24a0134a2cba..e7e9829cb05f 100644 > --- a/src/libcamera/include/v4l2_device.h > +++ b/src/libcamera/include/v4l2_device.h > @@ -13,19 +13,18 @@ > #include <linux/videodev2.h> > > #include "log.h" > +#include "v4l2_controls.h" > > namespace libcamera { > > -class V4L2ControlInfo; > -class V4L2ControlList; > - > class V4L2Device : protected Loggable > { > public: > void close(); > bool isOpen() const { return fd_ != -1; } > > - const V4L2ControlInfo *getControlInfo(unsigned int id) const; > + const V4L2ControlInfoMap &controls() const { return controls_; } > + > int getControls(V4L2ControlList *ctrls); > int setControls(V4L2ControlList *ctrls); > > @@ -48,7 +47,7 @@ private: > const struct v4l2_ext_control *v4l2Ctrls, > unsigned int count); > > - std::map<unsigned int, V4L2ControlInfo> controls_; > + V4L2ControlInfoMap controls_; > std::string deviceNode_; > int fd_; > }; > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp > index af017bce48ba..84258d9954d0 100644 > --- a/src/libcamera/v4l2_controls.cpp > +++ b/src/libcamera/v4l2_controls.cpp > @@ -114,6 +114,11 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl) > * \return The V4L2 control maximum value > */ > > +/** > + * \typedef V4L2ControlInfoMap > + * \brief A map of control ID to V4L2ControlInfo Nit: shouldn't these be plurals? > + */ > + > /** > * \class V4L2Control > * \brief A V4L2 control value > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index 06939dead705..59fc98cefd4e 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -111,19 +111,10 @@ void V4L2Device::close() > */ > > /** > - * \brief Retrieve information about a control > - * \param[in] id The control ID > - * \return A pointer to the V4L2ControlInfo for control \a id, or nullptr > - * if the device doesn't have such a control > + * \fn V4L2Device::controls() > + * \brief Retrieve the supported V4L2 controls " and the associated control information" ? > + * \return A map of the V4L2 controls supported by the device > */ > -const V4L2ControlInfo *V4L2Device::getControlInfo(unsigned int id) const > -{ > - auto it = controls_.find(id); > - if (it == controls_.end()) > - return nullptr; > - > - return &it->second; > -} > > /** > * \brief Read controls from the device > @@ -158,13 +149,14 @@ int V4L2Device::getControls(V4L2ControlList *ctrls) > > for (unsigned int i = 0; i < count; ++i) { > const V4L2Control *ctrl = ctrls->getByIndex(i); > - const V4L2ControlInfo *info = getControlInfo(ctrl->id()); > - if (!info) { > + const auto iter = controls_.find(ctrl->id()); > + if (iter == controls_.end()) { > LOG(V4L2, Error) > << "Control '" << ctrl->id() << "' not found"; > return -EINVAL; > } > > + const V4L2ControlInfo *info = &iter->second; > controlInfo[i] = info; > v4l2Ctrls[i].id = info->id(); > } > @@ -232,13 +224,14 @@ int V4L2Device::setControls(V4L2ControlList *ctrls) > > for (unsigned int i = 0; i < count; ++i) { > const V4L2Control *ctrl = ctrls->getByIndex(i); > - const V4L2ControlInfo *info = getControlInfo(ctrl->id()); > - if (!info) { > + const auto iter = controls_.find(ctrl->id()); > + if (iter == controls_.end()) { > LOG(V4L2, Error) > << "Control '" << ctrl->id() << "' not found"; > return -EINVAL; > } > > + const V4L2ControlInfo *info = &iter->second; An operation wrapping the explicit lookup was probably shorter to write and read, but the change is good. Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > controlInfo[i] = info; > v4l2Ctrls[i].id = info->id(); > > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On Mon, Jul 01, 2019 at 05:05:44PM +0200, Jacopo Mondi wrote: > On Mon, Jul 01, 2019 at 02:38:06AM +0300, Laurent Pinchart wrote: > > Add a new controls() method to the V4L2Device class to retrieve the map > > of all supported controls. This is needed in order to dynamically query > > the supported controls, for instance for drivers that support different > > sets of controls depending on the device model. > > > > To make the API easier to use, create a type alias for the control ID to > > ControlInfo and use it. > > > > Remove the getControlInfo() method that is not used externally, as it > > can now be replaced by accessing the full list of controls. > > > > Update the CameraSensor API accordingly. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/libcamera/camera_sensor.cpp | 10 ++++------ > > src/libcamera/include/camera_sensor.h | 5 ++--- > > src/libcamera/include/v4l2_controls.h | 6 ++++-- > > src/libcamera/include/v4l2_device.h | 9 ++++----- > > src/libcamera/v4l2_controls.cpp | 5 +++++ > > src/libcamera/v4l2_device.cpp | 25 +++++++++---------------- > > 6 files changed, 28 insertions(+), 32 deletions(-) > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > > index 471c3e2e6e47..14f631e8ecf7 100644 > > --- a/src/libcamera/camera_sensor.cpp > > +++ b/src/libcamera/camera_sensor.cpp > > @@ -248,14 +248,12 @@ int CameraSensor::setFormat(V4L2SubdeviceFormat *format) > > } > > > > /** > > - * \brief Retrieve information about a control > > - * \param[in] id The control ID > > - * \return A pointer to the V4L2ControlInfo for control \a id, or nullptr > > - * if the sensor doesn't have such a control > > + * \brief Retrieve the supported V4L2 controls > > + * \return A map of the V4L2 controls supported by the sensor > > */ > > -const V4L2ControlInfo *CameraSensor::getControlInfo(unsigned int id) const > > +const V4L2ControlInfoMap &CameraSensor::controls() const > > { > > - return subdev_->getControlInfo(id); > > + return subdev_->controls(); > > } > > > > /** > > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h > > index b42e7b8e1343..fe033fb374c1 100644 > > --- a/src/libcamera/include/camera_sensor.h > > +++ b/src/libcamera/include/camera_sensor.h > > @@ -13,12 +13,11 @@ > > #include <libcamera/geometry.h> > > > > #include "log.h" > > +#include "v4l2_controls.h" > > > > namespace libcamera { > > > > class MediaEntity; > > -class V4L2ControlInfo; > > -class V4L2ControlList; > > class V4L2Subdevice; > > > > struct V4L2SubdeviceFormat; > > @@ -43,7 +42,7 @@ public: > > const Size &size) const; > > int setFormat(V4L2SubdeviceFormat *format); > > > > - const V4L2ControlInfo *getControlInfo(unsigned int id) const; > > + const V4L2ControlInfoMap &controls() const; > > int getControls(V4L2ControlList *ctrls); > > int setControls(V4L2ControlList *ctrls); > > > > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h > > index 0047efab11fa..10b726504951 100644 > > --- a/src/libcamera/include/v4l2_controls.h > > +++ b/src/libcamera/include/v4l2_controls.h > > @@ -8,11 +8,11 @@ > > #ifndef __LIBCAMERA_V4L2_CONTROLS_H__ > > #define __LIBCAMERA_V4L2_CONTROLS_H__ > > > > +#include <map> > > +#include <stdint.h> > > Nit: C and C++ includes mixed? I wondered about that, as it seemed to me it was the general rules in libcamera, and the fact that they were separated here made me hesitate. I checked through the code base, and we generally mix them. I also thought we had documented this in the coding style, but it seems we haven't. I then remembered it was documented in https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes. > > #include <string> > > #include <vector> > > > > -#include <stdint.h> > > - > > #include <linux/v4l2-controls.h> > > #include <linux/videodev2.h> > > > > @@ -41,6 +41,8 @@ private: > > int64_t max_; > > }; > > > > +using V4L2ControlInfoMap = std::map<unsigned int, V4L2ControlInfo>; > > + > > class V4L2Control > > { > > public: > > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h > > index 24a0134a2cba..e7e9829cb05f 100644 > > --- a/src/libcamera/include/v4l2_device.h > > +++ b/src/libcamera/include/v4l2_device.h > > @@ -13,19 +13,18 @@ > > #include <linux/videodev2.h> > > > > #include "log.h" > > +#include "v4l2_controls.h" > > > > namespace libcamera { > > > > -class V4L2ControlInfo; > > -class V4L2ControlList; > > - > > class V4L2Device : protected Loggable > > { > > public: > > void close(); > > bool isOpen() const { return fd_ != -1; } > > > > - const V4L2ControlInfo *getControlInfo(unsigned int id) const; > > + const V4L2ControlInfoMap &controls() const { return controls_; } > > + > > int getControls(V4L2ControlList *ctrls); > > int setControls(V4L2ControlList *ctrls); > > > > @@ -48,7 +47,7 @@ private: > > const struct v4l2_ext_control *v4l2Ctrls, > > unsigned int count); > > > > - std::map<unsigned int, V4L2ControlInfo> controls_; > > + V4L2ControlInfoMap controls_; > > std::string deviceNode_; > > int fd_; > > }; > > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp > > index af017bce48ba..84258d9954d0 100644 > > --- a/src/libcamera/v4l2_controls.cpp > > +++ b/src/libcamera/v4l2_controls.cpp > > @@ -114,6 +114,11 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl) > > * \return The V4L2 control maximum value > > */ > > > > +/** > > + * \typedef V4L2ControlInfoMap > > + * \brief A map of control ID to V4L2ControlInfo > > Nit: shouldn't these be plurals? I think both work ("this container maps a control ID to a V4L2ControlInfo" or "this containter maps control IDs to V4L2ControlInfo instances"). I went for the former to avoid adding "instances". Now that I'm re-reading the code I will also replace control ID with ControlId. > > + */ > > + > > /** > > * \class V4L2Control > > * \brief A V4L2 control value > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > index 06939dead705..59fc98cefd4e 100644 > > --- a/src/libcamera/v4l2_device.cpp > > +++ b/src/libcamera/v4l2_device.cpp > > @@ -111,19 +111,10 @@ void V4L2Device::close() > > */ > > > > /** > > - * \brief Retrieve information about a control > > - * \param[in] id The control ID > > - * \return A pointer to the V4L2ControlInfo for control \a id, or nullptr > > - * if the device doesn't have such a control > > + * \fn V4L2Device::controls() > > + * \brief Retrieve the supported V4L2 controls > > " and the associated control information" ? I'm not sure that's needed, but I'll add it :-) How about "and their information" to make it shorter ? > > + * \return A map of the V4L2 controls supported by the device > > */ > > -const V4L2ControlInfo *V4L2Device::getControlInfo(unsigned int id) const > > -{ > > - auto it = controls_.find(id); > > - if (it == controls_.end()) > > - return nullptr; > > - > > - return &it->second; > > -} > > > > /** > > * \brief Read controls from the device > > @@ -158,13 +149,14 @@ int V4L2Device::getControls(V4L2ControlList *ctrls) > > > > for (unsigned int i = 0; i < count; ++i) { > > const V4L2Control *ctrl = ctrls->getByIndex(i); > > - const V4L2ControlInfo *info = getControlInfo(ctrl->id()); > > - if (!info) { > > + const auto iter = controls_.find(ctrl->id()); > > + if (iter == controls_.end()) { > > LOG(V4L2, Error) > > << "Control '" << ctrl->id() << "' not found"; > > return -EINVAL; > > } > > > > + const V4L2ControlInfo *info = &iter->second; > > controlInfo[i] = info; > > v4l2Ctrls[i].id = info->id(); > > } > > @@ -232,13 +224,14 @@ int V4L2Device::setControls(V4L2ControlList *ctrls) > > > > for (unsigned int i = 0; i < count; ++i) { > > const V4L2Control *ctrl = ctrls->getByIndex(i); > > - const V4L2ControlInfo *info = getControlInfo(ctrl->id()); > > - if (!info) { > > + const auto iter = controls_.find(ctrl->id()); > > + if (iter == controls_.end()) { > > LOG(V4L2, Error) > > << "Control '" << ctrl->id() << "' not found"; > > return -EINVAL; > > } > > > > + const V4L2ControlInfo *info = &iter->second; > > An operation wrapping the explicit lookup was probably shorter to > write and read, but the change is good. Kieran made the same comment, and I agree. If the slightly longer error check is only needed here I think it's fine. If we end up finding that a pointer-or-nullptr version would be useful in more places, then I would like to look into how we could provide a utility to extend containers instead of having to add custom functions every time. e.g. const V4L2ControlInfo *info = utils::find(video_.controls(), ctrl->id()); That would still be slightly longer to write than const V4L2ControlInfo *info = video_.getControlInfo(ctrl->id()); but in my opinion reasonably so, while providing the convenience helper for all our containers, without needing to explicitly provide a separate method each time. > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > controlInfo[i] = info; > > v4l2Ctrls[i].id = info->id(); > >
Hi Laurent, On Mon, Jul 01, 2019 at 07:05:04PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > On Mon, Jul 01, 2019 at 05:05:44PM +0200, Jacopo Mondi wrote: > > On Mon, Jul 01, 2019 at 02:38:06AM +0300, Laurent Pinchart wrote: > > > Add a new controls() method to the V4L2Device class to retrieve the map > > > of all supported controls. This is needed in order to dynamically query > > > the supported controls, for instance for drivers that support different > > > sets of controls depending on the device model. > > > > > > To make the API easier to use, create a type alias for the control ID to > > > ControlInfo and use it. > > > > > > Remove the getControlInfo() method that is not used externally, as it > > > can now be replaced by accessing the full list of controls. > > > > > > Update the CameraSensor API accordingly. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > src/libcamera/camera_sensor.cpp | 10 ++++------ > > > src/libcamera/include/camera_sensor.h | 5 ++--- > > > src/libcamera/include/v4l2_controls.h | 6 ++++-- > > > src/libcamera/include/v4l2_device.h | 9 ++++----- > > > src/libcamera/v4l2_controls.cpp | 5 +++++ > > > src/libcamera/v4l2_device.cpp | 25 +++++++++---------------- > > > 6 files changed, 28 insertions(+), 32 deletions(-) > > > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > > > index 471c3e2e6e47..14f631e8ecf7 100644 > > > --- a/src/libcamera/camera_sensor.cpp > > > +++ b/src/libcamera/camera_sensor.cpp > > > @@ -248,14 +248,12 @@ int CameraSensor::setFormat(V4L2SubdeviceFormat *format) > > > } > > > > > > /** > > > - * \brief Retrieve information about a control > > > - * \param[in] id The control ID > > > - * \return A pointer to the V4L2ControlInfo for control \a id, or nullptr > > > - * if the sensor doesn't have such a control > > > + * \brief Retrieve the supported V4L2 controls > > > + * \return A map of the V4L2 controls supported by the sensor > > > */ > > > -const V4L2ControlInfo *CameraSensor::getControlInfo(unsigned int id) const > > > +const V4L2ControlInfoMap &CameraSensor::controls() const > > > { > > > - return subdev_->getControlInfo(id); > > > + return subdev_->controls(); > > > } > > > > > > /** > > > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h > > > index b42e7b8e1343..fe033fb374c1 100644 > > > --- a/src/libcamera/include/camera_sensor.h > > > +++ b/src/libcamera/include/camera_sensor.h > > > @@ -13,12 +13,11 @@ > > > #include <libcamera/geometry.h> > > > > > > #include "log.h" > > > +#include "v4l2_controls.h" > > > > > > namespace libcamera { > > > > > > class MediaEntity; > > > -class V4L2ControlInfo; > > > -class V4L2ControlList; > > > class V4L2Subdevice; > > > > > > struct V4L2SubdeviceFormat; > > > @@ -43,7 +42,7 @@ public: > > > const Size &size) const; > > > int setFormat(V4L2SubdeviceFormat *format); > > > > > > - const V4L2ControlInfo *getControlInfo(unsigned int id) const; > > > + const V4L2ControlInfoMap &controls() const; > > > int getControls(V4L2ControlList *ctrls); > > > int setControls(V4L2ControlList *ctrls); > > > > > > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h > > > index 0047efab11fa..10b726504951 100644 > > > --- a/src/libcamera/include/v4l2_controls.h > > > +++ b/src/libcamera/include/v4l2_controls.h > > > @@ -8,11 +8,11 @@ > > > #ifndef __LIBCAMERA_V4L2_CONTROLS_H__ > > > #define __LIBCAMERA_V4L2_CONTROLS_H__ > > > > > > +#include <map> > > > +#include <stdint.h> > > > > Nit: C and C++ includes mixed? > > I wondered about that, as it seemed to me it was the general rules in > libcamera, and the fact that they were separated here made me hesitate. > I checked through the code base, and we generally mix them. > > I also thought we had documented this in the coding style, but it seems > we haven't. I then remembered it was documented in > https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes. I see. I've been doing it wrong then > > > > #include <string> > > > #include <vector> > > > > > > -#include <stdint.h> > > > - > > > #include <linux/v4l2-controls.h> > > > #include <linux/videodev2.h> > > > > > > @@ -41,6 +41,8 @@ private: > > > int64_t max_; > > > }; > > > > > > +using V4L2ControlInfoMap = std::map<unsigned int, V4L2ControlInfo>; > > > + > > > class V4L2Control > > > { > > > public: > > > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h > > > index 24a0134a2cba..e7e9829cb05f 100644 > > > --- a/src/libcamera/include/v4l2_device.h > > > +++ b/src/libcamera/include/v4l2_device.h > > > @@ -13,19 +13,18 @@ > > > #include <linux/videodev2.h> > > > > > > #include "log.h" > > > +#include "v4l2_controls.h" > > > > > > namespace libcamera { > > > > > > -class V4L2ControlInfo; > > > -class V4L2ControlList; > > > - > > > class V4L2Device : protected Loggable > > > { > > > public: > > > void close(); > > > bool isOpen() const { return fd_ != -1; } > > > > > > - const V4L2ControlInfo *getControlInfo(unsigned int id) const; > > > + const V4L2ControlInfoMap &controls() const { return controls_; } > > > + > > > int getControls(V4L2ControlList *ctrls); > > > int setControls(V4L2ControlList *ctrls); > > > > > > @@ -48,7 +47,7 @@ private: > > > const struct v4l2_ext_control *v4l2Ctrls, > > > unsigned int count); > > > > > > - std::map<unsigned int, V4L2ControlInfo> controls_; > > > + V4L2ControlInfoMap controls_; > > > std::string deviceNode_; > > > int fd_; > > > }; > > > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp > > > index af017bce48ba..84258d9954d0 100644 > > > --- a/src/libcamera/v4l2_controls.cpp > > > +++ b/src/libcamera/v4l2_controls.cpp > > > @@ -114,6 +114,11 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl) > > > * \return The V4L2 control maximum value > > > */ > > > > > > +/** > > > + * \typedef V4L2ControlInfoMap > > > + * \brief A map of control ID to V4L2ControlInfo > > > > Nit: shouldn't these be plurals? > > I think both work ("this container maps a control ID to a > V4L2ControlInfo" or "this containter maps control IDs to V4L2ControlInfo > instances"). I went for the former to avoid adding "instances". Now that > I'm re-reading the code I will also replace control ID with ControlId. > No worries, was a very minor comment. > > > + */ > > > + > > > /** > > > * \class V4L2Control > > > * \brief A V4L2 control value > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > > index 06939dead705..59fc98cefd4e 100644 > > > --- a/src/libcamera/v4l2_device.cpp > > > +++ b/src/libcamera/v4l2_device.cpp > > > @@ -111,19 +111,10 @@ void V4L2Device::close() > > > */ > > > > > > /** > > > - * \brief Retrieve information about a control > > > - * \param[in] id The control ID > > > - * \return A pointer to the V4L2ControlInfo for control \a id, or nullptr > > > - * if the device doesn't have such a control > > > + * \fn V4L2Device::controls() > > > + * \brief Retrieve the supported V4L2 controls > > > > " and the associated control information" ? > > I'm not sure that's needed, but I'll add it :-) How about "and their > information" to make it shorter ? i this was very minor as well. As you wish. > > > > + * \return A map of the V4L2 controls supported by the device > > > */ > > > -const V4L2ControlInfo *V4L2Device::getControlInfo(unsigned int id) const > > > -{ > > > - auto it = controls_.find(id); > > > - if (it == controls_.end()) > > > - return nullptr; > > > - > > > - return &it->second; > > > -} > > > > > > /** > > > * \brief Read controls from the device > > > @@ -158,13 +149,14 @@ int V4L2Device::getControls(V4L2ControlList *ctrls) > > > > > > for (unsigned int i = 0; i < count; ++i) { > > > const V4L2Control *ctrl = ctrls->getByIndex(i); > > > - const V4L2ControlInfo *info = getControlInfo(ctrl->id()); > > > - if (!info) { > > > + const auto iter = controls_.find(ctrl->id()); > > > + if (iter == controls_.end()) { > > > LOG(V4L2, Error) > > > << "Control '" << ctrl->id() << "' not found"; > > > return -EINVAL; > > > } > > > > > > + const V4L2ControlInfo *info = &iter->second; > > > controlInfo[i] = info; > > > v4l2Ctrls[i].id = info->id(); > > > } > > > @@ -232,13 +224,14 @@ int V4L2Device::setControls(V4L2ControlList *ctrls) > > > > > > for (unsigned int i = 0; i < count; ++i) { > > > const V4L2Control *ctrl = ctrls->getByIndex(i); > > > - const V4L2ControlInfo *info = getControlInfo(ctrl->id()); > > > - if (!info) { > > > + const auto iter = controls_.find(ctrl->id()); > > > + if (iter == controls_.end()) { > > > LOG(V4L2, Error) > > > << "Control '" << ctrl->id() << "' not found"; > > > return -EINVAL; > > > } > > > > > > + const V4L2ControlInfo *info = &iter->second; > > > > An operation wrapping the explicit lookup was probably shorter to > > write and read, but the change is good. > > Kieran made the same comment, and I agree. If the slightly longer error > check is only needed here I think it's fine. If we end up finding that a > pointer-or-nullptr version would be useful in more places, then I would > like to look into how we could provide a utility to extend containers > instead of having to add custom functions every time. e.g. > > const V4L2ControlInfo *info = utils::find(video_.controls(), ctrl->id()); I would then prefer what we have here :) > > That would still be slightly longer to write than > > const V4L2ControlInfo *info = video_.getControlInfo(ctrl->id()); > > but in my opinion reasonably so, while providing the convenience helper > for all our containers, without needing to explicitly provide a separate > method each time. No worries, we're talking about details. What you have here is fine! Thanks j > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > controlInfo[i] = info; > > > v4l2Ctrls[i].id = info->id(); > > > > > -- > Regards, > > Laurent Pinchart
Hi Laurent, On 01/07/2019 12:00, Laurent Pinchart wrote: > Hi Kieran, > > On Mon, Jul 01, 2019 at 09:03:09AM +0100, Kieran Bingham wrote: >> On 01/07/2019 00:38, Laurent Pinchart wrote: >>> Add a new controls() method to the V4L2Device class to retrieve the map >>> of all supported controls. This is needed in order to dynamically query >>> the supported controls, for instance for drivers that support different >>> sets of controls depending on the device model. >>> >>> To make the API easier to use, create a type alias for the control ID to >>> ControlInfo and use it. >>> >>> Remove the getControlInfo() method that is not used externally, as it >>> can now be replaced by accessing the full list of controls. >> >> Maybe it's the C-dev in me but I kinda prefer the lookup, with a null >> return check over a lookup with open coding the iter != controls_.end(); >> >> Functionally the same I guess, I think it's just more ingrained in my >> head, but the .end() check is more 'STL' so it's certainly not a problem >> really. > > I agree, so if we find this is too much hasle to use, I'm fine adding > back a method to look up a single control (or I wonder if we could add a > helper method to make look up easier on std::map). > >>> Update the CameraSensor API accordingly. >> >> Small comment on the map type chosen, but I think either std::map is >> probably fine too. >> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> --- >>> src/libcamera/camera_sensor.cpp | 10 ++++------ >>> src/libcamera/include/camera_sensor.h | 5 ++--- >>> src/libcamera/include/v4l2_controls.h | 6 ++++-- >>> src/libcamera/include/v4l2_device.h | 9 ++++----- >>> src/libcamera/v4l2_controls.cpp | 5 +++++ >>> src/libcamera/v4l2_device.cpp | 25 +++++++++---------------- >>> 6 files changed, 28 insertions(+), 32 deletions(-) >>> >>> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp >>> index 471c3e2e6e47..14f631e8ecf7 100644 >>> --- a/src/libcamera/camera_sensor.cpp >>> +++ b/src/libcamera/camera_sensor.cpp >>> @@ -248,14 +248,12 @@ int CameraSensor::setFormat(V4L2SubdeviceFormat *format) >>> } >>> >>> /** >>> - * \brief Retrieve information about a control >>> - * \param[in] id The control ID >>> - * \return A pointer to the V4L2ControlInfo for control \a id, or nullptr >>> - * if the sensor doesn't have such a control >>> + * \brief Retrieve the supported V4L2 controls >>> + * \return A map of the V4L2 controls supported by the sensor >>> */ >>> -const V4L2ControlInfo *CameraSensor::getControlInfo(unsigned int id) const >>> +const V4L2ControlInfoMap &CameraSensor::controls() const >>> { >>> - return subdev_->getControlInfo(id); >>> + return subdev_->controls(); >>> } >>> >>> /** >>> diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h >>> index b42e7b8e1343..fe033fb374c1 100644 >>> --- a/src/libcamera/include/camera_sensor.h >>> +++ b/src/libcamera/include/camera_sensor.h >>> @@ -13,12 +13,11 @@ >>> #include <libcamera/geometry.h> >>> >>> #include "log.h" >>> +#include "v4l2_controls.h" >>> >>> namespace libcamera { >>> >>> class MediaEntity; >>> -class V4L2ControlInfo; >>> -class V4L2ControlList; >>> class V4L2Subdevice; >>> >>> struct V4L2SubdeviceFormat; >>> @@ -43,7 +42,7 @@ public: >>> const Size &size) const; >>> int setFormat(V4L2SubdeviceFormat *format); >>> >>> - const V4L2ControlInfo *getControlInfo(unsigned int id) const; >>> + const V4L2ControlInfoMap &controls() const; >>> int getControls(V4L2ControlList *ctrls); >>> int setControls(V4L2ControlList *ctrls); >>> >>> diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h >>> index 0047efab11fa..10b726504951 100644 >>> --- a/src/libcamera/include/v4l2_controls.h >>> +++ b/src/libcamera/include/v4l2_controls.h >>> @@ -8,11 +8,11 @@ >>> #ifndef __LIBCAMERA_V4L2_CONTROLS_H__ >>> #define __LIBCAMERA_V4L2_CONTROLS_H__ >>> >>> +#include <map> >>> +#include <stdint.h> >>> #include <string> >>> #include <vector> >>> >>> -#include <stdint.h> >>> - >>> #include <linux/v4l2-controls.h> >>> #include <linux/videodev2.h> >>> >>> @@ -41,6 +41,8 @@ private: >>> int64_t max_; >>> }; >>> >>> +using V4L2ControlInfoMap = std::map<unsigned int, V4L2ControlInfo>; >> >> Does V4L2ControlInfoMap need to be ordered? >> (I don't think we do at this part do we?) >> >> std::unordered_map could be faster if we don't need to order the map. >> I think at the scale of controls we'll have it probably won't make much >> difference... >> >> We could do some measurements perhaps on a map of 1000 elements vs an >> unordered_map of 1000 elements, but I think that's the approx max scale >> that we'll contain right? > > I think we'll be one, if not two orders of magnitude smaller. Would you > like to perform measurements ? :-) I've measured doing 1000 random lookups on maps of 1000 V4L2controlInfo structures: times in nanoSeconds: std::map<unsigned int, V4L2ControlInfo>; ~479000 nS std::unordered_map<unsigned int, V4L2ControlInfo>; ~175000 nS When the number of entries is less than 50, the differences are very minor. When it comes to creating a map and inserting only a small number of entries, in fact the std::map starts to win. Inserting 10 items, for 1000000 repeats 2707779741 nS: std::map: Create and Insert 3415575028 nS: std::unordered_map: Create and Insert Yet, we're still talking the difference between 2.7 microSeconds and 3.4 microseconds per list. So I think the optimisations here are minimal :) -- Kieran >> As the key is an unsigned int, I think the hasher will function out of >> the box? > > Correct. > >>> + >>> class V4L2Control >>> { >>> public: >>> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h >>> index 24a0134a2cba..e7e9829cb05f 100644 >>> --- a/src/libcamera/include/v4l2_device.h >>> +++ b/src/libcamera/include/v4l2_device.h >>> @@ -13,19 +13,18 @@ >>> #include <linux/videodev2.h> >>> >>> #include "log.h" >>> +#include "v4l2_controls.h" >>> >>> namespace libcamera { >>> >>> -class V4L2ControlInfo; >>> -class V4L2ControlList; >>> - >>> class V4L2Device : protected Loggable >>> { >>> public: >>> void close(); >>> bool isOpen() const { return fd_ != -1; } >>> >>> - const V4L2ControlInfo *getControlInfo(unsigned int id) const; >>> + const V4L2ControlInfoMap &controls() const { return controls_; } >>> + >>> int getControls(V4L2ControlList *ctrls); >>> int setControls(V4L2ControlList *ctrls); >>> >>> @@ -48,7 +47,7 @@ private: >>> const struct v4l2_ext_control *v4l2Ctrls, >>> unsigned int count); >>> >>> - std::map<unsigned int, V4L2ControlInfo> controls_; >>> + V4L2ControlInfoMap controls_; >>> std::string deviceNode_; >>> int fd_; >>> }; >>> diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp >>> index af017bce48ba..84258d9954d0 100644 >>> --- a/src/libcamera/v4l2_controls.cpp >>> +++ b/src/libcamera/v4l2_controls.cpp >>> @@ -114,6 +114,11 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl) >>> * \return The V4L2 control maximum value >>> */ >>> >>> +/** >>> + * \typedef V4L2ControlInfoMap >>> + * \brief A map of control ID to V4L2ControlInfo >>> + */ >>> + >>> /** >>> * \class V4L2Control >>> * \brief A V4L2 control value >>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp >>> index 06939dead705..59fc98cefd4e 100644 >>> --- a/src/libcamera/v4l2_device.cpp >>> +++ b/src/libcamera/v4l2_device.cpp >>> @@ -111,19 +111,10 @@ void V4L2Device::close() >>> */ >>> >>> /** >>> - * \brief Retrieve information about a control >>> - * \param[in] id The control ID >>> - * \return A pointer to the V4L2ControlInfo for control \a id, or nullptr >>> - * if the device doesn't have such a control >>> + * \fn V4L2Device::controls() >>> + * \brief Retrieve the supported V4L2 controls >>> + * \return A map of the V4L2 controls supported by the device >>> */ >>> -const V4L2ControlInfo *V4L2Device::getControlInfo(unsigned int id) const >>> -{ >>> - auto it = controls_.find(id); >>> - if (it == controls_.end()) >>> - return nullptr; >>> - >>> - return &it->second; >>> -} >>> >>> /** >>> * \brief Read controls from the device >>> @@ -158,13 +149,14 @@ int V4L2Device::getControls(V4L2ControlList *ctrls) >>> >>> for (unsigned int i = 0; i < count; ++i) { >>> const V4L2Control *ctrl = ctrls->getByIndex(i); >>> - const V4L2ControlInfo *info = getControlInfo(ctrl->id()); >>> - if (!info) { >>> + const auto iter = controls_.find(ctrl->id()); >>> + if (iter == controls_.end()) { >>> LOG(V4L2, Error) >>> << "Control '" << ctrl->id() << "' not found"; >>> return -EINVAL; >>> } >>> >>> + const V4L2ControlInfo *info = &iter->second; >>> controlInfo[i] = info; >>> v4l2Ctrls[i].id = info->id(); >>> } >>> @@ -232,13 +224,14 @@ int V4L2Device::setControls(V4L2ControlList *ctrls) >>> >>> for (unsigned int i = 0; i < count; ++i) { >>> const V4L2Control *ctrl = ctrls->getByIndex(i); >>> - const V4L2ControlInfo *info = getControlInfo(ctrl->id()); >>> - if (!info) { >>> + const auto iter = controls_.find(ctrl->id()); >>> + if (iter == controls_.end()) { >>> LOG(V4L2, Error) >>> << "Control '" << ctrl->id() << "' not found"; >>> return -EINVAL; >>> } >>> >>> + const V4L2ControlInfo *info = &iter->second; >>> controlInfo[i] = info; >>> v4l2Ctrls[i].id = info->id(); >>> >
diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp index 471c3e2e6e47..14f631e8ecf7 100644 --- a/src/libcamera/camera_sensor.cpp +++ b/src/libcamera/camera_sensor.cpp @@ -248,14 +248,12 @@ int CameraSensor::setFormat(V4L2SubdeviceFormat *format) } /** - * \brief Retrieve information about a control - * \param[in] id The control ID - * \return A pointer to the V4L2ControlInfo for control \a id, or nullptr - * if the sensor doesn't have such a control + * \brief Retrieve the supported V4L2 controls + * \return A map of the V4L2 controls supported by the sensor */ -const V4L2ControlInfo *CameraSensor::getControlInfo(unsigned int id) const +const V4L2ControlInfoMap &CameraSensor::controls() const { - return subdev_->getControlInfo(id); + return subdev_->controls(); } /** diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h index b42e7b8e1343..fe033fb374c1 100644 --- a/src/libcamera/include/camera_sensor.h +++ b/src/libcamera/include/camera_sensor.h @@ -13,12 +13,11 @@ #include <libcamera/geometry.h> #include "log.h" +#include "v4l2_controls.h" namespace libcamera { class MediaEntity; -class V4L2ControlInfo; -class V4L2ControlList; class V4L2Subdevice; struct V4L2SubdeviceFormat; @@ -43,7 +42,7 @@ public: const Size &size) const; int setFormat(V4L2SubdeviceFormat *format); - const V4L2ControlInfo *getControlInfo(unsigned int id) const; + const V4L2ControlInfoMap &controls() const; int getControls(V4L2ControlList *ctrls); int setControls(V4L2ControlList *ctrls); diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h index 0047efab11fa..10b726504951 100644 --- a/src/libcamera/include/v4l2_controls.h +++ b/src/libcamera/include/v4l2_controls.h @@ -8,11 +8,11 @@ #ifndef __LIBCAMERA_V4L2_CONTROLS_H__ #define __LIBCAMERA_V4L2_CONTROLS_H__ +#include <map> +#include <stdint.h> #include <string> #include <vector> -#include <stdint.h> - #include <linux/v4l2-controls.h> #include <linux/videodev2.h> @@ -41,6 +41,8 @@ private: int64_t max_; }; +using V4L2ControlInfoMap = std::map<unsigned int, V4L2ControlInfo>; + class V4L2Control { public: diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h index 24a0134a2cba..e7e9829cb05f 100644 --- a/src/libcamera/include/v4l2_device.h +++ b/src/libcamera/include/v4l2_device.h @@ -13,19 +13,18 @@ #include <linux/videodev2.h> #include "log.h" +#include "v4l2_controls.h" namespace libcamera { -class V4L2ControlInfo; -class V4L2ControlList; - class V4L2Device : protected Loggable { public: void close(); bool isOpen() const { return fd_ != -1; } - const V4L2ControlInfo *getControlInfo(unsigned int id) const; + const V4L2ControlInfoMap &controls() const { return controls_; } + int getControls(V4L2ControlList *ctrls); int setControls(V4L2ControlList *ctrls); @@ -48,7 +47,7 @@ private: const struct v4l2_ext_control *v4l2Ctrls, unsigned int count); - std::map<unsigned int, V4L2ControlInfo> controls_; + V4L2ControlInfoMap controls_; std::string deviceNode_; int fd_; }; diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp index af017bce48ba..84258d9954d0 100644 --- a/src/libcamera/v4l2_controls.cpp +++ b/src/libcamera/v4l2_controls.cpp @@ -114,6 +114,11 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl) * \return The V4L2 control maximum value */ +/** + * \typedef V4L2ControlInfoMap + * \brief A map of control ID to V4L2ControlInfo + */ + /** * \class V4L2Control * \brief A V4L2 control value diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp index 06939dead705..59fc98cefd4e 100644 --- a/src/libcamera/v4l2_device.cpp +++ b/src/libcamera/v4l2_device.cpp @@ -111,19 +111,10 @@ void V4L2Device::close() */ /** - * \brief Retrieve information about a control - * \param[in] id The control ID - * \return A pointer to the V4L2ControlInfo for control \a id, or nullptr - * if the device doesn't have such a control + * \fn V4L2Device::controls() + * \brief Retrieve the supported V4L2 controls + * \return A map of the V4L2 controls supported by the device */ -const V4L2ControlInfo *V4L2Device::getControlInfo(unsigned int id) const -{ - auto it = controls_.find(id); - if (it == controls_.end()) - return nullptr; - - return &it->second; -} /** * \brief Read controls from the device @@ -158,13 +149,14 @@ int V4L2Device::getControls(V4L2ControlList *ctrls) for (unsigned int i = 0; i < count; ++i) { const V4L2Control *ctrl = ctrls->getByIndex(i); - const V4L2ControlInfo *info = getControlInfo(ctrl->id()); - if (!info) { + const auto iter = controls_.find(ctrl->id()); + if (iter == controls_.end()) { LOG(V4L2, Error) << "Control '" << ctrl->id() << "' not found"; return -EINVAL; } + const V4L2ControlInfo *info = &iter->second; controlInfo[i] = info; v4l2Ctrls[i].id = info->id(); } @@ -232,13 +224,14 @@ int V4L2Device::setControls(V4L2ControlList *ctrls) for (unsigned int i = 0; i < count; ++i) { const V4L2Control *ctrl = ctrls->getByIndex(i); - const V4L2ControlInfo *info = getControlInfo(ctrl->id()); - if (!info) { + const auto iter = controls_.find(ctrl->id()); + if (iter == controls_.end()) { LOG(V4L2, Error) << "Control '" << ctrl->id() << "' not found"; return -EINVAL; } + const V4L2ControlInfo *info = &iter->second; controlInfo[i] = info; v4l2Ctrls[i].id = info->id();
Add a new controls() method to the V4L2Device class to retrieve the map of all supported controls. This is needed in order to dynamically query the supported controls, for instance for drivers that support different sets of controls depending on the device model. To make the API easier to use, create a type alias for the control ID to ControlInfo and use it. Remove the getControlInfo() method that is not used externally, as it can now be replaced by accessing the full list of controls. Update the CameraSensor API accordingly. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/camera_sensor.cpp | 10 ++++------ src/libcamera/include/camera_sensor.h | 5 ++--- src/libcamera/include/v4l2_controls.h | 6 ++++-- src/libcamera/include/v4l2_device.h | 9 ++++----- src/libcamera/v4l2_controls.cpp | 5 +++++ src/libcamera/v4l2_device.cpp | 25 +++++++++---------------- 6 files changed, 28 insertions(+), 32 deletions(-)