[libcamera-devel] [PATCH v4 1/3] libcamera: Introduce camera sensor database

[Jacopo Mondi]

Hello,

On Tue, Apr 13, 2021 at 04:00:25PM +0900, Hirokazu Honda wrote:
> > > > > > > > > +
> > > > > > > > > +/**
> > > > > > > > > + * \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.
> >

Indeed I used std::pair to be able to declare it as constexpr.

I'll make it a map if that's a not a requirement. I'll see if it's
possbile to statically initialize it as I'm currently doing with the
pair

>
> 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().
>

The map needs to be statically initialized, it could be done inside
the ::get() function most probably. Something like (not tested)

const SensorInfo *SensorDatabase::get(const std::string &sensor)
{
	static const std::map<const char *const, const SensorInfo *> sensorDb = {
		SENSOR_INFO(imx219),
		SENSOR_INFO(ov5670),
		SENSOR_INFO(ov13858),
	};

	const auto it = sensorDb.find(sensor.c_str());
	if (it == sensorDb.end())
		return nullptr;

	return it->second;
}

Thanks
   j



> > > > > > 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