[FFmpeg-devel] ffprobe: Do not decode zero-sized packets in ffprobe -show_frame

[Stefano Sabatini]

On date Thursday 2012-02-16 09:55:56 +0100, Petter Ericson encoded:
> On Wed, Feb 15, 2012 at 05:39:15PM +0100, Michael Niedermayer wrote:
> > On Tue, Feb 14, 2012 at 05:26:13PM +0100, Petter Ericson wrote:
> > > Greetings
> > > 
> > > Ticket #997 details a segfault in ffprobe that was exposed by the file
> > > http://titan.codemill.se/~peteri/120210144737.ts
> > > 
> > > This patch fixes the segfault. However, the desyncing issue that I
> > > mentioned in the ticket still remains (I was sloppy when looking at the
> > > output - It is still present in git master). If anyone could advice on what
> > > it is that is causing transcoding to result in desynced output, I would be
> > > most grateful. 
> > > 
> > > The patched ffmpeg passes make fate.
> > > 
> > > Best regards
> > > 
> > > Petter Ericson
> > > 
> > 
> > >  ffprobe.c |    2 ++
> > >  1 file changed, 2 insertions(+)
> > > d4cd04827b5c74b8d5d67f85d3f49b9646ae392d  patch
> > > commit e40952af2f5b6eccf24b34926bf09747117affdc
> > > Author: Petter Ericson <petter.ericson at codemill.se>
> > > Date:   Tue Feb 14 16:59:56 2012 +0100
> > > 
> > >     ffprobe: Do not avcodec_decode_video2 video packets with size == 0
> > > 
> > > diff --git a/ffprobe.c b/ffprobe.c
> > > index 79f7494..e23ef98 100644
> > > --- a/ffprobe.c
> > > +++ b/ffprobe.c
> > > @@ -1349,6 +1349,8 @@ static av_always_inline int get_decoded_frame(AVFormatContext *fmt_ctx,
> > >      *got_frame = 0;
> > >      switch (dec_ctx->codec_type) {
> > >      case AVMEDIA_TYPE_VIDEO:
> > > +        if(pkt->size == 0)
> > > +            return ret;
> > >          ret = avcodec_decode_video2(dec_ctx, frame, got_frame, pkt);
> > 
> > This also will break flushing the last frames
> > the check could be moved to the non flushing codepath
> > 
> > [...]
> > 
> 
> Ah, so it will. Amended patch attached. The reason why the check can't
> easily be done in the non-flushing loop is because zero-sized audio packets
> have to be passed through. One could go with just checking the codec_type,
> but that would lead to quite the ugly if-condition. Easier to just have a
> flushing switch to get_decode_frame imo.
> 
> Still, I have not managed to hit this particular segfault with files from
> any other source, so it is likely that something weird is going on, anyway, 
> especially considering the timing problems.
> 
> /P

> commit 95d2777c00ca3f3756aa6090ce900ae5c7b9ca2b
> Author: Petter Ericson <petter.ericson at codemill.se>
> Date:   Tue Feb 14 16:59:56 2012 +0100
> 
>     ffprobe: Do not avcodec_decode_video2 video packets with size == 0 unless flushing
> 
> diff --git a/ffprobe.c b/ffprobe.c
> index 79f7494..b5ca44c 100644
> --- a/ffprobe.c
> +++ b/ffprobe.c
> @@ -1341,7 +1341,7 @@ static void show_frame(WriterContext *w, AVFrame *frame, AVStream *stream)
>  
>  static av_always_inline int get_decoded_frame(AVFormatContext *fmt_ctx,
>                                                AVFrame *frame, int *got_frame,
> -                                              AVPacket *pkt)
> +                                              AVPacket *pkt, int flushing)
>  {
>      AVCodecContext *dec_ctx = fmt_ctx->streams[pkt->stream_index]->codec;
>      int ret = 0;
> @@ -1349,6 +1349,8 @@ static av_always_inline int get_decoded_frame(AVFormatContext *fmt_ctx,
>      *got_frame = 0;
>      switch (dec_ctx->codec_type) {
>      case AVMEDIA_TYPE_VIDEO:
> +        if(!flushing && pkt->size == 0)
> +            return ret;
>          ret = avcodec_decode_video2(dec_ctx, frame, got_frame, pkt);
>          break;
>  
> @@ -1375,7 +1377,7 @@ static void show_packets(WriterContext *w, AVFormatContext *fmt_ctx)
>              pkt1 = pkt;
>              while (1) {
>                  avcodec_get_frame_defaults(&frame);
> -                ret = get_decoded_frame(fmt_ctx, &frame, &got_frame, &pkt1);
> +                ret = get_decoded_frame(fmt_ctx, &frame, &got_frame, &pkt1, 0);
>                  if (ret < 0 || !got_frame)
>                      break;
>                  show_frame(w, &frame, fmt_ctx->streams[pkt.stream_index]);
> @@ -1391,7 +1393,7 @@ static void show_packets(WriterContext *w, AVFormatContext *fmt_ctx)
>      //Flush remaining frames that are cached in the decoder
>      for (i = 0; i < fmt_ctx->nb_streams; i++) {
>          pkt.stream_index = i;
> -        while (get_decoded_frame(fmt_ctx, &frame, &got_frame, &pkt) >= 0 && got_frame)
> +        while (get_decoded_frame(fmt_ctx, &frame, &got_frame, &pkt, 1) >= 0 && got_frame)
>              show_frame(w, &frame, fmt_ctx->streams[pkt.stream_index]);
>      }
>  }

This should be fine, but I'd prefer to fix the crash in the decoder
instead.

Indeed from my reading of the documentation there is nothing wrong
about passing video packets with size of 0, and the expected behavior
should be to just ignore such packets, thus avoiding to complicate
application level code.
-- 
FFmpeg = Forgiving and Fantastic Maxi Picky Elitarian Guide