[libcamera-devel] [PATCH v3 3/5] android: Add CameraHalConfig class
Jacopo Mondi
jacopo at jmondi.org
Wed Mar 31 13:27:59 CEST 2021
Hi Hiro,
On Wed, Mar 31, 2021 at 06:30:34PM +0900, Hirokazu Honda wrote:
> Hi Jacopo, thanks for the patch.
>
> On Tue, Mar 30, 2021 at 11:20 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
> >
> > Add a CameraHalConfig class to the Android Camera3 HAL layer.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > README.rst | 2 +-
> > src/android/camera_hal_config.cpp | 407 ++++++++++++++++++++++++++++++
> > src/android/camera_hal_config.h | 36 +++
> > src/android/meson.build | 2 +
> > 4 files changed, 446 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..b109ad24e1d1
> > --- /dev/null
> > +++ b/src/android/camera_hal_config.cpp
> > @@ -0,0 +1,407 @@
> > +/* 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 <yaml.h>
> > +
> > +#include <hardware/camera3.h>
> > +
> > +#include "libcamera/internal/file.h"
> > +#include "libcamera/internal/log.h"
> > +
> > +using namespace libcamera;
> > +
> > +LOG_DEFINE_CATEGORY(HALConfig)
> > +
> > +namespace {
> > +
> > +std::map<std::string, CameraProps> cameras;
> > +
> > +std::string parseValue(yaml_parser_t *parser)
> > +{
> > + 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 "";
> > + }
> > +
> > + 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 parseKey(yaml_parser_t *parser)
> > +{
> > + 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) {
> > + yaml_token_delete(&token);
> > + 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 parseValueBlock(yaml_parser_t *parser)
> > +{
> > + yaml_token_t token;
> > +
> > + /* 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);
> > + 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;
> > + }
> > + yaml_token_delete(&token);
> > +
> > + return 0;
> > +}
> > +
> > +int parseCameraLocation(CameraProps *cameraProps, const std::string &location)
> > +{
> > + if (location == "front") {
> > + cameraProps->facing = CAMERA_FACING_FRONT;
> > + } else if (location == "back") {
> > + cameraProps->facing = CAMERA_FACING_BACK;
> > + } else if (location == "external") {
> > + cameraProps->facing = CAMERA_FACING_EXTERNAL;
> > + } else {
> > + cameraProps->facing = -EINVAL;
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +int parseCameraProps(yaml_parser_t *parser, const std::string &cameraID)
> > +{
> > + int ret = parseValueBlock(parser);
> > + if (ret)
> > + return ret;
> > +
> > + /*
> > + * Parse the camera properties and store them in a cameraProps 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 cameraProps{};
> > + 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(parser);
> > + std::string value = parseValue(parser);
> > + if (key.empty() || value.empty())
> > + return -EINVAL;
> > +
> > + if (key == "location") {
> > + ret = parseCameraLocation(&cameraProps, value);
> > + if (ret) {
> > + LOG(HALConfig, Error)
> > + << "Unupported location: "
> > + << value;
> > + return -EINVAL;
> > + }
> > + } else if (key == "rotation") {
> > + cameraProps.rotation = strtoul(value.c_str(),
> > + NULL, 10);
> > + if (cameraProps.rotation < 0) {
> > + LOG(HALConfig, Error)
> > + << "Unknown rotation: "
> > + << cameraProps.rotation;
> > + return -EINVAL;
> > + }
> > + } else if (key == "model") {
> > + cameraProps.model = value;
> > + } else {
> > + LOG(HALConfig, Error)
> > + << "Unknown key: " << key;
> > + return -EINVAL;
> > + }
> > + break;
> > + }
> > + case YAML_BLOCK_END_TOKEN:
> > + blockEnd = true;
> > + /* Fall-through */
> > + default:
> > + yaml_token_delete(&token);
> > + break;
> > + }
> > +
> > + --sentinel;
> > + } while (!blockEnd && sentinel);
> > + if (!sentinel)
> > + return -EINVAL;
> > +
> > + cameraProps.valid = true;
> > + cameras[cameraID] = cameraProps;
> > +
> > + return 0;
> > +}
> > +
> > +int parseCameras(yaml_parser_t *parser)
> > +{
> > + int ret = parseValueBlock(parser);
> > + if (ret) {
> > + LOG(HALConfig, Error) << "Configuration file is not valid";
> > + return ret;
> > + }
> > +
> > + /*
> > + * 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(parser);
> > + if (cameraId.empty())
> > + return -EINVAL;
> > +
> > + ret = parseCameraProps(parser, cameraId);
> > + if (ret)
> > + return -EINVAL;
> > + break;
> > + }
> > + case YAML_BLOCK_END_TOKEN:
> > + blockEnd = true;
> > + /* Fall-through */
> > + default:
> > + yaml_token_delete(&token);
> > + break;
> > + }
> > + } while (!blockEnd);
> > +
> > + return 0;
> > +}
> > +
> > +int parseEntry(yaml_parser_t *parser)
> > +{
> > + int ret = -EINVAL;
> > +
> > + /*
> > + * Parse each key we find in the file.
> > + *
> > + * The 'cameras' keys maps to a list of (lists) of camera properties.
> > + */
> > +
> > + std::string key = parseKey(parser);
> > + if (key.empty())
> > + return ret;
> > +
> > + if (key == "cameras") {
> > + ret = parseCameras(parser);
> > + } else
> > + LOG(HALConfig, Error) << "Unknown key: " << key;
> > +
> > + return ret;
> > +}
> > +
> > +int parseConfigFile(yaml_parser_t *parser)
> > +{
> > + yaml_token_t token;
> > + int ret = 0;
> > +
> > + yaml_parser_scan(parser, &token);
> > + if (token.type != YAML_STREAM_START_TOKEN) {
> > + LOG(HALConfig, Error) << "Configuration file is not valid";
> > + yaml_token_delete(&token);
> > + 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);
> > + 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(parser);
> > + break;
> > +
> > + case YAML_STREAM_END_TOKEN:
> > + ret = -ENOENT;
> > + /* Fall-through */
> > + default:
> > + yaml_token_delete(&token);
> > + break;
> > + }
> > + } while (ret >= 0);
> > +
> > + if (ret && ret != -ENOENT)
> > + LOG(HALConfig, Error) << "Configuration file is not valid";
> > +
> > + return ret == -ENOENT ? 0 : ret;
> > +}
> > +
> > +} /* namespace */
> > +
> > +std::string CameraHalConfig::findFilePath(const std::string &filename) const
> > +{
> > + static std::array<std::string, 2> searchPaths = {
> > + LIBCAMERA_SYSCONF_DIR,
> > + LIBCAMERA_DATA_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;
> > +}
> > +
>
> I don't know if we would like to hide like this way.
> We may want to achieve this by D-pointer like libcamera::CameraManager.
> I at least think cameras should be a member variable of CameraHalConfig.
> I would wait for Laurent's comments.
>
Yes, that might not be the most elegant solution. I went for static
functions as otherwise I would have the need to declare private
functions as part of the class, and they all take a yaml_parser_t
argument, so that would have required including "yaml.h" in the header
file. Maybe I could have get away just forward declaring it ?
I considered other patterns like PIMPL, but I also considered that:
1) Parsing is done just once at HAL initialization time, having static
fields like 'cameras' does not require keeping track of their state
(ie. cleaning them before a new parsing, as it only happens once)
2) We want to avoid exposing "yaml.h" to the rest of the HAL, but
afaict we do not plan to change the parsing backend, which kind of
defeat the purpose of pimpl which guarantees a stable interface
despite the backend implementation
3) the CameraHalConfig API is internal to the HAL and no external
component depend on it
Although having the parsing functions as class members would allow
me to make CameraProps a proper class with the internal parser class
declared as friend, so that class members fields can be made private with
proper accessor functions instead of having them public (as I currently need to set
them from static functions context).
All in all yes, this can be made more elegant, but I wonder if that
really required given the current use case.
Thanks
j
> > +/*
> > + * Open the HAL configuration file and validate its content.
> > + * Return 0 on success, a negative error code otherwise
> > + */
> > +int CameraHalConfig::open()
> > +{
> > + yaml_parser_t parser;
> > + int ret = yaml_parser_initialize(&parser);
> > + if (!ret) {
> > + LOG(HALConfig, Fatal) << "Failed to initialize yaml parser";
> > + return -EINVAL;
> > + }
> > +
> > + FILE *fh = openFile("camera_hal.yaml");
> > + if (!fh)
> > + return -ENOENT;
> > +
> > + yaml_parser_set_input_file(&parser, fh);
> > + ret = parseConfigFile(&parser);
> > + fclose(fh);
> > + yaml_parser_delete(&parser);
> > + if (ret)
> > + return ret;
> > +
> > + for (const auto &c : cameras) {
> > + const std::string &cameraId = c.first;
> > + const CameraProps &camera = c.second;
> > + LOG(HALConfig, Debug) << "'" << cameraId << "' "
> > + << " model: " << camera.model
> > + << "(" << camera.facing << ")["
> > + << camera.rotation << "]";
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +const CameraProps &CameraHalConfig::cameraProps(const std::string &cameraID) const
> > +{
> > + static CameraProps empty;
> > +
> > + const auto &it = cameras.find(cameraID);
> > + if (it == cameras.end()) {
> > + LOG(HALConfig, Error)
> > + << "Camera '" << cameraID
> > + << "' not described in the HAL configuration file";
> > + return empty;
> > + }
> > +
> > + return it->second;
> > +}
> > diff --git a/src/android/camera_hal_config.h b/src/android/camera_hal_config.h
> > new file mode 100644
> > index 000000000000..5491a12fdffd
> > --- /dev/null
> > +++ b/src/android/camera_hal_config.h
> > @@ -0,0 +1,36 @@
> > +/* 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 <string>
> > +
> > +class CameraProps
> > +{
> > +public:
> > + CameraProps()
> > + : valid(false) {}
> > +
> > + int facing;
> > + std::string model;
> > + unsigned int rotation;
> > +
> > + bool valid;
> > +};
>
> This doesn't have a function.
> So I would declare this as struct.
>
> https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes
>
> Best Regards,
> -Hiro
> > +
> > +class CameraHalConfig
> > +{
> > +public:
> > + int open();
> > + const CameraProps &cameraProps(const std::string &cameraID) const;
> > +
> > +private:
> > + std::string findFilePath(const std::string &filename) const;
> > + FILE *openFile(const std::string &filename);
> > +};
> > +#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',
> > 'camera_metadata.cpp',
> > 'camera_ops.cpp',
> > 'camera_stream.cpp',
> > --
> > 2.30.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list