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

[Kieran Bingham]

Hi Jacopo,

On 24/06/2019 09:15, Jacopo Mondi wrote:
> Hi Laurent,
> 
> On Sat, Jun 22, 2019 at 08:09:06PM +0300, Laurent Pinchart wrote:
>> Hi Jacopo,
>>
>> Thank you for the patch.
>>
>> 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.

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?


> 
>>> +
>>> +	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
> 
> Thanks
>    j
> 
>>
>>> + */
>>> +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
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards
--
Kieran