[libcamera-devel,3/6] libcamera: Add V4L2Controls

1332 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 | 147 +++++++++++++
 src/libcamera/meson.build             |   1 +
 src/libcamera/v4l2_controls.cpp       | 301 ++++++++++++++++++++++++++
 3 files changed, 449 insertions(+)
 create mode 100644 src/libcamera/include/v4l2_controls.h
 create mode 100644 src/libcamera/v4l2_controls.cpp

Hi Jacopo,

On 02/06/2019 14:04, 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.


I haven't done a full documentation review here, I'm just trying to
focus on the code parts first.

--
KB

> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/include/v4l2_controls.h | 147 +++++++++++++
>  src/libcamera/meson.build             |   1 +
>  src/libcamera/v4l2_controls.cpp       | 301 ++++++++++++++++++++++++++
>  3 files changed, 449 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..df955c2e10b2
> --- /dev/null
> +++ b/src/libcamera/include/v4l2_controls.h
> @@ -0,0 +1,147 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * v4l2_controls.h - V4L2 Extended Control Support
> + */
> +
> +#ifndef __LIBCAMERA_V4L2_CONTROL_H__
> +#define __LIBCAMERA_V4L2_CONTROL_H__
> +
> +#include <cstring>
> +
> +#include <linux/v4l2-controls.h>
> +#include <linux/videodev2.h>
> +
> +namespace libcamera {
> +
> +class V4L2Control
> +{
> +public:
> +	virtual ~V4L2Control()
> +	{
> +	}
> +
> +	unsigned int id() { return id_; }
> +	unsigned int size() { return size_; }
> +	enum v4l2_ctrl_type type() const { return type_; }
> +
> +protected:
> +	V4L2Control(unsigned int id, unsigned int size,
> +		    enum v4l2_ctrl_type type)
> +		: id_(id), size_(size), type_(type)
> +	{
> +	}
> +
> +private:
> +	unsigned int id_;
> +	unsigned int size_;
> +	enum v4l2_ctrl_type type_;
> +};
> +
> +template<typename T>
> +class V4L2ControlValue : public V4L2Control

Interesting, I thought about templating to do the types for the
Libcamera Controls, but instead ended up embedding the type in the
object to avoid having handle casting things ...

Although that then leads me to require having a getBool, getInteger,
getXXX so we can evaluate both options side by side, and perhaps then
unify the model for both.

(I'm hoping to get some patches posted soon which will show the
difference in approach)

The templating might organise memory better, as in my implementation I
can add a bool, and int to a union, but not a std::string object, so I
incur wasted space.


> +{
> +public:
> +	T value() const { return value_; }
> +	T *mem() const { return memvalue_; }

s/memvalue_/mem_/? or /mem()/memvalue()/?

(I prefer mem())

> +
> +protected:
> +	V4L2ControlValue(unsigned int id, unsigned int size,
> +			 enum v4l2_ctrl_type type, T value)
> +		: V4L2Control(id, size, type)
> +	{
> +		value_ = value;
> +		memvalue_ = nullptr;
> +	}
> +
> +	V4L2ControlValue(unsigned int id, unsigned int size,
> +			 enum v4l2_ctrl_type type, T *value)
> +		: V4L2Control(id, size, type)
> +	{
> +		value_ = 0;
> +		memvalue_ = static_cast<T *>(new T[size]);
> +		memcpy(memvalue_, value, size);
> +	}
> +
> +	~V4L2ControlValue()
> +	{
> +		delete[] memvalue_;
> +	}
> +
> +private:
> +	T value_;
> +	T *memvalue_;
> +};
> +
> +class V4L2IntControl : public V4L2ControlValue<int32_t>
> +{
> +public:
> +	V4L2IntControl(unsigned int id, int32_t value)
> +		: V4L2ControlValue<int32_t>(id, sizeof(int32_t),
> +					    V4L2_CTRL_TYPE_INTEGER, value)
> +	{
> +	}
> +};
> +
> +class V4L2Int64Control : public V4L2ControlValue<int64_t>
> +{
> +public:
> +	V4L2Int64Control(unsigned int id, int64_t value)
> +		: V4L2ControlValue<int64_t>(id, sizeof(int64_t),
> +					    V4L2_CTRL_TYPE_INTEGER64, value)
> +	{
> +	}
> +};
> +
> +class V4L2BoolControl : public V4L2ControlValue<bool>
> +{
> +public:
> +	V4L2BoolControl(unsigned int id, bool value)
> +		: V4L2ControlValue<bool>(id, sizeof(bool),
> +					 V4L2_CTRL_TYPE_BOOLEAN, value)
> +	{
> +	}
> +};
> +
> +class V4L2StringControl : public V4L2ControlValue<std::string>

You might want/need to to support char* types too.


> +{
> +public:
> +	V4L2StringControl(unsigned int id, std::string value)
> +		: V4L2ControlValue<std::string>(id, value.length(),
> +						V4L2_CTRL_TYPE_STRING, value)
> +	{
> +	}
> +};
> +
> +class V4L2U8Control : public V4L2ControlValue<uint8_t>
> +{
> +public:
> +	V4L2U8Control(unsigned int id, unsigned int size, uint8_t *mem)
> +		: V4L2ControlValue<uint8_t>(id, size, V4L2_CTRL_TYPE_U8, mem)
> +	{
> +	}
> +};
> +
> +class V4L2U16Control : public V4L2ControlValue<uint16_t>
> +{
> +public:
> +	V4L2U16Control(unsigned int id, unsigned int size, uint16_t *mem)
> +		: V4L2ControlValue<uint16_t>(id, size, V4L2_CTRL_TYPE_U16, mem)
> +	{
> +	}
> +};
> +
> +class V4L2U32Control : public V4L2ControlValue<uint32_t>
> +{
> +public:
> +	V4L2U32Control(unsigned int id, unsigned int size, uint32_t *mem)
> +		: V4L2ControlValue<uint32_t>(id, size, V4L2_CTRL_TYPE_U32, mem)
> +	{
> +	}
> +};
> +
> +}; /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_V4L2_CONTROL_H__ */
> +
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 6d858a22531e..fa1fbcb5faf5 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -22,6 +22,7 @@ libcamera_sources = files([
>      'timer.cpp',
>      'utils.cpp',
>      'v4l2_base.cpp',
> +    'v4l2_controls.cpp',
>      'v4l2_device.cpp',
>      'v4l2_subdevice.cpp',
>  ])
> diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> new file mode 100644
> index 000000000000..f50f3cdfa7d7
> --- /dev/null
> +++ b/src/libcamera/v4l2_controls.cpp
> @@ -0,0 +1,301 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * v4l2_controls.cpp - V4L2 Extended Control Support
> + */
> +
> +/**
> + * \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

"an application" or "applications"

"a set of"

application? Are we talking about applications that use libcamera? Or
applications in general?


> + * of configurable parameters on the video device or subdevice of interest.  The

/.  The/. The/ <double space>



> + * 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 'setControl()' operation call or
> + * retrieved with a 'getControl()' one.
> + *
> + * 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
> + * matrixes, stored in a contiguous memory locations associated with the control

s/matrixes/matrices/

> + * and called 'the payload'. The content have to be opportunely copied into the
> + * application memory when retrieving a control's value and provided to the V4L2
> + * device when setting it.
> + *
> + * Libcamera supports control using the V4L2 'Extended Control' framework, which
> + * allows easier handling of controls with payloads of arbitrary sizes.
> + *
> + * The Libcamera V4L2 Controls framework operates on lists of controls, to match
> + * the V4L2 extended controls framework, but also provides operations to get
> + * and set single controls. The interface to set and get control is implemented
> + * by the V4L2Base class, while this file only provides the data type
> + * definitions.
> + *
> + * The Libcamera V4L2 control framework data types define a base class
> + * V4L2Control that contains the fields common to all controls, regardless of
> + * their type, such as the  control's id, its type and the expected control's
> + * value data size. The V4L2Control class is not meant to be instantiated
> + * directly, but is instead used as placeholder to store controls in the
> + * standard library provided containers.
> + *
> + * A parametric derived class V4L2ControlValue stores the control's value and
> + * provide accessors to the value itself for control with no payload, or to the
> + * memory area that contains the control's data payload. This class is not
> + * intended to be directly used and cannot be constructed, but it is used
> + * instead to define specialized derived classes which specialize the control's
> + * data value type.
> + *
> + * In order to set and get controls, user of the Libcamera V4L2 control
> + * framework should create instances of the specialized derived classes,
> + * which are publicly constructible, and use them to access the control's data
> + * content using the 'value()' method for controls with no payload, or
> + * retrieving reference to the memory location that contains the data payload
> + * with the 'mem()' operation for controls which transport a data payload.
> + *
> + * \todo Support setting controls with data payload.
> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \class V4L2Control
> + * \brief Base class for V4L2 Controls
> + *
> + * The V4L2Control base class is the base class that contains the fields common
> + * to all controls (id, type and size).
> + *
> + * This class is not meant to be instantiated directly, but is instead used as a
> + * place holder to store controls in arrays or other containers. User of the
> + * libcamera V4L2 control framework should access the controls content by
> + * instantiating one of the provided specialized derived classes.
> + */
> +
> +/**
> + * \fn V4L2Control::V4L2Control
> + * \brief Construct a V4L2Control instance
> + * \param id The control's id
> + * \param size The control's data size
> + * \param type The control's type
> + */
> +
> +/**
> + * \fn V4L2Control::id()
> + * \brief Retrieve the control's id
> + *
> + * Retrieve the control's numerical id value as defined by the V4L2
> + * specification.
> + *
> + * \return The control's id
> + */
> +
> +/**
> + * \fn V4L2Control::size()
> + * \brief Retrieve the control's data value size in bytes
> + *
> + * \todo Better define the value of size() for controls with payload data.
> + *
> + * \return The control's size
> + */
> +
> +/**
> + * \fn V4L2Control::type()
> + * \brief Retrieve the control's type
> + *
> + * Retrieve the control's type as defined by the V4L2 specification.
> + *
> + * \return The control's type
> + */
> +
> +/**
> + * \class V4L2ControlValue
> + * \brief Template base class that represent values of a V4L2 control
> + *
> + * The V4L2ControlValue template base class represents a V4L2 control with
> + * its different value types.
> + *
> + * It provides two operations to access the control's value or the pointer
> + * to the memory location that contains to the control's value data payload.
> + */
> +
> +/**
> + * \fn V4L2ControlValue::V4L2ControlValue(unsigned int id, unsigned int size, enum v4l2_ctrl_type type, T value)
> + * \brief Contruct a V4L2ControlValue with a value

/Contruct/Construct/

> + * \param id The control's id
> + * \param size The control's size
> + * \param type The control's type
> + * \param value The control's value
> + */
> +
> +/**
> + * \fn V4L2ControlValue::V4L2ControlValue(unsigned int id, unsigned int size, enum v4l2_ctrl_type type, T* value)
> + * \brief Contruct a V4L2ControlValue with a pointer to a data payload

/Contruct/Construct/

> + * \param id The control's id
> + * \param size The control's size
> + * \param type The control's type
> + * \param value The pointer to the control's data payload
> + */
> +
> +/**
> + * \fn V4L2ControlValue::~V4L2ControlValue
> + * \brief Release the memory reserved for the control's data payload, if any
> + */
> +
> +/**
> + * \fn V4L2ControlValue::value()
> + * \brief Retrieve the control's value
> + *
> + * Retrieve the control's value. Valid only for controls with no payload.
> + * Access a control's value by calling the value() operation on instances
> + * of the specialized derived classes.
> + */
> +
> +/**
> + * \fn V4L2ControlValue::mem()
> + * \brief Retrieve a pointer to the memory location of the control's data
> + *
> + * Retrieve a pointer to the memory location that contains the control's data
> + * payload. Valid only for controls with data payload. Access the memory
> + * location of the control's data by calling the mem() operation on instances
> + * of the specialized derived classes.
> + */
> +
> +/**
> + * \class V4L2IntControl
> + * \brief Specialized V4L2Control class that handles controls of
> + *        V4L2_CTRL_TYPE_INTEGER type.
> + *
> + * Access the control's data value by using the value() operation.
> + */
> +
> +/**
> + * \fn V4L2IntControl::V4L2IntControl()
> + * \brief Construct a V4L2Control that contains an int value
> + * \param id The control's id
> + * \param value The control's value
> + */
> +
> +/**
> + * \class V4L2Int64Control
> + * \brief Specialized V4L2Control class that handles controls of
> + *        V4L2_CTRL_TYPE_INTEGER64 type.
> + *
> + * Access the control's data value by using the value() operation.
> + */
> +
> +/**
> + * \fn V4L2Int64Control::V4L2Int64Control()
> + * \brief Construct a V4L2Control that contains an int64 value
> + * \param id The control's id
> + * \param value The control's value
> + */
> +
> +/**
> + * \class V4L2BoolControl
> + * \brief Specialized V4L2Control class that handles controls of
> + *        V4L2_CTRL_TYPE_BOOLEAN type.
> + *
> + * Access the control's data value by using the value() operation.
> + */
> +
> +/**
> + * \fn V4L2BoolControl::V4L2BoolControl()
> + * \brief Construct a V4L2Control that contains a boolean value
> + * \param id The control's id
> + * \param value The control's value
> + */
> +
> +/**
> + * \class V4L2StringControl
> + * \brief Specialized V4L2Control class that handles controls of
> + *        V4L2_CTRL_TYPE_STRING type.
> + *
> + * Access the control's data value by using the value() operation.
> + */
> +
> +/**
> + * \fn V4L2StringControl::V4L2StringControl()
> + * \brief Construct a V4L2Control that contains a string value
> + * \param id The control's id
> + * \param value The control's value
> + */
> +
> +/**
> + * \class V4L2U8Control
> + * \brief Specialized V4L2Control class that handles controls with payload of
> + *        V4L2_CTRL_TYPE_U8 type.
> + *
> + * Access the control's data value by using the retrieving the memory location
> + * where the data payload is stored with the mem() operation. The size of
> + * the payload data can be retrieved with the size() operation.
> + */
> +
> +/**
> + * \fn V4L2U8Control::V4L2U8Control()
> + * \brief Construct a V4L2Control with payload of uin8_t data
> + * \param id The control's id
> + * \param size The size in bytes of the payload content
> + * \param mem Pointer to the memory location of the payload content
> + *
> + * Memory is reserved in the newly created instance to hold the data payload
> + * and the data content is copied there. The reserved memory is then freed when
> + * the Control is destroyed. The memory where the control's payload was copied
> + * from should be released by the caller.
> + */
> +
> +/**
> + * \class V4L2U16Control
> + * \brief Specialized V4L2Control class that handles controls with payload of
> + *        V4L2_CTRL_TYPE_U16 type.
> + *
> + * Access the control's data value by using the retrieving the memory location
> + * where the data payload is stored with the mem() operation. The size of
> + * the payload data can be retrieved with the size() operation.
> + */
> +
> +/**
> + * \fn V4L2U16Control::V4L2U16Control(unsigned int id, unsigned int size, uint16_t *mem)
> + * \brief Construct a V4L2Control with payload of uin16_t data
> + * \param id The control's id
> + * \param size The size in bytes of the payload content
> + * \param mem Pointer to the memory location of the payload content
> + *
> + * Memory is reserved in the newly created instance to hold the data payload
> + * and the data content is copied there. The reserved memory is then freed when
> + * the Control is destroyed. The memory where the control's payload was copied
> + * from should be released by the caller.
> + */
> +
> +/**
> + * \class V4L2U32Control
> + * \brief Specialized V4L2Control class that handles controls with payload of
> + *        V4L2_CTRL_TYPE_U32 type.
> + *
> + * Access the control's data value by using the retrieving the memory location
> + * where the data payload is stored with the mem() operation. The size of
> + * the payload data can be retrieved with the size() operation.
> + */
> +
> +/**
> + * \fn V4L2U32Control::V4L2U32Control(unsigned int id, unsigned int size, uint32_t *mem)
> + * \brief Construct a V4L2Control with payload of uin32_t data
> + * \param id The control's id
> + * \param size The size in bytes of the payload content
> + * \param mem Pointer to the memory location of the payload content
> + *
> + * Memory is reserved in the newly created instance to hold the data payload
> + * and the data content is copied there. The reserved memory is then freed when
> + * the Control is destroyed. The memory where the control's payload was copied
> + * from should be released by the caller.
> + */
> +
> +}; /* namespace libcamera */
>

Hello Jacopo,

Thank you for the patch.

On Mon, Jun 03, 2019 at 10:47:00AM +0100, Kieran Bingham wrote:
> On 02/06/2019 14:04, 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.
> 
> 
> I haven't done a full documentation review here, I'm just trying to
> focus on the code parts first.
> 
> --
> KB
> 
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/libcamera/include/v4l2_controls.h | 147 +++++++++++++
> >  src/libcamera/meson.build             |   1 +
> >  src/libcamera/v4l2_controls.cpp       | 301 ++++++++++++++++++++++++++
> >  3 files changed, 449 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..df955c2e10b2
> > --- /dev/null
> > +++ b/src/libcamera/include/v4l2_controls.h
> > @@ -0,0 +1,147 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * v4l2_controls.h - V4L2 Extended Control Support
> > + */
> > +
> > +#ifndef __LIBCAMERA_V4L2_CONTROL_H__
> > +#define __LIBCAMERA_V4L2_CONTROL_H__

s/CONTROL/CONTROLS/

> > +
> > +#include <cstring>
> > +
> > +#include <linux/v4l2-controls.h>
> > +#include <linux/videodev2.h>
> > +
> > +namespace libcamera {
> > +
> > +class V4L2Control
> > +{
> > +public:
> > +	virtual ~V4L2Control()
> > +	{
> > +	}
> > +
> > +	unsigned int id() { return id_; }
> > +	unsigned int size() { return size_; }
> > +	enum v4l2_ctrl_type type() const { return type_; }
> > +
> > +protected:
> > +	V4L2Control(unsigned int id, unsigned int size,
> > +		    enum v4l2_ctrl_type type)
> > +		: id_(id), size_(size), type_(type)
> > +	{
> > +	}
> > +
> > +private:
> > +	unsigned int id_;
> > +	unsigned int size_;
> > +	enum v4l2_ctrl_type type_;
> > +};
> > +
> > +template<typename T>
> > +class V4L2ControlValue : public V4L2Control
> 
> Interesting, I thought about templating to do the types for the
> Libcamera Controls, but instead ended up embedding the type in the
> object to avoid having handle casting things ...
> 
> Although that then leads me to require having a getBool, getInteger,
> getXXX so we can evaluate both options side by side, and perhaps then
> unify the model for both.
> 
> (I'm hoping to get some patches posted soon which will show the
> difference in approach)
> 
> The templating might organise memory better, as in my implementation I
> can add a bool, and int to a union, but not a std::string object, so I
> incur wasted space.

The trouble with the template is that it requires passing containers of
V4L2Control pointers, making it complicated for the caller (especially
when deleting all the elements).

An alternative would be to create a V4L2Controls class that would wrap
around the container, and provide accessors to get/set values, and to
iterate over controls. How about the following pseudo-code from the
point of view of the caller ?

	V4L2Device *dev = ...;
	V4L2Controls ctrls;

	ctrls.set(V4L2_CID_EXPOSURE, 42);
	ctrls.set(V4L2_CID_GAIN, 100);

	dev->setControls(&ctrls);

and

	V4L2Device *dev = ...;
	V4L2Controls ctrls;

	ctrls.get(V4L2_CID_EXPOSURE);
	ctrls.get(V4L2_CID_GAIN);

	dev->getControls(&ctrls);

	unsigned int exposure = ctrls.value(V4L2_CID_EXPOSURE);

It's important to make the API easy to use for pipeline handlers and
efficient in terms of both CPU time and memory. I expect tradeoffs of
course :-)

> > +{
> > +public:
> > +	T value() const { return value_; }
> > +	T *mem() const { return memvalue_; }
> 
> s/memvalue_/mem_/? or /mem()/memvalue()/?
> 
> (I prefer mem())
> 
> > +
> > +protected:
> > +	V4L2ControlValue(unsigned int id, unsigned int size,
> > +			 enum v4l2_ctrl_type type, T value)
> > +		: V4L2Control(id, size, type)
> > +	{
> > +		value_ = value;
> > +		memvalue_ = nullptr;
> > +	}
> > +
> > +	V4L2ControlValue(unsigned int id, unsigned int size,
> > +			 enum v4l2_ctrl_type type, T *value)
> > +		: V4L2Control(id, size, type)
> > +	{
> > +		value_ = 0;
> > +		memvalue_ = static_cast<T *>(new T[size]);
> > +		memcpy(memvalue_, value, size);
> > +	}
> > +
> > +	~V4L2ControlValue()
> > +	{
> > +		delete[] memvalue_;
> > +	}
> > +
> > +private:
> > +	T value_;
> > +	T *memvalue_;
> > +};
> > +
> > +class V4L2IntControl : public V4L2ControlValue<int32_t>
> > +{
> > +public:
> > +	V4L2IntControl(unsigned int id, int32_t value)
> > +		: V4L2ControlValue<int32_t>(id, sizeof(int32_t),
> > +					    V4L2_CTRL_TYPE_INTEGER, value)
> > +	{
> > +	}
> > +};
> > +
> > +class V4L2Int64Control : public V4L2ControlValue<int64_t>
> > +{
> > +public:
> > +	V4L2Int64Control(unsigned int id, int64_t value)
> > +		: V4L2ControlValue<int64_t>(id, sizeof(int64_t),
> > +					    V4L2_CTRL_TYPE_INTEGER64, value)
> > +	{
> > +	}
> > +};
> > +
> > +class V4L2BoolControl : public V4L2ControlValue<bool>
> > +{
> > +public:
> > +	V4L2BoolControl(unsigned int id, bool value)
> > +		: V4L2ControlValue<bool>(id, sizeof(bool),
> > +					 V4L2_CTRL_TYPE_BOOLEAN, value)
> > +	{
> > +	}
> > +};
> > +
> > +class V4L2StringControl : public V4L2ControlValue<std::string>
> 
> You might want/need to to support char* types too.
> 
> > +{
> > +public:
> > +	V4L2StringControl(unsigned int id, std::string value)
> > +		: V4L2ControlValue<std::string>(id, value.length(),
> > +						V4L2_CTRL_TYPE_STRING, value)
> > +	{
> > +	}
> > +};
> > +
> > +class V4L2U8Control : public V4L2ControlValue<uint8_t>
> > +{
> > +public:
> > +	V4L2U8Control(unsigned int id, unsigned int size, uint8_t *mem)
> > +		: V4L2ControlValue<uint8_t>(id, size, V4L2_CTRL_TYPE_U8, mem)
> > +	{
> > +	}
> > +};
> > +
> > +class V4L2U16Control : public V4L2ControlValue<uint16_t>
> > +{
> > +public:
> > +	V4L2U16Control(unsigned int id, unsigned int size, uint16_t *mem)
> > +		: V4L2ControlValue<uint16_t>(id, size, V4L2_CTRL_TYPE_U16, mem)
> > +	{
> > +	}
> > +};
> > +
> > +class V4L2U32Control : public V4L2ControlValue<uint32_t>
> > +{
> > +public:
> > +	V4L2U32Control(unsigned int id, unsigned int size, uint32_t *mem)
> > +		: V4L2ControlValue<uint32_t>(id, size, V4L2_CTRL_TYPE_U32, mem)
> > +	{
> > +	}
> > +};
> > +
> > +}; /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_V4L2_CONTROL_H__ */
> > +
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index 6d858a22531e..fa1fbcb5faf5 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -22,6 +22,7 @@ libcamera_sources = files([
> >      'timer.cpp',
> >      'utils.cpp',
> >      'v4l2_base.cpp',
> > +    'v4l2_controls.cpp',
> >      'v4l2_device.cpp',
> >      'v4l2_subdevice.cpp',
> >  ])
> > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> > new file mode 100644
> > index 000000000000..f50f3cdfa7d7
> > --- /dev/null
> > +++ b/src/libcamera/v4l2_controls.cpp
> > @@ -0,0 +1,301 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * v4l2_controls.cpp - V4L2 Extended Control Support
> > + */
> > +
> > +/**
> > + * \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
> 
> "an application" or "applications"
> 
> "a set of"
> 
> application? Are we talking about applications that use libcamera? Or
> applications in general?
> 
> 
> > + * of configurable parameters on the video device or subdevice of interest.  The
> 
> /.  The/. The/ <double space>
> 
> 
> 
> > + * 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 'setControl()' operation call or
> > + * retrieved with a 'getControl()' one.
> > + *
> > + * 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
> > + * matrixes, stored in a contiguous memory locations associated with the control
> 
> s/matrixes/matrices/
> 
> > + * and called 'the payload'. The content have to be opportunely copied into the
> > + * application memory when retrieving a control's value and provided to the V4L2
> > + * device when setting it.
> > + *
> > + * Libcamera supports control using the V4L2 'Extended Control' framework, which
> > + * allows easier handling of controls with payloads of arbitrary sizes.
> > + *
> > + * The Libcamera V4L2 Controls framework operates on lists of controls, to match
> > + * the V4L2 extended controls framework, but also provides operations to get
> > + * and set single controls. The interface to set and get control is implemented
> > + * by the V4L2Base class, while this file only provides the data type
> > + * definitions.
> > + *
> > + * The Libcamera V4L2 control framework data types define a base class
> > + * V4L2Control that contains the fields common to all controls, regardless of
> > + * their type, such as the  control's id, its type and the expected control's
> > + * value data size. The V4L2Control class is not meant to be instantiated
> > + * directly, but is instead used as placeholder to store controls in the
> > + * standard library provided containers.
> > + *
> > + * A parametric derived class V4L2ControlValue stores the control's value and
> > + * provide accessors to the value itself for control with no payload, or to the
> > + * memory area that contains the control's data payload. This class is not
> > + * intended to be directly used and cannot be constructed, but it is used
> > + * instead to define specialized derived classes which specialize the control's
> > + * data value type.
> > + *
> > + * In order to set and get controls, user of the Libcamera V4L2 control
> > + * framework should create instances of the specialized derived classes,
> > + * which are publicly constructible, and use them to access the control's data
> > + * content using the 'value()' method for controls with no payload, or
> > + * retrieving reference to the memory location that contains the data payload
> > + * with the 'mem()' operation for controls which transport a data payload.
> > + *
> > + * \todo Support setting controls with data payload.
> > + */
> > +
> > +namespace libcamera {
> > +
> > +/**
> > + * \class V4L2Control
> > + * \brief Base class for V4L2 Controls
> > + *
> > + * The V4L2Control base class is the base class that contains the fields common
> > + * to all controls (id, type and size).
> > + *
> > + * This class is not meant to be instantiated directly, but is instead used as a
> > + * place holder to store controls in arrays or other containers. User of the
> > + * libcamera V4L2 control framework should access the controls content by
> > + * instantiating one of the provided specialized derived classes.
> > + */
> > +
> > +/**
> > + * \fn V4L2Control::V4L2Control
> > + * \brief Construct a V4L2Control instance
> > + * \param id The control's id
> > + * \param size The control's data size
> > + * \param type The control's type
> > + */
> > +
> > +/**
> > + * \fn V4L2Control::id()
> > + * \brief Retrieve the control's id
> > + *
> > + * Retrieve the control's numerical id value as defined by the V4L2
> > + * specification.
> > + *
> > + * \return The control's id
> > + */
> > +
> > +/**
> > + * \fn V4L2Control::size()
> > + * \brief Retrieve the control's data value size in bytes
> > + *
> > + * \todo Better define the value of size() for controls with payload data.
> > + *
> > + * \return The control's size
> > + */
> > +
> > +/**
> > + * \fn V4L2Control::type()
> > + * \brief Retrieve the control's type
> > + *
> > + * Retrieve the control's type as defined by the V4L2 specification.
> > + *
> > + * \return The control's type
> > + */
> > +
> > +/**
> > + * \class V4L2ControlValue
> > + * \brief Template base class that represent values of a V4L2 control
> > + *
> > + * The V4L2ControlValue template base class represents a V4L2 control with
> > + * its different value types.
> > + *
> > + * It provides two operations to access the control's value or the pointer
> > + * to the memory location that contains to the control's value data payload.
> > + */
> > +
> > +/**
> > + * \fn V4L2ControlValue::V4L2ControlValue(unsigned int id, unsigned int size, enum v4l2_ctrl_type type, T value)
> > + * \brief Contruct a V4L2ControlValue with a value
> 
> /Contruct/Construct/
> 
> > + * \param id The control's id
> > + * \param size The control's size
> > + * \param type The control's type
> > + * \param value The control's value
> > + */
> > +
> > +/**
> > + * \fn V4L2ControlValue::V4L2ControlValue(unsigned int id, unsigned int size, enum v4l2_ctrl_type type, T* value)
> > + * \brief Contruct a V4L2ControlValue with a pointer to a data payload
> 
> /Contruct/Construct/
> 
> > + * \param id The control's id
> > + * \param size The control's size
> > + * \param type The control's type
> > + * \param value The pointer to the control's data payload
> > + */
> > +
> > +/**
> > + * \fn V4L2ControlValue::~V4L2ControlValue
> > + * \brief Release the memory reserved for the control's data payload, if any
> > + */
> > +
> > +/**
> > + * \fn V4L2ControlValue::value()
> > + * \brief Retrieve the control's value
> > + *
> > + * Retrieve the control's value. Valid only for controls with no payload.
> > + * Access a control's value by calling the value() operation on instances
> > + * of the specialized derived classes.
> > + */
> > +
> > +/**
> > + * \fn V4L2ControlValue::mem()
> > + * \brief Retrieve a pointer to the memory location of the control's data
> > + *
> > + * Retrieve a pointer to the memory location that contains the control's data
> > + * payload. Valid only for controls with data payload. Access the memory
> > + * location of the control's data by calling the mem() operation on instances
> > + * of the specialized derived classes.
> > + */
> > +
> > +/**
> > + * \class V4L2IntControl
> > + * \brief Specialized V4L2Control class that handles controls of
> > + *        V4L2_CTRL_TYPE_INTEGER type.
> > + *
> > + * Access the control's data value by using the value() operation.
> > + */
> > +
> > +/**
> > + * \fn V4L2IntControl::V4L2IntControl()
> > + * \brief Construct a V4L2Control that contains an int value
> > + * \param id The control's id
> > + * \param value The control's value
> > + */
> > +
> > +/**
> > + * \class V4L2Int64Control
> > + * \brief Specialized V4L2Control class that handles controls of
> > + *        V4L2_CTRL_TYPE_INTEGER64 type.
> > + *
> > + * Access the control's data value by using the value() operation.
> > + */
> > +
> > +/**
> > + * \fn V4L2Int64Control::V4L2Int64Control()
> > + * \brief Construct a V4L2Control that contains an int64 value
> > + * \param id The control's id
> > + * \param value The control's value
> > + */
> > +
> > +/**
> > + * \class V4L2BoolControl
> > + * \brief Specialized V4L2Control class that handles controls of
> > + *        V4L2_CTRL_TYPE_BOOLEAN type.
> > + *
> > + * Access the control's data value by using the value() operation.
> > + */
> > +
> > +/**
> > + * \fn V4L2BoolControl::V4L2BoolControl()
> > + * \brief Construct a V4L2Control that contains a boolean value
> > + * \param id The control's id
> > + * \param value The control's value
> > + */
> > +
> > +/**
> > + * \class V4L2StringControl
> > + * \brief Specialized V4L2Control class that handles controls of
> > + *        V4L2_CTRL_TYPE_STRING type.
> > + *
> > + * Access the control's data value by using the value() operation.
> > + */
> > +
> > +/**
> > + * \fn V4L2StringControl::V4L2StringControl()
> > + * \brief Construct a V4L2Control that contains a string value
> > + * \param id The control's id
> > + * \param value The control's value
> > + */
> > +
> > +/**
> > + * \class V4L2U8Control
> > + * \brief Specialized V4L2Control class that handles controls with payload of
> > + *        V4L2_CTRL_TYPE_U8 type.
> > + *
> > + * Access the control's data value by using the retrieving the memory location
> > + * where the data payload is stored with the mem() operation. The size of
> > + * the payload data can be retrieved with the size() operation.
> > + */
> > +
> > +/**
> > + * \fn V4L2U8Control::V4L2U8Control()
> > + * \brief Construct a V4L2Control with payload of uin8_t data
> > + * \param id The control's id
> > + * \param size The size in bytes of the payload content
> > + * \param mem Pointer to the memory location of the payload content
> > + *
> > + * Memory is reserved in the newly created instance to hold the data payload
> > + * and the data content is copied there. The reserved memory is then freed when
> > + * the Control is destroyed. The memory where the control's payload was copied
> > + * from should be released by the caller.
> > + */
> > +
> > +/**
> > + * \class V4L2U16Control
> > + * \brief Specialized V4L2Control class that handles controls with payload of
> > + *        V4L2_CTRL_TYPE_U16 type.
> > + *
> > + * Access the control's data value by using the retrieving the memory location
> > + * where the data payload is stored with the mem() operation. The size of
> > + * the payload data can be retrieved with the size() operation.
> > + */
> > +
> > +/**
> > + * \fn V4L2U16Control::V4L2U16Control(unsigned int id, unsigned int size, uint16_t *mem)
> > + * \brief Construct a V4L2Control with payload of uin16_t data
> > + * \param id The control's id
> > + * \param size The size in bytes of the payload content
> > + * \param mem Pointer to the memory location of the payload content
> > + *
> > + * Memory is reserved in the newly created instance to hold the data payload
> > + * and the data content is copied there. The reserved memory is then freed when
> > + * the Control is destroyed. The memory where the control's payload was copied
> > + * from should be released by the caller.
> > + */
> > +
> > +/**
> > + * \class V4L2U32Control
> > + * \brief Specialized V4L2Control class that handles controls with payload of
> > + *        V4L2_CTRL_TYPE_U32 type.
> > + *
> > + * Access the control's data value by using the retrieving the memory location
> > + * where the data payload is stored with the mem() operation. The size of
> > + * the payload data can be retrieved with the size() operation.
> > + */
> > +
> > +/**
> > + * \fn V4L2U32Control::V4L2U32Control(unsigned int id, unsigned int size, uint32_t *mem)
> > + * \brief Construct a V4L2Control with payload of uin32_t data
> > + * \param id The control's id
> > + * \param size The size in bytes of the payload content
> > + * \param mem Pointer to the memory location of the payload content
> > + *
> > + * Memory is reserved in the newly created instance to hold the data payload
> > + * and the data content is copied there. The reserved memory is then freed when
> > + * the Control is destroyed. The memory where the control's payload was copied
> > + * from should be released by the caller.
> > + */
> > +
> > +}; /* namespace libcamera */

Hi Laurent,

On Sat, Jun 08, 2019 at 07:17:33PM +0300, Laurent Pinchart wrote:
> Hello Jacopo,
>
> Thank you for the patch.
>
> On Mon, Jun 03, 2019 at 10:47:00AM +0100, Kieran Bingham wrote:
> > On 02/06/2019 14:04, 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.
> >
> >
> > I haven't done a full documentation review here, I'm just trying to
> > focus on the code parts first.
> >

That's fine, thanks anyway!

> > --
> > KB
> >
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  src/libcamera/include/v4l2_controls.h | 147 +++++++++++++
> > >  src/libcamera/meson.build             |   1 +
> > >  src/libcamera/v4l2_controls.cpp       | 301 ++++++++++++++++++++++++++
> > >  3 files changed, 449 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..df955c2e10b2
> > > --- /dev/null
> > > +++ b/src/libcamera/include/v4l2_controls.h
> > > @@ -0,0 +1,147 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * v4l2_controls.h - V4L2 Extended Control Support
> > > + */
> > > +
> > > +#ifndef __LIBCAMERA_V4L2_CONTROL_H__
> > > +#define __LIBCAMERA_V4L2_CONTROL_H__
>
> s/CONTROL/CONTROLS/
>
> > > +
> > > +#include <cstring>
> > > +
> > > +#include <linux/v4l2-controls.h>
> > > +#include <linux/videodev2.h>
> > > +
> > > +namespace libcamera {
> > > +
> > > +class V4L2Control
> > > +{
> > > +public:
> > > +	virtual ~V4L2Control()
> > > +	{
> > > +	}
> > > +
> > > +	unsigned int id() { return id_; }
> > > +	unsigned int size() { return size_; }
> > > +	enum v4l2_ctrl_type type() const { return type_; }
> > > +
> > > +protected:
> > > +	V4L2Control(unsigned int id, unsigned int size,
> > > +		    enum v4l2_ctrl_type type)
> > > +		: id_(id), size_(size), type_(type)
> > > +	{
> > > +	}
> > > +
> > > +private:
> > > +	unsigned int id_;
> > > +	unsigned int size_;
> > > +	enum v4l2_ctrl_type type_;
> > > +};
> > > +
> > > +template<typename T>
> > > +class V4L2ControlValue : public V4L2Control
> >
> > Interesting, I thought about templating to do the types for the
> > Libcamera Controls, but instead ended up embedding the type in the
> > object to avoid having handle casting things ...
> >
> > Although that then leads me to require having a getBool, getInteger,
> > getXXX so we can evaluate both options side by side, and perhaps then
> > unify the model for both.
> >
> > (I'm hoping to get some patches posted soon which will show the
> > difference in approach)
> >
> > The templating might organise memory better, as in my implementation I
> > can add a bool, and int to a union, but not a std::string object, so I
> > incur wasted space.
>
> The trouble with the template is that it requires passing containers of
> V4L2Control pointers, making it complicated for the caller (especially
> when deleting all the elements).
>
> An alternative would be to create a V4L2Controls class that would wrap
> around the container, and provide accessors to get/set values, and to
> iterate over controls. How about the following pseudo-code from the
> point of view of the caller ?
>
> 	V4L2Device *dev = ...;
> 	V4L2Controls ctrls;

I like this API but

>
> 	ctrls.set(V4L2_CID_EXPOSURE, 42);
> 	ctrls.set(V4L2_CID_GAIN, 100);
>

I uinderstand I could overload on the second parameter type (but that
required called to be very precise, as in
                        ctrls.set(V4L2_CID_...,
                                  static_cast<uint8_t *>(val));

                        ctrls.set(V4L2_CID_...,
                                  static_cast<uint16_t *>(val2));

Or I could make this a template and specialize based on the type
returned by query_ctrl()

> 	dev->setControls(&ctrls);
>
> and
>
> 	V4L2Device *dev = ...;
> 	V4L2Controls ctrls;
>
> 	ctrls.get(V4L2_CID_EXPOSURE);
> 	ctrls.get(V4L2_CID_GAIN);
>
> 	dev->getControls(&ctrls);
>
> 	unsigned int exposure = ctrls.value(V4L2_CID_EXPOSURE);

I'm not sure I can think how that would ever work considering
overloading on the return type and deducing the type of a template on
return value is not possible.

Soemthing like this should not work afaict:

        T V4L2Controls::value(unsigned int);

As that could only be called on a specialized instance where T has a
type assigned already, and the V4L2Control class that wraps the
container should not expose any of the specialized derived classes
to the API users.

I can try to play around a bit with this approach, but if you have any
track I should follow let me know.

>
> It's important to make the API easy to use for pipeline handlers and
> efficient in terms of both CPU time and memory. I expect tradeoffs of
> course :-)
>
> > > +{
> > > +public:
> > > +	T value() const { return value_; }
> > > +	T *mem() const { return memvalue_; }
> >
> > s/memvalue_/mem_/? or /mem()/memvalue()/?
> >
> > (I prefer mem())
> >
> > > +
> > > +protected:
> > > +	V4L2ControlValue(unsigned int id, unsigned int size,
> > > +			 enum v4l2_ctrl_type type, T value)
> > > +		: V4L2Control(id, size, type)
> > > +	{
> > > +		value_ = value;
> > > +		memvalue_ = nullptr;
> > > +	}
> > > +
> > > +	V4L2ControlValue(unsigned int id, unsigned int size,
> > > +			 enum v4l2_ctrl_type type, T *value)
> > > +		: V4L2Control(id, size, type)
> > > +	{
> > > +		value_ = 0;
> > > +		memvalue_ = static_cast<T *>(new T[size]);
> > > +		memcpy(memvalue_, value, size);
> > > +	}
> > > +
> > > +	~V4L2ControlValue()
> > > +	{
> > > +		delete[] memvalue_;
> > > +	}
> > > +
> > > +private:
> > > +	T value_;
> > > +	T *memvalue_;
> > > +};
> > > +
> > > +class V4L2IntControl : public V4L2ControlValue<int32_t>
> > > +{
> > > +public:
> > > +	V4L2IntControl(unsigned int id, int32_t value)
> > > +		: V4L2ControlValue<int32_t>(id, sizeof(int32_t),
> > > +					    V4L2_CTRL_TYPE_INTEGER, value)
> > > +	{
> > > +	}
> > > +};
> > > +
> > > +class V4L2Int64Control : public V4L2ControlValue<int64_t>
> > > +{
> > > +public:
> > > +	V4L2Int64Control(unsigned int id, int64_t value)
> > > +		: V4L2ControlValue<int64_t>(id, sizeof(int64_t),
> > > +					    V4L2_CTRL_TYPE_INTEGER64, value)
> > > +	{
> > > +	}
> > > +};
> > > +
> > > +class V4L2BoolControl : public V4L2ControlValue<bool>
> > > +{
> > > +public:
> > > +	V4L2BoolControl(unsigned int id, bool value)
> > > +		: V4L2ControlValue<bool>(id, sizeof(bool),
> > > +					 V4L2_CTRL_TYPE_BOOLEAN, value)
> > > +	{
> > > +	}
> > > +};
> > > +
> > > +class V4L2StringControl : public V4L2ControlValue<std::string>
> >
> > You might want/need to to support char* types too.
> >
> > > +{
> > > +public:
> > > +	V4L2StringControl(unsigned int id, std::string value)
> > > +		: V4L2ControlValue<std::string>(id, value.length(),
> > > +						V4L2_CTRL_TYPE_STRING, value)
> > > +	{
> > > +	}
> > > +};
> > > +
> > > +class V4L2U8Control : public V4L2ControlValue<uint8_t>
> > > +{
> > > +public:
> > > +	V4L2U8Control(unsigned int id, unsigned int size, uint8_t *mem)
> > > +		: V4L2ControlValue<uint8_t>(id, size, V4L2_CTRL_TYPE_U8, mem)
> > > +	{
> > > +	}
> > > +};
> > > +
> > > +class V4L2U16Control : public V4L2ControlValue<uint16_t>
> > > +{
> > > +public:
> > > +	V4L2U16Control(unsigned int id, unsigned int size, uint16_t *mem)
> > > +		: V4L2ControlValue<uint16_t>(id, size, V4L2_CTRL_TYPE_U16, mem)
> > > +	{
> > > +	}
> > > +};
> > > +
> > > +class V4L2U32Control : public V4L2ControlValue<uint32_t>
> > > +{
> > > +public:
> > > +	V4L2U32Control(unsigned int id, unsigned int size, uint32_t *mem)
> > > +		: V4L2ControlValue<uint32_t>(id, size, V4L2_CTRL_TYPE_U32, mem)
> > > +	{
> > > +	}
> > > +};
> > > +
> > > +}; /* namespace libcamera */
> > > +
> > > +#endif /* __LIBCAMERA_V4L2_CONTROL_H__ */
> > > +
> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > index 6d858a22531e..fa1fbcb5faf5 100644
> > > --- a/src/libcamera/meson.build
> > > +++ b/src/libcamera/meson.build
> > > @@ -22,6 +22,7 @@ libcamera_sources = files([
> > >      'timer.cpp',
> > >      'utils.cpp',
> > >      'v4l2_base.cpp',
> > > +    'v4l2_controls.cpp',
> > >      'v4l2_device.cpp',
> > >      'v4l2_subdevice.cpp',
> > >  ])
> > > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> > > new file mode 100644
> > > index 000000000000..f50f3cdfa7d7
> > > --- /dev/null
> > > +++ b/src/libcamera/v4l2_controls.cpp
> > > @@ -0,0 +1,301 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * v4l2_controls.cpp - V4L2 Extended Control Support
> > > + */
> > > +
> > > +/**
> > > + * \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
> >
> > "an application" or "applications"
> >
> > "a set of"
> >
> > application? Are we talking about applications that use libcamera? Or
> > applications in general?
> >
> >
> > > + * of configurable parameters on the video device or subdevice of interest.  The
> >
> > /.  The/. The/ <double space>
> >
> >
> >
> > > + * 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 'setControl()' operation call or
> > > + * retrieved with a 'getControl()' one.
> > > + *
> > > + * 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
> > > + * matrixes, stored in a contiguous memory locations associated with the control
> >
> > s/matrixes/matrices/
> >
> > > + * and called 'the payload'. The content have to be opportunely copied into the
> > > + * application memory when retrieving a control's value and provided to the V4L2
> > > + * device when setting it.
> > > + *
> > > + * Libcamera supports control using the V4L2 'Extended Control' framework, which
> > > + * allows easier handling of controls with payloads of arbitrary sizes.
> > > + *
> > > + * The Libcamera V4L2 Controls framework operates on lists of controls, to match
> > > + * the V4L2 extended controls framework, but also provides operations to get
> > > + * and set single controls. The interface to set and get control is implemented
> > > + * by the V4L2Base class, while this file only provides the data type
> > > + * definitions.
> > > + *
> > > + * The Libcamera V4L2 control framework data types define a base class
> > > + * V4L2Control that contains the fields common to all controls, regardless of
> > > + * their type, such as the  control's id, its type and the expected control's
> > > + * value data size. The V4L2Control class is not meant to be instantiated
> > > + * directly, but is instead used as placeholder to store controls in the
> > > + * standard library provided containers.
> > > + *
> > > + * A parametric derived class V4L2ControlValue stores the control's value and
> > > + * provide accessors to the value itself for control with no payload, or to the
> > > + * memory area that contains the control's data payload. This class is not
> > > + * intended to be directly used and cannot be constructed, but it is used
> > > + * instead to define specialized derived classes which specialize the control's
> > > + * data value type.
> > > + *
> > > + * In order to set and get controls, user of the Libcamera V4L2 control
> > > + * framework should create instances of the specialized derived classes,
> > > + * which are publicly constructible, and use them to access the control's data
> > > + * content using the 'value()' method for controls with no payload, or
> > > + * retrieving reference to the memory location that contains the data payload
> > > + * with the 'mem()' operation for controls which transport a data payload.
> > > + *
> > > + * \todo Support setting controls with data payload.
> > > + */
> > > +
> > > +namespace libcamera {
> > > +
> > > +/**
> > > + * \class V4L2Control
> > > + * \brief Base class for V4L2 Controls
> > > + *
> > > + * The V4L2Control base class is the base class that contains the fields common
> > > + * to all controls (id, type and size).
> > > + *
> > > + * This class is not meant to be instantiated directly, but is instead used as a
> > > + * place holder to store controls in arrays or other containers. User of the
> > > + * libcamera V4L2 control framework should access the controls content by
> > > + * instantiating one of the provided specialized derived classes.
> > > + */
> > > +
> > > +/**
> > > + * \fn V4L2Control::V4L2Control
> > > + * \brief Construct a V4L2Control instance
> > > + * \param id The control's id
> > > + * \param size The control's data size
> > > + * \param type The control's type
> > > + */
> > > +
> > > +/**
> > > + * \fn V4L2Control::id()
> > > + * \brief Retrieve the control's id
> > > + *
> > > + * Retrieve the control's numerical id value as defined by the V4L2
> > > + * specification.
> > > + *
> > > + * \return The control's id
> > > + */
> > > +
> > > +/**
> > > + * \fn V4L2Control::size()
> > > + * \brief Retrieve the control's data value size in bytes
> > > + *
> > > + * \todo Better define the value of size() for controls with payload data.
> > > + *
> > > + * \return The control's size
> > > + */
> > > +
> > > +/**
> > > + * \fn V4L2Control::type()
> > > + * \brief Retrieve the control's type
> > > + *
> > > + * Retrieve the control's type as defined by the V4L2 specification.
> > > + *
> > > + * \return The control's type
> > > + */
> > > +
> > > +/**
> > > + * \class V4L2ControlValue
> > > + * \brief Template base class that represent values of a V4L2 control
> > > + *
> > > + * The V4L2ControlValue template base class represents a V4L2 control with
> > > + * its different value types.
> > > + *
> > > + * It provides two operations to access the control's value or the pointer
> > > + * to the memory location that contains to the control's value data payload.
> > > + */
> > > +
> > > +/**
> > > + * \fn V4L2ControlValue::V4L2ControlValue(unsigned int id, unsigned int size, enum v4l2_ctrl_type type, T value)
> > > + * \brief Contruct a V4L2ControlValue with a value
> >
> > /Contruct/Construct/
> >
> > > + * \param id The control's id
> > > + * \param size The control's size
> > > + * \param type The control's type
> > > + * \param value The control's value
> > > + */
> > > +
> > > +/**
> > > + * \fn V4L2ControlValue::V4L2ControlValue(unsigned int id, unsigned int size, enum v4l2_ctrl_type type, T* value)
> > > + * \brief Contruct a V4L2ControlValue with a pointer to a data payload
> >
> > /Contruct/Construct/
> >

Thanks Kieran, I'll take you suggestions on comments in.

Thanks
  j

> > > + * \param id The control's id
> > > + * \param size The control's size
> > > + * \param type The control's type
> > > + * \param value The pointer to the control's data payload
> > > + */
> > > +
> > > +/**
> > > + * \fn V4L2ControlValue::~V4L2ControlValue
> > > + * \brief Release the memory reserved for the control's data payload, if any
> > > + */
> > > +
> > > +/**
> > > + * \fn V4L2ControlValue::value()
> > > + * \brief Retrieve the control's value
> > > + *
> > > + * Retrieve the control's value. Valid only for controls with no payload.
> > > + * Access a control's value by calling the value() operation on instances
> > > + * of the specialized derived classes.
> > > + */
> > > +
> > > +/**
> > > + * \fn V4L2ControlValue::mem()
> > > + * \brief Retrieve a pointer to the memory location of the control's data
> > > + *
> > > + * Retrieve a pointer to the memory location that contains the control's data
> > > + * payload. Valid only for controls with data payload. Access the memory
> > > + * location of the control's data by calling the mem() operation on instances
> > > + * of the specialized derived classes.
> > > + */
> > > +
> > > +/**
> > > + * \class V4L2IntControl
> > > + * \brief Specialized V4L2Control class that handles controls of
> > > + *        V4L2_CTRL_TYPE_INTEGER type.
> > > + *
> > > + * Access the control's data value by using the value() operation.
> > > + */
> > > +
> > > +/**
> > > + * \fn V4L2IntControl::V4L2IntControl()
> > > + * \brief Construct a V4L2Control that contains an int value
> > > + * \param id The control's id
> > > + * \param value The control's value
> > > + */
> > > +
> > > +/**
> > > + * \class V4L2Int64Control
> > > + * \brief Specialized V4L2Control class that handles controls of
> > > + *        V4L2_CTRL_TYPE_INTEGER64 type.
> > > + *
> > > + * Access the control's data value by using the value() operation.
> > > + */
> > > +
> > > +/**
> > > + * \fn V4L2Int64Control::V4L2Int64Control()
> > > + * \brief Construct a V4L2Control that contains an int64 value
> > > + * \param id The control's id
> > > + * \param value The control's value
> > > + */
> > > +
> > > +/**
> > > + * \class V4L2BoolControl
> > > + * \brief Specialized V4L2Control class that handles controls of
> > > + *        V4L2_CTRL_TYPE_BOOLEAN type.
> > > + *
> > > + * Access the control's data value by using the value() operation.
> > > + */
> > > +
> > > +/**
> > > + * \fn V4L2BoolControl::V4L2BoolControl()
> > > + * \brief Construct a V4L2Control that contains a boolean value
> > > + * \param id The control's id
> > > + * \param value The control's value
> > > + */
> > > +
> > > +/**
> > > + * \class V4L2StringControl
> > > + * \brief Specialized V4L2Control class that handles controls of
> > > + *        V4L2_CTRL_TYPE_STRING type.
> > > + *
> > > + * Access the control's data value by using the value() operation.
> > > + */
> > > +
> > > +/**
> > > + * \fn V4L2StringControl::V4L2StringControl()
> > > + * \brief Construct a V4L2Control that contains a string value
> > > + * \param id The control's id
> > > + * \param value The control's value
> > > + */
> > > +
> > > +/**
> > > + * \class V4L2U8Control
> > > + * \brief Specialized V4L2Control class that handles controls with payload of
> > > + *        V4L2_CTRL_TYPE_U8 type.
> > > + *
> > > + * Access the control's data value by using the retrieving the memory location
> > > + * where the data payload is stored with the mem() operation. The size of
> > > + * the payload data can be retrieved with the size() operation.
> > > + */
> > > +
> > > +/**
> > > + * \fn V4L2U8Control::V4L2U8Control()
> > > + * \brief Construct a V4L2Control with payload of uin8_t data
> > > + * \param id The control's id
> > > + * \param size The size in bytes of the payload content
> > > + * \param mem Pointer to the memory location of the payload content
> > > + *
> > > + * Memory is reserved in the newly created instance to hold the data payload
> > > + * and the data content is copied there. The reserved memory is then freed when
> > > + * the Control is destroyed. The memory where the control's payload was copied
> > > + * from should be released by the caller.
> > > + */
> > > +
> > > +/**
> > > + * \class V4L2U16Control
> > > + * \brief Specialized V4L2Control class that handles controls with payload of
> > > + *        V4L2_CTRL_TYPE_U16 type.
> > > + *
> > > + * Access the control's data value by using the retrieving the memory location
> > > + * where the data payload is stored with the mem() operation. The size of
> > > + * the payload data can be retrieved with the size() operation.
> > > + */
> > > +
> > > +/**
> > > + * \fn V4L2U16Control::V4L2U16Control(unsigned int id, unsigned int size, uint16_t *mem)
> > > + * \brief Construct a V4L2Control with payload of uin16_t data
> > > + * \param id The control's id
> > > + * \param size The size in bytes of the payload content
> > > + * \param mem Pointer to the memory location of the payload content
> > > + *
> > > + * Memory is reserved in the newly created instance to hold the data payload
> > > + * and the data content is copied there. The reserved memory is then freed when
> > > + * the Control is destroyed. The memory where the control's payload was copied
> > > + * from should be released by the caller.
> > > + */
> > > +
> > > +/**
> > > + * \class V4L2U32Control
> > > + * \brief Specialized V4L2Control class that handles controls with payload of
> > > + *        V4L2_CTRL_TYPE_U32 type.
> > > + *
> > > + * Access the control's data value by using the retrieving the memory location
> > > + * where the data payload is stored with the mem() operation. The size of
> > > + * the payload data can be retrieved with the size() operation.
> > > + */
> > > +
> > > +/**
> > > + * \fn V4L2U32Control::V4L2U32Control(unsigned int id, unsigned int size, uint32_t *mem)
> > > + * \brief Construct a V4L2Control with payload of uin32_t data
> > > + * \param id The control's id
> > > + * \param size The size in bytes of the payload content
> > > + * \param mem Pointer to the memory location of the payload content
> > > + *
> > > + * Memory is reserved in the newly created instance to hold the data payload
> > > + * and the data content is copied there. The reserved memory is then freed when
> > > + * the Control is destroyed. The memory where the control's payload was copied
> > > + * from should be released by the caller.
> > > + */
> > > +
> > > +}; /* namespace libcamera */
>
> --
> Regards,
>
> Laurent Pinchart

Hi Jacopo,

On Mon, Jun 10, 2019 at 09:53:23AM +0200, Jacopo Mondi wrote:
> On Sat, Jun 08, 2019 at 07:17:33PM +0300, Laurent Pinchart wrote:
> > On Mon, Jun 03, 2019 at 10:47:00AM +0100, Kieran Bingham wrote:
> >> On 02/06/2019 14:04, 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.
> >>
> >> I haven't done a full documentation review here, I'm just trying to
> >> focus on the code parts first.
> 
> That's fine, thanks anyway!
> 
> >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>> ---
> >>>  src/libcamera/include/v4l2_controls.h | 147 +++++++++++++
> >>>  src/libcamera/meson.build             |   1 +
> >>>  src/libcamera/v4l2_controls.cpp       | 301 ++++++++++++++++++++++++++
> >>>  3 files changed, 449 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..df955c2e10b2
> >>> --- /dev/null
> >>> +++ b/src/libcamera/include/v4l2_controls.h
> >>> @@ -0,0 +1,147 @@
> >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >>> +/*
> >>> + * Copyright (C) 2019, Google Inc.
> >>> + *
> >>> + * v4l2_controls.h - V4L2 Extended Control Support
> >>> + */
> >>> +
> >>> +#ifndef __LIBCAMERA_V4L2_CONTROL_H__
> >>> +#define __LIBCAMERA_V4L2_CONTROL_H__
> >
> > s/CONTROL/CONTROLS/
> >
> >>> +
> >>> +#include <cstring>
> >>> +
> >>> +#include <linux/v4l2-controls.h>
> >>> +#include <linux/videodev2.h>
> >>> +
> >>> +namespace libcamera {
> >>> +
> >>> +class V4L2Control
> >>> +{
> >>> +public:
> >>> +	virtual ~V4L2Control()
> >>> +	{
> >>> +	}
> >>> +
> >>> +	unsigned int id() { return id_; }
> >>> +	unsigned int size() { return size_; }
> >>> +	enum v4l2_ctrl_type type() const { return type_; }
> >>> +
> >>> +protected:
> >>> +	V4L2Control(unsigned int id, unsigned int size,
> >>> +		    enum v4l2_ctrl_type type)
> >>> +		: id_(id), size_(size), type_(type)
> >>> +	{
> >>> +	}
> >>> +
> >>> +private:
> >>> +	unsigned int id_;
> >>> +	unsigned int size_;
> >>> +	enum v4l2_ctrl_type type_;
> >>> +};
> >>> +
> >>> +template<typename T>
> >>> +class V4L2ControlValue : public V4L2Control
> >>
> >> Interesting, I thought about templating to do the types for the
> >> Libcamera Controls, but instead ended up embedding the type in the
> >> object to avoid having handle casting things ...
> >>
> >> Although that then leads me to require having a getBool, getInteger,
> >> getXXX so we can evaluate both options side by side, and perhaps then
> >> unify the model for both.
> >>
> >> (I'm hoping to get some patches posted soon which will show the
> >> difference in approach)
> >>
> >> The templating might organise memory better, as in my implementation I
> >> can add a bool, and int to a union, but not a std::string object, so I
> >> incur wasted space.
> >
> > The trouble with the template is that it requires passing containers of
> > V4L2Control pointers, making it complicated for the caller (especially
> > when deleting all the elements).
> >
> > An alternative would be to create a V4L2Controls class that would wrap
> > around the container, and provide accessors to get/set values, and to
> > iterate over controls. How about the following pseudo-code from the
> > point of view of the caller ?
> >
> > 	V4L2Device *dev = ...;
> > 	V4L2Controls ctrls;
> 
> I like this API but
> 
> > 	ctrls.set(V4L2_CID_EXPOSURE, 42);
> > 	ctrls.set(V4L2_CID_GAIN, 100);
> 
> I uinderstand I could overload on the second parameter type (but that
> required called to be very precise, as in
> 
>                         ctrls.set(V4L2_CID_...,
>                                   static_cast<uint8_t *>(val));
> 
>                         ctrls.set(V4L2_CID_...,
>                                   static_cast<uint16_t *>(val2));

You only need to cast if the type is incorrect in the first place.

> Or I could make this a template and specialize based on the type
> returned by query_ctrl()

But you can't make it a template without allocating each control
instance dynamically, which isn't very efficient and is quite annoying
to use (as shown by the patches that delete the controls explicitly).

You can use the type returned by query_ctrl() internally, and perform
sanity checks based on it (in order for instance to reject setting a
string control to an integer value).

> > 	dev->setControls(&ctrls);
> >
> > and
> >
> > 	V4L2Device *dev = ...;
> > 	V4L2Controls ctrls;
> >
> > 	ctrls.get(V4L2_CID_EXPOSURE);
> > 	ctrls.get(V4L2_CID_GAIN);
> >
> > 	dev->getControls(&ctrls);
> >
> > 	unsigned int exposure = ctrls.value(V4L2_CID_EXPOSURE);
> 
> I'm not sure I can think how that would ever work considering
> overloading on the return type and deducing the type of a template on
> return value is not possible.
> 
> Soemthing like this should not work afaict:
> 
>         T V4L2Controls::value(unsigned int);

Yes, that won't work. We may need a valueInteger(), valueString(), ...

> As that could only be called on a specialized instance where T has a
> type assigned already, and the V4L2Control class that wraps the
> container should not expose any of the specialized derived classes
> to the API users.
> 
> I can try to play around a bit with this approach, but if you have any
> track I should follow let me know.

I don't have a detailed proposal at the moment, so please try to play
around a bit :-) What I think is really important is both ease of use
and efficiency.

> > It's important to make the API easy to use for pipeline handlers and
> > efficient in terms of both CPU time and memory. I expect tradeoffs of
> > course :-)
> >
> >>> +{
> >>> +public:
> >>> +	T value() const { return value_; }
> >>> +	T *mem() const { return memvalue_; }
> >>
> >> s/memvalue_/mem_/? or /mem()/memvalue()/?
> >>
> >> (I prefer mem())
> >>
> >>> +
> >>> +protected:
> >>> +	V4L2ControlValue(unsigned int id, unsigned int size,
> >>> +			 enum v4l2_ctrl_type type, T value)
> >>> +		: V4L2Control(id, size, type)
> >>> +	{
> >>> +		value_ = value;
> >>> +		memvalue_ = nullptr;
> >>> +	}
> >>> +
> >>> +	V4L2ControlValue(unsigned int id, unsigned int size,
> >>> +			 enum v4l2_ctrl_type type, T *value)
> >>> +		: V4L2Control(id, size, type)
> >>> +	{
> >>> +		value_ = 0;
> >>> +		memvalue_ = static_cast<T *>(new T[size]);
> >>> +		memcpy(memvalue_, value, size);
> >>> +	}
> >>> +
> >>> +	~V4L2ControlValue()
> >>> +	{
> >>> +		delete[] memvalue_;
> >>> +	}
> >>> +
> >>> +private:
> >>> +	T value_;
> >>> +	T *memvalue_;
> >>> +};
> >>> +
> >>> +class V4L2IntControl : public V4L2ControlValue<int32_t>
> >>> +{
> >>> +public:
> >>> +	V4L2IntControl(unsigned int id, int32_t value)
> >>> +		: V4L2ControlValue<int32_t>(id, sizeof(int32_t),
> >>> +					    V4L2_CTRL_TYPE_INTEGER, value)
> >>> +	{
> >>> +	}
> >>> +};
> >>> +
> >>> +class V4L2Int64Control : public V4L2ControlValue<int64_t>
> >>> +{
> >>> +public:
> >>> +	V4L2Int64Control(unsigned int id, int64_t value)
> >>> +		: V4L2ControlValue<int64_t>(id, sizeof(int64_t),
> >>> +					    V4L2_CTRL_TYPE_INTEGER64, value)
> >>> +	{
> >>> +	}
> >>> +};
> >>> +
> >>> +class V4L2BoolControl : public V4L2ControlValue<bool>
> >>> +{
> >>> +public:
> >>> +	V4L2BoolControl(unsigned int id, bool value)
> >>> +		: V4L2ControlValue<bool>(id, sizeof(bool),
> >>> +					 V4L2_CTRL_TYPE_BOOLEAN, value)
> >>> +	{
> >>> +	}
> >>> +};
> >>> +
> >>> +class V4L2StringControl : public V4L2ControlValue<std::string>
> >>
> >> You might want/need to to support char* types too.
> >>
> >>> +{
> >>> +public:
> >>> +	V4L2StringControl(unsigned int id, std::string value)
> >>> +		: V4L2ControlValue<std::string>(id, value.length(),
> >>> +						V4L2_CTRL_TYPE_STRING, value)
> >>> +	{
> >>> +	}
> >>> +};
> >>> +
> >>> +class V4L2U8Control : public V4L2ControlValue<uint8_t>
> >>> +{
> >>> +public:
> >>> +	V4L2U8Control(unsigned int id, unsigned int size, uint8_t *mem)
> >>> +		: V4L2ControlValue<uint8_t>(id, size, V4L2_CTRL_TYPE_U8, mem)
> >>> +	{
> >>> +	}
> >>> +};
> >>> +
> >>> +class V4L2U16Control : public V4L2ControlValue<uint16_t>
> >>> +{
> >>> +public:
> >>> +	V4L2U16Control(unsigned int id, unsigned int size, uint16_t *mem)
> >>> +		: V4L2ControlValue<uint16_t>(id, size, V4L2_CTRL_TYPE_U16, mem)
> >>> +	{
> >>> +	}
> >>> +};
> >>> +
> >>> +class V4L2U32Control : public V4L2ControlValue<uint32_t>
> >>> +{
> >>> +public:
> >>> +	V4L2U32Control(unsigned int id, unsigned int size, uint32_t *mem)
> >>> +		: V4L2ControlValue<uint32_t>(id, size, V4L2_CTRL_TYPE_U32, mem)
> >>> +	{
> >>> +	}
> >>> +};
> >>> +
> >>> +}; /* namespace libcamera */
> >>> +
> >>> +#endif /* __LIBCAMERA_V4L2_CONTROL_H__ */
> >>> +
> >>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> >>> index 6d858a22531e..fa1fbcb5faf5 100644
> >>> --- a/src/libcamera/meson.build
> >>> +++ b/src/libcamera/meson.build
> >>> @@ -22,6 +22,7 @@ libcamera_sources = files([
> >>>      'timer.cpp',
> >>>      'utils.cpp',
> >>>      'v4l2_base.cpp',
> >>> +    'v4l2_controls.cpp',
> >>>      'v4l2_device.cpp',
> >>>      'v4l2_subdevice.cpp',
> >>>  ])
> >>> diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> >>> new file mode 100644
> >>> index 000000000000..f50f3cdfa7d7
> >>> --- /dev/null
> >>> +++ b/src/libcamera/v4l2_controls.cpp
> >>> @@ -0,0 +1,301 @@
> >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >>> +/*
> >>> + * Copyright (C) 2019, Google Inc.
> >>> + *
> >>> + * v4l2_controls.cpp - V4L2 Extended Control Support
> >>> + */
> >>> +
> >>> +/**
> >>> + * \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
> >>
> >> "an application" or "applications"
> >>
> >> "a set of"
> >>
> >> application? Are we talking about applications that use libcamera? Or
> >> applications in general?
> >>
> >>> + * of configurable parameters on the video device or subdevice of interest.  The
> >>
> >> /.  The/. The/ <double space>
> >>
> >>> + * 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 'setControl()' operation call or
> >>> + * retrieved with a 'getControl()' one.
> >>> + *
> >>> + * 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
> >>> + * matrixes, stored in a contiguous memory locations associated with the control
> >>
> >> s/matrixes/matrices/
> >>
> >>> + * and called 'the payload'. The content have to be opportunely copied into the
> >>> + * application memory when retrieving a control's value and provided to the V4L2
> >>> + * device when setting it.
> >>> + *
> >>> + * Libcamera supports control using the V4L2 'Extended Control' framework, which
> >>> + * allows easier handling of controls with payloads of arbitrary sizes.
> >>> + *
> >>> + * The Libcamera V4L2 Controls framework operates on lists of controls, to match
> >>> + * the V4L2 extended controls framework, but also provides operations to get
> >>> + * and set single controls. The interface to set and get control is implemented
> >>> + * by the V4L2Base class, while this file only provides the data type
> >>> + * definitions.
> >>> + *
> >>> + * The Libcamera V4L2 control framework data types define a base class
> >>> + * V4L2Control that contains the fields common to all controls, regardless of
> >>> + * their type, such as the  control's id, its type and the expected control's
> >>> + * value data size. The V4L2Control class is not meant to be instantiated
> >>> + * directly, but is instead used as placeholder to store controls in the
> >>> + * standard library provided containers.
> >>> + *
> >>> + * A parametric derived class V4L2ControlValue stores the control's value and
> >>> + * provide accessors to the value itself for control with no payload, or to the
> >>> + * memory area that contains the control's data payload. This class is not
> >>> + * intended to be directly used and cannot be constructed, but it is used
> >>> + * instead to define specialized derived classes which specialize the control's
> >>> + * data value type.
> >>> + *
> >>> + * In order to set and get controls, user of the Libcamera V4L2 control
> >>> + * framework should create instances of the specialized derived classes,
> >>> + * which are publicly constructible, and use them to access the control's data
> >>> + * content using the 'value()' method for controls with no payload, or
> >>> + * retrieving reference to the memory location that contains the data payload
> >>> + * with the 'mem()' operation for controls which transport a data payload.
> >>> + *
> >>> + * \todo Support setting controls with data payload.
> >>> + */
> >>> +
> >>> +namespace libcamera {
> >>> +
> >>> +/**
> >>> + * \class V4L2Control
> >>> + * \brief Base class for V4L2 Controls
> >>> + *
> >>> + * The V4L2Control base class is the base class that contains the fields common
> >>> + * to all controls (id, type and size).
> >>> + *
> >>> + * This class is not meant to be instantiated directly, but is instead used as a
> >>> + * place holder to store controls in arrays or other containers. User of the
> >>> + * libcamera V4L2 control framework should access the controls content by
> >>> + * instantiating one of the provided specialized derived classes.
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn V4L2Control::V4L2Control
> >>> + * \brief Construct a V4L2Control instance
> >>> + * \param id The control's id
> >>> + * \param size The control's data size
> >>> + * \param type The control's type
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn V4L2Control::id()
> >>> + * \brief Retrieve the control's id
> >>> + *
> >>> + * Retrieve the control's numerical id value as defined by the V4L2
> >>> + * specification.
> >>> + *
> >>> + * \return The control's id
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn V4L2Control::size()
> >>> + * \brief Retrieve the control's data value size in bytes
> >>> + *
> >>> + * \todo Better define the value of size() for controls with payload data.
> >>> + *
> >>> + * \return The control's size
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn V4L2Control::type()
> >>> + * \brief Retrieve the control's type
> >>> + *
> >>> + * Retrieve the control's type as defined by the V4L2 specification.
> >>> + *
> >>> + * \return The control's type
> >>> + */
> >>> +
> >>> +/**
> >>> + * \class V4L2ControlValue
> >>> + * \brief Template base class that represent values of a V4L2 control
> >>> + *
> >>> + * The V4L2ControlValue template base class represents a V4L2 control with
> >>> + * its different value types.
> >>> + *
> >>> + * It provides two operations to access the control's value or the pointer
> >>> + * to the memory location that contains to the control's value data payload.
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn V4L2ControlValue::V4L2ControlValue(unsigned int id, unsigned int size, enum v4l2_ctrl_type type, T value)
> >>> + * \brief Contruct a V4L2ControlValue with a value
> >>
> >> /Contruct/Construct/
> >>
> >>> + * \param id The control's id
> >>> + * \param size The control's size
> >>> + * \param type The control's type
> >>> + * \param value The control's value
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn V4L2ControlValue::V4L2ControlValue(unsigned int id, unsigned int size, enum v4l2_ctrl_type type, T* value)
> >>> + * \brief Contruct a V4L2ControlValue with a pointer to a data payload
> >>
> >> /Contruct/Construct/
> >>
> 
> Thanks Kieran, I'll take you suggestions on comments in.
> 
> >>> + * \param id The control's id
> >>> + * \param size The control's size
> >>> + * \param type The control's type
> >>> + * \param value The pointer to the control's data payload
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn V4L2ControlValue::~V4L2ControlValue
> >>> + * \brief Release the memory reserved for the control's data payload, if any
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn V4L2ControlValue::value()
> >>> + * \brief Retrieve the control's value
> >>> + *
> >>> + * Retrieve the control's value. Valid only for controls with no payload.
> >>> + * Access a control's value by calling the value() operation on instances
> >>> + * of the specialized derived classes.
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn V4L2ControlValue::mem()
> >>> + * \brief Retrieve a pointer to the memory location of the control's data
> >>> + *
> >>> + * Retrieve a pointer to the memory location that contains the control's data
> >>> + * payload. Valid only for controls with data payload. Access the memory
> >>> + * location of the control's data by calling the mem() operation on instances
> >>> + * of the specialized derived classes.
> >>> + */
> >>> +
> >>> +/**
> >>> + * \class V4L2IntControl
> >>> + * \brief Specialized V4L2Control class that handles controls of
> >>> + *        V4L2_CTRL_TYPE_INTEGER type.
> >>> + *
> >>> + * Access the control's data value by using the value() operation.
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn V4L2IntControl::V4L2IntControl()
> >>> + * \brief Construct a V4L2Control that contains an int value
> >>> + * \param id The control's id
> >>> + * \param value The control's value
> >>> + */
> >>> +
> >>> +/**
> >>> + * \class V4L2Int64Control
> >>> + * \brief Specialized V4L2Control class that handles controls of
> >>> + *        V4L2_CTRL_TYPE_INTEGER64 type.
> >>> + *
> >>> + * Access the control's data value by using the value() operation.
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn V4L2Int64Control::V4L2Int64Control()
> >>> + * \brief Construct a V4L2Control that contains an int64 value
> >>> + * \param id The control's id
> >>> + * \param value The control's value
> >>> + */
> >>> +
> >>> +/**
> >>> + * \class V4L2BoolControl
> >>> + * \brief Specialized V4L2Control class that handles controls of
> >>> + *        V4L2_CTRL_TYPE_BOOLEAN type.
> >>> + *
> >>> + * Access the control's data value by using the value() operation.
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn V4L2BoolControl::V4L2BoolControl()
> >>> + * \brief Construct a V4L2Control that contains a boolean value
> >>> + * \param id The control's id
> >>> + * \param value The control's value
> >>> + */
> >>> +
> >>> +/**
> >>> + * \class V4L2StringControl
> >>> + * \brief Specialized V4L2Control class that handles controls of
> >>> + *        V4L2_CTRL_TYPE_STRING type.
> >>> + *
> >>> + * Access the control's data value by using the value() operation.
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn V4L2StringControl::V4L2StringControl()
> >>> + * \brief Construct a V4L2Control that contains a string value
> >>> + * \param id The control's id
> >>> + * \param value The control's value
> >>> + */
> >>> +
> >>> +/**
> >>> + * \class V4L2U8Control
> >>> + * \brief Specialized V4L2Control class that handles controls with payload of
> >>> + *        V4L2_CTRL_TYPE_U8 type.
> >>> + *
> >>> + * Access the control's data value by using the retrieving the memory location
> >>> + * where the data payload is stored with the mem() operation. The size of
> >>> + * the payload data can be retrieved with the size() operation.
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn V4L2U8Control::V4L2U8Control()
> >>> + * \brief Construct a V4L2Control with payload of uin8_t data
> >>> + * \param id The control's id
> >>> + * \param size The size in bytes of the payload content
> >>> + * \param mem Pointer to the memory location of the payload content
> >>> + *
> >>> + * Memory is reserved in the newly created instance to hold the data payload
> >>> + * and the data content is copied there. The reserved memory is then freed when
> >>> + * the Control is destroyed. The memory where the control's payload was copied
> >>> + * from should be released by the caller.
> >>> + */
> >>> +
> >>> +/**
> >>> + * \class V4L2U16Control
> >>> + * \brief Specialized V4L2Control class that handles controls with payload of
> >>> + *        V4L2_CTRL_TYPE_U16 type.
> >>> + *
> >>> + * Access the control's data value by using the retrieving the memory location
> >>> + * where the data payload is stored with the mem() operation. The size of
> >>> + * the payload data can be retrieved with the size() operation.
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn V4L2U16Control::V4L2U16Control(unsigned int id, unsigned int size, uint16_t *mem)
> >>> + * \brief Construct a V4L2Control with payload of uin16_t data
> >>> + * \param id The control's id
> >>> + * \param size The size in bytes of the payload content
> >>> + * \param mem Pointer to the memory location of the payload content
> >>> + *
> >>> + * Memory is reserved in the newly created instance to hold the data payload
> >>> + * and the data content is copied there. The reserved memory is then freed when
> >>> + * the Control is destroyed. The memory where the control's payload was copied
> >>> + * from should be released by the caller.
> >>> + */
> >>> +
> >>> +/**
> >>> + * \class V4L2U32Control
> >>> + * \brief Specialized V4L2Control class that handles controls with payload of
> >>> + *        V4L2_CTRL_TYPE_U32 type.
> >>> + *
> >>> + * Access the control's data value by using the retrieving the memory location
> >>> + * where the data payload is stored with the mem() operation. The size of
> >>> + * the payload data can be retrieved with the size() operation.
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn V4L2U32Control::V4L2U32Control(unsigned int id, unsigned int size, uint32_t *mem)
> >>> + * \brief Construct a V4L2Control with payload of uin32_t data
> >>> + * \param id The control's id
> >>> + * \param size The size in bytes of the payload content
> >>> + * \param mem Pointer to the memory location of the payload content
> >>> + *
> >>> + * Memory is reserved in the newly created instance to hold the data payload
> >>> + * and the data content is copied there. The reserved memory is then freed when
> >>> + * the Control is destroyed. The memory where the control's payload was copied
> >>> + * from should be released by the caller.
> >>> + */
> >>> +
> >>> +}; /* namespace libcamera */

HI Laurent,

On Mon, Jun 10, 2019 at 11:28:40AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Mon, Jun 10, 2019 at 09:53:23AM +0200, Jacopo Mondi wrote:
> > On Sat, Jun 08, 2019 at 07:17:33PM +0300, Laurent Pinchart wrote:
> > > On Mon, Jun 03, 2019 at 10:47:00AM +0100, Kieran Bingham wrote:
> > >> On 02/06/2019 14:04, 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.
> > >>
> > >> I haven't done a full documentation review here, I'm just trying to
> > >> focus on the code parts first.
> >
> > That's fine, thanks anyway!
> >
> > >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > >>> ---
> > >>>  src/libcamera/include/v4l2_controls.h | 147 +++++++++++++
> > >>>  src/libcamera/meson.build             |   1 +
> > >>>  src/libcamera/v4l2_controls.cpp       | 301 ++++++++++++++++++++++++++
> > >>>  3 files changed, 449 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..df955c2e10b2
> > >>> --- /dev/null
> > >>> +++ b/src/libcamera/include/v4l2_controls.h
> > >>> @@ -0,0 +1,147 @@
> > >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > >>> +/*
> > >>> + * Copyright (C) 2019, Google Inc.
> > >>> + *
> > >>> + * v4l2_controls.h - V4L2 Extended Control Support
> > >>> + */
> > >>> +
> > >>> +#ifndef __LIBCAMERA_V4L2_CONTROL_H__
> > >>> +#define __LIBCAMERA_V4L2_CONTROL_H__
> > >
> > > s/CONTROL/CONTROLS/
> > >
> > >>> +
> > >>> +#include <cstring>
> > >>> +
> > >>> +#include <linux/v4l2-controls.h>
> > >>> +#include <linux/videodev2.h>
> > >>> +
> > >>> +namespace libcamera {
> > >>> +
> > >>> +class V4L2Control
> > >>> +{
> > >>> +public:
> > >>> +	virtual ~V4L2Control()
> > >>> +	{
> > >>> +	}
> > >>> +
> > >>> +	unsigned int id() { return id_; }
> > >>> +	unsigned int size() { return size_; }
> > >>> +	enum v4l2_ctrl_type type() const { return type_; }
> > >>> +
> > >>> +protected:
> > >>> +	V4L2Control(unsigned int id, unsigned int size,
> > >>> +		    enum v4l2_ctrl_type type)
> > >>> +		: id_(id), size_(size), type_(type)
> > >>> +	{
> > >>> +	}
> > >>> +
> > >>> +private:
> > >>> +	unsigned int id_;
> > >>> +	unsigned int size_;
> > >>> +	enum v4l2_ctrl_type type_;
> > >>> +};
> > >>> +
> > >>> +template<typename T>
> > >>> +class V4L2ControlValue : public V4L2Control
> > >>
> > >> Interesting, I thought about templating to do the types for the
> > >> Libcamera Controls, but instead ended up embedding the type in the
> > >> object to avoid having handle casting things ...
> > >>
> > >> Although that then leads me to require having a getBool, getInteger,
> > >> getXXX so we can evaluate both options side by side, and perhaps then
> > >> unify the model for both.
> > >>
> > >> (I'm hoping to get some patches posted soon which will show the
> > >> difference in approach)
> > >>
> > >> The templating might organise memory better, as in my implementation I
> > >> can add a bool, and int to a union, but not a std::string object, so I
> > >> incur wasted space.
> > >
> > > The trouble with the template is that it requires passing containers of
> > > V4L2Control pointers, making it complicated for the caller (especially
> > > when deleting all the elements).
> > >
> > > An alternative would be to create a V4L2Controls class that would wrap
> > > around the container, and provide accessors to get/set values, and to
> > > iterate over controls. How about the following pseudo-code from the
> > > point of view of the caller ?
> > >
> > > 	V4L2Device *dev = ...;
> > > 	V4L2Controls ctrls;
> >
> > I like this API but
> >
> > > 	ctrls.set(V4L2_CID_EXPOSURE, 42);
> > > 	ctrls.set(V4L2_CID_GAIN, 100);
> >
> > I uinderstand I could overload on the second parameter type (but that
> > required called to be very precise, as in
> >
> >                         ctrls.set(V4L2_CID_...,
> >                                   static_cast<uint8_t *>(val));
> >
> >                         ctrls.set(V4L2_CID_...,
> >                                   static_cast<uint16_t *>(val2));
>
> You only need to cast if the type is incorrect in the first place.
>

Not only that, it means that users of the API will have to instantiate
the types of the control very precisely, such as in the above example,
instead of simply declare and "int *p".

Same for uin32_t and uint64_t types. We require the API users to
declare variables with the precise control type, instead of having the
constructor take just an int.

> > Or I could make this a template and specialize based on the type
> > returned by query_ctrl()
>
> But you can't make it a template without allocating each control
> instance dynamically, which isn't very efficient and is quite annoying
> to use (as shown by the patches that delete the controls explicitly).

I think the annoying part is having to delete controls and going
through casts, and I guess we might try to hide in the class that
wraps the container.

>
> You can use the type returned by query_ctrl() internally, and perform
> sanity checks based on it (in order for instance to reject setting a
> string control to an integer value).
>
> > > 	dev->setControls(&ctrls);
> > >
> > > and
> > >
> > > 	V4L2Device *dev = ...;
> > > 	V4L2Controls ctrls;
> > >
> > > 	ctrls.get(V4L2_CID_EXPOSURE);
> > > 	ctrls.get(V4L2_CID_GAIN);
> > >
> > > 	dev->getControls(&ctrls);
> > >
> > > 	unsigned int exposure = ctrls.value(V4L2_CID_EXPOSURE);
> >
> > I'm not sure I can think how that would ever work considering
> > overloading on the return type and deducing the type of a template on
> > return value is not possible.
> >
> > Soemthing like this should not work afaict:
> >
> >         T V4L2Controls::value(unsigned int);
>
> Yes, that won't work. We may need a valueInteger(), valueString(), ...
>

Which defeats a bit the purpose of a unified interface. API users are
required to know the precise type of the control, and this is no
different than casting it to the correct specialized class.

> > As that could only be called on a specialized instance where T has a
> > type assigned already, and the V4L2Control class that wraps the
> > container should not expose any of the specialized derived classes
> > to the API users.
> >
> > I can try to play around a bit with this approach, but if you have any
> > track I should follow let me know.
>

------------------------------------------------------------------------
> I don't have a detailed proposal at the moment, so please try to play
> around a bit :-) What I think is really important is both ease of use
> and efficiency.
>
> > > It's important to make the API easy to use for pipeline handlers and
> > > efficient in terms of both CPU time and memory. I expect tradeoffs of
> > > course :-)
> > >
-------------------------------------------------------------------------

Ah damn, I thought we were going for APIs that were as much as
inefficient and obscure to use as possible! You should have told me
from the beginning, I would have sent better patches...

> > >>> +{
> > >>> +public:
> > >>> +	T value() const { return value_; }
> > >>> +	T *mem() const { return memvalue_; }
> > >>
> > >> s/memvalue_/mem_/? or /mem()/memvalue()/?
> > >>
> > >> (I prefer mem())
> > >>
> > >>> +
> > >>> +protected:
> > >>> +	V4L2ControlValue(unsigned int id, unsigned int size,
> > >>> +			 enum v4l2_ctrl_type type, T value)
> > >>> +		: V4L2Control(id, size, type)
> > >>> +	{
> > >>> +		value_ = value;
> > >>> +		memvalue_ = nullptr;
> > >>> +	}
> > >>> +
> > >>> +	V4L2ControlValue(unsigned int id, unsigned int size,
> > >>> +			 enum v4l2_ctrl_type type, T *value)
> > >>> +		: V4L2Control(id, size, type)
> > >>> +	{
> > >>> +		value_ = 0;
> > >>> +		memvalue_ = static_cast<T *>(new T[size]);
> > >>> +		memcpy(memvalue_, value, size);
> > >>> +	}
> > >>> +
> > >>> +	~V4L2ControlValue()
> > >>> +	{
> > >>> +		delete[] memvalue_;
> > >>> +	}
> > >>> +
> > >>> +private:
> > >>> +	T value_;
> > >>> +	T *memvalue_;
> > >>> +};
> > >>> +
> > >>> +class V4L2IntControl : public V4L2ControlValue<int32_t>
> > >>> +{
> > >>> +public:
> > >>> +	V4L2IntControl(unsigned int id, int32_t value)
> > >>> +		: V4L2ControlValue<int32_t>(id, sizeof(int32_t),
> > >>> +					    V4L2_CTRL_TYPE_INTEGER, value)
> > >>> +	{
> > >>> +	}
> > >>> +};
> > >>> +
> > >>> +class V4L2Int64Control : public V4L2ControlValue<int64_t>
> > >>> +{
> > >>> +public:
> > >>> +	V4L2Int64Control(unsigned int id, int64_t value)
> > >>> +		: V4L2ControlValue<int64_t>(id, sizeof(int64_t),
> > >>> +					    V4L2_CTRL_TYPE_INTEGER64, value)
> > >>> +	{
> > >>> +	}
> > >>> +};
> > >>> +
> > >>> +class V4L2BoolControl : public V4L2ControlValue<bool>
> > >>> +{
> > >>> +public:
> > >>> +	V4L2BoolControl(unsigned int id, bool value)
> > >>> +		: V4L2ControlValue<bool>(id, sizeof(bool),
> > >>> +					 V4L2_CTRL_TYPE_BOOLEAN, value)
> > >>> +	{
> > >>> +	}
> > >>> +};
> > >>> +
> > >>> +class V4L2StringControl : public V4L2ControlValue<std::string>
> > >>
> > >> You might want/need to to support char* types too.
> > >>
> > >>> +{
> > >>> +public:
> > >>> +	V4L2StringControl(unsigned int id, std::string value)
> > >>> +		: V4L2ControlValue<std::string>(id, value.length(),
> > >>> +						V4L2_CTRL_TYPE_STRING, value)
> > >>> +	{
> > >>> +	}
> > >>> +};
> > >>> +
> > >>> +class V4L2U8Control : public V4L2ControlValue<uint8_t>
> > >>> +{
> > >>> +public:
> > >>> +	V4L2U8Control(unsigned int id, unsigned int size, uint8_t *mem)
> > >>> +		: V4L2ControlValue<uint8_t>(id, size, V4L2_CTRL_TYPE_U8, mem)
> > >>> +	{
> > >>> +	}
> > >>> +};
> > >>> +
> > >>> +class V4L2U16Control : public V4L2ControlValue<uint16_t>
> > >>> +{
> > >>> +public:
> > >>> +	V4L2U16Control(unsigned int id, unsigned int size, uint16_t *mem)
> > >>> +		: V4L2ControlValue<uint16_t>(id, size, V4L2_CTRL_TYPE_U16, mem)
> > >>> +	{
> > >>> +	}
> > >>> +};
> > >>> +
> > >>> +class V4L2U32Control : public V4L2ControlValue<uint32_t>
> > >>> +{
> > >>> +public:
> > >>> +	V4L2U32Control(unsigned int id, unsigned int size, uint32_t *mem)
> > >>> +		: V4L2ControlValue<uint32_t>(id, size, V4L2_CTRL_TYPE_U32, mem)
> > >>> +	{
> > >>> +	}
> > >>> +};
> > >>> +
> > >>> +}; /* namespace libcamera */
> > >>> +
> > >>> +#endif /* __LIBCAMERA_V4L2_CONTROL_H__ */
> > >>> +
> > >>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > >>> index 6d858a22531e..fa1fbcb5faf5 100644
> > >>> --- a/src/libcamera/meson.build
> > >>> +++ b/src/libcamera/meson.build
> > >>> @@ -22,6 +22,7 @@ libcamera_sources = files([
> > >>>      'timer.cpp',
> > >>>      'utils.cpp',
> > >>>      'v4l2_base.cpp',
> > >>> +    'v4l2_controls.cpp',
> > >>>      'v4l2_device.cpp',
> > >>>      'v4l2_subdevice.cpp',
> > >>>  ])
> > >>> diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> > >>> new file mode 100644
> > >>> index 000000000000..f50f3cdfa7d7
> > >>> --- /dev/null
> > >>> +++ b/src/libcamera/v4l2_controls.cpp
> > >>> @@ -0,0 +1,301 @@
> > >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > >>> +/*
> > >>> + * Copyright (C) 2019, Google Inc.
> > >>> + *
> > >>> + * v4l2_controls.cpp - V4L2 Extended Control Support
> > >>> + */
> > >>> +
> > >>> +/**
> > >>> + * \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
> > >>
> > >> "an application" or "applications"
> > >>
> > >> "a set of"
> > >>
> > >> application? Are we talking about applications that use libcamera? Or
> > >> applications in general?
> > >>
> > >>> + * of configurable parameters on the video device or subdevice of interest.  The
> > >>
> > >> /.  The/. The/ <double space>
> > >>
> > >>> + * 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 'setControl()' operation call or
> > >>> + * retrieved with a 'getControl()' one.
> > >>> + *
> > >>> + * 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
> > >>> + * matrixes, stored in a contiguous memory locations associated with the control
> > >>
> > >> s/matrixes/matrices/
> > >>
> > >>> + * and called 'the payload'. The content have to be opportunely copied into the
> > >>> + * application memory when retrieving a control's value and provided to the V4L2
> > >>> + * device when setting it.
> > >>> + *
> > >>> + * Libcamera supports control using the V4L2 'Extended Control' framework, which
> > >>> + * allows easier handling of controls with payloads of arbitrary sizes.
> > >>> + *
> > >>> + * The Libcamera V4L2 Controls framework operates on lists of controls, to match
> > >>> + * the V4L2 extended controls framework, but also provides operations to get
> > >>> + * and set single controls. The interface to set and get control is implemented
> > >>> + * by the V4L2Base class, while this file only provides the data type
> > >>> + * definitions.
> > >>> + *
> > >>> + * The Libcamera V4L2 control framework data types define a base class
> > >>> + * V4L2Control that contains the fields common to all controls, regardless of
> > >>> + * their type, such as the  control's id, its type and the expected control's
> > >>> + * value data size. The V4L2Control class is not meant to be instantiated
> > >>> + * directly, but is instead used as placeholder to store controls in the
> > >>> + * standard library provided containers.
> > >>> + *
> > >>> + * A parametric derived class V4L2ControlValue stores the control's value and
> > >>> + * provide accessors to the value itself for control with no payload, or to the
> > >>> + * memory area that contains the control's data payload. This class is not
> > >>> + * intended to be directly used and cannot be constructed, but it is used
> > >>> + * instead to define specialized derived classes which specialize the control's
> > >>> + * data value type.
> > >>> + *
> > >>> + * In order to set and get controls, user of the Libcamera V4L2 control
> > >>> + * framework should create instances of the specialized derived classes,
> > >>> + * which are publicly constructible, and use them to access the control's data
> > >>> + * content using the 'value()' method for controls with no payload, or
> > >>> + * retrieving reference to the memory location that contains the data payload
> > >>> + * with the 'mem()' operation for controls which transport a data payload.
> > >>> + *
> > >>> + * \todo Support setting controls with data payload.
> > >>> + */
> > >>> +
> > >>> +namespace libcamera {
> > >>> +
> > >>> +/**
> > >>> + * \class V4L2Control
> > >>> + * \brief Base class for V4L2 Controls
> > >>> + *
> > >>> + * The V4L2Control base class is the base class that contains the fields common
> > >>> + * to all controls (id, type and size).
> > >>> + *
> > >>> + * This class is not meant to be instantiated directly, but is instead used as a
> > >>> + * place holder to store controls in arrays or other containers. User of the
> > >>> + * libcamera V4L2 control framework should access the controls content by
> > >>> + * instantiating one of the provided specialized derived classes.
> > >>> + */
> > >>> +
> > >>> +/**
> > >>> + * \fn V4L2Control::V4L2Control
> > >>> + * \brief Construct a V4L2Control instance
> > >>> + * \param id The control's id
> > >>> + * \param size The control's data size
> > >>> + * \param type The control's type
> > >>> + */
> > >>> +
> > >>> +/**
> > >>> + * \fn V4L2Control::id()
> > >>> + * \brief Retrieve the control's id
> > >>> + *
> > >>> + * Retrieve the control's numerical id value as defined by the V4L2
> > >>> + * specification.
> > >>> + *
> > >>> + * \return The control's id
> > >>> + */
> > >>> +
> > >>> +/**
> > >>> + * \fn V4L2Control::size()
> > >>> + * \brief Retrieve the control's data value size in bytes
> > >>> + *
> > >>> + * \todo Better define the value of size() for controls with payload data.
> > >>> + *
> > >>> + * \return The control's size
> > >>> + */
> > >>> +
> > >>> +/**
> > >>> + * \fn V4L2Control::type()
> > >>> + * \brief Retrieve the control's type
> > >>> + *
> > >>> + * Retrieve the control's type as defined by the V4L2 specification.
> > >>> + *
> > >>> + * \return The control's type
> > >>> + */
> > >>> +
> > >>> +/**
> > >>> + * \class V4L2ControlValue
> > >>> + * \brief Template base class that represent values of a V4L2 control
> > >>> + *
> > >>> + * The V4L2ControlValue template base class represents a V4L2 control with
> > >>> + * its different value types.
> > >>> + *
> > >>> + * It provides two operations to access the control's value or the pointer
> > >>> + * to the memory location that contains to the control's value data payload.
> > >>> + */
> > >>> +
> > >>> +/**
> > >>> + * \fn V4L2ControlValue::V4L2ControlValue(unsigned int id, unsigned int size, enum v4l2_ctrl_type type, T value)
> > >>> + * \brief Contruct a V4L2ControlValue with a value
> > >>
> > >> /Contruct/Construct/
> > >>
> > >>> + * \param id The control's id
> > >>> + * \param size The control's size
> > >>> + * \param type The control's type
> > >>> + * \param value The control's value
> > >>> + */
> > >>> +
> > >>> +/**
> > >>> + * \fn V4L2ControlValue::V4L2ControlValue(unsigned int id, unsigned int size, enum v4l2_ctrl_type type, T* value)
> > >>> + * \brief Contruct a V4L2ControlValue with a pointer to a data payload
> > >>
> > >> /Contruct/Construct/
> > >>
> >
> > Thanks Kieran, I'll take you suggestions on comments in.
> >
> > >>> + * \param id The control's id
> > >>> + * \param size The control's size
> > >>> + * \param type The control's type
> > >>> + * \param value The pointer to the control's data payload
> > >>> + */
> > >>> +
> > >>> +/**
> > >>> + * \fn V4L2ControlValue::~V4L2ControlValue
> > >>> + * \brief Release the memory reserved for the control's data payload, if any
> > >>> + */
> > >>> +
> > >>> +/**
> > >>> + * \fn V4L2ControlValue::value()
> > >>> + * \brief Retrieve the control's value
> > >>> + *
> > >>> + * Retrieve the control's value. Valid only for controls with no payload.
> > >>> + * Access a control's value by calling the value() operation on instances
> > >>> + * of the specialized derived classes.
> > >>> + */
> > >>> +
> > >>> +/**
> > >>> + * \fn V4L2ControlValue::mem()
> > >>> + * \brief Retrieve a pointer to the memory location of the control's data
> > >>> + *
> > >>> + * Retrieve a pointer to the memory location that contains the control's data
> > >>> + * payload. Valid only for controls with data payload. Access the memory
> > >>> + * location of the control's data by calling the mem() operation on instances
> > >>> + * of the specialized derived classes.
> > >>> + */
> > >>> +
> > >>> +/**
> > >>> + * \class V4L2IntControl
> > >>> + * \brief Specialized V4L2Control class that handles controls of
> > >>> + *        V4L2_CTRL_TYPE_INTEGER type.
> > >>> + *
> > >>> + * Access the control's data value by using the value() operation.
> > >>> + */
> > >>> +
> > >>> +/**
> > >>> + * \fn V4L2IntControl::V4L2IntControl()
> > >>> + * \brief Construct a V4L2Control that contains an int value
> > >>> + * \param id The control's id
> > >>> + * \param value The control's value
> > >>> + */
> > >>> +
> > >>> +/**
> > >>> + * \class V4L2Int64Control
> > >>> + * \brief Specialized V4L2Control class that handles controls of
> > >>> + *        V4L2_CTRL_TYPE_INTEGER64 type.
> > >>> + *
> > >>> + * Access the control's data value by using the value() operation.
> > >>> + */
> > >>> +
> > >>> +/**
> > >>> + * \fn V4L2Int64Control::V4L2Int64Control()
> > >>> + * \brief Construct a V4L2Control that contains an int64 value
> > >>> + * \param id The control's id
> > >>> + * \param value The control's value
> > >>> + */
> > >>> +
> > >>> +/**
> > >>> + * \class V4L2BoolControl
> > >>> + * \brief Specialized V4L2Control class that handles controls of
> > >>> + *        V4L2_CTRL_TYPE_BOOLEAN type.
> > >>> + *
> > >>> + * Access the control's data value by using the value() operation.
> > >>> + */
> > >>> +
> > >>> +/**
> > >>> + * \fn V4L2BoolControl::V4L2BoolControl()
> > >>> + * \brief Construct a V4L2Control that contains a boolean value
> > >>> + * \param id The control's id
> > >>> + * \param value The control's value
> > >>> + */
> > >>> +
> > >>> +/**
> > >>> + * \class V4L2StringControl
> > >>> + * \brief Specialized V4L2Control class that handles controls of
> > >>> + *        V4L2_CTRL_TYPE_STRING type.
> > >>> + *
> > >>> + * Access the control's data value by using the value() operation.
> > >>> + */
> > >>> +
> > >>> +/**
> > >>> + * \fn V4L2StringControl::V4L2StringControl()
> > >>> + * \brief Construct a V4L2Control that contains a string value
> > >>> + * \param id The control's id
> > >>> + * \param value The control's value
> > >>> + */
> > >>> +
> > >>> +/**
> > >>> + * \class V4L2U8Control
> > >>> + * \brief Specialized V4L2Control class that handles controls with payload of
> > >>> + *        V4L2_CTRL_TYPE_U8 type.
> > >>> + *
> > >>> + * Access the control's data value by using the retrieving the memory location
> > >>> + * where the data payload is stored with the mem() operation. The size of
> > >>> + * the payload data can be retrieved with the size() operation.
> > >>> + */
> > >>> +
> > >>> +/**
> > >>> + * \fn V4L2U8Control::V4L2U8Control()
> > >>> + * \brief Construct a V4L2Control with payload of uin8_t data
> > >>> + * \param id The control's id
> > >>> + * \param size The size in bytes of the payload content
> > >>> + * \param mem Pointer to the memory location of the payload content
> > >>> + *
> > >>> + * Memory is reserved in the newly created instance to hold the data payload
> > >>> + * and the data content is copied there. The reserved memory is then freed when
> > >>> + * the Control is destroyed. The memory where the control's payload was copied
> > >>> + * from should be released by the caller.
> > >>> + */
> > >>> +
> > >>> +/**
> > >>> + * \class V4L2U16Control
> > >>> + * \brief Specialized V4L2Control class that handles controls with payload of
> > >>> + *        V4L2_CTRL_TYPE_U16 type.
> > >>> + *
> > >>> + * Access the control's data value by using the retrieving the memory location
> > >>> + * where the data payload is stored with the mem() operation. The size of
> > >>> + * the payload data can be retrieved with the size() operation.
> > >>> + */
> > >>> +
> > >>> +/**
> > >>> + * \fn V4L2U16Control::V4L2U16Control(unsigned int id, unsigned int size, uint16_t *mem)
> > >>> + * \brief Construct a V4L2Control with payload of uin16_t data
> > >>> + * \param id The control's id
> > >>> + * \param size The size in bytes of the payload content
> > >>> + * \param mem Pointer to the memory location of the payload content
> > >>> + *
> > >>> + * Memory is reserved in the newly created instance to hold the data payload
> > >>> + * and the data content is copied there. The reserved memory is then freed when
> > >>> + * the Control is destroyed. The memory where the control's payload was copied
> > >>> + * from should be released by the caller.
> > >>> + */
> > >>> +
> > >>> +/**
> > >>> + * \class V4L2U32Control
> > >>> + * \brief Specialized V4L2Control class that handles controls with payload of
> > >>> + *        V4L2_CTRL_TYPE_U32 type.
> > >>> + *
> > >>> + * Access the control's data value by using the retrieving the memory location
> > >>> + * where the data payload is stored with the mem() operation. The size of
> > >>> + * the payload data can be retrieved with the size() operation.
> > >>> + */
> > >>> +
> > >>> +/**
> > >>> + * \fn V4L2U32Control::V4L2U32Control(unsigned int id, unsigned int size, uint32_t *mem)
> > >>> + * \brief Construct a V4L2Control with payload of uin32_t data
> > >>> + * \param id The control's id
> > >>> + * \param size The size in bytes of the payload content
> > >>> + * \param mem Pointer to the memory location of the payload content
> > >>> + *
> > >>> + * Memory is reserved in the newly created instance to hold the data payload
> > >>> + * and the data content is copied there. The reserved memory is then freed when
> > >>> + * the Control is destroyed. The memory where the control's payload was copied
> > >>> + * from should be released by the caller.
> > >>> + */
> > >>> +
> > >>> +}; /* namespace libcamera */
>
> --
> Regards,
>
> Laurent Pinchart