On Fri, Apr 28, 2017 at 5:51 PM, wm4 <nfx...@googlemail.com> wrote:
> On Fri, 28 Apr 2017 17:19:13 +0700
> Muhammad Faiz <mfc...@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 <mfc...@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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
