[FFmpeg-devel] [PATCH] Move the |die| member of FrameThreadContext to PerThreadContext.

[Wan-Teh Chang]

Hi Ronald,

On Thu, Feb 25, 2016 at 12:00 PM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
>
> But does it fix an actual bug? (Is there a sample this fixes?)

I assume you agree that reading and writing the non-atomic int |die|
flag without mutex protection is a data race. Code inspection verifies
libavcodec/pthread_frame.c reads and writes |die| without mutex
protection.

To fix the data race, we can either protect |die| with a mutex, or
declare |die| as an atomic int.

> If anything, you can use atomic ints instead of regular ints if we don't
> already [1].
>
[...]
> [1] https://ffmpeg.org/doxygen/trunk/atomic_8h_source.html

Thanks for the link. avpriv_atomic_int_get and avpriv_atomic_int_set
perform the load and store with sequential consistency, which requires
a full memory barrier and is slower than simply relying on the
existing per-thread mutex. The drawback of my patch is that it uses
more memory. I think that's the right trade-off, but I would be happy
to use an atomic int. Please let me know what you prefer.

Thanks,
Wan-Teh Chang

PS: Here is the relevant part of the ThreadSanitizer report. I'm using
an old version of ffmpeg (the code is in libavcodec/pthread.c in that
version):

WARNING: ThreadSanitizer: data race (pid=959745)
  Write of size 4 at 0x7d1800055a24 by main thread (mutexes: write M18754):
    #0 frame_thread_free ffmpeg/libavcodec/pthread.c:761:15
(c9c6e60608e58c16d6d8dc75627a71ed+0x000000985b5a)
    #1 ff_thread_free ffmpeg/libavcodec/pthread.c:1103:9
(c9c6e60608e58c16d6d8dc75627a71ed+0x0000009859ea)
    #2 avcodec_close ffmpeg/libavcodec/utils.c:1825:13
(c9c6e60608e58c16d6d8dc75627a71ed+0x000000a8f7a6)
    ...

  Previous read of size 4 at 0x7d1800055a24 by thread T11 (mutexes:
write M18771):
    #0 frame_worker_thread ffmpeg/libavcodec/pthread.c:378:60
(c9c6e60608e58c16d6d8dc75627a71ed+0x00000098656b)

Note that although some mutexes were acquired when the write and read
occurred, they were not the same mutex. Here are the relevant code
snippets:

 361 /**
 362  * Codec worker thread.
 363  *
 364  * Automatically calls ff_thread_finish_setup() if the codec does
 365  * not provide an update_thread_context method, or if the codec returns
 366  * before calling it.
 367  */
 368 static attribute_align_arg void *frame_worker_thread(void *arg)
 369 {
 370     PerThreadContext *p = arg;
 371     FrameThreadContext *fctx = p->parent;
 372     AVCodecContext *avctx = p->avctx;
 373     const AVCodec *codec = avctx->codec;
 374
 375     pthread_mutex_lock(&p->mutex);
 376     while (1) {
 377         int i;
 378             while (p->state == STATE_INPUT_READY && !fctx->die)
 379                 pthread_cond_wait(&p->input_cond, &p->mutex);
 380
 381         if (fctx->die) break;
 382
...

 750 static void frame_thread_free(AVCodecContext *avctx, int thread_count)
 751 {
 752     FrameThreadContext *fctx = avctx->thread_opaque;
 753     const AVCodec *codec = avctx->codec;
 754     int i;
 755
 756     park_frame_worker_threads(fctx, thread_count);
 757
 758     if (fctx->prev_thread && fctx->prev_thread != fctx->threads)
 759         update_context_from_thread(fctx->threads->avctx,
fctx->prev_thread-     >avctx, 0);
 760
 761     fctx->die = 1;
 762
 763     for (i = 0; i < thread_count; i++) {
 764         PerThreadContext *p = &fctx->threads[i];
 765
 766         pthread_mutex_lock(&p->mutex);
 767         pthread_cond_signal(&p->input_cond);
 768         pthread_mutex_unlock(&p->mutex);
 769
 770         if (p->thread_init)
 771             pthread_join(p->thread, NULL);
 772         p->thread_init=0;
 773
 774         if (codec->close)
 775             codec->close(p->avctx);
 776
 777         avctx->codec = NULL;
 778
 779         release_delayed_buffers(p);
 780     }