[{"id":15906,"web_url":"https://patchwork.libcamera.org/comment/15906/","msgid":"","date":"2021-03-25T22:33:31","subject":"Re: [libcamera-devel] [PATCH 4/7] android: Add CameraHalConfig class","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your work.\n\nOn 2021-03-24 12:25:24 +0100, Jacopo Mondi wrote:\n> Add a CameraHalConfig class to the Android Camera3 HAL layer.\n> \n> Signed-off-by: Jacopo Mondi \n\nThere are a few small nits below but I think this is a great step in the \nright direction.\n\nReviewed-by: Niklas Söderlund \n\n> ---\n> src/android/camera_hal_config.cpp | 385 ++++++++++++++++++++++++++++++\n> src/android/camera_hal_config.h | 54 +++++\n> src/android/meson.build | 1 +\n> 3 files changed, 440 insertions(+)\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/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp\n> new file mode 100644\n> index 000000000000..7e33dfb750ff\n> --- /dev/null\n> +++ b/src/android/camera_hal_config.cpp\n> @@ -0,0 +1,385 @@\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 \"libcamera/internal/file.h\"\n> +#include \"libcamera/internal/log.h\"\n> +\n> +using namespace libcamera;\n> +\n> +LOG_DEFINE_CATEGORY(HALConfig)\n> +\n> +const std::string CameraHalConfig::CameraBlock::toString() const\n> +{\n> +\tstd::stringstream ss;\n> +\n> +\tss << \"\\'\" << name << \"\\'\"\n> +\t << \" model: \" << model\n> +\t << \"(\" << location << \")[\" << rotation << \"]\";\n\nnit: The usage of the pattern \"key: \" have been discouraged in \nthe core, do we extend the same to the HAL?\n\n> +\n> +\treturn ss.str();\n> +}\n> +\n> +CameraHalConfig::CameraHalConfig()\n> +{\n> +\tif (!yaml_parser_initialize(&parser_))\n> +\t\tLOG(HALConfig, Error) << \"Failed to initialize yaml parser\";\n\nFatal?\n\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> +const std::string CameraHalConfig::findFilePath(const std::string &filename)\n> +{\n> +\tstatic std::array searchPaths = {\n> +\t\tLIBCAMERA_SYSCONF_DIR,\n> +\t\tLIBCAMERA_DATA_DIR,\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> +\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> +/*\n> + * \\brief Open the HAL configuration file and validate its content\n> + * \\return 0 on success, a negative error code otherwise\n\nnit: We don't Doxygen the HAL is it not confusing to use it mark up here \nthen?\n\n> + */\n> +int CameraHalConfig::open()\n> +{\n> +\tint ret;\n> +\n> +\tconst std::string configPath = \"camera_hal.yaml\";\n\ns/configPath/configFile/ ?\n\n> +\tconst std::string filePath = findFilePath(configPath);\n\nAs this is the only call site would it make sens to have findFilePath() \nreturn a FILE * to get compartmentalizing of the error paths?\n\n> +\tif (filePath.empty()) {\n> +\t\tLOG(HALConfig, Warning)\n> +\t\t\t<< \"File: \\\"\" << configPath << \"\\\" not found\";\n> +\t\treturn -ENOENT;\n> +\t}\n> +\n> +\tFILE *fh = fopen(filePath.c_str(), \"r\");\n> +\tif (!fh) {\n> +\t\tret = -errno;\n> +\t\tLOG(HALConfig, Error) << \"Failed to open file \" << filePath\n> +\t\t\t\t << \": \" << strerror(-ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tyaml_parser_set_input_file(&parser_, fh);\n> +\n> +\tLOG(HALConfig, Debug) << \"Reading configuration from \" << filePath;\n> +\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 CameraBlock camera = c.second;\n> +\t\tLOG(HALConfig, Info) << camera.toString();\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +const std::string &CameraHalConfig::cameraLocation(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 CameraBlock &cam = it->second;\n> +\treturn cam.location;\n> +}\n> +\n> +unsigned int CameraHalConfig::cameraRotation(const std::string &camera) const\n> +{\n> +\tstatic const std::string empty(\"\");\n\nempty not used.\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 CameraBlock &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 CameraBlock &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> +\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_KEY_TOKEN) {\n> +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> +\t\treturn \"\";\n> +\t}\n> +\tyaml_token_delete(&token);\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 key(scalar, token.data.scalar.length);\n> +\tyaml_token_delete(&token);\n> +\n> +\treturn key;\n> +}\n> +\n> +int CameraHalConfig::parseCameraBlock(yaml_token_t &token)\n> +{\n> +\t/* The 'camera' block is a VALUE token type. */\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> +\t/*\n> +\t * Parse the content of the camera block until we have an unbalanced\n> +\t * BLOCK_END_TOKEN which closes the camera block.\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> +\tCameraBlock cameraBlock{};\n> +\tint blockCounter = 0;\n> +\tdo {\n> +\t\tyaml_parser_scan(&parser_, &token);\n> +\t\tswitch (token.type) {\n> +\t\tcase YAML_BLOCK_ENTRY_TOKEN:\n> +\t\t\tyaml_token_delete(&token);\n> +\t\t\tblockCounter++;\n> +\t\t\tbreak;\n> +\t\tcase YAML_BLOCK_END_TOKEN:\n> +\t\t\tyaml_token_delete(&token);\n> +\t\t\tblockCounter--;\n> +\t\t\tbreak;\n> +\t\tcase YAML_BLOCK_MAPPING_START_TOKEN: {\n> +\t\t\tyaml_token_delete(&token);\n> +\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\t/* Validate that the parsed key is valid. */\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 == \"name\") {\n> +\t\t\t\tcameraBlock.name = value;\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\tdefault:\n> +\t\t\tyaml_token_delete(&token);\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\t--sentinel;\n> +\t} while (blockCounter >= 0 && sentinel);\n> +\tif (!sentinel) {\n> +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid \";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tcameras_[cameraBlock.name] = cameraBlock;\n> +\n> +\treturn 0;\n> +}\n> +\n> +int CameraHalConfig::parseEntryBlock(yaml_token_t &token)\n> +{\n> +\tint ret;\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> +\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 == \"camera\") {\n> +\t\tyaml_token_delete(&token);\n> +\t\tret = parseCameraBlock(token);\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\t} else if (key == \"manufacturer\") {\n> +\t\tyaml_token_delete(&token);\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\tyaml_token_delete(&token);\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\tyaml_token_delete(&token);\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> +\t/*\n> +\t * Start parsing the content of the configuration file which\n> +\t * starts as with sequence block.\n> +\t */\n> +\tyaml_parser_scan(&parser_, &token);\n> +\tif (token.type != YAML_BLOCK_SEQUENCE_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 single entry blocks. */\n> +\tdo {\n> +\t\tyaml_parser_scan(&parser_, &token);\n> +\t\tswitch (token.type) {\n> +\t\tcase YAML_BLOCK_ENTRY_TOKEN:\n> +\t\t\tyaml_token_delete(&token);\n> +\t\t\tret = parseEntryBlock(token);\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\t/* Release resources before re-parsing. */\n> +\t\t\tyaml_token_delete(&token);\n> +\t\t\tbreak;\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..69d42658e90a\n> --- /dev/null\n> +++ b/src/android/camera_hal_config.h\n> @@ -0,0 +1,54 @@\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> +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> +\tconst std::string &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 CameraBlock\n> +\t{\n> +\tpublic:\n> +\t\tstd::string name;\n> +\t\tstd::string location;\n> +\t\tstd::string model;\n> +\t\tunsigned int rotation;\n> +\n> +\t\tconst std::string toString() const;\n> +\t};\n> +\tstd::map cameras_;\n> +\n> +\tconst std::string findFilePath(const std::string &filename);\n> +\tstd::string parseValue(yaml_token_t &token);\n> +\tstd::string parseKey(yaml_token_t &token);\n> +\tint parseCameraBlock(yaml_token_t &token);\n> +\tint parseEntryBlock(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 19f94a4073f1..34e5c494a492 100644\n> --- a/src/android/meson.build\n> +++ b/src/android/meson.build\n> @@ -44,6 +44,7 @@ android_hal_sources = files([\n> 'camera3_hal.cpp',\n> 'camera_hal_manager.cpp',\n> 'camera_device.cpp',\n> + 'camera_hal_config.cpp',\n> 'camera_metadata.cpp',\n> 'camera_ops.cpp',\n> 'camera_stream.cpp',\n> -- \n> 2.30.0\n> \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 3D5ACC32EA\n\tfor ;\n\tThu, 25 Mar 2021 22:33:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9F864603FF;\n\tThu, 25 Mar 2021 23:33:37 +0100 (CET)","from mail-lj1-x229.google.com (mail-lj1-x229.google.com\n\t[IPv6:2a00:1450:4864:20::229])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D9E28603FE\n\tfor ;\n\tThu, 25 Mar 2021 23:33:35 +0100 (CET)","by mail-lj1-x229.google.com with SMTP id z25so5198215lja.3\n\tfor ;\n\tThu, 25 Mar 2021 15:33:35 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tu28sm668111lfl.302.2021.03.25.15.33.32\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 25 Mar 2021 15:33:33 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"NKFqMIdg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=CnNTxgorp9NvDgAR+GHdj6Mg6hv5yyGavq9ZpvStvM4=;\n\tb=NKFqMIdggHjCSjRHE9ibcIMD40+nJAtDhbNfAGv+bE0wTsKTxhPwA9P4zSaIQyv6QU\n\tvslENgEpTCtfpOudopJVHwobawPYM1TkJUQy4RTqMIrdiMtb8VIEo2VsIN2cB3hGrZUE\n\tXD1pZT8vN6nq9qvk6g5Mzh7LjTxtDryc0KVeSXJIfLoJHFhqbYrwn5lXRkp6QYZWihFJ\n\twJJUUcEr+iolaKKGpvrBK3aFjiLREM+rf/uJqm70khZgl1NlvaUIT/k8pCHEhMy1sEMp\n\teX06L0uYTy0fiV1F/zOpGB8QvQoF9p9Nwgr+XItS/BABmyrrvtF5TbfYOUbo5LeIVbxq\n\t+aUg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=CnNTxgorp9NvDgAR+GHdj6Mg6hv5yyGavq9ZpvStvM4=;\n\tb=jfMZKItChRL0HjEviSbPdEbahFJXH+JysxDNueAW6SWFJCQbpe9gvlAGDbNRcV6648\n\tfAdP3Bsk7zSksl9GGbXtcLmY4SQKdmntdyBMGDjv+Bgedn/hxQZuZ+oVMsriK8VInsEF\n\tE7XikimKmvU9joIUIGLoGUJmO7A10WJGnq0Jqfzin3XrSoPkrE0Gcs8y0YHmn7YfCkTp\n\tOYwBff+KzN7yaG1G1u8dOIDW5DOK/1EssBmoZxlLPfAMhjZ0GD3HQaOUDXXKFFnK33MR\n\tM08VNitSrCq4X+5IjRQPSpHPX2JhrNPKRLSKQETbQskr55dDjSnpOp6+7RvDyIWbIYAd\n\tQmdw==","X-Gm-Message-State":"AOAM533fs+ouugf0jIVYWojQyGnwRzz8GRzcFs5UDhI82KtYmwJ3mEb5\n\trJjIrF1oN/0W2OSTuW1Mpv+Z7A==","X-Google-Smtp-Source":"ABdhPJyEns/wnsjq9ZWMuhZGPop4v6YGSTQ/s3IePJiQM60ttBTcR9vysbVmkhkeEz3DLTN2Gq6K1Q==","X-Received":"by 2002:a2e:9a42:: with SMTP id k2mr7202437ljj.363.1616711614587;\n\tThu, 25 Mar 2021 15:33:34 -0700 (PDT)","Date":"Thu, 25 Mar 2021 23:33:31 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210324112527.63701-1-jacopo@jmondi.org>\n\t<20210324112527.63701-5-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210324112527.63701-5-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 4/7] android: Add CameraHalConfig class","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=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15934,"web_url":"https://patchwork.libcamera.org/comment/15934/","msgid":"","date":"2021-03-26T04:07:07","subject":"Re: [libcamera-devel] [PATCH 4/7] android: Add CameraHalConfig class","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 Wed, Mar 24, 2021 at 12:25:24PM +0100, Jacopo Mondi wrote:\n> Add a CameraHalConfig class to the Android Camera3 HAL layer.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/android/camera_hal_config.cpp | 385 ++++++++++++++++++++++++++++++\n> src/android/camera_hal_config.h | 54 +++++\n> src/android/meson.build | 1 +\n> 3 files changed, 440 insertions(+)\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/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp\n> new file mode 100644\n> index 000000000000..7e33dfb750ff\n> --- /dev/null\n> +++ b/src/android/camera_hal_config.cpp\n> @@ -0,0 +1,385 @@\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 \"libcamera/internal/file.h\"\n> +#include \"libcamera/internal/log.h\"\n> +\n> +using namespace libcamera;\n> +\n> +LOG_DEFINE_CATEGORY(HALConfig)\n> +\n> +const std::string CameraHalConfig::CameraBlock::toString() const\n> +{\n> +\tstd::stringstream ss;\n> +\n> +\tss << \"\\'\" << name << \"\\'\"\n> +\t << \" model: \" << model\n> +\t << \"(\" << location << \")[\" << rotation << \"]\";\n> +\n> +\treturn ss.str();\n> +}\n\nAs the amount of information will grow, I don't think we'll be able to\nprint everything. This function is only used in a single location below,\nfor debugging purpose, I wonder if we should keep it.\n\n> +\n> +CameraHalConfig::CameraHalConfig()\n> +{\n> +\tif (!yaml_parser_initialize(&parser_))\n> +\t\tLOG(HALConfig, Error) << \"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\nCan we run the camera HAL uninstalled ? :-)\n\n> + */\n> +const std::string CameraHalConfig::findFilePath(const std::string &filename)\n> +{\n> +\tstatic std::array searchPaths = {\n> +\t\tLIBCAMERA_SYSCONF_DIR,\n> +\t\tLIBCAMERA_DATA_DIR,\n\nThe data dir may not be very useful here, the configuration file is\nreally system-specific, right ?\n\nI expect we may need to get the configuration file from system-specific\nlocations, depending on whether we run on Chrome OS or Android, but we\ncan handle that later.\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> +\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> +/*\n> + * \\brief 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> +\tconst std::string configPath = \"camera_hal.yaml\";\n> +\tconst std::string filePath = findFilePath(configPath);\n> +\tif (filePath.empty()) {\n> +\t\tLOG(HALConfig, Warning)\n\nThis can be an Error, as we can't continue.\n\n> +\t\t\t<< \"File: \\\"\" << configPath << \"\\\" not found\";\n\nMaybe s/File:/Configuration file/ ?\n\n> +\t\treturn -ENOENT;\n> +\t}\n> +\n> +\tFILE *fh = fopen(filePath.c_str(), \"r\");\n> +\tif (!fh) {\n> +\t\tret = -errno;\n> +\t\tLOG(HALConfig, Error) << \"Failed to open file \" << filePath\n\nSame here, \"configuration file\" ?\n\n> +\t\t\t\t << \": \" << strerror(-ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tyaml_parser_set_input_file(&parser_, fh);\n> +\n> +\tLOG(HALConfig, Debug) << \"Reading configuration from \" << filePath;\n> +\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 CameraBlock camera = c.second;\n> +\t\tLOG(HALConfig, Info) << camera.toString();\n> +\t}\n\nI wonder if dumping the parsed file shouldn't be a debugging feature\ninstead, or even if we need it at all.\n\n> +\n> +\treturn 0;\n> +}\n> +\n> +const std::string &CameraHalConfig::cameraLocation(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 CameraBlock &cam = it->second;\n> +\treturn cam.location;\n> +}\n> +\n> +unsigned int CameraHalConfig::cameraRotation(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 0;\n> +\t}\n> +\n> +\tconst CameraBlock &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 CameraBlock &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> +\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_KEY_TOKEN) {\n> +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> +\t\treturn \"\";\n> +\t}\n> +\tyaml_token_delete(&token);\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 key(scalar, token.data.scalar.length);\n> +\tyaml_token_delete(&token);\n> +\n> +\treturn key;\n> +}\n> +\n> +int CameraHalConfig::parseCameraBlock(yaml_token_t &token)\n> +{\n> +\t/* The 'camera' block is a VALUE token type. */\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> +\t/*\n> +\t * Parse the content of the camera block until we have an unbalanced\n> +\t * BLOCK_END_TOKEN which closes the camera block.\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> +\tCameraBlock cameraBlock{};\n> +\tint blockCounter = 0;\n> +\tdo {\n> +\t\tyaml_parser_scan(&parser_, &token);\n> +\t\tswitch (token.type) {\n> +\t\tcase YAML_BLOCK_ENTRY_TOKEN:\n> +\t\t\tyaml_token_delete(&token);\n> +\t\t\tblockCounter++;\n> +\t\t\tbreak;\n> +\t\tcase YAML_BLOCK_END_TOKEN:\n> +\t\t\tyaml_token_delete(&token);\n> +\t\t\tblockCounter--;\n> +\t\t\tbreak;\n> +\t\tcase YAML_BLOCK_MAPPING_START_TOKEN: {\n> +\t\t\tyaml_token_delete(&token);\n> +\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\t/* Validate that the parsed key is valid. */\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 == \"name\") {\n> +\t\t\t\tcameraBlock.name = value;\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\tdefault:\n> +\t\t\tyaml_token_delete(&token);\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\t--sentinel;\n> +\t} while (blockCounter >= 0 && sentinel);\n> +\tif (!sentinel) {\n> +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid \";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tcameras_[cameraBlock.name] = cameraBlock;\n> +\n> +\treturn 0;\n> +}\n> +\n> +int CameraHalConfig::parseEntryBlock(yaml_token_t &token)\n> +{\n> +\tint ret;\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> +\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 == \"camera\") {\n> +\t\tyaml_token_delete(&token);\n> +\t\tret = parseCameraBlock(token);\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\t} else if (key == \"manufacturer\") {\n> +\t\tyaml_token_delete(&token);\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\tyaml_token_delete(&token);\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\tyaml_token_delete(&token);\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\nJust to make sure you're aware of it, there's yaml_parser_load() that\nproduces a parsed document that can then be walked. There's nothing\nwrong about using event-based parsing as you do though.\n\nI've skipped review of the parser implementation itself, as it may be\nchanged depending on how patch 7/7 evolves.\n\n> +\tif (token.type != YAML_STREAM_START_TOKEN) {\n> +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> +\t\treturn EINVAL;\n\n-EINVAL\n\n> +\t}\n> +\tyaml_token_delete(&token);\n> +\n> +\t/*\n> +\t * Start parsing the content of the configuration file which\n> +\t * starts as with sequence block.\n\ns/as // ?\n\n> +\t */\n> +\tyaml_parser_scan(&parser_, &token);\n> +\tif (token.type != YAML_BLOCK_SEQUENCE_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 single entry blocks. */\n> +\tdo {\n> +\t\tyaml_parser_scan(&parser_, &token);\n> +\t\tswitch (token.type) {\n> +\t\tcase YAML_BLOCK_ENTRY_TOKEN:\n> +\t\t\tyaml_token_delete(&token);\n> +\t\t\tret = parseEntryBlock(token);\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\t/* Release resources before re-parsing. */\n> +\t\t\tyaml_token_delete(&token);\n> +\t\t\tbreak;\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..69d42658e90a\n> --- /dev/null\n> +++ b/src/android/camera_hal_config.h\n> @@ -0,0 +1,54 @@\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\nIt would be nice to hide the YAML perser from this header. It could be\ndone by moving the parsing to a CameraHalConfigParser class, private to\nthe camera_hal_config.cpp file.\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> +\tconst std::string &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 CameraBlock\n> +\t{\n> +\tpublic:\n> +\t\tstd::string name;\n> +\t\tstd::string location;\n> +\t\tstd::string model;\n> +\t\tunsigned int rotation;\n> +\n> +\t\tconst std::string toString() const;\n> +\t};\n\nHow about making this class public (and probably renaming it), and replacing the last three public\nfunctions with\n\n\tconst CameraBlock *cameraConfig(const std::string &name) const;\n\n? That way each CameraDevice could store a reference to the\ncorresponding camera configuration only.\n\n> +\tstd::map cameras_;\n> +\n> +\tconst std::string findFilePath(const std::string &filename);\n\nNo need for const at the beginning as you return by value, but you can\nadd a const at the end.\n\n> +\tstd::string parseValue(yaml_token_t &token);\n> +\tstd::string parseKey(yaml_token_t &token);\n> +\tint parseCameraBlock(yaml_token_t &token);\n> +\tint parseEntryBlock(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 19f94a4073f1..34e5c494a492 100644\n> --- a/src/android/meson.build\n> +++ b/src/android/meson.build\n> @@ -44,6 +44,7 @@ android_hal_sources = files([\n> 'camera3_hal.cpp',\n> 'camera_hal_manager.cpp',\n> 'camera_device.cpp',\n> + 'camera_hal_config.cpp',\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 53484BDC66\n\tfor ;\n\tFri, 26 Mar 2021 04:07:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A5E4668D66;\n\tFri, 26 Mar 2021 05:07:52 +0100 (CET)","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 ADB8C602E3\n\tfor ;\n\tFri, 26 Mar 2021 05:07:51 +0100 (CET)","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 06710443;\n\tFri, 26 Mar 2021 05:07:50 +0100 (CET)"],"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=\"HuGadFxm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616731671;\n\tbh=Je0lGsSdaeaA1HiNb0OSX+aQJxOnqod539i32oCGV4M=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HuGadFxmuPYr4qWOQ1wZcnx5A7ouHrPamiziB/h9cpvlueIsNSow234tB0bNHCd01\n\tEo+S4XV3g9A0NBbemd1BhTfDot5Ektv7iiVX74/TO/n+E3EgR9CxziAVdsGrd+UAsu\n\txc1zUQAb9IpQ9J+DYTQyhzkhtAQSwoJoyhYR+CFI=","Date":"Fri, 26 Mar 2021 06:07:07 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210324112527.63701-1-jacopo@jmondi.org>\n\t<20210324112527.63701-5-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210324112527.63701-5-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 4/7] android: Add CameraHalConfig class","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":16018,"web_url":"https://patchwork.libcamera.org/comment/16018/","msgid":"<20210329142308.sekalomdb3tqbpg7@uno.localdomain>","date":"2021-03-29T14:23:08","subject":"Re: [libcamera-devel] [PATCH 4/7] android: Add CameraHalConfig class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Thu, Mar 25, 2021 at 11:33:31PM +0100, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> Thanks for your work.\n>\n> On 2021-03-24 12:25:24 +0100, Jacopo Mondi wrote:\n> > Add a CameraHalConfig class to the Android Camera3 HAL layer.\n> >\n> > Signed-off-by: Jacopo Mondi \n>\n> There are a few small nits below but I think this is a great step in the\n> right direction.\n>\n> Reviewed-by: Niklas Söderlund \n>\n> > ---\n> > src/android/camera_hal_config.cpp | 385 ++++++++++++++++++++++++++++++\n> > src/android/camera_hal_config.h | 54 +++++\n> > src/android/meson.build | 1 +\n> > 3 files changed, 440 insertions(+)\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/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp\n> > new file mode 100644\n> > index 000000000000..7e33dfb750ff\n> > --- /dev/null\n> > +++ b/src/android/camera_hal_config.cpp\n> > @@ -0,0 +1,385 @@\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 \"libcamera/internal/file.h\"\n> > +#include \"libcamera/internal/log.h\"\n> > +\n> > +using namespace libcamera;\n> > +\n> > +LOG_DEFINE_CATEGORY(HALConfig)\n> > +\n> > +const std::string CameraHalConfig::CameraBlock::toString() const\n> > +{\n> > +\tstd::stringstream ss;\n> > +\n> > +\tss << \"\\'\" << name << \"\\'\"\n> > +\t << \" model: \" << model\n> > +\t << \"(\" << location << \")[\" << rotation << \"]\";\n>\n> nit: The usage of the pattern \"key: \" have been discouraged in\n> the core, do we extend the same to the HAL?\n>\n\nNot sure what you mean here -.-\n\n> > +\n> > +\treturn ss.str();\n> > +}\n> > +\n> > +CameraHalConfig::CameraHalConfig()\n> > +{\n> > +\tif (!yaml_parser_initialize(&parser_))\n> > +\t\tLOG(HALConfig, Error) << \"Failed to initialize yaml parser\";\n>\n> Fatal?\n>\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> > +const std::string CameraHalConfig::findFilePath(const std::string &filename)\n> > +{\n> > +\tstatic std::array searchPaths = {\n> > +\t\tLIBCAMERA_SYSCONF_DIR,\n> > +\t\tLIBCAMERA_DATA_DIR,\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> > +\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> > +/*\n> > + * \\brief Open the HAL configuration file and validate its content\n> > + * \\return 0 on success, a negative error code otherwise\n>\n> nit: We don't Doxygen the HAL is it not confusing to use it mark up here\n> then?\n>\n> > + */\n> > +int CameraHalConfig::open()\n> > +{\n> > +\tint ret;\n> > +\n> > +\tconst std::string configPath = \"camera_hal.yaml\";\n>\n> s/configPath/configFile/ ?\n>\n> > +\tconst std::string filePath = findFilePath(configPath);\n>\n> As this is the only call site would it make sens to have findFilePath()\n> return a FILE * to get compartmentalizing of the error paths?\n>\n> > +\tif (filePath.empty()) {\n> > +\t\tLOG(HALConfig, Warning)\n> > +\t\t\t<< \"File: \\\"\" << configPath << \"\\\" not found\";\n> > +\t\treturn -ENOENT;\n> > +\t}\n> > +\n> > +\tFILE *fh = fopen(filePath.c_str(), \"r\");\n> > +\tif (!fh) {\n> > +\t\tret = -errno;\n> > +\t\tLOG(HALConfig, Error) << \"Failed to open file \" << filePath\n> > +\t\t\t\t << \": \" << strerror(-ret);\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tyaml_parser_set_input_file(&parser_, fh);\n> > +\n> > +\tLOG(HALConfig, Debug) << \"Reading configuration from \" << filePath;\n> > +\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 CameraBlock camera = c.second;\n> > +\t\tLOG(HALConfig, Info) << camera.toString();\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +const std::string &CameraHalConfig::cameraLocation(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 CameraBlock &cam = it->second;\n> > +\treturn cam.location;\n> > +}\n> > +\n> > +unsigned int CameraHalConfig::cameraRotation(const std::string &camera) const\n> > +{\n> > +\tstatic const std::string empty(\"\");\n>\n> empty not used.\n>\n\nOuch, leftover\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 CameraBlock &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 CameraBlock &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> > +\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_KEY_TOKEN) {\n> > +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > +\t\treturn \"\";\n> > +\t}\n> > +\tyaml_token_delete(&token);\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 key(scalar, token.data.scalar.length);\n> > +\tyaml_token_delete(&token);\n> > +\n> > +\treturn key;\n> > +}\n> > +\n> > +int CameraHalConfig::parseCameraBlock(yaml_token_t &token)\n> > +{\n> > +\t/* The 'camera' block is a VALUE token type. */\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> > +\t/*\n> > +\t * Parse the content of the camera block until we have an unbalanced\n> > +\t * BLOCK_END_TOKEN which closes the camera block.\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> > +\tCameraBlock cameraBlock{};\n> > +\tint blockCounter = 0;\n> > +\tdo {\n> > +\t\tyaml_parser_scan(&parser_, &token);\n> > +\t\tswitch (token.type) {\n> > +\t\tcase YAML_BLOCK_ENTRY_TOKEN:\n> > +\t\t\tyaml_token_delete(&token);\n> > +\t\t\tblockCounter++;\n> > +\t\t\tbreak;\n> > +\t\tcase YAML_BLOCK_END_TOKEN:\n> > +\t\t\tyaml_token_delete(&token);\n> > +\t\t\tblockCounter--;\n> > +\t\t\tbreak;\n> > +\t\tcase YAML_BLOCK_MAPPING_START_TOKEN: {\n> > +\t\t\tyaml_token_delete(&token);\n> > +\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\t/* Validate that the parsed key is valid. */\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 == \"name\") {\n> > +\t\t\t\tcameraBlock.name = value;\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\tdefault:\n> > +\t\t\tyaml_token_delete(&token);\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t\t--sentinel;\n> > +\t} while (blockCounter >= 0 && sentinel);\n> > +\tif (!sentinel) {\n> > +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid \";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tcameras_[cameraBlock.name] = cameraBlock;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int CameraHalConfig::parseEntryBlock(yaml_token_t &token)\n> > +{\n> > +\tint ret;\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> > +\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 == \"camera\") {\n> > +\t\tyaml_token_delete(&token);\n> > +\t\tret = parseCameraBlock(token);\n> > +\t\tif (ret)\n> > +\t\t\treturn ret;\n> > +\t} else if (key == \"manufacturer\") {\n> > +\t\tyaml_token_delete(&token);\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\tyaml_token_delete(&token);\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\tyaml_token_delete(&token);\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> > +\t/*\n> > +\t * Start parsing the content of the configuration file which\n> > +\t * starts as with sequence block.\n> > +\t */\n> > +\tyaml_parser_scan(&parser_, &token);\n> > +\tif (token.type != YAML_BLOCK_SEQUENCE_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 single entry blocks. */\n> > +\tdo {\n> > +\t\tyaml_parser_scan(&parser_, &token);\n> > +\t\tswitch (token.type) {\n> > +\t\tcase YAML_BLOCK_ENTRY_TOKEN:\n> > +\t\t\tyaml_token_delete(&token);\n> > +\t\t\tret = parseEntryBlock(token);\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\t/* Release resources before re-parsing. */\n> > +\t\t\tyaml_token_delete(&token);\n> > +\t\t\tbreak;\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..69d42658e90a\n> > --- /dev/null\n> > +++ b/src/android/camera_hal_config.h\n> > @@ -0,0 +1,54 @@\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> > +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> > +\tconst std::string &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 CameraBlock\n> > +\t{\n> > +\tpublic:\n> > +\t\tstd::string name;\n> > +\t\tstd::string location;\n> > +\t\tstd::string model;\n> > +\t\tunsigned int rotation;\n> > +\n> > +\t\tconst std::string toString() const;\n> > +\t};\n> > +\tstd::map cameras_;\n> > +\n> > +\tconst std::string findFilePath(const std::string &filename);\n> > +\tstd::string parseValue(yaml_token_t &token);\n> > +\tstd::string parseKey(yaml_token_t &token);\n> > +\tint parseCameraBlock(yaml_token_t &token);\n> > +\tint parseEntryBlock(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 19f94a4073f1..34e5c494a492 100644\n> > --- a/src/android/meson.build\n> > +++ b/src/android/meson.build\n> > @@ -44,6 +44,7 @@ android_hal_sources = files([\n> > 'camera3_hal.cpp',\n> > 'camera_hal_manager.cpp',\n> > 'camera_device.cpp',\n> > + 'camera_hal_config.cpp',\n> > 'camera_metadata.cpp',\n> > 'camera_ops.cpp',\n> > 'camera_stream.cpp',\n> > --\n> > 2.30.0\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards,\n> Niklas Söderlund","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 44D24C32EA\n\tfor ;\n\tMon, 29 Mar 2021 14:22:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8A83768786;\n\tMon, 29 Mar 2021 16:22:35 +0200 (CEST)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5C8366877D\n\tfor ;\n\tMon, 29 Mar 2021 16:22:34 +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 relay2-d.mail.gandi.net (Postfix) with ESMTPSA id BCC8D40002;\n\tMon, 29 Mar 2021 14:22:33 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Mon, 29 Mar 2021 16:23:08 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20210329142308.sekalomdb3tqbpg7@uno.localdomain>","References":"<20210324112527.63701-1-jacopo@jmondi.org>\n\t<20210324112527.63701-5-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 4/7] android: Add CameraHalConfig class","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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16019,"web_url":"https://patchwork.libcamera.org/comment/16019/","msgid":"<20210329142722.nnpci3qlot5wtvky@uno.localdomain>","date":"2021-03-29T14:27:22","subject":"Re: [libcamera-devel] [PATCH 4/7] android: Add CameraHalConfig class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Fri, Mar 26, 2021 at 06:07:07AM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Wed, Mar 24, 2021 at 12:25:24PM +0100, Jacopo Mondi wrote:\n> > Add a CameraHalConfig class to the Android Camera3 HAL layer.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/android/camera_hal_config.cpp | 385 ++++++++++++++++++++++++++++++\n> > src/android/camera_hal_config.h | 54 +++++\n> > src/android/meson.build | 1 +\n> > 3 files changed, 440 insertions(+)\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/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp\n> > new file mode 100644\n> > index 000000000000..7e33dfb750ff\n> > --- /dev/null\n> > +++ b/src/android/camera_hal_config.cpp\n> > @@ -0,0 +1,385 @@\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 \"libcamera/internal/file.h\"\n> > +#include \"libcamera/internal/log.h\"\n> > +\n> > +using namespace libcamera;\n> > +\n> > +LOG_DEFINE_CATEGORY(HALConfig)\n> > +\n> > +const std::string CameraHalConfig::CameraBlock::toString() const\n> > +{\n> > +\tstd::stringstream ss;\n> > +\n> > +\tss << \"\\'\" << name << \"\\'\"\n> > +\t << \" model: \" << model\n> > +\t << \"(\" << location << \")[\" << rotation << \"]\";\n> > +\n> > +\treturn ss.str();\n> > +}\n>\n> As the amount of information will grow, I don't think we'll be able to\n> print everything. This function is only used in a single location below,\n> for debugging purpose, I wonder if we should keep it.\n>\n> > +\n> > +CameraHalConfig::CameraHalConfig()\n> > +{\n> > +\tif (!yaml_parser_initialize(&parser_))\n> > +\t\tLOG(HALConfig, Error) << \"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> Can we run the camera HAL uninstalled ? :-)\n>\n\nThis meant \"libcamera\" uninstalled.\n\n> > + */\n> > +const std::string CameraHalConfig::findFilePath(const std::string &filename)\n> > +{\n> > +\tstatic std::array searchPaths = {\n> > +\t\tLIBCAMERA_SYSCONF_DIR,\n> > +\t\tLIBCAMERA_DATA_DIR,\n>\n> The data dir may not be very useful here, the configuration file is\n> really system-specific, right ?\n>\n> I expect we may need to get the configuration file from system-specific\n> locations, depending on whether we run on Chrome OS or Android, but we\n> can handle that later.\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> > +\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> > +/*\n> > + * \\brief 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> > +\tconst std::string configPath = \"camera_hal.yaml\";\n> > +\tconst std::string filePath = findFilePath(configPath);\n> > +\tif (filePath.empty()) {\n> > +\t\tLOG(HALConfig, Warning)\n>\n> This can be an Error, as we can't continue.\n>\n> > +\t\t\t<< \"File: \\\"\" << configPath << \"\\\" not found\";\n>\n> Maybe s/File:/Configuration file/ ?\n>\n> > +\t\treturn -ENOENT;\n> > +\t}\n> > +\n> > +\tFILE *fh = fopen(filePath.c_str(), \"r\");\n> > +\tif (!fh) {\n> > +\t\tret = -errno;\n> > +\t\tLOG(HALConfig, Error) << \"Failed to open file \" << filePath\n>\n> Same here, \"configuration file\" ?\n>\n> > +\t\t\t\t << \": \" << strerror(-ret);\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tyaml_parser_set_input_file(&parser_, fh);\n> > +\n> > +\tLOG(HALConfig, Debug) << \"Reading configuration from \" << filePath;\n> > +\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 CameraBlock camera = c.second;\n> > +\t\tLOG(HALConfig, Info) << camera.toString();\n> > +\t}\n>\n> I wonder if dumping the parsed file shouldn't be a debugging feature\n> instead, or even if we need it at all.\n>\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +const std::string &CameraHalConfig::cameraLocation(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 CameraBlock &cam = it->second;\n> > +\treturn cam.location;\n> > +}\n> > +\n> > +unsigned int CameraHalConfig::cameraRotation(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 0;\n> > +\t}\n> > +\n> > +\tconst CameraBlock &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 CameraBlock &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> > +\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_KEY_TOKEN) {\n> > +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > +\t\treturn \"\";\n> > +\t}\n> > +\tyaml_token_delete(&token);\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 key(scalar, token.data.scalar.length);\n> > +\tyaml_token_delete(&token);\n> > +\n> > +\treturn key;\n> > +}\n> > +\n> > +int CameraHalConfig::parseCameraBlock(yaml_token_t &token)\n> > +{\n> > +\t/* The 'camera' block is a VALUE token type. */\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> > +\t/*\n> > +\t * Parse the content of the camera block until we have an unbalanced\n> > +\t * BLOCK_END_TOKEN which closes the camera block.\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> > +\tCameraBlock cameraBlock{};\n> > +\tint blockCounter = 0;\n> > +\tdo {\n> > +\t\tyaml_parser_scan(&parser_, &token);\n> > +\t\tswitch (token.type) {\n> > +\t\tcase YAML_BLOCK_ENTRY_TOKEN:\n> > +\t\t\tyaml_token_delete(&token);\n> > +\t\t\tblockCounter++;\n> > +\t\t\tbreak;\n> > +\t\tcase YAML_BLOCK_END_TOKEN:\n> > +\t\t\tyaml_token_delete(&token);\n> > +\t\t\tblockCounter--;\n> > +\t\t\tbreak;\n> > +\t\tcase YAML_BLOCK_MAPPING_START_TOKEN: {\n> > +\t\t\tyaml_token_delete(&token);\n> > +\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\t/* Validate that the parsed key is valid. */\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 == \"name\") {\n> > +\t\t\t\tcameraBlock.name = value;\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\tdefault:\n> > +\t\t\tyaml_token_delete(&token);\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t\t--sentinel;\n> > +\t} while (blockCounter >= 0 && sentinel);\n> > +\tif (!sentinel) {\n> > +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid \";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tcameras_[cameraBlock.name] = cameraBlock;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int CameraHalConfig::parseEntryBlock(yaml_token_t &token)\n> > +{\n> > +\tint ret;\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> > +\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 == \"camera\") {\n> > +\t\tyaml_token_delete(&token);\n> > +\t\tret = parseCameraBlock(token);\n> > +\t\tif (ret)\n> > +\t\t\treturn ret;\n> > +\t} else if (key == \"manufacturer\") {\n> > +\t\tyaml_token_delete(&token);\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\tyaml_token_delete(&token);\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\tyaml_token_delete(&token);\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>\n> Just to make sure you're aware of it, there's yaml_parser_load() that\n> produces a parsed document that can then be walked. There's nothing\n> wrong about using event-based parsing as you do though.\n\nI've found that feature under-documented with very few usage examples\naround.\n\n>\n> I've skipped review of the parser implementation itself, as it may be\n> changed depending on how patch 7/7 evolves.\n>\n> > +\tif (token.type != YAML_STREAM_START_TOKEN) {\n> > +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > +\t\treturn EINVAL;\n>\n> -EINVAL\n>\n> > +\t}\n> > +\tyaml_token_delete(&token);\n> > +\n> > +\t/*\n> > +\t * Start parsing the content of the configuration file which\n> > +\t * starts as with sequence block.\n>\n> s/as // ?\n>\n> > +\t */\n> > +\tyaml_parser_scan(&parser_, &token);\n> > +\tif (token.type != YAML_BLOCK_SEQUENCE_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 single entry blocks. */\n> > +\tdo {\n> > +\t\tyaml_parser_scan(&parser_, &token);\n> > +\t\tswitch (token.type) {\n> > +\t\tcase YAML_BLOCK_ENTRY_TOKEN:\n> > +\t\t\tyaml_token_delete(&token);\n> > +\t\t\tret = parseEntryBlock(token);\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\t/* Release resources before re-parsing. */\n> > +\t\t\tyaml_token_delete(&token);\n> > +\t\t\tbreak;\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..69d42658e90a\n> > --- /dev/null\n> > +++ b/src/android/camera_hal_config.h\n> > @@ -0,0 +1,54 @@\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> It would be nice to hide the YAML perser from this header. It could be\n> done by moving the parsing to a CameraHalConfigParser class, private to\n> the camera_hal_config.cpp file.\n>\n\n.. ok\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> > +\tconst std::string &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 CameraBlock\n> > +\t{\n> > +\tpublic:\n> > +\t\tstd::string name;\n> > +\t\tstd::string location;\n> > +\t\tstd::string model;\n> > +\t\tunsigned int rotation;\n> > +\n> > +\t\tconst std::string toString() const;\n> > +\t};\n>\n> How about making this class public (and probably renaming it), and replacing the last three public\n> functions with\n>\n> \tconst CameraBlock *cameraConfig(const std::string &name) const;\n>\n> ? That way each CameraDevice could store a reference to the\n> corresponding camera configuration only.\n>\n\nI prefer the CameraHalConfig::cameraProperty() interface but ok\n\n> > +\tstd::map cameras_;\n> > +\n> > +\tconst std::string findFilePath(const std::string &filename);\n>\n> No need for const at the beginning as you return by value, but you can\n> add a const at the end.\n>\n> > +\tstd::string parseValue(yaml_token_t &token);\n> > +\tstd::string parseKey(yaml_token_t &token);\n> > +\tint parseCameraBlock(yaml_token_t &token);\n> > +\tint parseEntryBlock(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 19f94a4073f1..34e5c494a492 100644\n> > --- a/src/android/meson.build\n> > +++ b/src/android/meson.build\n> > @@ -44,6 +44,7 @@ android_hal_sources = files([\n> > 'camera3_hal.cpp',\n> > 'camera_hal_manager.cpp',\n> > 'camera_device.cpp',\n> > + 'camera_hal_config.cpp',\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 D6DD8C32F0\n\tfor ;\n\tMon, 29 Mar 2021 14:26:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6893C68783;\n\tMon, 29 Mar 2021 16:26:49 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 891B368783\n\tfor ;\n\tMon, 29 Mar 2021 16:26:47 +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 relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 0B49660007;\n\tMon, 29 Mar 2021 14:26:46 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Mon, 29 Mar 2021 16:27:22 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210329142722.nnpci3qlot5wtvky@uno.localdomain>","References":"<20210324112527.63701-1-jacopo@jmondi.org>\n\t<20210324112527.63701-5-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 4/7] android: Add CameraHalConfig class","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":16020,"web_url":"https://patchwork.libcamera.org/comment/16020/","msgid":"","date":"2021-03-29T14:27:01","subject":"Re: [libcamera-devel] [PATCH 4/7] android: Add CameraHalConfig class","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nOn 2021-03-29 16:23:08 +0200, Jacopo Mondi wrote:\n> Hi Niklas,\n> \n> On Thu, Mar 25, 2021 at 11:33:31PM +0100, Niklas Söderlund wrote:\n> > Hi Jacopo,\n> >\n> > Thanks for your work.\n> >\n> > On 2021-03-24 12:25:24 +0100, Jacopo Mondi wrote:\n> > > Add a CameraHalConfig class to the Android Camera3 HAL layer.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> >\n> > There are a few small nits below but I think this is a great step in the\n> > right direction.\n> >\n> > Reviewed-by: Niklas Söderlund \n> >\n> > > ---\n> > > src/android/camera_hal_config.cpp | 385 ++++++++++++++++++++++++++++++\n> > > src/android/camera_hal_config.h | 54 +++++\n> > > src/android/meson.build | 1 +\n> > > 3 files changed, 440 insertions(+)\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/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp\n> > > new file mode 100644\n> > > index 000000000000..7e33dfb750ff\n> > > --- /dev/null\n> > > +++ b/src/android/camera_hal_config.cpp\n> > > @@ -0,0 +1,385 @@\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 \"libcamera/internal/file.h\"\n> > > +#include \"libcamera/internal/log.h\"\n> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +LOG_DEFINE_CATEGORY(HALConfig)\n> > > +\n> > > +const std::string CameraHalConfig::CameraBlock::toString() const\n> > > +{\n> > > +\tstd::stringstream ss;\n> > > +\n> > > +\tss << \"\\'\" << name << \"\\'\"\n> > > +\t << \" model: \" << model\n> > > +\t << \"(\" << location << \")[\" << rotation << \"]\";\n> >\n> > nit: The usage of the pattern \"key: \" have been discouraged in\n> > the core, do we extend the same to the HAL?\n> >\n> \n> Not sure what you mean here -.-\n\nSorry, I just noticed that the pattern of\n\n << \" model: \" << model \n\nHave been discouraged in the core in favor of\n\n << \" model \" << model \n\n> \n> > > +\n> > > +\treturn ss.str();\n> > > +}\n> > > +\n> > > +CameraHalConfig::CameraHalConfig()\n> > > +{\n> > > +\tif (!yaml_parser_initialize(&parser_))\n> > > +\t\tLOG(HALConfig, Error) << \"Failed to initialize yaml parser\";\n> >\n> > Fatal?\n> >\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> > > +const std::string CameraHalConfig::findFilePath(const std::string &filename)\n> > > +{\n> > > +\tstatic std::array searchPaths = {\n> > > +\t\tLIBCAMERA_SYSCONF_DIR,\n> > > +\t\tLIBCAMERA_DATA_DIR,\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> > > +\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> > > +/*\n> > > + * \\brief Open the HAL configuration file and validate its content\n> > > + * \\return 0 on success, a negative error code otherwise\n> >\n> > nit: We don't Doxygen the HAL is it not confusing to use it mark up here\n> > then?\n> >\n> > > + */\n> > > +int CameraHalConfig::open()\n> > > +{\n> > > +\tint ret;\n> > > +\n> > > +\tconst std::string configPath = \"camera_hal.yaml\";\n> >\n> > s/configPath/configFile/ ?\n> >\n> > > +\tconst std::string filePath = findFilePath(configPath);\n> >\n> > As this is the only call site would it make sens to have findFilePath()\n> > return a FILE * to get compartmentalizing of the error paths?\n> >\n> > > +\tif (filePath.empty()) {\n> > > +\t\tLOG(HALConfig, Warning)\n> > > +\t\t\t<< \"File: \\\"\" << configPath << \"\\\" not found\";\n> > > +\t\treturn -ENOENT;\n> > > +\t}\n> > > +\n> > > +\tFILE *fh = fopen(filePath.c_str(), \"r\");\n> > > +\tif (!fh) {\n> > > +\t\tret = -errno;\n> > > +\t\tLOG(HALConfig, Error) << \"Failed to open file \" << filePath\n> > > +\t\t\t\t << \": \" << strerror(-ret);\n> > > +\t\treturn ret;\n> > > +\t}\n> > > +\n> > > +\tyaml_parser_set_input_file(&parser_, fh);\n> > > +\n> > > +\tLOG(HALConfig, Debug) << \"Reading configuration from \" << filePath;\n> > > +\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 CameraBlock camera = c.second;\n> > > +\t\tLOG(HALConfig, Info) << camera.toString();\n> > > +\t}\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +const std::string &CameraHalConfig::cameraLocation(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 CameraBlock &cam = it->second;\n> > > +\treturn cam.location;\n> > > +}\n> > > +\n> > > +unsigned int CameraHalConfig::cameraRotation(const std::string &camera) const\n> > > +{\n> > > +\tstatic const std::string empty(\"\");\n> >\n> > empty not used.\n> >\n> \n> Ouch, leftover\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 CameraBlock &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 CameraBlock &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> > > +\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_KEY_TOKEN) {\n> > > +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > +\t\treturn \"\";\n> > > +\t}\n> > > +\tyaml_token_delete(&token);\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 key(scalar, token.data.scalar.length);\n> > > +\tyaml_token_delete(&token);\n> > > +\n> > > +\treturn key;\n> > > +}\n> > > +\n> > > +int CameraHalConfig::parseCameraBlock(yaml_token_t &token)\n> > > +{\n> > > +\t/* The 'camera' block is a VALUE token type. */\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> > > +\t/*\n> > > +\t * Parse the content of the camera block until we have an unbalanced\n> > > +\t * BLOCK_END_TOKEN which closes the camera block.\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> > > +\tCameraBlock cameraBlock{};\n> > > +\tint blockCounter = 0;\n> > > +\tdo {\n> > > +\t\tyaml_parser_scan(&parser_, &token);\n> > > +\t\tswitch (token.type) {\n> > > +\t\tcase YAML_BLOCK_ENTRY_TOKEN:\n> > > +\t\t\tyaml_token_delete(&token);\n> > > +\t\t\tblockCounter++;\n> > > +\t\t\tbreak;\n> > > +\t\tcase YAML_BLOCK_END_TOKEN:\n> > > +\t\t\tyaml_token_delete(&token);\n> > > +\t\t\tblockCounter--;\n> > > +\t\t\tbreak;\n> > > +\t\tcase YAML_BLOCK_MAPPING_START_TOKEN: {\n> > > +\t\t\tyaml_token_delete(&token);\n> > > +\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\t/* Validate that the parsed key is valid. */\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 == \"name\") {\n> > > +\t\t\t\tcameraBlock.name = value;\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\tdefault:\n> > > +\t\t\tyaml_token_delete(&token);\n> > > +\t\t\tbreak;\n> > > +\t\t}\n> > > +\t\t--sentinel;\n> > > +\t} while (blockCounter >= 0 && sentinel);\n> > > +\tif (!sentinel) {\n> > > +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid \";\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > +\n> > > +\tcameras_[cameraBlock.name] = cameraBlock;\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +int CameraHalConfig::parseEntryBlock(yaml_token_t &token)\n> > > +{\n> > > +\tint ret;\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> > > +\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 == \"camera\") {\n> > > +\t\tyaml_token_delete(&token);\n> > > +\t\tret = parseCameraBlock(token);\n> > > +\t\tif (ret)\n> > > +\t\t\treturn ret;\n> > > +\t} else if (key == \"manufacturer\") {\n> > > +\t\tyaml_token_delete(&token);\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\tyaml_token_delete(&token);\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\tyaml_token_delete(&token);\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> > > +\t/*\n> > > +\t * Start parsing the content of the configuration file which\n> > > +\t * starts as with sequence block.\n> > > +\t */\n> > > +\tyaml_parser_scan(&parser_, &token);\n> > > +\tif (token.type != YAML_BLOCK_SEQUENCE_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 single entry blocks. */\n> > > +\tdo {\n> > > +\t\tyaml_parser_scan(&parser_, &token);\n> > > +\t\tswitch (token.type) {\n> > > +\t\tcase YAML_BLOCK_ENTRY_TOKEN:\n> > > +\t\t\tyaml_token_delete(&token);\n> > > +\t\t\tret = parseEntryBlock(token);\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\t/* Release resources before re-parsing. */\n> > > +\t\t\tyaml_token_delete(&token);\n> > > +\t\t\tbreak;\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..69d42658e90a\n> > > --- /dev/null\n> > > +++ b/src/android/camera_hal_config.h\n> > > @@ -0,0 +1,54 @@\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> > > +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> > > +\tconst std::string &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 CameraBlock\n> > > +\t{\n> > > +\tpublic:\n> > > +\t\tstd::string name;\n> > > +\t\tstd::string location;\n> > > +\t\tstd::string model;\n> > > +\t\tunsigned int rotation;\n> > > +\n> > > +\t\tconst std::string toString() const;\n> > > +\t};\n> > > +\tstd::map cameras_;\n> > > +\n> > > +\tconst std::string findFilePath(const std::string &filename);\n> > > +\tstd::string parseValue(yaml_token_t &token);\n> > > +\tstd::string parseKey(yaml_token_t &token);\n> > > +\tint parseCameraBlock(yaml_token_t &token);\n> > > +\tint parseEntryBlock(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 19f94a4073f1..34e5c494a492 100644\n> > > --- a/src/android/meson.build\n> > > +++ b/src/android/meson.build\n> > > @@ -44,6 +44,7 @@ android_hal_sources = files([\n> > > 'camera3_hal.cpp',\n> > > 'camera_hal_manager.cpp',\n> > > 'camera_device.cpp',\n> > > + 'camera_hal_config.cpp',\n> > > 'camera_metadata.cpp',\n> > > 'camera_ops.cpp',\n> > > 'camera_stream.cpp',\n> > > --\n> > > 2.30.0\n> > >\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\n> >\n> > --\n> > Regards,\n> > Niklas Söderlund","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 E66F5C32F0\n\tfor ;\n\tMon, 29 Mar 2021 14:27:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A752468788;\n\tMon, 29 Mar 2021 16:27:07 +0200 (CEST)","from mail-lj1-x229.google.com (mail-lj1-x229.google.com\n\t[IPv6:2a00:1450:4864:20::229])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 834D768783\n\tfor ;\n\tMon, 29 Mar 2021 16:27:06 +0200 (CEST)","by mail-lj1-x229.google.com with SMTP id u4so16180730ljo.6\n\tfor ;\n\tMon, 29 Mar 2021 07:27:06 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\ty186sm1847690lfc.304.2021.03.29.07.27.01\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 29 Mar 2021 07:27:01 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"i8/Q+Tk3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=0DyMhduhj2fUU/W3nTCYfKM3C3UeAs59o7wfZJV3F3E=;\n\tb=i8/Q+Tk34D/1vvLgyVoLSXUr4Y4yhKuBd3eAEVJB/tzN9g5RcjTPEeT2eGrWd6yo//\n\tkYv7Vf2LO+ug9zVZWT0CcOfbcHUxsRW7/D9yd9cGtg1Y301sEbIxrKg2rCysMyJzITTF\n\tMdQH+cNKr5FUT81RFvGHqv66l0B272TBMDBEH+FT8U92Kz6i8uT3T7qB1qS5gvYoz/Gt\n\tlB29Eiuxcqlwe2mzkmK07zOpstmKPJ5R44tDKJWrs5l1gW0lTcGjpxwFKvK5XU4OG4Fh\n\t+zpHyDt/UqnOkNIY7iebAfIgVWay9YwakUSHaGXn3d+pcIFJSxr9qJoA5ALfhtO+Stt+\n\t5nVg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=0DyMhduhj2fUU/W3nTCYfKM3C3UeAs59o7wfZJV3F3E=;\n\tb=jPfzqUVzfTXo2ZCTLchQp8HfBtgzVb4J9V3iaRtBYiBzCYlJB+XlHGLMvcMBCWEhTP\n\tru3JeoDVmU3M8s6ak9tq+OHYRaB2iIPNU8MEV7I3BohdUX6ypXFSlbVSCPlJ/kBdamIw\n\tiKY3ijS6NKiTC0WgAn2c0OCDn5lOfPPZruN9pLB0J7XZadi53k+zngsjT1ss8m8PqT44\n\tkbalSiiPtN89BLuOqPYSwWfCUsAGXTRg5ZavaEzvoZhx85T6SREQeUf8rO+kzGqeu0cg\n\t7hhq1XEVUSeufrHCOEYmTQ+tUTYGdTyUE4cniPv/XjqLmY1xrvrm4+JWkHUGJLCciPoB\n\tnheA==","X-Gm-Message-State":"AOAM532eLmXT37WfDffQp79o29MtFccUE8NeQyMVGG+a4/snHLD01Z5U\n\t1FHC1qgOlgs9bqpIBZh68aoOZQ==","X-Google-Smtp-Source":"ABdhPJxT9kkvSnYfzOLu3UtDWnEuQx07s5ajc+vmvpmkPCoaQkpZCe0vvmw3MAzTPb051IfCABqg1w==","X-Received":"by 2002:a2e:7505:: with SMTP id\n\tq5mr18102191ljc.322.1617028025609; \n\tMon, 29 Mar 2021 07:27:05 -0700 (PDT)","Date":"Mon, 29 Mar 2021 16:27:01 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210324112527.63701-1-jacopo@jmondi.org>\n\t<20210324112527.63701-5-jacopo@jmondi.org>\n\t\n\t<20210329142308.sekalomdb3tqbpg7@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210329142308.sekalomdb3tqbpg7@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 4/7] android: Add CameraHalConfig class","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=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16021,"web_url":"https://patchwork.libcamera.org/comment/16021/","msgid":"<20210329144310.rcfavqf5hpupplyc@uno.localdomain>","date":"2021-03-29T14:43:10","subject":"Re: [libcamera-devel] [PATCH 4/7] android: Add CameraHalConfig class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"On Mon, Mar 29, 2021 at 04:27:01PM +0200, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> On 2021-03-29 16:23:08 +0200, Jacopo Mondi wrote:\n> > Hi Niklas,\n> >\n> > On Thu, Mar 25, 2021 at 11:33:31PM +0100, Niklas Söderlund wrote:\n> > > Hi Jacopo,\n> > >\n> > > Thanks for your work.\n> > >\n> > > On 2021-03-24 12:25:24 +0100, Jacopo Mondi wrote:\n> > > > Add a CameraHalConfig class to the Android Camera3 HAL layer.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > >\n> > > There are a few small nits below but I think this is a great step in the\n> > > right direction.\n> > >\n> > > Reviewed-by: Niklas Söderlund \n> > >\n> > > > ---\n> > > > src/android/camera_hal_config.cpp | 385 ++++++++++++++++++++++++++++++\n> > > > src/android/camera_hal_config.h | 54 +++++\n> > > > src/android/meson.build | 1 +\n> > > > 3 files changed, 440 insertions(+)\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/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp\n> > > > new file mode 100644\n> > > > index 000000000000..7e33dfb750ff\n> > > > --- /dev/null\n> > > > +++ b/src/android/camera_hal_config.cpp\n> > > > @@ -0,0 +1,385 @@\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 \"libcamera/internal/file.h\"\n> > > > +#include \"libcamera/internal/log.h\"\n> > > > +\n> > > > +using namespace libcamera;\n> > > > +\n> > > > +LOG_DEFINE_CATEGORY(HALConfig)\n> > > > +\n> > > > +const std::string CameraHalConfig::CameraBlock::toString() const\n> > > > +{\n> > > > +\tstd::stringstream ss;\n> > > > +\n> > > > +\tss << \"\\'\" << name << \"\\'\"\n> > > > +\t << \" model: \" << model\n> > > > +\t << \"(\" << location << \")[\" << rotation << \"]\";\n> > >\n> > > nit: The usage of the pattern \"key: \" have been discouraged in\n> > > the core, do we extend the same to the HAL?\n> > >\n> >\n> > Not sure what you mean here -.-\n>\n> Sorry, I just noticed that the pattern of\n>\n> << \" model: \" << model\n>\n> Have been discouraged in the core in favor of\n>\n> << \" model \" << model\n>\n\nAh ok, we're talking about the \":\" in an Info debug message.\nSorry I missed such change in the core.\n\nI'll fix\n\n> >\n> > > > +\n> > > > +\treturn ss.str();\n> > > > +}\n> > > > +\n> > > > +CameraHalConfig::CameraHalConfig()\n> > > > +{\n> > > > +\tif (!yaml_parser_initialize(&parser_))\n> > > > +\t\tLOG(HALConfig, Error) << \"Failed to initialize yaml parser\";\n> > >\n> > > Fatal?\n> > >\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> > > > +const std::string CameraHalConfig::findFilePath(const std::string &filename)\n> > > > +{\n> > > > +\tstatic std::array searchPaths = {\n> > > > +\t\tLIBCAMERA_SYSCONF_DIR,\n> > > > +\t\tLIBCAMERA_DATA_DIR,\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> > > > +\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> > > > +/*\n> > > > + * \\brief Open the HAL configuration file and validate its content\n> > > > + * \\return 0 on success, a negative error code otherwise\n> > >\n> > > nit: We don't Doxygen the HAL is it not confusing to use it mark up here\n> > > then?\n> > >\n> > > > + */\n> > > > +int CameraHalConfig::open()\n> > > > +{\n> > > > +\tint ret;\n> > > > +\n> > > > +\tconst std::string configPath = \"camera_hal.yaml\";\n> > >\n> > > s/configPath/configFile/ ?\n> > >\n> > > > +\tconst std::string filePath = findFilePath(configPath);\n> > >\n> > > As this is the only call site would it make sens to have findFilePath()\n> > > return a FILE * to get compartmentalizing of the error paths?\n> > >\n> > > > +\tif (filePath.empty()) {\n> > > > +\t\tLOG(HALConfig, Warning)\n> > > > +\t\t\t<< \"File: \\\"\" << configPath << \"\\\" not found\";\n> > > > +\t\treturn -ENOENT;\n> > > > +\t}\n> > > > +\n> > > > +\tFILE *fh = fopen(filePath.c_str(), \"r\");\n> > > > +\tif (!fh) {\n> > > > +\t\tret = -errno;\n> > > > +\t\tLOG(HALConfig, Error) << \"Failed to open file \" << filePath\n> > > > +\t\t\t\t << \": \" << strerror(-ret);\n> > > > +\t\treturn ret;\n> > > > +\t}\n> > > > +\n> > > > +\tyaml_parser_set_input_file(&parser_, fh);\n> > > > +\n> > > > +\tLOG(HALConfig, Debug) << \"Reading configuration from \" << filePath;\n> > > > +\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 CameraBlock camera = c.second;\n> > > > +\t\tLOG(HALConfig, Info) << camera.toString();\n> > > > +\t}\n> > > > +\n> > > > +\treturn 0;\n> > > > +}\n> > > > +\n> > > > +const std::string &CameraHalConfig::cameraLocation(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 CameraBlock &cam = it->second;\n> > > > +\treturn cam.location;\n> > > > +}\n> > > > +\n> > > > +unsigned int CameraHalConfig::cameraRotation(const std::string &camera) const\n> > > > +{\n> > > > +\tstatic const std::string empty(\"\");\n> > >\n> > > empty not used.\n> > >\n> >\n> > Ouch, leftover\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 CameraBlock &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 CameraBlock &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> > > > +\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_KEY_TOKEN) {\n> > > > +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > > +\t\treturn \"\";\n> > > > +\t}\n> > > > +\tyaml_token_delete(&token);\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 key(scalar, token.data.scalar.length);\n> > > > +\tyaml_token_delete(&token);\n> > > > +\n> > > > +\treturn key;\n> > > > +}\n> > > > +\n> > > > +int CameraHalConfig::parseCameraBlock(yaml_token_t &token)\n> > > > +{\n> > > > +\t/* The 'camera' block is a VALUE token type. */\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> > > > +\t/*\n> > > > +\t * Parse the content of the camera block until we have an unbalanced\n> > > > +\t * BLOCK_END_TOKEN which closes the camera block.\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> > > > +\tCameraBlock cameraBlock{};\n> > > > +\tint blockCounter = 0;\n> > > > +\tdo {\n> > > > +\t\tyaml_parser_scan(&parser_, &token);\n> > > > +\t\tswitch (token.type) {\n> > > > +\t\tcase YAML_BLOCK_ENTRY_TOKEN:\n> > > > +\t\t\tyaml_token_delete(&token);\n> > > > +\t\t\tblockCounter++;\n> > > > +\t\t\tbreak;\n> > > > +\t\tcase YAML_BLOCK_END_TOKEN:\n> > > > +\t\t\tyaml_token_delete(&token);\n> > > > +\t\t\tblockCounter--;\n> > > > +\t\t\tbreak;\n> > > > +\t\tcase YAML_BLOCK_MAPPING_START_TOKEN: {\n> > > > +\t\t\tyaml_token_delete(&token);\n> > > > +\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\t/* Validate that the parsed key is valid. */\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 == \"name\") {\n> > > > +\t\t\t\tcameraBlock.name = value;\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\tdefault:\n> > > > +\t\t\tyaml_token_delete(&token);\n> > > > +\t\t\tbreak;\n> > > > +\t\t}\n> > > > +\t\t--sentinel;\n> > > > +\t} while (blockCounter >= 0 && sentinel);\n> > > > +\tif (!sentinel) {\n> > > > +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid \";\n> > > > +\t\treturn -EINVAL;\n> > > > +\t}\n> > > > +\n> > > > +\tcameras_[cameraBlock.name] = cameraBlock;\n> > > > +\n> > > > +\treturn 0;\n> > > > +}\n> > > > +\n> > > > +int CameraHalConfig::parseEntryBlock(yaml_token_t &token)\n> > > > +{\n> > > > +\tint ret;\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> > > > +\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 == \"camera\") {\n> > > > +\t\tyaml_token_delete(&token);\n> > > > +\t\tret = parseCameraBlock(token);\n> > > > +\t\tif (ret)\n> > > > +\t\t\treturn ret;\n> > > > +\t} else if (key == \"manufacturer\") {\n> > > > +\t\tyaml_token_delete(&token);\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\tyaml_token_delete(&token);\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\tyaml_token_delete(&token);\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> > > > +\t/*\n> > > > +\t * Start parsing the content of the configuration file which\n> > > > +\t * starts as with sequence block.\n> > > > +\t */\n> > > > +\tyaml_parser_scan(&parser_, &token);\n> > > > +\tif (token.type != YAML_BLOCK_SEQUENCE_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 single entry blocks. */\n> > > > +\tdo {\n> > > > +\t\tyaml_parser_scan(&parser_, &token);\n> > > > +\t\tswitch (token.type) {\n> > > > +\t\tcase YAML_BLOCK_ENTRY_TOKEN:\n> > > > +\t\t\tyaml_token_delete(&token);\n> > > > +\t\t\tret = parseEntryBlock(token);\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\t/* Release resources before re-parsing. */\n> > > > +\t\t\tyaml_token_delete(&token);\n> > > > +\t\t\tbreak;\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..69d42658e90a\n> > > > --- /dev/null\n> > > > +++ b/src/android/camera_hal_config.h\n> > > > @@ -0,0 +1,54 @@\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> > > > +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> > > > +\tconst std::string &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 CameraBlock\n> > > > +\t{\n> > > > +\tpublic:\n> > > > +\t\tstd::string name;\n> > > > +\t\tstd::string location;\n> > > > +\t\tstd::string model;\n> > > > +\t\tunsigned int rotation;\n> > > > +\n> > > > +\t\tconst std::string toString() const;\n> > > > +\t};\n> > > > +\tstd::map cameras_;\n> > > > +\n> > > > +\tconst std::string findFilePath(const std::string &filename);\n> > > > +\tstd::string parseValue(yaml_token_t &token);\n> > > > +\tstd::string parseKey(yaml_token_t &token);\n> > > > +\tint parseCameraBlock(yaml_token_t &token);\n> > > > +\tint parseEntryBlock(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 19f94a4073f1..34e5c494a492 100644\n> > > > --- a/src/android/meson.build\n> > > > +++ b/src/android/meson.build\n> > > > @@ -44,6 +44,7 @@ android_hal_sources = files([\n> > > > 'camera3_hal.cpp',\n> > > > 'camera_hal_manager.cpp',\n> > > > 'camera_device.cpp',\n> > > > + 'camera_hal_config.cpp',\n> > > > 'camera_metadata.cpp',\n> > > > 'camera_ops.cpp',\n> > > > 'camera_stream.cpp',\n> > > > --\n> > > > 2.30.0\n> > > >\n> > > > _______________________________________________\n> > > > libcamera-devel mailing list\n> > > > libcamera-devel@lists.libcamera.org\n> > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > >\n> > > --\n> > > Regards,\n> > > Niklas Söderlund\n>\n> --\n> Regards,\n> Niklas Söderlund","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 09BE0C32EA\n\tfor ;\n\tMon, 29 Mar 2021 14:42:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 63E9768784;\n\tMon, 29 Mar 2021 16:42:37 +0200 (CEST)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2E7426877D\n\tfor ;\n\tMon, 29 Mar 2021 16:42:36 +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 relay5-d.mail.gandi.net (Postfix) with ESMTPSA id A28FF1C0012;\n\tMon, 29 Mar 2021 14:42:35 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Mon, 29 Mar 2021 16:43:10 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20210329144310.rcfavqf5hpupplyc@uno.localdomain>","References":"<20210324112527.63701-1-jacopo@jmondi.org>\n\t<20210324112527.63701-5-jacopo@jmondi.org>\n\t\n\t<20210329142308.sekalomdb3tqbpg7@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 4/7] android: Add CameraHalConfig class","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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16026,"web_url":"https://patchwork.libcamera.org/comment/16026/","msgid":"","date":"2021-03-29T17:43:15","subject":"Re: [libcamera-devel] [PATCH 4/7] android: Add CameraHalConfig class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Mar 29, 2021 at 04:27:22PM +0200, Jacopo Mondi wrote:\n> On Fri, Mar 26, 2021 at 06:07:07AM +0200, Laurent Pinchart wrote:\n> > On Wed, Mar 24, 2021 at 12:25:24PM +0100, Jacopo Mondi wrote:\n> > > Add a CameraHalConfig class to the Android Camera3 HAL layer.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/android/camera_hal_config.cpp | 385 ++++++++++++++++++++++++++++++\n> > > src/android/camera_hal_config.h | 54 +++++\n> > > src/android/meson.build | 1 +\n> > > 3 files changed, 440 insertions(+)\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/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp\n> > > new file mode 100644\n> > > index 000000000000..7e33dfb750ff\n> > > --- /dev/null\n> > > +++ b/src/android/camera_hal_config.cpp\n> > > @@ -0,0 +1,385 @@\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 \"libcamera/internal/file.h\"\n> > > +#include \"libcamera/internal/log.h\"\n> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +LOG_DEFINE_CATEGORY(HALConfig)\n> > > +\n> > > +const std::string CameraHalConfig::CameraBlock::toString() const\n> > > +{\n> > > +\tstd::stringstream ss;\n> > > +\n> > > +\tss << \"\\'\" << name << \"\\'\"\n> > > +\t << \" model: \" << model\n> > > +\t << \"(\" << location << \")[\" << rotation << \"]\";\n> > > +\n> > > +\treturn ss.str();\n> > > +}\n> >\n> > As the amount of information will grow, I don't think we'll be able to\n> > print everything. This function is only used in a single location below,\n> > for debugging purpose, I wonder if we should keep it.\n> >\n> > > +\n> > > +CameraHalConfig::CameraHalConfig()\n> > > +{\n> > > +\tif (!yaml_parser_initialize(&parser_))\n> > > +\t\tLOG(HALConfig, Error) << \"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> > Can we run the camera HAL uninstalled ? :-)\n> >\n> \n> This meant \"libcamera\" uninstalled.\n\nYes, but as this is the HAL configuration, I don't think we need to care\nabout the uninstalled case. But now that I think about it, maybe you\nmeant for this function to be ready for the libcamera core configuration\nfile too ?\n\n> > > + */\n> > > +const std::string CameraHalConfig::findFilePath(const std::string &filename)\n> > > +{\n> > > +\tstatic std::array searchPaths = {\n> > > +\t\tLIBCAMERA_SYSCONF_DIR,\n> > > +\t\tLIBCAMERA_DATA_DIR,\n> >\n> > The data dir may not be very useful here, the configuration file is\n> > really system-specific, right ?\n> >\n> > I expect we may need to get the configuration file from system-specific\n> > locations, depending on whether we run on Chrome OS or Android, but we\n> > can handle that later.\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> > > +\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> > > +/*\n> > > + * \\brief 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> > > +\tconst std::string configPath = \"camera_hal.yaml\";\n> > > +\tconst std::string filePath = findFilePath(configPath);\n> > > +\tif (filePath.empty()) {\n> > > +\t\tLOG(HALConfig, Warning)\n> >\n> > This can be an Error, as we can't continue.\n> >\n> > > +\t\t\t<< \"File: \\\"\" << configPath << \"\\\" not found\";\n> >\n> > Maybe s/File:/Configuration file/ ?\n> >\n> > > +\t\treturn -ENOENT;\n> > > +\t}\n> > > +\n> > > +\tFILE *fh = fopen(filePath.c_str(), \"r\");\n> > > +\tif (!fh) {\n> > > +\t\tret = -errno;\n> > > +\t\tLOG(HALConfig, Error) << \"Failed to open file \" << filePath\n> >\n> > Same here, \"configuration file\" ?\n> >\n> > > +\t\t\t\t << \": \" << strerror(-ret);\n> > > +\t\treturn ret;\n> > > +\t}\n> > > +\n> > > +\tyaml_parser_set_input_file(&parser_, fh);\n> > > +\n> > > +\tLOG(HALConfig, Debug) << \"Reading configuration from \" << filePath;\n> > > +\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 CameraBlock camera = c.second;\n> > > +\t\tLOG(HALConfig, Info) << camera.toString();\n> > > +\t}\n> >\n> > I wonder if dumping the parsed file shouldn't be a debugging feature\n> > instead, or even if we need it at all.\n> >\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +const std::string &CameraHalConfig::cameraLocation(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 CameraBlock &cam = it->second;\n> > > +\treturn cam.location;\n> > > +}\n> > > +\n> > > +unsigned int CameraHalConfig::cameraRotation(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 0;\n> > > +\t}\n> > > +\n> > > +\tconst CameraBlock &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 CameraBlock &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> > > +\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_KEY_TOKEN) {\n> > > +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > +\t\treturn \"\";\n> > > +\t}\n> > > +\tyaml_token_delete(&token);\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 key(scalar, token.data.scalar.length);\n> > > +\tyaml_token_delete(&token);\n> > > +\n> > > +\treturn key;\n> > > +}\n> > > +\n> > > +int CameraHalConfig::parseCameraBlock(yaml_token_t &token)\n> > > +{\n> > > +\t/* The 'camera' block is a VALUE token type. */\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> > > +\t/*\n> > > +\t * Parse the content of the camera block until we have an unbalanced\n> > > +\t * BLOCK_END_TOKEN which closes the camera block.\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> > > +\tCameraBlock cameraBlock{};\n> > > +\tint blockCounter = 0;\n> > > +\tdo {\n> > > +\t\tyaml_parser_scan(&parser_, &token);\n> > > +\t\tswitch (token.type) {\n> > > +\t\tcase YAML_BLOCK_ENTRY_TOKEN:\n> > > +\t\t\tyaml_token_delete(&token);\n> > > +\t\t\tblockCounter++;\n> > > +\t\t\tbreak;\n> > > +\t\tcase YAML_BLOCK_END_TOKEN:\n> > > +\t\t\tyaml_token_delete(&token);\n> > > +\t\t\tblockCounter--;\n> > > +\t\t\tbreak;\n> > > +\t\tcase YAML_BLOCK_MAPPING_START_TOKEN: {\n> > > +\t\t\tyaml_token_delete(&token);\n> > > +\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\t/* Validate that the parsed key is valid. */\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 == \"name\") {\n> > > +\t\t\t\tcameraBlock.name = value;\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\tdefault:\n> > > +\t\t\tyaml_token_delete(&token);\n> > > +\t\t\tbreak;\n> > > +\t\t}\n> > > +\t\t--sentinel;\n> > > +\t} while (blockCounter >= 0 && sentinel);\n> > > +\tif (!sentinel) {\n> > > +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid \";\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > +\n> > > +\tcameras_[cameraBlock.name] = cameraBlock;\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +int CameraHalConfig::parseEntryBlock(yaml_token_t &token)\n> > > +{\n> > > +\tint ret;\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> > > +\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 == \"camera\") {\n> > > +\t\tyaml_token_delete(&token);\n> > > +\t\tret = parseCameraBlock(token);\n> > > +\t\tif (ret)\n> > > +\t\t\treturn ret;\n> > > +\t} else if (key == \"manufacturer\") {\n> > > +\t\tyaml_token_delete(&token);\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\tyaml_token_delete(&token);\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\tyaml_token_delete(&token);\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> >\n> > Just to make sure you're aware of it, there's yaml_parser_load() that\n> > produces a parsed document that can then be walked. There's nothing\n> > wrong about using event-based parsing as you do though.\n> \n> I've found that feature under-documented with very few usage examples\n> around.\n\nAgreed, it's underdocumented in libyaml :-( I've found\nhttp://codepad.org/W7StVSkV that shows how to walk a document. If you\nthink that would be simpler to use, feel free to do so, otherwise the\ntoken-based parser can be kept.\n\n> > I've skipped review of the parser implementation itself, as it may be\n> > changed depending on how patch 7/7 evolves.\n> >\n> > > +\tif (token.type != YAML_STREAM_START_TOKEN) {\n> > > +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > +\t\treturn EINVAL;\n> >\n> > -EINVAL\n> >\n> > > +\t}\n> > > +\tyaml_token_delete(&token);\n> > > +\n> > > +\t/*\n> > > +\t * Start parsing the content of the configuration file which\n> > > +\t * starts as with sequence block.\n> >\n> > s/as // ?\n> >\n> > > +\t */\n> > > +\tyaml_parser_scan(&parser_, &token);\n> > > +\tif (token.type != YAML_BLOCK_SEQUENCE_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 single entry blocks. */\n> > > +\tdo {\n> > > +\t\tyaml_parser_scan(&parser_, &token);\n> > > +\t\tswitch (token.type) {\n> > > +\t\tcase YAML_BLOCK_ENTRY_TOKEN:\n> > > +\t\t\tyaml_token_delete(&token);\n> > > +\t\t\tret = parseEntryBlock(token);\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\t/* Release resources before re-parsing. */\n> > > +\t\t\tyaml_token_delete(&token);\n> > > +\t\t\tbreak;\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..69d42658e90a\n> > > --- /dev/null\n> > > +++ b/src/android/camera_hal_config.h\n> > > @@ -0,0 +1,54 @@\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> > It would be nice to hide the YAML perser from this header. It could be\n> > done by moving the parsing to a CameraHalConfigParser class, private to\n> > the camera_hal_config.cpp file.\n> \n> .. ok\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> > > +\tconst std::string &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 CameraBlock\n> > > +\t{\n> > > +\tpublic:\n> > > +\t\tstd::string name;\n> > > +\t\tstd::string location;\n> > > +\t\tstd::string model;\n> > > +\t\tunsigned int rotation;\n> > > +\n> > > +\t\tconst std::string toString() const;\n> > > +\t};\n> >\n> > How about making this class public (and probably renaming it), and replacing the last three public\n> > functions with\n> >\n> > \tconst CameraBlock *cameraConfig(const std::string &name) const;\n> >\n> > ? That way each CameraDevice could store a reference to the\n> > corresponding camera configuration only.\n> \n> I prefer the CameraHalConfig::cameraProperty() interface but ok\n\nI wouldn't be surprised if we had to create a deeper hierarchy of\nproperties in YAML, in which case cameraProperty() wouldn't scale well\n(there's also the lookup every time a property is accessed that we could\nsave).\n\n> > > +\tstd::map cameras_;\n> > > +\n> > > +\tconst std::string findFilePath(const std::string &filename);\n> >\n> > No need for const at the beginning as you return by value, but you can\n> > add a const at the end.\n> >\n> > > +\tstd::string parseValue(yaml_token_t &token);\n> > > +\tstd::string parseKey(yaml_token_t &token);\n> > > +\tint parseCameraBlock(yaml_token_t &token);\n> > > +\tint parseEntryBlock(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 19f94a4073f1..34e5c494a492 100644\n> > > --- a/src/android/meson.build\n> > > +++ b/src/android/meson.build\n> > > @@ -44,6 +44,7 @@ android_hal_sources = files([\n> > > 'camera3_hal.cpp',\n> > > 'camera_hal_manager.cpp',\n> > > 'camera_device.cpp',\n> > > + 'camera_hal_config.cpp',\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 E2748C32EA\n\tfor ;\n\tMon, 29 Mar 2021 17:44:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 45AC568786;\n\tMon, 29 Mar 2021 19:44:02 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B92946877D\n\tfor ;\n\tMon, 29 Mar 2021 19:44:00 +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 299E0503;\n\tMon, 29 Mar 2021 19:44:00 +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=\"Q4ZtILZS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1617039840;\n\tbh=jrU6zAaVUwmbP/7tfrdK0Fg+Vlrkl3DCcgzZSqizNoc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Q4ZtILZS2QgGVOdwqgmj2Asa1+pXBOXcEuXIVp75udfQ8mkxFNLKwOZEYAHZR8vQ8\n\tDExfIyBtVQntlazX3sD88HgC6f4v8ZKPNAGUIiqcemzv5KtU+w+Nk75mroD4qaZ/Cy\n\tcc00fCvM5DA+iJh9dUoi7KvcSTUlLLdMLPzW1kgo=","Date":"Mon, 29 Mar 2021 20:43:15 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210324112527.63701-1-jacopo@jmondi.org>\n\t<20210324112527.63701-5-jacopo@jmondi.org>\n\t\n\t<20210329142722.nnpci3qlot5wtvky@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210329142722.nnpci3qlot5wtvky@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 4/7] android: Add CameraHalConfig class","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":16027,"web_url":"https://patchwork.libcamera.org/comment/16027/","msgid":"","date":"2021-03-29T18:10:31","subject":"Re: [libcamera-devel] [PATCH 4/7] android: Add CameraHalConfig class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Mon, Mar 29, 2021 at 04:27:01PM +0200, Niklas Söderlund wrote:\n> On 2021-03-29 16:23:08 +0200, Jacopo Mondi wrote:\n> > On Thu, Mar 25, 2021 at 11:33:31PM +0100, Niklas Söderlund wrote:\n> > > On 2021-03-24 12:25:24 +0100, Jacopo Mondi wrote:\n> > > > Add a CameraHalConfig class to the Android Camera3 HAL layer.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > >\n> > > There are a few small nits below but I think this is a great step in the\n> > > right direction.\n> > >\n> > > Reviewed-by: Niklas Söderlund \n> > >\n> > > > ---\n> > > > src/android/camera_hal_config.cpp | 385 ++++++++++++++++++++++++++++++\n> > > > src/android/camera_hal_config.h | 54 +++++\n> > > > src/android/meson.build | 1 +\n> > > > 3 files changed, 440 insertions(+)\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/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp\n> > > > new file mode 100644\n> > > > index 000000000000..7e33dfb750ff\n> > > > --- /dev/null\n> > > > +++ b/src/android/camera_hal_config.cpp\n> > > > @@ -0,0 +1,385 @@\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 \"libcamera/internal/file.h\"\n> > > > +#include \"libcamera/internal/log.h\"\n> > > > +\n> > > > +using namespace libcamera;\n> > > > +\n> > > > +LOG_DEFINE_CATEGORY(HALConfig)\n> > > > +\n> > > > +const std::string CameraHalConfig::CameraBlock::toString() const\n> > > > +{\n> > > > +\tstd::stringstream ss;\n> > > > +\n> > > > +\tss << \"\\'\" << name << \"\\'\"\n> > > > +\t << \" model: \" << model\n> > > > +\t << \"(\" << location << \")[\" << rotation << \"]\";\n> > >\n> > > nit: The usage of the pattern \"key: \" have been discouraged in\n> > > the core, do we extend the same to the HAL?\n> > >\n> > \n> > Not sure what you mean here -.-\n> \n> Sorry, I just noticed that the pattern of\n> \n> << \" model: \" << model \n> \n> Have been discouraged in the core in favor of\n> \n> << \" model \" << model \n\nAs I suppose this comes from the fact I've asked to remove colons\nmultiple times during review, I'd like to clarify that there's nothing\nwrong with the \"key: value\" syntax per se. When I asked for the colon to\nbe dropped, that was because the message was written as a sentence. For\ninstance, \"The camera is rotated by 90 degrees\" is more natural language\nthan \"The camera is rotated by: 90 degrees\". I'd have no problem with a\nmessage such as \"configuration: width: 1280, height: 960, format: NV12\".\n\n> > > > +\n> > > > +\treturn ss.str();\n> > > > +}\n> > > > +\n> > > > +CameraHalConfig::CameraHalConfig()\n> > > > +{\n> > > > +\tif (!yaml_parser_initialize(&parser_))\n> > > > +\t\tLOG(HALConfig, Error) << \"Failed to initialize yaml parser\";\n> > >\n> > > Fatal?\n> > >\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> > > > +const std::string CameraHalConfig::findFilePath(const std::string &filename)\n> > > > +{\n> > > > +\tstatic std::array searchPaths = {\n> > > > +\t\tLIBCAMERA_SYSCONF_DIR,\n> > > > +\t\tLIBCAMERA_DATA_DIR,\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> > > > +\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> > > > +/*\n> > > > + * \\brief Open the HAL configuration file and validate its content\n> > > > + * \\return 0 on success, a negative error code otherwise\n> > >\n> > > nit: We don't Doxygen the HAL is it not confusing to use it mark up here\n> > > then?\n> > >\n> > > > + */\n> > > > +int CameraHalConfig::open()\n> > > > +{\n> > > > +\tint ret;\n> > > > +\n> > > > +\tconst std::string configPath = \"camera_hal.yaml\";\n> > >\n> > > s/configPath/configFile/ ?\n> > >\n> > > > +\tconst std::string filePath = findFilePath(configPath);\n> > >\n> > > As this is the only call site would it make sens to have findFilePath()\n> > > return a FILE * to get compartmentalizing of the error paths?\n> > >\n> > > > +\tif (filePath.empty()) {\n> > > > +\t\tLOG(HALConfig, Warning)\n> > > > +\t\t\t<< \"File: \\\"\" << configPath << \"\\\" not found\";\n> > > > +\t\treturn -ENOENT;\n> > > > +\t}\n> > > > +\n> > > > +\tFILE *fh = fopen(filePath.c_str(), \"r\");\n> > > > +\tif (!fh) {\n> > > > +\t\tret = -errno;\n> > > > +\t\tLOG(HALConfig, Error) << \"Failed to open file \" << filePath\n> > > > +\t\t\t\t << \": \" << strerror(-ret);\n> > > > +\t\treturn ret;\n> > > > +\t}\n> > > > +\n> > > > +\tyaml_parser_set_input_file(&parser_, fh);\n> > > > +\n> > > > +\tLOG(HALConfig, Debug) << \"Reading configuration from \" << filePath;\n> > > > +\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 CameraBlock camera = c.second;\n> > > > +\t\tLOG(HALConfig, Info) << camera.toString();\n> > > > +\t}\n> > > > +\n> > > > +\treturn 0;\n> > > > +}\n> > > > +\n> > > > +const std::string &CameraHalConfig::cameraLocation(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 CameraBlock &cam = it->second;\n> > > > +\treturn cam.location;\n> > > > +}\n> > > > +\n> > > > +unsigned int CameraHalConfig::cameraRotation(const std::string &camera) const\n> > > > +{\n> > > > +\tstatic const std::string empty(\"\");\n> > >\n> > > empty not used.\n> > >\n> > \n> > Ouch, leftover\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 CameraBlock &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 CameraBlock &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> > > > +\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_KEY_TOKEN) {\n> > > > +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > > +\t\treturn \"\";\n> > > > +\t}\n> > > > +\tyaml_token_delete(&token);\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 key(scalar, token.data.scalar.length);\n> > > > +\tyaml_token_delete(&token);\n> > > > +\n> > > > +\treturn key;\n> > > > +}\n> > > > +\n> > > > +int CameraHalConfig::parseCameraBlock(yaml_token_t &token)\n> > > > +{\n> > > > +\t/* The 'camera' block is a VALUE token type. */\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> > > > +\t/*\n> > > > +\t * Parse the content of the camera block until we have an unbalanced\n> > > > +\t * BLOCK_END_TOKEN which closes the camera block.\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> > > > +\tCameraBlock cameraBlock{};\n> > > > +\tint blockCounter = 0;\n> > > > +\tdo {\n> > > > +\t\tyaml_parser_scan(&parser_, &token);\n> > > > +\t\tswitch (token.type) {\n> > > > +\t\tcase YAML_BLOCK_ENTRY_TOKEN:\n> > > > +\t\t\tyaml_token_delete(&token);\n> > > > +\t\t\tblockCounter++;\n> > > > +\t\t\tbreak;\n> > > > +\t\tcase YAML_BLOCK_END_TOKEN:\n> > > > +\t\t\tyaml_token_delete(&token);\n> > > > +\t\t\tblockCounter--;\n> > > > +\t\t\tbreak;\n> > > > +\t\tcase YAML_BLOCK_MAPPING_START_TOKEN: {\n> > > > +\t\t\tyaml_token_delete(&token);\n> > > > +\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\t/* Validate that the parsed key is valid. */\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 == \"name\") {\n> > > > +\t\t\t\tcameraBlock.name = value;\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\tdefault:\n> > > > +\t\t\tyaml_token_delete(&token);\n> > > > +\t\t\tbreak;\n> > > > +\t\t}\n> > > > +\t\t--sentinel;\n> > > > +\t} while (blockCounter >= 0 && sentinel);\n> > > > +\tif (!sentinel) {\n> > > > +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid \";\n> > > > +\t\treturn -EINVAL;\n> > > > +\t}\n> > > > +\n> > > > +\tcameras_[cameraBlock.name] = cameraBlock;\n> > > > +\n> > > > +\treturn 0;\n> > > > +}\n> > > > +\n> > > > +int CameraHalConfig::parseEntryBlock(yaml_token_t &token)\n> > > > +{\n> > > > +\tint ret;\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> > > > +\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 == \"camera\") {\n> > > > +\t\tyaml_token_delete(&token);\n> > > > +\t\tret = parseCameraBlock(token);\n> > > > +\t\tif (ret)\n> > > > +\t\t\treturn ret;\n> > > > +\t} else if (key == \"manufacturer\") {\n> > > > +\t\tyaml_token_delete(&token);\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\tyaml_token_delete(&token);\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\tyaml_token_delete(&token);\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> > > > +\t/*\n> > > > +\t * Start parsing the content of the configuration file which\n> > > > +\t * starts as with sequence block.\n> > > > +\t */\n> > > > +\tyaml_parser_scan(&parser_, &token);\n> > > > +\tif (token.type != YAML_BLOCK_SEQUENCE_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 single entry blocks. */\n> > > > +\tdo {\n> > > > +\t\tyaml_parser_scan(&parser_, &token);\n> > > > +\t\tswitch (token.type) {\n> > > > +\t\tcase YAML_BLOCK_ENTRY_TOKEN:\n> > > > +\t\t\tyaml_token_delete(&token);\n> > > > +\t\t\tret = parseEntryBlock(token);\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\t/* Release resources before re-parsing. */\n> > > > +\t\t\tyaml_token_delete(&token);\n> > > > +\t\t\tbreak;\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..69d42658e90a\n> > > > --- /dev/null\n> > > > +++ b/src/android/camera_hal_config.h\n> > > > @@ -0,0 +1,54 @@\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> > > > +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> > > > +\tconst std::string &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 CameraBlock\n> > > > +\t{\n> > > > +\tpublic:\n> > > > +\t\tstd::string name;\n> > > > +\t\tstd::string location;\n> > > > +\t\tstd::string model;\n> > > > +\t\tunsigned int rotation;\n> > > > +\n> > > > +\t\tconst std::string toString() const;\n> > > > +\t};\n> > > > +\tstd::map cameras_;\n> > > > +\n> > > > +\tconst std::string findFilePath(const std::string &filename);\n> > > > +\tstd::string parseValue(yaml_token_t &token);\n> > > > +\tstd::string parseKey(yaml_token_t &token);\n> > > > +\tint parseCameraBlock(yaml_token_t &token);\n> > > > +\tint parseEntryBlock(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 19f94a4073f1..34e5c494a492 100644\n> > > > --- a/src/android/meson.build\n> > > > +++ b/src/android/meson.build\n> > > > @@ -44,6 +44,7 @@ android_hal_sources = files([\n> > > > 'camera3_hal.cpp',\n> > > > 'camera_hal_manager.cpp',\n> > > > 'camera_device.cpp',\n> > > > + 'camera_hal_config.cpp',\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 3FB5FC32EA\n\tfor ;\n\tMon, 29 Mar 2021 18:11:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9E9D468782;\n\tMon, 29 Mar 2021 20:11:18 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C8B856877D\n\tfor ;\n\tMon, 29 Mar 2021 20:11:16 +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 2E053503;\n\tMon, 29 Mar 2021 20:11:16 +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=\"nTM4DiU7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1617041476;\n\tbh=aU9H2mv1gAh8O0LwOk5UZq0BQgMxi700+wJtwOmA23w=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nTM4DiU7WpLwL7nHeqvTIaV2InE2rO1R3PmfLEIwkdHGsv9OfrpE8DYCopcX1KUBB\n\tXXjPtN1oBp3eO7wETkIPV6mbq0JogGT8p2z/nv9U6B9XZe1NCtaYvpA9U/YVIoiou7\n\t6praPn+1x1d/Z8XrbBshDsE/rNQRslyw+N5SGWxU=","Date":"Mon, 29 Mar 2021 21:10:31 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"","References":"<20210324112527.63701-1-jacopo@jmondi.org>\n\t<20210324112527.63701-5-jacopo@jmondi.org>\n\t\n\t<20210329142308.sekalomdb3tqbpg7@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 4/7] android: Add CameraHalConfig class","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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]