[libcamera-devel] [PATCH v5 1/3] libcamera: Introduce camera sensor database
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Apr 27 09:48:37 CEST 2021
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 at chromium.org>
> > > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > > Signed-off-by: Jacopo Mondi <jacopo at 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 */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list