[1/3] Generalize loading of dynamic libraries across unix and windows

Message ID 1468579396-35501-1-git-send-email-martin@martin.st
State New
Headers show

Commit Message

Martin Storsjö July 15, 2016, 10:43 a.m.
---
 configure                |  8 ++++++--
 libavformat/avisynth.c   | 14 +++++---------
 libavutil/dyn_lib_open.h | 39 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 50 insertions(+), 11 deletions(-)
 create mode 100644 libavutil/dyn_lib_open.h

Comments

Luca Barbato July 15, 2016, 1:21 p.m. | #1
On 15/07/16 12:43, Martin Storsjö wrote:
> ---
>  configure                |  8 ++++++--
>  libavformat/avisynth.c   | 14 +++++---------
>  libavutil/dyn_lib_open.h | 39 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 50 insertions(+), 11 deletions(-)
>  create mode 100644 libavutil/dyn_lib_open.h

Seems fine.
Diego Biurrun July 25, 2016, 11:01 a.m. | #2
On Fri, Jul 15, 2016 at 01:43:14PM +0300, Martin Storsjö wrote:
> --- a/configure
> +++ b/configure
> @@ -4565,9 +4566,12 @@ for func in $MATH_FUNCS; do
>  
> +# This needs to be evaluated here instead of via the generic mechanism
> +# (setting dyn_lib_open_deps_any) since we want it set here
> +enabled_any dlopen LoadLibrary && enable dyn_lib_open || disable dyn_lib_open

I'm not convinced this is necessary or better than adding dyn_libs_open
to avisynth_demuxer_deps.

>  # these are off by default, so fail if requested and not available
> -enabled avisynth          && { check_lib2 "avisynth/avisynth_c.h windows.h" LoadLibrary ||
> -                               check_lib2 "avxsynth/avxsynth_c.h dlfcn.h" dlopen -ldl   ||
> +enabled avisynth          && { { check_header "avisynth/avisynth_c.h" && enabled dyn_lib_open; } ||
>                                 die "ERROR: LoadLibrary/dlopen not found, or avisynth header not found"; }

Did you just drop support for avxsynth?

Also, checking for dyn_lib_open first would slightly speed up configure
when dyn_lib_open is not available.

Diego
Martin Storsjö July 25, 2016, 11:13 a.m. | #3
On Mon, 25 Jul 2016, Diego Biurrun wrote:

> On Fri, Jul 15, 2016 at 01:43:14PM +0300, Martin Storsjö wrote:
>> --- a/configure
>> +++ b/configure
>> @@ -4565,9 +4566,12 @@ for func in $MATH_FUNCS; do
>> 
>> +# This needs to be evaluated here instead of via the generic mechanism
>> +# (setting dyn_lib_open_deps_any) since we want it set here
>> +enabled_any dlopen LoadLibrary && enable dyn_lib_open || disable dyn_lib_open
>
> I'm not convinced this is necessary or better than adding dyn_libs_open
> to avisynth_demuxer_deps.

Hmm, what behaviour would you get then, if you do e.g. --enable-avisynth 
but don't have dyn_libs_open available - would you get a configure error, 
or a build without avisynth?

I don't particularly mind all that much though.

>>  # these are off by default, so fail if requested and not available
>> -enabled avisynth          && { check_lib2 "avisynth/avisynth_c.h windows.h" LoadLibrary ||
>> -                               check_lib2 "avxsynth/avxsynth_c.h dlfcn.h" dlopen -ldl   ||
>> +enabled avisynth          && { { check_header "avisynth/avisynth_c.h" && enabled dyn_lib_open; } ||
>>                                 die "ERROR: LoadLibrary/dlopen not found, or avisynth header not found"; }
>
> Did you just drop support for avxsynth?

Whoops, will amend.

> Also, checking for dyn_lib_open first would slightly speed up configure
> when dyn_lib_open is not available.

I think that's ricing, optimizing for the case when this enters the fail 
path. But the better question is which of them looks better in the code.

E.g. if one does it like this:

enabled avisynth          && { { { check_header "avisynth/avisynth_c.h" ||
                                    check_header "avxsynth/avxsynth_c.h"; } &&
                                  enabled dyn_lib_open; } ||
                                die "ERROR: LoadLibrary/dlopen not found, or avisynth header not found"; }

I guess it doesn't matter much if checking the headers or dyn_lib_open 
first.

// Martin
Diego Biurrun July 26, 2016, 1:02 p.m. | #4
On Mon, Jul 25, 2016 at 02:13:12PM +0300, Martin Storsjö wrote:
> On Mon, 25 Jul 2016, Diego Biurrun wrote:
> > On Fri, Jul 15, 2016 at 01:43:14PM +0300, Martin Storsjö wrote:
> >> --- a/configure
> >> +++ b/configure
> >> @@ -4565,9 +4566,12 @@ for func in $MATH_FUNCS; do
> >> 
> >> +# This needs to be evaluated here instead of via the generic mechanism
> >> +# (setting dyn_lib_open_deps_any) since we want it set here
> >> +enabled_any dlopen LoadLibrary && enable dyn_lib_open || disable dyn_lib_open
> >
> > I'm not convinced this is necessary or better than adding dyn_libs_open
> > to avisynth_demuxer_deps.
> 
> Hmm, what behaviour would you get then, if you do e.g. --enable-avisynth 
> but don't have dyn_libs_open available - would you get a configure error, 
> or a build without avisynth?

Depends on the avisynth check; right now a configure error.  In many
cases, however, we do fail silently.

> > Also, checking for dyn_lib_open first would slightly speed up configure
> > when dyn_lib_open is not available.
> 
> I think that's ricing, optimizing for the case when this enters the fail 
> path.

It certainly is.  But I'm starting to worry about configure speed as it
does not run in parallel.  I worry that at some point we might be like
small autotools projects where configure time dwarfs (parallel) make
time.

> But the better question is which of them looks better in the code.
> 
> E.g. if one does it like this:
> 
> enabled avisynth          && { { { check_header "avisynth/avisynth_c.h" ||
>                                     check_header "avxsynth/avxsynth_c.h"; } &&
>                                   enabled dyn_lib_open; } ||
>                                 die "ERROR: LoadLibrary/dlopen not found, or avisynth header not found"; }
> 
> I guess it doesn't matter much if checking the headers or dyn_lib_open 
> first.

Yes, pretty much the same.

Diego

Patch

diff --git a/configure b/configure
index ce52f50..a7671b7 100755
--- a/configure
+++ b/configure
@@ -1673,6 +1673,7 @@  CONFIG_EXTRA="
     cabac
     dirac_parse
     dvprofile
+    dyn_lib_open
     faandct
     faanidct
     fdctdsp
@@ -4565,9 +4566,12 @@  for func in $MATH_FUNCS; do
     eval check_mathfunc $func \${${func}_args:-1}
 done
 
+# This needs to be evaluated here instead of via the generic mechanism
+# (setting dyn_lib_open_deps_any) since we want it set here
+enabled_any dlopen LoadLibrary && enable dyn_lib_open || disable dyn_lib_open
+
 # these are off by default, so fail if requested and not available
-enabled avisynth          && { check_lib2 "avisynth/avisynth_c.h windows.h" LoadLibrary ||
-                               check_lib2 "avxsynth/avxsynth_c.h dlfcn.h" dlopen -ldl   ||
+enabled avisynth          && { { check_header "avisynth/avisynth_c.h" && enabled dyn_lib_open; } ||
                                die "ERROR: LoadLibrary/dlopen not found, or avisynth header not found"; }
 enabled cuda              && check_lib cuda.h cuInit -lcuda
 enabled frei0r            && { check_header frei0r.h || die "ERROR: frei0r.h header not found"; }
diff --git a/libavformat/avisynth.c b/libavformat/avisynth.c
index fe71a42..d6d28e9 100644
--- a/libavformat/avisynth.c
+++ b/libavformat/avisynth.c
@@ -19,6 +19,7 @@ 
  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
  */
 
+#include "libavutil/dyn_lib_open.h"
 #include "libavutil/internal.h"
 #include "libavcodec/internal.h"
 #include "avformat.h"
@@ -36,14 +37,9 @@ 
   #define AVISYNTH_LIB "avisynth"
   #define USING_AVISYNTH
 #else
-  #include <dlfcn.h>
   #include <avxsynth/avxsynth_c.h>
   #define AVISYNTH_NAME "libavxsynth"
   #define AVISYNTH_LIB AVISYNTH_NAME SLIBSUF
-
-  #define LoadLibrary(x) dlopen(x, RTLD_NOW | RTLD_LOCAL)
-  #define GetProcAddress dlsym
-  #define FreeLibrary dlclose
 #endif
 
 typedef struct AviSynthLibrary {
@@ -113,13 +109,13 @@  static av_cold void avisynth_atexit_handler(void);
 
 static av_cold int avisynth_load_library(void)
 {
-    avs_library.library = LoadLibrary(AVISYNTH_LIB);
+    avs_library.library = load_library(AVISYNTH_LIB);
     if (!avs_library.library)
         return AVERROR_UNKNOWN;
 
 #define LOAD_AVS_FUNC(name, continue_on_fail)                          \
         avs_library.name =                                             \
-            (void *)GetProcAddress(avs_library.library, #name);        \
+            (void *)get_function(avs_library.library, #name);          \
         if (!continue_on_fail && !avs_library.name)                    \
             goto fail;
 
@@ -154,7 +150,7 @@  static av_cold int avisynth_load_library(void)
     return 0;
 
 fail:
-    FreeLibrary(avs_library.library);
+    free_library(avs_library.library);
     return AVERROR_UNKNOWN;
 }
 
@@ -222,7 +218,7 @@  static av_cold void avisynth_atexit_handler(void)
         avisynth_context_destroy(avs);
         avs = next;
     }
-    FreeLibrary(avs_library.library);
+    free_library(avs_library.library);
 
     avs_atexit_called = 1;
 }
diff --git a/libavutil/dyn_lib_open.h b/libavutil/dyn_lib_open.h
new file mode 100644
index 0000000..d69d169
--- /dev/null
+++ b/libavutil/dyn_lib_open.h
@@ -0,0 +1,39 @@ 
+/*
+ * This file is part of Libav.
+ *
+ * Libav is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * Libav is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with Libav; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#ifndef AVUTIL_DYN_LIB_OPEN_H
+#define AVUTIL_DYN_LIB_OPEN_H
+
+#include "config.h"
+
+#if HAVE_LOADLIBRARY
+#include <windows.h>
+
+#define load_library LoadLibrary
+#define get_function GetProcAddress
+#define free_library FreeLibrary
+
+#elif HAVE_DLOPEN
+#include <dlfcn.h>
+
+#define load_library(x) dlopen(x, RTLD_NOW | RTLD_LOCAL)
+#define get_function dlsym
+#define free_library dlclose
+#endif
+
+#endif /* AVUTIL_DYN_LIB_OPEN_H */