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

[Jacopo Mondi]

Hi Laurent,

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

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

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

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

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

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

> > > > +};
> > > > +
> > > > +} /* namespace libcamera */
> > > > +
> > > > +#endif /* __LIBCAMERA_V4L2_CONTROLS_H__ */
> > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > > index f26ad5b2dc57..985aa7e8ab0e 100644
> > > > --- a/src/libcamera/meson.build
> > > > +++ b/src/libcamera/meson.build
> > > > @@ -23,6 +23,7 @@ libcamera_sources = files([
> > > >      'stream.cpp',
> > > >      'timer.cpp',
> > > >      'utils.cpp',
> > > > +    'v4l2_controls.cpp',
> > > >      'v4l2_device.cpp',
> > > >      'v4l2_subdevice.cpp',
> > > >      'v4l2_videodevice.cpp',
> > > > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> > > > new file mode 100644
> > > > index 000000000000..75b9c29ca133
> > > > --- /dev/null
> > > > +++ b/src/libcamera/v4l2_controls.cpp
> > > > @@ -0,0 +1,337 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2019, Google Inc.
> > > > + *
> > > > + * v4l2_controls.cpp - V4L2 Extended Control Support
> > > > + */
> > > > +
> > > > +#include "v4l2_controls.h"
> > > > +
> > > > +/**
> > > > + * \file v4l2_controls.h
> > > > + * \brief Support for V4L2 Controls using the V4L2 Extended Controls APIs.
> > > > + *
> > > > + * The V4L2 defined "Control API" allows application to inspect and modify set
> > > > + * of configurable parameters on the video device or subdevice of interest. The
> > > > + * nature of the parameters an application could modify using the control
> > > > + * framework depends on what the driver implements support for, and on the
> > > > + * characteristics of the underlying hardware platform. Generally controls are
> > > > + * used to modify user visible settings, such as the image brightness and
> > > > + * exposure time, or non-standard parameters which cannot be controlled through
> > > > + * the V4L2 format negotiation API.
> > > > + *
> > > > + * Controls are identified by a numerical id, defined by the V4L2 kernel headers
> > > > + * and have an associated type and class. Each control has a 'value', which is
> > >
> > > I would drop ' and class', we don't care about control classes.
> > >
> > > > + * the data that can be modified with a call to  the 'V4L2Device::setControls()'
> > >
> > > s/  / /
> > >
> > > > + * operation or retrieved with a call to 'V4L2Device::getControls()'.
> > >
> > > I don't think you need to enclose the method name in quotes, you can
> > > write
> > >
> > >  * Each control has a value, which is the data that can be modified with
> > >  * V4L2Device::setControls() or retrieved with V4L2Device::getControls().
> > >
> > > > + *
> > > > + * A control class defines the control purpose while its type (along with the
> > >
> > > Similarly, you can drop the class here.
> > >
> > > > + * control's flags) defines the type of the control's value content. Controls
> > > > + * might transport a single data value stored in variable inside the control, or
> > > > + * they might as well deal with more complex data types, such as arrays of
> > > > + * matrices, stored in a contiguous memory locations associated with the control
> > > > + * and called 'the payload'. Such controls are called 'compound controls' and
> > > > + * are currently not supported by the Libcamera V4L2 control framework.
> > >
> > > s/Libcamera/libcamera/
> > >
> > > > + *
> > > > + * \todo Add support for compound controls
> > > > + *
> > > > + * Libcamera implements support for control using the V4L2 'Extended Control'
> > >
> > > s/Libcamera/libcamera/
> > > s/control/controls/
> > >
> > > > + * framework, which allows easier future handling of controls with payloads
> > >
> > > s/framework/API/
> > > s/easier //
> > >
> > > > + * of arbitrary sizes.
> > > > + *
> > > > + * The Libcamera V4L2 Controls framework operates on lists of controls, wrapped
> > >
> > > s/Libcamera/libcamera/
> > >
> > > > + * by the V4L2Controls class, to match the V4L2 extended controls framework.
> > >
> > > s/framework/API/
> > >
> > > > + * The interface to set and get control is implemented by the V4L2Device class,
> > > > + * and this file only provides the data type definitions.
> > > > + */
> > > > +
> > > > +namespace libcamera {
> > > > +
> > > > +/**
> > > > + * \class V4L2ControlInfo
> > > > + * \brief Information on a V4L2 control
> > > > + *
> > > > + * The V4L2ControlInfo class represent all the informations relative to
> > >
> > > s/informations/information/ (uncoutable)
> > > s/relative/related/
> > >
> > > > + * a V4L2 control, such as its id, its type, its user-readable name and the
> > > > + * expected size of its value data.
> > > > + *
> > > > + * V4L2ControlInfo instances are created by inspecting the field of
> > > > + * a struct v4l2_query_ext_ctrl structure, after it has been filled by the
> > > > + * device driver as a consequence of an VIDIOC_QUERY_EXT_CTRL ioctl call.
> > >
> > > s/of an/of a/
> > >
> > > > + *
> > > > + * This class does not contain the control value, but only static informations
> > >
> > > s/informations/information/
> > >
> > > > + * on the control, which should ideally be cached the first time the control
> > >
> > > I'd be a bit more directive here, "which shall be cached by the caller
> > > at initialisation time or the first time the control information is
> > > needed." and drop the rest of the sentence.
> > >
> > > > + * is accessed to be reused each time the control value need to be read or
> > >
> > > s/need/needs/
> > >
> > > > + * applied to the hardware.
> > > > + */
> > > > +
> > > > +/**
> > > > + * \brief Construct a V4L2ControlInfo from a struct v4l2_query_ext_ctrl
> > > > + */
> > > > +V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
> > > > +{
> > > > +	id_ = ctrl.id;
> > > > +	type_ = ctrl.type;
> > > > +	name_ = static_cast<const char *>(ctrl.name);
> > > > +	size_ = ctrl.elem_size * ctrl.elems;
> > > > +}
> > > > +
> > > > +/**
> > > > + * \fn V4L2ControlInfo::id()
> > > > + * \brief Retrieve the control id this instance refers to
> > >
> > > I would just say "Retrieve the control ID"
> > >
> > > > + * \return The V4L2 control id
> > >
> > > s/id/ID/ (as well as in other locations below)
> > >
> > > > + */
> > > > +
> > > > +/**
> > > > + * \fn V4L2ControlInfo::type()
> > > > + * \brief Retrieve the control type as defined by V4L2_CTRL_TYPE_
> > >
> > > Maybe V4L2_CTRL_TYPE_* ?
> > >
> > > > + * \return The V4L2 control type
> > > > + */
> > > > +
> > > > +/**
> > > > + * \fn V4L2ControlInfo::size()
> > > > + * \brief Retrieve the control value data size (in bytes)
> > > > + * \return The V4L2 control value data size
> > > > + */
> > > > +
> > > > +/**
> > > > + * \fn V4L2ControlInfo::name()
> > > > + * \brief Retrieve the control user readable name
> > > > + * \return The V4L2 control user readable name
> > > > + */
> > > > +
> > > > +/**
> > > > + * \class V4L2Control
> > > > + * \brief A V4L2 control value
> > > > + *
> > > > + * The V4L2Control class represent the value of a V4L2 control.
> > > > + * The class stores values that have been read from or will be applied to
> > > > + * a V4L2 device.
> > > > + *
> > > > + * The value stored in the class instances does not reflect what is actually
> > > > + * applied to the hardware but is a pure software cache optionally initialized
> > > > + * at control creation time and only modified by a control read operation or a
> > > > + * call to V4L2Controls::set().
> > > > + *
> > > > + * The V4L2Control class instances are not meant to be directly used but are
> > > > + * instead intended to be grouped in V4L2Controls instances, which are then
> > > > + * passed as parameters to the set and get control operations implemented in
> > > > + * V4L2Device::setControls() and V4L2Device::setControls() respectively.
> > >
> > > The second one should be V4L2Device::getControls(), or you could write
> > >
> > > "passed as parameter to V4L2Device::getControls() and
> > > V4L2Device::setControls()."
> > >
> > > > + *
> > > > + * In facts, access to the class constructor and to the control value accessor
> > > > + * and modifier are restricted to the friend V4L2Controls and V4L2Device
> > > > + * classes.
> > >
> > > I think you can drop this paragraph.
> > >
> > > > + */
> > > > +
> > > > +/**
> > > > + * \fn V4L2Control::id()
> > > > + * \brief Retrieve the control id this instance refers to
> > > > + * \return The V4L2Control id
> > > > + */
> > > > +
> > > > +/**
> > > > + * \class V4L2Controls
> > > > + * \brief Wraps a list of V4L2Control
> > >
> > > I would say "Container of V4L2Control instance" and replace wrapper by
> > > container below.
> > >
> > > > + *
> > > > + * The V4L2Controls class works as a wrapper for a list of V4L2Control
> > > > + * instances. The class provides operations to add a new control to the list,
> > > > + * get back a control value, and reset the list of controls it contains.
> > > > + *
> > > > + * In order to set and get controls, user of the Libcamera V4L2 control
> > >
> > > s/Libcamera/libcamera/
> > >
> > > > + * framework should operate on instances of the V4L2Controls class, and use
> > > > + * them as argument for the V4L2Device::setControls() and
> > > > + * V4L2Device::getControls() operations, which write and read a list of
> > > > + * controls from or to a V4L2 device (a video device or a subdevice).
> > > > + *
> > > > + * Controls are added to a V4L2Controls instance with the add() method, with
> > > > + * or without an initial value.
> > > > + *
> > > > + * To write controls to a device, the controls of interest should be added
> > >
> > > s/should/shall/
> > >
> > > > + * with an initial value by calling V4L2Controls::add(unsigned int id, long
> > > > + * int value) to prepare for a write operation. The value of controls which
> > > > + * are already part of the instance could be updated with a call to
> > > > + * V4L2Controls::set() operation.
> > >
> > > Is there a use case for this ?
> >
> > See the above example.
> >
> > > > Once the values of all controls of interest
> > > > + * have been initialized in a V4L2Controls instance, this should be then
> > > > + * passed to the V4L2Device::setControls() operation, which applies each
> > > > + * control in the list to the hardware.
> > >
> > > Once all desired control have been added, the V4L2Controls instance is
> > > passed to V4L2Device::setControls(), which sets the controls on the
> > > device.
> > >
> > > > + *
> > > > + * Alternatively a control can be add without any initial value by calling
> > > > + * V4L2Controls::add(unsigned int id) and then read from the device passing
> > > > + * the V4L2Controls instance to V4L2Device::getControls() which read each
> > > > + * control value from the hardware.
> > > > + *
> > > > + * Reading controls with an initialized value from the device, overwrites the
> > > > + * control's value, reflecting what has been actually read from the hardware.
> > >
> > > Let's write this like the previous paragraph.
> > >
> > > "To read controls from a device, the desired controls are added with
> > > V4L2Controls::add(unsigned int id) to prepare for a read operation. The
> > > V4L2Controls instance is then passed to V4L2Device::getControls(), which
> > > reads the controls from the device and updates the values stored in
> > > V4L2Controls."
> > >
> > > > + *
> > > > + * V4L2Controls instances can be reset to remove all controls it contains and
> > >
> > > s/it contains/they contain/
> > >
> > > > + * prepare to be re-used for a new control write/read sequence. Alternatively,
> > > > + * the value of a control already part of the instance could be updated by using
> > > > + * the V4L2Controls::set() method, to avoid going through a reset() when a
> > > > + * control has to be read then written to the hardware in sequence.
> > >
> > > Given how set() is implemented, creating a new V4L2Controls instance
> > > could be cheaper. Do we really want to reuse V4L2Controls instances with
> > > set() ? If so I think we need to think about the use cases, and decide
> > > how to implement it in an efficient way. Alternatively, I would start
> > > simple and extend the API later.
> >
> > So how would you re-use the same V4L2Controls instance for a
> > getControls()/setControls() sequence? Go through a reset and re-add
> > all controls?
>
> Do we need that at all ? If you have a V4L2Controls instance with 10
> controls, call getControls(), check the values and update two of them,
> and then call setControls(), all 10 controls will be written to the
> driver. Wouldn't it be better to create a second V4L2Controls instance
> for the setControls() operation that would be populated with just the
> two controls that need to be set ? I also expect those read-modify-write

Or possibly "reset()" the existing one.

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

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


> > > > +	V4L2Control ctrl(id);
> > > > +	controls_.push_back(ctrl);
> > > > +}
> > > > +
> > > > +/**
> > > > + * \brief Retrieve the value of the control with \a id
> > > > + * \param id The V4L2 control id (V4L2_CID_xx)
> > > > + *
> > > > + * Retrieve the value of the control with id \a id.
> > > > + *
> > > > + * It is important to notice that the returned value is not read from the
> > > > + * hardware at the time this operation is called, but it's the cached value
> > > > + * obtained the most recent time the control has been read with
> > > > + * V4L2Device::getControls() or the value of the control has been set to by the
> > > > + * user with the add() or set() operation.
> > > > + *
> > > > + * \return The V4L2 control value or a negative error code if the control id
> > > > + * is not part of this instance
> > > > + */
> > > > +int V4L2Controls::get(unsigned int id)
> > > > +{
> > > > +	V4L2Control *ctrl = getControl(id);
> > > > +	if (!ctrl)
> > > > +		return -EINVAL;
> > > > +
> > > > +	return ctrl->value();
> > > > +}
> > > > +
> > > > +/**
> > > > + * \brief Set the value of the control with \a id
> > > > + * \param id The V4L2 control id (V4L2_CID_xx)
> > > > + * \param value The new V4L2 control value
> > > > + *
> > > > + * Set the value of the control with id \a id.
> > > > + *
> > > > + * It is important to notice that the value is not applied to the
> > > > + * hardware at the time this operation is called, but it will only
> > > > + * be actually pplied only by calling V4L2Device::setControls().
> > > > + *
> > > > + * \return 0 on success or a negative error code if the control id
> > > > + * is not part of this instance
> > > > + */
> > > > +int V4L2Controls::set(unsigned int id, long int value)
> > > > +{
> > > > +	V4L2Control *ctrl = getControl(id);
> > > > +	if (!ctrl)
> > > > +		return -EINVAL;
> > > > +
> > > > +	ctrl->setValue(value);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +/*
> > > > + * \brief Get a pointer the control with \a id
> > > > + * \param id The V4L2 control id (V4L2_CID_xx)
> > > > + * \return A pointer to the V4L2Control with id \a id or nullptr  if the
> > > > + * control id is not part of this instance.
> > > > + */
> > > > +V4L2Control *V4L2Controls::getControl(unsigned int id)
> > > > +{
> > > > +	for (V4L2Control &ctrl : controls_) {
> > > > +		if (ctrl.id() == id)
> > > > +			return &ctrl;
> > > > +	}
> > > > +
> > > > +	return nullptr;
> > > > +}
> > > > +
> > > > +} /* namespace libcamera */
>
> --
> Regards,
>
> Laurent Pinchart
-------------- 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/20190620/620936de/attachment-0001.sig>