[{"id":16038,"web_url":"https://patchwork.libcamera.org/comment/16038/","msgid":"","date":"2021-03-30T00:39:56","subject":"Re: [libcamera-devel] [PATCH v2 3/6] android: Add CameraHalConfig\n\tclass","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Mon, Mar 29, 2021 at 05:28:04PM +0200, Jacopo Mondi wrote:\n> Add a CameraHalConfig class to the Android Camera3 HAL layer.\n\nThere are quite a few comments from v1 that haven't been addressed.\nMaybe that's because my last reply to v1 came after you've sent this\nseries. The comments may also not all be correct, applicable and/or\ndesired, but could you then reply to them to explain why they're ignored\n?\n\nIn particular, I think that exposing CameraProps would simplify the\nimplementation quite a bit, by removing the need for the camera*()\nfunctions, with the lookup of the CameraProps entry every time, and the\nassociated error handling.\n\n> Signed-off-by: Jacopo Mondi \n> ---\n> README.rst | 2 +-\n> src/android/camera_hal_config.cpp | 463 ++++++++++++++++++++++++++++++\n> src/android/camera_hal_config.h | 53 ++++\n> src/android/meson.build | 2 +\n> 4 files changed, 519 insertions(+), 1 deletion(-)\n> create mode 100644 src/android/camera_hal_config.cpp\n> create mode 100644 src/android/camera_hal_config.h\n> \n> diff --git a/README.rst b/README.rst\n> index 7a98164bbb0a..f68d435196de 100644\n> --- a/README.rst\n> +++ b/README.rst\n> @@ -88,7 +88,7 @@ for tracing with lttng: [optional]\n> liblttng-ust-dev python3-jinja2 lttng-tools\n> \n> for android: [optional]\n> - libexif libjpeg\n> + libexif libjpeg libyaml\n> \n> Using GStreamer plugin\n> ~~~~~~~~~~~~~~~~~~~~~~\n> diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp\n> new file mode 100644\n> index 000000000000..846dd7357b0a\n> --- /dev/null\n> +++ b/src/android/camera_hal_config.cpp\n> @@ -0,0 +1,463 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * camera_hal_config.cpp - Camera HAL configuration file manager\n> + */\n> +#include \"camera_hal_config.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \"libcamera/internal/file.h\"\n> +#include \"libcamera/internal/log.h\"\n> +\n> +using namespace libcamera;\n> +\n> +LOG_DEFINE_CATEGORY(HALConfig)\n> +\n> +CameraHalConfig::CameraHalConfig()\n> +{\n> +\tif (!yaml_parser_initialize(&parser_))\n> +\t\tLOG(HALConfig, Fatal) << \"Failed to initialize yaml parser\";\n> +}\n> +\n> +CameraHalConfig::~CameraHalConfig()\n> +{\n> +\tyaml_parser_delete(&parser_);\n> +}\n> +\n> +/*\n> + * Configuration files can be stored in system paths, which are identified\n> + * through the build configuration.\n> + *\n> + * However, when running uninstalled - the source location takes precedence.\n> + */\n> +std::string CameraHalConfig::findFilePath(const std::string &filename) const\n> +{\n> +\tstatic std::array searchPaths = {\n> +\t\tLIBCAMERA_SYSCONF_DIR,\n> +\t\tLIBCAMERA_DATA_DIR,\n\nThere's also a comment in v1 about data dir. Data dir is typically\n/usr/share/libcamera/. Given that the configuration file is\ndevice-specific, I don't think we'll ever look for it there, but only in\nsysconf dir.\n\n> +\t};\n> +\n> +\tif (File::exists(filename))\n> +\t\treturn filename;\n> +\n> +\tstd::string root = utils::libcameraSourcePath();\n> +\tif (!root.empty()) {\n> +\t\tstd::string configurationPath = root + \"data/\" + filename;\n> +\t\tif (File::exists(configurationPath))\n> +\t\t\treturn configurationPath;\n> +\t}\n> +\n> +\tfor (std::string &path : searchPaths) {\n> +\t\tstd::string configurationPath = path + \"/\" + filename;\n> +\t\tif (File::exists(configurationPath))\n> +\t\t\treturn configurationPath;\n> +\t}\n> +\n> +\treturn \"\";\n> +}\n> +\n> +FILE *CameraHalConfig::openFile(const std::string &filename)\n> +{\n> +\tconst std::string filePath = findFilePath(filename);\n> +\tif (filePath.empty()) {\n> +\t\tLOG(HALConfig, Error)\n> +\t\t\t<< \"Configuration file: \\\"\" << filename << \"\\\" not found\";\n> +\t\treturn nullptr;\n> +\t}\n> +\n> +\tLOG(HALConfig, Debug) << \"Reading configuration file from \" << filePath;\n> +\n> +\tFILE *fh = fopen(filePath.c_str(), \"r\");\n> +\tif (!fh) {\n> +\t\tint ret = -errno;\n> +\t\tLOG(HALConfig, Error) << \"Failed to open configuration file \"\n> +\t\t\t\t << filePath << \": \" << strerror(-ret);\n> +\t\treturn nullptr;\n> +\t}\n> +\n> +\treturn fh;\n> +}\n> +\n> +/*\n> + * Open the HAL configuration file and validate its content.\n> + * Return 0 on success, a negative error code otherwise\n> + */\n> +int CameraHalConfig::open()\n> +{\n> +\tint ret;\n> +\n> +\tFILE *fh = openFile(\"camera_hal.yaml\");\n> +\tif (!fh)\n> +\t\treturn -ENOENT;\n> +\n> +\tyaml_parser_set_input_file(&parser_, fh);\n> +\tret = parseConfigFile();\n> +\tfclose(fh);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tLOG(HALConfig, Info) << \"Device model: \" << model_;\n> +\tLOG(HALConfig, Info) << \"Device maker: \" << maker_;\n> +\tfor (const auto &c : cameras_) {\n> +\t\tconst std::string &cameraId = c.first;\n> +\t\tconst CameraProps &camera = c.second;\n> +\t\tLOG(HALConfig, Info) << \"'\" << cameraId << \"' \"\n> +\t\t\t\t << \" model: \" << camera.model\n> +\t\t\t\t << \"(\" << camera.location << \")[\"\n> +\t\t\t\t << camera.rotation << \"]\";\n> +\t}\n\nThere's also a comment in v1 about whether this should be a debugging\nfeature.\n\n> +\n> +\treturn 0;\n> +}\n> +\n> +int CameraHalConfig::cameraLocation(const std::string &camera) const\n> +{\n> +\tconst auto &it = cameras_.find(camera);\n> +\tif (it == cameras_.end()) {\n> +\t\tLOG(HALConfig, Error)\n> +\t\t\t<< \"Camera '\" << camera\n> +\t\t\t<< \"' not described in the HAL configuration file\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tconst CameraProps &cam = it->second;\n> +\tif (cam.location.empty()) {\n> +\t\tLOG(HALConfig, Error) << \"Location for camera '\" << camera\n> +\t\t\t\t << \"' not available\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tif (cam.location == \"front\")\n> +\t\treturn CAMERA_FACING_FRONT;\n> +\telse if (cam.location == \"back\")\n> +\t\treturn CAMERA_FACING_BACK;\n> +\telse if (cam.location == \"external\")\n> +\t\treturn CAMERA_FACING_EXTERNAL;\n> +\n> +\tLOG(HALConfig, Error) << \"Unsupported camera location \" << cam.location;\n> +\treturn -EINVAL;\n\nCan we do this conversion when parsing the configuration file instead ?\nThat will ensure at parse time that the data is valid, instead of\ndelaying the check to later. The rotation is already handled that way,\nand you already check that the location string is valid in\nCameraHalConfig::parseCameraProps().\n\n> +}\n> +\n> +unsigned int CameraHalConfig::cameraRotation(const std::string &camera) const\n> +{\n> +\tconst auto &it = cameras_.find(camera);\n> +\tif (it == cameras_.end()) {\n> +\t\tLOG(HALConfig, Error)\n> +\t\t\t<< \"Camera '\" << camera\n> +\t\t\t<< \"' not described in the HAL configuration file\";\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tconst CameraProps &cam = it->second;\n> +\treturn cam.rotation;\n> +}\n> +\n> +const std::string &CameraHalConfig::cameraModel(const std::string &camera) const\n> +{\n> +\tstatic const std::string empty(\"\");\n> +\tconst auto &it = cameras_.find(camera);\n> +\tif (it == cameras_.end()) {\n> +\t\tLOG(HALConfig, Error)\n> +\t\t\t<< \"Camera '\" << camera\n> +\t\t\t<< \"' not described in the HAL configuration file\";\n> +\t\treturn empty;\n> +\t}\n> +\n> +\tconst CameraProps &cam = it->second;\n> +\treturn cam.model;\n> +}\n> +\n> +std::string CameraHalConfig::parseValue(yaml_token_t &token)\n> +{\n> +\t/* Make sure the token type is a value and get its content. */\n> +\tyaml_parser_scan(&parser_, &token);\n> +\tif (token.type != YAML_VALUE_TOKEN) {\n> +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> +\t\treturn \"\";\n> +\t}\n> +\tyaml_token_delete(&token);\n\nDoes the token need to be deleted in the error cases above and below ?\n\n> +\n> +\tyaml_parser_scan(&parser_, &token);\n> +\tif (token.type != YAML_SCALAR_TOKEN) {\n> +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> +\t\treturn \"\";\n> +\t}\n> +\n> +\tchar *scalar = reinterpret_cast(token.data.scalar.value);\n> +\tstd::string value(scalar, token.data.scalar.length);\n> +\tyaml_token_delete(&token);\n> +\n> +\treturn value;\n> +}\n> +\n> +std::string CameraHalConfig::parseKey(yaml_token_t &token)\n> +{\n> +\t/* Make sure the token type is a key and get its value. */\n> +\tyaml_parser_scan(&parser_, &token);\n> +\tif (token.type != YAML_SCALAR_TOKEN) {\n> +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> +\t\treturn \"\";\n> +\t}\n> +\n> +\tchar *scalar = reinterpret_cast(token.data.scalar.value);\n> +\tstd::string key(scalar, token.data.scalar.length);\n> +\tyaml_token_delete(&token);\n> +\n> +\treturn key;\n> +}\n> +\n> +int CameraHalConfig::parseCameraProps(yaml_token_t &token,\n> +\t\t\t\t const std::string &cameraID)\n> +{\n> +\tyaml_parser_scan(&parser_, &token);\n> +\tif (token.type != YAML_VALUE_TOKEN) {\n> +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\tyaml_token_delete(&token);\n> +\n> +\tyaml_parser_scan(&parser_, &token);\n> +\tif (token.type != YAML_BLOCK_MAPPING_START_TOKEN) {\n> +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\tyaml_token_delete(&token);\n> +\n> +\t/*\n> +\t * Parse the camera properties and store them in a cameraBlock instance.\n> +\t *\n> +\t * Add a safety counter to make sure we don't loop indefinitely in case\n> +\t * the configuration file is malformed.\n> +\t */\n> +\tunsigned int sentinel = 100;\n> +\tCameraProps cameraBlock{};\n> +\tbool blockEnd = false;\n> +\tdo {\n> +\t\tyaml_parser_scan(&parser_, &token);\n> +\t\tswitch (token.type) {\n> +\t\tcase YAML_KEY_TOKEN: {\n> +\t\t\tyaml_token_delete(&token);\n> +\n> +\t\t\t/*\n> +\t\t\t * Parse the camera property key and make sure it is\n> +\t\t\t * valid.\n> +\t\t\t */\n> +\t\t\tstd::string key = parseKey(token);\n> +\t\t\tstd::string value = parseValue(token);\n> +\t\t\tif (key.empty() || value.empty()) {\n> +\t\t\t\tLOG(HALConfig, Error)\n> +\t\t\t\t\t<< \"Configuration file is not valid\";\n> +\t\t\t\treturn -EINVAL;\n> +\t\t\t}\n> +\n> +\t\t\tif (key == \"location\") {\n> +\t\t\t\tif (value != \"front\" && value != \"back\" &&\n> +\t\t\t\t value != \"external\") {\n> +\t\t\t\t\tLOG(HALConfig, Error)\n> +\t\t\t\t\t\t<< \"Unknown location: \" << value;\n> +\t\t\t\t\treturn -EINVAL;\n> +\t\t\t\t}\n> +\t\t\t\tcameraBlock.location = value;\n> +\t\t\t} else if (key == \"rotation\") {\n> +\t\t\t\tcameraBlock.rotation = strtoul(value.c_str(),\n> +\t\t\t\t\t\t\t NULL, 10);\n> +\t\t\t\tif (cameraBlock.rotation < 0) {\n> +\t\t\t\t\tLOG(HALConfig, Error)\n> +\t\t\t\t\t\t<< \"Unknown rotation: \"\n> +\t\t\t\t\t\t<< cameraBlock.rotation;\n> +\t\t\t\t\treturn -EINVAL;\n> +\t\t\t\t}\n> +\t\t\t} else if (key == \"model\") {\n> +\t\t\t\tcameraBlock.model = value;\n> +\t\t\t} else {\n> +\t\t\t\tLOG(HALConfig, Error)\n> +\t\t\t\t\t<< \"Configuration file is not valid \"\n> +\t\t\t\t\t<< \"unknown key: \" << key;\n> +\t\t\t\treturn -EINVAL;\n> +\t\t\t}\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tcase YAML_BLOCK_END_TOKEN:\n> +\t\t\tyaml_token_delete(&token);\n> +\t\t\tblockEnd = true;\n> +\t\t\tbreak;\n> +\t\tdefault:\n> +\t\t\tyaml_token_delete(&token);\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\t--sentinel;\n> +\t} while (!blockEnd && sentinel);\n\nMissing blank line.\n\n> +\tif (!sentinel) {\n> +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid \";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tcameras_[cameraID] = cameraBlock;\n> +\n> +\treturn 0;\n> +}\n> +\n> +int CameraHalConfig::parseCameras(yaml_token_t &token)\n> +{\n> +\tint ret;\n> +\n> +\t/* The 'cameras' key maps a BLOCK_MAPPING_START block. */\n> +\tyaml_parser_scan(&parser_, &token);\n> +\tif (token.type != YAML_VALUE_TOKEN) {\n> +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\tyaml_token_delete(&token);\n> +\n> +\tyaml_parser_scan(&parser_, &token);\n> +\tif (token.type != YAML_BLOCK_MAPPING_START_TOKEN) {\n> +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\tyaml_token_delete(&token);\n> +\n> +\t/*\n> +\t * Parse the camera properties.\n> +\t *\n> +\t * Each camera properties block is a list of properties associated\n> +\t * with the ID (as assembled by CameraSensor::generateId()) of the\n> +\t * camera they refer to.\n> +\t *\n> +\t * cameras:\n> +\t * \"camera0 id\":\n> +\t * key: value\n> +\t * key: value\n> +\t * ...\n> +\t *\n> +\t * \"camera1 id\":\n> +\t * key: value\n> +\t * key: value\n> +\t * ...\n> +\t */\n> +\tbool blockEnd = false;\n> +\tdo {\n> +\t\tyaml_parser_scan(&parser_, &token);\n> +\t\tswitch (token.type) {\n> +\t\tcase YAML_KEY_TOKEN: {\n> +\t\t\tyaml_token_delete(&token);\n> +\n> +\t\t\t/* Parse the camera ID as key of the property list. */\n> +\t\t\tstd::string cameraId = parseKey(token);\n> +\t\t\tif (cameraId.empty()) {\n> +\t\t\t\tLOG(HALConfig, Error)\n> +\t\t\t\t\t<< \"Configuration file is not valid\";\n> +\t\t\t\treturn -EINVAL;\n> +\t\t\t}\n> +\n> +\t\t\tret = parseCameraProps(token, cameraId);\n> +\t\t\tif (ret) {\n> +\t\t\t\tLOG(HALConfig, Error)\n> +\t\t\t\t\t<< \"Configuration file is not valid\";\n> +\t\t\t\treturn -EINVAL;\n> +\t\t\t}\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tcase YAML_BLOCK_END_TOKEN:\n> +\t\t\tyaml_token_delete(&token);\n> +\t\t\tblockEnd = true;\n> +\t\t\tbreak;\n> +\t\tdefault:\n> +\t\t\tyaml_token_delete(&token);\n> +\t\t\tLOG(HALConfig, Error)\n> +\t\t\t\t<< \"Configuration file is not valid\";\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\t} while (!blockEnd);\n> +\n> +\treturn 0;\n> +}\n> +\n> +int CameraHalConfig::parseEntry(yaml_token_t &token)\n> +{\n> +\tint ret;\n> +\n> +\t/*\n> +\t * Parse each key we find in the file.\n> +\t *\n> +\t * Keys like 'model' and 'maker' are device properties and gets\n> +\t * stored as class members.\n> +\t *\n> +\t * The 'cameras' keys maps to a list of (lists) of camera properties.\n> +\t */\n> +\n> +\tstd::string key = parseKey(token);\n> +\tif (key.empty()) {\n> +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tif (key == \"cameras\") {\n> +\t\tret = parseCameras(token);\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\t} else if (key == \"manufacturer\") {\n> +\t\tmaker_ = parseValue(token);\n> +\t\tif (maker_.empty()) {\n> +\t\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\t} else if (key == \"model\") {\n> +\t\tmodel_ = parseValue(token);\n> +\t\tif (model_.empty()) {\n> +\t\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\t} else {\n> +\t\tLOG(HALConfig, Error) << \"Unknown key \" << key;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +int CameraHalConfig::parseConfigFile()\n> +{\n> +\tyaml_token_t token;\n> +\tint ret;\n> +\n> +\tyaml_parser_scan(&parser_, &token);\n> +\tif (token.type != YAML_STREAM_START_TOKEN) {\n> +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\tyaml_token_delete(&token);\n> +\n> +\tyaml_parser_scan(&parser_, &token);\n> +\tif (token.type != YAML_BLOCK_MAPPING_START_TOKEN) {\n> +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\tyaml_token_delete(&token);\n> +\n> +\t/* Parse the file and parse each single key one by one. */\n> +\tdo {\n> +\t\tyaml_parser_scan(&parser_, &token);\n> +\t\tswitch (token.type) {\n> +\t\tcase YAML_KEY_TOKEN:\n> +\t\t\tyaml_token_delete(&token);\n> +\t\t\tret = parseEntry(token);\n\nDo you need to pass the token to parseEntry(), given that you've just\ndeleted it ? It looks to me like the token is only the return value of\nyaml_parse_scan(), and can thus be a local variable everywhere.\n\n> +\t\t\tbreak;\n> +\t\tcase YAML_STREAM_END_TOKEN:\n> +\t\t\t/* Resources are released after the loop exit. */\n> +\t\t\tbreak;\n> +\t\tdefault:\n> +\t\t\tyaml_token_delete(&token);\n> +\t\t\tbreak;\n\nThe YAML_STREAM_END_TOKEN and default case are different, but the\nwhile() condition below accesses token.type. Is that intended ?\n\n> +\t\t}\n> +\t} while (token.type != YAML_STREAM_END_TOKEN && ret >= 0);\n> +\tyaml_token_delete(&token);\n> +\n> +\treturn ret;\n> +}\n> diff --git a/src/android/camera_hal_config.h b/src/android/camera_hal_config.h\n> new file mode 100644\n> index 000000000000..ce77b1ee1582\n> --- /dev/null\n> +++ b/src/android/camera_hal_config.h\n> @@ -0,0 +1,53 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * camera_hal_config.h - Camera HAL configuration file manager\n> + */\n> +#ifndef __ANDROID_CAMERA_HAL_CONFIG_H__\n> +#define __ANDROID_CAMERA_HAL_CONFIG_H__\n> +\n> +#include \n> +#include \n> +#include \n\nAs commented in v1, hiding the parser would be nice, but it can be done\nlater, on top.\n\n> +\n> +class CameraHalConfig\n> +{\n> +public:\n> +\tCameraHalConfig();\n> +\t~CameraHalConfig();\n> +\n> +\tint open();\n> +\n> +\tconst std::string &deviceModel() const { return model_; }\n> +\tconst std::string &deviceMaker() const { return maker_; }\n> +\n> +\tint cameraLocation(const std::string &camera) const;\n> +\tunsigned int cameraRotation(const std::string &camera) const;\n> +\tconst std::string &cameraModel(const std::string &camera) const;\n> +\n> +private:\n> +\tyaml_parser_t parser_;\n> +\n> +\tstd::string model_;\n> +\tstd::string maker_;\n> +\tclass CameraProps\n> +\t{\n> +\tpublic:\n> +\t\tstd::string location;\n> +\t\tstd::string model;\n> +\t\tunsigned int rotation;\n> +\t};\n> +\tstd::map cameras_;\n> +\n> +\tstd::string findFilePath(const std::string &filename) const;\n> +\tFILE *openFile(const std::string &filename);\n> +\tstd::string parseValue(yaml_token_t &token);\n> +\tstd::string parseKey(yaml_token_t &token);\n> +\tint parseCameraProps(yaml_token_t &token, const std::string &cameraID);\n> +\tint parseCameras(yaml_token_t &token);\n> +\tint parseEntry(yaml_token_t &token);\n> +\tint parseConfigFile();\n> +};\n> +\n> +#endif /* __ANDROID_CAMERA_HAL_CONFIG_H__ */\n> diff --git a/src/android/meson.build b/src/android/meson.build\n> index 8e7d07d9be3c..c0ede407bc38 100644\n> --- a/src/android/meson.build\n> +++ b/src/android/meson.build\n> @@ -3,6 +3,7 @@\n> android_deps = [\n> dependency('libexif', required : get_option('android')),\n> dependency('libjpeg', required : get_option('android')),\n> + dependency('yaml-0.1', required : get_option('android')),\n> ]\n> \n> android_enabled = true\n> @@ -41,6 +42,7 @@ android_hal_sources = files([\n> 'camera3_hal.cpp',\n> 'camera_hal_manager.cpp',\n> 'camera_device.cpp',\n> + 'camera_hal_config.cpp',\n\nWhile at it, could you move camera_hal_manager to the right place ?\n\n> 'camera_metadata.cpp',\n> 'camera_ops.cpp',\n> 'camera_stream.cpp',","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id CD31FC32EA\n\tfor ;\n\tTue, 30 Mar 2021 00:40:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3B80668786;\n\tTue, 30 Mar 2021 02:40:43 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3E622602D7\n\tfor ;\n\tTue, 30 Mar 2021 02:40:41 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 72A16102;\n\tTue, 30 Mar 2021 02:40:40 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Zj9MWtUA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1617064840;\n\tbh=CupUKBEqxPeZJMcLgm1xA8MiB5Xk2Jr+wvUQwqwLQEM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Zj9MWtUA7nt04utvUKL1sezlDXwb/xTk2Jg/2cn944RW8bZLjz3LPYSK0TIyVsm+W\n\tXYetcXUyevwXCO3SU6yZxZr7agCTaVL1z2EZCgtb63HrNJBxPPUUNtgNICnrjPl3lN\n\tiiHJjFvjCks8g6AibIlIGbHxF32ItGaKEBzoGckw=","Date":"Tue, 30 Mar 2021 03:39:56 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210329152807.28331-1-jacopo@jmondi.org>\n\t<20210329152807.28331-3-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210329152807.28331-3-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 3/6] android: Add CameraHalConfig\n\tclass","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16045,"web_url":"https://patchwork.libcamera.org/comment/16045/","msgid":"","date":"2021-03-30T03:48:20","subject":"Re: [libcamera-devel] [PATCH v2 3/6] android: Add CameraHalConfig\n\tclass","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo, Thanks for the patch.\n\nOn Tue, Mar 30, 2021 at 9:40 AM Laurent Pinchart\n wrote:\n>\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Mon, Mar 29, 2021 at 05:28:04PM +0200, Jacopo Mondi wrote:\n> > Add a CameraHalConfig class to the Android Camera3 HAL layer.\n>\n> There are quite a few comments from v1 that haven't been addressed.\n> Maybe that's because my last reply to v1 came after you've sent this\n> series. The comments may also not all be correct, applicable and/or\n> desired, but could you then reply to them to explain why they're ignored\n> ?\n>\n> In particular, I think that exposing CameraProps would simplify the\n> implementation quite a bit, by removing the need for the camera*()\n> functions, with the lookup of the CameraProps entry every time, and the\n> associated error handling.\n>\n\n+1. So we should pass the structure to CameraDevice by value.\n\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > README.rst | 2 +-\n> > src/android/camera_hal_config.cpp | 463 ++++++++++++++++++++++++++++++\n> > src/android/camera_hal_config.h | 53 ++++\n> > src/android/meson.build | 2 +\n> > 4 files changed, 519 insertions(+), 1 deletion(-)\n> > create mode 100644 src/android/camera_hal_config.cpp\n> > create mode 100644 src/android/camera_hal_config.h\n> >\n> > diff --git a/README.rst b/README.rst\n> > index 7a98164bbb0a..f68d435196de 100644\n> > --- a/README.rst\n> > +++ b/README.rst\n> > @@ -88,7 +88,7 @@ for tracing with lttng: [optional]\n> > liblttng-ust-dev python3-jinja2 lttng-tools\n> >\n> > for android: [optional]\n> > - libexif libjpeg\n> > + libexif libjpeg libyaml\n> >\n> > Using GStreamer plugin\n> > ~~~~~~~~~~~~~~~~~~~~~~\n> > diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp\n> > new file mode 100644\n> > index 000000000000..846dd7357b0a\n> > --- /dev/null\n> > +++ b/src/android/camera_hal_config.cpp\n> > @@ -0,0 +1,463 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Google Inc.\n> > + *\n> > + * camera_hal_config.cpp - Camera HAL configuration file manager\n> > + */\n> > +#include \"camera_hal_config.h\"\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"libcamera/internal/file.h\"\n> > +#include \"libcamera/internal/log.h\"\n> > +\n> > +using namespace libcamera;\n> > +\n> > +LOG_DEFINE_CATEGORY(HALConfig)\n> > +\n> > +CameraHalConfig::CameraHalConfig()\n> > +{\n> > + if (!yaml_parser_initialize(&parser_))\n> > + LOG(HALConfig, Fatal) << \"Failed to initialize yaml parser\";\n> > +}\n> > +\n> > +CameraHalConfig::~CameraHalConfig()\n> > +{\n> > + yaml_parser_delete(&parser_);\n> > +}\n> > +\n\nIt looks strange to keep parser_ alive as long as CameraHalConfig,\nbecause parsing is done in open().\nI would initialize the parser in open() and destroy it in the end of open().\nIn this direction, we would prefer moving parse* functions to .cpp\nfile and also moving yaml include to .cpp.\n\nRegards,\n-Hiro\n\n> > +/*\n> > + * Configuration files can be stored in system paths, which are identified\n> > + * through the build configuration.\n> > + *\n> > + * However, when running uninstalled - the source location takes precedence.\n> > + */\n> > +std::string CameraHalConfig::findFilePath(const std::string &filename) const\n> > +{\n> > + static std::array searchPaths = {\n> > + LIBCAMERA_SYSCONF_DIR,\n> > + LIBCAMERA_DATA_DIR,\n>\n> There's also a comment in v1 about data dir. Data dir is typically\n> /usr/share/libcamera/. Given that the configuration file is\n> device-specific, I don't think we'll ever look for it there, but only in\n> sysconf dir.\n>\n> > + };\n> > +\n> > + if (File::exists(filename))\n> > + return filename;\n> > +\n> > + std::string root = utils::libcameraSourcePath();\n> > + if (!root.empty()) {\n> > + std::string configurationPath = root + \"data/\" + filename;\n> > + if (File::exists(configurationPath))\n> > + return configurationPath;\n> > + }\n> > +\n> > + for (std::string &path : searchPaths) {\n> > + std::string configurationPath = path + \"/\" + filename;\n> > + if (File::exists(configurationPath))\n> > + return configurationPath;\n> > + }\n> > +\n> > + return \"\";\n> > +}\n> > +\n> > +FILE *CameraHalConfig::openFile(const std::string &filename)\n> > +{\n> > + const std::string filePath = findFilePath(filename);\n> > + if (filePath.empty()) {\n> > + LOG(HALConfig, Error)\n> > + << \"Configuration file: \\\"\" << filename << \"\\\" not found\";\n> > + return nullptr;\n> > + }\n> > +\n> > + LOG(HALConfig, Debug) << \"Reading configuration file from \" << filePath;\n> > +\n> > + FILE *fh = fopen(filePath.c_str(), \"r\");\n> > + if (!fh) {\n> > + int ret = -errno;\n> > + LOG(HALConfig, Error) << \"Failed to open configuration file \"\n> > + << filePath << \": \" << strerror(-ret);\n> > + return nullptr;\n> > + }\n> > +\n> > + return fh;\n> > +}\n> > +\n> > +/*\n> > + * Open the HAL configuration file and validate its content.\n> > + * Return 0 on success, a negative error code otherwise\n> > + */\n> > +int CameraHalConfig::open()\n> > +{\n> > + int ret;\n> > +\n> > + FILE *fh = openFile(\"camera_hal.yaml\");\n> > + if (!fh)\n> > + return -ENOENT;\n> > +\n> > + yaml_parser_set_input_file(&parser_, fh);\n> > + ret = parseConfigFile();\n> > + fclose(fh);\n> > + if (ret)\n> > + return ret;\n> > +\n> > + LOG(HALConfig, Info) << \"Device model: \" << model_;\n> > + LOG(HALConfig, Info) << \"Device maker: \" << maker_;\n> > + for (const auto &c : cameras_) {\n> > + const std::string &cameraId = c.first;\n> > + const CameraProps &camera = c.second;\n> > + LOG(HALConfig, Info) << \"'\" << cameraId << \"' \"\n> > + << \" model: \" << camera.model\n> > + << \"(\" << camera.location << \")[\"\n> > + << camera.rotation << \"]\";\n> > + }\n>\n> There's also a comment in v1 about whether this should be a debugging\n> feature.\n>\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +int CameraHalConfig::cameraLocation(const std::string &camera) const\n> > +{\n> > + const auto &it = cameras_.find(camera);\n> > + if (it == cameras_.end()) {\n> > + LOG(HALConfig, Error)\n> > + << \"Camera '\" << camera\n> > + << \"' not described in the HAL configuration file\";\n> > + return -EINVAL;\n> > + }\n> > +\n> > + const CameraProps &cam = it->second;\n> > + if (cam.location.empty()) {\n> > + LOG(HALConfig, Error) << \"Location for camera '\" << camera\n> > + << \"' not available\";\n> > + return -EINVAL;\n> > + }\n> > +\n> > + if (cam.location == \"front\")\n> > + return CAMERA_FACING_FRONT;\n> > + else if (cam.location == \"back\")\n> > + return CAMERA_FACING_BACK;\n> > + else if (cam.location == \"external\")\n> > + return CAMERA_FACING_EXTERNAL;\n> > +\n> > + LOG(HALConfig, Error) << \"Unsupported camera location \" << cam.location;\n> > + return -EINVAL;\n>\n> Can we do this conversion when parsing the configuration file instead ?\n> That will ensure at parse time that the data is valid, instead of\n> delaying the check to later. The rotation is already handled that way,\n> and you already check that the location string is valid in\n> CameraHalConfig::parseCameraProps().\n>\n> > +}\n> > +\n> > +unsigned int CameraHalConfig::cameraRotation(const std::string &camera) const\n> > +{\n> > + const auto &it = cameras_.find(camera);\n> > + if (it == cameras_.end()) {\n> > + LOG(HALConfig, Error)\n> > + << \"Camera '\" << camera\n> > + << \"' not described in the HAL configuration file\";\n> > + return 0;\n> > + }\n> > +\n> > + const CameraProps &cam = it->second;\n> > + return cam.rotation;\n> > +}\n> > +\n> > +const std::string &CameraHalConfig::cameraModel(const std::string &camera) const\n> > +{\n> > + static const std::string empty(\"\");\n> > + const auto &it = cameras_.find(camera);\n> > + if (it == cameras_.end()) {\n> > + LOG(HALConfig, Error)\n> > + << \"Camera '\" << camera\n> > + << \"' not described in the HAL configuration file\";\n> > + return empty;\n> > + }\n> > +\n> > + const CameraProps &cam = it->second;\n> > + return cam.model;\n> > +}\n> > +\n> > +std::string CameraHalConfig::parseValue(yaml_token_t &token)\n> > +{\n> > + /* Make sure the token type is a value and get its content. */\n> > + yaml_parser_scan(&parser_, &token);\n> > + if (token.type != YAML_VALUE_TOKEN) {\n> > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > + return \"\";\n> > + }\n> > + yaml_token_delete(&token);\n>\n> Does the token need to be deleted in the error cases above and below ?\n>\n> > +\n> > + yaml_parser_scan(&parser_, &token);\n> > + if (token.type != YAML_SCALAR_TOKEN) {\n> > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > + return \"\";\n> > + }\n> > +\n> > + char *scalar = reinterpret_cast(token.data.scalar.value);\n> > + std::string value(scalar, token.data.scalar.length);\n> > + yaml_token_delete(&token);\n> > +\n> > + return value;\n> > +}\n> > +\n> > +std::string CameraHalConfig::parseKey(yaml_token_t &token)\n> > +{\n> > + /* Make sure the token type is a key and get its value. */\n> > + yaml_parser_scan(&parser_, &token);\n> > + if (token.type != YAML_SCALAR_TOKEN) {\n> > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > + return \"\";\n> > + }\n> > +\n> > + char *scalar = reinterpret_cast(token.data.scalar.value);\n> > + std::string key(scalar, token.data.scalar.length);\n> > + yaml_token_delete(&token);\n> > +\n> > + return key;\n> > +}\n> > +\n> > +int CameraHalConfig::parseCameraProps(yaml_token_t &token,\n> > + const std::string &cameraID)\n> > +{\n> > + yaml_parser_scan(&parser_, &token);\n> > + if (token.type != YAML_VALUE_TOKEN) {\n> > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > + return -EINVAL;\n> > + }\n> > + yaml_token_delete(&token);\n> > +\n> > + yaml_parser_scan(&parser_, &token);\n> > + if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) {\n> > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > + return -EINVAL;\n> > + }\n> > + yaml_token_delete(&token);\n> > +\n> > + /*\n> > + * Parse the camera properties and store them in a cameraBlock instance.\n> > + *\n> > + * Add a safety counter to make sure we don't loop indefinitely in case\n> > + * the configuration file is malformed.\n> > + */\n> > + unsigned int sentinel = 100;\n> > + CameraProps cameraBlock{};\n> > + bool blockEnd = false;\n> > + do {\n> > + yaml_parser_scan(&parser_, &token);\n> > + switch (token.type) {\n> > + case YAML_KEY_TOKEN: {\n> > + yaml_token_delete(&token);\n> > +\n> > + /*\n> > + * Parse the camera property key and make sure it is\n> > + * valid.\n> > + */\n> > + std::string key = parseKey(token);\n> > + std::string value = parseValue(token);\n> > + if (key.empty() || value.empty()) {\n> > + LOG(HALConfig, Error)\n> > + << \"Configuration file is not valid\";\n> > + return -EINVAL;\n> > + }\n> > +\n> > + if (key == \"location\") {\n> > + if (value != \"front\" && value != \"back\" &&\n> > + value != \"external\") {\n> > + LOG(HALConfig, Error)\n> > + << \"Unknown location: \" << value;\n> > + return -EINVAL;\n> > + }\n> > + cameraBlock.location = value;\n> > + } else if (key == \"rotation\") {\n> > + cameraBlock.rotation = strtoul(value.c_str(),\n> > + NULL, 10);\n> > + if (cameraBlock.rotation < 0) {\n> > + LOG(HALConfig, Error)\n> > + << \"Unknown rotation: \"\n> > + << cameraBlock.rotation;\n> > + return -EINVAL;\n> > + }\n> > + } else if (key == \"model\") {\n> > + cameraBlock.model = value;\n> > + } else {\n> > + LOG(HALConfig, Error)\n> > + << \"Configuration file is not valid \"\n> > + << \"unknown key: \" << key;\n> > + return -EINVAL;\n> > + }\n> > + break;\n> > + }\n> > + case YAML_BLOCK_END_TOKEN:\n> > + yaml_token_delete(&token);\n> > + blockEnd = true;\n> > + break;\n> > + default:\n> > + yaml_token_delete(&token);\n> > + break;\n> > + }\n> > +\n> > + --sentinel;\n> > + } while (!blockEnd && sentinel);\n>\n> Missing blank line.\n>\n> > + if (!sentinel) {\n> > + LOG(HALConfig, Error) << \"Configuration file is not valid \";\n> > + return -EINVAL;\n> > + }\n> > +\n> > + cameras_[cameraID] = cameraBlock;\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +int CameraHalConfig::parseCameras(yaml_token_t &token)\n> > +{\n> > + int ret;\n> > +\n> > + /* The 'cameras' key maps a BLOCK_MAPPING_START block. */\n> > + yaml_parser_scan(&parser_, &token);\n> > + if (token.type != YAML_VALUE_TOKEN) {\n> > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > + return -EINVAL;\n> > + }\n> > + yaml_token_delete(&token);\n> > +\n> > + yaml_parser_scan(&parser_, &token);\n> > + if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) {\n> > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > + return -EINVAL;\n> > + }\n> > + yaml_token_delete(&token);\n> > +\n> > + /*\n> > + * Parse the camera properties.\n> > + *\n> > + * Each camera properties block is a list of properties associated\n> > + * with the ID (as assembled by CameraSensor::generateId()) of the\n> > + * camera they refer to.\n> > + *\n> > + * cameras:\n> > + * \"camera0 id\":\n> > + * key: value\n> > + * key: value\n> > + * ...\n> > + *\n> > + * \"camera1 id\":\n> > + * key: value\n> > + * key: value\n> > + * ...\n> > + */\n> > + bool blockEnd = false;\n> > + do {\n> > + yaml_parser_scan(&parser_, &token);\n> > + switch (token.type) {\n> > + case YAML_KEY_TOKEN: {\n> > + yaml_token_delete(&token);\n> > +\n> > + /* Parse the camera ID as key of the property list. */\n> > + std::string cameraId = parseKey(token);\n> > + if (cameraId.empty()) {\n> > + LOG(HALConfig, Error)\n> > + << \"Configuration file is not valid\";\n> > + return -EINVAL;\n> > + }\n> > +\n> > + ret = parseCameraProps(token, cameraId);\n> > + if (ret) {\n> > + LOG(HALConfig, Error)\n> > + << \"Configuration file is not valid\";\n> > + return -EINVAL;\n> > + }\n> > + break;\n> > + }\n> > + case YAML_BLOCK_END_TOKEN:\n> > + yaml_token_delete(&token);\n> > + blockEnd = true;\n> > + break;\n> > + default:\n> > + yaml_token_delete(&token);\n> > + LOG(HALConfig, Error)\n> > + << \"Configuration file is not valid\";\n> > + return -EINVAL;\n> > + }\n> > + } while (!blockEnd);\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +int CameraHalConfig::parseEntry(yaml_token_t &token)\n> > +{\n> > + int ret;\n> > +\n> > + /*\n> > + * Parse each key we find in the file.\n> > + *\n> > + * Keys like 'model' and 'maker' are device properties and gets\n> > + * stored as class members.\n> > + *\n> > + * The 'cameras' keys maps to a list of (lists) of camera properties.\n> > + */\n> > +\n> > + std::string key = parseKey(token);\n> > + if (key.empty()) {\n> > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > + return -EINVAL;\n> > + }\n> > +\n> > + if (key == \"cameras\") {\n> > + ret = parseCameras(token);\n> > + if (ret)\n> > + return ret;\n> > + } else if (key == \"manufacturer\") {\n> > + maker_ = parseValue(token);\n> > + if (maker_.empty()) {\n> > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > + return -EINVAL;\n> > + }\n> > + } else if (key == \"model\") {\n> > + model_ = parseValue(token);\n> > + if (model_.empty()) {\n> > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > + return -EINVAL;\n> > + }\n> > + } else {\n> > + LOG(HALConfig, Error) << \"Unknown key \" << key;\n> > + return -EINVAL;\n> > + }\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +int CameraHalConfig::parseConfigFile()\n> > +{\n> > + yaml_token_t token;\n> > + int ret;\n> > +\n> > + yaml_parser_scan(&parser_, &token);\n> > + if (token.type != YAML_STREAM_START_TOKEN) {\n> > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > + return -EINVAL;\n> > + }\n> > + yaml_token_delete(&token);\n> > +\n> > + yaml_parser_scan(&parser_, &token);\n> > + if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) {\n> > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > + return -EINVAL;\n> > + }\n> > + yaml_token_delete(&token);\n> > +\n> > + /* Parse the file and parse each single key one by one. */\n> > + do {\n> > + yaml_parser_scan(&parser_, &token);\n> > + switch (token.type) {\n> > + case YAML_KEY_TOKEN:\n> > + yaml_token_delete(&token);\n> > + ret = parseEntry(token);\n>\n> Do you need to pass the token to parseEntry(), given that you've just\n> deleted it ? It looks to me like the token is only the return value of\n> yaml_parse_scan(), and can thus be a local variable everywhere.\n>\n> > + break;\n> > + case YAML_STREAM_END_TOKEN:\n> > + /* Resources are released after the loop exit. */\n> > + break;\n> > + default:\n> > + yaml_token_delete(&token);\n> > + break;\n>\n> The YAML_STREAM_END_TOKEN and default case are different, but the\n> while() condition below accesses token.type. Is that intended ?\n>\n> > + }\n> > + } while (token.type != YAML_STREAM_END_TOKEN && ret >= 0);\n> > + yaml_token_delete(&token);\n> > +\n> > + return ret;\n> > +}\n> > diff --git a/src/android/camera_hal_config.h b/src/android/camera_hal_config.h\n> > new file mode 100644\n> > index 000000000000..ce77b1ee1582\n> > --- /dev/null\n> > +++ b/src/android/camera_hal_config.h\n> > @@ -0,0 +1,53 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Google Inc.\n> > + *\n> > + * camera_hal_config.h - Camera HAL configuration file manager\n> > + */\n> > +#ifndef __ANDROID_CAMERA_HAL_CONFIG_H__\n> > +#define __ANDROID_CAMERA_HAL_CONFIG_H__\n> > +\n> > +#include \n> > +#include \n> > +#include \n>\n> As commented in v1, hiding the parser would be nice, but it can be done\n> later, on top.\n>\n> > +\n> > +class CameraHalConfig\n> > +{\n> > +public:\n> > + CameraHalConfig();\n> > + ~CameraHalConfig();\n> > +\n> > + int open();\n> > +\n> > + const std::string &deviceModel() const { return model_; }\n> > + const std::string &deviceMaker() const { return maker_; }\n> > +\n> > + int cameraLocation(const std::string &camera) const;\n> > + unsigned int cameraRotation(const std::string &camera) const;\n> > + const std::string &cameraModel(const std::string &camera) const;\n> > +\n> > +private:\n> > + yaml_parser_t parser_;\n> > +\n> > + std::string model_;\n> > + std::string maker_;\n> > + class CameraProps\n> > + {\n> > + public:\n> > + std::string location;\n> > + std::string model;\n> > + unsigned int rotation;\n> > + };\n> > + std::map cameras_;\n> > +\n> > + std::string findFilePath(const std::string &filename) const;\n> > + FILE *openFile(const std::string &filename);\n> > + std::string parseValue(yaml_token_t &token);\n> > + std::string parseKey(yaml_token_t &token);\n> > + int parseCameraProps(yaml_token_t &token, const std::string &cameraID);\n> > + int parseCameras(yaml_token_t &token);\n> > + int parseEntry(yaml_token_t &token);\n> > + int parseConfigFile();\n> > +};\n> > +\n> > +#endif /* __ANDROID_CAMERA_HAL_CONFIG_H__ */\n> > diff --git a/src/android/meson.build b/src/android/meson.build\n> > index 8e7d07d9be3c..c0ede407bc38 100644\n> > --- a/src/android/meson.build\n> > +++ b/src/android/meson.build\n> > @@ -3,6 +3,7 @@\n> > android_deps = [\n> > dependency('libexif', required : get_option('android')),\n> > dependency('libjpeg', required : get_option('android')),\n> > + dependency('yaml-0.1', required : get_option('android')),\n> > ]\n> >\n> > android_enabled = true\n> > @@ -41,6 +42,7 @@ android_hal_sources = files([\n> > 'camera3_hal.cpp',\n> > 'camera_hal_manager.cpp',\n> > 'camera_device.cpp',\n> > + 'camera_hal_config.cpp',\n>\n> While at it, could you move camera_hal_manager to the right place ?\n>\n> > 'camera_metadata.cpp',\n> > 'camera_ops.cpp',\n> > 'camera_stream.cpp',\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6A790C32EA\n\tfor ;\n\tTue, 30 Mar 2021 03:48:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A5C6568786;\n\tTue, 30 Mar 2021 05:48:35 +0200 (CEST)","from mail-ed1-x52e.google.com (mail-ed1-x52e.google.com\n\t[IPv6:2a00:1450:4864:20::52e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D1C13602D7\n\tfor ;\n\tTue, 30 Mar 2021 05:48:33 +0200 (CEST)","by mail-ed1-x52e.google.com with SMTP id b16so16509193eds.7\n\tfor ;\n\tMon, 29 Mar 2021 20:48:33 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"C4Y4qvqV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=De4JjFUcp6dsPu2/Z8ngsA015T0bw4SXrtz4kC++pDU=;\n\tb=C4Y4qvqVFHvz+C4TdMKPwh7MKA9ZapnEb+2HP2111jkR3Gaf2CB6yfyPP9HkK+SyCr\n\tFqKvh7I/9HJBQvTdp0pAUvTY7Q1Tw0OC7ap3VdT0PWTtX9I6zBFvtdli8lSSGAf4zibe\n\tgfA1vIe4uvXD0vMeg3q1BKkyfskVohSgN8Qeg=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=De4JjFUcp6dsPu2/Z8ngsA015T0bw4SXrtz4kC++pDU=;\n\tb=YxtJavJgYVejBXpZrIY2LM75ImHFY8Kb87NauSEqRLOgssEn5JjrxgUO9Tpk1H3ebR\n\tI4UughLW2nBubfWRlqr1OvAYpa9vbZAkseKQBsV1GjP9/0esVqsdhFjt5JIjXLIImGtX\n\tULDiPHz6Qh2WslTJ5kd2FpIcHV1tASf1BlrsNd0kLg691MBMfTksG6977+ANZ/ctgEnY\n\tk5/zmtn/kZQppoL3tBYOA9k5+a4ifbG9H/yYe5Ur+NLipjPqfyXwkYe0orQNvvhDV/ma\n\t7Tgw6voh7f56lDK897ZGDZAgphMhPMKV9QMxgmRwfEwvxNCH2qhZwS75DoAAMGJhWDpd\n\tjxgg==","X-Gm-Message-State":"AOAM532ViLnzhn+MXpze+Db2mmVzopxgzCpdVs+f0D4c9tiE6rII1hWx\n\teK71AVFoDlh+dKMsBo5SbgbrZisChQew+hpJC8wkyA==","X-Google-Smtp-Source":"ABdhPJwwwm4pCW8NrF00aEYa29O7mqFZ9aS1C5YbD2aUF6F6J9X+LE/nL+yiHk6gPmLzoTGNWBVRNwTmgWQDlIfKV3E=","X-Received":"by 2002:aa7:c4cc:: with SMTP id\n\tp12mr30935783edr.325.1617076113238; \n\tMon, 29 Mar 2021 20:48:33 -0700 (PDT)","MIME-Version":"1.0","References":"<20210329152807.28331-1-jacopo@jmondi.org>\n\t<20210329152807.28331-3-jacopo@jmondi.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Tue, 30 Mar 2021 12:48:20 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v2 3/6] android: Add CameraHalConfig\n\tclass","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16047,"web_url":"https://patchwork.libcamera.org/comment/16047/","msgid":"","date":"2021-03-30T03:52:37","subject":"Re: [libcamera-devel] [PATCH v2 3/6] android: Add CameraHalConfig\n\tclass","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Tue, Mar 30, 2021 at 12:48:20PM +0900, Hirokazu Honda wrote:\n> On Tue, Mar 30, 2021 at 9:40 AM Laurent Pinchartwrote:\n> > On Mon, Mar 29, 2021 at 05:28:04PM +0200, Jacopo Mondi wrote:\n> > > Add a CameraHalConfig class to the Android Camera3 HAL layer.\n> >\n> > There are quite a few comments from v1 that haven't been addressed.\n> > Maybe that's because my last reply to v1 came after you've sent this\n> > series. The comments may also not all be correct, applicable and/or\n> > desired, but could you then reply to them to explain why they're ignored\n> > ?\n> >\n> > In particular, I think that exposing CameraProps would simplify the\n> > implementation quite a bit, by removing the need for the camera*()\n> > functions, with the lookup of the CameraProps entry every time, and the\n> > associated error handling.\n> \n> +1. So we should pass the structure to CameraDevice by value.\n\nWhy not by const reference ?\n\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > README.rst | 2 +-\n> > > src/android/camera_hal_config.cpp | 463 ++++++++++++++++++++++++++++++\n> > > src/android/camera_hal_config.h | 53 ++++\n> > > src/android/meson.build | 2 +\n> > > 4 files changed, 519 insertions(+), 1 deletion(-)\n> > > create mode 100644 src/android/camera_hal_config.cpp\n> > > create mode 100644 src/android/camera_hal_config.h\n> > >\n> > > diff --git a/README.rst b/README.rst\n> > > index 7a98164bbb0a..f68d435196de 100644\n> > > --- a/README.rst\n> > > +++ b/README.rst\n> > > @@ -88,7 +88,7 @@ for tracing with lttng: [optional]\n> > > liblttng-ust-dev python3-jinja2 lttng-tools\n> > >\n> > > for android: [optional]\n> > > - libexif libjpeg\n> > > + libexif libjpeg libyaml\n> > >\n> > > Using GStreamer plugin\n> > > ~~~~~~~~~~~~~~~~~~~~~~\n> > > diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp\n> > > new file mode 100644\n> > > index 000000000000..846dd7357b0a\n> > > --- /dev/null\n> > > +++ b/src/android/camera_hal_config.cpp\n> > > @@ -0,0 +1,463 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2021, Google Inc.\n> > > + *\n> > > + * camera_hal_config.cpp - Camera HAL configuration file manager\n> > > + */\n> > > +#include \"camera_hal_config.h\"\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \"libcamera/internal/file.h\"\n> > > +#include \"libcamera/internal/log.h\"\n> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +LOG_DEFINE_CATEGORY(HALConfig)\n> > > +\n> > > +CameraHalConfig::CameraHalConfig()\n> > > +{\n> > > + if (!yaml_parser_initialize(&parser_))\n> > > + LOG(HALConfig, Fatal) << \"Failed to initialize yaml parser\";\n> > > +}\n> > > +\n> > > +CameraHalConfig::~CameraHalConfig()\n> > > +{\n> > > + yaml_parser_delete(&parser_);\n> > > +}\n> > > +\n> \n> It looks strange to keep parser_ alive as long as CameraHalConfig,\n> because parsing is done in open().\n> I would initialize the parser in open() and destroy it in the end of open().\n> In this direction, we would prefer moving parse* functions to .cpp\n> file and also moving yaml include to .cpp.\n\nI've mentioned that before :-) It could be done on top though.\n\n> > > +/*\n> > > + * Configuration files can be stored in system paths, which are identified\n> > > + * through the build configuration.\n> > > + *\n> > > + * However, when running uninstalled - the source location takes precedence.\n> > > + */\n> > > +std::string CameraHalConfig::findFilePath(const std::string &filename) const\n> > > +{\n> > > + static std::array searchPaths = {\n> > > + LIBCAMERA_SYSCONF_DIR,\n> > > + LIBCAMERA_DATA_DIR,\n> >\n> > There's also a comment in v1 about data dir. Data dir is typically\n> > /usr/share/libcamera/. Given that the configuration file is\n> > device-specific, I don't think we'll ever look for it there, but only in\n> > sysconf dir.\n> >\n> > > + };\n> > > +\n> > > + if (File::exists(filename))\n> > > + return filename;\n> > > +\n> > > + std::string root = utils::libcameraSourcePath();\n> > > + if (!root.empty()) {\n> > > + std::string configurationPath = root + \"data/\" + filename;\n> > > + if (File::exists(configurationPath))\n> > > + return configurationPath;\n> > > + }\n> > > +\n> > > + for (std::string &path : searchPaths) {\n> > > + std::string configurationPath = path + \"/\" + filename;\n> > > + if (File::exists(configurationPath))\n> > > + return configurationPath;\n> > > + }\n> > > +\n> > > + return \"\";\n> > > +}\n> > > +\n> > > +FILE *CameraHalConfig::openFile(const std::string &filename)\n> > > +{\n> > > + const std::string filePath = findFilePath(filename);\n> > > + if (filePath.empty()) {\n> > > + LOG(HALConfig, Error)\n> > > + << \"Configuration file: \\\"\" << filename << \"\\\" not found\";\n> > > + return nullptr;\n> > > + }\n> > > +\n> > > + LOG(HALConfig, Debug) << \"Reading configuration file from \" << filePath;\n> > > +\n> > > + FILE *fh = fopen(filePath.c_str(), \"r\");\n> > > + if (!fh) {\n> > > + int ret = -errno;\n> > > + LOG(HALConfig, Error) << \"Failed to open configuration file \"\n> > > + << filePath << \": \" << strerror(-ret);\n> > > + return nullptr;\n> > > + }\n> > > +\n> > > + return fh;\n> > > +}\n> > > +\n> > > +/*\n> > > + * Open the HAL configuration file and validate its content.\n> > > + * Return 0 on success, a negative error code otherwise\n> > > + */\n> > > +int CameraHalConfig::open()\n> > > +{\n> > > + int ret;\n> > > +\n> > > + FILE *fh = openFile(\"camera_hal.yaml\");\n> > > + if (!fh)\n> > > + return -ENOENT;\n> > > +\n> > > + yaml_parser_set_input_file(&parser_, fh);\n> > > + ret = parseConfigFile();\n> > > + fclose(fh);\n> > > + if (ret)\n> > > + return ret;\n> > > +\n> > > + LOG(HALConfig, Info) << \"Device model: \" << model_;\n> > > + LOG(HALConfig, Info) << \"Device maker: \" << maker_;\n> > > + for (const auto &c : cameras_) {\n> > > + const std::string &cameraId = c.first;\n> > > + const CameraProps &camera = c.second;\n> > > + LOG(HALConfig, Info) << \"'\" << cameraId << \"' \"\n> > > + << \" model: \" << camera.model\n> > > + << \"(\" << camera.location << \")[\"\n> > > + << camera.rotation << \"]\";\n> > > + }\n> >\n> > There's also a comment in v1 about whether this should be a debugging\n> > feature.\n> >\n> > > +\n> > > + return 0;\n> > > +}\n> > > +\n> > > +int CameraHalConfig::cameraLocation(const std::string &camera) const\n> > > +{\n> > > + const auto &it = cameras_.find(camera);\n> > > + if (it == cameras_.end()) {\n> > > + LOG(HALConfig, Error)\n> > > + << \"Camera '\" << camera\n> > > + << \"' not described in the HAL configuration file\";\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + const CameraProps &cam = it->second;\n> > > + if (cam.location.empty()) {\n> > > + LOG(HALConfig, Error) << \"Location for camera '\" << camera\n> > > + << \"' not available\";\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + if (cam.location == \"front\")\n> > > + return CAMERA_FACING_FRONT;\n> > > + else if (cam.location == \"back\")\n> > > + return CAMERA_FACING_BACK;\n> > > + else if (cam.location == \"external\")\n> > > + return CAMERA_FACING_EXTERNAL;\n> > > +\n> > > + LOG(HALConfig, Error) << \"Unsupported camera location \" << cam.location;\n> > > + return -EINVAL;\n> >\n> > Can we do this conversion when parsing the configuration file instead ?\n> > That will ensure at parse time that the data is valid, instead of\n> > delaying the check to later. The rotation is already handled that way,\n> > and you already check that the location string is valid in\n> > CameraHalConfig::parseCameraProps().\n> >\n> > > +}\n> > > +\n> > > +unsigned int CameraHalConfig::cameraRotation(const std::string &camera) const\n> > > +{\n> > > + const auto &it = cameras_.find(camera);\n> > > + if (it == cameras_.end()) {\n> > > + LOG(HALConfig, Error)\n> > > + << \"Camera '\" << camera\n> > > + << \"' not described in the HAL configuration file\";\n> > > + return 0;\n> > > + }\n> > > +\n> > > + const CameraProps &cam = it->second;\n> > > + return cam.rotation;\n> > > +}\n> > > +\n> > > +const std::string &CameraHalConfig::cameraModel(const std::string &camera) const\n> > > +{\n> > > + static const std::string empty(\"\");\n> > > + const auto &it = cameras_.find(camera);\n> > > + if (it == cameras_.end()) {\n> > > + LOG(HALConfig, Error)\n> > > + << \"Camera '\" << camera\n> > > + << \"' not described in the HAL configuration file\";\n> > > + return empty;\n> > > + }\n> > > +\n> > > + const CameraProps &cam = it->second;\n> > > + return cam.model;\n> > > +}\n> > > +\n> > > +std::string CameraHalConfig::parseValue(yaml_token_t &token)\n> > > +{\n> > > + /* Make sure the token type is a value and get its content. */\n> > > + yaml_parser_scan(&parser_, &token);\n> > > + if (token.type != YAML_VALUE_TOKEN) {\n> > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > + return \"\";\n> > > + }\n> > > + yaml_token_delete(&token);\n> >\n> > Does the token need to be deleted in the error cases above and below ?\n> >\n> > > +\n> > > + yaml_parser_scan(&parser_, &token);\n> > > + if (token.type != YAML_SCALAR_TOKEN) {\n> > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > + return \"\";\n> > > + }\n> > > +\n> > > + char *scalar = reinterpret_cast(token.data.scalar.value);\n> > > + std::string value(scalar, token.data.scalar.length);\n> > > + yaml_token_delete(&token);\n> > > +\n> > > + return value;\n> > > +}\n> > > +\n> > > +std::string CameraHalConfig::parseKey(yaml_token_t &token)\n> > > +{\n> > > + /* Make sure the token type is a key and get its value. */\n> > > + yaml_parser_scan(&parser_, &token);\n> > > + if (token.type != YAML_SCALAR_TOKEN) {\n> > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > + return \"\";\n> > > + }\n> > > +\n> > > + char *scalar = reinterpret_cast(token.data.scalar.value);\n> > > + std::string key(scalar, token.data.scalar.length);\n> > > + yaml_token_delete(&token);\n> > > +\n> > > + return key;\n> > > +}\n> > > +\n> > > +int CameraHalConfig::parseCameraProps(yaml_token_t &token,\n> > > + const std::string &cameraID)\n> > > +{\n> > > + yaml_parser_scan(&parser_, &token);\n> > > + if (token.type != YAML_VALUE_TOKEN) {\n> > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > + return -EINVAL;\n> > > + }\n> > > + yaml_token_delete(&token);\n> > > +\n> > > + yaml_parser_scan(&parser_, &token);\n> > > + if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) {\n> > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > + return -EINVAL;\n> > > + }\n> > > + yaml_token_delete(&token);\n> > > +\n> > > + /*\n> > > + * Parse the camera properties and store them in a cameraBlock instance.\n> > > + *\n> > > + * Add a safety counter to make sure we don't loop indefinitely in case\n> > > + * the configuration file is malformed.\n> > > + */\n> > > + unsigned int sentinel = 100;\n> > > + CameraProps cameraBlock{};\n> > > + bool blockEnd = false;\n> > > + do {\n> > > + yaml_parser_scan(&parser_, &token);\n> > > + switch (token.type) {\n> > > + case YAML_KEY_TOKEN: {\n> > > + yaml_token_delete(&token);\n> > > +\n> > > + /*\n> > > + * Parse the camera property key and make sure it is\n> > > + * valid.\n> > > + */\n> > > + std::string key = parseKey(token);\n> > > + std::string value = parseValue(token);\n> > > + if (key.empty() || value.empty()) {\n> > > + LOG(HALConfig, Error)\n> > > + << \"Configuration file is not valid\";\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + if (key == \"location\") {\n> > > + if (value != \"front\" && value != \"back\" &&\n> > > + value != \"external\") {\n> > > + LOG(HALConfig, Error)\n> > > + << \"Unknown location: \" << value;\n> > > + return -EINVAL;\n> > > + }\n> > > + cameraBlock.location = value;\n> > > + } else if (key == \"rotation\") {\n> > > + cameraBlock.rotation = strtoul(value.c_str(),\n> > > + NULL, 10);\n> > > + if (cameraBlock.rotation < 0) {\n> > > + LOG(HALConfig, Error)\n> > > + << \"Unknown rotation: \"\n> > > + << cameraBlock.rotation;\n> > > + return -EINVAL;\n> > > + }\n> > > + } else if (key == \"model\") {\n> > > + cameraBlock.model = value;\n> > > + } else {\n> > > + LOG(HALConfig, Error)\n> > > + << \"Configuration file is not valid \"\n> > > + << \"unknown key: \" << key;\n> > > + return -EINVAL;\n> > > + }\n> > > + break;\n> > > + }\n> > > + case YAML_BLOCK_END_TOKEN:\n> > > + yaml_token_delete(&token);\n> > > + blockEnd = true;\n> > > + break;\n> > > + default:\n> > > + yaml_token_delete(&token);\n> > > + break;\n> > > + }\n> > > +\n> > > + --sentinel;\n> > > + } while (!blockEnd && sentinel);\n> >\n> > Missing blank line.\n> >\n> > > + if (!sentinel) {\n> > > + LOG(HALConfig, Error) << \"Configuration file is not valid \";\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + cameras_[cameraID] = cameraBlock;\n> > > +\n> > > + return 0;\n> > > +}\n> > > +\n> > > +int CameraHalConfig::parseCameras(yaml_token_t &token)\n> > > +{\n> > > + int ret;\n> > > +\n> > > + /* The 'cameras' key maps a BLOCK_MAPPING_START block. */\n> > > + yaml_parser_scan(&parser_, &token);\n> > > + if (token.type != YAML_VALUE_TOKEN) {\n> > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > + return -EINVAL;\n> > > + }\n> > > + yaml_token_delete(&token);\n> > > +\n> > > + yaml_parser_scan(&parser_, &token);\n> > > + if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) {\n> > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > + return -EINVAL;\n> > > + }\n> > > + yaml_token_delete(&token);\n> > > +\n> > > + /*\n> > > + * Parse the camera properties.\n> > > + *\n> > > + * Each camera properties block is a list of properties associated\n> > > + * with the ID (as assembled by CameraSensor::generateId()) of the\n> > > + * camera they refer to.\n> > > + *\n> > > + * cameras:\n> > > + * \"camera0 id\":\n> > > + * key: value\n> > > + * key: value\n> > > + * ...\n> > > + *\n> > > + * \"camera1 id\":\n> > > + * key: value\n> > > + * key: value\n> > > + * ...\n> > > + */\n> > > + bool blockEnd = false;\n> > > + do {\n> > > + yaml_parser_scan(&parser_, &token);\n> > > + switch (token.type) {\n> > > + case YAML_KEY_TOKEN: {\n> > > + yaml_token_delete(&token);\n> > > +\n> > > + /* Parse the camera ID as key of the property list. */\n> > > + std::string cameraId = parseKey(token);\n> > > + if (cameraId.empty()) {\n> > > + LOG(HALConfig, Error)\n> > > + << \"Configuration file is not valid\";\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + ret = parseCameraProps(token, cameraId);\n> > > + if (ret) {\n> > > + LOG(HALConfig, Error)\n> > > + << \"Configuration file is not valid\";\n> > > + return -EINVAL;\n> > > + }\n> > > + break;\n> > > + }\n> > > + case YAML_BLOCK_END_TOKEN:\n> > > + yaml_token_delete(&token);\n> > > + blockEnd = true;\n> > > + break;\n> > > + default:\n> > > + yaml_token_delete(&token);\n> > > + LOG(HALConfig, Error)\n> > > + << \"Configuration file is not valid\";\n> > > + return -EINVAL;\n> > > + }\n> > > + } while (!blockEnd);\n> > > +\n> > > + return 0;\n> > > +}\n> > > +\n> > > +int CameraHalConfig::parseEntry(yaml_token_t &token)\n> > > +{\n> > > + int ret;\n> > > +\n> > > + /*\n> > > + * Parse each key we find in the file.\n> > > + *\n> > > + * Keys like 'model' and 'maker' are device properties and gets\n> > > + * stored as class members.\n> > > + *\n> > > + * The 'cameras' keys maps to a list of (lists) of camera properties.\n> > > + */\n> > > +\n> > > + std::string key = parseKey(token);\n> > > + if (key.empty()) {\n> > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + if (key == \"cameras\") {\n> > > + ret = parseCameras(token);\n> > > + if (ret)\n> > > + return ret;\n> > > + } else if (key == \"manufacturer\") {\n> > > + maker_ = parseValue(token);\n> > > + if (maker_.empty()) {\n> > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > + return -EINVAL;\n> > > + }\n> > > + } else if (key == \"model\") {\n> > > + model_ = parseValue(token);\n> > > + if (model_.empty()) {\n> > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > + return -EINVAL;\n> > > + }\n> > > + } else {\n> > > + LOG(HALConfig, Error) << \"Unknown key \" << key;\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + return 0;\n> > > +}\n> > > +\n> > > +int CameraHalConfig::parseConfigFile()\n> > > +{\n> > > + yaml_token_t token;\n> > > + int ret;\n> > > +\n> > > + yaml_parser_scan(&parser_, &token);\n> > > + if (token.type != YAML_STREAM_START_TOKEN) {\n> > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > + return -EINVAL;\n> > > + }\n> > > + yaml_token_delete(&token);\n> > > +\n> > > + yaml_parser_scan(&parser_, &token);\n> > > + if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) {\n> > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > + return -EINVAL;\n> > > + }\n> > > + yaml_token_delete(&token);\n> > > +\n> > > + /* Parse the file and parse each single key one by one. */\n> > > + do {\n> > > + yaml_parser_scan(&parser_, &token);\n> > > + switch (token.type) {\n> > > + case YAML_KEY_TOKEN:\n> > > + yaml_token_delete(&token);\n> > > + ret = parseEntry(token);\n> >\n> > Do you need to pass the token to parseEntry(), given that you've just\n> > deleted it ? It looks to me like the token is only the return value of\n> > yaml_parse_scan(), and can thus be a local variable everywhere.\n> >\n> > > + break;\n> > > + case YAML_STREAM_END_TOKEN:\n> > > + /* Resources are released after the loop exit. */\n> > > + break;\n> > > + default:\n> > > + yaml_token_delete(&token);\n> > > + break;\n> >\n> > The YAML_STREAM_END_TOKEN and default case are different, but the\n> > while() condition below accesses token.type. Is that intended ?\n> >\n> > > + }\n> > > + } while (token.type != YAML_STREAM_END_TOKEN && ret >= 0);\n> > > + yaml_token_delete(&token);\n> > > +\n> > > + return ret;\n> > > +}\n> > > diff --git a/src/android/camera_hal_config.h b/src/android/camera_hal_config.h\n> > > new file mode 100644\n> > > index 000000000000..ce77b1ee1582\n> > > --- /dev/null\n> > > +++ b/src/android/camera_hal_config.h\n> > > @@ -0,0 +1,53 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2021, Google Inc.\n> > > + *\n> > > + * camera_hal_config.h - Camera HAL configuration file manager\n> > > + */\n> > > +#ifndef __ANDROID_CAMERA_HAL_CONFIG_H__\n> > > +#define __ANDROID_CAMERA_HAL_CONFIG_H__\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> >\n> > As commented in v1, hiding the parser would be nice, but it can be done\n> > later, on top.\n> >\n> > > +\n> > > +class CameraHalConfig\n> > > +{\n> > > +public:\n> > > + CameraHalConfig();\n> > > + ~CameraHalConfig();\n> > > +\n> > > + int open();\n> > > +\n> > > + const std::string &deviceModel() const { return model_; }\n> > > + const std::string &deviceMaker() const { return maker_; }\n> > > +\n> > > + int cameraLocation(const std::string &camera) const;\n> > > + unsigned int cameraRotation(const std::string &camera) const;\n> > > + const std::string &cameraModel(const std::string &camera) const;\n> > > +\n> > > +private:\n> > > + yaml_parser_t parser_;\n> > > +\n> > > + std::string model_;\n> > > + std::string maker_;\n> > > + class CameraProps\n> > > + {\n> > > + public:\n> > > + std::string location;\n> > > + std::string model;\n> > > + unsigned int rotation;\n> > > + };\n> > > + std::map cameras_;\n> > > +\n> > > + std::string findFilePath(const std::string &filename) const;\n> > > + FILE *openFile(const std::string &filename);\n> > > + std::string parseValue(yaml_token_t &token);\n> > > + std::string parseKey(yaml_token_t &token);\n> > > + int parseCameraProps(yaml_token_t &token, const std::string &cameraID);\n> > > + int parseCameras(yaml_token_t &token);\n> > > + int parseEntry(yaml_token_t &token);\n> > > + int parseConfigFile();\n> > > +};\n> > > +\n> > > +#endif /* __ANDROID_CAMERA_HAL_CONFIG_H__ */\n> > > diff --git a/src/android/meson.build b/src/android/meson.build\n> > > index 8e7d07d9be3c..c0ede407bc38 100644\n> > > --- a/src/android/meson.build\n> > > +++ b/src/android/meson.build\n> > > @@ -3,6 +3,7 @@\n> > > android_deps = [\n> > > dependency('libexif', required : get_option('android')),\n> > > dependency('libjpeg', required : get_option('android')),\n> > > + dependency('yaml-0.1', required : get_option('android')),\n> > > ]\n> > >\n> > > android_enabled = true\n> > > @@ -41,6 +42,7 @@ android_hal_sources = files([\n> > > 'camera3_hal.cpp',\n> > > 'camera_hal_manager.cpp',\n> > > 'camera_device.cpp',\n> > > + 'camera_hal_config.cpp',\n> >\n> > While at it, could you move camera_hal_manager to the right place ?\n> >\n> > > 'camera_metadata.cpp',\n> > > 'camera_ops.cpp',\n> > > 'camera_stream.cpp',","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9D08AC32F0\n\tfor ;\n\tTue, 30 Mar 2021 03:53:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5D82468786;\n\tTue, 30 Mar 2021 05:53:24 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4DF3C602D7\n\tfor ;\n\tTue, 30 Mar 2021 05:53:22 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BA4F5292;\n\tTue, 30 Mar 2021 05:53:21 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"afYDwTJt\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1617076402;\n\tbh=2/PpXluonggtWSsJmUUCGrHR1lXKFLYkwMOUfXrlR/c=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=afYDwTJtahmYEbggbwyAqtHJIMKS3nSacFvskz4d2q0YHOdj870GrQj7y6P7N6TQX\n\tYZxXUKzjndol5BmclFhA4zaPLepgRELDVrAkhCQLtPb2jR6KiIGRa7WolPdsXRY46f\n\tBPxNuYkLxoTd76LLfmvHrZdfrChx150Dcq6bQ0Ao=","Date":"Tue, 30 Mar 2021 06:52:37 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210329152807.28331-1-jacopo@jmondi.org>\n\t<20210329152807.28331-3-jacopo@jmondi.org>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 3/6] android: Add CameraHalConfig\n\tclass","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16049,"web_url":"https://patchwork.libcamera.org/comment/16049/","msgid":"","date":"2021-03-30T04:16:22","subject":"Re: [libcamera-devel] [PATCH v2 3/6] android: Add CameraHalConfig\n\tclass","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"On Tue, Mar 30, 2021 at 12:53 PM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> On Tue, Mar 30, 2021 at 12:48:20PM +0900, Hirokazu Honda wrote:\n> > On Tue, Mar 30, 2021 at 9:40 AM Laurent Pinchartwrote:\n> > > On Mon, Mar 29, 2021 at 05:28:04PM +0200, Jacopo Mondi wrote:\n> > > > Add a CameraHalConfig class to the Android Camera3 HAL layer.\n> > >\n> > > There are quite a few comments from v1 that haven't been addressed.\n> > > Maybe that's because my last reply to v1 came after you've sent this\n> > > series. The comments may also not all be correct, applicable and/or\n> > > desired, but could you then reply to them to explain why they're ignored\n> > > ?\n> > >\n> > > In particular, I think that exposing CameraProps would simplify the\n> > > implementation quite a bit, by removing the need for the camera*()\n> > > functions, with the lookup of the CameraProps entry every time, and the\n> > > associated error handling.\n> >\n> > +1. So we should pass the structure to CameraDevice by value.\n>\n> Why not by const reference ?\n>\n\nI would not like having const reference in member variables.\nI actually thought this was stated in Google C++ Style Guide, and\nsearched it, but found there was no statement like that.\nSo this is just my preference.\nFor the purpose, I usually use a const pointer.\nConsidering why I don't like having const reference member variables,\none of reasons might be what I don't get used to :p, but also from a\ncaller of the constructor the lifetime of CameraHalConfig is unclear.\nFor instance, CameraDevice(.., config) is called, but a caller has no\nidea when the config should be alive by, while CameraDevice(...,\n&config) tells a caller to keep the config object outlive to\nCameraDevice.\nAgain, this might be my personal preference. But does this description\nmake sense?\n\n-Hiro\n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > README.rst | 2 +-\n> > > > src/android/camera_hal_config.cpp | 463 ++++++++++++++++++++++++++++++\n> > > > src/android/camera_hal_config.h | 53 ++++\n> > > > src/android/meson.build | 2 +\n> > > > 4 files changed, 519 insertions(+), 1 deletion(-)\n> > > > create mode 100644 src/android/camera_hal_config.cpp\n> > > > create mode 100644 src/android/camera_hal_config.h\n> > > >\n> > > > diff --git a/README.rst b/README.rst\n> > > > index 7a98164bbb0a..f68d435196de 100644\n> > > > --- a/README.rst\n> > > > +++ b/README.rst\n> > > > @@ -88,7 +88,7 @@ for tracing with lttng: [optional]\n> > > > liblttng-ust-dev python3-jinja2 lttng-tools\n> > > >\n> > > > for android: [optional]\n> > > > - libexif libjpeg\n> > > > + libexif libjpeg libyaml\n> > > >\n> > > > Using GStreamer plugin\n> > > > ~~~~~~~~~~~~~~~~~~~~~~\n> > > > diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp\n> > > > new file mode 100644\n> > > > index 000000000000..846dd7357b0a\n> > > > --- /dev/null\n> > > > +++ b/src/android/camera_hal_config.cpp\n> > > > @@ -0,0 +1,463 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2021, Google Inc.\n> > > > + *\n> > > > + * camera_hal_config.cpp - Camera HAL configuration file manager\n> > > > + */\n> > > > +#include \"camera_hal_config.h\"\n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \"libcamera/internal/file.h\"\n> > > > +#include \"libcamera/internal/log.h\"\n> > > > +\n> > > > +using namespace libcamera;\n> > > > +\n> > > > +LOG_DEFINE_CATEGORY(HALConfig)\n> > > > +\n> > > > +CameraHalConfig::CameraHalConfig()\n> > > > +{\n> > > > + if (!yaml_parser_initialize(&parser_))\n> > > > + LOG(HALConfig, Fatal) << \"Failed to initialize yaml parser\";\n> > > > +}\n> > > > +\n> > > > +CameraHalConfig::~CameraHalConfig()\n> > > > +{\n> > > > + yaml_parser_delete(&parser_);\n> > > > +}\n> > > > +\n> >\n> > It looks strange to keep parser_ alive as long as CameraHalConfig,\n> > because parsing is done in open().\n> > I would initialize the parser in open() and destroy it in the end of open().\n> > In this direction, we would prefer moving parse* functions to .cpp\n> > file and also moving yaml include to .cpp.\n>\n> I've mentioned that before :-) It could be done on top though.\n>\n> > > > +/*\n> > > > + * Configuration files can be stored in system paths, which are identified\n> > > > + * through the build configuration.\n> > > > + *\n> > > > + * However, when running uninstalled - the source location takes precedence.\n> > > > + */\n> > > > +std::string CameraHalConfig::findFilePath(const std::string &filename) const\n> > > > +{\n> > > > + static std::array searchPaths = {\n> > > > + LIBCAMERA_SYSCONF_DIR,\n> > > > + LIBCAMERA_DATA_DIR,\n> > >\n> > > There's also a comment in v1 about data dir. Data dir is typically\n> > > /usr/share/libcamera/. Given that the configuration file is\n> > > device-specific, I don't think we'll ever look for it there, but only in\n> > > sysconf dir.\n> > >\n> > > > + };\n> > > > +\n> > > > + if (File::exists(filename))\n> > > > + return filename;\n> > > > +\n> > > > + std::string root = utils::libcameraSourcePath();\n> > > > + if (!root.empty()) {\n> > > > + std::string configurationPath = root + \"data/\" + filename;\n> > > > + if (File::exists(configurationPath))\n> > > > + return configurationPath;\n> > > > + }\n> > > > +\n> > > > + for (std::string &path : searchPaths) {\n> > > > + std::string configurationPath = path + \"/\" + filename;\n> > > > + if (File::exists(configurationPath))\n> > > > + return configurationPath;\n> > > > + }\n> > > > +\n> > > > + return \"\";\n> > > > +}\n> > > > +\n> > > > +FILE *CameraHalConfig::openFile(const std::string &filename)\n> > > > +{\n> > > > + const std::string filePath = findFilePath(filename);\n> > > > + if (filePath.empty()) {\n> > > > + LOG(HALConfig, Error)\n> > > > + << \"Configuration file: \\\"\" << filename << \"\\\" not found\";\n> > > > + return nullptr;\n> > > > + }\n> > > > +\n> > > > + LOG(HALConfig, Debug) << \"Reading configuration file from \" << filePath;\n> > > > +\n> > > > + FILE *fh = fopen(filePath.c_str(), \"r\");\n> > > > + if (!fh) {\n> > > > + int ret = -errno;\n> > > > + LOG(HALConfig, Error) << \"Failed to open configuration file \"\n> > > > + << filePath << \": \" << strerror(-ret);\n> > > > + return nullptr;\n> > > > + }\n> > > > +\n> > > > + return fh;\n> > > > +}\n> > > > +\n> > > > +/*\n> > > > + * Open the HAL configuration file and validate its content.\n> > > > + * Return 0 on success, a negative error code otherwise\n> > > > + */\n> > > > +int CameraHalConfig::open()\n> > > > +{\n> > > > + int ret;\n> > > > +\n> > > > + FILE *fh = openFile(\"camera_hal.yaml\");\n> > > > + if (!fh)\n> > > > + return -ENOENT;\n> > > > +\n> > > > + yaml_parser_set_input_file(&parser_, fh);\n> > > > + ret = parseConfigFile();\n> > > > + fclose(fh);\n> > > > + if (ret)\n> > > > + return ret;\n> > > > +\n> > > > + LOG(HALConfig, Info) << \"Device model: \" << model_;\n> > > > + LOG(HALConfig, Info) << \"Device maker: \" << maker_;\n> > > > + for (const auto &c : cameras_) {\n> > > > + const std::string &cameraId = c.first;\n> > > > + const CameraProps &camera = c.second;\n> > > > + LOG(HALConfig, Info) << \"'\" << cameraId << \"' \"\n> > > > + << \" model: \" << camera.model\n> > > > + << \"(\" << camera.location << \")[\"\n> > > > + << camera.rotation << \"]\";\n> > > > + }\n> > >\n> > > There's also a comment in v1 about whether this should be a debugging\n> > > feature.\n> > >\n> > > > +\n> > > > + return 0;\n> > > > +}\n> > > > +\n> > > > +int CameraHalConfig::cameraLocation(const std::string &camera) const\n> > > > +{\n> > > > + const auto &it = cameras_.find(camera);\n> > > > + if (it == cameras_.end()) {\n> > > > + LOG(HALConfig, Error)\n> > > > + << \"Camera '\" << camera\n> > > > + << \"' not described in the HAL configuration file\";\n> > > > + return -EINVAL;\n> > > > + }\n> > > > +\n> > > > + const CameraProps &cam = it->second;\n> > > > + if (cam.location.empty()) {\n> > > > + LOG(HALConfig, Error) << \"Location for camera '\" << camera\n> > > > + << \"' not available\";\n> > > > + return -EINVAL;\n> > > > + }\n> > > > +\n> > > > + if (cam.location == \"front\")\n> > > > + return CAMERA_FACING_FRONT;\n> > > > + else if (cam.location == \"back\")\n> > > > + return CAMERA_FACING_BACK;\n> > > > + else if (cam.location == \"external\")\n> > > > + return CAMERA_FACING_EXTERNAL;\n> > > > +\n> > > > + LOG(HALConfig, Error) << \"Unsupported camera location \" << cam.location;\n> > > > + return -EINVAL;\n> > >\n> > > Can we do this conversion when parsing the configuration file instead ?\n> > > That will ensure at parse time that the data is valid, instead of\n> > > delaying the check to later. The rotation is already handled that way,\n> > > and you already check that the location string is valid in\n> > > CameraHalConfig::parseCameraProps().\n> > >\n> > > > +}\n> > > > +\n> > > > +unsigned int CameraHalConfig::cameraRotation(const std::string &camera) const\n> > > > +{\n> > > > + const auto &it = cameras_.find(camera);\n> > > > + if (it == cameras_.end()) {\n> > > > + LOG(HALConfig, Error)\n> > > > + << \"Camera '\" << camera\n> > > > + << \"' not described in the HAL configuration file\";\n> > > > + return 0;\n> > > > + }\n> > > > +\n> > > > + const CameraProps &cam = it->second;\n> > > > + return cam.rotation;\n> > > > +}\n> > > > +\n> > > > +const std::string &CameraHalConfig::cameraModel(const std::string &camera) const\n> > > > +{\n> > > > + static const std::string empty(\"\");\n> > > > + const auto &it = cameras_.find(camera);\n> > > > + if (it == cameras_.end()) {\n> > > > + LOG(HALConfig, Error)\n> > > > + << \"Camera '\" << camera\n> > > > + << \"' not described in the HAL configuration file\";\n> > > > + return empty;\n> > > > + }\n> > > > +\n> > > > + const CameraProps &cam = it->second;\n> > > > + return cam.model;\n> > > > +}\n> > > > +\n> > > > +std::string CameraHalConfig::parseValue(yaml_token_t &token)\n> > > > +{\n> > > > + /* Make sure the token type is a value and get its content. */\n> > > > + yaml_parser_scan(&parser_, &token);\n> > > > + if (token.type != YAML_VALUE_TOKEN) {\n> > > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > > + return \"\";\n> > > > + }\n> > > > + yaml_token_delete(&token);\n> > >\n> > > Does the token need to be deleted in the error cases above and below ?\n> > >\n> > > > +\n> > > > + yaml_parser_scan(&parser_, &token);\n> > > > + if (token.type != YAML_SCALAR_TOKEN) {\n> > > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > > + return \"\";\n> > > > + }\n> > > > +\n> > > > + char *scalar = reinterpret_cast(token.data.scalar.value);\n> > > > + std::string value(scalar, token.data.scalar.length);\n> > > > + yaml_token_delete(&token);\n> > > > +\n> > > > + return value;\n> > > > +}\n> > > > +\n> > > > +std::string CameraHalConfig::parseKey(yaml_token_t &token)\n> > > > +{\n> > > > + /* Make sure the token type is a key and get its value. */\n> > > > + yaml_parser_scan(&parser_, &token);\n> > > > + if (token.type != YAML_SCALAR_TOKEN) {\n> > > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > > + return \"\";\n> > > > + }\n> > > > +\n> > > > + char *scalar = reinterpret_cast(token.data.scalar.value);\n> > > > + std::string key(scalar, token.data.scalar.length);\n> > > > + yaml_token_delete(&token);\n> > > > +\n> > > > + return key;\n> > > > +}\n> > > > +\n> > > > +int CameraHalConfig::parseCameraProps(yaml_token_t &token,\n> > > > + const std::string &cameraID)\n> > > > +{\n> > > > + yaml_parser_scan(&parser_, &token);\n> > > > + if (token.type != YAML_VALUE_TOKEN) {\n> > > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > > + return -EINVAL;\n> > > > + }\n> > > > + yaml_token_delete(&token);\n> > > > +\n> > > > + yaml_parser_scan(&parser_, &token);\n> > > > + if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) {\n> > > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > > + return -EINVAL;\n> > > > + }\n> > > > + yaml_token_delete(&token);\n> > > > +\n> > > > + /*\n> > > > + * Parse the camera properties and store them in a cameraBlock instance.\n> > > > + *\n> > > > + * Add a safety counter to make sure we don't loop indefinitely in case\n> > > > + * the configuration file is malformed.\n> > > > + */\n> > > > + unsigned int sentinel = 100;\n> > > > + CameraProps cameraBlock{};\n> > > > + bool blockEnd = false;\n> > > > + do {\n> > > > + yaml_parser_scan(&parser_, &token);\n> > > > + switch (token.type) {\n> > > > + case YAML_KEY_TOKEN: {\n> > > > + yaml_token_delete(&token);\n> > > > +\n> > > > + /*\n> > > > + * Parse the camera property key and make sure it is\n> > > > + * valid.\n> > > > + */\n> > > > + std::string key = parseKey(token);\n> > > > + std::string value = parseValue(token);\n> > > > + if (key.empty() || value.empty()) {\n> > > > + LOG(HALConfig, Error)\n> > > > + << \"Configuration file is not valid\";\n> > > > + return -EINVAL;\n> > > > + }\n> > > > +\n> > > > + if (key == \"location\") {\n> > > > + if (value != \"front\" && value != \"back\" &&\n> > > > + value != \"external\") {\n> > > > + LOG(HALConfig, Error)\n> > > > + << \"Unknown location: \" << value;\n> > > > + return -EINVAL;\n> > > > + }\n> > > > + cameraBlock.location = value;\n> > > > + } else if (key == \"rotation\") {\n> > > > + cameraBlock.rotation = strtoul(value.c_str(),\n> > > > + NULL, 10);\n> > > > + if (cameraBlock.rotation < 0) {\n> > > > + LOG(HALConfig, Error)\n> > > > + << \"Unknown rotation: \"\n> > > > + << cameraBlock.rotation;\n> > > > + return -EINVAL;\n> > > > + }\n> > > > + } else if (key == \"model\") {\n> > > > + cameraBlock.model = value;\n> > > > + } else {\n> > > > + LOG(HALConfig, Error)\n> > > > + << \"Configuration file is not valid \"\n> > > > + << \"unknown key: \" << key;\n> > > > + return -EINVAL;\n> > > > + }\n> > > > + break;\n> > > > + }\n> > > > + case YAML_BLOCK_END_TOKEN:\n> > > > + yaml_token_delete(&token);\n> > > > + blockEnd = true;\n> > > > + break;\n> > > > + default:\n> > > > + yaml_token_delete(&token);\n> > > > + break;\n> > > > + }\n> > > > +\n> > > > + --sentinel;\n> > > > + } while (!blockEnd && sentinel);\n> > >\n> > > Missing blank line.\n> > >\n> > > > + if (!sentinel) {\n> > > > + LOG(HALConfig, Error) << \"Configuration file is not valid \";\n> > > > + return -EINVAL;\n> > > > + }\n> > > > +\n> > > > + cameras_[cameraID] = cameraBlock;\n> > > > +\n> > > > + return 0;\n> > > > +}\n> > > > +\n> > > > +int CameraHalConfig::parseCameras(yaml_token_t &token)\n> > > > +{\n> > > > + int ret;\n> > > > +\n> > > > + /* The 'cameras' key maps a BLOCK_MAPPING_START block. */\n> > > > + yaml_parser_scan(&parser_, &token);\n> > > > + if (token.type != YAML_VALUE_TOKEN) {\n> > > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > > + return -EINVAL;\n> > > > + }\n> > > > + yaml_token_delete(&token);\n> > > > +\n> > > > + yaml_parser_scan(&parser_, &token);\n> > > > + if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) {\n> > > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > > + return -EINVAL;\n> > > > + }\n> > > > + yaml_token_delete(&token);\n> > > > +\n> > > > + /*\n> > > > + * Parse the camera properties.\n> > > > + *\n> > > > + * Each camera properties block is a list of properties associated\n> > > > + * with the ID (as assembled by CameraSensor::generateId()) of the\n> > > > + * camera they refer to.\n> > > > + *\n> > > > + * cameras:\n> > > > + * \"camera0 id\":\n> > > > + * key: value\n> > > > + * key: value\n> > > > + * ...\n> > > > + *\n> > > > + * \"camera1 id\":\n> > > > + * key: value\n> > > > + * key: value\n> > > > + * ...\n> > > > + */\n> > > > + bool blockEnd = false;\n> > > > + do {\n> > > > + yaml_parser_scan(&parser_, &token);\n> > > > + switch (token.type) {\n> > > > + case YAML_KEY_TOKEN: {\n> > > > + yaml_token_delete(&token);\n> > > > +\n> > > > + /* Parse the camera ID as key of the property list. */\n> > > > + std::string cameraId = parseKey(token);\n> > > > + if (cameraId.empty()) {\n> > > > + LOG(HALConfig, Error)\n> > > > + << \"Configuration file is not valid\";\n> > > > + return -EINVAL;\n> > > > + }\n> > > > +\n> > > > + ret = parseCameraProps(token, cameraId);\n> > > > + if (ret) {\n> > > > + LOG(HALConfig, Error)\n> > > > + << \"Configuration file is not valid\";\n> > > > + return -EINVAL;\n> > > > + }\n> > > > + break;\n> > > > + }\n> > > > + case YAML_BLOCK_END_TOKEN:\n> > > > + yaml_token_delete(&token);\n> > > > + blockEnd = true;\n> > > > + break;\n> > > > + default:\n> > > > + yaml_token_delete(&token);\n> > > > + LOG(HALConfig, Error)\n> > > > + << \"Configuration file is not valid\";\n> > > > + return -EINVAL;\n> > > > + }\n> > > > + } while (!blockEnd);\n> > > > +\n> > > > + return 0;\n> > > > +}\n> > > > +\n> > > > +int CameraHalConfig::parseEntry(yaml_token_t &token)\n> > > > +{\n> > > > + int ret;\n> > > > +\n> > > > + /*\n> > > > + * Parse each key we find in the file.\n> > > > + *\n> > > > + * Keys like 'model' and 'maker' are device properties and gets\n> > > > + * stored as class members.\n> > > > + *\n> > > > + * The 'cameras' keys maps to a list of (lists) of camera properties.\n> > > > + */\n> > > > +\n> > > > + std::string key = parseKey(token);\n> > > > + if (key.empty()) {\n> > > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > > + return -EINVAL;\n> > > > + }\n> > > > +\n> > > > + if (key == \"cameras\") {\n> > > > + ret = parseCameras(token);\n> > > > + if (ret)\n> > > > + return ret;\n> > > > + } else if (key == \"manufacturer\") {\n> > > > + maker_ = parseValue(token);\n> > > > + if (maker_.empty()) {\n> > > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > > + return -EINVAL;\n> > > > + }\n> > > > + } else if (key == \"model\") {\n> > > > + model_ = parseValue(token);\n> > > > + if (model_.empty()) {\n> > > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > > + return -EINVAL;\n> > > > + }\n> > > > + } else {\n> > > > + LOG(HALConfig, Error) << \"Unknown key \" << key;\n> > > > + return -EINVAL;\n> > > > + }\n> > > > +\n> > > > + return 0;\n> > > > +}\n> > > > +\n> > > > +int CameraHalConfig::parseConfigFile()\n> > > > +{\n> > > > + yaml_token_t token;\n> > > > + int ret;\n> > > > +\n> > > > + yaml_parser_scan(&parser_, &token);\n> > > > + if (token.type != YAML_STREAM_START_TOKEN) {\n> > > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > > + return -EINVAL;\n> > > > + }\n> > > > + yaml_token_delete(&token);\n> > > > +\n> > > > + yaml_parser_scan(&parser_, &token);\n> > > > + if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) {\n> > > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > > + return -EINVAL;\n> > > > + }\n> > > > + yaml_token_delete(&token);\n> > > > +\n> > > > + /* Parse the file and parse each single key one by one. */\n> > > > + do {\n> > > > + yaml_parser_scan(&parser_, &token);\n> > > > + switch (token.type) {\n> > > > + case YAML_KEY_TOKEN:\n> > > > + yaml_token_delete(&token);\n> > > > + ret = parseEntry(token);\n> > >\n> > > Do you need to pass the token to parseEntry(), given that you've just\n> > > deleted it ? It looks to me like the token is only the return value of\n> > > yaml_parse_scan(), and can thus be a local variable everywhere.\n> > >\n> > > > + break;\n> > > > + case YAML_STREAM_END_TOKEN:\n> > > > + /* Resources are released after the loop exit. */\n> > > > + break;\n> > > > + default:\n> > > > + yaml_token_delete(&token);\n> > > > + break;\n> > >\n> > > The YAML_STREAM_END_TOKEN and default case are different, but the\n> > > while() condition below accesses token.type. Is that intended ?\n> > >\n> > > > + }\n> > > > + } while (token.type != YAML_STREAM_END_TOKEN && ret >= 0);\n> > > > + yaml_token_delete(&token);\n> > > > +\n> > > > + return ret;\n> > > > +}\n> > > > diff --git a/src/android/camera_hal_config.h b/src/android/camera_hal_config.h\n> > > > new file mode 100644\n> > > > index 000000000000..ce77b1ee1582\n> > > > --- /dev/null\n> > > > +++ b/src/android/camera_hal_config.h\n> > > > @@ -0,0 +1,53 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2021, Google Inc.\n> > > > + *\n> > > > + * camera_hal_config.h - Camera HAL configuration file manager\n> > > > + */\n> > > > +#ifndef __ANDROID_CAMERA_HAL_CONFIG_H__\n> > > > +#define __ANDROID_CAMERA_HAL_CONFIG_H__\n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > >\n> > > As commented in v1, hiding the parser would be nice, but it can be done\n> > > later, on top.\n> > >\n> > > > +\n> > > > +class CameraHalConfig\n> > > > +{\n> > > > +public:\n> > > > + CameraHalConfig();\n> > > > + ~CameraHalConfig();\n> > > > +\n> > > > + int open();\n> > > > +\n> > > > + const std::string &deviceModel() const { return model_; }\n> > > > + const std::string &deviceMaker() const { return maker_; }\n> > > > +\n> > > > + int cameraLocation(const std::string &camera) const;\n> > > > + unsigned int cameraRotation(const std::string &camera) const;\n> > > > + const std::string &cameraModel(const std::string &camera) const;\n> > > > +\n> > > > +private:\n> > > > + yaml_parser_t parser_;\n> > > > +\n> > > > + std::string model_;\n> > > > + std::string maker_;\n> > > > + class CameraProps\n> > > > + {\n> > > > + public:\n> > > > + std::string location;\n> > > > + std::string model;\n> > > > + unsigned int rotation;\n> > > > + };\n> > > > + std::map cameras_;\n> > > > +\n> > > > + std::string findFilePath(const std::string &filename) const;\n> > > > + FILE *openFile(const std::string &filename);\n> > > > + std::string parseValue(yaml_token_t &token);\n> > > > + std::string parseKey(yaml_token_t &token);\n> > > > + int parseCameraProps(yaml_token_t &token, const std::string &cameraID);\n> > > > + int parseCameras(yaml_token_t &token);\n> > > > + int parseEntry(yaml_token_t &token);\n> > > > + int parseConfigFile();\n> > > > +};\n> > > > +\n> > > > +#endif /* __ANDROID_CAMERA_HAL_CONFIG_H__ */\n> > > > diff --git a/src/android/meson.build b/src/android/meson.build\n> > > > index 8e7d07d9be3c..c0ede407bc38 100644\n> > > > --- a/src/android/meson.build\n> > > > +++ b/src/android/meson.build\n> > > > @@ -3,6 +3,7 @@\n> > > > android_deps = [\n> > > > dependency('libexif', required : get_option('android')),\n> > > > dependency('libjpeg', required : get_option('android')),\n> > > > + dependency('yaml-0.1', required : get_option('android')),\n> > > > ]\n> > > >\n> > > > android_enabled = true\n> > > > @@ -41,6 +42,7 @@ android_hal_sources = files([\n> > > > 'camera3_hal.cpp',\n> > > > 'camera_hal_manager.cpp',\n> > > > 'camera_device.cpp',\n> > > > + 'camera_hal_config.cpp',\n> > >\n> > > While at it, could you move camera_hal_manager to the right place ?\n> > >\n> > > > 'camera_metadata.cpp',\n> > > > 'camera_ops.cpp',\n> > > > 'camera_stream.cpp',\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1834BC32F0\n\tfor ;\n\tTue, 30 Mar 2021 04:16:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 709B468786;\n\tTue, 30 Mar 2021 06:16:37 +0200 (CEST)","from mail-ej1-x636.google.com (mail-ej1-x636.google.com\n\t[IPv6:2a00:1450:4864:20::636])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9A6BB602D7\n\tfor ;\n\tTue, 30 Mar 2021 06:16:36 +0200 (CEST)","by mail-ej1-x636.google.com with SMTP id kt15so22651907ejb.12\n\tfor ;\n\tMon, 29 Mar 2021 21:16:36 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"PQ5MlQzM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=d4akXlrSpZa8kkv3YZj5dqYkr7jzj59TslItzrZM+VU=;\n\tb=PQ5MlQzMC/o1n7hGgh3C2f0+TgSminmWVn2j0xkq64R7U8w7gr0pUIQk0WbEjsPXr1\n\t98I/bUFGDO2ovscg1uvhkZDsEA7m5xXyg8sMbWQVkYfn8JLUn2L2RI+BnfaLBxAsdjB/\n\t9yRcgLbp4TucUnieuNBUr9pXu6lzcbPp9nVMU=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=d4akXlrSpZa8kkv3YZj5dqYkr7jzj59TslItzrZM+VU=;\n\tb=sR9MD+ocz6OhzkWYjueYtnAvNxnO/dUz5rNtpFRTCVAo6s1/QtReEhIki5GVlaLabL\n\tcXrVIOSeFbursa6rBOh1Z0+hGi/BaHRr6SqarF7qwJTJAklnoRb9cn52SU3twi7phEjK\n\tIznQKnl8ykYbfYUF/iRNxIZZWfxg/evH6L0ac3dU5liR+70VFPTxkTywGwzTn6Mrr8HJ\n\tPc/GStzg5GEjSFjWhJgk/YbdI9FNv3KH4hNh9binbruJYeQlXodKh9KEQ78REZqzeVgM\n\tGerIaOXcoelogvGCDC2icpZCczJs9QtdNCQRY2kIY0kNQU9ssZGo9R42e7N5JSaA4t2M\n\tRxXA==","X-Gm-Message-State":"AOAM531cSqnpFUc7oLlt5MifWhKG3swTFvUFCYWrTHT8G0sr+KmkE8yq\n\tdiEm6xPhLqQe5puENdTre26xn9RNYDPSdPGeXCzVKw==","X-Google-Smtp-Source":"ABdhPJzWyWNno/OePY3qu7yzxdg2cRClXwYrue4b+C6eSWAQmwW0yeWRJHV9sHxW7PCP2+iiyTwIIzzfIScjjEWFUYk=","X-Received":"by 2002:a17:907:36e:: with SMTP id\n\trs14mr30696003ejb.42.1617077796059; \n\tMon, 29 Mar 2021 21:16:36 -0700 (PDT)","MIME-Version":"1.0","References":"<20210329152807.28331-1-jacopo@jmondi.org>\n\t<20210329152807.28331-3-jacopo@jmondi.org>\n\t\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Tue, 30 Mar 2021 13:16:22 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v2 3/6] android: Add CameraHalConfig\n\tclass","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16058,"web_url":"https://patchwork.libcamera.org/comment/16058/","msgid":"<20210330103730.iocnmtlexhvaafuj@uno.localdomain>","date":"2021-03-30T10:37:30","subject":"Re: [libcamera-devel] [PATCH v2 3/6] android: Add CameraHalConfig\n\tclass","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Tue, Mar 30, 2021 at 03:39:56AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Mon, Mar 29, 2021 at 05:28:04PM +0200, Jacopo Mondi wrote:\n> > Add a CameraHalConfig class to the Android Camera3 HAL layer.\n>\n> There are quite a few comments from v1 that haven't been addressed.\n> Maybe that's because my last reply to v1 came after you've sent this\n> series. The comments may also not all be correct, applicable and/or\n> desired, but could you then reply to them to explain why they're ignored\n> ?\n>\n> In particular, I think that exposing CameraProps would simplify the\n> implementation quite a bit, by removing the need for the camera*()\n> functions, with the lookup of the CameraProps entry every time, and the\n> associated error handling.\n>\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > README.rst | 2 +-\n> > src/android/camera_hal_config.cpp | 463 ++++++++++++++++++++++++++++++\n> > src/android/camera_hal_config.h | 53 ++++\n> > src/android/meson.build | 2 +\n> > 4 files changed, 519 insertions(+), 1 deletion(-)\n> > create mode 100644 src/android/camera_hal_config.cpp\n> > create mode 100644 src/android/camera_hal_config.h\n> >\n> > diff --git a/README.rst b/README.rst\n> > index 7a98164bbb0a..f68d435196de 100644\n> > --- a/README.rst\n> > +++ b/README.rst\n> > @@ -88,7 +88,7 @@ for tracing with lttng: [optional]\n> > liblttng-ust-dev python3-jinja2 lttng-tools\n> >\n> > for android: [optional]\n> > - libexif libjpeg\n> > + libexif libjpeg libyaml\n> >\n> > Using GStreamer plugin\n> > ~~~~~~~~~~~~~~~~~~~~~~\n> > diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp\n> > new file mode 100644\n> > index 000000000000..846dd7357b0a\n> > --- /dev/null\n> > +++ b/src/android/camera_hal_config.cpp\n> > @@ -0,0 +1,463 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Google Inc.\n> > + *\n> > + * camera_hal_config.cpp - Camera HAL configuration file manager\n> > + */\n> > +#include \"camera_hal_config.h\"\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"libcamera/internal/file.h\"\n> > +#include \"libcamera/internal/log.h\"\n> > +\n> > +using namespace libcamera;\n> > +\n> > +LOG_DEFINE_CATEGORY(HALConfig)\n> > +\n> > +CameraHalConfig::CameraHalConfig()\n> > +{\n> > +\tif (!yaml_parser_initialize(&parser_))\n> > +\t\tLOG(HALConfig, Fatal) << \"Failed to initialize yaml parser\";\n> > +}\n> > +\n> > +CameraHalConfig::~CameraHalConfig()\n> > +{\n> > +\tyaml_parser_delete(&parser_);\n> > +}\n> > +\n> > +/*\n> > + * Configuration files can be stored in system paths, which are identified\n> > + * through the build configuration.\n> > + *\n> > + * However, when running uninstalled - the source location takes precedence.\n> > + */\n> > +std::string CameraHalConfig::findFilePath(const std::string &filename) const\n> > +{\n> > +\tstatic std::array searchPaths = {\n> > +\t\tLIBCAMERA_SYSCONF_DIR,\n> > +\t\tLIBCAMERA_DATA_DIR,\n>\n> There's also a comment in v1 about data dir. Data dir is typically\n> /usr/share/libcamera/. Given that the configuration file is\n> device-specific, I don't think we'll ever look for it there, but only in\n> sysconf dir.\n>\n> > +\t};\n> > +\n> > +\tif (File::exists(filename))\n> > +\t\treturn filename;\n> > +\n> > +\tstd::string root = utils::libcameraSourcePath();\n> > +\tif (!root.empty()) {\n> > +\t\tstd::string configurationPath = root + \"data/\" + filename;\n> > +\t\tif (File::exists(configurationPath))\n> > +\t\t\treturn configurationPath;\n> > +\t}\n> > +\n> > +\tfor (std::string &path : searchPaths) {\n> > +\t\tstd::string configurationPath = path + \"/\" + filename;\n> > +\t\tif (File::exists(configurationPath))\n> > +\t\t\treturn configurationPath;\n> > +\t}\n> > +\n> > +\treturn \"\";\n> > +}\n> > +\n> > +FILE *CameraHalConfig::openFile(const std::string &filename)\n> > +{\n> > +\tconst std::string filePath = findFilePath(filename);\n> > +\tif (filePath.empty()) {\n> > +\t\tLOG(HALConfig, Error)\n> > +\t\t\t<< \"Configuration file: \\\"\" << filename << \"\\\" not found\";\n> > +\t\treturn nullptr;\n> > +\t}\n> > +\n> > +\tLOG(HALConfig, Debug) << \"Reading configuration file from \" << filePath;\n> > +\n> > +\tFILE *fh = fopen(filePath.c_str(), \"r\");\n> > +\tif (!fh) {\n> > +\t\tint ret = -errno;\n> > +\t\tLOG(HALConfig, Error) << \"Failed to open configuration file \"\n> > +\t\t\t\t << filePath << \": \" << strerror(-ret);\n> > +\t\treturn nullptr;\n> > +\t}\n> > +\n> > +\treturn fh;\n> > +}\n> > +\n> > +/*\n> > + * Open the HAL configuration file and validate its content.\n> > + * Return 0 on success, a negative error code otherwise\n> > + */\n> > +int CameraHalConfig::open()\n> > +{\n> > +\tint ret;\n> > +\n> > +\tFILE *fh = openFile(\"camera_hal.yaml\");\n> > +\tif (!fh)\n> > +\t\treturn -ENOENT;\n> > +\n> > +\tyaml_parser_set_input_file(&parser_, fh);\n> > +\tret = parseConfigFile();\n> > +\tfclose(fh);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tLOG(HALConfig, Info) << \"Device model: \" << model_;\n> > +\tLOG(HALConfig, Info) << \"Device maker: \" << maker_;\n> > +\tfor (const auto &c : cameras_) {\n> > +\t\tconst std::string &cameraId = c.first;\n> > +\t\tconst CameraProps &camera = c.second;\n> > +\t\tLOG(HALConfig, Info) << \"'\" << cameraId << \"' \"\n> > +\t\t\t\t << \" model: \" << camera.model\n> > +\t\t\t\t << \"(\" << camera.location << \")[\"\n> > +\t\t\t\t << camera.rotation << \"]\";\n> > +\t}\n>\n> There's also a comment in v1 about whether this should be a debugging\n> feature.\n>\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int CameraHalConfig::cameraLocation(const std::string &camera) const\n> > +{\n> > +\tconst auto &it = cameras_.find(camera);\n> > +\tif (it == cameras_.end()) {\n> > +\t\tLOG(HALConfig, Error)\n> > +\t\t\t<< \"Camera '\" << camera\n> > +\t\t\t<< \"' not described in the HAL configuration file\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tconst CameraProps &cam = it->second;\n> > +\tif (cam.location.empty()) {\n> > +\t\tLOG(HALConfig, Error) << \"Location for camera '\" << camera\n> > +\t\t\t\t << \"' not available\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tif (cam.location == \"front\")\n> > +\t\treturn CAMERA_FACING_FRONT;\n> > +\telse if (cam.location == \"back\")\n> > +\t\treturn CAMERA_FACING_BACK;\n> > +\telse if (cam.location == \"external\")\n> > +\t\treturn CAMERA_FACING_EXTERNAL;\n> > +\n> > +\tLOG(HALConfig, Error) << \"Unsupported camera location \" << cam.location;\n> > +\treturn -EINVAL;\n>\n> Can we do this conversion when parsing the configuration file instead ?\n> That will ensure at parse time that the data is valid, instead of\n> delaying the check to later. The rotation is already handled that way,\n> and you already check that the location string is valid in\n> CameraHalConfig::parseCameraProps().\n>\n\nack\n\n> > +}\n> > +\n> > +unsigned int CameraHalConfig::cameraRotation(const std::string &camera) const\n> > +{\n> > +\tconst auto &it = cameras_.find(camera);\n> > +\tif (it == cameras_.end()) {\n> > +\t\tLOG(HALConfig, Error)\n> > +\t\t\t<< \"Camera '\" << camera\n> > +\t\t\t<< \"' not described in the HAL configuration file\";\n> > +\t\treturn 0;\n> > +\t}\n> > +\n> > +\tconst CameraProps &cam = it->second;\n> > +\treturn cam.rotation;\n> > +}\n> > +\n> > +const std::string &CameraHalConfig::cameraModel(const std::string &camera) const\n> > +{\n> > +\tstatic const std::string empty(\"\");\n> > +\tconst auto &it = cameras_.find(camera);\n> > +\tif (it == cameras_.end()) {\n> > +\t\tLOG(HALConfig, Error)\n> > +\t\t\t<< \"Camera '\" << camera\n> > +\t\t\t<< \"' not described in the HAL configuration file\";\n> > +\t\treturn empty;\n> > +\t}\n> > +\n> > +\tconst CameraProps &cam = it->second;\n> > +\treturn cam.model;\n> > +}\n> > +\n> > +std::string CameraHalConfig::parseValue(yaml_token_t &token)\n> > +{\n> > +\t/* Make sure the token type is a value and get its content. */\n> > +\tyaml_parser_scan(&parser_, &token);\n> > +\tif (token.type != YAML_VALUE_TOKEN) {\n> > +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > +\t\treturn \"\";\n> > +\t}\n> > +\tyaml_token_delete(&token);\n>\n> Does the token need to be deleted in the error cases above and below ?\n>\n\nAh yes, most probably. I thought it had to be deleted if resued, but\nit seems deletion is just about releasing resources allocated during\nparser_scan(), so it needs to be done in error paths too\n\nI hate I can't goto easily in C++ to implement saner error handling\npaths\n\n> > +\n> > +\tyaml_parser_scan(&parser_, &token);\n> > +\tif (token.type != YAML_SCALAR_TOKEN) {\n> > +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > +\t\treturn \"\";\n> > +\t}\n> > +\n> > +\tchar *scalar = reinterpret_cast(token.data.scalar.value);\n> > +\tstd::string value(scalar, token.data.scalar.length);\n> > +\tyaml_token_delete(&token);\n> > +\n> > +\treturn value;\n> > +}\n> > +\n> > +std::string CameraHalConfig::parseKey(yaml_token_t &token)\n> > +{\n> > +\t/* Make sure the token type is a key and get its value. */\n> > +\tyaml_parser_scan(&parser_, &token);\n> > +\tif (token.type != YAML_SCALAR_TOKEN) {\n> > +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > +\t\treturn \"\";\n> > +\t}\n> > +\n> > +\tchar *scalar = reinterpret_cast(token.data.scalar.value);\n> > +\tstd::string key(scalar, token.data.scalar.length);\n> > +\tyaml_token_delete(&token);\n> > +\n> > +\treturn key;\n> > +}\n> > +\n> > +int CameraHalConfig::parseCameraProps(yaml_token_t &token,\n> > +\t\t\t\t const std::string &cameraID)\n> > +{\n> > +\tyaml_parser_scan(&parser_, &token);\n> > +\tif (token.type != YAML_VALUE_TOKEN) {\n> > +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\tyaml_token_delete(&token);\n> > +\n> > +\tyaml_parser_scan(&parser_, &token);\n> > +\tif (token.type != YAML_BLOCK_MAPPING_START_TOKEN) {\n> > +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\tyaml_token_delete(&token);\n> > +\n> > +\t/*\n> > +\t * Parse the camera properties and store them in a cameraBlock instance.\n> > +\t *\n> > +\t * Add a safety counter to make sure we don't loop indefinitely in case\n> > +\t * the configuration file is malformed.\n> > +\t */\n> > +\tunsigned int sentinel = 100;\n> > +\tCameraProps cameraBlock{};\n> > +\tbool blockEnd = false;\n> > +\tdo {\n> > +\t\tyaml_parser_scan(&parser_, &token);\n> > +\t\tswitch (token.type) {\n> > +\t\tcase YAML_KEY_TOKEN: {\n> > +\t\t\tyaml_token_delete(&token);\n> > +\n> > +\t\t\t/*\n> > +\t\t\t * Parse the camera property key and make sure it is\n> > +\t\t\t * valid.\n> > +\t\t\t */\n> > +\t\t\tstd::string key = parseKey(token);\n> > +\t\t\tstd::string value = parseValue(token);\n> > +\t\t\tif (key.empty() || value.empty()) {\n> > +\t\t\t\tLOG(HALConfig, Error)\n> > +\t\t\t\t\t<< \"Configuration file is not valid\";\n> > +\t\t\t\treturn -EINVAL;\n> > +\t\t\t}\n> > +\n> > +\t\t\tif (key == \"location\") {\n> > +\t\t\t\tif (value != \"front\" && value != \"back\" &&\n> > +\t\t\t\t value != \"external\") {\n> > +\t\t\t\t\tLOG(HALConfig, Error)\n> > +\t\t\t\t\t\t<< \"Unknown location: \" << value;\n> > +\t\t\t\t\treturn -EINVAL;\n> > +\t\t\t\t}\n> > +\t\t\t\tcameraBlock.location = value;\n> > +\t\t\t} else if (key == \"rotation\") {\n> > +\t\t\t\tcameraBlock.rotation = strtoul(value.c_str(),\n> > +\t\t\t\t\t\t\t NULL, 10);\n> > +\t\t\t\tif (cameraBlock.rotation < 0) {\n> > +\t\t\t\t\tLOG(HALConfig, Error)\n> > +\t\t\t\t\t\t<< \"Unknown rotation: \"\n> > +\t\t\t\t\t\t<< cameraBlock.rotation;\n> > +\t\t\t\t\treturn -EINVAL;\n> > +\t\t\t\t}\n> > +\t\t\t} else if (key == \"model\") {\n> > +\t\t\t\tcameraBlock.model = value;\n> > +\t\t\t} else {\n> > +\t\t\t\tLOG(HALConfig, Error)\n> > +\t\t\t\t\t<< \"Configuration file is not valid \"\n> > +\t\t\t\t\t<< \"unknown key: \" << key;\n> > +\t\t\t\treturn -EINVAL;\n> > +\t\t\t}\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t\tcase YAML_BLOCK_END_TOKEN:\n> > +\t\t\tyaml_token_delete(&token);\n> > +\t\t\tblockEnd = true;\n> > +\t\t\tbreak;\n> > +\t\tdefault:\n> > +\t\t\tyaml_token_delete(&token);\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\n> > +\t\t--sentinel;\n> > +\t} while (!blockEnd && sentinel);\n>\n> Missing blank line.\n>\n\nIntentional\n\n> > +\tif (!sentinel) {\n> > +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid \";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tcameras_[cameraID] = cameraBlock;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int CameraHalConfig::parseCameras(yaml_token_t &token)\n> > +{\n> > +\tint ret;\n> > +\n> > +\t/* The 'cameras' key maps a BLOCK_MAPPING_START block. */\n> > +\tyaml_parser_scan(&parser_, &token);\n> > +\tif (token.type != YAML_VALUE_TOKEN) {\n> > +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\tyaml_token_delete(&token);\n> > +\n> > +\tyaml_parser_scan(&parser_, &token);\n> > +\tif (token.type != YAML_BLOCK_MAPPING_START_TOKEN) {\n> > +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\tyaml_token_delete(&token);\n> > +\n> > +\t/*\n> > +\t * Parse the camera properties.\n> > +\t *\n> > +\t * Each camera properties block is a list of properties associated\n> > +\t * with the ID (as assembled by CameraSensor::generateId()) of the\n> > +\t * camera they refer to.\n> > +\t *\n> > +\t * cameras:\n> > +\t * \"camera0 id\":\n> > +\t * key: value\n> > +\t * key: value\n> > +\t * ...\n> > +\t *\n> > +\t * \"camera1 id\":\n> > +\t * key: value\n> > +\t * key: value\n> > +\t * ...\n> > +\t */\n> > +\tbool blockEnd = false;\n> > +\tdo {\n> > +\t\tyaml_parser_scan(&parser_, &token);\n> > +\t\tswitch (token.type) {\n> > +\t\tcase YAML_KEY_TOKEN: {\n> > +\t\t\tyaml_token_delete(&token);\n> > +\n> > +\t\t\t/* Parse the camera ID as key of the property list. */\n> > +\t\t\tstd::string cameraId = parseKey(token);\n> > +\t\t\tif (cameraId.empty()) {\n> > +\t\t\t\tLOG(HALConfig, Error)\n> > +\t\t\t\t\t<< \"Configuration file is not valid\";\n> > +\t\t\t\treturn -EINVAL;\n> > +\t\t\t}\n> > +\n> > +\t\t\tret = parseCameraProps(token, cameraId);\n> > +\t\t\tif (ret) {\n> > +\t\t\t\tLOG(HALConfig, Error)\n> > +\t\t\t\t\t<< \"Configuration file is not valid\";\n> > +\t\t\t\treturn -EINVAL;\n> > +\t\t\t}\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t\tcase YAML_BLOCK_END_TOKEN:\n> > +\t\t\tyaml_token_delete(&token);\n> > +\t\t\tblockEnd = true;\n> > +\t\t\tbreak;\n> > +\t\tdefault:\n> > +\t\t\tyaml_token_delete(&token);\n> > +\t\t\tLOG(HALConfig, Error)\n> > +\t\t\t\t<< \"Configuration file is not valid\";\n> > +\t\t\treturn -EINVAL;\n> > +\t\t}\n> > +\t} while (!blockEnd);\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int CameraHalConfig::parseEntry(yaml_token_t &token)\n> > +{\n> > +\tint ret;\n> > +\n> > +\t/*\n> > +\t * Parse each key we find in the file.\n> > +\t *\n> > +\t * Keys like 'model' and 'maker' are device properties and gets\n> > +\t * stored as class members.\n> > +\t *\n> > +\t * The 'cameras' keys maps to a list of (lists) of camera properties.\n> > +\t */\n> > +\n> > +\tstd::string key = parseKey(token);\n> > +\tif (key.empty()) {\n> > +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tif (key == \"cameras\") {\n> > +\t\tret = parseCameras(token);\n> > +\t\tif (ret)\n> > +\t\t\treturn ret;\n> > +\t} else if (key == \"manufacturer\") {\n> > +\t\tmaker_ = parseValue(token);\n> > +\t\tif (maker_.empty()) {\n> > +\t\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > +\t\t\treturn -EINVAL;\n> > +\t\t}\n> > +\t} else if (key == \"model\") {\n> > +\t\tmodel_ = parseValue(token);\n> > +\t\tif (model_.empty()) {\n> > +\t\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > +\t\t\treturn -EINVAL;\n> > +\t\t}\n> > +\t} else {\n> > +\t\tLOG(HALConfig, Error) << \"Unknown key \" << key;\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int CameraHalConfig::parseConfigFile()\n> > +{\n> > +\tyaml_token_t token;\n> > +\tint ret;\n> > +\n> > +\tyaml_parser_scan(&parser_, &token);\n> > +\tif (token.type != YAML_STREAM_START_TOKEN) {\n> > +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\tyaml_token_delete(&token);\n> > +\n> > +\tyaml_parser_scan(&parser_, &token);\n> > +\tif (token.type != YAML_BLOCK_MAPPING_START_TOKEN) {\n> > +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\tyaml_token_delete(&token);\n> > +\n> > +\t/* Parse the file and parse each single key one by one. */\n> > +\tdo {\n> > +\t\tyaml_parser_scan(&parser_, &token);\n> > +\t\tswitch (token.type) {\n> > +\t\tcase YAML_KEY_TOKEN:\n> > +\t\t\tyaml_token_delete(&token);\n> > +\t\t\tret = parseEntry(token);\n>\n> Do you need to pass the token to parseEntry(), given that you've just\n> deleted it ? It looks to me like the token is only the return value of\n> yaml_parse_scan(), and can thus be a local variable everywhere.\n>\n\nGood question. Where's the parsing state kept ? in the token or in the\nparser ? If it's in the parser I can avoid passing token around\n\n> > +\t\t\tbreak;\n> > +\t\tcase YAML_STREAM_END_TOKEN:\n> > +\t\t\t/* Resources are released after the loop exit. */\n> > +\t\t\tbreak;\n> > +\t\tdefault:\n> > +\t\t\tyaml_token_delete(&token);\n> > +\t\t\tbreak;\n>\n> The YAML_STREAM_END_TOKEN and default case are different, but the\n> while() condition below accesses token.type. Is that intended ?\n>\n\nYes, the type has to stay valid in case of STREAM_END to be verified\nbelow. I can handle this with just ret if you prefer.\n\nThanks\n j\n\n\n> > +\t\t}\n> > +\t} while (token.type != YAML_STREAM_END_TOKEN && ret >= 0);\n> > +\tyaml_token_delete(&token);\n> > +\n> > +\treturn ret;\n> > +}\n> > diff --git a/src/android/camera_hal_config.h b/src/android/camera_hal_config.h\n> > new file mode 100644\n> > index 000000000000..ce77b1ee1582\n> > --- /dev/null\n> > +++ b/src/android/camera_hal_config.h\n> > @@ -0,0 +1,53 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Google Inc.\n> > + *\n> > + * camera_hal_config.h - Camera HAL configuration file manager\n> > + */\n> > +#ifndef __ANDROID_CAMERA_HAL_CONFIG_H__\n> > +#define __ANDROID_CAMERA_HAL_CONFIG_H__\n> > +\n> > +#include \n> > +#include \n> > +#include \n>\n> As commented in v1, hiding the parser would be nice, but it can be done\n> later, on top.\n>\n> > +\n> > +class CameraHalConfig\n> > +{\n> > +public:\n> > +\tCameraHalConfig();\n> > +\t~CameraHalConfig();\n> > +\n> > +\tint open();\n> > +\n> > +\tconst std::string &deviceModel() const { return model_; }\n> > +\tconst std::string &deviceMaker() const { return maker_; }\n> > +\n> > +\tint cameraLocation(const std::string &camera) const;\n> > +\tunsigned int cameraRotation(const std::string &camera) const;\n> > +\tconst std::string &cameraModel(const std::string &camera) const;\n> > +\n> > +private:\n> > +\tyaml_parser_t parser_;\n> > +\n> > +\tstd::string model_;\n> > +\tstd::string maker_;\n> > +\tclass CameraProps\n> > +\t{\n> > +\tpublic:\n> > +\t\tstd::string location;\n> > +\t\tstd::string model;\n> > +\t\tunsigned int rotation;\n> > +\t};\n> > +\tstd::map cameras_;\n> > +\n> > +\tstd::string findFilePath(const std::string &filename) const;\n> > +\tFILE *openFile(const std::string &filename);\n> > +\tstd::string parseValue(yaml_token_t &token);\n> > +\tstd::string parseKey(yaml_token_t &token);\n> > +\tint parseCameraProps(yaml_token_t &token, const std::string &cameraID);\n> > +\tint parseCameras(yaml_token_t &token);\n> > +\tint parseEntry(yaml_token_t &token);\n> > +\tint parseConfigFile();\n> > +};\n> > +\n> > +#endif /* __ANDROID_CAMERA_HAL_CONFIG_H__ */\n> > diff --git a/src/android/meson.build b/src/android/meson.build\n> > index 8e7d07d9be3c..c0ede407bc38 100644\n> > --- a/src/android/meson.build\n> > +++ b/src/android/meson.build\n> > @@ -3,6 +3,7 @@\n> > android_deps = [\n> > dependency('libexif', required : get_option('android')),\n> > dependency('libjpeg', required : get_option('android')),\n> > + dependency('yaml-0.1', required : get_option('android')),\n> > ]\n> >\n> > android_enabled = true\n> > @@ -41,6 +42,7 @@ android_hal_sources = files([\n> > 'camera3_hal.cpp',\n> > 'camera_hal_manager.cpp',\n> > 'camera_device.cpp',\n> > + 'camera_hal_config.cpp',\n>\n> While at it, could you move camera_hal_manager to the right place ?\n>\n> > 'camera_metadata.cpp',\n> > 'camera_ops.cpp',\n> > 'camera_stream.cpp',\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 66CF0C0DA4\n\tfor ;\n\tTue, 30 Mar 2021 10:36:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2F14468781;\n\tTue, 30 Mar 2021 12:36:58 +0200 (CEST)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5D776602D2\n\tfor ;\n\tTue, 30 Mar 2021 12:36:56 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id A2F5D24000B;\n\tTue, 30 Mar 2021 10:36:55 +0000 (UTC)"],"Date":"Tue, 30 Mar 2021 12:37:30 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210330103730.iocnmtlexhvaafuj@uno.localdomain>","References":"<20210329152807.28331-1-jacopo@jmondi.org>\n\t<20210329152807.28331-3-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 3/6] android: Add CameraHalConfig\n\tclass","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16105,"web_url":"https://patchwork.libcamera.org/comment/16105/","msgid":"","date":"2021-04-03T02:47:51","subject":"Re: [libcamera-devel] [PATCH v2 3/6] android: Add CameraHalConfig\n\tclass","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Tue, Mar 30, 2021 at 01:16:22PM +0900, Hirokazu Honda wrote:\n> On Tue, Mar 30, 2021 at 12:53 PM Laurent Pinchart wrote:\n> > On Tue, Mar 30, 2021 at 12:48:20PM +0900, Hirokazu Honda wrote:\n> > > On Tue, Mar 30, 2021 at 9:40 AM Laurent Pinchartwrote:\n> > > > On Mon, Mar 29, 2021 at 05:28:04PM +0200, Jacopo Mondi wrote:\n> > > > > Add a CameraHalConfig class to the Android Camera3 HAL layer.\n> > > >\n> > > > There are quite a few comments from v1 that haven't been addressed.\n> > > > Maybe that's because my last reply to v1 came after you've sent this\n> > > > series. The comments may also not all be correct, applicable and/or\n> > > > desired, but could you then reply to them to explain why they're ignored\n> > > > ?\n> > > >\n> > > > In particular, I think that exposing CameraProps would simplify the\n> > > > implementation quite a bit, by removing the need for the camera*()\n> > > > functions, with the lookup of the CameraProps entry every time, and the\n> > > > associated error handling.\n> > >\n> > > +1. So we should pass the structure to CameraDevice by value.\n> >\n> > Why not by const reference ?\n> \n> I would not like having const reference in member variables.\n> I actually thought this was stated in Google C++ Style Guide, and\n> searched it, but found there was no statement like that.\n> So this is just my preference.\n> For the purpose, I usually use a const pointer.\n> Considering why I don't like having const reference member variables,\n> one of reasons might be what I don't get used to :p, but also from a\n> caller of the constructor the lifetime of CameraHalConfig is unclear.\n> For instance, CameraDevice(.., config) is called, but a caller has no\n> idea when the config should be alive by, while CameraDevice(...,\n> &config) tells a caller to keep the config object outlive to\n> CameraDevice.\n\nWe have coding style rules to handle borrowed references, explained in\nDocumentation/coding-style.rst:\n\n When the object is stored in a std::unique_ptr<>, borrowing passes a\n reference to the object, not to the std::unique_ptr<>, as\n\n * a 'const &' when the object doesn't need to be modified and may not be\n null.\n * a pointer when the object may be modified or may be null. Unless\n otherwise specified, pointers passed to functions are considered as\n borrowed references valid for the duration of the function only.\n\n(I've just realized that we may want to update this, as the rule isn't\nspecific to std::unique_ptr<>, it can apply equally to, for instance,\nmember variables of a class.)\n\nThe difference between const reference and non-const pointer is not\nrelated to object ownership, but to constness. We tend to favour\npointers for variables that can be modified by the callee, and const\nreferences for variables that can't. This means that we can't use\nreference vs. pointer to express ownership and life time of objects.\n\nThis can of course be reconsidered, but I'd like the coding style to\nremain consistent, and thus first update the documentation and the code\nbase.\n\n> Again, this might be my personal preference. But does this description\n> make sense?\n> \n> > > > > Signed-off-by: Jacopo Mondi \n> > > > > ---\n> > > > > README.rst | 2 +-\n> > > > > src/android/camera_hal_config.cpp | 463 ++++++++++++++++++++++++++++++\n> > > > > src/android/camera_hal_config.h | 53 ++++\n> > > > > src/android/meson.build | 2 +\n> > > > > 4 files changed, 519 insertions(+), 1 deletion(-)\n> > > > > create mode 100644 src/android/camera_hal_config.cpp\n> > > > > create mode 100644 src/android/camera_hal_config.h\n> > > > >\n> > > > > diff --git a/README.rst b/README.rst\n> > > > > index 7a98164bbb0a..f68d435196de 100644\n> > > > > --- a/README.rst\n> > > > > +++ b/README.rst\n> > > > > @@ -88,7 +88,7 @@ for tracing with lttng: [optional]\n> > > > > liblttng-ust-dev python3-jinja2 lttng-tools\n> > > > >\n> > > > > for android: [optional]\n> > > > > - libexif libjpeg\n> > > > > + libexif libjpeg libyaml\n> > > > >\n> > > > > Using GStreamer plugin\n> > > > > ~~~~~~~~~~~~~~~~~~~~~~\n> > > > > diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp\n> > > > > new file mode 100644\n> > > > > index 000000000000..846dd7357b0a\n> > > > > --- /dev/null\n> > > > > +++ b/src/android/camera_hal_config.cpp\n> > > > > @@ -0,0 +1,463 @@\n> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2021, Google Inc.\n> > > > > + *\n> > > > > + * camera_hal_config.cpp - Camera HAL configuration file manager\n> > > > > + */\n> > > > > +#include \"camera_hal_config.h\"\n> > > > > +\n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +\n> > > > > +#include \n> > > > > +\n> > > > > +#include \"libcamera/internal/file.h\"\n> > > > > +#include \"libcamera/internal/log.h\"\n> > > > > +\n> > > > > +using namespace libcamera;\n> > > > > +\n> > > > > +LOG_DEFINE_CATEGORY(HALConfig)\n> > > > > +\n> > > > > +CameraHalConfig::CameraHalConfig()\n> > > > > +{\n> > > > > + if (!yaml_parser_initialize(&parser_))\n> > > > > + LOG(HALConfig, Fatal) << \"Failed to initialize yaml parser\";\n> > > > > +}\n> > > > > +\n> > > > > +CameraHalConfig::~CameraHalConfig()\n> > > > > +{\n> > > > > + yaml_parser_delete(&parser_);\n> > > > > +}\n> > > > > +\n> > >\n> > > It looks strange to keep parser_ alive as long as CameraHalConfig,\n> > > because parsing is done in open().\n> > > I would initialize the parser in open() and destroy it in the end of open().\n> > > In this direction, we would prefer moving parse* functions to .cpp\n> > > file and also moving yaml include to .cpp.\n> >\n> > I've mentioned that before :-) It could be done on top though.\n> >\n> > > > > +/*\n> > > > > + * Configuration files can be stored in system paths, which are identified\n> > > > > + * through the build configuration.\n> > > > > + *\n> > > > > + * However, when running uninstalled - the source location takes precedence.\n> > > > > + */\n> > > > > +std::string CameraHalConfig::findFilePath(const std::string &filename) const\n> > > > > +{\n> > > > > + static std::array searchPaths = {\n> > > > > + LIBCAMERA_SYSCONF_DIR,\n> > > > > + LIBCAMERA_DATA_DIR,\n> > > >\n> > > > There's also a comment in v1 about data dir. Data dir is typically\n> > > > /usr/share/libcamera/. Given that the configuration file is\n> > > > device-specific, I don't think we'll ever look for it there, but only in\n> > > > sysconf dir.\n> > > >\n> > > > > + };\n> > > > > +\n> > > > > + if (File::exists(filename))\n> > > > > + return filename;\n> > > > > +\n> > > > > + std::string root = utils::libcameraSourcePath();\n> > > > > + if (!root.empty()) {\n> > > > > + std::string configurationPath = root + \"data/\" + filename;\n> > > > > + if (File::exists(configurationPath))\n> > > > > + return configurationPath;\n> > > > > + }\n> > > > > +\n> > > > > + for (std::string &path : searchPaths) {\n> > > > > + std::string configurationPath = path + \"/\" + filename;\n> > > > > + if (File::exists(configurationPath))\n> > > > > + return configurationPath;\n> > > > > + }\n> > > > > +\n> > > > > + return \"\";\n> > > > > +}\n> > > > > +\n> > > > > +FILE *CameraHalConfig::openFile(const std::string &filename)\n> > > > > +{\n> > > > > + const std::string filePath = findFilePath(filename);\n> > > > > + if (filePath.empty()) {\n> > > > > + LOG(HALConfig, Error)\n> > > > > + << \"Configuration file: \\\"\" << filename << \"\\\" not found\";\n> > > > > + return nullptr;\n> > > > > + }\n> > > > > +\n> > > > > + LOG(HALConfig, Debug) << \"Reading configuration file from \" << filePath;\n> > > > > +\n> > > > > + FILE *fh = fopen(filePath.c_str(), \"r\");\n> > > > > + if (!fh) {\n> > > > > + int ret = -errno;\n> > > > > + LOG(HALConfig, Error) << \"Failed to open configuration file \"\n> > > > > + << filePath << \": \" << strerror(-ret);\n> > > > > + return nullptr;\n> > > > > + }\n> > > > > +\n> > > > > + return fh;\n> > > > > +}\n> > > > > +\n> > > > > +/*\n> > > > > + * Open the HAL configuration file and validate its content.\n> > > > > + * Return 0 on success, a negative error code otherwise\n> > > > > + */\n> > > > > +int CameraHalConfig::open()\n> > > > > +{\n> > > > > + int ret;\n> > > > > +\n> > > > > + FILE *fh = openFile(\"camera_hal.yaml\");\n> > > > > + if (!fh)\n> > > > > + return -ENOENT;\n> > > > > +\n> > > > > + yaml_parser_set_input_file(&parser_, fh);\n> > > > > + ret = parseConfigFile();\n> > > > > + fclose(fh);\n> > > > > + if (ret)\n> > > > > + return ret;\n> > > > > +\n> > > > > + LOG(HALConfig, Info) << \"Device model: \" << model_;\n> > > > > + LOG(HALConfig, Info) << \"Device maker: \" << maker_;\n> > > > > + for (const auto &c : cameras_) {\n> > > > > + const std::string &cameraId = c.first;\n> > > > > + const CameraProps &camera = c.second;\n> > > > > + LOG(HALConfig, Info) << \"'\" << cameraId << \"' \"\n> > > > > + << \" model: \" << camera.model\n> > > > > + << \"(\" << camera.location << \")[\"\n> > > > > + << camera.rotation << \"]\";\n> > > > > + }\n> > > >\n> > > > There's also a comment in v1 about whether this should be a debugging\n> > > > feature.\n> > > >\n> > > > > +\n> > > > > + return 0;\n> > > > > +}\n> > > > > +\n> > > > > +int CameraHalConfig::cameraLocation(const std::string &camera) const\n> > > > > +{\n> > > > > + const auto &it = cameras_.find(camera);\n> > > > > + if (it == cameras_.end()) {\n> > > > > + LOG(HALConfig, Error)\n> > > > > + << \"Camera '\" << camera\n> > > > > + << \"' not described in the HAL configuration file\";\n> > > > > + return -EINVAL;\n> > > > > + }\n> > > > > +\n> > > > > + const CameraProps &cam = it->second;\n> > > > > + if (cam.location.empty()) {\n> > > > > + LOG(HALConfig, Error) << \"Location for camera '\" << camera\n> > > > > + << \"' not available\";\n> > > > > + return -EINVAL;\n> > > > > + }\n> > > > > +\n> > > > > + if (cam.location == \"front\")\n> > > > > + return CAMERA_FACING_FRONT;\n> > > > > + else if (cam.location == \"back\")\n> > > > > + return CAMERA_FACING_BACK;\n> > > > > + else if (cam.location == \"external\")\n> > > > > + return CAMERA_FACING_EXTERNAL;\n> > > > > +\n> > > > > + LOG(HALConfig, Error) << \"Unsupported camera location \" << cam.location;\n> > > > > + return -EINVAL;\n> > > >\n> > > > Can we do this conversion when parsing the configuration file instead ?\n> > > > That will ensure at parse time that the data is valid, instead of\n> > > > delaying the check to later. The rotation is already handled that way,\n> > > > and you already check that the location string is valid in\n> > > > CameraHalConfig::parseCameraProps().\n> > > >\n> > > > > +}\n> > > > > +\n> > > > > +unsigned int CameraHalConfig::cameraRotation(const std::string &camera) const\n> > > > > +{\n> > > > > + const auto &it = cameras_.find(camera);\n> > > > > + if (it == cameras_.end()) {\n> > > > > + LOG(HALConfig, Error)\n> > > > > + << \"Camera '\" << camera\n> > > > > + << \"' not described in the HAL configuration file\";\n> > > > > + return 0;\n> > > > > + }\n> > > > > +\n> > > > > + const CameraProps &cam = it->second;\n> > > > > + return cam.rotation;\n> > > > > +}\n> > > > > +\n> > > > > +const std::string &CameraHalConfig::cameraModel(const std::string &camera) const\n> > > > > +{\n> > > > > + static const std::string empty(\"\");\n> > > > > + const auto &it = cameras_.find(camera);\n> > > > > + if (it == cameras_.end()) {\n> > > > > + LOG(HALConfig, Error)\n> > > > > + << \"Camera '\" << camera\n> > > > > + << \"' not described in the HAL configuration file\";\n> > > > > + return empty;\n> > > > > + }\n> > > > > +\n> > > > > + const CameraProps &cam = it->second;\n> > > > > + return cam.model;\n> > > > > +}\n> > > > > +\n> > > > > +std::string CameraHalConfig::parseValue(yaml_token_t &token)\n> > > > > +{\n> > > > > + /* Make sure the token type is a value and get its content. */\n> > > > > + yaml_parser_scan(&parser_, &token);\n> > > > > + if (token.type != YAML_VALUE_TOKEN) {\n> > > > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > > > + return \"\";\n> > > > > + }\n> > > > > + yaml_token_delete(&token);\n> > > >\n> > > > Does the token need to be deleted in the error cases above and below ?\n> > > >\n> > > > > +\n> > > > > + yaml_parser_scan(&parser_, &token);\n> > > > > + if (token.type != YAML_SCALAR_TOKEN) {\n> > > > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > > > + return \"\";\n> > > > > + }\n> > > > > +\n> > > > > + char *scalar = reinterpret_cast(token.data.scalar.value);\n> > > > > + std::string value(scalar, token.data.scalar.length);\n> > > > > + yaml_token_delete(&token);\n> > > > > +\n> > > > > + return value;\n> > > > > +}\n> > > > > +\n> > > > > +std::string CameraHalConfig::parseKey(yaml_token_t &token)\n> > > > > +{\n> > > > > + /* Make sure the token type is a key and get its value. */\n> > > > > + yaml_parser_scan(&parser_, &token);\n> > > > > + if (token.type != YAML_SCALAR_TOKEN) {\n> > > > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > > > + return \"\";\n> > > > > + }\n> > > > > +\n> > > > > + char *scalar = reinterpret_cast(token.data.scalar.value);\n> > > > > + std::string key(scalar, token.data.scalar.length);\n> > > > > + yaml_token_delete(&token);\n> > > > > +\n> > > > > + return key;\n> > > > > +}\n> > > > > +\n> > > > > +int CameraHalConfig::parseCameraProps(yaml_token_t &token,\n> > > > > + const std::string &cameraID)\n> > > > > +{\n> > > > > + yaml_parser_scan(&parser_, &token);\n> > > > > + if (token.type != YAML_VALUE_TOKEN) {\n> > > > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > > > + return -EINVAL;\n> > > > > + }\n> > > > > + yaml_token_delete(&token);\n> > > > > +\n> > > > > + yaml_parser_scan(&parser_, &token);\n> > > > > + if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) {\n> > > > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > > > + return -EINVAL;\n> > > > > + }\n> > > > > + yaml_token_delete(&token);\n> > > > > +\n> > > > > + /*\n> > > > > + * Parse the camera properties and store them in a cameraBlock instance.\n> > > > > + *\n> > > > > + * Add a safety counter to make sure we don't loop indefinitely in case\n> > > > > + * the configuration file is malformed.\n> > > > > + */\n> > > > > + unsigned int sentinel = 100;\n> > > > > + CameraProps cameraBlock{};\n> > > > > + bool blockEnd = false;\n> > > > > + do {\n> > > > > + yaml_parser_scan(&parser_, &token);\n> > > > > + switch (token.type) {\n> > > > > + case YAML_KEY_TOKEN: {\n> > > > > + yaml_token_delete(&token);\n> > > > > +\n> > > > > + /*\n> > > > > + * Parse the camera property key and make sure it is\n> > > > > + * valid.\n> > > > > + */\n> > > > > + std::string key = parseKey(token);\n> > > > > + std::string value = parseValue(token);\n> > > > > + if (key.empty() || value.empty()) {\n> > > > > + LOG(HALConfig, Error)\n> > > > > + << \"Configuration file is not valid\";\n> > > > > + return -EINVAL;\n> > > > > + }\n> > > > > +\n> > > > > + if (key == \"location\") {\n> > > > > + if (value != \"front\" && value != \"back\" &&\n> > > > > + value != \"external\") {\n> > > > > + LOG(HALConfig, Error)\n> > > > > + << \"Unknown location: \" << value;\n> > > > > + return -EINVAL;\n> > > > > + }\n> > > > > + cameraBlock.location = value;\n> > > > > + } else if (key == \"rotation\") {\n> > > > > + cameraBlock.rotation = strtoul(value.c_str(),\n> > > > > + NULL, 10);\n> > > > > + if (cameraBlock.rotation < 0) {\n> > > > > + LOG(HALConfig, Error)\n> > > > > + << \"Unknown rotation: \"\n> > > > > + << cameraBlock.rotation;\n> > > > > + return -EINVAL;\n> > > > > + }\n> > > > > + } else if (key == \"model\") {\n> > > > > + cameraBlock.model = value;\n> > > > > + } else {\n> > > > > + LOG(HALConfig, Error)\n> > > > > + << \"Configuration file is not valid \"\n> > > > > + << \"unknown key: \" << key;\n> > > > > + return -EINVAL;\n> > > > > + }\n> > > > > + break;\n> > > > > + }\n> > > > > + case YAML_BLOCK_END_TOKEN:\n> > > > > + yaml_token_delete(&token);\n> > > > > + blockEnd = true;\n> > > > > + break;\n> > > > > + default:\n> > > > > + yaml_token_delete(&token);\n> > > > > + break;\n> > > > > + }\n> > > > > +\n> > > > > + --sentinel;\n> > > > > + } while (!blockEnd && sentinel);\n> > > >\n> > > > Missing blank line.\n> > > >\n> > > > > + if (!sentinel) {\n> > > > > + LOG(HALConfig, Error) << \"Configuration file is not valid \";\n> > > > > + return -EINVAL;\n> > > > > + }\n> > > > > +\n> > > > > + cameras_[cameraID] = cameraBlock;\n> > > > > +\n> > > > > + return 0;\n> > > > > +}\n> > > > > +\n> > > > > +int CameraHalConfig::parseCameras(yaml_token_t &token)\n> > > > > +{\n> > > > > + int ret;\n> > > > > +\n> > > > > + /* The 'cameras' key maps a BLOCK_MAPPING_START block. */\n> > > > > + yaml_parser_scan(&parser_, &token);\n> > > > > + if (token.type != YAML_VALUE_TOKEN) {\n> > > > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > > > + return -EINVAL;\n> > > > > + }\n> > > > > + yaml_token_delete(&token);\n> > > > > +\n> > > > > + yaml_parser_scan(&parser_, &token);\n> > > > > + if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) {\n> > > > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > > > + return -EINVAL;\n> > > > > + }\n> > > > > + yaml_token_delete(&token);\n> > > > > +\n> > > > > + /*\n> > > > > + * Parse the camera properties.\n> > > > > + *\n> > > > > + * Each camera properties block is a list of properties associated\n> > > > > + * with the ID (as assembled by CameraSensor::generateId()) of the\n> > > > > + * camera they refer to.\n> > > > > + *\n> > > > > + * cameras:\n> > > > > + * \"camera0 id\":\n> > > > > + * key: value\n> > > > > + * key: value\n> > > > > + * ...\n> > > > > + *\n> > > > > + * \"camera1 id\":\n> > > > > + * key: value\n> > > > > + * key: value\n> > > > > + * ...\n> > > > > + */\n> > > > > + bool blockEnd = false;\n> > > > > + do {\n> > > > > + yaml_parser_scan(&parser_, &token);\n> > > > > + switch (token.type) {\n> > > > > + case YAML_KEY_TOKEN: {\n> > > > > + yaml_token_delete(&token);\n> > > > > +\n> > > > > + /* Parse the camera ID as key of the property list. */\n> > > > > + std::string cameraId = parseKey(token);\n> > > > > + if (cameraId.empty()) {\n> > > > > + LOG(HALConfig, Error)\n> > > > > + << \"Configuration file is not valid\";\n> > > > > + return -EINVAL;\n> > > > > + }\n> > > > > +\n> > > > > + ret = parseCameraProps(token, cameraId);\n> > > > > + if (ret) {\n> > > > > + LOG(HALConfig, Error)\n> > > > > + << \"Configuration file is not valid\";\n> > > > > + return -EINVAL;\n> > > > > + }\n> > > > > + break;\n> > > > > + }\n> > > > > + case YAML_BLOCK_END_TOKEN:\n> > > > > + yaml_token_delete(&token);\n> > > > > + blockEnd = true;\n> > > > > + break;\n> > > > > + default:\n> > > > > + yaml_token_delete(&token);\n> > > > > + LOG(HALConfig, Error)\n> > > > > + << \"Configuration file is not valid\";\n> > > > > + return -EINVAL;\n> > > > > + }\n> > > > > + } while (!blockEnd);\n> > > > > +\n> > > > > + return 0;\n> > > > > +}\n> > > > > +\n> > > > > +int CameraHalConfig::parseEntry(yaml_token_t &token)\n> > > > > +{\n> > > > > + int ret;\n> > > > > +\n> > > > > + /*\n> > > > > + * Parse each key we find in the file.\n> > > > > + *\n> > > > > + * Keys like 'model' and 'maker' are device properties and gets\n> > > > > + * stored as class members.\n> > > > > + *\n> > > > > + * The 'cameras' keys maps to a list of (lists) of camera properties.\n> > > > > + */\n> > > > > +\n> > > > > + std::string key = parseKey(token);\n> > > > > + if (key.empty()) {\n> > > > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > > > + return -EINVAL;\n> > > > > + }\n> > > > > +\n> > > > > + if (key == \"cameras\") {\n> > > > > + ret = parseCameras(token);\n> > > > > + if (ret)\n> > > > > + return ret;\n> > > > > + } else if (key == \"manufacturer\") {\n> > > > > + maker_ = parseValue(token);\n> > > > > + if (maker_.empty()) {\n> > > > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > > > + return -EINVAL;\n> > > > > + }\n> > > > > + } else if (key == \"model\") {\n> > > > > + model_ = parseValue(token);\n> > > > > + if (model_.empty()) {\n> > > > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > > > + return -EINVAL;\n> > > > > + }\n> > > > > + } else {\n> > > > > + LOG(HALConfig, Error) << \"Unknown key \" << key;\n> > > > > + return -EINVAL;\n> > > > > + }\n> > > > > +\n> > > > > + return 0;\n> > > > > +}\n> > > > > +\n> > > > > +int CameraHalConfig::parseConfigFile()\n> > > > > +{\n> > > > > + yaml_token_t token;\n> > > > > + int ret;\n> > > > > +\n> > > > > + yaml_parser_scan(&parser_, &token);\n> > > > > + if (token.type != YAML_STREAM_START_TOKEN) {\n> > > > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > > > + return -EINVAL;\n> > > > > + }\n> > > > > + yaml_token_delete(&token);\n> > > > > +\n> > > > > + yaml_parser_scan(&parser_, &token);\n> > > > > + if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) {\n> > > > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > > > + return -EINVAL;\n> > > > > + }\n> > > > > + yaml_token_delete(&token);\n> > > > > +\n> > > > > + /* Parse the file and parse each single key one by one. */\n> > > > > + do {\n> > > > > + yaml_parser_scan(&parser_, &token);\n> > > > > + switch (token.type) {\n> > > > > + case YAML_KEY_TOKEN:\n> > > > > + yaml_token_delete(&token);\n> > > > > + ret = parseEntry(token);\n> > > >\n> > > > Do you need to pass the token to parseEntry(), given that you've just\n> > > > deleted it ? It looks to me like the token is only the return value of\n> > > > yaml_parse_scan(), and can thus be a local variable everywhere.\n> > > >\n> > > > > + break;\n> > > > > + case YAML_STREAM_END_TOKEN:\n> > > > > + /* Resources are released after the loop exit. */\n> > > > > + break;\n> > > > > + default:\n> > > > > + yaml_token_delete(&token);\n> > > > > + break;\n> > > >\n> > > > The YAML_STREAM_END_TOKEN and default case are different, but the\n> > > > while() condition below accesses token.type. Is that intended ?\n> > > >\n> > > > > + }\n> > > > > + } while (token.type != YAML_STREAM_END_TOKEN && ret >= 0);\n> > > > > + yaml_token_delete(&token);\n> > > > > +\n> > > > > + return ret;\n> > > > > +}\n> > > > > diff --git a/src/android/camera_hal_config.h b/src/android/camera_hal_config.h\n> > > > > new file mode 100644\n> > > > > index 000000000000..ce77b1ee1582\n> > > > > --- /dev/null\n> > > > > +++ b/src/android/camera_hal_config.h\n> > > > > @@ -0,0 +1,53 @@\n> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2021, Google Inc.\n> > > > > + *\n> > > > > + * camera_hal_config.h - Camera HAL configuration file manager\n> > > > > + */\n> > > > > +#ifndef __ANDROID_CAMERA_HAL_CONFIG_H__\n> > > > > +#define __ANDROID_CAMERA_HAL_CONFIG_H__\n> > > > > +\n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > >\n> > > > As commented in v1, hiding the parser would be nice, but it can be done\n> > > > later, on top.\n> > > >\n> > > > > +\n> > > > > +class CameraHalConfig\n> > > > > +{\n> > > > > +public:\n> > > > > + CameraHalConfig();\n> > > > > + ~CameraHalConfig();\n> > > > > +\n> > > > > + int open();\n> > > > > +\n> > > > > + const std::string &deviceModel() const { return model_; }\n> > > > > + const std::string &deviceMaker() const { return maker_; }\n> > > > > +\n> > > > > + int cameraLocation(const std::string &camera) const;\n> > > > > + unsigned int cameraRotation(const std::string &camera) const;\n> > > > > + const std::string &cameraModel(const std::string &camera) const;\n> > > > > +\n> > > > > +private:\n> > > > > + yaml_parser_t parser_;\n> > > > > +\n> > > > > + std::string model_;\n> > > > > + std::string maker_;\n> > > > > + class CameraProps\n> > > > > + {\n> > > > > + public:\n> > > > > + std::string location;\n> > > > > + std::string model;\n> > > > > + unsigned int rotation;\n> > > > > + };\n> > > > > + std::map cameras_;\n> > > > > +\n> > > > > + std::string findFilePath(const std::string &filename) const;\n> > > > > + FILE *openFile(const std::string &filename);\n> > > > > + std::string parseValue(yaml_token_t &token);\n> > > > > + std::string parseKey(yaml_token_t &token);\n> > > > > + int parseCameraProps(yaml_token_t &token, const std::string &cameraID);\n> > > > > + int parseCameras(yaml_token_t &token);\n> > > > > + int parseEntry(yaml_token_t &token);\n> > > > > + int parseConfigFile();\n> > > > > +};\n> > > > > +\n> > > > > +#endif /* __ANDROID_CAMERA_HAL_CONFIG_H__ */\n> > > > > diff --git a/src/android/meson.build b/src/android/meson.build\n> > > > > index 8e7d07d9be3c..c0ede407bc38 100644\n> > > > > --- a/src/android/meson.build\n> > > > > +++ b/src/android/meson.build\n> > > > > @@ -3,6 +3,7 @@\n> > > > > android_deps = [\n> > > > > dependency('libexif', required : get_option('android')),\n> > > > > dependency('libjpeg', required : get_option('android')),\n> > > > > + dependency('yaml-0.1', required : get_option('android')),\n> > > > > ]\n> > > > >\n> > > > > android_enabled = true\n> > > > > @@ -41,6 +42,7 @@ android_hal_sources = files([\n> > > > > 'camera3_hal.cpp',\n> > > > > 'camera_hal_manager.cpp',\n> > > > > 'camera_device.cpp',\n> > > > > + 'camera_hal_config.cpp',\n> > > >\n> > > > While at it, could you move camera_hal_manager to the right place ?\n> > > >\n> > > > > 'camera_metadata.cpp',\n> > > > > 'camera_ops.cpp',\n> > > > > 'camera_stream.cpp',","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8260EC0DA3\n\tfor ;\n\tSat, 3 Apr 2021 02:48:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D165568791;\n\tSat, 3 Apr 2021 04:48:38 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B4906602D7\n\tfor ;\n\tSat, 3 Apr 2021 04:48:36 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 17C983D7;\n\tSat, 3 Apr 2021 04:48:36 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"MLOIOxTf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1617418116;\n\tbh=IIdn1K4+Q8StPe504pIhgdgYcefdZXe2RZcPntwAp64=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=MLOIOxTf9Gv3lY7pyjXpDUCsl40Vwe56IsoPr7Y/Ci7ppp2WCdNgnwnSsZTJeIfen\n\tdJguwLr9sJUVpXfJHM+sGHuW6ePKKGtk6InmR/B3W1mekfZ7qYuGTGvswBa6YNkkSh\n\tGhU4MVUzmjc4y1Q4jSbSgMmpsqaXJBj63vJ162gw=","Date":"Sat, 3 Apr 2021 05:47:51 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210329152807.28331-1-jacopo@jmondi.org>\n\t<20210329152807.28331-3-jacopo@jmondi.org>\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 3/6] android: Add CameraHalConfig\n\tclass","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16106,"web_url":"https://patchwork.libcamera.org/comment/16106/","msgid":"","date":"2021-04-03T02:55:53","subject":"Re: [libcamera-devel] [PATCH v2 3/6] android: Add CameraHalConfig\n\tclass","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Tue, Mar 30, 2021 at 12:37:30PM +0200, Jacopo Mondi wrote:\n> On Tue, Mar 30, 2021 at 03:39:56AM +0300, Laurent Pinchart wrote:\n> > On Mon, Mar 29, 2021 at 05:28:04PM +0200, Jacopo Mondi wrote:\n> > > Add a CameraHalConfig class to the Android Camera3 HAL layer.\n> >\n> > There are quite a few comments from v1 that haven't been addressed.\n> > Maybe that's because my last reply to v1 came after you've sent this\n> > series. The comments may also not all be correct, applicable and/or\n> > desired, but could you then reply to them to explain why they're ignored\n> > ?\n> >\n> > In particular, I think that exposing CameraProps would simplify the\n> > implementation quite a bit, by removing the need for the camera*()\n> > functions, with the lookup of the CameraProps entry every time, and the\n> > associated error handling.\n> >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > README.rst | 2 +-\n> > > src/android/camera_hal_config.cpp | 463 ++++++++++++++++++++++++++++++\n> > > src/android/camera_hal_config.h | 53 ++++\n> > > src/android/meson.build | 2 +\n> > > 4 files changed, 519 insertions(+), 1 deletion(-)\n> > > create mode 100644 src/android/camera_hal_config.cpp\n> > > create mode 100644 src/android/camera_hal_config.h\n> > >\n> > > diff --git a/README.rst b/README.rst\n> > > index 7a98164bbb0a..f68d435196de 100644\n> > > --- a/README.rst\n> > > +++ b/README.rst\n> > > @@ -88,7 +88,7 @@ for tracing with lttng: [optional]\n> > > liblttng-ust-dev python3-jinja2 lttng-tools\n> > >\n> > > for android: [optional]\n> > > - libexif libjpeg\n> > > + libexif libjpeg libyaml\n> > >\n> > > Using GStreamer plugin\n> > > ~~~~~~~~~~~~~~~~~~~~~~\n> > > diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp\n> > > new file mode 100644\n> > > index 000000000000..846dd7357b0a\n> > > --- /dev/null\n> > > +++ b/src/android/camera_hal_config.cpp\n> > > @@ -0,0 +1,463 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2021, Google Inc.\n> > > + *\n> > > + * camera_hal_config.cpp - Camera HAL configuration file manager\n> > > + */\n> > > +#include \"camera_hal_config.h\"\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \"libcamera/internal/file.h\"\n> > > +#include \"libcamera/internal/log.h\"\n> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +LOG_DEFINE_CATEGORY(HALConfig)\n> > > +\n> > > +CameraHalConfig::CameraHalConfig()\n> > > +{\n> > > +\tif (!yaml_parser_initialize(&parser_))\n> > > +\t\tLOG(HALConfig, Fatal) << \"Failed to initialize yaml parser\";\n> > > +}\n> > > +\n> > > +CameraHalConfig::~CameraHalConfig()\n> > > +{\n> > > +\tyaml_parser_delete(&parser_);\n> > > +}\n> > > +\n> > > +/*\n> > > + * Configuration files can be stored in system paths, which are identified\n> > > + * through the build configuration.\n> > > + *\n> > > + * However, when running uninstalled - the source location takes precedence.\n> > > + */\n> > > +std::string CameraHalConfig::findFilePath(const std::string &filename) const\n> > > +{\n> > > +\tstatic std::array searchPaths = {\n> > > +\t\tLIBCAMERA_SYSCONF_DIR,\n> > > +\t\tLIBCAMERA_DATA_DIR,\n> >\n> > There's also a comment in v1 about data dir. Data dir is typically\n> > /usr/share/libcamera/. Given that the configuration file is\n> > device-specific, I don't think we'll ever look for it there, but only in\n> > sysconf dir.\n> >\n> > > +\t};\n> > > +\n> > > +\tif (File::exists(filename))\n> > > +\t\treturn filename;\n> > > +\n> > > +\tstd::string root = utils::libcameraSourcePath();\n> > > +\tif (!root.empty()) {\n> > > +\t\tstd::string configurationPath = root + \"data/\" + filename;\n> > > +\t\tif (File::exists(configurationPath))\n> > > +\t\t\treturn configurationPath;\n> > > +\t}\n> > > +\n> > > +\tfor (std::string &path : searchPaths) {\n> > > +\t\tstd::string configurationPath = path + \"/\" + filename;\n> > > +\t\tif (File::exists(configurationPath))\n> > > +\t\t\treturn configurationPath;\n> > > +\t}\n> > > +\n> > > +\treturn \"\";\n> > > +}\n> > > +\n> > > +FILE *CameraHalConfig::openFile(const std::string &filename)\n> > > +{\n> > > +\tconst std::string filePath = findFilePath(filename);\n> > > +\tif (filePath.empty()) {\n> > > +\t\tLOG(HALConfig, Error)\n> > > +\t\t\t<< \"Configuration file: \\\"\" << filename << \"\\\" not found\";\n> > > +\t\treturn nullptr;\n> > > +\t}\n> > > +\n> > > +\tLOG(HALConfig, Debug) << \"Reading configuration file from \" << filePath;\n> > > +\n> > > +\tFILE *fh = fopen(filePath.c_str(), \"r\");\n> > > +\tif (!fh) {\n> > > +\t\tint ret = -errno;\n> > > +\t\tLOG(HALConfig, Error) << \"Failed to open configuration file \"\n> > > +\t\t\t\t << filePath << \": \" << strerror(-ret);\n> > > +\t\treturn nullptr;\n> > > +\t}\n> > > +\n> > > +\treturn fh;\n> > > +}\n> > > +\n> > > +/*\n> > > + * Open the HAL configuration file and validate its content.\n> > > + * Return 0 on success, a negative error code otherwise\n> > > + */\n> > > +int CameraHalConfig::open()\n> > > +{\n> > > +\tint ret;\n> > > +\n> > > +\tFILE *fh = openFile(\"camera_hal.yaml\");\n> > > +\tif (!fh)\n> > > +\t\treturn -ENOENT;\n> > > +\n> > > +\tyaml_parser_set_input_file(&parser_, fh);\n> > > +\tret = parseConfigFile();\n> > > +\tfclose(fh);\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > > +\n> > > +\tLOG(HALConfig, Info) << \"Device model: \" << model_;\n> > > +\tLOG(HALConfig, Info) << \"Device maker: \" << maker_;\n> > > +\tfor (const auto &c : cameras_) {\n> > > +\t\tconst std::string &cameraId = c.first;\n> > > +\t\tconst CameraProps &camera = c.second;\n> > > +\t\tLOG(HALConfig, Info) << \"'\" << cameraId << \"' \"\n> > > +\t\t\t\t << \" model: \" << camera.model\n> > > +\t\t\t\t << \"(\" << camera.location << \")[\"\n> > > +\t\t\t\t << camera.rotation << \"]\";\n> > > +\t}\n> >\n> > There's also a comment in v1 about whether this should be a debugging\n> > feature.\n> >\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +int CameraHalConfig::cameraLocation(const std::string &camera) const\n> > > +{\n> > > +\tconst auto &it = cameras_.find(camera);\n> > > +\tif (it == cameras_.end()) {\n> > > +\t\tLOG(HALConfig, Error)\n> > > +\t\t\t<< \"Camera '\" << camera\n> > > +\t\t\t<< \"' not described in the HAL configuration file\";\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > +\n> > > +\tconst CameraProps &cam = it->second;\n> > > +\tif (cam.location.empty()) {\n> > > +\t\tLOG(HALConfig, Error) << \"Location for camera '\" << camera\n> > > +\t\t\t\t << \"' not available\";\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > +\n> > > +\tif (cam.location == \"front\")\n> > > +\t\treturn CAMERA_FACING_FRONT;\n> > > +\telse if (cam.location == \"back\")\n> > > +\t\treturn CAMERA_FACING_BACK;\n> > > +\telse if (cam.location == \"external\")\n> > > +\t\treturn CAMERA_FACING_EXTERNAL;\n> > > +\n> > > +\tLOG(HALConfig, Error) << \"Unsupported camera location \" << cam.location;\n> > > +\treturn -EINVAL;\n> >\n> > Can we do this conversion when parsing the configuration file instead ?\n> > That will ensure at parse time that the data is valid, instead of\n> > delaying the check to later. The rotation is already handled that way,\n> > and you already check that the location string is valid in\n> > CameraHalConfig::parseCameraProps().\n> \n> ack\n> \n> > > +}\n> > > +\n> > > +unsigned int CameraHalConfig::cameraRotation(const std::string &camera) const\n> > > +{\n> > > +\tconst auto &it = cameras_.find(camera);\n> > > +\tif (it == cameras_.end()) {\n> > > +\t\tLOG(HALConfig, Error)\n> > > +\t\t\t<< \"Camera '\" << camera\n> > > +\t\t\t<< \"' not described in the HAL configuration file\";\n> > > +\t\treturn 0;\n> > > +\t}\n> > > +\n> > > +\tconst CameraProps &cam = it->second;\n> > > +\treturn cam.rotation;\n> > > +}\n> > > +\n> > > +const std::string &CameraHalConfig::cameraModel(const std::string &camera) const\n> > > +{\n> > > +\tstatic const std::string empty(\"\");\n> > > +\tconst auto &it = cameras_.find(camera);\n> > > +\tif (it == cameras_.end()) {\n> > > +\t\tLOG(HALConfig, Error)\n> > > +\t\t\t<< \"Camera '\" << camera\n> > > +\t\t\t<< \"' not described in the HAL configuration file\";\n> > > +\t\treturn empty;\n> > > +\t}\n> > > +\n> > > +\tconst CameraProps &cam = it->second;\n> > > +\treturn cam.model;\n> > > +}\n> > > +\n> > > +std::string CameraHalConfig::parseValue(yaml_token_t &token)\n> > > +{\n> > > +\t/* Make sure the token type is a value and get its content. */\n> > > +\tyaml_parser_scan(&parser_, &token);\n> > > +\tif (token.type != YAML_VALUE_TOKEN) {\n> > > +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > +\t\treturn \"\";\n> > > +\t}\n> > > +\tyaml_token_delete(&token);\n> >\n> > Does the token need to be deleted in the error cases above and below ?\n> \n> Ah yes, most probably. I thought it had to be deleted if resued, but\n> it seems deletion is just about releasing resources allocated during\n> parser_scan(), so it needs to be done in error paths too\n> \n> I hate I can't goto easily in C++ to implement saner error handling\n> paths\n\nYou could wrap the token in a C++ class with a destructor :-) Possibly\noverkill, and most probably something we'll handle later, when we'll\nhave multiple parsers.\n\n> > > +\n> > > +\tyaml_parser_scan(&parser_, &token);\n> > > +\tif (token.type != YAML_SCALAR_TOKEN) {\n> > > +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > +\t\treturn \"\";\n> > > +\t}\n> > > +\n> > > +\tchar *scalar = reinterpret_cast(token.data.scalar.value);\n> > > +\tstd::string value(scalar, token.data.scalar.length);\n> > > +\tyaml_token_delete(&token);\n> > > +\n> > > +\treturn value;\n> > > +}\n> > > +\n> > > +std::string CameraHalConfig::parseKey(yaml_token_t &token)\n> > > +{\n> > > +\t/* Make sure the token type is a key and get its value. */\n> > > +\tyaml_parser_scan(&parser_, &token);\n> > > +\tif (token.type != YAML_SCALAR_TOKEN) {\n> > > +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > +\t\treturn \"\";\n> > > +\t}\n> > > +\n> > > +\tchar *scalar = reinterpret_cast(token.data.scalar.value);\n> > > +\tstd::string key(scalar, token.data.scalar.length);\n> > > +\tyaml_token_delete(&token);\n> > > +\n> > > +\treturn key;\n> > > +}\n> > > +\n> > > +int CameraHalConfig::parseCameraProps(yaml_token_t &token,\n> > > +\t\t\t\t const std::string &cameraID)\n> > > +{\n> > > +\tyaml_parser_scan(&parser_, &token);\n> > > +\tif (token.type != YAML_VALUE_TOKEN) {\n> > > +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > +\tyaml_token_delete(&token);\n> > > +\n> > > +\tyaml_parser_scan(&parser_, &token);\n> > > +\tif (token.type != YAML_BLOCK_MAPPING_START_TOKEN) {\n> > > +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > +\tyaml_token_delete(&token);\n> > > +\n> > > +\t/*\n> > > +\t * Parse the camera properties and store them in a cameraBlock instance.\n> > > +\t *\n> > > +\t * Add a safety counter to make sure we don't loop indefinitely in case\n> > > +\t * the configuration file is malformed.\n> > > +\t */\n> > > +\tunsigned int sentinel = 100;\n> > > +\tCameraProps cameraBlock{};\n> > > +\tbool blockEnd = false;\n> > > +\tdo {\n> > > +\t\tyaml_parser_scan(&parser_, &token);\n> > > +\t\tswitch (token.type) {\n> > > +\t\tcase YAML_KEY_TOKEN: {\n> > > +\t\t\tyaml_token_delete(&token);\n> > > +\n> > > +\t\t\t/*\n> > > +\t\t\t * Parse the camera property key and make sure it is\n> > > +\t\t\t * valid.\n> > > +\t\t\t */\n> > > +\t\t\tstd::string key = parseKey(token);\n> > > +\t\t\tstd::string value = parseValue(token);\n> > > +\t\t\tif (key.empty() || value.empty()) {\n> > > +\t\t\t\tLOG(HALConfig, Error)\n> > > +\t\t\t\t\t<< \"Configuration file is not valid\";\n> > > +\t\t\t\treturn -EINVAL;\n> > > +\t\t\t}\n> > > +\n> > > +\t\t\tif (key == \"location\") {\n> > > +\t\t\t\tif (value != \"front\" && value != \"back\" &&\n> > > +\t\t\t\t value != \"external\") {\n> > > +\t\t\t\t\tLOG(HALConfig, Error)\n> > > +\t\t\t\t\t\t<< \"Unknown location: \" << value;\n> > > +\t\t\t\t\treturn -EINVAL;\n> > > +\t\t\t\t}\n> > > +\t\t\t\tcameraBlock.location = value;\n> > > +\t\t\t} else if (key == \"rotation\") {\n> > > +\t\t\t\tcameraBlock.rotation = strtoul(value.c_str(),\n> > > +\t\t\t\t\t\t\t NULL, 10);\n> > > +\t\t\t\tif (cameraBlock.rotation < 0) {\n> > > +\t\t\t\t\tLOG(HALConfig, Error)\n> > > +\t\t\t\t\t\t<< \"Unknown rotation: \"\n> > > +\t\t\t\t\t\t<< cameraBlock.rotation;\n> > > +\t\t\t\t\treturn -EINVAL;\n> > > +\t\t\t\t}\n> > > +\t\t\t} else if (key == \"model\") {\n> > > +\t\t\t\tcameraBlock.model = value;\n> > > +\t\t\t} else {\n> > > +\t\t\t\tLOG(HALConfig, Error)\n> > > +\t\t\t\t\t<< \"Configuration file is not valid \"\n> > > +\t\t\t\t\t<< \"unknown key: \" << key;\n> > > +\t\t\t\treturn -EINVAL;\n> > > +\t\t\t}\n> > > +\t\t\tbreak;\n> > > +\t\t}\n> > > +\t\tcase YAML_BLOCK_END_TOKEN:\n> > > +\t\t\tyaml_token_delete(&token);\n> > > +\t\t\tblockEnd = true;\n> > > +\t\t\tbreak;\n> > > +\t\tdefault:\n> > > +\t\t\tyaml_token_delete(&token);\n> > > +\t\t\tbreak;\n> > > +\t\t}\n> > > +\n> > > +\t\t--sentinel;\n> > > +\t} while (!blockEnd && sentinel);\n> >\n> > Missing blank line.\n> \n> Intentional\n> \n> > > +\tif (!sentinel) {\n> > > +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid \";\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > +\n> > > +\tcameras_[cameraID] = cameraBlock;\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +int CameraHalConfig::parseCameras(yaml_token_t &token)\n> > > +{\n> > > +\tint ret;\n> > > +\n> > > +\t/* The 'cameras' key maps a BLOCK_MAPPING_START block. */\n> > > +\tyaml_parser_scan(&parser_, &token);\n> > > +\tif (token.type != YAML_VALUE_TOKEN) {\n> > > +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > +\tyaml_token_delete(&token);\n> > > +\n> > > +\tyaml_parser_scan(&parser_, &token);\n> > > +\tif (token.type != YAML_BLOCK_MAPPING_START_TOKEN) {\n> > > +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > +\tyaml_token_delete(&token);\n> > > +\n> > > +\t/*\n> > > +\t * Parse the camera properties.\n> > > +\t *\n> > > +\t * Each camera properties block is a list of properties associated\n> > > +\t * with the ID (as assembled by CameraSensor::generateId()) of the\n> > > +\t * camera they refer to.\n> > > +\t *\n> > > +\t * cameras:\n> > > +\t * \"camera0 id\":\n> > > +\t * key: value\n> > > +\t * key: value\n> > > +\t * ...\n> > > +\t *\n> > > +\t * \"camera1 id\":\n> > > +\t * key: value\n> > > +\t * key: value\n> > > +\t * ...\n> > > +\t */\n> > > +\tbool blockEnd = false;\n> > > +\tdo {\n> > > +\t\tyaml_parser_scan(&parser_, &token);\n> > > +\t\tswitch (token.type) {\n> > > +\t\tcase YAML_KEY_TOKEN: {\n> > > +\t\t\tyaml_token_delete(&token);\n> > > +\n> > > +\t\t\t/* Parse the camera ID as key of the property list. */\n> > > +\t\t\tstd::string cameraId = parseKey(token);\n> > > +\t\t\tif (cameraId.empty()) {\n> > > +\t\t\t\tLOG(HALConfig, Error)\n> > > +\t\t\t\t\t<< \"Configuration file is not valid\";\n> > > +\t\t\t\treturn -EINVAL;\n> > > +\t\t\t}\n> > > +\n> > > +\t\t\tret = parseCameraProps(token, cameraId);\n> > > +\t\t\tif (ret) {\n> > > +\t\t\t\tLOG(HALConfig, Error)\n> > > +\t\t\t\t\t<< \"Configuration file is not valid\";\n> > > +\t\t\t\treturn -EINVAL;\n> > > +\t\t\t}\n> > > +\t\t\tbreak;\n> > > +\t\t}\n> > > +\t\tcase YAML_BLOCK_END_TOKEN:\n> > > +\t\t\tyaml_token_delete(&token);\n> > > +\t\t\tblockEnd = true;\n> > > +\t\t\tbreak;\n> > > +\t\tdefault:\n> > > +\t\t\tyaml_token_delete(&token);\n> > > +\t\t\tLOG(HALConfig, Error)\n> > > +\t\t\t\t<< \"Configuration file is not valid\";\n> > > +\t\t\treturn -EINVAL;\n> > > +\t\t}\n> > > +\t} while (!blockEnd);\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +int CameraHalConfig::parseEntry(yaml_token_t &token)\n> > > +{\n> > > +\tint ret;\n> > > +\n> > > +\t/*\n> > > +\t * Parse each key we find in the file.\n> > > +\t *\n> > > +\t * Keys like 'model' and 'maker' are device properties and gets\n> > > +\t * stored as class members.\n> > > +\t *\n> > > +\t * The 'cameras' keys maps to a list of (lists) of camera properties.\n> > > +\t */\n> > > +\n> > > +\tstd::string key = parseKey(token);\n> > > +\tif (key.empty()) {\n> > > +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > +\n> > > +\tif (key == \"cameras\") {\n> > > +\t\tret = parseCameras(token);\n> > > +\t\tif (ret)\n> > > +\t\t\treturn ret;\n> > > +\t} else if (key == \"manufacturer\") {\n> > > +\t\tmaker_ = parseValue(token);\n> > > +\t\tif (maker_.empty()) {\n> > > +\t\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > +\t\t\treturn -EINVAL;\n> > > +\t\t}\n> > > +\t} else if (key == \"model\") {\n> > > +\t\tmodel_ = parseValue(token);\n> > > +\t\tif (model_.empty()) {\n> > > +\t\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > +\t\t\treturn -EINVAL;\n> > > +\t\t}\n> > > +\t} else {\n> > > +\t\tLOG(HALConfig, Error) << \"Unknown key \" << key;\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +int CameraHalConfig::parseConfigFile()\n> > > +{\n> > > +\tyaml_token_t token;\n> > > +\tint ret;\n> > > +\n> > > +\tyaml_parser_scan(&parser_, &token);\n> > > +\tif (token.type != YAML_STREAM_START_TOKEN) {\n> > > +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > +\tyaml_token_delete(&token);\n> > > +\n> > > +\tyaml_parser_scan(&parser_, &token);\n> > > +\tif (token.type != YAML_BLOCK_MAPPING_START_TOKEN) {\n> > > +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > +\tyaml_token_delete(&token);\n> > > +\n> > > +\t/* Parse the file and parse each single key one by one. */\n> > > +\tdo {\n> > > +\t\tyaml_parser_scan(&parser_, &token);\n> > > +\t\tswitch (token.type) {\n> > > +\t\tcase YAML_KEY_TOKEN:\n> > > +\t\t\tyaml_token_delete(&token);\n> > > +\t\t\tret = parseEntry(token);\n> >\n> > Do you need to pass the token to parseEntry(), given that you've just\n> > deleted it ? It looks to me like the token is only the return value of\n> > yaml_parse_scan(), and can thus be a local variable everywhere.\n> \n> Good question. Where's the parsing state kept ? in the token or in the\n> parser ? If it's in the parser I can avoid passing token around\n\nIt's in the parser. yaml_parser_scan() starts with\n\n\tmemset(token, 0, sizeof(yaml_token_t));\n\n> > > +\t\t\tbreak;\n> > > +\t\tcase YAML_STREAM_END_TOKEN:\n> > > +\t\t\t/* Resources are released after the loop exit. */\n> > > +\t\t\tbreak;\n> > > +\t\tdefault:\n> > > +\t\t\tyaml_token_delete(&token);\n> > > +\t\t\tbreak;\n> >\n> > The YAML_STREAM_END_TOKEN and default case are different, but the\n> > while() condition below accesses token.type. Is that intended ?\n> \n> Yes, the type has to stay valid in case of STREAM_END to be verified\n> below. I can handle this with just ret if you prefer.\n> \n> > > +\t\t}\n> > > +\t} while (token.type != YAML_STREAM_END_TOKEN && ret >= 0);\n> > > +\tyaml_token_delete(&token);\n> > > +\n> > > +\treturn ret;\n> > > +}\n> > > diff --git a/src/android/camera_hal_config.h b/src/android/camera_hal_config.h\n> > > new file mode 100644\n> > > index 000000000000..ce77b1ee1582\n> > > --- /dev/null\n> > > +++ b/src/android/camera_hal_config.h\n> > > @@ -0,0 +1,53 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2021, Google Inc.\n> > > + *\n> > > + * camera_hal_config.h - Camera HAL configuration file manager\n> > > + */\n> > > +#ifndef __ANDROID_CAMERA_HAL_CONFIG_H__\n> > > +#define __ANDROID_CAMERA_HAL_CONFIG_H__\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> >\n> > As commented in v1, hiding the parser would be nice, but it can be done\n> > later, on top.\n> >\n> > > +\n> > > +class CameraHalConfig\n> > > +{\n> > > +public:\n> > > +\tCameraHalConfig();\n> > > +\t~CameraHalConfig();\n> > > +\n> > > +\tint open();\n> > > +\n> > > +\tconst std::string &deviceModel() const { return model_; }\n> > > +\tconst std::string &deviceMaker() const { return maker_; }\n> > > +\n> > > +\tint cameraLocation(const std::string &camera) const;\n> > > +\tunsigned int cameraRotation(const std::string &camera) const;\n> > > +\tconst std::string &cameraModel(const std::string &camera) const;\n> > > +\n> > > +private:\n> > > +\tyaml_parser_t parser_;\n> > > +\n> > > +\tstd::string model_;\n> > > +\tstd::string maker_;\n> > > +\tclass CameraProps\n> > > +\t{\n> > > +\tpublic:\n> > > +\t\tstd::string location;\n> > > +\t\tstd::string model;\n> > > +\t\tunsigned int rotation;\n> > > +\t};\n> > > +\tstd::map cameras_;\n> > > +\n> > > +\tstd::string findFilePath(const std::string &filename) const;\n> > > +\tFILE *openFile(const std::string &filename);\n> > > +\tstd::string parseValue(yaml_token_t &token);\n> > > +\tstd::string parseKey(yaml_token_t &token);\n> > > +\tint parseCameraProps(yaml_token_t &token, const std::string &cameraID);\n> > > +\tint parseCameras(yaml_token_t &token);\n> > > +\tint parseEntry(yaml_token_t &token);\n> > > +\tint parseConfigFile();\n> > > +};\n> > > +\n> > > +#endif /* __ANDROID_CAMERA_HAL_CONFIG_H__ */\n> > > diff --git a/src/android/meson.build b/src/android/meson.build\n> > > index 8e7d07d9be3c..c0ede407bc38 100644\n> > > --- a/src/android/meson.build\n> > > +++ b/src/android/meson.build\n> > > @@ -3,6 +3,7 @@\n> > > android_deps = [\n> > > dependency('libexif', required : get_option('android')),\n> > > dependency('libjpeg', required : get_option('android')),\n> > > + dependency('yaml-0.1', required : get_option('android')),\n> > > ]\n> > >\n> > > android_enabled = true\n> > > @@ -41,6 +42,7 @@ android_hal_sources = files([\n> > > 'camera3_hal.cpp',\n> > > 'camera_hal_manager.cpp',\n> > > 'camera_device.cpp',\n> > > + 'camera_hal_config.cpp',\n> >\n> > While at it, could you move camera_hal_manager to the right place ?\n> >\n> > > 'camera_metadata.cpp',\n> > > 'camera_ops.cpp',\n> > > 'camera_stream.cpp',","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 04705BD695\n\tfor ;\n\tSat, 3 Apr 2021 02:56:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 719FF6084F;\n\tSat, 3 Apr 2021 04:56:40 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0B8E4602D7\n\tfor ;\n\tSat, 3 Apr 2021 04:56:39 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5E6503D7;\n\tSat, 3 Apr 2021 04:56:38 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"rBlGYgRo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1617418598;\n\tbh=/eMrxyfGsOiggoLDRlDE9t/N7V2pNWcAZ1etFcfTIN4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rBlGYgRoUCvMGyB6PHC9y62Ea/bjOa45qtq+N4DJGTUhEJ614RPl7bSjMyp25Ey6M\n\t5Q0lYwt+rQ3F9Y6SfJjHVIeOnyX5WH74oWbPoEUgnblTcPA0SjubLSjCOARLiHKHP8\n\t3fFPtqLGCTIDieOny793oC1zD3VkeRGA/Sr1RG/0=","Date":"Sat, 3 Apr 2021 05:55:53 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210329152807.28331-1-jacopo@jmondi.org>\n\t<20210329152807.28331-3-jacopo@jmondi.org>\n\t\n\t<20210330103730.iocnmtlexhvaafuj@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210330103730.iocnmtlexhvaafuj@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 3/6] android: Add CameraHalConfig\n\tclass","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]