Re: [FFmpeg-devel] [PATCH 3/3] WIP: lavf/utils: try to avoid decoding a frame to get the codec parameters
On Thu, 29 Oct 2015 08:34:42 +0100
Clément Bœsch wrote:
> On Wed, Oct 28, 2015 at 06:34:06PM +0100, Hendrik Leppkes wrote:
> > On Sat, Oct 17, 2015 at 10:34 PM, Matthieu Bouron
> > wrote:
> > > From: Matthieu Bouron
> > >
> > > Avoid decoding twice images such as jpeg and png, once in the
> > > avformat_find_stream_info and once when the actual decode is made.
> > >
> > > The decoder must honor the skip_frame option in order to skip
> > > decoding. For now the AVDISCARD_ALL flag is only set for the mjpeg and
> > > png decoders.
> > > ---
> > > libavformat/utils.c | 15 +++
> > > 1 file changed, 15 insertions(+)
> > >
> > > diff --git a/libavformat/utils.c b/libavformat/utils.c
> > > index 689473e..67dfffc 100644
> > > --- a/libavformat/utils.c
> > > +++ b/libavformat/utils.c
> > > @@ -2676,11 +2676,16 @@ static int has_codec_parameters(AVStream *st,
> > > const char **errmsg_ptr)
> > > static int try_decode_frame(AVFormatContext *s, AVStream *st, AVPacket
> > > *avpkt,
> > > AVDictionary **options)
> > > {
> > > +int i;
> > > const AVCodec *codec;
> > > int got_picture = 1, ret = 0;
> > > AVFrame *frame = av_frame_alloc();
> > > AVSubtitle subtitle;
> > > AVPacket pkt = *avpkt;
> > > +int skip_frame;
> > > +static const enum AVCodecID no_decode_codecs[] = {
> > > +AV_CODEC_ID_MJPEG, AV_CODEC_ID_PNG,
> > > +};
> >
> > Hardcoded lists of codecs in random places are never a good thing to have.
> > If this is a feature we agree to have, its codecs should just get an
> > internal capability that tells this code if it can parse all params
> > without decoding.
> >
>
> This list is supposed to be temporary (yes I know) until all other
> decoders that currently support AVDISCARD_ALL set the information field
> required for probing.
I do not think this is the right direction. We should go away from
mutating AVCodecContext fields, towards returning all per-frame info in
AVFrame. We should also go away from decoding in libavformat.
Before this becomes final, maybe you could check whether this could be
done in a codec parser instead? Some parsers do set the pixfmt, e.g.
h264. Yes, this suggestion is a bit late. I don't actually mind this
hack being applied in this form; at least it makes it explicit that
it's a hack for the image formats, and can be easily fixed at a later
point.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/3] WIP: lavf/utils: try to avoid decoding a frame to get the codec parameters
On Thu, Oct 29, 2015 at 8:34 AM, Clément Bœsch wrote:
> On Wed, Oct 28, 2015 at 06:34:06PM +0100, Hendrik Leppkes wrote:
>> On Sat, Oct 17, 2015 at 10:34 PM, Matthieu Bouron
>> wrote:
>> > From: Matthieu Bouron
>> >
>> > Avoid decoding twice images such as jpeg and png, once in the
>> > avformat_find_stream_info and once when the actual decode is made.
>> >
>> > The decoder must honor the skip_frame option in order to skip
>> > decoding. For now the AVDISCARD_ALL flag is only set for the mjpeg and
>> > png decoders.
>> > ---
>> > libavformat/utils.c | 15 +++
>> > 1 file changed, 15 insertions(+)
>> >
>> > diff --git a/libavformat/utils.c b/libavformat/utils.c
>> > index 689473e..67dfffc 100644
>> > --- a/libavformat/utils.c
>> > +++ b/libavformat/utils.c
>> > @@ -2676,11 +2676,16 @@ static int has_codec_parameters(AVStream *st,
>> > const char **errmsg_ptr)
>> > static int try_decode_frame(AVFormatContext *s, AVStream *st, AVPacket
>> > *avpkt,
>> > AVDictionary **options)
>> > {
>> > +int i;
>> > const AVCodec *codec;
>> > int got_picture = 1, ret = 0;
>> > AVFrame *frame = av_frame_alloc();
>> > AVSubtitle subtitle;
>> > AVPacket pkt = *avpkt;
>> > +int skip_frame;
>> > +static const enum AVCodecID no_decode_codecs[] = {
>> > +AV_CODEC_ID_MJPEG, AV_CODEC_ID_PNG,
>> > +};
>>
>> Hardcoded lists of codecs in random places are never a good thing to have.
>> If this is a feature we agree to have, its codecs should just get an
>> internal capability that tells this code if it can parse all params
>> without decoding.
>>
>
> This list is supposed to be temporary (yes I know) until all other
> decoders that currently support AVDISCARD_ALL set the information field
> required for probing.
>
We all know what happens to temporary solutions, don't we. :)
Rather have a temporary internal capability, its not part of the
API/ABI or anything, but avoids such an ugly list inside generic code.
- Hendrik
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/3] WIP: lavf/utils: try to avoid decoding a frame to get the codec parameters
On Thu, Oct 29, 2015 at 08:34:42AM +0100, Clément Bœsch wrote:
> On Wed, Oct 28, 2015 at 06:34:06PM +0100, Hendrik Leppkes wrote:
> > On Sat, Oct 17, 2015 at 10:34 PM, Matthieu Bouron
> > wrote:
> > > From: Matthieu Bouron
> > >
> > > Avoid decoding twice images such as jpeg and png, once in the
> > > avformat_find_stream_info and once when the actual decode is made.
> > >
> > > The decoder must honor the skip_frame option in order to skip
> > > decoding. For now the AVDISCARD_ALL flag is only set for the mjpeg and
> > > png decoders.
> > > ---
> > > libavformat/utils.c | 15 +++
> > > 1 file changed, 15 insertions(+)
> > >
> > > diff --git a/libavformat/utils.c b/libavformat/utils.c
> > > index 689473e..67dfffc 100644
> > > --- a/libavformat/utils.c
> > > +++ b/libavformat/utils.c
> > > @@ -2676,11 +2676,16 @@ static int has_codec_parameters(AVStream *st,
> > > const char **errmsg_ptr)
> > > static int try_decode_frame(AVFormatContext *s, AVStream *st, AVPacket
> > > *avpkt,
> > > AVDictionary **options)
> > > {
> > > +int i;
> > > const AVCodec *codec;
> > > int got_picture = 1, ret = 0;
> > > AVFrame *frame = av_frame_alloc();
> > > AVSubtitle subtitle;
> > > AVPacket pkt = *avpkt;
> > > +int skip_frame;
> > > +static const enum AVCodecID no_decode_codecs[] = {
> > > +AV_CODEC_ID_MJPEG, AV_CODEC_ID_PNG,
> > > +};
> >
> > Hardcoded lists of codecs in random places are never a good thing to have.
> > If this is a feature we agree to have, its codecs should just get an
> > internal capability that tells this code if it can parse all params
> > without decoding.
> >
>
> This list is supposed to be temporary (yes I know) until all other
> decoders that currently support AVDISCARD_ALL set the information field
> required for probing.
>
> Not full frames decoding at probing but just filling the information will
> also be useful for these codecs (I think there are about 20?).
> Unfortunately, the changes are sensible so it's probably better to do it
> progressively one by one, updating this white list progressively. A
> blacklist system was also suggested earlier in the thread.
>
> Maybe a FIXME/XXX should be added above? (And maybe even with a compiler
> warning to not forget this?)
>
And forgot to say: if you choose to add a capability, it will have to be
dropped when the other decoders are updated to match the behaviour.
--
Clément B.
signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/3] WIP: lavf/utils: try to avoid decoding a frame to get the codec parameters
On Wed, Oct 28, 2015 at 06:34:06PM +0100, Hendrik Leppkes wrote:
> On Sat, Oct 17, 2015 at 10:34 PM, Matthieu Bouron
> wrote:
> > From: Matthieu Bouron
> >
> > Avoid decoding twice images such as jpeg and png, once in the
> > avformat_find_stream_info and once when the actual decode is made.
> >
> > The decoder must honor the skip_frame option in order to skip
> > decoding. For now the AVDISCARD_ALL flag is only set for the mjpeg and
> > png decoders.
> > ---
> > libavformat/utils.c | 15 +++
> > 1 file changed, 15 insertions(+)
> >
> > diff --git a/libavformat/utils.c b/libavformat/utils.c
> > index 689473e..67dfffc 100644
> > --- a/libavformat/utils.c
> > +++ b/libavformat/utils.c
> > @@ -2676,11 +2676,16 @@ static int has_codec_parameters(AVStream *st, const
> > char **errmsg_ptr)
> > static int try_decode_frame(AVFormatContext *s, AVStream *st, AVPacket
> > *avpkt,
> > AVDictionary **options)
> > {
> > +int i;
> > const AVCodec *codec;
> > int got_picture = 1, ret = 0;
> > AVFrame *frame = av_frame_alloc();
> > AVSubtitle subtitle;
> > AVPacket pkt = *avpkt;
> > +int skip_frame;
> > +static const enum AVCodecID no_decode_codecs[] = {
> > +AV_CODEC_ID_MJPEG, AV_CODEC_ID_PNG,
> > +};
>
> Hardcoded lists of codecs in random places are never a good thing to have.
> If this is a feature we agree to have, its codecs should just get an
> internal capability that tells this code if it can parse all params
> without decoding.
>
This list is supposed to be temporary (yes I know) until all other
decoders that currently support AVDISCARD_ALL set the information field
required for probing.
Not full frames decoding at probing but just filling the information will
also be useful for these codecs (I think there are about 20?).
Unfortunately, the changes are sensible so it's probably better to do it
progressively one by one, updating this white list progressively. A
blacklist system was also suggested earlier in the thread.
Maybe a FIXME/XXX should be added above? (And maybe even with a compiler
warning to not forget this?)
[...]
--
Clément B.
signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/3] WIP: lavf/utils: try to avoid decoding a frame to get the codec parameters
On Wed, 28 Oct 2015 13:30:59 -0400
"Ronald S. Bultje" wrote:
> Hi,
>
> On Sat, Oct 17, 2015 at 4:34 PM, Matthieu Bouron
> wrote:
>
> > From: Matthieu Bouron
> >
> > Avoid decoding twice images such as jpeg and png, once in the
> > avformat_find_stream_info and once when the actual decode is made.
> >
> > The decoder must honor the skip_frame option in order to skip
> > decoding. For now the AVDISCARD_ALL flag is only set for the mjpeg and
> > png decoders.
> > ---
> > libavformat/utils.c | 15 +++
> > 1 file changed, 15 insertions(+)
> >
> > diff --git a/libavformat/utils.c b/libavformat/utils.c
> > index 689473e..67dfffc 100644
> > --- a/libavformat/utils.c
> > +++ b/libavformat/utils.c
> > @@ -2676,11 +2676,16 @@ static int has_codec_parameters(AVStream *st,
> > const char **errmsg_ptr)
> > static int try_decode_frame(AVFormatContext *s, AVStream *st, AVPacket
> > *avpkt,
> > AVDictionary **options)
> > {
> > +int i;
> > const AVCodec *codec;
> > int got_picture = 1, ret = 0;
> > AVFrame *frame = av_frame_alloc();
> > AVSubtitle subtitle;
> > AVPacket pkt = *avpkt;
> > +int skip_frame;
> > +static const enum AVCodecID no_decode_codecs[] = {
> > +AV_CODEC_ID_MJPEG, AV_CODEC_ID_PNG,
> > +};
> >
> > if (!frame)
> > return AVERROR(ENOMEM);
> > @@ -2719,6 +2724,14 @@ static int try_decode_frame(AVFormatContext *s,
> > AVStream *st, AVPacket *avpkt,
> > goto fail;
> > }
> >
> > +skip_frame = st->codec->skip_frame;
> > +for (i = 0; i < FF_ARRAY_ELEMS(no_decode_codecs); i++) {
> > +if (st->codec->codec_id == no_decode_codecs[i]) {
> > +st->codec->skip_frame = AVDISCARD_ALL;
> > +break;
> > +}
> > +}
>
>
> I tend to find this kind of specific code in general interfaces fairly
> questionable. Why is this necessary?
The intention of this patch is to make these image codecs discard the
frame data (and not to decompress in the first frame), but still have
them set the parameters of the discarded frame.
It's a very ugly hack to make libavformat set the pixfmt field in the
AVStream codec contexts, without decoding image data. (Which would be
huge and take long.)
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/3] WIP: lavf/utils: try to avoid decoding a frame to get the codec parameters
On Sat, Oct 17, 2015 at 10:34 PM, Matthieu Bouron
wrote:
> From: Matthieu Bouron
>
> Avoid decoding twice images such as jpeg and png, once in the
> avformat_find_stream_info and once when the actual decode is made.
>
> The decoder must honor the skip_frame option in order to skip
> decoding. For now the AVDISCARD_ALL flag is only set for the mjpeg and
> png decoders.
> ---
> libavformat/utils.c | 15 +++
> 1 file changed, 15 insertions(+)
>
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index 689473e..67dfffc 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -2676,11 +2676,16 @@ static int has_codec_parameters(AVStream *st, const
> char **errmsg_ptr)
> static int try_decode_frame(AVFormatContext *s, AVStream *st, AVPacket
> *avpkt,
> AVDictionary **options)
> {
> +int i;
> const AVCodec *codec;
> int got_picture = 1, ret = 0;
> AVFrame *frame = av_frame_alloc();
> AVSubtitle subtitle;
> AVPacket pkt = *avpkt;
> +int skip_frame;
> +static const enum AVCodecID no_decode_codecs[] = {
> +AV_CODEC_ID_MJPEG, AV_CODEC_ID_PNG,
> +};
Hardcoded lists of codecs in random places are never a good thing to have.
If this is a feature we agree to have, its codecs should just get an
internal capability that tells this code if it can parse all params
without decoding.
>
> if (!frame)
> return AVERROR(ENOMEM);
> @@ -2719,6 +2724,14 @@ static int try_decode_frame(AVFormatContext *s,
> AVStream *st, AVPacket *avpkt,
> goto fail;
> }
>
> +skip_frame = st->codec->skip_frame;
> +for (i = 0; i < FF_ARRAY_ELEMS(no_decode_codecs); i++) {
> +if (st->codec->codec_id == no_decode_codecs[i]) {
> +st->codec->skip_frame = AVDISCARD_ALL;
> +break;
> +}
> +}
> +
> while ((pkt.size > 0 || (!pkt.data && got_picture)) &&
> ret >= 0 &&
> (!has_codec_parameters(st, NULL) ||
> !has_decode_delay_been_guessed(st) ||
> @@ -2753,6 +2766,8 @@ static int try_decode_frame(AVFormatContext *s,
> AVStream *st, AVPacket *avpkt,
> if (!pkt.data && !got_picture)
> ret = -1;
>
> +st->codec->skip_frame = skip_frame;
> +
> fail:
> av_frame_free(&frame);
> return ret;
> --
> 2.6.1
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/3] WIP: lavf/utils: try to avoid decoding a frame to get the codec parameters
Hi,
On Sat, Oct 17, 2015 at 4:34 PM, Matthieu Bouron
wrote:
> From: Matthieu Bouron
>
> Avoid decoding twice images such as jpeg and png, once in the
> avformat_find_stream_info and once when the actual decode is made.
>
> The decoder must honor the skip_frame option in order to skip
> decoding. For now the AVDISCARD_ALL flag is only set for the mjpeg and
> png decoders.
> ---
> libavformat/utils.c | 15 +++
> 1 file changed, 15 insertions(+)
>
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index 689473e..67dfffc 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -2676,11 +2676,16 @@ static int has_codec_parameters(AVStream *st,
> const char **errmsg_ptr)
> static int try_decode_frame(AVFormatContext *s, AVStream *st, AVPacket
> *avpkt,
> AVDictionary **options)
> {
> +int i;
> const AVCodec *codec;
> int got_picture = 1, ret = 0;
> AVFrame *frame = av_frame_alloc();
> AVSubtitle subtitle;
> AVPacket pkt = *avpkt;
> +int skip_frame;
> +static const enum AVCodecID no_decode_codecs[] = {
> +AV_CODEC_ID_MJPEG, AV_CODEC_ID_PNG,
> +};
>
> if (!frame)
> return AVERROR(ENOMEM);
> @@ -2719,6 +2724,14 @@ static int try_decode_frame(AVFormatContext *s,
> AVStream *st, AVPacket *avpkt,
> goto fail;
> }
>
> +skip_frame = st->codec->skip_frame;
> +for (i = 0; i < FF_ARRAY_ELEMS(no_decode_codecs); i++) {
> +if (st->codec->codec_id == no_decode_codecs[i]) {
> +st->codec->skip_frame = AVDISCARD_ALL;
> +break;
> +}
> +}
I tend to find this kind of specific code in general interfaces fairly
questionable. Why is this necessary?
Ronald
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/3] WIP: lavf/utils: try to avoid decoding a frame to get the codec parameters
On Sat, Oct 17, 2015 at 10:34:24PM +0200, Matthieu Bouron wrote:
> From: Matthieu Bouron
>
> Avoid decoding twice images such as jpeg and png, once in the
> avformat_find_stream_info and once when the actual decode is made.
>
> The decoder must honor the skip_frame option in order to skip
> decoding. For now the AVDISCARD_ALL flag is only set for the mjpeg and
> png decoders.
> ---
> libavformat/utils.c | 15 +++
> 1 file changed, 15 insertions(+)
>
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index 689473e..67dfffc 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -2676,11 +2676,16 @@ static int has_codec_parameters(AVStream *st, const
> char **errmsg_ptr)
> static int try_decode_frame(AVFormatContext *s, AVStream *st, AVPacket
> *avpkt,
> AVDictionary **options)
> {
> +int i;
> const AVCodec *codec;
> int got_picture = 1, ret = 0;
> AVFrame *frame = av_frame_alloc();
> AVSubtitle subtitle;
> AVPacket pkt = *avpkt;
> +int skip_frame;
> +static const enum AVCodecID no_decode_codecs[] = {
> +AV_CODEC_ID_MJPEG, AV_CODEC_ID_PNG,
> +};
>
> if (!frame)
> return AVERROR(ENOMEM);
> @@ -2719,6 +2724,14 @@ static int try_decode_frame(AVFormatContext *s,
> AVStream *st, AVPacket *avpkt,
> goto fail;
> }
>
> +skip_frame = st->codec->skip_frame;
> +for (i = 0; i < FF_ARRAY_ELEMS(no_decode_codecs); i++) {
> +if (st->codec->codec_id == no_decode_codecs[i]) {
> +st->codec->skip_frame = AVDISCARD_ALL;
> +break;
> +}
> +}
> +
> while ((pkt.size > 0 || (!pkt.data && got_picture)) &&
> ret >= 0 &&
> (!has_codec_parameters(st, NULL) ||
> !has_decode_delay_been_guessed(st) ||
> @@ -2753,6 +2766,8 @@ static int try_decode_frame(AVFormatContext *s,
> AVStream *st, AVPacket *avpkt,
> if (!pkt.data && !got_picture)
> ret = -1;
>
> +st->codec->skip_frame = skip_frame;
iam slightly unhappy about the possibility that someone could add
a if(whatever) goto fail between these 2 and it would leave
skip_frame in the modified state
otherwise LGTM
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato
signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
