From patchwork Mon Apr 3 14:24:42 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Ronald S. Bultje" X-Patchwork-Id: 3269 Delivered-To: ffmpegpatchwork@gmail.com Received: by 10.103.44.195 with SMTP id s186csp110095vss; Mon, 3 Apr 2017 07:33:07 -0700 (PDT) X-Received: by 10.223.177.138 with SMTP id q10mr16901481wra.164.1491229987904; Mon, 03 Apr 2017 07:33:07 -0700 (PDT) Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org. [79.124.17.100]) by mx.google.com with ESMTP id f15si20181303wrf.124.2017.04.03.07.33.07; Mon, 03 Apr 2017 07:33:07 -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=@gmail.com; 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; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=gmail.com Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id D0B3F688327; Mon, 3 Apr 2017 17:33:02 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-qt0-f182.google.com (mail-qt0-f182.google.com [209.85.216.182]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 045506805EA for ; Mon, 3 Apr 2017 17:32:55 +0300 (EEST) Received: by mail-qt0-f182.google.com with SMTP id x35so113265276qtc.2 for ; Mon, 03 Apr 2017 07:32:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=sDH/KR00KpkEvPLVlfawa3gmo+23W/SeRaNJkgyWCE0=; b=c8o8Jx/ylnG9G9iItoYlJ5h+d210aFpjUWEOel3I8baPEPIVoUvqTEGLeWJaPqbfP6 qq5HRmlszDMH45lgplBZEre1lnFN0knaPcMf5NQ4rGEaSQFr3EPp5AKKXbhWPNUnHsv2 gvRwe2R2/YiO69rs9c9+C+a9qig3WVnFt9r8/EpExihcLsFiOh+9KFbmCf6RxE3ZmBJ5 YNY/uXIAKZ3FpKkUBIGl8QevbXuzcNiobXi5PpAEwET6pJEvG/IzpIxrLU7y3ohTiquF MpvLonNywVa7CabtRtiunHuauL9rGT1vomsC08BEAREcciB7F/jXMTLyb0UPpzuc3GP/ gBxA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=sDH/KR00KpkEvPLVlfawa3gmo+23W/SeRaNJkgyWCE0=; b=d/LFhSZLRDFNQ885lwVmBxqDGiejfYcIwZb9lBRCluDc3tsKTLn5eodaG0zhRk0yJP Zxs3mO4DXRoForqVMZZAx0vMqJi9ENkDFzA/wxKSydYB6pVhJZyxi8Vpcg7kIzNUbM5d BqKb7+lcn6I2LOxykHBvohzYfEBIgOpjWM2cZHfnjD8du6qLV7D/+/ctJCR1qMucUseE BR19XvCF8cGWJR7E/Yg0gxtqrRYgoQoXFVPHk+Kk7lZxU2zEyv1VVJ/BYYWStsyA30j9 cgh8qmWxgt5tLlxbLsKJOFju2LbCChf+Q2QJGiyqtMhelb08kEXVgVOIPZJHr/RFPSoj cklw== X-Gm-Message-State: AFeK/H1cSL4ojjQfrYgUWfeL+uKwU6EwL6dwA1olUCebW0bh2Yftfawzn3U8dDGwT+JV/A== X-Received: by 10.237.33.69 with SMTP id 63mr16376974qtc.109.1491229484671; Mon, 03 Apr 2017 07:24:44 -0700 (PDT) Received: from localhost.localdomain ([65.206.95.146]) by smtp.gmail.com with ESMTPSA id q40sm9708633qtq.21.2017.04.03.07.24.43 (version=TLS1 cipher=AES128-SHA bits=128/128); Mon, 03 Apr 2017 07:24:44 -0700 (PDT) From: "Ronald S. Bultje" To: ffmpeg-devel@ffmpeg.org Date: Mon, 3 Apr 2017 10:24:42 -0400 Message-Id: <1491229482-25479-1-git-send-email-rsbultje@gmail.com> X-Mailer: git-send-email 2.8.1 In-Reply-To: <1490796744-76454-2-git-send-email-rsbultje@gmail.com> References: <1490796744-76454-2-git-send-email-rsbultje@gmail.com> Subject: [FFmpeg-devel] [PATCH] pthread_frame: allow per-field ThreadFrame owners. 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 Cc: "Ronald S. Bultje" MIME-Version: 1.0 Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" This tries to handle cases where separate invocations of decode_frame() (each running in separate threads) write to respective fields in the same AVFrame->data[]. Having per-field owners makes interaction between readers (the referencing thread) and writers (the decoding thread) slightly more optimal if both accesses are field-based, since they will use the respective producer's thread objects (mutex/cond) instead of sharing the thread objects of the first field's producer. In practice, this fixes the following tsan-warning in fate-h264: WARNING: ThreadSanitizer: data race (pid=21615) Read of size 4 at 0x7d640000d9fc by thread T2 (mutexes: write M1006): #0 ff_thread_report_progress pthread_frame.c:569 (ffmpeg:x86_64+0x100f7cf54) [..] Previous write of size 4 at 0x7d640000d9fc by main thread (mutexes: write M1004): #0 update_context_from_user pthread_frame.c:335 (ffmpeg:x86_64+0x100f81abb) --- libavcodec/h264_slice.c | 8 +++++--- libavcodec/pthread_frame.c | 18 ++++++++++-------- libavcodec/thread.h | 2 +- libavcodec/utils.c | 7 ++++--- 4 files changed, 20 insertions(+), 15 deletions(-) diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c index fa1e9ae..d4d31cc 100644 --- a/libavcodec/h264_slice.c +++ b/libavcodec/h264_slice.c @@ -1423,14 +1423,14 @@ static int h264_field_start(H264Context *h, const H264SliceContext *sl, * We have to do that before the "dummy" in-between frame allocation, * since that can modify h->cur_pic_ptr. */ if (h->first_field) { + int last_field = last_pic_structure == PICT_BOTTOM_FIELD; av_assert0(h->cur_pic_ptr); av_assert0(h->cur_pic_ptr->f->buf[0]); assert(h->cur_pic_ptr->reference != DELAYED_PIC_REF); /* Mark old field/frame as completed */ - if (h->cur_pic_ptr->tf.owner == h->avctx) { - ff_thread_report_progress(&h->cur_pic_ptr->tf, INT_MAX, - last_pic_structure == PICT_BOTTOM_FIELD); + if (h->cur_pic_ptr->tf.owner[last_field] == h->avctx) { + ff_thread_report_progress(&h->cur_pic_ptr->tf, INT_MAX, last_field); } /* figure out if we have a complementary field pair */ @@ -1568,7 +1568,9 @@ static int h264_field_start(H264Context *h, const H264SliceContext *sl, return AVERROR_INVALIDDATA; } } else { + int field = h->picture_structure == PICT_BOTTOM_FIELD; release_unused_pictures(h, 0); + h->cur_pic_ptr->tf.owner[field] = h->avctx; } /* Some macroblocks can be accessed before they're available in case * of lost slices, MBAFF or threading. */ diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c index 9a6b83a..c246c2f 100644 --- a/libavcodec/pthread_frame.c +++ b/libavcodec/pthread_frame.c @@ -564,10 +564,11 @@ void ff_thread_report_progress(ThreadFrame *f, int n, int field) atomic_load_explicit(&progress[field], memory_order_relaxed) >= n) return; - p = f->owner->internal->thread_ctx; + p = f->owner[field]->internal->thread_ctx; - if (f->owner->debug&FF_DEBUG_THREADS) - av_log(f->owner, AV_LOG_DEBUG, "%p finished %d field %d\n", progress, n, field); + if (f->owner[field]->debug&FF_DEBUG_THREADS) + av_log(f->owner[field], AV_LOG_DEBUG, + "%p finished %d field %d\n", progress, n, field); pthread_mutex_lock(&p->progress_mutex); @@ -586,10 +587,11 @@ void ff_thread_await_progress(ThreadFrame *f, int n, int field) atomic_load_explicit(&progress[field], memory_order_acquire) >= n) return; - p = f->owner->internal->thread_ctx; + p = f->owner[field]->internal->thread_ctx; - if (f->owner->debug&FF_DEBUG_THREADS) - av_log(f->owner, AV_LOG_DEBUG, "thread awaiting %d field %d from %p\n", n, field, progress); + if (f->owner[field]->debug&FF_DEBUG_THREADS) + av_log(f->owner[field], AV_LOG_DEBUG, + "thread awaiting %d field %d from %p\n", n, field, progress); pthread_mutex_lock(&p->progress_mutex); while (atomic_load_explicit(&progress[field], memory_order_relaxed) < n) @@ -882,7 +884,7 @@ static int thread_get_buffer_internal(AVCodecContext *avctx, ThreadFrame *f, int PerThreadContext *p = avctx->internal->thread_ctx; int err; - f->owner = avctx; + f->owner[0] = f->owner[1] = avctx; ff_init_buffer_info(avctx, f->f); @@ -986,7 +988,7 @@ void ff_thread_release_buffer(AVCodecContext *avctx, ThreadFrame *f) av_log(avctx, AV_LOG_DEBUG, "thread_release_buffer called on pic %p\n", f); av_buffer_unref(&f->progress); - f->owner = NULL; + f->owner[0] = f->owner[1] = NULL; if (can_direct_free) { av_frame_unref(f->f); diff --git a/libavcodec/thread.h b/libavcodec/thread.h index c848d7a..90864b5 100644 --- a/libavcodec/thread.h +++ b/libavcodec/thread.h @@ -34,7 +34,7 @@ typedef struct ThreadFrame { AVFrame *f; - AVCodecContext *owner; + AVCodecContext *owner[2]; // progress->data is an array of 2 ints holding progress for top/bottom // fields AVBufferRef *progress; diff --git a/libavcodec/utils.c b/libavcodec/utils.c index 3e8677d..0c68836 100644 --- a/libavcodec/utils.c +++ b/libavcodec/utils.c @@ -3971,7 +3971,8 @@ int ff_thread_ref_frame(ThreadFrame *dst, ThreadFrame *src) { int ret; - dst->owner = src->owner; + dst->owner[0] = src->owner[0]; + dst->owner[1] = src->owner[1]; ret = av_frame_ref(dst->f, src->f); if (ret < 0) @@ -3981,7 +3982,7 @@ int ff_thread_ref_frame(ThreadFrame *dst, ThreadFrame *src) if (src->progress && !(dst->progress = av_buffer_ref(src->progress))) { - ff_thread_release_buffer(dst->owner, dst); + ff_thread_release_buffer(dst->owner[0], dst); return AVERROR(ENOMEM); } @@ -3997,7 +3998,7 @@ enum AVPixelFormat ff_thread_get_format(AVCodecContext *avctx, const enum AVPixe int ff_thread_get_buffer(AVCodecContext *avctx, ThreadFrame *f, int flags) { - f->owner = avctx; + f->owner[0] = f->owner[1] = avctx; return ff_get_buffer(avctx, f->f, flags); }