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

[Petter Ericson]

On Mon, Feb 20, 2012 at 06:00:29PM +0100, Stefano Sabatini wrote:
> 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.

I agree that this would be ideal, but I am not familiar enough with the
relevant decoders and standards to feel comfortable in submitting a fix for
that.. And ffmpeg.c does not decode zero-sized packets unless flushing,
which is why the segfault does not appear when transcoding.

I will take another look at the problem, though, and see if I can find a
solution in the decoder instead.

/P