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

[Matthieu Bouron]

On Thu, Nov 26, 2015 at 08:08:25PM +0100, Michael Niedermayer wrote:
> On Thu, Nov 26, 2015 at 05:16:32PM +0100, Matthieu Bouron 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

Pushed, thanks.

Matthieu

[...]