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

[Jacopo Mondi]

Hi Laurent,

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

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

> > +	};
> > +
> > +	if (File::exists(filename))
> > +		return filename;
> > +
> > +	std::filesystem::path root = utils::libcameraSourcePath();
> > +	if (!root.empty()) {
> > +		std::string configurationPath = root / "data" / filename;
> > +		if (File::exists(configurationPath))
> > +			return configurationPath;
> > +	}
> > +
> > +	for (const std::filesystem::path &path : searchPaths) {
> > +		std::string configurationPath = path / filename;
> > +		if (File::exists(configurationPath))
> > +			return configurationPath;
> > +	}
> > +
> > +	return "";
> > +}
> > +
> > +FILE *CameraHalConfig::openFile(const std::string &filename)
> > +{
> > +	const std::string filePath = findFilePath(filename);
> > +	if (filePath.empty()) {
> > +		LOG(HALConfig, Error)
> > +			<< "Configuration file: \"" << filename << "\" not found";
> > +		return nullptr;
> > +	}
> > +
> > +	LOG(HALConfig, Debug) << "Reading configuration file from " << filePath;
> > +
> > +	FILE *fh = fopen(filePath.c_str(), "r");
> > +	if (!fh) {
> > +		int ret = -errno;
> > +		LOG(HALConfig, Error) << "Failed to open configuration file "
> > +				      << filePath << ": " << strerror(-ret);
> > +		return nullptr;
> > +	}
> > +
> > +	return fh;
> > +}
>
> You could possibly inline this function in CameraHalConfig::open(), up
> to you.

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

>
> Other than these two comments, this looks good.
>
> > +
> > +CameraHalConfig::CameraHalConfig()
> > +	: Extensible(new Private(this))
> > +{
> > +}
> > +
> > +/*
> > + * Open the HAL configuration file and validate its content.
> > + * Return 0 on success, a negative error code otherwise
> > + */
> > +int CameraHalConfig::open()
> > +{
> > +	FILE *fh = openFile("camera_hal.yaml");
> > +	if (!fh)
> > +		return -ENOENT;
> > +
> > +	Private *const d = LIBCAMERA_D_PTR();
> > +	int ret = d->parseConfigFile(fh, &cameras_);
> > +	fclose(fh);
> > +	if (ret)
> > +		return ret;
> > +
> > +	for (const auto &c : cameras_) {
> > +		const std::string &cameraId = c.first;
> > +		const CameraProps &camera = c.second;
> > +		LOG(HALConfig, Debug) << "'" << cameraId << "' "
> > +				      << "(" << camera.facing << ")["
> > +				      << camera.rotation << "]";
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +const CameraProps &CameraHalConfig::cameraProps(const std::string &cameraId) const
> > +{
> > +	static CameraProps empty;
> > +
> > +	const auto &it = cameras_.find(cameraId);
> > +	if (it == cameras_.end()) {
> > +		LOG(HALConfig, Error)
> > +			<< "Camera '" << cameraId
> > +			<< "' not described in the HAL configuration file";
> > +		return empty;
> > +	}
> > +
> > +	return it->second;
> > +}
> > diff --git a/src/android/camera_hal_config.h b/src/android/camera_hal_config.h
> > new file mode 100644
> > index 000000000000..65569559fbf0
> > --- /dev/null
> > +++ b/src/android/camera_hal_config.h
> > @@ -0,0 +1,38 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021, Google Inc.
> > + *
> > + * camera_hal_config.h - Camera HAL configuration file manager
> > + */
> > +#ifndef __ANDROID_CAMERA_HAL_CONFIG_H__
> > +#define __ANDROID_CAMERA_HAL_CONFIG_H__
> > +
> > +#include <map>
> > +#include <string>
> > +
> > +#include <libcamera/class.h>
> > +
> > +struct CameraProps {
> > +	int facing;
> > +	int rotation;
> > +
> > +	bool valid = false;
> > +};
> > +
> > +class CameraHalConfig final : public libcamera::Extensible
> > +{
> > +	LIBCAMERA_DECLARE_PRIVATE(CameraBuffer)
> > +
> > +public:
> > +	CameraHalConfig();
> > +
> > +	int open();
> > +	const CameraProps &cameraProps(const std::string &cameraId) const;
> > +
> > +private:
> > +	std::string findFilePath(const std::string &filename) const;
> > +	FILE *openFile(const std::string &filename);
> > +
> > +	std::map<std::string, CameraProps> cameras_;
> > +};
> > +#endif /* __ANDROID_CAMERA_HAL_CONFIG_H__ */
> > diff --git a/src/android/meson.build b/src/android/meson.build
> > index 2be20c97e118..3893e5b5b832 100644
> > --- a/src/android/meson.build
> > +++ b/src/android/meson.build
> > @@ -3,6 +3,7 @@
> >  android_deps = [
> >      dependency('libexif', required : get_option('android')),
> >      dependency('libjpeg', required : get_option('android')),
> > +    dependency('yaml-0.1', required : get_option('android')),
> >  ]
> >
> >  android_enabled = true
> > @@ -45,6 +46,7 @@ android_hal_sources = files([
> >      'camera3_hal.cpp',
> >      'camera_hal_manager.cpp',
> >      'camera_device.cpp',
> > +    'camera_hal_config.cpp',
> >      'camera_metadata.cpp',
> >      'camera_ops.cpp',
> >      'camera_stream.cpp',
>
> --
> Regards,
>
> Laurent Pinchart