[FFmpeg-devel] Fix for ticket 6796 (ffprobe show_frames ts dvbsubs infinate loop)

Submitted by Colin NG on Dec. 1, 2017, 11:55 p.m.

Details

Message ID DM5PR22MB0681D91B82A83B47C41B6B53FE390@DM5PR22MB0681.namprd22.prod.outlook.com
State New
Headers show

Commit Message

Colin NG Dec. 1, 2017, 11:55 p.m.
---
 fftools/ffprobe.c   | 5 +++++
 libavcodec/decode.c | 5 ++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

Comments

Hendrik Leppkes Dec. 2, 2017, 12:05 a.m.
On Sat, Dec 2, 2017 at 12:55 AM, Colin NG <colin_ng@hotmail.com> wrote:
> ---
>  fftools/ffprobe.c   | 5 +++++
>  libavcodec/decode.c | 5 ++++-
>  2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c
> index 0e7a771..20b64ef 100644
> --- a/fftools/ffprobe.c
> +++ b/fftools/ffprobe.c
> @@ -2276,10 +2276,14 @@ static av_always_inline int process_frame(WriterContext *w,
>
>          case AVMEDIA_TYPE_SUBTITLE:
>              ret = avcodec_decode_subtitle2(dec_ctx, &sub, &got_frame, pkt);
> +            if (ret == AVERROR(EINVAL) || ret == AVERROR_INVALIDDATA) {
> +                ret = 0;
> +            }


Ignoring certain error codes doesn't seem correct. If decoding of a
subtitle fails, it should be handled somewhere, not just ignored.

> @@ -2290,6 +2294,7 @@ static av_always_inline int process_frame(WriterContext *w,
>      if (got_frame) {
>          int is_sub = (par->codec_type == AVMEDIA_TYPE_SUBTITLE);
>          nb_streams_frames[pkt->stream_index]++;
> +        got_frame = 0;
>          if (do_show_frames)
>              if (is_sub)
>                  show_subtitle(w, &sub, ifile->streams[pkt->stream_index].st, fmt_ctx);
> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> index 3f5b086..d6cc671 100644
> --- a/libavcodec/decode.c
> +++ b/libavcodec/decode.c
> @@ -1024,7 +1024,10 @@ int avcodec_decode_subtitle2(AVCodecContext *avctx, AVSubtitle *sub,
>              if (avctx->pkt_timebase.num && avpkt->pts != AV_NOPTS_VALUE)
>                  sub->pts = av_rescale_q(avpkt->pts,
>                                          avctx->pkt_timebase, AV_TIME_BASE_Q);
> -            ret = avctx->codec->decode(avctx, sub, got_sub_ptr, &pkt_recoded);
> +            ret = avctx->codec->decode(avctx, sub, &pkt_recoded.size, &pkt_recoded);
> +            if (ret == avpkt->size) {
> +              *got_sub_ptr = 1;
> +            }
>              av_assert1((ret >= 0) >= !!*got_sub_ptr &&
>                         !!*got_sub_ptr >= !!sub->num_rects);
>

This looks definitely wrong. The decode API has a "*got_frame" ptr
there, not any size parameter, and additionally just because it
consumed data it does not mean any data was actually output.

- Hendrik
Hendrik Leppkes Dec. 2, 2017, 12:12 a.m.
On Sat, Dec 2, 2017 at 1:05 AM, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>
>> @@ -2290,6 +2294,7 @@ static av_always_inline int process_frame(WriterContext *w,
>>      if (got_frame) {
>>          int is_sub = (par->codec_type == AVMEDIA_TYPE_SUBTITLE);
>>          nb_streams_frames[pkt->stream_index]++;
>> +        got_frame = 0;

Having looked at the logic some more, this part seems to be the key
(although implemented too broadly). We don't want to repeat the loop
for subtitles, so you set got_frame = 0 here - but you should only do
it for subtitles, not for everyhting.

- Hendrik

Patch hide | download patch | download mbox

diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c
index 0e7a771..20b64ef 100644
--- a/fftools/ffprobe.c
+++ b/fftools/ffprobe.c
@@ -2276,10 +2276,14 @@  static av_always_inline int process_frame(WriterContext *w,
 
         case AVMEDIA_TYPE_SUBTITLE:
             ret = avcodec_decode_subtitle2(dec_ctx, &sub, &got_frame, pkt);
+            if (ret == AVERROR(EINVAL) || ret == AVERROR_INVALIDDATA) {
+                ret = 0;
+            }
             *packet_new = 0;
             break;
         default:
             *packet_new = 0;
+            break;
         }
     } else {
         *packet_new = 0;
@@ -2290,6 +2294,7 @@  static av_always_inline int process_frame(WriterContext *w,
     if (got_frame) {
         int is_sub = (par->codec_type == AVMEDIA_TYPE_SUBTITLE);
         nb_streams_frames[pkt->stream_index]++;
+        got_frame = 0;
         if (do_show_frames)
             if (is_sub)
                 show_subtitle(w, &sub, ifile->streams[pkt->stream_index].st, fmt_ctx);
diff --git a/libavcodec/decode.c b/libavcodec/decode.c
index 3f5b086..d6cc671 100644
--- a/libavcodec/decode.c
+++ b/libavcodec/decode.c
@@ -1024,7 +1024,10 @@  int avcodec_decode_subtitle2(AVCodecContext *avctx, AVSubtitle *sub,
             if (avctx->pkt_timebase.num && avpkt->pts != AV_NOPTS_VALUE)
                 sub->pts = av_rescale_q(avpkt->pts,
                                         avctx->pkt_timebase, AV_TIME_BASE_Q);
-            ret = avctx->codec->decode(avctx, sub, got_sub_ptr, &pkt_recoded);
+            ret = avctx->codec->decode(avctx, sub, &pkt_recoded.size, &pkt_recoded);
+            if (ret == avpkt->size) {
+              *got_sub_ptr = 1;
+            }
             av_assert1((ret >= 0) >= !!*got_sub_ptr &&
                        !!*got_sub_ptr >= !!sub->num_rects);