[libcamera-devel,7/9] libcamera: v4l2_device: Replace V4L2ControlList with ControlList

2135 diff mbox series

The V4L2Device class uses V4L2ControlList as a controls container for
the getControls() and setControls() operations. Having a distinct
container from ControlList will make the IPA API more complex,
especially when dealing with serialisation and deserialisation, as it
will need to explicitly support transporting both types of lists.

To simplify the IPA API, replace usage of V4L2ControlList with
ControlList in the V4L2Device (and thus CameraSensor) API. ControlList
however doesn't support accessing controls by numerical ID, which is a
common use case for V4L2 controls in pipeline handlers. To avoid
requiring each pipeline handler to map numerical IDs to ControlId
instances, turn the existing V4L2ControlList class into a helper derived
from ControlList. This allows easy building of a ControlList for a V4L2
device in pipeline handlers, while generalising the use of ControlList
everywhere else.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/camera_sensor.cpp       |   4 +-
 src/libcamera/include/camera_sensor.h |   5 +-
 src/libcamera/include/v4l2_controls.h |  41 +----
 src/libcamera/include/v4l2_device.h   |   8 +-
 src/libcamera/pipeline/ipu3/ipu3.cpp  |   9 +-
 src/libcamera/pipeline/uvcvideo.cpp   |  24 ++-
 src/libcamera/pipeline/vimc.cpp       |  25 ++-
 src/libcamera/v4l2_controls.cpp       | 216 +++++++-------------------
 src/libcamera/v4l2_device.cpp         |  50 +++---
 9 files changed, 128 insertions(+), 254 deletions(-)

Hi Laurent,
    just a note that this break compilation, but you know that already

On Tue, Oct 08, 2019 at 01:46:40AM +0300, Laurent Pinchart wrote:
> The V4L2Device class uses V4L2ControlList as a controls container for
> the getControls() and setControls() operations. Having a distinct
> container from ControlList will make the IPA API more complex,
> especially when dealing with serialisation and deserialisation, as it
> will need to explicitly support transporting both types of lists.
>
> To simplify the IPA API, replace usage of V4L2ControlList with
> ControlList in the V4L2Device (and thus CameraSensor) API. ControlList
> however doesn't support accessing controls by numerical ID, which is a
> common use case for V4L2 controls in pipeline handlers. To avoid
> requiring each pipeline handler to map numerical IDs to ControlId
> instances, turn the existing V4L2ControlList class into a helper derived
> from ControlList. This allows easy building of a ControlList for a V4L2
> device in pipeline handlers, while generalising the use of ControlList
> everywhere else.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/libcamera/camera_sensor.cpp       |   4 +-
>  src/libcamera/include/camera_sensor.h |   5 +-
>  src/libcamera/include/v4l2_controls.h |  41 +----
>  src/libcamera/include/v4l2_device.h   |   8 +-
>  src/libcamera/pipeline/ipu3/ipu3.cpp  |   9 +-
>  src/libcamera/pipeline/uvcvideo.cpp   |  24 ++-
>  src/libcamera/pipeline/vimc.cpp       |  25 ++-
>  src/libcamera/v4l2_controls.cpp       | 216 +++++++-------------------
>  src/libcamera/v4l2_device.cpp         |  50 +++---
>  9 files changed, 128 insertions(+), 254 deletions(-)
>
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index a7670b449b31..9e8b44a23850 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -279,7 +279,7 @@ const V4L2ControlInfoMap &CameraSensor::controls() const
>   * \retval -EINVAL One of the control is not supported or not accessible
>   * \retval i The index of the control that failed
>   */
> -int CameraSensor::getControls(V4L2ControlList *ctrls)
> +int CameraSensor::getControls(ControlList *ctrls)
>  {
>  	return subdev_->getControls(ctrls);
>  }
> @@ -309,7 +309,7 @@ int CameraSensor::getControls(V4L2ControlList *ctrls)
>   * \retval -EINVAL One of the control is not supported or not accessible
>   * \retval i The index of the control that failed
>   */
> -int CameraSensor::setControls(V4L2ControlList *ctrls)
> +int CameraSensor::setControls(ControlList *ctrls)
>  {
>  	return subdev_->setControls(ctrls);
>  }
> diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h
> index fe033fb374c1..5ad829232f26 100644
> --- a/src/libcamera/include/camera_sensor.h
> +++ b/src/libcamera/include/camera_sensor.h
> @@ -17,6 +17,7 @@
>
>  namespace libcamera {
>
> +class ControlList;
>  class MediaEntity;
>  class V4L2Subdevice;
>
> @@ -43,8 +44,8 @@ public:
>  	int setFormat(V4L2SubdeviceFormat *format);
>
>  	const V4L2ControlInfoMap &controls() const;
> -	int getControls(V4L2ControlList *ctrls);
> -	int setControls(V4L2ControlList *ctrls);
> +	int getControls(ControlList *ctrls);
> +	int setControls(ControlList *ctrls);
>
>  protected:
>  	std::string logPrefix() const;
> diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h
> index 71ce377fe4c5..1d85ecf9864a 100644
> --- a/src/libcamera/include/v4l2_controls.h
> +++ b/src/libcamera/include/v4l2_controls.h
> @@ -13,7 +13,6 @@
>  #include <string>
>  #include <vector>
>
> -#include <linux/v4l2-controls.h>
>  #include <linux/videodev2.h>
>
>  #include <libcamera/controls.h>
> @@ -47,45 +46,17 @@ private:
>
>  using V4L2ControlInfoMap = std::map<unsigned int, V4L2ControlInfo>;
>
> -class V4L2Control
> +class V4L2ControlList : public ControlList
>  {
>  public:
> -	V4L2Control(unsigned int id, const ControlValue &value = ControlValue())
> -		: id_(id), value_(value)
> -	{
> -	}
> +	V4L2ControlList(const V4L2ControlInfoMap &controls);

This comes from the video device and I think it's fine as it's
unlikely the V4L2ControlList outlives a video device, right ?

>
> -	unsigned int id() const { return id_; }
> -	const ControlValue &value() const { return value_; }
> -	ControlValue &value() { return value_; }
> +	bool contains(unsigned int id) const;
> +	const ControlValue &get(unsigned int id) const;
> +	void set(unsigned int id, const ControlValue &value);
>
>  private:
> -	unsigned int id_;
> -	ControlValue value_;
> -};
> -
> -class V4L2ControlList
> -{
> -public:
> -	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(); }
> -	std::size_t size() const { return controls_.size(); }
> -
> -	void clear() { controls_.clear(); }
> -	void add(unsigned int id, int64_t value = 0);
> -
> -	V4L2Control *getByIndex(unsigned int index);
> -	V4L2Control *operator[](unsigned int id);
> -
> -private:
> -	std::vector<V4L2Control> controls_;
> +	const V4L2ControlInfoMap &controls_;
>  };
>
>  } /* namespace libcamera */
> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> index 75a52c33d821..0375d3a1cb82 100644
> --- a/src/libcamera/include/v4l2_device.h
> +++ b/src/libcamera/include/v4l2_device.h
> @@ -17,6 +17,8 @@
>
>  namespace libcamera {
>
> +class ControlList;
> +
>  class V4L2Device : protected Loggable
>  {
>  public:
> @@ -25,8 +27,8 @@ public:
>
>  	const V4L2ControlInfoMap &controls() const { return controls_; }
>
> -	int getControls(V4L2ControlList *ctrls);
> -	int setControls(V4L2ControlList *ctrls);
> +	int getControls(ControlList *ctrls);
> +	int setControls(ControlList *ctrls);
>
>  	const std::string &deviceNode() const { return deviceNode_; }
>
> @@ -43,7 +45,7 @@ protected:
>
>  private:
>  	void listControls();
> -	void updateControls(V4L2ControlList *ctrls,
> +	void updateControls(ControlList *ctrls,
>  			    const V4L2ControlInfo **controlInfo,
>  			    const struct v4l2_ext_control *v4l2Ctrls,
>  			    unsigned int count);
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 827906d5cd2e..9776b36b88cd 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -199,6 +199,7 @@ class PipelineHandlerIPU3 : public PipelineHandler
>  {
>  public:
>  	static constexpr unsigned int V4L2_CID_IPU3_PIPE_MODE = 0x009819c1;
> +
>  	enum IPU3PipeModes {
>  		IPU3PipeModeVideo = 0,
>  		IPU3PipeModeStillCapture = 1,
> @@ -603,10 +604,10 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>  		return ret;
>
>  	/* Apply the "pipe_mode" control to the ImgU subdevice. */
> -	V4L2ControlList ctrls;
> -	ctrls.add(V4L2_CID_IPU3_PIPE_MODE, vfStream->active_ ?
> -					   IPU3PipeModeVideo :
> -					   IPU3PipeModeStillCapture);
> +	V4L2ControlList ctrls(imgu->imgu_->controls());
> +	ctrls.set(V4L2_CID_IPU3_PIPE_MODE,
> +		  static_cast<int32_t>(vfStream->active_ ? IPU3PipeModeVideo :
> +				       IPU3PipeModeStillCapture));
>  	ret = imgu->imgu_->setControls(&ctrls);
>  	if (ret) {
>  		LOG(IPU3, Error) << "Unable to set pipe_mode control";
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index 88f7fb9bc568..467bd9a9eaff 100644
> --- a/src/libcamera/pipeline/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> @@ -228,31 +228,30 @@ void PipelineHandlerUVC::stop(Camera *camera)
>
>  int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)
>  {
> -	V4L2ControlList controls;
> +	V4L2ControlList controls(data->video_->controls());
>
>  	for (auto it : request->controls()) {
>  		const ControlId &id = *it.first;
>  		ControlValue &value = it.second;
>
>  		if (id == controls::Brightness) {
> -			controls.add(V4L2_CID_BRIGHTNESS, value.get<int32_t>());
> +			controls.set(V4L2_CID_BRIGHTNESS, value);
>  		} else if (id == controls::Contrast) {
> -			controls.add(V4L2_CID_CONTRAST, value.get<int32_t>());
> +			controls.set(V4L2_CID_CONTRAST, value);
>  		} else if (id == controls::Saturation) {
> -			controls.add(V4L2_CID_SATURATION, value.get<int32_t>());
> +			controls.set(V4L2_CID_SATURATION, value);
>  		} else if (id == controls::ManualExposure) {
> -			controls.add(V4L2_CID_EXPOSURE_AUTO, 1);
> -			controls.add(V4L2_CID_EXPOSURE_ABSOLUTE, value.get<int32_t>());
> +			controls.set(V4L2_CID_EXPOSURE_AUTO, 1);
> +			controls.set(V4L2_CID_EXPOSURE_ABSOLUTE, value);
>  		} else if (id == controls::ManualGain) {
> -			controls.add(V4L2_CID_GAIN, value.get<int32_t>());
> +			controls.set(V4L2_CID_GAIN, value);
>  		}
>  	}
>
> -	for (const V4L2Control &ctrl : controls)
> +	for (const auto &ctrl : controls)
>  		LOG(UVC, Debug)
> -			<< "Setting control 0x"
> -			<< std::hex << std::setw(8) << ctrl.id() << std::dec
> -			<< " to " << ctrl.value().toString();
> +			<< "Setting control " << ctrl.first->name()
> +			<< " to " << ctrl.second.toString();
>
>  	int ret = data->video_->setControls(&controls);
>  	if (ret) {
> @@ -338,11 +337,10 @@ int UVCCameraData::init(MediaEntity *entity)
>  	/* Initialise the supported controls. */
>  	const V4L2ControlInfoMap &controls = video_->controls();
>  	for (const auto &ctrl : controls) {
> -		unsigned int v4l2Id = ctrl.first;
>  		const V4L2ControlInfo &info = ctrl.second;
>  		const ControlId *id;
>
> -		switch (v4l2Id) {
> +		switch (info.id().id()) {
>  		case V4L2_CID_BRIGHTNESS:
>  			id = &controls::Brightness;
>  			break;
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index 26477dccbb8f..600a1154c75e 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -281,26 +281,24 @@ void PipelineHandlerVimc::stop(Camera *camera)
>
>  int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)
>  {
> -	V4L2ControlList controls;
> +	V4L2ControlList controls(data->sensor_->controls());
>
>  	for (auto it : request->controls()) {
>  		const ControlId &id = *it.first;
>  		ControlValue &value = it.second;
>
> -		if (id == controls::Brightness) {
> -			controls.add(V4L2_CID_BRIGHTNESS, value.get<int32_t>());
> -		} else if (id == controls::Contrast) {
> -			controls.add(V4L2_CID_CONTRAST, value.get<int32_t>());
> -		} else if (id == controls::Saturation) {
> -			controls.add(V4L2_CID_SATURATION, value.get<int32_t>());
> -		}
> +		if (id == controls::Brightness)
> +			controls.set(V4L2_CID_BRIGHTNESS, value);
> +		else if (id == controls::Contrast)
> +			controls.set(V4L2_CID_CONTRAST, value);
> +		else if (id == controls::Saturation)
> +			controls.set(V4L2_CID_SATURATION, value);
>  	}
>
> -	for (const V4L2Control &ctrl : controls)
> +	for (const auto &ctrl : controls)
>  		LOG(VIMC, Debug)
> -			<< "Setting control 0x"
> -			<< std::hex << std::setw(8) << ctrl.id() << std::dec
> -			<< " to " << ctrl.value().toString();
> +			<< "Setting control " << ctrl.first->name()
> +			<< " to " << ctrl.second.toString();
>
>  	int ret = data->sensor_->setControls(&controls);
>  	if (ret) {
> @@ -417,11 +415,10 @@ int VimcCameraData::init(MediaDevice *media)
>  	/* Initialise the supported controls. */
>  	const V4L2ControlInfoMap &controls = sensor_->controls();
>  	for (const auto &ctrl : controls) {
> -		unsigned int v4l2Id = ctrl.first;
>  		const V4L2ControlInfo &info = ctrl.second;
>  		const ControlId *id;
>
> -		switch (v4l2Id) {
> +		switch (info.id().id()) {
>  		case V4L2_CID_BRIGHTNESS:
>  			id = &controls::Brightness;
>  			break;
> diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> index a630a2583d33..98b2b3fe9d0a 100644
> --- a/src/libcamera/v4l2_controls.cpp
> +++ b/src/libcamera/v4l2_controls.cpp
> @@ -167,191 +167,83 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
>   * \brief A map of control ID to V4L2ControlInfo
>   */
>
> -/**
> - * \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 modified by a control read or write operation.
> - *
> - * The write and read controls the V4L2Control class instances are not meant
> - * to be directly used but are instead intended to be grouped in
> - * V4L2ControlList instances, which are then passed as parameters to
> - * V4L2Device::setControls() and V4L2Device::getControls() operations.
> - */
> -
> -/**
> - * \fn V4L2Control::V4L2Control
> - * \brief Construct a V4L2 control with \a id and \a value
> - * \param id The V4L2 control ID
> - * \param value The control value
> - */
> -
> -/**
> - * \fn V4L2Control::value() const
> - * \brief Retrieve the value of the control
> - *
> - * This method is a const version of V4L2Control::value(), returning a const
> - * reference to the value.
> - *
> - * \return The V4L2 control value
> - */
> -
> -/**
> - * \fn V4L2Control::value()
> - * \brief Retrieve the value of the control
> - *
> - * This method returns the cached control value, initially set by
> - * V4L2ControlList::add() and then updated when the controls are read or
> - * written with V4L2Device::getControls() and V4L2Device::setControls().
> - *
> - * \return The V4L2 control value
> - */
> -
> -/**
> - * \fn V4L2Control::id()
> - * \brief Retrieve the control ID this instance refers to
> - * \return The V4L2Control ID
> - */
> -
>  /**
>   * \class V4L2ControlList
> - * \brief Container of V4L2Control instances
> + * \brief A list of controls for a V4L2 device
>   *
> - * The V4L2ControlList 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.
> + * This class specialises the ControList class for use with V4L2 devices. It
> + * offers a convenience API to access controls by numerical ID instead of
> + * ControlId.
>   *
> - * In order to set and get controls, user of the libcamera V4L2 control
> - * framework should operate on instances of the V4L2ControlList class, and use
> - * them as argument for the V4L2Device::setControls() and
> - * V4L2Device::getControls() operations, which write and read a list of
> - * controls to or from a V4L2 device (a video device or a subdevice).

I would not remove this part, as it explains how to use the
V4L2ControlList, and it still applies today even if those methods
accepts a ControlList *

> - *
> - * Controls are added to a V4L2ControlList instance with the add() method, with
> - * or without a value.
> - *
> - * To write controls to a device, the controls of interest shall be added with
> - * an initial value by calling V4L2ControlList::add(unsigned int id, int64_t
> - * value) to prepare for a write operation. Once the values of all controls of
> - * interest have been added, the V4L2ControlList 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
> - * V4L2ControlList::add(unsigned int id) to prepare for a read operation. The
> - * V4L2ControlList instance is then passed to V4L2Device::getControls(), which
> - * reads the controls from the device and updates the values stored in
> - * V4L2ControlList.
> - *
> - * V4L2ControlList instances can be reset to remove all controls they contain
> - * and prepare to be re-used for a new control write/read sequence.
> - */
> -
> -/**
> - * \typedef V4L2ControlList::iterator
> - * \brief Iterator on the V4L2 controls contained in the instance
> - */
> -
> -/**
> - * \typedef V4L2ControlList::const_iterator
> - * \brief Const iterator on the V4L2 controls contained in the instance
> + * V4L2ControlList should as much as possible not be used in place of
> + * ControlList in APIs, and be used instead solely for the purpose of easily
> + * constructing a ControlList of V4L2 controls for a device.
>   */
>
>  /**
> - * \fn iterator V4L2ControlList::begin()
> - * \brief Retrieve an iterator to the first V4L2Control in the instance
> - * \return An iterator to the first V4L2 control
> + * \brief Construct a V4L2ControlList associated with a V4L2 device
> + * \param[in] controls The V4L2 device control info map

The V4L2 device provided control info map ?

>   */
> -
> -/**
> - * \fn const_iterator V4L2ControlList::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 V4L2ControlList::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 V4L2ControlList::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 V4L2ControlList::empty()
> - * \brief Verify if the instance does not contain any control
> - * \return True if the instance does not contain any control, false otherwise
> - */
> -
> -/**
> - * \fn V4L2ControlList::size()
> - * \brief Retrieve the number on controls in the instance
> - * \return The number of V4L2Control stored in the instance
> - */
> -
> -/**
> - * \fn V4L2ControlList::clear()
> - * \brief Remove all controls in the instance
> - */
> -
> -/**
> - * \brief Add control with \a id and optional \a value to the instance
> - * \param id The V4L2 control ID (V4L2_CID_*)
> - * \param value The V4L2 control value
> - *
> - * This method adds a new V4L2 control to the V4L2ControlList. The newly
> - * inserted control shall not already be present in the control lists, otherwise
> - * this method, and further use of the control list lead to undefined behaviour.
> - */
> -void V4L2ControlList::add(unsigned int id, int64_t value)
> +V4L2ControlList::V4L2ControlList(const V4L2ControlInfoMap &controls)
> +	: ControlList(nullptr), controls_(controls)
>  {
> -	controls_.emplace_back(id, value);
>  }
>
>  /**
> - * \brief Retrieve the control at \a index
> - * \param[in] index The control index
> - *
> - * Controls are stored in a V4L2ControlList in order of insertion and this
> - * method retrieves the control at \a index.
> + * \brief Check if the list contains a control with the specified \a id
> + * \param[in] id The control ID
>   *
> - * \return A pointer to the V4L2Control at \a index or nullptr if the
> - * index is larger than the number of controls
> + * \return True if the list contains a matching control, false otherwise
>   */
> -V4L2Control *V4L2ControlList::getByIndex(unsigned int index)
> +bool V4L2ControlList::contains(unsigned int id) const
>  {
> -	if (index >= controls_.size())
> -		return nullptr;
> +	auto ctrl = controls_.find(id);
> +	if (ctrl == controls_.end())
> +		return false;
>
> -	return &controls_[index];
> +	return ControlList::contains(ctrl->second.id());

Ah wait, we have
ControlList::controls_ and
V4L2ControlList::controls_

is this intended ?

>  }
>
>  /**
> - * \brief Retrieve the control with \a id
> - * \param[in] id The V4L2 control ID (V4L2_CID_xx)
> - * \return A pointer to the V4L2Control with \a id or nullptr if the
> - * control ID is not part of this instance.
> + * \brief Get the value of control \a id
> + * \param[in] id The control ID
> + *
> + * The behaviour is undefined if the control \a id is not present in the list.
> + * Use v4L2ControlList::contains() to test for the presence of a control in the

s/v4L2/V4L2

> + * list before retrieving its value.
> + *
> + * \return The control value
>   */
> -V4L2Control *V4L2ControlList::operator[](unsigned int id)
> +const ControlValue &V4L2ControlList::get(unsigned int id) const
>  {
> -	for (V4L2Control &ctrl : controls_) {
> -		if (ctrl.id() == id)
> -			return &ctrl;
> +	auto ctrl = controls_.find(id);
> +	if (ctrl == controls_.end()) {
> +		static ControlValue zero;
> +		return zero;
>  	}
>
> -	return nullptr;
> +	return ControlList::get(ctrl->second.id());
> +}
> +
> +/**
> + * \brief Set the value of control \a id to \a value
> + * \param[in] id The control ID
> + * \param[in] value The control value
> + *
> + * This method sets the value of a control in the control list. If the control
> + * is already present in the list, its value is updated, otherwise it is added
> + * to the list.
> + *
> + * The behaviour is undefined if the control \a ctrl is not supported by the
> + * V4L2 device that the list refers to.
> + */

It does not seems undefined, but simply ignored..

> +void V4L2ControlList::set(unsigned int id, const ControlValue &value)
> +{
> +	auto ctrl = controls_.find(id);
> +	if (ctrl == controls_.end())
> +		return;
> +
> +	ControlList::set(ctrl->second.id(), value);
>  }
>
>  } /* namespace libcamera */
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 466c3d41f6e3..50616571dcef 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -12,6 +12,8 @@
>  #include <sys/ioctl.h>
>  #include <unistd.h>
>
> +#include <libcamera/controls.h>
> +
>  #include "log.h"
>  #include "v4l2_controls.h"
>
> @@ -163,7 +165,7 @@ void V4L2Device::close()
>   * \retval -EINVAL One of the control is not supported or not accessible
>   * \retval i The index of the control that failed
>   */
> -int V4L2Device::getControls(V4L2ControlList *ctrls)
> +int V4L2Device::getControls(ControlList *ctrls)
>  {
>  	unsigned int count = ctrls->size();
>  	if (count == 0)
> @@ -173,18 +175,21 @@ int V4L2Device::getControls(V4L2ControlList *ctrls)
>  	struct v4l2_ext_control v4l2Ctrls[count];
>  	memset(v4l2Ctrls, 0, sizeof(v4l2Ctrls));
>
> -	for (unsigned int i = 0; i < count; ++i) {
> -		const V4L2Control *ctrl = ctrls->getByIndex(i);
> -		const auto iter = controls_.find(ctrl->id());
> +	unsigned int i = 0;
> +	for (const auto &ctrl : *ctrls) {
> +		const ControlId *id = ctrl.first;
> +		const auto iter = controls_.find(id->id());
>  		if (iter == controls_.end()) {
>  			LOG(V4L2, Error)
> -				<< "Control '" << ctrl->id() << "' not found";
> +				<< "Control '" << id->name() << "' not found";
>  			return -EINVAL;
>  		}
>
>  		const V4L2ControlInfo *info = &iter->second;
>  		controlInfo[i] = info;
> -		v4l2Ctrls[i].id = ctrl->id();
> +		v4l2Ctrls[i].id = id->id();
> +
> +		i++;
>  	}
>
>  	struct v4l2_ext_controls v4l2ExtCtrls = {};
> @@ -238,7 +243,7 @@ int V4L2Device::getControls(V4L2ControlList *ctrls)
>   * \retval -EINVAL One of the control is not supported or not accessible
>   * \retval i The index of the control that failed
>   */
> -int V4L2Device::setControls(V4L2ControlList *ctrls)
> +int V4L2Device::setControls(ControlList *ctrls)
>  {
>  	unsigned int count = ctrls->size();
>  	if (count == 0)
> @@ -248,32 +253,36 @@ int V4L2Device::setControls(V4L2ControlList *ctrls)
>  	struct v4l2_ext_control v4l2Ctrls[count];
>  	memset(v4l2Ctrls, 0, sizeof(v4l2Ctrls));
>
> -	for (unsigned int i = 0; i < count; ++i) {
> -		const V4L2Control *ctrl = ctrls->getByIndex(i);
> -		const auto iter = controls_.find(ctrl->id());
> +	unsigned int i = 0;
> +	for (const auto &ctrl : *ctrls) {
> +		const ControlId *id = ctrl.first;
> +		const auto iter = controls_.find(id->id());
>  		if (iter == controls_.end()) {
>  			LOG(V4L2, Error)
> -				<< "Control '" << ctrl->id() << "' not found";
> +				<< "Control '" << id->name() << "' not found";
>  			return -EINVAL;
>  		}
>
>  		const V4L2ControlInfo *info = &iter->second;
>  		controlInfo[i] = info;
> -		v4l2Ctrls[i].id = ctrl->id();
> +		v4l2Ctrls[i].id = id->id();
>
>  		/* Set the v4l2_ext_control value for the write operation. */
> +		const ControlValue &value = ctrl.second;
>  		switch (info->type()) {
>  		case V4L2_CTRL_TYPE_INTEGER64:
> -			v4l2Ctrls[i].value64 = ctrl->value().get<int64_t>();
> +			v4l2Ctrls[i].value64 = value.get<int64_t>();
>  			break;
>  		default:
>  			/*
>  			 * \todo To be changed when support for string and
>  			 * compound controls will be added.
>  			 */
> -			v4l2Ctrls[i].value = ctrl->value().get<int32_t>();
> +			v4l2Ctrls[i].value64 = value.get<int32_t>();

value64 or value ?

>  			break;
>  		}
> +
> +		i++;
>  	}
>
>  	struct v4l2_ext_controls v4l2ExtCtrls = {};
> @@ -382,28 +391,31 @@ void V4L2Device::listControls()
>   * \param[in] v4l2Ctrls List of V4L2 extended controls as returned by the driver
>   * \param[in] count The number of controls to update
>   */
> -void V4L2Device::updateControls(V4L2ControlList *ctrls,
> +void V4L2Device::updateControls(ControlList *ctrls,
>  				const V4L2ControlInfo **controlInfo,
>  				const struct v4l2_ext_control *v4l2Ctrls,
>  				unsigned int count)
>  {
> -	for (unsigned int i = 0; i < count; ++i) {
> +	unsigned int i = 0;
> +	for (auto &ctrl : *ctrls) {
>  		const struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];
>  		const V4L2ControlInfo *info = controlInfo[i];
> -		V4L2Control *ctrl = ctrls->getByIndex(i);
> +		ControlValue &value = ctrl.second;

you should break when i == count

Thanks
   j
>
>  		switch (info->type()) {
>  		case V4L2_CTRL_TYPE_INTEGER64:
> -			ctrl->value().set<int64_t>(v4l2Ctrl->value64);
> +			value.set<int64_t>(v4l2Ctrl->value64);
>  			break;
>  		default:
>  			/*
>  			 * \todo To be changed when support for string and
>  			 * compound controls will be added.
>  			 */
> -			ctrl->value().set<int32_t>(v4l2Ctrl->value);
> +			value.set<int32_t>(v4l2Ctrl->value);
>  			break;
>  		}
> +
> +		i++;
>  	}
>  }
>
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

On Wed, Oct 09, 2019 at 11:13:53AM +0200, Jacopo Mondi wrote:
> Hi Laurent,
>     just a note that this break compilation, but you know that already

Isn't it patch 9/9 that breaks compilation ?

> On Tue, Oct 08, 2019 at 01:46:40AM +0300, Laurent Pinchart wrote:
> > The V4L2Device class uses V4L2ControlList as a controls container for
> > the getControls() and setControls() operations. Having a distinct
> > container from ControlList will make the IPA API more complex,
> > especially when dealing with serialisation and deserialisation, as it
> > will need to explicitly support transporting both types of lists.
> >
> > To simplify the IPA API, replace usage of V4L2ControlList with
> > ControlList in the V4L2Device (and thus CameraSensor) API. ControlList
> > however doesn't support accessing controls by numerical ID, which is a
> > common use case for V4L2 controls in pipeline handlers. To avoid
> > requiring each pipeline handler to map numerical IDs to ControlId
> > instances, turn the existing V4L2ControlList class into a helper derived
> > from ControlList. This allows easy building of a ControlList for a V4L2
> > device in pipeline handlers, while generalising the use of ControlList
> > everywhere else.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  src/libcamera/camera_sensor.cpp       |   4 +-
> >  src/libcamera/include/camera_sensor.h |   5 +-
> >  src/libcamera/include/v4l2_controls.h |  41 +----
> >  src/libcamera/include/v4l2_device.h   |   8 +-
> >  src/libcamera/pipeline/ipu3/ipu3.cpp  |   9 +-
> >  src/libcamera/pipeline/uvcvideo.cpp   |  24 ++-
> >  src/libcamera/pipeline/vimc.cpp       |  25 ++-
> >  src/libcamera/v4l2_controls.cpp       | 216 +++++++-------------------
> >  src/libcamera/v4l2_device.cpp         |  50 +++---
> >  9 files changed, 128 insertions(+), 254 deletions(-)
> >
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index a7670b449b31..9e8b44a23850 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -279,7 +279,7 @@ const V4L2ControlInfoMap &CameraSensor::controls() const
> >   * \retval -EINVAL One of the control is not supported or not accessible
> >   * \retval i The index of the control that failed
> >   */
> > -int CameraSensor::getControls(V4L2ControlList *ctrls)
> > +int CameraSensor::getControls(ControlList *ctrls)
> >  {
> >  	return subdev_->getControls(ctrls);
> >  }
> > @@ -309,7 +309,7 @@ int CameraSensor::getControls(V4L2ControlList *ctrls)
> >   * \retval -EINVAL One of the control is not supported or not accessible
> >   * \retval i The index of the control that failed
> >   */
> > -int CameraSensor::setControls(V4L2ControlList *ctrls)
> > +int CameraSensor::setControls(ControlList *ctrls)
> >  {
> >  	return subdev_->setControls(ctrls);
> >  }
> > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h
> > index fe033fb374c1..5ad829232f26 100644
> > --- a/src/libcamera/include/camera_sensor.h
> > +++ b/src/libcamera/include/camera_sensor.h
> > @@ -17,6 +17,7 @@
> >
> >  namespace libcamera {
> >
> > +class ControlList;
> >  class MediaEntity;
> >  class V4L2Subdevice;
> >
> > @@ -43,8 +44,8 @@ public:
> >  	int setFormat(V4L2SubdeviceFormat *format);
> >
> >  	const V4L2ControlInfoMap &controls() const;
> > -	int getControls(V4L2ControlList *ctrls);
> > -	int setControls(V4L2ControlList *ctrls);
> > +	int getControls(ControlList *ctrls);
> > +	int setControls(ControlList *ctrls);
> >
> >  protected:
> >  	std::string logPrefix() const;
> > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h
> > index 71ce377fe4c5..1d85ecf9864a 100644
> > --- a/src/libcamera/include/v4l2_controls.h
> > +++ b/src/libcamera/include/v4l2_controls.h
> > @@ -13,7 +13,6 @@
> >  #include <string>
> >  #include <vector>
> >
> > -#include <linux/v4l2-controls.h>
> >  #include <linux/videodev2.h>
> >
> >  #include <libcamera/controls.h>
> > @@ -47,45 +46,17 @@ private:
> >
> >  using V4L2ControlInfoMap = std::map<unsigned int, V4L2ControlInfo>;
> >
> > -class V4L2Control
> > +class V4L2ControlList : public ControlList
> >  {
> >  public:
> > -	V4L2Control(unsigned int id, const ControlValue &value = ControlValue())
> > -		: id_(id), value_(value)
> > -	{
> > -	}
> > +	V4L2ControlList(const V4L2ControlInfoMap &controls);
> 
> This comes from the video device and I think it's fine as it's
> unlikely the V4L2ControlList outlives a video device, right ?

Yes, it shouldn't be a problem.

> >
> > -	unsigned int id() const { return id_; }
> > -	const ControlValue &value() const { return value_; }
> > -	ControlValue &value() { return value_; }
> > +	bool contains(unsigned int id) const;
> > +	const ControlValue &get(unsigned int id) const;
> > +	void set(unsigned int id, const ControlValue &value);
> >
> >  private:
> > -	unsigned int id_;
> > -	ControlValue value_;
> > -};
> > -
> > -class V4L2ControlList
> > -{
> > -public:
> > -	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(); }
> > -	std::size_t size() const { return controls_.size(); }
> > -
> > -	void clear() { controls_.clear(); }
> > -	void add(unsigned int id, int64_t value = 0);
> > -
> > -	V4L2Control *getByIndex(unsigned int index);
> > -	V4L2Control *operator[](unsigned int id);
> > -
> > -private:
> > -	std::vector<V4L2Control> controls_;
> > +	const V4L2ControlInfoMap &controls_;
> >  };
> >
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> > index 75a52c33d821..0375d3a1cb82 100644
> > --- a/src/libcamera/include/v4l2_device.h
> > +++ b/src/libcamera/include/v4l2_device.h
> > @@ -17,6 +17,8 @@
> >
> >  namespace libcamera {
> >
> > +class ControlList;
> > +
> >  class V4L2Device : protected Loggable
> >  {
> >  public:
> > @@ -25,8 +27,8 @@ public:
> >
> >  	const V4L2ControlInfoMap &controls() const { return controls_; }
> >
> > -	int getControls(V4L2ControlList *ctrls);
> > -	int setControls(V4L2ControlList *ctrls);
> > +	int getControls(ControlList *ctrls);
> > +	int setControls(ControlList *ctrls);
> >
> >  	const std::string &deviceNode() const { return deviceNode_; }
> >
> > @@ -43,7 +45,7 @@ protected:
> >
> >  private:
> >  	void listControls();
> > -	void updateControls(V4L2ControlList *ctrls,
> > +	void updateControls(ControlList *ctrls,
> >  			    const V4L2ControlInfo **controlInfo,
> >  			    const struct v4l2_ext_control *v4l2Ctrls,
> >  			    unsigned int count);
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 827906d5cd2e..9776b36b88cd 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -199,6 +199,7 @@ class PipelineHandlerIPU3 : public PipelineHandler
> >  {
> >  public:
> >  	static constexpr unsigned int V4L2_CID_IPU3_PIPE_MODE = 0x009819c1;
> > +
> >  	enum IPU3PipeModes {
> >  		IPU3PipeModeVideo = 0,
> >  		IPU3PipeModeStillCapture = 1,
> > @@ -603,10 +604,10 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> >  		return ret;
> >
> >  	/* Apply the "pipe_mode" control to the ImgU subdevice. */
> > -	V4L2ControlList ctrls;
> > -	ctrls.add(V4L2_CID_IPU3_PIPE_MODE, vfStream->active_ ?
> > -					   IPU3PipeModeVideo :
> > -					   IPU3PipeModeStillCapture);
> > +	V4L2ControlList ctrls(imgu->imgu_->controls());
> > +	ctrls.set(V4L2_CID_IPU3_PIPE_MODE,
> > +		  static_cast<int32_t>(vfStream->active_ ? IPU3PipeModeVideo :
> > +				       IPU3PipeModeStillCapture));
> >  	ret = imgu->imgu_->setControls(&ctrls);
> >  	if (ret) {
> >  		LOG(IPU3, Error) << "Unable to set pipe_mode control";
> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> > index 88f7fb9bc568..467bd9a9eaff 100644
> > --- a/src/libcamera/pipeline/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo.cpp
> > @@ -228,31 +228,30 @@ void PipelineHandlerUVC::stop(Camera *camera)
> >
> >  int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)
> >  {
> > -	V4L2ControlList controls;
> > +	V4L2ControlList controls(data->video_->controls());
> >
> >  	for (auto it : request->controls()) {
> >  		const ControlId &id = *it.first;
> >  		ControlValue &value = it.second;
> >
> >  		if (id == controls::Brightness) {
> > -			controls.add(V4L2_CID_BRIGHTNESS, value.get<int32_t>());
> > +			controls.set(V4L2_CID_BRIGHTNESS, value);
> >  		} else if (id == controls::Contrast) {
> > -			controls.add(V4L2_CID_CONTRAST, value.get<int32_t>());
> > +			controls.set(V4L2_CID_CONTRAST, value);
> >  		} else if (id == controls::Saturation) {
> > -			controls.add(V4L2_CID_SATURATION, value.get<int32_t>());
> > +			controls.set(V4L2_CID_SATURATION, value);
> >  		} else if (id == controls::ManualExposure) {
> > -			controls.add(V4L2_CID_EXPOSURE_AUTO, 1);
> > -			controls.add(V4L2_CID_EXPOSURE_ABSOLUTE, value.get<int32_t>());
> > +			controls.set(V4L2_CID_EXPOSURE_AUTO, 1);
> > +			controls.set(V4L2_CID_EXPOSURE_ABSOLUTE, value);
> >  		} else if (id == controls::ManualGain) {
> > -			controls.add(V4L2_CID_GAIN, value.get<int32_t>());
> > +			controls.set(V4L2_CID_GAIN, value);
> >  		}
> >  	}
> >
> > -	for (const V4L2Control &ctrl : controls)
> > +	for (const auto &ctrl : controls)
> >  		LOG(UVC, Debug)
> > -			<< "Setting control 0x"
> > -			<< std::hex << std::setw(8) << ctrl.id() << std::dec
> > -			<< " to " << ctrl.value().toString();
> > +			<< "Setting control " << ctrl.first->name()
> > +			<< " to " << ctrl.second.toString();
> >
> >  	int ret = data->video_->setControls(&controls);
> >  	if (ret) {
> > @@ -338,11 +337,10 @@ int UVCCameraData::init(MediaEntity *entity)
> >  	/* Initialise the supported controls. */
> >  	const V4L2ControlInfoMap &controls = video_->controls();
> >  	for (const auto &ctrl : controls) {
> > -		unsigned int v4l2Id = ctrl.first;
> >  		const V4L2ControlInfo &info = ctrl.second;
> >  		const ControlId *id;
> >
> > -		switch (v4l2Id) {
> > +		switch (info.id().id()) {
> >  		case V4L2_CID_BRIGHTNESS:
> >  			id = &controls::Brightness;
> >  			break;
> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> > index 26477dccbb8f..600a1154c75e 100644
> > --- a/src/libcamera/pipeline/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc.cpp
> > @@ -281,26 +281,24 @@ void PipelineHandlerVimc::stop(Camera *camera)
> >
> >  int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)
> >  {
> > -	V4L2ControlList controls;
> > +	V4L2ControlList controls(data->sensor_->controls());
> >
> >  	for (auto it : request->controls()) {
> >  		const ControlId &id = *it.first;
> >  		ControlValue &value = it.second;
> >
> > -		if (id == controls::Brightness) {
> > -			controls.add(V4L2_CID_BRIGHTNESS, value.get<int32_t>());
> > -		} else if (id == controls::Contrast) {
> > -			controls.add(V4L2_CID_CONTRAST, value.get<int32_t>());
> > -		} else if (id == controls::Saturation) {
> > -			controls.add(V4L2_CID_SATURATION, value.get<int32_t>());
> > -		}
> > +		if (id == controls::Brightness)
> > +			controls.set(V4L2_CID_BRIGHTNESS, value);
> > +		else if (id == controls::Contrast)
> > +			controls.set(V4L2_CID_CONTRAST, value);
> > +		else if (id == controls::Saturation)
> > +			controls.set(V4L2_CID_SATURATION, value);
> >  	}
> >
> > -	for (const V4L2Control &ctrl : controls)
> > +	for (const auto &ctrl : controls)
> >  		LOG(VIMC, Debug)
> > -			<< "Setting control 0x"
> > -			<< std::hex << std::setw(8) << ctrl.id() << std::dec
> > -			<< " to " << ctrl.value().toString();
> > +			<< "Setting control " << ctrl.first->name()
> > +			<< " to " << ctrl.second.toString();
> >
> >  	int ret = data->sensor_->setControls(&controls);
> >  	if (ret) {
> > @@ -417,11 +415,10 @@ int VimcCameraData::init(MediaDevice *media)
> >  	/* Initialise the supported controls. */
> >  	const V4L2ControlInfoMap &controls = sensor_->controls();
> >  	for (const auto &ctrl : controls) {
> > -		unsigned int v4l2Id = ctrl.first;
> >  		const V4L2ControlInfo &info = ctrl.second;
> >  		const ControlId *id;
> >
> > -		switch (v4l2Id) {
> > +		switch (info.id().id()) {
> >  		case V4L2_CID_BRIGHTNESS:
> >  			id = &controls::Brightness;
> >  			break;
> > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> > index a630a2583d33..98b2b3fe9d0a 100644
> > --- a/src/libcamera/v4l2_controls.cpp
> > +++ b/src/libcamera/v4l2_controls.cpp
> > @@ -167,191 +167,83 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
> >   * \brief A map of control ID to V4L2ControlInfo
> >   */
> >
> > -/**
> > - * \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 modified by a control read or write operation.
> > - *
> > - * The write and read controls the V4L2Control class instances are not meant
> > - * to be directly used but are instead intended to be grouped in
> > - * V4L2ControlList instances, which are then passed as parameters to
> > - * V4L2Device::setControls() and V4L2Device::getControls() operations.
> > - */
> > -
> > -/**
> > - * \fn V4L2Control::V4L2Control
> > - * \brief Construct a V4L2 control with \a id and \a value
> > - * \param id The V4L2 control ID
> > - * \param value The control value
> > - */
> > -
> > -/**
> > - * \fn V4L2Control::value() const
> > - * \brief Retrieve the value of the control
> > - *
> > - * This method is a const version of V4L2Control::value(), returning a const
> > - * reference to the value.
> > - *
> > - * \return The V4L2 control value
> > - */
> > -
> > -/**
> > - * \fn V4L2Control::value()
> > - * \brief Retrieve the value of the control
> > - *
> > - * This method returns the cached control value, initially set by
> > - * V4L2ControlList::add() and then updated when the controls are read or
> > - * written with V4L2Device::getControls() and V4L2Device::setControls().
> > - *
> > - * \return The V4L2 control value
> > - */
> > -
> > -/**
> > - * \fn V4L2Control::id()
> > - * \brief Retrieve the control ID this instance refers to
> > - * \return The V4L2Control ID
> > - */
> > -
> >  /**
> >   * \class V4L2ControlList
> > - * \brief Container of V4L2Control instances
> > + * \brief A list of controls for a V4L2 device
> >   *
> > - * The V4L2ControlList 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.
> > + * This class specialises the ControList class for use with V4L2 devices. It
> > + * offers a convenience API to access controls by numerical ID instead of
> > + * ControlId.
> >   *
> > - * In order to set and get controls, user of the libcamera V4L2 control
> > - * framework should operate on instances of the V4L2ControlList class, and use
> > - * them as argument for the V4L2Device::setControls() and
> > - * V4L2Device::getControls() operations, which write and read a list of
> > - * controls to or from a V4L2 device (a video device or a subdevice).
> 
> I would not remove this part, as it explains how to use the
> V4L2ControlList, and it still applies today even if those methods
> accepts a ControlList *

Doesn't it belong to V4L2Device though ?

> > - *
> > - * Controls are added to a V4L2ControlList instance with the add() method, with
> > - * or without a value.
> > - *
> > - * To write controls to a device, the controls of interest shall be added with
> > - * an initial value by calling V4L2ControlList::add(unsigned int id, int64_t
> > - * value) to prepare for a write operation. Once the values of all controls of
> > - * interest have been added, the V4L2ControlList 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
> > - * V4L2ControlList::add(unsigned int id) to prepare for a read operation. The
> > - * V4L2ControlList instance is then passed to V4L2Device::getControls(), which
> > - * reads the controls from the device and updates the values stored in
> > - * V4L2ControlList.
> > - *
> > - * V4L2ControlList instances can be reset to remove all controls they contain
> > - * and prepare to be re-used for a new control write/read sequence.
> > - */
> > -
> > -/**
> > - * \typedef V4L2ControlList::iterator
> > - * \brief Iterator on the V4L2 controls contained in the instance
> > - */
> > -
> > -/**
> > - * \typedef V4L2ControlList::const_iterator
> > - * \brief Const iterator on the V4L2 controls contained in the instance
> > + * V4L2ControlList should as much as possible not be used in place of
> > + * ControlList in APIs, and be used instead solely for the purpose of easily
> > + * constructing a ControlList of V4L2 controls for a device.
> >   */
> >
> >  /**
> > - * \fn iterator V4L2ControlList::begin()
> > - * \brief Retrieve an iterator to the first V4L2Control in the instance
> > - * \return An iterator to the first V4L2 control
> > + * \brief Construct a V4L2ControlList associated with a V4L2 device
> > + * \param[in] controls The V4L2 device control info map
> 
> The V4L2 device provided control info map ?

I don't think we need "provided", do we ?

> >   */
> > -
> > -/**
> > - * \fn const_iterator V4L2ControlList::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 V4L2ControlList::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 V4L2ControlList::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 V4L2ControlList::empty()
> > - * \brief Verify if the instance does not contain any control
> > - * \return True if the instance does not contain any control, false otherwise
> > - */
> > -
> > -/**
> > - * \fn V4L2ControlList::size()
> > - * \brief Retrieve the number on controls in the instance
> > - * \return The number of V4L2Control stored in the instance
> > - */
> > -
> > -/**
> > - * \fn V4L2ControlList::clear()
> > - * \brief Remove all controls in the instance
> > - */
> > -
> > -/**
> > - * \brief Add control with \a id and optional \a value to the instance
> > - * \param id The V4L2 control ID (V4L2_CID_*)
> > - * \param value The V4L2 control value
> > - *
> > - * This method adds a new V4L2 control to the V4L2ControlList. The newly
> > - * inserted control shall not already be present in the control lists, otherwise
> > - * this method, and further use of the control list lead to undefined behaviour.
> > - */
> > -void V4L2ControlList::add(unsigned int id, int64_t value)
> > +V4L2ControlList::V4L2ControlList(const V4L2ControlInfoMap &controls)
> > +	: ControlList(nullptr), controls_(controls)
> >  {
> > -	controls_.emplace_back(id, value);
> >  }
> >
> >  /**
> > - * \brief Retrieve the control at \a index
> > - * \param[in] index The control index
> > - *
> > - * Controls are stored in a V4L2ControlList in order of insertion and this
> > - * method retrieves the control at \a index.
> > + * \brief Check if the list contains a control with the specified \a id
> > + * \param[in] id The control ID
> >   *
> > - * \return A pointer to the V4L2Control at \a index or nullptr if the
> > - * index is larger than the number of controls
> > + * \return True if the list contains a matching control, false otherwise
> >   */
> > -V4L2Control *V4L2ControlList::getByIndex(unsigned int index)
> > +bool V4L2ControlList::contains(unsigned int id) const
> >  {
> > -	if (index >= controls_.size())
> > -		return nullptr;
> > +	auto ctrl = controls_.find(id);
> > +	if (ctrl == controls_.end())
> > +		return false;
> >
> > -	return &controls_[index];
> > +	return ControlList::contains(ctrl->second.id());
> 
> Ah wait, we have
> ControlList::controls_ and
> V4L2ControlList::controls_
> 
> is this intended ?

Yes. I could rename the latter if you think it would be better. What
name would you prefer ?

> >  }
> >
> >  /**
> > - * \brief Retrieve the control with \a id
> > - * \param[in] id The V4L2 control ID (V4L2_CID_xx)
> > - * \return A pointer to the V4L2Control with \a id or nullptr if the
> > - * control ID is not part of this instance.
> > + * \brief Get the value of control \a id
> > + * \param[in] id The control ID
> > + *
> > + * The behaviour is undefined if the control \a id is not present in the list.
> > + * Use v4L2ControlList::contains() to test for the presence of a control in the
> 
> s/v4L2/V4L2

Good catch, fixed.

> > + * list before retrieving its value.
> > + *
> > + * \return The control value
> >   */
> > -V4L2Control *V4L2ControlList::operator[](unsigned int id)
> > +const ControlValue &V4L2ControlList::get(unsigned int id) const
> >  {
> > -	for (V4L2Control &ctrl : controls_) {
> > -		if (ctrl.id() == id)
> > -			return &ctrl;
> > +	auto ctrl = controls_.find(id);
> > +	if (ctrl == controls_.end()) {
> > +		static ControlValue zero;
> > +		return zero;
> >  	}
> >
> > -	return nullptr;
> > +	return ControlList::get(ctrl->second.id());
> > +}
> > +
> > +/**
> > + * \brief Set the value of control \a id to \a value
> > + * \param[in] id The control ID
> > + * \param[in] value The control value
> > + *
> > + * This method sets the value of a control in the control list. If the control
> > + * is already present in the list, its value is updated, otherwise it is added
> > + * to the list.
> > + *
> > + * The behaviour is undefined if the control \a ctrl is not supported by the
> > + * V4L2 device that the list refers to.
> > + */
> 
> It does not seems undefined, but simply ignored..

As discussed before, undefined doesn't mean that nobody can have a clue
on what happens. It means the API doesn't define the behaviour, and thus
that callers should really not rely on any specific behaviour, even if
the current implementation happens to work for them. The behaviour is
thus undefined.

> > +void V4L2ControlList::set(unsigned int id, const ControlValue &value)
> > +{
> > +	auto ctrl = controls_.find(id);
> > +	if (ctrl == controls_.end())
> > +		return;
> > +
> > +	ControlList::set(ctrl->second.id(), value);
> >  }
> >
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > index 466c3d41f6e3..50616571dcef 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -12,6 +12,8 @@
> >  #include <sys/ioctl.h>
> >  #include <unistd.h>
> >
> > +#include <libcamera/controls.h>
> > +
> >  #include "log.h"
> >  #include "v4l2_controls.h"
> >
> > @@ -163,7 +165,7 @@ void V4L2Device::close()
> >   * \retval -EINVAL One of the control is not supported or not accessible
> >   * \retval i The index of the control that failed
> >   */
> > -int V4L2Device::getControls(V4L2ControlList *ctrls)
> > +int V4L2Device::getControls(ControlList *ctrls)
> >  {
> >  	unsigned int count = ctrls->size();
> >  	if (count == 0)
> > @@ -173,18 +175,21 @@ int V4L2Device::getControls(V4L2ControlList *ctrls)
> >  	struct v4l2_ext_control v4l2Ctrls[count];
> >  	memset(v4l2Ctrls, 0, sizeof(v4l2Ctrls));
> >
> > -	for (unsigned int i = 0; i < count; ++i) {
> > -		const V4L2Control *ctrl = ctrls->getByIndex(i);
> > -		const auto iter = controls_.find(ctrl->id());
> > +	unsigned int i = 0;
> > +	for (const auto &ctrl : *ctrls) {
> > +		const ControlId *id = ctrl.first;
> > +		const auto iter = controls_.find(id->id());
> >  		if (iter == controls_.end()) {
> >  			LOG(V4L2, Error)
> > -				<< "Control '" << ctrl->id() << "' not found";
> > +				<< "Control '" << id->name() << "' not found";
> >  			return -EINVAL;
> >  		}
> >
> >  		const V4L2ControlInfo *info = &iter->second;
> >  		controlInfo[i] = info;
> > -		v4l2Ctrls[i].id = ctrl->id();
> > +		v4l2Ctrls[i].id = id->id();
> > +
> > +		i++;
> >  	}
> >
> >  	struct v4l2_ext_controls v4l2ExtCtrls = {};
> > @@ -238,7 +243,7 @@ int V4L2Device::getControls(V4L2ControlList *ctrls)
> >   * \retval -EINVAL One of the control is not supported or not accessible
> >   * \retval i The index of the control that failed
> >   */
> > -int V4L2Device::setControls(V4L2ControlList *ctrls)
> > +int V4L2Device::setControls(ControlList *ctrls)
> >  {
> >  	unsigned int count = ctrls->size();
> >  	if (count == 0)
> > @@ -248,32 +253,36 @@ int V4L2Device::setControls(V4L2ControlList *ctrls)
> >  	struct v4l2_ext_control v4l2Ctrls[count];
> >  	memset(v4l2Ctrls, 0, sizeof(v4l2Ctrls));
> >
> > -	for (unsigned int i = 0; i < count; ++i) {
> > -		const V4L2Control *ctrl = ctrls->getByIndex(i);
> > -		const auto iter = controls_.find(ctrl->id());
> > +	unsigned int i = 0;
> > +	for (const auto &ctrl : *ctrls) {
> > +		const ControlId *id = ctrl.first;
> > +		const auto iter = controls_.find(id->id());
> >  		if (iter == controls_.end()) {
> >  			LOG(V4L2, Error)
> > -				<< "Control '" << ctrl->id() << "' not found";
> > +				<< "Control '" << id->name() << "' not found";
> >  			return -EINVAL;
> >  		}
> >
> >  		const V4L2ControlInfo *info = &iter->second;
> >  		controlInfo[i] = info;
> > -		v4l2Ctrls[i].id = ctrl->id();
> > +		v4l2Ctrls[i].id = id->id();
> >
> >  		/* Set the v4l2_ext_control value for the write operation. */
> > +		const ControlValue &value = ctrl.second;
> >  		switch (info->type()) {
> >  		case V4L2_CTRL_TYPE_INTEGER64:
> > -			v4l2Ctrls[i].value64 = ctrl->value().get<int64_t>();
> > +			v4l2Ctrls[i].value64 = value.get<int64_t>();
> >  			break;
> >  		default:
> >  			/*
> >  			 * \todo To be changed when support for string and
> >  			 * compound controls will be added.
> >  			 */
> > -			v4l2Ctrls[i].value = ctrl->value().get<int32_t>();
> > +			v4l2Ctrls[i].value64 = value.get<int32_t>();
> 
> value64 or value ?

value. Good catch.

> >  			break;
> >  		}
> > +
> > +		i++;
> >  	}
> >
> >  	struct v4l2_ext_controls v4l2ExtCtrls = {};
> > @@ -382,28 +391,31 @@ void V4L2Device::listControls()
> >   * \param[in] v4l2Ctrls List of V4L2 extended controls as returned by the driver
> >   * \param[in] count The number of controls to update
> >   */
> > -void V4L2Device::updateControls(V4L2ControlList *ctrls,
> > +void V4L2Device::updateControls(ControlList *ctrls,
> >  				const V4L2ControlInfo **controlInfo,
> >  				const struct v4l2_ext_control *v4l2Ctrls,
> >  				unsigned int count)
> >  {
> > -	for (unsigned int i = 0; i < count; ++i) {
> > +	unsigned int i = 0;
> > +	for (auto &ctrl : *ctrls) {
> >  		const struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];
> >  		const V4L2ControlInfo *info = controlInfo[i];
> > -		V4L2Control *ctrl = ctrls->getByIndex(i);
> > +		ControlValue &value = ctrl.second;
> 
> you should break when i == count

Good catch too !

> >
> >  		switch (info->type()) {
> >  		case V4L2_CTRL_TYPE_INTEGER64:
> > -			ctrl->value().set<int64_t>(v4l2Ctrl->value64);
> > +			value.set<int64_t>(v4l2Ctrl->value64);
> >  			break;
> >  		default:
> >  			/*
> >  			 * \todo To be changed when support for string and
> >  			 * compound controls will be added.
> >  			 */
> > -			ctrl->value().set<int32_t>(v4l2Ctrl->value);
> > +			value.set<int32_t>(v4l2Ctrl->value);
> >  			break;
> >  		}
> > +
> > +		i++;
> >  	}
> >  }
> >

Hi Laurent,

On Wed, Oct 09, 2019 at 12:38:54PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Wed, Oct 09, 2019 at 11:13:53AM +0200, Jacopo Mondi wrote:
> > Hi Laurent,
> >     just a note that this break compilation, but you know that already
>
> Isn't it patch 9/9 that breaks compilation ?

yes it is, sorry I mis-read this

>
> > On Tue, Oct 08, 2019 at 01:46:40AM +0300, Laurent Pinchart wrote:
> > > The V4L2Device class uses V4L2ControlList as a controls container for
> > > the getControls() and setControls() operations. Having a distinct
> > > container from ControlList will make the IPA API more complex,
> > > especially when dealing with serialisation and deserialisation, as it
> > > will need to explicitly support transporting both types of lists.
> > >
> > > To simplify the IPA API, replace usage of V4L2ControlList with
> > > ControlList in the V4L2Device (and thus CameraSensor) API. ControlList
> > > however doesn't support accessing controls by numerical ID, which is a
> > > common use case for V4L2 controls in pipeline handlers. To avoid
> > > requiring each pipeline handler to map numerical IDs to ControlId
> > > instances, turn the existing V4L2ControlList class into a helper derived
> > > from ControlList. This allows easy building of a ControlList for a V4L2
> > > device in pipeline handlers, while generalising the use of ControlList
> > > everywhere else.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  src/libcamera/camera_sensor.cpp       |   4 +-
> > >  src/libcamera/include/camera_sensor.h |   5 +-
> > >  src/libcamera/include/v4l2_controls.h |  41 +----
> > >  src/libcamera/include/v4l2_device.h   |   8 +-
> > >  src/libcamera/pipeline/ipu3/ipu3.cpp  |   9 +-
> > >  src/libcamera/pipeline/uvcvideo.cpp   |  24 ++-
> > >  src/libcamera/pipeline/vimc.cpp       |  25 ++-
> > >  src/libcamera/v4l2_controls.cpp       | 216 +++++++-------------------
> > >  src/libcamera/v4l2_device.cpp         |  50 +++---
> > >  9 files changed, 128 insertions(+), 254 deletions(-)
> > >
> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > > index a7670b449b31..9e8b44a23850 100644
> > > --- a/src/libcamera/camera_sensor.cpp
> > > +++ b/src/libcamera/camera_sensor.cpp
> > > @@ -279,7 +279,7 @@ const V4L2ControlInfoMap &CameraSensor::controls() const
> > >   * \retval -EINVAL One of the control is not supported or not accessible
> > >   * \retval i The index of the control that failed
> > >   */
> > > -int CameraSensor::getControls(V4L2ControlList *ctrls)
> > > +int CameraSensor::getControls(ControlList *ctrls)
> > >  {
> > >  	return subdev_->getControls(ctrls);
> > >  }
> > > @@ -309,7 +309,7 @@ int CameraSensor::getControls(V4L2ControlList *ctrls)
> > >   * \retval -EINVAL One of the control is not supported or not accessible
> > >   * \retval i The index of the control that failed
> > >   */
> > > -int CameraSensor::setControls(V4L2ControlList *ctrls)
> > > +int CameraSensor::setControls(ControlList *ctrls)
> > >  {
> > >  	return subdev_->setControls(ctrls);
> > >  }
> > > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h
> > > index fe033fb374c1..5ad829232f26 100644
> > > --- a/src/libcamera/include/camera_sensor.h
> > > +++ b/src/libcamera/include/camera_sensor.h
> > > @@ -17,6 +17,7 @@
> > >
> > >  namespace libcamera {
> > >
> > > +class ControlList;
> > >  class MediaEntity;
> > >  class V4L2Subdevice;
> > >
> > > @@ -43,8 +44,8 @@ public:
> > >  	int setFormat(V4L2SubdeviceFormat *format);
> > >
> > >  	const V4L2ControlInfoMap &controls() const;
> > > -	int getControls(V4L2ControlList *ctrls);
> > > -	int setControls(V4L2ControlList *ctrls);
> > > +	int getControls(ControlList *ctrls);
> > > +	int setControls(ControlList *ctrls);
> > >
> > >  protected:
> > >  	std::string logPrefix() const;
> > > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h
> > > index 71ce377fe4c5..1d85ecf9864a 100644
> > > --- a/src/libcamera/include/v4l2_controls.h
> > > +++ b/src/libcamera/include/v4l2_controls.h
> > > @@ -13,7 +13,6 @@
> > >  #include <string>
> > >  #include <vector>
> > >
> > > -#include <linux/v4l2-controls.h>
> > >  #include <linux/videodev2.h>
> > >
> > >  #include <libcamera/controls.h>
> > > @@ -47,45 +46,17 @@ private:
> > >
> > >  using V4L2ControlInfoMap = std::map<unsigned int, V4L2ControlInfo>;
> > >
> > > -class V4L2Control
> > > +class V4L2ControlList : public ControlList
> > >  {
> > >  public:
> > > -	V4L2Control(unsigned int id, const ControlValue &value = ControlValue())
> > > -		: id_(id), value_(value)
> > > -	{
> > > -	}
> > > +	V4L2ControlList(const V4L2ControlInfoMap &controls);
> >
> > This comes from the video device and I think it's fine as it's
> > unlikely the V4L2ControlList outlives a video device, right ?
>
> Yes, it shouldn't be a problem.
>
> > >
> > > -	unsigned int id() const { return id_; }
> > > -	const ControlValue &value() const { return value_; }
> > > -	ControlValue &value() { return value_; }
> > > +	bool contains(unsigned int id) const;
> > > +	const ControlValue &get(unsigned int id) const;
> > > +	void set(unsigned int id, const ControlValue &value);
> > >
> > >  private:
> > > -	unsigned int id_;
> > > -	ControlValue value_;
> > > -};
> > > -
> > > -class V4L2ControlList
> > > -{
> > > -public:
> > > -	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(); }
> > > -	std::size_t size() const { return controls_.size(); }
> > > -
> > > -	void clear() { controls_.clear(); }
> > > -	void add(unsigned int id, int64_t value = 0);
> > > -
> > > -	V4L2Control *getByIndex(unsigned int index);
> > > -	V4L2Control *operator[](unsigned int id);
> > > -
> > > -private:
> > > -	std::vector<V4L2Control> controls_;
> > > +	const V4L2ControlInfoMap &controls_;
> > >  };
> > >
> > >  } /* namespace libcamera */
> > > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> > > index 75a52c33d821..0375d3a1cb82 100644
> > > --- a/src/libcamera/include/v4l2_device.h
> > > +++ b/src/libcamera/include/v4l2_device.h
> > > @@ -17,6 +17,8 @@
> > >
> > >  namespace libcamera {
> > >
> > > +class ControlList;
> > > +
> > >  class V4L2Device : protected Loggable
> > >  {
> > >  public:
> > > @@ -25,8 +27,8 @@ public:
> > >
> > >  	const V4L2ControlInfoMap &controls() const { return controls_; }
> > >
> > > -	int getControls(V4L2ControlList *ctrls);
> > > -	int setControls(V4L2ControlList *ctrls);
> > > +	int getControls(ControlList *ctrls);
> > > +	int setControls(ControlList *ctrls);
> > >
> > >  	const std::string &deviceNode() const { return deviceNode_; }
> > >
> > > @@ -43,7 +45,7 @@ protected:
> > >
> > >  private:
> > >  	void listControls();
> > > -	void updateControls(V4L2ControlList *ctrls,
> > > +	void updateControls(ControlList *ctrls,
> > >  			    const V4L2ControlInfo **controlInfo,
> > >  			    const struct v4l2_ext_control *v4l2Ctrls,
> > >  			    unsigned int count);
> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > index 827906d5cd2e..9776b36b88cd 100644
> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > @@ -199,6 +199,7 @@ class PipelineHandlerIPU3 : public PipelineHandler
> > >  {
> > >  public:
> > >  	static constexpr unsigned int V4L2_CID_IPU3_PIPE_MODE = 0x009819c1;
> > > +
> > >  	enum IPU3PipeModes {
> > >  		IPU3PipeModeVideo = 0,
> > >  		IPU3PipeModeStillCapture = 1,
> > > @@ -603,10 +604,10 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> > >  		return ret;
> > >
> > >  	/* Apply the "pipe_mode" control to the ImgU subdevice. */
> > > -	V4L2ControlList ctrls;
> > > -	ctrls.add(V4L2_CID_IPU3_PIPE_MODE, vfStream->active_ ?
> > > -					   IPU3PipeModeVideo :
> > > -					   IPU3PipeModeStillCapture);
> > > +	V4L2ControlList ctrls(imgu->imgu_->controls());
> > > +	ctrls.set(V4L2_CID_IPU3_PIPE_MODE,
> > > +		  static_cast<int32_t>(vfStream->active_ ? IPU3PipeModeVideo :
> > > +				       IPU3PipeModeStillCapture));
> > >  	ret = imgu->imgu_->setControls(&ctrls);
> > >  	if (ret) {
> > >  		LOG(IPU3, Error) << "Unable to set pipe_mode control";
> > > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> > > index 88f7fb9bc568..467bd9a9eaff 100644
> > > --- a/src/libcamera/pipeline/uvcvideo.cpp
> > > +++ b/src/libcamera/pipeline/uvcvideo.cpp
> > > @@ -228,31 +228,30 @@ void PipelineHandlerUVC::stop(Camera *camera)
> > >
> > >  int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)
> > >  {
> > > -	V4L2ControlList controls;
> > > +	V4L2ControlList controls(data->video_->controls());
> > >
> > >  	for (auto it : request->controls()) {
> > >  		const ControlId &id = *it.first;
> > >  		ControlValue &value = it.second;
> > >
> > >  		if (id == controls::Brightness) {
> > > -			controls.add(V4L2_CID_BRIGHTNESS, value.get<int32_t>());
> > > +			controls.set(V4L2_CID_BRIGHTNESS, value);
> > >  		} else if (id == controls::Contrast) {
> > > -			controls.add(V4L2_CID_CONTRAST, value.get<int32_t>());
> > > +			controls.set(V4L2_CID_CONTRAST, value);
> > >  		} else if (id == controls::Saturation) {
> > > -			controls.add(V4L2_CID_SATURATION, value.get<int32_t>());
> > > +			controls.set(V4L2_CID_SATURATION, value);
> > >  		} else if (id == controls::ManualExposure) {
> > > -			controls.add(V4L2_CID_EXPOSURE_AUTO, 1);
> > > -			controls.add(V4L2_CID_EXPOSURE_ABSOLUTE, value.get<int32_t>());
> > > +			controls.set(V4L2_CID_EXPOSURE_AUTO, 1);
> > > +			controls.set(V4L2_CID_EXPOSURE_ABSOLUTE, value);
> > >  		} else if (id == controls::ManualGain) {
> > > -			controls.add(V4L2_CID_GAIN, value.get<int32_t>());
> > > +			controls.set(V4L2_CID_GAIN, value);
> > >  		}
> > >  	}
> > >
> > > -	for (const V4L2Control &ctrl : controls)
> > > +	for (const auto &ctrl : controls)
> > >  		LOG(UVC, Debug)
> > > -			<< "Setting control 0x"
> > > -			<< std::hex << std::setw(8) << ctrl.id() << std::dec
> > > -			<< " to " << ctrl.value().toString();
> > > +			<< "Setting control " << ctrl.first->name()
> > > +			<< " to " << ctrl.second.toString();
> > >
> > >  	int ret = data->video_->setControls(&controls);
> > >  	if (ret) {
> > > @@ -338,11 +337,10 @@ int UVCCameraData::init(MediaEntity *entity)
> > >  	/* Initialise the supported controls. */
> > >  	const V4L2ControlInfoMap &controls = video_->controls();
> > >  	for (const auto &ctrl : controls) {
> > > -		unsigned int v4l2Id = ctrl.first;
> > >  		const V4L2ControlInfo &info = ctrl.second;
> > >  		const ControlId *id;
> > >
> > > -		switch (v4l2Id) {
> > > +		switch (info.id().id()) {
> > >  		case V4L2_CID_BRIGHTNESS:
> > >  			id = &controls::Brightness;
> > >  			break;
> > > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> > > index 26477dccbb8f..600a1154c75e 100644
> > > --- a/src/libcamera/pipeline/vimc.cpp
> > > +++ b/src/libcamera/pipeline/vimc.cpp
> > > @@ -281,26 +281,24 @@ void PipelineHandlerVimc::stop(Camera *camera)
> > >
> > >  int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)
> > >  {
> > > -	V4L2ControlList controls;
> > > +	V4L2ControlList controls(data->sensor_->controls());
> > >
> > >  	for (auto it : request->controls()) {
> > >  		const ControlId &id = *it.first;
> > >  		ControlValue &value = it.second;
> > >
> > > -		if (id == controls::Brightness) {
> > > -			controls.add(V4L2_CID_BRIGHTNESS, value.get<int32_t>());
> > > -		} else if (id == controls::Contrast) {
> > > -			controls.add(V4L2_CID_CONTRAST, value.get<int32_t>());
> > > -		} else if (id == controls::Saturation) {
> > > -			controls.add(V4L2_CID_SATURATION, value.get<int32_t>());
> > > -		}
> > > +		if (id == controls::Brightness)
> > > +			controls.set(V4L2_CID_BRIGHTNESS, value);
> > > +		else if (id == controls::Contrast)
> > > +			controls.set(V4L2_CID_CONTRAST, value);
> > > +		else if (id == controls::Saturation)
> > > +			controls.set(V4L2_CID_SATURATION, value);
> > >  	}
> > >
> > > -	for (const V4L2Control &ctrl : controls)
> > > +	for (const auto &ctrl : controls)
> > >  		LOG(VIMC, Debug)
> > > -			<< "Setting control 0x"
> > > -			<< std::hex << std::setw(8) << ctrl.id() << std::dec
> > > -			<< " to " << ctrl.value().toString();
> > > +			<< "Setting control " << ctrl.first->name()
> > > +			<< " to " << ctrl.second.toString();
> > >
> > >  	int ret = data->sensor_->setControls(&controls);
> > >  	if (ret) {
> > > @@ -417,11 +415,10 @@ int VimcCameraData::init(MediaDevice *media)
> > >  	/* Initialise the supported controls. */
> > >  	const V4L2ControlInfoMap &controls = sensor_->controls();
> > >  	for (const auto &ctrl : controls) {
> > > -		unsigned int v4l2Id = ctrl.first;
> > >  		const V4L2ControlInfo &info = ctrl.second;
> > >  		const ControlId *id;
> > >
> > > -		switch (v4l2Id) {
> > > +		switch (info.id().id()) {
> > >  		case V4L2_CID_BRIGHTNESS:
> > >  			id = &controls::Brightness;
> > >  			break;
> > > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> > > index a630a2583d33..98b2b3fe9d0a 100644
> > > --- a/src/libcamera/v4l2_controls.cpp
> > > +++ b/src/libcamera/v4l2_controls.cpp
> > > @@ -167,191 +167,83 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
> > >   * \brief A map of control ID to V4L2ControlInfo
> > >   */
> > >
> > > -/**
> > > - * \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 modified by a control read or write operation.
> > > - *
> > > - * The write and read controls the V4L2Control class instances are not meant
> > > - * to be directly used but are instead intended to be grouped in
> > > - * V4L2ControlList instances, which are then passed as parameters to
> > > - * V4L2Device::setControls() and V4L2Device::getControls() operations.
> > > - */
> > > -
> > > -/**
> > > - * \fn V4L2Control::V4L2Control
> > > - * \brief Construct a V4L2 control with \a id and \a value
> > > - * \param id The V4L2 control ID
> > > - * \param value The control value
> > > - */
> > > -
> > > -/**
> > > - * \fn V4L2Control::value() const
> > > - * \brief Retrieve the value of the control
> > > - *
> > > - * This method is a const version of V4L2Control::value(), returning a const
> > > - * reference to the value.
> > > - *
> > > - * \return The V4L2 control value
> > > - */
> > > -
> > > -/**
> > > - * \fn V4L2Control::value()
> > > - * \brief Retrieve the value of the control
> > > - *
> > > - * This method returns the cached control value, initially set by
> > > - * V4L2ControlList::add() and then updated when the controls are read or
> > > - * written with V4L2Device::getControls() and V4L2Device::setControls().
> > > - *
> > > - * \return The V4L2 control value
> > > - */
> > > -
> > > -/**
> > > - * \fn V4L2Control::id()
> > > - * \brief Retrieve the control ID this instance refers to
> > > - * \return The V4L2Control ID
> > > - */
> > > -
> > >  /**
> > >   * \class V4L2ControlList
> > > - * \brief Container of V4L2Control instances
> > > + * \brief A list of controls for a V4L2 device
> > >   *
> > > - * The V4L2ControlList 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.
> > > + * This class specialises the ControList class for use with V4L2 devices. It
> > > + * offers a convenience API to access controls by numerical ID instead of
> > > + * ControlId.
> > >   *
> > > - * In order to set and get controls, user of the libcamera V4L2 control
> > > - * framework should operate on instances of the V4L2ControlList class, and use
> > > - * them as argument for the V4L2Device::setControls() and
> > > - * V4L2Device::getControls() operations, which write and read a list of
> > > - * controls to or from a V4L2 device (a video device or a subdevice).
> >
> > I would not remove this part, as it explains how to use the
> > V4L2ControlList, and it still applies today even if those methods
> > accepts a ControlList *
>
> Doesn't it belong to V4L2Device though ?
>

Possibly, but it also seems to me that it explains how to use a
V4L2ControlList. Up to you..

> > > - *
> > > - * Controls are added to a V4L2ControlList instance with the add() method, with
> > > - * or without a value.
> > > - *
> > > - * To write controls to a device, the controls of interest shall be added with
> > > - * an initial value by calling V4L2ControlList::add(unsigned int id, int64_t
> > > - * value) to prepare for a write operation. Once the values of all controls of
> > > - * interest have been added, the V4L2ControlList 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
> > > - * V4L2ControlList::add(unsigned int id) to prepare for a read operation. The
> > > - * V4L2ControlList instance is then passed to V4L2Device::getControls(), which
> > > - * reads the controls from the device and updates the values stored in
> > > - * V4L2ControlList.
> > > - *
> > > - * V4L2ControlList instances can be reset to remove all controls they contain
> > > - * and prepare to be re-used for a new control write/read sequence.
> > > - */
> > > -
> > > -/**
> > > - * \typedef V4L2ControlList::iterator
> > > - * \brief Iterator on the V4L2 controls contained in the instance
> > > - */
> > > -
> > > -/**
> > > - * \typedef V4L2ControlList::const_iterator
> > > - * \brief Const iterator on the V4L2 controls contained in the instance
> > > + * V4L2ControlList should as much as possible not be used in place of
> > > + * ControlList in APIs, and be used instead solely for the purpose of easily
> > > + * constructing a ControlList of V4L2 controls for a device.
> > >   */
> > >
> > >  /**
> > > - * \fn iterator V4L2ControlList::begin()
> > > - * \brief Retrieve an iterator to the first V4L2Control in the instance
> > > - * \return An iterator to the first V4L2 control
> > > + * \brief Construct a V4L2ControlList associated with a V4L2 device
> > > + * \param[in] controls The V4L2 device control info map
> >
> > The V4L2 device provided control info map ?
>
> I don't think we need "provided", do we ?
>

Not sure, if it sounds well for you let's keep it this way

> > >   */
> > > -
> > > -/**
> > > - * \fn const_iterator V4L2ControlList::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 V4L2ControlList::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 V4L2ControlList::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 V4L2ControlList::empty()
> > > - * \brief Verify if the instance does not contain any control
> > > - * \return True if the instance does not contain any control, false otherwise
> > > - */
> > > -
> > > -/**
> > > - * \fn V4L2ControlList::size()
> > > - * \brief Retrieve the number on controls in the instance
> > > - * \return The number of V4L2Control stored in the instance
> > > - */
> > > -
> > > -/**
> > > - * \fn V4L2ControlList::clear()
> > > - * \brief Remove all controls in the instance
> > > - */
> > > -
> > > -/**
> > > - * \brief Add control with \a id and optional \a value to the instance
> > > - * \param id The V4L2 control ID (V4L2_CID_*)
> > > - * \param value The V4L2 control value
> > > - *
> > > - * This method adds a new V4L2 control to the V4L2ControlList. The newly
> > > - * inserted control shall not already be present in the control lists, otherwise
> > > - * this method, and further use of the control list lead to undefined behaviour.
> > > - */
> > > -void V4L2ControlList::add(unsigned int id, int64_t value)
> > > +V4L2ControlList::V4L2ControlList(const V4L2ControlInfoMap &controls)
> > > +	: ControlList(nullptr), controls_(controls)
> > >  {
> > > -	controls_.emplace_back(id, value);
> > >  }
> > >
> > >  /**
> > > - * \brief Retrieve the control at \a index
> > > - * \param[in] index The control index
> > > - *
> > > - * Controls are stored in a V4L2ControlList in order of insertion and this
> > > - * method retrieves the control at \a index.
> > > + * \brief Check if the list contains a control with the specified \a id
> > > + * \param[in] id The control ID
> > >   *
> > > - * \return A pointer to the V4L2Control at \a index or nullptr if the
> > > - * index is larger than the number of controls
> > > + * \return True if the list contains a matching control, false otherwise
> > >   */
> > > -V4L2Control *V4L2ControlList::getByIndex(unsigned int index)
> > > +bool V4L2ControlList::contains(unsigned int id) const
> > >  {
> > > -	if (index >= controls_.size())
> > > -		return nullptr;
> > > +	auto ctrl = controls_.find(id);
> > > +	if (ctrl == controls_.end())
> > > +		return false;
> > >
> > > -	return &controls_[index];
> > > +	return ControlList::contains(ctrl->second.id());
> >
> > Ah wait, we have
> > ControlList::controls_ and
> > V4L2ControlList::controls_
> >
> > is this intended ?
>
> Yes. I could rename the latter if you think it would be better. What
> name would you prefer ?

I'm a bit bothered by having two different things with the same name,
even if their scope is different... I would name the latter
controlInfoMap_ even if it's a bit longer

>
> > >  }
> > >
> > >  /**
> > > - * \brief Retrieve the control with \a id
> > > - * \param[in] id The V4L2 control ID (V4L2_CID_xx)
> > > - * \return A pointer to the V4L2Control with \a id or nullptr if the
> > > - * control ID is not part of this instance.
> > > + * \brief Get the value of control \a id
> > > + * \param[in] id The control ID
> > > + *
> > > + * The behaviour is undefined if the control \a id is not present in the list.
> > > + * Use v4L2ControlList::contains() to test for the presence of a control in the
> >
> > s/v4L2/V4L2
>
> Good catch, fixed.
>
> > > + * list before retrieving its value.
> > > + *
> > > + * \return The control value
> > >   */
> > > -V4L2Control *V4L2ControlList::operator[](unsigned int id)
> > > +const ControlValue &V4L2ControlList::get(unsigned int id) const
> > >  {
> > > -	for (V4L2Control &ctrl : controls_) {
> > > -		if (ctrl.id() == id)
> > > -			return &ctrl;
> > > +	auto ctrl = controls_.find(id);
> > > +	if (ctrl == controls_.end()) {
> > > +		static ControlValue zero;
> > > +		return zero;
> > >  	}
> > >
> > > -	return nullptr;
> > > +	return ControlList::get(ctrl->second.id());
> > > +}
> > > +
> > > +/**
> > > + * \brief Set the value of control \a id to \a value
> > > + * \param[in] id The control ID
> > > + * \param[in] value The control value
> > > + *
> > > + * This method sets the value of a control in the control list. If the control
> > > + * is already present in the list, its value is updated, otherwise it is added
> > > + * to the list.
> > > + *
> > > + * The behaviour is undefined if the control \a ctrl is not supported by the
> > > + * V4L2 device that the list refers to.
> > > + */
> >
> > It does not seems undefined, but simply ignored..
>
> As discussed before, undefined doesn't mean that nobody can have a clue
> on what happens. It means the API doesn't define the behaviour, and thus
> that callers should really not rely on any specific behaviour, even if
> the current implementation happens to work for them. The behaviour is
> thus undefined.
>

ok then (but still, the operations has simply no effect in this case)

> > > +void V4L2ControlList::set(unsigned int id, const ControlValue &value)
> > > +{
> > > +	auto ctrl = controls_.find(id);
> > > +	if (ctrl == controls_.end())
> > > +		return;
> > > +
> > > +	ControlList::set(ctrl->second.id(), value);
> > >  }
> > >
> > >  } /* namespace libcamera */
> > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > > index 466c3d41f6e3..50616571dcef 100644
> > > --- a/src/libcamera/v4l2_device.cpp
> > > +++ b/src/libcamera/v4l2_device.cpp
> > > @@ -12,6 +12,8 @@
> > >  #include <sys/ioctl.h>
> > >  #include <unistd.h>
> > >
> > > +#include <libcamera/controls.h>
> > > +
> > >  #include "log.h"
> > >  #include "v4l2_controls.h"
> > >
> > > @@ -163,7 +165,7 @@ void V4L2Device::close()
> > >   * \retval -EINVAL One of the control is not supported or not accessible
> > >   * \retval i The index of the control that failed
> > >   */
> > > -int V4L2Device::getControls(V4L2ControlList *ctrls)
> > > +int V4L2Device::getControls(ControlList *ctrls)
> > >  {
> > >  	unsigned int count = ctrls->size();
> > >  	if (count == 0)
> > > @@ -173,18 +175,21 @@ int V4L2Device::getControls(V4L2ControlList *ctrls)
> > >  	struct v4l2_ext_control v4l2Ctrls[count];
> > >  	memset(v4l2Ctrls, 0, sizeof(v4l2Ctrls));
> > >
> > > -	for (unsigned int i = 0; i < count; ++i) {
> > > -		const V4L2Control *ctrl = ctrls->getByIndex(i);
> > > -		const auto iter = controls_.find(ctrl->id());
> > > +	unsigned int i = 0;
> > > +	for (const auto &ctrl : *ctrls) {
> > > +		const ControlId *id = ctrl.first;
> > > +		const auto iter = controls_.find(id->id());
> > >  		if (iter == controls_.end()) {
> > >  			LOG(V4L2, Error)
> > > -				<< "Control '" << ctrl->id() << "' not found";
> > > +				<< "Control '" << id->name() << "' not found";
> > >  			return -EINVAL;
> > >  		}
> > >
> > >  		const V4L2ControlInfo *info = &iter->second;
> > >  		controlInfo[i] = info;
> > > -		v4l2Ctrls[i].id = ctrl->id();
> > > +		v4l2Ctrls[i].id = id->id();
> > > +
> > > +		i++;
> > >  	}
> > >
> > >  	struct v4l2_ext_controls v4l2ExtCtrls = {};
> > > @@ -238,7 +243,7 @@ int V4L2Device::getControls(V4L2ControlList *ctrls)
> > >   * \retval -EINVAL One of the control is not supported or not accessible
> > >   * \retval i The index of the control that failed
> > >   */
> > > -int V4L2Device::setControls(V4L2ControlList *ctrls)
> > > +int V4L2Device::setControls(ControlList *ctrls)
> > >  {
> > >  	unsigned int count = ctrls->size();
> > >  	if (count == 0)
> > > @@ -248,32 +253,36 @@ int V4L2Device::setControls(V4L2ControlList *ctrls)
> > >  	struct v4l2_ext_control v4l2Ctrls[count];
> > >  	memset(v4l2Ctrls, 0, sizeof(v4l2Ctrls));
> > >
> > > -	for (unsigned int i = 0; i < count; ++i) {
> > > -		const V4L2Control *ctrl = ctrls->getByIndex(i);
> > > -		const auto iter = controls_.find(ctrl->id());
> > > +	unsigned int i = 0;
> > > +	for (const auto &ctrl : *ctrls) {
> > > +		const ControlId *id = ctrl.first;
> > > +		const auto iter = controls_.find(id->id());
> > >  		if (iter == controls_.end()) {
> > >  			LOG(V4L2, Error)
> > > -				<< "Control '" << ctrl->id() << "' not found";
> > > +				<< "Control '" << id->name() << "' not found";
> > >  			return -EINVAL;
> > >  		}
> > >
> > >  		const V4L2ControlInfo *info = &iter->second;
> > >  		controlInfo[i] = info;
> > > -		v4l2Ctrls[i].id = ctrl->id();
> > > +		v4l2Ctrls[i].id = id->id();
> > >
> > >  		/* Set the v4l2_ext_control value for the write operation. */
> > > +		const ControlValue &value = ctrl.second;
> > >  		switch (info->type()) {
> > >  		case V4L2_CTRL_TYPE_INTEGER64:
> > > -			v4l2Ctrls[i].value64 = ctrl->value().get<int64_t>();
> > > +			v4l2Ctrls[i].value64 = value.get<int64_t>();
> > >  			break;
> > >  		default:
> > >  			/*
> > >  			 * \todo To be changed when support for string and
> > >  			 * compound controls will be added.
> > >  			 */
> > > -			v4l2Ctrls[i].value = ctrl->value().get<int32_t>();
> > > +			v4l2Ctrls[i].value64 = value.get<int32_t>();
> >
> > value64 or value ?
>
> value. Good catch.
>
> > >  			break;
> > >  		}
> > > +
> > > +		i++;
> > >  	}
> > >
> > >  	struct v4l2_ext_controls v4l2ExtCtrls = {};
> > > @@ -382,28 +391,31 @@ void V4L2Device::listControls()
> > >   * \param[in] v4l2Ctrls List of V4L2 extended controls as returned by the driver
> > >   * \param[in] count The number of controls to update
> > >   */
> > > -void V4L2Device::updateControls(V4L2ControlList *ctrls,
> > > +void V4L2Device::updateControls(ControlList *ctrls,
> > >  				const V4L2ControlInfo **controlInfo,
> > >  				const struct v4l2_ext_control *v4l2Ctrls,
> > >  				unsigned int count)
> > >  {
> > > -	for (unsigned int i = 0; i < count; ++i) {
> > > +	unsigned int i = 0;
> > > +	for (auto &ctrl : *ctrls) {
> > >  		const struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];
> > >  		const V4L2ControlInfo *info = controlInfo[i];
> > > -		V4L2Control *ctrl = ctrls->getByIndex(i);
> > > +		ControlValue &value = ctrl.second;
> >
> > you should break when i == count
>
> Good catch too !
>
> > >
> > >  		switch (info->type()) {
> > >  		case V4L2_CTRL_TYPE_INTEGER64:
> > > -			ctrl->value().set<int64_t>(v4l2Ctrl->value64);
> > > +			value.set<int64_t>(v4l2Ctrl->value64);
> > >  			break;
> > >  		default:
> > >  			/*
> > >  			 * \todo To be changed when support for string and
> > >  			 * compound controls will be added.
> > >  			 */
> > > -			ctrl->value().set<int32_t>(v4l2Ctrl->value);
> > > +			value.set<int32_t>(v4l2Ctrl->value);
> > >  			break;
> > >  		}
> > > +
> > > +		i++;
> > >  	}
> > >  }
> > >
>
> --
> Regards,
>
> Laurent Pinchart

Hi Jacopo,

On Wed, Oct 09, 2019 at 11:56:55AM +0200, Jacopo Mondi wrote:
> On Wed, Oct 09, 2019 at 12:38:54PM +0300, Laurent Pinchart wrote:
> > On Wed, Oct 09, 2019 at 11:13:53AM +0200, Jacopo Mondi wrote:
> >> Hi Laurent,
> >>     just a note that this break compilation, but you know that already
> >
> > Isn't it patch 9/9 that breaks compilation ?
> 
> yes it is, sorry I mis-read this
> 
> >> On Tue, Oct 08, 2019 at 01:46:40AM +0300, Laurent Pinchart wrote:
> >>> The V4L2Device class uses V4L2ControlList as a controls container for
> >>> the getControls() and setControls() operations. Having a distinct
> >>> container from ControlList will make the IPA API more complex,
> >>> especially when dealing with serialisation and deserialisation, as it
> >>> will need to explicitly support transporting both types of lists.
> >>>
> >>> To simplify the IPA API, replace usage of V4L2ControlList with
> >>> ControlList in the V4L2Device (and thus CameraSensor) API. ControlList
> >>> however doesn't support accessing controls by numerical ID, which is a
> >>> common use case for V4L2 controls in pipeline handlers. To avoid
> >>> requiring each pipeline handler to map numerical IDs to ControlId
> >>> instances, turn the existing V4L2ControlList class into a helper derived
> >>> from ControlList. This allows easy building of a ControlList for a V4L2
> >>> device in pipeline handlers, while generalising the use of ControlList
> >>> everywhere else.
> >>>
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>> ---
> >>>  src/libcamera/camera_sensor.cpp       |   4 +-
> >>>  src/libcamera/include/camera_sensor.h |   5 +-
> >>>  src/libcamera/include/v4l2_controls.h |  41 +----
> >>>  src/libcamera/include/v4l2_device.h   |   8 +-
> >>>  src/libcamera/pipeline/ipu3/ipu3.cpp  |   9 +-
> >>>  src/libcamera/pipeline/uvcvideo.cpp   |  24 ++-
> >>>  src/libcamera/pipeline/vimc.cpp       |  25 ++-
> >>>  src/libcamera/v4l2_controls.cpp       | 216 +++++++-------------------
> >>>  src/libcamera/v4l2_device.cpp         |  50 +++---
> >>>  9 files changed, 128 insertions(+), 254 deletions(-)
> >>>
> >>> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> >>> index a7670b449b31..9e8b44a23850 100644
> >>> --- a/src/libcamera/camera_sensor.cpp
> >>> +++ b/src/libcamera/camera_sensor.cpp
> >>> @@ -279,7 +279,7 @@ const V4L2ControlInfoMap &CameraSensor::controls() const
> >>>   * \retval -EINVAL One of the control is not supported or not accessible
> >>>   * \retval i The index of the control that failed
> >>>   */
> >>> -int CameraSensor::getControls(V4L2ControlList *ctrls)
> >>> +int CameraSensor::getControls(ControlList *ctrls)
> >>>  {
> >>>  	return subdev_->getControls(ctrls);
> >>>  }
> >>> @@ -309,7 +309,7 @@ int CameraSensor::getControls(V4L2ControlList *ctrls)
> >>>   * \retval -EINVAL One of the control is not supported or not accessible
> >>>   * \retval i The index of the control that failed
> >>>   */
> >>> -int CameraSensor::setControls(V4L2ControlList *ctrls)
> >>> +int CameraSensor::setControls(ControlList *ctrls)
> >>>  {
> >>>  	return subdev_->setControls(ctrls);
> >>>  }
> >>> diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h
> >>> index fe033fb374c1..5ad829232f26 100644
> >>> --- a/src/libcamera/include/camera_sensor.h
> >>> +++ b/src/libcamera/include/camera_sensor.h
> >>> @@ -17,6 +17,7 @@
> >>>
> >>>  namespace libcamera {
> >>>
> >>> +class ControlList;
> >>>  class MediaEntity;
> >>>  class V4L2Subdevice;
> >>>
> >>> @@ -43,8 +44,8 @@ public:
> >>>  	int setFormat(V4L2SubdeviceFormat *format);
> >>>
> >>>  	const V4L2ControlInfoMap &controls() const;
> >>> -	int getControls(V4L2ControlList *ctrls);
> >>> -	int setControls(V4L2ControlList *ctrls);
> >>> +	int getControls(ControlList *ctrls);
> >>> +	int setControls(ControlList *ctrls);
> >>>
> >>>  protected:
> >>>  	std::string logPrefix() const;
> >>> diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h
> >>> index 71ce377fe4c5..1d85ecf9864a 100644
> >>> --- a/src/libcamera/include/v4l2_controls.h
> >>> +++ b/src/libcamera/include/v4l2_controls.h
> >>> @@ -13,7 +13,6 @@
> >>>  #include <string>
> >>>  #include <vector>
> >>>
> >>> -#include <linux/v4l2-controls.h>
> >>>  #include <linux/videodev2.h>
> >>>
> >>>  #include <libcamera/controls.h>
> >>> @@ -47,45 +46,17 @@ private:
> >>>
> >>>  using V4L2ControlInfoMap = std::map<unsigned int, V4L2ControlInfo>;
> >>>
> >>> -class V4L2Control
> >>> +class V4L2ControlList : public ControlList
> >>>  {
> >>>  public:
> >>> -	V4L2Control(unsigned int id, const ControlValue &value = ControlValue())
> >>> -		: id_(id), value_(value)
> >>> -	{
> >>> -	}
> >>> +	V4L2ControlList(const V4L2ControlInfoMap &controls);
> >>
> >> This comes from the video device and I think it's fine as it's
> >> unlikely the V4L2ControlList outlives a video device, right ?
> >
> > Yes, it shouldn't be a problem.
> >
> >>>
> >>> -	unsigned int id() const { return id_; }
> >>> -	const ControlValue &value() const { return value_; }
> >>> -	ControlValue &value() { return value_; }
> >>> +	bool contains(unsigned int id) const;
> >>> +	const ControlValue &get(unsigned int id) const;
> >>> +	void set(unsigned int id, const ControlValue &value);
> >>>
> >>>  private:
> >>> -	unsigned int id_;
> >>> -	ControlValue value_;
> >>> -};
> >>> -
> >>> -class V4L2ControlList
> >>> -{
> >>> -public:
> >>> -	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(); }
> >>> -	std::size_t size() const { return controls_.size(); }
> >>> -
> >>> -	void clear() { controls_.clear(); }
> >>> -	void add(unsigned int id, int64_t value = 0);
> >>> -
> >>> -	V4L2Control *getByIndex(unsigned int index);
> >>> -	V4L2Control *operator[](unsigned int id);
> >>> -
> >>> -private:
> >>> -	std::vector<V4L2Control> controls_;
> >>> +	const V4L2ControlInfoMap &controls_;
> >>>  };
> >>>
> >>>  } /* namespace libcamera */
> >>> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> >>> index 75a52c33d821..0375d3a1cb82 100644
> >>> --- a/src/libcamera/include/v4l2_device.h
> >>> +++ b/src/libcamera/include/v4l2_device.h
> >>> @@ -17,6 +17,8 @@
> >>>
> >>>  namespace libcamera {
> >>>
> >>> +class ControlList;
> >>> +
> >>>  class V4L2Device : protected Loggable
> >>>  {
> >>>  public:
> >>> @@ -25,8 +27,8 @@ public:
> >>>
> >>>  	const V4L2ControlInfoMap &controls() const { return controls_; }
> >>>
> >>> -	int getControls(V4L2ControlList *ctrls);
> >>> -	int setControls(V4L2ControlList *ctrls);
> >>> +	int getControls(ControlList *ctrls);
> >>> +	int setControls(ControlList *ctrls);
> >>>
> >>>  	const std::string &deviceNode() const { return deviceNode_; }
> >>>
> >>> @@ -43,7 +45,7 @@ protected:
> >>>
> >>>  private:
> >>>  	void listControls();
> >>> -	void updateControls(V4L2ControlList *ctrls,
> >>> +	void updateControls(ControlList *ctrls,
> >>>  			    const V4L2ControlInfo **controlInfo,
> >>>  			    const struct v4l2_ext_control *v4l2Ctrls,
> >>>  			    unsigned int count);
> >>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>> index 827906d5cd2e..9776b36b88cd 100644
> >>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>> @@ -199,6 +199,7 @@ class PipelineHandlerIPU3 : public PipelineHandler
> >>>  {
> >>>  public:
> >>>  	static constexpr unsigned int V4L2_CID_IPU3_PIPE_MODE = 0x009819c1;
> >>> +
> >>>  	enum IPU3PipeModes {
> >>>  		IPU3PipeModeVideo = 0,
> >>>  		IPU3PipeModeStillCapture = 1,
> >>> @@ -603,10 +604,10 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> >>>  		return ret;
> >>>
> >>>  	/* Apply the "pipe_mode" control to the ImgU subdevice. */
> >>> -	V4L2ControlList ctrls;
> >>> -	ctrls.add(V4L2_CID_IPU3_PIPE_MODE, vfStream->active_ ?
> >>> -					   IPU3PipeModeVideo :
> >>> -					   IPU3PipeModeStillCapture);
> >>> +	V4L2ControlList ctrls(imgu->imgu_->controls());
> >>> +	ctrls.set(V4L2_CID_IPU3_PIPE_MODE,
> >>> +		  static_cast<int32_t>(vfStream->active_ ? IPU3PipeModeVideo :
> >>> +				       IPU3PipeModeStillCapture));
> >>>  	ret = imgu->imgu_->setControls(&ctrls);
> >>>  	if (ret) {
> >>>  		LOG(IPU3, Error) << "Unable to set pipe_mode control";
> >>> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> >>> index 88f7fb9bc568..467bd9a9eaff 100644
> >>> --- a/src/libcamera/pipeline/uvcvideo.cpp
> >>> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> >>> @@ -228,31 +228,30 @@ void PipelineHandlerUVC::stop(Camera *camera)
> >>>
> >>>  int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)
> >>>  {
> >>> -	V4L2ControlList controls;
> >>> +	V4L2ControlList controls(data->video_->controls());
> >>>
> >>>  	for (auto it : request->controls()) {
> >>>  		const ControlId &id = *it.first;
> >>>  		ControlValue &value = it.second;
> >>>
> >>>  		if (id == controls::Brightness) {
> >>> -			controls.add(V4L2_CID_BRIGHTNESS, value.get<int32_t>());
> >>> +			controls.set(V4L2_CID_BRIGHTNESS, value);
> >>>  		} else if (id == controls::Contrast) {
> >>> -			controls.add(V4L2_CID_CONTRAST, value.get<int32_t>());
> >>> +			controls.set(V4L2_CID_CONTRAST, value);
> >>>  		} else if (id == controls::Saturation) {
> >>> -			controls.add(V4L2_CID_SATURATION, value.get<int32_t>());
> >>> +			controls.set(V4L2_CID_SATURATION, value);
> >>>  		} else if (id == controls::ManualExposure) {
> >>> -			controls.add(V4L2_CID_EXPOSURE_AUTO, 1);
> >>> -			controls.add(V4L2_CID_EXPOSURE_ABSOLUTE, value.get<int32_t>());
> >>> +			controls.set(V4L2_CID_EXPOSURE_AUTO, 1);
> >>> +			controls.set(V4L2_CID_EXPOSURE_ABSOLUTE, value);
> >>>  		} else if (id == controls::ManualGain) {
> >>> -			controls.add(V4L2_CID_GAIN, value.get<int32_t>());
> >>> +			controls.set(V4L2_CID_GAIN, value);
> >>>  		}
> >>>  	}
> >>>
> >>> -	for (const V4L2Control &ctrl : controls)
> >>> +	for (const auto &ctrl : controls)
> >>>  		LOG(UVC, Debug)
> >>> -			<< "Setting control 0x"
> >>> -			<< std::hex << std::setw(8) << ctrl.id() << std::dec
> >>> -			<< " to " << ctrl.value().toString();
> >>> +			<< "Setting control " << ctrl.first->name()
> >>> +			<< " to " << ctrl.second.toString();
> >>>
> >>>  	int ret = data->video_->setControls(&controls);
> >>>  	if (ret) {
> >>> @@ -338,11 +337,10 @@ int UVCCameraData::init(MediaEntity *entity)
> >>>  	/* Initialise the supported controls. */
> >>>  	const V4L2ControlInfoMap &controls = video_->controls();
> >>>  	for (const auto &ctrl : controls) {
> >>> -		unsigned int v4l2Id = ctrl.first;
> >>>  		const V4L2ControlInfo &info = ctrl.second;
> >>>  		const ControlId *id;
> >>>
> >>> -		switch (v4l2Id) {
> >>> +		switch (info.id().id()) {
> >>>  		case V4L2_CID_BRIGHTNESS:
> >>>  			id = &controls::Brightness;
> >>>  			break;
> >>> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> >>> index 26477dccbb8f..600a1154c75e 100644
> >>> --- a/src/libcamera/pipeline/vimc.cpp
> >>> +++ b/src/libcamera/pipeline/vimc.cpp
> >>> @@ -281,26 +281,24 @@ void PipelineHandlerVimc::stop(Camera *camera)
> >>>
> >>>  int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)
> >>>  {
> >>> -	V4L2ControlList controls;
> >>> +	V4L2ControlList controls(data->sensor_->controls());
> >>>
> >>>  	for (auto it : request->controls()) {
> >>>  		const ControlId &id = *it.first;
> >>>  		ControlValue &value = it.second;
> >>>
> >>> -		if (id == controls::Brightness) {
> >>> -			controls.add(V4L2_CID_BRIGHTNESS, value.get<int32_t>());
> >>> -		} else if (id == controls::Contrast) {
> >>> -			controls.add(V4L2_CID_CONTRAST, value.get<int32_t>());
> >>> -		} else if (id == controls::Saturation) {
> >>> -			controls.add(V4L2_CID_SATURATION, value.get<int32_t>());
> >>> -		}
> >>> +		if (id == controls::Brightness)
> >>> +			controls.set(V4L2_CID_BRIGHTNESS, value);
> >>> +		else if (id == controls::Contrast)
> >>> +			controls.set(V4L2_CID_CONTRAST, value);
> >>> +		else if (id == controls::Saturation)
> >>> +			controls.set(V4L2_CID_SATURATION, value);
> >>>  	}
> >>>
> >>> -	for (const V4L2Control &ctrl : controls)
> >>> +	for (const auto &ctrl : controls)
> >>>  		LOG(VIMC, Debug)
> >>> -			<< "Setting control 0x"
> >>> -			<< std::hex << std::setw(8) << ctrl.id() << std::dec
> >>> -			<< " to " << ctrl.value().toString();
> >>> +			<< "Setting control " << ctrl.first->name()
> >>> +			<< " to " << ctrl.second.toString();
> >>>
> >>>  	int ret = data->sensor_->setControls(&controls);
> >>>  	if (ret) {
> >>> @@ -417,11 +415,10 @@ int VimcCameraData::init(MediaDevice *media)
> >>>  	/* Initialise the supported controls. */
> >>>  	const V4L2ControlInfoMap &controls = sensor_->controls();
> >>>  	for (const auto &ctrl : controls) {
> >>> -		unsigned int v4l2Id = ctrl.first;
> >>>  		const V4L2ControlInfo &info = ctrl.second;
> >>>  		const ControlId *id;
> >>>
> >>> -		switch (v4l2Id) {
> >>> +		switch (info.id().id()) {
> >>>  		case V4L2_CID_BRIGHTNESS:
> >>>  			id = &controls::Brightness;
> >>>  			break;
> >>> diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> >>> index a630a2583d33..98b2b3fe9d0a 100644
> >>> --- a/src/libcamera/v4l2_controls.cpp
> >>> +++ b/src/libcamera/v4l2_controls.cpp
> >>> @@ -167,191 +167,83 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
> >>>   * \brief A map of control ID to V4L2ControlInfo
> >>>   */
> >>>
> >>> -/**
> >>> - * \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 modified by a control read or write operation.
> >>> - *
> >>> - * The write and read controls the V4L2Control class instances are not meant
> >>> - * to be directly used but are instead intended to be grouped in
> >>> - * V4L2ControlList instances, which are then passed as parameters to
> >>> - * V4L2Device::setControls() and V4L2Device::getControls() operations.
> >>> - */
> >>> -
> >>> -/**
> >>> - * \fn V4L2Control::V4L2Control
> >>> - * \brief Construct a V4L2 control with \a id and \a value
> >>> - * \param id The V4L2 control ID
> >>> - * \param value The control value
> >>> - */
> >>> -
> >>> -/**
> >>> - * \fn V4L2Control::value() const
> >>> - * \brief Retrieve the value of the control
> >>> - *
> >>> - * This method is a const version of V4L2Control::value(), returning a const
> >>> - * reference to the value.
> >>> - *
> >>> - * \return The V4L2 control value
> >>> - */
> >>> -
> >>> -/**
> >>> - * \fn V4L2Control::value()
> >>> - * \brief Retrieve the value of the control
> >>> - *
> >>> - * This method returns the cached control value, initially set by
> >>> - * V4L2ControlList::add() and then updated when the controls are read or
> >>> - * written with V4L2Device::getControls() and V4L2Device::setControls().
> >>> - *
> >>> - * \return The V4L2 control value
> >>> - */
> >>> -
> >>> -/**
> >>> - * \fn V4L2Control::id()
> >>> - * \brief Retrieve the control ID this instance refers to
> >>> - * \return The V4L2Control ID
> >>> - */
> >>> -
> >>>  /**
> >>>   * \class V4L2ControlList
> >>> - * \brief Container of V4L2Control instances
> >>> + * \brief A list of controls for a V4L2 device
> >>>   *
> >>> - * The V4L2ControlList 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.
> >>> + * This class specialises the ControList class for use with V4L2 devices. It
> >>> + * offers a convenience API to access controls by numerical ID instead of
> >>> + * ControlId.
> >>>   *
> >>> - * In order to set and get controls, user of the libcamera V4L2 control
> >>> - * framework should operate on instances of the V4L2ControlList class, and use
> >>> - * them as argument for the V4L2Device::setControls() and
> >>> - * V4L2Device::getControls() operations, which write and read a list of
> >>> - * controls to or from a V4L2 device (a video device or a subdevice).
> >>
> >> I would not remove this part, as it explains how to use the
> >> V4L2ControlList, and it still applies today even if those methods
> >> accepts a ControlList *
> >
> > Doesn't it belong to V4L2Device though ?
> 
> Possibly, but it also seems to me that it explains how to use a
> V4L2ControlList. Up to you..

I thought about adding a reference to setControls() and getControls(),
but then realised that those two methods take a ControlList, not a
V4L2ControlList. How about the following ?

 * V4L2ControlList allows easy construction of a ControlList containing V4L2
 * controls for a device. It can be used to construct the list of controls
 * passed to the V4L2Device::getControls() and V4L2Device::setControls()
 * methods. The class should however not be used in place of ControlList in
 * APIs unless strictly required.

> >>> - *
> >>> - * Controls are added to a V4L2ControlList instance with the add() method, with
> >>> - * or without a value.
> >>> - *
> >>> - * To write controls to a device, the controls of interest shall be added with
> >>> - * an initial value by calling V4L2ControlList::add(unsigned int id, int64_t
> >>> - * value) to prepare for a write operation. Once the values of all controls of
> >>> - * interest have been added, the V4L2ControlList 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
> >>> - * V4L2ControlList::add(unsigned int id) to prepare for a read operation. The
> >>> - * V4L2ControlList instance is then passed to V4L2Device::getControls(), which
> >>> - * reads the controls from the device and updates the values stored in
> >>> - * V4L2ControlList.
> >>> - *
> >>> - * V4L2ControlList instances can be reset to remove all controls they contain
> >>> - * and prepare to be re-used for a new control write/read sequence.
> >>> - */
> >>> -
> >>> -/**
> >>> - * \typedef V4L2ControlList::iterator
> >>> - * \brief Iterator on the V4L2 controls contained in the instance
> >>> - */
> >>> -
> >>> -/**
> >>> - * \typedef V4L2ControlList::const_iterator
> >>> - * \brief Const iterator on the V4L2 controls contained in the instance
> >>> + * V4L2ControlList should as much as possible not be used in place of
> >>> + * ControlList in APIs, and be used instead solely for the purpose of easily
> >>> + * constructing a ControlList of V4L2 controls for a device.
> >>>   */
> >>>
> >>>  /**
> >>> - * \fn iterator V4L2ControlList::begin()
> >>> - * \brief Retrieve an iterator to the first V4L2Control in the instance
> >>> - * \return An iterator to the first V4L2 control
> >>> + * \brief Construct a V4L2ControlList associated with a V4L2 device
> >>> + * \param[in] controls The V4L2 device control info map
> >>
> >> The V4L2 device provided control info map ?
> >
> > I don't think we need "provided", do we ?
> 
> Not sure, if it sounds well for you let's keep it this way
> 
> >>>   */
> >>> -
> >>> -/**
> >>> - * \fn const_iterator V4L2ControlList::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 V4L2ControlList::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 V4L2ControlList::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 V4L2ControlList::empty()
> >>> - * \brief Verify if the instance does not contain any control
> >>> - * \return True if the instance does not contain any control, false otherwise
> >>> - */
> >>> -
> >>> -/**
> >>> - * \fn V4L2ControlList::size()
> >>> - * \brief Retrieve the number on controls in the instance
> >>> - * \return The number of V4L2Control stored in the instance
> >>> - */
> >>> -
> >>> -/**
> >>> - * \fn V4L2ControlList::clear()
> >>> - * \brief Remove all controls in the instance
> >>> - */
> >>> -
> >>> -/**
> >>> - * \brief Add control with \a id and optional \a value to the instance
> >>> - * \param id The V4L2 control ID (V4L2_CID_*)
> >>> - * \param value The V4L2 control value
> >>> - *
> >>> - * This method adds a new V4L2 control to the V4L2ControlList. The newly
> >>> - * inserted control shall not already be present in the control lists, otherwise
> >>> - * this method, and further use of the control list lead to undefined behaviour.
> >>> - */
> >>> -void V4L2ControlList::add(unsigned int id, int64_t value)
> >>> +V4L2ControlList::V4L2ControlList(const V4L2ControlInfoMap &controls)
> >>> +	: ControlList(nullptr), controls_(controls)
> >>>  {
> >>> -	controls_.emplace_back(id, value);
> >>>  }
> >>>
> >>>  /**
> >>> - * \brief Retrieve the control at \a index
> >>> - * \param[in] index The control index
> >>> - *
> >>> - * Controls are stored in a V4L2ControlList in order of insertion and this
> >>> - * method retrieves the control at \a index.
> >>> + * \brief Check if the list contains a control with the specified \a id
> >>> + * \param[in] id The control ID
> >>>   *
> >>> - * \return A pointer to the V4L2Control at \a index or nullptr if the
> >>> - * index is larger than the number of controls
> >>> + * \return True if the list contains a matching control, false otherwise
> >>>   */
> >>> -V4L2Control *V4L2ControlList::getByIndex(unsigned int index)
> >>> +bool V4L2ControlList::contains(unsigned int id) const
> >>>  {
> >>> -	if (index >= controls_.size())
> >>> -		return nullptr;
> >>> +	auto ctrl = controls_.find(id);
> >>> +	if (ctrl == controls_.end())
> >>> +		return false;
> >>>
> >>> -	return &controls_[index];
> >>> +	return ControlList::contains(ctrl->second.id());
> >>
> >> Ah wait, we have
> >> ControlList::controls_ and
> >> V4L2ControlList::controls_
> >>
> >> is this intended ?
> >
> > Yes. I could rename the latter if you think it would be better. What
> > name would you prefer ?
> 
> I'm a bit bothered by having two different things with the same name,
> even if their scope is different... I would name the latter
> controlInfoMap_ even if it's a bit longer

How about infoMap_ or even just info_ ?

> >>>  }
> >>>
> >>>  /**
> >>> - * \brief Retrieve the control with \a id
> >>> - * \param[in] id The V4L2 control ID (V4L2_CID_xx)
> >>> - * \return A pointer to the V4L2Control with \a id or nullptr if the
> >>> - * control ID is not part of this instance.
> >>> + * \brief Get the value of control \a id
> >>> + * \param[in] id The control ID
> >>> + *
> >>> + * The behaviour is undefined if the control \a id is not present in the list.
> >>> + * Use v4L2ControlList::contains() to test for the presence of a control in the
> >>
> >> s/v4L2/V4L2
> >
> > Good catch, fixed.
> >
> >>> + * list before retrieving its value.
> >>> + *
> >>> + * \return The control value
> >>>   */
> >>> -V4L2Control *V4L2ControlList::operator[](unsigned int id)
> >>> +const ControlValue &V4L2ControlList::get(unsigned int id) const
> >>>  {
> >>> -	for (V4L2Control &ctrl : controls_) {
> >>> -		if (ctrl.id() == id)
> >>> -			return &ctrl;
> >>> +	auto ctrl = controls_.find(id);
> >>> +	if (ctrl == controls_.end()) {
> >>> +		static ControlValue zero;
> >>> +		return zero;
> >>>  	}
> >>>
> >>> -	return nullptr;
> >>> +	return ControlList::get(ctrl->second.id());
> >>> +}
> >>> +
> >>> +/**
> >>> + * \brief Set the value of control \a id to \a value
> >>> + * \param[in] id The control ID
> >>> + * \param[in] value The control value
> >>> + *
> >>> + * This method sets the value of a control in the control list. If the control
> >>> + * is already present in the list, its value is updated, otherwise it is added
> >>> + * to the list.
> >>> + *
> >>> + * The behaviour is undefined if the control \a ctrl is not supported by the
> >>> + * V4L2 device that the list refers to.
> >>> + */
> >>
> >> It does not seems undefined, but simply ignored..
> >
> > As discussed before, undefined doesn't mean that nobody can have a clue
> > on what happens. It means the API doesn't define the behaviour, and thus
> > that callers should really not rely on any specific behaviour, even if
> > the current implementation happens to work for them. The behaviour is
> > thus undefined.
> 
> ok then (but still, the operations has simply no effect in this case)

Your description of the current implementation is correct, but it should
*not* be part of the documentation. That's the whole point of defining
it as undefined behaviour.

> >>> +void V4L2ControlList::set(unsigned int id, const ControlValue &value)
> >>> +{
> >>> +	auto ctrl = controls_.find(id);
> >>> +	if (ctrl == controls_.end())
> >>> +		return;
> >>> +
> >>> +	ControlList::set(ctrl->second.id(), value);
> >>>  }
> >>>
> >>>  } /* namespace libcamera */
> >>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> >>> index 466c3d41f6e3..50616571dcef 100644
> >>> --- a/src/libcamera/v4l2_device.cpp
> >>> +++ b/src/libcamera/v4l2_device.cpp
> >>> @@ -12,6 +12,8 @@
> >>>  #include <sys/ioctl.h>
> >>>  #include <unistd.h>
> >>>
> >>> +#include <libcamera/controls.h>
> >>> +
> >>>  #include "log.h"
> >>>  #include "v4l2_controls.h"
> >>>
> >>> @@ -163,7 +165,7 @@ void V4L2Device::close()
> >>>   * \retval -EINVAL One of the control is not supported or not accessible
> >>>   * \retval i The index of the control that failed
> >>>   */
> >>> -int V4L2Device::getControls(V4L2ControlList *ctrls)
> >>> +int V4L2Device::getControls(ControlList *ctrls)
> >>>  {
> >>>  	unsigned int count = ctrls->size();
> >>>  	if (count == 0)
> >>> @@ -173,18 +175,21 @@ int V4L2Device::getControls(V4L2ControlList *ctrls)
> >>>  	struct v4l2_ext_control v4l2Ctrls[count];
> >>>  	memset(v4l2Ctrls, 0, sizeof(v4l2Ctrls));
> >>>
> >>> -	for (unsigned int i = 0; i < count; ++i) {
> >>> -		const V4L2Control *ctrl = ctrls->getByIndex(i);
> >>> -		const auto iter = controls_.find(ctrl->id());
> >>> +	unsigned int i = 0;
> >>> +	for (const auto &ctrl : *ctrls) {
> >>> +		const ControlId *id = ctrl.first;
> >>> +		const auto iter = controls_.find(id->id());
> >>>  		if (iter == controls_.end()) {
> >>>  			LOG(V4L2, Error)
> >>> -				<< "Control '" << ctrl->id() << "' not found";
> >>> +				<< "Control '" << id->name() << "' not found";
> >>>  			return -EINVAL;
> >>>  		}
> >>>
> >>>  		const V4L2ControlInfo *info = &iter->second;
> >>>  		controlInfo[i] = info;
> >>> -		v4l2Ctrls[i].id = ctrl->id();
> >>> +		v4l2Ctrls[i].id = id->id();
> >>> +
> >>> +		i++;
> >>>  	}
> >>>
> >>>  	struct v4l2_ext_controls v4l2ExtCtrls = {};
> >>> @@ -238,7 +243,7 @@ int V4L2Device::getControls(V4L2ControlList *ctrls)
> >>>   * \retval -EINVAL One of the control is not supported or not accessible
> >>>   * \retval i The index of the control that failed
> >>>   */
> >>> -int V4L2Device::setControls(V4L2ControlList *ctrls)
> >>> +int V4L2Device::setControls(ControlList *ctrls)
> >>>  {
> >>>  	unsigned int count = ctrls->size();
> >>>  	if (count == 0)
> >>> @@ -248,32 +253,36 @@ int V4L2Device::setControls(V4L2ControlList *ctrls)
> >>>  	struct v4l2_ext_control v4l2Ctrls[count];
> >>>  	memset(v4l2Ctrls, 0, sizeof(v4l2Ctrls));
> >>>
> >>> -	for (unsigned int i = 0; i < count; ++i) {
> >>> -		const V4L2Control *ctrl = ctrls->getByIndex(i);
> >>> -		const auto iter = controls_.find(ctrl->id());
> >>> +	unsigned int i = 0;
> >>> +	for (const auto &ctrl : *ctrls) {
> >>> +		const ControlId *id = ctrl.first;
> >>> +		const auto iter = controls_.find(id->id());
> >>>  		if (iter == controls_.end()) {
> >>>  			LOG(V4L2, Error)
> >>> -				<< "Control '" << ctrl->id() << "' not found";
> >>> +				<< "Control '" << id->name() << "' not found";
> >>>  			return -EINVAL;
> >>>  		}
> >>>
> >>>  		const V4L2ControlInfo *info = &iter->second;
> >>>  		controlInfo[i] = info;
> >>> -		v4l2Ctrls[i].id = ctrl->id();
> >>> +		v4l2Ctrls[i].id = id->id();
> >>>
> >>>  		/* Set the v4l2_ext_control value for the write operation. */
> >>> +		const ControlValue &value = ctrl.second;
> >>>  		switch (info->type()) {
> >>>  		case V4L2_CTRL_TYPE_INTEGER64:
> >>> -			v4l2Ctrls[i].value64 = ctrl->value().get<int64_t>();
> >>> +			v4l2Ctrls[i].value64 = value.get<int64_t>();
> >>>  			break;
> >>>  		default:
> >>>  			/*
> >>>  			 * \todo To be changed when support for string and
> >>>  			 * compound controls will be added.
> >>>  			 */
> >>> -			v4l2Ctrls[i].value = ctrl->value().get<int32_t>();
> >>> +			v4l2Ctrls[i].value64 = value.get<int32_t>();
> >>
> >> value64 or value ?
> >
> > value. Good catch.
> >
> >>>  			break;
> >>>  		}
> >>> +
> >>> +		i++;
> >>>  	}
> >>>
> >>>  	struct v4l2_ext_controls v4l2ExtCtrls = {};
> >>> @@ -382,28 +391,31 @@ void V4L2Device::listControls()
> >>>   * \param[in] v4l2Ctrls List of V4L2 extended controls as returned by the driver
> >>>   * \param[in] count The number of controls to update
> >>>   */
> >>> -void V4L2Device::updateControls(V4L2ControlList *ctrls,
> >>> +void V4L2Device::updateControls(ControlList *ctrls,
> >>>  				const V4L2ControlInfo **controlInfo,
> >>>  				const struct v4l2_ext_control *v4l2Ctrls,
> >>>  				unsigned int count)
> >>>  {
> >>> -	for (unsigned int i = 0; i < count; ++i) {
> >>> +	unsigned int i = 0;
> >>> +	for (auto &ctrl : *ctrls) {
> >>>  		const struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];
> >>>  		const V4L2ControlInfo *info = controlInfo[i];
> >>> -		V4L2Control *ctrl = ctrls->getByIndex(i);
> >>> +		ControlValue &value = ctrl.second;
> >>
> >> you should break when i == count
> >
> > Good catch too !
> >
> >>>
> >>>  		switch (info->type()) {
> >>>  		case V4L2_CTRL_TYPE_INTEGER64:
> >>> -			ctrl->value().set<int64_t>(v4l2Ctrl->value64);
> >>> +			value.set<int64_t>(v4l2Ctrl->value64);
> >>>  			break;
> >>>  		default:
> >>>  			/*
> >>>  			 * \todo To be changed when support for string and
> >>>  			 * compound controls will be added.
> >>>  			 */
> >>> -			ctrl->value().set<int32_t>(v4l2Ctrl->value);
> >>> +			value.set<int32_t>(v4l2Ctrl->value);
> >>>  			break;
> >>>  		}
> >>> +
> >>> +		i++;
> >>>  	}
> >>>  }
> >>>

Hi Laurent

On Wed, Oct 09, 2019 at 02:54:31PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Wed, Oct 09, 2019 at 11:56:55AM +0200, Jacopo Mondi wrote:
> > On Wed, Oct 09, 2019 at 12:38:54PM +0300, Laurent Pinchart wrote:
> > > On Wed, Oct 09, 2019 at 11:13:53AM +0200, Jacopo Mondi wrote:
> > >> Hi Laurent,
> > >>     just a note that this break compilation, but you know that already
> > >
> > > Isn't it patch 9/9 that breaks compilation ?
> >

[snip]

> > >>> - * In order to set and get controls, user of the libcamera V4L2 control
> > >>> - * framework should operate on instances of the V4L2ControlList class, and use
> > >>> - * them as argument for the V4L2Device::setControls() and
> > >>> - * V4L2Device::getControls() operations, which write and read a list of
> > >>> - * controls to or from a V4L2 device (a video device or a subdevice).
> > >>
> > >> I would not remove this part, as it explains how to use the
> > >> V4L2ControlList, and it still applies today even if those methods
> > >> accepts a ControlList *
> > >
> > > Doesn't it belong to V4L2Device though ?
> >
> > Possibly, but it also seems to me that it explains how to use a
> > V4L2ControlList. Up to you..
>
> I thought about adding a reference to setControls() and getControls(),
> but then realised that those two methods take a ControlList, not a
> V4L2ControlList. How about the following ?
>
>  * V4L2ControlList allows easy construction of a ControlList containing V4L2
>  * controls for a device. It can be used to construct the list of controls
>  * passed to the V4L2Device::getControls() and V4L2Device::setControls()
>  * methods. The class should however not be used in place of ControlList in
>  * APIs unless strictly required.

I like it!

>
[snip]

> > >>> -	return &controls_[index];
> > >>> +	return ControlList::contains(ctrl->second.id());
> > >>
> > >> Ah wait, we have
> > >> ControlList::controls_ and
> > >> V4L2ControlList::controls_
> > >>
> > >> is this intended ?
> > >
> > > Yes. I could rename the latter if you think it would be better. What
> > > name would you prefer ?
> >
> > I'm a bit bothered by having two different things with the same name,
> > even if their scope is different... I would name the latter
> > controlInfoMap_ even if it's a bit longer
>
> How about infoMap_ or even just info_ ?

You know, I would have kept 'control' somewhere in the name, but I'm
fine dropping it for something shorter like 'info_'.

Minor stuff though, do whatever you like the most!