[libcamera-devel] [PATCH v3 3/5] android: Add CameraHalConfig class
Jacopo Mondi
jacopo at jmondi.org
Tue Apr 6 16:50:00 CEST 2021
Hi Hiro,
On Thu, Apr 01, 2021 at 01:48:25PM +0900, Hirokazu Honda wrote:
> Hi Jacopo,
>
> On Wed, Mar 31, 2021 at 8:27 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
> >
> > 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;
>
> 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]?
Oh right, now that I've updated clang it also throws an error here.
I'll use std::stoi()
>
> > > > + }
> > > > + } 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;
> > > > +
>
> 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) {
> 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 = {
> > > > + 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.
>
> > > > + 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 "/"?
> 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
>
> By the way, since C++17, C++ has a filesystem library as a standard
> library, we may want to use it.
>
>
> > > > + 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.
> 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.
>
> > 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
> > > >
>
> We may want to add a unit test for CameraHalConfig although it is fine
> in another patch series.
>
> Best Regards,
> -Hiro
> > > > _______________________________________________
> > > > 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