[libcamera-devel] [PATCH v2 13/14] libcamera: v4l2_device: Replace V4L2ControlList with ControlList
Niklas Söderlund
niklas.soderlund at ragnatech.se
Sun Oct 13 18:28:12 CEST 2019
Hi Laurent,
Thanks for your work.
On 2019-10-12 21:44:06 +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 makes the IPA API more complex, as it
> needs to explicitly transport both types of lists. This will become even
> more painful when implementing serialisation and deserialisation.
>
> To simplify the IPA API and ease the implementation of serialisation and
> deserialisation, replace usage of V4L2ControlList with ControlList in
> the V4L2Device (and thus CameraSensor) API. The V4L2ControlList class
> becomes a thin wrapper around ControlList that slightly simplifies the
> creation of control lists for V4L2 controls, and may be removed in the
> future.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> Changes since v1:
>
> - Fix typo in comment
> - Use the v4l2_ext_control::value field instead of value64 where
> appropriate
> - Break loop in updateControls() when reaching count
> - Rename controls_ to info_ (and related variables)
> - Add reference to V4L2Device::[gs]etControls() to V4L2ControlList class
> documentation
> - Update the rkisp1 IPA
> - Add V4L2ControlInfoMap class
> - Remove ID-based accessors from V4L2ControlList as they are now present
> in the base ControlList class
> - Don't include v4l2_controls.h in camera_sensor.h
> ---
> src/ipa/rkisp1/rkisp1.cpp | 18 +--
> src/libcamera/camera_sensor.cpp | 4 +-
> src/libcamera/include/camera_sensor.h | 7 +-
> src/libcamera/include/v4l2_controls.h | 44 ++----
> src/libcamera/include/v4l2_device.h | 6 +-
> src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +-
> src/libcamera/pipeline/uvcvideo.cpp | 24 ++--
> src/libcamera/pipeline/vimc.cpp | 25 ++--
> src/libcamera/v4l2_controls.cpp | 196 +++-----------------------
> src/libcamera/v4l2_device.cpp | 51 ++++---
> test/v4l2_videodevice/controls.cpp | 32 ++---
> 11 files changed, 124 insertions(+), 292 deletions(-)
>
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index b0d23dd154be..69ced840585f 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -49,6 +49,8 @@ private:
>
> std::map<unsigned int, BufferMemory> bufferInfo_;
>
> + V4L2ControlInfoMap ctrls_;
> +
> /* Camera sensor controls. */
> bool autoExposure_;
> uint32_t exposure_;
> @@ -65,16 +67,16 @@ void IPARkISP1::configure(const std::map<unsigned int, IPAStream> &streamConfig,
> if (entityControls.empty())
> return;
>
> - const V4L2ControlInfoMap &ctrls = entityControls.at(0);
> + ctrls_ = entityControls.at(0);
>
> - const auto itExp = ctrls.find(V4L2_CID_EXPOSURE);
> - if (itExp == ctrls.end()) {
> + const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);
> + if (itExp == ctrls_.end()) {
> LOG(IPARkISP1, Error) << "Can't find exposure control";
> return;
> }
>
> - const auto itGain = ctrls.find(V4L2_CID_ANALOGUE_GAIN);
> - if (itGain == ctrls.end()) {
> + const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);
> + if (itGain == ctrls_.end()) {
> LOG(IPARkISP1, Error) << "Can't find gain control";
> return;
> }
> @@ -210,9 +212,9 @@ void IPARkISP1::setControls(unsigned int frame)
> IPAOperationData op;
> op.operation = RKISP1_IPA_ACTION_V4L2_SET;
>
> - V4L2ControlList ctrls;
> - ctrls.add(V4L2_CID_EXPOSURE, exposure_);
> - ctrls.add(V4L2_CID_ANALOGUE_GAIN, gain_);
> + V4L2ControlList ctrls(ctrls_);
> + ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));
> + ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_));
> op.v4l2controls.push_back(ctrls);
>
> queueFrameAction.emit(frame, op);
> 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..f426e29b84bf 100644
> --- a/src/libcamera/include/camera_sensor.h
> +++ b/src/libcamera/include/camera_sensor.h
> @@ -13,11 +13,12 @@
> #include <libcamera/geometry.h>
>
> #include "log.h"
> -#include "v4l2_controls.h"
>
> namespace libcamera {
>
> +class ControlList;
> class MediaEntity;
> +class V4L2ControlInfoMap;
> class V4L2Subdevice;
>
> struct V4L2SubdeviceFormat;
> @@ -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 8990e755a385..89cc74485e6f 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>
> @@ -57,47 +56,20 @@ public:
> using std::map<unsigned int, V4L2ControlInfo>::size;
> using std::map<unsigned int, V4L2ControlInfo>::count;
> using std::map<unsigned int, V4L2ControlInfo>::find;
> +
> + const ControlIdMap &idmap() const { return idmap_; }
> +
> +private:
> + ControlIdMap idmap_;
> };
>
> -class V4L2Control
> +class V4L2ControlList : public ControlList
> {
> public:
> - V4L2Control(unsigned int id, const ControlValue &value = ControlValue())
> - : id_(id), value_(value)
> + V4L2ControlList(const V4L2ControlInfoMap &info)
> + : ControlList(info.idmap())
> {
> }
> -
> - unsigned int id() const { return id_; }
> - const ControlValue &value() const { return value_; }
> - ControlValue &value() { return 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_;
> };
>
> } /* namespace libcamera */
> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> index 75a52c33d821..daa762d58d2b 100644
> --- a/src/libcamera/include/v4l2_device.h
> +++ b/src/libcamera/include/v4l2_device.h
> @@ -25,8 +25,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 +43,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..547ad5ca4e55 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, static_cast<int32_t>(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 f1cfd0ed35cf..53d00360eb6e 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -279,26 +279,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) {
> @@ -415,11 +413,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 438e8d8bdb99..b4bb7e791491 100644
> --- a/src/libcamera/v4l2_controls.cpp
> +++ b/src/libcamera/v4l2_controls.cpp
> @@ -40,7 +40,7 @@
> * sizes.
> *
> * The libcamera V4L2 Controls framework operates on lists of controls, wrapped
> - * by the V4L2ControlList class, to match the V4L2 extended controls API. The
> + * by the ControlList class, to match the V4L2 extended controls API. The
> * interface to set and get control is implemented by the V4L2Device class, and
> * this file only provides the data type definitions.
> *
> @@ -172,194 +172,42 @@ V4L2ControlInfoMap &V4L2ControlInfoMap::operator=(std::map<unsigned int, V4L2Con
> {
> std::map<unsigned int, V4L2ControlInfo>::operator=(std::move(info));
>
> + idmap_.clear();
> + for (const auto &ctrl : *this)
> + idmap_[ctrl.first] = &ctrl.second.id();
> +
> return *this;
> }
>
> /**
> - * \class V4L2Control
> - * \brief A V4L2 control value
> + * \fn const ControlIdMap &V4L2ControlInfoMap::idmap() const
> + * \brief Retrieve the ControlId map
> *
> - * 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.
> + * Constructing ControlList instances for V4L2 controls requires a ControlIdMap
> + * for the V4L2 device that the control list targets. This helper method
> + * returns a suitable idmap for that purpose.
> *
> - * 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
> + * \return The ControlId map
> */
>
> /**
> * \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 create a ControlList from a V4L2ControlInfoMap.
> *
> - * 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).
> - *
> - * 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
> - */
> -
> -/**
> - * \fn iterator V4L2ControlList::begin()
> - * \brief Retrieve an iterator to the first V4L2Control in the instance
> - * \return An iterator to the first V4L2 control
> - */
> -
> -/**
> - * \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
> + * 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.
> */
>
> /**
> - * \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.
> + * \fn V4L2ControlList::V4L2ControlList(const V4L2ControlInfoMap &info)
> + * \brief Construct a V4L2ControlList associated with a V4L2 device
> + * \param[in] info The V4L2 device control info map
> */
> -void V4L2ControlList::add(unsigned int id, int64_t value)
> -{
> - 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.
> - *
> - * \return A pointer to the V4L2Control at \a index or nullptr if the
> - * index is larger than the number of controls
> - */
> -V4L2Control *V4L2ControlList::getByIndex(unsigned int index)
> -{
> - if (index >= controls_.size())
> - return nullptr;
> -
> - return &controls_[index];
> -}
> -
> -/**
> - * \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.
> - */
> -V4L2Control *V4L2ControlList::operator[](unsigned int id)
> -{
> - for (V4L2Control &ctrl : controls_) {
> - if (ctrl.id() == id)
> - return &ctrl;
> - }
> -
> - return nullptr;
> -}
>
> } /* namespace libcamera */
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 1f755f0f3ef6..b47ba448f354 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -163,7 +163,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 +173,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 +241,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 +251,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->id().type()) {
> case ControlTypeInteger64:
> - 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].value = value.get<int32_t>();
> break;
> }
> +
> + i++;
> }
>
> struct v4l2_ext_controls v4l2ExtCtrls = {};
> @@ -385,28 +392,34 @@ 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) {
> + if (i == count)
> + break;
> +
> const struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];
> const V4L2ControlInfo *info = controlInfo[i];
> - V4L2Control *ctrl = ctrls->getByIndex(i);
> + ControlValue &value = ctrl.second;
>
> switch (info->id().type()) {
> case ControlTypeInteger64:
> - 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++;
> }
> }
>
> diff --git a/test/v4l2_videodevice/controls.cpp b/test/v4l2_videodevice/controls.cpp
> index 4a7535245c00..783b695b8cbf 100644
> --- a/test/v4l2_videodevice/controls.cpp
> +++ b/test/v4l2_videodevice/controls.cpp
> @@ -44,10 +44,10 @@ protected:
> const V4L2ControlInfo &saturation = info.find(V4L2_CID_SATURATION)->second;
>
> /* Test getting controls. */
> - V4L2ControlList ctrls;
> - ctrls.add(V4L2_CID_BRIGHTNESS);
> - ctrls.add(V4L2_CID_CONTRAST);
> - ctrls.add(V4L2_CID_SATURATION);
> + V4L2ControlList ctrls(info);
> + ctrls.set(V4L2_CID_BRIGHTNESS, -1);
> + ctrls.set(V4L2_CID_CONTRAST, -1);
> + ctrls.set(V4L2_CID_SATURATION, -1);
>
> int ret = capture_->getControls(&ctrls);
> if (ret != 0) {
> @@ -55,17 +55,17 @@ protected:
> return TestFail;
> }
>
> - if (ctrls[V4L2_CID_BRIGHTNESS]->value().get<int32_t>() == -1 ||
> - ctrls[V4L2_CID_CONTRAST]->value().get<int32_t>() == -1 ||
> - ctrls[V4L2_CID_SATURATION]->value().get<int32_t>() == -1) {
> + if (ctrls.get(V4L2_CID_BRIGHTNESS).get<int32_t>() == -1 ||
> + ctrls.get(V4L2_CID_CONTRAST).get<int32_t>() == -1 ||
> + ctrls.get(V4L2_CID_SATURATION).get<int32_t>() == -1) {
> cerr << "Incorrect value for retrieved controls" << endl;
> return TestFail;
> }
>
> /* Test setting controls. */
> - ctrls[V4L2_CID_BRIGHTNESS]->value() = brightness.range().min();
> - ctrls[V4L2_CID_CONTRAST]->value() = contrast.range().max();
> - ctrls[V4L2_CID_SATURATION]->value() = saturation.range().min();
> + ctrls.set(V4L2_CID_BRIGHTNESS, brightness.range().min());
> + ctrls.set(V4L2_CID_CONTRAST, contrast.range().max());
> + ctrls.set(V4L2_CID_SATURATION, saturation.range().min());
>
> ret = capture_->setControls(&ctrls);
> if (ret != 0) {
> @@ -74,9 +74,9 @@ protected:
> }
>
> /* Test setting controls outside of range. */
> - ctrls[V4L2_CID_BRIGHTNESS]->value() = brightness.range().min().get<int32_t>() - 1;
> - ctrls[V4L2_CID_CONTRAST]->value() = contrast.range().max().get<int32_t>() + 1;
> - ctrls[V4L2_CID_SATURATION]->value() = saturation.range().min().get<int32_t>() + 1;
> + ctrls.set(V4L2_CID_BRIGHTNESS, brightness.range().min().get<int32_t>() - 1);
> + ctrls.set(V4L2_CID_CONTRAST, contrast.range().max().get<int32_t>() + 1);
> + ctrls.set(V4L2_CID_SATURATION, saturation.range().min().get<int32_t>() + 1);
>
> ret = capture_->setControls(&ctrls);
> if (ret != 0) {
> @@ -84,9 +84,9 @@ protected:
> return TestFail;
> }
>
> - if (ctrls[V4L2_CID_BRIGHTNESS]->value() != brightness.range().min() ||
> - ctrls[V4L2_CID_CONTRAST]->value() != brightness.range().max() ||
> - ctrls[V4L2_CID_SATURATION]->value() != saturation.range().min().get<int32_t>() + 1) {
> + if (ctrls.get(V4L2_CID_BRIGHTNESS) != brightness.range().min() ||
> + ctrls.get(V4L2_CID_CONTRAST) != brightness.range().max() ||
> + ctrls.get(V4L2_CID_SATURATION) != saturation.range().min().get<int32_t>() + 1) {
> cerr << "Controls not updated when set" << endl;
> return TestFail;
> }
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list