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

2015-11-26 Thread 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 
> > > >  > > > > 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.bou...@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.bou...@gmail.com>
> > > > > > > > wrote:
> > > > > > > >
> > > > > > > > > From: Matthieu Bouron 
> > > > > > > > >
> > > > > > > > > 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();
> > > > > > > > >  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

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

2015-11-26 Thread Matthieu Bouron 
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  > > 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.bou...@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.bou...@gmail.com>
> > > > > > wrote:
> > > > > >
> > > > > > > From: Matthieu Bouron 
> > > > > > >
> > > > > > > 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();
> > > > > > >  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
> > > >

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

2015-11-26 Thread 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 
> > >  > > > 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.bou...@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.bou...@gmail.com>
> > > > > > > wrote:
> > > > > > >
> > > > > > > > From: Matthieu Bouron 
> > > > > > > >
> > > > > > > > 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();
> > > > > > > >  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

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

2015-11-26 Thread Ronald S. Bultje 
Hi,

On Thu, Nov 26, 2015 at 11:16 AM, 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.bou...@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.bou...@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.bou...@gmail.com>
> > > > > > > wrote:
> > > > > > >
> > > > > > > > From: Matthieu Bouron 
> > > > > > > >
> > > > > > > > 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();
> > > > > > > >  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
> >

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

2015-11-19 Thread Matthieu Bouron 
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  > 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.bou...@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.bou...@gmail.com>
> > > > > wrote:
> > > > >
> > > > > > From: Matthieu Bouron 
> > > > > >
> > > > > > 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();
> > > > > >  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

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

2015-11-16 Thread Matthieu Bouron 
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 
> 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.bou...@gmail.com>
> > > wrote:
> > >
> > > > From: Matthieu Bouron 
> > > >
> > > > 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();
> > > >  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.

Matthieu
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2015-11-16 Thread Ronald S. Bultje 
Hi,

On Mon, Nov 16, 2015 at 11:06 AM, Matthieu Bouron  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.bou...@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.bou...@gmail.com>
> > > > wrote:
> > > >
> > > > > From: Matthieu Bouron 
> > > > >
> > > > > 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();
> > > > >  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

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

2015-11-16 Thread 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  > 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.bou...@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.bou...@gmail.com>
> > > > > wrote:
> > > > >
> > > > > > From: Matthieu Bouron 
> > > > > >
> > > > > > 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();
> > > > > >  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

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

2015-11-15 Thread Matthieu Bouron 
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 
> wrote:
> 
> > From: Matthieu Bouron 
> >
> > 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();
> >  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.

What do you think of this approach ?

Matthieu

[...]
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2015-11-15 Thread Ronald S. Bultje 
Hi,

On Sun, Nov 15, 2015 at 4:49 AM, Matthieu Bouron 
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.bou...@gmail.com>
> > wrote:
> >
> > > From: Matthieu Bouron 
> > >
> > > 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();
> > >  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.

Ronald
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2015-11-02 Thread Ronald S. Bultje 
Hi,

On Mon, Nov 2, 2015 at 5:45 AM, Matthieu Bouron 
wrote:

> From: Matthieu Bouron 
>
> 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();
>  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.

(These features break easily and subtly, and ideally we'd detect such
breakage in fate, not 6 months after the next release.)

Ronald
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2015-11-02 Thread Matthieu Bouron 
From: Matthieu Bouron 

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();
 return ret;
 }
-- 
2.6.2

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel