[libcamera-devel] [PATCH v4 1/3] libcamera: Introduce camera sensor database
Hirokazu Honda
hiroh at chromium.org
Tue Apr 13 09:00:25 CEST 2021
Hi Laurent,
On Tue, Apr 13, 2021 at 11:42 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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.
>
Thanks. I don't remember the description, although I think I read it previously.
That sounds good to me. Perhaps, the map would be put as a const
static variable within CameraSensorInfo::get().
> > > > > 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