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

Ronald S. Bultje rsbultje at gmail.com
Thu Nov 26 20:23:02 CET 2015


Hi,

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

> On Thu, Nov 19, 2015 at 12:10:20PM +0100, Matthieu Bouron wrote:
> > On Mon, Nov 16, 2015 at 11:16:42AM -0500, Ronald S. Bultje wrote:
> > > 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.
> >
> > I see your point but I still fail to see how I could add an
> > assert here (even though I totally agree with you adding this kind of
> > check) as the decoder could fail to fill the parameters even if the
> > skip_frame option is not used (the codec_has_parameters check will
> > handle that case).
> >
> > Sorry to repeat myself but the only way I see to check for regressions
> > here is to add some test in fate.
> >
>
> There is now fate tests related to this feature. Are you OK if I push the
> patch ?


Yes from me also.

(I'll admit I didn't actually look at the test yet, sorry about that, but
I'm going to assume it's OK and will look at it soon. Thanks for working on
this!)

Ronald


More information about the ffmpeg-devel mailing list