[libcamera-devel] [PATCH v4 3/5] android: Add CameraHalConfig class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Apr 13 05:40:50 CEST 2021
Hi Jacopo,
Thank you for the patch.
On Wed, Apr 07, 2021 at 04:52:22PM +0900, Hirokazu Honda wrote:
> On Wed, Apr 7, 2021 at 4:46 PM Jacopo Mondi wrote:
> > On Wed, Apr 07, 2021 at 12:45:12PM +0900, Hirokazu Honda wrote:
> > > Hi Jacopo, Thanks for this patch.
> > >
> > > > +}
> > > > +
> > > > +std::string CameraHalConfig::findFilePath(const std::string &filename) const
> > > > +{
> > > > + static const std::array<std::filesystem::path, 2> searchPaths = {
> > > > + LIBCAMERA_SYSCONF_DIR,
> > > > + LIBCAMERA_DATA_DIR,
> > > > + };
> > > > +
> > > > + if (File::exists(filename))
> > >
> > > nit: Not only the file
> >
> > I'm sorry I didn't get this comment...
> >
>
> Sorry, please ignore. I would write a comment about std::filesystem
> described below.
>
> > > > + return filename;
> > > > +
> > > > + std::filesystem::path root = utils::libcameraSourcePath();
> > > > + if (!root.empty()) {
> > > > + std::string configurationPath = root / "data" / filename;
> > > > + if (File::exists(configurationPath))
> > > > + return configurationPath;
> > > > + }
> > > > +
> > > > + for (const std::filesystem::path &path : searchPaths) {
> > > > + std::string configurationPath = path / filename;
> > > > + if (File::exists(configurationPath))
> > > > + return configurationPath;
> > > > + }
> > > > +
> > > > + return "";
> > > > +}
> > > > +
> > >
> > > If we use std::filesystem::path, we can use
> > > std::filesystem::is_character_file(filepath). FWIW,
> > > is_character_file() returns false if the file doesn't exist.
> > > I would do as following,
> > > std::filesystem::path findFilePath(const std::filesystem::path&)
> > > FILE *CameraHalConfig::openFile(const std::filesystem::path&)
> > > Then oepnFile("camera_hal.yaml") should work std::filesystem::path can
> > > construct from char*.
> >
> > File::exists() is used library-wide. To be honest I don't see what we
> > would gain.
>
> I prefer using a standard library.
> Since we already use std::filesystem here, I think it would be
> consistency to use std::filesystem function.
> I am ok to use File::exists() though.
Our File class predates the switch to C++17. I'm not opposed to moving
to std::filesystem. One feature that File provides is RAII semantics for
mmap(), and as far as I can tell, the C++ standard library doesn't
provide that. We could keep the File class for that purpose, and drop
File::exists() in favour of std::filesystem. It may not have to be done
in this patch series though, we could do so library-wide later.
https://bugs.libcamera.org/show_bug.cgi?id=25
> > > > +FILE *CameraHalConfig::openFile(const std::string &filename)
> > > > +{
> > > > + const std::string filePath = findFilePath(filename);
> > > > + if (filePath.empty()) {
> > > > + LOG(HALConfig, Error)
> > > > + << "Configuration file: \"" << filename << "\" not found";
> > > > + return nullptr;
> > > > + }
> > > > +
> > > > + LOG(HALConfig, Debug) << "Reading configuration file from " << filePath;
> > > > +
> > > > + FILE *fh = fopen(filePath.c_str(), "r");
> > > > + if (!fh) {
> > > > + int ret = -errno;
> > > > + LOG(HALConfig, Error) << "Failed to open configuration file "
> > > > + << filePath << ": " << strerror(-ret);
> > > > + return nullptr;
> > > > + }
> > > > +
> > > > + return fh;
> > > > +}
> > > > +
> > > > +CameraHalConfig::CameraHalConfig()
> > > > + : Extensible(new Private(this))
> > > > +{
> > > > +}
> > > > +
> > > > +/*
> > > > + * Open the HAL configuration file and validate its content.
> > > > + * Return 0 on success, a negative error code otherwise
> > > > + */
> > > > +int CameraHalConfig::open()
> > > > +{
> > > > + FILE *fh = openFile("camera_hal.yaml");
> > > > + if (!fh)
> > > > + return -ENOENT;
> > > > +
> > > > + Private *const d = LIBCAMERA_D_PTR();
> > > > + int ret = d->parseConfigFile(fh, &cameras_);
> > > > + fclose(fh);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + for (const auto &c : cameras_) {
> > > > + const std::string &cameraId = c.first;
> > > > + const CameraProps &camera = c.second;
> > > > + LOG(HALConfig, Debug) << "'" << cameraId << "' "
> > > > + << "(" << camera.facing << ")["
> > > > + << camera.rotation << "]";
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +const CameraProps &CameraHalConfig::cameraProps(const std::string &cameraId) const
> > > > +{
> > > > + static CameraProps empty;
> > > > +
> > > > + const auto &it = cameras_.find(cameraId);
> > > > + if (it == cameras_.end()) {
> > > > + LOG(HALConfig, Error)
> > > > + << "Camera '" << cameraId
> > > > + << "' not described in the HAL configuration file";
> > > > + return empty;
> > > > + }
> > > > +
> > > > + return it->second;
> > > > +}
> > > > diff --git a/src/android/camera_hal_config.h b/src/android/camera_hal_config.h
> > > > new file mode 100644
> > > > index 000000000000..65569559fbf0
> > > > --- /dev/null
> > > > +++ b/src/android/camera_hal_config.h
> > > > @@ -0,0 +1,38 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2021, Google Inc.
> > > > + *
> > > > + * camera_hal_config.h - Camera HAL configuration file manager
> > > > + */
> > > > +#ifndef __ANDROID_CAMERA_HAL_CONFIG_H__
> > > > +#define __ANDROID_CAMERA_HAL_CONFIG_H__
> > > > +
> > > > +#include <map>
> > > > +#include <string>
> > > > +
> > > > +#include <libcamera/class.h>
> > > > +
> > > > +struct CameraProps {
> > > > + int facing;
> > > > + int rotation;
> > > > +
> > > > + bool valid = false;
> > > > +};
> > > > +
> > >
> > > nit: I would set facing and rotation to -1 as invalid values.
> >
> > It seems like the 'valid' flag might get set to true also for camera
> > entries that do not specify 'location', 'rotation' or none of them. I
> > could drop 'valid' and initialize the single field, and fail out loud
> > if the value is retrieved from the configuration file but not
> > initialized.
> >
> > > With those nits,
> > > Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
> > >
> > > > +class CameraHalConfig final : public libcamera::Extensible
> > > > +{
> > > > + LIBCAMERA_DECLARE_PRIVATE(CameraBuffer)
> > > > +
> > > > +public:
> > > > + CameraHalConfig();
> > > > +
> > > > + int open();
> > > > + const CameraProps &cameraProps(const std::string &cameraId) const;
> > > > +
> > > > +private:
> > > > + std::string findFilePath(const std::string &filename) const;
> > > > + FILE *openFile(const std::string &filename);
> > > > +
> > > > + std::map<std::string, CameraProps> cameras_;
> > > > +};
> > > > +#endif /* __ANDROID_CAMERA_HAL_CONFIG_H__ */
> > > > diff --git a/src/android/meson.build b/src/android/meson.build
> > > > index 2be20c97e118..3893e5b5b832 100644
> > > > --- a/src/android/meson.build
> > > > +++ b/src/android/meson.build
> > > > @@ -3,6 +3,7 @@
> > > > android_deps = [
> > > > dependency('libexif', required : get_option('android')),
> > > > dependency('libjpeg', required : get_option('android')),
> > > > + dependency('yaml-0.1', required : get_option('android')),
> > > > ]
> > > >
> > > > android_enabled = true
> > > > @@ -45,6 +46,7 @@ android_hal_sources = files([
> > > > 'camera3_hal.cpp',
> > > > 'camera_hal_manager.cpp',
> > > > 'camera_device.cpp',
> > > > + 'camera_hal_config.cpp',
> > > > 'camera_metadata.cpp',
> > > > 'camera_ops.cpp',
> > > > 'camera_stream.cpp',
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list