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

[Jacopo Mondi]

Hi Laurent,

On Sat, Jun 22, 2019 at 08:09:06PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thu, Jun 20, 2019 at 05:31:39PM +0200, Jacopo Mondi wrote:
> > Add Libcamera V4L2 control support, implemented using the V4L2 Extended
>
> s/Libcamera/libcamera/ :-)
>
> > Control APIs. This patch defines the types used to define and manage controls.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/include/v4l2_controls.h |  86 ++++++++
> >  src/libcamera/meson.build             |   1 +
> >  src/libcamera/v4l2_controls.cpp       | 282 ++++++++++++++++++++++++++
> >  3 files changed, 369 insertions(+)
> >  create mode 100644 src/libcamera/include/v4l2_controls.h
> >  create mode 100644 src/libcamera/v4l2_controls.cpp
> >
> > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h
> > new file mode 100644
> > index 000000000000..acd23dabd831
> > --- /dev/null
> > +++ b/src/libcamera/include/v4l2_controls.h
> > @@ -0,0 +1,86 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * v4l2_controls.h - V4L2 Controls Support
> > + */
> > +
> > +#ifndef __LIBCAMERA_V4L2_CONTROLS_H__
> > +#define __LIBCAMERA_V4L2_CONTROLS_H__
> > +
> > +#include <string>
> > +#include <vector>
> > +
> > +#include <stdint.h>
> > +
> > +#include <linux/v4l2-controls.h>
> > +#include <linux/videodev2.h>
> > +
> > +namespace libcamera {
> > +
> > +class V4L2ControlInfo
> > +{
> > +public:
> > +	V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl);
> > +
> > +	unsigned int id() const { return id_; }
> > +	unsigned int type() const { return type_; }
> > +	size_t size() const { return size_; }
> > +	const std::string &name() const { return name_; }
> > +
> > +private:
> > +	unsigned int type_;
> > +	std::string name_;
> > +	unsigned int id_;
> > +	size_t size_;
>
> As requested by Niklas,
>
> 	unsigned int id_;
> 	unsigned int type_;
> 	size_t size_;
> 	std::string name_;
>
> ?
>
> > +};
> > +
> > +class V4L2Control
> > +{
> > +public:
> > +	V4L2Control(unsigned int id, int value = 0)
> > +		: id_(id), value_(value) {}
> > +
> > +	int64_t value() const { return value_; }
> > +	void setValue(int64_t value) { value_ = value; }
> > +
> > +	unsigned int id() const { return id_; }
> > +
> > +private:
> > +	unsigned int id_;
> > +	int64_t value_;
> > +};
> > +
> > +class V4L2Controls
>
> I was wondering if we should rename this to V4L2ControlList, as
> V4L2Controls is very close to V4L2Control and easy to mistake.
>
> > +{
> > +public:
> > +	V4L2Controls &operator=(const V4L2Controls &) = delete;
>
> As commented before, I think you should either delete both the
> assignement operator and the copy constructor, or neither of them.
>
> > +
> > +	using iterator = std::vector<V4L2Control>::iterator;
> > +	using const_iterator = std::vector<V4L2Control>::const_iterator;
> > +
> > +	iterator begin() { return controls_.begin(); }
> > +	const_iterator begin() const { return controls_.begin(); }
> > +	iterator end() { return controls_.end(); }
> > +	const_iterator end() const { return controls_.end(); }
> > +
> > +	bool empty() const { return controls_.empty(); }
> > +	size_t size() const { return controls_.size(); }
>
> std::size_t to match std::vector ?
>
> > +
> > +	void reset() { controls_.clear(); }
> > +	void add(unsigned int id, int64_t value = 0);
> > +
> > +	V4L2Control *operator[](unsigned int index)
> > +	{
> > +		return &controls_[index];
> > +	}
>
> Do we need to access a control by index for any other purpose than
> iterating over the controls, which the iterator already gives us ?
> Couldn't operator[] take the control ID, and getControl() be removed ?
>

I really don't like this.
operator[] for vector works on indexes, I would now use the same
method but with a different semantic. It would be confusing for
everyone. I can drop this operation if you like to.

> > +
> > +	V4L2Control *getControl(unsigned int id);
> > +
> > +private:
> > +	std::vector<V4L2Control> controls_;
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_V4L2_CONTROLS_H__ */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index f26ad5b2dc57..985aa7e8ab0e 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -23,6 +23,7 @@ libcamera_sources = files([
> >      'stream.cpp',
> >      'timer.cpp',
> >      'utils.cpp',
> > +    'v4l2_controls.cpp',
> >      'v4l2_device.cpp',
> >      'v4l2_subdevice.cpp',
> >      'v4l2_videodevice.cpp',
> > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> > new file mode 100644
> > index 000000000000..abf596e7f1ef
> > --- /dev/null
> > +++ b/src/libcamera/v4l2_controls.cpp
> > @@ -0,0 +1,282 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * v4l2_controls.cpp - V4L2 Controls Support
> > + */
> > +
> > +#include "v4l2_controls.h"
> > +
> > +/**
> > + * \file v4l2_controls.h
> > + * \brief Support for V4L2 Controls using the V4L2 Extended Controls APIs.
>
> s/\.$//
>
> > + *
> > + * The V4L2 defined "Control API" allows application to inspect and modify set
>
> "The V4L2 Control API allows applications ..."
>
> s/set/sets/
>
> > + * of configurable parameters on the video device or subdevice of interest. The
>
> s/on the video device or subdevice of interest/on a video device or subdevice/
>
> > + * nature of the parameters an application could modify using the control
>
> s/could/can/
>
> > + * framework depends on what the driver implements support for, and on the
> > + * characteristics of the underlying hardware platform. Generally controls are
> > + * used to modify user visible settings, such as the image brightness and
> > + * exposure time, or non-standard parameters which cannot be controlled through
> > + * the V4L2 format negotiation API.
>
> The last two sentences describe more the V4L2 API than the V4L2Control*
> classes API. I expect users of this code to know about V4L2 already, so
> they could be dropped, but I also don't mind if you keep them. Just
> don't invest too much time in writing document about V4L2 itself, as
> that's a bit out of scope.
>

I feel it doesn't hurt, as it doesn't go in great lenght describing
controls but I'll drop.

> > + *
> > + * Controls are identified by a numerical ID, defined by the V4L2 kernel headers
> > + * and have an associated type. Each control has a value, which is the data
> > + * that can be modified with V4L2Device::setControls() or retrieved with
> > + * V4L2Device::getControls().
> > + *
> > + * The control's type along with the control's flags defines the type of the
>
> s/defines/define/
>
> > + * control's value content. Controls might transport a single data value
>
> s/might/can/
>
> > + * stored in variable inside the control, or they might as well deal with more
>
> s/might/can/
>
> > + * complex data types, such as arrays of matrices, stored in a contiguous
> > + * memory locations associated with the control and called 'the payload'. Such
> > + * controls are called 'compound controls' and are currently not supported by
> > + * the libcamera V4L2 control framework.
>
> Here too you could simply have said that we don't support compound
> controls yet, but I don't mind keeping the more detailed text as-is.
>
> > + *
> > + * libcamera implements support for controls using the V4L2 'Extended Control'
> > + * API, which allows future handling of controls with payloads of arbitrary
>
> It's not the controls that are extended, it's the API. It's the extended
> V4L2 control API, not the V4L2 API for extended controls. I would thus
> remove the quotes.
>

I see. I should have moved the ending quote to include the "API" part.

> > + * sizes.
> > + *
> > + * The libcamera V4L2 Controls framework operates on lists of controls,
> > + * wrapped by the V4L2Controls class, to match the V4L2 extended controls API.
>
> Calling it V4L2ControlList would match the description nicely :-)
>

Ack

> > + * The interface to set and get control is implemented by the V4L2Device
> > + * class, and this file only provides the data type definitions.
> > + *
> > + * \todo Add support for compound controls
> > + */
> > +
> > +namespace libcamera {
> > +
> > +/**
> > + * \class V4L2ControlInfo
> > + * \brief Information on a V4L2 control
> > + *
> > + * The V4L2ControlInfo class represent all the information related to
>
> s/represent/represents/
>
> > + * a V4L2 control, such as its ID, its type, its user-readable name and the
> > + * expected size of its value data.
> > + *
> > + * V4L2ControlInfo instances are created by inspecting the fieldS of a struct
> > + * v4l2_query_ext_ctrl structure, after it has been filled by the device
> > + * driver as a consequence of a VIDIOC_QUERY_EXT_CTRL ioctl call.
> > + *
> > + * This class does not contain the control value, but only static information
> > + * on the control, which shall be cached by the caller at initialisation time
> > + * or the first time the control information is accessed.
> > + */
> > +
> > +/**
> > + * \brief Construct a V4L2ControlInfo from a struct v4l2_query_ext_ctrl
> > + * \param ctrl The struct v4l2_query_ext_ctrl as returned by the kernel
> > + */
> > +V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
> > +{
> > +	id_ = ctrl.id;
> > +	type_ = ctrl.type;
> > +	name_ = static_cast<const char *>(ctrl.name);
> > +	size_ = ctrl.elem_size * ctrl.elems;
> > +}
> > +
> > +/**
> > + * \fn V4L2ControlInfo::id()
> > + * \brief Retrieve the control ID
> > + * \return The V4L2 control ID
> > + */
> > +
> > +/**
> > + * \fn V4L2ControlInfo::type()
> > + * \brief Retrieve the control type as defined by V4L2_CTRL_TYPE_*
> > + * \return The V4L2 control type
> > + */
> > +
> > +/**
> > + * \fn V4L2ControlInfo::size()
> > + * \brief Retrieve the control value data size (in bytes)
> > + * \return The V4L2 control value data size
> > + */
> > +
> > +/**
> > + * \fn V4L2ControlInfo::name()
> > + * \brief Retrieve the control user readable name
> > + * \return The V4L2 control user readable name
> > + */
> > +
> > +/**
> > + * \class V4L2Control
> > + * \brief A V4L2 control value
> > + *
> > + * The V4L2Control class represent the value of a V4L2 control.
> > + * The class stores values that have been read from or will be applied to
> > + * a V4L2 device.
> > + *
> > + * The value stored in the class instances does not reflect what is actually
> > + * applied to the hardware but is a pure software cache optionally initialized
> > + * at control creation time and only modified by a control read operation.
>
> With setControls() updating the value, this isn't true anymore.
>

True. Nice catch.

> > + *
> > + * The write and read controls the V4L2Control class instances are not meant
> > + * to be directly used but are instead intended to be grouped in
> > + * V4L2Controls instances, which are then passed as parameters to
> > + * V4L2Device::setControls() and V4L2Device::getControls() operations.
> > + */
> > +
> > +/**
> > + * \fn V4L2Control::V4L2Control
> > + * \brief Construct a V4L2 control with ID \a id and value \a value
>
> "with an \a id and a \a value" ? Or "Construct an instance of control \a
> ID with a \a value" ?
>

I'm always uncertain on how to handle these situation, where the name
of the parameter matches the generic name.

> > + * \param id The V4L2 control ID
> > + * \param value The control value
> > + */
> > +
> > +/**
> > + * \fn V4L2Control::value()
> > + * \brief Retrieve the value of the control
> > + *
> > + * It is important to notice that the returned value is not read from the
>
> s/notice/note/
>
> > + * hardware at the time this operation is called, but it's the cached value
> > + * obtained the most recent time the control has been read with
> > + * V4L2Device::getControls() or the value of the control has been set to by the
> > + * user with the add() operation.
>
> You should mention setControls() here. How about
>
> This method returns the cached control value, initially set by
> V4L2Controls::add() and then updated when the controls are read or
> written with V4L2Device::getControls() and V4L2Device::setControls().
>
> > + *
> > + * \return The V4L2 control value. The value might be 0 if the control has
> > + * never been read from the device before this operation is called.
>
> I would drop the second sentence, it's redundant with the above
> paragraph, and a bit confusing too as the value may also be 0 after
> being read.
>

Ack.

> > + */
> > +
> > +/**
> > + * \fn V4L2Control::setValue()
> > + * \brief Set the value of the control
> > + * \param value The new V4L2 control value
> > + *
> > + * It is important to notice that the value is not applied to the
> > + * hardware at the time this operation is called, but it will only
> > + * be actually applied by calling V4L2Device::setControls().
>
> Let's simplify it a bit, by explaining what the method does, not what it
> does not do :-)
>
> "This method stores the control value, which will be applied to the
> device when calling V4L2Device::setControls()."
>
> > + */
> > +
> > +/**
> > + * \fn V4L2Control::id()
> > + * \brief Retrieve the control ID this instance refers to
> > + * \return The V4L2Control ID
> > + */
> > +
> > +/**
> > + * \class V4L2Controls
> > + * \brief Container of V4L2Control instances
> > + *
> > + * The V4L2Controls class works as a container for a list of V4L2Control
> > + * instances. The class provides operations to add a new control to the list,
> > + * get back a control value, and reset the list of controls it contains.
> > + *
> > + * In order to set and get controls, user of the libcamera V4L2 control
> > + * framework should operate on instances of the V4L2Controls class, and use
> > + * them as argument for the V4L2Device::setControls() and
> > + * V4L2Device::getControls() operations, which write and read a list of
> > + * controls from or to a V4L2 device (a video device or a subdevice).
>
> "to and from" (as it's "write and read")
>
> > + *
> > + * Controls are added to a V4L2Controls instance with the add() method, with
> > + * or without an initial value.
> > + *
> > + * To write controls to a device, the controls of interest shall be added with
> > + * an initial value by calling V4L2Controls::add(unsigned int id, int64_t
>
> s/an initial value/a value/
>
> > + * value) to prepare for a write operation. Once the values of all controls of
> > + * interest have been added, the V4L2Controls instance is passed to the
> > + * V4L2Device::setControls(), which sets the controls on the device.
> > + *
> > + * To read controls from a device, the desired controls are added with
> > + * V4L2Controls::add(unsigned int id) to prepare for a read operation. The
> > + * V4L2Controls instance is then passed to V4L2Device::getControls(), which
> > + * reads the controls from the device and updates the values stored in
> > + * V4L2Controls.
> > + *
> > + * V4L2Controls instances can be reset to remove all controls they contain and
> > + * prepare to be re-used for a new control write/read sequence.
> > + */
> > +
> > +/**
> > + * \typedef V4L2Controls::iterator
> > + * \brief Iterator on the V4L2 controls contained in the instance
> > + */
> > +
> > +/**
> > + * \typedef V4L2Controls::const_iterator
> > + * \brief Const iterator on the V4L2 controls contained in the instance
> > + */
> > +
> > +/**
> > + * \fn iterator V4L2Controls::begin()
> > + * \brief Retrieve an iterator to the first V4L2Control in the instance
> > + * \return An iterator to the first V4L2 control
> > + */
> > +
> > +/**
> > + * \fn const_iterator V4L2Controls::begin() const
> > + * \brief Retrieve a constant iterator to the first V4L2Control in the instance
> > + * \return A constant iterator to the first V4L2 control
> > + */
> > +
> > +/**
> > + * \fn iterator V4L2Controls::end()
> > + * \brief Retrieve an iterator pointing to the past-the-end V4L2Control in the
> > + * instance
> > + * \return An iterator to the element following the last V4L2 control in the
> > + * instance
> > + */
> > +
> > +/**
> > + * \fn const_iterator V4L2Controls::end() const
> > + * \brief Retrieve a constant iterator pointing to the past-the-end V4L2Control
> > + * in the instance
> > + * \return A constant iterator to the element following the last V4L2 control
> > + * in the instance
> > + */
> > +
> > +/**
> > + * \fn V4L2Controls::empty()
> > + * \brief Verify if the instance does not contain any control
> > + * \return True if the instance does not contain any control, false otherwise
> > + */
> > +
> > +/**
> > + * \fn V4L2Controls::size()
> > + * \brief Retrieve the number on controls in the instance
> > + * \return The number of V4L2Control stored in the instance
> > + */
> > +
> > +/**
> > + * \fn V4L2Controls::reset()
> > + * \brief Removes all controls in the instance
>
> s/Removes/Remove/
>
> > + */
> > +
> > +/**
> > + * \brief Add control with ID \a id and optional \a value to the instance
> > + * \param id The V4L2 control ID (V4L2_CID_*)
> > + * \param value The V4L2 control value
> > + *
> > + * V4L2Control can be added with an explicit value, or without one. In that
> > + * case the control's value is defaulted to 0.
>
> s/is defaulted/defaults to/
>
> But I think we can skip the sentence, the doxygen documentation should
> show the = 0.

Ack

Thanks
   j

>
> > + */
> > +void V4L2Controls::add(unsigned int id, int64_t value)
> > +{
> > +	controls_.emplace_back(id, value);
> > +}
> > +
> > +/**
> > + * \fn V4L2Controls::operator[](unsigned int index)
> > + * \brief Access the control at index \a index
> > + * \param index The index to access
> > + * \return The V4L2 control at index \a index
> > + */
> > +
> > +/**
> > + * \brief Get a pointer the control with ID \a id
> > + * \param id The V4L2 control ID (V4L2_CID_xx)
> > + * \return A pointer to the V4L2Control with ID \a id or nullptr if the
> > + * control ID is not part of this instance.
> > + */
> > +V4L2Control *V4L2Controls::getControl(unsigned int id)
> > +{
> > +	for (V4L2Control &ctrl : controls_) {
> > +		if (ctrl.id() == id)
> > +			return &ctrl;
> > +	}
> > +
> > +	return nullptr;
> > +}
> > +
> > +} /* namespace libcamera */
>
> --
> Regards,
>
> Laurent Pinchart
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190624/599ca428/attachment.sig>