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

[Michael Niedermayer]

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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

When the tyrant has disposed of foreign enemies by conquest or treaty, and
there is nothing more to fear from them, then he is always stirring up
some war or other, in order that the people may require a leader. -- Plato
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20151126/0b4d731c/attachment.sig>