[libcamera-devel] [PATCH v4 1/3] libcamera: Introduce camera sensor database
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Apr 13 04:42:09 CEST 2021
Hi Hiro,
On Tue, Apr 13, 2021 at 11:19:03AM +0900, Hirokazu Honda wrote:
> On Tue, Apr 13, 2021 at 11:03 AM Laurent Pinchart wrote:
> > On Tue, Apr 13, 2021 at 10:56:40AM +0900, Hirokazu Honda wrote:
> > > On Tue, Apr 13, 2021 at 10:45 AM Laurent Pinchart wrote:
> > > > On Mon, Apr 12, 2021 at 10:07:17PM +0900, Hirokazu Honda wrote:
> > > > > On Mon, Apr 12, 2021 at 6:10 PM Niklas Söderlund wrote:
> > > > > > On 2021-04-12 09:57:00 +0200, Jacopo Mondi wrote:
> > > > > > > Introduce a 'database' of camera sensor information, which contains
> > > > > > > a the camera sensor properties which is not possible, or desirable,
> > > >
> > > > s/is not/are not/
> > > >
> > > > > > s/^a //
> > > > > >
> > > > > > > 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 at jmondi.org>
> > > > > >
> > > > > > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > > > > >
> > > > > > > ---
> > > > > > > include/libcamera/internal/meson.build | 1 +
> > > > > > > include/libcamera/internal/sensor_database.h | 28 ++++++
> > > > > > > src/libcamera/meson.build | 1 +
> > > > > > > src/libcamera/sensor_database.cpp | 97 ++++++++++++++++++++
> > > > > > > 4 files changed, 127 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..7d743e46102d
> > > > > > > --- /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 SensorInfo {
> > > >
> > > > I'd name this CameraSensorInfo, as we may have to support other types of
> > > > sensor in the future.
> > > >
> > > > > > > + Size unitCellSize;
> > > > > > > +};
> > > > > > > +
> > > > > > > +class SensorDatabase
> > > > > > > +{
> > > > > > > +public:
> > > > > > > + static const SensorInfo *get(const std::string &sensor);
> > > > > > > +};
> > > > > > > +
> > > > >
> > > > > Just FYI: https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_and_Global_Functions
> > > > > > Do not create classes only to group static members; this is no different than just giving the names a common prefix, and such grouping is usually unnecessary anyway.
> > > > >
> > > > > However, I am not against this code. I am personally fine with class +
> > > > > static member functions.
> > > >
> > > > Moving the static function to the SensorInfo class would solve this
> > > > problem.
> > > >
> > > > > > > +} /* 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..68e69e8bf1b6
> > > > > > > --- /dev/null
> > > > > > > +++ b/src/libcamera/sensor_database.cpp
> > > > > > > @@ -0,0 +1,97 @@
> > > > > > > +/* 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 <algorithm>
> > > > > > > +
> > > > > > > +namespace libcamera {
> > > >
> > > > The namespace goes after \file.
> > > >
> > > > > > > +
> > > > > > > +/**
> > > > > > > + * \file sensor_database.h
> > > > > > > + * \brief Database of camera sensor properties
> > > > > > > + *
> > > > > > > + * The camera sensor database collects information on camera sensors
> > > >
> > > > s/information on/static information about/
> > > >
> > > > > > > + * which is not possible or desirable to retrieve at run-time.
> > > >
> > > > s/which/that/
> > > > s/run-time/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
> > > >
> > > > The function is called get(). But in any case, let's move it to the
> > > > SensorInfo class.
> > > >
> > > > > > > + * 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
> > > >
> > > > s/pixel/a pixel/
> > > >
> > > > > > > + * nano-meters.
> > > >
> > > > s/nano-meters/nanometers/
> > > >
> > > > This field is a Size, so no need to repeat Width x height.
> > > >
> > > > * \brief The physical size of a pixel, including pixel edges, in nanometers
> > > >
> > > > > > > + */
> > > > > > > +
> > > > > > > +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),
> > > > > > > +};
> > > > > > > +
> > > > > > > +} /* 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)
> > > > > > > +{
> > > > > > > + for (unsigned int i = 0; i < std::size(sensorDatabase__); ++i) {
> > > > > > > + if (sensorDatabase__[i].first == sensor)
> > > > > > > + return sensorDatabase__[i].second;
> > > > > > > + }
> > > >
> > > > Why not std::map ?
> > >
> > > I expect the reason is std::map cannot be constructed as constexpr.
> >
> > But the only function that uses it is get(), which isn't constexpr, so
> > is that really a problem ?
>
> Global and static function should be trivially destructible.
> https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables
We have a more relaxed rule, documented in
Documentation/coding_style.rst. In this case, a map is much more
convenient, and as its initialization and destruction is independent of
all other global variables, it doesn't risk causing most of the issues
mentioned in the above link.
> > > > namespace {
> > > >
> > > > const std::map<std::string, SensorInfo> sensorDatabase{
> > > > { "imx219", {
> > > > .unitCellSize = { 1120, 1120 },
> > > > } },
> > > > ...
> > > > };
> > > >
> > > > } /* namespace */
> > > >
> > > > and this function can become
> > > >
> > > > const CameraSensorInfo *CameraSensorInfo::get(const std::string &sensor)
> > > > {
> > > > auto iter = sensorDatabase.find(sensor);
> > > > if (iter == sensorDatabase.end())
> > > > return nullptr;
> > > >
> > > > return *iter;
> > > > }
> > > >
> > > > > > > +
> > > > > > > + return nullptr;
> > > > > > > +}
> > > > > > > +
> > > > > > > +} /* namespace libcamera */
> > > > >
> > > > > Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list