[libcamera-devel] [PATCH 4/7] android: Add CameraHalConfig class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Mar 29 19:43:15 CEST 2021
Hi Jacopo,
On Mon, Mar 29, 2021 at 04:27:22PM +0200, Jacopo Mondi wrote:
> On Fri, Mar 26, 2021 at 06:07:07AM +0200, Laurent Pinchart wrote:
> > 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 ? :-)
> >
>
> This meant "libcamera" uninstalled.
Yes, but as this is the HAL configuration, I don't think we need to care
about the uninstalled case. But now that I think about it, maybe you
meant for this function to be ready for the libcamera core configuration
file too ?
> > > + */
> > > +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 found that feature under-documented with very few usage examples
> around.
Agreed, it's underdocumented in libyaml :-( I've found
http://codepad.org/W7StVSkV that shows how to walk a document. If you
think that would be simpler to use, feel free to do so, otherwise the
token-based parser can be kept.
> > 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.
>
> .. ok
>
> > > +
> > > +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.
>
> I prefer the CameraHalConfig::cameraProperty() interface but ok
I wouldn't be surprised if we had to create a deeper hierarchy of
properties in YAML, in which case cameraProperty() wouldn't scale well
(there's also the lookup every time a property is accessed that we could
save).
> > > + 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
More information about the libcamera-devel
mailing list