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

[Laurent Pinchart]

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 at 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 */

-- 
Regards,

Laurent Pinchart