Message ID | 20221003212128.32429-8-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: > Avoid naked pointer with memory allocation by returning a unique_ptr > from PipelineHandlerFactory::createInstance(), in order to increase > memory allocation safety. > > This allows iterating over factories in the CameraManager and unit tests > using const pointers. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > include/libcamera/internal/pipeline_handler.h | 8 +++++--- > src/libcamera/camera_manager.cpp | 4 ++-- > src/libcamera/pipeline_handler.cpp | 8 ++++---- > test/ipa/ipa_interface_test.cpp | 4 ++-- > 4 files changed, 13 insertions(+), 11 deletions(-) > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > index ebbdf2aa391f..ad74dc823290 100644 > --- a/include/libcamera/internal/pipeline_handler.h > +++ b/include/libcamera/internal/pipeline_handler.h > @@ -113,7 +113,8 @@ public: > private: > static void registerType(PipelineHandlerFactory *factory); > > - virtual PipelineHandler *createInstance(CameraManager *manager) const = 0; > + virtual std::unique_ptr<PipelineHandler> > + createInstance(CameraManager *manager) const = 0; > > std::string name_; > }; > @@ -125,9 +126,10 @@ public: \ > handler##Factory() : PipelineHandlerFactory(#handler) {} \ > \ > private: \ > - PipelineHandler *createInstance(CameraManager *manager) const \ > + std::unique_ptr<PipelineHandler> \ > + createInstance(CameraManager *manager) const \ > { \ > - return new handler(manager); \ > + return std::make_unique<handler>(manager); \ > } \ > }; \ > static handler##Factory global_##handler##Factory; > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp > index 7ff4bc6fd019..2c3f2d86c469 100644 > --- a/src/libcamera/camera_manager.cpp > +++ b/src/libcamera/camera_manager.cpp > @@ -142,10 +142,10 @@ void CameraManager::Private::createPipelineHandlers() > * file and only fallback on all handlers if there is no > * configuration file. > */ > - std::vector<PipelineHandlerFactory *> &factories = > + const std::vector<PipelineHandlerFactory *> &factories = > PipelineHandlerFactory::factories(); > > - for (PipelineHandlerFactory *factory : factories) { > + for (const PipelineHandlerFactory *factory : factories) { > LOG(Camera, Debug) > << "Found registered pipeline handler '" > << factory->name() << "'"; > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index 4470e9041e8e..488dd8d32cdf 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -678,9 +678,9 @@ PipelineHandlerFactory::PipelineHandlerFactory(const char *name) > */ > std::shared_ptr<PipelineHandler> PipelineHandlerFactory::create(CameraManager *manager) const > { > - PipelineHandler *handler = createInstance(manager); > + std::unique_ptr<PipelineHandler> handler = createInstance(manager); > handler->name_ = name_.c_str(); > - return std::shared_ptr<PipelineHandler>(handler); > + return std::shared_ptr<PipelineHandler>(std::move(handler)); > } > > /** > @@ -727,8 +727,8 @@ std::vector<PipelineHandlerFactory *> &PipelineHandlerFactory::factories() > * macro. It creates a pipeline handler instance associated with the camera > * \a manager. > * > - * \return a pointer to a newly constructed instance of the PipelineHandler > - * subclass corresponding to the factory > + * \return A unique pointer to a newly constructed instance of the > + * PipelineHandler subclass corresponding to the factory > */ > > /** > diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp > index 3c0df843ea61..a629abc28da7 100644 > --- a/test/ipa/ipa_interface_test.cpp > +++ b/test/ipa/ipa_interface_test.cpp > @@ -52,9 +52,9 @@ protected: > ipaManager_ = make_unique<IPAManager>(); > > /* Create a pipeline handler for vimc. */ > - std::vector<PipelineHandlerFactory *> &factories = > + const std::vector<PipelineHandlerFactory *> &factories = > PipelineHandlerFactory::factories(); > - for (PipelineHandlerFactory *factory : factories) { > + for (const PipelineHandlerFactory *factory : factories) { > if (factory->name() == "PipelineHandlerVimc") { > pipe_ = factory->create(nullptr); > break; Reviewed-by: Xavier Roumegue <xavier.roumegue@oss.nxp.com>
Hi Laurent, On Tue, Oct 04, 2022 at 12:21:27AM +0300, Laurent Pinchart via libcamera-devel wrote: > Avoid naked pointer with memory allocation by returning a unique_ptr > from PipelineHandlerFactory::createInstance(), in order to increase > memory allocation safety. > > This allows iterating over factories in the CameraManager and unit tests > using const pointers. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > --- > include/libcamera/internal/pipeline_handler.h | 8 +++++--- > src/libcamera/camera_manager.cpp | 4 ++-- > src/libcamera/pipeline_handler.cpp | 8 ++++---- > test/ipa/ipa_interface_test.cpp | 4 ++-- > 4 files changed, 13 insertions(+), 11 deletions(-) > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > index ebbdf2aa391f..ad74dc823290 100644 > --- a/include/libcamera/internal/pipeline_handler.h > +++ b/include/libcamera/internal/pipeline_handler.h > @@ -113,7 +113,8 @@ public: > private: > static void registerType(PipelineHandlerFactory *factory); > > - virtual PipelineHandler *createInstance(CameraManager *manager) const = 0; > + virtual std::unique_ptr<PipelineHandler> > + createInstance(CameraManager *manager) const = 0; > > std::string name_; > }; > @@ -125,9 +126,10 @@ public: \ > handler##Factory() : PipelineHandlerFactory(#handler) {} \ > \ > private: \ > - PipelineHandler *createInstance(CameraManager *manager) const \ > + std::unique_ptr<PipelineHandler> \ > + createInstance(CameraManager *manager) const \ > { \ > - return new handler(manager); \ > + return std::make_unique<handler>(manager); \ > } \ > }; \ > static handler##Factory global_##handler##Factory; > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp > index 7ff4bc6fd019..2c3f2d86c469 100644 > --- a/src/libcamera/camera_manager.cpp > +++ b/src/libcamera/camera_manager.cpp > @@ -142,10 +142,10 @@ void CameraManager::Private::createPipelineHandlers() > * file and only fallback on all handlers if there is no > * configuration file. > */ > - std::vector<PipelineHandlerFactory *> &factories = > + const std::vector<PipelineHandlerFactory *> &factories = > PipelineHandlerFactory::factories(); > > - for (PipelineHandlerFactory *factory : factories) { > + for (const PipelineHandlerFactory *factory : factories) { > LOG(Camera, Debug) > << "Found registered pipeline handler '" > << factory->name() << "'"; > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index 4470e9041e8e..488dd8d32cdf 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -678,9 +678,9 @@ PipelineHandlerFactory::PipelineHandlerFactory(const char *name) > */ > std::shared_ptr<PipelineHandler> PipelineHandlerFactory::create(CameraManager *manager) const > { > - PipelineHandler *handler = createInstance(manager); > + std::unique_ptr<PipelineHandler> handler = createInstance(manager); > handler->name_ = name_.c_str(); > - return std::shared_ptr<PipelineHandler>(handler); > + return std::shared_ptr<PipelineHandler>(std::move(handler)); > } > > /** > @@ -727,8 +727,8 @@ std::vector<PipelineHandlerFactory *> &PipelineHandlerFactory::factories() > * macro. It creates a pipeline handler instance associated with the camera > * \a manager. > * > - * \return a pointer to a newly constructed instance of the PipelineHandler > - * subclass corresponding to the factory > + * \return A unique pointer to a newly constructed instance of the > + * PipelineHandler subclass corresponding to the factory > */ > > /** > diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp > index 3c0df843ea61..a629abc28da7 100644 > --- a/test/ipa/ipa_interface_test.cpp > +++ b/test/ipa/ipa_interface_test.cpp > @@ -52,9 +52,9 @@ protected: > ipaManager_ = make_unique<IPAManager>(); > > /* Create a pipeline handler for vimc. */ > - std::vector<PipelineHandlerFactory *> &factories = > + const std::vector<PipelineHandlerFactory *> &factories = > PipelineHandlerFactory::factories(); > - for (PipelineHandlerFactory *factory : factories) { > + for (const PipelineHandlerFactory *factory : factories) { > if (factory->name() == "PipelineHandlerVimc") { > pipe_ = factory->create(nullptr); > break; > -- > Regards, > > Laurent Pinchart >
diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h index ebbdf2aa391f..ad74dc823290 100644 --- a/include/libcamera/internal/pipeline_handler.h +++ b/include/libcamera/internal/pipeline_handler.h @@ -113,7 +113,8 @@ public: private: static void registerType(PipelineHandlerFactory *factory); - virtual PipelineHandler *createInstance(CameraManager *manager) const = 0; + virtual std::unique_ptr<PipelineHandler> + createInstance(CameraManager *manager) const = 0; std::string name_; }; @@ -125,9 +126,10 @@ public: \ handler##Factory() : PipelineHandlerFactory(#handler) {} \ \ private: \ - PipelineHandler *createInstance(CameraManager *manager) const \ + std::unique_ptr<PipelineHandler> \ + createInstance(CameraManager *manager) const \ { \ - return new handler(manager); \ + return std::make_unique<handler>(manager); \ } \ }; \ static handler##Factory global_##handler##Factory; diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp index 7ff4bc6fd019..2c3f2d86c469 100644 --- a/src/libcamera/camera_manager.cpp +++ b/src/libcamera/camera_manager.cpp @@ -142,10 +142,10 @@ void CameraManager::Private::createPipelineHandlers() * file and only fallback on all handlers if there is no * configuration file. */ - std::vector<PipelineHandlerFactory *> &factories = + const std::vector<PipelineHandlerFactory *> &factories = PipelineHandlerFactory::factories(); - for (PipelineHandlerFactory *factory : factories) { + for (const PipelineHandlerFactory *factory : factories) { LOG(Camera, Debug) << "Found registered pipeline handler '" << factory->name() << "'"; diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index 4470e9041e8e..488dd8d32cdf 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -678,9 +678,9 @@ PipelineHandlerFactory::PipelineHandlerFactory(const char *name) */ std::shared_ptr<PipelineHandler> PipelineHandlerFactory::create(CameraManager *manager) const { - PipelineHandler *handler = createInstance(manager); + std::unique_ptr<PipelineHandler> handler = createInstance(manager); handler->name_ = name_.c_str(); - return std::shared_ptr<PipelineHandler>(handler); + return std::shared_ptr<PipelineHandler>(std::move(handler)); } /** @@ -727,8 +727,8 @@ std::vector<PipelineHandlerFactory *> &PipelineHandlerFactory::factories() * macro. It creates a pipeline handler instance associated with the camera * \a manager. * - * \return a pointer to a newly constructed instance of the PipelineHandler - * subclass corresponding to the factory + * \return A unique pointer to a newly constructed instance of the + * PipelineHandler subclass corresponding to the factory */ /** diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp index 3c0df843ea61..a629abc28da7 100644 --- a/test/ipa/ipa_interface_test.cpp +++ b/test/ipa/ipa_interface_test.cpp @@ -52,9 +52,9 @@ protected: ipaManager_ = make_unique<IPAManager>(); /* Create a pipeline handler for vimc. */ - std::vector<PipelineHandlerFactory *> &factories = + const std::vector<PipelineHandlerFactory *> &factories = PipelineHandlerFactory::factories(); - for (PipelineHandlerFactory *factory : factories) { + for (const PipelineHandlerFactory *factory : factories) { if (factory->name() == "PipelineHandlerVimc") { pipe_ = factory->create(nullptr); break;
Avoid naked pointer with memory allocation by returning a unique_ptr from PipelineHandlerFactory::createInstance(), in order to increase memory allocation safety. This allows iterating over factories in the CameraManager and unit tests using const pointers. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- include/libcamera/internal/pipeline_handler.h | 8 +++++--- src/libcamera/camera_manager.cpp | 4 ++-- src/libcamera/pipeline_handler.cpp | 8 ++++---- test/ipa/ipa_interface_test.cpp | 4 ++-- 4 files changed, 13 insertions(+), 11 deletions(-)