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 <matthieu.bou...@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
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
