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

[Laurent Pinchart]

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 at 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 at 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.

-- 
Regards,

Laurent Pinchart