[libcamera-devel] [PATCH 08/12] libcamera: v4l2_controls: Use the ControlValue class for data storage
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Sep 29 15:30:49 CEST 2019
Hi Jacopo,
On Sun, Sep 29, 2019 at 12:10:25PM +0200, Jacopo Mondi wrote:
> On Sat, Sep 28, 2019 at 06:22:34PM +0300, Laurent Pinchart wrote:
> > Use the ControlValue class to replace the manually crafted data storage
> > in V4L2Control. This will help sharing code when more data types will be
> > supported.
>
> Happy indeed to see that at third attempt we might be getting there
It's nice to share code, but I'd like to make it clear that my first
focus is a good API towards applications, my second focus a good API
towards pipeline handlers and IPAs, and internal code sharing is only my
third focus. As long as libcamera and V4L2 controls are similar enough
that code can be shared, I have no issue with that. If we figure out
later that differences are needed, I will choose separate
implementations over any regression in the public API for the purpose of
code sharing.
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > src/libcamera/include/v4l2_controls.h | 15 +++++++++------
> > src/libcamera/pipeline/uvcvideo.cpp | 2 +-
> > src/libcamera/pipeline/vimc.cpp | 2 +-
> > src/libcamera/v4l2_controls.cpp | 19 ++++++++++---------
> > src/libcamera/v4l2_device.cpp | 8 ++++----
> > 5 files changed, 25 insertions(+), 21 deletions(-)
> >
> > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h
> > index 10b726504951..f2b67c5d44e1 100644
> > --- a/src/libcamera/include/v4l2_controls.h
> > +++ b/src/libcamera/include/v4l2_controls.h
> > @@ -16,6 +16,8 @@
> > #include <linux/v4l2-controls.h>
> > #include <linux/videodev2.h>
> >
> > +#include <libcamera/controls.h>
> > +
> > namespace libcamera {
> >
> > class V4L2ControlInfo
> > @@ -46,17 +48,18 @@ using V4L2ControlInfoMap = std::map<unsigned int, V4L2ControlInfo>;
> > class V4L2Control
> > {
> > public:
> > - V4L2Control(unsigned int id, int value = 0)
> > - : id_(id), value_(value) {}
> > -
> > - int64_t value() const { return value_; }
> > - void setValue(int64_t value) { value_ = value; }
> > + V4L2Control(unsigned int id, const ControlValue &value = ControlValue())
> > + : id_(id), value_(value)
>
> The default copy constructor is fine so far, as it copies the union
> content. With compound controls this will fall short I fear.
Yes, we will then need an explicit copy constructor.
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
> > + {
> > + }
> >
> > unsigned int id() const { return id_; }
> > + const ControlValue &value() const { return value_; }
> > + ControlValue &value() { return value_; }
> >
> > private:
> > unsigned int id_;
> > - int64_t value_;
> > + ControlValue value_;
> > };
> >
> > class V4L2ControlList
> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> > index 3635ea83a7b1..a80f8a43a116 100644
> > --- a/src/libcamera/pipeline/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo.cpp
> > @@ -252,7 +252,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)
> > LOG(UVC, Debug)
> > << "Setting control 0x"
> > << std::hex << std::setw(8) << ctrl.id() << std::dec
> > - << " to " << ctrl.value();
> > + << " to " << ctrl.value().toString();
> >
> > int ret = data->video_->setControls(&controls);
> > if (ret) {
> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> > index d9dd031e173c..5d6909f58cf9 100644
> > --- a/src/libcamera/pipeline/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc.cpp
> > @@ -300,7 +300,7 @@ int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)
> > LOG(VIMC, Debug)
> > << "Setting control 0x"
> > << std::hex << std::setw(8) << ctrl.id() << std::dec
> > - << " to " << ctrl.value();
> > + << " to " << ctrl.value().toString();
> >
> > int ret = data->sensor_->setControls(&controls);
> > if (ret) {
> > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> > index 84258d9954d0..64f0555fff7c 100644
> > --- a/src/libcamera/v4l2_controls.cpp
> > +++ b/src/libcamera/v4l2_controls.cpp
> > @@ -143,6 +143,16 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
> > * \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
> > @@ -154,15 +164,6 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
> > * \return The V4L2 control value
> > */
> >
> > -/**
> > - * \fn V4L2Control::setValue()
> > - * \brief Set the value of the control
> > - * \param value The new V4L2 control value
> > - *
> > - * This method stores the control value, which will be applied to the
> > - * device when calling V4L2Device::setControls().
> > - */
> > -
> > /**
> > * \fn V4L2Control::id()
> > * \brief Retrieve the control ID this instance refers to
> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > index 349bf2d29704..fd4b9c6d5c62 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -264,14 +264,14 @@ int V4L2Device::setControls(V4L2ControlList *ctrls)
> > /* Set the v4l2_ext_control value for the write operation. */
> > switch (info->type()) {
> > case V4L2_CTRL_TYPE_INTEGER64:
> > - v4l2Ctrls[i].value64 = ctrl->value();
> > + v4l2Ctrls[i].value64 = ctrl->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();
> > + v4l2Ctrls[i].value = ctrl->value().get<int32_t>();
> > break;
> > }
> > }
> > @@ -393,14 +393,14 @@ void V4L2Device::updateControls(V4L2ControlList *ctrls,
> >
> > switch (info->type()) {
> > case V4L2_CTRL_TYPE_INTEGER64:
> > - ctrl->setValue(v4l2Ctrl->value64);
> > + ctrl->value().set<int64_t>(v4l2Ctrl->value64);
> > break;
> > default:
> > /*
> > * \todo To be changed when support for string and
> > * compound controls will be added.
> > */
> > - ctrl->setValue(v4l2Ctrl->value);
> > + ctrl->value().set<int32_t>(v4l2Ctrl->value);
> > break;
> > }
> > }
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list