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

[Jacopo Mondi]

Hi Laurent,

On Wed, Dec 23, 2020 at 08:56:13AM +0200, Laurent Pinchart wrote:
> 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.
>

Good question.

Are you referring to information contained in cam_helper_xxxx.cpp ?
Are we sure that information like the number of frames to mis-trust is
something that should be part of the sensor database. Ideally one
should be able to expand the database with only the support of the
sensor's datasheet, am I wrong ? It seems to me that some information
there are implementation decision specific...

Thanks
  j

> > > +
> > > +} /* 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