[vlc-devel] [PATCH] android: threads support

Rafaël Carré funman at videolan.org
Thu Oct 4 23:22:09 CEST 2012


Le 04/10/2012 21:42, Rémi Denis-Courmont a écrit :
> Le jeudi 4 octobre 2012 21:16:02, Rafaël Carré a écrit :
>> ---
>>  include/vlc_threads.h |   13 +
>>  src/Makefile.am       |    2 +-
>>  src/android/thread.c  |  988
>> +++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 1002
>> insertions(+), 1 deletion(-)
>>  create mode 100644 src/android/thread.c
>>
>> diff --git a/include/vlc_threads.h b/include/vlc_threads.h
>> index cc0e8b1..77ab8a5 100644
>> --- a/include/vlc_threads.h
>> +++ b/include/vlc_threads.h
>> @@ -42,6 +42,15 @@
>>
>>  #   define pthread_sigmask  sigprocmask
>>
>> +#elif defined( __ANDROID__ )      /* pthreads without pthread_cancel() */
> 
> That comment is a tad abusive. Like NaCl-newlib, Android does not have 
> pthreads. Clock selection is done differently and cancellation is not optional 
> in pthreads.

edited to add "pthreads subset"

>> +#   define LIBVLC_USE_PTHREAD 1
>> +
>> +#   include <unistd.h> /* _POSIX_SPIN_LOCKS */
>> +#   include <pthread.h>
>> +#   include <poll.h>
>> +#   include <semaphore.h>
>> +
>>  #else                                         /* pthreads (like Linux &
>> BSD) */ #   define LIBVLC_USE_PTHREAD 1
>>  #   define LIBVLC_USE_PTHREAD_CANCEL 1
>> @@ -117,7 +126,11 @@
>>  
>> **************************************************************************
>> ***/
>>
>>  #if defined (LIBVLC_USE_PTHREAD)
>> +# ifdef LIBVLC_USE_PTHREAD_CANCEL
>>  typedef pthread_t       vlc_thread_t;
>> +# else
>> +typedef struct vlc_thread *vlc_thread_t;
>> +# endif
>>  typedef pthread_mutex_t vlc_mutex_t;
>>  #define VLC_STATIC_MUTEX PTHREAD_MUTEX_INITIALIZER
>>  typedef pthread_cond_t  vlc_cond_t;
>> diff --git a/src/Makefile.am b/src/Makefile.am
>> index 9d1d87d..ab8c46c 100644
>> --- a/src/Makefile.am
>> +++ b/src/Makefile.am
>> @@ -259,9 +259,9 @@ SOURCES_libvlc_darwin = \
>>
>>  SOURCES_libvlc_android = \
>>  	android/dirs.c \
>> +	android/thread.c \
>>  	posix/filesystem.c \
>>  	posix/plugin.c \
>> -	posix/thread.c \
>>  	posix/timer.c \
>>  	posix/linux_cpu.c \
>>  	posix/linux_specific.c \
>> diff --git a/src/android/thread.c b/src/android/thread.c
>> new file mode 100644
>> index 0000000..1aea7e2
>> --- /dev/null
>> +++ b/src/android/thread.c
>> @@ -0,0 +1,988 @@
>> +/*************************************************************************
>> **** + * thread.c : android pthread back-end for LibVLC
>> +
>> **************************************************************************
>> *** + * Copyright (C) 1999-2012 VLC authors and VideoLAN
>> + *
>> + * Authors: Jean-Marc Dressler <polux at via.ecp.fr>
>> + *          Samuel Hocevar <sam at zoy.org>
>> + *          Gildas Bazin <gbazin at netcourrier.com>
>> + *          Clément Sténac
>> + *          Rémi Denis-Courmont
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU Lesser General Public License as published
>> by + * the Free Software Foundation; either version 2.1 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> License + * along with this program; if not, write to the Free Software
>> Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston MA
>> 02110-1301, USA. +
>> **************************************************************************
>> ***/ +
>> +#ifdef HAVE_CONFIG_H
>> +# include "config.h"
>> +#endif
>> +
>> +#include <vlc_common.h>
>> +#include <vlc_atomic.h>
>> +
>> +#include "libvlc.h"
>> +#include <stdarg.h>
>> +#include <signal.h>
>> +#include <errno.h>
>> +#include <time.h>
>> +#include <assert.h>
>> +
>> +#include <sys/types.h>
>> +#include <unistd.h> /* fsync() */
>> +#include <pthread.h>
>> +#include <sched.h>
>> +
>> +#include <android/log.h>
>> +#include <sys/syscall.h> /* __NR_gettid */
>> +
>> +# warning Monotonic clock not available. Expect timing issues.
> 
> I don't know nor care about older versions but Android 4.x definitely has 
> monotonic clock support, including condition variables.

OK I left a note, it'd be interesting to have runtime support for 4.x

>> +
>> +#undef assert
>> +#define assert(x) do { \
>> +    if (unlikely(!x)) { \
>> +    __android_log_print(ANDROID_LOG_ERROR, "vlc", "assert failed %s:%d:
>> %s", \ +        __FILE__, __LINE__, #x \
>> +        ); \
>> +        abort(); \
>> +    } \
>> +} while(0)
> 
> Undefining standard macros is pretty much never a good idea.

used vlc_assert

>> +
>> +static struct timespec mtime_to_ts (mtime_t date)
>> +{
>> +    lldiv_t d = lldiv (date, CLOCK_FREQ);
>> +    struct timespec ts = { d.quot, d.rem * (1000000000 / CLOCK_FREQ) };
>> +
>> +    return ts;
>> +}
>> +
>> +#ifndef NDEBUG
>> +/**
>> + * Reports a fatal error from the threading layer, for debugging purposes.
>> + */
> 
> Duplicated documentation is a bad idea. It will confuse Doxygen, and we do not 
> know which version will be used. 

Removed /** doxygen comments for now.

>> +static void
>> +vlc_thread_fatal (const char *action, int error,
>> +                  const char *function, const char *file, unsigned line)
>> +{
>> +    int canc = vlc_savecancel ();
> 
> That should not be needed since Android does not support cancellation.

removed

>> +    __android_log_print(ANDROID_LOG_ERROR, "vlc",
>> +        "LibVLC fatal error %s (%d) in thread %d ",
>> +        action, error, syscall (__NR_gettid));
>> +     fprintf (stderr, "at %s:%u in %s\n", file, line, function);
>> +     fflush (stderr); /* needed before switch to low-level I/O */
>> +     fsync (2);
>> +
>> +    /* Sometimes strerror_r() crashes too, so make sure we print an error
>> +     * message before we invoke it */
>> +    char buf[1000];
>> +    const char *msg;
>> +
>> +    switch (strerror_r (error, buf, sizeof (buf)))
>> +    {
>> +        case 0:
>> +            msg = buf;
>> +            break;
>> +        case ERANGE: /* should never happen */
>> +            msg = "unknown (too big to display)";
>> +            break;
>> +        default:
>> +            msg = "unknown (invalid error number)";
>> +            break;
>> +    }
>> +    fprintf (stderr, " Error message: %s\n", msg);
>> +    fflush (stderr);
>> +
>> +    vlc_restorecancel (canc);
>> +    abort ();
>> +}
>> +
>> +# define VLC_THREAD_ASSERT( action ) \
>> +    if (unlikely(val)) \
>> +        vlc_thread_fatal (action, val, __func__, __FILE__, __LINE__)
>> +#else
>> +# define VLC_THREAD_ASSERT( action ) ((void)val)
>> +#endif
>> +
>> +/**
>> + * Initializes a fast mutex.
>> + */
>> +void vlc_mutex_init( vlc_mutex_t *p_mutex )
>> +{
>> +    pthread_mutexattr_t attr;
>> +
>> +    if (unlikely(pthread_mutexattr_init (&attr)))
>> +        abort();
> 
> This function cannot fail on Android. The if() is redundant IMHO.
> 
> (In fact, it cannot fail on any Linux run-time that I know. But it might fail 
> on some arcane POSIX systems, hence the abort()s in src/posix/.)

removed

>> +#ifdef NDEBUG
>> +    pthread_mutexattr_settype (&attr, PTHREAD_MUTEX_DEFAULT);
>> +#else
>> +    pthread_mutexattr_settype (&attr, PTHREAD_MUTEX_ERRORCHECK);
>> +#endif
>> +    if (unlikely(pthread_mutex_init (p_mutex, &attr)))
>> +        abort();
> 
> Same here, if I am not mistaken and further.

You're right, removed.

>> +    pthread_mutexattr_destroy( &attr );
>> +}
>> +
>> +/**
>> + * Initializes a recursive mutex.
>> + * \warning This is strongly discouraged. Please use normal mutexes.
>> + */
>> +void vlc_mutex_init_recursive( vlc_mutex_t *p_mutex )
>> +{
>> +    pthread_mutexattr_t attr;
>> +
>> +    if (unlikely(pthread_mutexattr_init (&attr)))
>> +        abort();
>> +    pthread_mutexattr_settype (&attr, PTHREAD_MUTEX_RECURSIVE);
>> +    if (unlikely(pthread_mutex_init (p_mutex, &attr)))
>> +        abort();
>> +    pthread_mutexattr_destroy( &attr );
>> +}
>> +
>> +
>> +/**
>> + * Destroys a mutex. The mutex must not be locked.
>> + *
>> + * @param p_mutex mutex to destroy
>> + * @return always succeeds
>> + */
>> +void vlc_mutex_destroy (vlc_mutex_t *p_mutex)
>> +{
>> +    int val = pthread_mutex_destroy( p_mutex );
>> +    VLC_THREAD_ASSERT ("destroying mutex");
>> +}
>> +
>> +#ifndef NDEBUG
>> +# ifdef HAVE_VALGRIND_VALGRIND_H
>> +#  include <valgrind/valgrind.h>
>> +# else
>> +#  define RUNNING_ON_VALGRIND (0)
>> +# endif
> 
> Are there really versions of the SDK with valgrind.h and versions without it? 
> This seems likely redundant.

Nope, removed these headers.

>> +
>> +/**
>> + * Asserts that a mutex is locked by the calling thread.
>> + */
>> +void vlc_assert_locked (vlc_mutex_t *p_mutex)
>> +{
>> +    if (RUNNING_ON_VALGRIND > 0)
>> +        return;
>> +    assert (pthread_mutex_lock (p_mutex) == EDEADLK);
>> +}
>> +#endif
>> +
>> +/**
>> + * Acquires a mutex. If needed, waits for any other thread to release it.
>> + * Beware of deadlocks when locking multiple mutexes at the same time,
>> + * or when using mutexes from callbacks.
>> + * This function is not a cancellation-point.
>> + *
>> + * @param p_mutex mutex initialized with vlc_mutex_init() or
>> + *                vlc_mutex_init_recursive()
>> + */
>> +void vlc_mutex_lock (vlc_mutex_t *p_mutex)
>> +{
>> +    int val = pthread_mutex_lock( p_mutex );
>> +    VLC_THREAD_ASSERT ("locking mutex");
>> +}
>> +
>> +/**
>> + * Acquires a mutex if and only if it is not currently held by another
>> thread. + * This function never sleeps and can be used in delay-critical
>> code paths. + * This function is not a cancellation-point.
>> + *
>> + * <b>Beware</b>: If this function fails, then the mutex is held... by
>> another + * thread. The calling thread must deal with the error
>> appropriately. That + * typically implies postponing the operations that
>> would have required the + * mutex. If the thread cannot defer those
>> operations, then it must use + * vlc_mutex_lock(). If in doubt, use
>> vlc_mutex_lock() instead.
>> + *
>> + * @param p_mutex mutex initialized with vlc_mutex_init() or
>> + *                vlc_mutex_init_recursive()
>> + * @return 0 if the mutex could be acquired, an error code otherwise.
>> + */
>> +int vlc_mutex_trylock (vlc_mutex_t *p_mutex)
>> +{
>> +    int val = pthread_mutex_trylock( p_mutex );
>> +
>> +    if (val != EBUSY)
>> +        VLC_THREAD_ASSERT ("locking mutex");
>> +    return val;
>> +}
>> +
>> +/**
>> + * Releases a mutex (or crashes if the mutex is not locked by the caller).
>> + * @param p_mutex mutex locked with vlc_mutex_lock().
>> + */
>> +void vlc_mutex_unlock (vlc_mutex_t *p_mutex)
>> +{
>> +    int val = pthread_mutex_unlock( p_mutex );
>> +    VLC_THREAD_ASSERT ("unlocking mutex");
>> +}
>> +
>> +struct vlc_thread
>> +{
>> +    pthread_t      thread;
>> +    pthread_cond_t *cond; /// Non-null if thread waiting on cond
>> +    pthread_mutex_t *lock ; /// Non-null if thread waiting on cond
>> +    vlc_cleanup_t *cleaners;
>> +
>> +    void *(*entry)(void*);
>> +    void *data;
>> +
>> +    bool killable;
>> +    bool killed;
>> +    bool finished;
>> +};
>> +
>> +static pthread_key_t thread_key = 0;
>> +
>> +static void threads_init(void) __attribute__((__constructor__));
>> +static void threads_init(void)
>> +{
>> +    static struct vlc_thread main_thread = {
>> +        .cond     = NULL,
>> +        .lock     = NULL,
>> +        .cleaners = NULL,
>> +        .killable = false,
>> +        .killed   = false,
>> +        .finished = false,
>> +        .entry    = NULL,
>> +        .data     = NULL,
>> +    };
>> +
>> +    main_thread.thread = pthread_self();
>> +    pthread_key_create(&thread_key, NULL);
>> +    if (unlikely(pthread_setspecific(thread_key, &main_thread)))
>> +        abort();
>> +}
>> +
>> +static void threads_deinit(void) __attribute__((__destructor__));
>> +static void threads_deinit(void)
>> +{
>> +    pthread_key_delete(thread_key);
>> +}
> 
> If __thread is supported, it would be far simpler.

I think I tried that first, but it didn't work.

We should look in which version of gcc it appeared.

>> +
>> +static void *andro_Thread(void *data)
>> +{
>> +    vlc_thread_t thread = data;
>> +    if (unlikely(pthread_setspecific(thread_key, thread)))
>> +        abort();
> 
> Failure should be impossible, thus I suggest an assertion.

ok, used VLC_THREAD_ASSERT

>> +    void *ret = thread->entry(thread->data);
>> +    thread->finished = true;
>> +    return ret;
>> +}
>> +
>> +/**
>> + * Initializes a condition variable.
>> + */
>> +void vlc_cond_init (vlc_cond_t *p_condvar)
>> +{
>> +    pthread_condattr_t attr;
>> +
>> +    if (unlikely(pthread_condattr_init (&attr)))
>> +        abort ();
> 
> This is pointless. Use NULL.

done


>> +/**
>> + * Initializes a semaphore.
>> + */
>> +void vlc_sem_init (vlc_sem_t *sem, unsigned value)
> 
> This is not going to interoperate with cancellation.

What do you mean?

Some of those are mandated to be cancellation points?

I think I checked the list on POSIX spec already.

> I think the generic VLC 
> semaphore implementation should be used instead. You are in luck; I split it a 
> few weeks ago.
> 
> This comment also applies to read/write locks.



>> +{
>> +#if defined(__APPLE__)
> 
> Android on Darwin, really?

removed


>> +static bool rt_priorities = false;
>> +static int rt_offset;
>> +
>> +void vlc_threads_setup (libvlc_int_t *p_libvlc)
>> +{
>> +    static vlc_mutex_t lock = VLC_STATIC_MUTEX;
>> +    static bool initialized = false;
>> +
>> +    vlc_mutex_lock (&lock);
>> +    /* Initializes real-time priorities before any thread is created,
>> +     * just once per process. */
>> +    if (!initialized)
>> +    {
>> +#ifndef __APPLE__
> 
> ??
> 
>> +        if (var_InheritBool (p_libvlc, "rt-priority"))
>> +#endif
>> +        {
>> +            rt_offset = var_InheritInteger (p_libvlc, "rt-offset");
>> +            rt_priorities = true;
> 
> Are RT priorities even available to applications on Android?

Nope, whole function removed.

>> +        }
>> +        initialized = true;
>> +    }
>> +    vlc_mutex_unlock (&lock);
>> +}
>> +
>> +
>> +static int vlc_clone_attr (vlc_thread_t *th, pthread_attr_t *attr,
>> +                           void *(*entry) (void *), void *data, int
>> priority) +{
>> +    int ret;
>> +
>> +    /* Block the signals that signals interface plugin handles.
>> +     * If the LibVLC caller wants to handle some signals by itself, it
>> should +     * block these before whenever invoking LibVLC. And it must
>> obviously not +     * start the VLC signals interface plugin.
>> +     *
>> +     * LibVLC will normally ignore any interruption caused by an
>> asynchronous +     * signal during a system call. But there may well be
>> some buggy cases +     * where it fails to handle EINTR (bug reports
>> welcome). Some underlying +     * libraries might also not handle EINTR
>> properly.
>> +     */
> 
> This comment is stale (there is no signal plugin). That is one reason why I do 
> not like to duplicate documentation and comments.

I removed the comment about signal plugin.
Any other suggestion? should I remove the whole comment?

>> +    sigset_t oldset;
>> +    {
>> +        sigset_t set;
>> +        sigemptyset (&set);
>> +        sigdelset (&set, SIGHUP);
>> +        sigaddset (&set, SIGINT);
>> +        sigaddset (&set, SIGQUIT);
>> +        sigaddset (&set, SIGTERM);
>> +
>> +        sigaddset (&set, SIGPIPE); /* We don't want this one, really! */
>> +        pthread_sigmask (SIG_BLOCK, &set, &oldset);
>> +    }
>> +
>> +    (void) priority;
>> +
>> +    /* The thread stack size.
>> +     * The lower the value, the less address space per thread, the highest
>> +     * maximum simultaneous threads per process. Too low values will cause
>> +     * stack overflows and weird crashes. Set with caution. Also keep in
>> mind +     * that 64-bits platforms consume more stack than 32-bits one. +
>>     *
>> +     * Thanks to on-demand paging, thread stack size only affects address
>> space +     * consumption. In terms of memory, threads only use what they
>> need +     * (rounded up to the page boundary).
>> +     *
>> +     * For example, on Linux i386, the default is 2 mega-bytes, which
>> supports +     * about 320 threads per processes. */
> 
> Not very relevant comment.
> 
>> +#define VLC_STACKSIZE (128 * sizeof (void *) * 1024)
> 
> It should probably be smaller on Android. There is no gettext, is there?

Nope, do you suggest 64, or even keeping OS default (1024 * 1024)

>> +
>> +#ifdef VLC_STACKSIZE
>> +    ret = pthread_attr_setstacksize (attr, VLC_STACKSIZE);
>> +    assert (ret == 0); /* fails iif VLC_STACKSIZE is invalid */
>> +#endif
>> +
>> +    vlc_thread_t thread = malloc (sizeof (*thread));
>> +    if (unlikely(thread == NULL))
>> +        return ENOMEM;
>> +
>> +    thread->killable = true;
>> +    thread->killed = false;
>> +    thread->finished = false,
>> +    thread->cond = NULL;
>> +    thread->lock = NULL;
>> +    thread->cleaners = NULL;
>> +    thread->entry = entry;
>> +    thread->data = data;
>> +
>> +    *th = thread;
>> +    ret = pthread_create (&thread->thread, attr, andro_Thread, thread);
>> +
>> +    pthread_sigmask (SIG_SETMASK, &oldset, NULL);
>> +    pthread_attr_destroy (attr);
>> +    return ret;
>> +}
>> +
>> +/**
>> + * Creates and starts new thread.
>> + *
>> + * The thread must be <i>joined</i> with vlc_join() to reclaim resources
>> + * when it is not needed anymore.
>> + *
>> + * @param th [OUT] pointer to write the handle of the created thread to
>> + *                 (mandatory, must be non-NULL)
>> + * @param entry entry point for the thread
>> + * @param data data parameter given to the entry point
>> + * @param priority thread priority value
>> + * @return 0 on success, a standard error code on error.
>> + */
>> +int vlc_clone (vlc_thread_t *th, void *(*entry) (void *), void *data,
>> +               int priority)
>> +{
>> +    pthread_attr_t attr;
>> +
>> +    pthread_attr_init (&attr);
>> +    return vlc_clone_attr (th, &attr, entry, data, priority);
>> +}
>> +
>> +/**
>> + * Waits for a thread to complete (if needed), then destroys it.
>> + * This is a cancellation point; in case of cancellation, the join does
>> _not_ + * occur.
>> + * @warning
>> + * A thread cannot join itself (normally VLC will abort if this is
>> attempted). + * Also, a detached thread <b>cannot</b> be joined.
>> + *
>> + * @param handle thread handle
>> + * @param p_result [OUT] pointer to write the thread return value or NULL
>> + */
>> +void vlc_join (vlc_thread_t handle, void **result)
>> +{
>> +    do {
>> +        vlc_testcancel();
>> +        msleep(CLOCK_FREQ / 100);
>> +    } while (!handle->finished);
> 
> Better than nothing, but it will waste precious µWh.

Yes ...

>> +
>> +    int val = pthread_join (handle->thread, result);
>> +    VLC_THREAD_ASSERT ("joining thread");
>> +    free(handle);
>> +}
>> +
>> +/**
>> + * Creates and starts new detached thread.
>> + * A detached thread cannot be joined. Its resources will be automatically
>> + * released whenever the thread exits (in particular, its call stack will
>> be + * reclaimed).
>> + *
>> + * Detached thread are particularly useful when some work needs to be done
>> + * asynchronously, that is likely to be completed much earlier than the
>> thread + * can practically be joined. In this case, thread detach can
>> spare memory. + *
>> + * A detached thread may be cancelled, so as to expedite its termination.
>> + * Be extremely careful if you do this: while a normal joinable thread can
>> + * safely be cancelled after it has already exited, cancelling an already
>> + * exited detached thread is undefined: The thread handle would is
>> destroyed + * immediately when the detached thread exits. So you need to
>> ensure that the + * detached thread is still running before cancellation
>> is attempted. + *
>> + * @warning Care must be taken that any resources used by the detached
>> thread + * remains valid until the thread completes.
>> + *
>> + * @note A detached thread must eventually exit just like another other
>> + * thread. In practice, LibVLC will wait for detached threads to exit
>> before + * it unloads the plugins.
>> + *
>> + * @param th [OUT] pointer to hold the thread handle, or NULL
>> + * @param entry entry point for the thread
>> + * @param data data parameter given to the entry point
>> + * @param priority thread priority value
>> + * @return 0 on success, a standard error code on error.
>> + */
>> +int vlc_clone_detach (vlc_thread_t *th, void *(*entry) (void *), void
>> *data, +                      int priority)
>> +{
>> +    vlc_thread_t dummy;
>> +    pthread_attr_t attr;
>> +
>> +    if (th == NULL)
>> +        th = &dummy;
>> +
>> +    pthread_attr_init (&attr);
>> +    pthread_attr_setdetachstate (&attr, PTHREAD_CREATE_DETACHED);
>> +    return vlc_clone_attr (th, &attr, entry, data, priority);
>> +}
>> +
>> +int vlc_set_priority (vlc_thread_t th, int priority)
>> +{
>> +    (void) th; (void) priority;
>> +    return VLC_SUCCESS;
>> +}
>> +
>> +/**
>> + * Marks a thread as cancelled. Next time the target thread reaches a
>> + * cancellation point (while not having disabled cancellation), it will
>> + * run its cancellation cleanup handler, the thread variable destructors,
>> and + * terminate. vlc_join() must be used afterward regardless of a
>> thread being + * cancelled or not.
>> + */
>> +void vlc_cancel (vlc_thread_t thread_id)
>> +{
>> +    bool self = thread_id == pthread_getspecific(thread_key);
>> +
>> +    thread_id->killed = true;
>> +    if (!thread_id->killable)
>> +        return;
>> +
>> +    vlc_mutex_t *lock = thread_id->lock;
> 
> I fail to grasp where the memory barriers are.

Hm i am not sure how barriers work.

On android ldr/str is one instruction so it should be atomic?

>> +
>> +    if (lock) {
>> +        if (!self)
>> +            vlc_mutex_lock(lock);
> 
> I don't see what warrants that the mutex still exists at this point.

vlc_join(thread_id) has not been called yet.

>> +        if (thread_id->cond)
>> +            pthread_cond_broadcast(thread_id->cond);
>> +        if (!self)
>> +            vlc_mutex_unlock(lock);
>> +    }
>> +
>> +    if (self)
>> +        vlc_testcancel();
> 
> No, vlc_cancel() is not a cancellation point.

In this case, the thread is cancelling itself (can that happen?).

>> +}
>> +
>> +/**
>> + * Save the current cancellation state (enabled or disabled), then disable
>> + * cancellation for the calling thread.
>> + * This function must be called before entering a piece of code that is
>> not + * cancellation-safe, unless it can be proven that the calling thread
>> will not + * be cancelled.
>> + * @return Previous cancellation state (opaque value for
>> vlc_restorecancel()). + */
>> +int vlc_savecancel (void)
>> +{
>> +    vlc_thread_t thread = pthread_getspecific(thread_key);
>> +    if (!thread) /* not created by VLC, can't be cancelled */
>> +        return true;
>> +
>> +    int oldstate = thread->killable;
>> +    thread->killable = false;
>> +    return oldstate;
>> +}
>> +
>> +/**
>> + * Restore the cancellation state for the calling thread.
>> + * @param state previous state as returned by vlc_savecancel().
>> + * @return Nothing, always succeeds.
>> + */
>> +void vlc_restorecancel (int state)
>> +{
>> +    vlc_thread_t thread = pthread_getspecific(thread_key);
>> +    if (!thread) /* not created by VLC, can't be cancelled */
>> +        return;
>> +
>> +    int val = thread->killable != false;
>> +    VLC_THREAD_ASSERT("restoring cancellation while not disabled");
>> +    thread->killable = state;
>> +}
>> +
>> +/**
>> + * Issues an explicit deferred cancellation point.
>> + * This has no effect if thread cancellation is disabled.
>> + * This can be called when there is a rather slow non-sleeping operation.
>> + * This is also used to force a cancellation point in a function that
>> would + * otherwise "not always" be a one (block_FifoGet() is an example).
>> + */
>> +void vlc_testcancel (void)
>> +{
>> +    vlc_thread_t thread = pthread_getspecific(thread_key);
>> +    if (!thread) /* not created by VLC, can't be cancelled */
>> +        return;
>> +    if (!thread->killable || !thread->killed)
>> +        return;
>> +
>> +    for (vlc_cleanup_t *p = thread->cleaners; p != NULL; p = p->next)
>> +        p->proc (p->data);
> 
> Why don't you just call pthread_exit() and let bionic run the handlers?

in vlc_thread.h, use of pthread cleanup is under
#if defined (LIBVLC_USE_PTHREAD_CANCEL)

I guess it should be LIBVLC_USE_PTHREAD and we can remove the above
code, I'll try this way.

>> +
>> +    thread->finished = true;
>> +    pthread_exit(NULL);
>> +}
>> +
>> +void vlc_control_cancel (int cmd, ...)
>> +{
>> +    vlc_thread_t thread = pthread_getspecific(thread_key);
>> +    if (!thread) /* not created by VLC, can't be cancelled */
>> +        return;
>> +    /* NOTE: This function only modifies thread-specific data, so there is
>> no +     * need to lock anything. */
>> +    va_list ap;
>> +
>> +    va_start (ap, cmd);
>> +    switch (cmd)
>> +    {
>> +        case VLC_CLEANUP_PUSH:
>> +        {
>> +            /* cleaner is a pointer to the caller stack, no need to
>> allocate +             * and copy anything. As a nice side effect, this
>> cannot fail. */ +            vlc_cleanup_t *cleaner = va_arg (ap,
>> vlc_cleanup_t *); +            cleaner->next = thread->cleaners;
>> +            thread->cleaners = cleaner;
>> +            break;
>> +        }
>> +
>> +        case VLC_CLEANUP_POP:
>> +        {
>> +            thread->cleaners = thread->cleaners->next;
>> +            break;
>> +        }
>> +    }
>> +    va_end (ap);
>> +}
>> +
>> +/**
>> + * Precision monotonic clock.
>> + *
>> + * In principles, the clock has a precision of 1 MHz. But the actual
>> resolution + * may be much lower, especially when it comes to sleeping
>> with mwait() or + * msleep(). Most general-purpose operating systems
>> provide a resolution of + * only 100 to 1000 Hz.
>> + *
>> + * @warning The origin date (time value "zero") is not specified. It is
>> + * typically the time the kernel started, but this is platform-dependent.
>> + * If you need wall clock time, use gettimeofday() instead.
>> + *
>> + * @return a timestamp in microseconds.
>> + */
>> +mtime_t mdate (void)
>> +{
>> +    struct timespec ts;
>> +
>> +    if (unlikely(clock_gettime (CLOCK_REALTIME, &ts) != 0))
>> +        abort ();
>> +
>> +    return (INT64_C(1000000) * ts.tv_sec) + (ts.tv_nsec / 1000);
>> +}
>> +
>> +#undef mwait
>> +/**
>> + * Waits until a deadline (possibly later due to OS scheduling).
>> + * @param deadline timestamp to wait for (see mdate())
>> + */
>> +void mwait (mtime_t deadline)
>> +{
>> +    deadline -= mdate ();
>> +    if (deadline > 0)
>> +        msleep (deadline);
>> +}
>> +
>> +#undef msleep
>> +/**
>> + * Waits for an interval of time.
>> + * @param delay how long to wait (in microseconds)
>> + */
>> +void msleep (mtime_t delay)
>> +{
>> +    struct timespec ts = mtime_to_ts (delay);
>> +
>> +    vlc_testcancel();
>> +    for (;;) {
>> +        struct timespec t = { 0, 10 * 1000 * 1000 };
>> +        if (ts.tv_sec <= 0 && t.tv_nsec > ts.tv_nsec)
>> +            t.tv_nsec = ts.tv_nsec;
>> +        while (nanosleep (&t, &t) == -1) {
>> +            vlc_testcancel();
>> +            assert (errno == EINTR);
>> +        }
>> +
>> +        ts.tv_nsec -= 10 * 1000 * 1000;
>> +        if (ts.tv_nsec < 0) {
>> +            if (--ts.tv_sec < 0)
>> +                return;
>> +            ts.tv_nsec += 1000 * 1000 * 1000;
>> +        }
>> +    }
>> +}
>> +
>> +/**
>> + * Count CPUs.
>> + * @return number of available (logical) CPUs.
>> + */
>> +unsigned vlc_GetCPUCount(void)
>> +{
>> +    return sysconf(_SC_NPROCESSORS_CONF);
>> +}
> 

Updated patch attached.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-android-threads-support.patch
Type: text/x-patch
Size: 28806 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20121004/f415ba84/attachment.bin>


More information about the vlc-devel mailing list