[libcamera-devel,1/1] libcamera: replaced Meyer's singleton

Message ID 39c4470d6fa04c17828f49a9afc8ffd9@mtkmbs07n1.mediatek.inc
State Superseded
Headers show
Series
  • [libcamera-devel,1/1] libcamera: replaced Meyer's singleton
Related show

Commit Message

Rynn Wu (吳育恩) Nov. 8, 2019, 4:22 a.m. UTC
Hi there,

I found a Meyer’s singleton in IPAManager, basically Meyer’s singleton is supposed to be removed since it causes some unexpected exceptions in Android platforms while destroying the process.

I replaced it by std::call_once and std::unique_ptr, here is the patch, please kindly help to review it, thanks :-)


Rynn.


From 7efdb2e21263ff559cda8af3ec49721967ba8c60 Mon Sep 17 00:00:00 2001
From: Rynn Wu <rynn.wu@mediatek.com>
Date: Fri, 8 Nov 2019 12:05:55 +0800
Subject: [PATCH] libcamera: replaced Meyer's singleton

Since destroying order of Meyer's singleton is not guarantee and
there were some unexpected behaviors in Android platforms, it's better
to replace it by other ways.

Change-Id: Id2da20ea641f580cb3566f034cc9958bc391eac7
Signed-off-by: Rynn Wu <rynn.wu@mediatek.com>
---
src/libcamera/include/ipa_manager.h |  8 ++++++++
src/libcamera/ipa_manager.cpp       | 12 ++++++++++--
2 files changed, 18 insertions(+), 2 deletions(-)

}
+std::unique_ptr<IPAManager, std::function<void(IPAManager*)>> IPAManager::singleton_;
+std::once_flag IPAManager:: singletonFlag_;
+
/**
  * \brief Retrieve the IPA manager instance
  *
@@ -90,8 +94,12 @@ IPAManager::~IPAManager()
  */
IPAManager *IPAManager::instance()
{
-              static IPAManager ipaManager;
-              return &ipaManager;
+             std::call_once(IPAManager::singletonFlag_, [](){
+                                             /* never release, give an empty customized deleter */
+                                             IPAManager::singleton_ = std::unique_ptr<IPAManager, std::function<void(IPAManager*)>>(
+                                                             new IPAManager, [](IPAManager*){});
+                                             });
+             return IPAManager::singleton_.get();
}
 /**
--
2.18.0

Comments

Laurent Pinchart Nov. 8, 2019, 11:28 a.m. UTC | #1
Hello Rynn Wu,

Thank you for the patch.

On Fri, Nov 08, 2019 at 04:22:33AM +0000, Rynn Wu (吳育恩) wrote:
> Hi there,
> 
> I found a Meyer’s singleton in IPAManager, basically Meyer’s singleton is
> supposed to be removed since it causes some unexpected exceptions in Android
> platforms while destroying the process.

I wasn't aware of this issue, would you have a pointer to documentation
explaining why it causes problems specifically for Android ? Or is it a
design pattern that should always be avoided ?

> I replaced it by std::call_once and std::unique_ptr, here is the patch, please
> kindly help to review it, thanks :-)

Your mailer or mail server has corrupted the patch by adding blank
lines and converting tabs to spaces, so I can't apply it properly. Is
this something you could fix ? Maybe by using git-send-email to send the
patch ?

> From 7efdb2e21263ff559cda8af3ec49721967ba8c60 Mon Sep 17 00:00:00 2001
> 
> From: Rynn Wu <rynn.wu@mediatek.com>
> 
> Date: Fri, 8 Nov 2019 12:05:55 +0800
> 
> Subject: [PATCH] libcamera: replaced Meyer's singleton
> 
>  
> 
> Since destroying order of Meyer's singleton is not guarantee and
> 
> there were some unexpected behaviors in Android platforms, it's better
> 
> to replace it by other ways.
> 
>  
> 
> Change-Id: Id2da20ea641f580cb3566f034cc9958bc391eac7

Change-Id isn't applicable to libcamera upstream development, please
don't include it in patches.

> Signed-off-by: Rynn Wu <rynn.wu@mediatek.com>
> 
> ---
> 
> src/libcamera/include/ipa_manager.h |  8 ++++++++
> 
> src/libcamera/ipa_manager.cpp       | 12 ++++++++++--
> 
> 2 files changed, 18 insertions(+), 2 deletions(-)
> 
>  
> 
> diff --git a/src/libcamera/include/ipa_manager.h b/src/libcamera/include/
> ipa_manager.h
> 
> index 126f8ba..edaedf9 100644
> 
> --- a/src/libcamera/include/ipa_manager.h
> 
> +++ b/src/libcamera/include/ipa_manager.h
> 
> @@ -7,6 +7,9 @@
> 
> #ifndef __LIBCAMERA_IPA_MANAGER_H__
> 
> #define __LIBCAMERA_IPA_MANAGER_H__
> 
> +#include <functional>
> 
> +#include <memory>
> 
> +#include <mutex>
> 
> #include <vector>
> 
>  #include <ipa/ipa_interface.h>
> 
> @@ -33,6 +36,11 @@ private:
> 
>                ~IPAManager();
> 
>                 int addDir(const char *libDir);
> 
> +
> 
> +private:
> 
> +             static std::unique_ptr<IPAManager, std::function<void
> (IPAManager*)>> singleton_;
> 
> +             static std::once_flag singletonFlag_;
> 
> +
> 
> };
> 
>  } /* namespace libcamera */
> 
> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> 
> index f3180c0..8f60be4 100644
> 
> --- a/src/libcamera/ipa_manager.cpp
> 
> +++ b/src/libcamera/ipa_manager.cpp
> 
> @@ -9,6 +9,7 @@
> 
>  #include <algorithm>
> 
> #include <dirent.h>
> 
> +#include <memory>
> 
> #include <string.h>
> 
> #include <sys/types.h>
> 
> @@ -79,6 +80,9 @@ IPAManager::~IPAManager()
> 
>                                delete module;
> 
> }
> 
> +std::unique_ptr<IPAManager, std::function<void(IPAManager*)>>
> IPAManager::singleton_;
> 
> +std::once_flag IPAManager:: singletonFlag_;
> 
> +
> 
> /**
> 
>   * \brief Retrieve the IPA manager instance
> 
>   *
> 
> @@ -90,8 +94,12 @@ IPAManager::~IPAManager()
> 
>   */
> 
> IPAManager *IPAManager::instance()
> 
> {
> 
> -              static IPAManager ipaManager;
> 
> -              return &ipaManager;
> 
> +             std::call_once(IPAManager::singletonFlag_, [](){
> 
> +                                             /* never release, give an empty
> customized deleter */

This causes a memory leak, which will show up in valgrind. We want to
avoid this, otherwise it gets difficult to tell true leaks apart from
false positives in a valgrind report.

> 
> +                                             IPAManager::singleton_ =
> std::unique_ptr<IPAManager, std::function<void(IPAManager*)>>(
> 
> +                                                             new IPAManager,
> [](IPAManager*){});
> 
> +                                             });
> 
> +             return IPAManager::singleton_.get();
> 
> }
> 
>  /**
> 
> --
> 
> 2.18.0
> 
>  
> 
> ************* MEDIATEK Confidentiality Notice ********************
> The information contained in this e-mail message (including any
> attachments) may be confidential, proprietary, privileged, or otherwise
> exempt from disclosure under applicable laws. It is intended to be
> conveyed only to the designated recipient(s). Any use, dissemination,
> distribution, printing, retaining or copying of this e-mail (including its
> attachments) by unintended recipient(s) is strictly prohibited and may
> be unlawful. If you are not an intended recipient of this e-mail, or believe
> that you have received this e-mail in error, please notify the sender
> immediately (by replying to this e-mail), delete any and all copies of
> this e-mail (including any attachments) from your system, and do not
> disclose the content of this e-mail to any other person. Thank you!

This prevents me from applying the patch, sorry. Please drop such
headers on upstream open-source contributions, as it explicitly states
that the content is confidential and can't be used.

Patch

diff --git a/src/libcamera/include/ipa_manager.h b/src/libcamera/include/ipa_manager.h
index 126f8ba..edaedf9 100644
--- a/src/libcamera/include/ipa_manager.h
+++ b/src/libcamera/include/ipa_manager.h
@@ -7,6 +7,9 @@ 
#ifndef __LIBCAMERA_IPA_MANAGER_H__
#define __LIBCAMERA_IPA_MANAGER_H__
+#include <functional>
+#include <memory>
+#include <mutex>
#include <vector>
 #include <ipa/ipa_interface.h>
@@ -33,6 +36,11 @@  private:
               ~IPAManager();
                int addDir(const char *libDir);
+
+private:
+             static std::unique_ptr<IPAManager, std::function<void(IPAManager*)>> singleton_;
+             static std::once_flag singletonFlag_;
+
};
 } /* namespace libcamera */
diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
index f3180c0..8f60be4 100644
--- a/src/libcamera/ipa_manager.cpp
+++ b/src/libcamera/ipa_manager.cpp
@@ -9,6 +9,7 @@ 
 #include <algorithm>
#include <dirent.h>
+#include <memory>
#include <string.h>
#include <sys/types.h>
@@ -79,6 +80,9 @@  IPAManager::~IPAManager()
                               delete module;