diff mbox

[FFmpeg-devel] avcodec/h264_slice: don't sync default_ref[] between threads.

Message ID 20170718233440.81731-1-wtc@google.com
State Accepted
Commit 8c3b329da21a05a977b21d1a3ad3ed8ce72f997b
Headers show

Commit Message

Wan-Teh Chang July 18, 2017, 11:34 p.m. UTC
default_ref[] is unconditionally initialized in h264_initialise_ref_list()
(called from ff_h264_build_ref_list(), called from h264_slice_init()).

This fixes the following tsan warning when running fate-h264:

WARNING: ThreadSanitizer: data race (pid=31070)
  Write of size 8 at 0x7bbc000082a8 by thread T1 (mutexes: write M1628):
    #0 memcpy /work/release-test/final/llvm.src/projects/compiler-rt/lib/tsan/../sanitizer_common/sanitizer_common_interceptors.inc:655:5
(ffmpeg+0x10de9d)
    #1 h264_initialise_ref_list ffmpeg/libavcodec/h264_refs.c:214:29 (ffmpeg+0x1186b3f)
    #2 ff_h264_build_ref_list ffmpeg/libavcodec/h264_refs.c:306 (ffmpeg+0x1186b3f)
    #3 h264_slice_init ffmpeg/libavcodec/h264_slice.c:1900:11 (ffmpeg+0x1191149)
[..]
  Previous read of size 8 at 0x7bbc000082a8 by main thread (mutexes:
write M1630):
    #0 memcpy /work/release-test/final/llvm.src/projects/compiler-rt/lib/tsan/../sanitizer_common/sanitizer_common_interceptors.inc:655:5
(ffmpeg+0x10de9d)
    #1 ff_h264_update_thread_context ffmpeg/libavcodec/h264_slice.c:411:5 (ffmpeg+0x118b7dc)

Signed-off-by: Wan-Teh Chang <wtc@google.com>
---
 libavcodec/h264_slice.c | 1 -
 1 file changed, 1 deletion(-)

Comments

Wan-Teh Chang July 19, 2017, 2:09 a.m. UTC | #1
I researched the history of the line in h264_slice.c that this patch deleted.

That line was implicitly added in commit
4da2ac5c7a491b20be62ad19d77526e62aa57c69:

http://git.videolan.org/gitweb.cgi/ffmpeg.git/?p=ffmpeg.git;a=patch;h=4da2ac5c7a491b20be62ad19d77526e62aa57c69

Although commit 4da2ac5c7a491b20be62ad19d77526e62aa57c69 didn't modify
h264_slice.c, the copy_fields macro call in
ff_h264_update_thread_context() would copy the new default_ref[] field
of H264Context. The copying of default_ref[] was made explicit in
commit 98456d4d69e0fdcc328bb9e684ae776f5bc824e1, which replaced the
copy_fields macro with field-by-field copies.

http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=98456d4d69e0fdcc328bb9e684ae776f5bc824e1

Commit 4da2ac5c7a491b20be62ad19d77526e62aa57c69 makes it clear that
the default_ref[] field of H264Context is supposed to be initialized
(to values from the associated H264SliceContext) in
h264_initialise_ref_list(), so it should not be propagated from one
decoding thread to the next.

Wan-Teh Chang
Ronald S. Bultje July 27, 2017, 9:22 p.m. UTC | #2
Hi,

On Tue, Jul 18, 2017 at 7:34 PM, Wan-Teh Chang <wtc-at-google.com@ffmpeg.org
> wrote:

> default_ref[] is unconditionally initialized in h264_initialise_ref_list()
> (called from ff_h264_build_ref_list(), called from h264_slice_init()).
>
> This fixes the following tsan warning when running fate-h264:
>
> WARNING: ThreadSanitizer: data race (pid=31070)
>   Write of size 8 at 0x7bbc000082a8 by thread T1 (mutexes: write M1628):
>     #0 memcpy /work/release-test/final/llvm.src/projects/compiler-rt/lib/
> tsan/../sanitizer_common/sanitizer_common_interceptors.inc:655:5
> (ffmpeg+0x10de9d)
>     #1 h264_initialise_ref_list ffmpeg/libavcodec/h264_refs.c:214:29
> (ffmpeg+0x1186b3f)
>     #2 ff_h264_build_ref_list ffmpeg/libavcodec/h264_refs.c:306
> (ffmpeg+0x1186b3f)
>     #3 h264_slice_init ffmpeg/libavcodec/h264_slice.c:1900:11
> (ffmpeg+0x1191149)
> [..]
>   Previous read of size 8 at 0x7bbc000082a8 by main thread (mutexes:
> write M1630):
>     #0 memcpy /work/release-test/final/llvm.src/projects/compiler-rt/lib/
> tsan/../sanitizer_common/sanitizer_common_interceptors.inc:655:5
> (ffmpeg+0x10de9d)
>     #1 ff_h264_update_thread_context ffmpeg/libavcodec/h264_slice.c:411:5
> (ffmpeg+0x118b7dc)
>
> Signed-off-by: Wan-Teh Chang <wtc@google.com>
> ---
>  libavcodec/h264_slice.c | 1 -
>  1 file changed, 1 deletion(-)


Pushed.

Ronald
diff mbox

Patch

diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
index 6deb08fe6d..2fb89b189d 100644
--- a/libavcodec/h264_slice.c
+++ b/libavcodec/h264_slice.c
@@ -408,7 +408,6 @@  int ff_h264_update_thread_context(AVCodecContext *dst,
 
     memcpy(&h->poc,        &h1->poc,        sizeof(h->poc));
 
-    memcpy(h->default_ref, h1->default_ref, sizeof(h->default_ref));
     memcpy(h->short_ref,   h1->short_ref,   sizeof(h->short_ref));
     memcpy(h->long_ref,    h1->long_ref,    sizeof(h->long_ref));
     memcpy(h->delayed_pic, h1->delayed_pic, sizeof(h->delayed_pic));