| Message ID | 20221003212128.32429-5-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thanks for the patch. On 10/3/22 23:21, 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. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@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. > * > * 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 > + */ > + > +/** > + * \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. > + * > + * The factory \a name is used for debug purpose and shall be unique. > + */ > + > /** > * \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); > + 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); > > } /* 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 " With the typo fixed, Reviewed-by: Xavier Roumegue <xavier.roumegue@oss.nxp.com>
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@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. 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 ? > + > +/** > + * \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.. > + * > + * The factory \a name is used for debug purpose and shall be unique. Don't we actually match on the name ? > + */ > + > /** > * \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 ? > + 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@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 >
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@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@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 "
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. * * 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 + */ + +/** + * \class CameraSensorHelperFactory + * \brief Registration of CameraSensorHelperFactory classes and creation of instances + * \tparam _Helper The camera sensor helepr class type for this factory + * + * 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. + * + * The factory \a name is used for debug purpose and shall be unique. + */ + /** * \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); + 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); } /* 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 "
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. Signed-off-by: Laurent Pinchart <laurent.pinchart@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(-)