[libcamera-devel] [PATCH 4/7] android: Add CameraHalConfig class

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Mar 24, 2021 at 12:25:24PM +0100, Jacopo Mondi wrote:
> Add a CameraHalConfig class to the Android Camera3 HAL layer.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/android/camera_hal_config.cpp | 385 ++++++++++++++++++++++++++++++
>  src/android/camera_hal_config.h   |  54 +++++
>  src/android/meson.build           |   1 +
>  3 files changed, 440 insertions(+)
>  create mode 100644 src/android/camera_hal_config.cpp
>  create mode 100644 src/android/camera_hal_config.h
> 
> diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp
> new file mode 100644
> index 000000000000..7e33dfb750ff
> --- /dev/null
> +++ b/src/android/camera_hal_config.cpp
> @@ -0,0 +1,385 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * camera_hal_config.cpp - Camera HAL configuration file manager
> + */
> +#include "camera_hal_config.h"
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +
> +#include "libcamera/internal/file.h"
> +#include "libcamera/internal/log.h"
> +
> +using namespace libcamera;
> +
> +LOG_DEFINE_CATEGORY(HALConfig)
> +
> +const std::string CameraHalConfig::CameraBlock::toString() const
> +{
> +	std::stringstream ss;
> +
> +	ss << "\'" << name << "\'"
> +	   << " model: " << model
> +	   << "(" << location << ")[" << rotation << "]";
> +
> +	return ss.str();
> +}

As the amount of information will grow, I don't think we'll be able to
print everything. This function is only used in a single location below,
for debugging purpose, I wonder if we should keep it.

> +
> +CameraHalConfig::CameraHalConfig()
> +{
> +	if (!yaml_parser_initialize(&parser_))
> +		LOG(HALConfig, Error) << "Failed to initialize yaml parser";
> +}
> +
> +CameraHalConfig::~CameraHalConfig()
> +{
> +	yaml_parser_delete(&parser_);
> +}
> +
> +/*
> + * Configuration files can be stored in system paths, which are identified
> + * through the build configuration.
> + *
> + * However, when running uninstalled - the source location takes precedence.

Can we run the camera HAL uninstalled ? :-)

> + */
> +const std::string CameraHalConfig::findFilePath(const std::string &filename)
> +{
> +	static std::array<std::string, 2> searchPaths = {
> +		LIBCAMERA_SYSCONF_DIR,
> +		LIBCAMERA_DATA_DIR,

The data dir may not be very useful here, the configuration file is
really system-specific, right ?

I expect we may need to get the configuration file from system-specific
locations, depending on whether we run on Chrome OS or Android, but we
can handle that later.

> +	};
> +
> +	if (File::exists(filename))
> +		return filename;
> +
> +	std::string root = utils::libcameraSourcePath();
> +	if (!root.empty()) {
> +		std::string configurationPath = root + "data/" + filename;
> +
> +		if (File::exists(configurationPath))
> +			return configurationPath;
> +	}
> +
> +	for (std::string &path : searchPaths) {
> +		std::string configurationPath = path + "/" + filename;
> +		if (File::exists(configurationPath))
> +			return configurationPath;
> +	}
> +
> +	return "";
> +}
> +
> +/*
> + * \brief Open the HAL configuration file and validate its content
> + * \return 0 on success, a negative error code otherwise
> + */
> +int CameraHalConfig::open()
> +{
> +	int ret;
> +
> +	const std::string configPath = "camera_hal.yaml";
> +	const std::string filePath = findFilePath(configPath);
> +	if (filePath.empty()) {
> +		LOG(HALConfig, Warning)

This can be an Error, as we can't continue.

> +			<< "File: \"" << configPath << "\" not found";

Maybe s/File:/Configuration file/ ?

> +		return -ENOENT;
> +	}
> +
> +	FILE *fh = fopen(filePath.c_str(), "r");
> +	if (!fh) {
> +		ret = -errno;
> +		LOG(HALConfig, Error) << "Failed to open file " << filePath

Same here, "configuration file" ?

> +				      << ": " << strerror(-ret);
> +		return ret;
> +	}
> +
> +	yaml_parser_set_input_file(&parser_, fh);
> +
> +	LOG(HALConfig, Debug) << "Reading configuration from " << filePath;
> +
> +	ret = parseConfigFile();
> +	fclose(fh);
> +	if (ret)
> +		return ret;
> +
> +	LOG(HALConfig, Info) << "Device model: " << model_;
> +	LOG(HALConfig, Info) << "Device maker: " << maker_;
> +	for (const auto &c : cameras_) {
> +		const CameraBlock camera = c.second;
> +		LOG(HALConfig, Info) << camera.toString();
> +	}

I wonder if dumping the parsed file shouldn't be a debugging feature
instead, or even if we need it at all.

> +
> +	return 0;
> +}
> +
> +const std::string &CameraHalConfig::cameraLocation(const std::string &camera) const
> +{
> +	static const std::string empty("");
> +	const auto &it = cameras_.find(camera);
> +	if (it == cameras_.end()) {
> +		LOG(HALConfig, Error)
> +			<< "Camera '" << camera
> +			<< "' not described in the HAL configuration file";
> +		return empty;
> +	}
> +
> +	const CameraBlock &cam = it->second;
> +	return cam.location;
> +}
> +
> +unsigned int CameraHalConfig::cameraRotation(const std::string &camera) const
> +{
> +	static const std::string empty("");
> +	const auto &it = cameras_.find(camera);
> +	if (it == cameras_.end()) {
> +		LOG(HALConfig, Error)
> +			<< "Camera '" << camera
> +			<< "' not described in the HAL configuration file";
> +		return 0;
> +	}
> +
> +	const CameraBlock &cam = it->second;
> +	return cam.rotation;
> +}
> +
> +const std::string &CameraHalConfig::cameraModel(const std::string &camera) const
> +{
> +	static const std::string empty("");
> +	const auto &it = cameras_.find(camera);
> +	if (it == cameras_.end()) {
> +		LOG(HALConfig, Error)
> +			<< "Camera '" << camera
> +			<< "' not described in the HAL configuration file";
> +		return empty;
> +	}
> +
> +	const CameraBlock &cam = it->second;
> +	return cam.model;
> +}
> +
> +std::string CameraHalConfig::parseValue(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) {
> +		LOG(HALConfig, Error) << "Configuration file is not valid";
> +		return "";
> +	}
> +	yaml_token_delete(&token);
> +
> +	yaml_parser_scan(&parser_, &token);
> +	if (token.type != YAML_SCALAR_TOKEN) {
> +		LOG(HALConfig, Error) << "Configuration file is not valid";
> +		return "";
> +	}
> +
> +	char *scalar = reinterpret_cast<char *>(token.data.scalar.value);
> +	std::string value(scalar, token.data.scalar.length);
> +	yaml_token_delete(&token);
> +
> +	return value;
> +}
> +
> +std::string CameraHalConfig::parseKey(yaml_token_t &token)
> +{
> +	/* Make sure the token type is a key and get its value. */
> +	yaml_parser_scan(&parser_, &token);
> +	if (token.type != YAML_KEY_TOKEN) {
> +		LOG(HALConfig, Error) << "Configuration file is not valid";
> +		return "";
> +	}
> +	yaml_token_delete(&token);
> +
> +	yaml_parser_scan(&parser_, &token);
> +	if (token.type != YAML_SCALAR_TOKEN) {
> +		LOG(HALConfig, Error) << "Configuration file is not valid";
> +		return "";
> +	}
> +
> +	char *scalar = reinterpret_cast<char *>(token.data.scalar.value);
> +	std::string key(scalar, token.data.scalar.length);
> +	yaml_token_delete(&token);
> +
> +	return key;
> +}
> +
> +int CameraHalConfig::parseCameraBlock(yaml_token_t &token)
> +{
> +	/* The 'camera' block is a VALUE token type. */
> +	yaml_parser_scan(&parser_, &token);
> +	if (token.type != YAML_VALUE_TOKEN) {
> +		LOG(HALConfig, Error) << "Configuration file is not valid";
> +		return -EINVAL;
> +	}
> +	yaml_token_delete(&token);
> +
> +	/*
> +	 * Parse the content of the camera block until we have an unbalanced
> +	 * BLOCK_END_TOKEN which closes the camera block.
> +	 *
> +	 * Add a safety counter to make sure we don't loop indefinitely in case
> +	 * the configuration file is malformed.
> +	 */
> +	unsigned int sentinel = 100;
> +	CameraBlock cameraBlock{};
> +	int blockCounter = 0;
> +	do {
> +		yaml_parser_scan(&parser_, &token);
> +		switch (token.type) {
> +		case YAML_BLOCK_ENTRY_TOKEN:
> +			yaml_token_delete(&token);
> +			blockCounter++;
> +			break;
> +		case YAML_BLOCK_END_TOKEN:
> +			yaml_token_delete(&token);
> +			blockCounter--;
> +			break;
> +		case YAML_BLOCK_MAPPING_START_TOKEN: {
> +			yaml_token_delete(&token);
> +
> +			std::string key = parseKey(token);
> +			std::string value = parseValue(token);
> +			if (key.empty() || value.empty()) {
> +				LOG(HALConfig, Error)
> +					<< "Configuration file is not valid";
> +				return -EINVAL;
> +			}
> +
> +			/* Validate that the parsed key is valid. */
> +			if (key == "location") {
> +				if (value != "front" && value != "back" &&
> +				    value != "external") {
> +					LOG(HALConfig, Error)
> +						<< "Unknown location: " << value;
> +					return -EINVAL;
> +				}
> +				cameraBlock.location = value;
> +			} else if (key == "rotation") {
> +				cameraBlock.rotation = strtoul(value.c_str(),
> +							       NULL, 10);
> +				if (cameraBlock.rotation < 0) {
> +					LOG(HALConfig, Error)
> +						<< "Unknown rotation: "
> +						<< cameraBlock.rotation;
> +					return -EINVAL;
> +				}
> +			} else if (key == "name") {
> +				cameraBlock.name = value;
> +			} else if (key == "model") {
> +				cameraBlock.model = value;
> +			} else {
> +				LOG(HALConfig, Error)
> +					<< "Configuration file is not valid "
> +					<< "unknown key: " << key;
> +				return -EINVAL;
> +			}
> +			break;
> +		}
> +		default:
> +			yaml_token_delete(&token);
> +			break;
> +		}
> +		--sentinel;
> +	} while (blockCounter >= 0 && sentinel);
> +	if (!sentinel) {
> +		LOG(HALConfig, Error) << "Configuration file is not valid ";
> +		return -EINVAL;
> +	}
> +
> +	cameras_[cameraBlock.name] = cameraBlock;
> +
> +	return 0;
> +}
> +
> +int CameraHalConfig::parseEntryBlock(yaml_token_t &token)
> +{
> +	int ret;
> +
> +	yaml_parser_scan(&parser_, &token);
> +	if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) {
> +		LOG(HALConfig, Error) << "Configuration file is not valid";
> +		return -EINVAL;
> +	}
> +	yaml_token_delete(&token);
> +
> +	std::string key = parseKey(token);
> +	if (key.empty()) {
> +		LOG(HALConfig, Error) << "Configuration file is not valid";
> +		return -EINVAL;
> +	}
> +
> +	if (key == "camera") {
> +		yaml_token_delete(&token);
> +		ret = parseCameraBlock(token);
> +		if (ret)
> +			return ret;
> +	} else if (key == "manufacturer") {
> +		yaml_token_delete(&token);
> +		maker_ = parseValue(token);
> +		if (maker_.empty()) {
> +			LOG(HALConfig, Error) << "Configuration file is not valid";
> +			return -EINVAL;
> +		}
> +	} else if (key == "model") {
> +		yaml_token_delete(&token);
> +		model_ = parseValue(token);
> +		if (model_.empty()) {
> +			LOG(HALConfig, Error) << "Configuration file is not valid";
> +			return -EINVAL;
> +		}
> +	} else {
> +		yaml_token_delete(&token);
> +		LOG(HALConfig, Error) << "Unknown key " << key;
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +int CameraHalConfig::parseConfigFile()
> +{
> +	yaml_token_t token;
> +	int ret;
> +
> +	yaml_parser_scan(&parser_, &token);

Just to make sure you're aware of it, there's yaml_parser_load() that
produces a parsed document that can then be walked. There's nothing
wrong about using event-based parsing as you do though.

I've skipped review of the parser implementation itself, as it may be
changed depending on how patch 7/7 evolves.

> +	if (token.type != YAML_STREAM_START_TOKEN) {
> +		LOG(HALConfig, Error) << "Configuration file is not valid";
> +		return EINVAL;

-EINVAL

> +	}
> +	yaml_token_delete(&token);
> +
> +	/*
> +	 * Start parsing the content of the configuration file which
> +	 * starts as with sequence block.

s/as // ?

> +	 */
> +	yaml_parser_scan(&parser_, &token);
> +	if (token.type != YAML_BLOCK_SEQUENCE_START_TOKEN) {
> +		LOG(HALConfig, Error) << "Configuration file is not valid";
> +		return -EINVAL;
> +	}
> +	yaml_token_delete(&token);
> +
> +	/* Parse the single entry blocks. */
> +	do {
> +		yaml_parser_scan(&parser_, &token);
> +		switch (token.type) {
> +		case YAML_BLOCK_ENTRY_TOKEN:
> +			yaml_token_delete(&token);
> +			ret = parseEntryBlock(token);
> +			break;
> +		case YAML_STREAM_END_TOKEN:
> +			/* Resources are released after the loop exit. */
> +			break;
> +		default:
> +			/* Release resources before re-parsing. */
> +			yaml_token_delete(&token);
> +			break;
> +		}
> +	} while (token.type != YAML_STREAM_END_TOKEN && ret >= 0);
> +	yaml_token_delete(&token);
> +
> +	return ret;
> +}
> diff --git a/src/android/camera_hal_config.h b/src/android/camera_hal_config.h
> new file mode 100644
> index 000000000000..69d42658e90a
> --- /dev/null
> +++ b/src/android/camera_hal_config.h
> @@ -0,0 +1,54 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * camera_hal_config.h - Camera HAL configuration file manager
> + */
> +#ifndef __ANDROID_CAMERA_HAL_CONFIG_H__
> +#define __ANDROID_CAMERA_HAL_CONFIG_H__
> +
> +#include <map>
> +#include <string>
> +#include <yaml.h>

It would be nice to hide the YAML perser from this header. It could be
done by moving the parsing to a CameraHalConfigParser class, private to
the camera_hal_config.cpp file.

> +
> +class CameraHalConfig
> +{
> +public:
> +	CameraHalConfig();
> +	~CameraHalConfig();
> +
> +	int open();
> +
> +	const std::string &deviceModel() const { return model_; }
> +	const std::string &deviceMaker() const { return maker_; }
> +
> +	const std::string &cameraLocation(const std::string &camera) const;
> +	unsigned int cameraRotation(const std::string &camera) const;
> +	const std::string &cameraModel(const std::string &camera) const;
> +
> +private:
> +	yaml_parser_t parser_;
> +
> +	std::string model_;
> +	std::string maker_;
> +	class CameraBlock
> +	{
> +	public:
> +		std::string name;
> +		std::string location;
> +		std::string model;
> +		unsigned int rotation;
> +
> +		const std::string toString() const;
> +	};

How about making this class public (and probably renaming it), and replacing the last three public
functions with

	const CameraBlock *cameraConfig(const std::string &name) const;

? That way each CameraDevice could store a reference to the
corresponding camera configuration only.

> +	std::map<std::string, CameraBlock> cameras_;
> +
> +	const std::string findFilePath(const std::string &filename);

No need for const at the beginning as you return by value, but you can
add a const at the end.

> +	std::string parseValue(yaml_token_t &token);
> +	std::string parseKey(yaml_token_t &token);
> +	int parseCameraBlock(yaml_token_t &token);
> +	int parseEntryBlock(yaml_token_t &token);
> +	int parseConfigFile();
> +};
> +
> +#endif /* __ANDROID_CAMERA_HAL_CONFIG_H__ */
> diff --git a/src/android/meson.build b/src/android/meson.build
> index 19f94a4073f1..34e5c494a492 100644
> --- a/src/android/meson.build
> +++ b/src/android/meson.build
> @@ -44,6 +44,7 @@ android_hal_sources = files([
>      'camera3_hal.cpp',
>      'camera_hal_manager.cpp',
>      'camera_device.cpp',
> +    'camera_hal_config.cpp',
>      'camera_metadata.cpp',
>      'camera_ops.cpp',
>      'camera_stream.cpp',

-- 
Regards,

Laurent Pinchart