From patchwork Thu Jul 20 17:16:22 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Wan-Teh Chang X-Patchwork-Id: 4392 Delivered-To: ffmpegpatchwork@gmail.com Received: by 10.103.1.76 with SMTP id 73csp2402331vsb; Thu, 20 Jul 2017 10:16:34 -0700 (PDT) X-Received: by 10.223.180.88 with SMTP id v24mr7299388wrd.29.1500570994675; Thu, 20 Jul 2017 10:16:34 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1500570994; cv=none; d=google.com; s=arc-20160816; b=DY7VXiBRHaUQ82K2J9SB63vH+NVrq0A9SS3r5ydNOX/jJ+vj/ffkDd2YHxjL42OZ0N mFM4WjKuQRHynifm2Z5UxZ2FlRiKR4NbtwQ0wa0A++0rDhk40juDyLHIag3IKdcrExXt rJ6Xz+gG/rvIyh+h1iWAFMWmElSJ42ZbIKK4fIbLqpc6+VeHeiYqzcG+Xbu5OWaLe6de vxv0fA9PvfI1bysWGNGioZ+8SpQxLr3VI/twFamnt0ubavismVttFNbW1rAcKrsol6zh eqrG2fxL5bvf8cx/XXdgOvTmqxlZpOS6Fz4I17sc4SBHYOJTfhg20pQHWoaEnx9BJtjT 7qhA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=sender:errors-to:content-transfer-encoding:reply-to:list-subscribe :list-help:list-post:list-archive:list-unsubscribe:list-id :precedence:subject:to:message-id:date:from:references:in-reply-to :mime-version:dkim-signature:delivered-to:arc-authentication-results; bh=rI4I9HKtbgko7ihYB0x4tl6LsPL5NwInjyAenZ0xms4=; b=Yb5c1EqL7tByYllje4Q1nFgNqb7iOkRsmoUibkivWmwP1wYu98xThc8pgyhZBb+ZEr QZdGkeQ9+oAP6yzDL04MI48d0sJ+jF6FyCO68LnLootHAvfZaVuLJn7s/sLWRRzzTg2W a1A9cUJJPcUAq1T4G4qD22b6XdOE49LAyIveg/UHxKNlx4kP2sUORp6ZtGXIbDqBpQsY oC9aTDJt6eO0Z1A0d/jXwpqRaa0FXsHDQBgRfmms5i2QKRywJCceda6F92HVsb51e+lo Ti6AKOxcJiKKNw0DBmoLFAEe15UHKcy127p1o56W5GBZeh5dOAAsy+ynTESPGA6dhpYd 9J7A== ARC-Authentication-Results: i=1; mx.google.com; dkim=neutral (body hash did not verify) header.i=@google.com header.b=BI97Oqau; spf=pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) smtp.mailfrom=ffmpeg-devel-bounces@ffmpeg.org Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org. [79.124.17.100]) by mx.google.com with ESMTP id i135si2023524wmd.125.2017.07.20.10.16.34; Thu, 20 Jul 2017 10:16:34 -0700 (PDT) Received-SPF: pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) client-ip=79.124.17.100; Authentication-Results: mx.google.com; dkim=neutral (body hash did not verify) header.i=@google.com header.b=BI97Oqau; spf=pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) smtp.mailfrom=ffmpeg-devel-bounces@ffmpeg.org Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 225F4689A36; Thu, 20 Jul 2017 20:16:23 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-wr0-f178.google.com (mail-wr0-f178.google.com [209.85.128.178]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 404D06898C1 for ; Thu, 20 Jul 2017 20:16:16 +0300 (EEST) Received: by mail-wr0-f178.google.com with SMTP id v105so43251132wrb.0 for ; Thu, 20 Jul 2017 10:16:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to; bh=T5eY/JQCJfYlJXCaWsjhvYqy0UF9GbdEtsfJoPAGiZo=; b=BI97OqaugrIWeayAC/01hDQhK/31V/5vJO0Cg5+o/6zbDWvv/a3ohM6NeTjybg+w9L W35SoE+h0CHDifWJX+huTXH/GFOK/GUMRQu9oN8TWS5YG5xblhhtULqFffxal0UhKDjy eNiu8b4Vqjif2/x0rs/HNIEvX5dzW1OF1eOF4TJSr5de/+Tn7Az24RVMiJXkscrsEzeJ GzVYvtZ4e3J9KtCGPILEotLw1dPPJEpeJBpchPM1aGSZ0vA/wJSv+Lj0zSfyCw5N6giv nZ9Wi/Em/Lk1Jp0SIeZm+j102Bq4k289Zs6/cdJR9nlImDfpnpdxitGBqZ5mRW8iDZas Vhpw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to; bh=T5eY/JQCJfYlJXCaWsjhvYqy0UF9GbdEtsfJoPAGiZo=; b=F5P80LbqLAguN7liNkOPsB5QrKOt3dqces1OY6Qlw/PeHUX5vjPB+RX01m04mUq9ug Iui6tQLa/Jc1P2gn9vYqpkrjFa8YpXyq3zQOl/gAnLldziU7P2yQTXWiIXztcj6uuntz max+SJlY1F+4GVHBB8k0UNQD2OC09dY8ZgXIL1u4khGn9W9s9REA2WVOavjjAQKWSDK5 8iakIy5GBDzHB3zgBy7ccpER46EV+I5PiOxX163N0xDIGIkQGckLIG159RzBVhlbXr5U uO0Drf1we+LTzCo04vEBkJK7f0wHlzGLi7OhIgq26JOtattKu4zf/ouRs23r4fcv3rG8 TwIw== X-Gm-Message-State: AIVw112MapThsDVJDSTgKtQKaeXUwHMQujpLbpSFrcLmlbacJDu8USaQ lfSZpHyms+aZkPjSZbKThPXWqifFF+AAsmNlRw== X-Received: by 10.223.130.144 with SMTP id 16mr7244280wrc.166.1500570983280; Thu, 20 Jul 2017 10:16:23 -0700 (PDT) MIME-Version: 1.0 Received: by 10.28.184.85 with HTTP; Thu, 20 Jul 2017 10:16:22 -0700 (PDT) In-Reply-To: References: From: Wan-Teh Chang Date: Thu, 20 Jul 2017 10:16:22 -0700 Message-ID: To: FFmpeg development discussions and patches Subject: Re: [FFmpeg-devel] tsan warning about a data race in libavcodec/h264_direct.c X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Hi Ronald, On Wed, Jul 19, 2017 at 11:50 AM, Ronald S. Bultje wrote: > Hi Wan-Teh, > > On Wed, Jul 19, 2017 at 2:31 PM, Wan-Teh Chang > 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 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 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,