Hi Ronald, On Thu, Feb 25, 2016 at 12:00 PM, Ronald S. Bultje <rsbul...@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 } _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
