[libcamera-devel] [PATCH v3 3/5] android: Add CameraHalConfig class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Apr 3 05:51:11 CEST 2021
Hi Jacopo,
Thank you for the patch. I'm happy to see that the configuration file is
just around the corner :-)
On Thu, Apr 01, 2021 at 01:48:25PM +0900, Hirokazu Honda wrote:
> On Wed, Mar 31, 2021 at 8:27 PM Jacopo Mondi wrote:
> > On Wed, Mar 31, 2021 at 06:30:34PM +0900, Hirokazu Honda wrote:
> > > On Tue, Mar 30, 2021 at 11:20 PM Jacopo Mondi 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);
You could also write this
std::string value(reinterpret_cast<char *>(token.data.scalar.value),
token.data.scalar.length);
Same below.
> > > > + 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)
s/cameraID/cameraId/ (and everywhere below)
> > > > +{
> > > > + 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{};
No need for {}.
> > > > + 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: "
s/Unupported/Unsupported/
> > > > + << value;
> > > > + return -EINVAL;
> > > > + }
> > > > + } else if (key == "rotation") {
> > > > + cameraProps.rotation = strtoul(value.c_str(),
> > > > + NULL, 10);
> > > > + if (cameraProps.rotation < 0) {
> > > > + LOG(HALConfig, Error)
> > > > + << "Unknown rotation: "
Nitpicking, I'd use "unsupported" like above, or replace the above with
"unknown".
> > > > + << cameraProps.rotation;
> > > > + return -EINVAL;
>
> nit: std::stoul(value) in c++ way.
>
> This seems to be strange, strtoul() returns *unsigned* integer and the
> if-clause check if it is negative.
> Perhaps, you mean stol() [c++] or strtol() [c]?
>
> > > > + }
> > > > + } 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 */
[[fallthrough]] in C++17.
> > > > + default:
> > > > + yaml_token_delete(&token);
> > > > + break;
> > > > + }
Have you tested this with comments in the YAML file ? One of the main
reasons we picked YAML over JSON is for the ability to add comments :-)
> > > > +
> > > > + --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;
> > > > +
>
> You may want to remove parenthesis.
>
> > > > + 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;
>
> How is the following code?
> int ret = 0;
> while (token.type != YAML_STREAM_END_TOKEN) {
> yaml_token_delete(&token);
> if (token.type == YAML_KEY_TOKEN) {
This won't work as yaml_token_delete() memset()s the token.
> ret = parseEntry(parser);
> if (!ret)
> break;
> }
> }
> yaml_token_delete(&token);
>
> if (ret)
> LOG(HALConfig, Error) << "Configuration file is not valid";
>
> return ret;
>
> > > > +}
> > > > +
> > > > +} /* namespace */
> > > > +
> > > > +std::string CameraHalConfig::findFilePath(const std::string &filename) const
> > > > +{
> > > > + static std::array<std::string, 2> searchPaths = {
static const
> > > > + LIBCAMERA_SYSCONF_DIR,
> > > > + LIBCAMERA_DATA_DIR,
> > > > + };
> > > > +
>
> Is this static beneficial? I think not much performance is gained by
> this, and findFilePath will likely be executed once.
It may not bring a large improvement, but isn't it best to specify
static data as static ?
> > > > + 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;
> > > > + }
> > > > +
>
> Just in case, is it guaranteed path and root doesn't end with "/"?
root ends with a "/", LIBCAMERA_SYSCONF_DIR and LIBCAMERA_DATA_DIR
don't.
> I am a bit surprised we don't have a class to deal with filepath in
> libcamera like chromium base::FilePath.
> https://source.chromium.org/chromium/chromium/src/+/master:base/files/file_path.h
We haven't had to deal with files much so far :-) We add helper classes
as needed, so I wouldn't be surprised if we got new file helpers in the
not too distant future.
> By the way, since C++17, C++ has a filesystem library as a standard
> library, we may want to use it.
That's a good point, now that we've switched to C++17, this should be
evaluated. I like std::filesystem::operator/ to concatenate path
elements
(https://en.cppreference.com/w/cpp/filesystem/path/operator_slash).
> > > > + 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.
>
> I would wait for Laurent's comments, while I think the current
> implementation is not so bad as we can go this direction.
As this is internal to the HAL and not something we'll reuse as-is or
expose in the public API I won't insist, but wrapping the parser, file
and possibly other data in a private class will simplify error handling
(and will also not require passing the parser pointer to all functions).
This being said, when we'll handle other configuration files and copy
this code, I'll probably insist to start with a refactoring :-) Using
the Extensible base class would give us a CameraHalConfig::Private that
could nicely store all the private data.
> One point I would like to change is to make camera a member variable
> of CameraHalConfig.
> If it is static, it should not be initialized more than once, but
> CameraHalConfig itself doesn't avoid it.
> Besides, CameraHalConfig is a member variable of CameraHalManager,
> which is static.
> We can let it be a member variable of CameraHalConfig by passing
> camera pointer or reference to parseCameraProps() through some
> functions out of CameraHalConfig.
>
> > > > +/*
> > > > + * 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)
This will leak the parser, you need to call yaml_parser_delete(). Not
sure if this is enough to convince you a private class will be less
painful :-)
> > > > + 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.
It has a constructor :-) Still, your point stands, and we could also
express this with
struct CameraProps {
int facing;
std::string model;
unsigned int rotation;
bool valid = false;
};
> > > So I would declare this as struct.
> > >
> > > https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes
> > >
> > > > +
> > > > +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',
>
> We may want to add a unit test for CameraHalConfig although it is fine
> in another patch series.
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list