[{"id":16131,"web_url":"https://patchwork.libcamera.org/comment/16131/","msgid":"","date":"2021-04-07T03:45:12","subject":"Re: [libcamera-devel] [PATCH v4 3/5] android: Add CameraHalConfig\n\tclass","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo, Thanks for this patch.\n\nOn Wed, Apr 7, 2021 at 12:45 AM Jacopo Mondi wrote:\n>\n> Add a CameraHalConfig class to the Android Camera3 HAL layer.\n>\n> Signed-off-by: Jacopo Mondi \n> ---\n> README.rst | 2 +-\n> src/android/camera_hal_config.cpp | 433 ++++++++++++++++++++++++++++++\n> src/android/camera_hal_config.h | 38 +++\n> src/android/meson.build | 2 +\n> 4 files changed, 474 insertions(+), 1 deletion(-)\n> create mode 100644 src/android/camera_hal_config.cpp\n> create mode 100644 src/android/camera_hal_config.h\n>\n> diff --git a/README.rst b/README.rst\n> index c77e54b48b7a..fcf0f47f14c5 100644\n> --- a/README.rst\n> +++ b/README.rst\n> @@ -88,7 +88,7 @@ for tracing with lttng: [optional]\n> liblttng-ust-dev python3-jinja2 lttng-tools\n>\n> for android: [optional]\n> - libexif libjpeg\n> + libexif libjpeg libyaml\n>\n> Using GStreamer plugin\n> ~~~~~~~~~~~~~~~~~~~~~~\n> diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp\n> new file mode 100644\n> index 000000000000..5b51fa4dd61b\n> --- /dev/null\n> +++ b/src/android/camera_hal_config.cpp\n> @@ -0,0 +1,433 @@\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> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \"libcamera/internal/file.h\"\n> +#include \"libcamera/internal/log.h\"\n> +\n> +using namespace libcamera;\n> +\n> +LOG_DEFINE_CATEGORY(HALConfig)\n> +\n> +class CameraHalConfig::Private : public Extensible::Private\n> +{\n> + LIBCAMERA_DECLARE_PUBLIC(CameraHalConfig)\n> +\n> +public:\n> + Private(CameraHalConfig *halConfig);\n> +\n> + int parseConfigFile(FILE *fh, std::map *cameras);\n> +\n> +private:\n> + std::string parseValue();\n> + std::string parseKey();\n> + int parseValueBlock();\n> + int parseCameraLocation(CameraProps *cameraProps, const std::string &location);\n> + int parseCameraProps(const std::string &cameraId);\n> + int parseCameras();\n> + int parseEntry();\n> +\n> + yaml_parser_t parser_;\n> + std::map *cameras_;\n> +};\n> +\n> +CameraHalConfig::Private::Private(CameraHalConfig *halConfig)\n> + : Extensible::Private(halConfig)\n> +{\n> +}\n> +\n> +std::string CameraHalConfig::Private::parseValue()\n> +{\n> + yaml_token_t token;\n> +\n> + /* Make sure the token type is a value and get its content. */\n> + yaml_parser_scan(&parser_, &token);\n> + if (token.type != YAML_VALUE_TOKEN) {\n> + yaml_token_delete(&token);\n> + return \"\";\n> + }\n> + yaml_token_delete(&token);\n> +\n> + yaml_parser_scan(&parser_, &token);\n> + if (token.type != YAML_SCALAR_TOKEN) {\n> + yaml_token_delete(&token);\n> + return \"\";\n> + }\n> +\n> + std::string value(reinterpret_cast(token.data.scalar.value),\n> + token.data.scalar.length);\n> + yaml_token_delete(&token);\n> +\n> + return value;\n> +}\n> +\n> +std::string CameraHalConfig::Private::parseKey()\n> +{\n> + yaml_token_t token;\n> +\n> + /* Make sure the token type is a key and get its value. */\n> + yaml_parser_scan(&parser_, &token);\n> + if (token.type != YAML_SCALAR_TOKEN) {\n> + yaml_token_delete(&token);\n> + return \"\";\n> + }\n> +\n> + std::string value(reinterpret_cast(token.data.scalar.value),\n> + token.data.scalar.length);\n> + yaml_token_delete(&token);\n> +\n> + return value;\n> +}\n> +\n> +int CameraHalConfig::Private::parseValueBlock()\n> +{\n> + yaml_token_t token;\n> +\n> + /* Make sure the next token are VALUE and BLOCK_MAPPING_START. */\n> + yaml_parser_scan(&parser_, &token);\n> + if (token.type != YAML_VALUE_TOKEN) {\n> + yaml_token_delete(&token);\n> + return -EINVAL;\n> + }\n> + yaml_token_delete(&token);\n> +\n> + yaml_parser_scan(&parser_, &token);\n> + if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) {\n> + yaml_token_delete(&token);\n> + return -EINVAL;\n> + }\n> + yaml_token_delete(&token);\n> +\n> + return 0;\n> +}\n> +\n> +int CameraHalConfig::Private::parseCameraLocation(CameraProps *cameraProps, const std::string &location)\n> +{\n> + if (location == \"front\")\n> + cameraProps->facing = CAMERA_FACING_FRONT;\n> + else if (location == \"back\")\n> + cameraProps->facing = CAMERA_FACING_BACK;\n> + else if (location == \"external\")\n> + cameraProps->facing = CAMERA_FACING_EXTERNAL;\n> + else\n> + return -EINVAL;\n> +\n> + return 0;\n> +}\n> +\n> +int CameraHalConfig::Private::parseCameraProps(const std::string &cameraId)\n> +{\n> + int ret = parseValueBlock();\n> + if (ret)\n> + return ret;\n> +\n> + /*\n> + * Parse the camera properties and store them in a cameraProps instance.\n> + *\n> + * Add a safety counter to make sure we don't loop indefinitely in case\n> + * the configuration file is malformed.\n> + */\n> + unsigned int sentinel = 100;\n> + CameraProps cameraProps;\n> + bool blockEnd = false;\n> + yaml_token_t token;\n> +\n> + do {\n> + yaml_parser_scan(&parser_, &token);\n> + switch (token.type) {\n> + case YAML_KEY_TOKEN: {\n> + yaml_token_delete(&token);\n> +\n> + /*\n> + * Parse the camera property key and make sure it is\n> + * valid.\n> + */\n> + std::string key = parseKey();\n> + std::string value = parseValue();\n> + if (key.empty() || value.empty())\n> + return -EINVAL;\n> +\n> + if (key == \"location\") {\n> + ret = parseCameraLocation(&cameraProps, value);\n> + if (ret) {\n> + LOG(HALConfig, Error)\n> + << \"Unknown location: \"\n> + << value;\n> + return -EINVAL;\n> + }\n> + } else if (key == \"rotation\") {\n> + ret = std::stoi(value);\n> + if (ret < 0 || ret >= 360) {\n> + LOG(HALConfig, Error)\n> + << \"Unknown rotation: \"\n> + << cameraProps.rotation;\n> + return -EINVAL;\n> + }\n> + cameraProps.rotation = ret;\n> + } else {\n> + LOG(HALConfig, Error)\n> + << \"Unknown key: \" << key;\n> + return -EINVAL;\n> + }\n> + break;\n> + }\n> +\n> + case YAML_BLOCK_END_TOKEN:\n> + blockEnd = true;\n> + [[fallthrough]];\n> + default:\n> + yaml_token_delete(&token);\n> + break;\n> + }\n> +\n> + --sentinel;\n> + } while (!blockEnd && sentinel);\n> + if (!sentinel)\n> + return -EINVAL;\n> +\n> + cameraProps.valid = true;\n> + (*cameras_)[cameraId] = cameraProps;\n> +\n> + return 0;\n> +}\n> +\n> +int CameraHalConfig::Private::parseCameras()\n> +{\n> + int ret = parseValueBlock();\n> + if (ret) {\n> + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> + return ret;\n> + }\n> +\n> + /*\n> + * Parse the camera properties.\n> + *\n> + * Each camera properties block is a list of properties associated\n> + * with the ID (as assembled by CameraSensor::generateId()) of the\n> + * camera they refer to.\n> + *\n> + * cameras:\n> + * \"camera0 id\":\n> + * key: value\n> + * key: value\n> + * ...\n> + *\n> + * \"camera1 id\":\n> + * key: value\n> + * key: value\n> + * ...\n> + */\n> + bool blockEnd = false;\n> + yaml_token_t token;\n> + do {\n> + yaml_parser_scan(&parser_, &token);\n> + switch (token.type) {\n> + case YAML_KEY_TOKEN: {\n> + yaml_token_delete(&token);\n> +\n> + /* Parse the camera ID as key of the property list. */\n> + std::string cameraId = parseKey();\n> + if (cameraId.empty())\n> + return -EINVAL;\n> +\n> + ret = parseCameraProps(cameraId);\n> + if (ret)\n> + return -EINVAL;\n> + break;\n> + }\n> + case YAML_BLOCK_END_TOKEN:\n> + blockEnd = true;\n> + [[fallthrough]];\n> + default:\n> + yaml_token_delete(&token);\n> + break;\n> + }\n> + } while (!blockEnd);\n> +\n> + return 0;\n> +}\n> +\n> +int CameraHalConfig::Private::parseEntry()\n> +{\n> + int ret = -EINVAL;\n> +\n> + /*\n> + * Parse each key we find in the file.\n> + *\n> + * The 'cameras' keys maps to a list of (lists) of camera properties.\n> + */\n> +\n> + std::string key = parseKey();\n> + if (key.empty())\n> + return ret;\n> +\n> + if (key == \"cameras\")\n> + ret = parseCameras();\n> + else\n> + LOG(HALConfig, Error) << \"Unknown key: \" << key;\n> +\n> + return ret;\n> +}\n> +\n> +int CameraHalConfig::Private::parseConfigFile(FILE *fh,\n> + std::map *cameras)\n> +{\n> + cameras_ = cameras;\n> +\n> + int ret = yaml_parser_initialize(&parser_);\n> + if (!ret) {\n> + LOG(HALConfig, Fatal) << \"Failed to initialize yaml parser\";\n> + return -EINVAL;\n> + }\n> + yaml_parser_set_input_file(&parser_, fh);\n> +\n> + yaml_token_t token;\n> + yaml_parser_scan(&parser_, &token);\n> + if (token.type != YAML_STREAM_START_TOKEN) {\n> + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> + yaml_token_delete(&token);\n> + yaml_parser_delete(&parser_);\n> + return -EINVAL;\n> + }\n> + yaml_token_delete(&token);\n> +\n> + yaml_parser_scan(&parser_, &token);\n> + if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) {\n> + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> + yaml_token_delete(&token);\n> + yaml_parser_delete(&parser_);\n> + return -EINVAL;\n> + }\n> + yaml_token_delete(&token);\n> +\n> + /* Parse the file and parse each single key one by one. */\n> + do {\n> + yaml_parser_scan(&parser_, &token);\n> + switch (token.type) {\n> + case YAML_KEY_TOKEN:\n> + yaml_token_delete(&token);\n> + ret = parseEntry();\n> + break;\n> +\n> + case YAML_STREAM_END_TOKEN:\n> + ret = -ENOENT;\n> + [[fallthrough]];\n> + default:\n> + yaml_token_delete(&token);\n> + break;\n> + }\n> + } while (ret >= 0);\n> + yaml_parser_delete(&parser_);\n> +\n> + if (ret && ret != -ENOENT)\n> + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> +\n> + return ret == -ENOENT ? 0 : ret;\n> +}\n> +\n> +std::string CameraHalConfig::findFilePath(const std::string &filename) const\n> +{\n> + static const std::array searchPaths = {\n> + LIBCAMERA_SYSCONF_DIR,\n> + LIBCAMERA_DATA_DIR,\n> + };\n> +\n> + if (File::exists(filename))\n\nnit: Not only the file\n> + return filename;\n> +\n> + std::filesystem::path root = utils::libcameraSourcePath();\n> + if (!root.empty()) {\n> + std::string configurationPath = root / \"data\" / filename;\n> + if (File::exists(configurationPath))\n> + return configurationPath;\n> + }\n> +\n> + for (const std::filesystem::path &path : searchPaths) {\n> + std::string configurationPath = path / filename;\n> + if (File::exists(configurationPath))\n> + return configurationPath;\n> + }\n> +\n> + return \"\";\n> +}\n> +\n\nIf we use std::filesystem::path, we can use\nstd::filesystem::is_character_file(filepath). FWIW,\nis_character_file() returns false if the file doesn't exist.\nI would do as following,\nstd::filesystem::path findFilePath(const std::filesystem::path&)\nFILE *CameraHalConfig::openFile(const std::filesystem::path&)\nThen oepnFile(\"camera_hal.yaml\") should work std::filesystem::path can\nconstruct from char*.\n\n> +FILE *CameraHalConfig::openFile(const std::string &filename)\n> +{\n> + const std::string filePath = findFilePath(filename);\n> + if (filePath.empty()) {\n> + LOG(HALConfig, Error)\n> + << \"Configuration file: \\\"\" << filename << \"\\\" not found\";\n> + return nullptr;\n> + }\n> +\n> + LOG(HALConfig, Debug) << \"Reading configuration file from \" << filePath;\n> +\n> + FILE *fh = fopen(filePath.c_str(), \"r\");\n> + if (!fh) {\n> + int ret = -errno;\n> + LOG(HALConfig, Error) << \"Failed to open configuration file \"\n> + << filePath << \": \" << strerror(-ret);\n> + return nullptr;\n> + }\n> +\n> + return fh;\n> +}\n> +\n> +CameraHalConfig::CameraHalConfig()\n> + : Extensible(new Private(this))\n> +{\n> +}\n> +\n> +/*\n> + * Open the HAL configuration file and validate its content.\n> + * Return 0 on success, a negative error code otherwise\n> + */\n> +int CameraHalConfig::open()\n> +{\n> + FILE *fh = openFile(\"camera_hal.yaml\");\n> + if (!fh)\n> + return -ENOENT;\n> +\n> + Private *const d = LIBCAMERA_D_PTR();\n> + int ret = d->parseConfigFile(fh, &cameras_);\n> + fclose(fh);\n> + if (ret)\n> + return ret;\n> +\n> + for (const auto &c : cameras_) {\n> + const std::string &cameraId = c.first;\n> + const CameraProps &camera = c.second;\n> + LOG(HALConfig, Debug) << \"'\" << cameraId << \"' \"\n> + << \"(\" << camera.facing << \")[\"\n> + << camera.rotation << \"]\";\n> + }\n> +\n> + return 0;\n> +}\n> +\n> +const CameraProps &CameraHalConfig::cameraProps(const std::string &cameraId) const\n> +{\n> + static CameraProps empty;\n> +\n> + const auto &it = cameras_.find(cameraId);\n> + if (it == cameras_.end()) {\n> + LOG(HALConfig, Error)\n> + << \"Camera '\" << cameraId\n> + << \"' not described in the HAL configuration file\";\n> + return empty;\n> + }\n> +\n> + return it->second;\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..65569559fbf0\n> --- /dev/null\n> +++ b/src/android/camera_hal_config.h\n> @@ -0,0 +1,38 @@\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> +\n> +#include \n> +\n> +struct CameraProps {\n> + int facing;\n> + int rotation;\n> +\n> + bool valid = false;\n> +};\n> +\n\nnit: I would set facing and rotation to -1 as invalid values.\n\nWith those nits,\nReviewed-by: Hirokazu Honda \n\nThanks,\n-Hiro\n> +class CameraHalConfig final : public libcamera::Extensible\n> +{\n> + LIBCAMERA_DECLARE_PRIVATE(CameraBuffer)\n> +\n> +public:\n> + CameraHalConfig();\n> +\n> + int open();\n> + const CameraProps &cameraProps(const std::string &cameraId) const;\n> +\n> +private:\n> + std::string findFilePath(const std::string &filename) const;\n> + FILE *openFile(const std::string &filename);\n> +\n> + std::map cameras_;\n> +};\n> +#endif /* __ANDROID_CAMERA_HAL_CONFIG_H__ */\n> diff --git a/src/android/meson.build b/src/android/meson.build\n> index 2be20c97e118..3893e5b5b832 100644\n> --- a/src/android/meson.build\n> +++ b/src/android/meson.build\n> @@ -3,6 +3,7 @@\n> android_deps = [\n> dependency('libexif', required : get_option('android')),\n> dependency('libjpeg', required : get_option('android')),\n> + dependency('yaml-0.1', required : get_option('android')),\n> ]\n>\n> android_enabled = true\n> @@ -45,6 +46,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.31.1\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 5B2C4C0DA3\n\tfor ;\n\tWed, 7 Apr 2021 03:45:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B6D2F6879F;\n\tWed, 7 Apr 2021 05:45:24 +0200 (CEST)","from mail-ej1-x62a.google.com (mail-ej1-x62a.google.com\n\t[IPv6:2a00:1450:4864:20::62a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 38F1D602CD\n\tfor ;\n\tWed, 7 Apr 2021 05:45:23 +0200 (CEST)","by mail-ej1-x62a.google.com with SMTP id hq27so25234816ejc.9\n\tfor ;\n\tTue, 06 Apr 2021 20:45:23 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"Ab4L1oUE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=H3X6vNW5A5vYCaHffrq+6UJm+enHc14xbKeToXwwDB0=;\n\tb=Ab4L1oUEqG46uwxRD42srC9Z9WbhsFPfiI2ylU91MwKOoUowD0S6HcaSza59QkgER9\n\tCqllgs/7SXsJez3qNA1U4xAESHXaFdvbADAH5eCs9cn4zV2krPvmhWShLUxhy6Y3b3yH\n\tVmKzFt0X8V2cCiBBfnlPqs2wdlow34HupFdRE=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=H3X6vNW5A5vYCaHffrq+6UJm+enHc14xbKeToXwwDB0=;\n\tb=JyqGv7rkcpbZlAWj3A37QIQVZVZ657FAKEqjS8B0QQ/QZtFs51OPnpK2Jk9vAzTpMg\n\t4VXfndEY4Pj149V3DEGsPNXKHCc5RfQQciMC48yc+KlVGSVtrEn2XtLKOy8zE7ZlE7Cv\n\tVxLK1JR8NXQBDEdt+TkR2FVYQGq/8H9pzMXsac3UgkFtxtk2ZmmVaFASaB+A1trAiMqB\n\tzMQC82pW18SZEGO4D2zDCLYqt7sbB1ae/974qykSW70+eL/qWf7fmsDE5zjYYA2iXFil\n\tYzoV+Q6zTtQwaqC+5KPYIMaDkWeR8/ccoVh9amf15jW24niM5rwOnYvnwL3YtNmcBFMP\n\tvlYw==","X-Gm-Message-State":"AOAM531ODgcZUErlnRqZAwikHyvJa8SitIAFy6NBjz6Uw5eoyE7kg+f5\n\tGm2L8hTWjOO0hYyX2PFQ3X51wd6Y7ibh+Tb3GtgbwQ==","X-Google-Smtp-Source":"ABdhPJwxAAj0VRb5Hs9CBFBf98l8mOU85+vsbeBxF+1NqlsJIqOqmzsP5EpEGGmFi4DNei88mrznZB+K9uGXievWpZY=","X-Received":"by 2002:a17:906:7fd9:: with SMTP id\n\tr25mr1347045ejs.221.1617767122704; \n\tTue, 06 Apr 2021 20:45:22 -0700 (PDT)","MIME-Version":"1.0","References":"<20210406154557.27303-1-jacopo@jmondi.org>\n\t<20210406154557.27303-4-jacopo@jmondi.org>","In-Reply-To":"<20210406154557.27303-4-jacopo@jmondi.org>","From":"Hirokazu Honda ","Date":"Wed, 7 Apr 2021 12:45:12 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v4 3/5] android: Add CameraHalConfig\n\tclass","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16132,"web_url":"https://patchwork.libcamera.org/comment/16132/","msgid":"<20210407074713.ypsnpxd5qmrdjizt@uno.localdomain>","date":"2021-04-07T07:47:13","subject":"Re: [libcamera-devel] [PATCH v4 3/5] android: Add CameraHalConfig\n\tclass","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Wed, Apr 07, 2021 at 12:45:12PM +0900, Hirokazu Honda wrote:\n> Hi Jacopo, Thanks for this patch.\n>\n> > +}\n> > +\n> > +std::string CameraHalConfig::findFilePath(const std::string &filename) const\n> > +{\n> > + static const std::array searchPaths = {\n> > + LIBCAMERA_SYSCONF_DIR,\n> > + LIBCAMERA_DATA_DIR,\n> > + };\n> > +\n> > + if (File::exists(filename))\n>\n> nit: Not only the file\n\nI'm sorry I didn't get this comment...\n\n> > + return filename;\n> > +\n> > + std::filesystem::path root = utils::libcameraSourcePath();\n> > + if (!root.empty()) {\n> > + std::string configurationPath = root / \"data\" / filename;\n> > + if (File::exists(configurationPath))\n> > + return configurationPath;\n> > + }\n> > +\n> > + for (const std::filesystem::path &path : searchPaths) {\n> > + std::string configurationPath = path / filename;\n> > + if (File::exists(configurationPath))\n> > + return configurationPath;\n> > + }\n> > +\n> > + return \"\";\n> > +}\n> > +\n>\n> If we use std::filesystem::path, we can use\n> std::filesystem::is_character_file(filepath). FWIW,\n> is_character_file() returns false if the file doesn't exist.\n> I would do as following,\n> std::filesystem::path findFilePath(const std::filesystem::path&)\n> FILE *CameraHalConfig::openFile(const std::filesystem::path&)\n> Then oepnFile(\"camera_hal.yaml\") should work std::filesystem::path can\n> construct from char*.\n>\n\nFile::exists() is used library-wide. To be honest I don't see what we\nwould gain.\n\n> > +FILE *CameraHalConfig::openFile(const std::string &filename)\n> > +{\n> > + const std::string filePath = findFilePath(filename);\n> > + if (filePath.empty()) {\n> > + LOG(HALConfig, Error)\n> > + << \"Configuration file: \\\"\" << filename << \"\\\" not found\";\n> > + return nullptr;\n> > + }\n> > +\n> > + LOG(HALConfig, Debug) << \"Reading configuration file from \" << filePath;\n> > +\n> > + FILE *fh = fopen(filePath.c_str(), \"r\");\n> > + if (!fh) {\n> > + int ret = -errno;\n> > + LOG(HALConfig, Error) << \"Failed to open configuration file \"\n> > + << filePath << \": \" << strerror(-ret);\n> > + return nullptr;\n> > + }\n> > +\n> > + return fh;\n> > +}\n> > +\n> > +CameraHalConfig::CameraHalConfig()\n> > + : Extensible(new Private(this))\n> > +{\n> > +}\n> > +\n> > +/*\n> > + * Open the HAL configuration file and validate its content.\n> > + * Return 0 on success, a negative error code otherwise\n> > + */\n> > +int CameraHalConfig::open()\n> > +{\n> > + FILE *fh = openFile(\"camera_hal.yaml\");\n> > + if (!fh)\n> > + return -ENOENT;\n> > +\n> > + Private *const d = LIBCAMERA_D_PTR();\n> > + int ret = d->parseConfigFile(fh, &cameras_);\n> > + fclose(fh);\n> > + if (ret)\n> > + return ret;\n> > +\n> > + for (const auto &c : cameras_) {\n> > + const std::string &cameraId = c.first;\n> > + const CameraProps &camera = c.second;\n> > + LOG(HALConfig, Debug) << \"'\" << cameraId << \"' \"\n> > + << \"(\" << camera.facing << \")[\"\n> > + << camera.rotation << \"]\";\n> > + }\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +const CameraProps &CameraHalConfig::cameraProps(const std::string &cameraId) const\n> > +{\n> > + static CameraProps empty;\n> > +\n> > + const auto &it = cameras_.find(cameraId);\n> > + if (it == cameras_.end()) {\n> > + LOG(HALConfig, Error)\n> > + << \"Camera '\" << cameraId\n> > + << \"' not described in the HAL configuration file\";\n> > + return empty;\n> > + }\n> > +\n> > + return it->second;\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..65569559fbf0\n> > --- /dev/null\n> > +++ b/src/android/camera_hal_config.h\n> > @@ -0,0 +1,38 @@\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> > +\n> > +#include \n> > +\n> > +struct CameraProps {\n> > + int facing;\n> > + int rotation;\n> > +\n> > + bool valid = false;\n> > +};\n> > +\n>\n> nit: I would set facing and rotation to -1 as invalid values.\n>\n\nIt seems like the 'valid' flag might get set to true also for camera\nentries that do not specify 'location', 'rotation' or none of them. I\ncould drop 'valid' and initialize the single field, and fail out loud\nif the value is retrieved from the configuration file but not\ninitialized.\n\nThanks\n j\n\n\n> With those nits,\n> Reviewed-by: Hirokazu Honda \n>\n> Thanks,\n> -Hiro\n> > +class CameraHalConfig final : public libcamera::Extensible\n> > +{\n> > + LIBCAMERA_DECLARE_PRIVATE(CameraBuffer)\n> > +\n> > +public:\n> > + CameraHalConfig();\n> > +\n> > + int open();\n> > + const CameraProps &cameraProps(const std::string &cameraId) const;\n> > +\n> > +private:\n> > + std::string findFilePath(const std::string &filename) const;\n> > + FILE *openFile(const std::string &filename);\n> > +\n> > + std::map cameras_;\n> > +};\n> > +#endif /* __ANDROID_CAMERA_HAL_CONFIG_H__ */\n> > diff --git a/src/android/meson.build b/src/android/meson.build\n> > index 2be20c97e118..3893e5b5b832 100644\n> > --- a/src/android/meson.build\n> > +++ b/src/android/meson.build\n> > @@ -3,6 +3,7 @@\n> > android_deps = [\n> > dependency('libexif', required : get_option('android')),\n> > dependency('libjpeg', required : get_option('android')),\n> > + dependency('yaml-0.1', required : get_option('android')),\n> > ]\n> >\n> > android_enabled = true\n> > @@ -45,6 +46,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.31.1\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 DE6F7BD224\n\tfor ;\n\tWed, 7 Apr 2021 07:46:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3AFDA687A2;\n\tWed, 7 Apr 2021 09:46:38 +0200 (CEST)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2406C68795\n\tfor ;\n\tWed, 7 Apr 2021 09:46: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 relay7-d.mail.gandi.net (Postfix) with ESMTPSA id AA34820004;\n\tWed, 7 Apr 2021 07:46:35 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Wed, 7 Apr 2021 09:47:13 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210407074713.ypsnpxd5qmrdjizt@uno.localdomain>","References":"<20210406154557.27303-1-jacopo@jmondi.org>\n\t<20210406154557.27303-4-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 3/5] android: Add CameraHalConfig\n\tclass","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16133,"web_url":"https://patchwork.libcamera.org/comment/16133/","msgid":"","date":"2021-04-07T07:52:22","subject":"Re: [libcamera-devel] [PATCH v4 3/5] android: Add CameraHalConfig\n\tclass","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo,\n\nOn Wed, Apr 7, 2021 at 4:46 PM Jacopo Mondi wrote:\n>\n> Hi Hiro,\n>\n> On Wed, Apr 07, 2021 at 12:45:12PM +0900, Hirokazu Honda wrote:\n> > Hi Jacopo, Thanks for this patch.\n> >\n> > > +}\n> > > +\n> > > +std::string CameraHalConfig::findFilePath(const std::string &filename) const\n> > > +{\n> > > + static const std::array searchPaths = {\n> > > + LIBCAMERA_SYSCONF_DIR,\n> > > + LIBCAMERA_DATA_DIR,\n> > > + };\n> > > +\n> > > + if (File::exists(filename))\n> >\n> > nit: Not only the file\n>\n> I'm sorry I didn't get this comment...\n>\n\nSorry, please ignore. I would write a comment about std::filesystem\ndescribed below.\n\n> > > + return filename;\n> > > +\n> > > + std::filesystem::path root = utils::libcameraSourcePath();\n> > > + if (!root.empty()) {\n> > > + std::string configurationPath = root / \"data\" / filename;\n> > > + if (File::exists(configurationPath))\n> > > + return configurationPath;\n> > > + }\n> > > +\n> > > + for (const std::filesystem::path &path : searchPaths) {\n> > > + std::string configurationPath = path / filename;\n> > > + if (File::exists(configurationPath))\n> > > + return configurationPath;\n> > > + }\n> > > +\n> > > + return \"\";\n> > > +}\n> > > +\n> >\n> > If we use std::filesystem::path, we can use\n> > std::filesystem::is_character_file(filepath). FWIW,\n> > is_character_file() returns false if the file doesn't exist.\n> > I would do as following,\n> > std::filesystem::path findFilePath(const std::filesystem::path&)\n> > FILE *CameraHalConfig::openFile(const std::filesystem::path&)\n> > Then oepnFile(\"camera_hal.yaml\") should work std::filesystem::path can\n> > construct from char*.\n> >\n>\n> File::exists() is used library-wide. To be honest I don't see what we\n> would gain.\n>\n\nI prefer using a standard library.\nSince we already use std::filesystem here, I think it would be\nconsistency to use std::filesystem function.\nI am ok to use File::exists() though.\n\nThanks,\n-Hiro\n> > > +FILE *CameraHalConfig::openFile(const std::string &filename)\n> > > +{\n> > > + const std::string filePath = findFilePath(filename);\n> > > + if (filePath.empty()) {\n> > > + LOG(HALConfig, Error)\n> > > + << \"Configuration file: \\\"\" << filename << \"\\\" not found\";\n> > > + return nullptr;\n> > > + }\n> > > +\n> > > + LOG(HALConfig, Debug) << \"Reading configuration file from \" << filePath;\n> > > +\n> > > + FILE *fh = fopen(filePath.c_str(), \"r\");\n> > > + if (!fh) {\n> > > + int ret = -errno;\n> > > + LOG(HALConfig, Error) << \"Failed to open configuration file \"\n> > > + << filePath << \": \" << strerror(-ret);\n> > > + return nullptr;\n> > > + }\n> > > +\n> > > + return fh;\n> > > +}\n> > > +\n> > > +CameraHalConfig::CameraHalConfig()\n> > > + : Extensible(new Private(this))\n> > > +{\n> > > +}\n> > > +\n> > > +/*\n> > > + * Open the HAL configuration file and validate its content.\n> > > + * Return 0 on success, a negative error code otherwise\n> > > + */\n> > > +int CameraHalConfig::open()\n> > > +{\n> > > + FILE *fh = openFile(\"camera_hal.yaml\");\n> > > + if (!fh)\n> > > + return -ENOENT;\n> > > +\n> > > + Private *const d = LIBCAMERA_D_PTR();\n> > > + int ret = d->parseConfigFile(fh, &cameras_);\n> > > + fclose(fh);\n> > > + if (ret)\n> > > + return ret;\n> > > +\n> > > + for (const auto &c : cameras_) {\n> > > + const std::string &cameraId = c.first;\n> > > + const CameraProps &camera = c.second;\n> > > + LOG(HALConfig, Debug) << \"'\" << cameraId << \"' \"\n> > > + << \"(\" << camera.facing << \")[\"\n> > > + << camera.rotation << \"]\";\n> > > + }\n> > > +\n> > > + return 0;\n> > > +}\n> > > +\n> > > +const CameraProps &CameraHalConfig::cameraProps(const std::string &cameraId) const\n> > > +{\n> > > + static CameraProps empty;\n> > > +\n> > > + const auto &it = cameras_.find(cameraId);\n> > > + if (it == cameras_.end()) {\n> > > + LOG(HALConfig, Error)\n> > > + << \"Camera '\" << cameraId\n> > > + << \"' not described in the HAL configuration file\";\n> > > + return empty;\n> > > + }\n> > > +\n> > > + return it->second;\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..65569559fbf0\n> > > --- /dev/null\n> > > +++ b/src/android/camera_hal_config.h\n> > > @@ -0,0 +1,38 @@\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> > > +\n> > > +#include \n> > > +\n> > > +struct CameraProps {\n> > > + int facing;\n> > > + int rotation;\n> > > +\n> > > + bool valid = false;\n> > > +};\n> > > +\n> >\n> > nit: I would set facing and rotation to -1 as invalid values.\n> >\n>\n> It seems like the 'valid' flag might get set to true also for camera\n> entries that do not specify 'location', 'rotation' or none of them. I\n> could drop 'valid' and initialize the single field, and fail out loud\n> if the value is retrieved from the configuration file but not\n> initialized.\n>\n> Thanks\n> j\n>\n>\n> > With those nits,\n> > Reviewed-by: Hirokazu Honda \n> >\n> > Thanks,\n> > -Hiro\n> > > +class CameraHalConfig final : public libcamera::Extensible\n> > > +{\n> > > + LIBCAMERA_DECLARE_PRIVATE(CameraBuffer)\n> > > +\n> > > +public:\n> > > + CameraHalConfig();\n> > > +\n> > > + int open();\n> > > + const CameraProps &cameraProps(const std::string &cameraId) const;\n> > > +\n> > > +private:\n> > > + std::string findFilePath(const std::string &filename) const;\n> > > + FILE *openFile(const std::string &filename);\n> > > +\n> > > + std::map cameras_;\n> > > +};\n> > > +#endif /* __ANDROID_CAMERA_HAL_CONFIG_H__ */\n> > > diff --git a/src/android/meson.build b/src/android/meson.build\n> > > index 2be20c97e118..3893e5b5b832 100644\n> > > --- a/src/android/meson.build\n> > > +++ b/src/android/meson.build\n> > > @@ -3,6 +3,7 @@\n> > > android_deps = [\n> > > dependency('libexif', required : get_option('android')),\n> > > dependency('libjpeg', required : get_option('android')),\n> > > + dependency('yaml-0.1', required : get_option('android')),\n> > > ]\n> > >\n> > > android_enabled = true\n> > > @@ -45,6 +46,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.31.1\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 A863EBD1F6\n\tfor ;\n\tWed, 7 Apr 2021 07:52:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0728D687D5;\n\tWed, 7 Apr 2021 09:52:35 +0200 (CEST)","from mail-ed1-x533.google.com (mail-ed1-x533.google.com\n\t[IPv6:2a00:1450:4864:20::533])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 31D0768795\n\tfor ;\n\tWed, 7 Apr 2021 09:52:34 +0200 (CEST)","by mail-ed1-x533.google.com with SMTP id r22so7156481edq.9\n\tfor ;\n\tWed, 07 Apr 2021 00:52:34 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"bqcqWqqd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=Pdx43B1s6/26p4KUdro9apPkX5lzYipXLVxY/uFLsZI=;\n\tb=bqcqWqqdtBIcE04oe5z3mefG0k0T51TcnUB3f79IVXuc6uz6Z/SutgrwoMcfnHwPJ/\n\t8639lraBO48f0+f6TXVehKPbjVuR6bWle/h6eVJ0mB6Ydq3jrUL08crmrteQQhTFSmIE\n\tV0Bm/BRYXQapXCkyTKJ9W8bnMgpGhHGqG+TR8=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=Pdx43B1s6/26p4KUdro9apPkX5lzYipXLVxY/uFLsZI=;\n\tb=gQQ7PgAtC8EumxSAbIOrIBPpNJnIGMqWWjOR/nuj8he0r0NRVAstzyRBtdONUHxlJN\n\t+87WVQtmR8MiZP8KTCaA//UmMG6T+CdeKaYP2YjP9+R0LqHxtEwtksekZVlt4qzK6LsC\n\taHABHZ6oab+XhixehmSSo6NwHnZz7xHXroUhhxVPVg5Tp93G7vHfRvUGcpM/1FD0YyP+\n\t3ozGo12AksqVoRqHtKczG24VutUvvZRFyf5p9cUO8OOaeEUyknaYBUD27rxAKOAKOGdZ\n\tSxMx+xgOCamTVLh8g4jJ7oWrDp6fTmH+jsCkFsR611ljVzzEHqUVRZLrcjT+zDJVJPAz\n\t1Sxw==","X-Gm-Message-State":"AOAM533p1k0hrmZS9A+fTyjoSGVCU0LujT1zokbi/sWim6uZYA19hafX\n\tv35cHbmS90hyOSFWp+XG6KxDwz+wABc6NdSUVi+SpK5y5u0=","X-Google-Smtp-Source":"ABdhPJxDrfJBngX4iKzViOwPpowzghQ4UxCyIYIyAHfRDoIHeO8eNTWZZH/KU5nGDWZdfOH7cENDk9Or05IOOUpsNhU=","X-Received":"by 2002:a05:6402:51cd:: with SMTP id\n\tr13mr2860428edd.116.1617781953573; \n\tWed, 07 Apr 2021 00:52:33 -0700 (PDT)","MIME-Version":"1.0","References":"<20210406154557.27303-1-jacopo@jmondi.org>\n\t<20210406154557.27303-4-jacopo@jmondi.org>\n\t\n\t<20210407074713.ypsnpxd5qmrdjizt@uno.localdomain>","In-Reply-To":"<20210407074713.ypsnpxd5qmrdjizt@uno.localdomain>","From":"Hirokazu Honda ","Date":"Wed, 7 Apr 2021 16:52:22 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v4 3/5] android: Add CameraHalConfig\n\tclass","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16211,"web_url":"https://patchwork.libcamera.org/comment/16211/","msgid":"","date":"2021-04-13T03:40:50","subject":"Re: [libcamera-devel] [PATCH v4 3/5] android: Add CameraHalConfig\n\tclass","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Wed, Apr 07, 2021 at 04:52:22PM +0900, Hirokazu Honda wrote:\n> On Wed, Apr 7, 2021 at 4:46 PM Jacopo Mondi wrote:\n> > On Wed, Apr 07, 2021 at 12:45:12PM +0900, Hirokazu Honda wrote:\n> > > Hi Jacopo, Thanks for this patch.\n> > >\n> > > > +}\n> > > > +\n> > > > +std::string CameraHalConfig::findFilePath(const std::string &filename) const\n> > > > +{\n> > > > + static const std::array searchPaths = {\n> > > > + LIBCAMERA_SYSCONF_DIR,\n> > > > + LIBCAMERA_DATA_DIR,\n> > > > + };\n> > > > +\n> > > > + if (File::exists(filename))\n> > >\n> > > nit: Not only the file\n> >\n> > I'm sorry I didn't get this comment...\n> >\n> \n> Sorry, please ignore. I would write a comment about std::filesystem\n> described below.\n> \n> > > > + return filename;\n> > > > +\n> > > > + std::filesystem::path root = utils::libcameraSourcePath();\n> > > > + if (!root.empty()) {\n> > > > + std::string configurationPath = root / \"data\" / filename;\n> > > > + if (File::exists(configurationPath))\n> > > > + return configurationPath;\n> > > > + }\n> > > > +\n> > > > + for (const std::filesystem::path &path : searchPaths) {\n> > > > + std::string configurationPath = path / filename;\n> > > > + if (File::exists(configurationPath))\n> > > > + return configurationPath;\n> > > > + }\n> > > > +\n> > > > + return \"\";\n> > > > +}\n> > > > +\n> > >\n> > > If we use std::filesystem::path, we can use\n> > > std::filesystem::is_character_file(filepath). FWIW,\n> > > is_character_file() returns false if the file doesn't exist.\n> > > I would do as following,\n> > > std::filesystem::path findFilePath(const std::filesystem::path&)\n> > > FILE *CameraHalConfig::openFile(const std::filesystem::path&)\n> > > Then oepnFile(\"camera_hal.yaml\") should work std::filesystem::path can\n> > > construct from char*.\n> >\n> > File::exists() is used library-wide. To be honest I don't see what we\n> > would gain.\n> \n> I prefer using a standard library.\n> Since we already use std::filesystem here, I think it would be\n> consistency to use std::filesystem function.\n> I am ok to use File::exists() though.\n\nOur File class predates the switch to C++17. I'm not opposed to moving\nto std::filesystem. One feature that File provides is RAII semantics for\nmmap(), and as far as I can tell, the C++ standard library doesn't\nprovide that. We could keep the File class for that purpose, and drop\nFile::exists() in favour of std::filesystem. It may not have to be done\nin this patch series though, we could do so library-wide later.\n\nhttps://bugs.libcamera.org/show_bug.cgi?id=25\n\n> > > > +FILE *CameraHalConfig::openFile(const std::string &filename)\n> > > > +{\n> > > > + const std::string filePath = findFilePath(filename);\n> > > > + if (filePath.empty()) {\n> > > > + LOG(HALConfig, Error)\n> > > > + << \"Configuration file: \\\"\" << filename << \"\\\" not found\";\n> > > > + return nullptr;\n> > > > + }\n> > > > +\n> > > > + LOG(HALConfig, Debug) << \"Reading configuration file from \" << filePath;\n> > > > +\n> > > > + FILE *fh = fopen(filePath.c_str(), \"r\");\n> > > > + if (!fh) {\n> > > > + int ret = -errno;\n> > > > + LOG(HALConfig, Error) << \"Failed to open configuration file \"\n> > > > + << filePath << \": \" << strerror(-ret);\n> > > > + return nullptr;\n> > > > + }\n> > > > +\n> > > > + return fh;\n> > > > +}\n> > > > +\n> > > > +CameraHalConfig::CameraHalConfig()\n> > > > + : Extensible(new Private(this))\n> > > > +{\n> > > > +}\n> > > > +\n> > > > +/*\n> > > > + * Open the HAL configuration file and validate its content.\n> > > > + * Return 0 on success, a negative error code otherwise\n> > > > + */\n> > > > +int CameraHalConfig::open()\n> > > > +{\n> > > > + FILE *fh = openFile(\"camera_hal.yaml\");\n> > > > + if (!fh)\n> > > > + return -ENOENT;\n> > > > +\n> > > > + Private *const d = LIBCAMERA_D_PTR();\n> > > > + int ret = d->parseConfigFile(fh, &cameras_);\n> > > > + fclose(fh);\n> > > > + if (ret)\n> > > > + return ret;\n> > > > +\n> > > > + for (const auto &c : cameras_) {\n> > > > + const std::string &cameraId = c.first;\n> > > > + const CameraProps &camera = c.second;\n> > > > + LOG(HALConfig, Debug) << \"'\" << cameraId << \"' \"\n> > > > + << \"(\" << camera.facing << \")[\"\n> > > > + << camera.rotation << \"]\";\n> > > > + }\n> > > > +\n> > > > + return 0;\n> > > > +}\n> > > > +\n> > > > +const CameraProps &CameraHalConfig::cameraProps(const std::string &cameraId) const\n> > > > +{\n> > > > + static CameraProps empty;\n> > > > +\n> > > > + const auto &it = cameras_.find(cameraId);\n> > > > + if (it == cameras_.end()) {\n> > > > + LOG(HALConfig, Error)\n> > > > + << \"Camera '\" << cameraId\n> > > > + << \"' not described in the HAL configuration file\";\n> > > > + return empty;\n> > > > + }\n> > > > +\n> > > > + return it->second;\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..65569559fbf0\n> > > > --- /dev/null\n> > > > +++ b/src/android/camera_hal_config.h\n> > > > @@ -0,0 +1,38 @@\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> > > > +\n> > > > +#include \n> > > > +\n> > > > +struct CameraProps {\n> > > > + int facing;\n> > > > + int rotation;\n> > > > +\n> > > > + bool valid = false;\n> > > > +};\n> > > > +\n> > >\n> > > nit: I would set facing and rotation to -1 as invalid values.\n> >\n> > It seems like the 'valid' flag might get set to true also for camera\n> > entries that do not specify 'location', 'rotation' or none of them. I\n> > could drop 'valid' and initialize the single field, and fail out loud\n> > if the value is retrieved from the configuration file but not\n> > initialized.\n> >\n> > > With those nits,\n> > > Reviewed-by: Hirokazu Honda \n> > >\n> > > > +class CameraHalConfig final : public libcamera::Extensible\n> > > > +{\n> > > > + LIBCAMERA_DECLARE_PRIVATE(CameraBuffer)\n> > > > +\n> > > > +public:\n> > > > + CameraHalConfig();\n> > > > +\n> > > > + int open();\n> > > > + const CameraProps &cameraProps(const std::string &cameraId) const;\n> > > > +\n> > > > +private:\n> > > > + std::string findFilePath(const std::string &filename) const;\n> > > > + FILE *openFile(const std::string &filename);\n> > > > +\n> > > > + std::map cameras_;\n> > > > +};\n> > > > +#endif /* __ANDROID_CAMERA_HAL_CONFIG_H__ */\n> > > > diff --git a/src/android/meson.build b/src/android/meson.build\n> > > > index 2be20c97e118..3893e5b5b832 100644\n> > > > --- a/src/android/meson.build\n> > > > +++ b/src/android/meson.build\n> > > > @@ -3,6 +3,7 @@\n> > > > android_deps = [\n> > > > dependency('libexif', required : get_option('android')),\n> > > > dependency('libjpeg', required : get_option('android')),\n> > > > + dependency('yaml-0.1', required : get_option('android')),\n> > > > ]\n> > > >\n> > > > android_enabled = true\n> > > > @@ -45,6 +46,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 A8E01BD224\n\tfor ;\n\tTue, 13 Apr 2021 03:41:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 36A10687F4;\n\tTue, 13 Apr 2021 05:41:41 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 250B5687F4\n\tfor ;\n\tTue, 13 Apr 2021 05:41:40 +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 8E7496F2;\n\tTue, 13 Apr 2021 05:41:39 +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=\"Mpzj7Tf4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618285299;\n\tbh=lbNg6gjBP4EoRWi6WJbUqI20o7LcDwXx1h6XSBKNO84=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Mpzj7Tf4tLRPohNthKtdwWOZAK+vHjX661jGwS/3ndUgd9v89o55pmuWJ/9pMWOyx\n\tVJyWAsr6xgRBS0tDIY3jtXIeoWtMKlfPf3v06Q+3cbnpm+L/2+UbLMUxiUUcEDzI4Y\n\tNgefCVauj21Nc24p3FJmD+PeJao39829TMhWiIDk=","Date":"Tue, 13 Apr 2021 06:40:50 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210406154557.27303-1-jacopo@jmondi.org>\n\t<20210406154557.27303-4-jacopo@jmondi.org>\n\t\n\t<20210407074713.ypsnpxd5qmrdjizt@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 3/5] android: Add CameraHalConfig\n\tclass","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16212,"web_url":"https://patchwork.libcamera.org/comment/16212/","msgid":"","date":"2021-04-13T04:18:47","subject":"Re: [libcamera-devel] [PATCH v4 3/5] android: Add CameraHalConfig\n\tclass","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Tue, Apr 06, 2021 at 05:45:55PM +0200, Jacopo Mondi wrote:\n> Add a CameraHalConfig class to the Android Camera3 HAL layer.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> README.rst | 2 +-\n> src/android/camera_hal_config.cpp | 433 ++++++++++++++++++++++++++++++\n> src/android/camera_hal_config.h | 38 +++\n> src/android/meson.build | 2 +\n> 4 files changed, 474 insertions(+), 1 deletion(-)\n> create mode 100644 src/android/camera_hal_config.cpp\n> create mode 100644 src/android/camera_hal_config.h\n> \n> diff --git a/README.rst b/README.rst\n> index c77e54b48b7a..fcf0f47f14c5 100644\n> --- a/README.rst\n> +++ b/README.rst\n> @@ -88,7 +88,7 @@ for tracing with lttng: [optional]\n> liblttng-ust-dev python3-jinja2 lttng-tools\n> \n> for android: [optional]\n> - libexif libjpeg\n> + libexif libjpeg libyaml\n> \n> Using GStreamer plugin\n> ~~~~~~~~~~~~~~~~~~~~~~\n> diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp\n> new file mode 100644\n> index 000000000000..5b51fa4dd61b\n> --- /dev/null\n> +++ b/src/android/camera_hal_config.cpp\n> @@ -0,0 +1,433 @@\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> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \"libcamera/internal/file.h\"\n> +#include \"libcamera/internal/log.h\"\n> +\n> +using namespace libcamera;\n> +\n> +LOG_DEFINE_CATEGORY(HALConfig)\n> +\n> +class CameraHalConfig::Private : public Extensible::Private\n> +{\n> +\tLIBCAMERA_DECLARE_PUBLIC(CameraHalConfig)\n> +\n> +public:\n> +\tPrivate(CameraHalConfig *halConfig);\n> +\n> +\tint parseConfigFile(FILE *fh, std::map *cameras);\n> +\n> +private:\n> +\tstd::string parseValue();\n> +\tstd::string parseKey();\n> +\tint parseValueBlock();\n> +\tint parseCameraLocation(CameraProps *cameraProps, const std::string &location);\n> +\tint parseCameraProps(const std::string &cameraId);\n> +\tint parseCameras();\n> +\tint parseEntry();\n> +\n> +\tyaml_parser_t parser_;\n> +\tstd::map *cameras_;\n> +};\n> +\n> +CameraHalConfig::Private::Private(CameraHalConfig *halConfig)\n> +\t: Extensible::Private(halConfig)\n> +{\n> +}\n> +\n> +std::string CameraHalConfig::Private::parseValue()\n> +{\n> +\tyaml_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\tyaml_token_delete(&token);\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\tyaml_token_delete(&token);\n> +\t\treturn \"\";\n> +\t}\n> +\n> +\tstd::string value(reinterpret_cast(token.data.scalar.value),\n> +\t\t\t token.data.scalar.length);\n> +\tyaml_token_delete(&token);\n> +\n> +\treturn value;\n> +}\n> +\n> +std::string CameraHalConfig::Private::parseKey()\n> +{\n> +\tyaml_token_t token;\n> +\n> +\t/* Make sure the token type is a key and get its value. */\n> +\tyaml_parser_scan(&parser_, &token);\n> +\tif (token.type != YAML_SCALAR_TOKEN) {\n> +\t\tyaml_token_delete(&token);\n> +\t\treturn \"\";\n> +\t}\n> +\n> +\tstd::string value(reinterpret_cast(token.data.scalar.value),\n> +\t\t\t token.data.scalar.length);\n> +\tyaml_token_delete(&token);\n> +\n> +\treturn value;\n> +}\n> +\n> +int CameraHalConfig::Private::parseValueBlock()\n> +{\n> +\tyaml_token_t token;\n> +\n> +\t/* Make sure the next token are VALUE and BLOCK_MAPPING_START. */\n> +\tyaml_parser_scan(&parser_, &token);\n> +\tif (token.type != YAML_VALUE_TOKEN) {\n> +\t\tyaml_token_delete(&token);\n> +\t\treturn -EINVAL;\n> +\t}\n> +\tyaml_token_delete(&token);\n> +\n> +\tyaml_parser_scan(&parser_, &token);\n> +\tif (token.type != YAML_BLOCK_MAPPING_START_TOKEN) {\n> +\t\tyaml_token_delete(&token);\n> +\t\treturn -EINVAL;\n> +\t}\n> +\tyaml_token_delete(&token);\n> +\n> +\treturn 0;\n> +}\n> +\n> +int CameraHalConfig::Private::parseCameraLocation(CameraProps *cameraProps, const std::string &location)\n> +{\n> +\tif (location == \"front\")\n> +\t\tcameraProps->facing = CAMERA_FACING_FRONT;\n> +\telse if (location == \"back\")\n> +\t\tcameraProps->facing = CAMERA_FACING_BACK;\n> +\telse if (location == \"external\")\n> +\t\tcameraProps->facing = CAMERA_FACING_EXTERNAL;\n> +\telse\n> +\t\treturn -EINVAL;\n> +\n> +\treturn 0;\n> +}\n> +\n> +int CameraHalConfig::Private::parseCameraProps(const std::string &cameraId)\n> +{\n> +\tint ret = parseValueBlock();\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\t/*\n> +\t * Parse the camera properties and store them in a cameraProps instance.\n> +\t *\n> +\t * Add a safety counter to make sure we don't loop indefinitely in case\n> +\t * the configuration file is malformed.\n> +\t */\n> +\tunsigned int sentinel = 100;\n> +\tCameraProps cameraProps;\n> +\tbool blockEnd = false;\n> +\tyaml_token_t token;\n> +\n> +\tdo {\n> +\t\tyaml_parser_scan(&parser_, &token);\n> +\t\tswitch (token.type) {\n> +\t\tcase YAML_KEY_TOKEN: {\n> +\t\t\tyaml_token_delete(&token);\n> +\n> +\t\t\t/*\n> +\t\t\t * Parse the camera property key and make sure it is\n> +\t\t\t * valid.\n> +\t\t\t */\n> +\t\t\tstd::string key = parseKey();\n> +\t\t\tstd::string value = parseValue();\n> +\t\t\tif (key.empty() || value.empty())\n> +\t\t\t\treturn -EINVAL;\n> +\n> +\t\t\tif (key == \"location\") {\n> +\t\t\t\tret = parseCameraLocation(&cameraProps, value);\n> +\t\t\t\tif (ret) {\n> +\t\t\t\t\tLOG(HALConfig, Error)\n> +\t\t\t\t\t\t<< \"Unknown location: \"\n> +\t\t\t\t\t\t<< value;\n> +\t\t\t\t\treturn -EINVAL;\n> +\t\t\t\t}\n> +\t\t\t} else if (key == \"rotation\") {\n> +\t\t\t\tret = std::stoi(value);\n> +\t\t\t\tif (ret < 0 || ret >= 360) {\n> +\t\t\t\t\tLOG(HALConfig, Error)\n> +\t\t\t\t\t\t<< \"Unknown rotation: \"\n> +\t\t\t\t\t\t<< cameraProps.rotation;\n> +\t\t\t\t\treturn -EINVAL;\n> +\t\t\t\t}\n> +\t\t\t\tcameraProps.rotation = ret;\n> +\t\t\t} else {\n> +\t\t\t\tLOG(HALConfig, Error)\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> +\n> +\t\tcase YAML_BLOCK_END_TOKEN:\n> +\t\t\tblockEnd = true;\n> +\t\t\t[[fallthrough]];\n> +\t\tdefault:\n> +\t\t\tyaml_token_delete(&token);\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\t--sentinel;\n> +\t} while (!blockEnd && sentinel);\n> +\tif (!sentinel)\n> +\t\treturn -EINVAL;\n> +\n> +\tcameraProps.valid = true;\n> +\t(*cameras_)[cameraId] = cameraProps;\n> +\n> +\treturn 0;\n> +}\n> +\n> +int CameraHalConfig::Private::parseCameras()\n> +{\n> +\tint ret = parseValueBlock();\n> +\tif (ret) {\n> +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> +\t\treturn ret;\n> +\t}\n> +\n> +\t/*\n> +\t * Parse the camera properties.\n> +\t *\n> +\t * Each camera properties block is a list of properties associated\n> +\t * with the ID (as assembled by CameraSensor::generateId()) of the\n> +\t * camera they refer to.\n> +\t *\n> +\t * cameras:\n> +\t * \"camera0 id\":\n> +\t * key: value\n> +\t * key: value\n> +\t * ...\n> +\t *\n> +\t * \"camera1 id\":\n> +\t * key: value\n> +\t * key: value\n> +\t * ...\n> +\t */\n> +\tbool blockEnd = false;\n> +\tyaml_token_t token;\n> +\tdo {\n> +\t\tyaml_parser_scan(&parser_, &token);\n> +\t\tswitch (token.type) {\n> +\t\tcase YAML_KEY_TOKEN: {\n> +\t\t\tyaml_token_delete(&token);\n> +\n> +\t\t\t/* Parse the camera ID as key of the property list. */\n> +\t\t\tstd::string cameraId = parseKey();\n> +\t\t\tif (cameraId.empty())\n> +\t\t\t\treturn -EINVAL;\n> +\n> +\t\t\tret = parseCameraProps(cameraId);\n> +\t\t\tif (ret)\n> +\t\t\t\treturn -EINVAL;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tcase YAML_BLOCK_END_TOKEN:\n> +\t\t\tblockEnd = true;\n> +\t\t\t[[fallthrough]];\n> +\t\tdefault:\n> +\t\t\tyaml_token_delete(&token);\n> +\t\t\tbreak;\n> +\t\t}\n> +\t} while (!blockEnd);\n> +\n> +\treturn 0;\n> +}\n> +\n> +int CameraHalConfig::Private::parseEntry()\n> +{\n> +\tint ret = -EINVAL;\n> +\n> +\t/*\n> +\t * Parse each key we find in the file.\n> +\t *\n> +\t * The 'cameras' keys maps to a list of (lists) of camera properties.\n> +\t */\n> +\n> +\tstd::string key = parseKey();\n> +\tif (key.empty())\n> +\t\treturn ret;\n> +\n> +\tif (key == \"cameras\")\n> +\t\tret = parseCameras();\n> +\telse\n> +\t\tLOG(HALConfig, Error) << \"Unknown key: \" << key;\n> +\n> +\treturn ret;\n> +}\n> +\n> +int CameraHalConfig::Private::parseConfigFile(FILE *fh,\n> +\t\t\t\t\t std::map *cameras)\n> +{\n> +\tcameras_ = cameras;\n> +\n> +\tint ret = yaml_parser_initialize(&parser_);\n> +\tif (!ret) {\n> +\t\tLOG(HALConfig, Fatal) << \"Failed to initialize yaml parser\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\tyaml_parser_set_input_file(&parser_, fh);\n> +\n> +\tyaml_token_t token;\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\tyaml_token_delete(&token);\n> +\t\tyaml_parser_delete(&parser_);\n> +\t\treturn -EINVAL;\n> +\t}\n> +\tyaml_token_delete(&token);\n> +\n> +\tyaml_parser_scan(&parser_, &token);\n> +\tif (token.type != YAML_BLOCK_MAPPING_START_TOKEN) {\n> +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> +\t\tyaml_token_delete(&token);\n> +\t\tyaml_parser_delete(&parser_);\n> +\t\treturn -EINVAL;\n> +\t}\n> +\tyaml_token_delete(&token);\n> +\n> +\t/* Parse the file and parse each single key one by one. */\n> +\tdo {\n> +\t\tyaml_parser_scan(&parser_, &token);\n> +\t\tswitch (token.type) {\n> +\t\tcase YAML_KEY_TOKEN:\n> +\t\t\tyaml_token_delete(&token);\n> +\t\t\tret = parseEntry();\n> +\t\t\tbreak;\n> +\n> +\t\tcase YAML_STREAM_END_TOKEN:\n> +\t\t\tret = -ENOENT;\n> +\t\t\t[[fallthrough]];\n> +\t\tdefault:\n> +\t\t\tyaml_token_delete(&token);\n> +\t\t\tbreak;\n> +\t\t}\n> +\t} while (ret >= 0);\n> +\tyaml_parser_delete(&parser_);\n> +\n> +\tif (ret && ret != -ENOENT)\n> +\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> +\n> +\treturn ret == -ENOENT ? 0 : ret;\n> +}\n> +\n> +std::string CameraHalConfig::findFilePath(const std::string &filename) const\n> +{\n> +\tstatic const std::array searchPaths = {\n> +\t\tLIBCAMERA_SYSCONF_DIR,\n> +\t\tLIBCAMERA_DATA_DIR,\n\nCould you please reply to the comments in v1 or v2 about this ?\n\n> +\t};\n> +\n> +\tif (File::exists(filename))\n> +\t\treturn filename;\n> +\n> +\tstd::filesystem::path root = utils::libcameraSourcePath();\n> +\tif (!root.empty()) {\n> +\t\tstd::string configurationPath = root / \"data\" / filename;\n> +\t\tif (File::exists(configurationPath))\n> +\t\t\treturn configurationPath;\n> +\t}\n> +\n> +\tfor (const std::filesystem::path &path : searchPaths) {\n> +\t\tstd::string configurationPath = path / filename;\n> +\t\tif (File::exists(configurationPath))\n> +\t\t\treturn configurationPath;\n> +\t}\n> +\n> +\treturn \"\";\n> +}\n> +\n> +FILE *CameraHalConfig::openFile(const std::string &filename)\n> +{\n> +\tconst std::string filePath = findFilePath(filename);\n> +\tif (filePath.empty()) {\n> +\t\tLOG(HALConfig, Error)\n> +\t\t\t<< \"Configuration file: \\\"\" << filename << \"\\\" not found\";\n> +\t\treturn nullptr;\n> +\t}\n> +\n> +\tLOG(HALConfig, Debug) << \"Reading configuration file from \" << filePath;\n> +\n> +\tFILE *fh = fopen(filePath.c_str(), \"r\");\n> +\tif (!fh) {\n> +\t\tint ret = -errno;\n> +\t\tLOG(HALConfig, Error) << \"Failed to open configuration file \"\n> +\t\t\t\t << filePath << \": \" << strerror(-ret);\n> +\t\treturn nullptr;\n> +\t}\n> +\n> +\treturn fh;\n> +}\n\nYou could possibly inline this function in CameraHalConfig::open(), up\nto you.\n\nOther than these two comments, this looks good.\n\n> +\n> +CameraHalConfig::CameraHalConfig()\n> +\t: Extensible(new Private(this))\n> +{\n> +}\n> +\n> +/*\n> + * Open the HAL configuration file and validate its content.\n> + * Return 0 on success, a negative error code otherwise\n> + */\n> +int CameraHalConfig::open()\n> +{\n> +\tFILE *fh = openFile(\"camera_hal.yaml\");\n> +\tif (!fh)\n> +\t\treturn -ENOENT;\n> +\n> +\tPrivate *const d = LIBCAMERA_D_PTR();\n> +\tint ret = d->parseConfigFile(fh, &cameras_);\n> +\tfclose(fh);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tfor (const auto &c : cameras_) {\n> +\t\tconst std::string &cameraId = c.first;\n> +\t\tconst CameraProps &camera = c.second;\n> +\t\tLOG(HALConfig, Debug) << \"'\" << cameraId << \"' \"\n> +\t\t\t\t << \"(\" << camera.facing << \")[\"\n> +\t\t\t\t << camera.rotation << \"]\";\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +const CameraProps &CameraHalConfig::cameraProps(const std::string &cameraId) const\n> +{\n> +\tstatic CameraProps empty;\n> +\n> +\tconst auto &it = cameras_.find(cameraId);\n> +\tif (it == cameras_.end()) {\n> +\t\tLOG(HALConfig, Error)\n> +\t\t\t<< \"Camera '\" << cameraId\n> +\t\t\t<< \"' not described in the HAL configuration file\";\n> +\t\treturn empty;\n> +\t}\n> +\n> +\treturn it->second;\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..65569559fbf0\n> --- /dev/null\n> +++ b/src/android/camera_hal_config.h\n> @@ -0,0 +1,38 @@\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> +\n> +#include \n> +\n> +struct CameraProps {\n> +\tint facing;\n> +\tint rotation;\n> +\n> +\tbool valid = false;\n> +};\n> +\n> +class CameraHalConfig final : public libcamera::Extensible\n> +{\n> +\tLIBCAMERA_DECLARE_PRIVATE(CameraBuffer)\n> +\n> +public:\n> +\tCameraHalConfig();\n> +\n> +\tint open();\n> +\tconst CameraProps &cameraProps(const std::string &cameraId) const;\n> +\n> +private:\n> +\tstd::string findFilePath(const std::string &filename) const;\n> +\tFILE *openFile(const std::string &filename);\n> +\n> +\tstd::map cameras_;\n> +};\n> +#endif /* __ANDROID_CAMERA_HAL_CONFIG_H__ */\n> diff --git a/src/android/meson.build b/src/android/meson.build\n> index 2be20c97e118..3893e5b5b832 100644\n> --- a/src/android/meson.build\n> +++ b/src/android/meson.build\n> @@ -3,6 +3,7 @@\n> android_deps = [\n> dependency('libexif', required : get_option('android')),\n> dependency('libjpeg', required : get_option('android')),\n> + dependency('yaml-0.1', required : get_option('android')),\n> ]\n> \n> android_enabled = true\n> @@ -45,6 +46,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 DFC12BD1F6\n\tfor ;\n\tTue, 13 Apr 2021 04:19:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 656CF687FF;\n\tTue, 13 Apr 2021 06:19:38 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E4C07687EC\n\tfor ;\n\tTue, 13 Apr 2021 06:19:36 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5B9756F2;\n\tTue, 13 Apr 2021 06:19:36 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"pPJPukGZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618287576;\n\tbh=fxcjn/JahwGbcQfcn2ENjzhPC41Qo/p/KHogNqMRpRk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pPJPukGZiETq2r8xnP6fv6mc/RIwEiuPovyMl/CDP/NLG6oQv+2edX+30GgJJmlsg\n\tAxkQn0vdcOmwuF/3gEGwL+IPeCB4uIPyOhYqaJUBfQeV7tPLEzseCRlIFghkhs3RVe\n\tmWyb0w+ZhrLTEaaGZofVIeKcKH2U4m3uiUNlNj2E=","Date":"Tue, 13 Apr 2021 07:18:47 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210406154557.27303-1-jacopo@jmondi.org>\n\t<20210406154557.27303-4-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210406154557.27303-4-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v4 3/5] android: Add CameraHalConfig\n\tclass","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16222,"web_url":"https://patchwork.libcamera.org/comment/16222/","msgid":"<20210413075109.kjb76zw55liiepbe@uno.localdomain>","date":"2021-04-13T07:51:09","subject":"Re: [libcamera-devel] [PATCH v4 3/5] android: Add CameraHalConfig\n\tclass","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Tue, Apr 13, 2021 at 07:18:47AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> > +\n> > +std::string CameraHalConfig::findFilePath(const std::string &filename) const\n> > +{\n> > +\tstatic const std::array searchPaths = {\n> > +\t\tLIBCAMERA_SYSCONF_DIR,\n> > +\t\tLIBCAMERA_DATA_DIR,\n>\n> Could you please reply to the comments in v1 or v2 about this ?\n>\n\nSorry missed that. If I got it right it's enough to drop DATA_DIR\n\n> > +\t};\n> > +\n> > +\tif (File::exists(filename))\n> > +\t\treturn filename;\n> > +\n> > +\tstd::filesystem::path root = utils::libcameraSourcePath();\n> > +\tif (!root.empty()) {\n> > +\t\tstd::string configurationPath = root / \"data\" / filename;\n> > +\t\tif (File::exists(configurationPath))\n> > +\t\t\treturn configurationPath;\n> > +\t}\n> > +\n> > +\tfor (const std::filesystem::path &path : searchPaths) {\n> > +\t\tstd::string configurationPath = path / filename;\n> > +\t\tif (File::exists(configurationPath))\n> > +\t\t\treturn configurationPath;\n> > +\t}\n> > +\n> > +\treturn \"\";\n> > +}\n> > +\n> > +FILE *CameraHalConfig::openFile(const std::string &filename)\n> > +{\n> > +\tconst std::string filePath = findFilePath(filename);\n> > +\tif (filePath.empty()) {\n> > +\t\tLOG(HALConfig, Error)\n> > +\t\t\t<< \"Configuration file: \\\"\" << filename << \"\\\" not found\";\n> > +\t\treturn nullptr;\n> > +\t}\n> > +\n> > +\tLOG(HALConfig, Debug) << \"Reading configuration file from \" << filePath;\n> > +\n> > +\tFILE *fh = fopen(filePath.c_str(), \"r\");\n> > +\tif (!fh) {\n> > +\t\tint ret = -errno;\n> > +\t\tLOG(HALConfig, Error) << \"Failed to open configuration file \"\n> > +\t\t\t\t << filePath << \": \" << strerror(-ret);\n> > +\t\treturn nullptr;\n> > +\t}\n> > +\n> > +\treturn fh;\n> > +}\n>\n> You could possibly inline this function in CameraHalConfig::open(), up\n> to you.\n\nI think I've bee asked to break it out in previous iteration, and\nsincerely I like it too, as it centralizes handling the FILE *.\n\n>\n> Other than these two comments, this looks good.\n>\n> > +\n> > +CameraHalConfig::CameraHalConfig()\n> > +\t: Extensible(new Private(this))\n> > +{\n> > +}\n> > +\n> > +/*\n> > + * Open the HAL configuration file and validate its content.\n> > + * Return 0 on success, a negative error code otherwise\n> > + */\n> > +int CameraHalConfig::open()\n> > +{\n> > +\tFILE *fh = openFile(\"camera_hal.yaml\");\n> > +\tif (!fh)\n> > +\t\treturn -ENOENT;\n> > +\n> > +\tPrivate *const d = LIBCAMERA_D_PTR();\n> > +\tint ret = d->parseConfigFile(fh, &cameras_);\n> > +\tfclose(fh);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tfor (const auto &c : cameras_) {\n> > +\t\tconst std::string &cameraId = c.first;\n> > +\t\tconst CameraProps &camera = c.second;\n> > +\t\tLOG(HALConfig, Debug) << \"'\" << cameraId << \"' \"\n> > +\t\t\t\t << \"(\" << camera.facing << \")[\"\n> > +\t\t\t\t << camera.rotation << \"]\";\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +const CameraProps &CameraHalConfig::cameraProps(const std::string &cameraId) const\n> > +{\n> > +\tstatic CameraProps empty;\n> > +\n> > +\tconst auto &it = cameras_.find(cameraId);\n> > +\tif (it == cameras_.end()) {\n> > +\t\tLOG(HALConfig, Error)\n> > +\t\t\t<< \"Camera '\" << cameraId\n> > +\t\t\t<< \"' not described in the HAL configuration file\";\n> > +\t\treturn empty;\n> > +\t}\n> > +\n> > +\treturn it->second;\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..65569559fbf0\n> > --- /dev/null\n> > +++ b/src/android/camera_hal_config.h\n> > @@ -0,0 +1,38 @@\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> > +\n> > +#include \n> > +\n> > +struct CameraProps {\n> > +\tint facing;\n> > +\tint rotation;\n> > +\n> > +\tbool valid = false;\n> > +};\n> > +\n> > +class CameraHalConfig final : public libcamera::Extensible\n> > +{\n> > +\tLIBCAMERA_DECLARE_PRIVATE(CameraBuffer)\n> > +\n> > +public:\n> > +\tCameraHalConfig();\n> > +\n> > +\tint open();\n> > +\tconst CameraProps &cameraProps(const std::string &cameraId) const;\n> > +\n> > +private:\n> > +\tstd::string findFilePath(const std::string &filename) const;\n> > +\tFILE *openFile(const std::string &filename);\n> > +\n> > +\tstd::map cameras_;\n> > +};\n> > +#endif /* __ANDROID_CAMERA_HAL_CONFIG_H__ */\n> > diff --git a/src/android/meson.build b/src/android/meson.build\n> > index 2be20c97e118..3893e5b5b832 100644\n> > --- a/src/android/meson.build\n> > +++ b/src/android/meson.build\n> > @@ -3,6 +3,7 @@\n> > android_deps = [\n> > dependency('libexif', required : get_option('android')),\n> > dependency('libjpeg', required : get_option('android')),\n> > + dependency('yaml-0.1', required : get_option('android')),\n> > ]\n> >\n> > android_enabled = true\n> > @@ -45,6 +46,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 8BE32BD224\n\tfor ;\n\tTue, 13 Apr 2021 07:50:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DE0B16880C;\n\tTue, 13 Apr 2021 09:50:31 +0200 (CEST)","from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A60BA687F3\n\tfor ;\n\tTue, 13 Apr 2021 09:50:30 +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 relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 440CEE0002;\n\tTue, 13 Apr 2021 07:50:30 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Tue, 13 Apr 2021 09:51:09 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210413075109.kjb76zw55liiepbe@uno.localdomain>","References":"<20210406154557.27303-1-jacopo@jmondi.org>\n\t<20210406154557.27303-4-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 3/5] android: Add CameraHalConfig\n\tclass","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]