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

[Muhammad Faiz]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-avcodec-pthread_frame-decode-allow-errors-to-happen-.patch
Type: text/x-patch
Size: 4844 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170428/71b4e5c5/attachment.bin>