diff mbox

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

Message ID CAJsO9v4GohUXXJp7WC4WW_FY8wVac5AD7o4bipVxx0WKL4UL9A@mail.gmail.com
State Accepted
Headers show

Commit Message

Muhammad Faiz April 28, 2017, 4:23 p.m. UTC 
On Fri, Apr 28, 2017 at 5:55 PM, Ronald S. Bultje <rsbultje@gmail.com> wrote:
> Hi,
>
> On Fri, Apr 28, 2017 at 6:19 AM, Muhammad Faiz <mfcc64@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@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

Comments

Muhammad Faiz April 28, 2017, 9:10 p.m. UTC | #1 
On Fri, Apr 28, 2017 at 11:23 PM, Muhammad Faiz <mfcc64@gmail.com> wrote:
> On Fri, Apr 28, 2017 at 5:55 PM, Ronald S. Bultje <rsbultje@gmail.com> wrote:
>> Hi,
>>
>> On Fri, Apr 28, 2017 at 6:19 AM, Muhammad Faiz <mfcc64@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@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

>+        } else {
>+            avci->draining_done = 1;
>+            ret = AVERROR_EOF;
>+        }
>+    }
>
>    avci->compat_decode_consumed += ret;

I'm not sure about changing ret. It seems not trivial. So, I drop the
updated patch, and use the original patch.

Thank's
Michael Niedermayer April 28, 2017, 11:01 p.m. UTC | #2 
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@gmail.com> wrote:
> > Hi,
> >
> > On Fri, Apr 28, 2017 at 6:19 AM, Muhammad Faiz <mfcc64@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@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@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@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

[...]
Muhammad Faiz April 28, 2017, 11:18 p.m. UTC | #3 
On Sat, Apr 29, 2017 at 6:01 AM, Michael Niedermayer
<michael@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@gmail.com> wrote:
>> > Hi,
>> >
>> > On Fri, Apr 28, 2017 at 6:19 AM, Muhammad Faiz <mfcc64@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@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@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@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
Muhammad Faiz April 29, 2017, 11:09 p.m. UTC | #4 
On Sat, Apr 29, 2017 at 6:18 AM, Muhammad Faiz <mfcc64@gmail.com> wrote:
> On Sat, Apr 29, 2017 at 6:01 AM, Michael Niedermayer
> <michael@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@gmail.com> wrote:
>>> > Hi,
>>> >
>>> > On Fri, Apr 28, 2017 at 6:19 AM, Muhammad Faiz <mfcc64@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@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@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@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)

Thank's
wm4 April 30, 2017, 1:27 p.m. UTC | #5 
On Sun, 30 Apr 2017 06:09:18 +0700
Muhammad Faiz <mfcc64@gmail.com> wrote:

> On Sat, Apr 29, 2017 at 6:18 AM, Muhammad Faiz <mfcc64@gmail.com> wrote:
> > On Sat, Apr 29, 2017 at 6:01 AM, Michael Niedermayer
> > <michael@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@gmail.com> wrote:  
> >>> > Hi,
> >>> >
> >>> > On Fri, Apr 28, 2017 at 6:19 AM, Muhammad Faiz <mfcc64@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@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@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@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.
Muhammad Faiz April 30, 2017, 3:42 p.m. UTC | #6 
On Sun, Apr 30, 2017 at 8:27 PM, wm4 <nfxjfg@googlemail.com> wrote:
> On Sun, 30 Apr 2017 06:09:18 +0700
> Muhammad Faiz <mfcc64@gmail.com> wrote:
>
>> On Sat, Apr 29, 2017 at 6:18 AM, Muhammad Faiz <mfcc64@gmail.com> wrote:
>> > On Sat, Apr 29, 2017 at 6:01 AM, Michael Niedermayer
>> > <michael@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@gmail.com> wrote:
>> >>> > Hi,
>> >>> >
>> >>> > On Fri, Apr 28, 2017 at 6:19 AM, Muhammad Faiz <mfcc64@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@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@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@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.

I'm sorry for pushing it too quickly. I refer on
https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2017-March/208718.html.
There are no clear rules, anyway.

Thank's.
Nicolas George April 30, 2017, 3:46 p.m. UTC | #7 
Le primidi 11 floréal, an CCXXV, Muhammad Faiz a écrit :
> I'm sorry for pushing it too quickly. I refer on
> https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2017-March/208718.html.

This is wrong, do not take it into account.

No objections after one ping and a few days means the patch is OK.
Anything less means nothing.

And you should be careful when putting punctuation after URLs.

Regards,
James Almer April 30, 2017, 3:53 p.m. UTC | #8 
On 4/30/2017 12:46 PM, Nicolas George wrote:
> Le primidi 11 floréal, an CCXXV, Muhammad Faiz a écrit :
>> I'm sorry for pushing it too quickly. I refer on
>> https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2017-March/208718.html.
> 
> This is wrong, do not take it into account.
> 
> No objections after one ping and a few days means the patch is OK.
> Anything less means nothing.

I don't think there's any explicit written rule about it, but in any
case, both Ronald and wm4 had given him the ok for the patch he pushed,
so waiting 24 hours after that is fine.

It could have waited a bit mainly because it's delicate code that could
break badly if something is done wrong, so the more testing it's given
the better.

> 
> And you should be careful when putting punctuation after URLs.
> 
> Regards,
Nicolas George April 30, 2017, 5:04 p.m. UTC | #9 
Le primidi 11 floréal, an CCXXV, James Almer a écrit :
> I don't think there's any explicit written rule about it, but in any
> case, both Ronald and wm4 had given him the ok for the patch he pushed,
> so waiting 24 hours after that is fine.
> 
> It could have waited a bit mainly because it's delicate code that could
> break badly if something is done wrong, so the more testing it's given
> the better.

I have not read the specifics of this thread, I do not think that
Muhammad did anything wrong here. I only react to the mail quoted
earlier:

https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2017-March/208718.html

"No objections issues after 24hrs means patch is OK in FFmpeg
development."

This is just not true, and harmfully so.

Regards,
diff mbox

Patch

From f684770e016fa36d458d08383065815882cbc7f8 Mon Sep 17 00:00:00 2001
From: Muhammad Faiz <mfcc64@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@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);
+            }
+        } 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