[libcamera-devel] [PATCH 3/5] libcamera: v4l2_controls: Use DataValue and DataInfo
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Sep 20 13:15:07 CEST 2019
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"
Otherwise, I think this all looks fine.
Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 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
More information about the libcamera-devel
mailing list