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

[Muhammad Faiz]

On Fri, Apr 28, 2017 at 5:51 PM, wm4 <nfxjfg at googlemail.com> wrote:
> On Fri, 28 Apr 2017 17:19:13 +0700
> 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;
>> +        }
>> +    }
>
> Yeah, that's fancy and should mostly work. 20 should be large enough
> to account for h264 and hevc, but in theory not all decoders need to
> honor this delay (consider hardware decoder wrappers). But this is only
> about the case of errors during draining, so it should be still ok.
>
> In any case better than my earlier shithacks.
>
>>
>>      avci->compat_decode_consumed += ret;
>>
>> @@ -1659,6 +1675,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);
>>
>
> Seems ok.
>
> Do we still need the loop?

We still need to skip !*got_picture_ptr && err >= 0 during draining.

Thank's.