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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
