[FFmpeg-devel] [PATCH] pthread_frame: make accesses to debug field be protected by owner lock.

[Wan-Teh Chang]

Hi Ronald,

A variant of this patch is committed as
2e664b9c1e73c80aab91070c1eb7676f04bdd12d:

http://ffmpeg.org/pipermail/ffmpeg-cvslog/2017-April/106577.html

I believe it does not fix the tsan warning.

On Thu, Apr 6, 2017 at 10:48 AM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> ..., but this way the accesses to the
> field (reads and writes) are always protected by a mutex.

The patch protects the reads of the |debug| field with
p->progress_mutex, where |p| points to a PerThreadContext. But the
write of the |debug| field in update_context_from_user() is not
protected by progress_mutex of any PerThreadContext.

The tsan warning this patch tried to fix is:

> 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)

The code in libavcodec/pthread_frame.c related to the write of the
|debug| field shown in the tsan warning is the following:

 326 static int update_context_from_user(AVCodecContext *dst,
AVCodecContext *sr     c)
 327 {
 ,,,
 333
 334     dst->opaque   = src->opaque;
 335     dst->debug    = src->debug;
 336     dst->debug_mv = src->debug_mv;

...

 383 static int submit_packet(PerThreadContext *p, AVCodecContext *user_avctx,
 384                          AVPacket *avpkt)
 385 {
 ...
 393
 394     pthread_mutex_lock(&p->mutex);
 395
 396     ret = update_context_from_user(p->avctx, user_avctx);

...

 472 int ff_thread_decode_frame(AVCodecContext *avctx,
 473                            AVFrame *picture, int *got_picture_ptr,
 474                            AVPacket *avpkt)
 475 {
 ...
 480
 481     /* release the async lock, permitting blocked hwaccel threads to
 482      * go forward while we are in this function */
 483     async_unlock(fctx);
 484
 485     /*
 486      * Submit a packet to the next decoding thread.
 487      */
 488
 489     p = &fctx->threads[fctx->next_decoding];
 490     err = submit_packet(p, avctx, avpkt);

The code snippets show the write of dst->debug is only protected by
p->mutex (acquired at line 394) for some |p|. progress_mutex (of any
AVCodecContext) is not involved at all.

Wan-Teh Chang