[libcamera-devel] [PATCH v3 03/14] libcamera: v4l2_device: Add method to retrieve all supported controls
Jacopo Mondi
jacopo at jmondi.org
Mon Jul 1 17:05:44 CEST 2019
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 at 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 at jmondi.org>
Thanks
j
> controlInfo[i] = info;
> v4l2Ctrls[i].id = info->id();
>
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190701/5bdb0d11/attachment-0001.sig>
More information about the libcamera-devel
mailing list