[{"id":22734,"web_url":"https://patchwork.libcamera.org/comment/22734/","msgid":"<Yl6Vwe2amKu2bJSr@pendragon.ideasonboard.com>","date":"2022-04-19T10:58:09","subject":"Re: [libcamera-devel] [PATCH v3 2/2] android: camera_hal_config:\n\tUse YamlParser to parse android HAL config","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Han-Lin,\n\nThank you for the patch.\n\nOn Mon, Apr 18, 2022 at 08:09:23PM +0800, Han-Lin Chen via libcamera-devel wrote:\n> Use YamlParser to parse android HAL config files, instead of handling\n> YAML tokens directly, as a preparation for the further parameter extension.\n> \n> Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n> ---\n>  src/android/camera_hal_config.cpp | 334 +++++++-----------------------\n>  src/android/meson.build           |   1 -\n>  2 files changed, 74 insertions(+), 261 deletions(-)\n> \n> diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp\n> index aa90dac7..d269ab25 100644\n> --- a/src/android/camera_hal_config.cpp\n> +++ b/src/android/camera_hal_config.cpp\n> @@ -14,15 +14,15 @@ namespace filesystem = std::experimental::filesystem;\n>  #else\n>  #include <filesystem>\n>  #endif\n> -#include <stdio.h>\n>  #include <stdlib.h>\n>  #include <string>\n> -#include <yaml.h>\n>  \n>  #include <hardware/camera3.h>\n>  \n>  #include <libcamera/base/log.h>\n>  \n> +#include <libcamera/internal/yaml_parser.h>\n> +\n>  using namespace libcamera;\n>  \n>  LOG_DEFINE_CATEGORY(HALConfig)\n> @@ -37,16 +37,10 @@ public:\n>  \tint parseConfigFile(FILE *fh, std::map<std::string, CameraConfigData> *cameras);\n>  \n>  private:\n> -\tstd::string parseValue();\n> -\tstd::string parseKey();\n> -\tint parseValueBlock();\n> -\tint parseCameraLocation(CameraConfigData *cameraConfigData,\n> -\t\t\t\tconst std::string &location);\n> -\tint parseCameraConfigData(const std::string &cameraId);\n> -\tint parseCameras();\n> -\tint parseEntry();\n> -\n> -\tyaml_parser_t parser_;\n> +\tint parseCameraConfigData(const std::string &cameraId, const YamlObject &);\n> +\tint parseLocation(const YamlObject &, CameraConfigData &cameraConfigData);\n> +\tint parseRotation(const YamlObject &, CameraConfigData &cameraConfigData);\n> +\n>  \tstd::map<std::string, CameraConfigData> *cameras_;\n>  };\n>  \n> @@ -54,290 +48,110 @@ CameraHalConfig::Private::Private()\n>  {\n>  }\n>  \n> -std::string CameraHalConfig::Private::parseValue()\n> +int CameraHalConfig::Private::parseConfigFile(FILE *fh,\n> +\t\t\t\t\t      std::map<std::string, CameraConfigData> *cameras)\n>  {\n> -\tyaml_token_t token;\n> -\n> -\t/* Make sure the token type is a value and get its content. */\n> -\tyaml_parser_scan(&parser_, &token);\n> -\tif (token.type != YAML_VALUE_TOKEN) {\n> -\t\tyaml_token_delete(&token);\n> -\t\treturn \"\";\n> -\t}\n> -\tyaml_token_delete(&token);\n> -\n> -\tyaml_parser_scan(&parser_, &token);\n> -\tif (token.type != YAML_SCALAR_TOKEN) {\n> -\t\tyaml_token_delete(&token);\n> -\t\treturn \"\";\n> -\t}\n> -\n> -\tstd::string value(reinterpret_cast<char *>(token.data.scalar.value),\n> -\t\t\t  token.data.scalar.length);\n> -\tyaml_token_delete(&token);\n> -\n> -\treturn value;\n> -}\n> +\t/*\n> +\t * Parse the HAL properties.\n> +\t *\n> +\t * Each camera properties block is a list of properties associated\n> +\t * with the ID (as assembled by CameraSensor::generateId()) of the\n> +\t * camera they refer to.\n> +\t *\n> +\t * cameras:\n> +\t *   \"camera0 id\":\n> +\t *     location: value\n> +\t *     rotation: value\n> +\t *     ...\n> +\t *\n> +\t *   \"camera1 id\":\n> +\t *     location: value\n> +\t *     rotation: value\n> +\t *     ...\n> +\t */\n>  \n> -std::string CameraHalConfig::Private::parseKey()\n> -{\n> -\tyaml_token_t token;\n> +\tcameras_ = cameras;\n>  \n> -\t/* Make sure the token type is a key and get its value. */\n> -\tyaml_parser_scan(&parser_, &token);\n> -\tif (token.type != YAML_SCALAR_TOKEN) {\n> -\t\tyaml_token_delete(&token);\n> -\t\treturn \"\";\n> -\t}\n> +\tstd::unique_ptr<YamlObject> root = YamlParser::parse(fh);\n> +\tif (!root)\n> +\t\treturn -EINVAL;\n>  \n> -\tstd::string value(reinterpret_cast<char *>(token.data.scalar.value),\n> -\t\t\t  token.data.scalar.length);\n> -\tyaml_token_delete(&token);\n> +\tif (!root->isDictionary())\n> +\t\treturn -EINVAL;\n>  \n> -\treturn value;\n> -}\n> +\t/* Parse property \"cameras\" */\n> +\tif (!root->contains(\"cameras\"))\n> +\t\treturn -EINVAL;\n>  \n> -int CameraHalConfig::Private::parseValueBlock()\n> -{\n> -\tyaml_token_t token;\n> +\tconst YamlObject &yamlObjectCameras = (*root)[\"cameras\"];\n>  \n> -\t/* Make sure the next token are VALUE and BLOCK_MAPPING_START. */\n> -\tyaml_parser_scan(&parser_, &token);\n> -\tif (token.type != YAML_VALUE_TOKEN) {\n> -\t\tyaml_token_delete(&token);\n> +\tif (!yamlObjectCameras.isDictionary())\n>  \t\treturn -EINVAL;\n> -\t}\n> -\tyaml_token_delete(&token);\n>  \n> -\tyaml_parser_scan(&parser_, &token);\n> -\tif (token.type != YAML_BLOCK_MAPPING_START_TOKEN) {\n> -\t\tyaml_token_delete(&token);\n> -\t\treturn -EINVAL;\n> +\tstd::vector<std::string> cameraIds = yamlObjectCameras.getMemberNames();\n> +\tfor (const std::string &cameraId : cameraIds) {\n> +\t\tif (parseCameraConfigData(cameraId,\n> +\t\t\t\t\t  yamlObjectCameras[cameraId]))\n> +\t\t\treturn -EINVAL;\n>  \t}\n> -\tyaml_token_delete(&token);\n>  \n>  \treturn 0;\n>  }\n>  \n> -int CameraHalConfig::Private::parseCameraLocation(CameraConfigData *cameraConfigData,\n> -\t\t\t\t\t\t  const std::string &location)\n> -{\n> -\tif (location == \"front\")\n> -\t\tcameraConfigData->facing = CAMERA_FACING_FRONT;\n> -\telse if (location == \"back\")\n> -\t\tcameraConfigData->facing = CAMERA_FACING_BACK;\n> -\telse\n> -\t\treturn -EINVAL;\n> +int CameraHalConfig::Private::parseCameraConfigData(const std::string &cameraId,\n> +\t\t\t\t\t\t    const YamlObject &cameraObject)\n>  \n> -\treturn 0;\n> -}\n> -\n> -int CameraHalConfig::Private::parseCameraConfigData(const std::string &cameraId)\n>  {\n> -\tint ret = parseValueBlock();\n> -\tif (ret)\n> -\t\treturn ret;\n> -\n> -\t/*\n> -\t * Parse the camera properties and store them in a cameraConfigData\n> -\t * instance.\n> -\t *\n> -\t * Add a safety counter to make sure we don't loop indefinitely in case\n> -\t * the configuration file is malformed.\n> -\t */\n> -\tCameraConfigData cameraConfigData;\n> -\tunsigned int sentinel = 100;\n> -\tbool blockEnd = false;\n> -\tyaml_token_t token;\n> -\n> -\tdo {\n> -\t\tyaml_parser_scan(&parser_, &token);\n> -\t\tswitch (token.type) {\n> -\t\tcase YAML_KEY_TOKEN: {\n> -\t\t\tyaml_token_delete(&token);\n> -\n> -\t\t\t/*\n> -\t\t\t * Parse the camera property key and make sure it is\n> -\t\t\t * valid.\n> -\t\t\t */\n> -\t\t\tstd::string key = parseKey();\n> -\t\t\tstd::string value = parseValue();\n> -\t\t\tif (key.empty() || value.empty())\n> -\t\t\t\treturn -EINVAL;\n> -\n> -\t\t\tif (key == \"location\") {\n> -\t\t\t\tret = parseCameraLocation(&cameraConfigData, value);\n> -\t\t\t\tif (ret) {\n> -\t\t\t\t\tLOG(HALConfig, Error)\n> -\t\t\t\t\t\t<< \"Unknown location: \" << value;\n> -\t\t\t\t\treturn -EINVAL;\n> -\t\t\t\t}\n> -\t\t\t} else if (key == \"rotation\") {\n> -\t\t\t\tret = std::stoi(value);\n> -\t\t\t\tif (ret < 0 || ret >= 360) {\n> -\t\t\t\t\tLOG(HALConfig, Error)\n> -\t\t\t\t\t\t<< \"Unknown rotation: \" << value;\n> -\t\t\t\t\treturn -EINVAL;\n> -\t\t\t\t}\n> -\t\t\t\tcameraConfigData.rotation = ret;\n> -\t\t\t} else {\n> -\t\t\t\tLOG(HALConfig, Error)\n> -\t\t\t\t\t<< \"Unknown key: \" << key;\n> -\t\t\t\treturn -EINVAL;\n> -\t\t\t}\n> -\t\t\tbreak;\n> -\t\t}\n> -\n> -\t\tcase YAML_BLOCK_END_TOKEN:\n> -\t\t\tblockEnd = true;\n> -\t\t\t[[fallthrough]];\n> -\t\tdefault:\n> -\t\t\tyaml_token_delete(&token);\n> -\t\t\tbreak;\n> -\t\t}\n> -\n> -\t\t--sentinel;\n> -\t} while (!blockEnd && sentinel);\n> -\tif (!sentinel)\n> +\tif (!cameraObject.isDictionary())\n>  \t\treturn -EINVAL;\n>  \n> -\t(*cameras_)[cameraId] = cameraConfigData;\n> -\n> -\treturn 0;\n> -}\n> +\tCameraConfigData &cameraConfigData = (*cameras_)[cameraId];\n>  \n> -int CameraHalConfig::Private::parseCameras()\n> -{\n> -\tint ret = parseValueBlock();\n> -\tif (ret) {\n> -\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> -\t\treturn ret;\n> -\t}\n> +\t/* Parse property \"location\" */\n> +\tif (parseLocation(cameraObject, cameraConfigData))\n> +\t\treturn -EINVAL;\n>  \n> -\t/*\n> -\t * Parse the camera properties.\n> -\t *\n> -\t * Each camera properties block is a list of properties associated\n> -\t * with the ID (as assembled by CameraSensor::generateId()) of the\n> -\t * camera they refer to.\n> -\t *\n> -\t * cameras:\n> -\t *   \"camera0 id\":\n> -\t *     key: value\n> -\t *     key: value\n> -\t *     ...\n> -\t *\n> -\t *   \"camera1 id\":\n> -\t *     key: value\n> -\t *     key: value\n> -\t *     ...\n> -\t */\n> -\tbool blockEnd = false;\n> -\tyaml_token_t token;\n> -\tdo {\n> -\t\tyaml_parser_scan(&parser_, &token);\n> -\t\tswitch (token.type) {\n> -\t\tcase YAML_KEY_TOKEN: {\n> -\t\t\tyaml_token_delete(&token);\n> -\n> -\t\t\t/* Parse the camera ID as key of the property list. */\n> -\t\t\tstd::string cameraId = parseKey();\n> -\t\t\tif (cameraId.empty())\n> -\t\t\t\treturn -EINVAL;\n> -\n> -\t\t\tret = parseCameraConfigData(cameraId);\n> -\t\t\tif (ret)\n> -\t\t\t\treturn -EINVAL;\n> -\t\t\tbreak;\n> -\t\t}\n> -\t\tcase YAML_BLOCK_END_TOKEN:\n> -\t\t\tblockEnd = true;\n> -\t\t\t[[fallthrough]];\n> -\t\tdefault:\n> -\t\t\tyaml_token_delete(&token);\n> -\t\t\tbreak;\n> -\t\t}\n> -\t} while (!blockEnd);\n> +\t/* Parse property \"rotation\" */\n> +\tif (parseRotation(cameraObject, cameraConfigData))\n> +\t\treturn -EINVAL;\n>  \n>  \treturn 0;\n>  }\n>  \n> -int CameraHalConfig::Private::parseEntry()\n> +int CameraHalConfig::Private::parseLocation(const YamlObject &cameraObject,\n> +\t\t\t\t\t    CameraConfigData &cameraConfigData)\n>  {\n> -\tint ret = -EINVAL;\n> +\tif (!cameraObject.contains(\"location\"))\n> +\t\treturn -EINVAL;\n>  \n> -\t/*\n> -\t * Parse each key we find in the file.\n> -\t *\n> -\t * The 'cameras' keys maps to a list of (lists) of camera properties.\n> -\t */\n> +\tstd::string location = cameraObject[\"location\"].get<std::string>(\"\");\n>  \n> -\tstd::string key = parseKey();\n> -\tif (key.empty())\n> -\t\treturn ret;\n> -\n> -\tif (key == \"cameras\")\n> -\t\tret = parseCameras();\n> +\tif (location == \"front\")\n> +\t\tcameraConfigData.facing = CAMERA_FACING_FRONT;\n> +\telse if (location == \"back\")\n> +\t\tcameraConfigData.facing = CAMERA_FACING_BACK;\n>  \telse\n> -\t\tLOG(HALConfig, Error) << \"Unknown key: \" << key;\n> +\t\treturn -EINVAL;\n>  \n> -\treturn ret;\n> +\treturn 0;\n>  }\n>  \n> -int CameraHalConfig::Private::parseConfigFile(FILE *fh,\n> -\t\t\t\t\t      std::map<std::string, CameraConfigData> *cameras)\n> +int CameraHalConfig::Private::parseRotation(const YamlObject &cameraObject,\n> +\t\t\t\t\t    CameraConfigData &cameraConfigData)\n>  {\n> -\tcameras_ = cameras;\n> -\n> -\tint ret = yaml_parser_initialize(&parser_);\n> -\tif (!ret) {\n> -\t\tLOG(HALConfig, Error) << \"Failed to initialize yaml parser\";\n> -\t\treturn -EINVAL;\n> -\t}\n> -\tyaml_parser_set_input_file(&parser_, fh);\n> -\n> -\tyaml_token_t token;\n> -\tyaml_parser_scan(&parser_, &token);\n> -\tif (token.type != YAML_STREAM_START_TOKEN) {\n> -\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> -\t\tyaml_token_delete(&token);\n> -\t\tyaml_parser_delete(&parser_);\n> +\tif (!cameraObject.contains(\"rotation\"))\n>  \t\treturn -EINVAL;\n> -\t}\n> -\tyaml_token_delete(&token);\n>  \n> -\tyaml_parser_scan(&parser_, &token);\n> -\tif (token.type != YAML_BLOCK_MAPPING_START_TOKEN) {\n> -\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> -\t\tyaml_token_delete(&token);\n> -\t\tyaml_parser_delete(&parser_);\n> +\tint32_t rotation = cameraObject[\"rotation\"].get<int32_t>(-1);\n> +\n> +\tif (rotation < 0 || rotation >= 360) {\n> +\t\tLOG(HALConfig, Error)\n> +\t\t\t<< \"Unknown rotation: \" << rotation;\n>  \t\treturn -EINVAL;\n>  \t}\n> -\tyaml_token_delete(&token);\n> -\n> -\t/* Parse the file and parse each single key one by one. */\n> -\tdo {\n> -\t\tyaml_parser_scan(&parser_, &token);\n> -\t\tswitch (token.type) {\n> -\t\tcase YAML_KEY_TOKEN:\n> -\t\t\tyaml_token_delete(&token);\n> -\t\t\tret = parseEntry();\n> -\t\t\tbreak;\n> -\n> -\t\tcase YAML_STREAM_END_TOKEN:\n> -\t\t\tret = -ENOENT;\n> -\t\t\t[[fallthrough]];\n> -\t\tdefault:\n> -\t\t\tyaml_token_delete(&token);\n> -\t\t\tbreak;\n> -\t\t}\n> -\t} while (ret >= 0);\n> -\tyaml_parser_delete(&parser_);\n> -\n> -\tif (ret && ret != -ENOENT)\n> -\t\tLOG(HALConfig, Error) << \"Configuration file is not valid\";\n> -\n> -\treturn ret == -ENOENT ? 0 : ret;\n> +\n> +\tcameraConfigData.rotation = rotation;\n> +\treturn 0;\n>  }\n>  \n>  CameraHalConfig::CameraHalConfig()\n> diff --git a/src/android/meson.build b/src/android/meson.build\n> index 75b4bf20..1bba54de 100644\n> --- a/src/android/meson.build\n> +++ b/src/android/meson.build\n> @@ -3,7 +3,6 @@\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>      libcamera_private,\n>  ]\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 1D982C3256\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 19 Apr 2022 10:58:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 845F56563D;\n\tTue, 19 Apr 2022 12:58:30 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4994C6563C\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 19 Apr 2022 12:58:28 +0200 (CEST)","from pendragon.ideasonboard.com (85-76-66-170-nat.elisa-mobile.fi\n\t[85.76.66.170])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5E3195D;\n\tTue, 19 Apr 2022 12:58:20 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1650365910;\n\tbh=tRe6Oe9Gg+G9JGG0AXhokaqbampL/eTv9WkumWkzcVc=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=oMamidQBE5BReQAKrUfhROtEfY/BpCfo0OFAYv1254pfE89yGjP16+ZuwDP79wW8+\n\tBHoq5jzEy6LmV07Kz44aGZps+Vl/qUfDiB4txoAHkmpk2Lq5Z7bk7OTxHG6KZvpPB0\n\tNSY25RLNYo/Xsgj5yZcs01VraLMCnNTB7YcCAYvLOE7F+WVaBQ2T7PdmqkPkaLlOXv\n\t62opxPPFGmJkAMw8S1TUf598koeioIRgv/b8bFhNlOtNtcH18k/Jc7spEUzBzlDLHX\n\tyQrn8jt2a+/tOqa8un6QHyjlIlz9vv4+yNWiAhVCPCYqBjX++uRVR34wVlwB4Y8h1Q\n\t4uEQh8A6/rAQw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1650365907;\n\tbh=tRe6Oe9Gg+G9JGG0AXhokaqbampL/eTv9WkumWkzcVc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hKlovF/ZhDHX1hrXSMyKbtVOKoUAjZ0U3KzP0jescYkoD6XynwVTU8IkFgm6GAZs4\n\tE8DJs92chao3A/AUxG770QRgXV3RpKBJv+gw6sJD8DEXgDVvcmRiAOU8tAnvbWODAl\n\tYfLYlpC5vbzDjvsFKTdAg7qaldXrZiIsV3JePaww="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"hKlovF/Z\"; dkim-atps=neutral","Date":"Tue, 19 Apr 2022 13:58:09 +0300","To":"Han-Lin Chen <hanlinchen@chromium.org>","Message-ID":"<Yl6Vwe2amKu2bJSr@pendragon.ideasonboard.com>","References":"<20220418120923.453131-1-hanlinchen@chromium.org>\n\t<20220418120923.453131-3-hanlinchen@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220418120923.453131-3-hanlinchen@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v3 2/2] android: camera_hal_config:\n\tUse YamlParser to parse android HAL config","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]