[libcamera-devel,v3,0/4] libcamera: Introduce sensor database
mbox series

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

Message

Jacopo Mondi Dec. 28, 2020, 4:51 p.m. UTC 
Hello, this v3 redesigns the sensor database to:

- use a custom structure to report properties in place of a ControlList
  - This allows dropping the controversial list-initialization contructor
    for the ControlList class

- make SensorDatabase a static class to only interface with the sensor
  database which is now not exposed anymore to the rest of the library

- use a plain C-style array for the sensor database
  Here, I struggled quite a lot to make everything a constexpr, not even being
  100% sure it -really- makes a difference compared to static const declaration
  (the documentation I found gave me conflicting suggestions, or maybe I didn't
  fully get it :)

  So, I had a map as the database underlying structure. Making a map a constexpr
  would have required sub-classing it, and exposing only constexpr methods,
  which I considered not worth it. So I used an array of std::pair.
  Constructing pairs has its own peculiarities, and I managed to get it work
  with clang on my laptop, but failed to have it compile on cros (which uses
  clang, but an older version). I assumed this was an indication I was looking
  for troubles, as supporting all compilers in the build compilers matrix would
  have been complex, so I fell back to plain C-style array, which are easier but
  indeed less C++-ish than using an STL container. Also, we lose a bit of
  efficiency in lookups, but I don't expect this to happen in critical paths, and
  I don't expect to have thousands of sensors to support.

Series based on the two series I have in review (CFA and exposure times
calculations).

Thanks
   j

Jacopo Mondi (4):
  libcamera: Introduce camera sensor database
  libcamera: camera_sensor: Register static properties
  android: camera_device: Report sensor physical size
  android: camera_device: Align style of active area 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                |  49 +++++----
 src/libcamera/camera_sensor.cpp              |  21 +++-
 src/libcamera/meson.build                    |   1 +
 src/libcamera/sensor_database.cpp            | 102 +++++++++++++++++++
 7 files changed, 180 insertions(+), 23 deletions(-)
 create mode 100644 include/libcamera/internal/sensor_database.h
 create mode 100644 src/libcamera/sensor_database.cpp

--
2.29.2

Comments

Paul Elder Dec. 29, 2020, 4:29 a.m. UTC | #1 
Hi Jacopo,

On Mon, Dec 28, 2020 at 05:52:00PM +0100, Jacopo Mondi wrote:
> Introduce a 'database' of camera sensor information, which contains
> a the camera sensor properties which is not possible, or desirable,

s/a //
s/is/are/

> 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 information.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  include/libcamera/internal/meson.build       |   1 +
>  include/libcamera/internal/sensor_database.h |  28 +++++
>  src/libcamera/meson.build                    |   1 +
>  src/libcamera/sensor_database.cpp            | 102 +++++++++++++++++++
>  4 files changed, 132 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 1b1bdc779512..e1f22f238a10 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..e074b3029932
> --- /dev/null
> +++ b/include/libcamera/internal/sensor_database.h
> @@ -0,0 +1,28 @@
> +/* 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 <string>
> +
> +#include <libcamera/geometry.h>
> +
> +namespace libcamera {
> +
> +struct SensorInfo {
> +	Size unitCellSize;
> +};
> +
> +class SensorDatabase
> +{
> +public:
> +	static const SensorInfo *get(const std::string &sensor);
> +};
> +
> +} /* 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..5f8538a62388
> --- /dev/null
> +++ b/src/libcamera/sensor_database.cpp
> @@ -0,0 +1,102 @@
> +/* 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 <algorithm>
> +
> +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 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
> + * SensorInfo list of properties.
> + *
> + * The class provides a single static getInfo() method to access the sensor
> + * database entries. If the sensor is not supported in the database nullptr is
> + * returned.
> + */
> +
> +/**
> + * \struct SensorInfo
> + * \brief List of sensor properties
> + *
> + * \var SensorInfo::unitCellSize
> + * \brief The physical size of pixel, including pixel edges. Width x height in
> + * nano-meters.
> + */
> +
> +namespace {
> +
> +/**
> + * \brief Sony IMX219 sensor properties
> + */
> +constexpr SensorInfo imx219Info = {
> +	.unitCellSize = { 1120, 1120 }
> +};
> +
> +/**
> + * \brief Omnivision ov5670 sensor properties
> + */
> +constexpr SensorInfo ov5670Info = {
> +	.unitCellSize = { 1120, 1120 }
> +};
> +
> +/**
> + * \brief Omnivision 13858 sensor properties
> + */
> +constexpr SensorInfo ov13858Info = {
> +	.unitCellSize = { 1120, 1120 }
> +};
> +
> +#define SENSOR_INFO(_sensor) \
> +	{ #_sensor, &_sensor##Info }
> +
> +/*
> + * \brief The database of sensor properties
> + */
> +constexpr std::pair<const char *const, const SensorInfo *> sensorDatabase__[] = {
> +	SENSOR_INFO(imx219),
> +	SENSOR_INFO(ov5670),
> +	SENSOR_INFO(ov13858),
> +};
> +
> +const SensorInfo *getInfo(const std::string &sensor)
> +{
> +	for (unsigned int i = 0; i < std::size(sensorDatabase__); ++i) {
> +		if (sensorDatabase__[i].first == sensor)
> +			return sensorDatabase__[i].second;
> +	}
> +
> +	return nullptr;
> +}
> +
> +} /* namespace */
> +
> +/**
> + * \brief Retrieve the properties associated with a sensor
> + * \param sensor The sensor model name
> + * \return A pointer to the SensorInfo instance associated with a sensor or
> + * nullptr if the sensor is not supported in the database
> + */
> +const SensorInfo *SensorDatabase::get(const std::string &sensor)
> +{
> +	return getInfo(sensor);
> +}
> +
> +} /* namespace libcamera */
> -- 
> 2.29.2
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Niklas Söderlund Dec. 29, 2020, 5:20 p.m. UTC | #2 
Hi Jacopo,

Thanks for your work.

On 2020-12-28 17:52:00 +0100, Jacopo Mondi wrote:
> Introduce a 'database' of camera sensor information, which contains
> a the camera sensor properties which is 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 information.
> 
> 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            | 102 +++++++++++++++++++
>  4 files changed, 132 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 1b1bdc779512..e1f22f238a10 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..e074b3029932
> --- /dev/null
> +++ b/include/libcamera/internal/sensor_database.h
> @@ -0,0 +1,28 @@
> +/* 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 <string>
> +
> +#include <libcamera/geometry.h>
> +
> +namespace libcamera {
> +
> +struct SensorInfo {
> +	Size unitCellSize;
> +};
> +
> +class SensorDatabase
> +{
> +public:
> +	static const SensorInfo *get(const std::string &sensor);
> +};
> +
> +} /* 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..5f8538a62388
> --- /dev/null
> +++ b/src/libcamera/sensor_database.cpp
> @@ -0,0 +1,102 @@
> +/* 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 <algorithm>
> +
> +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 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
> + * SensorInfo list of properties.
> + *
> + * The class provides a single static getInfo() method to access the sensor
> + * database entries. If the sensor is not supported in the database nullptr is
> + * returned.
> + */
> +
> +/**
> + * \struct SensorInfo
> + * \brief List of sensor properties
> + *
> + * \var SensorInfo::unitCellSize
> + * \brief The physical size of pixel, including pixel edges. Width x height in
> + * nano-meters.
> + */
> +
> +namespace {
> +
> +/**
> + * \brief Sony IMX219 sensor properties
> + */
> +constexpr SensorInfo imx219Info = {
> +	.unitCellSize = { 1120, 1120 }
> +};
> +
> +/**
> + * \brief Omnivision ov5670 sensor properties
> + */
> +constexpr SensorInfo ov5670Info = {
> +	.unitCellSize = { 1120, 1120 }
> +};
> +
> +/**
> + * \brief Omnivision 13858 sensor properties
> + */
> +constexpr SensorInfo ov13858Info = {
> +	.unitCellSize = { 1120, 1120 }
> +};
> +
> +#define SENSOR_INFO(_sensor) \
> +	{ #_sensor, &_sensor##Info }
> +
> +/*
> + * \brief The database of sensor properties
> + */
> +constexpr std::pair<const char *const, const SensorInfo *> sensorDatabase__[] = {

Maybe something for later but if model registration similar to 
REGISTER_PIPELINE_HANDLER() this could be a std::map which would improve 
lookup speeds, if we ever get a large set of sensors. But I think this 
could be done on-top as it won't impact any users.

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> +	SENSOR_INFO(imx219),
> +	SENSOR_INFO(ov5670),
> +	SENSOR_INFO(ov13858),
> +};
> +
> +const SensorInfo *getInfo(const std::string &sensor)
> +{
> +	for (unsigned int i = 0; i < std::size(sensorDatabase__); ++i) {
> +		if (sensorDatabase__[i].first == sensor)
> +			return sensorDatabase__[i].second;
> +	}
> +
> +	return nullptr;
> +}
> +
> +} /* namespace */
> +
> +/**
> + * \brief Retrieve the properties associated with a sensor
> + * \param sensor The sensor model name
> + * \return A pointer to the SensorInfo instance associated with a sensor or
> + * nullptr if the sensor is not supported in the database
> + */
> +const SensorInfo *SensorDatabase::get(const std::string &sensor)
> +{
> +	return getInfo(sensor);
> +}
> +
> +} /* namespace libcamera */
> -- 
> 2.29.2
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Jacopo Mondi Dec. 30, 2020, 8:13 a.m. UTC | #3 
Hi Niklas,

On Tue, Dec 29, 2020 at 06:20:40PM +0100, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your work.
>
> On 2020-12-28 17:52:00 +0100, Jacopo Mondi wrote:
> > Introduce a 'database' of camera sensor information, which contains
> > a the camera sensor properties which is 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 information.
> >
> > 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            | 102 +++++++++++++++++++
> >  4 files changed, 132 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 1b1bdc779512..e1f22f238a10 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..e074b3029932
> > --- /dev/null
> > +++ b/include/libcamera/internal/sensor_database.h
> > @@ -0,0 +1,28 @@
> > +/* 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 <string>
> > +
> > +#include <libcamera/geometry.h>
> > +
> > +namespace libcamera {
> > +
> > +struct SensorInfo {
> > +	Size unitCellSize;
> > +};
> > +
> > +class SensorDatabase
> > +{
> > +public:
> > +	static const SensorInfo *get(const std::string &sensor);
> > +};
> > +
> > +} /* 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..5f8538a62388
> > --- /dev/null
> > +++ b/src/libcamera/sensor_database.cpp
> > @@ -0,0 +1,102 @@
> > +/* 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 <algorithm>
> > +
> > +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 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
> > + * SensorInfo list of properties.
> > + *
> > + * The class provides a single static getInfo() method to access the sensor
> > + * database entries. If the sensor is not supported in the database nullptr is
> > + * returned.
> > + */
> > +
> > +/**
> > + * \struct SensorInfo
> > + * \brief List of sensor properties
> > + *
> > + * \var SensorInfo::unitCellSize
> > + * \brief The physical size of pixel, including pixel edges. Width x height in
> > + * nano-meters.
> > + */
> > +
> > +namespace {
> > +
> > +/**
> > + * \brief Sony IMX219 sensor properties
> > + */
> > +constexpr SensorInfo imx219Info = {
> > +	.unitCellSize = { 1120, 1120 }
> > +};
> > +
> > +/**
> > + * \brief Omnivision ov5670 sensor properties
> > + */
> > +constexpr SensorInfo ov5670Info = {
> > +	.unitCellSize = { 1120, 1120 }
> > +};
> > +
> > +/**
> > + * \brief Omnivision 13858 sensor properties
> > + */
> > +constexpr SensorInfo ov13858Info = {
> > +	.unitCellSize = { 1120, 1120 }
> > +};
> > +
> > +#define SENSOR_INFO(_sensor) \
> > +	{ #_sensor, &_sensor##Info }
> > +
> > +/*
> > + * \brief The database of sensor properties
> > + */
> > +constexpr std::pair<const char *const, const SensorInfo *> sensorDatabase__[] = {
>
> Maybe something for later but if model registration similar to
> REGISTER_PIPELINE_HANDLER() this could be a std::map which would improve
> lookup speeds, if we ever get a large set of sensors. But I think this
> could be done on-top as it won't impact any users.

My understanding is that the factory implementation we have realized
with REGISTER_PIPELINE_HANDLER() guarantees the static list of
handlers is initialized at library startup, but does not guarantee
that it is compile-time evaluated and statically initialized as
constexpr does.

as detailed in the cover letter I moved from an std::map, to an
std::array, and finally fell back on a plain C-style array to be able
to ensure that and please all the compilers I've tested with.

Thanks
  j


>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
>
> > +	SENSOR_INFO(imx219),
> > +	SENSOR_INFO(ov5670),
> > +	SENSOR_INFO(ov13858),
> > +};
> > +
> > +const SensorInfo *getInfo(const std::string &sensor)
> > +{
> > +	for (unsigned int i = 0; i < std::size(sensorDatabase__); ++i) {
> > +		if (sensorDatabase__[i].first == sensor)
> > +			return sensorDatabase__[i].second;
> > +	}
> > +
> > +	return nullptr;
> > +}
> > +
> > +} /* namespace */
> > +
> > +/**
> > + * \brief Retrieve the properties associated with a sensor
> > + * \param sensor The sensor model name
> > + * \return A pointer to the SensorInfo instance associated with a sensor or
> > + * nullptr if the sensor is not supported in the database
> > + */
> > +const SensorInfo *SensorDatabase::get(const std::string &sensor)
> > +{
> > +	return getInfo(sensor);
> > +}
> > +
> > +} /* namespace libcamera */
> > --
> > 2.29.2
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund