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

[Michael Niedermayer]

On Sat, Dec 02, 2017 at 12:42:22AM +, Colin NG wrote:
> ---
>  fftools/ffprobe.c  |  2 ++
>  libavcodec/dvbsubdec.c | 10 +-
>  2 files changed, 7 insertions(+), 5 deletions(-)

This should be split in 2 patches
one for the lib and one for the application

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Observe your enemies, for they first find out your faults. -- Antisthenes


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Hendrik Leppkes]

On Sat, Dec 2, 2017 at 1:05 AM, Hendrik Leppkes  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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Hendrik Leppkes]

On Sat, Dec 2, 2017 at 12:55 AM, Colin NG  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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel