[libcamera-devel,4/9] android: camera_hal_config: Use YamlParser to parse android hal config

15348 diff mbox series

Use YamlParser to parse android hal config files, instead of handling
yaml tokens directly, as a preparation for the further parameter extension.

Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
---
 src/android/camera_hal_config.cpp | 335 +++++++-----------------------
 1 file changed, 76 insertions(+), 259 deletions(-)

Hi Han-Lin,

Thank you for the patch.

On Wed, Feb 09, 2022 at 03:19:12PM +0800, Han-Lin Chen wrote:
> Use YamlParser to parse android hal config files, instead of handling

s/hal/HAL/

> yaml tokens directly, as a preparation for the further parameter extension.

s/yaml/YAML/

> 
> Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> ---
>  src/android/camera_hal_config.cpp | 335 +++++++-----------------------
>  1 file changed, 76 insertions(+), 259 deletions(-)

That's a nice diffstat :-)

The libyaml dependency should be dropped from src/android/meson.build,
or rather, added to src/libcamera/meson.build in patch 3/9.

> diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp
> index aa90dac7..54611956 100644
> --- a/src/android/camera_hal_config.cpp
> +++ b/src/android/camera_hal_config.cpp
> @@ -14,15 +14,15 @@ namespace filesystem = std::experimental::filesystem;
>  #else
>  #include <filesystem>
>  #endif
> -#include <stdio.h>
>  #include <stdlib.h>
>  #include <string>
> -#include <yaml.h>
>  
>  #include <hardware/camera3.h>
>  
>  #include <libcamera/base/log.h>
>  
> +#include <libcamera/internal/yaml_parser.h>
> +
>  using namespace libcamera;
>  
>  LOG_DEFINE_CATEGORY(HALConfig)
> @@ -37,307 +37,124 @@ public:
>  	int parseConfigFile(FILE *fh, std::map<std::string, CameraConfigData> *cameras);
>  
>  private:
> -	std::string parseValue();
> -	std::string parseKey();
> -	int parseValueBlock();
> -	int parseCameraLocation(CameraConfigData *cameraConfigData,
> -				const std::string &location);
> -	int parseCameraConfigData(const std::string &cameraId);
> -	int parseCameras();
> -	int parseEntry();
> -
> -	yaml_parser_t parser_;
> +	int parseCameraConfigData(const std::string &cameraId, const YamlObject &);
> +	int parseLocation(const YamlObject &, CameraConfigData &cameraConfigData);
> +	int parseRotation(const YamlObject &, CameraConfigData &cameraConfigData);
> +
>  	std::map<std::string, CameraConfigData> *cameras_;
> +	YamlParser yamlParser_;
>  };
>  
>  CameraHalConfig::Private::Private()
>  {
>  }
>  
> -std::string CameraHalConfig::Private::parseValue()
> +int CameraHalConfig::Private::parseConfigFile(FILE *fh,
> +					      std::map<std::string, CameraConfigData> *cameras)
>  {
> -	yaml_token_t token;
> -
> -	/* Make sure the token type is a value and get its content. */
> -	yaml_parser_scan(&parser_, &token);
> -	if (token.type != YAML_VALUE_TOKEN) {
> -		yaml_token_delete(&token);
> -		return "";
> -	}
> -	yaml_token_delete(&token);
> -
> -	yaml_parser_scan(&parser_, &token);
> -	if (token.type != YAML_SCALAR_TOKEN) {
> -		yaml_token_delete(&token);
> -		return "";
> -	}
> -
> -	std::string value(reinterpret_cast<char *>(token.data.scalar.value),
> -			  token.data.scalar.length);
> -	yaml_token_delete(&token);
> -
> -	return value;
> -}
> +	/*
> +	 * Parse the HAL properties.
> +	 *
> +	 * Each camera properties block is a list of properties associated
> +	 * with the ID (as assembled by CameraSensor::generateId()) of the
> +	 * camera they refer to.
> +	 *
> +	 * cameras:
> +	 *   "camera0 id":
> +	 *     location: value
> +	 *     rotation: value
> +	 *     ...
> +	 *
> +	 *   "camera1 id":
> +	 *     location: value
> +	 *     rotation: value
> +	 *     ...
> +	 */
>  
> -std::string CameraHalConfig::Private::parseKey()
> -{
> -	yaml_token_t token;
> +	cameras_ = cameras;
>  
> -	/* Make sure the token type is a key and get its value. */
> -	yaml_parser_scan(&parser_, &token);
> -	if (token.type != YAML_SCALAR_TOKEN) {
> -		yaml_token_delete(&token);
> -		return "";
> -	}
> +	YamlObject yamlObjectRoot;

You could name the variable yamlRoot (or just root) if you wanted to
shorten lines. Same for yamlObjectCameras, it could be yamlCameras.
Another option is rootObject and camerasObject, which would be
consistent with the parameter name of the parseCameraConfigData()
function.

> +	if (yamlParser_.ParseAsYamlObject(fh, yamlObjectRoot))
> +		return -EINVAL;
>  
> -	std::string value(reinterpret_cast<char *>(token.data.scalar.value),
> -			  token.data.scalar.length);
> -	yaml_token_delete(&token);
> +	if (!yamlObjectRoot.isDictionary())
> +		return -EINVAL;
>  
> -	return value;
> -}
> +	/* Parse property "cameras" */
> +	if (!yamlObjectRoot.isMember("cameras"))
> +		return -EINVAL;
>  
> -int CameraHalConfig::Private::parseValueBlock()
> -{
> -	yaml_token_t token;
> +	const YamlObject &yamlObjectCameras = yamlObjectRoot.get("cameras");
>  
> -	/* Make sure the next token are VALUE and BLOCK_MAPPING_START. */
> -	yaml_parser_scan(&parser_, &token);
> -	if (token.type != YAML_VALUE_TOKEN) {
> -		yaml_token_delete(&token);
> +	if (!yamlObjectCameras.isDictionary())
>  		return -EINVAL;
> -	}
> -	yaml_token_delete(&token);
>  
> -	yaml_parser_scan(&parser_, &token);
> -	if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) {
> -		yaml_token_delete(&token);
> -		return -EINVAL;
> +	std::vector<std::string> cameraIds = yamlObjectCameras.getMemberNames();
> +	for (const std::string &cameraId : cameraIds) {
> +		if (parseCameraConfigData(cameraId,
> +					  yamlObjectCameras.get(cameraId)))
> +			return -EINVAL;
>  	}
> -	yaml_token_delete(&token);
>  
>  	return 0;
>  }
>  
> -int CameraHalConfig::Private::parseCameraLocation(CameraConfigData *cameraConfigData,
> -						  const std::string &location)
> -{
> -	if (location == "front")
> -		cameraConfigData->facing = CAMERA_FACING_FRONT;
> -	else if (location == "back")
> -		cameraConfigData->facing = CAMERA_FACING_BACK;
> -	else
> -		return -EINVAL;
> -
> -	return 0;
> -}
> +int CameraHalConfig::Private::parseCameraConfigData(const std::string &cameraId,
> +						    const YamlObject &cameraObject)
>  
> -int CameraHalConfig::Private::parseCameraConfigData(const std::string &cameraId)
>  {
> -	int ret = parseValueBlock();
> -	if (ret)
> -		return ret;
> -
> -	/*
> -	 * Parse the camera properties and store them in a cameraConfigData
> -	 * instance.
> -	 *
> -	 * Add a safety counter to make sure we don't loop indefinitely in case
> -	 * the configuration file is malformed.
> -	 */
>  	CameraConfigData cameraConfigData;
> -	unsigned int sentinel = 100;
> -	bool blockEnd = false;
> -	yaml_token_t token;
> -
> -	do {
> -		yaml_parser_scan(&parser_, &token);
> -		switch (token.type) {
> -		case YAML_KEY_TOKEN: {
> -			yaml_token_delete(&token);
> -
> -			/*
> -			 * Parse the camera property key and make sure it is
> -			 * valid.
> -			 */
> -			std::string key = parseKey();
> -			std::string value = parseValue();
> -			if (key.empty() || value.empty())
> -				return -EINVAL;
> -
> -			if (key == "location") {
> -				ret = parseCameraLocation(&cameraConfigData, value);
> -				if (ret) {
> -					LOG(HALConfig, Error)
> -						<< "Unknown location: " << value;
> -					return -EINVAL;
> -				}
> -			} else if (key == "rotation") {
> -				ret = std::stoi(value);
> -				if (ret < 0 || ret >= 360) {
> -					LOG(HALConfig, Error)
> -						<< "Unknown rotation: " << value;
> -					return -EINVAL;
> -				}
> -				cameraConfigData.rotation = ret;
> -			} else {
> -				LOG(HALConfig, Error)
> -					<< "Unknown key: " << key;
> -				return -EINVAL;
> -			}
> -			break;
> -		}
> -
> -		case YAML_BLOCK_END_TOKEN:
> -			blockEnd = true;
> -			[[fallthrough]];
> -		default:
> -			yaml_token_delete(&token);
> -			break;
> -		}
> -
> -		--sentinel;
> -	} while (!blockEnd && sentinel);
> -	if (!sentinel)
> -		return -EINVAL;
>  
> -	(*cameras_)[cameraId] = cameraConfigData;
> +	if (!cameraObject.isDictionary())
> +		return -EINVAL;
>  
> -	return 0;
> -}
> +	/* Parse property "location" */
> +	if (parseLocation(cameraObject, cameraConfigData))
> +		return -EINVAL;
>  
> -int CameraHalConfig::Private::parseCameras()
> -{
> -	int ret = parseValueBlock();
> -	if (ret) {
> -		LOG(HALConfig, Error) << "Configuration file is not valid";
> -		return ret;
> -	}
> +	/* Parse property "rotation" */
> +	if (parseRotation(cameraObject, cameraConfigData))
> +		return -EINVAL;
>  
> -	/*
> -	 * Parse the camera properties.
> -	 *
> -	 * Each camera properties block is a list of properties associated
> -	 * with the ID (as assembled by CameraSensor::generateId()) of the
> -	 * camera they refer to.
> -	 *
> -	 * cameras:
> -	 *   "camera0 id":
> -	 *     key: value
> -	 *     key: value
> -	 *     ...
> -	 *
> -	 *   "camera1 id":
> -	 *     key: value
> -	 *     key: value
> -	 *     ...
> -	 */
> -	bool blockEnd = false;
> -	yaml_token_t token;
> -	do {
> -		yaml_parser_scan(&parser_, &token);
> -		switch (token.type) {
> -		case YAML_KEY_TOKEN: {
> -			yaml_token_delete(&token);
> -
> -			/* Parse the camera ID as key of the property list. */
> -			std::string cameraId = parseKey();
> -			if (cameraId.empty())
> -				return -EINVAL;
> -
> -			ret = parseCameraConfigData(cameraId);
> -			if (ret)
> -				return -EINVAL;
> -			break;
> -		}
> -		case YAML_BLOCK_END_TOKEN:
> -			blockEnd = true;
> -			[[fallthrough]];
> -		default:
> -			yaml_token_delete(&token);
> -			break;
> -		}
> -	} while (!blockEnd);
> +	(*cameras_)[cameraId] = cameraConfigData;

Shuffling this around, and writing

	if (!cameraObject.isDictionary())
		return -EINVAL;

	CameraConfigData &cameraConfigData = (*cameras_)[cameraId];

	/* Parse property "location" */
	if (parseLocation(cameraObject, cameraConfigData))
		return -EINVAL;

	/* Parse property "rotation" */
	if (parseRotation(cameraObject, cameraConfigData))
		return -EINVAL;

	return 0;

would avoid a copy when inserting in the map.

This otherwise looks very good to me, but I'll have a second look after
the patch is updated based on the changes to the YamlParser and
YamlObject API in the next version of 3/9.

The YAML parser could also be used to replace the dependency on boost in
the RPi IPA. Would you be able to submit a v2 of patches 3/9 and 4/9 as
a separate series while I continue reviewing the rest of this series ?
It would allow fast-tracking the parser.

>  
>  	return 0;
>  }
>  
> -int CameraHalConfig::Private::parseEntry()
> +int CameraHalConfig::Private::parseLocation(const YamlObject &cameraObject,
> +					    CameraConfigData &cameraConfigData)
>  {
> -	int ret = -EINVAL;
> -
> -	/*
> -	 * Parse each key we find in the file.
> -	 *
> -	 * The 'cameras' keys maps to a list of (lists) of camera properties.
> -	 */
> +	if (!cameraObject.isMember("location"))
> +		return -EINVAL;
>  
> -	std::string key = parseKey();
> -	if (key.empty())
> -		return ret;
> +	std::string location = cameraObject.get("location").asString();
>  
> -	if (key == "cameras")
> -		ret = parseCameras();
> +	if (location == "front")
> +		cameraConfigData.facing = CAMERA_FACING_FRONT;
> +	else if (location == "back")
> +		cameraConfigData.facing = CAMERA_FACING_BACK;
>  	else
> -		LOG(HALConfig, Error) << "Unknown key: " << key;
> +		return -EINVAL;
>  
> -	return ret;
> +	return 0;
>  }
>  
> -int CameraHalConfig::Private::parseConfigFile(FILE *fh,
> -					      std::map<std::string, CameraConfigData> *cameras)
> +int CameraHalConfig::Private::parseRotation(const YamlObject &cameraObject,
> +					    CameraConfigData &cameraConfigData)
>  {
> -	cameras_ = cameras;
> -
> -	int ret = yaml_parser_initialize(&parser_);
> -	if (!ret) {
> -		LOG(HALConfig, Error) << "Failed to initialize yaml parser";
> -		return -EINVAL;
> -	}
> -	yaml_parser_set_input_file(&parser_, fh);
> -
> -	yaml_token_t token;
> -	yaml_parser_scan(&parser_, &token);
> -	if (token.type != YAML_STREAM_START_TOKEN) {
> -		LOG(HALConfig, Error) << "Configuration file is not valid";
> -		yaml_token_delete(&token);
> -		yaml_parser_delete(&parser_);
> +	if (!cameraObject.isMember("rotation"))
>  		return -EINVAL;
> -	}
> -	yaml_token_delete(&token);
>  
> -	yaml_parser_scan(&parser_, &token);
> -	if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) {
> -		LOG(HALConfig, Error) << "Configuration file is not valid";
> -		yaml_token_delete(&token);
> -		yaml_parser_delete(&parser_);
> +	int32_t rotation = cameraObject.get("rotation").asInt32();
> +
> +	if (rotation < 0 || rotation >= 360) {
> +		LOG(HALConfig, Error)
> +			<< "Unknown rotation: " << rotation;
>  		return -EINVAL;
>  	}
> -	yaml_token_delete(&token);
> -
> -	/* Parse the file and parse each single key one by one. */
> -	do {
> -		yaml_parser_scan(&parser_, &token);
> -		switch (token.type) {
> -		case YAML_KEY_TOKEN:
> -			yaml_token_delete(&token);
> -			ret = parseEntry();
> -			break;
> -
> -		case YAML_STREAM_END_TOKEN:
> -			ret = -ENOENT;
> -			[[fallthrough]];
> -		default:
> -			yaml_token_delete(&token);
> -			break;
> -		}
> -	} while (ret >= 0);
> -	yaml_parser_delete(&parser_);
> -
> -	if (ret && ret != -ENOENT)
> -		LOG(HALConfig, Error) << "Configuration file is not valid";
> -
> -	return ret == -ENOENT ? 0 : ret;
> +
> +	cameraConfigData.rotation = rotation;
> +	return 0;
>  }
>  
>  CameraHalConfig::CameraHalConfig()