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

[Laurent Pinchart]

Hi Jacopo,

On Thu, Jun 20, 2019 at 02:44:48PM +0200, Jacopo Mondi wrote:
> On Thu, Jun 20, 2019 at 02:56:29PM +0300, Laurent Pinchart wrote:
> > On Thu, Jun 20, 2019 at 10:55:52AM +0200, Jacopo Mondi wrote:
> >> On Wed, Jun 19, 2019 at 10:41:27PM +0300, Laurent Pinchart wrote:
> >>> On Wed, Jun 19, 2019 at 01:08:53PM +0200, Jacopo Mondi wrote:
> >>>> Add Libcamera V4L2 control support, implemented using the V4L2 Extended
> >>>> Control APIs. This patch defines the types used to define and manage controls.
> >>>>
> >>>> Signed-off-by: Jacopo Mondi <jacopo 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.

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

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

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

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

-- 
Regards,

Laurent Pinchart