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

[Ronald S. Bultje]

Hi,

On Mon, Nov 16, 2015 at 11:06 AM, Matthieu Bouron <matthieu.bouron at gmail.com
> wrote:

> On Sun, Nov 15, 2015 at 08:12:57AM -0500, Ronald S. Bultje wrote:
> > Hi,
>
> 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.
>
> The try_decode_frame function still need to be executed even if the
> decoder supports FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM as it still
> need to parse the first frame to set the relevant parameters.
>
> The flag only says that the decoder still do the actual parsing even if
> the frame is discarded due to the skip_frame option.


Oh right, so I guess the assert should then say that after 1 frame (or 2,
or whatever), the try_decode_loop actually succeeded?

I mean, you' see what I'm trying to get at right? I'm trying to get you to
add a check to make sure that this flag does not undetectably break after a
few months/years.

Ronald