[{"id":16073,"web_url":"https://patchwork.libcamera.org/comment/16073/","msgid":"","date":"2021-03-31T09:30:34","subject":"Re: [libcamera-devel] [PATCH v3 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 the patch.\n\nOn Tue, Mar 30, 2021 at 11:20 PM 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 | 407 ++++++++++++++++++++++++++++++\n> src/android/camera_hal_config.h | 36 +++\n> src/android/meson.build | 2 +\n> 4 files changed, 446 insertions(+), 1 deletion(-)\n> create mode 100644 src/android/camera_hal_config.cpp\n> create mode 100644 src/android/camera_hal_config.h\n>\n> diff --git a/README.rst b/README.rst\n> index 7a98164bbb0a..f68d435196de 100644\n> --- a/README.rst\n> +++ b/README.rst\n> @@ -88,7 +88,7 @@ for tracing with lttng: [optional]\n> liblttng-ust-dev python3-jinja2 lttng-tools\n>\n> for android: [optional]\n> - libexif libjpeg\n> + libexif libjpeg libyaml\n>\n> Using GStreamer plugin\n> ~~~~~~~~~~~~~~~~~~~~~~\n> diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp\n> new file mode 100644\n> index 000000000000..b109ad24e1d1\n> --- /dev/null\n> +++ b/src/android/camera_hal_config.cpp\n> @@ -0,0 +1,407 @@\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> +\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> +namespace {\n> +\n> +std::map cameras;\n> +\n> +std::string parseValue(yaml_parser_t *parser)\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> + char *scalar = reinterpret_cast(token.data.scalar.value);\n> + std::string value(scalar, token.data.scalar.length);\n> + yaml_token_delete(&token);\n> +\n> + return value;\n> +}\n> +\n> +std::string parseKey(yaml_parser_t *parser)\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> + char *scalar = reinterpret_cast(token.data.scalar.value);\n> + std::string key(scalar, token.data.scalar.length);\n> + yaml_token_delete(&token);\n> +\n> + return key;\n> +}\n> +\n> +int parseValueBlock(yaml_parser_t *parser)\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 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> + cameraProps->facing = -EINVAL;\n> + return -EINVAL;\n> + }\n> +\n> + return 0;\n> +}\n> +\n> +int parseCameraProps(yaml_parser_t *parser, const std::string &cameraID)\n> +{\n> + int ret = parseValueBlock(parser);\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(parser);\n> + std::string value = parseValue(parser);\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> + << \"Unupported location: \"\n> + << value;\n> + return -EINVAL;\n> + }\n> + } else if (key == \"rotation\") {\n> + cameraProps.rotation = strtoul(value.c_str(),\n> + NULL, 10);\n> + if (cameraProps.rotation < 0) {\n> + LOG(HALConfig, Error)\n> + << \"Unknown rotation: \"\n> + << cameraProps.rotation;\n> + return -EINVAL;\n> + }\n> + } else if (key == \"model\") {\n> + cameraProps.model = value;\n> + } else {\n> + LOG(HALConfig, Error)\n> + << \"Unknown key: \" << key;\n> + return -EINVAL;\n> + }\n> + break;\n> + }\n> + case YAML_BLOCK_END_TOKEN:\n> + blockEnd = true;\n> + /* Fall-through */\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 parseCameras(yaml_parser_t *parser)\n> +{\n> + int ret = parseValueBlock(parser);\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(parser);\n> + if (cameraId.empty())\n> + return -EINVAL;\n> +\n> + ret = parseCameraProps(parser, cameraId);\n> + if (ret)\n> + return -EINVAL;\n> + break;\n> + }\n> + case YAML_BLOCK_END_TOKEN:\n> + blockEnd = true;\n> + /* Fall-through */\n> + default:\n> + yaml_token_delete(&token);\n> + break;\n> + }\n> + } while (!blockEnd);\n> +\n> + return 0;\n> +}\n> +\n> +int parseEntry(yaml_parser_t *parser)\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(parser);\n> + if (key.empty())\n> + return ret;\n> +\n> + if (key == \"cameras\") {\n> + ret = parseCameras(parser);\n> + } else\n> + LOG(HALConfig, Error) << \"Unknown key: \" << key;\n> +\n> + return ret;\n> +}\n> +\n> +int parseConfigFile(yaml_parser_t *parser)\n> +{\n> + yaml_token_t token;\n> + int ret = 0;\n> +\n> + yaml_parser_scan(parser, &token);\n> + if (token.type != YAML_STREAM_START_TOKEN) {\n> + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> + 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> + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> + yaml_token_delete(&token);\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(parser);\n> + break;\n> +\n> + case YAML_STREAM_END_TOKEN:\n> + ret = -ENOENT;\n> + /* Fall-through */\n> + default:\n> + yaml_token_delete(&token);\n> + break;\n> + }\n> + } while (ret >= 0);\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> +} /* namespace */\n> +\n> +std::string CameraHalConfig::findFilePath(const std::string &filename) const\n> +{\n> + static std::array searchPaths = {\n> + LIBCAMERA_SYSCONF_DIR,\n> + LIBCAMERA_DATA_DIR,\n> + };\n> +\n> + if (File::exists(filename))\n> + return filename;\n> +\n> + std::string root = utils::libcameraSourcePath();\n> + if (!root.empty()) {\n> + std::string configurationPath = root + \"data/\" + filename;\n> + if (File::exists(configurationPath))\n> + return configurationPath;\n> + }\n> +\n> + for (std::string &path : searchPaths) {\n> + std::string configurationPath = path + \"/\" + filename;\n> + if (File::exists(configurationPath))\n> + return configurationPath;\n> + }\n> +\n> + return \"\";\n> +}\n> +\n> +FILE *CameraHalConfig::openFile(const std::string &filename)\n> +{\n> + const std::string filePath = findFilePath(filename);\n> + if (filePath.empty()) {\n> + LOG(HALConfig, Error)\n> + << \"Configuration file: \\\"\" << filename << \"\\\" not found\";\n> + return nullptr;\n> + }\n> +\n> + LOG(HALConfig, Debug) << \"Reading configuration file from \" << filePath;\n> +\n> + FILE *fh = fopen(filePath.c_str(), \"r\");\n> + if (!fh) {\n> + int ret = -errno;\n> + LOG(HALConfig, Error) << \"Failed to open configuration file \"\n> + << filePath << \": \" << strerror(-ret);\n> + return nullptr;\n> + }\n> +\n> + return fh;\n> +}\n> +\n\nI don't know if we would like to hide like this way.\nWe may want to achieve this by D-pointer like libcamera::CameraManager.\nI at least think cameras should be a member variable of CameraHalConfig.\nI would wait for Laurent's comments.\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> + yaml_parser_t parser;\n> + int ret = yaml_parser_initialize(&parser);\n> + if (!ret) {\n> + LOG(HALConfig, Fatal) << \"Failed to initialize yaml parser\";\n> + return -EINVAL;\n> + }\n> +\n> + FILE *fh = openFile(\"camera_hal.yaml\");\n> + if (!fh)\n> + return -ENOENT;\n> +\n> + yaml_parser_set_input_file(&parser, fh);\n> + ret = parseConfigFile(&parser);\n> + fclose(fh);\n> + yaml_parser_delete(&parser);\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> + << \" model: \" << camera.model\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..5491a12fdffd\n> --- /dev/null\n> +++ b/src/android/camera_hal_config.h\n> @@ -0,0 +1,36 @@\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> +\n> +class CameraProps\n> +{\n> +public:\n> + CameraProps()\n> + : valid(false) {}\n> +\n> + int facing;\n> + std::string model;\n> + unsigned int rotation;\n> +\n> + bool valid;\n> +};\n\nThis doesn't have a function.\nSo I would declare this as struct.\n\nhttps://google.github.io/styleguide/cppguide.html#Structs_vs._Classes\n\nBest Regards,\n-Hiro\n> +\n> +class CameraHalConfig\n> +{\n> +public:\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> +#endif /* __ANDROID_CAMERA_HAL_CONFIG_H__ */\n> +\n> diff --git a/src/android/meson.build b/src/android/meson.build\n> index 8e7d07d9be3c..c0ede407bc38 100644\n> --- a/src/android/meson.build\n> +++ b/src/android/meson.build\n> @@ -3,6 +3,7 @@\n> android_deps = [\n> dependency('libexif', required : get_option('android')),\n> dependency('libjpeg', required : get_option('android')),\n> + dependency('yaml-0.1', required : get_option('android')),\n> ]\n>\n> android_enabled = true\n> @@ -41,6 +42,7 @@ android_hal_sources = files([\n> 'camera3_hal.cpp',\n> 'camera_hal_manager.cpp',\n> 'camera_device.cpp',\n> + 'camera_hal_config.cpp',\n> '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 1A86BC0DA4\n\tfor ;\n\tWed, 31 Mar 2021 09:30:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 61FD668781;\n\tWed, 31 Mar 2021 11:30:46 +0200 (CEST)","from mail-ej1-x631.google.com (mail-ej1-x631.google.com\n\t[IPv6:2a00:1450:4864:20::631])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D4285602D7\n\tfor ;\n\tWed, 31 Mar 2021 11:30:44 +0200 (CEST)","by mail-ej1-x631.google.com with SMTP id u9so29021356ejj.7\n\tfor ;\n\tWed, 31 Mar 2021 02:30:44 -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=\"H4bVrB/N\"; 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=PJkWjOS4lxzqxryukHmOLqnIw2D3VW/WH80MR+slkGE=;\n\tb=H4bVrB/N6E9gW/EVLa4n75A8XjzcvlVYBVYTdQY+LqOw5CTTWfhxgWDWai7yJDMgPJ\n\tTIAsnYfak8NtkSGLC88/47qIjcBlUXVawMOvjHNbjJ9hgyBZu2B+DunrOFHc/3f6cDsI\n\t8ADTSzS47xdYRVey4shPl1GLQ8nVWIvZk61ro=","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=PJkWjOS4lxzqxryukHmOLqnIw2D3VW/WH80MR+slkGE=;\n\tb=dJi8A7u4ImdR5HkzmpQBajgzdfnEJ3KCsoq77ivIbpbymbBMieFeSQgQq2oaPojJzg\n\t5wlIMevuGEQNJRenVyplKjvsbR+tumZtRsR/QPZZA6104Ac2WKX4QLv4kzSl1TPZsSdl\n\th3hGgnweBouzsjHh/os0vdqA5VWQ53N7+tdcEO4YyzV+V0QnTy4stHqXuTT/P60kJkur\n\tmpqm2iWfsod0yAo5dBteI4Hf8ikY62U4+nHzss4Auz/GWhfFVs3mh5yAoHqfUUeGnSTo\n\tFie1ir3Fv+3N3g7AdwVP/hCMLfeGHyDWvq77I5gKrnsmWdFd9y7802nEj4GV8dfPuPMf\n\tjXdA==","X-Gm-Message-State":"AOAM530nfPkQF4M4c3CU1jzGITVcN6r2kWg+oveolaLU7iiZnSsAOm41\n\tpQhyzn94yLgqyQskQWlxRs2indssoq3UUtIGMtiT3v7eFPKA8g==","X-Google-Smtp-Source":"ABdhPJxu/EPhqH8RH+JNARSQM2gVG4k8kHQM8kKaZLUN8Y5AxoXXOofcVDgqPtIvAwSvi7EuxyGvvcMXL0RwaMFhlOs=","X-Received":"by 2002:a17:906:4747:: with SMTP id\n\tj7mr2481183ejs.221.1617183044389; \n\tWed, 31 Mar 2021 02:30:44 -0700 (PDT)","MIME-Version":"1.0","References":"<20210330142113.37457-1-jacopo@jmondi.org>\n\t<20210330142113.37457-4-jacopo@jmondi.org>","In-Reply-To":"<20210330142113.37457-4-jacopo@jmondi.org>","From":"Hirokazu Honda ","Date":"Wed, 31 Mar 2021 18:30:34 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v3 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":16075,"web_url":"https://patchwork.libcamera.org/comment/16075/","msgid":"<20210331112759.bm6c5gxsaf6pqc5u@uno.localdomain>","date":"2021-03-31T11:27:59","subject":"Re: [libcamera-devel] [PATCH v3 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, Mar 31, 2021 at 06:30:34PM +0900, Hirokazu Honda wrote:\n> Hi Jacopo, thanks for the patch.\n>\n> On Tue, Mar 30, 2021 at 11:20 PM 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 | 407 ++++++++++++++++++++++++++++++\n> > src/android/camera_hal_config.h | 36 +++\n> > src/android/meson.build | 2 +\n> > 4 files changed, 446 insertions(+), 1 deletion(-)\n> > create mode 100644 src/android/camera_hal_config.cpp\n> > create mode 100644 src/android/camera_hal_config.h\n> >\n> > diff --git a/README.rst b/README.rst\n> > index 7a98164bbb0a..f68d435196de 100644\n> > --- a/README.rst\n> > +++ b/README.rst\n> > @@ -88,7 +88,7 @@ for tracing with lttng: [optional]\n> > liblttng-ust-dev python3-jinja2 lttng-tools\n> >\n> > for android: [optional]\n> > - libexif libjpeg\n> > + libexif libjpeg libyaml\n> >\n> > Using GStreamer plugin\n> > ~~~~~~~~~~~~~~~~~~~~~~\n> > diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp\n> > new file mode 100644\n> > index 000000000000..b109ad24e1d1\n> > --- /dev/null\n> > +++ b/src/android/camera_hal_config.cpp\n> > @@ -0,0 +1,407 @@\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> > +\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> > +namespace {\n> > +\n> > +std::map cameras;\n> > +\n> > +std::string parseValue(yaml_parser_t *parser)\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> > + char *scalar = reinterpret_cast(token.data.scalar.value);\n> > + std::string value(scalar, token.data.scalar.length);\n> > + yaml_token_delete(&token);\n> > +\n> > + return value;\n> > +}\n> > +\n> > +std::string parseKey(yaml_parser_t *parser)\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> > + char *scalar = reinterpret_cast(token.data.scalar.value);\n> > + std::string key(scalar, token.data.scalar.length);\n> > + yaml_token_delete(&token);\n> > +\n> > + return key;\n> > +}\n> > +\n> > +int parseValueBlock(yaml_parser_t *parser)\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 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> > + cameraProps->facing = -EINVAL;\n> > + return -EINVAL;\n> > + }\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +int parseCameraProps(yaml_parser_t *parser, const std::string &cameraID)\n> > +{\n> > + int ret = parseValueBlock(parser);\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(parser);\n> > + std::string value = parseValue(parser);\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> > + << \"Unupported location: \"\n> > + << value;\n> > + return -EINVAL;\n> > + }\n> > + } else if (key == \"rotation\") {\n> > + cameraProps.rotation = strtoul(value.c_str(),\n> > + NULL, 10);\n> > + if (cameraProps.rotation < 0) {\n> > + LOG(HALConfig, Error)\n> > + << \"Unknown rotation: \"\n> > + << cameraProps.rotation;\n> > + return -EINVAL;\n> > + }\n> > + } else if (key == \"model\") {\n> > + cameraProps.model = value;\n> > + } else {\n> > + LOG(HALConfig, Error)\n> > + << \"Unknown key: \" << key;\n> > + return -EINVAL;\n> > + }\n> > + break;\n> > + }\n> > + case YAML_BLOCK_END_TOKEN:\n> > + blockEnd = true;\n> > + /* Fall-through */\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 parseCameras(yaml_parser_t *parser)\n> > +{\n> > + int ret = parseValueBlock(parser);\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(parser);\n> > + if (cameraId.empty())\n> > + return -EINVAL;\n> > +\n> > + ret = parseCameraProps(parser, cameraId);\n> > + if (ret)\n> > + return -EINVAL;\n> > + break;\n> > + }\n> > + case YAML_BLOCK_END_TOKEN:\n> > + blockEnd = true;\n> > + /* Fall-through */\n> > + default:\n> > + yaml_token_delete(&token);\n> > + break;\n> > + }\n> > + } while (!blockEnd);\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +int parseEntry(yaml_parser_t *parser)\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(parser);\n> > + if (key.empty())\n> > + return ret;\n> > +\n> > + if (key == \"cameras\") {\n> > + ret = parseCameras(parser);\n> > + } else\n> > + LOG(HALConfig, Error) << \"Unknown key: \" << key;\n> > +\n> > + return ret;\n> > +}\n> > +\n> > +int parseConfigFile(yaml_parser_t *parser)\n> > +{\n> > + yaml_token_t token;\n> > + int ret = 0;\n> > +\n> > + yaml_parser_scan(parser, &token);\n> > + if (token.type != YAML_STREAM_START_TOKEN) {\n> > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > + 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> > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > + yaml_token_delete(&token);\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(parser);\n> > + break;\n> > +\n> > + case YAML_STREAM_END_TOKEN:\n> > + ret = -ENOENT;\n> > + /* Fall-through */\n> > + default:\n> > + yaml_token_delete(&token);\n> > + break;\n> > + }\n> > + } while (ret >= 0);\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> > +} /* namespace */\n> > +\n> > +std::string CameraHalConfig::findFilePath(const std::string &filename) const\n> > +{\n> > + static std::array searchPaths = {\n> > + LIBCAMERA_SYSCONF_DIR,\n> > + LIBCAMERA_DATA_DIR,\n> > + };\n> > +\n> > + if (File::exists(filename))\n> > + return filename;\n> > +\n> > + std::string root = utils::libcameraSourcePath();\n> > + if (!root.empty()) {\n> > + std::string configurationPath = root + \"data/\" + filename;\n> > + if (File::exists(configurationPath))\n> > + return configurationPath;\n> > + }\n> > +\n> > + for (std::string &path : searchPaths) {\n> > + std::string configurationPath = path + \"/\" + filename;\n> > + if (File::exists(configurationPath))\n> > + return configurationPath;\n> > + }\n> > +\n> > + return \"\";\n> > +}\n> > +\n> > +FILE *CameraHalConfig::openFile(const std::string &filename)\n> > +{\n> > + const std::string filePath = findFilePath(filename);\n> > + if (filePath.empty()) {\n> > + LOG(HALConfig, Error)\n> > + << \"Configuration file: \\\"\" << filename << \"\\\" not found\";\n> > + return nullptr;\n> > + }\n> > +\n> > + LOG(HALConfig, Debug) << \"Reading configuration file from \" << filePath;\n> > +\n> > + FILE *fh = fopen(filePath.c_str(), \"r\");\n> > + if (!fh) {\n> > + int ret = -errno;\n> > + LOG(HALConfig, Error) << \"Failed to open configuration file \"\n> > + << filePath << \": \" << strerror(-ret);\n> > + return nullptr;\n> > + }\n> > +\n> > + return fh;\n> > +}\n> > +\n>\n> I don't know if we would like to hide like this way.\n> We may want to achieve this by D-pointer like libcamera::CameraManager.\n> I at least think cameras should be a member variable of CameraHalConfig.\n> I would wait for Laurent's comments.\n>\n\nYes, that might not be the most elegant solution. I went for static\nfunctions as otherwise I would have the need to declare private\nfunctions as part of the class, and they all take a yaml_parser_t\nargument, so that would have required including \"yaml.h\" in the header\nfile. Maybe I could have get away just forward declaring it ?\n\nI considered other patterns like PIMPL, but I also considered that:\n1) Parsing is done just once at HAL initialization time, having static\nfields like 'cameras' does not require keeping track of their state\n(ie. cleaning them before a new parsing, as it only happens once)\n2) We want to avoid exposing \"yaml.h\" to the rest of the HAL, but\nafaict we do not plan to change the parsing backend, which kind of\ndefeat the purpose of pimpl which guarantees a stable interface\ndespite the backend implementation\n3) the CameraHalConfig API is internal to the HAL and no external\ncomponent depend on it\n\nAlthough having the parsing functions as class members would allow\nme to make CameraProps a proper class with the internal parser class\ndeclared as friend, so that class members fields can be made private with\nproper accessor functions instead of having them public (as I currently need to set\nthem from static functions context).\n\nAll in all yes, this can be made more elegant, but I wonder if that\nreally required given the current use case.\n\nThanks\n j\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> > + yaml_parser_t parser;\n> > + int ret = yaml_parser_initialize(&parser);\n> > + if (!ret) {\n> > + LOG(HALConfig, Fatal) << \"Failed to initialize yaml parser\";\n> > + return -EINVAL;\n> > + }\n> > +\n> > + FILE *fh = openFile(\"camera_hal.yaml\");\n> > + if (!fh)\n> > + return -ENOENT;\n> > +\n> > + yaml_parser_set_input_file(&parser, fh);\n> > + ret = parseConfigFile(&parser);\n> > + fclose(fh);\n> > + yaml_parser_delete(&parser);\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> > + << \" model: \" << camera.model\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..5491a12fdffd\n> > --- /dev/null\n> > +++ b/src/android/camera_hal_config.h\n> > @@ -0,0 +1,36 @@\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> > +\n> > +class CameraProps\n> > +{\n> > +public:\n> > + CameraProps()\n> > + : valid(false) {}\n> > +\n> > + int facing;\n> > + std::string model;\n> > + unsigned int rotation;\n> > +\n> > + bool valid;\n> > +};\n>\n> This doesn't have a function.\n> So I would declare this as struct.\n>\n> https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes\n>\n> Best Regards,\n> -Hiro\n> > +\n> > +class CameraHalConfig\n> > +{\n> > +public:\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> > +#endif /* __ANDROID_CAMERA_HAL_CONFIG_H__ */\n> > +\n> > diff --git a/src/android/meson.build b/src/android/meson.build\n> > index 8e7d07d9be3c..c0ede407bc38 100644\n> > --- a/src/android/meson.build\n> > +++ b/src/android/meson.build\n> > @@ -3,6 +3,7 @@\n> > android_deps = [\n> > dependency('libexif', required : get_option('android')),\n> > dependency('libjpeg', required : get_option('android')),\n> > + dependency('yaml-0.1', required : get_option('android')),\n> > ]\n> >\n> > android_enabled = true\n> > @@ -41,6 +42,7 @@ android_hal_sources = files([\n> > 'camera3_hal.cpp',\n> > 'camera_hal_manager.cpp',\n> > 'camera_device.cpp',\n> > + 'camera_hal_config.cpp',\n> > '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 82086C0DA4\n\tfor ;\n\tWed, 31 Mar 2021 11:27:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0FA8568781;\n\tWed, 31 Mar 2021 13:27:33 +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 D87906051B\n\tfor ;\n\tWed, 31 Mar 2021 13:27:31 +0200 (CEST)","from uno.localdomain (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 2B26AE0011;\n\tWed, 31 Mar 2021 11:27:30 +0000 (UTC)"],"X-Originating-IP":"93.61.96.190","Date":"Wed, 31 Mar 2021 13:27:59 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210331112759.bm6c5gxsaf6pqc5u@uno.localdomain>","References":"<20210330142113.37457-1-jacopo@jmondi.org>\n\t<20210330142113.37457-4-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 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":16078,"web_url":"https://patchwork.libcamera.org/comment/16078/","msgid":"","date":"2021-04-01T04:48:25","subject":"Re: [libcamera-devel] [PATCH v3 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, Mar 31, 2021 at 8:27 PM Jacopo Mondi wrote:\n>\n> Hi Hiro,\n>\n> On Wed, Mar 31, 2021 at 06:30:34PM +0900, Hirokazu Honda wrote:\n> > Hi Jacopo, thanks for the patch.\n> >\n> > On Tue, Mar 30, 2021 at 11:20 PM 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 | 407 ++++++++++++++++++++++++++++++\n> > > src/android/camera_hal_config.h | 36 +++\n> > > src/android/meson.build | 2 +\n> > > 4 files changed, 446 insertions(+), 1 deletion(-)\n> > > create mode 100644 src/android/camera_hal_config.cpp\n> > > create mode 100644 src/android/camera_hal_config.h\n> > >\n> > > diff --git a/README.rst b/README.rst\n> > > index 7a98164bbb0a..f68d435196de 100644\n> > > --- a/README.rst\n> > > +++ b/README.rst\n> > > @@ -88,7 +88,7 @@ for tracing with lttng: [optional]\n> > > liblttng-ust-dev python3-jinja2 lttng-tools\n> > >\n> > > for android: [optional]\n> > > - libexif libjpeg\n> > > + libexif libjpeg libyaml\n> > >\n> > > Using GStreamer plugin\n> > > ~~~~~~~~~~~~~~~~~~~~~~\n> > > diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp\n> > > new file mode 100644\n> > > index 000000000000..b109ad24e1d1\n> > > --- /dev/null\n> > > +++ b/src/android/camera_hal_config.cpp\n> > > @@ -0,0 +1,407 @@\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> > > +\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> > > +namespace {\n> > > +\n> > > +std::map cameras;\n> > > +\n> > > +std::string parseValue(yaml_parser_t *parser)\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> > > + char *scalar = reinterpret_cast(token.data.scalar.value);\n> > > + std::string value(scalar, token.data.scalar.length);\n> > > + yaml_token_delete(&token);\n> > > +\n> > > + return value;\n> > > +}\n> > > +\n> > > +std::string parseKey(yaml_parser_t *parser)\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> > > + char *scalar = reinterpret_cast(token.data.scalar.value);\n> > > + std::string key(scalar, token.data.scalar.length);\n> > > + yaml_token_delete(&token);\n> > > +\n> > > + return key;\n> > > +}\n> > > +\n> > > +int parseValueBlock(yaml_parser_t *parser)\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 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> > > + cameraProps->facing = -EINVAL;\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + return 0;\n> > > +}\n> > > +\n> > > +int parseCameraProps(yaml_parser_t *parser, const std::string &cameraID)\n> > > +{\n> > > + int ret = parseValueBlock(parser);\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(parser);\n> > > + std::string value = parseValue(parser);\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> > > + << \"Unupported location: \"\n> > > + << value;\n> > > + return -EINVAL;\n> > > + }\n> > > + } else if (key == \"rotation\") {\n> > > + cameraProps.rotation = strtoul(value.c_str(),\n> > > + NULL, 10);\n> > > + if (cameraProps.rotation < 0) {\n> > > + LOG(HALConfig, Error)\n> > > + << \"Unknown rotation: \"\n> > > + << cameraProps.rotation;\n> > > + return -EINVAL;\n\nnit: std::stoul(value) in c++ way.\n\nThis seems to be strange, strtoul() returns *unsigned* integer and the\nif-clause check if it is negative.\nPerhaps, you mean stol() [c++] or strtol() [c]?\n\n> > > + }\n> > > + } else if (key == \"model\") {\n> > > + cameraProps.model = value;\n> > > + } else {\n> > > + LOG(HALConfig, Error)\n> > > + << \"Unknown key: \" << key;\n> > > + return -EINVAL;\n> > > + }\n> > > + break;\n> > > + }\n> > > + case YAML_BLOCK_END_TOKEN:\n> > > + blockEnd = true;\n> > > + /* Fall-through */\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 parseCameras(yaml_parser_t *parser)\n> > > +{\n> > > + int ret = parseValueBlock(parser);\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(parser);\n> > > + if (cameraId.empty())\n> > > + return -EINVAL;\n> > > +\n> > > + ret = parseCameraProps(parser, cameraId);\n> > > + if (ret)\n> > > + return -EINVAL;\n> > > + break;\n> > > + }\n> > > + case YAML_BLOCK_END_TOKEN:\n> > > + blockEnd = true;\n> > > + /* Fall-through */\n> > > + default:\n> > > + yaml_token_delete(&token);\n> > > + break;\n> > > + }\n> > > + } while (!blockEnd);\n> > > +\n> > > + return 0;\n> > > +}\n> > > +\n> > > +int parseEntry(yaml_parser_t *parser)\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(parser);\n> > > + if (key.empty())\n> > > + return ret;\n> > > +\n> > > + if (key == \"cameras\") {\n> > > + ret = parseCameras(parser);\n> > > + } else\n> > > + LOG(HALConfig, Error) << \"Unknown key: \" << key;\n> > > +\n\nYou may want to remove parenthesis.\n\n> > > + return ret;\n> > > +}\n> > > +\n> > > +int parseConfigFile(yaml_parser_t *parser)\n> > > +{\n> > > + yaml_token_t token;\n> > > + int ret = 0;\n> > > +\n> > > + yaml_parser_scan(parser, &token);\n> > > + if (token.type != YAML_STREAM_START_TOKEN) {\n> > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > + 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> > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > + yaml_token_delete(&token);\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(parser);\n> > > + break;\n> > > +\n> > > + case YAML_STREAM_END_TOKEN:\n> > > + ret = -ENOENT;\n> > > + /* Fall-through */\n> > > + default:\n> > > + yaml_token_delete(&token);\n> > > + break;\n> > > + }\n> > > + } while (ret >= 0);\n> > > +\n> > > + if (ret && ret != -ENOENT)\n> > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > +\n> > > + return ret == -ENOENT ? 0 : ret;\n\nHow is the following code?\n int ret = 0;\n while (token.type != YAML_STREAM_END_TOKEN) {\n yaml_token_delete(&token);\n if (token.type == YAML_KEY_TOKEN) {\n ret = parseEntry(parser);\n if (!ret)\n break;\n }\n }\n yaml_token_delete(&token);\n\n if (ret)\n LOG(HALConfig, Error) << \"Configuration file is not valid\";\n\n return ret;\n\n> > > +}\n> > > +\n> > > +} /* namespace */\n> > > +\n> > > +std::string CameraHalConfig::findFilePath(const std::string &filename) const\n> > > +{\n> > > + static std::array searchPaths = {\n> > > + LIBCAMERA_SYSCONF_DIR,\n> > > + LIBCAMERA_DATA_DIR,\n> > > + };\n> > > +\n\nIs this static beneficial? I think not much performance is gained by\nthis, and findFilePath will likely be executed once.\n\n> > > + if (File::exists(filename))\n> > > + return filename;\n> > > +\n> > > + std::string root = utils::libcameraSourcePath();\n> > > + if (!root.empty()) {\n> > > + std::string configurationPath = root + \"data/\" + filename;\n> > > + if (File::exists(configurationPath))\n> > > + return configurationPath;\n> > > + }\n> > > +\n> > > + for (std::string &path : searchPaths) {\n> > > + std::string configurationPath = path + \"/\" + filename;\n> > > + if (File::exists(configurationPath))\n> > > + return configurationPath;\n> > > + }\n> > > +\n\nJust in case, is it guaranteed path and root doesn't end with \"/\"?\nI am a bit surprised we don't have a class to deal with filepath in\nlibcamera like chromium base::FilePath.\nhttps://source.chromium.org/chromium/chromium/src/+/master:base/files/file_path.h\n\nBy the way, since C++17, C++ has a filesystem library as a standard\nlibrary, we may want to use it.\n\n\n> > > + return \"\";\n> > > +}\n> > > +\n> > > +FILE *CameraHalConfig::openFile(const std::string &filename)\n> > > +{\n> > > + const std::string filePath = findFilePath(filename);\n> > > + if (filePath.empty()) {\n> > > + LOG(HALConfig, Error)\n> > > + << \"Configuration file: \\\"\" << filename << \"\\\" not found\";\n> > > + return nullptr;\n> > > + }\n> > > +\n> > > + LOG(HALConfig, Debug) << \"Reading configuration file from \" << filePath;\n> > > +\n> > > + FILE *fh = fopen(filePath.c_str(), \"r\");\n> > > + if (!fh) {\n> > > + int ret = -errno;\n> > > + LOG(HALConfig, Error) << \"Failed to open configuration file \"\n> > > + << filePath << \": \" << strerror(-ret);\n> > > + return nullptr;\n> > > + }\n> > > +\n> > > + return fh;\n> > > +}\n> > > +\n> >\n> > I don't know if we would like to hide like this way.\n> > We may want to achieve this by D-pointer like libcamera::CameraManager.\n> > I at least think cameras should be a member variable of CameraHalConfig.\n> > I would wait for Laurent's comments.\n> >\n>\n> Yes, that might not be the most elegant solution. I went for static\n> functions as otherwise I would have the need to declare private\n> functions as part of the class, and they all take a yaml_parser_t\n> argument, so that would have required including \"yaml.h\" in the header\n> file. Maybe I could have get away just forward declaring it ?\n>\n> I considered other patterns like PIMPL, but I also considered that:\n> 1) Parsing is done just once at HAL initialization time, having static\n> fields like 'cameras' does not require keeping track of their state\n> (ie. cleaning them before a new parsing, as it only happens once)\n> 2) We want to avoid exposing \"yaml.h\" to the rest of the HAL, but\n> afaict we do not plan to change the parsing backend, which kind of\n> defeat the purpose of pimpl which guarantees a stable interface\n> despite the backend implementation\n> 3) the CameraHalConfig API is internal to the HAL and no external\n> component depend on it\n>\n> Although having the parsing functions as class members would allow\n> me to make CameraProps a proper class with the internal parser class\n> declared as friend, so that class members fields can be made private with\n> proper accessor functions instead of having them public (as I currently need to set\n> them from static functions context).\n>\n> All in all yes, this can be made more elegant, but I wonder if that\n> really required given the current use case.\n>\n\nI would wait for Laurent's comments, while I think the current\nimplementation is not so bad as we can go this direction.\nOne point I would like to change is to make camera a member variable\nof CameraHalConfig.\nIf it is static, it should not be initialized more than once, but\nCameraHalConfig itself doesn't avoid it.\nBesides, CameraHalConfig is a member variable of CameraHalManager,\nwhich is static.\nWe can let it be a member variable of CameraHalConfig by passing\ncamera pointer or reference to parseCameraProps() through some\nfunctions out of CameraHalConfig.\n\n> Thanks\n> j\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> > > + yaml_parser_t parser;\n> > > + int ret = yaml_parser_initialize(&parser);\n> > > + if (!ret) {\n> > > + LOG(HALConfig, Fatal) << \"Failed to initialize yaml parser\";\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + FILE *fh = openFile(\"camera_hal.yaml\");\n> > > + if (!fh)\n> > > + return -ENOENT;\n> > > +\n> > > + yaml_parser_set_input_file(&parser, fh);\n> > > + ret = parseConfigFile(&parser);\n> > > + fclose(fh);\n> > > + yaml_parser_delete(&parser);\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> > > + << \" model: \" << camera.model\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..5491a12fdffd\n> > > --- /dev/null\n> > > +++ b/src/android/camera_hal_config.h\n> > > @@ -0,0 +1,36 @@\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> > > +\n> > > +class CameraProps\n> > > +{\n> > > +public:\n> > > + CameraProps()\n> > > + : valid(false) {}\n> > > +\n> > > + int facing;\n> > > + std::string model;\n> > > + unsigned int rotation;\n> > > +\n> > > + bool valid;\n> > > +};\n> >\n> > This doesn't have a function.\n> > So I would declare this as struct.\n> >\n> > https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes\n> >\n> > Best Regards,\n> > -Hiro\n> > > +\n> > > +class CameraHalConfig\n> > > +{\n> > > +public:\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> > > +#endif /* __ANDROID_CAMERA_HAL_CONFIG_H__ */\n> > > +\n> > > diff --git a/src/android/meson.build b/src/android/meson.build\n> > > index 8e7d07d9be3c..c0ede407bc38 100644\n> > > --- a/src/android/meson.build\n> > > +++ b/src/android/meson.build\n> > > @@ -3,6 +3,7 @@\n> > > android_deps = [\n> > > dependency('libexif', required : get_option('android')),\n> > > dependency('libjpeg', required : get_option('android')),\n> > > + dependency('yaml-0.1', required : get_option('android')),\n> > > ]\n> > >\n> > > android_enabled = true\n> > > @@ -41,6 +42,7 @@ android_hal_sources = files([\n> > > 'camera3_hal.cpp',\n> > > 'camera_hal_manager.cpp',\n> > > 'camera_device.cpp',\n> > > + 'camera_hal_config.cpp',\n> > > 'camera_metadata.cpp',\n> > > 'camera_ops.cpp',\n> > > 'camera_stream.cpp',\n> > > --\n> > > 2.30.0\n> > >\n\nWe may want to add a unit test for CameraHalConfig although it is fine\nin another patch series.\n\nBest Regards,\n-Hiro\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 07520C0DA4\n\tfor ;\n\tThu, 1 Apr 2021 04:48:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5364E6877D;\n\tThu, 1 Apr 2021 06:48:35 +0200 (CEST)","from mail-ed1-x52a.google.com (mail-ed1-x52a.google.com\n\t[IPv6:2a00:1450:4864:20::52a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D071A602D2\n\tfor ;\n\tThu, 1 Apr 2021 06:48:33 +0200 (CEST)","by mail-ed1-x52a.google.com with SMTP id bx7so406751edb.12\n\tfor ;\n\tWed, 31 Mar 2021 21:48:33 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"dg6+bx4t\"; 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=pu9TvQu9z8ZAiXjSq6hQEMkk/b17+M/PZDpJDwshNmI=;\n\tb=dg6+bx4tG1rx1q1VvA3gQ1wfSOvLYI3mQlAO4Qo2VMfMWxfFylbJkzWVcrno66ZB2Z\n\tx4Z76ehdgJCqFaS6cN+WzNeqa2Xkmf4XL0sph/aTa+evrM1cnVS3cQHTNPUrLP+QEdRT\n\tyifX3zqlKUNWvIXP7862bKevd+hBVgrydA77Y=","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=pu9TvQu9z8ZAiXjSq6hQEMkk/b17+M/PZDpJDwshNmI=;\n\tb=Tpu0QR7hvuJSCAVpBaPvKihaN1f4boYJkd+E1wNAInZYgzOXJbNg0iWPipdTcmpnnS\n\tnA19l8TyqM2n74qka/XL6wZ1Bxojj2rCPbW/O0Y2ALByNuIUGRDeqX0ekzw3WE6Md5H8\n\t0tmi4D8V57h4vfm5Kor6IloGTW8zRNNRIEhx6jibGYqTNhEKQOQGjy1YqGOXwv/2KNOd\n\tfVZQxf7WSP0BK+X/2ldlRHhl0MsbCJMg5BR1fOIMW+tqRdPcrwwwcF3GpTExiGWO7by5\n\tqQSjDsMaC0KLP0a1VRvtYX/PPKohlS/J99QGArN4IuMnPukHwU3J1TMlABTHZbhU1+fW\n\t+nkQ==","X-Gm-Message-State":"AOAM531MuT6FEEqkfxF74ycjV6pcEiOKbs9XpovQIo2w08P73gdVnB4R\n\t0PwmHwP26pJoPIgsQUPHheKVRjTJEVXWpqraxbs4+g==","X-Google-Smtp-Source":"ABdhPJyvaqw/kuDaLLfkr/W9c/z/wd/pldpixdxk2poLhlpxRachcAtbA6/E4UnFWwtGqpgAu3eTZMphCWV0f2Ntzd8=","X-Received":"by 2002:aa7:d4cb:: with SMTP id\n\tt11mr7641589edr.202.1617252513185; \n\tWed, 31 Mar 2021 21:48:33 -0700 (PDT)","MIME-Version":"1.0","References":"<20210330142113.37457-1-jacopo@jmondi.org>\n\t<20210330142113.37457-4-jacopo@jmondi.org>\n\t\n\t<20210331112759.bm6c5gxsaf6pqc5u@uno.localdomain>","In-Reply-To":"<20210331112759.bm6c5gxsaf6pqc5u@uno.localdomain>","From":"Hirokazu Honda ","Date":"Thu, 1 Apr 2021 13:48:25 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v3 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":16109,"web_url":"https://patchwork.libcamera.org/comment/16109/","msgid":"","date":"2021-04-03T03:51:11","subject":"Re: [libcamera-devel] [PATCH v3 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. I'm happy to see that the configuration file is\njust around the corner :-)\n\nOn Thu, Apr 01, 2021 at 01:48:25PM +0900, Hirokazu Honda wrote:\n> On Wed, Mar 31, 2021 at 8:27 PM Jacopo Mondi wrote:\n> > On Wed, Mar 31, 2021 at 06:30:34PM +0900, Hirokazu Honda wrote:\n> > > On Tue, Mar 30, 2021 at 11:20 PM 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 | 407 ++++++++++++++++++++++++++++++\n> > > > src/android/camera_hal_config.h | 36 +++\n> > > > src/android/meson.build | 2 +\n> > > > 4 files changed, 446 insertions(+), 1 deletion(-)\n> > > > create mode 100644 src/android/camera_hal_config.cpp\n> > > > create mode 100644 src/android/camera_hal_config.h\n> > > >\n> > > > diff --git a/README.rst b/README.rst\n> > > > index 7a98164bbb0a..f68d435196de 100644\n> > > > --- a/README.rst\n> > > > +++ b/README.rst\n> > > > @@ -88,7 +88,7 @@ for tracing with lttng: [optional]\n> > > > liblttng-ust-dev python3-jinja2 lttng-tools\n> > > >\n> > > > for android: [optional]\n> > > > - libexif libjpeg\n> > > > + libexif libjpeg libyaml\n> > > >\n> > > > Using GStreamer plugin\n> > > > ~~~~~~~~~~~~~~~~~~~~~~\n> > > > diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp\n> > > > new file mode 100644\n> > > > index 000000000000..b109ad24e1d1\n> > > > --- /dev/null\n> > > > +++ b/src/android/camera_hal_config.cpp\n> > > > @@ -0,0 +1,407 @@\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> > > > +\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> > > > +namespace {\n> > > > +\n> > > > +std::map cameras;\n> > > > +\n> > > > +std::string parseValue(yaml_parser_t *parser)\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> > > > + char *scalar = reinterpret_cast(token.data.scalar.value);\n> > > > + std::string value(scalar, token.data.scalar.length);\n\nYou could also write this\n\n\tstd::string value(reinterpret_cast(token.data.scalar.value),\n\t\t\t token.data.scalar.length);\n\nSame below.\n\n> > > > + yaml_token_delete(&token);\n> > > > +\n> > > > + return value;\n> > > > +}\n> > > > +\n> > > > +std::string parseKey(yaml_parser_t *parser)\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> > > > + char *scalar = reinterpret_cast(token.data.scalar.value);\n> > > > + std::string key(scalar, token.data.scalar.length);\n> > > > + yaml_token_delete(&token);\n> > > > +\n> > > > + return key;\n> > > > +}\n> > > > +\n> > > > +int parseValueBlock(yaml_parser_t *parser)\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 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> > > > + cameraProps->facing = -EINVAL;\n> > > > + return -EINVAL;\n> > > > + }\n> > > > +\n> > > > + return 0;\n> > > > +}\n> > > > +\n> > > > +int parseCameraProps(yaml_parser_t *parser, const std::string &cameraID)\n\ns/cameraID/cameraId/ (and everywhere below)\n\n> > > > +{\n> > > > + int ret = parseValueBlock(parser);\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\nNo need for {}.\n\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(parser);\n> > > > + std::string value = parseValue(parser);\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> > > > + << \"Unupported location: \"\n\ns/Unupported/Unsupported/\n\n> > > > + << value;\n> > > > + return -EINVAL;\n> > > > + }\n> > > > + } else if (key == \"rotation\") {\n> > > > + cameraProps.rotation = strtoul(value.c_str(),\n> > > > + NULL, 10);\n> > > > + if (cameraProps.rotation < 0) {\n> > > > + LOG(HALConfig, Error)\n> > > > + << \"Unknown rotation: \"\n\nNitpicking, I'd use \"unsupported\" like above, or replace the above with\n\"unknown\".\n\n> > > > + << cameraProps.rotation;\n> > > > + return -EINVAL;\n> \n> nit: std::stoul(value) in c++ way.\n> \n> This seems to be strange, strtoul() returns *unsigned* integer and the\n> if-clause check if it is negative.\n> Perhaps, you mean stol() [c++] or strtol() [c]?\n> \n> > > > + }\n> > > > + } else if (key == \"model\") {\n> > > > + cameraProps.model = value;\n> > > > + } else {\n> > > > + LOG(HALConfig, Error)\n> > > > + << \"Unknown key: \" << key;\n> > > > + return -EINVAL;\n> > > > + }\n> > > > + break;\n> > > > + }\n> > > > + case YAML_BLOCK_END_TOKEN:\n> > > > + blockEnd = true;\n> > > > + /* Fall-through */\n\n[[fallthrough]] in C++17.\n\n> > > > + default:\n> > > > + yaml_token_delete(&token);\n> > > > + break;\n> > > > + }\n\nHave you tested this with comments in the YAML file ? One of the main\nreasons we picked YAML over JSON is for the ability to add comments :-)\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 parseCameras(yaml_parser_t *parser)\n> > > > +{\n> > > > + int ret = parseValueBlock(parser);\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(parser);\n> > > > + if (cameraId.empty())\n> > > > + return -EINVAL;\n> > > > +\n> > > > + ret = parseCameraProps(parser, cameraId);\n> > > > + if (ret)\n> > > > + return -EINVAL;\n> > > > + break;\n> > > > + }\n> > > > + case YAML_BLOCK_END_TOKEN:\n> > > > + blockEnd = true;\n> > > > + /* Fall-through */\n> > > > + default:\n> > > > + yaml_token_delete(&token);\n> > > > + break;\n> > > > + }\n> > > > + } while (!blockEnd);\n> > > > +\n> > > > + return 0;\n> > > > +}\n> > > > +\n> > > > +int parseEntry(yaml_parser_t *parser)\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(parser);\n> > > > + if (key.empty())\n> > > > + return ret;\n> > > > +\n> > > > + if (key == \"cameras\") {\n> > > > + ret = parseCameras(parser);\n> > > > + } else\n> > > > + LOG(HALConfig, Error) << \"Unknown key: \" << key;\n> > > > +\n> \n> You may want to remove parenthesis.\n> \n> > > > + return ret;\n> > > > +}\n> > > > +\n> > > > +int parseConfigFile(yaml_parser_t *parser)\n> > > > +{\n> > > > + yaml_token_t token;\n> > > > + int ret = 0;\n> > > > +\n> > > > + yaml_parser_scan(parser, &token);\n> > > > + if (token.type != YAML_STREAM_START_TOKEN) {\n> > > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > > + 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> > > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > > + yaml_token_delete(&token);\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(parser);\n> > > > + break;\n> > > > +\n> > > > + case YAML_STREAM_END_TOKEN:\n> > > > + ret = -ENOENT;\n> > > > + /* Fall-through */\n> > > > + default:\n> > > > + yaml_token_delete(&token);\n> > > > + break;\n> > > > + }\n> > > > + } while (ret >= 0);\n> > > > +\n> > > > + if (ret && ret != -ENOENT)\n> > > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > > +\n> > > > + return ret == -ENOENT ? 0 : ret;\n> \n> How is the following code?\n> int ret = 0;\n> while (token.type != YAML_STREAM_END_TOKEN) {\n> yaml_token_delete(&token);\n> if (token.type == YAML_KEY_TOKEN) {\n\nThis won't work as yaml_token_delete() memset()s the token.\n\n> ret = parseEntry(parser);\n> if (!ret)\n> break;\n> }\n> }\n> yaml_token_delete(&token);\n> \n> if (ret)\n> LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> \n> return ret;\n> \n> > > > +}\n> > > > +\n> > > > +} /* namespace */\n> > > > +\n> > > > +std::string CameraHalConfig::findFilePath(const std::string &filename) const\n> > > > +{\n> > > > + static std::array searchPaths = {\n\nstatic const\n\n> > > > + LIBCAMERA_SYSCONF_DIR,\n> > > > + LIBCAMERA_DATA_DIR,\n> > > > + };\n> > > > +\n> \n> Is this static beneficial? I think not much performance is gained by\n> this, and findFilePath will likely be executed once.\n\nIt may not bring a large improvement, but isn't it best to specify\nstatic data as static ?\n\n> > > > + if (File::exists(filename))\n> > > > + return filename;\n> > > > +\n> > > > + std::string root = utils::libcameraSourcePath();\n> > > > + if (!root.empty()) {\n> > > > + std::string configurationPath = root + \"data/\" + filename;\n> > > > + if (File::exists(configurationPath))\n> > > > + return configurationPath;\n> > > > + }\n> > > > +\n> > > > + for (std::string &path : searchPaths) {\n> > > > + std::string configurationPath = path + \"/\" + filename;\n> > > > + if (File::exists(configurationPath))\n> > > > + return configurationPath;\n> > > > + }\n> > > > +\n> \n> Just in case, is it guaranteed path and root doesn't end with \"/\"?\n\nroot ends with a \"/\", LIBCAMERA_SYSCONF_DIR and LIBCAMERA_DATA_DIR\ndon't.\n\n> I am a bit surprised we don't have a class to deal with filepath in\n> libcamera like chromium base::FilePath.\n> https://source.chromium.org/chromium/chromium/src/+/master:base/files/file_path.h\n\nWe haven't had to deal with files much so far :-) We add helper classes\nas needed, so I wouldn't be surprised if we got new file helpers in the\nnot too distant future.\n\n> By the way, since C++17, C++ has a filesystem library as a standard\n> library, we may want to use it.\n\nThat's a good point, now that we've switched to C++17, this should be\nevaluated. I like std::filesystem::operator/ to concatenate path\nelements\n(https://en.cppreference.com/w/cpp/filesystem/path/operator_slash).\n\n> > > > + return \"\";\n> > > > +}\n> > > > +\n> > > > +FILE *CameraHalConfig::openFile(const std::string &filename)\n> > > > +{\n> > > > + const std::string filePath = findFilePath(filename);\n> > > > + if (filePath.empty()) {\n> > > > + LOG(HALConfig, Error)\n> > > > + << \"Configuration file: \\\"\" << filename << \"\\\" not found\";\n> > > > + return nullptr;\n> > > > + }\n> > > > +\n> > > > + LOG(HALConfig, Debug) << \"Reading configuration file from \" << filePath;\n> > > > +\n> > > > + FILE *fh = fopen(filePath.c_str(), \"r\");\n> > > > + if (!fh) {\n> > > > + int ret = -errno;\n> > > > + LOG(HALConfig, Error) << \"Failed to open configuration file \"\n> > > > + << filePath << \": \" << strerror(-ret);\n> > > > + return nullptr;\n> > > > + }\n> > > > +\n> > > > + return fh;\n> > > > +}\n> > > > +\n> > >\n> > > I don't know if we would like to hide like this way.\n> > > We may want to achieve this by D-pointer like libcamera::CameraManager.\n> > > I at least think cameras should be a member variable of CameraHalConfig.\n> > > I would wait for Laurent's comments.\n> >\n> > Yes, that might not be the most elegant solution. I went for static\n> > functions as otherwise I would have the need to declare private\n> > functions as part of the class, and they all take a yaml_parser_t\n> > argument, so that would have required including \"yaml.h\" in the header\n> > file. Maybe I could have get away just forward declaring it ?\n> >\n> > I considered other patterns like PIMPL, but I also considered that:\n> > 1) Parsing is done just once at HAL initialization time, having static\n> > fields like 'cameras' does not require keeping track of their state\n> > (ie. cleaning them before a new parsing, as it only happens once)\n> > 2) We want to avoid exposing \"yaml.h\" to the rest of the HAL, but\n> > afaict we do not plan to change the parsing backend, which kind of\n> > defeat the purpose of pimpl which guarantees a stable interface\n> > despite the backend implementation\n> > 3) the CameraHalConfig API is internal to the HAL and no external\n> > component depend on it\n> >\n> > Although having the parsing functions as class members would allow\n> > me to make CameraProps a proper class with the internal parser class\n> > declared as friend, so that class members fields can be made private with\n> > proper accessor functions instead of having them public (as I currently need to set\n> > them from static functions context).\n> >\n> > All in all yes, this can be made more elegant, but I wonder if that\n> > really required given the current use case.\n> \n> I would wait for Laurent's comments, while I think the current\n> implementation is not so bad as we can go this direction.\n\nAs this is internal to the HAL and not something we'll reuse as-is or\nexpose in the public API I won't insist, but wrapping the parser, file\nand possibly other data in a private class will simplify error handling\n(and will also not require passing the parser pointer to all functions).\n\nThis being said, when we'll handle other configuration files and copy\nthis code, I'll probably insist to start with a refactoring :-) Using\nthe Extensible base class would give us a CameraHalConfig::Private that\ncould nicely store all the private data.\n\n> One point I would like to change is to make camera a member variable\n> of CameraHalConfig.\n> If it is static, it should not be initialized more than once, but\n> CameraHalConfig itself doesn't avoid it.\n> Besides, CameraHalConfig is a member variable of CameraHalManager,\n> which is static.\n> We can let it be a member variable of CameraHalConfig by passing\n> camera pointer or reference to parseCameraProps() through some\n> functions out of CameraHalConfig.\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> > > > + yaml_parser_t parser;\n> > > > + int ret = yaml_parser_initialize(&parser);\n> > > > + if (!ret) {\n> > > > + LOG(HALConfig, Fatal) << \"Failed to initialize yaml parser\";\n> > > > + return -EINVAL;\n> > > > + }\n> > > > +\n> > > > + FILE *fh = openFile(\"camera_hal.yaml\");\n> > > > + if (!fh)\n\nThis will leak the parser, you need to call yaml_parser_delete(). Not\nsure if this is enough to convince you a private class will be less\npainful :-)\n\n> > > > + return -ENOENT;\n> > > > +\n> > > > + yaml_parser_set_input_file(&parser, fh);\n> > > > + ret = parseConfigFile(&parser);\n> > > > + fclose(fh);\n> > > > + yaml_parser_delete(&parser);\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> > > > + << \" model: \" << camera.model\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..5491a12fdffd\n> > > > --- /dev/null\n> > > > +++ b/src/android/camera_hal_config.h\n> > > > @@ -0,0 +1,36 @@\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> > > > +\n> > > > +class CameraProps\n> > > > +{\n> > > > +public:\n> > > > + CameraProps()\n> > > > + : valid(false) {}\n> > > > +\n> > > > + int facing;\n> > > > + std::string model;\n> > > > + unsigned int rotation;\n> > > > +\n> > > > + bool valid;\n> > > > +};\n> > >\n> > > This doesn't have a function.\n\nIt has a constructor :-) Still, your point stands, and we could also\nexpress this with\n\nstruct CameraProps {\n int facing;\n std::string model;\n unsigned int rotation;\n\n bool valid = false;\n};\n\n> > > So I would declare this as struct.\n> > >\n> > > https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes\n> > >\n> > > > +\n> > > > +class CameraHalConfig\n> > > > +{\n> > > > +public:\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> > > > +#endif /* __ANDROID_CAMERA_HAL_CONFIG_H__ */\n> > > > +\n> > > > diff --git a/src/android/meson.build b/src/android/meson.build\n> > > > index 8e7d07d9be3c..c0ede407bc38 100644\n> > > > --- a/src/android/meson.build\n> > > > +++ b/src/android/meson.build\n> > > > @@ -3,6 +3,7 @@\n> > > > android_deps = [\n> > > > dependency('libexif', required : get_option('android')),\n> > > > dependency('libjpeg', required : get_option('android')),\n> > > > + dependency('yaml-0.1', required : get_option('android')),\n> > > > ]\n> > > >\n> > > > android_enabled = true\n> > > > @@ -41,6 +42,7 @@ android_hal_sources = files([\n> > > > 'camera3_hal.cpp',\n> > > > 'camera_hal_manager.cpp',\n> > > > 'camera_device.cpp',\n> > > > + 'camera_hal_config.cpp',\n> > > > 'camera_metadata.cpp',\n> > > > 'camera_ops.cpp',\n> > > > 'camera_stream.cpp',\n> \n> We may want to add a unit test for CameraHalConfig although it is fine\n> in another patch series.","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 A2281BD695\n\tfor ;\n\tSat, 3 Apr 2021 03:51:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F0EA068788;\n\tSat, 3 Apr 2021 05:51:57 +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 6AB5A602D7\n\tfor ;\n\tSat, 3 Apr 2021 05:51:56 +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 A0A2D3D7;\n\tSat, 3 Apr 2021 05:51:55 +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=\"Mnl0Kw53\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1617421915;\n\tbh=5rkpAsdLbyqApm6nU3dE3gqNVU3ty36yL2D1NQNjMwg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Mnl0Kw539AVH22rTlYhg2CbMIhp6LVX/Zact/qiXTzuUdYK7JU6GCQCN1zuaTkn5X\n\tSHl0eUMLK6NZLHL2Mh2wUSdymlthyKOhawPy0LOA1Z1bXzIpoklG4sG+pvQtstH7G2\n\tH78U/poYPx6/otKWrXE5waHGmSwRUYJZHgMGuWbY=","Date":"Sat, 3 Apr 2021 06:51:11 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210330142113.37457-1-jacopo@jmondi.org>\n\t<20210330142113.37457-4-jacopo@jmondi.org>\n\t\n\t<20210331112759.bm6c5gxsaf6pqc5u@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 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":16128,"web_url":"https://patchwork.libcamera.org/comment/16128/","msgid":"<20210406145000.5igwfteqaic7udi2@uno.localdomain>","date":"2021-04-06T14:50:00","subject":"Re: [libcamera-devel] [PATCH v3 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 Thu, Apr 01, 2021 at 01:48:25PM +0900, Hirokazu Honda wrote:\n> Hi Jacopo,\n>\n> On Wed, Mar 31, 2021 at 8:27 PM Jacopo Mondi wrote:\n> >\n> > Hi Hiro,\n> >\n> > On Wed, Mar 31, 2021 at 06:30:34PM +0900, Hirokazu Honda wrote:\n> > > Hi Jacopo, thanks for the patch.\n> > >\n> > > On Tue, Mar 30, 2021 at 11:20 PM 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 | 407 ++++++++++++++++++++++++++++++\n> > > > src/android/camera_hal_config.h | 36 +++\n> > > > src/android/meson.build | 2 +\n> > > > 4 files changed, 446 insertions(+), 1 deletion(-)\n> > > > create mode 100644 src/android/camera_hal_config.cpp\n> > > > create mode 100644 src/android/camera_hal_config.h\n> > > >\n> > > > diff --git a/README.rst b/README.rst\n> > > > index 7a98164bbb0a..f68d435196de 100644\n> > > > --- a/README.rst\n> > > > +++ b/README.rst\n> > > > @@ -88,7 +88,7 @@ for tracing with lttng: [optional]\n> > > > liblttng-ust-dev python3-jinja2 lttng-tools\n> > > >\n> > > > for android: [optional]\n> > > > - libexif libjpeg\n> > > > + libexif libjpeg libyaml\n> > > >\n> > > > Using GStreamer plugin\n> > > > ~~~~~~~~~~~~~~~~~~~~~~\n> > > > diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp\n> > > > new file mode 100644\n> > > > index 000000000000..b109ad24e1d1\n> > > > --- /dev/null\n> > > > +++ b/src/android/camera_hal_config.cpp\n> > > > @@ -0,0 +1,407 @@\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> > > > +\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> > > > +namespace {\n> > > > +\n> > > > +std::map cameras;\n> > > > +\n> > > > +std::string parseValue(yaml_parser_t *parser)\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> > > > + char *scalar = reinterpret_cast(token.data.scalar.value);\n> > > > + std::string value(scalar, token.data.scalar.length);\n> > > > + yaml_token_delete(&token);\n> > > > +\n> > > > + return value;\n> > > > +}\n> > > > +\n> > > > +std::string parseKey(yaml_parser_t *parser)\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> > > > + char *scalar = reinterpret_cast(token.data.scalar.value);\n> > > > + std::string key(scalar, token.data.scalar.length);\n> > > > + yaml_token_delete(&token);\n> > > > +\n> > > > + return key;\n> > > > +}\n> > > > +\n> > > > +int parseValueBlock(yaml_parser_t *parser)\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 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> > > > + cameraProps->facing = -EINVAL;\n> > > > + return -EINVAL;\n> > > > + }\n> > > > +\n> > > > + return 0;\n> > > > +}\n> > > > +\n> > > > +int parseCameraProps(yaml_parser_t *parser, const std::string &cameraID)\n> > > > +{\n> > > > + int ret = parseValueBlock(parser);\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(parser);\n> > > > + std::string value = parseValue(parser);\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> > > > + << \"Unupported location: \"\n> > > > + << value;\n> > > > + return -EINVAL;\n> > > > + }\n> > > > + } else if (key == \"rotation\") {\n> > > > + cameraProps.rotation = strtoul(value.c_str(),\n> > > > + NULL, 10);\n> > > > + if (cameraProps.rotation < 0) {\n> > > > + LOG(HALConfig, Error)\n> > > > + << \"Unknown rotation: \"\n> > > > + << cameraProps.rotation;\n> > > > + return -EINVAL;\n>\n> nit: std::stoul(value) in c++ way.\n>\n> This seems to be strange, strtoul() returns *unsigned* integer and the\n> if-clause check if it is negative.\n> Perhaps, you mean stol() [c++] or strtol() [c]?\n\nOh right, now that I've updated clang it also throws an error here.\nI'll use std::stoi()\n\n>\n> > > > + }\n> > > > + } else if (key == \"model\") {\n> > > > + cameraProps.model = value;\n> > > > + } else {\n> > > > + LOG(HALConfig, Error)\n> > > > + << \"Unknown key: \" << key;\n> > > > + return -EINVAL;\n> > > > + }\n> > > > + break;\n> > > > + }\n> > > > + case YAML_BLOCK_END_TOKEN:\n> > > > + blockEnd = true;\n> > > > + /* Fall-through */\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 parseCameras(yaml_parser_t *parser)\n> > > > +{\n> > > > + int ret = parseValueBlock(parser);\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(parser);\n> > > > + if (cameraId.empty())\n> > > > + return -EINVAL;\n> > > > +\n> > > > + ret = parseCameraProps(parser, cameraId);\n> > > > + if (ret)\n> > > > + return -EINVAL;\n> > > > + break;\n> > > > + }\n> > > > + case YAML_BLOCK_END_TOKEN:\n> > > > + blockEnd = true;\n> > > > + /* Fall-through */\n> > > > + default:\n> > > > + yaml_token_delete(&token);\n> > > > + break;\n> > > > + }\n> > > > + } while (!blockEnd);\n> > > > +\n> > > > + return 0;\n> > > > +}\n> > > > +\n> > > > +int parseEntry(yaml_parser_t *parser)\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(parser);\n> > > > + if (key.empty())\n> > > > + return ret;\n> > > > +\n> > > > + if (key == \"cameras\") {\n> > > > + ret = parseCameras(parser);\n> > > > + } else\n> > > > + LOG(HALConfig, Error) << \"Unknown key: \" << key;\n> > > > +\n>\n> You may want to remove parenthesis.\n>\n> > > > + return ret;\n> > > > +}\n> > > > +\n> > > > +int parseConfigFile(yaml_parser_t *parser)\n> > > > +{\n> > > > + yaml_token_t token;\n> > > > + int ret = 0;\n> > > > +\n> > > > + yaml_parser_scan(parser, &token);\n> > > > + if (token.type != YAML_STREAM_START_TOKEN) {\n> > > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > > + 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> > > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > > + yaml_token_delete(&token);\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(parser);\n> > > > + break;\n> > > > +\n> > > > + case YAML_STREAM_END_TOKEN:\n> > > > + ret = -ENOENT;\n> > > > + /* Fall-through */\n> > > > + default:\n> > > > + yaml_token_delete(&token);\n> > > > + break;\n> > > > + }\n> > > > + } while (ret >= 0);\n> > > > +\n> > > > + if (ret && ret != -ENOENT)\n> > > > + LOG(HALConfig, Error) << \"Configuration file is not valid\";\n> > > > +\n> > > > + return ret == -ENOENT ? 0 : ret;\n>\n> How is the following code?\n> int ret = 0;\n> while (token.type != YAML_STREAM_END_TOKEN) {\n> yaml_token_delete(&token);\n> if (token.type == YAML_KEY_TOKEN) {\n> ret = parseEntry(parser);\n> if (!ret)\n> break;\n> }\n> }\n> yaml_token_delete(&token);\n>\n> if (ret)\n> LOG(HALConfig, Error) << \"Configuration file is not valid\";\n>\n> return ret;\n>\n> > > > +}\n> > > > +\n> > > > +} /* namespace */\n> > > > +\n> > > > +std::string CameraHalConfig::findFilePath(const std::string &filename) const\n> > > > +{\n> > > > + static std::array searchPaths = {\n> > > > + LIBCAMERA_SYSCONF_DIR,\n> > > > + LIBCAMERA_DATA_DIR,\n> > > > + };\n> > > > +\n>\n> Is this static beneficial? I think not much performance is gained by\n> this, and findFilePath will likely be executed once.\n>\n> > > > + if (File::exists(filename))\n> > > > + return filename;\n> > > > +\n> > > > + std::string root = utils::libcameraSourcePath();\n> > > > + if (!root.empty()) {\n> > > > + std::string configurationPath = root + \"data/\" + filename;\n> > > > + if (File::exists(configurationPath))\n> > > > + return configurationPath;\n> > > > + }\n> > > > +\n> > > > + for (std::string &path : searchPaths) {\n> > > > + std::string configurationPath = path + \"/\" + filename;\n> > > > + if (File::exists(configurationPath))\n> > > > + return configurationPath;\n> > > > + }\n> > > > +\n>\n> Just in case, is it guaranteed path and root doesn't end with \"/\"?\n> I am a bit surprised we don't have a class to deal with filepath in\n> libcamera like chromium base::FilePath.\n> https://source.chromium.org/chromium/chromium/src/+/master:base/files/file_path.h\n>\n> By the way, since C++17, C++ has a filesystem library as a standard\n> library, we may want to use it.\n>\n>\n> > > > + return \"\";\n> > > > +}\n> > > > +\n> > > > +FILE *CameraHalConfig::openFile(const std::string &filename)\n> > > > +{\n> > > > + const std::string filePath = findFilePath(filename);\n> > > > + if (filePath.empty()) {\n> > > > + LOG(HALConfig, Error)\n> > > > + << \"Configuration file: \\\"\" << filename << \"\\\" not found\";\n> > > > + return nullptr;\n> > > > + }\n> > > > +\n> > > > + LOG(HALConfig, Debug) << \"Reading configuration file from \" << filePath;\n> > > > +\n> > > > + FILE *fh = fopen(filePath.c_str(), \"r\");\n> > > > + if (!fh) {\n> > > > + int ret = -errno;\n> > > > + LOG(HALConfig, Error) << \"Failed to open configuration file \"\n> > > > + << filePath << \": \" << strerror(-ret);\n> > > > + return nullptr;\n> > > > + }\n> > > > +\n> > > > + return fh;\n> > > > +}\n> > > > +\n> > >\n> > > I don't know if we would like to hide like this way.\n> > > We may want to achieve this by D-pointer like libcamera::CameraManager.\n> > > I at least think cameras should be a member variable of CameraHalConfig.\n> > > I would wait for Laurent's comments.\n> > >\n> >\n> > Yes, that might not be the most elegant solution. I went for static\n> > functions as otherwise I would have the need to declare private\n> > functions as part of the class, and they all take a yaml_parser_t\n> > argument, so that would have required including \"yaml.h\" in the header\n> > file. Maybe I could have get away just forward declaring it ?\n> >\n> > I considered other patterns like PIMPL, but I also considered that:\n> > 1) Parsing is done just once at HAL initialization time, having static\n> > fields like 'cameras' does not require keeping track of their state\n> > (ie. cleaning them before a new parsing, as it only happens once)\n> > 2) We want to avoid exposing \"yaml.h\" to the rest of the HAL, but\n> > afaict we do not plan to change the parsing backend, which kind of\n> > defeat the purpose of pimpl which guarantees a stable interface\n> > despite the backend implementation\n> > 3) the CameraHalConfig API is internal to the HAL and no external\n> > component depend on it\n> >\n> > Although having the parsing functions as class members would allow\n> > me to make CameraProps a proper class with the internal parser class\n> > declared as friend, so that class members fields can be made private with\n> > proper accessor functions instead of having them public (as I currently need to set\n> > them from static functions context).\n> >\n> > All in all yes, this can be made more elegant, but I wonder if that\n> > really required given the current use case.\n> >\n>\n> I would wait for Laurent's comments, while I think the current\n> implementation is not so bad as we can go this direction.\n> One point I would like to change is to make camera a member variable\n> of CameraHalConfig.\n> If it is static, it should not be initialized more than once, but\n> CameraHalConfig itself doesn't avoid it.\n> Besides, CameraHalConfig is a member variable of CameraHalManager,\n> which is static.\n> We can let it be a member variable of CameraHalConfig by passing\n> camera pointer or reference to parseCameraProps() through some\n> functions out of CameraHalConfig.\n>\n> > Thanks\n> > j\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> > > > + yaml_parser_t parser;\n> > > > + int ret = yaml_parser_initialize(&parser);\n> > > > + if (!ret) {\n> > > > + LOG(HALConfig, Fatal) << \"Failed to initialize yaml parser\";\n> > > > + return -EINVAL;\n> > > > + }\n> > > > +\n> > > > + FILE *fh = openFile(\"camera_hal.yaml\");\n> > > > + if (!fh)\n> > > > + return -ENOENT;\n> > > > +\n> > > > + yaml_parser_set_input_file(&parser, fh);\n> > > > + ret = parseConfigFile(&parser);\n> > > > + fclose(fh);\n> > > > + yaml_parser_delete(&parser);\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> > > > + << \" model: \" << camera.model\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..5491a12fdffd\n> > > > --- /dev/null\n> > > > +++ b/src/android/camera_hal_config.h\n> > > > @@ -0,0 +1,36 @@\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> > > > +\n> > > > +class CameraProps\n> > > > +{\n> > > > +public:\n> > > > + CameraProps()\n> > > > + : valid(false) {}\n> > > > +\n> > > > + int facing;\n> > > > + std::string model;\n> > > > + unsigned int rotation;\n> > > > +\n> > > > + bool valid;\n> > > > +};\n> > >\n> > > This doesn't have a function.\n> > > So I would declare this as struct.\n> > >\n> > > https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes\n> > >\n> > > Best Regards,\n> > > -Hiro\n> > > > +\n> > > > +class CameraHalConfig\n> > > > +{\n> > > > +public:\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> > > > +#endif /* __ANDROID_CAMERA_HAL_CONFIG_H__ */\n> > > > +\n> > > > diff --git a/src/android/meson.build b/src/android/meson.build\n> > > > index 8e7d07d9be3c..c0ede407bc38 100644\n> > > > --- a/src/android/meson.build\n> > > > +++ b/src/android/meson.build\n> > > > @@ -3,6 +3,7 @@\n> > > > android_deps = [\n> > > > dependency('libexif', required : get_option('android')),\n> > > > dependency('libjpeg', required : get_option('android')),\n> > > > + dependency('yaml-0.1', required : get_option('android')),\n> > > > ]\n> > > >\n> > > > android_enabled = true\n> > > > @@ -41,6 +42,7 @@ android_hal_sources = files([\n> > > > 'camera3_hal.cpp',\n> > > > 'camera_hal_manager.cpp',\n> > > > 'camera_device.cpp',\n> > > > + 'camera_hal_config.cpp',\n> > > > 'camera_metadata.cpp',\n> > > > 'camera_ops.cpp',\n> > > > 'camera_stream.cpp',\n> > > > --\n> > > > 2.30.0\n> > > >\n>\n> We may want to add a unit test for CameraHalConfig although it is fine\n> in another patch series.\n>\n> Best Regards,\n> -Hiro\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 85720C0DA3\n\tfor ;\n\tTue, 6 Apr 2021 14:49:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EF1076879F;\n\tTue, 6 Apr 2021 16:49:25 +0200 (CEST)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A1B166084F\n\tfor ;\n\tTue, 6 Apr 2021 16:49:24 +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 relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 2E2F11BF220;\n\tTue, 6 Apr 2021 14:49:23 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Tue, 6 Apr 2021 16:50:00 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210406145000.5igwfteqaic7udi2@uno.localdomain>","References":"<20210330142113.37457-1-jacopo@jmondi.org>\n\t<20210330142113.37457-4-jacopo@jmondi.org>\n\t\n\t<20210331112759.bm6c5gxsaf6pqc5u@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 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\" "}}]