[libcamera-devel] [RFC PATCH] libcamera: base: Make the registration of log categories atomic

[Umang Jain]

Hi Nicolas

On 3/14/23 2:11 AM, Nicolas Dufresne via libcamera-devel wrote:
> From: Nicolas Dufresne <nicolas.dufresne at collabora.com>
>
> Logger:::create() is not currently thread safe and causes crashes

Indeed. Given the fact that gstlibcamerasrc should be ideally moving 
away from using libcamera_private dependency (as LogCategory::create()) 
is an internal API

I can see that it's being called from other internal threads as well 
(like PostProcessorWorker from android HAL layer) so making it thread 
safe makes sense.
> noticeable on RaspberryPi 4. This adds a mutex around the creation
> of categories.
>
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> ---
>
>   src/libcamera/base/log.cpp | 3 +++
>   1 file changed, 3 insertions(+)
>
> This is a quick fix. The crash I'm getting is very frequent, the backtrace
> is the following. Pretty much 50% of the time on Raspberry Pi using:
>
>    gst-launch-1.0 libcamerasrc ! queue ! xvimagesink
>
> Thread 6 "gst-launch-1.0" received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 0x7ff48cc180 (LWP 88100)]
> 0x0000007ff5f48a40 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::compare(char const*) const () from /lib/aarch64-linux-gnu/libstdc++.so.6
> (gdb) bt
> #0  0x0000007ff5f48a40 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::compare(char const*) const () at /lib/aarch64-linux-gnu/libstdc++.so.6
> #1  0x0000007ff664efe0 in std::operator==<char, std::char_traits<char>, std::allocator<char> >(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, char const*)
>      (__lhs=<error reading variable: Cannot access memory at address 0x705f317073696b7a>, __rhs=0x7ff61c4a28 "Event") at /usr/include/c++/10/bits/basic_string.h:6187
> #2  0x0000007ff61b48d4 in operator()<libcamera::LogCategory*>(libcamera::LogCategory*) const (__closure=0x7ff48cb008, c=0x705f317073696b72) at ../src/libcamera/base/log.cpp:748
> #3  0x0000007ff61b4910 in __gnu_cxx::__ops::_Iter_pred<libcamera::Logger::findCategory(char const*) const::<lambda(auto:2)> >::operator()<__gnu_cxx::__normal_iterator<libcamera::LogCategory* const*, std::vector<libcamera::LogCategory*> > >(__gnu_cxx::__normal_iterator<libcamera::LogCategory* const*, std::vector<libcamera::LogCategory*, std::allocator<libcamera::LogCategory*> > >)
>      (this=0x7ff48cb008, __it=0x705f317073696b72) at /usr/include/c++/10/bits/predefined_ops.h:316
> #4  0x0000007ff61b487c in std::__find_if<__gnu_cxx::__normal_iterator<libcamera::LogCategory* const*, std::vector<libcamera::LogCategory*> >, __gnu_cxx::__ops::_Iter_pred<libcamera::Logger::findCategory(char const*) const::<lambda(auto:2)> > >(__gnu_cxx::__normal_iterator<libcamera::LogCategory* const*, std::vector<libcamera::LogCategory*, std::allocator<libcamera::LogCategory*> > >, __gnu_cxx::__normal_iterator<libcamera::LogCategory* const*, std::vector<libcamera::LogCategory*, std::allocator<libcamera::LogCategory*> > >, __gnu_cxx::__ops::_Iter_pred<libcamera::Logger::findCategory(char const*) const::<lambda(auto:2)> >, std::random_access_iterator_tag) (__first=0x705f317073696b72, __last=0x303100736d617261, __pred=...) at /usr/include/c++/10/bits/stl_algobase.h:1959
> #5  0x0000007ff61b46b4 in std::__find_if<__gnu_cxx::__normal_iterator<libcamera::LogCategory* const*, std::vector<libcamera::LogCategory*> >, __gnu_cxx::__ops::_Iter_pred<libcamera::Logger::findCategory(char const*) const::<lambda(auto:2)> > >(__gnu_cxx::__normal_iterator<libcamera::LogCategory* const*, std::vector<libcamera::LogCategory*, std::allocator<libcamera::LogCategory*> > >, __gnu_cxx::__normal_iterator<libcamera::LogCategory* const*, std::vector<libcamera::LogCategory*, std::allocator<libcamera::LogCategory*> > >, __gnu_cxx::__ops::_Iter_pred<libcamera::Logger::findCategory(char const*) const::<lambda(auto:2)> >)
>      (__first=0x555582c130, __last=0x303100736d617261, __pred=...) at /usr/include/c++/10/bits/stl_algobase.h:1974
> #6  0x0000007ff61b4648 in std::find_if<__gnu_cxx::__normal_iterator<libcamera::LogCategory* const*, std::vector<libcamera::LogCategory*> >, libcamera::Logger::findCategory(char const*) const::<lambda(auto:2)> >(__gnu_cxx::__normal_iterator<libcamera::LogCategory* const*, std::vector<libcamera::LogCategory*, std::allocator<libcamera::LogCategory*> > >, __gnu_cxx::__normal_iterator<libcamera::LogCategory* const*, std::vector<libcamera::LogCategory*, std::allocator<libcamera::LogCategory*> > >, struct {...}) (__first=0x555582c130, __last=0x303100736d617261, __pred=...) at /usr/include/c++/10/bits/stl_algo.h:3929
> #7  0x0000007ff61b3f60 in libcamera::Logger::findCategory(char const*) const (this=0x7ff61ec968 <libcamera::Logger::instance()::instance>, name=0x7ff61c4a28 "Event") at ../src/libcamera/base/log.cpp:748
> #8  0x0000007ff61b3fc4 in libcamera::LogCategory::create(char const*) (name=0x7ff61c4a28 "Event") at ../src/libcamera/base/log.cpp:791
> #9  0x0000007ff61a9ffc in libcamera::logCategoryEvent() () at ../src/libcamera/base/event_dispatcher.cpp:17
> #10 0x0000007ff61aac5c in libcamera::EventDispatcherPoll::poll(std::vector<pollfd, std::allocator<pollfd> >*) (this=0x7fe8006f90, pollfds=0x7ff48cb3e8) at ../src/libcamera/base/event_dispatcher_poll.cpp:216
> #11 0x0000007ff61aa8fc in libcamera::EventDispatcherPoll::processEvents() (this=0x7fe8006f90) at ../src/libcamera/base/event_dispatcher_poll.cpp:160
> #12 0x0000007ff61be0bc in libcamera::Thread::exec() (this=0x55555d0380) at ../src/libcamera/base/thread.cpp:341
> #13 0x0000007ff6643e88 in libcamera::CameraManager::Private::run() (this=0x55555d0370) at ../src/libcamera/camera_manager.cpp:122
> #14 0x0000007ff61be038 in libcamera::Thread::startThread() (this=0x55555d0380) at ../src/libcamera/base/thread.cpp:319
> #15 0x0000007ff61c2070 in std::__invoke_impl<void, void (libcamera::Thread::*)(), libcamera::Thread*>(std::__invoke_memfun_deref, void (libcamera::Thread::*&&)(), libcamera::Thread*&&)
>      (__f=@0x5555852ef0: (void (libcamera::Thread::*)(class libcamera::Thread * const)) 0x7ff61bdf38 <libcamera::Thread::startThread()>, __t=@0x5555852ee8: 0x55555d0380) at /usr/include/c++/10/bits/invoke.h:73
> #16 0x0000007ff61c1f88 in std::__invoke<void (libcamera::Thread::*)(), libcamera::Thread*>(void (libcamera::Thread::*&&)(), libcamera::Thread*&&)
>      (__fn=@0x5555852ef0: (void (libcamera::Thread::*)(class libcamera::Thread * const)) 0x7ff61bdf38 <libcamera::Thread::startThread()>) at /usr/include/c++/10/bits/invoke.h:95
> #17 0x0000007ff61c1eec in std::thread::_Invoker<std::tuple<void (libcamera::Thread::*)(), libcamera::Thread*> >::_M_invoke<0ul, 1ul>(std::_Index_tuple<0ul, 1ul>) (this=0x5555852ee8)
>      at /usr/include/c++/10/thread:264
> #18 0x0000007ff61c1ea4 in std::thread::_Invoker<std::tuple<void (libcamera::Thread::*)(), libcamera::Thread*> >::operator()() (this=0x5555852ee8) at /usr/include/c++/10/thread:271
> #19 0x0000007ff61c1e84 in std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (libcamera::Thread::*)(), libcamera::Thread*> > >::_M_run() (this=0x5555852ee0) at /usr/include/c++/10/thread:215
> #20 0x0000007ff5ee1cac in  () at /lib/aarch64-linux-gnu/libstdc++.so.6
> #21 0x0000007ff7c78648 in start_thread (arg=0x7ff48cba80) at pthread_create.c:477
> #22 0x0000007ff7bcec1c in thread_start () at ../sysdeps/unix/sysv/linux/aarch64/clone.S:78
>
>
> diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp
> index 55fbd7b0..c8045ef7 100644
> --- a/src/libcamera/base/log.cpp
> +++ b/src/libcamera/base/log.cpp
> @@ -21,6 +21,7 @@
>   #include <libcamera/logging.h>
>   
>   #include <libcamera/base/backtrace.h>
> +#include <libcamera/base/mutex.h>
>   #include <libcamera/base/thread.h>
>   #include <libcamera/base/utils.h>
>   
> @@ -788,6 +789,8 @@ LogCategory *Logger::findCategory(const char *name) const
>    */
>   LogCategory *LogCategory::create(const char *name)
>   {
> +	static Mutex mutex_;
> +	MutexLocker locker(mutex_);
>   	LogCategory *category = Logger::instance()->findCategory(name);
>   
>   	if (!category) {