[libcamera-devel,v2,1/2] libcamera: pipeline: Remove unnecessary v4l2_controls.h includes
diff mbox series

Message ID 20210510054242.2735473-1-hiroh@chromium.org
State Accepted
Headers show
Series
  • [libcamera-devel,v2,1/2] libcamera: pipeline: Remove unnecessary v4l2_controls.h includes
Related show

Commit Message

Hirokazu Honda May 10, 2021, 5:42 a.m. UTC 
v4l2_controls.h is included in some places in pipeline codes.
But V4l2Control classes are not used there. This removes the
redundant v4l2_controls.h includes.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/pipeline/ipu3/ipu3.cpp               | 1 -
 src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 1 -
 src/libcamera/pipeline/uvcvideo/uvcvideo.cpp       | 1 -
 src/libcamera/pipeline/vimc/vimc.cpp               | 1 -
 4 files changed, 4 deletions(-)

Comments

Hirokazu Honda May 20, 2021, 9:03 a.m. UTC | #1 
Gentle ping for review, or could the patch series be merged?

On Mon, May 10, 2021 at 2:42 PM Hirokazu Honda <hiroh@chromium.org> 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@chromium.org>
> Reviewed-by: Jacopo Mondi <jacopo@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
>
> 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));
> +               ctrls.emplace(controlIds_.back().get(),
> v4l2ControlInfo(ctrl));
>         }
>
>         controls_ = std::move(ctrls);
> --
> 2.31.1.607.g51e8a6a459-goog
>
>
Laurent Pinchart May 24, 2021, 3:51 a.m. UTC | #2 
Hi Hiro,

Thank you for the patch.

On Mon, May 10, 2021 at 02:42:41PM +0900, Hirokazu Honda wrote:
> v4l2_controls.h is included in some places in pipeline codes.
> But V4l2Control classes are not used there. This removes the
> redundant v4l2_controls.h includes.
> 
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp               | 1 -
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 1 -
>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp       | 1 -
>  src/libcamera/pipeline/vimc/vimc.cpp               | 1 -
>  4 files changed, 4 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index ade8ffbd..411e70b7 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -28,7 +28,6 @@
>  #include "libcamera/internal/media_device.h"
>  #include "libcamera/internal/pipeline_handler.h"
>  #include "libcamera/internal/utils.h"
> -#include "libcamera/internal/v4l2_controls.h"
>  
>  #include "cio2.h"
>  #include "frames.h"
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index b2256493..e485f25f 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -36,7 +36,6 @@
>  #include "libcamera/internal/media_device.h"
>  #include "libcamera/internal/pipeline_handler.h"
>  #include "libcamera/internal/utils.h"
> -#include "libcamera/internal/v4l2_controls.h"
>  #include "libcamera/internal/v4l2_videodevice.h"
>  
>  #include "dma_heaps.h"
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index faa8d6b0..12a85b24 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -25,7 +25,6 @@
>  #include "libcamera/internal/pipeline_handler.h"
>  #include "libcamera/internal/sysfs.h"
>  #include "libcamera/internal/utils.h"
> -#include "libcamera/internal/v4l2_controls.h"
>  #include "libcamera/internal/v4l2_videodevice.h"
>  
>  namespace libcamera {
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> index ce83dcaa..d89ab33a 100644
> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> @@ -30,7 +30,6 @@
>  #include "libcamera/internal/media_device.h"
>  #include "libcamera/internal/pipeline_handler.h"
>  #include "libcamera/internal/utils.h"
> -#include "libcamera/internal/v4l2_controls.h"
>  #include "libcamera/internal/v4l2_subdevice.h"
>  #include "libcamera/internal/v4l2_videodevice.h"
>
Laurent Pinchart May 24, 2021, 3:57 a.m. UTC | #3 
Hi Hiro,

Thank you for the patch.

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@chromium.org>
> Reviewed-by: Jacopo Mondi <jacopo@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@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));
> +		ctrls.emplace(controlIds_.back().get(), v4l2ControlInfo(ctrl));
>  	}
>  
>  	controls_ = std::move(ctrls);
Laurent Pinchart May 24, 2021, 4:20 a.m. UTC | #4 
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@chromium.org>
> > Reviewed-by: Jacopo Mondi <jacopo@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@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.

> > +		ctrls.emplace(controlIds_.back().get(), v4l2ControlInfo(ctrl));
> >  	}
> >  
> >  	controls_ = std::move(ctrls);
Hirokazu Honda May 24, 2021, 4:48 a.m. UTC | #5 
Hi Laurent,

On Mon, May 24, 2021 at 1:20 PM Laurent Pinchart <
laurent.pinchart@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@chromium.org>
> > > Reviewed-by: Jacopo Mondi <jacopo@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@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
>

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index ade8ffbd..411e70b7 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -28,7 +28,6 @@ 
 #include "libcamera/internal/media_device.h"
 #include "libcamera/internal/pipeline_handler.h"
 #include "libcamera/internal/utils.h"
-#include "libcamera/internal/v4l2_controls.h"
 
 #include "cio2.h"
 #include "frames.h"
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
index b2256493..e485f25f 100644
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
@@ -36,7 +36,6 @@ 
 #include "libcamera/internal/media_device.h"
 #include "libcamera/internal/pipeline_handler.h"
 #include "libcamera/internal/utils.h"
-#include "libcamera/internal/v4l2_controls.h"
 #include "libcamera/internal/v4l2_videodevice.h"
 
 #include "dma_heaps.h"
diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
index faa8d6b0..12a85b24 100644
--- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
+++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
@@ -25,7 +25,6 @@ 
 #include "libcamera/internal/pipeline_handler.h"
 #include "libcamera/internal/sysfs.h"
 #include "libcamera/internal/utils.h"
-#include "libcamera/internal/v4l2_controls.h"
 #include "libcamera/internal/v4l2_videodevice.h"
 
 namespace libcamera {
diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
index ce83dcaa..d89ab33a 100644
--- a/src/libcamera/pipeline/vimc/vimc.cpp
+++ b/src/libcamera/pipeline/vimc/vimc.cpp
@@ -30,7 +30,6 @@ 
 #include "libcamera/internal/media_device.h"
 #include "libcamera/internal/pipeline_handler.h"
 #include "libcamera/internal/utils.h"
-#include "libcamera/internal/v4l2_controls.h"
 #include "libcamera/internal/v4l2_subdevice.h"
 #include "libcamera/internal/v4l2_videodevice.h"