Hi Ronald,
On Wed, Jul 19, 2017 at 11:50 AM, Ronald S. Bultje <rsbul...@gmail.com> wrote:
> Hi Wan-Teh,
>
> On Wed, Jul 19, 2017 at 2:31 PM, Wan-Teh Chang <wtc-at-google....@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
