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

[Michael Niedermayer]

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

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No snowflake in an avalanche ever feels responsible. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170429/ddc3156b/attachment.sig>