[libcamera-devel] [PATCH 4/6] libcamera: v4l2_base: Add V4L2 control support
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Jun 8 18:07:08 CEST 2019
Hi Jacopo,
Thank you for the patch.
On Mon, Jun 03, 2019 at 01:32:58PM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> Some thoughts out loud inline....
>
> On 02/06/2019 14:04, Jacopo Mondi wrote:
> > Implement operations to set and get V4L2 controls in the V4L2Base class.
> > The operations allow to set and get a single or a list of controls.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > src/libcamera/include/v4l2_base.h | 12 ++
> > src/libcamera/v4l2_base.cpp | 290 ++++++++++++++++++++++++++++++
> > 2 files changed, 302 insertions(+)
> >
> > diff --git a/src/libcamera/include/v4l2_base.h b/src/libcamera/include/v4l2_base.h
> > index 2fda81a960d2..7d4e238dadfa 100644
> > --- a/src/libcamera/include/v4l2_base.h
> > +++ b/src/libcamera/include/v4l2_base.h
> > @@ -9,6 +9,8 @@
> >
> > #include <vector>
> >
> > +#include <v4l2_controls.h>
This should be "v4l2_controls.h", but you can simply forward-declare the
V4L2Control class instead.
> > +
> > namespace libcamera {
> >
> > class V4L2Base
> > @@ -22,8 +24,18 @@ public:
> > virtual void close() = 0;
> > bool isOpen() const;
> >
> > + std::vector<V4L2Control *> getControls(std::vector<unsigned int> &ctrl_ids);
A vector of pointer isn't very nice as it requires the caller to delete
each item. I would prefer passing a reference to a vector (or map) of
values to the function, and having it fill the values.
> In my controls, I pass a list <well a map, or a set; I have two
> implementations currently> of the actual ControlValue objects, which get
> filled in.
>
> I wonder if there will be much differences between the two interface
> designs.
One big difference is that a vector is ordered while a map or set isn't.
The order in which controls are set can make a difference.
> > + V4L2Control *getControl(unsigned int ctrl_id);
> > + int setControls(std::vector<V4L2Control *> &ctrls);
> > + int setControl(V4L2Control *ctrl);
>
> I think I'd group those as getControls setControls, and then getControl,
> setControl, so that they are grouped by 'functionality' but that might
> not be a preferred sorting... so either way.
I would prefer that as well.
> > +
> > protected:
> > int fd_;
> > +
> > +private:
> > + int queryControl(unsigned int ctrl_id,
> > + struct v4l2_query_ext_ctrl *qext_ctrl);
> > +
> > };
> >
> > }; /* namespace libcamera */
> > diff --git a/src/libcamera/v4l2_base.cpp b/src/libcamera/v4l2_base.cpp
> > index 7d05a3c82e4d..423a4d84e276 100644
> > --- a/src/libcamera/v4l2_base.cpp
> > +++ b/src/libcamera/v4l2_base.cpp
> > @@ -5,7 +5,13 @@
> > * v4l2_base.cpp - Common base for V4L2 devices and subdevices
> > */
> >
> > +#include <linux/v4l2-subdev.h>
> > +#include <linux/videodev2.h>
> > +#include <sys/ioctl.h>
> > +
> > +#include "log.h"
> > #include "v4l2_base.h"
The issue comes from a previous patch, but v4l2_base.h should be the
very first header included, to help ensuring it is self-contained.
> > +#include "v4l2_controls.h"
> >
> > /**
> > * \file v4l2_base.h
> > @@ -14,6 +20,8 @@
> >
> > namespace libcamera {
> >
> > +LOG_DEFINE_CATEGORY(V4L2Base)
> > +
> > /**
> > * \class V4L2Base
> > * \brief Base class for V4L2Device and V4L2Subdevice
> > @@ -31,6 +39,7 @@ namespace libcamera {
> > */
> >
> > /**
> > + * \fn V4L2Base::open()
Does this belong to this patch ? Same for the next method.
> > * \brief Open a V4L2 device or subdevice
> > *
> > * Pure virtual method to be implemented by the derived classes.
> > @@ -39,6 +48,7 @@ namespace libcamera {
> > */
> >
> > /**
> > + * \fn V4L2Base::close()
> > * \brief Close the device or subdevice
> > *
> > * Pure virtual method to be implemented by the derived classes.
> > @@ -53,6 +63,286 @@ bool V4L2Base::isOpen() const
> > return fd_ != -1;
> > }
> >
> > +int V4L2Base::queryControl(unsigned int id,
> > + struct v4l2_query_ext_ctrl *qext_ctrl)
> > +{
> > + qext_ctrl->id = id;
> > +
> > + int ret = ioctl(fd_, VIDIOC_QUERY_EXT_CTRL, qext_ctrl);
> > + if (ret) {
> > + ret = -errno;
> > + LOG(V4L2Base, Error)
> > + << "Unable to query extended control 0x"
s/extended //
> > + << std::hex << qext_ctrl->id
> > + << ": " << strerror(-ret);
> > + return ret;
> > + }
> > +
> > + if (qext_ctrl->flags & V4L2_CTRL_FLAG_DISABLED) {
> > + LOG(V4L2Base, Error)
> > + << "Extended control 0x" << std::hex
> > + << qext_ctrl->id << " is disabled";
> > + return -EINVAL;
> > + }
I would handle this in the caller, it's not a queryControl() error as
such.
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * \brief Get values of a list of control IDs
> > + * \param ctrl_ids The list of control IDs to retrieve value of
> > + * \return A list of V4L2Control on success or a empty list otherwise
> > + */
> > +std::vector<V4L2Control *> V4L2Base::getControls(std::vector<unsigned int> &ctrl_ids)
> > +{
> > + unsigned int count = ctrl_ids.size();
> > + if (!count)
> > + return std::vector<V4L2Control *> {};
> > +
> > + struct v4l2_ext_control *v4l2_ctrls = static_cast<struct v4l2_ext_control *>
> > + (new struct v4l2_ext_control[count]);
> > + if (!v4l2_ctrls)
> > + return std::vector<V4L2Control *> {};
> > +
> > + /*
> > + * Query each control in the vector to get back its type and expected
> > + * size in order to get its current value.
> > + */
> > + for (unsigned int i = 0; i < count; ++i) {
> > + struct v4l2_ext_control *v4l2_ctrl = &v4l2_ctrls[i];
> > + unsigned int ctrl_id = ctrl_ids[i];
> > + struct v4l2_query_ext_ctrl qext_ctrl = {};
> > +
> > + int ret = queryControl(ctrl_id, &qext_ctrl);
> > + if (ret)
> > + return std::vector<V4L2Control *> {};
> > +
> > + v4l2_ctrl->id = ctrl_id;
> > + v4l2_ctrl->size = qext_ctrl.elem_size * qext_ctrl.elems;
> > +
> > + /* Reserve memory for controls with payload. */
> > + if (!(qext_ctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD))
> > + continue;
> > +
> > + switch (qext_ctrl.type) {
> > + case V4L2_CTRL_TYPE_U8:
> > + v4l2_ctrl->p_u8 = static_cast<uint8_t *>
> > + (new uint8_t[v4l2_ctrl->size]);
> > + break;
> > + case V4L2_CTRL_TYPE_U16:
> > + v4l2_ctrl->p_u16 = static_cast<uint16_t *>
> > + (new uint16_t[v4l2_ctrl->size]);
> > + break;
> > + case V4L2_CTRL_TYPE_U32:
> > + v4l2_ctrl->p_u32 = static_cast<uint32_t *>
> > + (new uint32_t[v4l2_ctrl->size]);
> > + break;
> > + }
> > + }
> > +
> > + struct v4l2_ext_controls v4l2_ext_ctrls = {};
> > + v4l2_ext_ctrls.which = V4L2_CTRL_WHICH_CUR_VAL;
> > + v4l2_ext_ctrls.count = count;
> > + v4l2_ext_ctrls.controls = v4l2_ctrls;
> > +
> > + int ret = ioctl(fd_, VIDIOC_G_EXT_CTRLS, &v4l2_ext_ctrls);
> > + if (ret) {
> > + ret = -errno;
> > + LOG(V4L2Base, Error)
> > + << "Unable to get extended controls: " << strerror(-ret);
> > + return std::vector<V4L2Control *> {};
> > + }
> > +
> > + /*
> > + * For each requested control, create a V4L2Control which contains
> > + * the current value and store it in the vector returned to the caller.
> > + */
> > + std::vector<V4L2Control *> controls = {};
> > + for (unsigned int i = 0; i < count; ++i) {
> > + struct v4l2_ext_control *v4l2_ctrl = &v4l2_ctrls[i];
> > + struct v4l2_query_ext_ctrl qext_ctrl = {};
> > +
> > + int ret = queryControl(v4l2_ctrl->id, &qext_ctrl);
> > + if (ret)
> > + return std::vector<V4L2Control *> {};
> > +
> > + unsigned int size = qext_ctrl.elem_size * qext_ctrl.elem_size;
> > + if (qext_ctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {
> > + /* Control types with payload */
> > + switch (qext_ctrl.type) {
> > + case V4L2_CTRL_TYPE_U8:
> > + {
> > + V4L2U8Control *c =
> > + new V4L2U8Control(v4l2_ctrl->id, size,
> > + v4l2_ctrl->p_u8);
> > + controls.push_back(c);
> > + }
> > + break;
> > + case V4L2_CTRL_TYPE_U16:
> > + {
> > + V4L2U16Control *c =
> > + new V4L2U16Control(v4l2_ctrl->id, size,
> > + v4l2_ctrl->p_u16);
> > + controls.push_back(c);
> > + }
> > + break;
> > + case V4L2_CTRL_TYPE_U32:
> > + {
> > + V4L2U32Control *c =
> > + new V4L2U32Control(v4l2_ctrl->id, size,
> > + v4l2_ctrl->p_u32);
> > + controls.push_back(c);
> > + }
> > + break;
> > + default:
> > + LOG(V4L2Base, Error)
> > + << "Compound control types not supported: "
> > + << std::hex << qext_ctrl.type;
> > + return std::vector<V4L2Control *> {};
> > + }
> > + } else {
> > + /* Control types without payload. */
> > + switch (qext_ctrl.type) {
> > + case V4L2_CTRL_TYPE_INTEGER64:
> > + {
> > + V4L2Int64Control *c =
> > + new V4L2Int64Control(v4l2_ctrl->id,
> > + v4l2_ctrl->value64);
> > + controls.push_back(c);
>
> I think this could be a
> controls.emplace(V4L2Int64Control(v4l2_ctrl->id,
> v4l2_ctrl->value64);
>
> if you prefer that style ...
>
> Aha - except you have a vector of pointers, rather than a vector of
> objects ...
>
> > + }
> > + break;
> > + case V4L2_CTRL_TYPE_INTEGER:
> > + case V4L2_CTRL_TYPE_BOOLEAN:
> > + case V4L2_CTRL_TYPE_MENU:
> > + case V4L2_CTRL_TYPE_BUTTON:
> > + {
> > + V4L2IntControl *c =
> > + new V4L2IntControl(v4l2_ctrl->id,
> > + v4l2_ctrl->value);
> > + controls.push_back(c);
> > + }
> > + break;
> > + default:
> > + LOG(V4L2Base, Error)
> > + << "Invalid non-compound control type :"
> > + << std::hex << qext_ctrl.type;
> > + return std::vector<V4L2Control *> {};
> > + }
> > + }
> > + }
> > +
> > + return controls;
> > +}
> > +
> > +/**
> > + * \brief Get the value of control with id \a ctrl_id
> > + * \param ctrl_id The control id
> > + * \return The V4L2Control which represent the value of the control on success
> > + * nullptr otherwise
> > + */
> > +V4L2Control *V4L2Base::getControl(unsigned int ctrl_id)
> > +{
> > + std::vector<unsigned int> v = { ctrl_id, };
> > + std::vector<V4L2Control *> ctrls = getControls(v);
> > +
> > + if (ctrls.empty())
> > + return nullptr;
> > +
> > + return ctrls[0];
> > +}
> > +
> > +/**
> > + * \brief Apply a list of controls to the device
> > + * \param ctrls The list of controls to apply
> > + * \return 0 on success or a negative error code otherwise
> > + */
> > +int V4L2Base::setControls(std::vector<V4L2Control *> &ctrls)
> > +{
> > + unsigned int count = ctrls.size();
size_t instead of unsigned int ?
> > + if (!count)
> > + return -EINVAL;
> > +
> > + struct v4l2_ext_control *v4l2_ctrls = static_cast<struct v4l2_ext_control *>
> > + (new struct v4l2_ext_control[count]);
Do you need the cast ?
> > + if (!v4l2_ctrls)
> > + return -ENOMEM;
new() will throw an exception if it can't construct the object, so
there's no need to check the return value.
> > +
> > + /*
> > + * Query each control in the vector to get back its type and expected
> > + * size and set the new desired value in each v4l2_ext_control member.
> > + */
> > + for (unsigned int i = 0; i < count; ++i) {
> > + struct v4l2_ext_control *v4l2_ctrl = &v4l2_ctrls[i];
> > + struct v4l2_query_ext_ctrl qext_ctrl = {};
> > + V4L2Control *ctrl = ctrls[i];
> > +
> > + int ret = queryControl(ctrl->id(), &qext_ctrl);
> > + if (ret)
> > + return ret;
It's annoying to have to query each control every time. We should either
enumerate all of them when opening the device, or query them at runtime
but cache the control information.
> > +
> > + v4l2_ctrl->id = ctrl->id();
> > + v4l2_ctrl->size = qext_ctrl.elem_size * qext_ctrl.elems;
> > +
> > + if (qext_ctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {
> > + /** \todo: Support set controls with payload. */
> > + LOG(V4L2Base, Error)
> > + << "Controls with payload not supported";
> > + return -EINVAL;
> > + } else {
> > + switch (qext_ctrl.type) {
> > + case V4L2_CTRL_TYPE_INTEGER64:
> > + {
> > + V4L2Int64Control *c =
> > + static_cast<V4L2Int64Control *>(ctrl);
>
> It's these static_casts that feel a bit horrible to me. In my
> implementation there is just single class, so there is no casting necessary,
I don't like the casts much either, especially given that you cast based
on the type returned by the kernel, which could possibly not match the
type of the control as set by the application. This would likely lead to
memory corruption and crashes.
> > + v4l2_ctrl->value64 = c->value();
>
> However, this would be c->getInteger64();
>
> > + }
> > + break;
> > + case V4L2_CTRL_TYPE_INTEGER:
> > + case V4L2_CTRL_TYPE_BOOLEAN:
> > + case V4L2_CTRL_TYPE_MENU:
> > + case V4L2_CTRL_TYPE_BUTTON:
> > + {
> > + V4L2IntControl *c =
> > + static_cast<V4L2IntControl *>(ctrl);
> > + v4l2_ctrl->value = c->value();
>
> and getInteger(); (although getBoolean() can be separated as well);
>
> As we know the types in this switch, I wonder if there is much
> difference int taste between the two approaches...
>
> > + }
> > + break;
> > + default:
> > + LOG(V4L2Base, Error)
> > + << "Invalid non-compound control type :"
> > + << qext_ctrl.type;
> > + return -EINVAL;
> > + }
> > + }
> > + }
> > +
> > + struct v4l2_ext_controls v4l2_ext_ctrls = {};
> > + v4l2_ext_ctrls.which = V4L2_CTRL_WHICH_CUR_VAL;
> > + v4l2_ext_ctrls.count = count;
> > + v4l2_ext_ctrls.controls = v4l2_ctrls;
> > +
> > + int ret = ioctl(fd_, VIDIOC_S_EXT_CTRLS, &v4l2_ext_ctrls);
> > + if (ret) {
> > + ret = -errno;
> > + LOG(V4L2Base, Error)
> > + << "Unable to set extended controls: " << strerror(-ret);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * \brief Apply the control \a ctrl to the device
> > + * \param ctrl The control to apply to the device
> > + * \return 0 on success, a negative error code otherwise
> > + */
> > +int V4L2Base::setControl(V4L2Control *ctrl)
> > +{
> > + std::vector<V4L2Control *> v = { ctrl, };
> > + return setControls(v);
> > +}
> > +
> > /**
> > * \var V4L2Base::fd_
> > * \brief The V4L2 device or subdevice device node file descriptor
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list