[libcamera-devel] [PATCH 7/9] libcamera: v4l2_device: Replace V4L2ControlList with ControlList
Jacopo Mondi
jacopo at jmondi.org
Wed Oct 9 11:56:55 CEST 2019
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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20191009/f2c8c76f/attachment-0001.sig>
More information about the libcamera-devel
mailing list