[libcamera-devel] [PATCH v3 03/14] libcamera: v4l2_device: Add method to retrieve all supported controls
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jul 1 18:05:04 CEST 2019
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 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?
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 at jmondi.org>
>
> > controlInfo[i] = info;
> > v4l2Ctrls[i].id = info->id();
> >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list