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

[Michael Niedermayer]

On Thu, Sep 13, 2012 at 01:35:08PM -0700, Ben Jackson wrote:
> On Thu, Sep 13, 2012 at 09:41:37PM +0200, Michael Niedermayer wrote:
[...]
> > > @@ -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
> 
> The job_count is tied up with stopping the workers.  The mechanism that
> controls the workers is strange and I didn't want to mess with it.  I
> found it interesting that thread_count (not available through
> ThreadContext *c but necessary to test for completeness) was already
> passed in -- I wondered if someone had deleted code like mine in the
> past not understanding the necessity.

well, the job count can change for each avcodec_thread_execute()
i see now its not really helping in simplifying things but it feels
more correct to set it to 0 when there are no jobs to be done


> 
> > > @@ -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
> 
> Really?  Remember it only matters that it changes the value from execute
> to execute.  I could just as easily have written '^= 1' instead of '++'.

^=1 or unsigned or anything equivalent is fine


> 
> I was just trying to make a minimal change to fix the problem.  To me
> these comments treat the patch more harshly than the original code.  If
> you'll only be happy with a rewrite of the slice worker code please say
> so directly...

no, the original code is broken and the patch fixes it. I just hoped
that a simpler condition would work but it seems thats not the case

so the patch with the overflow fixed is ok

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

Avoid a single point of failure, be that a person or equipment.
-------------- 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/20120914/f2699948/attachment.asc>