[libcamera-devel,v5,0/3] Introduce sensor database
mbox series

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

Message

Jacopo Mondi April 13, 2021, 10:14 a.m. UTC 
v4->v5:
- Use a static const std::map inside SensorDatabase::get()
- Remove multiple call to initStaticProperties in CameraSensor
- Documentation fixes

Thanks
  j

Jacopo Mondi (3):
  libcamera: Introduce camera sensor database
  libcamera: camera_sensor: Register static properties
  android: camera_device: Report sensor physical size

 include/libcamera/internal/camera_sensor.h   |  1 +
 include/libcamera/internal/meson.build       |  1 +
 include/libcamera/internal/sensor_database.h | 28 ++++++
 src/android/camera_device.cpp                | 30 ++++---
 src/libcamera/camera_sensor.cpp              | 16 +++-
 src/libcamera/meson.build                    |  1 +
 src/libcamera/sensor_database.cpp            | 94 ++++++++++++++++++++
 7 files changed, 156 insertions(+), 15 deletions(-)
 create mode 100644 include/libcamera/internal/sensor_database.h
 create mode 100644 src/libcamera/sensor_database.cpp

--
2.31.1

Comments

Laurent Pinchart April 27, 2021, 6:47 a.m. UTC | #1 
Hi Jacopo,

Thank you for the patch.

On Tue, Apr 13, 2021 at 12:14:37PM +0200, Jacopo Mondi wrote:
> Introduce a 'database' of camera sensor information, which contains
> the camera sensor properties which are not possible, or desirable,
> to retrieve at run time.
> 
> The camera sensor database is accessed through the static SensorDatabase
> class which provides a single method to obtain the sensor properties.

It's not the class that is static, but the function.

> Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  include/libcamera/internal/meson.build       |  1 +
>  include/libcamera/internal/sensor_database.h | 28 ++++++
>  src/libcamera/meson.build                    |  1 +
>  src/libcamera/sensor_database.cpp            | 94 ++++++++++++++++++++
>  4 files changed, 124 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 1fe3918cfe93..4aaa76ac1db1 100644
> --- a/include/libcamera/internal/meson.build
> +++ b/include/libcamera/internal/meson.build
> @@ -38,6 +38,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..5f58b17a7b1d
> --- /dev/null
> +++ b/include/libcamera/internal/sensor_database.h
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * sensor_database.h - Database of camera sensor properties
> + */
> +#ifndef __LIBCAMERA_SENSOR_DATABASE_H__
> +#define __LIBCAMERA_SENSOR_DATABASE_H__
> +
> +#include <string>
> +
> +#include <libcamera/geometry.h>
> +
> +namespace libcamera {
> +
> +struct CameraSensorProperties {
> +	Size unitCellSize;
> +};
> +
> +class SensorDatabase
> +{
> +public:
> +	static const CameraSensorProperties *get(const std::string &sensor);
> +};

Do we need this class ? It would seem simpler to me to move this
function to the CameraSensorProperties class. It's not big deal though,
but if you keep the class, I'd name it CameraSensorDatabase for
consistency.

> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_SENSOR_DATABASE_H__ */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index e0a48aa23ea5..effae0ef5186 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -44,6 +44,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..d6a97e06adfc
> --- /dev/null
> +++ b/src/libcamera/sensor_database.cpp
> @@ -0,0 +1,94 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * sensor_database.cpp - Database of camera sensor properties
> + */
> +
> +#include "libcamera/internal/sensor_database.h"
> +
> +#include <map>
> +
> +#include "libcamera/internal/log.h"
> +
> +/**
> + * \file sensor_database.h
> + * \brief Database of camera sensor properties
> + *
> + * The camera sensor database collects static information about camera sensors
> + * that is not possible or desirable to retrieve at run time.

Should we say "to retrieve from the device", as, from the point of view
of a user of this class, calling get() is a runtime operation ?

> + */
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(SensorDatabase)
> +
> +/**
> + * \struct CameraSensorProperties
> + * \brief List of sensor properties
> + *
> + * \var CameraSensorProperties::unitCellSize
> + * \brief The physical size of a pixel, including pixel edges, in nanometers.
> + */
> +
> +/**
> + * \class SensorDatabase
> + * \brief Access the 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
> + * CameraSensorProperties list of properties.
> + *
> + * The class provides a single static get() method to access the sensor database
> + * entries. If the sensor is not supported in the database nullptr is returned.

You can drop the last sentence, that's documented below already.

> + */
> +
> +namespace {
> +
> +/** \brief Sony IMX219 sensor properties */
> +constexpr CameraSensorProperties imx219Props = {
> +	.unitCellSize = { 1120, 1120 }
> +};
> +
> +/** \brief Omnivision ov5670 sensor properties */
> +constexpr CameraSensorProperties ov5670Props = {
> +	.unitCellSize = { 1120, 1120 }
> +};
> +
> +/** \brief Omnivision 13858 sensor properties */
> +constexpr CameraSensorProperties ov13858Props = {
> +	.unitCellSize = { 1120, 1120 }
> +};
> +
> +#define SENSOR_PROPERTIES(_sensor) \
> +	{ #_sensor, &_sensor##Props }
> +
> +} /* namespace */
> +
> +/**
> + * \brief Retrieve the properties associated with a sensor
> + * \param sensor The sensor model name
> + * \return A pointer to the CameraSensorProperties instance associated with a sensor
> + * or nullptr if the sensor is not supported in the database
> + */
> +const CameraSensorProperties *SensorDatabase::get(const std::string &sensor)
> +{
> +	static const std::map<std::string, const CameraSensorProperties *> sensorDatabase = {
> +		SENSOR_PROPERTIES(imx219),
> +		SENSOR_PROPERTIES(ov5670),
> +		SENSOR_PROPERTIES(ov13858),
> +	};

I don't really see what the constexpr data above brings us, expect a
macro that obfuscates the code a bit. Isn't this better ?

	static const std::map<std::string, const CameraSensorProperties *> sensorDatabase = {
		{ "imx219", {
			.unitCellSize = { 1120, 1120 },
		} },
		{ "ov5670", {
			.unitCellSize = { 1120, 1120 },
		} },
		{ "ov13858", {
			.unitCellSize = { 1120, 1120 },
		} },
	};

> +
> +	const auto it = sensorDatabase.find(sensor);
> +	if (it == sensorDatabase.end()) {
> +		LOG(SensorDatabase, Warning)
> +			<< "No static properties available for '" << sensor << "'";
> +		LOG(SensorDatabase, Warning)
> +			<< "Please consider updating the sensor database";
> +		return nullptr;
> +	}
> +
> +	return it->second;
> +}
> +
> +} /* namespace libcamera */
Jacopo Mondi April 27, 2021, 7:29 a.m. UTC | #2 
On Tue, Apr 27, 2021 at 09:47:35AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Tue, Apr 13, 2021 at 12:14:37PM +0200, Jacopo Mondi wrote:
> > Introduce a 'database' of camera sensor information, which contains
> > the camera sensor properties which are not possible, or desirable,
> > to retrieve at run time.
> >
> > The camera sensor database is accessed through the static SensorDatabase
> > class which provides a single method to obtain the sensor properties.
>
> It's not the class that is static, but the function.
>
> > Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  include/libcamera/internal/meson.build       |  1 +
> >  include/libcamera/internal/sensor_database.h | 28 ++++++
> >  src/libcamera/meson.build                    |  1 +
> >  src/libcamera/sensor_database.cpp            | 94 ++++++++++++++++++++
> >  4 files changed, 124 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 1fe3918cfe93..4aaa76ac1db1 100644
> > --- a/include/libcamera/internal/meson.build
> > +++ b/include/libcamera/internal/meson.build
> > @@ -38,6 +38,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..5f58b17a7b1d
> > --- /dev/null
> > +++ b/include/libcamera/internal/sensor_database.h
> > @@ -0,0 +1,28 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021, Google Inc.
> > + *
> > + * sensor_database.h - Database of camera sensor properties
> > + */
> > +#ifndef __LIBCAMERA_SENSOR_DATABASE_H__
> > +#define __LIBCAMERA_SENSOR_DATABASE_H__
> > +
> > +#include <string>
> > +
> > +#include <libcamera/geometry.h>
> > +
> > +namespace libcamera {
> > +
> > +struct CameraSensorProperties {
> > +	Size unitCellSize;
> > +};
> > +
> > +class SensorDatabase
> > +{
> > +public:
> > +	static const CameraSensorProperties *get(const std::string &sensor);
> > +};
>
> Do we need this class ? It would seem simpler to me to move this
> function to the CameraSensorProperties class. It's not big deal though,

What's the CameraSensorProperties class ?

> but if you keep the class, I'd name it CameraSensorDatabase for
> consistency.
>
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_SENSOR_DATABASE_H__ */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index e0a48aa23ea5..effae0ef5186 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -44,6 +44,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..d6a97e06adfc
> > --- /dev/null
> > +++ b/src/libcamera/sensor_database.cpp
> > @@ -0,0 +1,94 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021, Google Inc.
> > + *
> > + * sensor_database.cpp - Database of camera sensor properties
> > + */
> > +
> > +#include "libcamera/internal/sensor_database.h"
> > +
> > +#include <map>
> > +
> > +#include "libcamera/internal/log.h"
> > +
> > +/**
> > + * \file sensor_database.h
> > + * \brief Database of camera sensor properties
> > + *
> > + * The camera sensor database collects static information about camera sensors
> > + * that is not possible or desirable to retrieve at run time.
>
> Should we say "to retrieve from the device", as, from the point of view
> of a user of this class, calling get() is a runtime operation ?
>
> > + */
> > +
> > +namespace libcamera {
> > +
> > +LOG_DEFINE_CATEGORY(SensorDatabase)
> > +
> > +/**
> > + * \struct CameraSensorProperties
> > + * \brief List of sensor properties
> > + *
> > + * \var CameraSensorProperties::unitCellSize
> > + * \brief The physical size of a pixel, including pixel edges, in nanometers.
> > + */
> > +
> > +/**
> > + * \class SensorDatabase
> > + * \brief Access the 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
> > + * CameraSensorProperties list of properties.
> > + *
> > + * The class provides a single static get() method to access the sensor database
> > + * entries. If the sensor is not supported in the database nullptr is returned.
>
> You can drop the last sentence, that's documented below already.
>
> > + */
> > +
> > +namespace {
> > +
> > +/** \brief Sony IMX219 sensor properties */
> > +constexpr CameraSensorProperties imx219Props = {
> > +	.unitCellSize = { 1120, 1120 }
> > +};
> > +
> > +/** \brief Omnivision ov5670 sensor properties */
> > +constexpr CameraSensorProperties ov5670Props = {
> > +	.unitCellSize = { 1120, 1120 }
> > +};
> > +
> > +/** \brief Omnivision 13858 sensor properties */
> > +constexpr CameraSensorProperties ov13858Props = {
> > +	.unitCellSize = { 1120, 1120 }
> > +};
> > +
> > +#define SENSOR_PROPERTIES(_sensor) \
> > +	{ #_sensor, &_sensor##Props }
> > +
> > +} /* namespace */
> > +
> > +/**
> > + * \brief Retrieve the properties associated with a sensor
> > + * \param sensor The sensor model name
> > + * \return A pointer to the CameraSensorProperties instance associated with a sensor
> > + * or nullptr if the sensor is not supported in the database
> > + */
> > +const CameraSensorProperties *SensorDatabase::get(const std::string &sensor)
> > +{
> > +	static const std::map<std::string, const CameraSensorProperties *> sensorDatabase = {
> > +		SENSOR_PROPERTIES(imx219),
> > +		SENSOR_PROPERTIES(ov5670),
> > +		SENSOR_PROPERTIES(ov13858),
> > +	};
>
> I don't really see what the constexpr data above brings us, expect a
> macro that obfuscates the code a bit. Isn't this better ?
>
> 	static const std::map<std::string, const CameraSensorProperties *> sensorDatabase = {
> 		{ "imx219", {
> 			.unitCellSize = { 1120, 1120 },
> 		} },
> 		{ "ov5670", {
> 			.unitCellSize = { 1120, 1120 },
> 		} },
> 		{ "ov13858", {
> 			.unitCellSize = { 1120, 1120 },
> 		} },

Thinking forward, when the sensor db will have more fields, I think
declaring the map using the macro is more compact. I can drop it if
you feel strongly about this.

> 	};
>
> > +
> > +	const auto it = sensorDatabase.find(sensor);
> > +	if (it == sensorDatabase.end()) {
> > +		LOG(SensorDatabase, Warning)
> > +			<< "No static properties available for '" << sensor << "'";
> > +		LOG(SensorDatabase, Warning)
> > +			<< "Please consider updating the sensor database";
> > +		return nullptr;
> > +	}
> > +
> > +	return it->second;
> > +}
> > +
> > +} /* namespace libcamera */
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart April 27, 2021, 7:48 a.m. UTC | #3 
Hi Jacopo,

On Tue, Apr 27, 2021 at 09:29:03AM +0200, Jacopo Mondi wrote:
> On Tue, Apr 27, 2021 at 09:47:35AM +0300, Laurent Pinchart wrote:
> > On Tue, Apr 13, 2021 at 12:14:37PM +0200, Jacopo Mondi wrote:
> > > Introduce a 'database' of camera sensor information, which contains
> > > the camera sensor properties which are not possible, or desirable,
> > > to retrieve at run time.
> > >
> > > The camera sensor database is accessed through the static SensorDatabase
> > > class which provides a single method to obtain the sensor properties.
> >
> > It's not the class that is static, but the function.
> >
> > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  include/libcamera/internal/meson.build       |  1 +
> > >  include/libcamera/internal/sensor_database.h | 28 ++++++
> > >  src/libcamera/meson.build                    |  1 +
> > >  src/libcamera/sensor_database.cpp            | 94 ++++++++++++++++++++
> > >  4 files changed, 124 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 1fe3918cfe93..4aaa76ac1db1 100644
> > > --- a/include/libcamera/internal/meson.build
> > > +++ b/include/libcamera/internal/meson.build
> > > @@ -38,6 +38,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..5f58b17a7b1d
> > > --- /dev/null
> > > +++ b/include/libcamera/internal/sensor_database.h
> > > @@ -0,0 +1,28 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2021, Google Inc.
> > > + *
> > > + * sensor_database.h - Database of camera sensor properties
> > > + */
> > > +#ifndef __LIBCAMERA_SENSOR_DATABASE_H__
> > > +#define __LIBCAMERA_SENSOR_DATABASE_H__
> > > +
> > > +#include <string>
> > > +
> > > +#include <libcamera/geometry.h>
> > > +
> > > +namespace libcamera {
> > > +
> > > +struct CameraSensorProperties {
> > > +	Size unitCellSize;
> > > +};
> > > +
> > > +class SensorDatabase
> > > +{
> > > +public:
> > > +	static const CameraSensorProperties *get(const std::string &sensor);
> > > +};
> >
> > Do we need this class ? It would seem simpler to me to move this
> > function to the CameraSensorProperties class. It's not big deal though,
> 
> What's the CameraSensorProperties class ?

Seems to be a struct :-)

> > but if you keep the class, I'd name it CameraSensorDatabase for
> > consistency.
> >
> > > +
> > > +} /* namespace libcamera */
> > > +
> > > +#endif /* __LIBCAMERA_SENSOR_DATABASE_H__ */
> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > index e0a48aa23ea5..effae0ef5186 100644
> > > --- a/src/libcamera/meson.build
> > > +++ b/src/libcamera/meson.build
> > > @@ -44,6 +44,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..d6a97e06adfc
> > > --- /dev/null
> > > +++ b/src/libcamera/sensor_database.cpp
> > > @@ -0,0 +1,94 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2021, Google Inc.
> > > + *
> > > + * sensor_database.cpp - Database of camera sensor properties
> > > + */
> > > +
> > > +#include "libcamera/internal/sensor_database.h"
> > > +
> > > +#include <map>
> > > +
> > > +#include "libcamera/internal/log.h"
> > > +
> > > +/**
> > > + * \file sensor_database.h
> > > + * \brief Database of camera sensor properties
> > > + *
> > > + * The camera sensor database collects static information about camera sensors
> > > + * that is not possible or desirable to retrieve at run time.
> >
> > Should we say "to retrieve from the device", as, from the point of view
> > of a user of this class, calling get() is a runtime operation ?
> >
> > > + */
> > > +
> > > +namespace libcamera {
> > > +
> > > +LOG_DEFINE_CATEGORY(SensorDatabase)
> > > +
> > > +/**
> > > + * \struct CameraSensorProperties
> > > + * \brief List of sensor properties
> > > + *
> > > + * \var CameraSensorProperties::unitCellSize
> > > + * \brief The physical size of a pixel, including pixel edges, in nanometers.
> > > + */
> > > +
> > > +/**
> > > + * \class SensorDatabase
> > > + * \brief Access the 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
> > > + * CameraSensorProperties list of properties.
> > > + *
> > > + * The class provides a single static get() method to access the sensor database
> > > + * entries. If the sensor is not supported in the database nullptr is returned.
> >
> > You can drop the last sentence, that's documented below already.
> >
> > > + */
> > > +
> > > +namespace {
> > > +
> > > +/** \brief Sony IMX219 sensor properties */
> > > +constexpr CameraSensorProperties imx219Props = {
> > > +	.unitCellSize = { 1120, 1120 }
> > > +};
> > > +
> > > +/** \brief Omnivision ov5670 sensor properties */
> > > +constexpr CameraSensorProperties ov5670Props = {
> > > +	.unitCellSize = { 1120, 1120 }
> > > +};
> > > +
> > > +/** \brief Omnivision 13858 sensor properties */
> > > +constexpr CameraSensorProperties ov13858Props = {
> > > +	.unitCellSize = { 1120, 1120 }
> > > +};
> > > +
> > > +#define SENSOR_PROPERTIES(_sensor) \
> > > +	{ #_sensor, &_sensor##Props }
> > > +
> > > +} /* namespace */
> > > +
> > > +/**
> > > + * \brief Retrieve the properties associated with a sensor
> > > + * \param sensor The sensor model name
> > > + * \return A pointer to the CameraSensorProperties instance associated with a sensor
> > > + * or nullptr if the sensor is not supported in the database
> > > + */
> > > +const CameraSensorProperties *SensorDatabase::get(const std::string &sensor)
> > > +{
> > > +	static const std::map<std::string, const CameraSensorProperties *> sensorDatabase = {
> > > +		SENSOR_PROPERTIES(imx219),
> > > +		SENSOR_PROPERTIES(ov5670),
> > > +		SENSOR_PROPERTIES(ov13858),
> > > +	};
> >
> > I don't really see what the constexpr data above brings us, expect a
> > macro that obfuscates the code a bit. Isn't this better ?
> >
> > 	static const std::map<std::string, const CameraSensorProperties *> sensorDatabase = {
> > 		{ "imx219", {
> > 			.unitCellSize = { 1120, 1120 },
> > 		} },
> > 		{ "ov5670", {
> > 			.unitCellSize = { 1120, 1120 },
> > 		} },
> > 		{ "ov13858", {
> > 			.unitCellSize = { 1120, 1120 },
> > 		} },
> 
> Thinking forward, when the sensor db will have more fields, I think
> declaring the map using the macro is more compact. I can drop it if
> you feel strongly about this.

I find the style that we use in, for example, the pixelFormatInfo map in
formats.cpp, more readable than the constexpr + macro here.

> > 	};
> >
> > > +
> > > +	const auto it = sensorDatabase.find(sensor);
> > > +	if (it == sensorDatabase.end()) {
> > > +		LOG(SensorDatabase, Warning)
> > > +			<< "No static properties available for '" << sensor << "'";
> > > +		LOG(SensorDatabase, Warning)
> > > +			<< "Please consider updating the sensor database";
> > > +		return nullptr;
> > > +	}
> > > +
> > > +	return it->second;
> > > +}
> > > +
> > > +} /* namespace libcamera */
Jacopo Mondi April 30, 2021, 4:43 p.m. UTC | #4 
Hi Laurent,

On Tue, Apr 27, 2021 at 10:48:37AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Tue, Apr 27, 2021 at 09:29:03AM +0200, Jacopo Mondi wrote:
> > On Tue, Apr 27, 2021 at 09:47:35AM +0300, Laurent Pinchart wrote:
> > > On Tue, Apr 13, 2021 at 12:14:37PM +0200, Jacopo Mondi wrote:
> > > > Introduce a 'database' of camera sensor information, which contains
> > > > the camera sensor properties which are not possible, or desirable,
> > > > to retrieve at run time.
> > > >
> > > > The camera sensor database is accessed through the static SensorDatabase
> > > > class which provides a single method to obtain the sensor properties.
> > >
> > > It's not the class that is static, but the function.
> > >
> > > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> > > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > ---
> > > >  include/libcamera/internal/meson.build       |  1 +
> > > >  include/libcamera/internal/sensor_database.h | 28 ++++++
> > > >  src/libcamera/meson.build                    |  1 +
> > > >  src/libcamera/sensor_database.cpp            | 94 ++++++++++++++++++++
> > > >  4 files changed, 124 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 1fe3918cfe93..4aaa76ac1db1 100644
> > > > --- a/include/libcamera/internal/meson.build
> > > > +++ b/include/libcamera/internal/meson.build
> > > > @@ -38,6 +38,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..5f58b17a7b1d
> > > > --- /dev/null
> > > > +++ b/include/libcamera/internal/sensor_database.h
> > > > @@ -0,0 +1,28 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2021, Google Inc.
> > > > + *
> > > > + * sensor_database.h - Database of camera sensor properties
> > > > + */
> > > > +#ifndef __LIBCAMERA_SENSOR_DATABASE_H__
> > > > +#define __LIBCAMERA_SENSOR_DATABASE_H__
> > > > +
> > > > +#include <string>
> > > > +
> > > > +#include <libcamera/geometry.h>
> > > > +
> > > > +namespace libcamera {
> > > > +
> > > > +struct CameraSensorProperties {
> > > > +	Size unitCellSize;
> > > > +};
> > > > +
> > > > +class SensorDatabase
> > > > +{
> > > > +public:
> > > > +	static const CameraSensorProperties *get(const std::string &sensor);
> > > > +};
> > >
> > > Do we need this class ? It would seem simpler to me to move this
> > > function to the CameraSensorProperties class. It's not big deal though,
> >
> > What's the CameraSensorProperties class ?
>
> Seems to be a struct :-)

Took me a while to get what you meant

  const CameraSensorProperties *props = CameraSensorProperties::get("imx219");

This might work as well, the SensorDatabase class contains that single
function after all.

A small nit: if CameraSensorProperties is all it remains, should I
name the file(s) camera_sensor_properties.cpp(.h) ?



>
> > > but if you keep the class, I'd name it CameraSensorDatabase for
> > > consistency.
> > >
> > > > +
> > > > +} /* namespace libcamera */
> > > > +
> > > > +#endif /* __LIBCAMERA_SENSOR_DATABASE_H__ */
> > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > > index e0a48aa23ea5..effae0ef5186 100644
> > > > --- a/src/libcamera/meson.build
> > > > +++ b/src/libcamera/meson.build
> > > > @@ -44,6 +44,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..d6a97e06adfc
> > > > --- /dev/null
> > > > +++ b/src/libcamera/sensor_database.cpp
> > > > @@ -0,0 +1,94 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2021, Google Inc.
> > > > + *
> > > > + * sensor_database.cpp - Database of camera sensor properties
> > > > + */
> > > > +
> > > > +#include "libcamera/internal/sensor_database.h"
> > > > +
> > > > +#include <map>
> > > > +
> > > > +#include "libcamera/internal/log.h"
> > > > +
> > > > +/**
> > > > + * \file sensor_database.h
> > > > + * \brief Database of camera sensor properties
> > > > + *
> > > > + * The camera sensor database collects static information about camera sensors
> > > > + * that is not possible or desirable to retrieve at run time.
> > >
> > > Should we say "to retrieve from the device", as, from the point of view
> > > of a user of this class, calling get() is a runtime operation ?
> > >
> > > > + */
> > > > +
> > > > +namespace libcamera {
> > > > +
> > > > +LOG_DEFINE_CATEGORY(SensorDatabase)
> > > > +
> > > > +/**
> > > > + * \struct CameraSensorProperties
> > > > + * \brief List of sensor properties
> > > > + *
> > > > + * \var CameraSensorProperties::unitCellSize
> > > > + * \brief The physical size of a pixel, including pixel edges, in nanometers.
> > > > + */
> > > > +
> > > > +/**
> > > > + * \class SensorDatabase
> > > > + * \brief Access the 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
> > > > + * CameraSensorProperties list of properties.
> > > > + *
> > > > + * The class provides a single static get() method to access the sensor database
> > > > + * entries. If the sensor is not supported in the database nullptr is returned.
> > >
> > > You can drop the last sentence, that's documented below already.
> > >
> > > > + */
> > > > +
> > > > +namespace {
> > > > +
> > > > +/** \brief Sony IMX219 sensor properties */
> > > > +constexpr CameraSensorProperties imx219Props = {
> > > > +	.unitCellSize = { 1120, 1120 }
> > > > +};
> > > > +
> > > > +/** \brief Omnivision ov5670 sensor properties */
> > > > +constexpr CameraSensorProperties ov5670Props = {
> > > > +	.unitCellSize = { 1120, 1120 }
> > > > +};
> > > > +
> > > > +/** \brief Omnivision 13858 sensor properties */
> > > > +constexpr CameraSensorProperties ov13858Props = {
> > > > +	.unitCellSize = { 1120, 1120 }
> > > > +};
> > > > +
> > > > +#define SENSOR_PROPERTIES(_sensor) \
> > > > +	{ #_sensor, &_sensor##Props }
> > > > +
> > > > +} /* namespace */
> > > > +
> > > > +/**
> > > > + * \brief Retrieve the properties associated with a sensor
> > > > + * \param sensor The sensor model name
> > > > + * \return A pointer to the CameraSensorProperties instance associated with a sensor
> > > > + * or nullptr if the sensor is not supported in the database
> > > > + */
> > > > +const CameraSensorProperties *SensorDatabase::get(const std::string &sensor)
> > > > +{
> > > > +	static const std::map<std::string, const CameraSensorProperties *> sensorDatabase = {
> > > > +		SENSOR_PROPERTIES(imx219),
> > > > +		SENSOR_PROPERTIES(ov5670),
> > > > +		SENSOR_PROPERTIES(ov13858),
> > > > +	};
> > >
> > > I don't really see what the constexpr data above brings us, expect a
> > > macro that obfuscates the code a bit. Isn't this better ?
> > >
> > > 	static const std::map<std::string, const CameraSensorProperties *> sensorDatabase = {
> > > 		{ "imx219", {
> > > 			.unitCellSize = { 1120, 1120 },
> > > 		} },
> > > 		{ "ov5670", {
> > > 			.unitCellSize = { 1120, 1120 },
> > > 		} },
> > > 		{ "ov13858", {
> > > 			.unitCellSize = { 1120, 1120 },
> > > 		} },
> >
> > Thinking forward, when the sensor db will have more fields, I think
> > declaring the map using the macro is more compact. I can drop it if
> > you feel strongly about this.
>
> I find the style that we use in, for example, the pixelFormatInfo map in
> formats.cpp, more readable than the constexpr + macro here.
>
> > > 	};
> > >
> > > > +
> > > > +	const auto it = sensorDatabase.find(sensor);
> > > > +	if (it == sensorDatabase.end()) {
> > > > +		LOG(SensorDatabase, Warning)
> > > > +			<< "No static properties available for '" << sensor << "'";
> > > > +		LOG(SensorDatabase, Warning)
> > > > +			<< "Please consider updating the sensor database";
> > > > +		return nullptr;
> > > > +	}
> > > > +
> > > > +	return it->second;
> > > > +}
> > > > +
> > > > +} /* namespace libcamera */
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart May 1, 2021, 2:45 a.m. UTC | #5 
Hi Jacopo,

On Fri, Apr 30, 2021 at 06:43:41PM +0200, Jacopo Mondi wrote:
> On Tue, Apr 27, 2021 at 10:48:37AM +0300, Laurent Pinchart wrote:
> > On Tue, Apr 27, 2021 at 09:29:03AM +0200, Jacopo Mondi wrote:
> > > On Tue, Apr 27, 2021 at 09:47:35AM +0300, Laurent Pinchart wrote:
> > > > On Tue, Apr 13, 2021 at 12:14:37PM +0200, Jacopo Mondi wrote:
> > > > > Introduce a 'database' of camera sensor information, which contains
> > > > > the camera sensor properties which are not possible, or desirable,
> > > > > to retrieve at run time.
> > > > >
> > > > > The camera sensor database is accessed through the static SensorDatabase
> > > > > class which provides a single method to obtain the sensor properties.
> > > >
> > > > It's not the class that is static, but the function.
> > > >
> > > > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> > > > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > > ---
> > > > >  include/libcamera/internal/meson.build       |  1 +
> > > > >  include/libcamera/internal/sensor_database.h | 28 ++++++
> > > > >  src/libcamera/meson.build                    |  1 +
> > > > >  src/libcamera/sensor_database.cpp            | 94 ++++++++++++++++++++
> > > > >  4 files changed, 124 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 1fe3918cfe93..4aaa76ac1db1 100644
> > > > > --- a/include/libcamera/internal/meson.build
> > > > > +++ b/include/libcamera/internal/meson.build
> > > > > @@ -38,6 +38,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..5f58b17a7b1d
> > > > > --- /dev/null
> > > > > +++ b/include/libcamera/internal/sensor_database.h
> > > > > @@ -0,0 +1,28 @@
> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > > +/*
> > > > > + * Copyright (C) 2021, Google Inc.
> > > > > + *
> > > > > + * sensor_database.h - Database of camera sensor properties
> > > > > + */
> > > > > +#ifndef __LIBCAMERA_SENSOR_DATABASE_H__
> > > > > +#define __LIBCAMERA_SENSOR_DATABASE_H__
> > > > > +
> > > > > +#include <string>
> > > > > +
> > > > > +#include <libcamera/geometry.h>
> > > > > +
> > > > > +namespace libcamera {
> > > > > +
> > > > > +struct CameraSensorProperties {
> > > > > +	Size unitCellSize;
> > > > > +};
> > > > > +
> > > > > +class SensorDatabase
> > > > > +{
> > > > > +public:
> > > > > +	static const CameraSensorProperties *get(const std::string &sensor);
> > > > > +};
> > > >
> > > > Do we need this class ? It would seem simpler to me to move this
> > > > function to the CameraSensorProperties class. It's not big deal though,
> > >
> > > What's the CameraSensorProperties class ?
> >
> > Seems to be a struct :-)
> 
> Took me a while to get what you meant
> 
>   const CameraSensorProperties *props = CameraSensorProperties::get("imx219");
> 
> This might work as well, the SensorDatabase class contains that single
> function after all.
> 
> A small nit: if CameraSensorProperties is all it remains, should I
> name the file(s) camera_sensor_properties.cpp(.h) ?

That would work for me. I was even thinking about moving this to
camera_sensor.cpp, but it may grow a bit big in the future. Up to you.

> > > > but if you keep the class, I'd name it CameraSensorDatabase for
> > > > consistency.
> > > >
> > > > > +
> > > > > +} /* namespace libcamera */
> > > > > +
> > > > > +#endif /* __LIBCAMERA_SENSOR_DATABASE_H__ */
> > > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > > > index e0a48aa23ea5..effae0ef5186 100644
> > > > > --- a/src/libcamera/meson.build
> > > > > +++ b/src/libcamera/meson.build
> > > > > @@ -44,6 +44,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..d6a97e06adfc
> > > > > --- /dev/null
> > > > > +++ b/src/libcamera/sensor_database.cpp
> > > > > @@ -0,0 +1,94 @@
> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > > +/*
> > > > > + * Copyright (C) 2021, Google Inc.
> > > > > + *
> > > > > + * sensor_database.cpp - Database of camera sensor properties
> > > > > + */
> > > > > +
> > > > > +#include "libcamera/internal/sensor_database.h"
> > > > > +
> > > > > +#include <map>
> > > > > +
> > > > > +#include "libcamera/internal/log.h"
> > > > > +
> > > > > +/**
> > > > > + * \file sensor_database.h
> > > > > + * \brief Database of camera sensor properties
> > > > > + *
> > > > > + * The camera sensor database collects static information about camera sensors
> > > > > + * that is not possible or desirable to retrieve at run time.
> > > >
> > > > Should we say "to retrieve from the device", as, from the point of view
> > > > of a user of this class, calling get() is a runtime operation ?
> > > >
> > > > > + */
> > > > > +
> > > > > +namespace libcamera {
> > > > > +
> > > > > +LOG_DEFINE_CATEGORY(SensorDatabase)
> > > > > +
> > > > > +/**
> > > > > + * \struct CameraSensorProperties
> > > > > + * \brief List of sensor properties
> > > > > + *
> > > > > + * \var CameraSensorProperties::unitCellSize
> > > > > + * \brief The physical size of a pixel, including pixel edges, in nanometers.
> > > > > + */
> > > > > +
> > > > > +/**
> > > > > + * \class SensorDatabase
> > > > > + * \brief Access the 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
> > > > > + * CameraSensorProperties list of properties.
> > > > > + *
> > > > > + * The class provides a single static get() method to access the sensor database
> > > > > + * entries. If the sensor is not supported in the database nullptr is returned.
> > > >
> > > > You can drop the last sentence, that's documented below already.
> > > >
> > > > > + */
> > > > > +
> > > > > +namespace {
> > > > > +
> > > > > +/** \brief Sony IMX219 sensor properties */
> > > > > +constexpr CameraSensorProperties imx219Props = {
> > > > > +	.unitCellSize = { 1120, 1120 }
> > > > > +};
> > > > > +
> > > > > +/** \brief Omnivision ov5670 sensor properties */
> > > > > +constexpr CameraSensorProperties ov5670Props = {
> > > > > +	.unitCellSize = { 1120, 1120 }
> > > > > +};
> > > > > +
> > > > > +/** \brief Omnivision 13858 sensor properties */
> > > > > +constexpr CameraSensorProperties ov13858Props = {
> > > > > +	.unitCellSize = { 1120, 1120 }
> > > > > +};
> > > > > +
> > > > > +#define SENSOR_PROPERTIES(_sensor) \
> > > > > +	{ #_sensor, &_sensor##Props }
> > > > > +
> > > > > +} /* namespace */
> > > > > +
> > > > > +/**
> > > > > + * \brief Retrieve the properties associated with a sensor
> > > > > + * \param sensor The sensor model name
> > > > > + * \return A pointer to the CameraSensorProperties instance associated with a sensor
> > > > > + * or nullptr if the sensor is not supported in the database
> > > > > + */
> > > > > +const CameraSensorProperties *SensorDatabase::get(const std::string &sensor)
> > > > > +{
> > > > > +	static const std::map<std::string, const CameraSensorProperties *> sensorDatabase = {
> > > > > +		SENSOR_PROPERTIES(imx219),
> > > > > +		SENSOR_PROPERTIES(ov5670),
> > > > > +		SENSOR_PROPERTIES(ov13858),
> > > > > +	};
> > > >
> > > > I don't really see what the constexpr data above brings us, expect a
> > > > macro that obfuscates the code a bit. Isn't this better ?
> > > >
> > > > 	static const std::map<std::string, const CameraSensorProperties *> sensorDatabase = {
> > > > 		{ "imx219", {
> > > > 			.unitCellSize = { 1120, 1120 },
> > > > 		} },
> > > > 		{ "ov5670", {
> > > > 			.unitCellSize = { 1120, 1120 },
> > > > 		} },
> > > > 		{ "ov13858", {
> > > > 			.unitCellSize = { 1120, 1120 },
> > > > 		} },
> > >
> > > Thinking forward, when the sensor db will have more fields, I think
> > > declaring the map using the macro is more compact. I can drop it if
> > > you feel strongly about this.
> >
> > I find the style that we use in, for example, the pixelFormatInfo map in
> > formats.cpp, more readable than the constexpr + macro here.
> >
> > > > 	};
> > > >
> > > > > +
> > > > > +	const auto it = sensorDatabase.find(sensor);
> > > > > +	if (it == sensorDatabase.end()) {
> > > > > +		LOG(SensorDatabase, Warning)
> > > > > +			<< "No static properties available for '" << sensor << "'";
> > > > > +		LOG(SensorDatabase, Warning)
> > > > > +			<< "Please consider updating the sensor database";
> > > > > +		return nullptr;
> > > > > +	}
> > > > > +
> > > > > +	return it->second;
> > > > > +}
> > > > > +
> > > > > +} /* namespace libcamera */