[FFmpeg-devel] [PATCH] winrt: multithreading support

[Matt Oliver]

On 2 October 2015 at 05:14, Hendrik Leppkes <h.leppkes at gmail.com> wrote:

> On Thu, Oct 1, 2015 at 9:05 PM, wm4 <nfxjfg at googlemail.com> wrote:
> > On Fri, 2 Oct 2015 02:58:52 +0800
> > Wang Bin <wbsecg1 at gmail.com> wrote:
> >
> >> From b8b5ad2d65107781116666c2a03ae5cfbe727ee6 Mon Sep 17 00:00:00 2001
> >> From: wang-bin <wbsecg1 at gmail.com>
> >> Date: Tue, 29 Sep 2015 18:11:03 +0800
> >> Subject: [PATCH] winrt: multithreading support
> >>
> >> _beginthreadex is for desktop only. CreateThread is available for
> windows store apps on windows (and phone) 8.1 and later.
> http://msdn.microsoft.com/en-us/library/ms682453%28VS.85%29.aspx
> >> ---
> >>  compat/w32pthreads.h | 14 ++++++++++++++
> >>  configure            |  4 ++++
> >>  libavutil/cpu.c      | 14 +++++++++++++-
> >>  3 files changed, 31 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/compat/w32pthreads.h b/compat/w32pthreads.h
> >> index deb1c53..7491cab 100644
> >> --- a/compat/w32pthreads.h
> >> +++ b/compat/w32pthreads.h
> >> @@ -37,7 +37,16 @@
> >>
> >>  #define WIN32_LEAN_AND_MEAN
> >>  #include <windows.h>
> >> +#ifdef WINAPI_FAMILY
> >> +#include <winapifamily.h>
> >> +#if !WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_DESKTOP)
> >> +#define TARGET_OS_WINRT
> >> +#endif
> >> +#endif
> >> +#ifndef TARGET_OS_WINRT
> >>  #include <process.h>
> >> +#endif
> >> +
> >>
> >>  #include "libavutil/attributes.h"
> >>  #include "libavutil/common.h"
> >> @@ -82,8 +91,13 @@ static av_unused int pthread_create(pthread_t
> *thread, const void *unused_attr,
> >>  {
> >>      thread->func   = start_routine;
> >>      thread->arg    = arg;
> >> +#ifndef TARGET_OS_WINRT
> >>      thread->handle = (void*)_beginthreadex(NULL, 0,
> win32thread_worker, thread,
> >>                                             0, NULL);
> >> +#else
> >> +    thread->handle = (void*)CreateThread(NULL, 0, win32thread_worker,
> thread,
> >> +                                           0, NULL);
> >> +#endif
> >
> > Why can't it always use CreateThread? This looks very suspicious.
>
> MSDN warns about using CreateThread with C code, something with the
> CRT init, and recommend _beginthreadex instead.
> Using CreateThread on WinRT seems a bit fishy for the same reason.
>

CreateThread is just a Win32 call. _beginthreadex is better as it adds
additional setup for the C runtime as well as wrapping a call to
CreateThread. Without the C runtime initialisation then standard CRT
functions can mem-leak and otherwise misbehave. So _beginthreadex is
definitely the correct option most of the time. However when creating
windows store/phone apps _beginthreadex is not available and CreateThread
is the only other option. The above issues are overcome by ensuring that
you link against the dynamic release microsoft c runtime dll, which is also
a requirement for windows store/phone apps (but not one you want to require
any other time) so it should work itself out.

So the above check for WINAPI_PARTITION_DESKTOP will only enable
CreateThread when building a store/phone app which is I think is the only
way to go (blame microsoft for annoying requirements).


>
>
>
> >>      return !thread->handle;
> >>  }
> >>
> >> diff --git a/configure b/configure
> >> index 361c024..08d0d5d 100755
> >> --- a/configure
> >> +++ b/configure
> >> @@ -5189,6 +5189,10 @@ check_type "vdpau/vdpau.h" "VdpPictureInfoHEVC"
> >>  if ! disabled w32threads && ! enabled pthreads; then
> >>      check_func_headers "windows.h process.h" _beginthreadex &&
> >>          enable w32threads || disable w32threads
> >> +    if ! enabled w32threads; then
> >> +        check_func_headers "windows.h" CreateThread &&
> >> +            enable w32threads || disable w32threads
> >> +    fi
> >>  fi
> >>
>

This is a little ambiguous. The assumption that the only time
_beginthreadex is not available and CreateThread is available is when
building a store/phone app may not always be correct. If for some obscure
reason _beginthreadex is not available and its not an app build then the
w32pthread code will fail. For clarity you should probably add to this a
check !WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_DESKTOP) and CreateThread
before enabling.

Also the big pile of ifdefary that is added to w32pthread.h and cpu.c can
probably be avoided by adding another option to configure (something like
HAVE_WINRT) that is enabled with a check of !WINAPI_FAMILY_PARTITION(
WINAPI_PARTITION_DESKTOP). Then this should be used above so the check for
CreateThread is only when using WinRT. Having this available will also help
in the future as any additional WinRT changes are required.


> >>  # check for some common methods of building with pthread support
> >> diff --git a/libavutil/cpu.c b/libavutil/cpu.c
> >> index 780368d..c562e86 100644
> >> --- a/libavutil/cpu.c
> >> +++ b/libavutil/cpu.c
> >> @@ -30,8 +30,14 @@
> >>  #endif
> >>  #include <sched.h>
> >>  #endif
> >> -#if HAVE_GETPROCESSAFFINITYMASK
> >> +#if HAVE_WINDOWS_H
> >>  #include <windows.h>
> >> +#ifdef WINAPI_FAMILY
> >> +#include <winapifamily.h>
> >> +#if !WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_DESKTOP)
> >> +#define TARGET_OS_WINRT
> >> +#endif
> >> +#endif
> >>  #endif
> >>  #if HAVE_SYSCTL
> >>  #if HAVE_SYS_PARAM_H
> >> @@ -253,6 +259,9 @@ int av_cpu_count(void)
> >>      static volatile int printed;
> >>
> >>      int nb_cpus = 1;
> >> +#ifdef TARGET_OS_WINRT
> >> +    SYSTEM_INFO sysinfo;
> >> +#endif
> >>  #if HAVE_SCHED_GETAFFINITY && defined(CPU_COUNT)
> >>      cpu_set_t cpuset;
> >>
> >> @@ -274,6 +283,9 @@ int av_cpu_count(void)
> >>      nb_cpus = sysconf(_SC_NPROC_ONLN);
> >>  #elif HAVE_SYSCONF && defined(_SC_NPROCESSORS_ONLN)
> >>      nb_cpus = sysconf(_SC_NPROCESSORS_ONLN);
> >> +#elif defined(TARGET_OS_WINRT)
> >> +    GetNativeSystemInfo(&sysinfo);
> >> +    nb_cpus = sysinfo.dwNumberOfProcessors;
> >>  #endif
> >
> > You could avoid the first ifdef by opening a new scope.
> >
> > What's the difference to GetProcessAffinityMask() (which is apparently
> > not available on WinRT)?
>
> GetProcessAffinityMask takes into account the settings the user makes,
> ie. he could tell the process to always run on a particular set of
> cores only.
>

As stated above. GetProcessAffinityMask is not available in WinRT as with
apps the user shouldn't have control to change things like thread affinity.
Which is why we have to use GetNativeSystemInfo on WinRT but should
continue to use GetProcessAffinityMask for all else.


>
> >
> >>
> >>      if (!printed) {
> >
>