| Message ID | 20170428101913.12093-1-mfcc64@gmail.com |
|---|---|
| State | Accepted |
| Commit | d535e0c14004a15bb38ea288fa9a4f2e27d26f6b |
| Headers | show |
On Fri, 28 Apr 2017 17:19:13 +0700 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; > + } > + } 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?
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). Also, Marton suggested to return AVERROR_EOF, maybe handle that here also in addition to ret=0? Ronald
On Fri, Apr 28, 2017 at 5:51 PM, wm4 <nfxjfg@googlemail.com> wrote: > On Fri, 28 Apr 2017 17:19:13 +0700 > 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; >> + } >> + } > > 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.
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; + } + } 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);
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(-)