[FFmpeg-devel] [PATCH] compat/w32pthreads: change pthread_t into pointer to malloced struct

[Martin Storsjö]

On Thu, 12 Dec 2024, Anton Khirnov wrote:

> Quoting Martin Storsjö (2024-12-12 18:40:27)
>> On Thu, 12 Dec 2024, Anton Khirnov wrote:
>> 
>> > pthread_t is currently defined as a struct, which gets placed into
>> > caller's memory and filled by pthread_create() (which accepts a
>> > pthread_t*).
>> >
>> > The problem with this approach is that pthread_join() accepts pthread_t
>> > itself rather than a pointer to it, so it gets a _copy_ of this
>> > structure. This causes non-deterministic failures of pthread_join() to
>> > produce the correct return value - depending on whether the thread
>> > already finished before pthread_join() is called (and thus the copy
>> > contains the correct value), or not (then it contains 0).
>> >
>> > Change the definition of pthread_t into a pointer to a struct, that gets
>> > malloced by pthread_create() and freed by pthread_join().
>> >
>> > Fixes random failures of fate-ffmpeg-error-rate-fail on Windows.
>> > ---
>> > compat/w32pthreads.h | 39 +++++++++++++++++++++++++++------------
>> > 1 file changed, 27 insertions(+), 12 deletions(-)
>> 
>> As you've pinpointed the commit that made this issue appear much more 
>> frequently, it'd be nice to include that info.
>
> Sure.
>
>> > diff --git a/compat/w32pthreads.h b/compat/w32pthreads.h
>> > index 2ff9735227..fd6428e29f 100644
>> > --- a/compat/w32pthreads.h
>> > +++ b/compat/w32pthreads.h
>> > @@ -50,7 +50,7 @@ typedef struct pthread_t {
>> >     void *(*func)(void* arg);
>> >     void *arg;
>> >     void *ret;
>> > -} pthread_t;
>> > +} *pthread_t;
>> >
>> > /* use light weight mutex/condition variable API for Windows Vista and later */
>> > typedef SRWLOCK pthread_mutex_t;
>> > @@ -74,7 +74,7 @@ typedef CONDITION_VARIABLE pthread_cond_t;
>> > static av_unused THREADFUNC_RETTYPE
>> > __stdcall attribute_align_arg win32thread_worker(void *arg)
>> > {
>> > -    pthread_t *h = (pthread_t*)arg;
>> > +    pthread_t h = (pthread_t)arg;
>> >     h->ret = h->func(h->arg);
>> >     return 0;
>> > }
>> > @@ -82,21 +82,35 @@ __stdcall attribute_align_arg win32thread_worker(void *arg)
>> > static av_unused int pthread_create(pthread_t *thread, const void *unused_attr,
>> >                                     void *(*start_routine)(void*), void *arg)
>> > {
>> > -    thread->func   = start_routine;
>> > -    thread->arg    = arg;
>> > +    pthread_t ret;
>> > +
>> > +    ret = av_mallocz(sizeof(*ret));
>> > +    if (!ret)
>> > +        return EAGAIN;
>> 
>> ENOMEM?
>
> POSIX specifies EAGAIN for "Insufficient resources to create another
> thread.", so...

That's weird - but fair enough then.

It could btw be useful trivia to link to 
https://code.videolan.org/videolan/x264/-/commit/23829dd2b2c909855481f46cc884b3c25d92c2d7 
as well, and say that this commit fixes the same issue, but we preferred 
to fix it this way, as this version doesn't break if the pthread_t is 
moved/copied.

// Martin