[libcamera-devel] [PATCH v2 3/6] android: Add CameraHalConfig class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Mar 30 02:39:56 CEST 2021
Hi Jacopo,
Thank you for the patch.
On Mon, Mar 29, 2021 at 05:28:04PM +0200, Jacopo Mondi wrote:
> Add a CameraHalConfig class to the Android Camera3 HAL layer.
There are quite a few comments from v1 that haven't been addressed.
Maybe that's because my last reply to v1 came after you've sent this
series. The comments may also not all be correct, applicable and/or
desired, but could you then reply to them to explain why they're ignored
?
In particular, I think that exposing CameraProps would simplify the
implementation quite a bit, by removing the need for the camera*()
functions, with the lookup of the CameraProps entry every time, and the
associated error handling.
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> README.rst | 2 +-
> src/android/camera_hal_config.cpp | 463 ++++++++++++++++++++++++++++++
> src/android/camera_hal_config.h | 53 ++++
> src/android/meson.build | 2 +
> 4 files changed, 519 insertions(+), 1 deletion(-)
> create mode 100644 src/android/camera_hal_config.cpp
> create mode 100644 src/android/camera_hal_config.h
>
> diff --git a/README.rst b/README.rst
> index 7a98164bbb0a..f68d435196de 100644
> --- a/README.rst
> +++ b/README.rst
> @@ -88,7 +88,7 @@ for tracing with lttng: [optional]
> liblttng-ust-dev python3-jinja2 lttng-tools
>
> for android: [optional]
> - libexif libjpeg
> + libexif libjpeg libyaml
>
> Using GStreamer plugin
> ~~~~~~~~~~~~~~~~~~~~~~
> diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp
> new file mode 100644
> index 000000000000..846dd7357b0a
> --- /dev/null
> +++ b/src/android/camera_hal_config.cpp
> @@ -0,0 +1,463 @@
> +/* 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 <hardware/camera3.h>
> +
> +#include "libcamera/internal/file.h"
> +#include "libcamera/internal/log.h"
> +
> +using namespace libcamera;
> +
> +LOG_DEFINE_CATEGORY(HALConfig)
> +
> +CameraHalConfig::CameraHalConfig()
> +{
> + if (!yaml_parser_initialize(&parser_))
> + LOG(HALConfig, Fatal) << "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.
> + */
> +std::string CameraHalConfig::findFilePath(const std::string &filename) const
> +{
> + static std::array<std::string, 2> searchPaths = {
> + LIBCAMERA_SYSCONF_DIR,
> + LIBCAMERA_DATA_DIR,
There's also a comment in v1 about data dir. Data dir is typically
/usr/share/libcamera/. Given that the configuration file is
device-specific, I don't think we'll ever look for it there, but only in
sysconf dir.
> + };
> +
> + 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 "";
> +}
> +
> +FILE *CameraHalConfig::openFile(const std::string &filename)
> +{
> + const std::string filePath = findFilePath(filename);
> + if (filePath.empty()) {
> + LOG(HALConfig, Error)
> + << "Configuration file: \"" << filename << "\" not found";
> + return nullptr;
> + }
> +
> + LOG(HALConfig, Debug) << "Reading configuration file from " << filePath;
> +
> + FILE *fh = fopen(filePath.c_str(), "r");
> + if (!fh) {
> + int ret = -errno;
> + LOG(HALConfig, Error) << "Failed to open configuration file "
> + << filePath << ": " << strerror(-ret);
> + return nullptr;
> + }
> +
> + return fh;
> +}
> +
> +/*
> + * Open the HAL configuration file and validate its content.
> + * Return 0 on success, a negative error code otherwise
> + */
> +int CameraHalConfig::open()
> +{
> + int ret;
> +
> + FILE *fh = openFile("camera_hal.yaml");
> + if (!fh)
> + return -ENOENT;
> +
> + yaml_parser_set_input_file(&parser_, fh);
> + 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 std::string &cameraId = c.first;
> + const CameraProps &camera = c.second;
> + LOG(HALConfig, Info) << "'" << cameraId << "' "
> + << " model: " << camera.model
> + << "(" << camera.location << ")["
> + << camera.rotation << "]";
> + }
There's also a comment in v1 about whether this should be a debugging
feature.
> +
> + return 0;
> +}
> +
> +int CameraHalConfig::cameraLocation(const std::string &camera) const
> +{
> + const auto &it = cameras_.find(camera);
> + if (it == cameras_.end()) {
> + LOG(HALConfig, Error)
> + << "Camera '" << camera
> + << "' not described in the HAL configuration file";
> + return -EINVAL;
> + }
> +
> + const CameraProps &cam = it->second;
> + if (cam.location.empty()) {
> + LOG(HALConfig, Error) << "Location for camera '" << camera
> + << "' not available";
> + return -EINVAL;
> + }
> +
> + if (cam.location == "front")
> + return CAMERA_FACING_FRONT;
> + else if (cam.location == "back")
> + return CAMERA_FACING_BACK;
> + else if (cam.location == "external")
> + return CAMERA_FACING_EXTERNAL;
> +
> + LOG(HALConfig, Error) << "Unsupported camera location " << cam.location;
> + return -EINVAL;
Can we do this conversion when parsing the configuration file instead ?
That will ensure at parse time that the data is valid, instead of
delaying the check to later. The rotation is already handled that way,
and you already check that the location string is valid in
CameraHalConfig::parseCameraProps().
> +}
> +
> +unsigned int CameraHalConfig::cameraRotation(const std::string &camera) const
> +{
> + 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 CameraProps &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 CameraProps &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);
Does the token need to be deleted in the error cases above and below ?
> +
> + 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_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::parseCameraProps(yaml_token_t &token,
> + const std::string &cameraID)
> +{
> + 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);
> +
> + 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);
> +
> + /*
> + * Parse the camera properties and store them in a cameraBlock instance.
> + *
> + * Add a safety counter to make sure we don't loop indefinitely in case
> + * the configuration file is malformed.
> + */
> + unsigned int sentinel = 100;
> + CameraProps cameraBlock{};
> + bool blockEnd = false;
> + 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(token);
> + std::string value = parseValue(token);
> + if (key.empty() || value.empty()) {
> + LOG(HALConfig, Error)
> + << "Configuration file is not valid";
> + return -EINVAL;
> + }
> +
> + 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 == "model") {
> + cameraBlock.model = value;
> + } else {
> + LOG(HALConfig, Error)
> + << "Configuration file is not valid "
> + << "unknown key: " << key;
> + return -EINVAL;
> + }
> + break;
> + }
> + case YAML_BLOCK_END_TOKEN:
> + yaml_token_delete(&token);
> + blockEnd = true;
> + break;
> + default:
> + yaml_token_delete(&token);
> + break;
> + }
> +
> + --sentinel;
> + } while (!blockEnd && sentinel);
Missing blank line.
> + if (!sentinel) {
> + LOG(HALConfig, Error) << "Configuration file is not valid ";
> + return -EINVAL;
> + }
> +
> + cameras_[cameraID] = cameraBlock;
> +
> + return 0;
> +}
> +
> +int CameraHalConfig::parseCameras(yaml_token_t &token)
> +{
> + int ret;
> +
> + /* The 'cameras' key maps a BLOCK_MAPPING_START block. */
> + 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);
> +
> + 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);
> +
> + /*
> + * 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;
> + 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(token);
> + if (cameraId.empty()) {
> + LOG(HALConfig, Error)
> + << "Configuration file is not valid";
> + return -EINVAL;
> + }
> +
> + ret = parseCameraProps(token, cameraId);
> + if (ret) {
> + LOG(HALConfig, Error)
> + << "Configuration file is not valid";
> + return -EINVAL;
> + }
> + break;
> + }
> + case YAML_BLOCK_END_TOKEN:
> + yaml_token_delete(&token);
> + blockEnd = true;
> + break;
> + default:
> + yaml_token_delete(&token);
> + LOG(HALConfig, Error)
> + << "Configuration file is not valid";
> + return -EINVAL;
> + }
> + } while (!blockEnd);
> +
> + return 0;
> +}
> +
> +int CameraHalConfig::parseEntry(yaml_token_t &token)
> +{
> + int ret;
> +
> + /*
> + * Parse each key we find in the file.
> + *
> + * Keys like 'model' and 'maker' are device properties and gets
> + * stored as class members.
> + *
> + * The 'cameras' keys maps to a list of (lists) of camera properties.
> + */
> +
> + std::string key = parseKey(token);
> + if (key.empty()) {
> + LOG(HALConfig, Error) << "Configuration file is not valid";
> + return -EINVAL;
> + }
> +
> + if (key == "cameras") {
> + ret = parseCameras(token);
> + if (ret)
> + return ret;
> + } else if (key == "manufacturer") {
> + maker_ = parseValue(token);
> + if (maker_.empty()) {
> + LOG(HALConfig, Error) << "Configuration file is not valid";
> + return -EINVAL;
> + }
> + } else if (key == "model") {
> + model_ = parseValue(token);
> + if (model_.empty()) {
> + LOG(HALConfig, Error) << "Configuration file is not valid";
> + return -EINVAL;
> + }
> + } else {
> + LOG(HALConfig, Error) << "Unknown key " << key;
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +int CameraHalConfig::parseConfigFile()
> +{
> + yaml_token_t token;
> + int ret;
> +
> + yaml_parser_scan(&parser_, &token);
> + if (token.type != YAML_STREAM_START_TOKEN) {
> + LOG(HALConfig, Error) << "Configuration file is not valid";
> + 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";
> + 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(token);
Do you need to pass the token to parseEntry(), given that you've just
deleted it ? It looks to me like the token is only the return value of
yaml_parse_scan(), and can thus be a local variable everywhere.
> + break;
> + case YAML_STREAM_END_TOKEN:
> + /* Resources are released after the loop exit. */
> + break;
> + default:
> + yaml_token_delete(&token);
> + break;
The YAML_STREAM_END_TOKEN and default case are different, but the
while() condition below accesses token.type. Is that intended ?
> + }
> + } 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..ce77b1ee1582
> --- /dev/null
> +++ b/src/android/camera_hal_config.h
> @@ -0,0 +1,53 @@
> +/* 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>
As commented in v1, hiding the parser would be nice, but it can be done
later, on top.
> +
> +class CameraHalConfig
> +{
> +public:
> + CameraHalConfig();
> + ~CameraHalConfig();
> +
> + int open();
> +
> + const std::string &deviceModel() const { return model_; }
> + const std::string &deviceMaker() const { return maker_; }
> +
> + int 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 CameraProps
> + {
> + public:
> + std::string location;
> + std::string model;
> + unsigned int rotation;
> + };
> + std::map<std::string, CameraProps> cameras_;
> +
> + std::string findFilePath(const std::string &filename) const;
> + FILE *openFile(const std::string &filename);
> + std::string parseValue(yaml_token_t &token);
> + std::string parseKey(yaml_token_t &token);
> + int parseCameraProps(yaml_token_t &token, const std::string &cameraID);
> + int parseCameras(yaml_token_t &token);
> + int parseEntry(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 8e7d07d9be3c..c0ede407bc38 100644
> --- a/src/android/meson.build
> +++ b/src/android/meson.build
> @@ -3,6 +3,7 @@
> android_deps = [
> dependency('libexif', required : get_option('android')),
> dependency('libjpeg', required : get_option('android')),
> + dependency('yaml-0.1', required : get_option('android')),
> ]
>
> android_enabled = true
> @@ -41,6 +42,7 @@ android_hal_sources = files([
> 'camera3_hal.cpp',
> 'camera_hal_manager.cpp',
> 'camera_device.cpp',
> + 'camera_hal_config.cpp',
While at it, could you move camera_hal_manager to the right place ?
> 'camera_metadata.cpp',
> 'camera_ops.cpp',
> 'camera_stream.cpp',
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list