[FFmpeg-devel] [PATCH 2/4] lavf/utils: avoid decoding a frame to get the codec parameters

[Ronald S. Bultje]

Hi,

On Sun, Nov 15, 2015 at 4:49 AM, Matthieu Bouron <matthieu.bouron at gmail.com>
wrote:

> On Mon, Nov 02, 2015 at 07:56:50AM -0500, Ronald S. Bultje wrote:
> > Hi,
> >
> > On Mon, Nov 2, 2015 at 5:45 AM, Matthieu Bouron <
> matthieu.bouron at gmail.com>
> > wrote:
> >
> > > From: Matthieu Bouron <matthieu.bouron at stupeflix.com>
> > >
> > > Avoid decoding a frame to get the codec parameters while the codec
> > > supports FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM. This is particulary useful
> > > to avoid decoding twice images (once in avformat_find_stream_info and
> > > once when the actual decode is made).
> > > ---
> > >  libavformat/utils.c | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > >
> > > diff --git a/libavformat/utils.c b/libavformat/utils.c
> > > index 5c4d452..ba62f2b 100644
> > > --- a/libavformat/utils.c
> > > +++ b/libavformat/utils.c
> > > @@ -2695,6 +2695,8 @@ static int try_decode_frame(AVFormatContext *s,
> > > AVStream *st, AVPacket *avpkt,
> > >      AVFrame *frame = av_frame_alloc();
> > >      AVSubtitle subtitle;
> > >      AVPacket pkt = *avpkt;
> > > +    int do_skip_frame = 0;
> > > +    enum AVDiscard skip_frame;
> > >
> > >      if (!frame)
> > >          return AVERROR(ENOMEM);
> > > @@ -2733,6 +2735,12 @@ static int try_decode_frame(AVFormatContext *s,
> > > AVStream *st, AVPacket *avpkt,
> > >          goto fail;
> > >      }
> > >
> > > +    if (st->codec->codec->caps_internal &
> > > FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM) {
> > > +        do_skip_frame = 1;
> > > +        skip_frame = st->codec->skip_frame;
> > > +        st->codec->skip_frame = AVDISCARD_ALL;
> > > +    }
> > > +
> > >      while ((pkt.size > 0 || (!pkt.data && got_picture)) &&
> > >             ret >= 0 &&
> > >             (!has_codec_parameters(st, NULL) ||
> > > !has_decode_delay_been_guessed(st) ||
> > > @@ -2768,6 +2776,10 @@ static int try_decode_frame(AVFormatContext *s,
> > > AVStream *st, AVPacket *avpkt,
> > >          ret = -1;
> > >
> > >  fail:
> > > +    if (do_skip_frame) {
> > > +        st->codec->skip_frame = skip_frame;
> > > +    }
> > > +
> > >      av_frame_free(&frame);
> > >      return ret;
> > >  }
> > > --
> > > 2.6.2
> >
> >
> > I think we need an assert in the try_decode loop to ensure that it indeed
> > did fill all the params. This is to prevent the case where someone adds a
> > new "thing" to the list of things required for find_stream_info to
> succeed,
> > and forgets to update the codecs.
>
> While the codec_has_parameters function fits that role, it does not check
> all codec parameters as they can be codec dependant.
>
> I'm not sure if we can create a generic rule here for the same reason as
> above, as the parameters set can be codec specific and maybe stream
> specific.
>
> Having some fate test to cover this area seems to be another option but
> would
> require to inspect all the relevant parameters of AVCodecContext depending
> on the codec and a lot of different streams.


I don't care about _which_ parameters were filled. The goal of this option
is only directly to fill parameters, but the purpose goes far beyond that.
The indirect goal of this change is to _not_ call try_decode_frame() for
certain image codecs. Whether they do that by dancing on the table or by
filling AVCodecContext parameters when a special flag is set, is merely an
implementation detail.

I want the test to confirm that we did not call try_decode_frame() when the
skip-flag was set. It seems easiest to me that we check that by adding a
one-line assert, for example inside try_decode_frame, that checks that the
flag does not apply to this codec, right? If the assert triggers, clearly
something broke, and either the flag should be removed, or the check before
starting try_decode_frame fixed.

Ronald