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

[Hirokazu Honda]

Hi Jacopo,

On Tue, Apr 13, 2021 at 4:39 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> 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


Yep, this code looks good to me. (and should work to my best of knowledge)
-Hiro
>
>
>
> > > > > > > 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