[libcamera-devel,v2,2/7] IPAManager: remove instance() and make createIPA() static
diff mbox series

Message ID 20200605090106.15424-3-paul.elder@ideasonboard.com
State Accepted
Headers show
Series
  • Support qv4l2 with v4l2-compat
Related show

Commit Message

Paul Elder June 5, 2020, 9:01 a.m. UTC 
As the only usage of IPAManager::instance() is by the pipeline handlers
to call IPAManager::createIPA(), remove the former and make the latter
static. Update the pipeline handlers and tests accordingly.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
New in v2
---
 include/libcamera/internal/ipa_manager.h       |  8 +++-----
 src/libcamera/ipa_manager.cpp                  | 18 ++----------------
 .../pipeline/raspberrypi/raspberrypi.cpp       |  2 +-
 src/libcamera/pipeline/rkisp1/rkisp1.cpp       |  2 +-
 src/libcamera/pipeline/vimc/vimc.cpp           |  2 +-
 test/ipa/ipa_interface_test.cpp                |  2 +-
 6 files changed, 9 insertions(+), 25 deletions(-)

Comments

Laurent Pinchart June 5, 2020, 10:15 a.m. UTC | #1 
Hi Paul,

Thank you for the patch.

On Fri, Jun 05, 2020 at 06:01:01PM +0900, Paul Elder wrote:
> As the only usage of IPAManager::instance() is by the pipeline handlers
> to call IPAManager::createIPA(), remove the former and make the latter
> static. Update the pipeline handlers and tests accordingly.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> 
> ---
> New in v2
> ---
>  include/libcamera/internal/ipa_manager.h       |  8 +++-----
>  src/libcamera/ipa_manager.cpp                  | 18 ++----------------
>  .../pipeline/raspberrypi/raspberrypi.cpp       |  2 +-
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp       |  2 +-
>  src/libcamera/pipeline/vimc/vimc.cpp           |  2 +-
>  test/ipa/ipa_interface_test.cpp                |  2 +-
>  6 files changed, 9 insertions(+), 25 deletions(-)
> 
> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h
> index 43f700d3..05643e5e 100644
> --- a/include/libcamera/internal/ipa_manager.h
> +++ b/include/libcamera/internal/ipa_manager.h
> @@ -25,11 +25,9 @@ public:
>  	IPAManager();
>  	~IPAManager();
>  
> -	static IPAManager *instance();
> -
> -	std::unique_ptr<IPAProxy> createIPA(PipelineHandler *pipe,
> -					    uint32_t maxVersion,
> -					    uint32_t minVersion);
> +	static std::unique_ptr<IPAProxy> createIPA(PipelineHandler *pipe,
> +						   uint32_t maxVersion,
> +						   uint32_t minVersion);
>  
>  private:
>  	static IPAManager *self_;
> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> index abb681a3..9d0c2069 100644
> --- a/src/libcamera/ipa_manager.cpp
> +++ b/src/libcamera/ipa_manager.cpp
> @@ -159,20 +159,6 @@ IPAManager::~IPAManager()
>  	self_ = nullptr;
>  }
>  
> -/**
> - * \brief Retrieve the IPA manager instance
> - *
> - * The IPAManager is a singleton and can't be constructed manually. This
> - * function shall instead be used to retrieve the single global instance of the
> - * manager.
> - *
> - * \return The IPA manager instance
> - */
> -IPAManager *IPAManager::instance()
> -{
> -	return self_;
> -}
> -
>  /**
>   * \brief Identify shared library objects within a directory
>   * \param[in] libDir The directory to search for shared objects
> @@ -274,7 +260,7 @@ std::unique_ptr<IPAProxy> IPAManager::createIPA(PipelineHandler *pipe,
>  {
>  	IPAModule *m = nullptr;
>  
> -	for (IPAModule *module : modules_) {
> +	for (IPAModule *module : self_->modules_) {
>  		if (module->match(pipe, minVersion, maxVersion)) {
>  			m = module;
>  			break;
> @@ -290,7 +276,7 @@ std::unique_ptr<IPAProxy> IPAManager::createIPA(PipelineHandler *pipe,
>  	 *
>  	 * \todo Implement a better proxy selection
>  	 */
> -	const char *proxyName = isSignatureValid(m)
> +	const char *proxyName = self_->isSignatureValid(m)
>  			      ? "IPAProxyThread" : "IPAProxyLinux";
>  	IPAProxyFactory *pf = nullptr;
>  
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index e16a9c7f..b9b88506 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1118,7 +1118,7 @@ void RPiCameraData::frameStarted(uint32_t sequence)
>  
>  int RPiCameraData::loadIPA()
>  {
> -	ipa_ = IPAManager::instance()->createIPA(pipe_, 1, 1);
> +	ipa_ = IPAManager::createIPA(pipe_, 1, 1);
>  	if (!ipa_)
>  		return -ENOENT;
>  
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index d807fc2c..900f873a 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -395,7 +395,7 @@ private:
>  
>  int RkISP1CameraData::loadIPA()
>  {
> -	ipa_ = IPAManager::instance()->createIPA(pipe_, 1, 1);
> +	ipa_ = IPAManager::createIPA(pipe_, 1, 1);
>  	if (!ipa_)
>  		return -ENOENT;
>  
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> index ba9fca50..3881545b 100644
> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> @@ -410,7 +410,7 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
>  
>  	std::unique_ptr<VimcCameraData> data = std::make_unique<VimcCameraData>(this, media);
>  
> -	data->ipa_ = IPAManager::instance()->createIPA(this, 0, 0);
> +	data->ipa_ = IPAManager::createIPA(this, 0, 0);
>  	if (data->ipa_ != nullptr) {
>  		std::string conf = data->ipa_->configurationFile("vimc.conf");
>  		data->ipa_->init(IPASettings{ conf });
> diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp
> index 153493ba..1bc93a63 100644
> --- a/test/ipa/ipa_interface_test.cpp
> +++ b/test/ipa/ipa_interface_test.cpp
> @@ -95,7 +95,7 @@ protected:
>  		EventDispatcher *dispatcher = thread()->eventDispatcher();
>  		Timer timer;
>  
> -		ipa_ = IPAManager::instance()->createIPA(pipe_.get(), 0, 0);
> +		ipa_ = IPAManager::createIPA(pipe_.get(), 0, 0);
>  		if (!ipa_) {
>  			cerr << "Failed to create VIMC IPA interface" << endl;
>  			return TestFail;
Niklas Söderlund June 5, 2020, 6:09 p.m. UTC | #2 
Hi Paul,

Thanks for your work.

On 2020-06-05 18:01:01 +0900, Paul Elder wrote:
> As the only usage of IPAManager::instance() is by the pipeline handlers
> to call IPAManager::createIPA(), remove the former and make the latter
> static. Update the pipeline handlers and tests accordingly.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> 
> ---
> New in v2
> ---
>  include/libcamera/internal/ipa_manager.h       |  8 +++-----
>  src/libcamera/ipa_manager.cpp                  | 18 ++----------------
>  .../pipeline/raspberrypi/raspberrypi.cpp       |  2 +-
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp       |  2 +-
>  src/libcamera/pipeline/vimc/vimc.cpp           |  2 +-
>  test/ipa/ipa_interface_test.cpp                |  2 +-
>  6 files changed, 9 insertions(+), 25 deletions(-)
> 
> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h
> index 43f700d3..05643e5e 100644
> --- a/include/libcamera/internal/ipa_manager.h
> +++ b/include/libcamera/internal/ipa_manager.h
> @@ -25,11 +25,9 @@ public:
>  	IPAManager();
>  	~IPAManager();
>  
> -	static IPAManager *instance();
> -
> -	std::unique_ptr<IPAProxy> createIPA(PipelineHandler *pipe,
> -					    uint32_t maxVersion,
> -					    uint32_t minVersion);
> +	static std::unique_ptr<IPAProxy> createIPA(PipelineHandler *pipe,
> +						   uint32_t maxVersion,
> +						   uint32_t minVersion);
>  
>  private:
>  	static IPAManager *self_;
> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> index abb681a3..9d0c2069 100644
> --- a/src/libcamera/ipa_manager.cpp
> +++ b/src/libcamera/ipa_manager.cpp
> @@ -159,20 +159,6 @@ IPAManager::~IPAManager()
>  	self_ = nullptr;
>  }
>  
> -/**
> - * \brief Retrieve the IPA manager instance
> - *
> - * The IPAManager is a singleton and can't be constructed manually. This
> - * function shall instead be used to retrieve the single global instance of the
> - * manager.
> - *
> - * \return The IPA manager instance
> - */
> -IPAManager *IPAManager::instance()
> -{
> -	return self_;
> -}
> -
>  /**
>   * \brief Identify shared library objects within a directory
>   * \param[in] libDir The directory to search for shared objects
> @@ -274,7 +260,7 @@ std::unique_ptr<IPAProxy> IPAManager::createIPA(PipelineHandler *pipe,
>  {
>  	IPAModule *m = nullptr;
>  
> -	for (IPAModule *module : modules_) {
> +	for (IPAModule *module : self_->modules_) {
>  		if (module->match(pipe, minVersion, maxVersion)) {
>  			m = module;
>  			break;
> @@ -290,7 +276,7 @@ std::unique_ptr<IPAProxy> IPAManager::createIPA(PipelineHandler *pipe,
>  	 *
>  	 * \todo Implement a better proxy selection
>  	 */
> -	const char *proxyName = isSignatureValid(m)
> +	const char *proxyName = self_->isSignatureValid(m)
>  			      ? "IPAProxyThread" : "IPAProxyLinux";
>  	IPAProxyFactory *pf = nullptr;
>  
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index e16a9c7f..b9b88506 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1118,7 +1118,7 @@ void RPiCameraData::frameStarted(uint32_t sequence)
>  
>  int RPiCameraData::loadIPA()
>  {
> -	ipa_ = IPAManager::instance()->createIPA(pipe_, 1, 1);
> +	ipa_ = IPAManager::createIPA(pipe_, 1, 1);
>  	if (!ipa_)
>  		return -ENOENT;
>  
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index d807fc2c..900f873a 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -395,7 +395,7 @@ private:
>  
>  int RkISP1CameraData::loadIPA()
>  {
> -	ipa_ = IPAManager::instance()->createIPA(pipe_, 1, 1);
> +	ipa_ = IPAManager::createIPA(pipe_, 1, 1);
>  	if (!ipa_)
>  		return -ENOENT;
>  
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> index ba9fca50..3881545b 100644
> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> @@ -410,7 +410,7 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
>  
>  	std::unique_ptr<VimcCameraData> data = std::make_unique<VimcCameraData>(this, media);
>  
> -	data->ipa_ = IPAManager::instance()->createIPA(this, 0, 0);
> +	data->ipa_ = IPAManager::createIPA(this, 0, 0);
>  	if (data->ipa_ != nullptr) {
>  		std::string conf = data->ipa_->configurationFile("vimc.conf");
>  		data->ipa_->init(IPASettings{ conf });
> diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp
> index 153493ba..1bc93a63 100644
> --- a/test/ipa/ipa_interface_test.cpp
> +++ b/test/ipa/ipa_interface_test.cpp
> @@ -95,7 +95,7 @@ protected:
>  		EventDispatcher *dispatcher = thread()->eventDispatcher();
>  		Timer timer;
>  
> -		ipa_ = IPAManager::instance()->createIPA(pipe_.get(), 0, 0);
> +		ipa_ = IPAManager::createIPA(pipe_.get(), 0, 0);
>  		if (!ipa_) {
>  			cerr << "Failed to create VIMC IPA interface" << endl;
>  			return TestFail;
> -- 
> 2.20.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series 

diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h
index 43f700d3..05643e5e 100644
--- a/include/libcamera/internal/ipa_manager.h
+++ b/include/libcamera/internal/ipa_manager.h
@@ -25,11 +25,9 @@  public:
 	IPAManager();
 	~IPAManager();
 
-	static IPAManager *instance();
-
-	std::unique_ptr<IPAProxy> createIPA(PipelineHandler *pipe,
-					    uint32_t maxVersion,
-					    uint32_t minVersion);
+	static std::unique_ptr<IPAProxy> createIPA(PipelineHandler *pipe,
+						   uint32_t maxVersion,
+						   uint32_t minVersion);
 
 private:
 	static IPAManager *self_;
diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
index abb681a3..9d0c2069 100644
--- a/src/libcamera/ipa_manager.cpp
+++ b/src/libcamera/ipa_manager.cpp
@@ -159,20 +159,6 @@  IPAManager::~IPAManager()
 	self_ = nullptr;
 }
 
-/**
- * \brief Retrieve the IPA manager instance
- *
- * The IPAManager is a singleton and can't be constructed manually. This
- * function shall instead be used to retrieve the single global instance of the
- * manager.
- *
- * \return The IPA manager instance
- */
-IPAManager *IPAManager::instance()
-{
-	return self_;
-}
-
 /**
  * \brief Identify shared library objects within a directory
  * \param[in] libDir The directory to search for shared objects
@@ -274,7 +260,7 @@  std::unique_ptr<IPAProxy> IPAManager::createIPA(PipelineHandler *pipe,
 {
 	IPAModule *m = nullptr;
 
-	for (IPAModule *module : modules_) {
+	for (IPAModule *module : self_->modules_) {
 		if (module->match(pipe, minVersion, maxVersion)) {
 			m = module;
 			break;
@@ -290,7 +276,7 @@  std::unique_ptr<IPAProxy> IPAManager::createIPA(PipelineHandler *pipe,
 	 *
 	 * \todo Implement a better proxy selection
 	 */
-	const char *proxyName = isSignatureValid(m)
+	const char *proxyName = self_->isSignatureValid(m)
 			      ? "IPAProxyThread" : "IPAProxyLinux";
 	IPAProxyFactory *pf = nullptr;
 
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
index e16a9c7f..b9b88506 100644
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
@@ -1118,7 +1118,7 @@  void RPiCameraData::frameStarted(uint32_t sequence)
 
 int RPiCameraData::loadIPA()
 {
-	ipa_ = IPAManager::instance()->createIPA(pipe_, 1, 1);
+	ipa_ = IPAManager::createIPA(pipe_, 1, 1);
 	if (!ipa_)
 		return -ENOENT;
 
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index d807fc2c..900f873a 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -395,7 +395,7 @@  private:
 
 int RkISP1CameraData::loadIPA()
 {
-	ipa_ = IPAManager::instance()->createIPA(pipe_, 1, 1);
+	ipa_ = IPAManager::createIPA(pipe_, 1, 1);
 	if (!ipa_)
 		return -ENOENT;
 
diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
index ba9fca50..3881545b 100644
--- a/src/libcamera/pipeline/vimc/vimc.cpp
+++ b/src/libcamera/pipeline/vimc/vimc.cpp
@@ -410,7 +410,7 @@  bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
 
 	std::unique_ptr<VimcCameraData> data = std::make_unique<VimcCameraData>(this, media);
 
-	data->ipa_ = IPAManager::instance()->createIPA(this, 0, 0);
+	data->ipa_ = IPAManager::createIPA(this, 0, 0);
 	if (data->ipa_ != nullptr) {
 		std::string conf = data->ipa_->configurationFile("vimc.conf");
 		data->ipa_->init(IPASettings{ conf });
diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp
index 153493ba..1bc93a63 100644
--- a/test/ipa/ipa_interface_test.cpp
+++ b/test/ipa/ipa_interface_test.cpp
@@ -95,7 +95,7 @@  protected:
 		EventDispatcher *dispatcher = thread()->eventDispatcher();
 		Timer timer;
 
-		ipa_ = IPAManager::instance()->createIPA(pipe_.get(), 0, 0);
+		ipa_ = IPAManager::createIPA(pipe_.get(), 0, 0);
 		if (!ipa_) {
 			cerr << "Failed to create VIMC IPA interface" << endl;
 			return TestFail;