[libcamera-devel,v4,3/5] android: Add CameraHalConfig class

11848 diff mbox series

Add a CameraHalConfig class to the Android Camera3 HAL layer.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 README.rst                        |   2 +-
 src/android/camera_hal_config.cpp | 433 ++++++++++++++++++++++++++++++
 src/android/camera_hal_config.h   |  38 +++
 src/android/meson.build           |   2 +
 4 files changed, 474 insertions(+), 1 deletion(-)
 create mode 100644 src/android/camera_hal_config.cpp
 create mode 100644 src/android/camera_hal_config.h

Hi Jacopo, Thanks for this patch.

On Wed, Apr 7, 2021 at 12:45 AM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Add a CameraHalConfig class to the Android Camera3 HAL layer.
>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  README.rst                        |   2 +-
>  src/android/camera_hal_config.cpp | 433 ++++++++++++++++++++++++++++++
>  src/android/camera_hal_config.h   |  38 +++
>  src/android/meson.build           |   2 +
>  4 files changed, 474 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 c77e54b48b7a..fcf0f47f14c5 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..5b51fa4dd61b
> --- /dev/null
> +++ b/src/android/camera_hal_config.cpp
> @@ -0,0 +1,433 @@
> +/* 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 <filesystem>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string>
> +#include <yaml.h>
> +
> +#include <hardware/camera3.h>
> +
> +#include "libcamera/internal/file.h"
> +#include "libcamera/internal/log.h"
> +
> +using namespace libcamera;
> +
> +LOG_DEFINE_CATEGORY(HALConfig)
> +
> +class CameraHalConfig::Private : public Extensible::Private
> +{
> +       LIBCAMERA_DECLARE_PUBLIC(CameraHalConfig)
> +
> +public:
> +       Private(CameraHalConfig *halConfig);
> +
> +       int parseConfigFile(FILE *fh, std::map<std::string, CameraProps> *cameras);
> +
> +private:
> +       std::string parseValue();
> +       std::string parseKey();
> +       int parseValueBlock();
> +       int parseCameraLocation(CameraProps *cameraProps, const std::string &location);
> +       int parseCameraProps(const std::string &cameraId);
> +       int parseCameras();
> +       int parseEntry();
> +
> +       yaml_parser_t parser_;
> +       std::map<std::string, CameraProps> *cameras_;
> +};
> +
> +CameraHalConfig::Private::Private(CameraHalConfig *halConfig)
> +       : Extensible::Private(halConfig)
> +{
> +}
> +
> +std::string CameraHalConfig::Private::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) {
> +               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 "";
> +       }
> +
> +       std::string value(reinterpret_cast<char *>(token.data.scalar.value),
> +                         token.data.scalar.length);
> +       yaml_token_delete(&token);
> +
> +       return value;
> +}
> +
> +std::string CameraHalConfig::Private::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) {
> +               yaml_token_delete(&token);
> +               return "";
> +       }
> +
> +       std::string value(reinterpret_cast<char *>(token.data.scalar.value),
> +                         token.data.scalar.length);
> +       yaml_token_delete(&token);
> +
> +       return value;
> +}
> +
> +int CameraHalConfig::Private::parseValueBlock()
> +{
> +       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 CameraHalConfig::Private::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
> +               return -EINVAL;
> +
> +       return 0;
> +}
> +
> +int CameraHalConfig::Private::parseCameraProps(const std::string &cameraId)
> +{
> +       int ret = parseValueBlock();
> +       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();
> +                       std::string value = parseValue();
> +                       if (key.empty() || value.empty())
> +                               return -EINVAL;
> +
> +                       if (key == "location") {
> +                               ret = parseCameraLocation(&cameraProps, value);
> +                               if (ret) {
> +                                       LOG(HALConfig, Error)
> +                                               << "Unknown location: "
> +                                               << value;
> +                                       return -EINVAL;
> +                               }
> +                       } else if (key == "rotation") {
> +                               ret = std::stoi(value);
> +                               if (ret < 0 || ret >= 360) {
> +                                       LOG(HALConfig, Error)
> +                                               << "Unknown rotation: "
> +                                               << cameraProps.rotation;
> +                                       return -EINVAL;
> +                               }
> +                               cameraProps.rotation = ret;
> +                       } else {
> +                               LOG(HALConfig, Error)
> +                                       << "Unknown key: " << key;
> +                               return -EINVAL;
> +                       }
> +                       break;
> +               }
> +
> +               case YAML_BLOCK_END_TOKEN:
> +                       blockEnd = true;
> +                       [[fallthrough]];
> +               default:
> +                       yaml_token_delete(&token);
> +                       break;
> +               }
> +
> +               --sentinel;
> +       } while (!blockEnd && sentinel);
> +       if (!sentinel)
> +               return -EINVAL;
> +
> +       cameraProps.valid = true;
> +       (*cameras_)[cameraId] = cameraProps;
> +
> +       return 0;
> +}
> +
> +int CameraHalConfig::Private::parseCameras()
> +{
> +       int ret = parseValueBlock();
> +       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();
> +                       if (cameraId.empty())
> +                               return -EINVAL;
> +
> +                       ret = parseCameraProps(cameraId);
> +                       if (ret)
> +                               return -EINVAL;
> +                       break;
> +               }
> +               case YAML_BLOCK_END_TOKEN:
> +                       blockEnd = true;
> +                       [[fallthrough]];
> +               default:
> +                       yaml_token_delete(&token);
> +                       break;
> +               }
> +       } while (!blockEnd);
> +
> +       return 0;
> +}
> +
> +int CameraHalConfig::Private::parseEntry()
> +{
> +       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();
> +       if (key.empty())
> +               return ret;
> +
> +       if (key == "cameras")
> +               ret = parseCameras();
> +       else
> +               LOG(HALConfig, Error) << "Unknown key: " << key;
> +
> +       return ret;
> +}
> +
> +int CameraHalConfig::Private::parseConfigFile(FILE *fh,
> +                                             std::map<std::string, CameraProps> *cameras)
> +{
> +       cameras_ = cameras;
> +
> +       int ret = yaml_parser_initialize(&parser_);
> +       if (!ret) {
> +               LOG(HALConfig, Fatal) << "Failed to initialize yaml parser";
> +               return -EINVAL;
> +       }
> +       yaml_parser_set_input_file(&parser_, fh);
> +
> +       yaml_token_t token;
> +       yaml_parser_scan(&parser_, &token);
> +       if (token.type != YAML_STREAM_START_TOKEN) {
> +               LOG(HALConfig, Error) << "Configuration file is not valid";
> +               yaml_token_delete(&token);
> +               yaml_parser_delete(&parser_);
> +               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);
> +               yaml_parser_delete(&parser_);
> +               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();
> +                       break;
> +
> +               case YAML_STREAM_END_TOKEN:
> +                       ret = -ENOENT;
> +                       [[fallthrough]];
> +               default:
> +                       yaml_token_delete(&token);
> +                       break;
> +               }
> +       } while (ret >= 0);
> +       yaml_parser_delete(&parser_);
> +
> +       if (ret && ret != -ENOENT)
> +               LOG(HALConfig, Error) << "Configuration file is not valid";
> +
> +       return ret == -ENOENT ? 0 : ret;
> +}
> +
> +std::string CameraHalConfig::findFilePath(const std::string &filename) const
> +{
> +       static const std::array<std::filesystem::path, 2> searchPaths = {
> +               LIBCAMERA_SYSCONF_DIR,
> +               LIBCAMERA_DATA_DIR,
> +       };
> +
> +       if (File::exists(filename))

nit: Not only the file
> +               return filename;
> +
> +       std::filesystem::path root = utils::libcameraSourcePath();
> +       if (!root.empty()) {
> +               std::string configurationPath = root / "data" / filename;
> +               if (File::exists(configurationPath))
> +                       return configurationPath;
> +       }
> +
> +       for (const std::filesystem::path &path : searchPaths) {
> +               std::string configurationPath = path / filename;
> +               if (File::exists(configurationPath))
> +                       return configurationPath;
> +       }
> +
> +       return "";
> +}
> +

If we use std::filesystem::path, we can use
std::filesystem::is_character_file(filepath). FWIW,
is_character_file() returns false if the file doesn't exist.
I would do as following,
std::filesystem::path findFilePath(const std::filesystem::path&)
FILE *CameraHalConfig::openFile(const std::filesystem::path&)
Then oepnFile("camera_hal.yaml") should work std::filesystem::path can
construct from char*.

> +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;
> +}
> +
> +CameraHalConfig::CameraHalConfig()
> +       : Extensible(new Private(this))
> +{
> +}
> +
> +/*
> + * Open the HAL configuration file and validate its content.
> + * Return 0 on success, a negative error code otherwise
> + */
> +int CameraHalConfig::open()
> +{
> +       FILE *fh = openFile("camera_hal.yaml");
> +       if (!fh)
> +               return -ENOENT;
> +
> +       Private *const d = LIBCAMERA_D_PTR();
> +       int ret = d->parseConfigFile(fh, &cameras_);
> +       fclose(fh);
> +       if (ret)
> +               return ret;
> +
> +       for (const auto &c : cameras_) {
> +               const std::string &cameraId = c.first;
> +               const CameraProps &camera = c.second;
> +               LOG(HALConfig, Debug) << "'" << cameraId << "' "
> +                                     << "(" << 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..65569559fbf0
> --- /dev/null
> +++ b/src/android/camera_hal_config.h
> @@ -0,0 +1,38 @@
> +/* 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 <libcamera/class.h>
> +
> +struct CameraProps {
> +       int facing;
> +       int rotation;
> +
> +       bool valid = false;
> +};
> +

nit: I would set facing and rotation to -1 as invalid values.

With those nits,
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>

Thanks,
-Hiro
> +class CameraHalConfig final : public libcamera::Extensible
> +{
> +       LIBCAMERA_DECLARE_PRIVATE(CameraBuffer)
> +
> +public:
> +       CameraHalConfig();
> +
> +       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);
> +
> +       std::map<std::string, CameraProps> cameras_;
> +};
> +#endif /* __ANDROID_CAMERA_HAL_CONFIG_H__ */
> diff --git a/src/android/meson.build b/src/android/meson.build
> index 2be20c97e118..3893e5b5b832 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
> @@ -45,6 +46,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.31.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Hiro,

On Wed, Apr 07, 2021 at 12:45:12PM +0900, Hirokazu Honda wrote:
> Hi Jacopo, Thanks for this patch.
>
> > +}
> > +
> > +std::string CameraHalConfig::findFilePath(const std::string &filename) const
> > +{
> > +       static const std::array<std::filesystem::path, 2> searchPaths = {
> > +               LIBCAMERA_SYSCONF_DIR,
> > +               LIBCAMERA_DATA_DIR,
> > +       };
> > +
> > +       if (File::exists(filename))
>
> nit: Not only the file

I'm sorry I didn't get this comment...

> > +               return filename;
> > +
> > +       std::filesystem::path root = utils::libcameraSourcePath();
> > +       if (!root.empty()) {
> > +               std::string configurationPath = root / "data" / filename;
> > +               if (File::exists(configurationPath))
> > +                       return configurationPath;
> > +       }
> > +
> > +       for (const std::filesystem::path &path : searchPaths) {
> > +               std::string configurationPath = path / filename;
> > +               if (File::exists(configurationPath))
> > +                       return configurationPath;
> > +       }
> > +
> > +       return "";
> > +}
> > +
>
> If we use std::filesystem::path, we can use
> std::filesystem::is_character_file(filepath). FWIW,
> is_character_file() returns false if the file doesn't exist.
> I would do as following,
> std::filesystem::path findFilePath(const std::filesystem::path&)
> FILE *CameraHalConfig::openFile(const std::filesystem::path&)
> Then oepnFile("camera_hal.yaml") should work std::filesystem::path can
> construct from char*.
>

File::exists() is used library-wide. To be honest I don't see what we
would gain.

> > +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;
> > +}
> > +
> > +CameraHalConfig::CameraHalConfig()
> > +       : Extensible(new Private(this))
> > +{
> > +}
> > +
> > +/*
> > + * Open the HAL configuration file and validate its content.
> > + * Return 0 on success, a negative error code otherwise
> > + */
> > +int CameraHalConfig::open()
> > +{
> > +       FILE *fh = openFile("camera_hal.yaml");
> > +       if (!fh)
> > +               return -ENOENT;
> > +
> > +       Private *const d = LIBCAMERA_D_PTR();
> > +       int ret = d->parseConfigFile(fh, &cameras_);
> > +       fclose(fh);
> > +       if (ret)
> > +               return ret;
> > +
> > +       for (const auto &c : cameras_) {
> > +               const std::string &cameraId = c.first;
> > +               const CameraProps &camera = c.second;
> > +               LOG(HALConfig, Debug) << "'" << cameraId << "' "
> > +                                     << "(" << 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..65569559fbf0
> > --- /dev/null
> > +++ b/src/android/camera_hal_config.h
> > @@ -0,0 +1,38 @@
> > +/* 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 <libcamera/class.h>
> > +
> > +struct CameraProps {
> > +       int facing;
> > +       int rotation;
> > +
> > +       bool valid = false;
> > +};
> > +
>
> nit: I would set facing and rotation to -1 as invalid values.
>

It seems like the 'valid' flag might get set to true also for camera
entries that do not specify 'location', 'rotation' or none of them. I
could drop 'valid' and initialize the single field, and fail out loud
if the value is retrieved from the configuration file but not
initialized.

Thanks
  j


> With those nits,
> Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
>
> Thanks,
> -Hiro
> > +class CameraHalConfig final : public libcamera::Extensible
> > +{
> > +       LIBCAMERA_DECLARE_PRIVATE(CameraBuffer)
> > +
> > +public:
> > +       CameraHalConfig();
> > +
> > +       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);
> > +
> > +       std::map<std::string, CameraProps> cameras_;
> > +};
> > +#endif /* __ANDROID_CAMERA_HAL_CONFIG_H__ */
> > diff --git a/src/android/meson.build b/src/android/meson.build
> > index 2be20c97e118..3893e5b5b832 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
> > @@ -45,6 +46,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.31.1
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

On Wed, Apr 7, 2021 at 4:46 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Hiro,
>
> On Wed, Apr 07, 2021 at 12:45:12PM +0900, Hirokazu Honda wrote:
> > Hi Jacopo, Thanks for this patch.
> >
> > > +}
> > > +
> > > +std::string CameraHalConfig::findFilePath(const std::string &filename) const
> > > +{
> > > +       static const std::array<std::filesystem::path, 2> searchPaths = {
> > > +               LIBCAMERA_SYSCONF_DIR,
> > > +               LIBCAMERA_DATA_DIR,
> > > +       };
> > > +
> > > +       if (File::exists(filename))
> >
> > nit: Not only the file
>
> I'm sorry I didn't get this comment...
>

Sorry, please ignore. I would write a comment about std::filesystem
described below.

> > > +               return filename;
> > > +
> > > +       std::filesystem::path root = utils::libcameraSourcePath();
> > > +       if (!root.empty()) {
> > > +               std::string configurationPath = root / "data" / filename;
> > > +               if (File::exists(configurationPath))
> > > +                       return configurationPath;
> > > +       }
> > > +
> > > +       for (const std::filesystem::path &path : searchPaths) {
> > > +               std::string configurationPath = path / filename;
> > > +               if (File::exists(configurationPath))
> > > +                       return configurationPath;
> > > +       }
> > > +
> > > +       return "";
> > > +}
> > > +
> >
> > If we use std::filesystem::path, we can use
> > std::filesystem::is_character_file(filepath). FWIW,
> > is_character_file() returns false if the file doesn't exist.
> > I would do as following,
> > std::filesystem::path findFilePath(const std::filesystem::path&)
> > FILE *CameraHalConfig::openFile(const std::filesystem::path&)
> > Then oepnFile("camera_hal.yaml") should work std::filesystem::path can
> > construct from char*.
> >
>
> File::exists() is used library-wide. To be honest I don't see what we
> would gain.
>

I prefer using a standard library.
Since we already use std::filesystem here, I think it would be
consistency to use std::filesystem function.
I am ok to use File::exists() though.

Thanks,
-Hiro
> > > +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;
> > > +}
> > > +
> > > +CameraHalConfig::CameraHalConfig()
> > > +       : Extensible(new Private(this))
> > > +{
> > > +}
> > > +
> > > +/*
> > > + * Open the HAL configuration file and validate its content.
> > > + * Return 0 on success, a negative error code otherwise
> > > + */
> > > +int CameraHalConfig::open()
> > > +{
> > > +       FILE *fh = openFile("camera_hal.yaml");
> > > +       if (!fh)
> > > +               return -ENOENT;
> > > +
> > > +       Private *const d = LIBCAMERA_D_PTR();
> > > +       int ret = d->parseConfigFile(fh, &cameras_);
> > > +       fclose(fh);
> > > +       if (ret)
> > > +               return ret;
> > > +
> > > +       for (const auto &c : cameras_) {
> > > +               const std::string &cameraId = c.first;
> > > +               const CameraProps &camera = c.second;
> > > +               LOG(HALConfig, Debug) << "'" << cameraId << "' "
> > > +                                     << "(" << 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..65569559fbf0
> > > --- /dev/null
> > > +++ b/src/android/camera_hal_config.h
> > > @@ -0,0 +1,38 @@
> > > +/* 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 <libcamera/class.h>
> > > +
> > > +struct CameraProps {
> > > +       int facing;
> > > +       int rotation;
> > > +
> > > +       bool valid = false;
> > > +};
> > > +
> >
> > nit: I would set facing and rotation to -1 as invalid values.
> >
>
> It seems like the 'valid' flag might get set to true also for camera
> entries that do not specify 'location', 'rotation' or none of them. I
> could drop 'valid' and initialize the single field, and fail out loud
> if the value is retrieved from the configuration file but not
> initialized.
>
> Thanks
>   j
>
>
> > With those nits,
> > Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> >
> > Thanks,
> > -Hiro
> > > +class CameraHalConfig final : public libcamera::Extensible
> > > +{
> > > +       LIBCAMERA_DECLARE_PRIVATE(CameraBuffer)
> > > +
> > > +public:
> > > +       CameraHalConfig();
> > > +
> > > +       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);
> > > +
> > > +       std::map<std::string, CameraProps> cameras_;
> > > +};
> > > +#endif /* __ANDROID_CAMERA_HAL_CONFIG_H__ */
> > > diff --git a/src/android/meson.build b/src/android/meson.build
> > > index 2be20c97e118..3893e5b5b832 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
> > > @@ -45,6 +46,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.31.1
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel@lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

Thank you for the patch.

On Wed, Apr 07, 2021 at 04:52:22PM +0900, Hirokazu Honda wrote:
> On Wed, Apr 7, 2021 at 4:46 PM Jacopo Mondi wrote:
> > On Wed, Apr 07, 2021 at 12:45:12PM +0900, Hirokazu Honda wrote:
> > > Hi Jacopo, Thanks for this patch.
> > >
> > > > +}
> > > > +
> > > > +std::string CameraHalConfig::findFilePath(const std::string &filename) const
> > > > +{
> > > > +       static const std::array<std::filesystem::path, 2> searchPaths = {
> > > > +               LIBCAMERA_SYSCONF_DIR,
> > > > +               LIBCAMERA_DATA_DIR,
> > > > +       };
> > > > +
> > > > +       if (File::exists(filename))
> > >
> > > nit: Not only the file
> >
> > I'm sorry I didn't get this comment...
> >
> 
> Sorry, please ignore. I would write a comment about std::filesystem
> described below.
> 
> > > > +               return filename;
> > > > +
> > > > +       std::filesystem::path root = utils::libcameraSourcePath();
> > > > +       if (!root.empty()) {
> > > > +               std::string configurationPath = root / "data" / filename;
> > > > +               if (File::exists(configurationPath))
> > > > +                       return configurationPath;
> > > > +       }
> > > > +
> > > > +       for (const std::filesystem::path &path : searchPaths) {
> > > > +               std::string configurationPath = path / filename;
> > > > +               if (File::exists(configurationPath))
> > > > +                       return configurationPath;
> > > > +       }
> > > > +
> > > > +       return "";
> > > > +}
> > > > +
> > >
> > > If we use std::filesystem::path, we can use
> > > std::filesystem::is_character_file(filepath). FWIW,
> > > is_character_file() returns false if the file doesn't exist.
> > > I would do as following,
> > > std::filesystem::path findFilePath(const std::filesystem::path&)
> > > FILE *CameraHalConfig::openFile(const std::filesystem::path&)
> > > Then oepnFile("camera_hal.yaml") should work std::filesystem::path can
> > > construct from char*.
> >
> > File::exists() is used library-wide. To be honest I don't see what we
> > would gain.
> 
> I prefer using a standard library.
> Since we already use std::filesystem here, I think it would be
> consistency to use std::filesystem function.
> I am ok to use File::exists() though.

Our File class predates the switch to C++17. I'm not opposed to moving
to std::filesystem. One feature that File provides is RAII semantics for
mmap(), and as far as I can tell, the C++ standard library doesn't
provide that. We could keep the File class for that purpose, and drop
File::exists() in favour of std::filesystem. It may not have to be done
in this patch series though, we could do so library-wide later.

https://bugs.libcamera.org/show_bug.cgi?id=25

> > > > +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;
> > > > +}
> > > > +
> > > > +CameraHalConfig::CameraHalConfig()
> > > > +       : Extensible(new Private(this))
> > > > +{
> > > > +}
> > > > +
> > > > +/*
> > > > + * Open the HAL configuration file and validate its content.
> > > > + * Return 0 on success, a negative error code otherwise
> > > > + */
> > > > +int CameraHalConfig::open()
> > > > +{
> > > > +       FILE *fh = openFile("camera_hal.yaml");
> > > > +       if (!fh)
> > > > +               return -ENOENT;
> > > > +
> > > > +       Private *const d = LIBCAMERA_D_PTR();
> > > > +       int ret = d->parseConfigFile(fh, &cameras_);
> > > > +       fclose(fh);
> > > > +       if (ret)
> > > > +               return ret;
> > > > +
> > > > +       for (const auto &c : cameras_) {
> > > > +               const std::string &cameraId = c.first;
> > > > +               const CameraProps &camera = c.second;
> > > > +               LOG(HALConfig, Debug) << "'" << cameraId << "' "
> > > > +                                     << "(" << 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..65569559fbf0
> > > > --- /dev/null
> > > > +++ b/src/android/camera_hal_config.h
> > > > @@ -0,0 +1,38 @@
> > > > +/* 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 <libcamera/class.h>
> > > > +
> > > > +struct CameraProps {
> > > > +       int facing;
> > > > +       int rotation;
> > > > +
> > > > +       bool valid = false;
> > > > +};
> > > > +
> > >
> > > nit: I would set facing and rotation to -1 as invalid values.
> >
> > It seems like the 'valid' flag might get set to true also for camera
> > entries that do not specify 'location', 'rotation' or none of them. I
> > could drop 'valid' and initialize the single field, and fail out loud
> > if the value is retrieved from the configuration file but not
> > initialized.
> >
> > > With those nits,
> > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> > >
> > > > +class CameraHalConfig final : public libcamera::Extensible
> > > > +{
> > > > +       LIBCAMERA_DECLARE_PRIVATE(CameraBuffer)
> > > > +
> > > > +public:
> > > > +       CameraHalConfig();
> > > > +
> > > > +       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);
> > > > +
> > > > +       std::map<std::string, CameraProps> cameras_;
> > > > +};
> > > > +#endif /* __ANDROID_CAMERA_HAL_CONFIG_H__ */
> > > > diff --git a/src/android/meson.build b/src/android/meson.build
> > > > index 2be20c97e118..3893e5b5b832 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
> > > > @@ -45,6 +46,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',

Hi Jacopo,

Thank you for the patch.

On Tue, Apr 06, 2021 at 05:45:55PM +0200, Jacopo Mondi wrote:
> Add a CameraHalConfig class to the Android Camera3 HAL layer.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  README.rst                        |   2 +-
>  src/android/camera_hal_config.cpp | 433 ++++++++++++++++++++++++++++++
>  src/android/camera_hal_config.h   |  38 +++
>  src/android/meson.build           |   2 +
>  4 files changed, 474 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 c77e54b48b7a..fcf0f47f14c5 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..5b51fa4dd61b
> --- /dev/null
> +++ b/src/android/camera_hal_config.cpp
> @@ -0,0 +1,433 @@
> +/* 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 <filesystem>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string>
> +#include <yaml.h>
> +
> +#include <hardware/camera3.h>
> +
> +#include "libcamera/internal/file.h"
> +#include "libcamera/internal/log.h"
> +
> +using namespace libcamera;
> +
> +LOG_DEFINE_CATEGORY(HALConfig)
> +
> +class CameraHalConfig::Private : public Extensible::Private
> +{
> +	LIBCAMERA_DECLARE_PUBLIC(CameraHalConfig)
> +
> +public:
> +	Private(CameraHalConfig *halConfig);
> +
> +	int parseConfigFile(FILE *fh, std::map<std::string, CameraProps> *cameras);
> +
> +private:
> +	std::string parseValue();
> +	std::string parseKey();
> +	int parseValueBlock();
> +	int parseCameraLocation(CameraProps *cameraProps, const std::string &location);
> +	int parseCameraProps(const std::string &cameraId);
> +	int parseCameras();
> +	int parseEntry();
> +
> +	yaml_parser_t parser_;
> +	std::map<std::string, CameraProps> *cameras_;
> +};
> +
> +CameraHalConfig::Private::Private(CameraHalConfig *halConfig)
> +	: Extensible::Private(halConfig)
> +{
> +}
> +
> +std::string CameraHalConfig::Private::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) {
> +		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 "";
> +	}
> +
> +	std::string value(reinterpret_cast<char *>(token.data.scalar.value),
> +			  token.data.scalar.length);
> +	yaml_token_delete(&token);
> +
> +	return value;
> +}
> +
> +std::string CameraHalConfig::Private::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) {
> +		yaml_token_delete(&token);
> +		return "";
> +	}
> +
> +	std::string value(reinterpret_cast<char *>(token.data.scalar.value),
> +			  token.data.scalar.length);
> +	yaml_token_delete(&token);
> +
> +	return value;
> +}
> +
> +int CameraHalConfig::Private::parseValueBlock()
> +{
> +	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 CameraHalConfig::Private::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
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +int CameraHalConfig::Private::parseCameraProps(const std::string &cameraId)
> +{
> +	int ret = parseValueBlock();
> +	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();
> +			std::string value = parseValue();
> +			if (key.empty() || value.empty())
> +				return -EINVAL;
> +
> +			if (key == "location") {
> +				ret = parseCameraLocation(&cameraProps, value);
> +				if (ret) {
> +					LOG(HALConfig, Error)
> +						<< "Unknown location: "
> +						<< value;
> +					return -EINVAL;
> +				}
> +			} else if (key == "rotation") {
> +				ret = std::stoi(value);
> +				if (ret < 0 || ret >= 360) {
> +					LOG(HALConfig, Error)
> +						<< "Unknown rotation: "
> +						<< cameraProps.rotation;
> +					return -EINVAL;
> +				}
> +				cameraProps.rotation = ret;
> +			} else {
> +				LOG(HALConfig, Error)
> +					<< "Unknown key: " << key;
> +				return -EINVAL;
> +			}
> +			break;
> +		}
> +
> +		case YAML_BLOCK_END_TOKEN:
> +			blockEnd = true;
> +			[[fallthrough]];
> +		default:
> +			yaml_token_delete(&token);
> +			break;
> +		}
> +
> +		--sentinel;
> +	} while (!blockEnd && sentinel);
> +	if (!sentinel)
> +		return -EINVAL;
> +
> +	cameraProps.valid = true;
> +	(*cameras_)[cameraId] = cameraProps;
> +
> +	return 0;
> +}
> +
> +int CameraHalConfig::Private::parseCameras()
> +{
> +	int ret = parseValueBlock();
> +	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();
> +			if (cameraId.empty())
> +				return -EINVAL;
> +
> +			ret = parseCameraProps(cameraId);
> +			if (ret)
> +				return -EINVAL;
> +			break;
> +		}
> +		case YAML_BLOCK_END_TOKEN:
> +			blockEnd = true;
> +			[[fallthrough]];
> +		default:
> +			yaml_token_delete(&token);
> +			break;
> +		}
> +	} while (!blockEnd);
> +
> +	return 0;
> +}
> +
> +int CameraHalConfig::Private::parseEntry()
> +{
> +	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();
> +	if (key.empty())
> +		return ret;
> +
> +	if (key == "cameras")
> +		ret = parseCameras();
> +	else
> +		LOG(HALConfig, Error) << "Unknown key: " << key;
> +
> +	return ret;
> +}
> +
> +int CameraHalConfig::Private::parseConfigFile(FILE *fh,
> +					      std::map<std::string, CameraProps> *cameras)
> +{
> +	cameras_ = cameras;
> +
> +	int ret = yaml_parser_initialize(&parser_);
> +	if (!ret) {
> +		LOG(HALConfig, Fatal) << "Failed to initialize yaml parser";
> +		return -EINVAL;
> +	}
> +	yaml_parser_set_input_file(&parser_, fh);
> +
> +	yaml_token_t token;
> +	yaml_parser_scan(&parser_, &token);
> +	if (token.type != YAML_STREAM_START_TOKEN) {
> +		LOG(HALConfig, Error) << "Configuration file is not valid";
> +		yaml_token_delete(&token);
> +		yaml_parser_delete(&parser_);
> +		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);
> +		yaml_parser_delete(&parser_);
> +		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();
> +			break;
> +
> +		case YAML_STREAM_END_TOKEN:
> +			ret = -ENOENT;
> +			[[fallthrough]];
> +		default:
> +			yaml_token_delete(&token);
> +			break;
> +		}
> +	} while (ret >= 0);
> +	yaml_parser_delete(&parser_);
> +
> +	if (ret && ret != -ENOENT)
> +		LOG(HALConfig, Error) << "Configuration file is not valid";
> +
> +	return ret == -ENOENT ? 0 : ret;
> +}
> +
> +std::string CameraHalConfig::findFilePath(const std::string &filename) const
> +{
> +	static const std::array<std::filesystem::path, 2> searchPaths = {
> +		LIBCAMERA_SYSCONF_DIR,
> +		LIBCAMERA_DATA_DIR,

Could you please reply to the comments in v1 or v2 about this ?

> +	};
> +
> +	if (File::exists(filename))
> +		return filename;
> +
> +	std::filesystem::path root = utils::libcameraSourcePath();
> +	if (!root.empty()) {
> +		std::string configurationPath = root / "data" / filename;
> +		if (File::exists(configurationPath))
> +			return configurationPath;
> +	}
> +
> +	for (const std::filesystem::path &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;
> +}

You could possibly inline this function in CameraHalConfig::open(), up
to you.

Other than these two comments, this looks good.

> +
> +CameraHalConfig::CameraHalConfig()
> +	: Extensible(new Private(this))
> +{
> +}
> +
> +/*
> + * Open the HAL configuration file and validate its content.
> + * Return 0 on success, a negative error code otherwise
> + */
> +int CameraHalConfig::open()
> +{
> +	FILE *fh = openFile("camera_hal.yaml");
> +	if (!fh)
> +		return -ENOENT;
> +
> +	Private *const d = LIBCAMERA_D_PTR();
> +	int ret = d->parseConfigFile(fh, &cameras_);
> +	fclose(fh);
> +	if (ret)
> +		return ret;
> +
> +	for (const auto &c : cameras_) {
> +		const std::string &cameraId = c.first;
> +		const CameraProps &camera = c.second;
> +		LOG(HALConfig, Debug) << "'" << cameraId << "' "
> +				      << "(" << 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..65569559fbf0
> --- /dev/null
> +++ b/src/android/camera_hal_config.h
> @@ -0,0 +1,38 @@
> +/* 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 <libcamera/class.h>
> +
> +struct CameraProps {
> +	int facing;
> +	int rotation;
> +
> +	bool valid = false;
> +};
> +
> +class CameraHalConfig final : public libcamera::Extensible
> +{
> +	LIBCAMERA_DECLARE_PRIVATE(CameraBuffer)
> +
> +public:
> +	CameraHalConfig();
> +
> +	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);
> +
> +	std::map<std::string, CameraProps> cameras_;
> +};
> +#endif /* __ANDROID_CAMERA_HAL_CONFIG_H__ */
> diff --git a/src/android/meson.build b/src/android/meson.build
> index 2be20c97e118..3893e5b5b832 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
> @@ -45,6 +46,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',

Hi Laurent,

On Tue, Apr 13, 2021 at 07:18:47AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> > +
> > +std::string CameraHalConfig::findFilePath(const std::string &filename) const
> > +{
> > +	static const std::array<std::filesystem::path, 2> searchPaths = {
> > +		LIBCAMERA_SYSCONF_DIR,
> > +		LIBCAMERA_DATA_DIR,
>
> Could you please reply to the comments in v1 or v2 about this ?
>

Sorry missed that. If I got it right it's enough to drop DATA_DIR

> > +	};
> > +
> > +	if (File::exists(filename))
> > +		return filename;
> > +
> > +	std::filesystem::path root = utils::libcameraSourcePath();
> > +	if (!root.empty()) {
> > +		std::string configurationPath = root / "data" / filename;
> > +		if (File::exists(configurationPath))
> > +			return configurationPath;
> > +	}
> > +
> > +	for (const std::filesystem::path &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;
> > +}
>
> You could possibly inline this function in CameraHalConfig::open(), up
> to you.

I think I've bee asked to break it out in previous iteration, and
sincerely I like it too, as it centralizes handling the FILE *.

>
> Other than these two comments, this looks good.
>
> > +
> > +CameraHalConfig::CameraHalConfig()
> > +	: Extensible(new Private(this))
> > +{
> > +}
> > +
> > +/*
> > + * Open the HAL configuration file and validate its content.
> > + * Return 0 on success, a negative error code otherwise
> > + */
> > +int CameraHalConfig::open()
> > +{
> > +	FILE *fh = openFile("camera_hal.yaml");
> > +	if (!fh)
> > +		return -ENOENT;
> > +
> > +	Private *const d = LIBCAMERA_D_PTR();
> > +	int ret = d->parseConfigFile(fh, &cameras_);
> > +	fclose(fh);
> > +	if (ret)
> > +		return ret;
> > +
> > +	for (const auto &c : cameras_) {
> > +		const std::string &cameraId = c.first;
> > +		const CameraProps &camera = c.second;
> > +		LOG(HALConfig, Debug) << "'" << cameraId << "' "
> > +				      << "(" << 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..65569559fbf0
> > --- /dev/null
> > +++ b/src/android/camera_hal_config.h
> > @@ -0,0 +1,38 @@
> > +/* 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 <libcamera/class.h>
> > +
> > +struct CameraProps {
> > +	int facing;
> > +	int rotation;
> > +
> > +	bool valid = false;
> > +};
> > +
> > +class CameraHalConfig final : public libcamera::Extensible
> > +{
> > +	LIBCAMERA_DECLARE_PRIVATE(CameraBuffer)
> > +
> > +public:
> > +	CameraHalConfig();
> > +
> > +	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);
> > +
> > +	std::map<std::string, CameraProps> cameras_;
> > +};
> > +#endif /* __ANDROID_CAMERA_HAL_CONFIG_H__ */
> > diff --git a/src/android/meson.build b/src/android/meson.build
> > index 2be20c97e118..3893e5b5b832 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
> > @@ -45,6 +46,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