[FFmpeg-devel] [PATCH v2] avcodec/pthread_frame, decode: allow errors to happen on draining

[wm4]

On Sun, 30 Apr 2017 06:09:18 +0700
Muhammad Faiz <mfcc64 at gmail.com> wrote:

> On Sat, Apr 29, 2017 at 6:18 AM, Muhammad Faiz <mfcc64 at gmail.com> wrote:
> > On Sat, Apr 29, 2017 at 6:01 AM, Michael Niedermayer
> > <michael at niedermayer.cc> wrote:  
> >> On Fri, Apr 28, 2017 at 11:23:10PM +0700, Muhammad Faiz wrote:  
> >>> On Fri, Apr 28, 2017 at 5:55 PM, Ronald S. Bultje <rsbultje at gmail.com> wrote:  
> >>> > Hi,
> >>> >
> >>> > On Fri, Apr 28, 2017 at 6:19 AM, Muhammad Faiz <mfcc64 at gmail.com> 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 <mfcc64 at gmail.com>
> >>> >> ---
> >>> >>  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  
> >>  
> >>>  decode.c        |   23 +++++++++++++++++++++--
> >>>  internal.h      |    3 +++
> >>>  pthread_frame.c |   15 +++++++--------
> >>>  3 files changed, 31 insertions(+), 10 deletions(-)
> >>> d3049c52598070baa9566fc98a089111732595fa  0001-avcodec-pthread_frame-decode-allow-errors-to-happen-.patch
> >>> From f684770e016fa36d458d08383065815882cbc7f8 Mon Sep 17 00:00:00 2001
> >>> From: Muhammad Faiz <mfcc64 at gmail.com>
> >>> 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 <mfcc64 at gmail.com>
> >>> ---
> >>>  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);  
> >>
> >> Please build with --assert-level=2
> >> this triggers for the 2 files i sent you yesterday  
> >
> > I've already dropped the updated patch.
> >
> > Thank's  
> 
> Applied (PATCH v2)

This one could probably have waited a bit longer.

Thanks for looking into this and fixing it, anyway.