[{"id":22596,"web_url":"https://patchwork.libcamera.org/comment/22596/","msgid":"","date":"2022-04-06T00:37:31","subject":"Re: [libcamera-devel] [PATCH 4/9] android: camera_hal_config: Use\n\tYamlParser 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 Wed, Feb 09, 2022 at 03:19:12PM +0800, Han-Lin Chen wrote:\n> Use YamlParser to parse android hal config files, instead of handling\n\ns/hal/HAL/\n\n> yaml tokens directly, as a preparation for the further parameter extension.\n\ns/yaml/YAML/\n\n> \n> Signed-off-by: Han-Lin Chen \n> ---\n> src/android/camera_hal_config.cpp | 335 +++++++-----------------------\n> 1 file changed, 76 insertions(+), 259 deletions(-)\n\nThat's a nice diffstat :-)\n\nThe libyaml dependency should be dropped from src/android/meson.build,\nor rather, added to src/libcamera/meson.build in patch 3/9.\n\n> diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp\n> index aa90dac7..54611956 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 \n> #endif\n> -#include \n> #include \n> #include \n> -#include \n> \n> #include \n> \n> #include \n> \n> +#include \n> +\n> using namespace libcamera;\n> \n> LOG_DEFINE_CATEGORY(HALConfig)\n> @@ -37,307 +37,124 @@ public:\n> \tint parseConfigFile(FILE *fh, std::map *cameras);\n> \n> private:\n> -\tstd::string parseValue();\n> -\tstd::string parseKey();\n> -\tint parseValueBlock();\n> -\tint parseCameraLocation(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 *cameras_;\n> +\tYamlParser yamlParser_;\n> };\n> \n> 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 *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(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> +\tYamlObject yamlObjectRoot;\n\nYou could name the variable yamlRoot (or just root) if you wanted to\nshorten lines. Same for yamlObjectCameras, it could be yamlCameras.\nAnother option is rootObject and camerasObject, which would be\nconsistent with the parameter name of the parseCameraConfigData()\nfunction.\n\n> +\tif (yamlParser_.ParseAsYamlObject(fh, yamlObjectRoot))\n> +\t\treturn -EINVAL;\n> \n> -\tstd::string value(reinterpret_cast(token.data.scalar.value),\n> -\t\t\t token.data.scalar.length);\n> -\tyaml_token_delete(&token);\n> +\tif (!yamlObjectRoot.isDictionary())\n> +\t\treturn -EINVAL;\n> \n> -\treturn value;\n> -}\n> +\t/* Parse property \"cameras\" */\n> +\tif (!yamlObjectRoot.isMember(\"cameras\"))\n> +\t\treturn -EINVAL;\n> \n> -int CameraHalConfig::Private::parseValueBlock()\n> -{\n> -\tyaml_token_t token;\n> +\tconst YamlObject &yamlObjectCameras = yamlObjectRoot.get(\"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 cameraIds = yamlObjectCameras.getMemberNames();\n> +\tfor (const std::string &cameraId : cameraIds) {\n> +\t\tif (parseCameraConfigData(cameraId,\n> +\t\t\t\t\t yamlObjectCameras.get(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> -\n> -\treturn 0;\n> -}\n> +int CameraHalConfig::Private::parseCameraConfigData(const std::string &cameraId,\n> +\t\t\t\t\t\t const YamlObject &cameraObject)\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> -\t\treturn -EINVAL;\n> \n> -\t(*cameras_)[cameraId] = cameraConfigData;\n> +\tif (!cameraObject.isDictionary())\n> +\t\treturn -EINVAL;\n> \n> -\treturn 0;\n> -}\n> +\t/* Parse property \"location\" */\n> +\tif (parseLocation(cameraObject, cameraConfigData))\n> +\t\treturn -EINVAL;\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 \"rotation\" */\n> +\tif (parseRotation(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(*cameras_)[cameraId] = cameraConfigData;\n\nShuffling this around, and writing\n\n\tif (!cameraObject.isDictionary())\n\t\treturn -EINVAL;\n\n\tCameraConfigData &cameraConfigData = (*cameras_)[cameraId];\n\n\t/* Parse property \"location\" */\n\tif (parseLocation(cameraObject, cameraConfigData))\n\t\treturn -EINVAL;\n\n\t/* Parse property \"rotation\" */\n\tif (parseRotation(cameraObject, cameraConfigData))\n\t\treturn -EINVAL;\n\n\treturn 0;\n\nwould avoid a copy when inserting in the map.\n\nThis otherwise looks very good to me, but I'll have a second look after\nthe patch is updated based on the changes to the YamlParser and\nYamlObject API in the next version of 3/9.\n\nThe YAML parser could also be used to replace the dependency on boost in\nthe RPi IPA. Would you be able to submit a v2 of patches 3/9 and 4/9 as\na separate series while I continue reviewing the rest of this series ?\nIt would allow fast-tracking the parser.\n\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> -\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> +\tif (!cameraObject.isMember(\"location\"))\n> +\t\treturn -EINVAL;\n> \n> -\tstd::string key = parseKey();\n> -\tif (key.empty())\n> -\t\treturn ret;\n> +\tstd::string location = cameraObject.get(\"location\").asString();\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 *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.isMember(\"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.get(\"rotation\").asInt32();\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()","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 AE7FEC0F1B\n\tfor ;\n\tWed, 6 Apr 2022 00:37:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 19A9465642;\n\tWed, 6 Apr 2022 02:37:37 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B8AA3633A5\n\tfor ;\n\tWed, 6 Apr 2022 02:37:35 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CCB08482;\n\tWed, 6 Apr 2022 02:37:34 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649205457;\n\tbh=DBZglXeGpff9gx4WthAjUd9ipBSHZc7eOWXrZdc/jPM=;\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=E9IUfz6b1pvQMi9mziHrHKF6hUrhPWkqWv/X5479uRs7t1CVhW5VvgCVMORA4z4tE\n\t5ahaa/MS2bbel6+vHhDpKLdOZQrdJh114qfAE4uQVlhJLg+8DPX+A/9sxwzMywqbma\n\tAxf1mLqxgcvQrufdzVr3D7e/viOX8ZskLJUb/TumPro9JeWCsvi2QyRfp2RDL+ZXEY\n\tXtXxHpBvgMmZbaE7tmYvJtYoi5Vuf51f1nu7kkeIt6cDNJzPDumwkr1baLP3So0iu2\n\tjrtz0bp5Im+LJ32/DDNOia5k4WoojNngIWIWxGCcSgn9kwQXdKSbjbe6pbR315QSkA\n\tPb32j1oYxIP0A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649205454;\n\tbh=DBZglXeGpff9gx4WthAjUd9ipBSHZc7eOWXrZdc/jPM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=BXabl6y4OSJhJJSV2q5xbqfaKAGIbOvnTWro5fSTfQAeOeneIkAvUo+6YQxFhffNV\n\t/zNodGHD4AMN7VriXXDhTqMqP3dBmoX6N2vRL6IGemkL1OAZ8AXr/isVuj1QN/3kEw\n\t42xgqLwE4qjpElqkVgQayDMs58alHUG57T8HDpM0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"BXabl6y4\"; dkim-atps=neutral","Date":"Wed, 6 Apr 2022 03:37:31 +0300","To":"Han-Lin Chen ","Message-ID":"","References":"<20220209071917.559993-1-hanlinchen@chromium.org>\n\t<20220209071917.559993-5-hanlinchen@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220209071917.559993-5-hanlinchen@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH 4/9] android: camera_hal_config: Use\n\tYamlParser to parse android hal config","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]