| Message ID | 20170306222021.17917-2-jeebjp@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Mar 07, 2017 at 12:20:20AM +0200, Jan Ekström wrote: > x264-params does the same thing and this leaves us with a single > option that is name-matched with x265-params available in the > libx265 wrapper. > --- > libavcodec/libx264.c | 29 ----------------------------- > 1 file changed, 29 deletions(-) please wait with this its a long time ago but i faintly remember that the option you are about to remove worked while the one remaining had bugs i just tested a bit and thes behave differently for at least no-fast-pskip, i dont remember what the original bug was though also this would break scripts [...]
2017-03-06 23:20 GMT+01:00 Jan Ekström <jeebjp@gmail.com>: > x264-params does the same thing and this leaves us with a single > option that is name-matched with x265-params available in the > libx265 wrapper. (Why?) You have to deprecate an option before removing it. Carl Eugen
On Tue, 7 Mar 2017 04:24:23 +0100, Michael Niedermayer wrote: > its a long time ago but i faintly remember that the option you are > about to remove worked while the one remaining had bugs Can you provide a reproducible case? I will try as well. > also this would break scripts I've never liked this argument. Scripts could have been broken when we removed libfaac, libstagefright, libquvi, etc but I don't recall any users complaining. Scripts can be fixed: we shouldn't be beholden to alleged, random, moldy, old "scripts" in the ether, otherwise we'll never clean up anything. If users want to write-it-and-forget-it they can just use a release branch. There is much inertia facing attempts to tidy up the code, and I believe the strong expectation of encountering this often prevents people from even trying.
On Tue, Mar 7, 2017 at 5:24 AM, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Tue, Mar 07, 2017 at 12:20:20AM +0200, Jan Ekström wrote: >> x264-params does the same thing and this leaves us with a single >> option that is name-matched with x265-params available in the >> libx265 wrapper. >> --- >> libavcodec/libx264.c | 29 ----------------------------- >> 1 file changed, 29 deletions(-) > > please wait with this > The patch set has [RFC] in its topic as I knew any sort of removal is going to be a multi-step process. As I noted, I wasn't sure of the process so I noted that in my cover letter. > ... > its a long time ago but i faintly remember that the option you are > about to remove worked while the one remaining had bugs The x264opt code seems to: * Use custom dict parsing instead of av_dict_parse_string * Try real hard to set you a "1" even if you set something else: > char param[256]={0}, val[256]={0}; > if(sscanf(p, "%255[^:=]=%255[^:]", param, val) == 1){ > OPT_STR(param, "1"); > }else > OPT_STR(param, val); * The OPT_STR macro tries to catch X264_PARAM_BAD_NAME, while x264-params just outputs "Error parsing option '%s = %s'.\n" in all failure cases of x264_param_parse. The x264-params code seems to: * Use av_dict_parse_string/av_dict_get > av_dict_parse_string(&dict, x4->x264_params, "=", ":", 0) > en = av_dict_get(dict, "", en, AV_DICT_IGNORE_SUFFIX) * Just pass key=value one by one into x264_param_parse > x264_param_parse(&x4->params, en->key, en->value) Personally I prefer the latter as it utilizes generic tools we have in our libraries and seems to keep the idea closer to KISS. Catching X264_PARAM_BAD_NAME might be a good addition to x264-params, but not sure how much more helpful that would be in the end.
On Tue, Mar 07, 2017 at 10:08:59AM -0900, Lou Logan wrote: > On Tue, 7 Mar 2017 04:24:23 +0100, Michael Niedermayer wrote: > > > its a long time ago but i faintly remember that the option you are > > about to remove worked while the one remaining had bugs > > Can you provide a reproducible case? I will try as well. the fast pskip i mentioned yesterday ./ffmpeg -y -i ~/videos/matrixbench_mpeg2.mpg -vcodec libx264 -x264-params fast-pskip -vframes 15 -an ref-p.nut ./ffmpeg -y -i ~/videos/matrixbench_mpeg2.mpg -vcodec libx264 -x264-params no-fast-pskip -vframes 15 -an ref-np.nut ./ffmpeg -y -i ~/videos/matrixbench_mpeg2.mpg -vcodec libx264 -x264opts no-fast-pskip -vframes 15 -an ref-no.nut ./ffmpeg -y -i ~/videos/matrixbench_mpeg2.mpg -vcodec libx264 -x264opts fast-pskip -vframes 15 -an ref-o.nut ffd2c735cfb268d5f4291c2f6baab386 ref-o.nut ffd2c735cfb268d5f4291c2f6baab386 ref-p.nut f424075a964018aa83f98f2bf5ab2830 ref-no.nut ffd2c735cfb268d5f4291c2f6baab386 ref-np.nu it appears *fast-pskip has no effect in x264-params i have no idea if thats the only issue also to keep both x264-params and x264opts working you need just 1 line more than if you drop one. Its just one entry in the AVOption array to pass both to the same implementation > > > also this would break scripts > > I've never liked this argument. Scripts could have been broken when > we removed libfaac, libstagefright, libquvi, etc but I don't recall any > users complaining. > Scripts can be fixed: we shouldn't be beholden to > alleged, random, moldy, old "scripts" in the ether, otherwise we'll scripts as in examples all over the net, mailing list acrhives blogs and so on. they can be updated but not by the people using them and in many places also not by the authors who wrote them. also testcase we have on trac can break when things get removed > never clean up anything. If users want to write-it-and-forget-it they > can just use a release branch. cleanup is good and i support it, and theres a lot of code we have that would benefit from cleanup. > > There is much inertia facing attempts to tidy up the code, and I > believe the strong expectation of encountering this often prevents > people from even trying. There is inertia facing removing API, removing features, breaking users of FFmpeg, ... cleanup is much more than that [...]
On Wed, Mar 8, 2017 at 1:01 AM, Michael Niedermayer <michael@niedermayer.cc> wrote: > the fast pskip i mentioned yesterday > > ./ffmpeg -y -i ~/videos/matrixbench_mpeg2.mpg -vcodec libx264 -x264-params fast-pskip -vframes 15 -an ref-p.nut > ./ffmpeg -y -i ~/videos/matrixbench_mpeg2.mpg -vcodec libx264 -x264-params no-fast-pskip -vframes 15 -an ref-np.nut > ./ffmpeg -y -i ~/videos/matrixbench_mpeg2.mpg -vcodec libx264 -x264opts no-fast-pskip -vframes 15 -an ref-no.nut > ./ffmpeg -y -i ~/videos/matrixbench_mpeg2.mpg -vcodec libx264 -x264opts fast-pskip -vframes 15 -an ref-o.nut > > ffd2c735cfb268d5f4291c2f6baab386 ref-o.nut > ffd2c735cfb268d5f4291c2f6baab386 ref-p.nut > f424075a964018aa83f98f2bf5ab2830 ref-no.nut > ffd2c735cfb268d5f4291c2f6baab386 ref-np.nu > > it appears *fast-pskip has no effect in x264-params Right, as I noted in my previous message, the custom dictionary parsing code in there sets the value to "1" if a value is not available. The API always expects a key and a value (even if a NULL value would be valid for a boolean option). So to emulate what x264opts is doing you would just set "fast-pskip=1" or "no-fast-pskip=1". You can by this guess that fast-pskip is indeed the default behavior during medium preset in libx264. In my personal opinion I'm not sure this is worth it to have a Yet Another Dictionary Parser inside libx264.c, esp. since av_dict_parse_string() behavior is already the defining one in multiple components where key/value lists are handled (you can see our Doxygen for some sort of list of usage: https://www.ffmpeg.org/doxygen/trunk/group__lavu__dict.html#gaca5ff7c251e60bd13164d13c82f21b79 ). > > i have no idea if thats the only issue > Unless the very short code around it, or av_dict_parse_string is globally bugged in some way, most likely all "issues" with x264-params are due to the implicit value being set for key-value pairs without a value. > also to keep both x264-params and x264opts working you need just > 1 line more than if you drop one. Its just one entry in the AVOption > array to pass both to the same implementation > That is another alternative and at the very least would remove the duplicated implementation which would be a positive step forward. For some reason I would prefer only having a single option for this so that there would be no questions regarding why the behavior changed for x264opts with regards to implicit values. Best regards, Jan Ekström
On Wed, Mar 08, 2017 at 01:41:38AM +0200, Jan Ekstrom wrote: > On Wed, Mar 8, 2017 at 1:01 AM, Michael Niedermayer > <michael@niedermayer.cc> wrote: > > the fast pskip i mentioned yesterday > > > > ./ffmpeg -y -i ~/videos/matrixbench_mpeg2.mpg -vcodec libx264 -x264-params fast-pskip -vframes 15 -an ref-p.nut > > ./ffmpeg -y -i ~/videos/matrixbench_mpeg2.mpg -vcodec libx264 -x264-params no-fast-pskip -vframes 15 -an ref-np.nut > > ./ffmpeg -y -i ~/videos/matrixbench_mpeg2.mpg -vcodec libx264 -x264opts no-fast-pskip -vframes 15 -an ref-no.nut > > ./ffmpeg -y -i ~/videos/matrixbench_mpeg2.mpg -vcodec libx264 -x264opts fast-pskip -vframes 15 -an ref-o.nut > > > > ffd2c735cfb268d5f4291c2f6baab386 ref-o.nut > > ffd2c735cfb268d5f4291c2f6baab386 ref-p.nut > > f424075a964018aa83f98f2bf5ab2830 ref-no.nut > > ffd2c735cfb268d5f4291c2f6baab386 ref-np.nu > > > > it appears *fast-pskip has no effect in x264-params > > Right, as I noted in my previous message, the custom dictionary > parsing code in there sets the value to "1" if a value is not > available. The API always expects a key and a value (even if a NULL > value would be valid for a boolean option). > So to emulate what x264opts is doing you would just set "fast-pskip=1" > or "no-fast-pskip=1". You can by this guess that fast-pskip is indeed > the default behavior during medium preset in libx264. > > In my personal opinion I'm not sure this is worth it to have a Yet > Another Dictionary Parser inside libx264.c, esp. since > av_dict_parse_string() behavior is already the defining one in > multiple components where key/value lists are handled (you can see our > Doxygen for some sort of list of usage: > https://www.ffmpeg.org/doxygen/trunk/group__lavu__dict.html#gaca5ff7c251e60bd13164d13c82f21b79 > ). As it is the code that uses the common parser is buggy that is if a user specifies no-fast-pskip its not disabled in fact it does not even error out or give any warning IIUC you want an implementation thats based on a common parser ? can you write an implementation thats based on a common parser and which works without bugs in corner cases ? i think that should be possible but if iam missing something, please elaborate [...]
diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c index b11ede6..5cbc376 100644 --- a/libavcodec/libx264.c +++ b/libavcodec/libx264.c @@ -53,7 +53,6 @@ typedef struct X264Context { char *level; int fastfirstpass; char *wpredp; - char *x264opts; float crf; float crf_max; int cqp; @@ -396,20 +395,6 @@ static av_cold int X264_close(AVCodecContext *avctx) return 0; } -#define OPT_STR(opt, param) \ - do { \ - int ret; \ - if ((ret = x264_param_parse(&x4->params, opt, param)) < 0) { \ - if(ret == X264_PARAM_BAD_NAME) \ - av_log(avctx, AV_LOG_ERROR, \ - "bad option '%s': '%s'\n", opt, param); \ - else \ - av_log(avctx, AV_LOG_ERROR, \ - "bad value for '%s': '%s'\n", opt, param); \ - return -1; \ - } \ - } while (0) - static int convert_pix_fmt(enum AVPixelFormat pix_fmt) { switch (pix_fmt) { @@ -774,19 +759,6 @@ FF_ENABLE_DEPRECATION_WARNINGS if (avctx->flags & AV_CODEC_FLAG_GLOBAL_HEADER) x4->params.b_repeat_headers = 0; - if(x4->x264opts){ - const char *p= x4->x264opts; - while(p){ - char param[4096]={0}, val[4096]={0}; - if(sscanf(p, "%4095[^:=]=%4095[^:]", param, val) == 1){ - OPT_STR(param, "1"); - }else - OPT_STR(param, val); - p= strchr(p, ':'); - p+=!!p; - } - } - if (x4->x264_params) { AVDictionary *dict = NULL; AVDictionaryEntry *en = NULL; @@ -908,7 +880,6 @@ static const AVOption options[] = { {"passlogfile", "Filename for 2 pass stats", OFFSET(stats), AV_OPT_TYPE_STRING, {.str=NULL}, 0, 0, VE}, {"wpredp", "Weighted prediction for P-frames", OFFSET(wpredp), AV_OPT_TYPE_STRING, {.str=NULL}, 0, 0, VE}, {"a53cc", "Use A53 Closed Captions (if available)", OFFSET(a53_cc), AV_OPT_TYPE_BOOL, {.i64 = 1}, 0, 1, VE}, - {"x264opts", "x264 options", OFFSET(x264opts), AV_OPT_TYPE_STRING, {.str=NULL}, 0, 0, VE}, { "crf", "Select the quality for constant quality mode", OFFSET(crf), AV_OPT_TYPE_FLOAT, {.dbl = -1 }, -1, FLT_MAX, VE }, { "crf_max", "In CRF mode, prevents VBV from lowering quality beyond this point.",OFFSET(crf_max), AV_OPT_TYPE_FLOAT, {.dbl = -1 }, -1, FLT_MAX, VE }, { "qp", "Constant quantization parameter rate control method",OFFSET(cqp), AV_OPT_TYPE_INT, { .i64 = -1 }, -1, INT_MAX, VE },