On Thu, 6 Apr 2017 13:48:41 -0400
"Ronald S. Bultje" <rsbul...@gmail.com> wrote:
> The av_log() is done outside the lock, but this way the accesses to the
> field (reads and writes) are always protected by a mutex. The av_log()
> is not run inside the lock context because it may involve user callbacks
> and doing that in performance-sensitive code is probably not a good idea.
>
> This should fix occasional tsan warnings when running fate-h264, like:
>
> WARNING: ThreadSanitizer: data race (pid=10916)
> Write of size 4 at 0x7d64000174fc by main thread (mutexes: write M2313):
> #0 update_context_from_user src/libavcodec/pthread_frame.c:335
> (ffmpeg+0x000000df7b06)
> [..]
> Previous read of size 4 at 0x7d64000174fc by thread T1 (mutexes: write
> M2311):
> #0 ff_thread_await_progress src/libavcodec/pthread_frame.c:592
> (ffmpeg+0x000000df8b3e)
> ---
> libavcodec/pthread_frame.c | 20 ++++++++++++--------
> 1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c
> index c246c2f..8857bfc 100644
> --- a/libavcodec/pthread_frame.c
> +++ b/libavcodec/pthread_frame.c
> @@ -559,6 +559,7 @@ void ff_thread_report_progress(ThreadFrame *f, int n, int
> field)
> {
> PerThreadContext *p;
> atomic_int *progress = f->progress ? (atomic_int*)f->progress->data :
> NULL;
> + int debug_mv;
>
> if (!progress ||
> atomic_load_explicit(&progress[field], memory_order_relaxed) >= n)
> @@ -566,22 +567,24 @@ void ff_thread_report_progress(ThreadFrame *f, int n,
> int field)
>
> p = f->owner[field]->internal->thread_ctx;
>
> - if (f->owner[field]->debug&FF_DEBUG_THREADS)
> - av_log(f->owner[field], AV_LOG_DEBUG,
> - "%p finished %d field %d\n", progress, n, field);
> -
> pthread_mutex_lock(&p->progress_mutex);
> + debug_mv = f->owner[field]->debug&FF_DEBUG_THREADS;
>
> atomic_store_explicit(&progress[field], n, memory_order_release);
>
> pthread_cond_broadcast(&p->progress_cond);
> pthread_mutex_unlock(&p->progress_mutex);
> +
> + if (debug_mv)
> + av_log(f->owner[field], AV_LOG_DEBUG,
> + "%p finished %d field %d\n", progress, n, field);
> }
>
> void ff_thread_await_progress(ThreadFrame *f, int n, int field)
> {
> PerThreadContext *p;
> atomic_int *progress = f->progress ? (atomic_int*)f->progress->data :
> NULL;
> + int debug_mv;
>
> if (!progress ||
> atomic_load_explicit(&progress[field], memory_order_acquire) >= n)
> @@ -589,14 +592,15 @@ void ff_thread_await_progress(ThreadFrame *f, int n,
> int field)
>
> p = f->owner[field]->internal->thread_ctx;
>
> - if (f->owner[field]->debug&FF_DEBUG_THREADS)
> - av_log(f->owner[field], AV_LOG_DEBUG,
> - "thread awaiting %d field %d from %p\n", n, field, progress);
> -
> pthread_mutex_lock(&p->progress_mutex);
> + debug_mv = f->owner[field]->debug&FF_DEBUG_THREADS;
> while (atomic_load_explicit(&progress[field], memory_order_relaxed) < n)
> pthread_cond_wait(&p->progress_cond, &p->progress_mutex);
> pthread_mutex_unlock(&p->progress_mutex);
> +
> + if (debug_mv)
> + av_log(f->owner[field], AV_LOG_DEBUG,
> + "thread awaited %d field %d from %p\n", n, field, progress);
> }
>
> void ff_thread_finish_setup(AVCodecContext *avctx) {
I'm not sure what "debug_mv" stands for, and I think this is a bit
overkill. It's run only when FF_DEBUG_THREADS is set (who even sets
that, and why does access to it need to be synchronized?), so I see no
big offense in calling av_log() inside of it.
But other than that LGTM.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
