From patchwork Fri Apr 28 16:23:10 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Muhammad Faiz X-Patchwork-Id: 3511 Delivered-To: ffmpegpatchwork@gmail.com Received: by 10.103.3.129 with SMTP id 123csp321949vsd; Fri, 28 Apr 2017 09:23:23 -0700 (PDT) X-Received: by 10.223.131.67 with SMTP id 61mr7250066wrd.37.1493396603065; Fri, 28 Apr 2017 09:23:23 -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 b8si6402697wra.295.2017.04.28.09.23.22; Fri, 28 Apr 2017 09:23:23 -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 0442068982F; Fri, 28 Apr 2017 19:23:17 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-qk0-f194.google.com (mail-qk0-f194.google.com [209.85.220.194]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id C473E689200 for ; Fri, 28 Apr 2017 19:23:09 +0300 (EEST) Received: by mail-qk0-f194.google.com with SMTP id a19so9602891qkg.2 for ; Fri, 28 Apr 2017 09:23:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to; bh=7B8HW/cX2lcWKONvrXmNEbCAGPHYVYFcqT9zSbuOOkY=; b=AndnznJjQdpIalgwnKdts3RFf6iQToCva/KWJ9KRX4zrpQvPXb2yWyhp3ctPjAKJ65 RGwVZLK/EYCvjzHk6gO7bANU1pXyjFqOjv0jovK8t2RZVpuDMpDvYWp43EiLSUcuYiHO vhMBqvAPHblzVg1xUDLy7nVSq2stlpQq+ClcvTiUKkSa9mNSopDgs0UWM210BAIonItM bn6l90Ihb6EbqrFJ9ZbZUqAMW/+iPXbeSA+5yVYMSwuJ79nnppH89f/Bgvq+rBXPeD32 lU8KKfURWJjPiGGWbiEONiGIO2SQr7keZ2q84ToCwRaRzjTtY2I9jAdy+b4wWb8Tkow/ vsng== 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=7B8HW/cX2lcWKONvrXmNEbCAGPHYVYFcqT9zSbuOOkY=; b=hzeCqxAW6hfDKbxD/YTwob0qYf9NibN46V1tl1nQUQ0mhyJ0VwoZg6bWNpHrXKGK5h 1xSIvm0O0u1jqgZ+MGIIsJN4jXAvyQhP7iDvRq7AEd2ahyCzQmYgtXgM9dFmZYLr4I1d 4Isjs688jjnYF/rRqJgt72l3o0RYC2RluHNECRyE5DDe/2t0VxwobphyxajxswZyQQoU horIHQ3HIaFXu1hrthZMwDvdxfTz5XT7uM8lq1Vkch+DGDyMujoGPdIN1ZRTZeqpttFI /fZypsl4YJ4tra8VAJoVS+jMSa018/vVk6/ecyGcWD1sEt2T86Nz34JYPVEtZGpjQ+Li uMow== X-Gm-Message-State: AN3rC/46XTbQvC4cz01BMDHwjM8XBqDFpac95OT1kbeX3qWJgVfmPMi3 Ly78VrJB2NAKt73Ssq8Qr9DXSPfotacv X-Received: by 10.55.17.32 with SMTP id b32mr11035809qkh.5.1493396590899; Fri, 28 Apr 2017 09:23:10 -0700 (PDT) MIME-Version: 1.0 Received: by 10.237.34.9 with HTTP; Fri, 28 Apr 2017 09:23:10 -0700 (PDT) In-Reply-To: References: <20170428101913.12093-1-mfcc64@gmail.com> From: Muhammad Faiz Date: Fri, 28 Apr 2017 23:23:10 +0700 Message-ID: To: FFmpeg development discussions and patches Subject: Re: [FFmpeg-devel] [PATCH v2] avcodec/pthread_frame, decode: allow errors to happen on draining 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" On Fri, Apr 28, 2017 at 5:55 PM, Ronald S. Bultje wrote: > Hi, > > On Fri, Apr 28, 2017 at 6:19 AM, Muhammad Faiz wrote: >> >> So, all frames and errors are correctly reported in order. >> Also limit the numbers of error during draining to prevent infinite loop. >> >> This fix fate failure with THREADS>=4: >> make fate-h264-attachment-631 THREADS=4 >> This also reverts a755b725ec1d657609c8bd726ce37e7cf193d03f. >> >> Suggested-by: wm4, Ronald S. Bultje, Marton Balint >> Signed-off-by: Muhammad Faiz >> --- >> libavcodec/decode.c | 21 +++++++++++++++++++-- >> libavcodec/internal.h | 3 +++ >> libavcodec/pthread_frame.c | 15 +++++++-------- >> 3 files changed, 29 insertions(+), 10 deletions(-) >> >> diff --git a/libavcodec/decode.c b/libavcodec/decode.c >> index 6ff3c40..edfae55 100644 >> --- a/libavcodec/decode.c >> +++ b/libavcodec/decode.c >> @@ -568,8 +568,24 @@ FF_ENABLE_DEPRECATION_WARNINGS >> avctx->time_base = av_inv_q(av_mul_q(avctx->framerate, >> (AVRational){avctx->ticks_per_frame, 1})); >> #endif >> >> - if (avctx->internal->draining && !got_frame) >> - avci->draining_done = 1; >> + /* do not stop draining when got_frame != 0 or ret < 0 */ >> + if (avctx->internal->draining && !got_frame) { >> + if (ret < 0) { >> + /* prevent infinite loop if a decoder wrongly always return >> error on draining */ >> + /* reasonable nb_errors_max = maximum b frames + thread count >> */ >> + int nb_errors_max = 20 + (HAVE_THREADS && >> avctx->active_thread_type & FF_THREAD_FRAME ? >> + avctx->thread_count : 1); >> + >> + if (avci->nb_draining_errors++ >= nb_errors_max) { >> + av_log(avctx, AV_LOG_ERROR, "Too many errors when >> draining, this is a bug. " >> + "Stop draining and force EOF.\n"); >> + avci->draining_done = 1; >> + ret = AVERROR_BUG; >> + } >> + } else { >> + avci->draining_done = 1; >> + } >> + } > > > Hm... I guess this is OK, it would be really nice to have a way of breaking > in developer builds (e.g. av_assert or so, although I guess technically this > could be enabled in prod builds also). Add av_assert2(). > > Also, Marton suggested to return AVERROR_EOF, maybe handle that here also in > addition to ret=0? Modified. Updated patch attached. Thank's From f684770e016fa36d458d08383065815882cbc7f8 Mon Sep 17 00:00:00 2001 From: Muhammad Faiz Date: Fri, 28 Apr 2017 17:08:39 +0700 Subject: [PATCH v3] avcodec/pthread_frame, decode: allow errors to happen on draining So, all frames and errors are correctly reported in order. Also limit the number of errors during draining to prevent infinite loop. Also return AVERROR_EOF directly on EOF instead of only setting draining_done. This fix fate failure with THREADS>=4: make fate-h264-attachment-631 THREADS=4 This also reverts a755b725ec1d657609c8bd726ce37e7cf193d03f. Suggested-by: wm4, Ronald S. Bultje, Marton Balint Signed-off-by: Muhammad Faiz --- libavcodec/decode.c | 23 +++++++++++++++++++++-- libavcodec/internal.h | 3 +++ libavcodec/pthread_frame.c | 15 +++++++-------- 3 files changed, 31 insertions(+), 10 deletions(-) diff --git a/libavcodec/decode.c b/libavcodec/decode.c index 6ff3c40..fb4d4af 100644 --- a/libavcodec/decode.c +++ b/libavcodec/decode.c @@ -568,8 +568,26 @@ FF_ENABLE_DEPRECATION_WARNINGS avctx->time_base = av_inv_q(av_mul_q(avctx->framerate, (AVRational){avctx->ticks_per_frame, 1})); #endif - if (avctx->internal->draining && !got_frame) - avci->draining_done = 1; + /* do not stop draining when got_frame != 0 or ret < 0 */ + if (avctx->internal->draining && !got_frame) { + if (ret < 0) { + /* prevent infinite loop if a decoder wrongly always return error on draining */ + /* reasonable nb_errors_max = maximum b frames + thread count */ + int nb_errors_max = 20 + (HAVE_THREADS && avctx->active_thread_type & FF_THREAD_FRAME ? + avctx->thread_count : 1); + + if (avci->nb_draining_errors++ >= nb_errors_max) { + av_log(avctx, AV_LOG_ERROR, "Too many errors when draining, this is a bug. " + "Stop draining and force EOF.\n"); + avci->draining_done = 1; + ret = AVERROR_BUG; + av_assert2(0); + } + } else { + avci->draining_done = 1; + ret = AVERROR_EOF; + } + } avci->compat_decode_consumed += ret; @@ -1659,6 +1677,7 @@ void avcodec_flush_buffers(AVCodecContext *avctx) { avctx->internal->draining = 0; avctx->internal->draining_done = 0; + avctx->internal->nb_draining_errors = 0; av_frame_unref(avctx->internal->buffer_frame); av_frame_unref(avctx->internal->compat_decode_frame); av_packet_unref(avctx->internal->buffer_pkt); diff --git a/libavcodec/internal.h b/libavcodec/internal.h index 84d3362..caa46dc 100644 --- a/libavcodec/internal.h +++ b/libavcodec/internal.h @@ -200,6 +200,9 @@ typedef struct AVCodecInternal { int showed_multi_packet_warning; int skip_samples_multiplier; + + /* to prevent infinite loop on errors when draining */ + int nb_draining_errors; } AVCodecInternal; struct AVCodecDefault { diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c index 13d6828..363b139 100644 --- a/libavcodec/pthread_frame.c +++ b/libavcodec/pthread_frame.c @@ -509,8 +509,8 @@ int ff_thread_decode_frame(AVCodecContext *avctx, /* * Return the next available frame from the oldest thread. * If we're at the end of the stream, then we have to skip threads that - * didn't output a frame, because we don't want to accidentally signal - * EOF (avpkt->size == 0 && *got_picture_ptr == 0). + * didn't output a frame/error, because we don't want to accidentally signal + * EOF (avpkt->size == 0 && *got_picture_ptr == 0 && err >= 0). */ do { @@ -526,20 +526,19 @@ int ff_thread_decode_frame(AVCodecContext *avctx, av_frame_move_ref(picture, p->frame); *got_picture_ptr = p->got_frame; picture->pkt_dts = p->avpkt.dts; - - if (p->result < 0) - err = p->result; + err = p->result; /* * A later call with avkpt->size == 0 may loop over all threads, - * including this one, searching for a frame to return before being + * including this one, searching for a frame/error to return before being * stopped by the "finished != fctx->next_finished" condition. - * Make sure we don't mistakenly return the same frame again. + * Make sure we don't mistakenly return the same frame/error again. */ p->got_frame = 0; + p->result = 0; if (finished >= avctx->thread_count) finished = 0; - } while (!avpkt->size && !*got_picture_ptr && finished != fctx->next_finished); + } while (!avpkt->size && !*got_picture_ptr && err >= 0 && finished != fctx->next_finished); update_context_from_thread(avctx, p->avctx, 1); -- 2.9.3