diff mbox series

[FFmpeg-devel] fftools/cmdutils.c: Add cmd line option to override detection of cpu count

Message ID 5bf9410a-a108-0294-8c33-c7fc78e07c5f@mail.de
State New
Headers show
Series [FFmpeg-devel] fftools/cmdutils.c: Add cmd line option to override detection of cpu count | expand

Checks

Context Check Description
andriy/configure warning Failed to apply patch

Commit Message

Thilo Borgmann June 5, 2021, 12:29 p.m. UTC
Hi,

add an option to override auto-detection of cpu count. Defaults to auto-detection, of course.

-Thilo
From 38612f3e1339354dbaa6be4f36072320ff71c707 Mon Sep 17 00:00:00 2001
From: Thilo Borgmann <thilo.borgmann@mail.de>
Date: Sat, 5 Jun 2021 14:26:23 +0200
Subject: [PATCH] fftools/cmdutils.c: Add cmd line option to override detection
 of cpu count

Suggested-By: ffmpeg@fb.com
---
 doc/fftools-common-opts.texi |  7 +++++++
 fftools/cmdutils.c           | 27 +++++++++++++++++++++++++++
 fftools/cmdutils.h           |  7 +++++++
 libavutil/cpu.c              | 14 ++++++++++++++
 libavutil/cpu.h              |  6 ++++++
 5 files changed, 61 insertions(+)

Comments

Anton Khirnov June 5, 2021, 2:33 p.m. UTC | #1
Quoting Thilo Borgmann (2021-06-05 14:29:05)
> Hi,
> 
> add an option to override auto-detection of cpu count. Defaults to auto-detection, of course.
> 
> -Thilo
> 
> From 38612f3e1339354dbaa6be4f36072320ff71c707 Mon Sep 17 00:00:00 2001
> From: Thilo Borgmann <thilo.borgmann@mail.de>
> Date: Sat, 5 Jun 2021 14:26:23 +0200
> Subject: [PATCH] fftools/cmdutils.c: Add cmd line option to override detection
>  of cpu count
> 
> Suggested-By: ffmpeg@fb.com
> ---
>  doc/fftools-common-opts.texi |  7 +++++++
>  fftools/cmdutils.c           | 27 +++++++++++++++++++++++++++
>  fftools/cmdutils.h           |  7 +++++++
>  libavutil/cpu.c              | 14 ++++++++++++++
>  libavutil/cpu.h              |  6 ++++++
>  5 files changed, 61 insertions(+)
> 
> diff --git a/doc/fftools-common-opts.texi b/doc/fftools-common-opts.texi
> index 5260ecb8f3..7643dd8396 100644
> --- a/doc/fftools-common-opts.texi
> +++ b/doc/fftools-common-opts.texi
> @@ -353,6 +353,13 @@ Possible flags for this option are:
>  @end table
>  @end table
>  
> +@item -cpucount @var{count} (@emph{global})
> +Override detection of CPU count. This option is intended
> +for testing. Do not use it unless you know what you're doing.
> +@example
> +ffmpeg -cpucount 2
> +@end example
> +
>  @item -max_alloc @var{bytes}
>  Set the maximum size limit for allocating a block on the heap by ffmpeg's
>  family of malloc functions. Exercise @strong{extreme caution} when using
> diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c
> index 4eb68d2201..583a706e5d 100644
> --- a/fftools/cmdutils.c
> +++ b/fftools/cmdutils.c
> @@ -72,6 +72,7 @@ AVDictionary *format_opts, *codec_opts, *resample_opts;
>  static FILE *report_file;
>  static int report_file_level = AV_LOG_DEBUG;
>  int hide_banner = 0;
> +int cpu_count = -1;
>  
>  enum show_muxdemuxers {
>      SHOW_DEFAULT,
> @@ -866,6 +867,32 @@ int opt_cpuflags(void *optctx, const char *opt, const char *arg)
>      return 0;
>  }
>  
> +int opt_cpucount(void *optctx, const char *opt, const char *arg)
> +{
> +    int ret;
> +    int count;
> +
> +    static const AVOption opts[] = {
> +        {"count", NULL, 0, AV_OPT_TYPE_INT, { .i64 = -1}, -1, INT_MAX, NULL},
> +        {NULL},
> +    };
> +    static const AVClass class = {
> +        .class_name = "cpucount",
> +        .item_name  = av_default_item_name,
> +        .option     = opts,
> +        .version    = LIBAVUTIL_VERSION_INT,
> +    };
> +    const AVClass *pclass = &class;
> +
> +    ret = av_opt_eval_int(&pclass, opts, arg, &count);
> +
> +    if (!ret) {
> +        av_force_cpu_count(count);
> +    }
> +
> +    return ret;

This looks way overcomplicated. Why not just call strtol on arg?

> +}
> +
>  int opt_loglevel(void *optctx, const char *opt, const char *arg)
>  {
>      const struct { const char *name; int level; } log_levels[] = {
> diff --git a/fftools/cmdutils.h b/fftools/cmdutils.h
> index 1917510589..29a45dd0be 100644
> --- a/fftools/cmdutils.h
> +++ b/fftools/cmdutils.h
> @@ -50,6 +50,7 @@ extern AVDictionary *sws_dict;
>  extern AVDictionary *swr_opts;
>  extern AVDictionary *format_opts, *codec_opts, *resample_opts;
>  extern int hide_banner;
> +extern int cpu_count;
>  
>  /**
>   * Register a program-specific cleanup routine.
> @@ -88,6 +89,11 @@ void log_callback_help(void* ptr, int level, const char* fmt, va_list vl);
>   */
>  int opt_cpuflags(void *optctx, const char *opt, const char *arg);
>  
> +/**
> + * Override the cpucount.
> + */
> +int opt_cpucount(void *optctx, const char *opt, const char *arg);
> +
>  /**
>   * Fallback for options that are not explicitly handled, these will be
>   * parsed through AVOptions.
> @@ -239,6 +245,7 @@ void show_help_options(const OptionDef *options, const char *msg, int req_flags,
>      { "report",      0,                    { .func_arg = opt_report },       "generate a report" },                     \
>      { "max_alloc",   HAS_ARG,              { .func_arg = opt_max_alloc },    "set maximum size of a single allocated block", "bytes" }, \
>      { "cpuflags",    HAS_ARG | OPT_EXPERT, { .func_arg = opt_cpuflags },     "force specific cpu flags", "flags" },     \
> +    { "cpucount",    HAS_ARG | OPT_EXPERT, { .func_arg = opt_cpucount },     "force specific cpu count", "count" },     \
>      { "hide_banner", OPT_BOOL | OPT_EXPERT, {&hide_banner},     "do not show program banner", "hide_banner" },          \
>      CMDUTILS_COMMON_OPTIONS_AVDEVICE                                                                                    \
>  
> diff --git a/libavutil/cpu.c b/libavutil/cpu.c
> index 52f6b9a3bf..ccd5b5adac 100644
> --- a/libavutil/cpu.c
> +++ b/libavutil/cpu.c
> @@ -48,6 +48,7 @@
>  #endif
>  
>  static atomic_int cpu_flags = ATOMIC_VAR_INIT(-1);
> +static atomic_int cpu_count = ATOMIC_VAR_INIT(-1);
>  
>  static int get_cpu_flags(void)
>  {
> @@ -306,14 +307,27 @@ int av_cpu_count(void)
>      nb_cpus = sysinfo.dwNumberOfProcessors;
>  #endif
>  
> +    int count = atomic_load_explicit(&cpu_count, memory_order_relaxed);

Just use a normal atomic_load().

> +
>      if (!printed) {
>          av_log(NULL, AV_LOG_DEBUG, "detected %d logical cores\n", nb_cpus);
> +        if (count > 0) {
> +            av_log(NULL, AV_LOG_DEBUG, "overriding to %d logical cores\n", count);
> +        }
>          printed = 1;
>      }
>  
> +    if (count > 0) {
> +        nb_cpus = count;
> +    }
> +
>      return nb_cpus;
>  }
>  
> +void av_force_cpu_count(int count){

Style.

> +    atomic_store_explicit(&cpu_count, count, memory_order_relaxed);
> +}
> +
>  size_t av_cpu_max_align(void)
>  {
>      if (ARCH_MIPS)
> diff --git a/libavutil/cpu.h b/libavutil/cpu.h
> index 83099dd969..9c682ad50b 100644
> --- a/libavutil/cpu.h
> +++ b/libavutil/cpu.h
> @@ -119,6 +119,12 @@ int av_parse_cpu_caps(unsigned *flags, const char *s);
>   */
>  int av_cpu_count(void);
>  
> +/**
> + * Overrides cpu count detection and forces the specified count.
> + * Count < 1 disables forcing of specific count.
> + */
> +void av_force_cpu_count(int count);

I'd prefer if the name was namespaced as av_cpu_
Thilo Borgmann June 6, 2021, 10:44 a.m. UTC | #2
Hi,

Am 05.06.21 um 16:33 schrieb Anton Khirnov:
> Quoting Thilo Borgmann (2021-06-05 14:29:05)
>> Hi,
>>
>> add an option to override auto-detection of cpu count. Defaults to auto-detection, of course.
>>
>> -Thilo
>>
>> From 38612f3e1339354dbaa6be4f36072320ff71c707 Mon Sep 17 00:00:00 2001
>> From: Thilo Borgmann <thilo.borgmann@mail.de>
>> Date: Sat, 5 Jun 2021 14:26:23 +0200
>> Subject: [PATCH] fftools/cmdutils.c: Add cmd line option to override detection
>>  of cpu count
>>
>> Suggested-By: ffmpeg@fb.com
>> ---
>>  doc/fftools-common-opts.texi |  7 +++++++
>>  fftools/cmdutils.c           | 27 +++++++++++++++++++++++++++
>>  fftools/cmdutils.h           |  7 +++++++
>>  libavutil/cpu.c              | 14 ++++++++++++++
>>  libavutil/cpu.h              |  6 ++++++
>>  5 files changed, 61 insertions(+)
>>
>> [...]

>>  /**
>>   * Fallback for options that are not explicitly handled, these will be
>>   * parsed through AVOptions.
>> @@ -239,6 +245,7 @@ void show_help_options(const OptionDef *options, const char *msg, int req_flags,
>>      { "report",      0,                    { .func_arg = opt_report },       "generate a report" },                     \
>>      { "max_alloc",   HAS_ARG,              { .func_arg = opt_max_alloc },    "set maximum size of a single allocated block", "bytes" }, \
>>      { "cpuflags",    HAS_ARG | OPT_EXPERT, { .func_arg = opt_cpuflags },     "force specific cpu flags", "flags" },     \
>> +    { "cpucount",    HAS_ARG | OPT_EXPERT, { .func_arg = opt_cpucount },     "force specific cpu count", "count" },     \
>>      { "hide_banner", OPT_BOOL | OPT_EXPERT, {&hide_banner},     "do not show program banner", "hide_banner" },          \
>>      CMDUTILS_COMMON_OPTIONS_AVDEVICE                                                                                    \
>>  
>> diff --git a/libavutil/cpu.c b/libavutil/cpu.c
>> index 52f6b9a3bf..ccd5b5adac 100644
>> --- a/libavutil/cpu.c
>> +++ b/libavutil/cpu.c
>> @@ -48,6 +48,7 @@
>>  #endif
>>  
>>  static atomic_int cpu_flags = ATOMIC_VAR_INIT(-1);
>> +static atomic_int cpu_count = ATOMIC_VAR_INIT(-1);
>>  
>>  static int get_cpu_flags(void)
>>  {
>> @@ -306,14 +307,27 @@ int av_cpu_count(void)
>>      nb_cpus = sysinfo.dwNumberOfProcessors;
>>  #endif
>>  
>> +    int count = atomic_load_explicit(&cpu_count, memory_order_relaxed);
> 
> Just use a normal atomic_load().
> 
>> [...]

>> +/**
>> + * Overrides cpu count detection and forces the specified count.
>> + * Count < 1 disables forcing of specific count.
>> + */
>> +void av_force_cpu_count(int count);
> 
> I'd prefer if the name was namespaced as av_cpu_

For both of these, I copied the behavior of av_force_cpu_flags(). For function names as well as the atomic load.
Let me know which way you prefer. If you prefer your reviewed version, I could also send a patch for similar changes to av_force_cpu_flags() if that makes sense.

All other remarks changed locally.

Thanks,
Thilo
Andreas Rheinhardt June 6, 2021, 12:35 p.m. UTC | #3
Thilo Borgmann:
> Hi,
> 
> add an option to override auto-detection of cpu count. Defaults to auto-detection, of course.
> 
> -Thilo
> 

> 
>  doc/fftools-common-opts.texi |  7 +++++++
>  fftools/cmdutils.c           | 27 +++++++++++++++++++++++++++
>  fftools/cmdutils.h           |  7 +++++++
>  libavutil/cpu.c              | 14 ++++++++++++++
>  libavutil/cpu.h              |  6 ++++++

The changes to libavutil and cmdutils should be in separate patches; and
of course the commit message should mention that you are changing
libavutil -- I would have nearly missed this patch (given that I don't
pay much attention to fftools in general).

> 
> diff --git a/libavutil/cpu.c b/libavutil/cpu.c
> index 52f6b9a3bf..ccd5b5adac 100644
> --- a/libavutil/cpu.c
> +++ b/libavutil/cpu.c

52f6b9a3bf is the state of cpu.c after
e387fcd01cb84d9493f3b96158addd2a85f086c6. This is completely outdated.

> 
> @@ -306,14 +307,27 @@ int av_cpu_count(void)
>      nb_cpus = sysinfo.dwNumberOfProcessors;
>  #endif
>  
> +    int count = atomic_load_explicit(&cpu_count, memory_order_relaxed);
> +
>      if (!printed) {
>          av_log(NULL, AV_LOG_DEBUG, "detected %d logical cores\n", nb_cpus);
> +        if (count > 0) {
> +            av_log(NULL, AV_LOG_DEBUG, "overriding to %d logical cores\n", count);
> +        }
>          printed = 1;
>      }
>  
> +    if (count > 0) {
> +        nb_cpus = count;
> +    }
> +
>      return nb_cpus;
>  }
>  
In particular, this hunk doesn't apply to git master at all any more
(did I already mention that cpu.c only has 245 lines atm, not >300?).
And the declaration of count would lead to a statement-after-declaration
warning.

- Andreas
Thilo Borgmann June 6, 2021, 1:19 p.m. UTC | #4
Am 06.06.21 um 14:35 schrieb Andreas Rheinhardt:
> Thilo Borgmann:
>> Hi,
>>
>> add an option to override auto-detection of cpu count. Defaults to auto-detection, of course.
>>
>> -Thilo
>>
> 
>>
>>  doc/fftools-common-opts.texi |  7 +++++++
>>  fftools/cmdutils.c           | 27 +++++++++++++++++++++++++++
>>  fftools/cmdutils.h           |  7 +++++++
>>  libavutil/cpu.c              | 14 ++++++++++++++
>>  libavutil/cpu.h              |  6 ++++++
> 
> The changes to libavutil and cmdutils should be in separate patches; and
> of course the commit message should mention that you are changing
> libavutil -- I would have nearly missed this patch (given that I don't
> pay much attention to fftools in general).
> 
>>
>> diff --git a/libavutil/cpu.c b/libavutil/cpu.c
>> index 52f6b9a3bf..ccd5b5adac 100644
>> --- a/libavutil/cpu.c
>> +++ b/libavutil/cpu.c
> 
> 52f6b9a3bf is the state of cpu.c after
> e387fcd01cb84d9493f3b96158addd2a85f086c6. This is completely outdated.

Yes, messed up branches for the patch, thx!


>>
>> @@ -306,14 +307,27 @@ int av_cpu_count(void)
>>      nb_cpus = sysinfo.dwNumberOfProcessors;
>>  #endif
>>  
>> +    int count = atomic_load_explicit(&cpu_count, memory_order_relaxed);
>> +
>>      if (!printed) {
>>          av_log(NULL, AV_LOG_DEBUG, "detected %d logical cores\n", nb_cpus);
>> +        if (count > 0) {
>> +            av_log(NULL, AV_LOG_DEBUG, "overriding to %d logical cores\n", count);
>> +        }
>>          printed = 1;
>>      }
>>  
>> +    if (count > 0) {
>> +        nb_cpus = count;
>> +    }
>> +
>>      return nb_cpus;
>>  }
>>  
> In particular, this hunk doesn't apply to git master at all any more
> (did I already mention that cpu.c only has 245 lines atm, not >300?).
> And the declaration of count would lead to a statement-after-declaration
> warning.

Done locally to apply to today's HEAD. I think I'll wait with sending both patches once we decided on atomic loads in "[PATCH] Stop using _explicit atomic operations where not necessary.".

Thanks,
Thilo
Thilo Borgmann June 6, 2021, 1:19 p.m. UTC | #5
Am 06.06.21 um 14:35 schrieb Andreas Rheinhardt:
> Thilo Borgmann:
>> Hi,
>>
>> add an option to override auto-detection of cpu count. Defaults to auto-detection, of course.
>>
>> -Thilo
>>
> 
>>
>>  doc/fftools-common-opts.texi |  7 +++++++
>>  fftools/cmdutils.c           | 27 +++++++++++++++++++++++++++
>>  fftools/cmdutils.h           |  7 +++++++
>>  libavutil/cpu.c              | 14 ++++++++++++++
>>  libavutil/cpu.h              |  6 ++++++
> 
> The changes to libavutil and cmdutils should be in separate patches; and
> of course the commit message should mention that you are changing
> libavutil -- I would have nearly missed this patch (given that I don't
> pay much attention to fftools in general).
> 
>>
>> diff --git a/libavutil/cpu.c b/libavutil/cpu.c
>> index 52f6b9a3bf..ccd5b5adac 100644
>> --- a/libavutil/cpu.c
>> +++ b/libavutil/cpu.c
> 
> 52f6b9a3bf is the state of cpu.c after
> e387fcd01cb84d9493f3b96158addd2a85f086c6. This is completely outdated.

Yes, messed up branches for the patch, thx!


>>
>> @@ -306,14 +307,27 @@ int av_cpu_count(void)
>>      nb_cpus = sysinfo.dwNumberOfProcessors;
>>  #endif
>>  
>> +    int count = atomic_load_explicit(&cpu_count, memory_order_relaxed);
>> +
>>      if (!printed) {
>>          av_log(NULL, AV_LOG_DEBUG, "detected %d logical cores\n", nb_cpus);
>> +        if (count > 0) {
>> +            av_log(NULL, AV_LOG_DEBUG, "overriding to %d logical cores\n", count);
>> +        }
>>          printed = 1;
>>      }
>>  
>> +    if (count > 0) {
>> +        nb_cpus = count;
>> +    }
>> +
>>      return nb_cpus;
>>  }
>>  
> In particular, this hunk doesn't apply to git master at all any more
> (did I already mention that cpu.c only has 245 lines atm, not >300?).
> And the declaration of count would lead to a statement-after-declaration
> warning.

Done locally to apply to today's HEAD. I think I'll wait with sending both patches once we decided on atomic loads in "[PATCH] Stop using _explicit atomic operations where not necessary.".

Thanks,
Thilo
Anton Khirnov June 6, 2021, 2:20 p.m. UTC | #6
Quoting Thilo Borgmann (2021-06-06 12:44:41)
> Hi,
> 
> Am 05.06.21 um 16:33 schrieb Anton Khirnov:
> > Quoting Thilo Borgmann (2021-06-05 14:29:05)
> >> Hi,
> >>
> >> add an option to override auto-detection of cpu count. Defaults to auto-detection, of course.
> >>
> >> -Thilo
> >>
> >> From 38612f3e1339354dbaa6be4f36072320ff71c707 Mon Sep 17 00:00:00 2001
> >> From: Thilo Borgmann <thilo.borgmann@mail.de>
> >> Date: Sat, 5 Jun 2021 14:26:23 +0200
> >> Subject: [PATCH] fftools/cmdutils.c: Add cmd line option to override detection
> >>  of cpu count
> >>
> >> Suggested-By: ffmpeg@fb.com
> >> ---
> >>  doc/fftools-common-opts.texi |  7 +++++++
> >>  fftools/cmdutils.c           | 27 +++++++++++++++++++++++++++
> >>  fftools/cmdutils.h           |  7 +++++++
> >>  libavutil/cpu.c              | 14 ++++++++++++++
> >>  libavutil/cpu.h              |  6 ++++++
> >>  5 files changed, 61 insertions(+)
> >>
> >> [...]
> 
> >>  /**
> >>   * Fallback for options that are not explicitly handled, these will be
> >>   * parsed through AVOptions.
> >> @@ -239,6 +245,7 @@ void show_help_options(const OptionDef *options, const char *msg, int req_flags,
> >>      { "report",      0,                    { .func_arg = opt_report },       "generate a report" },                     \
> >>      { "max_alloc",   HAS_ARG,              { .func_arg = opt_max_alloc },    "set maximum size of a single allocated block", "bytes" }, \
> >>      { "cpuflags",    HAS_ARG | OPT_EXPERT, { .func_arg = opt_cpuflags },     "force specific cpu flags", "flags" },     \
> >> +    { "cpucount",    HAS_ARG | OPT_EXPERT, { .func_arg = opt_cpucount },     "force specific cpu count", "count" },     \
> >>      { "hide_banner", OPT_BOOL | OPT_EXPERT, {&hide_banner},     "do not show program banner", "hide_banner" },          \
> >>      CMDUTILS_COMMON_OPTIONS_AVDEVICE                                                                                    \
> >>  
> >> diff --git a/libavutil/cpu.c b/libavutil/cpu.c
> >> index 52f6b9a3bf..ccd5b5adac 100644
> >> --- a/libavutil/cpu.c
> >> +++ b/libavutil/cpu.c
> >> @@ -48,6 +48,7 @@
> >>  #endif
> >>  
> >>  static atomic_int cpu_flags = ATOMIC_VAR_INIT(-1);
> >> +static atomic_int cpu_count = ATOMIC_VAR_INIT(-1);
> >>  
> >>  static int get_cpu_flags(void)
> >>  {
> >> @@ -306,14 +307,27 @@ int av_cpu_count(void)
> >>      nb_cpus = sysinfo.dwNumberOfProcessors;
> >>  #endif
> >>  
> >> +    int count = atomic_load_explicit(&cpu_count, memory_order_relaxed);
> > 
> > Just use a normal atomic_load().
> > 
> >> [...]
> 
> >> +/**
> >> + * Overrides cpu count detection and forces the specified count.
> >> + * Count < 1 disables forcing of specific count.
> >> + */
> >> +void av_force_cpu_count(int count);
> > 
> > I'd prefer if the name was namespaced as av_cpu_
> 
> For both of these, I copied the behavior of av_force_cpu_flags(). For function names as well as the atomic load.
> Let me know which way you prefer. If you prefer your reviewed version, I could also send a patch for similar changes to av_force_cpu_flags() if that makes sense.

Sadly we cannot change existing function names. But at least new
functions can use consistent namespacing.
Thilo Borgmann July 4, 2021, 12:31 p.m. UTC | #7
Hi,

>>> add an option to override auto-detection of cpu count. Defaults to auto-detection, of course.
>>>
>>> -Thilo
>>>
>>
>>>
>>>  doc/fftools-common-opts.texi |  7 +++++++
>>>  fftools/cmdutils.c           | 27 +++++++++++++++++++++++++++
>>>  fftools/cmdutils.h           |  7 +++++++
>>>  libavutil/cpu.c              | 14 ++++++++++++++
>>>  libavutil/cpu.h              |  6 ++++++
>>
>> The changes to libavutil and cmdutils should be in separate patches; and
>> of course the commit message should mention that you are changing
>> libavutil -- I would have nearly missed this patch (given that I don't
>> pay much attention to fftools in general).
>>
>>>
>>> diff --git a/libavutil/cpu.c b/libavutil/cpu.c
>>> index 52f6b9a3bf..ccd5b5adac 100644
>>> --- a/libavutil/cpu.c
>>> +++ b/libavutil/cpu.c
>>
>> 52f6b9a3bf is the state of cpu.c after
>> e387fcd01cb84d9493f3b96158addd2a85f086c6. This is completely outdated.
> 
> Yes, messed up branches for the patch, thx!
> 
> 
>>>
>>> @@ -306,14 +307,27 @@ int av_cpu_count(void)
>>>      nb_cpus = sysinfo.dwNumberOfProcessors;
>>>  #endif
>>>  
>>> +    int count = atomic_load_explicit(&cpu_count, memory_order_relaxed);
>>> +
>>>      if (!printed) {
>>>          av_log(NULL, AV_LOG_DEBUG, "detected %d logical cores\n", nb_cpus);
>>> +        if (count > 0) {
>>> +            av_log(NULL, AV_LOG_DEBUG, "overriding to %d logical cores\n", count);
>>> +        }
>>>          printed = 1;
>>>      }
>>>  
>>> +    if (count > 0) {
>>> +        nb_cpus = count;
>>> +    }
>>> +
>>>      return nb_cpus;
>>>  }
>>>  
>> In particular, this hunk doesn't apply to git master at all any more
>> (did I already mention that cpu.c only has 245 lines atm, not >300?).
>> And the declaration of count would lead to a statement-after-declaration
>> warning.
> 
> Done locally to apply to today's HEAD. I think I'll wait with sending both patches once we decided on atomic loads in "[PATCH] Stop using _explicit atomic operations where not necessary.".

Discussion on atomic loads appear to have stalled mid-June... If there are no hard feeling, I'll push this version soon as the explicit (or not) loads can then be changed later.

-Thilo
Thilo Borgmann July 16, 2021, 8:20 a.m. UTC | #8
Am 04.07.21 um 14:31 schrieb Thilo Borgmann:
> Hi,
> 
>>>> add an option to override auto-detection of cpu count. Defaults to auto-detection, of course.
>>>>
>>>> -Thilo
>>>>
>>>
>>>>
>>>>  doc/fftools-common-opts.texi |  7 +++++++
>>>>  fftools/cmdutils.c           | 27 +++++++++++++++++++++++++++
>>>>  fftools/cmdutils.h           |  7 +++++++
>>>>  libavutil/cpu.c              | 14 ++++++++++++++
>>>>  libavutil/cpu.h              |  6 ++++++
>>>
>>> The changes to libavutil and cmdutils should be in separate patches; and
>>> of course the commit message should mention that you are changing
>>> libavutil -- I would have nearly missed this patch (given that I don't
>>> pay much attention to fftools in general).
>>>
>>>>
>>>> diff --git a/libavutil/cpu.c b/libavutil/cpu.c
>>>> index 52f6b9a3bf..ccd5b5adac 100644
>>>> --- a/libavutil/cpu.c
>>>> +++ b/libavutil/cpu.c
>>>
>>> 52f6b9a3bf is the state of cpu.c after
>>> e387fcd01cb84d9493f3b96158addd2a85f086c6. This is completely outdated.
>>
>> Yes, messed up branches for the patch, thx!
>>
>>
>>>>
>>>> @@ -306,14 +307,27 @@ int av_cpu_count(void)
>>>>      nb_cpus = sysinfo.dwNumberOfProcessors;
>>>>  #endif
>>>>  
>>>> +    int count = atomic_load_explicit(&cpu_count, memory_order_relaxed);
>>>> +
>>>>      if (!printed) {
>>>>          av_log(NULL, AV_LOG_DEBUG, "detected %d logical cores\n", nb_cpus);
>>>> +        if (count > 0) {
>>>> +            av_log(NULL, AV_LOG_DEBUG, "overriding to %d logical cores\n", count);
>>>> +        }
>>>>          printed = 1;
>>>>      }
>>>>  
>>>> +    if (count > 0) {
>>>> +        nb_cpus = count;
>>>> +    }
>>>> +
>>>>      return nb_cpus;
>>>>  }
>>>>  
>>> In particular, this hunk doesn't apply to git master at all any more
>>> (did I already mention that cpu.c only has 245 lines atm, not >300?).
>>> And the declaration of count would lead to a statement-after-declaration
>>> warning.
>>
>> Done locally to apply to today's HEAD. I think I'll wait with sending both patches once we decided on atomic loads in "[PATCH] Stop using _explicit atomic operations where not necessary.".
> 
> Discussion on atomic loads appear to have stalled mid-June... If there are no hard feeling, I'll push this version soon as the explicit (or not) loads can then be changed later.

Pushed, thanks!

-Thilo
James Almer July 16, 2021, 12:16 p.m. UTC | #9
On 6/6/2021 7:44 AM, Thilo Borgmann wrote:
> Hi,
> 
> Am 05.06.21 um 16:33 schrieb Anton Khirnov:
>> Quoting Thilo Borgmann (2021-06-05 14:29:05)
>>> Hi,
>>>
>>> add an option to override auto-detection of cpu count. Defaults to auto-detection, of course.
>>>
>>> -Thilo
>>>
>>>  From 38612f3e1339354dbaa6be4f36072320ff71c707 Mon Sep 17 00:00:00 2001
>>> From: Thilo Borgmann <thilo.borgmann@mail.de>
>>> Date: Sat, 5 Jun 2021 14:26:23 +0200
>>> Subject: [PATCH] fftools/cmdutils.c: Add cmd line option to override detection
>>>   of cpu count
>>>
>>> Suggested-By: ffmpeg@fb.com
>>> ---
>>>   doc/fftools-common-opts.texi |  7 +++++++
>>>   fftools/cmdutils.c           | 27 +++++++++++++++++++++++++++
>>>   fftools/cmdutils.h           |  7 +++++++
>>>   libavutil/cpu.c              | 14 ++++++++++++++
>>>   libavutil/cpu.h              |  6 ++++++
>>>   5 files changed, 61 insertions(+)
>>>
>>> [...]
> 
>>>   /**
>>>    * Fallback for options that are not explicitly handled, these will be
>>>    * parsed through AVOptions.
>>> @@ -239,6 +245,7 @@ void show_help_options(const OptionDef *options, const char *msg, int req_flags,
>>>       { "report",      0,                    { .func_arg = opt_report },       "generate a report" },                     \
>>>       { "max_alloc",   HAS_ARG,              { .func_arg = opt_max_alloc },    "set maximum size of a single allocated block", "bytes" }, \
>>>       { "cpuflags",    HAS_ARG | OPT_EXPERT, { .func_arg = opt_cpuflags },     "force specific cpu flags", "flags" },     \
>>> +    { "cpucount",    HAS_ARG | OPT_EXPERT, { .func_arg = opt_cpucount },     "force specific cpu count", "count" },     \
>>>       { "hide_banner", OPT_BOOL | OPT_EXPERT, {&hide_banner},     "do not show program banner", "hide_banner" },          \
>>>       CMDUTILS_COMMON_OPTIONS_AVDEVICE                                                                                    \
>>>   
>>> diff --git a/libavutil/cpu.c b/libavutil/cpu.c
>>> index 52f6b9a3bf..ccd5b5adac 100644
>>> --- a/libavutil/cpu.c
>>> +++ b/libavutil/cpu.c
>>> @@ -48,6 +48,7 @@
>>>   #endif
>>>   
>>>   static atomic_int cpu_flags = ATOMIC_VAR_INIT(-1);
>>> +static atomic_int cpu_count = ATOMIC_VAR_INIT(-1);
>>>   
>>>   static int get_cpu_flags(void)
>>>   {
>>> @@ -306,14 +307,27 @@ int av_cpu_count(void)
>>>       nb_cpus = sysinfo.dwNumberOfProcessors;
>>>   #endif
>>>   
>>> +    int count = atomic_load_explicit(&cpu_count, memory_order_relaxed);
>>
>> Just use a normal atomic_load().
>>
>>> [...]
> 
>>> +/**
>>> + * Overrides cpu count detection and forces the specified count.
>>> + * Count < 1 disables forcing of specific count.
>>> + */
>>> +void av_force_cpu_count(int count);
>>
>> I'd prefer if the name was namespaced as av_cpu_
> 
> For both of these, I copied the behavior of av_force_cpu_flags(). For function names as well as the atomic load.
> Let me know which way you prefer. If you prefer your reviewed version, I could also send a patch for similar changes to av_force_cpu_flags() if that makes sense.
> 
> All other remarks changed locally.

Looks like you did not rename it to av_cpu_ namespace.

> 
> Thanks,
> Thilo
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Thilo Borgmann July 19, 2021, 8:44 p.m. UTC | #10
Am 16.07.21 um 14:16 schrieb James Almer:
> On 6/6/2021 7:44 AM, Thilo Borgmann wrote:
>> Hi,
>>
>> Am 05.06.21 um 16:33 schrieb Anton Khirnov:
>>> Quoting Thilo Borgmann (2021-06-05 14:29:05)
>>>> Hi,
>>>>
>>>> add an option to override auto-detection of cpu count. Defaults to auto-detection, of course.
>>>>
>>>> -Thilo
>>>>
>>>>  From 38612f3e1339354dbaa6be4f36072320ff71c707 Mon Sep 17 00:00:00 2001
>>>> From: Thilo Borgmann <thilo.borgmann@mail.de>
>>>> Date: Sat, 5 Jun 2021 14:26:23 +0200
>>>> Subject: [PATCH] fftools/cmdutils.c: Add cmd line option to override detection
>>>>   of cpu count
>>>>
>>>> Suggested-By: ffmpeg@fb.com
>>>> ---
>>>>   doc/fftools-common-opts.texi |  7 +++++++
>>>>   fftools/cmdutils.c           | 27 +++++++++++++++++++++++++++
>>>>   fftools/cmdutils.h           |  7 +++++++
>>>>   libavutil/cpu.c              | 14 ++++++++++++++
>>>>   libavutil/cpu.h              |  6 ++++++
>>>>   5 files changed, 61 insertions(+)
>>>>
>>>> [...]
>>
>>>>   /**
>>>>    * Fallback for options that are not explicitly handled, these will be
>>>>    * parsed through AVOptions.
>>>> @@ -239,6 +245,7 @@ void show_help_options(const OptionDef *options, const char *msg, int req_flags,
>>>>       { "report",      0,                    { .func_arg = opt_report },       "generate a report" },                     \
>>>>       { "max_alloc",   HAS_ARG,              { .func_arg = opt_max_alloc },    "set maximum size of a single allocated block", "bytes" }, \
>>>>       { "cpuflags",    HAS_ARG | OPT_EXPERT, { .func_arg = opt_cpuflags },     "force specific cpu flags", "flags" },     \
>>>> +    { "cpucount",    HAS_ARG | OPT_EXPERT, { .func_arg = opt_cpucount },     "force specific cpu count", "count" },     \
>>>>       { "hide_banner", OPT_BOOL | OPT_EXPERT, {&hide_banner},     "do not show program banner", "hide_banner" },          \
>>>>       CMDUTILS_COMMON_OPTIONS_AVDEVICE                                                                                    \
>>>>   diff --git a/libavutil/cpu.c b/libavutil/cpu.c
>>>> index 52f6b9a3bf..ccd5b5adac 100644
>>>> --- a/libavutil/cpu.c
>>>> +++ b/libavutil/cpu.c
>>>> @@ -48,6 +48,7 @@
>>>>   #endif
>>>>     static atomic_int cpu_flags = ATOMIC_VAR_INIT(-1);
>>>> +static atomic_int cpu_count = ATOMIC_VAR_INIT(-1);
>>>>     static int get_cpu_flags(void)
>>>>   {
>>>> @@ -306,14 +307,27 @@ int av_cpu_count(void)
>>>>       nb_cpus = sysinfo.dwNumberOfProcessors;
>>>>   #endif
>>>>   +    int count = atomic_load_explicit(&cpu_count, memory_order_relaxed);
>>>
>>> Just use a normal atomic_load().
>>>
>>>> [...]
>>
>>>> +/**
>>>> + * Overrides cpu count detection and forces the specified count.
>>>> + * Count < 1 disables forcing of specific count.
>>>> + */
>>>> +void av_force_cpu_count(int count);
>>>
>>> I'd prefer if the name was namespaced as av_cpu_
>>
>> For both of these, I copied the behavior of av_force_cpu_flags(). For function names as well as the atomic load.
>> Let me know which way you prefer. If you prefer your reviewed version, I could also send a patch for similar changes to av_force_cpu_flags() if that makes sense.
>>
>> All other remarks changed locally.
> 
> Looks like you did not rename it to av_cpu_ namespace.

Indeed. Sent another patch to prefix them all to av_cpu_

Thanks,
Thilo
diff mbox series

Patch

diff --git a/doc/fftools-common-opts.texi b/doc/fftools-common-opts.texi
index 5260ecb8f3..7643dd8396 100644
--- a/doc/fftools-common-opts.texi
+++ b/doc/fftools-common-opts.texi
@@ -353,6 +353,13 @@  Possible flags for this option are:
 @end table
 @end table
 
+@item -cpucount @var{count} (@emph{global})
+Override detection of CPU count. This option is intended
+for testing. Do not use it unless you know what you're doing.
+@example
+ffmpeg -cpucount 2
+@end example
+
 @item -max_alloc @var{bytes}
 Set the maximum size limit for allocating a block on the heap by ffmpeg's
 family of malloc functions. Exercise @strong{extreme caution} when using
diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c
index 4eb68d2201..583a706e5d 100644
--- a/fftools/cmdutils.c
+++ b/fftools/cmdutils.c
@@ -72,6 +72,7 @@  AVDictionary *format_opts, *codec_opts, *resample_opts;
 static FILE *report_file;
 static int report_file_level = AV_LOG_DEBUG;
 int hide_banner = 0;
+int cpu_count = -1;
 
 enum show_muxdemuxers {
     SHOW_DEFAULT,
@@ -866,6 +867,32 @@  int opt_cpuflags(void *optctx, const char *opt, const char *arg)
     return 0;
 }
 
+int opt_cpucount(void *optctx, const char *opt, const char *arg)
+{
+    int ret;
+    int count;
+
+    static const AVOption opts[] = {
+        {"count", NULL, 0, AV_OPT_TYPE_INT, { .i64 = -1}, -1, INT_MAX, NULL},
+        {NULL},
+    };
+    static const AVClass class = {
+        .class_name = "cpucount",
+        .item_name  = av_default_item_name,
+        .option     = opts,
+        .version    = LIBAVUTIL_VERSION_INT,
+    };
+    const AVClass *pclass = &class;
+
+    ret = av_opt_eval_int(&pclass, opts, arg, &count);
+
+    if (!ret) {
+        av_force_cpu_count(count);
+    }
+
+    return ret;
+}
+
 int opt_loglevel(void *optctx, const char *opt, const char *arg)
 {
     const struct { const char *name; int level; } log_levels[] = {
diff --git a/fftools/cmdutils.h b/fftools/cmdutils.h
index 1917510589..29a45dd0be 100644
--- a/fftools/cmdutils.h
+++ b/fftools/cmdutils.h
@@ -50,6 +50,7 @@  extern AVDictionary *sws_dict;
 extern AVDictionary *swr_opts;
 extern AVDictionary *format_opts, *codec_opts, *resample_opts;
 extern int hide_banner;
+extern int cpu_count;
 
 /**
  * Register a program-specific cleanup routine.
@@ -88,6 +89,11 @@  void log_callback_help(void* ptr, int level, const char* fmt, va_list vl);
  */
 int opt_cpuflags(void *optctx, const char *opt, const char *arg);
 
+/**
+ * Override the cpucount.
+ */
+int opt_cpucount(void *optctx, const char *opt, const char *arg);
+
 /**
  * Fallback for options that are not explicitly handled, these will be
  * parsed through AVOptions.
@@ -239,6 +245,7 @@  void show_help_options(const OptionDef *options, const char *msg, int req_flags,
     { "report",      0,                    { .func_arg = opt_report },       "generate a report" },                     \
     { "max_alloc",   HAS_ARG,              { .func_arg = opt_max_alloc },    "set maximum size of a single allocated block", "bytes" }, \
     { "cpuflags",    HAS_ARG | OPT_EXPERT, { .func_arg = opt_cpuflags },     "force specific cpu flags", "flags" },     \
+    { "cpucount",    HAS_ARG | OPT_EXPERT, { .func_arg = opt_cpucount },     "force specific cpu count", "count" },     \
     { "hide_banner", OPT_BOOL | OPT_EXPERT, {&hide_banner},     "do not show program banner", "hide_banner" },          \
     CMDUTILS_COMMON_OPTIONS_AVDEVICE                                                                                    \
 
diff --git a/libavutil/cpu.c b/libavutil/cpu.c
index 52f6b9a3bf..ccd5b5adac 100644
--- a/libavutil/cpu.c
+++ b/libavutil/cpu.c
@@ -48,6 +48,7 @@ 
 #endif
 
 static atomic_int cpu_flags = ATOMIC_VAR_INIT(-1);
+static atomic_int cpu_count = ATOMIC_VAR_INIT(-1);
 
 static int get_cpu_flags(void)
 {
@@ -306,14 +307,27 @@  int av_cpu_count(void)
     nb_cpus = sysinfo.dwNumberOfProcessors;
 #endif
 
+    int count = atomic_load_explicit(&cpu_count, memory_order_relaxed);
+
     if (!printed) {
         av_log(NULL, AV_LOG_DEBUG, "detected %d logical cores\n", nb_cpus);
+        if (count > 0) {
+            av_log(NULL, AV_LOG_DEBUG, "overriding to %d logical cores\n", count);
+        }
         printed = 1;
     }
 
+    if (count > 0) {
+        nb_cpus = count;
+    }
+
     return nb_cpus;
 }
 
+void av_force_cpu_count(int count){
+    atomic_store_explicit(&cpu_count, count, memory_order_relaxed);
+}
+
 size_t av_cpu_max_align(void)
 {
     if (ARCH_MIPS)
diff --git a/libavutil/cpu.h b/libavutil/cpu.h
index 83099dd969..9c682ad50b 100644
--- a/libavutil/cpu.h
+++ b/libavutil/cpu.h
@@ -119,6 +119,12 @@  int av_parse_cpu_caps(unsigned *flags, const char *s);
  */
 int av_cpu_count(void);
 
+/**
+ * Overrides cpu count detection and forces the specified count.
+ * Count < 1 disables forcing of specific count.
+ */
+void av_force_cpu_count(int count);
+
 /**
  * Get the maximum data alignment that may be required by FFmpeg.
  *