| Message ID | d3a62921-3524-d84f-aeed-de11eca7dbe0@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Thu, 19 Apr 2018 12:15:07 +0200 Martin Dørum <martid0311@gmail.com> wrote: > When a program uses FFmpeg to decode or encode media, and uses > `avcodec_find_decoder` or `avcodec_find_encoder`, I interpret that as > the program not caring a whole lot what AVCodec is used; it just wants > something which can encode or decode the desired format. I think it > makes sense for the user to be able to specify what codec should be the > default in that case, because the user might know that they're on a > system with a fast hardware encoder or decoder. Currently, the only way > to convince FFmpeg to default to a different codec is to recompile it > without support for other codecs than the desired one, which feels > unnecessary. Obviously a program can do its own selection, and some programs do this. > An argument against this patch could be that it's the application's > responsibility to add an -encoder or -decoder option like the ffmpeg > CLI utilities do. If that's the "official" stance of FFmpeg, I'm fine > with that, even though I don't necessarily agree. It probably is. The main problem is actually that we have something against environment variables. The only cases in which the libs read and use env vars are when it's a broadly used standard convention (like http proxy override), or other special cases. I don't think we should add new ones. > Anotehr point is that some applications make assumptions about the > pix_fmt the decoder they get from `avcodec_find_decoder` uses. Chromium > does this. I believe this is a problem with those applications > (and I submitted an issue to the Chromium project: > https://bugs.chromium.org/p/webrtc/issues/detail?id=9137#c2), but it > should be considered. That's true, and in some special cases you don't have much of a choice. But in most cases (including this one) the application should not expect anything, because: 1. certain files might use different output parameters (e.g. 10 bit h264 files) 2. the output parameters could change if codec internals change (happens sometimes if optimizations or new features are added to a decoder) > (I also haven't submitted a patch to FFmpeg before, or any open-source > project which doesn't use GitHub pull requests, so I apologize if I > have followed the process incorrectly.) Looks ok. One issues is if you copy & pasted the git patch into your email client, it can happen that the patch gets corrupted, so either attaching the patch as text file, or using git send-email is preferred. Regarding this patch, personally I don't think using getenv() to configure what is pretty much API semantics is acceptable. But a new API function that restricts what codecs are used based on a string argument might be ok. Then applications could use it to implement codec selection control via environment or command line arguments or something else. > This patch makes it possible to choose what codec avcodec_find_encoder > and avcodec_find_decoder returns with environment variables. > FFMPEG_PREFER_CODEC is an environment variable with a comma-separated > list of codec names, and if it contains the name of an applicable > codec, that codec will be returned instead of whatever av_codec_iterate > happens to return first. > > There's also FFMPEG_PREFER_ENCODER and FFMPEG_PREFER_DECODER, which > specifically apply to avcodec_find_encoder and avcodec_find_decoder > respectively. These are prioritized above FFMPEG_PREFER_CODEC. > > Signed-off-by: Martin Dørum <martid0311@gmail.com> > --- > libavcodec/allcodecs.c | 59 ++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 54 insertions(+), 5 deletions(-) > > diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c > index 4d4ef530e4..3c85908969 100644 > --- a/libavcodec/allcodecs.c > +++ b/libavcodec/allcodecs.c > @@ -833,25 +833,74 @@ static enum AVCodecID remap_deprecated_codec_id(enum AVCodecID id) > } > } > > +static int codec_matches_preferred(const AVCodec *p, const char *str) > +{ > + int pi = 0, stri = 0; > + int match = 1; > + > + if (str == NULL) > + return 0; > + > + while (1) { > + if (str[stri] == '\0' || str[stri] == ',') { > + if (match && p->name[pi] == '\0') { > + return 1; > + } else if (str[stri] == '\0') { > + return 0; > + } else { > + match = 1; > + pi = 0; > + } > + } else if (p->name[pi] == '\0') { > + match = 0; > + } else { > + if (match && p->name[pi] != str[stri]) > + match = 0; > + pi += 1; > + } > + > + stri += 1; > + } > +} > + > static AVCodec *find_codec(enum AVCodecID id, int (*x)(const AVCodec *)) > { > - const AVCodec *p, *experimental = NULL; > + const AVCodec *p, *experimental = NULL, *fallback = NULL, *preferred = NULL; > void *i = 0; > > + char *preferred_codec = getenv("FFMPEG_PREFER_CODEC"); > + char *preferred_encdec = NULL; > + if (x == av_codec_is_encoder) > + preferred_encdec = getenv("FFMPEG_PREFER_ENCODER"); > + else if (x == av_codec_is_decoder) > + preferred_encdec = getenv("FFMPEG_PREFER_DECODER"); > + > id = remap_deprecated_codec_id(id); > > while ((p = av_codec_iterate(&i))) { > if (!x(p)) > continue; > if (p->id == id) { > - if (p->capabilities & AV_CODEC_CAP_EXPERIMENTAL && !experimental) { > - experimental = p; > - } else > + if (codec_matches_preferred(p, preferred_encdec)) { > return (AVCodec*)p; > + } else if (codec_matches_preferred(p, preferred_codec) && !preferred) { > + preferred = p; > + } else if (p->capabilities & AV_CODEC_CAP_EXPERIMENTAL && !experimental) { > + experimental = p; > + } else if (!fallback) { > + fallback = p; > + } > } > } > > - return (AVCodec*)experimental; > + if (preferred) > + return (AVCodec*)preferred; > + else if (fallback) > + return (AVCodec*)fallback; > + else if (experimental) > + return (AVCodec*)experimental; > + else > + return NULL; > } > > AVCodec *avcodec_find_encoder(enum AVCodecID id)
On 4/19/2018 11:40 AM, wm4 wrote: > Regarding this patch, personally I don't think using getenv() to > configure what is pretty much API semantics is acceptable. But a new > API function that restricts what codecs are used based on a string > argument might be ok. Then applications could use it to implement > codec selection control via environment or command line arguments or > something else. +1 no getenv - Derek
On Thu, Apr 19, 2018 at 12:40 PM, wm4 <nfxjfg@googlemail.com> wrote: > > Regarding this patch, personally I don't think using getenv() to > configure what is pretty much API semantics is acceptable. But a new > API function that restricts what codecs are used based on a string > argument might be ok. Then applications could use it to implement > codec selection control via environment or command line arguments or > something else. > > I agree with no getenv, handling env variables in a library is an iffy concept. A new API for that seems pretty weird as well, considering we already have functions to select codecs by name, which can be easily used for this purpose - the same way ffmpeg.c for example implements this. And if you need code changes anyway, might as well do it the way its intended. - Hendrik
On 4/19/2018 11:19 AM, Derek Buitenhuis wrote: > On 4/19/2018 11:40 AM, wm4 wrote: >> Regarding this patch, personally I don't think using getenv() to >> configure what is pretty much API semantics is acceptable. But a new >> API function that restricts what codecs are used based on a string >> argument might be ok. Then applications could use it to implement >> codec selection control via environment or command line arguments or >> something else. > > +1 no getenv We've rejected other patches for this same reason before, so yeah, no changes that depend on environment variables.
On Thu, 19 Apr 2018 16:33:09 +0200 Hendrik Leppkes <h.leppkes@gmail.com> wrote: > On Thu, Apr 19, 2018 at 12:40 PM, wm4 <nfxjfg@googlemail.com> wrote: > > > > Regarding this patch, personally I don't think using getenv() to > > configure what is pretty much API semantics is acceptable. But a new > > API function that restricts what codecs are used based on a string > > argument might be ok. Then applications could use it to implement > > codec selection control via environment or command line arguments or > > something else. > > > > > > I agree with no getenv, handling env variables in a library is an iffy concept. > > A new API for that seems pretty weird as well, considering we already > have functions to select codecs by name, which can be easily used for > this purpose - the same way ffmpeg.c for example implements this. > And if you need code changes anyway, might as well do it the way its intended. I meant an API that can select the correct codec given a list of decoders/encoders, and the actual codec. A codec list could for example contain "h264,hevc,...", and of course you always want to pick "hevc" if the input is HEVC. I don't think we have an API for this yet?
On Thu, Apr 19, 2018 at 4:45 PM, wm4 <nfxjfg@googlemail.com> wrote: > On Thu, 19 Apr 2018 16:33:09 +0200 > Hendrik Leppkes <h.leppkes@gmail.com> wrote: > >> On Thu, Apr 19, 2018 at 12:40 PM, wm4 <nfxjfg@googlemail.com> wrote: >> > >> > Regarding this patch, personally I don't think using getenv() to >> > configure what is pretty much API semantics is acceptable. But a new >> > API function that restricts what codecs are used based on a string >> > argument might be ok. Then applications could use it to implement >> > codec selection control via environment or command line arguments or >> > something else. >> > >> > >> >> I agree with no getenv, handling env variables in a library is an iffy concept. >> >> A new API for that seems pretty weird as well, considering we already >> have functions to select codecs by name, which can be easily used for >> this purpose - the same way ffmpeg.c for example implements this. >> And if you need code changes anyway, might as well do it the way its intended. > > I meant an API that can select the correct codec given a list of > decoders/encoders, and the actual codec. A codec list could for example > contain "h264,hevc,...", and of course you always want to pick "hevc" > if the input is HEVC. I don't think we have an API for this yet? We don't, but I don't think it adds a whole lot of value, you basically just save a tiny bit of lookup code yourself. But its a tiny function either way, so if someone wants to make the case for its usefulness, not stopping you. - Hendrik
2018-04-19 12:15 GMT+02:00, Martin Dørum <martid0311@gmail.com>: > Anotehr point is that some applications make assumptions about the > pix_fmt the decoder they get from `avcodec_find_decoder` uses. (Unrelated to your patch afaict) This assumption is invalid, FFmpeg has changed pix_fmt (and sample_fmt, the same is true for audio) for decoders before and may change it again in the future. (Iirc, it was changed without any version bump in the past.) Carl Eugen
diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c index 4d4ef530e4..3c85908969 100644 --- a/libavcodec/allcodecs.c +++ b/libavcodec/allcodecs.c @@ -833,25 +833,74 @@ static enum AVCodecID remap_deprecated_codec_id(enum AVCodecID id) } } +static int codec_matches_preferred(const AVCodec *p, const char *str) +{ + int pi = 0, stri = 0; + int match = 1; + + if (str == NULL) + return 0; + + while (1) { + if (str[stri] == '\0' || str[stri] == ',') { + if (match && p->name[pi] == '\0') { + return 1; + } else if (str[stri] == '\0') { + return 0; + } else { + match = 1; + pi = 0; + } + } else if (p->name[pi] == '\0') { + match = 0; + } else { + if (match && p->name[pi] != str[stri]) + match = 0; + pi += 1; + } + + stri += 1; + } +} + static AVCodec *find_codec(enum AVCodecID id, int (*x)(const AVCodec *)) { - const AVCodec *p, *experimental = NULL; + const AVCodec *p, *experimental = NULL, *fallback = NULL, *preferred = NULL; void *i = 0; + char *preferred_codec = getenv("FFMPEG_PREFER_CODEC"); + char *preferred_encdec = NULL; + if (x == av_codec_is_encoder) + preferred_encdec = getenv("FFMPEG_PREFER_ENCODER"); + else if (x == av_codec_is_decoder) + preferred_encdec = getenv("FFMPEG_PREFER_DECODER"); + id = remap_deprecated_codec_id(id); while ((p = av_codec_iterate(&i))) { if (!x(p)) continue; if (p->id == id) { - if (p->capabilities & AV_CODEC_CAP_EXPERIMENTAL && !experimental) { - experimental = p; - } else + if (codec_matches_preferred(p, preferred_encdec)) { return (AVCodec*)p; + } else if (codec_matches_preferred(p, preferred_codec) && !preferred) { + preferred = p; + } else if (p->capabilities & AV_CODEC_CAP_EXPERIMENTAL && !experimental) { + experimental = p; + } else if (!fallback) { + fallback = p; + } } } - return (AVCodec*)experimental; + if (preferred) + return (AVCodec*)preferred; + else if (fallback) + return (AVCodec*)fallback; + else if (experimental) + return (AVCodec*)experimental; + else + return NULL; } AVCodec *avcodec_find_encoder(enum AVCodecID id)
When a program uses FFmpeg to decode or encode media, and uses `avcodec_find_decoder` or `avcodec_find_encoder`, I interpret that as the program not caring a whole lot what AVCodec is used; it just wants something which can encode or decode the desired format. I think it makes sense for the user to be able to specify what codec should be the default in that case, because the user might know that they're on a system with a fast hardware encoder or decoder. Currently, the only way to convince FFmpeg to default to a different codec is to recompile it without support for other codecs than the desired one, which feels unnecessary. An argument against this patch could be that it's the application's responsibility to add an -encoder or -decoder option like the ffmpeg CLI utilities do. If that's the "official" stance of FFmpeg, I'm fine with that, even though I don't necessarily agree. Anotehr point is that some applications make assumptions about the pix_fmt the decoder they get from `avcodec_find_decoder` uses. Chromium does this. I believe this is a problem with those applications (and I submitted an issue to the Chromium project: https://bugs.chromium.org/p/webrtc/issues/detail?id=9137#c2), but it should be considered. (I also haven't submitted a patch to FFmpeg before, or any open-source project which doesn't use GitHub pull requests, so I apologize if I have followed the process incorrectly.) This patch makes it possible to choose what codec avcodec_find_encoder and avcodec_find_decoder returns with environment variables. FFMPEG_PREFER_CODEC is an environment variable with a comma-separated list of codec names, and if it contains the name of an applicable codec, that codec will be returned instead of whatever av_codec_iterate happens to return first. There's also FFMPEG_PREFER_ENCODER and FFMPEG_PREFER_DECODER, which specifically apply to avcodec_find_encoder and avcodec_find_decoder respectively. These are prioritized above FFMPEG_PREFER_CODEC. Signed-off-by: Martin Dørum <martid0311@gmail.com> --- libavcodec/allcodecs.c | 59 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 54 insertions(+), 5 deletions(-)