[FFmpeg-devel] [PATCH] pthread: Avoid crashes/odd behavior caused by spurious wakeups

[Michael Niedermayer]

On Wed, Sep 12, 2012 at 01:31:35AM -0700, Ben Jackson wrote:
> pthread_wait_cond can wake up for no reason (Wikipedia: Spurious_wakeup).
> The FF_THREAD_SLICE thread mechanism could spontaneously execute jobs or
> allow the caller of avctx->execute to return before all jobs were complete.
> This adds tests to both cases to ensure the wakeup is real.
> 
> Signed-off-by: Ben Jackson <ben at ben.com>
> ---
>  libavcodec/pthread.c |   10 ++++++++--
>  1 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/pthread.c b/libavcodec/pthread.c
> index 5123d1d..2d7e83c 100644
> --- a/libavcodec/pthread.c
> +++ b/libavcodec/pthread.c
> @@ -80,6 +80,7 @@ typedef struct ThreadContext {
>      pthread_cond_t current_job_cond;
>      pthread_mutex_t current_job_lock;
>      int current_job;
> +    int current_execute;
>      int done;
>  } ThreadContext;
>  
> @@ -204,6 +205,7 @@ static void* attribute_align_arg worker(void *v)
>      AVCodecContext *avctx = v;
>      ThreadContext *c = avctx->thread_opaque;
>      int our_job = c->job_count;
> +    int last_execute = 0;
>      int thread_count = avctx->thread_count;
>      int self_id;
>

> @@ -214,7 +216,9 @@ static void* attribute_align_arg worker(void *v)
>              if (c->current_job == thread_count + c->job_count)
>                  pthread_cond_signal(&c->last_job_cond);
>  
> -            pthread_cond_wait(&c->current_job_cond, &c->current_job_lock);
> +            while (last_execute == c->current_execute && !c->done)
> +                pthread_cond_wait(&c->current_job_cond, &c->current_job_lock);
> +            last_execute = c->current_execute;
>              our_job = self_id;
>  
>              if (c->done) {

this will fail in 1 of 1<<32 cases due to current_execute being 0



> @@ -234,7 +238,8 @@ static void* attribute_align_arg worker(void *v)
>  
>  static av_always_inline void avcodec_thread_park_workers(ThreadContext *c, int thread_count)
>  {
> -    pthread_cond_wait(&c->last_job_cond, &c->current_job_lock);
> +    while (c->current_job != thread_count + c->job_count)
> +        pthread_cond_wait(&c->last_job_cond, &c->current_job_lock);
>      pthread_mutex_unlock(&c->current_job_lock);
>  }

I somehow think this should reset job_count to 0
and then job_count can be used in the other test avoiding the
current_execute issues


>
> @@ -283,6 +288,7 @@ static int avcodec_thread_execute(AVCodecContext *avctx, action_func* func, void
>          c->rets = &dummy_ret;
>          c->rets_count = 1;
>      }
> +    c->current_execute++;

signed integer overflow and undefined behavior


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have often repented speaking, but never of holding my tongue.
-- Xenocrates
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120913/2bbade00/attachment.asc>