[FFmpeg-devel] tsan warning about a data race in libavcodec/h264_direct.c

[Wan-Teh Chang]

Hi Ronald,

On Wed, Jul 19, 2017 at 11:50 AM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> Hi Wan-Teh,
>
> On Wed, Jul 19, 2017 at 2:31 PM, Wan-Teh Chang <wtc-at-google.com at ffmpeg.org
>> wrote:
>
>> In libavcodec/h264_direct.c, there is already an
>> await_reference_mb_row() call before the read of
>> sl->ref_list[1][0].parent->mb_type[mb_xy] at line 505:
>>
>> 487 static void pred_temp_direct_motion(const H264Context *const h,
>> H264SliceCon    text *sl,
>> 488                                     int *mb_type)
>> 489 {
>> ...
>> 501
>> 502     await_reference_mb_row(h, &sl->ref_list[1][0],
>> 503                            sl->mb_y + !!IS_INTERLACED(*mb_type));
>> 504
>> 505     if (IS_INTERLACED(sl->ref_list[1][0].parent->mb_type[mb_xy]))
>> { // AFL/A    FR/FR/FL -> AFL/FL
>>
>> This seems like the wait you suggested, but I guess it is not?
>
> Yes, but clearly it's not doing the correct thing. :-). The ""fun"" in
> these type of issues is to figure out why not. ;-).

I debugged this for fun for three hours last night. I studied other
ff_thread_await_progress() calls, especially the ones in the h264
decoder. But I couldn't figure out the meaning of the third argument
(int field) of ff_thread_await_progress(). It seems to be only used
for h264, and seems important for this tsan warning. By playing with
the third argument, I was able to come up with a patch (pasted below)
that eliminates the tsan warning and makes "make fate-h264 THREADS=4"
pass under tsan. I also played with the second argument (int
progress), but had no success.

Description of the patch:

1. The new function await_reference_mb_row_both_fields() is a variant
of await_reference_mb_row(). pred_temp_direct_motion() is changed to
call await_reference_mb_row_both_fields() instead of
await_reference_mb_row() before it reads
sl->ref_list[1][0].parent->mb_type[mb_xy].

2. If ref_field_picture is true, await_reference_mb_row_both_fields()
calls ff_thread_await_progress() with both field=0 and field=1.
(await_reference_mb_row() calls ff_thread_await_progress() with only
field=ref_field in this case.)

3. If ref_field_picture is false, await_reference_mb_row_both_fields()
calls ff_thread_await_progress() with only field=0, the same as
await_reference_mb_row().

I doubt this patch is correct, but I am publishing it to solicit
ideas. I will try to debug this more this weekend.

Thanks,
Wan-Teh Chang

diff --git a/libavcodec/h264_direct.c b/libavcodec/h264_direct.c
index a7a107c8c2..e8d3811c67 100644
--- a/libavcodec/h264_direct.c
+++ b/libavcodec/h264_direct.c
@@ -197,6 +197,25 @@ static void await_reference_mb_row(const
H264Context *const h, H264Ref *ref,
                              ref_field_picture && ref_field);
 }

+/* Waits until it is also safe to access ref->parent->mb_type[mb_xy]. */
+static void await_reference_mb_row_both_fields(const H264Context *const h,
+                                               H264Ref *ref, int mb_y)
+{
+    int ref_field_picture = ref->parent->field_picture;
+    int ref_height        = 16 * h->mb_height >> ref_field_picture;
+    int row               = 16 * mb_y >> ref_field_picture;
+
+    if (!HAVE_THREADS || !(h->avctx->active_thread_type & FF_THREAD_FRAME))
+        return;
+
+    /* FIXME: This is an educated guess. Is this right? */
+    ff_thread_await_progress(&ref->parent->tf, FFMIN(row, ref_height - 1), 0);
+    if (ref_field_picture) {
+        ff_thread_await_progress(&ref->parent->tf, FFMIN(row, ref_height - 1),
+                                 1);
+    }
+}
+
 static void pred_spatial_direct_motion(const H264Context *const h,
H264SliceContext *sl,
                                        int *mb_type)
 {
@@ -499,7 +518,7 @@ static void pred_temp_direct_motion(const
H264Context *const h, H264SliceContext

     assert(sl->ref_list[1][0].reference & 3);

-    await_reference_mb_row(h, &sl->ref_list[1][0],
+    await_reference_mb_row_both_fields(h, &sl->ref_list[1][0],
                            sl->mb_y + !!IS_INTERLACED(*mb_type));

     if (IS_INTERLACED(sl->ref_list[1][0].parent->mb_type[mb_xy])) {
// AFL/AFR/FR/FL -> AFL/FL