[libcamera-devel] [PATCH 3/5] libcamera: v4l2_controls: Use DataValue and DataInfo
Jacopo Mondi
jacopo at jmondi.org
Mon Sep 23 13:07:53 CEST 2019
Hi kieran,
On Fri, Sep 20, 2019 at 12:15:07PM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 18/09/2019 11:34, Jacopo Mondi wrote:
> > Use DataValue and DataInfo in the V4L2 control handling classes to
> > maximize code reuse with the libcamera controls classes.
>
> This makes me happy :D
>
> This patch makes a subtle change to the way "->type()" is used.
>
> Could that be noted in the commit message please?
> (Just so that it's clear)
>
> Something like:
>
> "The control type is now represented by the DataType from the DataValue
> rather than the type_ previously stored in the V4L2ControlInfo"
>
Yes, good point. This needs to be tracked as it might require a
certain careful handling when we'll add support for compound types.
> Otherwise, I think this all looks fine.
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
Thanks
j
>
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > src/libcamera/include/v4l2_controls.h | 22 ++++--------
> > src/libcamera/include/v4l2_device.h | 1 -
> > src/libcamera/v4l2_controls.cpp | 49 +++++++--------------------
> > src/libcamera/v4l2_device.cpp | 25 ++++++--------
> > 4 files changed, 30 insertions(+), 67 deletions(-)
> >
> > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h
> > index 10b726504951..27b855f3407f 100644
> > --- a/src/libcamera/include/v4l2_controls.h
> > +++ b/src/libcamera/include/v4l2_controls.h
> > @@ -16,29 +16,18 @@
> > #include <linux/v4l2-controls.h>
> > #include <linux/videodev2.h>
> >
> > +#include <libcamera/data_value.h>
> > +
> > namespace libcamera {
> >
> > -class V4L2ControlInfo
> > +class V4L2ControlInfo : public DataInfo
> > {
> > public:
> > V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl);
> > -
> > unsigned int id() const { return id_; }
> > - unsigned int type() const { return type_; }
> > - size_t size() const { return size_; }
> > - const std::string &name() const { return name_; }
> > -
> > - int64_t min() const { return min_; }
> > - int64_t max() const { return max_; }
> >
> > private:
> > unsigned int id_;
> > - unsigned int type_;
> > - size_t size_;
> > - std::string name_;
> > -
> > - int64_t min_;
> > - int64_t max_;
> > };
> >
> > using V4L2ControlInfoMap = std::map<unsigned int, V4L2ControlInfo>;
> > @@ -49,14 +38,15 @@ public:
> > V4L2Control(unsigned int id, int value = 0)
> > : id_(id), value_(value) {}
> >
> > - int64_t value() const { return value_; }
> > + DataType type() const { return value_.type(); }
>
> Do you need to expose the type? Aren't they all int64_t here?
>
> Ah - ok I see below we switch from examining the V4L2ControlInfo->type()
> to examining the V4L2Control->type().
>
> So this is fine.
>
>
> > + int64_t value() const { return value_.getInt64(); }
> > void setValue(int64_t value) { value_ = value; }
> >
> > unsigned int id() const { return id_; }
> >
> > private:
> > unsigned int id_;
> > - int64_t value_;
> > + DataValue value_;
> > };
> >
> > class V4L2ControlList
> > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> > index 75a52c33d821..3144adc26e36 100644
> > --- a/src/libcamera/include/v4l2_device.h
> > +++ b/src/libcamera/include/v4l2_device.h
> > @@ -44,7 +44,6 @@ protected:
> > private:
> > void listControls();
> > void updateControls(V4L2ControlList *ctrls,
> > - const V4L2ControlInfo **controlInfo,
> > const struct v4l2_ext_control *v4l2Ctrls,
> > unsigned int count);
> >
> > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> > index 84258d9954d0..9bc4929cbd76 100644
> > --- a/src/libcamera/v4l2_controls.cpp
> > +++ b/src/libcamera/v4l2_controls.cpp
> > @@ -69,13 +69,10 @@ namespace libcamera {
> > * \param ctrl The struct v4l2_query_ext_ctrl as returned by the kernel
> > */
> > V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
> > + : DataInfo(DataValue(static_cast<int64_t>(ctrl.minimum)),
> > + DataValue(static_cast<int64_t>(ctrl.maximum))),
> > + id_(ctrl.id)
> > {
> > - id_ = ctrl.id;
> > - type_ = ctrl.type;
> > - name_ = static_cast<const char *>(ctrl.name);
> > - size_ = ctrl.elem_size * ctrl.elems;
> > - min_ = ctrl.minimum;
> > - max_ = ctrl.maximum;
> > }
> >
> > /**
> > @@ -84,36 +81,6 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
> > * \return The V4L2 control ID
> > */
> >
> > -/**
> > - * \fn V4L2ControlInfo::type()
> > - * \brief Retrieve the control type as defined by V4L2_CTRL_TYPE_*
>
> Oh - ok - so the types are subtly different now. But I think it's on a
> path towards good reuse of the DataValue, DataInfo classes.
>
>
> > - * \return The V4L2 control type
> > - */
> > -
> > -/**
> > - * \fn V4L2ControlInfo::size()
> > - * \brief Retrieve the control value data size (in bytes)
> > - * \return The V4L2 control value data size
> > - */
> > -
> > -/**
> > - * \fn V4L2ControlInfo::name()
> > - * \brief Retrieve the control user readable name
> > - * \return The V4L2 control user readable name
> > - */
> > -
> > -/**
> > - * \fn V4L2ControlInfo::min()
> > - * \brief Retrieve the control minimum value
> > - * \return The V4L2 control minimum value
> > - */
> > -
> > -/**
> > - * \fn V4L2ControlInfo::max()
> > - * \brief Retrieve the control maximum value
> > - * \return The V4L2 control maximum value
> > - */
> > -
> > /**
> > * \typedef V4L2ControlInfoMap
> > * \brief A map of control ID to V4L2ControlInfo
> > @@ -134,6 +101,10 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
> > * 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.
> > + *
> > + * \todo Currently all V4L2Controls are integers. For the sake of keeping the
> > + * implementation as simpler as possible treat all values as int64. The value()
> > + * and setValue() operations use that single data type for now.
> > */
> >
> > /**
> > @@ -143,6 +114,12 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
> > * \param value The control value
> > */
> >
> > +/**
> > + * \fn V4L2Control::type()
> > + * \brief Retrieve the type of the control
> > + * \return The control type
> > + */
> > +
> > /**
> > * \fn V4L2Control::value()
> > * \brief Retrieve the value of the control
> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > index 349bf2d29704..2b7e3b1993ca 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -210,7 +210,7 @@ int V4L2Device::getControls(V4L2ControlList *ctrls)
> > ret = errorIdx;
> > }
> >
> > - updateControls(ctrls, controlInfo, v4l2Ctrls, count);
> > + updateControls(ctrls, v4l2Ctrls, count);
> >
> > return ret;
> > }
> > @@ -262,8 +262,8 @@ int V4L2Device::setControls(V4L2ControlList *ctrls)
> > v4l2Ctrls[i].id = info->id();
> >
> > /* Set the v4l2_ext_control value for the write operation. */
> > - switch (info->type()) {
> > - case V4L2_CTRL_TYPE_INTEGER64:
> > + switch (ctrl->type()) {
>
> Hrm. ... this feels like a hidden change between switching on the info
> to the ctrl. I hope that's ok, it probably is.
>
> <second pass edit, Yes I see the change throughout>
>
>
>
> > + case DataTypeInteger64:
> > v4l2Ctrls[i].value64 = ctrl->value();
> > break;
> > default:
> > @@ -299,7 +299,7 @@ int V4L2Device::setControls(V4L2ControlList *ctrls)
> > ret = errorIdx;
> > }
> >
> > - updateControls(ctrls, controlInfo, v4l2Ctrls, count);
> > + updateControls(ctrls, v4l2Ctrls, count);
> >
> > return ret;
> > }
> > @@ -352,8 +352,7 @@ void V4L2Device::listControls()
> > ctrl.flags & V4L2_CTRL_FLAG_DISABLED)
> > continue;
> >
> > - V4L2ControlInfo info(ctrl);
> > - switch (info.type()) {
> > + switch (ctrl.type) {
> > case V4L2_CTRL_TYPE_INTEGER:
> > case V4L2_CTRL_TYPE_BOOLEAN:
> > case V4L2_CTRL_TYPE_MENU:
> > @@ -364,11 +363,12 @@ void V4L2Device::listControls()
> > break;
> > /* \todo Support compound controls. */
> > default:
> > - LOG(V4L2, Debug) << "Control type '" << info.type()
> > + LOG(V4L2, Debug) << "Control type '" << ctrl.type
> > << "' not supported";
> > continue;
> > }
> >
> > + V4L2ControlInfo info(ctrl);
> > controls_.emplace(ctrl.id, info);
> > }
> > }
> > @@ -382,25 +382,22 @@ void V4L2Device::listControls()
> > * \param[in] count The number of controls to update
> > */
> > void V4L2Device::updateControls(V4L2ControlList *ctrls,
> > - const V4L2ControlInfo **controlInfo,
> > const struct v4l2_ext_control *v4l2Ctrls,
> > unsigned int count)
> > {
> > for (unsigned int i = 0; i < count; ++i) {
> > - const struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];
> > - const V4L2ControlInfo *info = controlInfo[i];
> > V4L2Control *ctrl = ctrls->getByIndex(i);
> >
> > - switch (info->type()) {
> > - case V4L2_CTRL_TYPE_INTEGER64:
> > - ctrl->setValue(v4l2Ctrl->value64);
> > + switch (ctrl->type()) {
> > + case DataTypeInteger64:
> > + ctrl->setValue(v4l2Ctrls[i].value64);
> > break;
> > default:
> > /*
> > * \todo To be changed when support for string and
> > * compound controls will be added.
> > */
> > - ctrl->setValue(v4l2Ctrl->value);
> > + ctrl->setValue(v4l2Ctrls[i].value);
> > break;
> > }
> > }
> > --
> > 2.23.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
> >
>
> --
> Regards
> --
> Kieran
-------------- 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/20190923/7fde9864/attachment.sig>
More information about the libcamera-devel
mailing list