<div dir="ltr">Gentle ping for review, or could the patch series be merged?</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, May 10, 2021 at 2:42 PM Hirokazu Honda <<a href="mailto:hiroh@chromium.org">hiroh@chromium.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">V4L2ControlId and V4L2ControlInfo are just convenience classes to<br>
create ControlId and ControlInfo from v4l2_query_ext_control.<br>
Therefore, there is no need of being a class. It is used only<br>
from V4L2Device. This removes the classes and put the equivalent<br>
functions of creating ControlId and ControlInfo in<br>
v4l2_device.cpp.<br>
<br>
Signed-off-by: Hirokazu Honda <<a href="mailto:hiroh@chromium.org" target="_blank">hiroh@chromium.org</a>><br>
Reviewed-by: Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>><br>
---<br>
include/libcamera/internal/meson.build | 1 -<br>
include/libcamera/internal/v4l2_controls.h | 31 -----<br>
include/libcamera/internal/v4l2_device.h | 4 +-<br>
src/libcamera/meson.build | 1 -<br>
src/libcamera/v4l2_controls.cpp | 151 ---------------------<br>
src/libcamera/v4l2_device.cpp | 72 +++++++++-<br>
6 files changed, 71 insertions(+), 189 deletions(-)<br>
delete mode 100644 include/libcamera/internal/v4l2_controls.h<br>
delete mode 100644 src/libcamera/v4l2_controls.cpp<br>
<br>
diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build<br>
index 1fe3918c..3fb0216d 100644<br>
--- a/include/libcamera/internal/meson.build<br>
+++ b/include/libcamera/internal/meson.build<br>
@@ -42,7 +42,6 @@ libcamera_internal_headers = files([<br>
'thread.h',<br>
'timer.h',<br>
'utils.h',<br>
- 'v4l2_controls.h',<br>
'v4l2_device.h',<br>
'v4l2_pixelformat.h',<br>
'v4l2_subdevice.h',<br>
diff --git a/include/libcamera/internal/v4l2_controls.h b/include/libcamera/internal/v4l2_controls.h<br>
deleted file mode 100644<br>
index 0851b8dd..00000000<br>
--- a/include/libcamera/internal/v4l2_controls.h<br>
+++ /dev/null<br>
@@ -1,31 +0,0 @@<br>
-/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
-/*<br>
- * Copyright (C) 2019, Google Inc.<br>
- *<br>
- * v4l2_controls.h - V4L2 Controls Support<br>
- */<br>
-<br>
-#ifndef __LIBCAMERA_INTERNAL_V4L2_CONTROLS_H__<br>
-#define __LIBCAMERA_INTERNAL_V4L2_CONTROLS_H__<br>
-<br>
-#include <linux/videodev2.h><br>
-<br>
-#include <libcamera/controls.h><br>
-<br>
-namespace libcamera {<br>
-<br>
-class V4L2ControlId : public ControlId<br>
-{<br>
-public:<br>
- V4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl);<br>
-};<br>
-<br>
-class V4L2ControlInfo : public ControlInfo<br>
-{<br>
-public:<br>
- V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl);<br>
-};<br>
-<br>
-} /* namespace libcamera */<br>
-<br>
-#endif /* __LIBCAMERA_INTERNAL_V4L2_CONTROLS_H__ */<br>
diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h<br>
index 087f07e7..116bbbaf 100644<br>
--- a/include/libcamera/internal/v4l2_device.h<br>
+++ b/include/libcamera/internal/v4l2_device.h<br>
@@ -13,11 +13,11 @@<br>
<br>
#include <linux/videodev2.h><br>
<br>
+#include <libcamera/controls.h><br>
#include <libcamera/signal.h><br>
#include <libcamera/span.h><br>
<br>
#include "libcamera/internal/log.h"<br>
-#include "libcamera/internal/v4l2_controls.h"<br>
<br>
namespace libcamera {<br>
<br>
@@ -61,7 +61,7 @@ private:<br>
void eventAvailable(EventNotifier *notifier);<br>
<br>
std::map<unsigned int, struct v4l2_query_ext_ctrl> controlInfo_;<br>
- std::vector<std::unique_ptr<V4L2ControlId>> controlIds_;<br>
+ std::vector<std::unique_ptr<ControlId>> controlIds_;<br>
ControlInfoMap controls_;<br>
std::string deviceNode_;<br>
int fd_;<br>
diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build<br>
index e0a48aa2..62251a64 100644<br>
--- a/src/libcamera/meson.build<br>
+++ b/src/libcamera/meson.build<br>
@@ -51,7 +51,6 @@ libcamera_sources = files([<br>
'timer.cpp',<br>
'transform.cpp',<br>
'utils.cpp',<br>
- 'v4l2_controls.cpp',<br>
'v4l2_device.cpp',<br>
'v4l2_pixelformat.cpp',<br>
'v4l2_subdevice.cpp',<br>
diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp<br>
deleted file mode 100644<br>
index 3f8ec6ca..00000000<br>
--- a/src/libcamera/v4l2_controls.cpp<br>
+++ /dev/null<br>
@@ -1,151 +0,0 @@<br>
-/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
-/*<br>
- * Copyright (C) 2019, Google Inc.<br>
- *<br>
- * v4l2_controls.cpp - V4L2 Controls Support<br>
- */<br>
-<br>
-#include "libcamera/internal/v4l2_controls.h"<br>
-<br>
-#include <string.h><br>
-<br>
-/**<br>
- * \file v4l2_controls.h<br>
- * \brief Support for V4L2 Controls using the V4L2 Extended Controls APIs<br>
- *<br>
- * The V4L2 Control API allows application to inspect and modify sets of<br>
- * configurable parameters on a video device or subdevice. The nature of the<br>
- * parameters an application can modify using the control framework depends on<br>
- * what the driver implements support for, and on the characteristics of the<br>
- * underlying hardware platform. Generally controls are used to modify user<br>
- * visible settings, such as the image brightness and exposure time, or<br>
- * non-standard parameters which cannot be controlled through the V4L2 format<br>
- * negotiation API.<br>
- *<br>
- * Controls are identified by a numerical ID, defined by the V4L2 kernel headers<br>
- * and have an associated type. Each control has a value, which is the data that<br>
- * can be modified with V4L2Device::setControls() or retrieved with<br>
- * V4L2Device::getControls().<br>
- *<br>
- * The control's type along with the control's flags define the type of the<br>
- * control's value content. Controls can transport a single data value stored in<br>
- * variable inside the control, or they might as well deal with more complex<br>
- * data types, such as arrays of matrices, stored in a contiguous memory<br>
- * locations associated with the control and called 'the payload'. Such controls<br>
- * are called 'compound controls' and are currently not supported by the<br>
- * libcamera V4L2 control framework.<br>
- *<br>
- * libcamera implements support for controls using the V4L2 Extended Control<br>
- * API, which allows future handling of controls with payloads of arbitrary<br>
- * sizes.<br>
- *<br>
- * The libcamera V4L2 Controls framework operates on lists of controls, wrapped<br>
- * by the ControlList class, to match the V4L2 extended controls API. The<br>
- * interface to set and get control is implemented by the V4L2Device class, and<br>
- * this file only provides the data type definitions.<br>
- *<br>
- * \todo Add support for compound controls<br>
- */<br>
-<br>
-namespace libcamera {<br>
-<br>
-namespace {<br>
-<br>
-std::string v4l2_ctrl_name(const struct v4l2_query_ext_ctrl &ctrl)<br>
-{<br>
- size_t len = strnlen(<a href="http://ctrl.name" rel="noreferrer" target="_blank">ctrl.name</a>, sizeof(<a href="http://ctrl.name" rel="noreferrer" target="_blank">ctrl.name</a>));<br>
- return std::string(static_cast<const char *>(<a href="http://ctrl.name" rel="noreferrer" target="_blank">ctrl.name</a>), len);<br>
-}<br>
-<br>
-ControlType v4l2_ctrl_type(const struct v4l2_query_ext_ctrl &ctrl)<br>
-{<br>
- switch (ctrl.type) {<br>
- case V4L2_CTRL_TYPE_U8:<br>
- return ControlTypeByte;<br>
-<br>
- case V4L2_CTRL_TYPE_BOOLEAN:<br>
- return ControlTypeBool;<br>
-<br>
- case V4L2_CTRL_TYPE_INTEGER:<br>
- return ControlTypeInteger32;<br>
-<br>
- case V4L2_CTRL_TYPE_INTEGER64:<br>
- return ControlTypeInteger64;<br>
-<br>
- case V4L2_CTRL_TYPE_MENU:<br>
- case V4L2_CTRL_TYPE_BUTTON:<br>
- case V4L2_CTRL_TYPE_BITMASK:<br>
- case V4L2_CTRL_TYPE_INTEGER_MENU:<br>
- /*<br>
- * More precise types may be needed, for now use a 32-bit<br>
- * integer type.<br>
- */<br>
- return ControlTypeInteger32;<br>
-<br>
- default:<br>
- return ControlTypeNone;<br>
- }<br>
-}<br>
-<br>
-} /* namespace */<br>
-<br>
-/**<br>
- * \class V4L2ControlId<br>
- * \brief V4L2 control static metadata<br>
- *<br>
- * The V4L2ControlId class is a specialisation of the ControlId for V4L2<br>
- * controls.<br>
- */<br>
-<br>
-/**<br>
- * \brief Construct a V4L2ControlId from a struct v4l2_query_ext_ctrl<br>
- * \param[in] ctrl The struct v4l2_query_ext_ctrl as returned by the kernel<br>
- */<br>
-V4L2ControlId::V4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl)<br>
- : ControlId(<a href="http://ctrl.id" rel="noreferrer" target="_blank">ctrl.id</a>, v4l2_ctrl_name(ctrl), v4l2_ctrl_type(ctrl))<br>
-{<br>
-}<br>
-<br>
-/**<br>
- * \class V4L2ControlInfo<br>
- * \brief Convenience specialisation of ControlInfo for V4L2 controls<br>
- *<br>
- * The V4L2ControlInfo class is a specialisation of the ControlInfo for V4L2<br>
- * controls. It offers a convenience constructor from a struct<br>
- * v4l2_query_ext_ctrl, and is otherwise equivalent to the ControlInfo class.<br>
- */<br>
-<br>
-/**<br>
- * \brief Construct a V4L2ControlInfo from a struct v4l2_query_ext_ctrl<br>
- * \param[in] ctrl The struct v4l2_query_ext_ctrl as returned by the kernel<br>
- */<br>
-V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)<br>
-{<br>
- switch (ctrl.type) {<br>
- case V4L2_CTRL_TYPE_U8:<br>
- ControlInfo::operator=(ControlInfo(static_cast<uint8_t>(ctrl.minimum),<br>
- static_cast<uint8_t>(ctrl.maximum),<br>
- static_cast<uint8_t>(ctrl.default_value)));<br>
- break;<br>
-<br>
- case V4L2_CTRL_TYPE_BOOLEAN:<br>
- ControlInfo::operator=(ControlInfo(static_cast<bool>(ctrl.minimum),<br>
- static_cast<bool>(ctrl.maximum),<br>
- static_cast<bool>(ctrl.default_value)));<br>
- break;<br>
-<br>
- case V4L2_CTRL_TYPE_INTEGER64:<br>
- ControlInfo::operator=(ControlInfo(static_cast<int64_t>(ctrl.minimum),<br>
- static_cast<int64_t>(ctrl.maximum),<br>
- static_cast<int64_t>(ctrl.default_value)));<br>
- break;<br>
-<br>
- default:<br>
- ControlInfo::operator=(ControlInfo(static_cast<int32_t>(ctrl.minimum),<br>
- static_cast<int32_t>(ctrl.maximum),<br>
- static_cast<int32_t>(ctrl.default_value)));<br>
- break;<br>
- }<br>
-}<br>
-<br>
-} /* namespace libcamera */<br>
diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp<br>
index 397029ac..f9414043 100644<br>
--- a/src/libcamera/v4l2_device.cpp<br>
+++ b/src/libcamera/v4l2_device.cpp<br>
@@ -20,7 +20,6 @@<br>
#include "libcamera/internal/log.h"<br>
#include "libcamera/internal/sysfs.h"<br>
#include "libcamera/internal/utils.h"<br>
-#include "libcamera/internal/v4l2_controls.h"<br>
<br>
/**<br>
* \file v4l2_device.h<br>
@@ -31,6 +30,73 @@ namespace libcamera {<br>
<br>
LOG_DEFINE_CATEGORY(V4L2)<br>
<br>
+namespace {<br>
+<br>
+ControlType v4l2CtrlType(uint32_t ctrlType)<br>
+{<br>
+ switch (ctrlType) {<br>
+ case V4L2_CTRL_TYPE_U8:<br>
+ return ControlTypeByte;<br>
+<br>
+ case V4L2_CTRL_TYPE_BOOLEAN:<br>
+ return ControlTypeBool;<br>
+<br>
+ case V4L2_CTRL_TYPE_INTEGER:<br>
+ return ControlTypeInteger32;<br>
+<br>
+ case V4L2_CTRL_TYPE_INTEGER64:<br>
+ return ControlTypeInteger64;<br>
+<br>
+ case V4L2_CTRL_TYPE_MENU:<br>
+ case V4L2_CTRL_TYPE_BUTTON:<br>
+ case V4L2_CTRL_TYPE_BITMASK:<br>
+ case V4L2_CTRL_TYPE_INTEGER_MENU:<br>
+ /*<br>
+ * More precise types may be needed, for now use a 32-bit<br>
+ * integer type.<br>
+ */<br>
+ return ControlTypeInteger32;<br>
+<br>
+ default:<br>
+ return ControlTypeNone;<br>
+ }<br>
+}<br>
+<br>
+std::unique_ptr<ControlId> v4l2ControlId(const v4l2_query_ext_ctrl &ctrl)<br>
+{<br>
+ const size_t len = strnlen(<a href="http://ctrl.name" rel="noreferrer" target="_blank">ctrl.name</a>, sizeof(<a href="http://ctrl.name" rel="noreferrer" target="_blank">ctrl.name</a>));<br>
+ const std::string name(static_cast<const char *>(<a href="http://ctrl.name" rel="noreferrer" target="_blank">ctrl.name</a>), len);<br>
+ const ControlType type = v4l2CtrlType(ctrl.type);<br>
+<br>
+ return std::make_unique<ControlId>(<a href="http://ctrl.id" rel="noreferrer" target="_blank">ctrl.id</a>, name, type);<br>
+}<br>
+<br>
+ControlInfo v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl)<br>
+{<br>
+ switch (ctrl.type) {<br>
+ case V4L2_CTRL_TYPE_U8:<br>
+ return ControlInfo(static_cast<uint8_t>(ctrl.minimum),<br>
+ static_cast<uint8_t>(ctrl.maximum),<br>
+ static_cast<uint8_t>(ctrl.default_value));<br>
+<br>
+ case V4L2_CTRL_TYPE_BOOLEAN:<br>
+ return ControlInfo(static_cast<bool>(ctrl.minimum),<br>
+ static_cast<bool>(ctrl.maximum),<br>
+ static_cast<bool>(ctrl.default_value));<br>
+<br>
+ case V4L2_CTRL_TYPE_INTEGER64:<br>
+ return ControlInfo(static_cast<int64_t>(ctrl.minimum),<br>
+ static_cast<int64_t>(ctrl.maximum),<br>
+ static_cast<int64_t>(ctrl.default_value));<br>
+<br>
+ default:<br>
+ return ControlInfo(static_cast<int32_t>(ctrl.minimum),<br>
+ static_cast<int32_t>(ctrl.maximum),<br>
+ static_cast<int32_t>(ctrl.default_value));<br>
+ }<br>
+}<br>
+} /* namespace */<br>
+<br>
/**<br>
* \class V4L2Device<br>
* \brief Base class for V4L2VideoDevice and V4L2Subdevice<br>
@@ -502,10 +568,10 @@ void V4L2Device::listControls()<br>
continue;<br>
}<br>
<br>
- controlIds_.emplace_back(std::make_unique<V4L2ControlId>(ctrl));<br>
+ controlIds_.emplace_back(v4l2ControlId(ctrl));<br>
controlInfo_.emplace(<a href="http://ctrl.id" rel="noreferrer" target="_blank">ctrl.id</a>, ctrl);<br>
<br>
- ctrls.emplace(controlIds_.back().get(), V4L2ControlInfo(ctrl));<br>
+ ctrls.emplace(controlIds_.back().get(), v4l2ControlInfo(ctrl));<br>
}<br>
<br>
controls_ = std::move(ctrls);<br>
-- <br>
2.31.1.607.g51e8a6a459-goog<br>
<br>
</blockquote></div>