[libcamera-devel,5/5] libcamera: controls: Control framework refresh

Message ID 20190918103133.14296-6-jacopo@jmondi.org
State Superseded
Headers show
Series
  • libcamera: Control framework backend rework
Related show

Commit Message

Jacopo Mondi Sept. 18, 2019, 10:31 a.m. UTC
Now that the control data value and info storage have been unified with
V4L2 control a refresh of naming and minor bits had to be performed.
The control framework implementation uses the word 'control' in types
and definition to refer to different things, making it harder to follow.

Perform a rename of elements defined by the control framework to enforce
the following concepts:
- control metadata:
  static control metadata information such as id, type and name, defined by
  libcamera
- control information:
  constant information associated with a control used for validation of
  control values, provided by pipeline handlers and defined by the
  capabilities of the device the control applies to
- control:
  a control identifier with an associated value, provided by
  applications when setting a control to a specific value

Update the framework documentation trying to use those concepts
consistently.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 include/libcamera/controls.h        |  19 ++--
 src/libcamera/controls.cpp          | 129 +++++++++++++++++-----------
 src/libcamera/gen-controls.awk      |   2 +-
 src/libcamera/meson.build           |   6 +-
 src/libcamera/pipeline/uvcvideo.cpp |   2 +-
 src/libcamera/pipeline/vimc.cpp     |   2 +-
 test/controls/control_list.cpp      |   2 +-
 7 files changed, 97 insertions(+), 65 deletions(-)

Patch

diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
index d3065c0f3f16..174bc92732aa 100644
--- a/include/libcamera/controls.h
+++ b/include/libcamera/controls.h
@@ -19,7 +19,7 @@  namespace libcamera {
 
 class Camera;
 
-struct ControlIdentifier {
+struct ControlMetadata {
 	ControlId id;
 	const char *name;
 	DataType type;
@@ -37,21 +37,22 @@  public:
 	std::string toString() const;
 
 private:
-	const struct ControlIdentifier *ident_;
+	const struct ControlMetadata *ident_;
 };
 
 using ControlInfoMap = std::unordered_map<ControlId, ControlInfo>;
 
+using Control = DataValue;
 class ControlList
 {
 private:
-	using ControlListMap = std::unordered_map<const ControlInfo *, DataValue>;
+	using ControlMap = std::unordered_map<const ControlInfo *, Control>;
 
 public:
 	ControlList(const ControlInfoMap &infoMap);
 
-	using iterator = ControlListMap::iterator;
-	using const_iterator = ControlListMap::const_iterator;
+	using iterator = ControlMap::iterator;
+	using const_iterator = ControlMap::const_iterator;
 
 	iterator begin() { return controls_.begin(); }
 	iterator end() { return controls_.end(); }
@@ -64,14 +65,14 @@  public:
 	std::size_t size() const { return controls_.size(); }
 	void clear() { controls_.clear(); }
 
-	DataValue &operator[](ControlId id);
-	DataValue &operator[](const ControlInfo *info) { return controls_[info]; }
+	Control &operator[](ControlId id);
+	Control &operator[](const ControlInfo *info) { return controls_[info]; }
 
-	void update(const ControlList &list);
+	bool merge(const ControlList &list);
 
 private:
 	const ControlInfoMap &infoMap_;
-	ControlListMap controls_;
+	ControlMap controls_;
 };
 
 } /* namespace libcamera */
diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
index 184d544c05bc..028ffc1e80b9 100644
--- a/src/libcamera/controls.cpp
+++ b/src/libcamera/controls.cpp
@@ -7,9 +7,6 @@ 
 
 #include <libcamera/controls.h>
 
-#include <sstream>
-#include <string>
-
 #include <libcamera/camera.h>
 
 #include "log.h"
@@ -17,7 +14,7 @@ 
 
 /**
  * \file controls.h
- * \brief Describes control framework and controls supported by a camera
+ * \brief Control framework implementation
  */
 
 namespace libcamera {
@@ -72,38 +69,50 @@  LOG_DEFINE_CATEGORY(Controls)
  */
 
 /**
- * \struct ControlIdentifier
- * \brief Describe a ControlId with control specific constant meta-data
+ * \struct ControlMetadata
+ * \brief Describe the control static meta-data
+ *
+ * Defines the Control static meta-data information, auto-generated from the
+ * ControlId documentation.
+ *
+ * The control information are defined in the control_id.h file, parsed by
+ * the gen-controls.awk script to produce a control_metadata.cpp file that
+ * provides a static list of control id with an associated type and name. The
+ * file is not for public use, and so no suitable header exists as
+ * its sole usage is for the ControlMetadata definition.
  *
- * Defines a Control with a unique ID, a name, and a type.
- * This structure is used as static part of the auto-generated control
- * definitions, which are generated from the ControlId documentation.
+ * \todo Expand the control meta-data definition to support more complex
+ * control types and describe their characteristics (ie. Number of deta elements
+ * for compound control types, versioning etc).
  *
- * \var ControlIdentifier::id
+ * \var ControlMetadata::id
  * The unique ID for a control
- * \var ControlIdentifier::name
+ * \var ControlMetadata::name
  * The string representation of the control
- * \var ControlIdentifier::type
+ * \var ControlMetadata::type
  * The ValueType required to represent the control value
  */
 
 /*
- * The controlTypes are automatically generated to produce a control_types.cpp
- * output. This file is not for public use, and so no suitable header exists
- * for this sole usage of the controlTypes reference. As such the extern is
- * only defined here for use during the ControlInfo constructor and should not
+ * The ControlTypes map is defined by the generated control_metadata.cpp file
+ * and only used here during the ControlInfo construction and should not
  * be referenced directly elsewhere.
  */
-extern const std::unordered_map<ControlId, ControlIdentifier> controlTypes;
+extern const std::unordered_map<ControlId, ControlMetadata> controlTypes;
 
 /**
  * \class ControlInfo
  * \brief Describe the information and capabilities of a Control
  *
- * The ControlInfo represents control specific meta-data which is constant on a
- * per camera basis. ControlInfo classes are constructed by pipeline handlers
- * to expose the controls they support and the metadata needed to utilise those
- * controls.
+ * The ControlInfo class associates the control's constant metadata defined by
+ * libcamera and collected in the ControlMetadata class, with its static
+ * information, such the range of supported values and the control size,
+ * described by the DataInfo base class and defined by the capabilities of
+ * the Camera device that implements the control.
+ *
+ * ControlInfo instances are constructed by pipeline handlers and associated
+ * with the Camera devices they register to expose the list of controls they
+ * support and the static information required to use those controls.
  */
 
 /**
@@ -118,7 +127,7 @@  ControlInfo::ControlInfo(ControlId id, const DataValue &min,
 {
 	auto iter = controlTypes.find(id);
 	if (iter == controlTypes.end()) {
-		LOG(Controls, Fatal) << "Attempt to create invalid ControlInfo";
+		LOG(Controls, Fatal) << "Control " << id << " not defined";
 		return;
 	}
 
@@ -127,19 +136,19 @@  ControlInfo::ControlInfo(ControlId id, const DataValue &min,
 
 /**
  * \fn ControlInfo::id()
- * \brief Retrieve the control ID
+ * \brief Retrieve the control ID from the control constant metadata
  * \return The control ID
  */
 
 /**
  * \fn ControlInfo::name()
- * \brief Retrieve the control name string
+ * \brief Retrieve the control name string from the control constant metadata
  * \return The control name string
  */
 
 /**
  * \fn ControlInfo::type()
- * \brief Retrieve the control designated type
+ * \brief Retrieve the control designated type from the control constant metadata
  * \return The control type
  */
 
@@ -161,21 +170,37 @@  std::string ControlInfo::toString() const
  * \brief A map of ControlId to ControlInfo
  */
 
+/**
+ * \typedef Control
+ * \brief Use 'Control' in place of DataValue in the ControlList class
+ *
+ * \todo If more data and operations than the ones defined by DataValue
+ * will need to be implemented for controls, make this typedef a class that
+ * inherits from DataValue.
+ */
+
 /**
  * \class ControlList
- * \brief Associate a list of ControlId with their values for a camera
+ * \brief List of controls values
  *
- * A ControlList wraps a map of ControlId to DataValue and provide
- * additional validation against the control information exposed by a Camera.
+ * A ControlList wraps a map of ControlId to Control instances and provide
+ * additional validation against the control information reported by a Camera.
  *
- * A list is only valid for as long as the camera it refers to is valid. After
- * that calling any method of the ControlList class other than its destructor
- * will cause undefined behaviour.
+ * ControlList instances are initially created empty and should be populated by
+ * assigning a value to each control added to the list. The added control is
+ * validated to verify it is supported by the Camera the list refers to, and
+ * validate the provided value against  the static information provided by the
+ * ControlInfo instance associated with the control.
  */
 
 /**
- * \brief Construct a ControlList with a reference to the Camera it applies on
+ * \brief Construct a ControlList with a map of control information
  * \param[in] infoMap The ControlInfoMap of the camera the control list refers to
+ *
+ * The provided \a infoMap collects the control id and the associated static
+ * information for each control that can be set on the list. \a infoMap is
+ * provided by the Camera the list of controls refers to and it is used to make
+ * sure only controls supported by the camera can be added to the list.
  */
 ControlList::ControlList(const ControlInfoMap &infoMap)
 	: infoMap_(infoMap)
@@ -221,10 +246,6 @@  ControlList::ControlList(const ControlInfoMap &infoMap)
 /**
  * \brief Check if the list contains a control with the specified \a id
  * \param[in] id The control ID
- *
- * The behaviour is undefined if the control \a id is not supported by the
- * camera that the ControlList refers to.
- *
  * \return True if the list contains a matching control, false otherwise
  */
 bool ControlList::contains(ControlId id) const
@@ -258,7 +279,7 @@  bool ControlList::contains(const ControlInfo *info) const
 /**
  * \fn ControlList::size()
  * \brief Retrieve the number of controls in the list
- * \return The number of DataValue entries stored in the list
+ * \return The number of Control entries stored in the list
  */
 
 /**
@@ -276,15 +297,16 @@  bool ControlList::contains(const ControlInfo *info) const
  * The behaviour is undefined if the control \a id is not supported by the
  * camera that the ControlList refers to.
  *
- * \return A reference to the value of the control identified by \a id
+ * \return A reference to the value of the control identified by \a id if the
+ * control is supported, an empty control otherwise
  */
-DataValue &ControlList::operator[](ControlId id)
+Control &ControlList::operator[](ControlId id)
 {
 	const auto info = infoMap_.find(id);
 	if (info == infoMap_.end()) {
 		LOG(Controls, Error) << "Control " << id << " not supported";
 
-		static DataValue empty;
+		static Control empty;
 		return empty;
 	}
 
@@ -299,22 +321,29 @@  DataValue &ControlList::operator[](ControlId id)
  * This method returns a reference to the control identified by \a info,
  * inserting it in the list if the info is not already present.
  *
- * \return A reference to the value of the control identified by \a info
+ * The behaviour is undefined if the control \a info is not supported by the
+ * camera that the ControlList refers to.
+ *
+ * \return A reference to the value of the control identified by \a info if the
+ * control is supported, an mpty control otherwise
  */
 
 /**
- * \brief Update the list with a union of itself and \a other
- * \param other The other list
+ * \brief Merge the control in the list with controls from \a other
+ * \param other The control list to merge with
  *
  * Update the control list to include all values from the \a other list.
- * Elements in the list whose control IDs are contained in \a other are updated
+ * Elements in the list whose control IDs are contained in \a other are merged
  * with the value from \a other. Elements in the \a other list that have no
  * corresponding element in the list are added to the list with their value.
  *
- * The behaviour is undefined if the two lists refer to different Camera
- * instances.
+ * All controls in \a other should be supported by the list, otherwise no
+ * control is updated.
+ *
+ * \return True if control list have been merged, false otherwise
+ *
  */
-void ControlList::update(const ControlList &other)
+bool ControlList::merge(const ControlList &other)
 {
 	/*
 	 * Make sure if all controls in other's list are supported by this
@@ -326,14 +355,16 @@  void ControlList::update(const ControlList &other)
 
 		if (infoMap_.find(id) == infoMap_.end()) {
 			LOG(Controls, Error)
-				<< "Failed to update control list with control: "
+				<< "Failed to merge control list with control: "
 				<< id;
-			return;
+			return false;
 		}
 	}
 
 	for (auto &control : other)
 		controls_[control.first] = control.second;
+
+	return true;
 }
 
 } /* namespace libcamera */
diff --git a/src/libcamera/gen-controls.awk b/src/libcamera/gen-controls.awk
index abeb0218546b..b7f6532dfd43 100755
--- a/src/libcamera/gen-controls.awk
+++ b/src/libcamera/gen-controls.awk
@@ -89,7 +89,7 @@  function GenerateTable(file) {
 
 	EnterNameSpace(file)
 
-	print "extern const std::unordered_map<ControlId, ControlIdentifier>" > file
+	print "extern const std::unordered_map<ControlId, ControlMetadata>" > file
 	print "controlTypes {" > file
 	for (i=1; i <= id; ++i) {
 		printf "\t{ %s, { %s, \"%s\", DataType%s } },\n", names[i], names[i], names[i], types[i] > file
diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
index c3100a1709e0..c4fcd0569bd7 100644
--- a/src/libcamera/meson.build
+++ b/src/libcamera/meson.build
@@ -60,13 +60,13 @@  endif
 
 gen_controls = files('gen-controls.awk')
 
-control_types_cpp = custom_target('control_types_cpp',
+control_metadata_cpp = custom_target('control_metadata_cpp',
                                   input : files('controls.cpp'),
-                                  output : 'control_types.cpp',
+                                  output : 'control_metadata.cpp',
                                   depend_files : gen_controls,
                                   command : [gen_controls, '@INPUT@', '--code', '@OUTPUT@'])
 
-libcamera_sources += control_types_cpp
+libcamera_sources += control_metadata_cpp
 
 gen_version = join_paths(meson.source_root(), 'utils', 'gen-version.sh')
 
diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
index dd253f94ca3d..d7f7fb0d6322 100644
--- a/src/libcamera/pipeline/uvcvideo.cpp
+++ b/src/libcamera/pipeline/uvcvideo.cpp
@@ -231,7 +231,7 @@  int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)
 
 	for (auto it : request->controls()) {
 		const ControlInfo *ci = it.first;
-		DataValue &value = it.second;
+		Control &value = it.second;
 
 		switch (ci->id()) {
 		case Brightness:
diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
index 3df239bdb277..1d36ec54bc3b 100644
--- a/src/libcamera/pipeline/vimc.cpp
+++ b/src/libcamera/pipeline/vimc.cpp
@@ -284,7 +284,7 @@  int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)
 
 	for (auto it : request->controls()) {
 		const ControlInfo *ci = it.first;
-		DataValue &value = it.second;
+		Control &value = it.second;
 
 		switch (ci->id()) {
 		case Brightness:
diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp
index 3c6eb40c091b..2a90cffe5106 100644
--- a/test/controls/control_list.cpp
+++ b/test/controls/control_list.cpp
@@ -159,7 +159,7 @@  protected:
 
 		newList[Brightness] = 128;
 		newList[Saturation] = 255;
-		newList.update(list);
+		newList.merge(list);
 
 		list.clear();