[libcamera-devel] [PATCH 4/8] ipa: camera_sensor_helper: Implement factories through class templates
Jacopo Mondi
jacopo at jmondi.org
Fri Oct 7 16:08:41 CEST 2022
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.
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 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