[libcamera-devel] [PATCH v2 3/6] android: Add CameraHalConfig class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Apr 3 04:55:53 CEST 2021
Hi Jacopo,
On Tue, Mar 30, 2021 at 12:37:30PM +0200, Jacopo Mondi wrote:
> On Tue, Mar 30, 2021 at 03:39:56AM +0300, Laurent Pinchart wrote:
> > 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().
>
> ack
>
> > > +}
> > > +
> > > +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 ?
>
> Ah yes, most probably. I thought it had to be deleted if resued, but
> it seems deletion is just about releasing resources allocated during
> parser_scan(), so it needs to be done in error paths too
>
> I hate I can't goto easily in C++ to implement saner error handling
> paths
You could wrap the token in a C++ class with a destructor :-) Possibly
overkill, and most probably something we'll handle later, when we'll
have multiple parsers.
> > > +
> > > + 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.
>
> Intentional
>
> > > + 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.
>
> Good question. Where's the parsing state kept ? in the token or in the
> parser ? If it's in the parser I can avoid passing token around
It's in the parser. yaml_parser_scan() starts with
memset(token, 0, sizeof(yaml_token_t));
> > > + 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 ?
>
> Yes, the type has to stay valid in case of STREAM_END to be verified
> below. I can handle this with just ret if you prefer.
>
> > > + }
> > > + } 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