[vlc-devel] [PATCH 4/4] thread: make vlc_thread_self() always work

Thomas Guillem thomas at gllm.fr
Tue Feb 18 11:07:19 CET 2020


I don't like recursive locking either.
I though recursive locking was avalaible on all platforms, that why I wondered why you would add something to implement it.

Anyway, I'm OK with this patch, vlc_thread_self() might be very useful for debug purpose.

On Tue, Feb 18, 2020, at 11:02, Rémi Denis-Courmont wrote:
> Hi,
> 
> IMO, recursive locking is bad design to begin with. With that said, VLC uses it even though not all platforms support it ditectly, and I'm not going to remove the usages.
> 
> But if you want to, be my guest. Then we can drop vlc_thread_self() entirely.
> 
> Le 18 février 2020 10:57:43 GMT+02:00, Thomas Guillem <thomas at gllm.fr> a écrit :
>> 
>> On Mon, Feb 17, 2020, at 21:56, Rémi Denis-Courmont wrote:
>>> This changes the return type of vlc_thread_self() to match (one of) the
>>> C run-time native thread handle type. Then we add a macro to compare
>>> handles for equality, which may be useful for self-debugging or to
>>> implement recursive locking.
>> 
>> I am curious, why a project like VLC would want to implement recursive locking ?
>> I think it's already available on all OS, no ?
>> It reminds me this MR on libbluray: https://code.videolan.org/videolan/libbluray/merge_requests/17
>> 
>> > 
>>> Without this, vlc_thread_self() returned NULL on non-POSIX platforms
>>> on threads not created by vlc_clone() - such as the main thread - which
>>> was not terribly helpful.
>>> 
>>> This function overlaps with vlc_thread_id(). But unlike the later, it
>>> works on all platforms. There are in particular no portable ways to
>>> implement vlc_thread_id() on POSIX Threads. include/vlc_threads.h | 36 ++++++++++++++++++++++++++++--------
>>>  src/android/thread.c  |  4 ++--
>>>  src/os2/thread.c      |  4 ++--
>>>  src/win32/thread.c    |  4 ++--
>>>  4 files changed, 34 insertions(+), 14 deletions(-)
>>> 
>>> diff --git a/include/vlc_threads.h b/include/vlc_threads.h
>>> index 647471e517..02f630ae52 100644
>>> --- a/include/vlc_threads.h
>>> +++ b/include/vlc_threads.h
>>> @@ -54,6 +54,10 @@ VLC_API void vlc_testcancel(void);
>>>  
>>>  typedef struct vlc_thread *vlc_thread_t;
>>>  # define VLC_THREAD_CANCELED NULL
>>> +
>>> +typedef HANDLE vlc_osthread_t;
>>> +#define vlc_thread_equal(a,b) ((a) == (b))
>>> +
>>>  # define LIBVLC_NEED_SLEEP
>>>  typedef struct
>>>  {
>>> @@ -99,6 +103,10 @@ static inline int vlc_poll(struct pollfd *fds, 
>>> unsigned nfds, int timeout)
>>>  
>>>  typedef struct vlc_thread *vlc_thread_t;
>>>  #define VLC_THREAD_CANCELED NULL
>>> +
>>> +typedef unsigned long vlc_osthread_t;
>>> +#define vlc_thread_equal(a,b) ((a) == (b))
>>> +
>>>  typedef struct
>>>  {
>>>      bool dynamic;
>>> @@ -164,6 +172,8 @@ static inline int vlc_poll (struct pollfd *fds, 
>>> unsigned nfds, int timeout)
>>>  
>>>  typedef struct vlc_thread *vlc_thread_t;
>>>  #define VLC_THREAD_CANCELED NULL
>>> +typedef pthread_t vlc_osthread_t;
>>> +#define vlc_thread_equal(a,b) pthread_equal(a,b)
>>>  typedef pthread_mutex_t vlc_mutex_t;
>>>  #define VLC_STATIC_MUTEX PTHREAD_MUTEX_INITIALIZER
>>>  typedef pthread_once_t  vlc_once_t;
>>> @@ -209,6 +219,8 @@ static inline int vlc_poll (struct pollfd *fds, 
>>> unsigned nfds, int timeout)
>>>  
>>>  typedef pthread_t       vlc_thread_t;
>>>  #define VLC_THREAD_CANCELED PTHREAD_CANCELED
>>> +typedef pthread_t       vlc_osthread_t;
>>> +#define vlc_thread_equal(a,b) pthread_equal(a,b)
>>>  typedef pthread_mutex_t vlc_mutex_t;
>>>  #define VLC_STATIC_MUTEX PTHREAD_MUTEX_INITIALIZER
>>>  typedef pthread_rwlock_t vlc_rwlock_t;
>>> @@ -252,6 +264,9 @@ typedef struct
>>>   */
>>>  #define VLC_THREAD_CANCELED PTHREAD_CANCELED
>>>  
>>> +typedef pthread_t vlc_osthread_t;
>>> +#define vlc_thread_equal(a,b) pthread_equal(a,b)
>>> +
>>>  /**
>>>   * Mutex.
>>>   *
>>> @@ -816,18 +831,21 @@ VLC_API void vlc_control_cancel(vlc_cleanup_t *);
>>>   * Thread handle.
>>>   *
>>>   * This function returns the thread handle of the calling thread.
>>> + * This works even if the thread was <b>not</b> created with vlc_clone().
>>> + * As a consequence, depending on the platform, this might or might not be the
>>> + * same as the @ref vlc_thread_t thread handle returned by vlc_clone().
>>>   *
>>> - * \note The exact type of the thread handle depends on the platform,
>>> - * including an integer type, a pointer type or a compound type of any size.
>>> - * If you need an integer identifier, use vlc_thread_id() instead.
>>> + * Also depending on the platform, this might be an integer type, a pointer
>>> + * type, or a compound type of any (reasonable) size. To compare two thread
>>> + * handles, use the vlc_thread_equal() macro, not a hand-coded comparison.
>>> + * Comparing the calling thread for equality with another thread is in fact
>>> + * pretty much the only purpose of this function.
>>>   *
>>> - * \note vlc_join(vlc_thread_self(), NULL) is undefined,
>>> - * as it obviously does not make any sense (it might result in a deadlock, but
>>> - * there are no warranties that it will).
>>> + * \note If you need an integer identifier, use vlc_thread_id() instead.
>>>   *
>>> - * \return the thread handle
>>> + * \return the OS run-time thread handle
>>>   */
>>> -VLC_API vlc_thread_t vlc_thread_self(void) VLC_USED;
>>> +VLC_API vlc_osthread_t vlc_thread_self(void) VLC_USED;
>>>  
>>>  /**
>>>   * Thread identifier.
>>> @@ -841,6 +859,8 @@ VLC_API vlc_thread_t vlc_thread_self(void) VLC_USED;
>>>   * There are no particular semantics to the thread ID with LibVLC.
>>>   * It is provided mainly for tracing and debugging.
>>>   *
>>> + * See also vlc_thread_self().
>>> + *
>>>   * \warning This function is not currently implemented on all supported
>>>   * platforms. Where not implemented, it returns (unsigned long)-1.
>>>   *
>>> diff --git a/src/android/thread.c b/src/android/thread.c
>>> index d3e8b25c47..da128a769c 100644
>>> --- a/src/android/thread.c
>>> +++ b/src/android/thread.c
>>> @@ -150,9 +150,9 @@ struct vlc_thread
>>>  
>>>  static thread_local struct vlc_thread *thread = NULL;
>>>  
>>> -vlc_thread_t vlc_thread_self (void)
>>> +pthread_t vlc_thread_self(void)
>>>  {
>>> -    return thread;
>>> +    return pthread_self();
>>>  }
>>>  
>>>  void vlc_threads_setup (libvlc_int_t *p_libvlc)
>>> diff --git a/src/os2/thread.c b/src/os2/thread.c
>>> index f2eb435129..150b916988 100644
>>> --- a/src/os2/thread.c
>>> +++ b/src/os2/thread.c
>>> @@ -674,9 +674,9 @@ int vlc_set_priority (vlc_thread_t th, int priority)
>>>      return VLC_SUCCESS;
>>>  }
>>>  
>>> -vlc_thread_t vlc_thread_self (void)
>>> +unsigned long vlc_thread_self(void)
>>>  {
>>> -    return vlc_threadvar_get (thread_key);
>>> +    return vlc_thread_id();
>>>  }
>>>  
>>>  unsigned long vlc_thread_id (void)
>>> diff --git a/src/win32/thread.c b/src/win32/thread.c
>>> index e0539f9322..59d55b3925 100644
>>> --- a/src/win32/thread.c
>>> +++ b/src/win32/thread.c
>>> @@ -536,9 +536,9 @@ int vlc_clone_detach (vlc_thread_t *p_handle, void 
>>> *(*entry) (void *),
>>>      return vlc_clone_attr (p_handle, true, entry, data, priority);
>>>  }
>>>  
>>> -vlc_thread_t vlc_thread_self (void)
>>> +unsigned long vlc_thread_self(void)
>>>  {
>>> -    return TlsGetValue(thread_key);
>>> +    return GetCurrentThreadId();
>>>  }
>>>  
>>>  unsigned long vlc_thread_id (void)
>>> -- 
>>> 2.25.0vlc-devel mailing list
>>> To unsubscribe or modify your subscription options:
>>> https://mailman.videolan.org/listinfo/vlc-devel
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
> 
> -- 
> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté. 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20200218/8610302c/attachment.html>


More information about the vlc-devel mailing list