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

[Wan-Teh Chang]

Hi Ronald,

On Wed, Jul 5, 2017 at 3:31 PM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> Hi Wan-Teh,
>
> On Wed, Jul 5, 2017 at 6:12 PM, Wan-Teh Chang <wtc at google.com> wrote:
>>
>> Thank you for all the tsan warning fixes. In the meantime, it would be
>> good to revert 2e664b9c1e73c80aab91070c1eb7676f04bdd12d to avoid
>> confusion.
>
>
> Why? I believe it fixes a subset of the issue.

Could you explain why it fixes a subset of the issue?

The data race that tsan warned about was between a write and a read:

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)

But the write is not protected by the mutex that
2e664b9c1e73c80aab91070c1eb7676f04bdd12d uses to protect the reads.

I can't reproduce the tsan warning by running fate-h264 (with and
without 2e664b9c1e73c80aab91070c1eb7676f04bdd12d), but I can still
reproduce the tsan warning by running a test program in my project
with a recent (one or two weeks old) snapshot of ffmpeg:

WARNING: ThreadSanitizer: data race (pid=86821)
  Read of size 4 at 0x7b640003b8fc by thread T19 (mutexes: write
M525086672791228232, write M524523722837806536):
    #0 ff_thread_await_progress
[...]/libavcodec/pthread_frame.c:591:26
(5ab42bb1a6f4b068d7863dabe9b2bacc+0xe749a1)
    #1 await_references [...]/libavcodec/h264_mb.c
(5ab42bb1a6f4b068d7863dabe9b2bacc+0x9530c4)
    #2 hl_motion_420_simple_8 [...]/libavcodec/h264_mc_template.c:80:9
(5ab42bb1a6f4b068d7863dabe9b2bacc+0x95106b)
    #3 hl_decode_mb_simple_8 [...]/libavcodec/h264_mb_template.c:180
(5ab42bb1a6f4b068d7863dabe9b2bacc+0x95106b)
    #4 ff_h264_hl_decode_mb [...]/libavcodec/h264_mb.c:816:9
(5ab42bb1a6f4b068d7863dabe9b2bacc+0x940953)
    #5 decode_slice [...]/libavcodec/h264_slice.c:2572:17
(5ab42bb1a6f4b068d7863dabe9b2bacc+0x982ce7)
    #6 ff_h264_execute_decode_slices
[...]/libavcodec/h264_slice.c:2747:15
(5ab42bb1a6f4b068d7863dabe9b2bacc+0x981fb9)
    #7 decode_nal_units [...]/libavcodec/h264dec.c:718:27
(5ab42bb1a6f4b068d7863dabe9b2bacc+0x99257b)
    #8 h264_decode_frame [...]/libavcodec/h264dec.c:1008
(5ab42bb1a6f4b068d7863dabe9b2bacc+0x99257b)
    #9 frame_worker_thread [...]/libavcodec/pthread_frame.c:199:21
(5ab42bb1a6f4b068d7863dabe9b2bacc+0xe75b4c)

  Previous write of size 4 at 0x7b640003b8fc by main thread (mutexes:
write M524242247861095840):
    #0 update_context_from_user
[...]/libavcodec/pthread_frame.c:335:19
(5ab42bb1a6f4b068d7863dabe9b2bacc+0xe73859)
    #1 submit_packet [...]/libavcodec/pthread_frame.c:396
(5ab42bb1a6f4b068d7863dabe9b2bacc+0xe73859)
    #2 ff_thread_decode_frame [...]/libavcodec/pthread_frame.c:490
(5ab42bb1a6f4b068d7863dabe9b2bacc+0xe73859)
    #3 decode_simple_internal [...]/libavcodec/decode.c:415:15
(5ab42bb1a6f4b068d7863dabe9b2bacc+0x7d49b9)
    #4 decode_simple_receive_frame [...]/libavcodec/decode.c:620
(5ab42bb1a6f4b068d7863dabe9b2bacc+0x7d49b9)
    #5 decode_receive_frame_internal [...]/libavcodec/decode.c:638
(5ab42bb1a6f4b068d7863dabe9b2bacc+0x7d49b9)
    #6 avcodec_send_packet [...]/libavcodec/decode.c:678:15
(5ab42bb1a6f4b068d7863dabe9b2bacc+0x7d3ea7)
    #7 compat_decode [...]/libavcodec/decode.c:853:15
(5ab42bb1a6f4b068d7863dabe9b2bacc+0x7d627d)
    #8 avcodec_decode_video2 [...]/libavcodec/decode.c:914:12
(5ab42bb1a6f4b068d7863dabe9b2bacc+0x7d617e)
    ...

The tsan warning shows the read and the write are still protected by
different mutexes. The read is now protected by two mutexes because of
2e664b9c1e73c80aab91070c1eb7676f04bdd12d.

I hope this convinces you that
2e664b9c1e73c80aab91070c1eb7676f04bdd12d is not working as intended
and should be reverted to avoid confusion.

Thanks,
Wan-Teh Chang