[libcamera-devel] [PATCH 4/8] ipa: camera_sensor_helper: Implement factories through class templates
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Oct 7 16:27:10 CEST 2022
Hi Jacopo,
On Fri, Oct 07, 2022 at 04:08:41PM +0200, Jacopo Mondi wrote:
> On Tue, Oct 04, 2022 at 12:21:24AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > The REGISTER_CAMERA_SENSOR_HELPER() macro defines a class type that
> > inherits from the CameraSensorHelperFactory class, and implements a
> > constructor and createInstance() function. Replace the code generation
> > through macro with the C++ equivalent, a class template, as done by the
> > Algorithm factory.
>
> I am been looking for a way to remove "Base" from the factory name,
> but it seems we're running out of names...
:-)
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > src/ipa/ipu3/ipu3.cpp | 2 +-
> > src/ipa/libipa/camera_sensor_helper.cpp | 79 +++++++++++++++----------
> > src/ipa/libipa/camera_sensor_helper.h | 43 ++++++++------
> > src/ipa/rkisp1/rkisp1.cpp | 2 +-
> > 4 files changed, 75 insertions(+), 51 deletions(-)
> >
> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > index b93a09d40c39..852deb710956 100644
> > --- a/src/ipa/ipu3/ipu3.cpp
> > +++ b/src/ipa/ipu3/ipu3.cpp
> > @@ -326,7 +326,7 @@ int IPAIPU3::init(const IPASettings &settings,
> > const ControlInfoMap &sensorControls,
> > ControlInfoMap *ipaControls)
> > {
> > - camHelper_ = CameraSensorHelperFactory::create(settings.sensorModel);
> > + camHelper_ = CameraSensorHelperFactoryBase::create(settings.sensorModel);
> > if (camHelper_ == nullptr) {
> > LOG(IPAIPU3, Error)
> > << "Failed to create camera sensor helper for "
> > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> > index 3a7d701d8616..e655be255f2b 100644
> > --- a/src/ipa/libipa/camera_sensor_helper.cpp
> > +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> > @@ -43,7 +43,8 @@ namespace ipa {
> > * \brief Construct a CameraSensorHelper instance
> > *
> > * CameraSensorHelper derived class instances shall never be constructed
> > - * manually but always through the CameraSensorHelperFactory::create() function.
> > + * manually but always through the CameraSensorHelperFactoryBase::create()
> > + * function.
> > */
> >
> > /**
> > @@ -217,27 +218,25 @@ double CameraSensorHelper::gain(uint32_t gainCode) const
> > */
> >
> > /**
> > - * \class CameraSensorHelperFactory
> > - * \brief Registration of CameraSensorHelperFactory classes and creation of instances
> > + * \class CameraSensorHelperFactoryBase
> > + * \brief Base class for camera sensor helper factories
> > *
> > - * To facilitate discovery and instantiation of CameraSensorHelper classes, the
> > - * CameraSensorHelperFactory class maintains a registry of camera sensor helper
> > - * sub-classes. Each CameraSensorHelper subclass shall register itself using the
> > - * REGISTER_CAMERA_SENSOR_HELPER() macro, which will create a corresponding
> > - * instance of a CameraSensorHelperFactory subclass and register it with the
> > - * static list of factories.
> > + * The CameraSensorHelperFactoryBase class is the base of all specializations of
> > + * the CameraSensorHelperFactory class template. It implements the factory
> > + * registration, maintains a registry of factories, and provides access to the
> > + * registered factories.
> > */
> >
> > /**
> > - * \brief Construct a camera sensor helper factory
> > + * \brief Construct a camera sensor helper factory base
> > * \param[in] name Name of the camera sensor helper class
> > *
> > - * Creating an instance of the factory registers it with the global list of
> > + * Creating an instance of the factory base registers it with the global list of
> > * factories, accessible through the factories() function.
>
> As this is only called by the subclasses to register their types,
> reading "Creating an instance of the factory base -> registers it"
> while it's actually the construction of the derived class that uses
> the base class constructor for registration.
What I meant is that the constructor of the base class registers the
instance. It's of course called by the constructor of the derived class.
> A detail though
>
> > *
> > * The factory \a name is used for debug purpose and shall be unique.
> > */
> > -CameraSensorHelperFactory::CameraSensorHelperFactory(const std::string name)
> > +CameraSensorHelperFactoryBase::CameraSensorHelperFactoryBase(const std::string name)
> > : name_(name)
> > {
> > registerType(this);
> > @@ -252,12 +251,12 @@ CameraSensorHelperFactory::CameraSensorHelperFactory(const std::string name)
> > * corresponding to the named factory or a null pointer if no such factory
> > * exists
> > */
> > -std::unique_ptr<CameraSensorHelper> CameraSensorHelperFactory::create(const std::string &name)
> > +std::unique_ptr<CameraSensorHelper> CameraSensorHelperFactoryBase::create(const std::string &name)
> > {
> > - const std::vector<CameraSensorHelperFactory *> &factories =
> > - CameraSensorHelperFactory::factories();
> > + const std::vector<CameraSensorHelperFactoryBase *> &factories =
> > + CameraSensorHelperFactoryBase::factories();
> >
> > - for (const CameraSensorHelperFactory *factory : factories) {
> > + for (const CameraSensorHelperFactoryBase *factory : factories) {
> > if (name != factory->name_)
> > continue;
> >
> > @@ -274,10 +273,10 @@ std::unique_ptr<CameraSensorHelper> CameraSensorHelperFactory::create(const std:
> > * The caller is responsible to guarantee the uniqueness of the camera sensor
> > * helper name.
> > */
> > -void CameraSensorHelperFactory::registerType(CameraSensorHelperFactory *factory)
> > +void CameraSensorHelperFactoryBase::registerType(CameraSensorHelperFactoryBase *factory)
> > {
> > - std::vector<CameraSensorHelperFactory *> &factories =
> > - CameraSensorHelperFactory::factories();
> > + std::vector<CameraSensorHelperFactoryBase *> &factories =
> > + CameraSensorHelperFactoryBase::factories();
> >
> > factories.push_back(factory);
> > }
> > @@ -286,35 +285,55 @@ void CameraSensorHelperFactory::registerType(CameraSensorHelperFactory *factory)
> > * \brief Retrieve the list of all camera sensor helper factories
> > * \return The list of camera sensor helper factories
> > */
> > -std::vector<CameraSensorHelperFactory *> &CameraSensorHelperFactory::factories()
> > +std::vector<CameraSensorHelperFactoryBase *> &CameraSensorHelperFactoryBase::factories()
> > {
> > /*
> > * The static factories map is defined inside the function to ensure
> > * it gets initialized on first use, without any dependency on link
> > * order.
> > */
> > - static std::vector<CameraSensorHelperFactory *> factories;
> > + static std::vector<CameraSensorHelperFactoryBase *> factories;
> > return factories;
> > }
> >
> > +/**
> > + * \var CameraSensorHelperFactoryBase::name_
> > + * \brief The name of the factory
> > + */
>
> Should we document a private field ?
This patch only moves the preexisting documentation, but you're right,
I'll drop this.
> > +
> > +/**
> > + * \class CameraSensorHelperFactory
> > + * \brief Registration of CameraSensorHelperFactory classes and creation of instances
> > + * \tparam _Helper The camera sensor helepr class type for this factory
>
> s/helepr/helper
>
> > + *
> > + * To facilitate discovery and instantiation of CameraSensorHelper classes, the
> > + * CameraSensorHelperFactory class implements auto-registration of camera sensor
> > + * helpers. Each CameraSensorHelper subclass shall register itself using the
> > + * REGISTER_CAMERA_SENSOR_HELPER() macro, which will create a corresponding
> > + * instance of a CameraSensorHelperFactory subclass and register it with the
> > + * static list of factories.
> > + */
> > +
> > +/**
> > + * \fn CameraSensorHelperFactory::CameraSensorHelperFactory(const char *name)
> > + * \brief Construct a camera sensor helper factory
> > + * \param[in] name Name of the camera sensor helper class
> > + *
> > + * Creating an instance of the factory registers it with the global list of
> > + * factories, accessible through the factories() function.
>
> factories() is not part of this class..
Indeed it's in the base class. I'll update that.
> > + *
> > + * The factory \a name is used for debug purpose and shall be unique.
>
> Don't we actually match on the name ?
We do. I'll fix this.
> > + */
> > +
> > /**
> > * \fn CameraSensorHelperFactory::createInstance() const
> > * \brief Create an instance of the CameraSensorHelper corresponding to the
> > * factory
> > *
> > - * This virtual function is implemented by the REGISTER_CAMERA_SENSOR_HELPER()
> > - * macro. It creates a camera sensor helper instance associated with the camera
> > - * sensor model.
> > - *
> > * \return A unique pointer to a newly constructed instance of the
> > * CameraSensorHelper subclass corresponding to the factory
> > */
> >
> > -/**
> > - * \var CameraSensorHelperFactory::name_
> > - * \brief The name of the factory
> > - */
> > -
> > /**
> > * \def REGISTER_CAMERA_SENSOR_HELPER
> > * \brief Register a camera sensor helper with the camera sensor helper factory
> > diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h
> > index 21ee43cc9f9f..3ea1806cb1fd 100644
> > --- a/src/ipa/libipa/camera_sensor_helper.h
> > +++ b/src/ipa/libipa/camera_sensor_helper.h
> > @@ -58,39 +58,44 @@ private:
> > LIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelper)
> > };
> >
> > -class CameraSensorHelperFactory
> > +class CameraSensorHelperFactoryBase
> > {
> > public:
> > - CameraSensorHelperFactory(const std::string name);
> > - virtual ~CameraSensorHelperFactory() = default;
> > + CameraSensorHelperFactoryBase(const std::string name);
>
> Can this be made protected now ?
It could, and would then make it impossible to instantiate the class
directly, but that's impossible already, as there's a pure virtual
function.
> > + virtual ~CameraSensorHelperFactoryBase() = default;
> >
> > static std::unique_ptr<CameraSensorHelper> create(const std::string &name);
> >
> > - static std::vector<CameraSensorHelperFactory *> &factories();
> > + static std::vector<CameraSensorHelperFactoryBase *> &factories();
> >
> > private:
> > - LIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelperFactory)
> > + LIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelperFactoryBase)
> >
> > - static void registerType(CameraSensorHelperFactory *factory);
> > + static void registerType(CameraSensorHelperFactoryBase *factory);
> >
> > virtual std::unique_ptr<CameraSensorHelper> createInstance() const = 0;
> >
> > std::string name_;
> > };
> >
> > -#define REGISTER_CAMERA_SENSOR_HELPER(name, helper) \
> > -class helper##Factory final : public CameraSensorHelperFactory \
> > -{ \
> > -public: \
> > - helper##Factory() : CameraSensorHelperFactory(name) {} \
> > - \
> > -private: \
> > - std::unique_ptr<CameraSensorHelper> createInstance() const \
> > - { \
> > - return std::make_unique<helper>(); \
> > - } \
> > -}; \
> > -static helper##Factory global_##helper##Factory;
> > +template<typename _Helper>
> > +class CameraSensorHelperFactory final : public CameraSensorHelperFactoryBase
> > +{
> > +public:
> > + CameraSensorHelperFactory(const char *name)
> > + : CameraSensorHelperFactoryBase(name)
> > + {
> > + }
> > +
> > +private:
> > + std::unique_ptr<CameraSensorHelper> createInstance() const
> > + {
> > + return std::make_unique<_Helper>();
> > + }
> > +};
> > +
> > +#define REGISTER_CAMERA_SENSOR_HELPER(name, helper) \
> > +static CameraSensorHelperFactory<helper> global_##helper##Factory(name);
>
> All minors
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
> >
> > } /* namespace ipa */
> >
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index 1335e3d14df2..9451cb03a285 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -143,7 +143,7 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
> > /* Cache the value to set it in configure. */
> > hwRevision_ = static_cast<rkisp1_cif_isp_version>(hwRevision);
> >
> > - camHelper_ = CameraSensorHelperFactory::create(settings.sensorModel);
> > + camHelper_ = CameraSensorHelperFactoryBase::create(settings.sensorModel);
> > if (!camHelper_) {
> > LOG(IPARkISP1, Error)
> > << "Failed to create camera sensor helper for "
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list