[libcamera-devel,v4,1/6] libcamera: Add V4L2Controls

1474 diff mbox series

Add Libcamera V4L2 control support, implemented using the V4L2 Extended
Control APIs. This patch defines the types used to define and manage controls.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/include/v4l2_controls.h |  96 ++++++++
 src/libcamera/meson.build             |   1 +
 src/libcamera/v4l2_controls.cpp       | 337 ++++++++++++++++++++++++++
 3 files changed, 434 insertions(+)
 create mode 100644 src/libcamera/include/v4l2_controls.h
 create mode 100644 src/libcamera/v4l2_controls.cpp

Hi Jacopo,

Thank you for the patch.

On Wed, Jun 19, 2019 at 01:08:53PM +0200, Jacopo Mondi wrote:
> Add Libcamera V4L2 control support, implemented using the V4L2 Extended
> Control APIs. This patch defines the types used to define and manage controls.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/include/v4l2_controls.h |  96 ++++++++
>  src/libcamera/meson.build             |   1 +
>  src/libcamera/v4l2_controls.cpp       | 337 ++++++++++++++++++++++++++
>  3 files changed, 434 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..d7b12801329e
> --- /dev/null
> +++ b/src/libcamera/include/v4l2_controls.h
> @@ -0,0 +1,96 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * v4l2_controls.h - V4L2 Extended Control Support

Maybe just V4L2 Control Support ?

> + */
> +
> +#ifndef __LIBCAMERA_V4L2_CONTROLS_H__
> +#define __LIBCAMERA_V4L2_CONTROLS_H__
> +
> +#include <cstring>
> +#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_; }

The name is only used in patch 6/6 to print messages. Do you think we
should keep it, or can we drop it to save memory ? It's an open
question.

> +
> +private:
> +	unsigned int type_;
> +	std::string name_;
> +	unsigned int id_;
> +	size_t size_;
> +};
> +
> +class V4L2Control
> +{
> +public:
> +	unsigned int id() const { return id_; }
> +
> +private:
> +	friend class V4L2Device;
> +	friend class V4L2Controls;
> +
> +	V4L2Control(unsigned int id)
> +		: id_(id) {}
> +	V4L2Control(unsigned int id, int value)
> +		: id_(id), value_(value) {}
> +
> +	long int value() const { return value_; }
> +	void setValue(long int value) { value_ = value; }

long int and int are equivalent. I would use types with an explicit size
(uint_* or int_*). I would also use the same type for the value in the
constructor.

> +
> +	unsigned int id_;
> +	long int value_;
> +};
> +
> +class V4L2Controls
> +{
> +public:
> +	V4L2Controls &operator=(const V4L2Controls &) = delete;

Should you then also delete the copy constructor ?

> +
> +	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() { return controls_.size(); }

This method can be const too.

> +	void reset() { controls_.clear(); }
> +
> +	V4L2Control *operator[](unsigned int index)
> +	{
> +		return &controls_[index];
> +	}
> +
> +	void add(unsigned int id, long int value);
> +	void add(unsigned int id);

You could merge these two functions into one with

	void add(unsigned int id, long int value = 0);

That way all control values will be initialised to a value, there will
be no uninitialised memory.

> +
> +	int get(unsigned int id);
> +	int set(unsigned int id, long int value);

Integer types with explicit lengths should be used here too.

What is the purpose of the set() function ? Is it used by V4L2Device
only, to update the value after a get or set ? If so I would make the
controls_ vector available to V4L2Device, which could iterate over it
without having to check for IDs, as the internal controls array that is
used for the ioctls will be in the same order as the vector. That would
optimise the getControls() and setControls() operations.

I also had a look at the usage pattern of the get() function in patch
6/6, and it looks a bit inefficient. The caller loops over controls
using the public iterators, to then extract the control ID using the
V4L2Control::id() method, and then calls V4L2Controls::get() with the
ID, which searches the controls_ vector again. We start with an item in
the vector, and then look it up.

One option would be to drop the get() method here, and make the value()
method public on V4L2Control. Both get() and set() would then be
removed, with only add() staying.

> +
> +private:
> +	V4L2Control *getControl(unsigned int id);
> +
> +	std::vector<V4L2Control> controls_;

This makes control lookup by ID a bit inefficient (and requires the
getControl() method). Should we have a std::map<unsigned int,
V4L2Control *> that stores pointers to the controls in the controls_
vector ? getControl() could then be dropped. It would cost a bit more
memory and a bit more CPU time in the add() operation, but would save
time at set() and get() time. If set() gets dropped as proposed above,
only get() will remain, and I wonder if it will be worth it. For small
sets of controls possibly not, but for large sets it could make a
difference. What do you think ?

> +};
> +
> +} /* 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..75b9c29ca133
> --- /dev/null
> +++ b/src/libcamera/v4l2_controls.cpp
> @@ -0,0 +1,337 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * v4l2_controls.cpp - V4L2 Extended Control Support
> + */
> +
> +#include "v4l2_controls.h"
> +
> +/**
> + * \file v4l2_controls.h
> + * \brief Support for V4L2 Controls using the V4L2 Extended Controls APIs.
> + *
> + * The V4L2 defined "Control API" allows application to inspect and modify set
> + * of configurable parameters on the video device or subdevice of interest. The
> + * nature of the parameters an application could modify using the control
> + * 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.
> + *
> + * Controls are identified by a numerical id, defined by the V4L2 kernel headers
> + * and have an associated type and class. Each control has a 'value', which is

I would drop ' and class', we don't care about control classes.

> + * the data that can be modified with a call to  the 'V4L2Device::setControls()'

s/  / /

> + * operation or retrieved with a call to 'V4L2Device::getControls()'.

I don't think you need to enclose the method name in quotes, you can
write

 * Each control has a value, which is the data that can be modified with
 * V4L2Device::setControls() or retrieved with V4L2Device::getControls().

> + *
> + * A control class defines the control purpose while its type (along with the

Similarly, you can drop the class here.

> + * control's flags) defines the type of the control's value content. Controls
> + * might transport a single data value stored in variable inside the control, or
> + * they might as well deal with more 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.

s/Libcamera/libcamera/

> + *
> + * \todo Add support for compound controls
> + *
> + * Libcamera implements support for control using the V4L2 'Extended Control'

s/Libcamera/libcamera/
s/control/controls/

> + * framework, which allows easier future handling of controls with payloads

s/framework/API/
s/easier //

> + * of arbitrary sizes.
> + *
> + * The Libcamera V4L2 Controls framework operates on lists of controls, wrapped

s/Libcamera/libcamera/

> + * by the V4L2Controls class, to match the V4L2 extended controls framework.

s/framework/API/

> + * The interface to set and get control is implemented by the V4L2Device class,
> + * and this file only provides the data type definitions.
> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \class V4L2ControlInfo
> + * \brief Information on a V4L2 control
> + *
> + * The V4L2ControlInfo class represent all the informations relative to

s/informations/information/ (uncoutable)
s/relative/related/

> + * 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 field of
> + * a struct v4l2_query_ext_ctrl structure, after it has been filled by the
> + * device driver as a consequence of an VIDIOC_QUERY_EXT_CTRL ioctl call.

s/of an/of a/

> + *
> + * This class does not contain the control value, but only static informations

s/informations/information/

> + * on the control, which should ideally be cached the first time the control

I'd be a bit more directive here, "which shall be cached by the caller
at initialisation time or the first time the control information is
needed." and drop the rest of the sentence.

> + * is accessed to be reused each time the control value need to be read or

s/need/needs/

> + * applied to the hardware.
> + */
> +
> +/**
> + * \brief Construct a V4L2ControlInfo from a struct v4l2_query_ext_ctrl
> + */
> +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 this instance refers to

I would just say "Retrieve the control ID"

> + * \return The V4L2 control id

s/id/ID/ (as well as in other locations below)

> + */
> +
> +/**
> + * \fn V4L2ControlInfo::type()
> + * \brief Retrieve the control type as defined by V4L2_CTRL_TYPE_

Maybe 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 or a
> + * call to V4L2Controls::set().
> + *
> + * 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 the set and get control operations implemented in
> + * V4L2Device::setControls() and V4L2Device::setControls() respectively.

The second one should be V4L2Device::getControls(), or you could write

"passed as parameter to V4L2Device::getControls() and
V4L2Device::setControls()."

> + *
> + * In facts, access to the class constructor and to the control value accessor
> + * and modifier are restricted to the friend V4L2Controls and V4L2Device
> + * classes.

I think you can drop this paragraph.

> + */
> +
> +/**
> + * \fn V4L2Control::id()
> + * \brief Retrieve the control id this instance refers to
> + * \return The V4L2Control id
> + */
> +
> +/**
> + * \class V4L2Controls
> + * \brief Wraps a list of V4L2Control

I would say "Container of V4L2Control instance" and replace wrapper by
container below.

> + *
> + * The V4L2Controls class works as a wrapper 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

s/Libcamera/libcamera/

> + * 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).
> + *
> + * 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 should be added

s/should/shall/

> + * with an initial value by calling V4L2Controls::add(unsigned int id, long
> + * int value) to prepare for a write operation. The value of controls which
> + * are already part of the instance could be updated with a call to
> + * V4L2Controls::set() operation.

Is there a use case for this ?

> Once the values of all controls of interest
> + * have been initialized in a V4L2Controls instance, this should be then
> + * passed to the V4L2Device::setControls() operation, which applies each
> + * control in the list to the hardware.

Once all desired control have been added, the V4L2Controls instance is
passed to V4L2Device::setControls(), which sets the controls on the
device.

> + *
> + * Alternatively a control can be add without any initial value by calling
> + * V4L2Controls::add(unsigned int id) and then read from the device passing
> + * the V4L2Controls instance to V4L2Device::getControls() which read each
> + * control value from the hardware.
> + *
> + * Reading controls with an initialized value from the device, overwrites the
> + * control's value, reflecting what has been actually read from the hardware.

Let's write this like the previous paragraph.

"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 it contains and

s/it contains/they contain/

> + * prepare to be re-used for a new control write/read sequence. Alternatively,
> + * the value of a control already part of the instance could be updated by using
> + * the V4L2Controls::set() method, to avoid going through a reset() when a
> + * control has to be read then written to the hardware in sequence.

Given how set() is implemented, creating a new V4L2Controls instance
could be cheaper. Do we really want to reuse V4L2Controls instances with
set() ? If so I think we need to think about the use cases, and decide
how to implement it in an efficient way. Alternatively, I would start
simple and extend the API later.

> + *
> + * In facts, the following pseudo-code sequences lead to the same result:

s/In facts, the/The/

> + *
> + * V4L2Controls controls;
> + * controls.add(V4L2_CID_xx);
> + * dev->getControls(&controls);
> + * controls.set(V4L2_CID_xx, value);
> + * dev->setControls(&controls);
> + *
> + * V4L2Controls controls;
> + * controls.add(V4L2_CID_xx);
> + * dev->getControls(&controls);
> + * controls.reset();
> + * controls.add(V4L2_CID_xx, value);
> + * dev->setControls(&controls);
> + */
> +
> +/**
> + * \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
> + */
> +
> +/**
> + * \fn V4L2Controls::operator[](unsigned int index)
> + * \brief Access the control at index \a index
> + * \param index The index to access

\return ?

> + */
> +
> +/**
> + * \brief Add control \a id with a \a value to the instance
> + * \param id The V4L2 control id (V4L2_CID_xx)

s/control id/control ID/
s/V4L2_CID_xx/V4L2_CID_*/

> + * \param value The V4L2 control value
> + */
> +void V4L2Controls::add(unsigned int id, long int value)
> +{
> +	/* Cannot use emplace_back() as the V4L2Control ctor is private. */
> +	V4L2Control ctrl(id, value);
> +	controls_.push_back(ctrl);
> +}
> +
> +/**
> + * \brief Add control \a id without an initial value  to the instance

s/  / /

> + * \param id The V4L2 control id (V4L2_CID_xx)

s/control id/control ID/
s/V4L2_CID_xx/V4L2_CID_*/

Same in a few locations below.

> + */
> +void V4L2Controls::add(unsigned int id)
> +{
> +	/* Cannot use emplace_back() as the V4L2Control ctor is private. */

Would it make sense to make the constructor public then ? You have
documented extensively that V4L2Control isn't supposed to be
instantiated directly by the application, and there will be no way to
use a directly instantiated V4L2Control anway, so things should be fine.

> +	V4L2Control ctrl(id);
> +	controls_.push_back(ctrl);
> +}
> +
> +/**
> + * \brief Retrieve the value of the control with \a id
> + * \param id The V4L2 control id (V4L2_CID_xx)
> + *
> + * Retrieve the value of the control with id \a id.
> + *
> + * It is important to notice that the returned value is not read from the
> + * 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() or set() operation.
> + *
> + * \return The V4L2 control value or a negative error code if the control id
> + * is not part of this instance
> + */
> +int V4L2Controls::get(unsigned int id)
> +{
> +	V4L2Control *ctrl = getControl(id);
> +	if (!ctrl)
> +		return -EINVAL;
> +
> +	return ctrl->value();
> +}
> +
> +/**
> + * \brief Set the value of the control with \a id
> + * \param id The V4L2 control id (V4L2_CID_xx)
> + * \param value The new V4L2 control value
> + *
> + * Set the value of the control with id \a id.
> + *
> + * 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 pplied only by calling V4L2Device::setControls().
> + *
> + * \return 0 on success or a negative error code if the control id
> + * is not part of this instance
> + */
> +int V4L2Controls::set(unsigned int id, long int value)
> +{
> +	V4L2Control *ctrl = getControl(id);
> +	if (!ctrl)
> +		return -EINVAL;
> +
> +	ctrl->setValue(value);
> +
> +	return 0;
> +}
> +
> +/*
> + * \brief Get a pointer the control with \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 */

Hi Jacopo,

Thanks for your work.

On 2019-06-19 13:08:53 +0200, Jacopo Mondi wrote:
> Add Libcamera V4L2 control support, implemented using the V4L2 Extended
> Control APIs. This patch defines the types used to define and manage controls.

Over all I agree with all Laurents comments on the code and structure, 
specially that V4L2Controls::controls_ should be a std::map. If you 
really feel splitting the id and value apart you could try for a 
std::set, but that would be a tad more code to implement as you wound 
need to implement the logic to index on the control id of the elements.

Some small nits, bellow.

> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/include/v4l2_controls.h |  96 ++++++++
>  src/libcamera/meson.build             |   1 +
>  src/libcamera/v4l2_controls.cpp       | 337 ++++++++++++++++++++++++++
>  3 files changed, 434 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..d7b12801329e
> --- /dev/null
> +++ b/src/libcamera/include/v4l2_controls.h
> @@ -0,0 +1,96 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * v4l2_controls.h - V4L2 Extended Control Support
> + */
> +
> +#ifndef __LIBCAMERA_V4L2_CONTROLS_H__
> +#define __LIBCAMERA_V4L2_CONTROLS_H__
> +
> +#include <cstring>
> +#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_; }

nit, it bugs me that the order of get methods here differs from the 
order of variables bellow ;-)

> +
> +private:
> +	unsigned int type_;
> +	std::string name_;
> +	unsigned int id_;
> +	size_t size_;
> +};
> +
> +class V4L2Control
> +{
> +public:
> +	unsigned int id() const { return id_; }
> +
> +private:
> +	friend class V4L2Device;
> +	friend class V4L2Controls;
> +
> +	V4L2Control(unsigned int id)
> +		: id_(id) {}
> +	V4L2Control(unsigned int id, int value)
> +		: id_(id), value_(value) {}
> +
> +	long int value() const { return value_; }
> +	void setValue(long int value) { value_ = value; }
> +
> +	unsigned int id_;
> +	long int value_;
> +};
> +
> +class V4L2Controls
> +{
> +public:
> +	V4L2Controls &operator=(const V4L2Controls &) = delete;
> +
> +	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(); }

I like empty() better then isEmpty(), but isEmpty() is what we use in 
libcamera ;-)

> +	size_t size() { return controls_.size(); }
> +	void reset() { controls_.clear(); }
> +
> +	V4L2Control *operator[](unsigned int index)
> +	{
> +		return &controls_[index];
> +	}
> +
> +	void add(unsigned int id, long int value);
> +	void add(unsigned int id);
> +
> +	int get(unsigned int id);
> +	int set(unsigned int id, long int value);
> +
> +private:
> +	V4L2Control *getControl(unsigned int id);
> +
> +	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..75b9c29ca133
> --- /dev/null
> +++ b/src/libcamera/v4l2_controls.cpp
> @@ -0,0 +1,337 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * v4l2_controls.cpp - V4L2 Extended Control Support
> + */
> +
> +#include "v4l2_controls.h"
> +
> +/**
> + * \file v4l2_controls.h
> + * \brief Support for V4L2 Controls using the V4L2 Extended Controls APIs.
> + *
> + * The V4L2 defined "Control API" allows application to inspect and modify set
> + * of configurable parameters on the video device or subdevice of interest. The
> + * nature of the parameters an application could modify using the control
> + * 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.
> + *
> + * Controls are identified by a numerical id, defined by the V4L2 kernel headers
> + * and have an associated type and class. Each control has a 'value', which is
> + * the data that can be modified with a call to  the 'V4L2Device::setControls()'
> + * operation or retrieved with a call to 'V4L2Device::getControls()'.
> + *
> + * A control class defines the control purpose while its type (along with the
> + * control's flags) defines the type of the control's value content. Controls
> + * might transport a single data value stored in variable inside the control, or
> + * they might as well deal with more 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.
> + *
> + * \todo Add support for compound controls
> + *
> + * Libcamera implements support for control using the V4L2 'Extended Control'
> + * framework, which allows easier future handling of controls with payloads
> + * of arbitrary sizes.
> + *
> + * The Libcamera V4L2 Controls framework operates on lists of controls, wrapped
> + * by the V4L2Controls class, to match the V4L2 extended controls framework.
> + * The interface to set and get control is implemented by the V4L2Device class,
> + * and this file only provides the data type definitions.
> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \class V4L2ControlInfo
> + * \brief Information on a V4L2 control
> + *
> + * The V4L2ControlInfo class represent all the informations relative to
> + * 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 field of
> + * a struct v4l2_query_ext_ctrl structure, after it has been filled by the
> + * device driver as a consequence of an VIDIOC_QUERY_EXT_CTRL ioctl call.
> + *
> + * This class does not contain the control value, but only static informations
> + * on the control, which should ideally be cached the first time the control
> + * is accessed to be reused each time the control value need to be read or
> + * applied to the hardware.
> + */
> +
> +/**
> + * \brief Construct a V4L2ControlInfo from a struct v4l2_query_ext_ctrl
> + */
> +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 this instance refers to
> + * \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 or a
> + * call to V4L2Controls::set().
> + *
> + * 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 the set and get control operations implemented in
> + * V4L2Device::setControls() and V4L2Device::setControls() respectively.
> + *
> + * In facts, access to the class constructor and to the control value accessor
> + * and modifier are restricted to the friend V4L2Controls and V4L2Device
> + * classes.
> + */
> +
> +/**
> + * \fn V4L2Control::id()
> + * \brief Retrieve the control id this instance refers to
> + * \return The V4L2Control id
> + */
> +
> +/**
> + * \class V4L2Controls
> + * \brief Wraps a list of V4L2Control
> + *
> + * The V4L2Controls class works as a wrapper 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).
> + *
> + * 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 should be added
> + * with an initial value by calling V4L2Controls::add(unsigned int id, long
> + * int value) to prepare for a write operation. The value of controls which
> + * are already part of the instance could be updated with a call to
> + * V4L2Controls::set() operation. Once the values of all controls of interest
> + * have been initialized in a V4L2Controls instance, this should be then
> + * passed to the V4L2Device::setControls() operation, which applies each
> + * control in the list to the hardware.
> + *
> + * Alternatively a control can be add without any initial value by calling
> + * V4L2Controls::add(unsigned int id) and then read from the device passing
> + * the V4L2Controls instance to V4L2Device::getControls() which read each
> + * control value from the hardware.
> + *
> + * Reading controls with an initialized value from the device, overwrites the
> + * control's value, reflecting what has been actually read from the hardware.
> + *
> + * V4L2Controls instances can be reset to remove all controls it contains and
> + * prepare to be re-used for a new control write/read sequence. Alternatively,
> + * the value of a control already part of the instance could be updated by using
> + * the V4L2Controls::set() method, to avoid going through a reset() when a
> + * control has to be read then written to the hardware in sequence.
> + *
> + * In facts, the following pseudo-code sequences lead to the same result:
> + *
> + * V4L2Controls controls;
> + * controls.add(V4L2_CID_xx);
> + * dev->getControls(&controls);
> + * controls.set(V4L2_CID_xx, value);
> + * dev->setControls(&controls);
> + *
> + * V4L2Controls controls;
> + * controls.add(V4L2_CID_xx);
> + * dev->getControls(&controls);
> + * controls.reset();
> + * controls.add(V4L2_CID_xx, value);
> + * dev->setControls(&controls);
> + */
> +
> +/**
> + * \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
> + */
> +
> +/**
> + * \fn V4L2Controls::operator[](unsigned int index)
> + * \brief Access the control at index \a index
> + * \param index The index to access
> + */
> +
> +/**
> + * \brief Add control \a id with a \a value to the instance
> + * \param id The V4L2 control id (V4L2_CID_xx)
> + * \param value The V4L2 control value
> + */
> +void V4L2Controls::add(unsigned int id, long int value)
> +{
> +	/* Cannot use emplace_back() as the V4L2Control ctor is private. */
> +	V4L2Control ctrl(id, value);
> +	controls_.push_back(ctrl);
> +}
> +
> +/**
> + * \brief Add control \a id without an initial value  to the instance
> + * \param id The V4L2 control id (V4L2_CID_xx)
> + */
> +void V4L2Controls::add(unsigned int id)
> +{
> +	/* Cannot use emplace_back() as the V4L2Control ctor is private. */
> +	V4L2Control ctrl(id);
> +	controls_.push_back(ctrl);
> +}
> +
> +/**
> + * \brief Retrieve the value of the control with \a id
> + * \param id The V4L2 control id (V4L2_CID_xx)
> + *
> + * Retrieve the value of the control with id \a id.
> + *
> + * It is important to notice that the returned value is not read from the
> + * 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() or set() operation.
> + *
> + * \return The V4L2 control value or a negative error code if the control id
> + * is not part of this instance
> + */
> +int V4L2Controls::get(unsigned int id)
> +{
> +	V4L2Control *ctrl = getControl(id);
> +	if (!ctrl)
> +		return -EINVAL;
> +
> +	return ctrl->value();
> +}
> +
> +/**
> + * \brief Set the value of the control with \a id
> + * \param id The V4L2 control id (V4L2_CID_xx)
> + * \param value The new V4L2 control value
> + *
> + * Set the value of the control with id \a id.
> + *
> + * 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 pplied only by calling V4L2Device::setControls().
> + *
> + * \return 0 on success or a negative error code if the control id
> + * is not part of this instance
> + */
> +int V4L2Controls::set(unsigned int id, long int value)
> +{
> +	V4L2Control *ctrl = getControl(id);
> +	if (!ctrl)
> +		return -EINVAL;
> +
> +	ctrl->setValue(value);
> +
> +	return 0;
> +}
> +
> +/*
> + * \brief Get a pointer the control with \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 */
> -- 
> 2.21.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Niklas,

On Thu, Jun 20, 2019 at 01:48:11AM +0200, Niklas Söderlund wrote:
> On 2019-06-19 13:08:53 +0200, Jacopo Mondi wrote:
> > Add Libcamera V4L2 control support, implemented using the V4L2 Extended
> > Control APIs. This patch defines the types used to define and manage controls.
> 
> Over all I agree with all Laurents comments on the code and structure, 
> specially that V4L2Controls::controls_ should be a std::map. If you 
> really feel splitting the id and value apart you could try for a 
> std::set, but that would be a tad more code to implement as you wound 
> need to implement the logic to index on the control id of the elements.

I don't think we can replace the vector with a map as the order in which
controls are set may matter. My advice was to supplement it with a map
to optimise the lookup operation.

> Some small nits, bellow.
> 
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/libcamera/include/v4l2_controls.h |  96 ++++++++
> >  src/libcamera/meson.build             |   1 +
> >  src/libcamera/v4l2_controls.cpp       | 337 ++++++++++++++++++++++++++
> >  3 files changed, 434 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..d7b12801329e
> > --- /dev/null
> > +++ b/src/libcamera/include/v4l2_controls.h
> > @@ -0,0 +1,96 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * v4l2_controls.h - V4L2 Extended Control Support
> > + */
> > +
> > +#ifndef __LIBCAMERA_V4L2_CONTROLS_H__
> > +#define __LIBCAMERA_V4L2_CONTROLS_H__
> > +
> > +#include <cstring>
> > +#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_; }
> 
> nit, it bugs me that the order of get methods here differs from the 
> order of variables bellow ;-)

Indeed, I would also reorder the variables.

> > +
> > +private:
> > +	unsigned int type_;
> > +	std::string name_;
> > +	unsigned int id_;
> > +	size_t size_;
> > +};
> > +
> > +class V4L2Control
> > +{
> > +public:
> > +	unsigned int id() const { return id_; }
> > +
> > +private:
> > +	friend class V4L2Device;
> > +	friend class V4L2Controls;
> > +
> > +	V4L2Control(unsigned int id)
> > +		: id_(id) {}
> > +	V4L2Control(unsigned int id, int value)
> > +		: id_(id), value_(value) {}
> > +
> > +	long int value() const { return value_; }
> > +	void setValue(long int value) { value_ = value; }
> > +
> > +	unsigned int id_;
> > +	long int value_;
> > +};
> > +
> > +class V4L2Controls
> > +{
> > +public:
> > +	V4L2Controls &operator=(const V4L2Controls &) = delete;
> > +
> > +	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(); }
> 
> I like empty() better then isEmpty(), but isEmpty() is what we use in 
> libcamera ;-)

Note there's an exception for APIs to mimick an STL container, like in
CameraConfiguration, and I think that exception can apply here.

> > +	size_t size() { return controls_.size(); }

The return type should be std::size_t to follow std::vector.

> > +	void reset() { controls_.clear(); }
> > +
> > +	V4L2Control *operator[](unsigned int index)
> > +	{
> > +		return &controls_[index];
> > +	}
> > +
> > +	void add(unsigned int id, long int value);
> > +	void add(unsigned int id);
> > +
> > +	int get(unsigned int id);
> > +	int set(unsigned int id, long int value);
> > +
> > +private:
> > +	V4L2Control *getControl(unsigned int id);
> > +
> > +	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..75b9c29ca133
> > --- /dev/null
> > +++ b/src/libcamera/v4l2_controls.cpp
> > @@ -0,0 +1,337 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * v4l2_controls.cpp - V4L2 Extended Control Support
> > + */
> > +
> > +#include "v4l2_controls.h"
> > +
> > +/**
> > + * \file v4l2_controls.h
> > + * \brief Support for V4L2 Controls using the V4L2 Extended Controls APIs.
> > + *
> > + * The V4L2 defined "Control API" allows application to inspect and modify set
> > + * of configurable parameters on the video device or subdevice of interest. The
> > + * nature of the parameters an application could modify using the control
> > + * 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.
> > + *
> > + * Controls are identified by a numerical id, defined by the V4L2 kernel headers
> > + * and have an associated type and class. Each control has a 'value', which is
> > + * the data that can be modified with a call to  the 'V4L2Device::setControls()'
> > + * operation or retrieved with a call to 'V4L2Device::getControls()'.
> > + *
> > + * A control class defines the control purpose while its type (along with the
> > + * control's flags) defines the type of the control's value content. Controls
> > + * might transport a single data value stored in variable inside the control, or
> > + * they might as well deal with more 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.
> > + *
> > + * \todo Add support for compound controls
> > + *
> > + * Libcamera implements support for control using the V4L2 'Extended Control'
> > + * framework, which allows easier future handling of controls with payloads
> > + * of arbitrary sizes.
> > + *
> > + * The Libcamera V4L2 Controls framework operates on lists of controls, wrapped
> > + * by the V4L2Controls class, to match the V4L2 extended controls framework.
> > + * The interface to set and get control is implemented by the V4L2Device class,
> > + * and this file only provides the data type definitions.
> > + */
> > +
> > +namespace libcamera {
> > +
> > +/**
> > + * \class V4L2ControlInfo
> > + * \brief Information on a V4L2 control
> > + *
> > + * The V4L2ControlInfo class represent all the informations relative to
> > + * 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 field of
> > + * a struct v4l2_query_ext_ctrl structure, after it has been filled by the
> > + * device driver as a consequence of an VIDIOC_QUERY_EXT_CTRL ioctl call.
> > + *
> > + * This class does not contain the control value, but only static informations
> > + * on the control, which should ideally be cached the first time the control
> > + * is accessed to be reused each time the control value need to be read or
> > + * applied to the hardware.
> > + */
> > +
> > +/**
> > + * \brief Construct a V4L2ControlInfo from a struct v4l2_query_ext_ctrl
> > + */
> > +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 this instance refers to
> > + * \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 or a
> > + * call to V4L2Controls::set().
> > + *
> > + * 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 the set and get control operations implemented in
> > + * V4L2Device::setControls() and V4L2Device::setControls() respectively.
> > + *
> > + * In facts, access to the class constructor and to the control value accessor
> > + * and modifier are restricted to the friend V4L2Controls and V4L2Device
> > + * classes.
> > + */
> > +
> > +/**
> > + * \fn V4L2Control::id()
> > + * \brief Retrieve the control id this instance refers to
> > + * \return The V4L2Control id
> > + */
> > +
> > +/**
> > + * \class V4L2Controls
> > + * \brief Wraps a list of V4L2Control
> > + *
> > + * The V4L2Controls class works as a wrapper 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).
> > + *
> > + * 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 should be added
> > + * with an initial value by calling V4L2Controls::add(unsigned int id, long
> > + * int value) to prepare for a write operation. The value of controls which
> > + * are already part of the instance could be updated with a call to
> > + * V4L2Controls::set() operation. Once the values of all controls of interest
> > + * have been initialized in a V4L2Controls instance, this should be then
> > + * passed to the V4L2Device::setControls() operation, which applies each
> > + * control in the list to the hardware.
> > + *
> > + * Alternatively a control can be add without any initial value by calling
> > + * V4L2Controls::add(unsigned int id) and then read from the device passing
> > + * the V4L2Controls instance to V4L2Device::getControls() which read each
> > + * control value from the hardware.
> > + *
> > + * Reading controls with an initialized value from the device, overwrites the
> > + * control's value, reflecting what has been actually read from the hardware.
> > + *
> > + * V4L2Controls instances can be reset to remove all controls it contains and
> > + * prepare to be re-used for a new control write/read sequence. Alternatively,
> > + * the value of a control already part of the instance could be updated by using
> > + * the V4L2Controls::set() method, to avoid going through a reset() when a
> > + * control has to be read then written to the hardware in sequence.
> > + *
> > + * In facts, the following pseudo-code sequences lead to the same result:
> > + *
> > + * V4L2Controls controls;
> > + * controls.add(V4L2_CID_xx);
> > + * dev->getControls(&controls);
> > + * controls.set(V4L2_CID_xx, value);
> > + * dev->setControls(&controls);
> > + *
> > + * V4L2Controls controls;
> > + * controls.add(V4L2_CID_xx);
> > + * dev->getControls(&controls);
> > + * controls.reset();
> > + * controls.add(V4L2_CID_xx, value);
> > + * dev->setControls(&controls);
> > + */
> > +
> > +/**
> > + * \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
> > + */
> > +
> > +/**
> > + * \fn V4L2Controls::operator[](unsigned int index)
> > + * \brief Access the control at index \a index
> > + * \param index The index to access
> > + */
> > +
> > +/**
> > + * \brief Add control \a id with a \a value to the instance
> > + * \param id The V4L2 control id (V4L2_CID_xx)
> > + * \param value The V4L2 control value
> > + */
> > +void V4L2Controls::add(unsigned int id, long int value)
> > +{
> > +	/* Cannot use emplace_back() as the V4L2Control ctor is private. */
> > +	V4L2Control ctrl(id, value);
> > +	controls_.push_back(ctrl);
> > +}
> > +
> > +/**
> > + * \brief Add control \a id without an initial value  to the instance
> > + * \param id The V4L2 control id (V4L2_CID_xx)
> > + */
> > +void V4L2Controls::add(unsigned int id)
> > +{
> > +	/* Cannot use emplace_back() as the V4L2Control ctor is private. */
> > +	V4L2Control ctrl(id);
> > +	controls_.push_back(ctrl);
> > +}
> > +
> > +/**
> > + * \brief Retrieve the value of the control with \a id
> > + * \param id The V4L2 control id (V4L2_CID_xx)
> > + *
> > + * Retrieve the value of the control with id \a id.
> > + *
> > + * It is important to notice that the returned value is not read from the
> > + * 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() or set() operation.
> > + *
> > + * \return The V4L2 control value or a negative error code if the control id
> > + * is not part of this instance
> > + */
> > +int V4L2Controls::get(unsigned int id)
> > +{
> > +	V4L2Control *ctrl = getControl(id);
> > +	if (!ctrl)
> > +		return -EINVAL;
> > +
> > +	return ctrl->value();
> > +}
> > +
> > +/**
> > + * \brief Set the value of the control with \a id
> > + * \param id The V4L2 control id (V4L2_CID_xx)
> > + * \param value The new V4L2 control value
> > + *
> > + * Set the value of the control with id \a id.
> > + *
> > + * 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 pplied only by calling V4L2Device::setControls().
> > + *
> > + * \return 0 on success or a negative error code if the control id
> > + * is not part of this instance
> > + */
> > +int V4L2Controls::set(unsigned int id, long int value)
> > +{
> > +	V4L2Control *ctrl = getControl(id);
> > +	if (!ctrl)
> > +		return -EINVAL;
> > +
> > +	ctrl->setValue(value);
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * \brief Get a pointer the control with \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 */

Hi Laurent,

On Wed, Jun 19, 2019 at 10:41:27PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Jun 19, 2019 at 01:08:53PM +0200, Jacopo Mondi wrote:
> > Add Libcamera V4L2 control support, implemented using the V4L2 Extended
> > Control APIs. This patch defines the types used to define and manage controls.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/libcamera/include/v4l2_controls.h |  96 ++++++++
> >  src/libcamera/meson.build             |   1 +
> >  src/libcamera/v4l2_controls.cpp       | 337 ++++++++++++++++++++++++++
> >  3 files changed, 434 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..d7b12801329e
> > --- /dev/null
> > +++ b/src/libcamera/include/v4l2_controls.h
> > @@ -0,0 +1,96 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * v4l2_controls.h - V4L2 Extended Control Support
>
> Maybe just V4L2 Control Support ?
>
> > + */
> > +
> > +#ifndef __LIBCAMERA_V4L2_CONTROLS_H__
> > +#define __LIBCAMERA_V4L2_CONTROLS_H__
> > +
> > +#include <cstring>
> > +#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_; }
>
> The name is only used in patch 6/6 to print messages. Do you think we
> should keep it, or can we drop it to save memory ? It's an open
> question.
>

I think the human readable name might be quite useful for debug messages,
and I do expect pipeline handlers developers might want to access it.

It could be added when required, as patch 6/6 is just an hack to
demonstrate how to use controls, but I would keep it there to be
honest.


> > +
> > +private:
> > +	unsigned int type_;
> > +	std::string name_;
> > +	unsigned int id_;
> > +	size_t size_;
> > +};
> > +
> > +class V4L2Control
> > +{
> > +public:
> > +	unsigned int id() const { return id_; }
> > +
> > +private:
> > +	friend class V4L2Device;
> > +	friend class V4L2Controls;
> > +
> > +	V4L2Control(unsigned int id)
> > +		: id_(id) {}
> > +	V4L2Control(unsigned int id, int value)
> > +		: id_(id), value_(value) {}
> > +
> > +	long int value() const { return value_; }
> > +	void setValue(long int value) { value_ = value; }
>
> long int and int are equivalent. I would use types with an explicit size
> (uint_* or int_*). I would also use the same type for the value in the
> constructor.
>

Not on 64bits platforms :)

$ ./a.out
sizeof(long): 8
sizeof(int): 4

Anyway, to avoid this incongruences, I should probably use types with
an explicit size.

However, I wonder if that would require users to go through a cast
everytime they set the value, which I don't really like.

So my idea was to always use a long int (fixed at 8 Bytes in my head)
and eventually cast it down to 32bits for controls that use an int32_t
type.

> > +
> > +	unsigned int id_;
> > +	long int value_;
> > +};
> > +
> > +class V4L2Controls
> > +{
> > +public:
> > +	V4L2Controls &operator=(const V4L2Controls &) = delete;
>
> Should you then also delete the copy constructor ?
>

I was undecided. Do we want to be able to copy controls?

> > +
> > +	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() { return controls_.size(); }
>
> This method can be const too.
>
> > +	void reset() { controls_.clear(); }
> > +
> > +	V4L2Control *operator[](unsigned int index)
> > +	{
> > +		return &controls_[index];
> > +	}
> > +
> > +	void add(unsigned int id, long int value);
> > +	void add(unsigned int id);
>
> You could merge these two functions into one with
>
> 	void add(unsigned int id, long int value = 0);
>
> That way all control values will be initialised to a value, there will
> be no uninitialised memory.
>

Indeed, good suggestion.

> > +
> > +	int get(unsigned int id);
> > +	int set(unsigned int id, long int value);
>
> Integer types with explicit lengths should be used here too.
>
> What is the purpose of the set() function ? Is it used by V4L2Device
> only, to update the value after a get or set ? If so I would make the
> controls_ vector available to V4L2Device, which could iterate over it
> without having to check for IDs, as the internal controls array that is
> used for the ioctls will be in the same order as the vector. That would
> optimise the getControls() and setControls() operations.

Not just by V4L2Device, but if you look at patch 6/6 you'll see that
users of the controls API might want to set values of a control
instance to have it then written to the device.

The usage patter is:
V4L2Controls controls;
controls.add(V4L2_CID_...);
v4l2Device->getControls(&controls);

/* Inspect the control, if you don't like the value then change it */
controls.set(V4L2_CID_..., $newvalue)l
v4l2Device->setControl(&controls);

So I think we should keep set() as re-using a control already part of
a V4L2Controls instance is something useful.
>
> I also had a look at the usage pattern of the get() function in patch
> 6/6, and it looks a bit inefficient. The caller loops over controls
> using the public iterators, to then extract the control ID using the
> V4L2Control::id() method, and then calls V4L2Controls::get() with the
> ID, which searches the controls_ vector again. We start with an item in
> the vector, and then look it up.

Indeed, it sucks a bit, as it requires O(n^2) iterations on the
controls_ vector.

Anyway, I do not expect users to loop over controls in the way I'm
doing in 6/6 but rather access single controls with get() as they
should already know the control ID they're looking for.

>
> One option would be to drop the get() method here, and make the value()
> method public on V4L2Control. Both get() and set() would then be
> removed, with only add() staying.

But then in that case you could only access a single control by
iterating on the V4L2Controls instance. I'm not sure this is the only
usage patter we want to enforce. In that case, being able to access
the control value directly might be useful to avoid an unecessary
iteration on controls_ again, so I might consider making it public,
but I won't remove get() completely.

>
> > +
> > +private:
> > +	V4L2Control *getControl(unsigned int id);
> > +
> > +	std::vector<V4L2Control> controls_;
>
> This makes control lookup by ID a bit inefficient (and requires the
> getControl() method). Should we have a std::map<unsigned int,
> V4L2Control *> that stores pointers to the controls in the controls_
> vector ? getControl() could then be dropped. It would cost a bit more
> memory and a bit more CPU time in the add() operation, but would save
> time at set() and get() time. If set() gets dropped as proposed above,
> only get() will remain, and I wonder if it will be worth it. For small
> sets of controls possibly not, but for large sets it could make a
> difference. What do you think ?
>

As getControl() is private to V4L2Controls it could be dropped an a
map could be used instead. However, I'm not sure what is the
advantage, considering the number of controls we expect to have in a
device. In other words, the backing storage of maps are RB trees, and
the complexity of an access is O(log N) compared to the linear search
of a vector. However insertion and deletion is a bit more costly than
vector, and we cannot replace the vector completely as you have
pointed out, as the order in which to set controls is relevant.

So we're going to increase the memory occupation for little gain in my
opinion.

(What I read online is also that for small sets ( < hundreds of
elements) vectors might be more efficient than maps, but since it's
just something I found on stackoverflow without too much information
in support of the claim, I take it with a grain of salt).

To sum up, I could add a map to support the existing vector, but not
replace it completely and the gain is not that clear to me, also
considering the expected number of elements in the containers.

> > +};
> > +
> > +} /* 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..75b9c29ca133
> > --- /dev/null
> > +++ b/src/libcamera/v4l2_controls.cpp
> > @@ -0,0 +1,337 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * v4l2_controls.cpp - V4L2 Extended Control Support
> > + */
> > +
> > +#include "v4l2_controls.h"
> > +
> > +/**
> > + * \file v4l2_controls.h
> > + * \brief Support for V4L2 Controls using the V4L2 Extended Controls APIs.
> > + *
> > + * The V4L2 defined "Control API" allows application to inspect and modify set
> > + * of configurable parameters on the video device or subdevice of interest. The
> > + * nature of the parameters an application could modify using the control
> > + * 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.
> > + *
> > + * Controls are identified by a numerical id, defined by the V4L2 kernel headers
> > + * and have an associated type and class. Each control has a 'value', which is
>
> I would drop ' and class', we don't care about control classes.
>
> > + * the data that can be modified with a call to  the 'V4L2Device::setControls()'
>
> s/  / /
>
> > + * operation or retrieved with a call to 'V4L2Device::getControls()'.
>
> I don't think you need to enclose the method name in quotes, you can
> write
>
>  * Each control has a value, which is the data that can be modified with
>  * V4L2Device::setControls() or retrieved with V4L2Device::getControls().
>
> > + *
> > + * A control class defines the control purpose while its type (along with the
>
> Similarly, you can drop the class here.
>
> > + * control's flags) defines the type of the control's value content. Controls
> > + * might transport a single data value stored in variable inside the control, or
> > + * they might as well deal with more 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.
>
> s/Libcamera/libcamera/
>
> > + *
> > + * \todo Add support for compound controls
> > + *
> > + * Libcamera implements support for control using the V4L2 'Extended Control'
>
> s/Libcamera/libcamera/
> s/control/controls/
>
> > + * framework, which allows easier future handling of controls with payloads
>
> s/framework/API/
> s/easier //
>
> > + * of arbitrary sizes.
> > + *
> > + * The Libcamera V4L2 Controls framework operates on lists of controls, wrapped
>
> s/Libcamera/libcamera/
>
> > + * by the V4L2Controls class, to match the V4L2 extended controls framework.
>
> s/framework/API/
>
> > + * The interface to set and get control is implemented by the V4L2Device class,
> > + * and this file only provides the data type definitions.
> > + */
> > +
> > +namespace libcamera {
> > +
> > +/**
> > + * \class V4L2ControlInfo
> > + * \brief Information on a V4L2 control
> > + *
> > + * The V4L2ControlInfo class represent all the informations relative to
>
> s/informations/information/ (uncoutable)
> s/relative/related/
>
> > + * 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 field of
> > + * a struct v4l2_query_ext_ctrl structure, after it has been filled by the
> > + * device driver as a consequence of an VIDIOC_QUERY_EXT_CTRL ioctl call.
>
> s/of an/of a/
>
> > + *
> > + * This class does not contain the control value, but only static informations
>
> s/informations/information/
>
> > + * on the control, which should ideally be cached the first time the control
>
> I'd be a bit more directive here, "which shall be cached by the caller
> at initialisation time or the first time the control information is
> needed." and drop the rest of the sentence.
>
> > + * is accessed to be reused each time the control value need to be read or
>
> s/need/needs/
>
> > + * applied to the hardware.
> > + */
> > +
> > +/**
> > + * \brief Construct a V4L2ControlInfo from a struct v4l2_query_ext_ctrl
> > + */
> > +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 this instance refers to
>
> I would just say "Retrieve the control ID"
>
> > + * \return The V4L2 control id
>
> s/id/ID/ (as well as in other locations below)
>
> > + */
> > +
> > +/**
> > + * \fn V4L2ControlInfo::type()
> > + * \brief Retrieve the control type as defined by V4L2_CTRL_TYPE_
>
> Maybe 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 or a
> > + * call to V4L2Controls::set().
> > + *
> > + * 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 the set and get control operations implemented in
> > + * V4L2Device::setControls() and V4L2Device::setControls() respectively.
>
> The second one should be V4L2Device::getControls(), or you could write
>
> "passed as parameter to V4L2Device::getControls() and
> V4L2Device::setControls()."
>
> > + *
> > + * In facts, access to the class constructor and to the control value accessor
> > + * and modifier are restricted to the friend V4L2Controls and V4L2Device
> > + * classes.
>
> I think you can drop this paragraph.
>
> > + */
> > +
> > +/**
> > + * \fn V4L2Control::id()
> > + * \brief Retrieve the control id this instance refers to
> > + * \return The V4L2Control id
> > + */
> > +
> > +/**
> > + * \class V4L2Controls
> > + * \brief Wraps a list of V4L2Control
>
> I would say "Container of V4L2Control instance" and replace wrapper by
> container below.
>
> > + *
> > + * The V4L2Controls class works as a wrapper 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
>
> s/Libcamera/libcamera/
>
> > + * 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).
> > + *
> > + * 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 should be added
>
> s/should/shall/
>
> > + * with an initial value by calling V4L2Controls::add(unsigned int id, long
> > + * int value) to prepare for a write operation. The value of controls which
> > + * are already part of the instance could be updated with a call to
> > + * V4L2Controls::set() operation.
>
> Is there a use case for this ?
>

See the above example.

> > Once the values of all controls of interest
> > + * have been initialized in a V4L2Controls instance, this should be then
> > + * passed to the V4L2Device::setControls() operation, which applies each
> > + * control in the list to the hardware.
>
> Once all desired control have been added, the V4L2Controls instance is
> passed to V4L2Device::setControls(), which sets the controls on the
> device.
>
> > + *
> > + * Alternatively a control can be add without any initial value by calling
> > + * V4L2Controls::add(unsigned int id) and then read from the device passing
> > + * the V4L2Controls instance to V4L2Device::getControls() which read each
> > + * control value from the hardware.
> > + *
> > + * Reading controls with an initialized value from the device, overwrites the
> > + * control's value, reflecting what has been actually read from the hardware.
>
> Let's write this like the previous paragraph.
>
> "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 it contains and
>
> s/it contains/they contain/
>
> > + * prepare to be re-used for a new control write/read sequence. Alternatively,
> > + * the value of a control already part of the instance could be updated by using
> > + * the V4L2Controls::set() method, to avoid going through a reset() when a
> > + * control has to be read then written to the hardware in sequence.
>
> Given how set() is implemented, creating a new V4L2Controls instance
> could be cheaper. Do we really want to reuse V4L2Controls instances with
> set() ? If so I think we need to think about the use cases, and decide
> how to implement it in an efficient way. Alternatively, I would start
> simple and extend the API later.
>

So how would you re-use the same V4L2Controls instance for a
getControls()/setControls() sequence? Go through a reset and re-add
all controls?

> > + *
> > + * In facts, the following pseudo-code sequences lead to the same result:
>
> s/In facts, the/The/
>
> > + *
> > + * V4L2Controls controls;
> > + * controls.add(V4L2_CID_xx);
> > + * dev->getControls(&controls);
> > + * controls.set(V4L2_CID_xx, value);
> > + * dev->setControls(&controls);
> > + *
> > + * V4L2Controls controls;
> > + * controls.add(V4L2_CID_xx);
> > + * dev->getControls(&controls);
> > + * controls.reset();
> > + * controls.add(V4L2_CID_xx, value);
> > + * dev->setControls(&controls);
> > + */
> > +
> > +/**
> > + * \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
> > + */
> > +
> > +/**
> > + * \fn V4L2Controls::operator[](unsigned int index)
> > + * \brief Access the control at index \a index
> > + * \param index The index to access
>
> \return ?
>
> > + */
> > +
> > +/**
> > + * \brief Add control \a id with a \a value to the instance
> > + * \param id The V4L2 control id (V4L2_CID_xx)
>
> s/control id/control ID/
> s/V4L2_CID_xx/V4L2_CID_*/
>
> > + * \param value The V4L2 control value
> > + */
> > +void V4L2Controls::add(unsigned int id, long int value)
> > +{
> > +	/* Cannot use emplace_back() as the V4L2Control ctor is private. */
> > +	V4L2Control ctrl(id, value);
> > +	controls_.push_back(ctrl);
> > +}
> > +
> > +/**
> > + * \brief Add control \a id without an initial value  to the instance
>
> s/  / /
>
> > + * \param id The V4L2 control id (V4L2_CID_xx)
>
> s/control id/control ID/
> s/V4L2_CID_xx/V4L2_CID_*/
>
> Same in a few locations below.
>

Thanks for all the comments in documentation, I'll take them in.

> > + */
> > +void V4L2Controls::add(unsigned int id)
> > +{
> > +	/* Cannot use emplace_back() as the V4L2Control ctor is private. */
>
> Would it make sense to make the constructor public then ? You have
> documented extensively that V4L2Control isn't supposed to be
> instantiated directly by the application, and there will be no way to
> use a directly instantiated V4L2Control anway, so things should be fine.
>

If the only price to pay is not to use emplace_back() I don't think
so.

Shouldn't the following code be equally efficient than emplace_back()?
Isn't the new instance constructed inside the container directly
instead of copied? (emplace_back would just be a syntactic sugar in
that case).

> > +	V4L2Control ctrl(id);
> > +	controls_.push_back(ctrl);
> > +}
> > +
> > +/**
> > + * \brief Retrieve the value of the control with \a id
> > + * \param id The V4L2 control id (V4L2_CID_xx)
> > + *
> > + * Retrieve the value of the control with id \a id.
> > + *
> > + * It is important to notice that the returned value is not read from the
> > + * 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() or set() operation.
> > + *
> > + * \return The V4L2 control value or a negative error code if the control id
> > + * is not part of this instance
> > + */
> > +int V4L2Controls::get(unsigned int id)
> > +{
> > +	V4L2Control *ctrl = getControl(id);
> > +	if (!ctrl)
> > +		return -EINVAL;
> > +
> > +	return ctrl->value();
> > +}
> > +
> > +/**
> > + * \brief Set the value of the control with \a id
> > + * \param id The V4L2 control id (V4L2_CID_xx)
> > + * \param value The new V4L2 control value
> > + *
> > + * Set the value of the control with id \a id.
> > + *
> > + * 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 pplied only by calling V4L2Device::setControls().
> > + *
> > + * \return 0 on success or a negative error code if the control id
> > + * is not part of this instance
> > + */
> > +int V4L2Controls::set(unsigned int id, long int value)
> > +{
> > +	V4L2Control *ctrl = getControl(id);
> > +	if (!ctrl)
> > +		return -EINVAL;
> > +
> > +	ctrl->setValue(value);
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * \brief Get a pointer the control with \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

Hi Jacopo,

On Thu, Jun 20, 2019 at 10:55:52AM +0200, Jacopo Mondi wrote:
> On Wed, Jun 19, 2019 at 10:41:27PM +0300, Laurent Pinchart wrote:
> > On Wed, Jun 19, 2019 at 01:08:53PM +0200, Jacopo Mondi wrote:
> > > Add Libcamera V4L2 control support, implemented using the V4L2 Extended
> > > Control APIs. This patch defines the types used to define and manage controls.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  src/libcamera/include/v4l2_controls.h |  96 ++++++++
> > >  src/libcamera/meson.build             |   1 +
> > >  src/libcamera/v4l2_controls.cpp       | 337 ++++++++++++++++++++++++++
> > >  3 files changed, 434 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..d7b12801329e
> > > --- /dev/null
> > > +++ b/src/libcamera/include/v4l2_controls.h
> > > @@ -0,0 +1,96 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * v4l2_controls.h - V4L2 Extended Control Support
> >
> > Maybe just V4L2 Control Support ?
> >
> > > + */
> > > +
> > > +#ifndef __LIBCAMERA_V4L2_CONTROLS_H__
> > > +#define __LIBCAMERA_V4L2_CONTROLS_H__
> > > +
> > > +#include <cstring>
> > > +#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_; }
> >
> > The name is only used in patch 6/6 to print messages. Do you think we
> > should keep it, or can we drop it to save memory ? It's an open
> > question.
> 
> I think the human readable name might be quite useful for debug messages,
> and I do expect pipeline handlers developers might want to access it.
> 
> It could be added when required, as patch 6/6 is just an hack to
> demonstrate how to use controls, but I would keep it there to be
> honest.

Let's keep it then.

> > > +
> > > +private:
> > > +	unsigned int type_;
> > > +	std::string name_;
> > > +	unsigned int id_;
> > > +	size_t size_;
> > > +};
> > > +
> > > +class V4L2Control
> > > +{
> > > +public:
> > > +	unsigned int id() const { return id_; }
> > > +
> > > +private:
> > > +	friend class V4L2Device;
> > > +	friend class V4L2Controls;
> > > +
> > > +	V4L2Control(unsigned int id)
> > > +		: id_(id) {}
> > > +	V4L2Control(unsigned int id, int value)
> > > +		: id_(id), value_(value) {}
> > > +
> > > +	long int value() const { return value_; }
> > > +	void setValue(long int value) { value_ = value; }
> >
> > long int and int are equivalent. I would use types with an explicit size
> > (uint_* or int_*). I would also use the same type for the value in the
> > constructor.
> 
> Not on 64bits platforms :)
> 
> $ ./a.out
> sizeof(long): 8
> sizeof(int): 4

Yes, I realised that after writing it, it was too late in the evening
:-)

> Anyway, to avoid this incongruences, I should probably use types with
> an explicit size.
> 
> However, I wonder if that would require users to go through a cast
> everytime they set the value, which I don't really like.

I don't think so, the compiler should cast automatically between
integers of different sizes.

> So my idea was to always use a long int (fixed at 8 Bytes in my head)
> and eventually cast it down to 32bits for controls that use an int32_t
> type.

If you always want to use a 64-bit integer, then you should go for
int64_t or uint64_t.

> > > +
> > > +	unsigned int id_;
> > > +	long int value_;
> > > +};
> > > +
> > > +class V4L2Controls
> > > +{
> > > +public:
> > > +	V4L2Controls &operator=(const V4L2Controls &) = delete;
> >
> > Should you then also delete the copy constructor ?
> 
> I was undecided. Do we want to be able to copy controls?

I don't really see a need to, but on the other hand it won't cost us
anything to allow it. Disabling copy means that the compiler will
complain if an application copies when it meant to use a reference, but
it's not necessarily our job to avoid applications shooting themselves
in the foot :-)

> > > +
> > > +	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() { return controls_.size(); }
> >
> > This method can be const too.
> >
> > > +	void reset() { controls_.clear(); }
> > > +
> > > +	V4L2Control *operator[](unsigned int index)
> > > +	{
> > > +		return &controls_[index];
> > > +	}
> > > +
> > > +	void add(unsigned int id, long int value);
> > > +	void add(unsigned int id);
> >
> > You could merge these two functions into one with
> >
> > 	void add(unsigned int id, long int value = 0);
> >
> > That way all control values will be initialised to a value, there will
> > be no uninitialised memory.
> 
> Indeed, good suggestion.
> 
> > > +
> > > +	int get(unsigned int id);
> > > +	int set(unsigned int id, long int value);
> >
> > Integer types with explicit lengths should be used here too.
> >
> > What is the purpose of the set() function ? Is it used by V4L2Device
> > only, to update the value after a get or set ? If so I would make the
> > controls_ vector available to V4L2Device, which could iterate over it
> > without having to check for IDs, as the internal controls array that is
> > used for the ioctls will be in the same order as the vector. That would
> > optimise the getControls() and setControls() operations.
> 
> Not just by V4L2Device, but if you look at patch 6/6 you'll see that
> users of the controls API might want to set values of a control
> instance to have it then written to the device.
> 
> The usage patter is:
> V4L2Controls controls;
> controls.add(V4L2_CID_...);
> v4l2Device->getControls(&controls);
> 
> /* Inspect the control, if you don't like the value then change it */
> controls.set(V4L2_CID_..., $newvalue)l
> v4l2Device->setControl(&controls);
> 
> So I think we should keep set() as re-using a control already part of
> a V4L2Controls instance is something useful.

What's your expected use case for that ? If we keep set, I wonder if we
shouldn't instead add a function to loop up a V4L2Control by ID, and
make the get/set value methods public there. In your above example,
"Inspect the control" would call get(), performing a lookup, and then
set(), performing another lookup for the same control.

> > I also had a look at the usage pattern of the get() function in patch
> > 6/6, and it looks a bit inefficient. The caller loops over controls
> > using the public iterators, to then extract the control ID using the
> > V4L2Control::id() method, and then calls V4L2Controls::get() with the
> > ID, which searches the controls_ vector again. We start with an item in
> > the vector, and then look it up.
> 
> Indeed, it sucks a bit, as it requires O(n^2) iterations on the
> controls_ vector.
> 
> Anyway, I do not expect users to loop over controls in the way I'm
> doing in 6/6 but rather access single controls with get() as they
> should already know the control ID they're looking for.

Still, get() + set() would be 2*O(n) instead of O(n) (which is still
O(n) :-)). I think exposing V4L2Control more would solve both issues.

> > One option would be to drop the get() method here, and make the value()
> > method public on V4L2Control. Both get() and set() would then be
> > removed, with only add() staying.
> 
> But then in that case you could only access a single control by
> iterating on the V4L2Controls instance. I'm not sure this is the only
> usage patter we want to enforce. In that case, being able to access
> the control value directly might be useful to avoid an unecessary
> iteration on controls_ again, so I might consider making it public,
> but I won't remove get() completely.

No, we would also need to expose a function to look up a control by ID,
in addition to operator[]. V4L2Controls would handle the look up,
V4L2Control the get/set value.

> > > +
> > > +private:
> > > +	V4L2Control *getControl(unsigned int id);
> > > +
> > > +	std::vector<V4L2Control> controls_;
> >
> > This makes control lookup by ID a bit inefficient (and requires the
> > getControl() method). Should we have a std::map<unsigned int,
> > V4L2Control *> that stores pointers to the controls in the controls_
> > vector ? getControl() could then be dropped. It would cost a bit more
> > memory and a bit more CPU time in the add() operation, but would save
> > time at set() and get() time. If set() gets dropped as proposed above,
> > only get() will remain, and I wonder if it will be worth it. For small
> > sets of controls possibly not, but for large sets it could make a
> > difference. What do you think ?
> 
> As getControl() is private to V4L2Controls it could be dropped an a
> map could be used instead. However, I'm not sure what is the
> advantage, considering the number of controls we expect to have in a
> device. In other words, the backing storage of maps are RB trees, and
> the complexity of an access is O(log N) compared to the linear search
> of a vector. However insertion and deletion is a bit more costly than
> vector, and we cannot replace the vector completely as you have
> pointed out, as the order in which to set controls is relevant.
> 
> So we're going to increase the memory occupation for little gain in my
> opinion.

I agree that it will likely depend on how many controls we have to
handle. O(n) <=> O(log n) depends on the value of n.

> (What I read online is also that for small sets ( < hundreds of
> elements) vectors might be more efficient than maps, but since it's
> just something I found on stackoverflow without too much information
> in support of the claim, I take it with a grain of salt).
> 
> To sum up, I could add a map to support the existing vector, but not
> replace it completely and the gain is not that clear to me, also
> considering the expected number of elements in the containers.

Then I think we just need to add instrumentation that will report
measurements to a web service, and decide what to do base on that ;-)

> > > +};
> > > +
> > > +} /* 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..75b9c29ca133
> > > --- /dev/null
> > > +++ b/src/libcamera/v4l2_controls.cpp
> > > @@ -0,0 +1,337 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * v4l2_controls.cpp - V4L2 Extended Control Support
> > > + */
> > > +
> > > +#include "v4l2_controls.h"
> > > +
> > > +/**
> > > + * \file v4l2_controls.h
> > > + * \brief Support for V4L2 Controls using the V4L2 Extended Controls APIs.
> > > + *
> > > + * The V4L2 defined "Control API" allows application to inspect and modify set
> > > + * of configurable parameters on the video device or subdevice of interest. The
> > > + * nature of the parameters an application could modify using the control
> > > + * 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.
> > > + *
> > > + * Controls are identified by a numerical id, defined by the V4L2 kernel headers
> > > + * and have an associated type and class. Each control has a 'value', which is
> >
> > I would drop ' and class', we don't care about control classes.
> >
> > > + * the data that can be modified with a call to  the 'V4L2Device::setControls()'
> >
> > s/  / /
> >
> > > + * operation or retrieved with a call to 'V4L2Device::getControls()'.
> >
> > I don't think you need to enclose the method name in quotes, you can
> > write
> >
> >  * Each control has a value, which is the data that can be modified with
> >  * V4L2Device::setControls() or retrieved with V4L2Device::getControls().
> >
> > > + *
> > > + * A control class defines the control purpose while its type (along with the
> >
> > Similarly, you can drop the class here.
> >
> > > + * control's flags) defines the type of the control's value content. Controls
> > > + * might transport a single data value stored in variable inside the control, or
> > > + * they might as well deal with more 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.
> >
> > s/Libcamera/libcamera/
> >
> > > + *
> > > + * \todo Add support for compound controls
> > > + *
> > > + * Libcamera implements support for control using the V4L2 'Extended Control'
> >
> > s/Libcamera/libcamera/
> > s/control/controls/
> >
> > > + * framework, which allows easier future handling of controls with payloads
> >
> > s/framework/API/
> > s/easier //
> >
> > > + * of arbitrary sizes.
> > > + *
> > > + * The Libcamera V4L2 Controls framework operates on lists of controls, wrapped
> >
> > s/Libcamera/libcamera/
> >
> > > + * by the V4L2Controls class, to match the V4L2 extended controls framework.
> >
> > s/framework/API/
> >
> > > + * The interface to set and get control is implemented by the V4L2Device class,
> > > + * and this file only provides the data type definitions.
> > > + */
> > > +
> > > +namespace libcamera {
> > > +
> > > +/**
> > > + * \class V4L2ControlInfo
> > > + * \brief Information on a V4L2 control
> > > + *
> > > + * The V4L2ControlInfo class represent all the informations relative to
> >
> > s/informations/information/ (uncoutable)
> > s/relative/related/
> >
> > > + * 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 field of
> > > + * a struct v4l2_query_ext_ctrl structure, after it has been filled by the
> > > + * device driver as a consequence of an VIDIOC_QUERY_EXT_CTRL ioctl call.
> >
> > s/of an/of a/
> >
> > > + *
> > > + * This class does not contain the control value, but only static informations
> >
> > s/informations/information/
> >
> > > + * on the control, which should ideally be cached the first time the control
> >
> > I'd be a bit more directive here, "which shall be cached by the caller
> > at initialisation time or the first time the control information is
> > needed." and drop the rest of the sentence.
> >
> > > + * is accessed to be reused each time the control value need to be read or
> >
> > s/need/needs/
> >
> > > + * applied to the hardware.
> > > + */
> > > +
> > > +/**
> > > + * \brief Construct a V4L2ControlInfo from a struct v4l2_query_ext_ctrl
> > > + */
> > > +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 this instance refers to
> >
> > I would just say "Retrieve the control ID"
> >
> > > + * \return The V4L2 control id
> >
> > s/id/ID/ (as well as in other locations below)
> >
> > > + */
> > > +
> > > +/**
> > > + * \fn V4L2ControlInfo::type()
> > > + * \brief Retrieve the control type as defined by V4L2_CTRL_TYPE_
> >
> > Maybe 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 or a
> > > + * call to V4L2Controls::set().
> > > + *
> > > + * 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 the set and get control operations implemented in
> > > + * V4L2Device::setControls() and V4L2Device::setControls() respectively.
> >
> > The second one should be V4L2Device::getControls(), or you could write
> >
> > "passed as parameter to V4L2Device::getControls() and
> > V4L2Device::setControls()."
> >
> > > + *
> > > + * In facts, access to the class constructor and to the control value accessor
> > > + * and modifier are restricted to the friend V4L2Controls and V4L2Device
> > > + * classes.
> >
> > I think you can drop this paragraph.
> >
> > > + */
> > > +
> > > +/**
> > > + * \fn V4L2Control::id()
> > > + * \brief Retrieve the control id this instance refers to
> > > + * \return The V4L2Control id
> > > + */
> > > +
> > > +/**
> > > + * \class V4L2Controls
> > > + * \brief Wraps a list of V4L2Control
> >
> > I would say "Container of V4L2Control instance" and replace wrapper by
> > container below.
> >
> > > + *
> > > + * The V4L2Controls class works as a wrapper 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
> >
> > s/Libcamera/libcamera/
> >
> > > + * 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).
> > > + *
> > > + * 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 should be added
> >
> > s/should/shall/
> >
> > > + * with an initial value by calling V4L2Controls::add(unsigned int id, long
> > > + * int value) to prepare for a write operation. The value of controls which
> > > + * are already part of the instance could be updated with a call to
> > > + * V4L2Controls::set() operation.
> >
> > Is there a use case for this ?
> 
> See the above example.
> 
> > > Once the values of all controls of interest
> > > + * have been initialized in a V4L2Controls instance, this should be then
> > > + * passed to the V4L2Device::setControls() operation, which applies each
> > > + * control in the list to the hardware.
> >
> > Once all desired control have been added, the V4L2Controls instance is
> > passed to V4L2Device::setControls(), which sets the controls on the
> > device.
> >
> > > + *
> > > + * Alternatively a control can be add without any initial value by calling
> > > + * V4L2Controls::add(unsigned int id) and then read from the device passing
> > > + * the V4L2Controls instance to V4L2Device::getControls() which read each
> > > + * control value from the hardware.
> > > + *
> > > + * Reading controls with an initialized value from the device, overwrites the
> > > + * control's value, reflecting what has been actually read from the hardware.
> >
> > Let's write this like the previous paragraph.
> >
> > "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 it contains and
> >
> > s/it contains/they contain/
> >
> > > + * prepare to be re-used for a new control write/read sequence. Alternatively,
> > > + * the value of a control already part of the instance could be updated by using
> > > + * the V4L2Controls::set() method, to avoid going through a reset() when a
> > > + * control has to be read then written to the hardware in sequence.
> >
> > Given how set() is implemented, creating a new V4L2Controls instance
> > could be cheaper. Do we really want to reuse V4L2Controls instances with
> > set() ? If so I think we need to think about the use cases, and decide
> > how to implement it in an efficient way. Alternatively, I would start
> > simple and extend the API later.
> 
> So how would you re-use the same V4L2Controls instance for a
> getControls()/setControls() sequence? Go through a reset and re-add
> all controls?

Do we need that at all ? If you have a V4L2Controls instance with 10
controls, call getControls(), check the values and update two of them,
and then call setControls(), all 10 controls will be written to the
driver. Wouldn't it be better to create a second V4L2Controls instance
for the setControls() operation that would be populated with just the
two controls that need to be set ? I also expect those read-modify-write
sequences to be quite uncommon, as I believe getControls() will be used
mainly at initialisation time, at runtime to read volatile controls
(e.g. what was the real exposure value or lens position) or to read
controls that have changed after receiving a V4L2 control change event.

> > > + *
> > > + * In facts, the following pseudo-code sequences lead to the same result:
> >
> > s/In facts, the/The/
> >
> > > + *
> > > + * V4L2Controls controls;
> > > + * controls.add(V4L2_CID_xx);
> > > + * dev->getControls(&controls);
> > > + * controls.set(V4L2_CID_xx, value);
> > > + * dev->setControls(&controls);
> > > + *
> > > + * V4L2Controls controls;
> > > + * controls.add(V4L2_CID_xx);
> > > + * dev->getControls(&controls);
> > > + * controls.reset();
> > > + * controls.add(V4L2_CID_xx, value);
> > > + * dev->setControls(&controls);
> > > + */
> > > +
> > > +/**
> > > + * \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
> > > + */
> > > +
> > > +/**
> > > + * \fn V4L2Controls::operator[](unsigned int index)
> > > + * \brief Access the control at index \a index
> > > + * \param index The index to access
> >
> > \return ?
> >
> > > + */
> > > +
> > > +/**
> > > + * \brief Add control \a id with a \a value to the instance
> > > + * \param id The V4L2 control id (V4L2_CID_xx)
> >
> > s/control id/control ID/
> > s/V4L2_CID_xx/V4L2_CID_*/
> >
> > > + * \param value The V4L2 control value
> > > + */
> > > +void V4L2Controls::add(unsigned int id, long int value)
> > > +{
> > > +	/* Cannot use emplace_back() as the V4L2Control ctor is private. */
> > > +	V4L2Control ctrl(id, value);
> > > +	controls_.push_back(ctrl);
> > > +}
> > > +
> > > +/**
> > > + * \brief Add control \a id without an initial value  to the instance
> >
> > s/  / /
> >
> > > + * \param id The V4L2 control id (V4L2_CID_xx)
> >
> > s/control id/control ID/
> > s/V4L2_CID_xx/V4L2_CID_*/
> >
> > Same in a few locations below.
> 
> Thanks for all the comments in documentation, I'll take them in.
> 
> > > + */
> > > +void V4L2Controls::add(unsigned int id)
> > > +{
> > > +	/* Cannot use emplace_back() as the V4L2Control ctor is private. */
> >
> > Would it make sense to make the constructor public then ? You have
> > documented extensively that V4L2Control isn't supposed to be
> > instantiated directly by the application, and there will be no way to
> > use a directly instantiated V4L2Control anway, so things should be fine.
> >
> 
> If the only price to pay is not to use emplace_back() I don't think
> so.
> 
> Shouldn't the following code be equally efficient than emplace_back()?
> Isn't the new instance constructed inside the container directly
> instead of copied? (emplace_back would just be a syntactic sugar in
> that case).

The code below makes use of the copy constructor, while emplace_back
constructs the new instance in-place.

> > > +	V4L2Control ctrl(id);
> > > +	controls_.push_back(ctrl);
> > > +}
> > > +
> > > +/**
> > > + * \brief Retrieve the value of the control with \a id
> > > + * \param id The V4L2 control id (V4L2_CID_xx)
> > > + *
> > > + * Retrieve the value of the control with id \a id.
> > > + *
> > > + * It is important to notice that the returned value is not read from the
> > > + * 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() or set() operation.
> > > + *
> > > + * \return The V4L2 control value or a negative error code if the control id
> > > + * is not part of this instance
> > > + */
> > > +int V4L2Controls::get(unsigned int id)
> > > +{
> > > +	V4L2Control *ctrl = getControl(id);
> > > +	if (!ctrl)
> > > +		return -EINVAL;
> > > +
> > > +	return ctrl->value();
> > > +}
> > > +
> > > +/**
> > > + * \brief Set the value of the control with \a id
> > > + * \param id The V4L2 control id (V4L2_CID_xx)
> > > + * \param value The new V4L2 control value
> > > + *
> > > + * Set the value of the control with id \a id.
> > > + *
> > > + * 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 pplied only by calling V4L2Device::setControls().
> > > + *
> > > + * \return 0 on success or a negative error code if the control id
> > > + * is not part of this instance
> > > + */
> > > +int V4L2Controls::set(unsigned int id, long int value)
> > > +{
> > > +	V4L2Control *ctrl = getControl(id);
> > > +	if (!ctrl)
> > > +		return -EINVAL;
> > > +
> > > +	ctrl->setValue(value);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/*
> > > + * \brief Get a pointer the control with \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 */

Hi Laurent,

On Thu, Jun 20, 2019 at 02:56:29PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Thu, Jun 20, 2019 at 10:55:52AM +0200, Jacopo Mondi wrote:
> > On Wed, Jun 19, 2019 at 10:41:27PM +0300, Laurent Pinchart wrote:
> > > On Wed, Jun 19, 2019 at 01:08:53PM +0200, Jacopo Mondi wrote:
> > > > Add Libcamera V4L2 control support, implemented using the V4L2 Extended
> > > > Control APIs. This patch defines the types used to define and manage controls.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > ---
> > > >  src/libcamera/include/v4l2_controls.h |  96 ++++++++
> > > >  src/libcamera/meson.build             |   1 +
> > > >  src/libcamera/v4l2_controls.cpp       | 337 ++++++++++++++++++++++++++
> > > >  3 files changed, 434 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..d7b12801329e
> > > > --- /dev/null
> > > > +++ b/src/libcamera/include/v4l2_controls.h
> > > > @@ -0,0 +1,96 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2019, Google Inc.
> > > > + *
> > > > + * v4l2_controls.h - V4L2 Extended Control Support
> > >
> > > Maybe just V4L2 Control Support ?
> > >
> > > > + */
> > > > +
> > > > +#ifndef __LIBCAMERA_V4L2_CONTROLS_H__
> > > > +#define __LIBCAMERA_V4L2_CONTROLS_H__
> > > > +
> > > > +#include <cstring>
> > > > +#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_; }
> > >
> > > The name is only used in patch 6/6 to print messages. Do you think we
> > > should keep it, or can we drop it to save memory ? It's an open
> > > question.
> >
> > I think the human readable name might be quite useful for debug messages,
> > and I do expect pipeline handlers developers might want to access it.
> >
> > It could be added when required, as patch 6/6 is just an hack to
> > demonstrate how to use controls, but I would keep it there to be
> > honest.
>
> Let's keep it then.
>
> > > > +
> > > > +private:
> > > > +	unsigned int type_;
> > > > +	std::string name_;
> > > > +	unsigned int id_;
> > > > +	size_t size_;
> > > > +};
> > > > +
> > > > +class V4L2Control
> > > > +{
> > > > +public:
> > > > +	unsigned int id() const { return id_; }
> > > > +
> > > > +private:
> > > > +	friend class V4L2Device;
> > > > +	friend class V4L2Controls;
> > > > +
> > > > +	V4L2Control(unsigned int id)
> > > > +		: id_(id) {}
> > > > +	V4L2Control(unsigned int id, int value)
> > > > +		: id_(id), value_(value) {}
> > > > +
> > > > +	long int value() const { return value_; }
> > > > +	void setValue(long int value) { value_ = value; }
> > >
> > > long int and int are equivalent. I would use types with an explicit size
> > > (uint_* or int_*). I would also use the same type for the value in the
> > > constructor.
> >
> > Not on 64bits platforms :)
> >
> > $ ./a.out
> > sizeof(long): 8
> > sizeof(int): 4
>
> Yes, I realised that after writing it, it was too late in the evening
> :-)
>
> > Anyway, to avoid this incongruences, I should probably use types with
> > an explicit size.
> >
> > However, I wonder if that would require users to go through a cast
> > everytime they set the value, which I don't really like.
>
> I don't think so, the compiler should cast automatically between
> integers of different sizes.
>
> > So my idea was to always use a long int (fixed at 8 Bytes in my head)
> > and eventually cast it down to 32bits for controls that use an int32_t
> > type.
>
> If you always want to use a 64-bit integer, then you should go for
> int64_t or uint64_t.
>

I'll go for int64_t as that the largest supported type for
non-compound controls.

> > > > +
> > > > +	unsigned int id_;
> > > > +	long int value_;
> > > > +};
> > > > +
> > > > +class V4L2Controls
> > > > +{
> > > > +public:
> > > > +	V4L2Controls &operator=(const V4L2Controls &) = delete;
> > >
> > > Should you then also delete the copy constructor ?
> >
> > I was undecided. Do we want to be able to copy controls?
>
> I don't really see a need to, but on the other hand it won't cost us
> anything to allow it. Disabling copy means that the compiler will
> complain if an application copies when it meant to use a reference, but
> it's not necessarily our job to avoid applications shooting themselves
> in the foot :-)
>
> > > > +
> > > > +	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() { return controls_.size(); }
> > >
> > > This method can be const too.
> > >
> > > > +	void reset() { controls_.clear(); }
> > > > +
> > > > +	V4L2Control *operator[](unsigned int index)
> > > > +	{
> > > > +		return &controls_[index];
> > > > +	}
> > > > +
> > > > +	void add(unsigned int id, long int value);
> > > > +	void add(unsigned int id);
> > >
> > > You could merge these two functions into one with
> > >
> > > 	void add(unsigned int id, long int value = 0);
> > >
> > > That way all control values will be initialised to a value, there will
> > > be no uninitialised memory.
> >
> > Indeed, good suggestion.
> >
> > > > +
> > > > +	int get(unsigned int id);
> > > > +	int set(unsigned int id, long int value);
> > >
> > > Integer types with explicit lengths should be used here too.
> > >
> > > What is the purpose of the set() function ? Is it used by V4L2Device
> > > only, to update the value after a get or set ? If so I would make the
> > > controls_ vector available to V4L2Device, which could iterate over it
> > > without having to check for IDs, as the internal controls array that is
> > > used for the ioctls will be in the same order as the vector. That would
> > > optimise the getControls() and setControls() operations.
> >
> > Not just by V4L2Device, but if you look at patch 6/6 you'll see that
> > users of the controls API might want to set values of a control
> > instance to have it then written to the device.
> >
> > The usage patter is:
> > V4L2Controls controls;
> > controls.add(V4L2_CID_...);
> > v4l2Device->getControls(&controls);
> >
> > /* Inspect the control, if you don't like the value then change it */
> > controls.set(V4L2_CID_..., $newvalue)l
> > v4l2Device->setControl(&controls);
> >
> > So I think we should keep set() as re-using a control already part of
> > a V4L2Controls instance is something useful.
>
> What's your expected use case for that ? If we keep set, I wonder if we
> shouldn't instead add a function to loop up a V4L2Control by ID, and
> make the get/set value methods public there. In your above example,
> "Inspect the control" would call get(), performing a lookup, and then
> set(), performing another lookup for the same control.
>
> > > I also had a look at the usage pattern of the get() function in patch
> > > 6/6, and it looks a bit inefficient. The caller loops over controls
> > > using the public iterators, to then extract the control ID using the
> > > V4L2Control::id() method, and then calls V4L2Controls::get() with the
> > > ID, which searches the controls_ vector again. We start with an item in
> > > the vector, and then look it up.
> >
> > Indeed, it sucks a bit, as it requires O(n^2) iterations on the
> > controls_ vector.
> >
> > Anyway, I do not expect users to loop over controls in the way I'm
> > doing in 6/6 but rather access single controls with get() as they
> > should already know the control ID they're looking for.
>
> Still, get() + set() would be 2*O(n) instead of O(n) (which is still
> O(n) :-)). I think exposing V4L2Control more would solve both issues.
>

Probably that's the best and would solve most of the lookup issues.

> > > One option would be to drop the get() method here, and make the value()
> > > method public on V4L2Control. Both get() and set() would then be
> > > removed, with only add() staying.
> >
> > But then in that case you could only access a single control by
> > iterating on the V4L2Controls instance. I'm not sure this is the only
> > usage patter we want to enforce. In that case, being able to access
> > the control value directly might be useful to avoid an unecessary
> > iteration on controls_ again, so I might consider making it public,
> > but I won't remove get() completely.
>
> No, we would also need to expose a function to look up a control by ID,
> in addition to operator[]. V4L2Controls would handle the look up,
> V4L2Control the get/set value.
>
> > > > +
> > > > +private:
> > > > +	V4L2Control *getControl(unsigned int id);
> > > > +
> > > > +	std::vector<V4L2Control> controls_;
> > >
> > > This makes control lookup by ID a bit inefficient (and requires the
> > > getControl() method). Should we have a std::map<unsigned int,
> > > V4L2Control *> that stores pointers to the controls in the controls_
> > > vector ? getControl() could then be dropped. It would cost a bit more
> > > memory and a bit more CPU time in the add() operation, but would save
> > > time at set() and get() time. If set() gets dropped as proposed above,
> > > only get() will remain, and I wonder if it will be worth it. For small
> > > sets of controls possibly not, but for large sets it could make a
> > > difference. What do you think ?
> >
> > As getControl() is private to V4L2Controls it could be dropped an a
> > map could be used instead. However, I'm not sure what is the
> > advantage, considering the number of controls we expect to have in a
> > device. In other words, the backing storage of maps are RB trees, and
> > the complexity of an access is O(log N) compared to the linear search
> > of a vector. However insertion and deletion is a bit more costly than
> > vector, and we cannot replace the vector completely as you have
> > pointed out, as the order in which to set controls is relevant.
> >
> > So we're going to increase the memory occupation for little gain in my
> > opinion.
>
> I agree that it will likely depend on how many controls we have to
> handle. O(n) <=> O(log n) depends on the value of n.
>
> > (What I read online is also that for small sets ( < hundreds of
> > elements) vectors might be more efficient than maps, but since it's
> > just something I found on stackoverflow without too much information
> > in support of the claim, I take it with a grain of salt).
> >
> > To sum up, I could add a map to support the existing vector, but not
> > replace it completely and the gain is not that clear to me, also
> > considering the expected number of elements in the containers.
>
> Then I think we just need to add instrumentation that will report
> measurements to a web service, and decide what to do base on that ;-)
>

That seems a very good idea. I would make all users aware that we now
support backdoors^W^Wmetrics! it's a killer feature for a camera library ;)

> > > > +};
> > > > +
> > > > +} /* 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..75b9c29ca133
> > > > --- /dev/null
> > > > +++ b/src/libcamera/v4l2_controls.cpp
> > > > @@ -0,0 +1,337 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2019, Google Inc.
> > > > + *
> > > > + * v4l2_controls.cpp - V4L2 Extended Control Support
> > > > + */
> > > > +
> > > > +#include "v4l2_controls.h"
> > > > +
> > > > +/**
> > > > + * \file v4l2_controls.h
> > > > + * \brief Support for V4L2 Controls using the V4L2 Extended Controls APIs.
> > > > + *
> > > > + * The V4L2 defined "Control API" allows application to inspect and modify set
> > > > + * of configurable parameters on the video device or subdevice of interest. The
> > > > + * nature of the parameters an application could modify using the control
> > > > + * 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.
> > > > + *
> > > > + * Controls are identified by a numerical id, defined by the V4L2 kernel headers
> > > > + * and have an associated type and class. Each control has a 'value', which is
> > >
> > > I would drop ' and class', we don't care about control classes.
> > >
> > > > + * the data that can be modified with a call to  the 'V4L2Device::setControls()'
> > >
> > > s/  / /
> > >
> > > > + * operation or retrieved with a call to 'V4L2Device::getControls()'.
> > >
> > > I don't think you need to enclose the method name in quotes, you can
> > > write
> > >
> > >  * Each control has a value, which is the data that can be modified with
> > >  * V4L2Device::setControls() or retrieved with V4L2Device::getControls().
> > >
> > > > + *
> > > > + * A control class defines the control purpose while its type (along with the
> > >
> > > Similarly, you can drop the class here.
> > >
> > > > + * control's flags) defines the type of the control's value content. Controls
> > > > + * might transport a single data value stored in variable inside the control, or
> > > > + * they might as well deal with more 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.
> > >
> > > s/Libcamera/libcamera/
> > >
> > > > + *
> > > > + * \todo Add support for compound controls
> > > > + *
> > > > + * Libcamera implements support for control using the V4L2 'Extended Control'
> > >
> > > s/Libcamera/libcamera/
> > > s/control/controls/
> > >
> > > > + * framework, which allows easier future handling of controls with payloads
> > >
> > > s/framework/API/
> > > s/easier //
> > >
> > > > + * of arbitrary sizes.
> > > > + *
> > > > + * The Libcamera V4L2 Controls framework operates on lists of controls, wrapped
> > >
> > > s/Libcamera/libcamera/
> > >
> > > > + * by the V4L2Controls class, to match the V4L2 extended controls framework.
> > >
> > > s/framework/API/
> > >
> > > > + * The interface to set and get control is implemented by the V4L2Device class,
> > > > + * and this file only provides the data type definitions.
> > > > + */
> > > > +
> > > > +namespace libcamera {
> > > > +
> > > > +/**
> > > > + * \class V4L2ControlInfo
> > > > + * \brief Information on a V4L2 control
> > > > + *
> > > > + * The V4L2ControlInfo class represent all the informations relative to
> > >
> > > s/informations/information/ (uncoutable)
> > > s/relative/related/
> > >
> > > > + * 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 field of
> > > > + * a struct v4l2_query_ext_ctrl structure, after it has been filled by the
> > > > + * device driver as a consequence of an VIDIOC_QUERY_EXT_CTRL ioctl call.
> > >
> > > s/of an/of a/
> > >
> > > > + *
> > > > + * This class does not contain the control value, but only static informations
> > >
> > > s/informations/information/
> > >
> > > > + * on the control, which should ideally be cached the first time the control
> > >
> > > I'd be a bit more directive here, "which shall be cached by the caller
> > > at initialisation time or the first time the control information is
> > > needed." and drop the rest of the sentence.
> > >
> > > > + * is accessed to be reused each time the control value need to be read or
> > >
> > > s/need/needs/
> > >
> > > > + * applied to the hardware.
> > > > + */
> > > > +
> > > > +/**
> > > > + * \brief Construct a V4L2ControlInfo from a struct v4l2_query_ext_ctrl
> > > > + */
> > > > +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 this instance refers to
> > >
> > > I would just say "Retrieve the control ID"
> > >
> > > > + * \return The V4L2 control id
> > >
> > > s/id/ID/ (as well as in other locations below)
> > >
> > > > + */
> > > > +
> > > > +/**
> > > > + * \fn V4L2ControlInfo::type()
> > > > + * \brief Retrieve the control type as defined by V4L2_CTRL_TYPE_
> > >
> > > Maybe 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 or a
> > > > + * call to V4L2Controls::set().
> > > > + *
> > > > + * 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 the set and get control operations implemented in
> > > > + * V4L2Device::setControls() and V4L2Device::setControls() respectively.
> > >
> > > The second one should be V4L2Device::getControls(), or you could write
> > >
> > > "passed as parameter to V4L2Device::getControls() and
> > > V4L2Device::setControls()."
> > >
> > > > + *
> > > > + * In facts, access to the class constructor and to the control value accessor
> > > > + * and modifier are restricted to the friend V4L2Controls and V4L2Device
> > > > + * classes.
> > >
> > > I think you can drop this paragraph.
> > >
> > > > + */
> > > > +
> > > > +/**
> > > > + * \fn V4L2Control::id()
> > > > + * \brief Retrieve the control id this instance refers to
> > > > + * \return The V4L2Control id
> > > > + */
> > > > +
> > > > +/**
> > > > + * \class V4L2Controls
> > > > + * \brief Wraps a list of V4L2Control
> > >
> > > I would say "Container of V4L2Control instance" and replace wrapper by
> > > container below.
> > >
> > > > + *
> > > > + * The V4L2Controls class works as a wrapper 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
> > >
> > > s/Libcamera/libcamera/
> > >
> > > > + * 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).
> > > > + *
> > > > + * 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 should be added
> > >
> > > s/should/shall/
> > >
> > > > + * with an initial value by calling V4L2Controls::add(unsigned int id, long
> > > > + * int value) to prepare for a write operation. The value of controls which
> > > > + * are already part of the instance could be updated with a call to
> > > > + * V4L2Controls::set() operation.
> > >
> > > Is there a use case for this ?
> >
> > See the above example.
> >
> > > > Once the values of all controls of interest
> > > > + * have been initialized in a V4L2Controls instance, this should be then
> > > > + * passed to the V4L2Device::setControls() operation, which applies each
> > > > + * control in the list to the hardware.
> > >
> > > Once all desired control have been added, the V4L2Controls instance is
> > > passed to V4L2Device::setControls(), which sets the controls on the
> > > device.
> > >
> > > > + *
> > > > + * Alternatively a control can be add without any initial value by calling
> > > > + * V4L2Controls::add(unsigned int id) and then read from the device passing
> > > > + * the V4L2Controls instance to V4L2Device::getControls() which read each
> > > > + * control value from the hardware.
> > > > + *
> > > > + * Reading controls with an initialized value from the device, overwrites the
> > > > + * control's value, reflecting what has been actually read from the hardware.
> > >
> > > Let's write this like the previous paragraph.
> > >
> > > "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 it contains and
> > >
> > > s/it contains/they contain/
> > >
> > > > + * prepare to be re-used for a new control write/read sequence. Alternatively,
> > > > + * the value of a control already part of the instance could be updated by using
> > > > + * the V4L2Controls::set() method, to avoid going through a reset() when a
> > > > + * control has to be read then written to the hardware in sequence.
> > >
> > > Given how set() is implemented, creating a new V4L2Controls instance
> > > could be cheaper. Do we really want to reuse V4L2Controls instances with
> > > set() ? If so I think we need to think about the use cases, and decide
> > > how to implement it in an efficient way. Alternatively, I would start
> > > simple and extend the API later.
> >
> > So how would you re-use the same V4L2Controls instance for a
> > getControls()/setControls() sequence? Go through a reset and re-add
> > all controls?
>
> Do we need that at all ? If you have a V4L2Controls instance with 10
> controls, call getControls(), check the values and update two of them,
> and then call setControls(), all 10 controls will be written to the
> driver. Wouldn't it be better to create a second V4L2Controls instance
> for the setControls() operation that would be populated with just the
> two controls that need to be set ? I also expect those read-modify-write

Or possibly "reset()" the existing one.

> sequences to be quite uncommon, as I believe getControls() will be used
> mainly at initialisation time, at runtime to read volatile controls
> (e.g. what was the real exposure value or lens position) or to read
> controls that have changed after receiving a V4L2 control change event.
>
> > > > + *
> > > > + * In facts, the following pseudo-code sequences lead to the same result:
> > >
> > > s/In facts, the/The/
> > >
> > > > + *
> > > > + * V4L2Controls controls;
> > > > + * controls.add(V4L2_CID_xx);
> > > > + * dev->getControls(&controls);
> > > > + * controls.set(V4L2_CID_xx, value);
> > > > + * dev->setControls(&controls);
> > > > + *
> > > > + * V4L2Controls controls;
> > > > + * controls.add(V4L2_CID_xx);
> > > > + * dev->getControls(&controls);
> > > > + * controls.reset();
> > > > + * controls.add(V4L2_CID_xx, value);
> > > > + * dev->setControls(&controls);
> > > > + */
> > > > +
> > > > +/**
> > > > + * \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
> > > > + */
> > > > +
> > > > +/**
> > > > + * \fn V4L2Controls::operator[](unsigned int index)
> > > > + * \brief Access the control at index \a index
> > > > + * \param index The index to access
> > >
> > > \return ?
> > >
> > > > + */
> > > > +
> > > > +/**
> > > > + * \brief Add control \a id with a \a value to the instance
> > > > + * \param id The V4L2 control id (V4L2_CID_xx)
> > >
> > > s/control id/control ID/
> > > s/V4L2_CID_xx/V4L2_CID_*/
> > >
> > > > + * \param value The V4L2 control value
> > > > + */
> > > > +void V4L2Controls::add(unsigned int id, long int value)
> > > > +{
> > > > +	/* Cannot use emplace_back() as the V4L2Control ctor is private. */
> > > > +	V4L2Control ctrl(id, value);
> > > > +	controls_.push_back(ctrl);
> > > > +}
> > > > +
> > > > +/**
> > > > + * \brief Add control \a id without an initial value  to the instance
> > >
> > > s/  / /
> > >
> > > > + * \param id The V4L2 control id (V4L2_CID_xx)
> > >
> > > s/control id/control ID/
> > > s/V4L2_CID_xx/V4L2_CID_*/
> > >
> > > Same in a few locations below.
> >
> > Thanks for all the comments in documentation, I'll take them in.
> >
> > > > + */
> > > > +void V4L2Controls::add(unsigned int id)
> > > > +{
> > > > +	/* Cannot use emplace_back() as the V4L2Control ctor is private. */
> > >
> > > Would it make sense to make the constructor public then ? You have
> > > documented extensively that V4L2Control isn't supposed to be
> > > instantiated directly by the application, and there will be no way to
> > > use a directly instantiated V4L2Control anway, so things should be fine.
> > >
> >
> > If the only price to pay is not to use emplace_back() I don't think
> > so.
> >
> > Shouldn't the following code be equally efficient than emplace_back()?
> > Isn't the new instance constructed inside the container directly
> > instead of copied? (emplace_back would just be a syntactic sugar in
> > that case).
>
> The code below makes use of the copy constructor, while emplace_back
> constructs the new instance in-place.
>

Ok, I'll make the constructor accessible. It was for the V4L2Controls
friend call, but in this case it needs to be public as it's
std::vector that tries to construct it.


> > > > +	V4L2Control ctrl(id);
> > > > +	controls_.push_back(ctrl);
> > > > +}
> > > > +
> > > > +/**
> > > > + * \brief Retrieve the value of the control with \a id
> > > > + * \param id The V4L2 control id (V4L2_CID_xx)
> > > > + *
> > > > + * Retrieve the value of the control with id \a id.
> > > > + *
> > > > + * It is important to notice that the returned value is not read from the
> > > > + * 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() or set() operation.
> > > > + *
> > > > + * \return The V4L2 control value or a negative error code if the control id
> > > > + * is not part of this instance
> > > > + */
> > > > +int V4L2Controls::get(unsigned int id)
> > > > +{
> > > > +	V4L2Control *ctrl = getControl(id);
> > > > +	if (!ctrl)
> > > > +		return -EINVAL;
> > > > +
> > > > +	return ctrl->value();
> > > > +}
> > > > +
> > > > +/**
> > > > + * \brief Set the value of the control with \a id
> > > > + * \param id The V4L2 control id (V4L2_CID_xx)
> > > > + * \param value The new V4L2 control value
> > > > + *
> > > > + * Set the value of the control with id \a id.
> > > > + *
> > > > + * 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 pplied only by calling V4L2Device::setControls().
> > > > + *
> > > > + * \return 0 on success or a negative error code if the control id
> > > > + * is not part of this instance
> > > > + */
> > > > +int V4L2Controls::set(unsigned int id, long int value)
> > > > +{
> > > > +	V4L2Control *ctrl = getControl(id);
> > > > +	if (!ctrl)
> > > > +		return -EINVAL;
> > > > +
> > > > +	ctrl->setValue(value);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +/*
> > > > + * \brief Get a pointer the control with \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

Hi Jacopo,

On Thu, Jun 20, 2019 at 02:44:48PM +0200, Jacopo Mondi wrote:
> On Thu, Jun 20, 2019 at 02:56:29PM +0300, Laurent Pinchart wrote:
> > On Thu, Jun 20, 2019 at 10:55:52AM +0200, Jacopo Mondi wrote:
> >> On Wed, Jun 19, 2019 at 10:41:27PM +0300, Laurent Pinchart wrote:
> >>> On Wed, Jun 19, 2019 at 01:08:53PM +0200, Jacopo Mondi wrote:
> >>>> Add Libcamera V4L2 control support, implemented using the V4L2 Extended
> >>>> Control APIs. This patch defines the types used to define and manage controls.
> >>>>
> >>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>>> ---
> >>>>  src/libcamera/include/v4l2_controls.h |  96 ++++++++
> >>>>  src/libcamera/meson.build             |   1 +
> >>>>  src/libcamera/v4l2_controls.cpp       | 337 ++++++++++++++++++++++++++
> >>>>  3 files changed, 434 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..d7b12801329e
> >>>> --- /dev/null
> >>>> +++ b/src/libcamera/include/v4l2_controls.h
> >>>> @@ -0,0 +1,96 @@
> >>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >>>> +/*
> >>>> + * Copyright (C) 2019, Google Inc.
> >>>> + *
> >>>> + * v4l2_controls.h - V4L2 Extended Control Support
> >>>
> >>> Maybe just V4L2 Control Support ?
> >>>
> >>>> + */
> >>>> +
> >>>> +#ifndef __LIBCAMERA_V4L2_CONTROLS_H__
> >>>> +#define __LIBCAMERA_V4L2_CONTROLS_H__
> >>>> +
> >>>> +#include <cstring>
> >>>> +#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_; }
> >>>
> >>> The name is only used in patch 6/6 to print messages. Do you think we
> >>> should keep it, or can we drop it to save memory ? It's an open
> >>> question.
> >>
> >> I think the human readable name might be quite useful for debug messages,
> >> and I do expect pipeline handlers developers might want to access it.
> >>
> >> It could be added when required, as patch 6/6 is just an hack to
> >> demonstrate how to use controls, but I would keep it there to be
> >> honest.
> >
> > Let's keep it then.
> >
> >>>> +
> >>>> +private:
> >>>> +	unsigned int type_;
> >>>> +	std::string name_;
> >>>> +	unsigned int id_;
> >>>> +	size_t size_;
> >>>> +};
> >>>> +
> >>>> +class V4L2Control
> >>>> +{
> >>>> +public:
> >>>> +	unsigned int id() const { return id_; }
> >>>> +
> >>>> +private:
> >>>> +	friend class V4L2Device;
> >>>> +	friend class V4L2Controls;
> >>>> +
> >>>> +	V4L2Control(unsigned int id)
> >>>> +		: id_(id) {}
> >>>> +	V4L2Control(unsigned int id, int value)
> >>>> +		: id_(id), value_(value) {}
> >>>> +
> >>>> +	long int value() const { return value_; }
> >>>> +	void setValue(long int value) { value_ = value; }
> >>>
> >>> long int and int are equivalent. I would use types with an explicit size
> >>> (uint_* or int_*). I would also use the same type for the value in the
> >>> constructor.
> >>
> >> Not on 64bits platforms :)
> >>
> >> $ ./a.out
> >> sizeof(long): 8
> >> sizeof(int): 4
> >
> > Yes, I realised that after writing it, it was too late in the evening
> > :-)
> >
> >> Anyway, to avoid this incongruences, I should probably use types with
> >> an explicit size.
> >>
> >> However, I wonder if that would require users to go through a cast
> >> everytime they set the value, which I don't really like.
> >
> > I don't think so, the compiler should cast automatically between
> > integers of different sizes.
> >
> >> So my idea was to always use a long int (fixed at 8 Bytes in my head)
> >> and eventually cast it down to 32bits for controls that use an int32_t
> >> type.
> >
> > If you always want to use a 64-bit integer, then you should go for
> > int64_t or uint64_t.
> 
> I'll go for int64_t as that the largest supported type for
> non-compound controls.
> 
> >>>> +
> >>>> +	unsigned int id_;
> >>>> +	long int value_;
> >>>> +};
> >>>> +
> >>>> +class V4L2Controls
> >>>> +{
> >>>> +public:
> >>>> +	V4L2Controls &operator=(const V4L2Controls &) = delete;
> >>>
> >>> Should you then also delete the copy constructor ?
> >>
> >> I was undecided. Do we want to be able to copy controls?
> >
> > I don't really see a need to, but on the other hand it won't cost us
> > anything to allow it. Disabling copy means that the compiler will
> > complain if an application copies when it meant to use a reference, but
> > it's not necessarily our job to avoid applications shooting themselves
> > in the foot :-)
> >
> >>>> +
> >>>> +	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() { return controls_.size(); }
> >>>
> >>> This method can be const too.
> >>>
> >>>> +	void reset() { controls_.clear(); }
> >>>> +
> >>>> +	V4L2Control *operator[](unsigned int index)
> >>>> +	{
> >>>> +		return &controls_[index];
> >>>> +	}
> >>>> +
> >>>> +	void add(unsigned int id, long int value);
> >>>> +	void add(unsigned int id);
> >>>
> >>> You could merge these two functions into one with
> >>>
> >>> 	void add(unsigned int id, long int value = 0);
> >>>
> >>> That way all control values will be initialised to a value, there will
> >>> be no uninitialised memory.
> >>
> >> Indeed, good suggestion.
> >>
> >>>> +
> >>>> +	int get(unsigned int id);
> >>>> +	int set(unsigned int id, long int value);
> >>>
> >>> Integer types with explicit lengths should be used here too.
> >>>
> >>> What is the purpose of the set() function ? Is it used by V4L2Device
> >>> only, to update the value after a get or set ? If so I would make the
> >>> controls_ vector available to V4L2Device, which could iterate over it
> >>> without having to check for IDs, as the internal controls array that is
> >>> used for the ioctls will be in the same order as the vector. That would
> >>> optimise the getControls() and setControls() operations.
> >>
> >> Not just by V4L2Device, but if you look at patch 6/6 you'll see that
> >> users of the controls API might want to set values of a control
> >> instance to have it then written to the device.
> >>
> >> The usage patter is:
> >> V4L2Controls controls;
> >> controls.add(V4L2_CID_...);
> >> v4l2Device->getControls(&controls);
> >>
> >> /* Inspect the control, if you don't like the value then change it */
> >> controls.set(V4L2_CID_..., $newvalue)l
> >> v4l2Device->setControl(&controls);
> >>
> >> So I think we should keep set() as re-using a control already part of
> >> a V4L2Controls instance is something useful.
> >
> > What's your expected use case for that ? If we keep set, I wonder if we
> > shouldn't instead add a function to loop up a V4L2Control by ID, and
> > make the get/set value methods public there. In your above example,
> > "Inspect the control" would call get(), performing a lookup, and then
> > set(), performing another lookup for the same control.
> >
> >>> I also had a look at the usage pattern of the get() function in patch
> >>> 6/6, and it looks a bit inefficient. The caller loops over controls
> >>> using the public iterators, to then extract the control ID using the
> >>> V4L2Control::id() method, and then calls V4L2Controls::get() with the
> >>> ID, which searches the controls_ vector again. We start with an item in
> >>> the vector, and then look it up.
> >>
> >> Indeed, it sucks a bit, as it requires O(n^2) iterations on the
> >> controls_ vector.
> >>
> >> Anyway, I do not expect users to loop over controls in the way I'm
> >> doing in 6/6 but rather access single controls with get() as they
> >> should already know the control ID they're looking for.
> >
> > Still, get() + set() would be 2*O(n) instead of O(n) (which is still
> > O(n) :-)). I think exposing V4L2Control more would solve both issues.
> >
> 
> Probably that's the best and would solve most of the lookup issues.
> 
> >>> One option would be to drop the get() method here, and make the value()
> >>> method public on V4L2Control. Both get() and set() would then be
> >>> removed, with only add() staying.
> >>
> >> But then in that case you could only access a single control by
> >> iterating on the V4L2Controls instance. I'm not sure this is the only
> >> usage patter we want to enforce. In that case, being able to access
> >> the control value directly might be useful to avoid an unecessary
> >> iteration on controls_ again, so I might consider making it public,
> >> but I won't remove get() completely.
> >
> > No, we would also need to expose a function to look up a control by ID,
> > in addition to operator[]. V4L2Controls would handle the look up,
> > V4L2Control the get/set value.
> >
> >>>> +
> >>>> +private:
> >>>> +	V4L2Control *getControl(unsigned int id);
> >>>> +
> >>>> +	std::vector<V4L2Control> controls_;
> >>>
> >>> This makes control lookup by ID a bit inefficient (and requires the
> >>> getControl() method). Should we have a std::map<unsigned int,
> >>> V4L2Control *> that stores pointers to the controls in the controls_
> >>> vector ? getControl() could then be dropped. It would cost a bit more
> >>> memory and a bit more CPU time in the add() operation, but would save
> >>> time at set() and get() time. If set() gets dropped as proposed above,
> >>> only get() will remain, and I wonder if it will be worth it. For small
> >>> sets of controls possibly not, but for large sets it could make a
> >>> difference. What do you think ?
> >>
> >> As getControl() is private to V4L2Controls it could be dropped an a
> >> map could be used instead. However, I'm not sure what is the
> >> advantage, considering the number of controls we expect to have in a
> >> device. In other words, the backing storage of maps are RB trees, and
> >> the complexity of an access is O(log N) compared to the linear search
> >> of a vector. However insertion and deletion is a bit more costly than
> >> vector, and we cannot replace the vector completely as you have
> >> pointed out, as the order in which to set controls is relevant.
> >>
> >> So we're going to increase the memory occupation for little gain in my
> >> opinion.
> >
> > I agree that it will likely depend on how many controls we have to
> > handle. O(n) <=> O(log n) depends on the value of n.
> >
> >> (What I read online is also that for small sets ( < hundreds of
> >> elements) vectors might be more efficient than maps, but since it's
> >> just something I found on stackoverflow without too much information
> >> in support of the claim, I take it with a grain of salt).
> >>
> >> To sum up, I could add a map to support the existing vector, but not
> >> replace it completely and the gain is not that clear to me, also
> >> considering the expected number of elements in the containers.
> >
> > Then I think we just need to add instrumentation that will report
> > measurements to a web service, and decide what to do base on that ;-)
> >
> 
> That seems a very good idea. I would make all users aware that we now
> support backdoors^W^Wmetrics! it's a killer feature for a camera library ;)
> 
> >>>> +};
> >>>> +
> >>>> +} /* 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..75b9c29ca133
> >>>> --- /dev/null
> >>>> +++ b/src/libcamera/v4l2_controls.cpp
> >>>> @@ -0,0 +1,337 @@
> >>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >>>> +/*
> >>>> + * Copyright (C) 2019, Google Inc.
> >>>> + *
> >>>> + * v4l2_controls.cpp - V4L2 Extended Control Support
> >>>> + */
> >>>> +
> >>>> +#include "v4l2_controls.h"
> >>>> +
> >>>> +/**
> >>>> + * \file v4l2_controls.h
> >>>> + * \brief Support for V4L2 Controls using the V4L2 Extended Controls APIs.
> >>>> + *
> >>>> + * The V4L2 defined "Control API" allows application to inspect and modify set
> >>>> + * of configurable parameters on the video device or subdevice of interest. The
> >>>> + * nature of the parameters an application could modify using the control
> >>>> + * 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.
> >>>> + *
> >>>> + * Controls are identified by a numerical id, defined by the V4L2 kernel headers
> >>>> + * and have an associated type and class. Each control has a 'value', which is
> >>>
> >>> I would drop ' and class', we don't care about control classes.
> >>>
> >>>> + * the data that can be modified with a call to  the 'V4L2Device::setControls()'
> >>>
> >>> s/  / /
> >>>
> >>>> + * operation or retrieved with a call to 'V4L2Device::getControls()'.
> >>>
> >>> I don't think you need to enclose the method name in quotes, you can
> >>> write
> >>>
> >>>  * Each control has a value, which is the data that can be modified with
> >>>  * V4L2Device::setControls() or retrieved with V4L2Device::getControls().
> >>>
> >>>> + *
> >>>> + * A control class defines the control purpose while its type (along with the
> >>>
> >>> Similarly, you can drop the class here.
> >>>
> >>>> + * control's flags) defines the type of the control's value content. Controls
> >>>> + * might transport a single data value stored in variable inside the control, or
> >>>> + * they might as well deal with more 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.
> >>>
> >>> s/Libcamera/libcamera/
> >>>
> >>>> + *
> >>>> + * \todo Add support for compound controls
> >>>> + *
> >>>> + * Libcamera implements support for control using the V4L2 'Extended Control'
> >>>
> >>> s/Libcamera/libcamera/
> >>> s/control/controls/
> >>>
> >>>> + * framework, which allows easier future handling of controls with payloads
> >>>
> >>> s/framework/API/
> >>> s/easier //
> >>>
> >>>> + * of arbitrary sizes.
> >>>> + *
> >>>> + * The Libcamera V4L2 Controls framework operates on lists of controls, wrapped
> >>>
> >>> s/Libcamera/libcamera/
> >>>
> >>>> + * by the V4L2Controls class, to match the V4L2 extended controls framework.
> >>>
> >>> s/framework/API/
> >>>
> >>>> + * The interface to set and get control is implemented by the V4L2Device class,
> >>>> + * and this file only provides the data type definitions.
> >>>> + */
> >>>> +
> >>>> +namespace libcamera {
> >>>> +
> >>>> +/**
> >>>> + * \class V4L2ControlInfo
> >>>> + * \brief Information on a V4L2 control
> >>>> + *
> >>>> + * The V4L2ControlInfo class represent all the informations relative to
> >>>
> >>> s/informations/information/ (uncoutable)
> >>> s/relative/related/
> >>>
> >>>> + * 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 field of
> >>>> + * a struct v4l2_query_ext_ctrl structure, after it has been filled by the
> >>>> + * device driver as a consequence of an VIDIOC_QUERY_EXT_CTRL ioctl call.
> >>>
> >>> s/of an/of a/
> >>>
> >>>> + *
> >>>> + * This class does not contain the control value, but only static informations
> >>>
> >>> s/informations/information/
> >>>
> >>>> + * on the control, which should ideally be cached the first time the control
> >>>
> >>> I'd be a bit more directive here, "which shall be cached by the caller
> >>> at initialisation time or the first time the control information is
> >>> needed." and drop the rest of the sentence.
> >>>
> >>>> + * is accessed to be reused each time the control value need to be read or
> >>>
> >>> s/need/needs/
> >>>
> >>>> + * applied to the hardware.
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \brief Construct a V4L2ControlInfo from a struct v4l2_query_ext_ctrl
> >>>> + */
> >>>> +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 this instance refers to
> >>>
> >>> I would just say "Retrieve the control ID"
> >>>
> >>>> + * \return The V4L2 control id
> >>>
> >>> s/id/ID/ (as well as in other locations below)
> >>>
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \fn V4L2ControlInfo::type()
> >>>> + * \brief Retrieve the control type as defined by V4L2_CTRL_TYPE_
> >>>
> >>> Maybe 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 or a
> >>>> + * call to V4L2Controls::set().
> >>>> + *
> >>>> + * 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 the set and get control operations implemented in
> >>>> + * V4L2Device::setControls() and V4L2Device::setControls() respectively.
> >>>
> >>> The second one should be V4L2Device::getControls(), or you could write
> >>>
> >>> "passed as parameter to V4L2Device::getControls() and
> >>> V4L2Device::setControls()."
> >>>
> >>>> + *
> >>>> + * In facts, access to the class constructor and to the control value accessor
> >>>> + * and modifier are restricted to the friend V4L2Controls and V4L2Device
> >>>> + * classes.
> >>>
> >>> I think you can drop this paragraph.
> >>>
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \fn V4L2Control::id()
> >>>> + * \brief Retrieve the control id this instance refers to
> >>>> + * \return The V4L2Control id
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \class V4L2Controls
> >>>> + * \brief Wraps a list of V4L2Control
> >>>
> >>> I would say "Container of V4L2Control instance" and replace wrapper by
> >>> container below.
> >>>
> >>>> + *
> >>>> + * The V4L2Controls class works as a wrapper 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
> >>>
> >>> s/Libcamera/libcamera/
> >>>
> >>>> + * 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).
> >>>> + *
> >>>> + * 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 should be added
> >>>
> >>> s/should/shall/
> >>>
> >>>> + * with an initial value by calling V4L2Controls::add(unsigned int id, long
> >>>> + * int value) to prepare for a write operation. The value of controls which
> >>>> + * are already part of the instance could be updated with a call to
> >>>> + * V4L2Controls::set() operation.
> >>>
> >>> Is there a use case for this ?
> >>
> >> See the above example.
> >>
> >>>> Once the values of all controls of interest
> >>>> + * have been initialized in a V4L2Controls instance, this should be then
> >>>> + * passed to the V4L2Device::setControls() operation, which applies each
> >>>> + * control in the list to the hardware.
> >>>
> >>> Once all desired control have been added, the V4L2Controls instance is
> >>> passed to V4L2Device::setControls(), which sets the controls on the
> >>> device.
> >>>
> >>>> + *
> >>>> + * Alternatively a control can be add without any initial value by calling
> >>>> + * V4L2Controls::add(unsigned int id) and then read from the device passing
> >>>> + * the V4L2Controls instance to V4L2Device::getControls() which read each
> >>>> + * control value from the hardware.
> >>>> + *
> >>>> + * Reading controls with an initialized value from the device, overwrites the
> >>>> + * control's value, reflecting what has been actually read from the hardware.
> >>>
> >>> Let's write this like the previous paragraph.
> >>>
> >>> "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 it contains and
> >>>
> >>> s/it contains/they contain/
> >>>
> >>>> + * prepare to be re-used for a new control write/read sequence. Alternatively,
> >>>> + * the value of a control already part of the instance could be updated by using
> >>>> + * the V4L2Controls::set() method, to avoid going through a reset() when a
> >>>> + * control has to be read then written to the hardware in sequence.
> >>>
> >>> Given how set() is implemented, creating a new V4L2Controls instance
> >>> could be cheaper. Do we really want to reuse V4L2Controls instances with
> >>> set() ? If so I think we need to think about the use cases, and decide
> >>> how to implement it in an efficient way. Alternatively, I would start
> >>> simple and extend the API later.
> >>
> >> So how would you re-use the same V4L2Controls instance for a
> >> getControls()/setControls() sequence? Go through a reset and re-add
> >> all controls?
> >
> > Do we need that at all ? If you have a V4L2Controls instance with 10
> > controls, call getControls(), check the values and update two of them,
> > and then call setControls(), all 10 controls will be written to the
> > driver. Wouldn't it be better to create a second V4L2Controls instance
> > for the setControls() operation that would be populated with just the
> > two controls that need to be set ? I also expect those read-modify-write
> 
> Or possibly "reset()" the existing one.

Yes, but I would expect the caller to populate the write set while
iterating over the read set.

> > sequences to be quite uncommon, as I believe getControls() will be used
> > mainly at initialisation time, at runtime to read volatile controls
> > (e.g. what was the real exposure value or lens position) or to read
> > controls that have changed after receiving a V4L2 control change event.
> >
> >>>> + *
> >>>> + * In facts, the following pseudo-code sequences lead to the same result:
> >>>
> >>> s/In facts, the/The/
> >>>
> >>>> + *
> >>>> + * V4L2Controls controls;
> >>>> + * controls.add(V4L2_CID_xx);
> >>>> + * dev->getControls(&controls);
> >>>> + * controls.set(V4L2_CID_xx, value);
> >>>> + * dev->setControls(&controls);
> >>>> + *
> >>>> + * V4L2Controls controls;
> >>>> + * controls.add(V4L2_CID_xx);
> >>>> + * dev->getControls(&controls);
> >>>> + * controls.reset();
> >>>> + * controls.add(V4L2_CID_xx, value);
> >>>> + * dev->setControls(&controls);
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \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
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \fn V4L2Controls::operator[](unsigned int index)
> >>>> + * \brief Access the control at index \a index
> >>>> + * \param index The index to access
> >>>
> >>> \return ?
> >>>
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \brief Add control \a id with a \a value to the instance
> >>>> + * \param id The V4L2 control id (V4L2_CID_xx)
> >>>
> >>> s/control id/control ID/
> >>> s/V4L2_CID_xx/V4L2_CID_*/
> >>>
> >>>> + * \param value The V4L2 control value
> >>>> + */
> >>>> +void V4L2Controls::add(unsigned int id, long int value)
> >>>> +{
> >>>> +	/* Cannot use emplace_back() as the V4L2Control ctor is private. */
> >>>> +	V4L2Control ctrl(id, value);
> >>>> +	controls_.push_back(ctrl);
> >>>> +}
> >>>> +
> >>>> +/**
> >>>> + * \brief Add control \a id without an initial value  to the instance
> >>>
> >>> s/  / /
> >>>
> >>>> + * \param id The V4L2 control id (V4L2_CID_xx)
> >>>
> >>> s/control id/control ID/
> >>> s/V4L2_CID_xx/V4L2_CID_*/
> >>>
> >>> Same in a few locations below.
> >>
> >> Thanks for all the comments in documentation, I'll take them in.
> >>
> >>>> + */
> >>>> +void V4L2Controls::add(unsigned int id)
> >>>> +{
> >>>> +	/* Cannot use emplace_back() as the V4L2Control ctor is private. */
> >>>
> >>> Would it make sense to make the constructor public then ? You have
> >>> documented extensively that V4L2Control isn't supposed to be
> >>> instantiated directly by the application, and there will be no way to
> >>> use a directly instantiated V4L2Control anway, so things should be fine.
> >>>
> >>
> >> If the only price to pay is not to use emplace_back() I don't think
> >> so.
> >>
> >> Shouldn't the following code be equally efficient than emplace_back()?
> >> Isn't the new instance constructed inside the container directly
> >> instead of copied? (emplace_back would just be a syntactic sugar in
> >> that case).
> >
> > The code below makes use of the copy constructor, while emplace_back
> > constructs the new instance in-place.
> >
> 
> Ok, I'll make the constructor accessible. It was for the V4L2Controls
> friend call, but in this case it needs to be public as it's
> std::vector that tries to construct it.

You could also make std::vector<V4L2Control> a friend, but then anyone
could construct a vector of V4L2Control, essentially giving a public way
to construct a V4L2Control.

> >>>> +	V4L2Control ctrl(id);
> >>>> +	controls_.push_back(ctrl);
> >>>> +}
> >>>> +
> >>>> +/**
> >>>> + * \brief Retrieve the value of the control with \a id
> >>>> + * \param id The V4L2 control id (V4L2_CID_xx)
> >>>> + *
> >>>> + * Retrieve the value of the control with id \a id.
> >>>> + *
> >>>> + * It is important to notice that the returned value is not read from the
> >>>> + * 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() or set() operation.
> >>>> + *
> >>>> + * \return The V4L2 control value or a negative error code if the control id
> >>>> + * is not part of this instance
> >>>> + */
> >>>> +int V4L2Controls::get(unsigned int id)
> >>>> +{
> >>>> +	V4L2Control *ctrl = getControl(id);
> >>>> +	if (!ctrl)
> >>>> +		return -EINVAL;
> >>>> +
> >>>> +	return ctrl->value();
> >>>> +}
> >>>> +
> >>>> +/**
> >>>> + * \brief Set the value of the control with \a id
> >>>> + * \param id The V4L2 control id (V4L2_CID_xx)
> >>>> + * \param value The new V4L2 control value
> >>>> + *
> >>>> + * Set the value of the control with id \a id.
> >>>> + *
> >>>> + * 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 pplied only by calling V4L2Device::setControls().
> >>>> + *
> >>>> + * \return 0 on success or a negative error code if the control id
> >>>> + * is not part of this instance
> >>>> + */
> >>>> +int V4L2Controls::set(unsigned int id, long int value)
> >>>> +{
> >>>> +	V4L2Control *ctrl = getControl(id);
> >>>> +	if (!ctrl)
> >>>> +		return -EINVAL;
> >>>> +
> >>>> +	ctrl->setValue(value);
> >>>> +
> >>>> +	return 0;
> >>>> +}
> >>>> +
> >>>> +/*
> >>>> + * \brief Get a pointer the control with \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 */