[libcamera-devel] [PATCH v5 1/6] libcamera: Add V4L2Controls

[Laurent Pinchart]

Hi Jacopo,

On Mon, Jun 24, 2019 at 10:51:15AM +0200, Jacopo Mondi wrote:
> On Mon, Jun 24, 2019 at 09:36:25AM +0100, Kieran Bingham wrote:
> > On 24/06/2019 09:15, Jacopo Mondi wrote:
> >> On Sat, Jun 22, 2019 at 08:09:06PM +0300, Laurent Pinchart wrote:
> >>> On Thu, Jun 20, 2019 at 05:31:39PM +0200, Jacopo Mondi wrote:
> >>>> Add Libcamera V4L2 control support, implemented using the V4L2 Extended
> >>>
> >>> s/Libcamera/libcamera/ :-)
> >>>
> >>>> Control APIs. This patch defines the types used to define and manage controls.
> >>>>
> >>>> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> >>>> ---
> >>>>  src/libcamera/include/v4l2_controls.h |  86 ++++++++
> >>>>  src/libcamera/meson.build             |   1 +
> >>>>  src/libcamera/v4l2_controls.cpp       | 282 ++++++++++++++++++++++++++
> >>>>  3 files changed, 369 insertions(+)
> >>>>  create mode 100644 src/libcamera/include/v4l2_controls.h
> >>>>  create mode 100644 src/libcamera/v4l2_controls.cpp
> >>>>
> >>>> diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h
> >>>> new file mode 100644
> >>>> index 000000000000..acd23dabd831
> >>>> --- /dev/null
> >>>> +++ b/src/libcamera/include/v4l2_controls.h
> >>>> @@ -0,0 +1,86 @@
> >>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >>>> +/*
> >>>> + * Copyright (C) 2019, Google Inc.
> >>>> + *
> >>>> + * v4l2_controls.h - V4L2 Controls Support
> >>>> + */
> >>>> +
> >>>> +#ifndef __LIBCAMERA_V4L2_CONTROLS_H__
> >>>> +#define __LIBCAMERA_V4L2_CONTROLS_H__
> >>>> +
> >>>> +#include <string>
> >>>> +#include <vector>
> >>>> +
> >>>> +#include <stdint.h>
> >>>> +
> >>>> +#include <linux/v4l2-controls.h>
> >>>> +#include <linux/videodev2.h>
> >>>> +
> >>>> +namespace libcamera {
> >>>> +
> >>>> +class V4L2ControlInfo
> >>>> +{
> >>>> +public:
> >>>> +	V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl);
> >>>> +
> >>>> +	unsigned int id() const { return id_; }
> >>>> +	unsigned int type() const { return type_; }
> >>>> +	size_t size() const { return size_; }
> >>>> +	const std::string &name() const { return name_; }
> >>>> +
> >>>> +private:
> >>>> +	unsigned int type_;
> >>>> +	std::string name_;
> >>>> +	unsigned int id_;
> >>>> +	size_t size_;
> >>>
> >>> As requested by Niklas,
> >>>
> >>> 	unsigned int id_;
> >>> 	unsigned int type_;
> >>> 	size_t size_;
> >>> 	std::string name_;
> >>>
> >>> ?
> >>>
> >>>> +};
> >>>> +
> >>>> +class V4L2Control
> >>>> +{
> >>>> +public:
> >>>> +	V4L2Control(unsigned int id, int value = 0)
> >>>> +		: id_(id), value_(value) {}
> >>>> +
> >>>> +	int64_t value() const { return value_; }
> >>>> +	void setValue(int64_t value) { value_ = value; }
> >>>> +
> >>>> +	unsigned int id() const { return id_; }
> >>>> +
> >>>> +private:
> >>>> +	unsigned int id_;
> >>>> +	int64_t value_;
> >>>> +};
> >>>> +
> >>>> +class V4L2Controls
> >>>
> >>> I was wondering if we should rename this to V4L2ControlList, as
> >>> V4L2Controls is very close to V4L2Control and easy to mistake.
> >>>
> >>>> +{
> >>>> +public:
> >>>> +	V4L2Controls &operator=(const V4L2Controls &) = delete;
> >>>
> >>> As commented before, I think you should either delete both the
> >>> assignement operator and the copy constructor, or neither of them.
> >>>
> >>>> +
> >>>> +	using iterator = std::vector<V4L2Control>::iterator;
> >>>> +	using const_iterator = std::vector<V4L2Control>::const_iterator;
> >>>> +
> >>>> +	iterator begin() { return controls_.begin(); }
> >>>> +	const_iterator begin() const { return controls_.begin(); }
> >>>> +	iterator end() { return controls_.end(); }
> >>>> +	const_iterator end() const { return controls_.end(); }
> >>>> +
> >>>> +	bool empty() const { return controls_.empty(); }
> >>>> +	size_t size() const { return controls_.size(); }
> >>>
> >>> std::size_t to match std::vector ?
> >>>
> >>>> +
> >>>> +	void reset() { controls_.clear(); }
> >>>> +	void add(unsigned int id, int64_t value = 0);
> >>>> +
> >>>> +	V4L2Control *operator[](unsigned int index)
> >>>> +	{
> >>>> +		return &controls_[index];
> >>>> +	}
> >>>
> >>> Do we need to access a control by index for any other purpose than
> >>> iterating over the controls, which the iterator already gives us ?
> >>> Couldn't operator[] take the control ID, and getControl() be removed ?
> >>>
> >>
> >> I really don't like this.
> >> operator[] for vector works on indexes, I would now use the same
> >> method but with a different semantic. It would be confusing for
> >> everyone. I can drop this operation if you like to.
> >
> > A user of a V4L2ControlList shouldn't know about the internal storage
> > mechanism, and indeed it could be changed without affecting the users.
> 
> Sorry but I don't agree. This class "quacks" like a vector, and mimics
> its interface. We could extend it of course, but diverging seems a
> very bad idea. Furthermore, 99% of usages of the [] operator I can
> think of work on indexes, I don't see a reason to change this and ask
> people to go and look at the documentation to know how operator[]
> works (for no clear gain, in my opinion)

The class provides iterators, and a few methods such as empty(), size()
and reset() (which I think should be called clear() if you want to
mimick the STL API), which are all provided by both std::vector and
std::map. Furthermore operator[] is defined in different ways for
different types of containers, the fact that you use more vectors than
map doesn't mean that the operator mostly works on indices :-)

I'm not saying it has to be turned into a key-based lookup, but if we
don't need index-based lookups, I would remove it.

> > That does add one concern about using a vector - Can two controls be
> > created with the same ID?
> >
> > What happens then?
> > Is that a valid use case?
> 
> That's a real concern instead. Nothing in the V4L2 extended control
> APIs prescribes to have unique entries, so if that happens here, this
> won't be an issue at the V4L2 control level, but it might be confusing
> for application and lead to some subtile bug when the same
> V4L2ControlList is manipulated by different methods.
> 
> So, it's not a bug, but it might be desirable to make sure that
> "add()" only adds unique entries, but at the same time this is not an
> API for applications, and pipeline handlers should know better. I'm
> debated, slightly leaning to leave it the way it is now.

If we allow add() to create multiple entries with the same ID then
getControl() won't work anymore. I don't see a use case for this, so I
would make the ID unique.

> >>>> +
> >>>> +	V4L2Control *getControl(unsigned int id);
> >>>> +
> >>>> +private:
> >>>> +	std::vector<V4L2Control> controls_;
> >>>> +};
> >>>> +
> >>>> +} /* namespace libcamera */
> >>>> +
> >>>> +#endif /* __LIBCAMERA_V4L2_CONTROLS_H__ */
> >>>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> >>>> index f26ad5b2dc57..985aa7e8ab0e 100644
> >>>> --- a/src/libcamera/meson.build
> >>>> +++ b/src/libcamera/meson.build
> >>>> @@ -23,6 +23,7 @@ libcamera_sources = files([
> >>>>      'stream.cpp',
> >>>>      'timer.cpp',
> >>>>      'utils.cpp',
> >>>> +    'v4l2_controls.cpp',
> >>>>      'v4l2_device.cpp',
> >>>>      'v4l2_subdevice.cpp',
> >>>>      'v4l2_videodevice.cpp',
> >>>> diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> >>>> new file mode 100644
> >>>> index 000000000000..abf596e7f1ef
> >>>> --- /dev/null
> >>>> +++ b/src/libcamera/v4l2_controls.cpp
> >>>> @@ -0,0 +1,282 @@
> >>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >>>> +/*
> >>>> + * Copyright (C) 2019, Google Inc.
> >>>> + *
> >>>> + * v4l2_controls.cpp - V4L2 Controls Support
> >>>> + */
> >>>> +
> >>>> +#include "v4l2_controls.h"
> >>>> +
> >>>> +/**
> >>>> + * \file v4l2_controls.h
> >>>> + * \brief Support for V4L2 Controls using the V4L2 Extended Controls APIs.
> >>>
> >>> s/\.$//
> >>>
> >>>> + *
> >>>> + * The V4L2 defined "Control API" allows application to inspect and modify set
> >>>
> >>> "The V4L2 Control API allows applications ..."
> >>>
> >>> s/set/sets/
> >>>
> >>>> + * of configurable parameters on the video device or subdevice of interest. The
> >>>
> >>> s/on the video device or subdevice of interest/on a video device or subdevice/
> >>>
> >>>> + * nature of the parameters an application could modify using the control
> >>>
> >>> s/could/can/
> >>>
> >>>> + * framework depends on what the driver implements support for, and on the
> >>>> + * characteristics of the underlying hardware platform. Generally controls are
> >>>> + * used to modify user visible settings, such as the image brightness and
> >>>> + * exposure time, or non-standard parameters which cannot be controlled through
> >>>> + * the V4L2 format negotiation API.
> >>>
> >>> The last two sentences describe more the V4L2 API than the V4L2Control*
> >>> classes API. I expect users of this code to know about V4L2 already, so
> >>> they could be dropped, but I also don't mind if you keep them. Just
> >>> don't invest too much time in writing document about V4L2 itself, as
> >>> that's a bit out of scope.
> >>>
> >>
> >> I feel it doesn't hurt, as it doesn't go in great lenght describing
> >> controls but I'll drop.
> >>
> >>>> + *
> >>>> + * Controls are identified by a numerical ID, defined by the V4L2 kernel headers
> >>>> + * and have an associated type. Each control has a value, which is the data
> >>>> + * that can be modified with V4L2Device::setControls() or retrieved with
> >>>> + * V4L2Device::getControls().
> >>>> + *
> >>>> + * The control's type along with the control's flags defines the type of the
> >>>
> >>> s/defines/define/
> >>>
> >>>> + * control's value content. Controls might transport a single data value
> >>>
> >>> s/might/can/
> >>>
> >>>> + * stored in variable inside the control, or they might as well deal with more
> >>>
> >>> s/might/can/
> >>>
> >>>> + * complex data types, such as arrays of matrices, stored in a contiguous
> >>>> + * memory locations associated with the control and called 'the payload'. Such
> >>>> + * controls are called 'compound controls' and are currently not supported by
> >>>> + * the libcamera V4L2 control framework.
> >>>
> >>> Here too you could simply have said that we don't support compound
> >>> controls yet, but I don't mind keeping the more detailed text as-is.
> >>>
> >>>> + *
> >>>> + * libcamera implements support for controls using the V4L2 'Extended Control'
> >>>> + * API, which allows future handling of controls with payloads of arbitrary
> >>>
> >>> It's not the controls that are extended, it's the API. It's the extended
> >>> V4L2 control API, not the V4L2 API for extended controls. I would thus
> >>> remove the quotes.
> >>>
> >>
> >> I see. I should have moved the ending quote to include the "API" part.
> >>
> >>>> + * sizes.
> >>>> + *
> >>>> + * The libcamera V4L2 Controls framework operates on lists of controls,
> >>>> + * wrapped by the V4L2Controls class, to match the V4L2 extended controls API.
> >>>
> >>> Calling it V4L2ControlList would match the description nicely :-)
> >>>
> >>
> >> Ack
> >>
> >>>> + * The interface to set and get control is implemented by the V4L2Device
> >>>> + * class, and this file only provides the data type definitions.
> >>>> + *
> >>>> + * \todo Add support for compound controls
> >>>> + */
> >>>> +
> >>>> +namespace libcamera {
> >>>> +
> >>>> +/**
> >>>> + * \class V4L2ControlInfo
> >>>> + * \brief Information on a V4L2 control
> >>>> + *
> >>>> + * The V4L2ControlInfo class represent all the information related to
> >>>
> >>> s/represent/represents/
> >>>
> >>>> + * a V4L2 control, such as its ID, its type, its user-readable name and the
> >>>> + * expected size of its value data.
> >>>> + *
> >>>> + * V4L2ControlInfo instances are created by inspecting the fieldS of a struct
> >>>> + * v4l2_query_ext_ctrl structure, after it has been filled by the device
> >>>> + * driver as a consequence of a VIDIOC_QUERY_EXT_CTRL ioctl call.
> >>>> + *
> >>>> + * This class does not contain the control value, but only static information
> >>>> + * on the control, which shall be cached by the caller at initialisation time
> >>>> + * or the first time the control information is accessed.
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \brief Construct a V4L2ControlInfo from a struct v4l2_query_ext_ctrl
> >>>> + * \param ctrl The struct v4l2_query_ext_ctrl as returned by the kernel
> >>>> + */
> >>>> +V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
> >>>> +{
> >>>> +	id_ = ctrl.id;
> >>>> +	type_ = ctrl.type;
> >>>> +	name_ = static_cast<const char *>(ctrl.name);
> >>>> +	size_ = ctrl.elem_size * ctrl.elems;
> >>>> +}
> >>>> +
> >>>> +/**
> >>>> + * \fn V4L2ControlInfo::id()
> >>>> + * \brief Retrieve the control ID
> >>>> + * \return The V4L2 control ID
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \fn V4L2ControlInfo::type()
> >>>> + * \brief Retrieve the control type as defined by V4L2_CTRL_TYPE_*
> >>>> + * \return The V4L2 control type
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \fn V4L2ControlInfo::size()
> >>>> + * \brief Retrieve the control value data size (in bytes)
> >>>> + * \return The V4L2 control value data size
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \fn V4L2ControlInfo::name()
> >>>> + * \brief Retrieve the control user readable name
> >>>> + * \return The V4L2 control user readable name
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \class V4L2Control
> >>>> + * \brief A V4L2 control value
> >>>> + *
> >>>> + * The V4L2Control class represent the value of a V4L2 control.
> >>>> + * The class stores values that have been read from or will be applied to
> >>>> + * a V4L2 device.
> >>>> + *
> >>>> + * The value stored in the class instances does not reflect what is actually
> >>>> + * applied to the hardware but is a pure software cache optionally initialized
> >>>> + * at control creation time and only modified by a control read operation.
> >>>
> >>> With setControls() updating the value, this isn't true anymore.
> >>>
> >>
> >> True. Nice catch.
> >>
> >>>> + *
> >>>> + * The write and read controls the V4L2Control class instances are not meant
> >>>> + * to be directly used but are instead intended to be grouped in
> >>>> + * V4L2Controls instances, which are then passed as parameters to
> >>>> + * V4L2Device::setControls() and V4L2Device::getControls() operations.
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \fn V4L2Control::V4L2Control
> >>>> + * \brief Construct a V4L2 control with ID \a id and value \a value
> >>>
> >>> "with an \a id and a \a value" ? Or "Construct an instance of control \a
> >>> ID with a \a value" ?
> >>>
> >>
> >> I'm always uncertain on how to handle these situation, where the name
> >> of the parameter matches the generic name.
> >>
> >>>> + * \param id The V4L2 control ID
> >>>> + * \param value The control value
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \fn V4L2Control::value()
> >>>> + * \brief Retrieve the value of the control
> >>>> + *
> >>>> + * It is important to notice that the returned value is not read from the
> >>>
> >>> s/notice/note/
> >>>
> >>>> + * hardware at the time this operation is called, but it's the cached value
> >>>> + * obtained the most recent time the control has been read with
> >>>> + * V4L2Device::getControls() or the value of the control has been set to by the
> >>>> + * user with the add() operation.
> >>>
> >>> You should mention setControls() here. How about
> >>>
> >>> This method returns the cached control value, initially set by
> >>> V4L2Controls::add() and then updated when the controls are read or
> >>> written with V4L2Device::getControls() and V4L2Device::setControls().
> >>>
> >>>> + *
> >>>> + * \return The V4L2 control value. The value might be 0 if the control has
> >>>> + * never been read from the device before this operation is called.
> >>>
> >>> I would drop the second sentence, it's redundant with the above
> >>> paragraph, and a bit confusing too as the value may also be 0 after
> >>> being read.
> >>>
> >>
> >> Ack.
> >>
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \fn V4L2Control::setValue()
> >>>> + * \brief Set the value of the control
> >>>> + * \param value The new V4L2 control value
> >>>> + *
> >>>> + * It is important to notice that the value is not applied to the
> >>>> + * hardware at the time this operation is called, but it will only
> >>>> + * be actually applied by calling V4L2Device::setControls().
> >>>
> >>> Let's simplify it a bit, by explaining what the method does, not what it
> >>> does not do :-)
> >>>
> >>> "This method stores the control value, which will be applied to the
> >>> device when calling V4L2Device::setControls()."
> >>>
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \fn V4L2Control::id()
> >>>> + * \brief Retrieve the control ID this instance refers to
> >>>> + * \return The V4L2Control ID
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \class V4L2Controls
> >>>> + * \brief Container of V4L2Control instances
> >>>> + *
> >>>> + * The V4L2Controls class works as a container for a list of V4L2Control
> >>>> + * instances. The class provides operations to add a new control to the list,
> >>>> + * get back a control value, and reset the list of controls it contains.
> >>>> + *
> >>>> + * In order to set and get controls, user of the libcamera V4L2 control
> >>>> + * framework should operate on instances of the V4L2Controls class, and use
> >>>> + * them as argument for the V4L2Device::setControls() and
> >>>> + * V4L2Device::getControls() operations, which write and read a list of
> >>>> + * controls from or to a V4L2 device (a video device or a subdevice).
> >>>
> >>> "to and from" (as it's "write and read")
> >>>
> >>>> + *
> >>>> + * Controls are added to a V4L2Controls instance with the add() method, with
> >>>> + * or without an initial value.
> >>>> + *
> >>>> + * To write controls to a device, the controls of interest shall be added with
> >>>> + * an initial value by calling V4L2Controls::add(unsigned int id, int64_t
> >>>
> >>> s/an initial value/a value/
> >>>
> >>>> + * value) to prepare for a write operation. Once the values of all controls of
> >>>> + * interest have been added, the V4L2Controls instance is passed to the
> >>>> + * V4L2Device::setControls(), which sets the controls on the device.
> >>>> + *
> >>>> + * To read controls from a device, the desired controls are added with
> >>>> + * V4L2Controls::add(unsigned int id) to prepare for a read operation. The
> >>>> + * V4L2Controls instance is then passed to V4L2Device::getControls(), which
> >>>> + * reads the controls from the device and updates the values stored in
> >>>> + * V4L2Controls.
> >>>> + *
> >>>> + * V4L2Controls instances can be reset to remove all controls they contain and
> >>>> + * prepare to be re-used for a new control write/read sequence.
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \typedef V4L2Controls::iterator
> >>>> + * \brief Iterator on the V4L2 controls contained in the instance
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \typedef V4L2Controls::const_iterator
> >>>> + * \brief Const iterator on the V4L2 controls contained in the instance
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \fn iterator V4L2Controls::begin()
> >>>> + * \brief Retrieve an iterator to the first V4L2Control in the instance
> >>>> + * \return An iterator to the first V4L2 control
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \fn const_iterator V4L2Controls::begin() const
> >>>> + * \brief Retrieve a constant iterator to the first V4L2Control in the instance
> >>>> + * \return A constant iterator to the first V4L2 control
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \fn iterator V4L2Controls::end()
> >>>> + * \brief Retrieve an iterator pointing to the past-the-end V4L2Control in the
> >>>> + * instance
> >>>> + * \return An iterator to the element following the last V4L2 control in the
> >>>> + * instance
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \fn const_iterator V4L2Controls::end() const
> >>>> + * \brief Retrieve a constant iterator pointing to the past-the-end V4L2Control
> >>>> + * in the instance
> >>>> + * \return A constant iterator to the element following the last V4L2 control
> >>>> + * in the instance
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \fn V4L2Controls::empty()
> >>>> + * \brief Verify if the instance does not contain any control
> >>>> + * \return True if the instance does not contain any control, false otherwise
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \fn V4L2Controls::size()
> >>>> + * \brief Retrieve the number on controls in the instance
> >>>> + * \return The number of V4L2Control stored in the instance
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \fn V4L2Controls::reset()
> >>>> + * \brief Removes all controls in the instance
> >>>
> >>> s/Removes/Remove/
> >>>
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \brief Add control with ID \a id and optional \a value to the instance
> >>>> + * \param id The V4L2 control ID (V4L2_CID_*)
> >>>> + * \param value The V4L2 control value
> >>>> + *
> >>>> + * V4L2Control can be added with an explicit value, or without one. In that
> >>>> + * case the control's value is defaulted to 0.
> >>>
> >>> s/is defaulted/defaults to/
> >>>
> >>> But I think we can skip the sentence, the doxygen documentation should
> >>> show the = 0.
> >>
> >> Ack
> >>
> >>>> + */
> >>>> +void V4L2Controls::add(unsigned int id, int64_t value)
> >>>> +{
> >>>> +	controls_.emplace_back(id, value);
> >>>> +}
> >>>> +
> >>>> +/**
> >>>> + * \fn V4L2Controls::operator[](unsigned int index)
> >>>> + * \brief Access the control at index \a index
> >>>> + * \param index The index to access
> >>>> + * \return The V4L2 control at index \a index
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \brief Get a pointer the control with ID \a id
> >>>> + * \param id The V4L2 control ID (V4L2_CID_xx)
> >>>> + * \return A pointer to the V4L2Control with ID \a id or nullptr if the
> >>>> + * control ID is not part of this instance.
> >>>> + */
> >>>> +V4L2Control *V4L2Controls::getControl(unsigned int id)
> >>>> +{
> >>>> +	for (V4L2Control &ctrl : controls_) {
> >>>> +		if (ctrl.id() == id)
> >>>> +			return &ctrl;
> >>>> +	}
> >>>> +
> >>>> +	return nullptr;
> >>>> +}
> >>>> +
> >>>> +} /* namespace libcamera */

-- 
Regards,

Laurent Pinchart