| Message ID | 39c4470d6fa04c17828f49a9afc8ffd9@mtkmbs07n1.mediatek.inc |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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.
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;