[libcamera-devel] [PATCH v2 2/2] libcamera: V4L2Control: remove V4L2Control classes
Hirokazu Honda
hiroh at chromium.org
Mon May 24 06:48:13 CEST 2021
Hi Laurent,
On Mon, May 24, 2021 at 1:20 PM Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> Hi Hiro,
>
> On Mon, May 24, 2021 at 06:57:03AM +0300, Laurent Pinchart wrote:
> > On Mon, May 10, 2021 at 02:42:42PM +0900, Hirokazu Honda wrote:
> > > V4L2ControlId and V4L2ControlInfo are just convenience classes to
> > > create ControlId and ControlInfo from v4l2_query_ext_control.
> > > Therefore, there is no need of being a class. It is used only
> > > from V4L2Device. This removes the classes and put the equivalent
> > > functions of creating ControlId and ControlInfo in
> > > v4l2_device.cpp.
> > >
> > > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > > ---
> > > include/libcamera/internal/meson.build | 1 -
> > > include/libcamera/internal/v4l2_controls.h | 31 -----
> > > include/libcamera/internal/v4l2_device.h | 4 +-
> > > src/libcamera/meson.build | 1 -
> > > src/libcamera/v4l2_controls.cpp | 151 ---------------------
> > > src/libcamera/v4l2_device.cpp | 72 +++++++++-
> > > 6 files changed, 71 insertions(+), 189 deletions(-)
> > > delete mode 100644 include/libcamera/internal/v4l2_controls.h
> > > delete mode 100644 src/libcamera/v4l2_controls.cpp
> >
> > Nice diffstat :-)
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > > diff --git a/include/libcamera/internal/meson.build
> b/include/libcamera/internal/meson.build
> > > index 1fe3918c..3fb0216d 100644
> > > --- a/include/libcamera/internal/meson.build
> > > +++ b/include/libcamera/internal/meson.build
> > > @@ -42,7 +42,6 @@ libcamera_internal_headers = files([
> > > 'thread.h',
> > > 'timer.h',
> > > 'utils.h',
> > > - 'v4l2_controls.h',
> > > 'v4l2_device.h',
> > > 'v4l2_pixelformat.h',
> > > 'v4l2_subdevice.h',
> > > diff --git a/include/libcamera/internal/v4l2_controls.h
> b/include/libcamera/internal/v4l2_controls.h
> > > deleted file mode 100644
> > > index 0851b8dd..00000000
> > > --- a/include/libcamera/internal/v4l2_controls.h
> > > +++ /dev/null
> > > @@ -1,31 +0,0 @@
> > > -/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > -/*
> > > - * Copyright (C) 2019, Google Inc.
> > > - *
> > > - * v4l2_controls.h - V4L2 Controls Support
> > > - */
> > > -
> > > -#ifndef __LIBCAMERA_INTERNAL_V4L2_CONTROLS_H__
> > > -#define __LIBCAMERA_INTERNAL_V4L2_CONTROLS_H__
> > > -
> > > -#include <linux/videodev2.h>
> > > -
> > > -#include <libcamera/controls.h>
> > > -
> > > -namespace libcamera {
> > > -
> > > -class V4L2ControlId : public ControlId
> > > -{
> > > -public:
> > > - V4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl);
> > > -};
> > > -
> > > -class V4L2ControlInfo : public ControlInfo
> > > -{
> > > -public:
> > > - V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl);
> > > -};
> > > -
> > > -} /* namespace libcamera */
> > > -
> > > -#endif /* __LIBCAMERA_INTERNAL_V4L2_CONTROLS_H__ */
> > > diff --git a/include/libcamera/internal/v4l2_device.h
> b/include/libcamera/internal/v4l2_device.h
> > > index 087f07e7..116bbbaf 100644
> > > --- a/include/libcamera/internal/v4l2_device.h
> > > +++ b/include/libcamera/internal/v4l2_device.h
> > > @@ -13,11 +13,11 @@
> > >
> > > #include <linux/videodev2.h>
> > >
> > > +#include <libcamera/controls.h>
> > > #include <libcamera/signal.h>
> > > #include <libcamera/span.h>
> > >
> > > #include "libcamera/internal/log.h"
> > > -#include "libcamera/internal/v4l2_controls.h"
> > >
> > > namespace libcamera {
> > >
> > > @@ -61,7 +61,7 @@ private:
> > > void eventAvailable(EventNotifier *notifier);
> > >
> > > std::map<unsigned int, struct v4l2_query_ext_ctrl> controlInfo_;
> > > - std::vector<std::unique_ptr<V4L2ControlId>> controlIds_;
> > > + std::vector<std::unique_ptr<ControlId>> controlIds_;
> > > ControlInfoMap controls_;
> > > std::string deviceNode_;
> > > int fd_;
> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > index e0a48aa2..62251a64 100644
> > > --- a/src/libcamera/meson.build
> > > +++ b/src/libcamera/meson.build
> > > @@ -51,7 +51,6 @@ libcamera_sources = files([
> > > 'timer.cpp',
> > > 'transform.cpp',
> > > 'utils.cpp',
> > > - 'v4l2_controls.cpp',
> > > 'v4l2_device.cpp',
> > > 'v4l2_pixelformat.cpp',
> > > 'v4l2_subdevice.cpp',
> > > diff --git a/src/libcamera/v4l2_controls.cpp
> b/src/libcamera/v4l2_controls.cpp
> > > deleted file mode 100644
> > > index 3f8ec6ca..00000000
> > > --- a/src/libcamera/v4l2_controls.cpp
> > > +++ /dev/null
> > > @@ -1,151 +0,0 @@
> > > -/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > -/*
> > > - * Copyright (C) 2019, Google Inc.
> > > - *
> > > - * v4l2_controls.cpp - V4L2 Controls Support
> > > - */
> > > -
> > > -#include "libcamera/internal/v4l2_controls.h"
> > > -
> > > -#include <string.h>
> > > -
> > > -/**
> > > - * \file v4l2_controls.h
> > > - * \brief Support for V4L2 Controls using the V4L2 Extended Controls
> APIs
> > > - *
> > > - * The V4L2 Control API allows application to inspect and modify sets
> of
> > > - * configurable parameters on a video device or subdevice. The nature
> of the
> > > - * parameters an application can modify using the control framework
> depends on
> > > - * what the driver implements support for, and on the characteristics
> of the
> > > - * underlying hardware platform. Generally controls are used to
> modify user
> > > - * visible settings, such as the image brightness and exposure time,
> or
> > > - * non-standard parameters which cannot be controlled through the
> V4L2 format
> > > - * negotiation API.
> > > - *
> > > - * Controls are identified by a numerical ID, defined by the V4L2
> kernel headers
> > > - * and have an associated type. Each control has a value, which is
> the data that
> > > - * can be modified with V4L2Device::setControls() or retrieved with
> > > - * V4L2Device::getControls().
> > > - *
> > > - * The control's type along with the control's flags define the type
> of the
> > > - * control's value content. Controls can transport a single data
> value stored in
> > > - * variable inside the control, or they might as well deal with more
> complex
> > > - * data types, such as arrays of matrices, stored in a contiguous
> memory
> > > - * locations associated with the control and called 'the payload'.
> Such controls
> > > - * are called 'compound controls' and are currently not supported by
> the
> > > - * libcamera V4L2 control framework.
> > > - *
> > > - * libcamera implements support for controls using the V4L2 Extended
> Control
> > > - * API, which allows future handling of controls with payloads of
> arbitrary
> > > - * sizes.
> > > - *
> > > - * The libcamera V4L2 Controls framework operates on lists of
> controls, wrapped
> > > - * 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.
> > > - *
> > > - * \todo Add support for compound controls
> > > - */
> > > -
> > > -namespace libcamera {
> > > -
> > > -namespace {
> > > -
> > > -std::string v4l2_ctrl_name(const struct v4l2_query_ext_ctrl &ctrl)
> > > -{
> > > - size_t len = strnlen(ctrl.name, sizeof(ctrl.name));
> > > - return std::string(static_cast<const char *>(ctrl.name), len);
> > > -}
> > > -
> > > -ControlType v4l2_ctrl_type(const struct v4l2_query_ext_ctrl &ctrl)
> > > -{
> > > - switch (ctrl.type) {
> > > - case V4L2_CTRL_TYPE_U8:
> > > - return ControlTypeByte;
> > > -
> > > - case V4L2_CTRL_TYPE_BOOLEAN:
> > > - return ControlTypeBool;
> > > -
> > > - case V4L2_CTRL_TYPE_INTEGER:
> > > - return ControlTypeInteger32;
> > > -
> > > - case V4L2_CTRL_TYPE_INTEGER64:
> > > - return ControlTypeInteger64;
> > > -
> > > - case V4L2_CTRL_TYPE_MENU:
> > > - case V4L2_CTRL_TYPE_BUTTON:
> > > - case V4L2_CTRL_TYPE_BITMASK:
> > > - case V4L2_CTRL_TYPE_INTEGER_MENU:
> > > - /*
> > > - * More precise types may be needed, for now use a 32-bit
> > > - * integer type.
> > > - */
> > > - return ControlTypeInteger32;
> > > -
> > > - default:
> > > - return ControlTypeNone;
> > > - }
> > > -}
> > > -
> > > -} /* namespace */
> > > -
> > > -/**
> > > - * \class V4L2ControlId
> > > - * \brief V4L2 control static metadata
> > > - *
> > > - * The V4L2ControlId class is a specialisation of the ControlId for
> V4L2
> > > - * controls.
> > > - */
> > > -
> > > -/**
> > > - * \brief Construct a V4L2ControlId from a struct v4l2_query_ext_ctrl
> > > - * \param[in] ctrl The struct v4l2_query_ext_ctrl as returned by the
> kernel
> > > - */
> > > -V4L2ControlId::V4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl)
> > > - : ControlId(ctrl.id, v4l2_ctrl_name(ctrl), v4l2_ctrl_type(ctrl))
> > > -{
> > > -}
> > > -
> > > -/**
> > > - * \class V4L2ControlInfo
> > > - * \brief Convenience specialisation of ControlInfo for V4L2 controls
> > > - *
> > > - * The V4L2ControlInfo class is a specialisation of the ControlInfo
> for V4L2
> > > - * controls. It offers a convenience constructor from a struct
> > > - * v4l2_query_ext_ctrl, and is otherwise equivalent to the
> ControlInfo class.
> > > - */
> > > -
> > > -/**
> > > - * \brief Construct a V4L2ControlInfo from a struct
> v4l2_query_ext_ctrl
> > > - * \param[in] ctrl The struct v4l2_query_ext_ctrl as returned by the
> kernel
> > > - */
> > > -V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl
> &ctrl)
> > > -{
> > > - switch (ctrl.type) {
> > > - case V4L2_CTRL_TYPE_U8:
> > > -
> ControlInfo::operator=(ControlInfo(static_cast<uint8_t>(ctrl.minimum),
> > > -
> static_cast<uint8_t>(ctrl.maximum),
> > > -
> static_cast<uint8_t>(ctrl.default_value)));
> > > - break;
> > > -
> > > - case V4L2_CTRL_TYPE_BOOLEAN:
> > > -
> ControlInfo::operator=(ControlInfo(static_cast<bool>(ctrl.minimum),
> > > -
> static_cast<bool>(ctrl.maximum),
> > > -
> static_cast<bool>(ctrl.default_value)));
> > > - break;
> > > -
> > > - case V4L2_CTRL_TYPE_INTEGER64:
> > > -
> ControlInfo::operator=(ControlInfo(static_cast<int64_t>(ctrl.minimum),
> > > -
> static_cast<int64_t>(ctrl.maximum),
> > > -
> static_cast<int64_t>(ctrl.default_value)));
> > > - break;
> > > -
> > > - default:
> > > -
> ControlInfo::operator=(ControlInfo(static_cast<int32_t>(ctrl.minimum),
> > > -
> static_cast<int32_t>(ctrl.maximum),
> > > -
> static_cast<int32_t>(ctrl.default_value)));
> > > - break;
> > > - }
> > > -}
> > > -
> > > -} /* namespace libcamera */
> > > diff --git a/src/libcamera/v4l2_device.cpp
> b/src/libcamera/v4l2_device.cpp
> > > index 397029ac..f9414043 100644
> > > --- a/src/libcamera/v4l2_device.cpp
> > > +++ b/src/libcamera/v4l2_device.cpp
> > > @@ -20,7 +20,6 @@
> > > #include "libcamera/internal/log.h"
> > > #include "libcamera/internal/sysfs.h"
> > > #include "libcamera/internal/utils.h"
> > > -#include "libcamera/internal/v4l2_controls.h"
> > >
> > > /**
> > > * \file v4l2_device.h
> > > @@ -31,6 +30,73 @@ namespace libcamera {
> > >
> > > LOG_DEFINE_CATEGORY(V4L2)
> > >
> > > +namespace {
> > > +
> > > +ControlType v4l2CtrlType(uint32_t ctrlType)
> > > +{
> > > + switch (ctrlType) {
> > > + case V4L2_CTRL_TYPE_U8:
> > > + return ControlTypeByte;
> > > +
> > > + case V4L2_CTRL_TYPE_BOOLEAN:
> > > + return ControlTypeBool;
> > > +
> > > + case V4L2_CTRL_TYPE_INTEGER:
> > > + return ControlTypeInteger32;
> > > +
> > > + case V4L2_CTRL_TYPE_INTEGER64:
> > > + return ControlTypeInteger64;
> > > +
> > > + case V4L2_CTRL_TYPE_MENU:
> > > + case V4L2_CTRL_TYPE_BUTTON:
> > > + case V4L2_CTRL_TYPE_BITMASK:
> > > + case V4L2_CTRL_TYPE_INTEGER_MENU:
> > > + /*
> > > + * More precise types may be needed, for now use a 32-bit
> > > + * integer type.
> > > + */
> > > + return ControlTypeInteger32;
> > > +
> > > + default:
> > > + return ControlTypeNone;
> > > + }
> > > +}
> > > +
> > > +std::unique_ptr<ControlId> v4l2ControlId(const v4l2_query_ext_ctrl
> &ctrl)
> > > +{
> > > + const size_t len = strnlen(ctrl.name, sizeof(ctrl.name));
> > > + const std::string name(static_cast<const char *>(ctrl.name), len);
> > > + const ControlType type = v4l2CtrlType(ctrl.type);
> > > +
> > > + return std::make_unique<ControlId>(ctrl.id, name, type);
> > > +}
> > > +
> > > +ControlInfo v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl)
> > > +{
> > > + switch (ctrl.type) {
> > > + case V4L2_CTRL_TYPE_U8:
> > > + return ControlInfo(static_cast<uint8_t>(ctrl.minimum),
> > > + static_cast<uint8_t>(ctrl.maximum),
> > > +
> static_cast<uint8_t>(ctrl.default_value));
> > > +
> > > + case V4L2_CTRL_TYPE_BOOLEAN:
> > > + return ControlInfo(static_cast<bool>(ctrl.minimum),
> > > + static_cast<bool>(ctrl.maximum),
> > > + static_cast<bool>(ctrl.default_value));
> > > +
> > > + case V4L2_CTRL_TYPE_INTEGER64:
> > > + return ControlInfo(static_cast<int64_t>(ctrl.minimum),
> > > + static_cast<int64_t>(ctrl.maximum),
> > > +
> static_cast<int64_t>(ctrl.default_value));
> > > +
> > > + default:
> > > + return ControlInfo(static_cast<int32_t>(ctrl.minimum),
> > > + static_cast<int32_t>(ctrl.maximum),
> > > +
> static_cast<int32_t>(ctrl.default_value));
> > > + }
> > > +}
> > > +} /* namespace */
> > > +
> > > /**
> > > * \class V4L2Device
> > > * \brief Base class for V4L2VideoDevice and V4L2Subdevice
> > > @@ -502,10 +568,10 @@ void V4L2Device::listControls()
> > > continue;
> > > }
> > >
> > > -
> controlIds_.emplace_back(std::make_unique<V4L2ControlId>(ctrl));
> > > + controlIds_.emplace_back(v4l2ControlId(ctrl));
> > > controlInfo_.emplace(ctrl.id, ctrl);
> > >
> > > - ctrls.emplace(controlIds_.back().get(),
> V4L2ControlInfo(ctrl));
>
> There's another instance of V4L2ControlInfo in
> V4L2Device::updateControlInfo(). I think it can just be replace with
> v4l2ControlInfo(). If that's fine with you, I can handle this when
> applying.
>
>
Indeed. Thanks in advance. I have no idea why I haven't caught it when
creating this pacth. :(
-Hiro
> > > + ctrls.emplace(controlIds_.back().get(),
> v4l2ControlInfo(ctrl));
> > > }
> > >
> > > controls_ = std::move(ctrls);
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210524/b2cfce2e/attachment-0001.htm>
More information about the libcamera-devel
mailing list