[FFmpeg-devel] [PATCH] Avoid creating unecessary dependencies on thread libraries.

[Hendrik Leppkes]

On Mon, Nov 21, 2016 at 5:23 PM, Gregory J Wolfe
<gregory.wolfe at kodakalaris.com> wrote:
>> -----Original Message-----
>> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On
>> Behalf Of wm4
>> Sent: Monday, November 21, 2016 10:05 AM
>> To: ffmpeg-devel at ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH] Avoid creating unecessary
>> dependencies on thread libraries.
>>
>> On Mon, 21 Nov 2016 09:58:33 -0500
>> "Gregory J. Wolfe" <gregory.wolfe at kodakalaris.com> wrote:
>>
>> > (1) Multi-threading support requires knowing the number of CPUs
>> available.
>> > When building with MinGW on a Windows system, both gcc and
>> Windows run
>> > time functions are available to get this information.  However, when
>> Windows
>> > threading has been selected, the Windows function should be used,
>> not the
>> > gcc function.  This avoids creating an unnecessary dependency on
>> the gcc
>> > thread library.
>> >
>> > (2) When ALL threading support is disabled, the build should not
>> create a
>> > dependency on ANY thread library.
>> >
>> > Signed-off-by: Gregory J. Wolfe <gregory.wolfe at kodakalaris.com>
>> > ---
>> >  libavutil/cpu.c | 8 +++++++-
>> >  1 file changed, 7 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/libavutil/cpu.c b/libavutil/cpu.c
>> > index f5785fc..3843778 100644
>> > --- a/libavutil/cpu.c
>> > +++ b/libavutil/cpu.c
>> > @@ -258,10 +258,15 @@ int av_cpu_count(void)
>> >      static volatile int printed;
>> >
>> >      int nb_cpus = 1;
>> > +#if HAVE_THREADS
>> >  #if HAVE_WINRT
>> >      SYSTEM_INFO sysinfo;
>> >  #endif
>> > -#if HAVE_SCHED_GETAFFINITY && defined(CPU_COUNT)
>> > +    // if HAVE_W32THREADS and HAVE_GETPROCESSAFFINITYMASK,
>> we will use
>> > +    // Windows GetProcessAffinityMask() instead of gcc library
>> function
>> > +    // sched_getaffinity().  This avoids creating a dependency on the
>> gcc
>> > +    // thread library that we don't need/want.
>> > +#if HAVE_SCHED_GETAFFINITY && defined(CPU_COUNT) &&
>> !(HAVE_W32THREADS && HAVE_GETPROCESSAFFINITYMASK)
>> >      cpu_set_t cpuset;
>> >
>> >      CPU_ZERO(&cpuset);
>> > @@ -286,6 +291,7 @@ int av_cpu_count(void)
>> >      GetNativeSystemInfo(&sysinfo);
>> >      nb_cpus = sysinfo.dwNumberOfProcessors;
>> >  #endif
>> > +#endif
>> >
>> >      if (!printed) {
>> >          av_log(NULL, AV_LOG_DEBUG, "detected %d logical cores\n",
>> nb_cpus);
>>
>> Wouldn't it be better to move the HAVE_GETPROCESSAFFINITYMASK
>> case
>> above HAVE_SCHED_GETAFFINITY, and avoid making the ifdeffery
>> mess
>> worse? GetProcessAffinityMask is available on every supported
>> Windows
>> and could as well be replaced by "ifdef _WIN32" instead.
>>
>
> Yeah, I had the same thought.  For the first go around I opted for the
> solution that moved around the least code, not knowing for sure if
> I might cause some undesirable side effects by reordering the existing
> code.  I can easily redo the patch if we're in agreement on this point.
>

Moving the code to prefer the win32 functions if available is
definitely preferable to further complicating the ifdefs to not only
check for features but also doing negative checks on other features.
The preferred option should just be on top - which in this case is the
OS-native function over the pthread functions, which are always some
sort of emulation layer ontop of the win32 functions anyway.

- Hendrik