| Message ID | 20210329152807.28331-3-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Mon, Mar 29, 2021 at 05:28:04PM +0200, Jacopo Mondi wrote: > Add a CameraHalConfig class to the Android Camera3 HAL layer. There are quite a few comments from v1 that haven't been addressed. Maybe that's because my last reply to v1 came after you've sent this series. The comments may also not all be correct, applicable and/or desired, but could you then reply to them to explain why they're ignored ? In particular, I think that exposing CameraProps would simplify the implementation quite a bit, by removing the need for the camera*() functions, with the lookup of the CameraProps entry every time, and the associated error handling. > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > README.rst | 2 +- > src/android/camera_hal_config.cpp | 463 ++++++++++++++++++++++++++++++ > src/android/camera_hal_config.h | 53 ++++ > src/android/meson.build | 2 + > 4 files changed, 519 insertions(+), 1 deletion(-) > create mode 100644 src/android/camera_hal_config.cpp > create mode 100644 src/android/camera_hal_config.h > > diff --git a/README.rst b/README.rst > index 7a98164bbb0a..f68d435196de 100644 > --- a/README.rst > +++ b/README.rst > @@ -88,7 +88,7 @@ for tracing with lttng: [optional] > liblttng-ust-dev python3-jinja2 lttng-tools > > for android: [optional] > - libexif libjpeg > + libexif libjpeg libyaml > > Using GStreamer plugin > ~~~~~~~~~~~~~~~~~~~~~~ > diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp > new file mode 100644 > index 000000000000..846dd7357b0a > --- /dev/null > +++ b/src/android/camera_hal_config.cpp > @@ -0,0 +1,463 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2021, Google Inc. > + * > + * camera_hal_config.cpp - Camera HAL configuration file manager > + */ > +#include "camera_hal_config.h" > + > +#include <stdio.h> > +#include <stdlib.h> > +#include <string.h> > + > +#include <hardware/camera3.h> > + > +#include "libcamera/internal/file.h" > +#include "libcamera/internal/log.h" > + > +using namespace libcamera; > + > +LOG_DEFINE_CATEGORY(HALConfig) > + > +CameraHalConfig::CameraHalConfig() > +{ > + if (!yaml_parser_initialize(&parser_)) > + LOG(HALConfig, Fatal) << "Failed to initialize yaml parser"; > +} > + > +CameraHalConfig::~CameraHalConfig() > +{ > + yaml_parser_delete(&parser_); > +} > + > +/* > + * Configuration files can be stored in system paths, which are identified > + * through the build configuration. > + * > + * However, when running uninstalled - the source location takes precedence. > + */ > +std::string CameraHalConfig::findFilePath(const std::string &filename) const > +{ > + static std::array<std::string, 2> searchPaths = { > + LIBCAMERA_SYSCONF_DIR, > + LIBCAMERA_DATA_DIR, There's also a comment in v1 about data dir. Data dir is typically /usr/share/libcamera/. Given that the configuration file is device-specific, I don't think we'll ever look for it there, but only in sysconf dir. > + }; > + > + if (File::exists(filename)) > + return filename; > + > + std::string root = utils::libcameraSourcePath(); > + if (!root.empty()) { > + std::string configurationPath = root + "data/" + filename; > + if (File::exists(configurationPath)) > + return configurationPath; > + } > + > + for (std::string &path : searchPaths) { > + std::string configurationPath = path + "/" + filename; > + if (File::exists(configurationPath)) > + return configurationPath; > + } > + > + return ""; > +} > + > +FILE *CameraHalConfig::openFile(const std::string &filename) > +{ > + const std::string filePath = findFilePath(filename); > + if (filePath.empty()) { > + LOG(HALConfig, Error) > + << "Configuration file: \"" << filename << "\" not found"; > + return nullptr; > + } > + > + LOG(HALConfig, Debug) << "Reading configuration file from " << filePath; > + > + FILE *fh = fopen(filePath.c_str(), "r"); > + if (!fh) { > + int ret = -errno; > + LOG(HALConfig, Error) << "Failed to open configuration file " > + << filePath << ": " << strerror(-ret); > + return nullptr; > + } > + > + return fh; > +} > + > +/* > + * Open the HAL configuration file and validate its content. > + * Return 0 on success, a negative error code otherwise > + */ > +int CameraHalConfig::open() > +{ > + int ret; > + > + FILE *fh = openFile("camera_hal.yaml"); > + if (!fh) > + return -ENOENT; > + > + yaml_parser_set_input_file(&parser_, fh); > + ret = parseConfigFile(); > + fclose(fh); > + if (ret) > + return ret; > + > + LOG(HALConfig, Info) << "Device model: " << model_; > + LOG(HALConfig, Info) << "Device maker: " << maker_; > + for (const auto &c : cameras_) { > + const std::string &cameraId = c.first; > + const CameraProps &camera = c.second; > + LOG(HALConfig, Info) << "'" << cameraId << "' " > + << " model: " << camera.model > + << "(" << camera.location << ")[" > + << camera.rotation << "]"; > + } There's also a comment in v1 about whether this should be a debugging feature. > + > + return 0; > +} > + > +int CameraHalConfig::cameraLocation(const std::string &camera) const > +{ > + const auto &it = cameras_.find(camera); > + if (it == cameras_.end()) { > + LOG(HALConfig, Error) > + << "Camera '" << camera > + << "' not described in the HAL configuration file"; > + return -EINVAL; > + } > + > + const CameraProps &cam = it->second; > + if (cam.location.empty()) { > + LOG(HALConfig, Error) << "Location for camera '" << camera > + << "' not available"; > + return -EINVAL; > + } > + > + if (cam.location == "front") > + return CAMERA_FACING_FRONT; > + else if (cam.location == "back") > + return CAMERA_FACING_BACK; > + else if (cam.location == "external") > + return CAMERA_FACING_EXTERNAL; > + > + LOG(HALConfig, Error) << "Unsupported camera location " << cam.location; > + return -EINVAL; Can we do this conversion when parsing the configuration file instead ? That will ensure at parse time that the data is valid, instead of delaying the check to later. The rotation is already handled that way, and you already check that the location string is valid in CameraHalConfig::parseCameraProps(). > +} > + > +unsigned int CameraHalConfig::cameraRotation(const std::string &camera) const > +{ > + const auto &it = cameras_.find(camera); > + if (it == cameras_.end()) { > + LOG(HALConfig, Error) > + << "Camera '" << camera > + << "' not described in the HAL configuration file"; > + return 0; > + } > + > + const CameraProps &cam = it->second; > + return cam.rotation; > +} > + > +const std::string &CameraHalConfig::cameraModel(const std::string &camera) const > +{ > + static const std::string empty(""); > + const auto &it = cameras_.find(camera); > + if (it == cameras_.end()) { > + LOG(HALConfig, Error) > + << "Camera '" << camera > + << "' not described in the HAL configuration file"; > + return empty; > + } > + > + const CameraProps &cam = it->second; > + return cam.model; > +} > + > +std::string CameraHalConfig::parseValue(yaml_token_t &token) > +{ > + /* Make sure the token type is a value and get its content. */ > + yaml_parser_scan(&parser_, &token); > + if (token.type != YAML_VALUE_TOKEN) { > + LOG(HALConfig, Error) << "Configuration file is not valid"; > + return ""; > + } > + yaml_token_delete(&token); Does the token need to be deleted in the error cases above and below ? > + > + yaml_parser_scan(&parser_, &token); > + if (token.type != YAML_SCALAR_TOKEN) { > + LOG(HALConfig, Error) << "Configuration file is not valid"; > + return ""; > + } > + > + char *scalar = reinterpret_cast<char *>(token.data.scalar.value); > + std::string value(scalar, token.data.scalar.length); > + yaml_token_delete(&token); > + > + return value; > +} > + > +std::string CameraHalConfig::parseKey(yaml_token_t &token) > +{ > + /* Make sure the token type is a key and get its value. */ > + yaml_parser_scan(&parser_, &token); > + if (token.type != YAML_SCALAR_TOKEN) { > + LOG(HALConfig, Error) << "Configuration file is not valid"; > + return ""; > + } > + > + char *scalar = reinterpret_cast<char *>(token.data.scalar.value); > + std::string key(scalar, token.data.scalar.length); > + yaml_token_delete(&token); > + > + return key; > +} > + > +int CameraHalConfig::parseCameraProps(yaml_token_t &token, > + const std::string &cameraID) > +{ > + yaml_parser_scan(&parser_, &token); > + if (token.type != YAML_VALUE_TOKEN) { > + LOG(HALConfig, Error) << "Configuration file is not valid"; > + return -EINVAL; > + } > + yaml_token_delete(&token); > + > + yaml_parser_scan(&parser_, &token); > + if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) { > + LOG(HALConfig, Error) << "Configuration file is not valid"; > + return -EINVAL; > + } > + yaml_token_delete(&token); > + > + /* > + * Parse the camera properties and store them in a cameraBlock instance. > + * > + * Add a safety counter to make sure we don't loop indefinitely in case > + * the configuration file is malformed. > + */ > + unsigned int sentinel = 100; > + CameraProps cameraBlock{}; > + bool blockEnd = false; > + do { > + yaml_parser_scan(&parser_, &token); > + switch (token.type) { > + case YAML_KEY_TOKEN: { > + yaml_token_delete(&token); > + > + /* > + * Parse the camera property key and make sure it is > + * valid. > + */ > + std::string key = parseKey(token); > + std::string value = parseValue(token); > + if (key.empty() || value.empty()) { > + LOG(HALConfig, Error) > + << "Configuration file is not valid"; > + return -EINVAL; > + } > + > + if (key == "location") { > + if (value != "front" && value != "back" && > + value != "external") { > + LOG(HALConfig, Error) > + << "Unknown location: " << value; > + return -EINVAL; > + } > + cameraBlock.location = value; > + } else if (key == "rotation") { > + cameraBlock.rotation = strtoul(value.c_str(), > + NULL, 10); > + if (cameraBlock.rotation < 0) { > + LOG(HALConfig, Error) > + << "Unknown rotation: " > + << cameraBlock.rotation; > + return -EINVAL; > + } > + } else if (key == "model") { > + cameraBlock.model = value; > + } else { > + LOG(HALConfig, Error) > + << "Configuration file is not valid " > + << "unknown key: " << key; > + return -EINVAL; > + } > + break; > + } > + case YAML_BLOCK_END_TOKEN: > + yaml_token_delete(&token); > + blockEnd = true; > + break; > + default: > + yaml_token_delete(&token); > + break; > + } > + > + --sentinel; > + } while (!blockEnd && sentinel); Missing blank line. > + if (!sentinel) { > + LOG(HALConfig, Error) << "Configuration file is not valid "; > + return -EINVAL; > + } > + > + cameras_[cameraID] = cameraBlock; > + > + return 0; > +} > + > +int CameraHalConfig::parseCameras(yaml_token_t &token) > +{ > + int ret; > + > + /* The 'cameras' key maps a BLOCK_MAPPING_START block. */ > + yaml_parser_scan(&parser_, &token); > + if (token.type != YAML_VALUE_TOKEN) { > + LOG(HALConfig, Error) << "Configuration file is not valid"; > + return -EINVAL; > + } > + yaml_token_delete(&token); > + > + yaml_parser_scan(&parser_, &token); > + if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) { > + LOG(HALConfig, Error) << "Configuration file is not valid"; > + return -EINVAL; > + } > + yaml_token_delete(&token); > + > + /* > + * Parse the camera properties. > + * > + * Each camera properties block is a list of properties associated > + * with the ID (as assembled by CameraSensor::generateId()) of the > + * camera they refer to. > + * > + * cameras: > + * "camera0 id": > + * key: value > + * key: value > + * ... > + * > + * "camera1 id": > + * key: value > + * key: value > + * ... > + */ > + bool blockEnd = false; > + do { > + yaml_parser_scan(&parser_, &token); > + switch (token.type) { > + case YAML_KEY_TOKEN: { > + yaml_token_delete(&token); > + > + /* Parse the camera ID as key of the property list. */ > + std::string cameraId = parseKey(token); > + if (cameraId.empty()) { > + LOG(HALConfig, Error) > + << "Configuration file is not valid"; > + return -EINVAL; > + } > + > + ret = parseCameraProps(token, cameraId); > + if (ret) { > + LOG(HALConfig, Error) > + << "Configuration file is not valid"; > + return -EINVAL; > + } > + break; > + } > + case YAML_BLOCK_END_TOKEN: > + yaml_token_delete(&token); > + blockEnd = true; > + break; > + default: > + yaml_token_delete(&token); > + LOG(HALConfig, Error) > + << "Configuration file is not valid"; > + return -EINVAL; > + } > + } while (!blockEnd); > + > + return 0; > +} > + > +int CameraHalConfig::parseEntry(yaml_token_t &token) > +{ > + int ret; > + > + /* > + * Parse each key we find in the file. > + * > + * Keys like 'model' and 'maker' are device properties and gets > + * stored as class members. > + * > + * The 'cameras' keys maps to a list of (lists) of camera properties. > + */ > + > + std::string key = parseKey(token); > + if (key.empty()) { > + LOG(HALConfig, Error) << "Configuration file is not valid"; > + return -EINVAL; > + } > + > + if (key == "cameras") { > + ret = parseCameras(token); > + if (ret) > + return ret; > + } else if (key == "manufacturer") { > + maker_ = parseValue(token); > + if (maker_.empty()) { > + LOG(HALConfig, Error) << "Configuration file is not valid"; > + return -EINVAL; > + } > + } else if (key == "model") { > + model_ = parseValue(token); > + if (model_.empty()) { > + LOG(HALConfig, Error) << "Configuration file is not valid"; > + return -EINVAL; > + } > + } else { > + LOG(HALConfig, Error) << "Unknown key " << key; > + return -EINVAL; > + } > + > + return 0; > +} > + > +int CameraHalConfig::parseConfigFile() > +{ > + yaml_token_t token; > + int ret; > + > + yaml_parser_scan(&parser_, &token); > + if (token.type != YAML_STREAM_START_TOKEN) { > + LOG(HALConfig, Error) << "Configuration file is not valid"; > + return -EINVAL; > + } > + yaml_token_delete(&token); > + > + yaml_parser_scan(&parser_, &token); > + if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) { > + LOG(HALConfig, Error) << "Configuration file is not valid"; > + return -EINVAL; > + } > + yaml_token_delete(&token); > + > + /* Parse the file and parse each single key one by one. */ > + do { > + yaml_parser_scan(&parser_, &token); > + switch (token.type) { > + case YAML_KEY_TOKEN: > + yaml_token_delete(&token); > + ret = parseEntry(token); Do you need to pass the token to parseEntry(), given that you've just deleted it ? It looks to me like the token is only the return value of yaml_parse_scan(), and can thus be a local variable everywhere. > + break; > + case YAML_STREAM_END_TOKEN: > + /* Resources are released after the loop exit. */ > + break; > + default: > + yaml_token_delete(&token); > + break; The YAML_STREAM_END_TOKEN and default case are different, but the while() condition below accesses token.type. Is that intended ? > + } > + } while (token.type != YAML_STREAM_END_TOKEN && ret >= 0); > + yaml_token_delete(&token); > + > + return ret; > +} > diff --git a/src/android/camera_hal_config.h b/src/android/camera_hal_config.h > new file mode 100644 > index 000000000000..ce77b1ee1582 > --- /dev/null > +++ b/src/android/camera_hal_config.h > @@ -0,0 +1,53 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2021, Google Inc. > + * > + * camera_hal_config.h - Camera HAL configuration file manager > + */ > +#ifndef __ANDROID_CAMERA_HAL_CONFIG_H__ > +#define __ANDROID_CAMERA_HAL_CONFIG_H__ > + > +#include <map> > +#include <string> > +#include <yaml.h> As commented in v1, hiding the parser would be nice, but it can be done later, on top. > + > +class CameraHalConfig > +{ > +public: > + CameraHalConfig(); > + ~CameraHalConfig(); > + > + int open(); > + > + const std::string &deviceModel() const { return model_; } > + const std::string &deviceMaker() const { return maker_; } > + > + int cameraLocation(const std::string &camera) const; > + unsigned int cameraRotation(const std::string &camera) const; > + const std::string &cameraModel(const std::string &camera) const; > + > +private: > + yaml_parser_t parser_; > + > + std::string model_; > + std::string maker_; > + class CameraProps > + { > + public: > + std::string location; > + std::string model; > + unsigned int rotation; > + }; > + std::map<std::string, CameraProps> cameras_; > + > + std::string findFilePath(const std::string &filename) const; > + FILE *openFile(const std::string &filename); > + std::string parseValue(yaml_token_t &token); > + std::string parseKey(yaml_token_t &token); > + int parseCameraProps(yaml_token_t &token, const std::string &cameraID); > + int parseCameras(yaml_token_t &token); > + int parseEntry(yaml_token_t &token); > + int parseConfigFile(); > +}; > + > +#endif /* __ANDROID_CAMERA_HAL_CONFIG_H__ */ > diff --git a/src/android/meson.build b/src/android/meson.build > index 8e7d07d9be3c..c0ede407bc38 100644 > --- a/src/android/meson.build > +++ b/src/android/meson.build > @@ -3,6 +3,7 @@ > android_deps = [ > dependency('libexif', required : get_option('android')), > dependency('libjpeg', required : get_option('android')), > + dependency('yaml-0.1', required : get_option('android')), > ] > > android_enabled = true > @@ -41,6 +42,7 @@ android_hal_sources = files([ > 'camera3_hal.cpp', > 'camera_hal_manager.cpp', > 'camera_device.cpp', > + 'camera_hal_config.cpp', While at it, could you move camera_hal_manager to the right place ? > 'camera_metadata.cpp', > 'camera_ops.cpp', > 'camera_stream.cpp',
Hi Jacopo, Thanks for the patch. On Tue, Mar 30, 2021 at 9:40 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Jacopo, > > Thank you for the patch. > > On Mon, Mar 29, 2021 at 05:28:04PM +0200, Jacopo Mondi wrote: > > Add a CameraHalConfig class to the Android Camera3 HAL layer. > > There are quite a few comments from v1 that haven't been addressed. > Maybe that's because my last reply to v1 came after you've sent this > series. The comments may also not all be correct, applicable and/or > desired, but could you then reply to them to explain why they're ignored > ? > > In particular, I think that exposing CameraProps would simplify the > implementation quite a bit, by removing the need for the camera*() > functions, with the lookup of the CameraProps entry every time, and the > associated error handling. > +1. So we should pass the structure to CameraDevice by value. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > README.rst | 2 +- > > src/android/camera_hal_config.cpp | 463 ++++++++++++++++++++++++++++++ > > src/android/camera_hal_config.h | 53 ++++ > > src/android/meson.build | 2 + > > 4 files changed, 519 insertions(+), 1 deletion(-) > > create mode 100644 src/android/camera_hal_config.cpp > > create mode 100644 src/android/camera_hal_config.h > > > > diff --git a/README.rst b/README.rst > > index 7a98164bbb0a..f68d435196de 100644 > > --- a/README.rst > > +++ b/README.rst > > @@ -88,7 +88,7 @@ for tracing with lttng: [optional] > > liblttng-ust-dev python3-jinja2 lttng-tools > > > > for android: [optional] > > - libexif libjpeg > > + libexif libjpeg libyaml > > > > Using GStreamer plugin > > ~~~~~~~~~~~~~~~~~~~~~~ > > diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp > > new file mode 100644 > > index 000000000000..846dd7357b0a > > --- /dev/null > > +++ b/src/android/camera_hal_config.cpp > > @@ -0,0 +1,463 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2021, Google Inc. > > + * > > + * camera_hal_config.cpp - Camera HAL configuration file manager > > + */ > > +#include "camera_hal_config.h" > > + > > +#include <stdio.h> > > +#include <stdlib.h> > > +#include <string.h> > > + > > +#include <hardware/camera3.h> > > + > > +#include "libcamera/internal/file.h" > > +#include "libcamera/internal/log.h" > > + > > +using namespace libcamera; > > + > > +LOG_DEFINE_CATEGORY(HALConfig) > > + > > +CameraHalConfig::CameraHalConfig() > > +{ > > + if (!yaml_parser_initialize(&parser_)) > > + LOG(HALConfig, Fatal) << "Failed to initialize yaml parser"; > > +} > > + > > +CameraHalConfig::~CameraHalConfig() > > +{ > > + yaml_parser_delete(&parser_); > > +} > > + It looks strange to keep parser_ alive as long as CameraHalConfig, because parsing is done in open(). I would initialize the parser in open() and destroy it in the end of open(). In this direction, we would prefer moving parse* functions to .cpp file and also moving yaml include to .cpp. Regards, -Hiro > > +/* > > + * Configuration files can be stored in system paths, which are identified > > + * through the build configuration. > > + * > > + * However, when running uninstalled - the source location takes precedence. > > + */ > > +std::string CameraHalConfig::findFilePath(const std::string &filename) const > > +{ > > + static std::array<std::string, 2> searchPaths = { > > + LIBCAMERA_SYSCONF_DIR, > > + LIBCAMERA_DATA_DIR, > > There's also a comment in v1 about data dir. Data dir is typically > /usr/share/libcamera/. Given that the configuration file is > device-specific, I don't think we'll ever look for it there, but only in > sysconf dir. > > > + }; > > + > > + if (File::exists(filename)) > > + return filename; > > + > > + std::string root = utils::libcameraSourcePath(); > > + if (!root.empty()) { > > + std::string configurationPath = root + "data/" + filename; > > + if (File::exists(configurationPath)) > > + return configurationPath; > > + } > > + > > + for (std::string &path : searchPaths) { > > + std::string configurationPath = path + "/" + filename; > > + if (File::exists(configurationPath)) > > + return configurationPath; > > + } > > + > > + return ""; > > +} > > + > > +FILE *CameraHalConfig::openFile(const std::string &filename) > > +{ > > + const std::string filePath = findFilePath(filename); > > + if (filePath.empty()) { > > + LOG(HALConfig, Error) > > + << "Configuration file: \"" << filename << "\" not found"; > > + return nullptr; > > + } > > + > > + LOG(HALConfig, Debug) << "Reading configuration file from " << filePath; > > + > > + FILE *fh = fopen(filePath.c_str(), "r"); > > + if (!fh) { > > + int ret = -errno; > > + LOG(HALConfig, Error) << "Failed to open configuration file " > > + << filePath << ": " << strerror(-ret); > > + return nullptr; > > + } > > + > > + return fh; > > +} > > + > > +/* > > + * Open the HAL configuration file and validate its content. > > + * Return 0 on success, a negative error code otherwise > > + */ > > +int CameraHalConfig::open() > > +{ > > + int ret; > > + > > + FILE *fh = openFile("camera_hal.yaml"); > > + if (!fh) > > + return -ENOENT; > > + > > + yaml_parser_set_input_file(&parser_, fh); > > + ret = parseConfigFile(); > > + fclose(fh); > > + if (ret) > > + return ret; > > + > > + LOG(HALConfig, Info) << "Device model: " << model_; > > + LOG(HALConfig, Info) << "Device maker: " << maker_; > > + for (const auto &c : cameras_) { > > + const std::string &cameraId = c.first; > > + const CameraProps &camera = c.second; > > + LOG(HALConfig, Info) << "'" << cameraId << "' " > > + << " model: " << camera.model > > + << "(" << camera.location << ")[" > > + << camera.rotation << "]"; > > + } > > There's also a comment in v1 about whether this should be a debugging > feature. > > > + > > + return 0; > > +} > > + > > +int CameraHalConfig::cameraLocation(const std::string &camera) const > > +{ > > + const auto &it = cameras_.find(camera); > > + if (it == cameras_.end()) { > > + LOG(HALConfig, Error) > > + << "Camera '" << camera > > + << "' not described in the HAL configuration file"; > > + return -EINVAL; > > + } > > + > > + const CameraProps &cam = it->second; > > + if (cam.location.empty()) { > > + LOG(HALConfig, Error) << "Location for camera '" << camera > > + << "' not available"; > > + return -EINVAL; > > + } > > + > > + if (cam.location == "front") > > + return CAMERA_FACING_FRONT; > > + else if (cam.location == "back") > > + return CAMERA_FACING_BACK; > > + else if (cam.location == "external") > > + return CAMERA_FACING_EXTERNAL; > > + > > + LOG(HALConfig, Error) << "Unsupported camera location " << cam.location; > > + return -EINVAL; > > Can we do this conversion when parsing the configuration file instead ? > That will ensure at parse time that the data is valid, instead of > delaying the check to later. The rotation is already handled that way, > and you already check that the location string is valid in > CameraHalConfig::parseCameraProps(). > > > +} > > + > > +unsigned int CameraHalConfig::cameraRotation(const std::string &camera) const > > +{ > > + const auto &it = cameras_.find(camera); > > + if (it == cameras_.end()) { > > + LOG(HALConfig, Error) > > + << "Camera '" << camera > > + << "' not described in the HAL configuration file"; > > + return 0; > > + } > > + > > + const CameraProps &cam = it->second; > > + return cam.rotation; > > +} > > + > > +const std::string &CameraHalConfig::cameraModel(const std::string &camera) const > > +{ > > + static const std::string empty(""); > > + const auto &it = cameras_.find(camera); > > + if (it == cameras_.end()) { > > + LOG(HALConfig, Error) > > + << "Camera '" << camera > > + << "' not described in the HAL configuration file"; > > + return empty; > > + } > > + > > + const CameraProps &cam = it->second; > > + return cam.model; > > +} > > + > > +std::string CameraHalConfig::parseValue(yaml_token_t &token) > > +{ > > + /* Make sure the token type is a value and get its content. */ > > + yaml_parser_scan(&parser_, &token); > > + if (token.type != YAML_VALUE_TOKEN) { > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > + return ""; > > + } > > + yaml_token_delete(&token); > > Does the token need to be deleted in the error cases above and below ? > > > + > > + yaml_parser_scan(&parser_, &token); > > + if (token.type != YAML_SCALAR_TOKEN) { > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > + return ""; > > + } > > + > > + char *scalar = reinterpret_cast<char *>(token.data.scalar.value); > > + std::string value(scalar, token.data.scalar.length); > > + yaml_token_delete(&token); > > + > > + return value; > > +} > > + > > +std::string CameraHalConfig::parseKey(yaml_token_t &token) > > +{ > > + /* Make sure the token type is a key and get its value. */ > > + yaml_parser_scan(&parser_, &token); > > + if (token.type != YAML_SCALAR_TOKEN) { > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > + return ""; > > + } > > + > > + char *scalar = reinterpret_cast<char *>(token.data.scalar.value); > > + std::string key(scalar, token.data.scalar.length); > > + yaml_token_delete(&token); > > + > > + return key; > > +} > > + > > +int CameraHalConfig::parseCameraProps(yaml_token_t &token, > > + const std::string &cameraID) > > +{ > > + yaml_parser_scan(&parser_, &token); > > + if (token.type != YAML_VALUE_TOKEN) { > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > + return -EINVAL; > > + } > > + yaml_token_delete(&token); > > + > > + yaml_parser_scan(&parser_, &token); > > + if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) { > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > + return -EINVAL; > > + } > > + yaml_token_delete(&token); > > + > > + /* > > + * Parse the camera properties and store them in a cameraBlock instance. > > + * > > + * Add a safety counter to make sure we don't loop indefinitely in case > > + * the configuration file is malformed. > > + */ > > + unsigned int sentinel = 100; > > + CameraProps cameraBlock{}; > > + bool blockEnd = false; > > + do { > > + yaml_parser_scan(&parser_, &token); > > + switch (token.type) { > > + case YAML_KEY_TOKEN: { > > + yaml_token_delete(&token); > > + > > + /* > > + * Parse the camera property key and make sure it is > > + * valid. > > + */ > > + std::string key = parseKey(token); > > + std::string value = parseValue(token); > > + if (key.empty() || value.empty()) { > > + LOG(HALConfig, Error) > > + << "Configuration file is not valid"; > > + return -EINVAL; > > + } > > + > > + if (key == "location") { > > + if (value != "front" && value != "back" && > > + value != "external") { > > + LOG(HALConfig, Error) > > + << "Unknown location: " << value; > > + return -EINVAL; > > + } > > + cameraBlock.location = value; > > + } else if (key == "rotation") { > > + cameraBlock.rotation = strtoul(value.c_str(), > > + NULL, 10); > > + if (cameraBlock.rotation < 0) { > > + LOG(HALConfig, Error) > > + << "Unknown rotation: " > > + << cameraBlock.rotation; > > + return -EINVAL; > > + } > > + } else if (key == "model") { > > + cameraBlock.model = value; > > + } else { > > + LOG(HALConfig, Error) > > + << "Configuration file is not valid " > > + << "unknown key: " << key; > > + return -EINVAL; > > + } > > + break; > > + } > > + case YAML_BLOCK_END_TOKEN: > > + yaml_token_delete(&token); > > + blockEnd = true; > > + break; > > + default: > > + yaml_token_delete(&token); > > + break; > > + } > > + > > + --sentinel; > > + } while (!blockEnd && sentinel); > > Missing blank line. > > > + if (!sentinel) { > > + LOG(HALConfig, Error) << "Configuration file is not valid "; > > + return -EINVAL; > > + } > > + > > + cameras_[cameraID] = cameraBlock; > > + > > + return 0; > > +} > > + > > +int CameraHalConfig::parseCameras(yaml_token_t &token) > > +{ > > + int ret; > > + > > + /* The 'cameras' key maps a BLOCK_MAPPING_START block. */ > > + yaml_parser_scan(&parser_, &token); > > + if (token.type != YAML_VALUE_TOKEN) { > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > + return -EINVAL; > > + } > > + yaml_token_delete(&token); > > + > > + yaml_parser_scan(&parser_, &token); > > + if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) { > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > + return -EINVAL; > > + } > > + yaml_token_delete(&token); > > + > > + /* > > + * Parse the camera properties. > > + * > > + * Each camera properties block is a list of properties associated > > + * with the ID (as assembled by CameraSensor::generateId()) of the > > + * camera they refer to. > > + * > > + * cameras: > > + * "camera0 id": > > + * key: value > > + * key: value > > + * ... > > + * > > + * "camera1 id": > > + * key: value > > + * key: value > > + * ... > > + */ > > + bool blockEnd = false; > > + do { > > + yaml_parser_scan(&parser_, &token); > > + switch (token.type) { > > + case YAML_KEY_TOKEN: { > > + yaml_token_delete(&token); > > + > > + /* Parse the camera ID as key of the property list. */ > > + std::string cameraId = parseKey(token); > > + if (cameraId.empty()) { > > + LOG(HALConfig, Error) > > + << "Configuration file is not valid"; > > + return -EINVAL; > > + } > > + > > + ret = parseCameraProps(token, cameraId); > > + if (ret) { > > + LOG(HALConfig, Error) > > + << "Configuration file is not valid"; > > + return -EINVAL; > > + } > > + break; > > + } > > + case YAML_BLOCK_END_TOKEN: > > + yaml_token_delete(&token); > > + blockEnd = true; > > + break; > > + default: > > + yaml_token_delete(&token); > > + LOG(HALConfig, Error) > > + << "Configuration file is not valid"; > > + return -EINVAL; > > + } > > + } while (!blockEnd); > > + > > + return 0; > > +} > > + > > +int CameraHalConfig::parseEntry(yaml_token_t &token) > > +{ > > + int ret; > > + > > + /* > > + * Parse each key we find in the file. > > + * > > + * Keys like 'model' and 'maker' are device properties and gets > > + * stored as class members. > > + * > > + * The 'cameras' keys maps to a list of (lists) of camera properties. > > + */ > > + > > + std::string key = parseKey(token); > > + if (key.empty()) { > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > + return -EINVAL; > > + } > > + > > + if (key == "cameras") { > > + ret = parseCameras(token); > > + if (ret) > > + return ret; > > + } else if (key == "manufacturer") { > > + maker_ = parseValue(token); > > + if (maker_.empty()) { > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > + return -EINVAL; > > + } > > + } else if (key == "model") { > > + model_ = parseValue(token); > > + if (model_.empty()) { > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > + return -EINVAL; > > + } > > + } else { > > + LOG(HALConfig, Error) << "Unknown key " << key; > > + return -EINVAL; > > + } > > + > > + return 0; > > +} > > + > > +int CameraHalConfig::parseConfigFile() > > +{ > > + yaml_token_t token; > > + int ret; > > + > > + yaml_parser_scan(&parser_, &token); > > + if (token.type != YAML_STREAM_START_TOKEN) { > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > + return -EINVAL; > > + } > > + yaml_token_delete(&token); > > + > > + yaml_parser_scan(&parser_, &token); > > + if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) { > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > + return -EINVAL; > > + } > > + yaml_token_delete(&token); > > + > > + /* Parse the file and parse each single key one by one. */ > > + do { > > + yaml_parser_scan(&parser_, &token); > > + switch (token.type) { > > + case YAML_KEY_TOKEN: > > + yaml_token_delete(&token); > > + ret = parseEntry(token); > > Do you need to pass the token to parseEntry(), given that you've just > deleted it ? It looks to me like the token is only the return value of > yaml_parse_scan(), and can thus be a local variable everywhere. > > > + break; > > + case YAML_STREAM_END_TOKEN: > > + /* Resources are released after the loop exit. */ > > + break; > > + default: > > + yaml_token_delete(&token); > > + break; > > The YAML_STREAM_END_TOKEN and default case are different, but the > while() condition below accesses token.type. Is that intended ? > > > + } > > + } while (token.type != YAML_STREAM_END_TOKEN && ret >= 0); > > + yaml_token_delete(&token); > > + > > + return ret; > > +} > > diff --git a/src/android/camera_hal_config.h b/src/android/camera_hal_config.h > > new file mode 100644 > > index 000000000000..ce77b1ee1582 > > --- /dev/null > > +++ b/src/android/camera_hal_config.h > > @@ -0,0 +1,53 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2021, Google Inc. > > + * > > + * camera_hal_config.h - Camera HAL configuration file manager > > + */ > > +#ifndef __ANDROID_CAMERA_HAL_CONFIG_H__ > > +#define __ANDROID_CAMERA_HAL_CONFIG_H__ > > + > > +#include <map> > > +#include <string> > > +#include <yaml.h> > > As commented in v1, hiding the parser would be nice, but it can be done > later, on top. > > > + > > +class CameraHalConfig > > +{ > > +public: > > + CameraHalConfig(); > > + ~CameraHalConfig(); > > + > > + int open(); > > + > > + const std::string &deviceModel() const { return model_; } > > + const std::string &deviceMaker() const { return maker_; } > > + > > + int cameraLocation(const std::string &camera) const; > > + unsigned int cameraRotation(const std::string &camera) const; > > + const std::string &cameraModel(const std::string &camera) const; > > + > > +private: > > + yaml_parser_t parser_; > > + > > + std::string model_; > > + std::string maker_; > > + class CameraProps > > + { > > + public: > > + std::string location; > > + std::string model; > > + unsigned int rotation; > > + }; > > + std::map<std::string, CameraProps> cameras_; > > + > > + std::string findFilePath(const std::string &filename) const; > > + FILE *openFile(const std::string &filename); > > + std::string parseValue(yaml_token_t &token); > > + std::string parseKey(yaml_token_t &token); > > + int parseCameraProps(yaml_token_t &token, const std::string &cameraID); > > + int parseCameras(yaml_token_t &token); > > + int parseEntry(yaml_token_t &token); > > + int parseConfigFile(); > > +}; > > + > > +#endif /* __ANDROID_CAMERA_HAL_CONFIG_H__ */ > > diff --git a/src/android/meson.build b/src/android/meson.build > > index 8e7d07d9be3c..c0ede407bc38 100644 > > --- a/src/android/meson.build > > +++ b/src/android/meson.build > > @@ -3,6 +3,7 @@ > > android_deps = [ > > dependency('libexif', required : get_option('android')), > > dependency('libjpeg', required : get_option('android')), > > + dependency('yaml-0.1', required : get_option('android')), > > ] > > > > android_enabled = true > > @@ -41,6 +42,7 @@ android_hal_sources = files([ > > 'camera3_hal.cpp', > > 'camera_hal_manager.cpp', > > 'camera_device.cpp', > > + 'camera_hal_config.cpp', > > While at it, could you move camera_hal_manager to the right place ? > > > 'camera_metadata.cpp', > > 'camera_ops.cpp', > > 'camera_stream.cpp', > > -- > Regards, > > Laurent Pinchart > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Hiro, On Tue, Mar 30, 2021 at 12:48:20PM +0900, Hirokazu Honda wrote: > On Tue, Mar 30, 2021 at 9:40 AM Laurent Pinchartwrote: > > On Mon, Mar 29, 2021 at 05:28:04PM +0200, Jacopo Mondi wrote: > > > Add a CameraHalConfig class to the Android Camera3 HAL layer. > > > > There are quite a few comments from v1 that haven't been addressed. > > Maybe that's because my last reply to v1 came after you've sent this > > series. The comments may also not all be correct, applicable and/or > > desired, but could you then reply to them to explain why they're ignored > > ? > > > > In particular, I think that exposing CameraProps would simplify the > > implementation quite a bit, by removing the need for the camera*() > > functions, with the lookup of the CameraProps entry every time, and the > > associated error handling. > > +1. So we should pass the structure to CameraDevice by value. Why not by const reference ? > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > README.rst | 2 +- > > > src/android/camera_hal_config.cpp | 463 ++++++++++++++++++++++++++++++ > > > src/android/camera_hal_config.h | 53 ++++ > > > src/android/meson.build | 2 + > > > 4 files changed, 519 insertions(+), 1 deletion(-) > > > create mode 100644 src/android/camera_hal_config.cpp > > > create mode 100644 src/android/camera_hal_config.h > > > > > > diff --git a/README.rst b/README.rst > > > index 7a98164bbb0a..f68d435196de 100644 > > > --- a/README.rst > > > +++ b/README.rst > > > @@ -88,7 +88,7 @@ for tracing with lttng: [optional] > > > liblttng-ust-dev python3-jinja2 lttng-tools > > > > > > for android: [optional] > > > - libexif libjpeg > > > + libexif libjpeg libyaml > > > > > > Using GStreamer plugin > > > ~~~~~~~~~~~~~~~~~~~~~~ > > > diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp > > > new file mode 100644 > > > index 000000000000..846dd7357b0a > > > --- /dev/null > > > +++ b/src/android/camera_hal_config.cpp > > > @@ -0,0 +1,463 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2021, Google Inc. > > > + * > > > + * camera_hal_config.cpp - Camera HAL configuration file manager > > > + */ > > > +#include "camera_hal_config.h" > > > + > > > +#include <stdio.h> > > > +#include <stdlib.h> > > > +#include <string.h> > > > + > > > +#include <hardware/camera3.h> > > > + > > > +#include "libcamera/internal/file.h" > > > +#include "libcamera/internal/log.h" > > > + > > > +using namespace libcamera; > > > + > > > +LOG_DEFINE_CATEGORY(HALConfig) > > > + > > > +CameraHalConfig::CameraHalConfig() > > > +{ > > > + if (!yaml_parser_initialize(&parser_)) > > > + LOG(HALConfig, Fatal) << "Failed to initialize yaml parser"; > > > +} > > > + > > > +CameraHalConfig::~CameraHalConfig() > > > +{ > > > + yaml_parser_delete(&parser_); > > > +} > > > + > > It looks strange to keep parser_ alive as long as CameraHalConfig, > because parsing is done in open(). > I would initialize the parser in open() and destroy it in the end of open(). > In this direction, we would prefer moving parse* functions to .cpp > file and also moving yaml include to .cpp. I've mentioned that before :-) It could be done on top though. > > > +/* > > > + * Configuration files can be stored in system paths, which are identified > > > + * through the build configuration. > > > + * > > > + * However, when running uninstalled - the source location takes precedence. > > > + */ > > > +std::string CameraHalConfig::findFilePath(const std::string &filename) const > > > +{ > > > + static std::array<std::string, 2> searchPaths = { > > > + LIBCAMERA_SYSCONF_DIR, > > > + LIBCAMERA_DATA_DIR, > > > > There's also a comment in v1 about data dir. Data dir is typically > > /usr/share/libcamera/. Given that the configuration file is > > device-specific, I don't think we'll ever look for it there, but only in > > sysconf dir. > > > > > + }; > > > + > > > + if (File::exists(filename)) > > > + return filename; > > > + > > > + std::string root = utils::libcameraSourcePath(); > > > + if (!root.empty()) { > > > + std::string configurationPath = root + "data/" + filename; > > > + if (File::exists(configurationPath)) > > > + return configurationPath; > > > + } > > > + > > > + for (std::string &path : searchPaths) { > > > + std::string configurationPath = path + "/" + filename; > > > + if (File::exists(configurationPath)) > > > + return configurationPath; > > > + } > > > + > > > + return ""; > > > +} > > > + > > > +FILE *CameraHalConfig::openFile(const std::string &filename) > > > +{ > > > + const std::string filePath = findFilePath(filename); > > > + if (filePath.empty()) { > > > + LOG(HALConfig, Error) > > > + << "Configuration file: \"" << filename << "\" not found"; > > > + return nullptr; > > > + } > > > + > > > + LOG(HALConfig, Debug) << "Reading configuration file from " << filePath; > > > + > > > + FILE *fh = fopen(filePath.c_str(), "r"); > > > + if (!fh) { > > > + int ret = -errno; > > > + LOG(HALConfig, Error) << "Failed to open configuration file " > > > + << filePath << ": " << strerror(-ret); > > > + return nullptr; > > > + } > > > + > > > + return fh; > > > +} > > > + > > > +/* > > > + * Open the HAL configuration file and validate its content. > > > + * Return 0 on success, a negative error code otherwise > > > + */ > > > +int CameraHalConfig::open() > > > +{ > > > + int ret; > > > + > > > + FILE *fh = openFile("camera_hal.yaml"); > > > + if (!fh) > > > + return -ENOENT; > > > + > > > + yaml_parser_set_input_file(&parser_, fh); > > > + ret = parseConfigFile(); > > > + fclose(fh); > > > + if (ret) > > > + return ret; > > > + > > > + LOG(HALConfig, Info) << "Device model: " << model_; > > > + LOG(HALConfig, Info) << "Device maker: " << maker_; > > > + for (const auto &c : cameras_) { > > > + const std::string &cameraId = c.first; > > > + const CameraProps &camera = c.second; > > > + LOG(HALConfig, Info) << "'" << cameraId << "' " > > > + << " model: " << camera.model > > > + << "(" << camera.location << ")[" > > > + << camera.rotation << "]"; > > > + } > > > > There's also a comment in v1 about whether this should be a debugging > > feature. > > > > > + > > > + return 0; > > > +} > > > + > > > +int CameraHalConfig::cameraLocation(const std::string &camera) const > > > +{ > > > + const auto &it = cameras_.find(camera); > > > + if (it == cameras_.end()) { > > > + LOG(HALConfig, Error) > > > + << "Camera '" << camera > > > + << "' not described in the HAL configuration file"; > > > + return -EINVAL; > > > + } > > > + > > > + const CameraProps &cam = it->second; > > > + if (cam.location.empty()) { > > > + LOG(HALConfig, Error) << "Location for camera '" << camera > > > + << "' not available"; > > > + return -EINVAL; > > > + } > > > + > > > + if (cam.location == "front") > > > + return CAMERA_FACING_FRONT; > > > + else if (cam.location == "back") > > > + return CAMERA_FACING_BACK; > > > + else if (cam.location == "external") > > > + return CAMERA_FACING_EXTERNAL; > > > + > > > + LOG(HALConfig, Error) << "Unsupported camera location " << cam.location; > > > + return -EINVAL; > > > > Can we do this conversion when parsing the configuration file instead ? > > That will ensure at parse time that the data is valid, instead of > > delaying the check to later. The rotation is already handled that way, > > and you already check that the location string is valid in > > CameraHalConfig::parseCameraProps(). > > > > > +} > > > + > > > +unsigned int CameraHalConfig::cameraRotation(const std::string &camera) const > > > +{ > > > + const auto &it = cameras_.find(camera); > > > + if (it == cameras_.end()) { > > > + LOG(HALConfig, Error) > > > + << "Camera '" << camera > > > + << "' not described in the HAL configuration file"; > > > + return 0; > > > + } > > > + > > > + const CameraProps &cam = it->second; > > > + return cam.rotation; > > > +} > > > + > > > +const std::string &CameraHalConfig::cameraModel(const std::string &camera) const > > > +{ > > > + static const std::string empty(""); > > > + const auto &it = cameras_.find(camera); > > > + if (it == cameras_.end()) { > > > + LOG(HALConfig, Error) > > > + << "Camera '" << camera > > > + << "' not described in the HAL configuration file"; > > > + return empty; > > > + } > > > + > > > + const CameraProps &cam = it->second; > > > + return cam.model; > > > +} > > > + > > > +std::string CameraHalConfig::parseValue(yaml_token_t &token) > > > +{ > > > + /* Make sure the token type is a value and get its content. */ > > > + yaml_parser_scan(&parser_, &token); > > > + if (token.type != YAML_VALUE_TOKEN) { > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > + return ""; > > > + } > > > + yaml_token_delete(&token); > > > > Does the token need to be deleted in the error cases above and below ? > > > > > + > > > + yaml_parser_scan(&parser_, &token); > > > + if (token.type != YAML_SCALAR_TOKEN) { > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > + return ""; > > > + } > > > + > > > + char *scalar = reinterpret_cast<char *>(token.data.scalar.value); > > > + std::string value(scalar, token.data.scalar.length); > > > + yaml_token_delete(&token); > > > + > > > + return value; > > > +} > > > + > > > +std::string CameraHalConfig::parseKey(yaml_token_t &token) > > > +{ > > > + /* Make sure the token type is a key and get its value. */ > > > + yaml_parser_scan(&parser_, &token); > > > + if (token.type != YAML_SCALAR_TOKEN) { > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > + return ""; > > > + } > > > + > > > + char *scalar = reinterpret_cast<char *>(token.data.scalar.value); > > > + std::string key(scalar, token.data.scalar.length); > > > + yaml_token_delete(&token); > > > + > > > + return key; > > > +} > > > + > > > +int CameraHalConfig::parseCameraProps(yaml_token_t &token, > > > + const std::string &cameraID) > > > +{ > > > + yaml_parser_scan(&parser_, &token); > > > + if (token.type != YAML_VALUE_TOKEN) { > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > + return -EINVAL; > > > + } > > > + yaml_token_delete(&token); > > > + > > > + yaml_parser_scan(&parser_, &token); > > > + if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) { > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > + return -EINVAL; > > > + } > > > + yaml_token_delete(&token); > > > + > > > + /* > > > + * Parse the camera properties and store them in a cameraBlock instance. > > > + * > > > + * Add a safety counter to make sure we don't loop indefinitely in case > > > + * the configuration file is malformed. > > > + */ > > > + unsigned int sentinel = 100; > > > + CameraProps cameraBlock{}; > > > + bool blockEnd = false; > > > + do { > > > + yaml_parser_scan(&parser_, &token); > > > + switch (token.type) { > > > + case YAML_KEY_TOKEN: { > > > + yaml_token_delete(&token); > > > + > > > + /* > > > + * Parse the camera property key and make sure it is > > > + * valid. > > > + */ > > > + std::string key = parseKey(token); > > > + std::string value = parseValue(token); > > > + if (key.empty() || value.empty()) { > > > + LOG(HALConfig, Error) > > > + << "Configuration file is not valid"; > > > + return -EINVAL; > > > + } > > > + > > > + if (key == "location") { > > > + if (value != "front" && value != "back" && > > > + value != "external") { > > > + LOG(HALConfig, Error) > > > + << "Unknown location: " << value; > > > + return -EINVAL; > > > + } > > > + cameraBlock.location = value; > > > + } else if (key == "rotation") { > > > + cameraBlock.rotation = strtoul(value.c_str(), > > > + NULL, 10); > > > + if (cameraBlock.rotation < 0) { > > > + LOG(HALConfig, Error) > > > + << "Unknown rotation: " > > > + << cameraBlock.rotation; > > > + return -EINVAL; > > > + } > > > + } else if (key == "model") { > > > + cameraBlock.model = value; > > > + } else { > > > + LOG(HALConfig, Error) > > > + << "Configuration file is not valid " > > > + << "unknown key: " << key; > > > + return -EINVAL; > > > + } > > > + break; > > > + } > > > + case YAML_BLOCK_END_TOKEN: > > > + yaml_token_delete(&token); > > > + blockEnd = true; > > > + break; > > > + default: > > > + yaml_token_delete(&token); > > > + break; > > > + } > > > + > > > + --sentinel; > > > + } while (!blockEnd && sentinel); > > > > Missing blank line. > > > > > + if (!sentinel) { > > > + LOG(HALConfig, Error) << "Configuration file is not valid "; > > > + return -EINVAL; > > > + } > > > + > > > + cameras_[cameraID] = cameraBlock; > > > + > > > + return 0; > > > +} > > > + > > > +int CameraHalConfig::parseCameras(yaml_token_t &token) > > > +{ > > > + int ret; > > > + > > > + /* The 'cameras' key maps a BLOCK_MAPPING_START block. */ > > > + yaml_parser_scan(&parser_, &token); > > > + if (token.type != YAML_VALUE_TOKEN) { > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > + return -EINVAL; > > > + } > > > + yaml_token_delete(&token); > > > + > > > + yaml_parser_scan(&parser_, &token); > > > + if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) { > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > + return -EINVAL; > > > + } > > > + yaml_token_delete(&token); > > > + > > > + /* > > > + * Parse the camera properties. > > > + * > > > + * Each camera properties block is a list of properties associated > > > + * with the ID (as assembled by CameraSensor::generateId()) of the > > > + * camera they refer to. > > > + * > > > + * cameras: > > > + * "camera0 id": > > > + * key: value > > > + * key: value > > > + * ... > > > + * > > > + * "camera1 id": > > > + * key: value > > > + * key: value > > > + * ... > > > + */ > > > + bool blockEnd = false; > > > + do { > > > + yaml_parser_scan(&parser_, &token); > > > + switch (token.type) { > > > + case YAML_KEY_TOKEN: { > > > + yaml_token_delete(&token); > > > + > > > + /* Parse the camera ID as key of the property list. */ > > > + std::string cameraId = parseKey(token); > > > + if (cameraId.empty()) { > > > + LOG(HALConfig, Error) > > > + << "Configuration file is not valid"; > > > + return -EINVAL; > > > + } > > > + > > > + ret = parseCameraProps(token, cameraId); > > > + if (ret) { > > > + LOG(HALConfig, Error) > > > + << "Configuration file is not valid"; > > > + return -EINVAL; > > > + } > > > + break; > > > + } > > > + case YAML_BLOCK_END_TOKEN: > > > + yaml_token_delete(&token); > > > + blockEnd = true; > > > + break; > > > + default: > > > + yaml_token_delete(&token); > > > + LOG(HALConfig, Error) > > > + << "Configuration file is not valid"; > > > + return -EINVAL; > > > + } > > > + } while (!blockEnd); > > > + > > > + return 0; > > > +} > > > + > > > +int CameraHalConfig::parseEntry(yaml_token_t &token) > > > +{ > > > + int ret; > > > + > > > + /* > > > + * Parse each key we find in the file. > > > + * > > > + * Keys like 'model' and 'maker' are device properties and gets > > > + * stored as class members. > > > + * > > > + * The 'cameras' keys maps to a list of (lists) of camera properties. > > > + */ > > > + > > > + std::string key = parseKey(token); > > > + if (key.empty()) { > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > + return -EINVAL; > > > + } > > > + > > > + if (key == "cameras") { > > > + ret = parseCameras(token); > > > + if (ret) > > > + return ret; > > > + } else if (key == "manufacturer") { > > > + maker_ = parseValue(token); > > > + if (maker_.empty()) { > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > + return -EINVAL; > > > + } > > > + } else if (key == "model") { > > > + model_ = parseValue(token); > > > + if (model_.empty()) { > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > + return -EINVAL; > > > + } > > > + } else { > > > + LOG(HALConfig, Error) << "Unknown key " << key; > > > + return -EINVAL; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +int CameraHalConfig::parseConfigFile() > > > +{ > > > + yaml_token_t token; > > > + int ret; > > > + > > > + yaml_parser_scan(&parser_, &token); > > > + if (token.type != YAML_STREAM_START_TOKEN) { > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > + return -EINVAL; > > > + } > > > + yaml_token_delete(&token); > > > + > > > + yaml_parser_scan(&parser_, &token); > > > + if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) { > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > + return -EINVAL; > > > + } > > > + yaml_token_delete(&token); > > > + > > > + /* Parse the file and parse each single key one by one. */ > > > + do { > > > + yaml_parser_scan(&parser_, &token); > > > + switch (token.type) { > > > + case YAML_KEY_TOKEN: > > > + yaml_token_delete(&token); > > > + ret = parseEntry(token); > > > > Do you need to pass the token to parseEntry(), given that you've just > > deleted it ? It looks to me like the token is only the return value of > > yaml_parse_scan(), and can thus be a local variable everywhere. > > > > > + break; > > > + case YAML_STREAM_END_TOKEN: > > > + /* Resources are released after the loop exit. */ > > > + break; > > > + default: > > > + yaml_token_delete(&token); > > > + break; > > > > The YAML_STREAM_END_TOKEN and default case are different, but the > > while() condition below accesses token.type. Is that intended ? > > > > > + } > > > + } while (token.type != YAML_STREAM_END_TOKEN && ret >= 0); > > > + yaml_token_delete(&token); > > > + > > > + return ret; > > > +} > > > diff --git a/src/android/camera_hal_config.h b/src/android/camera_hal_config.h > > > new file mode 100644 > > > index 000000000000..ce77b1ee1582 > > > --- /dev/null > > > +++ b/src/android/camera_hal_config.h > > > @@ -0,0 +1,53 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2021, Google Inc. > > > + * > > > + * camera_hal_config.h - Camera HAL configuration file manager > > > + */ > > > +#ifndef __ANDROID_CAMERA_HAL_CONFIG_H__ > > > +#define __ANDROID_CAMERA_HAL_CONFIG_H__ > > > + > > > +#include <map> > > > +#include <string> > > > +#include <yaml.h> > > > > As commented in v1, hiding the parser would be nice, but it can be done > > later, on top. > > > > > + > > > +class CameraHalConfig > > > +{ > > > +public: > > > + CameraHalConfig(); > > > + ~CameraHalConfig(); > > > + > > > + int open(); > > > + > > > + const std::string &deviceModel() const { return model_; } > > > + const std::string &deviceMaker() const { return maker_; } > > > + > > > + int cameraLocation(const std::string &camera) const; > > > + unsigned int cameraRotation(const std::string &camera) const; > > > + const std::string &cameraModel(const std::string &camera) const; > > > + > > > +private: > > > + yaml_parser_t parser_; > > > + > > > + std::string model_; > > > + std::string maker_; > > > + class CameraProps > > > + { > > > + public: > > > + std::string location; > > > + std::string model; > > > + unsigned int rotation; > > > + }; > > > + std::map<std::string, CameraProps> cameras_; > > > + > > > + std::string findFilePath(const std::string &filename) const; > > > + FILE *openFile(const std::string &filename); > > > + std::string parseValue(yaml_token_t &token); > > > + std::string parseKey(yaml_token_t &token); > > > + int parseCameraProps(yaml_token_t &token, const std::string &cameraID); > > > + int parseCameras(yaml_token_t &token); > > > + int parseEntry(yaml_token_t &token); > > > + int parseConfigFile(); > > > +}; > > > + > > > +#endif /* __ANDROID_CAMERA_HAL_CONFIG_H__ */ > > > diff --git a/src/android/meson.build b/src/android/meson.build > > > index 8e7d07d9be3c..c0ede407bc38 100644 > > > --- a/src/android/meson.build > > > +++ b/src/android/meson.build > > > @@ -3,6 +3,7 @@ > > > android_deps = [ > > > dependency('libexif', required : get_option('android')), > > > dependency('libjpeg', required : get_option('android')), > > > + dependency('yaml-0.1', required : get_option('android')), > > > ] > > > > > > android_enabled = true > > > @@ -41,6 +42,7 @@ android_hal_sources = files([ > > > 'camera3_hal.cpp', > > > 'camera_hal_manager.cpp', > > > 'camera_device.cpp', > > > + 'camera_hal_config.cpp', > > > > While at it, could you move camera_hal_manager to the right place ? > > > > > 'camera_metadata.cpp', > > > 'camera_ops.cpp', > > > 'camera_stream.cpp',
On Tue, Mar 30, 2021 at 12:53 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Hiro, > > On Tue, Mar 30, 2021 at 12:48:20PM +0900, Hirokazu Honda wrote: > > On Tue, Mar 30, 2021 at 9:40 AM Laurent Pinchartwrote: > > > On Mon, Mar 29, 2021 at 05:28:04PM +0200, Jacopo Mondi wrote: > > > > Add a CameraHalConfig class to the Android Camera3 HAL layer. > > > > > > There are quite a few comments from v1 that haven't been addressed. > > > Maybe that's because my last reply to v1 came after you've sent this > > > series. The comments may also not all be correct, applicable and/or > > > desired, but could you then reply to them to explain why they're ignored > > > ? > > > > > > In particular, I think that exposing CameraProps would simplify the > > > implementation quite a bit, by removing the need for the camera*() > > > functions, with the lookup of the CameraProps entry every time, and the > > > associated error handling. > > > > +1. So we should pass the structure to CameraDevice by value. > > Why not by const reference ? > I would not like having const reference in member variables. I actually thought this was stated in Google C++ Style Guide, and searched it, but found there was no statement like that. So this is just my preference. For the purpose, I usually use a const pointer. Considering why I don't like having const reference member variables, one of reasons might be what I don't get used to :p, but also from a caller of the constructor the lifetime of CameraHalConfig is unclear. For instance, CameraDevice(.., config) is called, but a caller has no idea when the config should be alive by, while CameraDevice(..., &config) tells a caller to keep the config object outlive to CameraDevice. Again, this might be my personal preference. But does this description make sense? -Hiro > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > --- > > > > README.rst | 2 +- > > > > src/android/camera_hal_config.cpp | 463 ++++++++++++++++++++++++++++++ > > > > src/android/camera_hal_config.h | 53 ++++ > > > > src/android/meson.build | 2 + > > > > 4 files changed, 519 insertions(+), 1 deletion(-) > > > > create mode 100644 src/android/camera_hal_config.cpp > > > > create mode 100644 src/android/camera_hal_config.h > > > > > > > > diff --git a/README.rst b/README.rst > > > > index 7a98164bbb0a..f68d435196de 100644 > > > > --- a/README.rst > > > > +++ b/README.rst > > > > @@ -88,7 +88,7 @@ for tracing with lttng: [optional] > > > > liblttng-ust-dev python3-jinja2 lttng-tools > > > > > > > > for android: [optional] > > > > - libexif libjpeg > > > > + libexif libjpeg libyaml > > > > > > > > Using GStreamer plugin > > > > ~~~~~~~~~~~~~~~~~~~~~~ > > > > diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp > > > > new file mode 100644 > > > > index 000000000000..846dd7357b0a > > > > --- /dev/null > > > > +++ b/src/android/camera_hal_config.cpp > > > > @@ -0,0 +1,463 @@ > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > > +/* > > > > + * Copyright (C) 2021, Google Inc. > > > > + * > > > > + * camera_hal_config.cpp - Camera HAL configuration file manager > > > > + */ > > > > +#include "camera_hal_config.h" > > > > + > > > > +#include <stdio.h> > > > > +#include <stdlib.h> > > > > +#include <string.h> > > > > + > > > > +#include <hardware/camera3.h> > > > > + > > > > +#include "libcamera/internal/file.h" > > > > +#include "libcamera/internal/log.h" > > > > + > > > > +using namespace libcamera; > > > > + > > > > +LOG_DEFINE_CATEGORY(HALConfig) > > > > + > > > > +CameraHalConfig::CameraHalConfig() > > > > +{ > > > > + if (!yaml_parser_initialize(&parser_)) > > > > + LOG(HALConfig, Fatal) << "Failed to initialize yaml parser"; > > > > +} > > > > + > > > > +CameraHalConfig::~CameraHalConfig() > > > > +{ > > > > + yaml_parser_delete(&parser_); > > > > +} > > > > + > > > > It looks strange to keep parser_ alive as long as CameraHalConfig, > > because parsing is done in open(). > > I would initialize the parser in open() and destroy it in the end of open(). > > In this direction, we would prefer moving parse* functions to .cpp > > file and also moving yaml include to .cpp. > > I've mentioned that before :-) It could be done on top though. > > > > > +/* > > > > + * Configuration files can be stored in system paths, which are identified > > > > + * through the build configuration. > > > > + * > > > > + * However, when running uninstalled - the source location takes precedence. > > > > + */ > > > > +std::string CameraHalConfig::findFilePath(const std::string &filename) const > > > > +{ > > > > + static std::array<std::string, 2> searchPaths = { > > > > + LIBCAMERA_SYSCONF_DIR, > > > > + LIBCAMERA_DATA_DIR, > > > > > > There's also a comment in v1 about data dir. Data dir is typically > > > /usr/share/libcamera/. Given that the configuration file is > > > device-specific, I don't think we'll ever look for it there, but only in > > > sysconf dir. > > > > > > > + }; > > > > + > > > > + if (File::exists(filename)) > > > > + return filename; > > > > + > > > > + std::string root = utils::libcameraSourcePath(); > > > > + if (!root.empty()) { > > > > + std::string configurationPath = root + "data/" + filename; > > > > + if (File::exists(configurationPath)) > > > > + return configurationPath; > > > > + } > > > > + > > > > + for (std::string &path : searchPaths) { > > > > + std::string configurationPath = path + "/" + filename; > > > > + if (File::exists(configurationPath)) > > > > + return configurationPath; > > > > + } > > > > + > > > > + return ""; > > > > +} > > > > + > > > > +FILE *CameraHalConfig::openFile(const std::string &filename) > > > > +{ > > > > + const std::string filePath = findFilePath(filename); > > > > + if (filePath.empty()) { > > > > + LOG(HALConfig, Error) > > > > + << "Configuration file: \"" << filename << "\" not found"; > > > > + return nullptr; > > > > + } > > > > + > > > > + LOG(HALConfig, Debug) << "Reading configuration file from " << filePath; > > > > + > > > > + FILE *fh = fopen(filePath.c_str(), "r"); > > > > + if (!fh) { > > > > + int ret = -errno; > > > > + LOG(HALConfig, Error) << "Failed to open configuration file " > > > > + << filePath << ": " << strerror(-ret); > > > > + return nullptr; > > > > + } > > > > + > > > > + return fh; > > > > +} > > > > + > > > > +/* > > > > + * Open the HAL configuration file and validate its content. > > > > + * Return 0 on success, a negative error code otherwise > > > > + */ > > > > +int CameraHalConfig::open() > > > > +{ > > > > + int ret; > > > > + > > > > + FILE *fh = openFile("camera_hal.yaml"); > > > > + if (!fh) > > > > + return -ENOENT; > > > > + > > > > + yaml_parser_set_input_file(&parser_, fh); > > > > + ret = parseConfigFile(); > > > > + fclose(fh); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + LOG(HALConfig, Info) << "Device model: " << model_; > > > > + LOG(HALConfig, Info) << "Device maker: " << maker_; > > > > + for (const auto &c : cameras_) { > > > > + const std::string &cameraId = c.first; > > > > + const CameraProps &camera = c.second; > > > > + LOG(HALConfig, Info) << "'" << cameraId << "' " > > > > + << " model: " << camera.model > > > > + << "(" << camera.location << ")[" > > > > + << camera.rotation << "]"; > > > > + } > > > > > > There's also a comment in v1 about whether this should be a debugging > > > feature. > > > > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +int CameraHalConfig::cameraLocation(const std::string &camera) const > > > > +{ > > > > + const auto &it = cameras_.find(camera); > > > > + if (it == cameras_.end()) { > > > > + LOG(HALConfig, Error) > > > > + << "Camera '" << camera > > > > + << "' not described in the HAL configuration file"; > > > > + return -EINVAL; > > > > + } > > > > + > > > > + const CameraProps &cam = it->second; > > > > + if (cam.location.empty()) { > > > > + LOG(HALConfig, Error) << "Location for camera '" << camera > > > > + << "' not available"; > > > > + return -EINVAL; > > > > + } > > > > + > > > > + if (cam.location == "front") > > > > + return CAMERA_FACING_FRONT; > > > > + else if (cam.location == "back") > > > > + return CAMERA_FACING_BACK; > > > > + else if (cam.location == "external") > > > > + return CAMERA_FACING_EXTERNAL; > > > > + > > > > + LOG(HALConfig, Error) << "Unsupported camera location " << cam.location; > > > > + return -EINVAL; > > > > > > Can we do this conversion when parsing the configuration file instead ? > > > That will ensure at parse time that the data is valid, instead of > > > delaying the check to later. The rotation is already handled that way, > > > and you already check that the location string is valid in > > > CameraHalConfig::parseCameraProps(). > > > > > > > +} > > > > + > > > > +unsigned int CameraHalConfig::cameraRotation(const std::string &camera) const > > > > +{ > > > > + const auto &it = cameras_.find(camera); > > > > + if (it == cameras_.end()) { > > > > + LOG(HALConfig, Error) > > > > + << "Camera '" << camera > > > > + << "' not described in the HAL configuration file"; > > > > + return 0; > > > > + } > > > > + > > > > + const CameraProps &cam = it->second; > > > > + return cam.rotation; > > > > +} > > > > + > > > > +const std::string &CameraHalConfig::cameraModel(const std::string &camera) const > > > > +{ > > > > + static const std::string empty(""); > > > > + const auto &it = cameras_.find(camera); > > > > + if (it == cameras_.end()) { > > > > + LOG(HALConfig, Error) > > > > + << "Camera '" << camera > > > > + << "' not described in the HAL configuration file"; > > > > + return empty; > > > > + } > > > > + > > > > + const CameraProps &cam = it->second; > > > > + return cam.model; > > > > +} > > > > + > > > > +std::string CameraHalConfig::parseValue(yaml_token_t &token) > > > > +{ > > > > + /* Make sure the token type is a value and get its content. */ > > > > + yaml_parser_scan(&parser_, &token); > > > > + if (token.type != YAML_VALUE_TOKEN) { > > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > > + return ""; > > > > + } > > > > + yaml_token_delete(&token); > > > > > > Does the token need to be deleted in the error cases above and below ? > > > > > > > + > > > > + yaml_parser_scan(&parser_, &token); > > > > + if (token.type != YAML_SCALAR_TOKEN) { > > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > > + return ""; > > > > + } > > > > + > > > > + char *scalar = reinterpret_cast<char *>(token.data.scalar.value); > > > > + std::string value(scalar, token.data.scalar.length); > > > > + yaml_token_delete(&token); > > > > + > > > > + return value; > > > > +} > > > > + > > > > +std::string CameraHalConfig::parseKey(yaml_token_t &token) > > > > +{ > > > > + /* Make sure the token type is a key and get its value. */ > > > > + yaml_parser_scan(&parser_, &token); > > > > + if (token.type != YAML_SCALAR_TOKEN) { > > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > > + return ""; > > > > + } > > > > + > > > > + char *scalar = reinterpret_cast<char *>(token.data.scalar.value); > > > > + std::string key(scalar, token.data.scalar.length); > > > > + yaml_token_delete(&token); > > > > + > > > > + return key; > > > > +} > > > > + > > > > +int CameraHalConfig::parseCameraProps(yaml_token_t &token, > > > > + const std::string &cameraID) > > > > +{ > > > > + yaml_parser_scan(&parser_, &token); > > > > + if (token.type != YAML_VALUE_TOKEN) { > > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > > + return -EINVAL; > > > > + } > > > > + yaml_token_delete(&token); > > > > + > > > > + yaml_parser_scan(&parser_, &token); > > > > + if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) { > > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > > + return -EINVAL; > > > > + } > > > > + yaml_token_delete(&token); > > > > + > > > > + /* > > > > + * Parse the camera properties and store them in a cameraBlock instance. > > > > + * > > > > + * Add a safety counter to make sure we don't loop indefinitely in case > > > > + * the configuration file is malformed. > > > > + */ > > > > + unsigned int sentinel = 100; > > > > + CameraProps cameraBlock{}; > > > > + bool blockEnd = false; > > > > + do { > > > > + yaml_parser_scan(&parser_, &token); > > > > + switch (token.type) { > > > > + case YAML_KEY_TOKEN: { > > > > + yaml_token_delete(&token); > > > > + > > > > + /* > > > > + * Parse the camera property key and make sure it is > > > > + * valid. > > > > + */ > > > > + std::string key = parseKey(token); > > > > + std::string value = parseValue(token); > > > > + if (key.empty() || value.empty()) { > > > > + LOG(HALConfig, Error) > > > > + << "Configuration file is not valid"; > > > > + return -EINVAL; > > > > + } > > > > + > > > > + if (key == "location") { > > > > + if (value != "front" && value != "back" && > > > > + value != "external") { > > > > + LOG(HALConfig, Error) > > > > + << "Unknown location: " << value; > > > > + return -EINVAL; > > > > + } > > > > + cameraBlock.location = value; > > > > + } else if (key == "rotation") { > > > > + cameraBlock.rotation = strtoul(value.c_str(), > > > > + NULL, 10); > > > > + if (cameraBlock.rotation < 0) { > > > > + LOG(HALConfig, Error) > > > > + << "Unknown rotation: " > > > > + << cameraBlock.rotation; > > > > + return -EINVAL; > > > > + } > > > > + } else if (key == "model") { > > > > + cameraBlock.model = value; > > > > + } else { > > > > + LOG(HALConfig, Error) > > > > + << "Configuration file is not valid " > > > > + << "unknown key: " << key; > > > > + return -EINVAL; > > > > + } > > > > + break; > > > > + } > > > > + case YAML_BLOCK_END_TOKEN: > > > > + yaml_token_delete(&token); > > > > + blockEnd = true; > > > > + break; > > > > + default: > > > > + yaml_token_delete(&token); > > > > + break; > > > > + } > > > > + > > > > + --sentinel; > > > > + } while (!blockEnd && sentinel); > > > > > > Missing blank line. > > > > > > > + if (!sentinel) { > > > > + LOG(HALConfig, Error) << "Configuration file is not valid "; > > > > + return -EINVAL; > > > > + } > > > > + > > > > + cameras_[cameraID] = cameraBlock; > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +int CameraHalConfig::parseCameras(yaml_token_t &token) > > > > +{ > > > > + int ret; > > > > + > > > > + /* The 'cameras' key maps a BLOCK_MAPPING_START block. */ > > > > + yaml_parser_scan(&parser_, &token); > > > > + if (token.type != YAML_VALUE_TOKEN) { > > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > > + return -EINVAL; > > > > + } > > > > + yaml_token_delete(&token); > > > > + > > > > + yaml_parser_scan(&parser_, &token); > > > > + if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) { > > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > > + return -EINVAL; > > > > + } > > > > + yaml_token_delete(&token); > > > > + > > > > + /* > > > > + * Parse the camera properties. > > > > + * > > > > + * Each camera properties block is a list of properties associated > > > > + * with the ID (as assembled by CameraSensor::generateId()) of the > > > > + * camera they refer to. > > > > + * > > > > + * cameras: > > > > + * "camera0 id": > > > > + * key: value > > > > + * key: value > > > > + * ... > > > > + * > > > > + * "camera1 id": > > > > + * key: value > > > > + * key: value > > > > + * ... > > > > + */ > > > > + bool blockEnd = false; > > > > + do { > > > > + yaml_parser_scan(&parser_, &token); > > > > + switch (token.type) { > > > > + case YAML_KEY_TOKEN: { > > > > + yaml_token_delete(&token); > > > > + > > > > + /* Parse the camera ID as key of the property list. */ > > > > + std::string cameraId = parseKey(token); > > > > + if (cameraId.empty()) { > > > > + LOG(HALConfig, Error) > > > > + << "Configuration file is not valid"; > > > > + return -EINVAL; > > > > + } > > > > + > > > > + ret = parseCameraProps(token, cameraId); > > > > + if (ret) { > > > > + LOG(HALConfig, Error) > > > > + << "Configuration file is not valid"; > > > > + return -EINVAL; > > > > + } > > > > + break; > > > > + } > > > > + case YAML_BLOCK_END_TOKEN: > > > > + yaml_token_delete(&token); > > > > + blockEnd = true; > > > > + break; > > > > + default: > > > > + yaml_token_delete(&token); > > > > + LOG(HALConfig, Error) > > > > + << "Configuration file is not valid"; > > > > + return -EINVAL; > > > > + } > > > > + } while (!blockEnd); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +int CameraHalConfig::parseEntry(yaml_token_t &token) > > > > +{ > > > > + int ret; > > > > + > > > > + /* > > > > + * Parse each key we find in the file. > > > > + * > > > > + * Keys like 'model' and 'maker' are device properties and gets > > > > + * stored as class members. > > > > + * > > > > + * The 'cameras' keys maps to a list of (lists) of camera properties. > > > > + */ > > > > + > > > > + std::string key = parseKey(token); > > > > + if (key.empty()) { > > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > > + return -EINVAL; > > > > + } > > > > + > > > > + if (key == "cameras") { > > > > + ret = parseCameras(token); > > > > + if (ret) > > > > + return ret; > > > > + } else if (key == "manufacturer") { > > > > + maker_ = parseValue(token); > > > > + if (maker_.empty()) { > > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > > + return -EINVAL; > > > > + } > > > > + } else if (key == "model") { > > > > + model_ = parseValue(token); > > > > + if (model_.empty()) { > > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > > + return -EINVAL; > > > > + } > > > > + } else { > > > > + LOG(HALConfig, Error) << "Unknown key " << key; > > > > + return -EINVAL; > > > > + } > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +int CameraHalConfig::parseConfigFile() > > > > +{ > > > > + yaml_token_t token; > > > > + int ret; > > > > + > > > > + yaml_parser_scan(&parser_, &token); > > > > + if (token.type != YAML_STREAM_START_TOKEN) { > > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > > + return -EINVAL; > > > > + } > > > > + yaml_token_delete(&token); > > > > + > > > > + yaml_parser_scan(&parser_, &token); > > > > + if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) { > > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > > + return -EINVAL; > > > > + } > > > > + yaml_token_delete(&token); > > > > + > > > > + /* Parse the file and parse each single key one by one. */ > > > > + do { > > > > + yaml_parser_scan(&parser_, &token); > > > > + switch (token.type) { > > > > + case YAML_KEY_TOKEN: > > > > + yaml_token_delete(&token); > > > > + ret = parseEntry(token); > > > > > > Do you need to pass the token to parseEntry(), given that you've just > > > deleted it ? It looks to me like the token is only the return value of > > > yaml_parse_scan(), and can thus be a local variable everywhere. > > > > > > > + break; > > > > + case YAML_STREAM_END_TOKEN: > > > > + /* Resources are released after the loop exit. */ > > > > + break; > > > > + default: > > > > + yaml_token_delete(&token); > > > > + break; > > > > > > The YAML_STREAM_END_TOKEN and default case are different, but the > > > while() condition below accesses token.type. Is that intended ? > > > > > > > + } > > > > + } while (token.type != YAML_STREAM_END_TOKEN && ret >= 0); > > > > + yaml_token_delete(&token); > > > > + > > > > + return ret; > > > > +} > > > > diff --git a/src/android/camera_hal_config.h b/src/android/camera_hal_config.h > > > > new file mode 100644 > > > > index 000000000000..ce77b1ee1582 > > > > --- /dev/null > > > > +++ b/src/android/camera_hal_config.h > > > > @@ -0,0 +1,53 @@ > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > > +/* > > > > + * Copyright (C) 2021, Google Inc. > > > > + * > > > > + * camera_hal_config.h - Camera HAL configuration file manager > > > > + */ > > > > +#ifndef __ANDROID_CAMERA_HAL_CONFIG_H__ > > > > +#define __ANDROID_CAMERA_HAL_CONFIG_H__ > > > > + > > > > +#include <map> > > > > +#include <string> > > > > +#include <yaml.h> > > > > > > As commented in v1, hiding the parser would be nice, but it can be done > > > later, on top. > > > > > > > + > > > > +class CameraHalConfig > > > > +{ > > > > +public: > > > > + CameraHalConfig(); > > > > + ~CameraHalConfig(); > > > > + > > > > + int open(); > > > > + > > > > + const std::string &deviceModel() const { return model_; } > > > > + const std::string &deviceMaker() const { return maker_; } > > > > + > > > > + int cameraLocation(const std::string &camera) const; > > > > + unsigned int cameraRotation(const std::string &camera) const; > > > > + const std::string &cameraModel(const std::string &camera) const; > > > > + > > > > +private: > > > > + yaml_parser_t parser_; > > > > + > > > > + std::string model_; > > > > + std::string maker_; > > > > + class CameraProps > > > > + { > > > > + public: > > > > + std::string location; > > > > + std::string model; > > > > + unsigned int rotation; > > > > + }; > > > > + std::map<std::string, CameraProps> cameras_; > > > > + > > > > + std::string findFilePath(const std::string &filename) const; > > > > + FILE *openFile(const std::string &filename); > > > > + std::string parseValue(yaml_token_t &token); > > > > + std::string parseKey(yaml_token_t &token); > > > > + int parseCameraProps(yaml_token_t &token, const std::string &cameraID); > > > > + int parseCameras(yaml_token_t &token); > > > > + int parseEntry(yaml_token_t &token); > > > > + int parseConfigFile(); > > > > +}; > > > > + > > > > +#endif /* __ANDROID_CAMERA_HAL_CONFIG_H__ */ > > > > diff --git a/src/android/meson.build b/src/android/meson.build > > > > index 8e7d07d9be3c..c0ede407bc38 100644 > > > > --- a/src/android/meson.build > > > > +++ b/src/android/meson.build > > > > @@ -3,6 +3,7 @@ > > > > android_deps = [ > > > > dependency('libexif', required : get_option('android')), > > > > dependency('libjpeg', required : get_option('android')), > > > > + dependency('yaml-0.1', required : get_option('android')), > > > > ] > > > > > > > > android_enabled = true > > > > @@ -41,6 +42,7 @@ android_hal_sources = files([ > > > > 'camera3_hal.cpp', > > > > 'camera_hal_manager.cpp', > > > > 'camera_device.cpp', > > > > + 'camera_hal_config.cpp', > > > > > > While at it, could you move camera_hal_manager to the right place ? > > > > > > > 'camera_metadata.cpp', > > > > 'camera_ops.cpp', > > > > 'camera_stream.cpp', > > -- > Regards, > > Laurent Pinchart
Hi Laurent, On Tue, Mar 30, 2021 at 03:39:56AM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Mon, Mar 29, 2021 at 05:28:04PM +0200, Jacopo Mondi wrote: > > Add a CameraHalConfig class to the Android Camera3 HAL layer. > > There are quite a few comments from v1 that haven't been addressed. > Maybe that's because my last reply to v1 came after you've sent this > series. The comments may also not all be correct, applicable and/or > desired, but could you then reply to them to explain why they're ignored > ? > > In particular, I think that exposing CameraProps would simplify the > implementation quite a bit, by removing the need for the camera*() > functions, with the lookup of the CameraProps entry every time, and the > associated error handling. > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > README.rst | 2 +- > > src/android/camera_hal_config.cpp | 463 ++++++++++++++++++++++++++++++ > > src/android/camera_hal_config.h | 53 ++++ > > src/android/meson.build | 2 + > > 4 files changed, 519 insertions(+), 1 deletion(-) > > create mode 100644 src/android/camera_hal_config.cpp > > create mode 100644 src/android/camera_hal_config.h > > > > diff --git a/README.rst b/README.rst > > index 7a98164bbb0a..f68d435196de 100644 > > --- a/README.rst > > +++ b/README.rst > > @@ -88,7 +88,7 @@ for tracing with lttng: [optional] > > liblttng-ust-dev python3-jinja2 lttng-tools > > > > for android: [optional] > > - libexif libjpeg > > + libexif libjpeg libyaml > > > > Using GStreamer plugin > > ~~~~~~~~~~~~~~~~~~~~~~ > > diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp > > new file mode 100644 > > index 000000000000..846dd7357b0a > > --- /dev/null > > +++ b/src/android/camera_hal_config.cpp > > @@ -0,0 +1,463 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2021, Google Inc. > > + * > > + * camera_hal_config.cpp - Camera HAL configuration file manager > > + */ > > +#include "camera_hal_config.h" > > + > > +#include <stdio.h> > > +#include <stdlib.h> > > +#include <string.h> > > + > > +#include <hardware/camera3.h> > > + > > +#include "libcamera/internal/file.h" > > +#include "libcamera/internal/log.h" > > + > > +using namespace libcamera; > > + > > +LOG_DEFINE_CATEGORY(HALConfig) > > + > > +CameraHalConfig::CameraHalConfig() > > +{ > > + if (!yaml_parser_initialize(&parser_)) > > + LOG(HALConfig, Fatal) << "Failed to initialize yaml parser"; > > +} > > + > > +CameraHalConfig::~CameraHalConfig() > > +{ > > + yaml_parser_delete(&parser_); > > +} > > + > > +/* > > + * Configuration files can be stored in system paths, which are identified > > + * through the build configuration. > > + * > > + * However, when running uninstalled - the source location takes precedence. > > + */ > > +std::string CameraHalConfig::findFilePath(const std::string &filename) const > > +{ > > + static std::array<std::string, 2> searchPaths = { > > + LIBCAMERA_SYSCONF_DIR, > > + LIBCAMERA_DATA_DIR, > > There's also a comment in v1 about data dir. Data dir is typically > /usr/share/libcamera/. Given that the configuration file is > device-specific, I don't think we'll ever look for it there, but only in > sysconf dir. > > > + }; > > + > > + if (File::exists(filename)) > > + return filename; > > + > > + std::string root = utils::libcameraSourcePath(); > > + if (!root.empty()) { > > + std::string configurationPath = root + "data/" + filename; > > + if (File::exists(configurationPath)) > > + return configurationPath; > > + } > > + > > + for (std::string &path : searchPaths) { > > + std::string configurationPath = path + "/" + filename; > > + if (File::exists(configurationPath)) > > + return configurationPath; > > + } > > + > > + return ""; > > +} > > + > > +FILE *CameraHalConfig::openFile(const std::string &filename) > > +{ > > + const std::string filePath = findFilePath(filename); > > + if (filePath.empty()) { > > + LOG(HALConfig, Error) > > + << "Configuration file: \"" << filename << "\" not found"; > > + return nullptr; > > + } > > + > > + LOG(HALConfig, Debug) << "Reading configuration file from " << filePath; > > + > > + FILE *fh = fopen(filePath.c_str(), "r"); > > + if (!fh) { > > + int ret = -errno; > > + LOG(HALConfig, Error) << "Failed to open configuration file " > > + << filePath << ": " << strerror(-ret); > > + return nullptr; > > + } > > + > > + return fh; > > +} > > + > > +/* > > + * Open the HAL configuration file and validate its content. > > + * Return 0 on success, a negative error code otherwise > > + */ > > +int CameraHalConfig::open() > > +{ > > + int ret; > > + > > + FILE *fh = openFile("camera_hal.yaml"); > > + if (!fh) > > + return -ENOENT; > > + > > + yaml_parser_set_input_file(&parser_, fh); > > + ret = parseConfigFile(); > > + fclose(fh); > > + if (ret) > > + return ret; > > + > > + LOG(HALConfig, Info) << "Device model: " << model_; > > + LOG(HALConfig, Info) << "Device maker: " << maker_; > > + for (const auto &c : cameras_) { > > + const std::string &cameraId = c.first; > > + const CameraProps &camera = c.second; > > + LOG(HALConfig, Info) << "'" << cameraId << "' " > > + << " model: " << camera.model > > + << "(" << camera.location << ")[" > > + << camera.rotation << "]"; > > + } > > There's also a comment in v1 about whether this should be a debugging > feature. > > > + > > + return 0; > > +} > > + > > +int CameraHalConfig::cameraLocation(const std::string &camera) const > > +{ > > + const auto &it = cameras_.find(camera); > > + if (it == cameras_.end()) { > > + LOG(HALConfig, Error) > > + << "Camera '" << camera > > + << "' not described in the HAL configuration file"; > > + return -EINVAL; > > + } > > + > > + const CameraProps &cam = it->second; > > + if (cam.location.empty()) { > > + LOG(HALConfig, Error) << "Location for camera '" << camera > > + << "' not available"; > > + return -EINVAL; > > + } > > + > > + if (cam.location == "front") > > + return CAMERA_FACING_FRONT; > > + else if (cam.location == "back") > > + return CAMERA_FACING_BACK; > > + else if (cam.location == "external") > > + return CAMERA_FACING_EXTERNAL; > > + > > + LOG(HALConfig, Error) << "Unsupported camera location " << cam.location; > > + return -EINVAL; > > Can we do this conversion when parsing the configuration file instead ? > That will ensure at parse time that the data is valid, instead of > delaying the check to later. The rotation is already handled that way, > and you already check that the location string is valid in > CameraHalConfig::parseCameraProps(). > ack > > +} > > + > > +unsigned int CameraHalConfig::cameraRotation(const std::string &camera) const > > +{ > > + const auto &it = cameras_.find(camera); > > + if (it == cameras_.end()) { > > + LOG(HALConfig, Error) > > + << "Camera '" << camera > > + << "' not described in the HAL configuration file"; > > + return 0; > > + } > > + > > + const CameraProps &cam = it->second; > > + return cam.rotation; > > +} > > + > > +const std::string &CameraHalConfig::cameraModel(const std::string &camera) const > > +{ > > + static const std::string empty(""); > > + const auto &it = cameras_.find(camera); > > + if (it == cameras_.end()) { > > + LOG(HALConfig, Error) > > + << "Camera '" << camera > > + << "' not described in the HAL configuration file"; > > + return empty; > > + } > > + > > + const CameraProps &cam = it->second; > > + return cam.model; > > +} > > + > > +std::string CameraHalConfig::parseValue(yaml_token_t &token) > > +{ > > + /* Make sure the token type is a value and get its content. */ > > + yaml_parser_scan(&parser_, &token); > > + if (token.type != YAML_VALUE_TOKEN) { > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > + return ""; > > + } > > + yaml_token_delete(&token); > > Does the token need to be deleted in the error cases above and below ? > Ah yes, most probably. I thought it had to be deleted if resued, but it seems deletion is just about releasing resources allocated during parser_scan(), so it needs to be done in error paths too I hate I can't goto easily in C++ to implement saner error handling paths > > + > > + yaml_parser_scan(&parser_, &token); > > + if (token.type != YAML_SCALAR_TOKEN) { > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > + return ""; > > + } > > + > > + char *scalar = reinterpret_cast<char *>(token.data.scalar.value); > > + std::string value(scalar, token.data.scalar.length); > > + yaml_token_delete(&token); > > + > > + return value; > > +} > > + > > +std::string CameraHalConfig::parseKey(yaml_token_t &token) > > +{ > > + /* Make sure the token type is a key and get its value. */ > > + yaml_parser_scan(&parser_, &token); > > + if (token.type != YAML_SCALAR_TOKEN) { > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > + return ""; > > + } > > + > > + char *scalar = reinterpret_cast<char *>(token.data.scalar.value); > > + std::string key(scalar, token.data.scalar.length); > > + yaml_token_delete(&token); > > + > > + return key; > > +} > > + > > +int CameraHalConfig::parseCameraProps(yaml_token_t &token, > > + const std::string &cameraID) > > +{ > > + yaml_parser_scan(&parser_, &token); > > + if (token.type != YAML_VALUE_TOKEN) { > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > + return -EINVAL; > > + } > > + yaml_token_delete(&token); > > + > > + yaml_parser_scan(&parser_, &token); > > + if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) { > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > + return -EINVAL; > > + } > > + yaml_token_delete(&token); > > + > > + /* > > + * Parse the camera properties and store them in a cameraBlock instance. > > + * > > + * Add a safety counter to make sure we don't loop indefinitely in case > > + * the configuration file is malformed. > > + */ > > + unsigned int sentinel = 100; > > + CameraProps cameraBlock{}; > > + bool blockEnd = false; > > + do { > > + yaml_parser_scan(&parser_, &token); > > + switch (token.type) { > > + case YAML_KEY_TOKEN: { > > + yaml_token_delete(&token); > > + > > + /* > > + * Parse the camera property key and make sure it is > > + * valid. > > + */ > > + std::string key = parseKey(token); > > + std::string value = parseValue(token); > > + if (key.empty() || value.empty()) { > > + LOG(HALConfig, Error) > > + << "Configuration file is not valid"; > > + return -EINVAL; > > + } > > + > > + if (key == "location") { > > + if (value != "front" && value != "back" && > > + value != "external") { > > + LOG(HALConfig, Error) > > + << "Unknown location: " << value; > > + return -EINVAL; > > + } > > + cameraBlock.location = value; > > + } else if (key == "rotation") { > > + cameraBlock.rotation = strtoul(value.c_str(), > > + NULL, 10); > > + if (cameraBlock.rotation < 0) { > > + LOG(HALConfig, Error) > > + << "Unknown rotation: " > > + << cameraBlock.rotation; > > + return -EINVAL; > > + } > > + } else if (key == "model") { > > + cameraBlock.model = value; > > + } else { > > + LOG(HALConfig, Error) > > + << "Configuration file is not valid " > > + << "unknown key: " << key; > > + return -EINVAL; > > + } > > + break; > > + } > > + case YAML_BLOCK_END_TOKEN: > > + yaml_token_delete(&token); > > + blockEnd = true; > > + break; > > + default: > > + yaml_token_delete(&token); > > + break; > > + } > > + > > + --sentinel; > > + } while (!blockEnd && sentinel); > > Missing blank line. > Intentional > > + if (!sentinel) { > > + LOG(HALConfig, Error) << "Configuration file is not valid "; > > + return -EINVAL; > > + } > > + > > + cameras_[cameraID] = cameraBlock; > > + > > + return 0; > > +} > > + > > +int CameraHalConfig::parseCameras(yaml_token_t &token) > > +{ > > + int ret; > > + > > + /* The 'cameras' key maps a BLOCK_MAPPING_START block. */ > > + yaml_parser_scan(&parser_, &token); > > + if (token.type != YAML_VALUE_TOKEN) { > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > + return -EINVAL; > > + } > > + yaml_token_delete(&token); > > + > > + yaml_parser_scan(&parser_, &token); > > + if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) { > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > + return -EINVAL; > > + } > > + yaml_token_delete(&token); > > + > > + /* > > + * Parse the camera properties. > > + * > > + * Each camera properties block is a list of properties associated > > + * with the ID (as assembled by CameraSensor::generateId()) of the > > + * camera they refer to. > > + * > > + * cameras: > > + * "camera0 id": > > + * key: value > > + * key: value > > + * ... > > + * > > + * "camera1 id": > > + * key: value > > + * key: value > > + * ... > > + */ > > + bool blockEnd = false; > > + do { > > + yaml_parser_scan(&parser_, &token); > > + switch (token.type) { > > + case YAML_KEY_TOKEN: { > > + yaml_token_delete(&token); > > + > > + /* Parse the camera ID as key of the property list. */ > > + std::string cameraId = parseKey(token); > > + if (cameraId.empty()) { > > + LOG(HALConfig, Error) > > + << "Configuration file is not valid"; > > + return -EINVAL; > > + } > > + > > + ret = parseCameraProps(token, cameraId); > > + if (ret) { > > + LOG(HALConfig, Error) > > + << "Configuration file is not valid"; > > + return -EINVAL; > > + } > > + break; > > + } > > + case YAML_BLOCK_END_TOKEN: > > + yaml_token_delete(&token); > > + blockEnd = true; > > + break; > > + default: > > + yaml_token_delete(&token); > > + LOG(HALConfig, Error) > > + << "Configuration file is not valid"; > > + return -EINVAL; > > + } > > + } while (!blockEnd); > > + > > + return 0; > > +} > > + > > +int CameraHalConfig::parseEntry(yaml_token_t &token) > > +{ > > + int ret; > > + > > + /* > > + * Parse each key we find in the file. > > + * > > + * Keys like 'model' and 'maker' are device properties and gets > > + * stored as class members. > > + * > > + * The 'cameras' keys maps to a list of (lists) of camera properties. > > + */ > > + > > + std::string key = parseKey(token); > > + if (key.empty()) { > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > + return -EINVAL; > > + } > > + > > + if (key == "cameras") { > > + ret = parseCameras(token); > > + if (ret) > > + return ret; > > + } else if (key == "manufacturer") { > > + maker_ = parseValue(token); > > + if (maker_.empty()) { > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > + return -EINVAL; > > + } > > + } else if (key == "model") { > > + model_ = parseValue(token); > > + if (model_.empty()) { > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > + return -EINVAL; > > + } > > + } else { > > + LOG(HALConfig, Error) << "Unknown key " << key; > > + return -EINVAL; > > + } > > + > > + return 0; > > +} > > + > > +int CameraHalConfig::parseConfigFile() > > +{ > > + yaml_token_t token; > > + int ret; > > + > > + yaml_parser_scan(&parser_, &token); > > + if (token.type != YAML_STREAM_START_TOKEN) { > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > + return -EINVAL; > > + } > > + yaml_token_delete(&token); > > + > > + yaml_parser_scan(&parser_, &token); > > + if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) { > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > + return -EINVAL; > > + } > > + yaml_token_delete(&token); > > + > > + /* Parse the file and parse each single key one by one. */ > > + do { > > + yaml_parser_scan(&parser_, &token); > > + switch (token.type) { > > + case YAML_KEY_TOKEN: > > + yaml_token_delete(&token); > > + ret = parseEntry(token); > > Do you need to pass the token to parseEntry(), given that you've just > deleted it ? It looks to me like the token is only the return value of > yaml_parse_scan(), and can thus be a local variable everywhere. > Good question. Where's the parsing state kept ? in the token or in the parser ? If it's in the parser I can avoid passing token around > > + break; > > + case YAML_STREAM_END_TOKEN: > > + /* Resources are released after the loop exit. */ > > + break; > > + default: > > + yaml_token_delete(&token); > > + break; > > The YAML_STREAM_END_TOKEN and default case are different, but the > while() condition below accesses token.type. Is that intended ? > Yes, the type has to stay valid in case of STREAM_END to be verified below. I can handle this with just ret if you prefer. Thanks j > > + } > > + } while (token.type != YAML_STREAM_END_TOKEN && ret >= 0); > > + yaml_token_delete(&token); > > + > > + return ret; > > +} > > diff --git a/src/android/camera_hal_config.h b/src/android/camera_hal_config.h > > new file mode 100644 > > index 000000000000..ce77b1ee1582 > > --- /dev/null > > +++ b/src/android/camera_hal_config.h > > @@ -0,0 +1,53 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2021, Google Inc. > > + * > > + * camera_hal_config.h - Camera HAL configuration file manager > > + */ > > +#ifndef __ANDROID_CAMERA_HAL_CONFIG_H__ > > +#define __ANDROID_CAMERA_HAL_CONFIG_H__ > > + > > +#include <map> > > +#include <string> > > +#include <yaml.h> > > As commented in v1, hiding the parser would be nice, but it can be done > later, on top. > > > + > > +class CameraHalConfig > > +{ > > +public: > > + CameraHalConfig(); > > + ~CameraHalConfig(); > > + > > + int open(); > > + > > + const std::string &deviceModel() const { return model_; } > > + const std::string &deviceMaker() const { return maker_; } > > + > > + int cameraLocation(const std::string &camera) const; > > + unsigned int cameraRotation(const std::string &camera) const; > > + const std::string &cameraModel(const std::string &camera) const; > > + > > +private: > > + yaml_parser_t parser_; > > + > > + std::string model_; > > + std::string maker_; > > + class CameraProps > > + { > > + public: > > + std::string location; > > + std::string model; > > + unsigned int rotation; > > + }; > > + std::map<std::string, CameraProps> cameras_; > > + > > + std::string findFilePath(const std::string &filename) const; > > + FILE *openFile(const std::string &filename); > > + std::string parseValue(yaml_token_t &token); > > + std::string parseKey(yaml_token_t &token); > > + int parseCameraProps(yaml_token_t &token, const std::string &cameraID); > > + int parseCameras(yaml_token_t &token); > > + int parseEntry(yaml_token_t &token); > > + int parseConfigFile(); > > +}; > > + > > +#endif /* __ANDROID_CAMERA_HAL_CONFIG_H__ */ > > diff --git a/src/android/meson.build b/src/android/meson.build > > index 8e7d07d9be3c..c0ede407bc38 100644 > > --- a/src/android/meson.build > > +++ b/src/android/meson.build > > @@ -3,6 +3,7 @@ > > android_deps = [ > > dependency('libexif', required : get_option('android')), > > dependency('libjpeg', required : get_option('android')), > > + dependency('yaml-0.1', required : get_option('android')), > > ] > > > > android_enabled = true > > @@ -41,6 +42,7 @@ android_hal_sources = files([ > > 'camera3_hal.cpp', > > 'camera_hal_manager.cpp', > > 'camera_device.cpp', > > + 'camera_hal_config.cpp', > > While at it, could you move camera_hal_manager to the right place ? > > > 'camera_metadata.cpp', > > 'camera_ops.cpp', > > 'camera_stream.cpp', > > -- > Regards, > > Laurent Pinchart
Hi Hiro, On Tue, Mar 30, 2021 at 01:16:22PM +0900, Hirokazu Honda wrote: > On Tue, Mar 30, 2021 at 12:53 PM Laurent Pinchart wrote: > > On Tue, Mar 30, 2021 at 12:48:20PM +0900, Hirokazu Honda wrote: > > > On Tue, Mar 30, 2021 at 9:40 AM Laurent Pinchartwrote: > > > > On Mon, Mar 29, 2021 at 05:28:04PM +0200, Jacopo Mondi wrote: > > > > > Add a CameraHalConfig class to the Android Camera3 HAL layer. > > > > > > > > There are quite a few comments from v1 that haven't been addressed. > > > > Maybe that's because my last reply to v1 came after you've sent this > > > > series. The comments may also not all be correct, applicable and/or > > > > desired, but could you then reply to them to explain why they're ignored > > > > ? > > > > > > > > In particular, I think that exposing CameraProps would simplify the > > > > implementation quite a bit, by removing the need for the camera*() > > > > functions, with the lookup of the CameraProps entry every time, and the > > > > associated error handling. > > > > > > +1. So we should pass the structure to CameraDevice by value. > > > > Why not by const reference ? > > I would not like having const reference in member variables. > I actually thought this was stated in Google C++ Style Guide, and > searched it, but found there was no statement like that. > So this is just my preference. > For the purpose, I usually use a const pointer. > Considering why I don't like having const reference member variables, > one of reasons might be what I don't get used to :p, but also from a > caller of the constructor the lifetime of CameraHalConfig is unclear. > For instance, CameraDevice(.., config) is called, but a caller has no > idea when the config should be alive by, while CameraDevice(..., > &config) tells a caller to keep the config object outlive to > CameraDevice. We have coding style rules to handle borrowed references, explained in Documentation/coding-style.rst: When the object is stored in a std::unique_ptr<>, borrowing passes a reference to the object, not to the std::unique_ptr<>, as * a 'const &' when the object doesn't need to be modified and may not be null. * a pointer when the object may be modified or may be null. Unless otherwise specified, pointers passed to functions are considered as borrowed references valid for the duration of the function only. (I've just realized that we may want to update this, as the rule isn't specific to std::unique_ptr<>, it can apply equally to, for instance, member variables of a class.) The difference between const reference and non-const pointer is not related to object ownership, but to constness. We tend to favour pointers for variables that can be modified by the callee, and const references for variables that can't. This means that we can't use reference vs. pointer to express ownership and life time of objects. This can of course be reconsidered, but I'd like the coding style to remain consistent, and thus first update the documentation and the code base. > Again, this might be my personal preference. But does this description > make sense? > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > --- > > > > > README.rst | 2 +- > > > > > src/android/camera_hal_config.cpp | 463 ++++++++++++++++++++++++++++++ > > > > > src/android/camera_hal_config.h | 53 ++++ > > > > > src/android/meson.build | 2 + > > > > > 4 files changed, 519 insertions(+), 1 deletion(-) > > > > > create mode 100644 src/android/camera_hal_config.cpp > > > > > create mode 100644 src/android/camera_hal_config.h > > > > > > > > > > diff --git a/README.rst b/README.rst > > > > > index 7a98164bbb0a..f68d435196de 100644 > > > > > --- a/README.rst > > > > > +++ b/README.rst > > > > > @@ -88,7 +88,7 @@ for tracing with lttng: [optional] > > > > > liblttng-ust-dev python3-jinja2 lttng-tools > > > > > > > > > > for android: [optional] > > > > > - libexif libjpeg > > > > > + libexif libjpeg libyaml > > > > > > > > > > Using GStreamer plugin > > > > > ~~~~~~~~~~~~~~~~~~~~~~ > > > > > diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp > > > > > new file mode 100644 > > > > > index 000000000000..846dd7357b0a > > > > > --- /dev/null > > > > > +++ b/src/android/camera_hal_config.cpp > > > > > @@ -0,0 +1,463 @@ > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > > > +/* > > > > > + * Copyright (C) 2021, Google Inc. > > > > > + * > > > > > + * camera_hal_config.cpp - Camera HAL configuration file manager > > > > > + */ > > > > > +#include "camera_hal_config.h" > > > > > + > > > > > +#include <stdio.h> > > > > > +#include <stdlib.h> > > > > > +#include <string.h> > > > > > + > > > > > +#include <hardware/camera3.h> > > > > > + > > > > > +#include "libcamera/internal/file.h" > > > > > +#include "libcamera/internal/log.h" > > > > > + > > > > > +using namespace libcamera; > > > > > + > > > > > +LOG_DEFINE_CATEGORY(HALConfig) > > > > > + > > > > > +CameraHalConfig::CameraHalConfig() > > > > > +{ > > > > > + if (!yaml_parser_initialize(&parser_)) > > > > > + LOG(HALConfig, Fatal) << "Failed to initialize yaml parser"; > > > > > +} > > > > > + > > > > > +CameraHalConfig::~CameraHalConfig() > > > > > +{ > > > > > + yaml_parser_delete(&parser_); > > > > > +} > > > > > + > > > > > > It looks strange to keep parser_ alive as long as CameraHalConfig, > > > because parsing is done in open(). > > > I would initialize the parser in open() and destroy it in the end of open(). > > > In this direction, we would prefer moving parse* functions to .cpp > > > file and also moving yaml include to .cpp. > > > > I've mentioned that before :-) It could be done on top though. > > > > > > > +/* > > > > > + * Configuration files can be stored in system paths, which are identified > > > > > + * through the build configuration. > > > > > + * > > > > > + * However, when running uninstalled - the source location takes precedence. > > > > > + */ > > > > > +std::string CameraHalConfig::findFilePath(const std::string &filename) const > > > > > +{ > > > > > + static std::array<std::string, 2> searchPaths = { > > > > > + LIBCAMERA_SYSCONF_DIR, > > > > > + LIBCAMERA_DATA_DIR, > > > > > > > > There's also a comment in v1 about data dir. Data dir is typically > > > > /usr/share/libcamera/. Given that the configuration file is > > > > device-specific, I don't think we'll ever look for it there, but only in > > > > sysconf dir. > > > > > > > > > + }; > > > > > + > > > > > + if (File::exists(filename)) > > > > > + return filename; > > > > > + > > > > > + std::string root = utils::libcameraSourcePath(); > > > > > + if (!root.empty()) { > > > > > + std::string configurationPath = root + "data/" + filename; > > > > > + if (File::exists(configurationPath)) > > > > > + return configurationPath; > > > > > + } > > > > > + > > > > > + for (std::string &path : searchPaths) { > > > > > + std::string configurationPath = path + "/" + filename; > > > > > + if (File::exists(configurationPath)) > > > > > + return configurationPath; > > > > > + } > > > > > + > > > > > + return ""; > > > > > +} > > > > > + > > > > > +FILE *CameraHalConfig::openFile(const std::string &filename) > > > > > +{ > > > > > + const std::string filePath = findFilePath(filename); > > > > > + if (filePath.empty()) { > > > > > + LOG(HALConfig, Error) > > > > > + << "Configuration file: \"" << filename << "\" not found"; > > > > > + return nullptr; > > > > > + } > > > > > + > > > > > + LOG(HALConfig, Debug) << "Reading configuration file from " << filePath; > > > > > + > > > > > + FILE *fh = fopen(filePath.c_str(), "r"); > > > > > + if (!fh) { > > > > > + int ret = -errno; > > > > > + LOG(HALConfig, Error) << "Failed to open configuration file " > > > > > + << filePath << ": " << strerror(-ret); > > > > > + return nullptr; > > > > > + } > > > > > + > > > > > + return fh; > > > > > +} > > > > > + > > > > > +/* > > > > > + * Open the HAL configuration file and validate its content. > > > > > + * Return 0 on success, a negative error code otherwise > > > > > + */ > > > > > +int CameraHalConfig::open() > > > > > +{ > > > > > + int ret; > > > > > + > > > > > + FILE *fh = openFile("camera_hal.yaml"); > > > > > + if (!fh) > > > > > + return -ENOENT; > > > > > + > > > > > + yaml_parser_set_input_file(&parser_, fh); > > > > > + ret = parseConfigFile(); > > > > > + fclose(fh); > > > > > + if (ret) > > > > > + return ret; > > > > > + > > > > > + LOG(HALConfig, Info) << "Device model: " << model_; > > > > > + LOG(HALConfig, Info) << "Device maker: " << maker_; > > > > > + for (const auto &c : cameras_) { > > > > > + const std::string &cameraId = c.first; > > > > > + const CameraProps &camera = c.second; > > > > > + LOG(HALConfig, Info) << "'" << cameraId << "' " > > > > > + << " model: " << camera.model > > > > > + << "(" << camera.location << ")[" > > > > > + << camera.rotation << "]"; > > > > > + } > > > > > > > > There's also a comment in v1 about whether this should be a debugging > > > > feature. > > > > > > > > > + > > > > > + return 0; > > > > > +} > > > > > + > > > > > +int CameraHalConfig::cameraLocation(const std::string &camera) const > > > > > +{ > > > > > + const auto &it = cameras_.find(camera); > > > > > + if (it == cameras_.end()) { > > > > > + LOG(HALConfig, Error) > > > > > + << "Camera '" << camera > > > > > + << "' not described in the HAL configuration file"; > > > > > + return -EINVAL; > > > > > + } > > > > > + > > > > > + const CameraProps &cam = it->second; > > > > > + if (cam.location.empty()) { > > > > > + LOG(HALConfig, Error) << "Location for camera '" << camera > > > > > + << "' not available"; > > > > > + return -EINVAL; > > > > > + } > > > > > + > > > > > + if (cam.location == "front") > > > > > + return CAMERA_FACING_FRONT; > > > > > + else if (cam.location == "back") > > > > > + return CAMERA_FACING_BACK; > > > > > + else if (cam.location == "external") > > > > > + return CAMERA_FACING_EXTERNAL; > > > > > + > > > > > + LOG(HALConfig, Error) << "Unsupported camera location " << cam.location; > > > > > + return -EINVAL; > > > > > > > > Can we do this conversion when parsing the configuration file instead ? > > > > That will ensure at parse time that the data is valid, instead of > > > > delaying the check to later. The rotation is already handled that way, > > > > and you already check that the location string is valid in > > > > CameraHalConfig::parseCameraProps(). > > > > > > > > > +} > > > > > + > > > > > +unsigned int CameraHalConfig::cameraRotation(const std::string &camera) const > > > > > +{ > > > > > + const auto &it = cameras_.find(camera); > > > > > + if (it == cameras_.end()) { > > > > > + LOG(HALConfig, Error) > > > > > + << "Camera '" << camera > > > > > + << "' not described in the HAL configuration file"; > > > > > + return 0; > > > > > + } > > > > > + > > > > > + const CameraProps &cam = it->second; > > > > > + return cam.rotation; > > > > > +} > > > > > + > > > > > +const std::string &CameraHalConfig::cameraModel(const std::string &camera) const > > > > > +{ > > > > > + static const std::string empty(""); > > > > > + const auto &it = cameras_.find(camera); > > > > > + if (it == cameras_.end()) { > > > > > + LOG(HALConfig, Error) > > > > > + << "Camera '" << camera > > > > > + << "' not described in the HAL configuration file"; > > > > > + return empty; > > > > > + } > > > > > + > > > > > + const CameraProps &cam = it->second; > > > > > + return cam.model; > > > > > +} > > > > > + > > > > > +std::string CameraHalConfig::parseValue(yaml_token_t &token) > > > > > +{ > > > > > + /* Make sure the token type is a value and get its content. */ > > > > > + yaml_parser_scan(&parser_, &token); > > > > > + if (token.type != YAML_VALUE_TOKEN) { > > > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > > > + return ""; > > > > > + } > > > > > + yaml_token_delete(&token); > > > > > > > > Does the token need to be deleted in the error cases above and below ? > > > > > > > > > + > > > > > + yaml_parser_scan(&parser_, &token); > > > > > + if (token.type != YAML_SCALAR_TOKEN) { > > > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > > > + return ""; > > > > > + } > > > > > + > > > > > + char *scalar = reinterpret_cast<char *>(token.data.scalar.value); > > > > > + std::string value(scalar, token.data.scalar.length); > > > > > + yaml_token_delete(&token); > > > > > + > > > > > + return value; > > > > > +} > > > > > + > > > > > +std::string CameraHalConfig::parseKey(yaml_token_t &token) > > > > > +{ > > > > > + /* Make sure the token type is a key and get its value. */ > > > > > + yaml_parser_scan(&parser_, &token); > > > > > + if (token.type != YAML_SCALAR_TOKEN) { > > > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > > > + return ""; > > > > > + } > > > > > + > > > > > + char *scalar = reinterpret_cast<char *>(token.data.scalar.value); > > > > > + std::string key(scalar, token.data.scalar.length); > > > > > + yaml_token_delete(&token); > > > > > + > > > > > + return key; > > > > > +} > > > > > + > > > > > +int CameraHalConfig::parseCameraProps(yaml_token_t &token, > > > > > + const std::string &cameraID) > > > > > +{ > > > > > + yaml_parser_scan(&parser_, &token); > > > > > + if (token.type != YAML_VALUE_TOKEN) { > > > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > > > + return -EINVAL; > > > > > + } > > > > > + yaml_token_delete(&token); > > > > > + > > > > > + yaml_parser_scan(&parser_, &token); > > > > > + if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) { > > > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > > > + return -EINVAL; > > > > > + } > > > > > + yaml_token_delete(&token); > > > > > + > > > > > + /* > > > > > + * Parse the camera properties and store them in a cameraBlock instance. > > > > > + * > > > > > + * Add a safety counter to make sure we don't loop indefinitely in case > > > > > + * the configuration file is malformed. > > > > > + */ > > > > > + unsigned int sentinel = 100; > > > > > + CameraProps cameraBlock{}; > > > > > + bool blockEnd = false; > > > > > + do { > > > > > + yaml_parser_scan(&parser_, &token); > > > > > + switch (token.type) { > > > > > + case YAML_KEY_TOKEN: { > > > > > + yaml_token_delete(&token); > > > > > + > > > > > + /* > > > > > + * Parse the camera property key and make sure it is > > > > > + * valid. > > > > > + */ > > > > > + std::string key = parseKey(token); > > > > > + std::string value = parseValue(token); > > > > > + if (key.empty() || value.empty()) { > > > > > + LOG(HALConfig, Error) > > > > > + << "Configuration file is not valid"; > > > > > + return -EINVAL; > > > > > + } > > > > > + > > > > > + if (key == "location") { > > > > > + if (value != "front" && value != "back" && > > > > > + value != "external") { > > > > > + LOG(HALConfig, Error) > > > > > + << "Unknown location: " << value; > > > > > + return -EINVAL; > > > > > + } > > > > > + cameraBlock.location = value; > > > > > + } else if (key == "rotation") { > > > > > + cameraBlock.rotation = strtoul(value.c_str(), > > > > > + NULL, 10); > > > > > + if (cameraBlock.rotation < 0) { > > > > > + LOG(HALConfig, Error) > > > > > + << "Unknown rotation: " > > > > > + << cameraBlock.rotation; > > > > > + return -EINVAL; > > > > > + } > > > > > + } else if (key == "model") { > > > > > + cameraBlock.model = value; > > > > > + } else { > > > > > + LOG(HALConfig, Error) > > > > > + << "Configuration file is not valid " > > > > > + << "unknown key: " << key; > > > > > + return -EINVAL; > > > > > + } > > > > > + break; > > > > > + } > > > > > + case YAML_BLOCK_END_TOKEN: > > > > > + yaml_token_delete(&token); > > > > > + blockEnd = true; > > > > > + break; > > > > > + default: > > > > > + yaml_token_delete(&token); > > > > > + break; > > > > > + } > > > > > + > > > > > + --sentinel; > > > > > + } while (!blockEnd && sentinel); > > > > > > > > Missing blank line. > > > > > > > > > + if (!sentinel) { > > > > > + LOG(HALConfig, Error) << "Configuration file is not valid "; > > > > > + return -EINVAL; > > > > > + } > > > > > + > > > > > + cameras_[cameraID] = cameraBlock; > > > > > + > > > > > + return 0; > > > > > +} > > > > > + > > > > > +int CameraHalConfig::parseCameras(yaml_token_t &token) > > > > > +{ > > > > > + int ret; > > > > > + > > > > > + /* The 'cameras' key maps a BLOCK_MAPPING_START block. */ > > > > > + yaml_parser_scan(&parser_, &token); > > > > > + if (token.type != YAML_VALUE_TOKEN) { > > > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > > > + return -EINVAL; > > > > > + } > > > > > + yaml_token_delete(&token); > > > > > + > > > > > + yaml_parser_scan(&parser_, &token); > > > > > + if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) { > > > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > > > + return -EINVAL; > > > > > + } > > > > > + yaml_token_delete(&token); > > > > > + > > > > > + /* > > > > > + * Parse the camera properties. > > > > > + * > > > > > + * Each camera properties block is a list of properties associated > > > > > + * with the ID (as assembled by CameraSensor::generateId()) of the > > > > > + * camera they refer to. > > > > > + * > > > > > + * cameras: > > > > > + * "camera0 id": > > > > > + * key: value > > > > > + * key: value > > > > > + * ... > > > > > + * > > > > > + * "camera1 id": > > > > > + * key: value > > > > > + * key: value > > > > > + * ... > > > > > + */ > > > > > + bool blockEnd = false; > > > > > + do { > > > > > + yaml_parser_scan(&parser_, &token); > > > > > + switch (token.type) { > > > > > + case YAML_KEY_TOKEN: { > > > > > + yaml_token_delete(&token); > > > > > + > > > > > + /* Parse the camera ID as key of the property list. */ > > > > > + std::string cameraId = parseKey(token); > > > > > + if (cameraId.empty()) { > > > > > + LOG(HALConfig, Error) > > > > > + << "Configuration file is not valid"; > > > > > + return -EINVAL; > > > > > + } > > > > > + > > > > > + ret = parseCameraProps(token, cameraId); > > > > > + if (ret) { > > > > > + LOG(HALConfig, Error) > > > > > + << "Configuration file is not valid"; > > > > > + return -EINVAL; > > > > > + } > > > > > + break; > > > > > + } > > > > > + case YAML_BLOCK_END_TOKEN: > > > > > + yaml_token_delete(&token); > > > > > + blockEnd = true; > > > > > + break; > > > > > + default: > > > > > + yaml_token_delete(&token); > > > > > + LOG(HALConfig, Error) > > > > > + << "Configuration file is not valid"; > > > > > + return -EINVAL; > > > > > + } > > > > > + } while (!blockEnd); > > > > > + > > > > > + return 0; > > > > > +} > > > > > + > > > > > +int CameraHalConfig::parseEntry(yaml_token_t &token) > > > > > +{ > > > > > + int ret; > > > > > + > > > > > + /* > > > > > + * Parse each key we find in the file. > > > > > + * > > > > > + * Keys like 'model' and 'maker' are device properties and gets > > > > > + * stored as class members. > > > > > + * > > > > > + * The 'cameras' keys maps to a list of (lists) of camera properties. > > > > > + */ > > > > > + > > > > > + std::string key = parseKey(token); > > > > > + if (key.empty()) { > > > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > > > + return -EINVAL; > > > > > + } > > > > > + > > > > > + if (key == "cameras") { > > > > > + ret = parseCameras(token); > > > > > + if (ret) > > > > > + return ret; > > > > > + } else if (key == "manufacturer") { > > > > > + maker_ = parseValue(token); > > > > > + if (maker_.empty()) { > > > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > > > + return -EINVAL; > > > > > + } > > > > > + } else if (key == "model") { > > > > > + model_ = parseValue(token); > > > > > + if (model_.empty()) { > > > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > > > + return -EINVAL; > > > > > + } > > > > > + } else { > > > > > + LOG(HALConfig, Error) << "Unknown key " << key; > > > > > + return -EINVAL; > > > > > + } > > > > > + > > > > > + return 0; > > > > > +} > > > > > + > > > > > +int CameraHalConfig::parseConfigFile() > > > > > +{ > > > > > + yaml_token_t token; > > > > > + int ret; > > > > > + > > > > > + yaml_parser_scan(&parser_, &token); > > > > > + if (token.type != YAML_STREAM_START_TOKEN) { > > > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > > > + return -EINVAL; > > > > > + } > > > > > + yaml_token_delete(&token); > > > > > + > > > > > + yaml_parser_scan(&parser_, &token); > > > > > + if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) { > > > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > > > + return -EINVAL; > > > > > + } > > > > > + yaml_token_delete(&token); > > > > > + > > > > > + /* Parse the file and parse each single key one by one. */ > > > > > + do { > > > > > + yaml_parser_scan(&parser_, &token); > > > > > + switch (token.type) { > > > > > + case YAML_KEY_TOKEN: > > > > > + yaml_token_delete(&token); > > > > > + ret = parseEntry(token); > > > > > > > > Do you need to pass the token to parseEntry(), given that you've just > > > > deleted it ? It looks to me like the token is only the return value of > > > > yaml_parse_scan(), and can thus be a local variable everywhere. > > > > > > > > > + break; > > > > > + case YAML_STREAM_END_TOKEN: > > > > > + /* Resources are released after the loop exit. */ > > > > > + break; > > > > > + default: > > > > > + yaml_token_delete(&token); > > > > > + break; > > > > > > > > The YAML_STREAM_END_TOKEN and default case are different, but the > > > > while() condition below accesses token.type. Is that intended ? > > > > > > > > > + } > > > > > + } while (token.type != YAML_STREAM_END_TOKEN && ret >= 0); > > > > > + yaml_token_delete(&token); > > > > > + > > > > > + return ret; > > > > > +} > > > > > diff --git a/src/android/camera_hal_config.h b/src/android/camera_hal_config.h > > > > > new file mode 100644 > > > > > index 000000000000..ce77b1ee1582 > > > > > --- /dev/null > > > > > +++ b/src/android/camera_hal_config.h > > > > > @@ -0,0 +1,53 @@ > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > > > +/* > > > > > + * Copyright (C) 2021, Google Inc. > > > > > + * > > > > > + * camera_hal_config.h - Camera HAL configuration file manager > > > > > + */ > > > > > +#ifndef __ANDROID_CAMERA_HAL_CONFIG_H__ > > > > > +#define __ANDROID_CAMERA_HAL_CONFIG_H__ > > > > > + > > > > > +#include <map> > > > > > +#include <string> > > > > > +#include <yaml.h> > > > > > > > > As commented in v1, hiding the parser would be nice, but it can be done > > > > later, on top. > > > > > > > > > + > > > > > +class CameraHalConfig > > > > > +{ > > > > > +public: > > > > > + CameraHalConfig(); > > > > > + ~CameraHalConfig(); > > > > > + > > > > > + int open(); > > > > > + > > > > > + const std::string &deviceModel() const { return model_; } > > > > > + const std::string &deviceMaker() const { return maker_; } > > > > > + > > > > > + int cameraLocation(const std::string &camera) const; > > > > > + unsigned int cameraRotation(const std::string &camera) const; > > > > > + const std::string &cameraModel(const std::string &camera) const; > > > > > + > > > > > +private: > > > > > + yaml_parser_t parser_; > > > > > + > > > > > + std::string model_; > > > > > + std::string maker_; > > > > > + class CameraProps > > > > > + { > > > > > + public: > > > > > + std::string location; > > > > > + std::string model; > > > > > + unsigned int rotation; > > > > > + }; > > > > > + std::map<std::string, CameraProps> cameras_; > > > > > + > > > > > + std::string findFilePath(const std::string &filename) const; > > > > > + FILE *openFile(const std::string &filename); > > > > > + std::string parseValue(yaml_token_t &token); > > > > > + std::string parseKey(yaml_token_t &token); > > > > > + int parseCameraProps(yaml_token_t &token, const std::string &cameraID); > > > > > + int parseCameras(yaml_token_t &token); > > > > > + int parseEntry(yaml_token_t &token); > > > > > + int parseConfigFile(); > > > > > +}; > > > > > + > > > > > +#endif /* __ANDROID_CAMERA_HAL_CONFIG_H__ */ > > > > > diff --git a/src/android/meson.build b/src/android/meson.build > > > > > index 8e7d07d9be3c..c0ede407bc38 100644 > > > > > --- a/src/android/meson.build > > > > > +++ b/src/android/meson.build > > > > > @@ -3,6 +3,7 @@ > > > > > android_deps = [ > > > > > dependency('libexif', required : get_option('android')), > > > > > dependency('libjpeg', required : get_option('android')), > > > > > + dependency('yaml-0.1', required : get_option('android')), > > > > > ] > > > > > > > > > > android_enabled = true > > > > > @@ -41,6 +42,7 @@ android_hal_sources = files([ > > > > > 'camera3_hal.cpp', > > > > > 'camera_hal_manager.cpp', > > > > > 'camera_device.cpp', > > > > > + 'camera_hal_config.cpp', > > > > > > > > While at it, could you move camera_hal_manager to the right place ? > > > > > > > > > 'camera_metadata.cpp', > > > > > 'camera_ops.cpp', > > > > > 'camera_stream.cpp',
Hi Jacopo, On Tue, Mar 30, 2021 at 12:37:30PM +0200, Jacopo Mondi wrote: > On Tue, Mar 30, 2021 at 03:39:56AM +0300, Laurent Pinchart wrote: > > On Mon, Mar 29, 2021 at 05:28:04PM +0200, Jacopo Mondi wrote: > > > Add a CameraHalConfig class to the Android Camera3 HAL layer. > > > > There are quite a few comments from v1 that haven't been addressed. > > Maybe that's because my last reply to v1 came after you've sent this > > series. The comments may also not all be correct, applicable and/or > > desired, but could you then reply to them to explain why they're ignored > > ? > > > > In particular, I think that exposing CameraProps would simplify the > > implementation quite a bit, by removing the need for the camera*() > > functions, with the lookup of the CameraProps entry every time, and the > > associated error handling. > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > README.rst | 2 +- > > > src/android/camera_hal_config.cpp | 463 ++++++++++++++++++++++++++++++ > > > src/android/camera_hal_config.h | 53 ++++ > > > src/android/meson.build | 2 + > > > 4 files changed, 519 insertions(+), 1 deletion(-) > > > create mode 100644 src/android/camera_hal_config.cpp > > > create mode 100644 src/android/camera_hal_config.h > > > > > > diff --git a/README.rst b/README.rst > > > index 7a98164bbb0a..f68d435196de 100644 > > > --- a/README.rst > > > +++ b/README.rst > > > @@ -88,7 +88,7 @@ for tracing with lttng: [optional] > > > liblttng-ust-dev python3-jinja2 lttng-tools > > > > > > for android: [optional] > > > - libexif libjpeg > > > + libexif libjpeg libyaml > > > > > > Using GStreamer plugin > > > ~~~~~~~~~~~~~~~~~~~~~~ > > > diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp > > > new file mode 100644 > > > index 000000000000..846dd7357b0a > > > --- /dev/null > > > +++ b/src/android/camera_hal_config.cpp > > > @@ -0,0 +1,463 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2021, Google Inc. > > > + * > > > + * camera_hal_config.cpp - Camera HAL configuration file manager > > > + */ > > > +#include "camera_hal_config.h" > > > + > > > +#include <stdio.h> > > > +#include <stdlib.h> > > > +#include <string.h> > > > + > > > +#include <hardware/camera3.h> > > > + > > > +#include "libcamera/internal/file.h" > > > +#include "libcamera/internal/log.h" > > > + > > > +using namespace libcamera; > > > + > > > +LOG_DEFINE_CATEGORY(HALConfig) > > > + > > > +CameraHalConfig::CameraHalConfig() > > > +{ > > > + if (!yaml_parser_initialize(&parser_)) > > > + LOG(HALConfig, Fatal) << "Failed to initialize yaml parser"; > > > +} > > > + > > > +CameraHalConfig::~CameraHalConfig() > > > +{ > > > + yaml_parser_delete(&parser_); > > > +} > > > + > > > +/* > > > + * Configuration files can be stored in system paths, which are identified > > > + * through the build configuration. > > > + * > > > + * However, when running uninstalled - the source location takes precedence. > > > + */ > > > +std::string CameraHalConfig::findFilePath(const std::string &filename) const > > > +{ > > > + static std::array<std::string, 2> searchPaths = { > > > + LIBCAMERA_SYSCONF_DIR, > > > + LIBCAMERA_DATA_DIR, > > > > There's also a comment in v1 about data dir. Data dir is typically > > /usr/share/libcamera/. Given that the configuration file is > > device-specific, I don't think we'll ever look for it there, but only in > > sysconf dir. > > > > > + }; > > > + > > > + if (File::exists(filename)) > > > + return filename; > > > + > > > + std::string root = utils::libcameraSourcePath(); > > > + if (!root.empty()) { > > > + std::string configurationPath = root + "data/" + filename; > > > + if (File::exists(configurationPath)) > > > + return configurationPath; > > > + } > > > + > > > + for (std::string &path : searchPaths) { > > > + std::string configurationPath = path + "/" + filename; > > > + if (File::exists(configurationPath)) > > > + return configurationPath; > > > + } > > > + > > > + return ""; > > > +} > > > + > > > +FILE *CameraHalConfig::openFile(const std::string &filename) > > > +{ > > > + const std::string filePath = findFilePath(filename); > > > + if (filePath.empty()) { > > > + LOG(HALConfig, Error) > > > + << "Configuration file: \"" << filename << "\" not found"; > > > + return nullptr; > > > + } > > > + > > > + LOG(HALConfig, Debug) << "Reading configuration file from " << filePath; > > > + > > > + FILE *fh = fopen(filePath.c_str(), "r"); > > > + if (!fh) { > > > + int ret = -errno; > > > + LOG(HALConfig, Error) << "Failed to open configuration file " > > > + << filePath << ": " << strerror(-ret); > > > + return nullptr; > > > + } > > > + > > > + return fh; > > > +} > > > + > > > +/* > > > + * Open the HAL configuration file and validate its content. > > > + * Return 0 on success, a negative error code otherwise > > > + */ > > > +int CameraHalConfig::open() > > > +{ > > > + int ret; > > > + > > > + FILE *fh = openFile("camera_hal.yaml"); > > > + if (!fh) > > > + return -ENOENT; > > > + > > > + yaml_parser_set_input_file(&parser_, fh); > > > + ret = parseConfigFile(); > > > + fclose(fh); > > > + if (ret) > > > + return ret; > > > + > > > + LOG(HALConfig, Info) << "Device model: " << model_; > > > + LOG(HALConfig, Info) << "Device maker: " << maker_; > > > + for (const auto &c : cameras_) { > > > + const std::string &cameraId = c.first; > > > + const CameraProps &camera = c.second; > > > + LOG(HALConfig, Info) << "'" << cameraId << "' " > > > + << " model: " << camera.model > > > + << "(" << camera.location << ")[" > > > + << camera.rotation << "]"; > > > + } > > > > There's also a comment in v1 about whether this should be a debugging > > feature. > > > > > + > > > + return 0; > > > +} > > > + > > > +int CameraHalConfig::cameraLocation(const std::string &camera) const > > > +{ > > > + const auto &it = cameras_.find(camera); > > > + if (it == cameras_.end()) { > > > + LOG(HALConfig, Error) > > > + << "Camera '" << camera > > > + << "' not described in the HAL configuration file"; > > > + return -EINVAL; > > > + } > > > + > > > + const CameraProps &cam = it->second; > > > + if (cam.location.empty()) { > > > + LOG(HALConfig, Error) << "Location for camera '" << camera > > > + << "' not available"; > > > + return -EINVAL; > > > + } > > > + > > > + if (cam.location == "front") > > > + return CAMERA_FACING_FRONT; > > > + else if (cam.location == "back") > > > + return CAMERA_FACING_BACK; > > > + else if (cam.location == "external") > > > + return CAMERA_FACING_EXTERNAL; > > > + > > > + LOG(HALConfig, Error) << "Unsupported camera location " << cam.location; > > > + return -EINVAL; > > > > Can we do this conversion when parsing the configuration file instead ? > > That will ensure at parse time that the data is valid, instead of > > delaying the check to later. The rotation is already handled that way, > > and you already check that the location string is valid in > > CameraHalConfig::parseCameraProps(). > > ack > > > > +} > > > + > > > +unsigned int CameraHalConfig::cameraRotation(const std::string &camera) const > > > +{ > > > + const auto &it = cameras_.find(camera); > > > + if (it == cameras_.end()) { > > > + LOG(HALConfig, Error) > > > + << "Camera '" << camera > > > + << "' not described in the HAL configuration file"; > > > + return 0; > > > + } > > > + > > > + const CameraProps &cam = it->second; > > > + return cam.rotation; > > > +} > > > + > > > +const std::string &CameraHalConfig::cameraModel(const std::string &camera) const > > > +{ > > > + static const std::string empty(""); > > > + const auto &it = cameras_.find(camera); > > > + if (it == cameras_.end()) { > > > + LOG(HALConfig, Error) > > > + << "Camera '" << camera > > > + << "' not described in the HAL configuration file"; > > > + return empty; > > > + } > > > + > > > + const CameraProps &cam = it->second; > > > + return cam.model; > > > +} > > > + > > > +std::string CameraHalConfig::parseValue(yaml_token_t &token) > > > +{ > > > + /* Make sure the token type is a value and get its content. */ > > > + yaml_parser_scan(&parser_, &token); > > > + if (token.type != YAML_VALUE_TOKEN) { > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > + return ""; > > > + } > > > + yaml_token_delete(&token); > > > > Does the token need to be deleted in the error cases above and below ? > > Ah yes, most probably. I thought it had to be deleted if resued, but > it seems deletion is just about releasing resources allocated during > parser_scan(), so it needs to be done in error paths too > > I hate I can't goto easily in C++ to implement saner error handling > paths You could wrap the token in a C++ class with a destructor :-) Possibly overkill, and most probably something we'll handle later, when we'll have multiple parsers. > > > + > > > + yaml_parser_scan(&parser_, &token); > > > + if (token.type != YAML_SCALAR_TOKEN) { > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > + return ""; > > > + } > > > + > > > + char *scalar = reinterpret_cast<char *>(token.data.scalar.value); > > > + std::string value(scalar, token.data.scalar.length); > > > + yaml_token_delete(&token); > > > + > > > + return value; > > > +} > > > + > > > +std::string CameraHalConfig::parseKey(yaml_token_t &token) > > > +{ > > > + /* Make sure the token type is a key and get its value. */ > > > + yaml_parser_scan(&parser_, &token); > > > + if (token.type != YAML_SCALAR_TOKEN) { > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > + return ""; > > > + } > > > + > > > + char *scalar = reinterpret_cast<char *>(token.data.scalar.value); > > > + std::string key(scalar, token.data.scalar.length); > > > + yaml_token_delete(&token); > > > + > > > + return key; > > > +} > > > + > > > +int CameraHalConfig::parseCameraProps(yaml_token_t &token, > > > + const std::string &cameraID) > > > +{ > > > + yaml_parser_scan(&parser_, &token); > > > + if (token.type != YAML_VALUE_TOKEN) { > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > + return -EINVAL; > > > + } > > > + yaml_token_delete(&token); > > > + > > > + yaml_parser_scan(&parser_, &token); > > > + if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) { > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > + return -EINVAL; > > > + } > > > + yaml_token_delete(&token); > > > + > > > + /* > > > + * Parse the camera properties and store them in a cameraBlock instance. > > > + * > > > + * Add a safety counter to make sure we don't loop indefinitely in case > > > + * the configuration file is malformed. > > > + */ > > > + unsigned int sentinel = 100; > > > + CameraProps cameraBlock{}; > > > + bool blockEnd = false; > > > + do { > > > + yaml_parser_scan(&parser_, &token); > > > + switch (token.type) { > > > + case YAML_KEY_TOKEN: { > > > + yaml_token_delete(&token); > > > + > > > + /* > > > + * Parse the camera property key and make sure it is > > > + * valid. > > > + */ > > > + std::string key = parseKey(token); > > > + std::string value = parseValue(token); > > > + if (key.empty() || value.empty()) { > > > + LOG(HALConfig, Error) > > > + << "Configuration file is not valid"; > > > + return -EINVAL; > > > + } > > > + > > > + if (key == "location") { > > > + if (value != "front" && value != "back" && > > > + value != "external") { > > > + LOG(HALConfig, Error) > > > + << "Unknown location: " << value; > > > + return -EINVAL; > > > + } > > > + cameraBlock.location = value; > > > + } else if (key == "rotation") { > > > + cameraBlock.rotation = strtoul(value.c_str(), > > > + NULL, 10); > > > + if (cameraBlock.rotation < 0) { > > > + LOG(HALConfig, Error) > > > + << "Unknown rotation: " > > > + << cameraBlock.rotation; > > > + return -EINVAL; > > > + } > > > + } else if (key == "model") { > > > + cameraBlock.model = value; > > > + } else { > > > + LOG(HALConfig, Error) > > > + << "Configuration file is not valid " > > > + << "unknown key: " << key; > > > + return -EINVAL; > > > + } > > > + break; > > > + } > > > + case YAML_BLOCK_END_TOKEN: > > > + yaml_token_delete(&token); > > > + blockEnd = true; > > > + break; > > > + default: > > > + yaml_token_delete(&token); > > > + break; > > > + } > > > + > > > + --sentinel; > > > + } while (!blockEnd && sentinel); > > > > Missing blank line. > > Intentional > > > > + if (!sentinel) { > > > + LOG(HALConfig, Error) << "Configuration file is not valid "; > > > + return -EINVAL; > > > + } > > > + > > > + cameras_[cameraID] = cameraBlock; > > > + > > > + return 0; > > > +} > > > + > > > +int CameraHalConfig::parseCameras(yaml_token_t &token) > > > +{ > > > + int ret; > > > + > > > + /* The 'cameras' key maps a BLOCK_MAPPING_START block. */ > > > + yaml_parser_scan(&parser_, &token); > > > + if (token.type != YAML_VALUE_TOKEN) { > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > + return -EINVAL; > > > + } > > > + yaml_token_delete(&token); > > > + > > > + yaml_parser_scan(&parser_, &token); > > > + if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) { > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > + return -EINVAL; > > > + } > > > + yaml_token_delete(&token); > > > + > > > + /* > > > + * Parse the camera properties. > > > + * > > > + * Each camera properties block is a list of properties associated > > > + * with the ID (as assembled by CameraSensor::generateId()) of the > > > + * camera they refer to. > > > + * > > > + * cameras: > > > + * "camera0 id": > > > + * key: value > > > + * key: value > > > + * ... > > > + * > > > + * "camera1 id": > > > + * key: value > > > + * key: value > > > + * ... > > > + */ > > > + bool blockEnd = false; > > > + do { > > > + yaml_parser_scan(&parser_, &token); > > > + switch (token.type) { > > > + case YAML_KEY_TOKEN: { > > > + yaml_token_delete(&token); > > > + > > > + /* Parse the camera ID as key of the property list. */ > > > + std::string cameraId = parseKey(token); > > > + if (cameraId.empty()) { > > > + LOG(HALConfig, Error) > > > + << "Configuration file is not valid"; > > > + return -EINVAL; > > > + } > > > + > > > + ret = parseCameraProps(token, cameraId); > > > + if (ret) { > > > + LOG(HALConfig, Error) > > > + << "Configuration file is not valid"; > > > + return -EINVAL; > > > + } > > > + break; > > > + } > > > + case YAML_BLOCK_END_TOKEN: > > > + yaml_token_delete(&token); > > > + blockEnd = true; > > > + break; > > > + default: > > > + yaml_token_delete(&token); > > > + LOG(HALConfig, Error) > > > + << "Configuration file is not valid"; > > > + return -EINVAL; > > > + } > > > + } while (!blockEnd); > > > + > > > + return 0; > > > +} > > > + > > > +int CameraHalConfig::parseEntry(yaml_token_t &token) > > > +{ > > > + int ret; > > > + > > > + /* > > > + * Parse each key we find in the file. > > > + * > > > + * Keys like 'model' and 'maker' are device properties and gets > > > + * stored as class members. > > > + * > > > + * The 'cameras' keys maps to a list of (lists) of camera properties. > > > + */ > > > + > > > + std::string key = parseKey(token); > > > + if (key.empty()) { > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > + return -EINVAL; > > > + } > > > + > > > + if (key == "cameras") { > > > + ret = parseCameras(token); > > > + if (ret) > > > + return ret; > > > + } else if (key == "manufacturer") { > > > + maker_ = parseValue(token); > > > + if (maker_.empty()) { > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > + return -EINVAL; > > > + } > > > + } else if (key == "model") { > > > + model_ = parseValue(token); > > > + if (model_.empty()) { > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > + return -EINVAL; > > > + } > > > + } else { > > > + LOG(HALConfig, Error) << "Unknown key " << key; > > > + return -EINVAL; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +int CameraHalConfig::parseConfigFile() > > > +{ > > > + yaml_token_t token; > > > + int ret; > > > + > > > + yaml_parser_scan(&parser_, &token); > > > + if (token.type != YAML_STREAM_START_TOKEN) { > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > + return -EINVAL; > > > + } > > > + yaml_token_delete(&token); > > > + > > > + yaml_parser_scan(&parser_, &token); > > > + if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) { > > > + LOG(HALConfig, Error) << "Configuration file is not valid"; > > > + return -EINVAL; > > > + } > > > + yaml_token_delete(&token); > > > + > > > + /* Parse the file and parse each single key one by one. */ > > > + do { > > > + yaml_parser_scan(&parser_, &token); > > > + switch (token.type) { > > > + case YAML_KEY_TOKEN: > > > + yaml_token_delete(&token); > > > + ret = parseEntry(token); > > > > Do you need to pass the token to parseEntry(), given that you've just > > deleted it ? It looks to me like the token is only the return value of > > yaml_parse_scan(), and can thus be a local variable everywhere. > > Good question. Where's the parsing state kept ? in the token or in the > parser ? If it's in the parser I can avoid passing token around It's in the parser. yaml_parser_scan() starts with memset(token, 0, sizeof(yaml_token_t)); > > > + break; > > > + case YAML_STREAM_END_TOKEN: > > > + /* Resources are released after the loop exit. */ > > > + break; > > > + default: > > > + yaml_token_delete(&token); > > > + break; > > > > The YAML_STREAM_END_TOKEN and default case are different, but the > > while() condition below accesses token.type. Is that intended ? > > Yes, the type has to stay valid in case of STREAM_END to be verified > below. I can handle this with just ret if you prefer. > > > > + } > > > + } while (token.type != YAML_STREAM_END_TOKEN && ret >= 0); > > > + yaml_token_delete(&token); > > > + > > > + return ret; > > > +} > > > diff --git a/src/android/camera_hal_config.h b/src/android/camera_hal_config.h > > > new file mode 100644 > > > index 000000000000..ce77b1ee1582 > > > --- /dev/null > > > +++ b/src/android/camera_hal_config.h > > > @@ -0,0 +1,53 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2021, Google Inc. > > > + * > > > + * camera_hal_config.h - Camera HAL configuration file manager > > > + */ > > > +#ifndef __ANDROID_CAMERA_HAL_CONFIG_H__ > > > +#define __ANDROID_CAMERA_HAL_CONFIG_H__ > > > + > > > +#include <map> > > > +#include <string> > > > +#include <yaml.h> > > > > As commented in v1, hiding the parser would be nice, but it can be done > > later, on top. > > > > > + > > > +class CameraHalConfig > > > +{ > > > +public: > > > + CameraHalConfig(); > > > + ~CameraHalConfig(); > > > + > > > + int open(); > > > + > > > + const std::string &deviceModel() const { return model_; } > > > + const std::string &deviceMaker() const { return maker_; } > > > + > > > + int cameraLocation(const std::string &camera) const; > > > + unsigned int cameraRotation(const std::string &camera) const; > > > + const std::string &cameraModel(const std::string &camera) const; > > > + > > > +private: > > > + yaml_parser_t parser_; > > > + > > > + std::string model_; > > > + std::string maker_; > > > + class CameraProps > > > + { > > > + public: > > > + std::string location; > > > + std::string model; > > > + unsigned int rotation; > > > + }; > > > + std::map<std::string, CameraProps> cameras_; > > > + > > > + std::string findFilePath(const std::string &filename) const; > > > + FILE *openFile(const std::string &filename); > > > + std::string parseValue(yaml_token_t &token); > > > + std::string parseKey(yaml_token_t &token); > > > + int parseCameraProps(yaml_token_t &token, const std::string &cameraID); > > > + int parseCameras(yaml_token_t &token); > > > + int parseEntry(yaml_token_t &token); > > > + int parseConfigFile(); > > > +}; > > > + > > > +#endif /* __ANDROID_CAMERA_HAL_CONFIG_H__ */ > > > diff --git a/src/android/meson.build b/src/android/meson.build > > > index 8e7d07d9be3c..c0ede407bc38 100644 > > > --- a/src/android/meson.build > > > +++ b/src/android/meson.build > > > @@ -3,6 +3,7 @@ > > > android_deps = [ > > > dependency('libexif', required : get_option('android')), > > > dependency('libjpeg', required : get_option('android')), > > > + dependency('yaml-0.1', required : get_option('android')), > > > ] > > > > > > android_enabled = true > > > @@ -41,6 +42,7 @@ android_hal_sources = files([ > > > 'camera3_hal.cpp', > > > 'camera_hal_manager.cpp', > > > 'camera_device.cpp', > > > + 'camera_hal_config.cpp', > > > > While at it, could you move camera_hal_manager to the right place ? > > > > > 'camera_metadata.cpp', > > > 'camera_ops.cpp', > > > 'camera_stream.cpp',
diff --git a/README.rst b/README.rst index 7a98164bbb0a..f68d435196de 100644 --- a/README.rst +++ b/README.rst @@ -88,7 +88,7 @@ for tracing with lttng: [optional] liblttng-ust-dev python3-jinja2 lttng-tools for android: [optional] - libexif libjpeg + libexif libjpeg libyaml Using GStreamer plugin ~~~~~~~~~~~~~~~~~~~~~~ diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp new file mode 100644 index 000000000000..846dd7357b0a --- /dev/null +++ b/src/android/camera_hal_config.cpp @@ -0,0 +1,463 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2021, Google Inc. + * + * camera_hal_config.cpp - Camera HAL configuration file manager + */ +#include "camera_hal_config.h" + +#include <stdio.h> +#include <stdlib.h> +#include <string.h> + +#include <hardware/camera3.h> + +#include "libcamera/internal/file.h" +#include "libcamera/internal/log.h" + +using namespace libcamera; + +LOG_DEFINE_CATEGORY(HALConfig) + +CameraHalConfig::CameraHalConfig() +{ + if (!yaml_parser_initialize(&parser_)) + LOG(HALConfig, Fatal) << "Failed to initialize yaml parser"; +} + +CameraHalConfig::~CameraHalConfig() +{ + yaml_parser_delete(&parser_); +} + +/* + * Configuration files can be stored in system paths, which are identified + * through the build configuration. + * + * However, when running uninstalled - the source location takes precedence. + */ +std::string CameraHalConfig::findFilePath(const std::string &filename) const +{ + static std::array<std::string, 2> searchPaths = { + LIBCAMERA_SYSCONF_DIR, + LIBCAMERA_DATA_DIR, + }; + + if (File::exists(filename)) + return filename; + + std::string root = utils::libcameraSourcePath(); + if (!root.empty()) { + std::string configurationPath = root + "data/" + filename; + if (File::exists(configurationPath)) + return configurationPath; + } + + for (std::string &path : searchPaths) { + std::string configurationPath = path + "/" + filename; + if (File::exists(configurationPath)) + return configurationPath; + } + + return ""; +} + +FILE *CameraHalConfig::openFile(const std::string &filename) +{ + const std::string filePath = findFilePath(filename); + if (filePath.empty()) { + LOG(HALConfig, Error) + << "Configuration file: \"" << filename << "\" not found"; + return nullptr; + } + + LOG(HALConfig, Debug) << "Reading configuration file from " << filePath; + + FILE *fh = fopen(filePath.c_str(), "r"); + if (!fh) { + int ret = -errno; + LOG(HALConfig, Error) << "Failed to open configuration file " + << filePath << ": " << strerror(-ret); + return nullptr; + } + + return fh; +} + +/* + * Open the HAL configuration file and validate its content. + * Return 0 on success, a negative error code otherwise + */ +int CameraHalConfig::open() +{ + int ret; + + FILE *fh = openFile("camera_hal.yaml"); + if (!fh) + return -ENOENT; + + yaml_parser_set_input_file(&parser_, fh); + ret = parseConfigFile(); + fclose(fh); + if (ret) + return ret; + + LOG(HALConfig, Info) << "Device model: " << model_; + LOG(HALConfig, Info) << "Device maker: " << maker_; + for (const auto &c : cameras_) { + const std::string &cameraId = c.first; + const CameraProps &camera = c.second; + LOG(HALConfig, Info) << "'" << cameraId << "' " + << " model: " << camera.model + << "(" << camera.location << ")[" + << camera.rotation << "]"; + } + + return 0; +} + +int CameraHalConfig::cameraLocation(const std::string &camera) const +{ + const auto &it = cameras_.find(camera); + if (it == cameras_.end()) { + LOG(HALConfig, Error) + << "Camera '" << camera + << "' not described in the HAL configuration file"; + return -EINVAL; + } + + const CameraProps &cam = it->second; + if (cam.location.empty()) { + LOG(HALConfig, Error) << "Location for camera '" << camera + << "' not available"; + return -EINVAL; + } + + if (cam.location == "front") + return CAMERA_FACING_FRONT; + else if (cam.location == "back") + return CAMERA_FACING_BACK; + else if (cam.location == "external") + return CAMERA_FACING_EXTERNAL; + + LOG(HALConfig, Error) << "Unsupported camera location " << cam.location; + return -EINVAL; +} + +unsigned int CameraHalConfig::cameraRotation(const std::string &camera) const +{ + const auto &it = cameras_.find(camera); + if (it == cameras_.end()) { + LOG(HALConfig, Error) + << "Camera '" << camera + << "' not described in the HAL configuration file"; + return 0; + } + + const CameraProps &cam = it->second; + return cam.rotation; +} + +const std::string &CameraHalConfig::cameraModel(const std::string &camera) const +{ + static const std::string empty(""); + const auto &it = cameras_.find(camera); + if (it == cameras_.end()) { + LOG(HALConfig, Error) + << "Camera '" << camera + << "' not described in the HAL configuration file"; + return empty; + } + + const CameraProps &cam = it->second; + return cam.model; +} + +std::string CameraHalConfig::parseValue(yaml_token_t &token) +{ + /* Make sure the token type is a value and get its content. */ + yaml_parser_scan(&parser_, &token); + if (token.type != YAML_VALUE_TOKEN) { + LOG(HALConfig, Error) << "Configuration file is not valid"; + return ""; + } + yaml_token_delete(&token); + + yaml_parser_scan(&parser_, &token); + if (token.type != YAML_SCALAR_TOKEN) { + LOG(HALConfig, Error) << "Configuration file is not valid"; + return ""; + } + + char *scalar = reinterpret_cast<char *>(token.data.scalar.value); + std::string value(scalar, token.data.scalar.length); + yaml_token_delete(&token); + + return value; +} + +std::string CameraHalConfig::parseKey(yaml_token_t &token) +{ + /* Make sure the token type is a key and get its value. */ + yaml_parser_scan(&parser_, &token); + if (token.type != YAML_SCALAR_TOKEN) { + LOG(HALConfig, Error) << "Configuration file is not valid"; + return ""; + } + + char *scalar = reinterpret_cast<char *>(token.data.scalar.value); + std::string key(scalar, token.data.scalar.length); + yaml_token_delete(&token); + + return key; +} + +int CameraHalConfig::parseCameraProps(yaml_token_t &token, + const std::string &cameraID) +{ + yaml_parser_scan(&parser_, &token); + if (token.type != YAML_VALUE_TOKEN) { + LOG(HALConfig, Error) << "Configuration file is not valid"; + return -EINVAL; + } + yaml_token_delete(&token); + + yaml_parser_scan(&parser_, &token); + if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) { + LOG(HALConfig, Error) << "Configuration file is not valid"; + return -EINVAL; + } + yaml_token_delete(&token); + + /* + * Parse the camera properties and store them in a cameraBlock instance. + * + * Add a safety counter to make sure we don't loop indefinitely in case + * the configuration file is malformed. + */ + unsigned int sentinel = 100; + CameraProps cameraBlock{}; + bool blockEnd = false; + do { + yaml_parser_scan(&parser_, &token); + switch (token.type) { + case YAML_KEY_TOKEN: { + yaml_token_delete(&token); + + /* + * Parse the camera property key and make sure it is + * valid. + */ + std::string key = parseKey(token); + std::string value = parseValue(token); + if (key.empty() || value.empty()) { + LOG(HALConfig, Error) + << "Configuration file is not valid"; + return -EINVAL; + } + + if (key == "location") { + if (value != "front" && value != "back" && + value != "external") { + LOG(HALConfig, Error) + << "Unknown location: " << value; + return -EINVAL; + } + cameraBlock.location = value; + } else if (key == "rotation") { + cameraBlock.rotation = strtoul(value.c_str(), + NULL, 10); + if (cameraBlock.rotation < 0) { + LOG(HALConfig, Error) + << "Unknown rotation: " + << cameraBlock.rotation; + return -EINVAL; + } + } else if (key == "model") { + cameraBlock.model = value; + } else { + LOG(HALConfig, Error) + << "Configuration file is not valid " + << "unknown key: " << key; + return -EINVAL; + } + break; + } + case YAML_BLOCK_END_TOKEN: + yaml_token_delete(&token); + blockEnd = true; + break; + default: + yaml_token_delete(&token); + break; + } + + --sentinel; + } while (!blockEnd && sentinel); + if (!sentinel) { + LOG(HALConfig, Error) << "Configuration file is not valid "; + return -EINVAL; + } + + cameras_[cameraID] = cameraBlock; + + return 0; +} + +int CameraHalConfig::parseCameras(yaml_token_t &token) +{ + int ret; + + /* The 'cameras' key maps a BLOCK_MAPPING_START block. */ + yaml_parser_scan(&parser_, &token); + if (token.type != YAML_VALUE_TOKEN) { + LOG(HALConfig, Error) << "Configuration file is not valid"; + return -EINVAL; + } + yaml_token_delete(&token); + + yaml_parser_scan(&parser_, &token); + if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) { + LOG(HALConfig, Error) << "Configuration file is not valid"; + return -EINVAL; + } + yaml_token_delete(&token); + + /* + * Parse the camera properties. + * + * Each camera properties block is a list of properties associated + * with the ID (as assembled by CameraSensor::generateId()) of the + * camera they refer to. + * + * cameras: + * "camera0 id": + * key: value + * key: value + * ... + * + * "camera1 id": + * key: value + * key: value + * ... + */ + bool blockEnd = false; + do { + yaml_parser_scan(&parser_, &token); + switch (token.type) { + case YAML_KEY_TOKEN: { + yaml_token_delete(&token); + + /* Parse the camera ID as key of the property list. */ + std::string cameraId = parseKey(token); + if (cameraId.empty()) { + LOG(HALConfig, Error) + << "Configuration file is not valid"; + return -EINVAL; + } + + ret = parseCameraProps(token, cameraId); + if (ret) { + LOG(HALConfig, Error) + << "Configuration file is not valid"; + return -EINVAL; + } + break; + } + case YAML_BLOCK_END_TOKEN: + yaml_token_delete(&token); + blockEnd = true; + break; + default: + yaml_token_delete(&token); + LOG(HALConfig, Error) + << "Configuration file is not valid"; + return -EINVAL; + } + } while (!blockEnd); + + return 0; +} + +int CameraHalConfig::parseEntry(yaml_token_t &token) +{ + int ret; + + /* + * Parse each key we find in the file. + * + * Keys like 'model' and 'maker' are device properties and gets + * stored as class members. + * + * The 'cameras' keys maps to a list of (lists) of camera properties. + */ + + std::string key = parseKey(token); + if (key.empty()) { + LOG(HALConfig, Error) << "Configuration file is not valid"; + return -EINVAL; + } + + if (key == "cameras") { + ret = parseCameras(token); + if (ret) + return ret; + } else if (key == "manufacturer") { + maker_ = parseValue(token); + if (maker_.empty()) { + LOG(HALConfig, Error) << "Configuration file is not valid"; + return -EINVAL; + } + } else if (key == "model") { + model_ = parseValue(token); + if (model_.empty()) { + LOG(HALConfig, Error) << "Configuration file is not valid"; + return -EINVAL; + } + } else { + LOG(HALConfig, Error) << "Unknown key " << key; + return -EINVAL; + } + + return 0; +} + +int CameraHalConfig::parseConfigFile() +{ + yaml_token_t token; + int ret; + + yaml_parser_scan(&parser_, &token); + if (token.type != YAML_STREAM_START_TOKEN) { + LOG(HALConfig, Error) << "Configuration file is not valid"; + return -EINVAL; + } + yaml_token_delete(&token); + + yaml_parser_scan(&parser_, &token); + if (token.type != YAML_BLOCK_MAPPING_START_TOKEN) { + LOG(HALConfig, Error) << "Configuration file is not valid"; + return -EINVAL; + } + yaml_token_delete(&token); + + /* Parse the file and parse each single key one by one. */ + do { + yaml_parser_scan(&parser_, &token); + switch (token.type) { + case YAML_KEY_TOKEN: + yaml_token_delete(&token); + ret = parseEntry(token); + break; + case YAML_STREAM_END_TOKEN: + /* Resources are released after the loop exit. */ + break; + default: + yaml_token_delete(&token); + break; + } + } while (token.type != YAML_STREAM_END_TOKEN && ret >= 0); + yaml_token_delete(&token); + + return ret; +} diff --git a/src/android/camera_hal_config.h b/src/android/camera_hal_config.h new file mode 100644 index 000000000000..ce77b1ee1582 --- /dev/null +++ b/src/android/camera_hal_config.h @@ -0,0 +1,53 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2021, Google Inc. + * + * camera_hal_config.h - Camera HAL configuration file manager + */ +#ifndef __ANDROID_CAMERA_HAL_CONFIG_H__ +#define __ANDROID_CAMERA_HAL_CONFIG_H__ + +#include <map> +#include <string> +#include <yaml.h> + +class CameraHalConfig +{ +public: + CameraHalConfig(); + ~CameraHalConfig(); + + int open(); + + const std::string &deviceModel() const { return model_; } + const std::string &deviceMaker() const { return maker_; } + + int cameraLocation(const std::string &camera) const; + unsigned int cameraRotation(const std::string &camera) const; + const std::string &cameraModel(const std::string &camera) const; + +private: + yaml_parser_t parser_; + + std::string model_; + std::string maker_; + class CameraProps + { + public: + std::string location; + std::string model; + unsigned int rotation; + }; + std::map<std::string, CameraProps> cameras_; + + std::string findFilePath(const std::string &filename) const; + FILE *openFile(const std::string &filename); + std::string parseValue(yaml_token_t &token); + std::string parseKey(yaml_token_t &token); + int parseCameraProps(yaml_token_t &token, const std::string &cameraID); + int parseCameras(yaml_token_t &token); + int parseEntry(yaml_token_t &token); + int parseConfigFile(); +}; + +#endif /* __ANDROID_CAMERA_HAL_CONFIG_H__ */ diff --git a/src/android/meson.build b/src/android/meson.build index 8e7d07d9be3c..c0ede407bc38 100644 --- a/src/android/meson.build +++ b/src/android/meson.build @@ -3,6 +3,7 @@ android_deps = [ dependency('libexif', required : get_option('android')), dependency('libjpeg', required : get_option('android')), + dependency('yaml-0.1', required : get_option('android')), ] android_enabled = true @@ -41,6 +42,7 @@ android_hal_sources = files([ 'camera3_hal.cpp', 'camera_hal_manager.cpp', 'camera_device.cpp', + 'camera_hal_config.cpp', 'camera_metadata.cpp', 'camera_ops.cpp', 'camera_stream.cpp',
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 | 463 ++++++++++++++++++++++++++++++ src/android/camera_hal_config.h | 53 ++++ src/android/meson.build | 2 + 4 files changed, 519 insertions(+), 1 deletion(-) create mode 100644 src/android/camera_hal_config.cpp create mode 100644 src/android/camera_hal_config.h