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

[Michael Niedermayer]

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?

"fill" could fail until some sequence header or keyframe is encountered

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

Observe your enemies, for they first find out your faults. -- Antisthenes
-------------- 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/20151116/e8a5b789/attachment.sig>