[libcamera-devel,v2,0/9] libcamera: Introduce sensor database
mbox series

Message ID 20201218164754.81422-1-jacopo@jmondi.org
Headers show
Series
  • libcamera: Introduce sensor database
Related show

Message

Jacopo Mondi Dec. 18, 2020, 4:47 p.m. UTC 
Well, introducing the sensor database is only part of the series.

Compared to v1 I've moved the sensor database to report a list
of properties instead of custom data.

The end goal of the series is to report two android static metadata.
One exposed through the sensor database, the other collected from the
sensor database.

In order to get there:
- Introduce two new draft properties in the first 2 patches
- Expand the BayerPatter class to support mbus codes
- Allow creation of ControlList with initializer lists and build the
  sensor database on top of this new feature
- Register in the CameraSensor class properties retrieved from the
  sensor database and inspect the list of formats to deduce the color filter
  arrangement
- Register the two properties in the Android camera HAL

A note in patch [2/9] for a possible Doxygen bug.

Thanks
  j

Jacopo Mondi (9):
  libcamera: properties: SensorPhysicalSize draft property
  libcamera: properties: ColorFilterArrangement draft property
  libcamera: bayer_format: Add support for mbus codes
  libcamera: camera_sensor: Register ColorFilterArrangement
  android: camera_device: Report ColorFilterArrangement
  libcamera: controls: List-initialize ControlList
  libcamera: Introduce camera sensor database
  libcamera: camera_sensor: Register static properties
  android: camera_device: Report sensor physical size

 include/libcamera/controls.h                 |   2 +
 include/libcamera/internal/bayer_format.h    |   3 +
 include/libcamera/internal/camera_sensor.h   |   1 +
 include/libcamera/internal/meson.build       |   1 +
 include/libcamera/internal/sensor_database.h |  37 +++++++
 src/android/camera_device.cpp                |  21 +++-
 src/libcamera/bayer_format.cpp               |  71 ++++++++++++-
 src/libcamera/camera_sensor.cpp              |  57 +++++++++-
 src/libcamera/controls.cpp                   |  11 ++
 src/libcamera/meson.build                    |   1 +
 src/libcamera/property_ids.yaml              |  37 +++++++
 src/libcamera/sensor_database.cpp            | 106 +++++++++++++++++++
 12 files changed, 337 insertions(+), 11 deletions(-)
 create mode 100644 include/libcamera/internal/sensor_database.h
 create mode 100644 src/libcamera/sensor_database.cpp

--
2.29.2

Comments

Laurent Pinchart Dec. 23, 2020, 5:49 a.m. UTC | #1 
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@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.

> +
> +} /* 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 */
Laurent Pinchart Dec. 23, 2020, 6:56 a.m. UTC | #2 
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@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 */
Jacopo Mondi Dec. 23, 2020, 4:40 p.m. UTC | #3 
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@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
Laurent Pinchart Dec. 23, 2020, 5:40 p.m. UTC | #4 
Hi Jacopo,

On Wed, Dec 23, 2020 at 05:40:01PM +0100, Jacopo Mondi wrote:
> On Wed, Dec 23, 2020 at 08:56:13AM +0200, Laurent Pinchart wrote:
> > On Wed, Dec 23, 2020 at 07:49:46AM +0200, Laurent Pinchart wrote:
> > > On Fri, Dec 18, 2020 at 05:47:52PM +0100, Jacopo Mondi wrote:
> > > > 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@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...

It was very implementation-specific, but with David's recent rework of
the AGC and AWB algorithms, the information exposed by the CamHelper
class has been refactored. It now only carries intrinsic properties of
the camera sensor, with the decision on how to use that information
moved to the IPA itself.

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