diff mbox series

[FFmpeg-devel] fftools: Accept more negative prefixes

Message ID 8472b869-e00a-171f-05d4-503379702c3b@jkqxz.net
State New
Headers show
Series [FFmpeg-devel] fftools: Accept more negative prefixes
Related show

Checks

Context Check Description
andriy/default pending
andriy/configure warning Failed to apply patch

Commit Message

Mark Thompson Aug. 20, 2020, 9:49 p.m. UTC
In addition to "no", also allow "no_", "disable" and "disable_".
---
On 20/08/2020 18:49, Nicolas George wrote:
> Alexander Strasser (12020-08-17):
>> Here are some suggestions in no particular order:
>>
>> * auto_conversion_filters (from Marton)
> 
> I can be ok with this one. I really dislike boolean options that default
> to yes and have to be disabled with no, because it requires remembering
> what the default is, but if that is what everybody else prefers.

But we can fix that to be nicer!

With this patch, -disable_auto_conversion_filters does what you want.

(The duplication below is because one part is used for ffmpeg and the other for ffplay/ffprobe.)

- Mark


  fftools/cmdutils.c | 42 +++++++++++++++++++++++++++++++++---------
  1 file changed, 33 insertions(+), 9 deletions(-)

Comments

Moritz Barsnick Aug. 21, 2020, 5:49 a.m. UTC | #1
On Thu, Aug 20, 2020 at 22:49:16 +0100, Mark Thompson wrote:

> With this patch, -disable_auto_conversion_filters does what you want.
[...]
> +    if (!po->name) {
> +        /* Try to match a boolean option with a negative prefix. */
> +        for (int i = 0; i < FF_ARRAY_ELEMS(negative_prefixes); i++) {
> +            size_t len = strlen(negative_prefixes[i]);
> +            if (!strncmp(opt, negative_prefixes[i], len)) {
> +                po = find_option(options, opt + len);
> +                if (po->name && (po->flags & OPT_BOOL)) {
> +                    arg = "0";
> +                    break;
> +                }
> +            }
> +        }

Without checking in more detail: Wouldn't this break the
"-noise_reduction", "-non_deterministic" and "-non_linear_quant"
options?

Cheers,
Moritz
Mark Thompson Aug. 22, 2020, 8:55 p.m. UTC | #2
On 21/08/2020 06:49, Moritz Barsnick wrote:
> On Thu, Aug 20, 2020 at 22:49:16 +0100, Mark Thompson wrote:
> 
>> With this patch, -disable_auto_conversion_filters does what you want.
> [...]
>> +    if (!po->name) {
>> +        /* Try to match a boolean option with a negative prefix. */
>> +        for (int i = 0; i < FF_ARRAY_ELEMS(negative_prefixes); i++) {
>> +            size_t len = strlen(negative_prefixes[i]);
>> +            if (!strncmp(opt, negative_prefixes[i], len)) {
>> +                po = find_option(options, opt + len);
>> +                if (po->name && (po->flags & OPT_BOOL)) {
>> +                    arg = "0";
>> +                    break;
>> +                }
>> +            }
>> +        }
> 
> Without checking in more detail: Wouldn't this break the
> "-noise_reduction", "-non_deterministic" and "-non_linear_quant"
> options?

The intent is to extend what already happens - given an argument like "-nougat", it first looks for the option of any kind called "nougat", then only if that is not found looks for a boolean option called "ugat".  In your example, the "noise_reduction" option is found, so "ise_reduction" is never searched for.

(Alternatively: if you are referencing a bug which you have spotted in my change but I am missing, could you explain in more detail?)

- Mark
Moritz Barsnick Aug. 26, 2020, 1:34 p.m. UTC | #3
On Sat, Aug 22, 2020 at 21:55:21 +0100, Mark Thompson wrote:
> > Without checking in more detail: Wouldn't this break the
> > "-noise_reduction", "-non_deterministic" and "-non_linear_quant"
> > options?
>
> The intent is to extend what already happens - given an argument like
> "-nougat", it first looks for the option of any kind called "nougat",
> then only if that is not found looks for a boolean option called
> "ugat". In your example, the "noise_reduction" option is found, so
> "ise_reduction" is never searched for.

Okay, I failed to check that. Thanks for the explanation, perfect.

Moritz
diff mbox series

Patch

diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c
index 88fdbeaf1e..1a1740b46e 100644
--- a/fftools/cmdutils.c
+++ b/fftools/cmdutils.c
@@ -345,6 +345,13 @@  static int write_option(void *optctx, const OptionDef *po, const char *opt,
      return 0;
  }

+static const char *negative_prefixes[] = {
+    "no_",
+    "no",
+    "disable_",
+    "disable",
+};
+
  int parse_option(void *optctx, const char *opt, const char *arg,
                   const OptionDef *options)
  {
@@ -352,11 +359,18 @@  int parse_option(void *optctx, const char *opt, const char *arg,
      int ret;

      po = find_option(options, opt);
-    if (!po->name && opt[0] == 'n' && opt[1] == 'o') {
-        /* handle 'no' bool option */
-        po = find_option(options, opt + 2);
-        if ((po->name && (po->flags & OPT_BOOL)))
-            arg = "0";
+    if (!po->name) {
+        /* Try to match a boolean option with a negative prefix. */
+        for (int i = 0; i < FF_ARRAY_ELEMS(negative_prefixes); i++) {
+            size_t len = strlen(negative_prefixes[i]);
+            if (!strncmp(opt, negative_prefixes[i], len)) {
+                po = find_option(options, opt + len);
+                if (po->name && (po->flags & OPT_BOOL)) {
+                    arg = "0";
+                    break;
+                }
+            }
+        }
      } else if (po->flags & OPT_BOOL)
          arg = "1";

@@ -764,6 +778,7 @@  int split_commandline(OptionParseContext *octx, int argc, char *argv[],
      while (optindex < argc) {
          const char *opt = argv[optindex++], *arg;
          const OptionDef *po;
+        int negative_match;
          int ret;

          av_log(NULL, AV_LOG_DEBUG, "Reading option '%s' ...", opt);
@@ -831,10 +846,19 @@  do {                                                                           \
              }
          }

-        /* boolean -nofoo options */
-        if (opt[0] == 'n' && opt[1] == 'o' &&
-            (po = find_option(options, opt + 2)) &&
-            po->name && po->flags & OPT_BOOL) {
+        /* Boolean options with a negative prefix. */
+        negative_match = 0;
+        for (int i = 0; i < FF_ARRAY_ELEMS(negative_prefixes); i++) {
+            size_t len = strlen(negative_prefixes[i]);
+            if (!strncmp(opt, negative_prefixes[i], len)) {
+                po = find_option(options, opt + len);
+                if (po->name && (po->flags & OPT_BOOL)) {
+                    negative_match = 1;
+                    break;
+                }
+            }
+        }
+        if (negative_match) {
              add_opt(octx, po, opt, "0");
              av_log(NULL, AV_LOG_DEBUG, " matched as option '%s' (%s) with "
                     "argument 0.\n", po->name, po->help);