[libcamera-devel] [PATCH v5 2/6] libcamera: v4l2_device: List valid controls at open
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jun 24 13:49:19 CEST 2019
Hi Jacopo,
On Mon, Jun 24, 2019 at 11:26:12AM +0200, Jacopo Mondi wrote:
> On Mon, Jun 24, 2019 at 10:19:17AM +0200, Jacopo Mondi wrote:
> > On Sat, Jun 22, 2019 at 08:16:52PM +0300, Laurent Pinchart wrote:
> >> On Thu, Jun 20, 2019 at 05:31:40PM +0200, Jacopo Mondi wrote:
> >>> Enumerate all the valid controls a device supports at open() time.
> >>> A control is valid only if its type is supported.
> >>>
> >>> Store the control informations in a map inside the device to save
> >>
> >> s/informations/information/
> >>
> >> https://en.wiktionary.org/wiki/informations
> >>
> >> "Most senses of the word “information” are uncountable. The legal sense,
> >> referring to court filings, is one that does form a plural."
> >>
> >>> querying the control when setting or getting its value from the device
> >>> and provide an operation to retrieve information by control ID.
> >>>
> >>> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> >>> ---
> >>> src/libcamera/include/v4l2_device.h | 9 ++++
> >>> src/libcamera/v4l2_device.cpp | 72 +++++++++++++++++++++++++++++
> >>> 2 files changed, 81 insertions(+)
> >>>
> >>> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> >>> index 75f23a05b903..91a72fcecbcc 100644
> >>> --- a/src/libcamera/include/v4l2_device.h
> >>> +++ b/src/libcamera/include/v4l2_device.h
> >>> @@ -7,18 +7,23 @@
> >>> #ifndef __LIBCAMERA_V4L2_DEVICE_H__
> >>> #define __LIBCAMERA_V4L2_DEVICE_H__
> >>>
> >>> +#include <map>
> >>> #include <string>
> >>>
> >>> #include "log.h"
> >>>
> >>> namespace libcamera {
> >>>
> >>> +class V4L2ControlInfo;
> >>> +
> >>> class V4L2Device : protected Loggable
> >>> {
> >>> public:
> >>> void close();
> >>> bool isOpen() const { return fd_ != -1; }
> >>>
> >>> + const V4L2ControlInfo *getControlInfo(unsigned int id) const;
> >>> +
> >>> const std::string &deviceNode() const { return deviceNode_; }
> >>>
> >>> protected:
> >>> @@ -32,6 +37,10 @@ protected:
> >>> int fd() { return fd_; }
> >>>
> >>> private:
> >>> + void listControls();
> >>> + int validateControlType(const V4L2ControlInfo *info);
> >>> +
> >>> + std::map<unsigned int, V4L2ControlInfo> controls_;
> >>> std::string deviceNode_;
> >>> int fd_;
> >>> };
> >>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> >>> index 99621a724b96..b113ff77e3cf 100644
> >>> --- a/src/libcamera/v4l2_device.cpp
> >>> +++ b/src/libcamera/v4l2_device.cpp
> >>> @@ -13,6 +13,7 @@
> >>> #include <unistd.h>
> >>>
> >>> #include "log.h"
> >>> +#include "v4l2_controls.h"
> >>>
> >>> /**
> >>> * \file v4l2_device.h
> >>> @@ -82,6 +83,8 @@ int V4L2Device::open(unsigned int flags)
> >>>
> >>> fd_ = ret;
> >>>
> >>> + listControls();
> >>> +
> >>> return 0;
> >>> }
> >>>
> >>> @@ -107,6 +110,21 @@ void V4L2Device::close()
> >>> * \return True if the V4L2 device node is open, false otherwise
> >>> */
> >>>
> >>> +/**
> >>> + * \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
> >>> + */
> >>> +const V4L2ControlInfo *V4L2Device::getControlInfo(unsigned int id) const
> >>> +{
> >>> + auto it = controls_.find(id);
> >>> + if (it == controls_.end())
> >>> + return nullptr;
> >>> +
> >>> + return &it->second;
> >>> +}
> >>> +
> >>> /**
> >>> * \brief Perform an IOCTL system call on the device node
> >>> * \param[in] request The IOCTL request code
> >>> @@ -137,4 +155,58 @@ int V4L2Device::ioctl(unsigned long request, void *argp)
> >>> * \return The V4L2 device file descriptor, -1 if the device node is not open
> >>> */
> >>>
> >>> +/*
> >>> + * \brief List and store information about all controls supported by the
> >>> + * V4L2 device
> >>> + */
> >>> +void V4L2Device::listControls()
> >>> +{
> >>> + struct v4l2_query_ext_ctrl ctrl = {};
> >>> +
> >>> + /* \todo Add support for menu and compound controls. */
> >>> + ctrl.id = V4L2_CTRL_FLAG_NEXT_CTRL;
> >>> + while (ioctl(VIDIOC_QUERY_EXT_CTRL, &ctrl) == 0) {
> >>> + if (ctrl.type == V4L2_CTRL_TYPE_CTRL_CLASS ||
> >>> + ctrl.flags & V4L2_CTRL_FLAG_DISABLED) {
> >>> + ctrl.id |= V4L2_CTRL_FLAG_NEXT_CTRL;
> >>> + continue;
> >>> + }
> >>> +
> >>> + V4L2ControlInfo info(ctrl);
> >>> + if (validateControlType(&info))
> >>> + continue;
> >>> +
> >>> + controls_.insert(std::pair<unsigned int, V4L2ControlInfo>
> >>> + (ctrl.id, info));
> >>
> >> Anything wrong with
> >>
> >> controls_[ctrl.id] = info;
> >>
> >> ?
> >
> > No, I could change it. I used std::vector::insert when I was
> > constructing the V4L2ControlInfo directly in the operation call, it's
> > a leftover.
>
> Adtually, I'm a bit puzzled here. My understanding is that
> controls_[ctrl.id] = info;
>
> should insert a new element by calling the copy constructor of
> V4L2ControlInfo (which is defaulted and accessible)
>
> Howevere, I get this back from the compiler
>
> /usr/include/c++/8.3.0/tuple:1668:70: error: no matching function for call to ‘libcamera::V4L2ControlInfo::V4L2ControlInfo()’
> second(std::forward<_Args2>(std::get<_Indexes2>(__tuple2))...)
> ^
> In file included from ../src/libcamera/v4l2_device.cpp:16:
> ../src/libcamera/include/v4l2_controls.h:25:2: note: candidate: ‘libcamera::V4L2ControlInfo::V4L2ControlInfo(const libcamera::V4L2ControlInfo&)’
> V4L2ControlInfo(const V4L2ControlInfo &) = default;
> ^~~~~~~~~~~~~~~
> ../src/libcamera/include/v4l2_controls.h:25:2: note: candidate expects 1 argument, 0 provided
> ../src/libcamera/include/v4l2_controls.h:24:2: note: candidate: ‘libcamera::V4L2ControlInfo::V4L2ControlInfo(const v4l2_query_ext_ctrl&)’
> V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl);
> ^~~~~~~~~~~~~~~
> ../src/libcamera/include/v4l2_controls.h:24:2: note: candidate expects 1 argument, 0 provided
controls_[ctrl.id] will create an empty element, and thus needs a
default constructor that V4L2ControlInfo doesn't provide. The operator
returns a reference to the newly created element, and the copy operator=
is then invoked. The copy constructor isn't involved at any time in that
line.
> I'm now keeping the explicit ::insert() call for now.
If you want to make it efficient, use map::emplace()
controls_.emplace(ctrl.id, info);
> >>> + ctrl.id |= V4L2_CTRL_FLAG_NEXT_CTRL;
> >>> + }
> >>> +}
> >>> +
> >>> +/*
> >>> + * \brief Make sure the control type is supported
> >>> + * \param[in] info The V4L2ControlInfo to inspect type of
> >>> + * \return 0 on success or a negative error code otherwise
> >>> + * \retval -EINVAL The control type is not supported
> >>> + */
> >>> +int V4L2Device::validateControlType(const V4L2ControlInfo *info)
> >>
> >> As this method has a single user, how about inlining it ?
> >> V4L2Device::listControls() isn't big.
> >
> > I would rather keep the type validation centralized in one place, and in
> > the caller it's easy enough to parse if (validateControlType(type))
> > for a reader than going through the switch cases.
> >
> > The cost of a function call is negligible considering this happens
> > only at device open time.
It's not about the cost of a function call, I would find it more
readable :-)
> >>> +{
> >>> + /* \todo Support compound controls. */
> >>> + switch (info->type()) {
> >>> + case V4L2_CTRL_TYPE_INTEGER:
> >>> + case V4L2_CTRL_TYPE_BOOLEAN:
> >>> + case V4L2_CTRL_TYPE_MENU:
> >>> + case V4L2_CTRL_TYPE_BUTTON:
> >>> + case V4L2_CTRL_TYPE_INTEGER64:
> >>> + case V4L2_CTRL_TYPE_BITMASK:
> >>> + case V4L2_CTRL_TYPE_INTEGER_MENU:
> >>> + break;
> >>> + default:
> >>> + LOG(V4L2, Error) << "Control type '" << info->type()
> >>> + << "' not supported";
> >>> + return -EINVAL;
> >>> + }
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> } /* namespace libcamera */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list