[libcamera-devel] [PATCH v2 7/9] libcamera: Introduce camera sensor database

[Laurent Pinchart]

Hi Jacopo,

Another comment.

On Wed, Dec 23, 2020 at 07:49:46AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
> On Fri, Dec 18, 2020 at 05:47:52PM +0100, Jacopo Mondi wrote:
> 
> Thank you for the patch.
> 
> > Introduce a 'database' of camera sensor information, which contains
> > a list of camera sensor properties which is not possible, or desirable,
> > to retrieve at run-time.
> > 
> > The camera sensor database is a global read-only library object.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  include/libcamera/internal/meson.build       |   1 +
> >  include/libcamera/internal/sensor_database.h |  37 +++++++
> >  src/libcamera/meson.build                    |   1 +
> >  src/libcamera/sensor_database.cpp            | 106 +++++++++++++++++++
> >  4 files changed, 145 insertions(+)
> >  create mode 100644 include/libcamera/internal/sensor_database.h
> >  create mode 100644 src/libcamera/sensor_database.cpp
> > 
> > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> > index 7cde023f7c3a..ac08f0761238 100644
> > --- a/include/libcamera/internal/meson.build
> > +++ b/include/libcamera/internal/meson.build
> > @@ -36,6 +36,7 @@ libcamera_internal_headers = files([
> >      'process.h',
> >      'pub_key.h',
> >      'semaphore.h',
> > +    'sensor_database.h',
> >      'sysfs.h',
> >      'thread.h',
> >      'timer.h',
> > diff --git a/include/libcamera/internal/sensor_database.h b/include/libcamera/internal/sensor_database.h
> > new file mode 100644
> > index 000000000000..b7d3a6e7468e
> > --- /dev/null
> > +++ b/include/libcamera/internal/sensor_database.h
> > @@ -0,0 +1,37 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2020, Google Inc.
> > + *
> > + * sensor_database.h - Database of camera sensor properties
> > + */
> > +#ifndef __LIBCAMERA_SENSOR_DATABASE_H__
> > +#define __LIBCAMERA_SENSOR_DATABASE_H__
> > +
> > +#include <initializer_list>
> > +#include <unordered_map>
> > +
> > +#include <libcamera/controls.h>
> > +#include <libcamera/geometry.h>
> > +
> > +namespace libcamera {
> > +
> > +class SensorDatabase : private std::unordered_map<std::string, const ControlList>
> > +{
> > +private:
> > +	using Map = std::unordered_map<std::string, const ControlList>;
> > +
> > +public:
> > +	SensorDatabase(std::initializer_list<Map::value_type> items)
> > +		: Map(items)
> > +	{
> > +	}
> > +
> > +	bool contains(Map::key_type sensor) const;
> > +	const ControlList &properties(Map::key_type sensor) const;
> 
> Should both functions take a const reference as argument ?
> 
> > +};
> > +
> > +extern const SensorDatabase sensorDatabase;
> 
> To reduce the exposure of details to the outside of this class, what
> would you think of
> 
> class SensorDatabase 
> {
> public:
> 	static bool contains(const std::string &model);
> 	static const ControlList &properties(const std::string &model);
> };
> 
> ? Everything else could be moved from the header file to the .cpp file.
> No instance would need to be created, the functions could operate on a
> static map.

I wonder if the sensor database should actually expose a ControList, or
if we should use a custom structure instead. One of my short term goals
with this is to move the sensor static information stored in the RPi
IPA, and there are no corresponding libcamera properties.

> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_SENSOR_DATABASE_H__ */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index 387d5d88ecae..54f3b81ad7b2 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -41,6 +41,7 @@ libcamera_sources = files([
> >      'pub_key.cpp',
> >      'request.cpp',
> >      'semaphore.cpp',
> > +    'sensor_database.cpp',
> >      'signal.cpp',
> >      'stream.cpp',
> >      'sysfs.cpp',
> > diff --git a/src/libcamera/sensor_database.cpp b/src/libcamera/sensor_database.cpp
> > new file mode 100644
> > index 000000000000..b90eb45ed46d
> > --- /dev/null
> > +++ b/src/libcamera/sensor_database.cpp
> > @@ -0,0 +1,106 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2020, Google Inc.
> > + *
> > + * sensor_database.cpp - Database of camera sensor properties
> > + */
> > +
> > +#include "libcamera/internal/sensor_database.h"
> > +
> > +#include <libcamera/property_ids.h>
> > +
> > +namespace libcamera {
> > +
> > +/**
> > + * \file sensor_database.h
> > + * \brief Database of camera sensor properties
> > + *
> > + * The camera sensor database collects information on camera sensors
> > + * which is not possible or desirable to retrieve at run-time.
> > + */
> > +
> > +/**
> > + * \class SensorDatabase
> > + * \brief Database of camera sensor properties
> > + *
> > + * The database is indexed using the camera sensor model, as reported by the
> > + * properties::Model property, and for each supported sensor it contains a
> > + * list of properties.
> > + *
> > + * The database is statically allocated and cannot be modified at runtime, and
> > + * only provides two methods: one to verify if a sensor is supported
> > + * (SensorDatabase::contains()) and one to retrieve the sensor properties
> > + * (SensorDatabase::properties()).
> > + */
> > +
> > +/**
> > + * \fn SensorDatabase::SensorDatabase(std::initializer_list<Map::value_type> items)
> > + * \brief Contruct the sensor database with a list of sensor properties
> > + * \param[in] items The list of sensor properties
> > + */
> > +
> > +/**
> > + * \brief Verify if a sensor is part of the database
> > + * \param sensor The sensor model name
> > + * \return True if the sensor has an associated list of properties in the
> > + * database, false otherwise
> > + */
> > +bool SensorDatabase::contains(Map::key_type sensor) const
> > +{
> > +	return Map::find(sensor) != Map::end();
> > +}
> > +
> > +/**
> > + * \brief Retrieve the properties associated with a sensor
> > + * \param sensor The sensor model name
> > + * \return The properties list associated with a sensor or an empty list if the
> > + * sensor is not supported
> > + */
> > +const ControlList &SensorDatabase::properties(Map::key_type sensor) const
> > +{
> > +	static ControlList empty{};
> > +
> > +	auto it = Map::find(sensor);
> > +	if (it != Map::end())
> > +		return it->second;
> > +
> > +	return empty;
> > +}
> > +
> > +/**
> > + * \brief Sony IMX219 sensor properties
> > + */
> > +extern const ControlList imx219Properties = {
> > +	{ &properties::draft::SensorPhysicalSize, Span<const float>({ 5.095, 4.93 }) },
> > +	{ &properties::UnitCellSize, Size(1120, 1120) },
> > +};
> > +
> > +/**
> > + * \brief Omnivision ov5670 sensor properties
> > + */
> > +extern const ControlList ov5670Properties = {
> > +	{ &properties::draft::SensorPhysicalSize, Span<const float>({ 2.9457, 2.214 }) },
> > +	{ &properties::UnitCellSize, Size(1120, 1120) }
> > +};
> > +
> > +/**
> > + * \brief Omnivision 13858 sensor properties
> > + */
> > +extern const ControlList ov13858Properties = {
> > +	{ &properties::draft::SensorPhysicalSize, Span<const float>({ 4.7497, 3.53549 }) },
> > +	{ &properties::UnitCellSize, Size(1120, 1120) }
> > +};
> 
> We'll probably need a mechanism to ensure that all properties are
> included for all sensors, but that can come later.
> 
> > +
> > +#define SENSOR_ENTRY(_sensor)	\
> > +	{ #_sensor, _sensor ## Properties }
> > +
> > +/**
> > + * \brief Database of sensor properties
> > + */
> > +extern const SensorDatabase sensorDatabase = {
> > +	SENSOR_ENTRY(imx219),
> > +	SENSOR_ENTRY(ov5670),
> > +	SENSOR_ENTRY(ov13858),
> > +};
> > +
> > +} /* namespace libcamera */

-- 
Regards,

Laurent Pinchart