Re: [FFmpeg-devel] [PATCH 1/2] libx264: remove the "x264opts" AVOption

[Michael Niedermayer]

On Wed, Mar 08, 2017 at 01:41:38AM +0200, Jan Ekstrom wrote:
> On Wed, Mar 8, 2017 at 1:01 AM, Michael Niedermayer
>  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

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] libx264: remove the "x264opts" AVOption

[Jan Ekstrom]

On Wed, Mar 8, 2017 at 1:01 AM, Michael Niedermayer
 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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] libx264: remove the "x264opts" AVOption

[Michael Niedermayer]

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

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Asymptotically faster algorithms should always be preferred if you have
asymptotical amounts of data


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] libx264: remove the "x264opts" AVOption

[Jan Ekstrom]

On Tue, Mar 7, 2017 at 5:24 AM, Michael Niedermayer
 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(, 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(>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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] libx264: remove the "x264opts" AVOption

[Lou Logan]

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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] libx264: remove the "x264opts" AVOption

[Carl Eugen Hoyos]

2017-03-06 23:20 GMT+01:00 Jan Ekström :
> 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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 1/2] libx264: remove the "x264opts" AVOption

[Jan Ekström]

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(-)

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(>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 
},
-- 
2.9.3

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