[3/3] Allow loading the OpenH264 library dynamically

Message ID 1420039277-53673-3-git-send-email-martin@martin.st
State New
Headers show

Commit Message

Martin Storsjö Dec. 31, 2014, 3:21 p.m.
This simplifies use of the patent license, simplifying use with a
library that has been downloaded at runtime (making it possible
to actually load and run libavcodec before the corresponding
OpenH264 library exists).

I'm not sure if this is worthwhile doing though. It does make the
code (and configure) quite a bit more messy.
---
 configure                   |  8 +++++-
 libavcodec/libopenh264enc.c | 59 ++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 62 insertions(+), 5 deletions(-)

Comments

Luca Barbato Dec. 31, 2014, 3:41 p.m. | #1
On 31/12/14 16:21, Martin Storsjö wrote:
> This simplifies use of the patent license, simplifying use with a
> library that has been downloaded at runtime (making it possible
> to actually load and run libavcodec before the corresponding
> OpenH264 library exists).
>
> I'm not sure if this is worthwhile doing though. It does make the
> code (and configure) quite a bit more messy.

Doesn't look that bad IMHO.

lu
Derek Buitenhuis Dec. 31, 2014, 4:51 p.m. | #2
On 12/31/2014 3:21 PM, Martin Storsjö wrote:
> I'm not sure if this is worthwhile doing though. It does make the
> code (and configure) quite a bit more messy.

I am sort of inclined to agree.

It's not that this patch is bad per se, but it kinda sets the
precedent for much worse things like dynamically loading 8/10-bit
libx264 or libx265 (which just came up again like 2 days ago
too).

Nonetheless, comments follow.

> +enabled libopenh264_dyn   && { { check_header wels/codec_ver.h && enabled_any dlopen LoadLibrary; } ||
> +                                 die "ERROR: OpenH264 1.3 header not found, or dlopen/LoadLibrary not found"; }

I assume this means we already have a working dlopen check that
takes into account linking differences between platforms?

> +#if HAVE_LOADLIBRARY
> +#include <windows.h>
> +#endif
> +
> +#if HAVE_DLOPEN && !HAVE_LOADLIBRARY
> +#include <dlfcn.h>
> +#define LoadLibrary(x) dlopen(x, RTLD_NOW | RTLD_GLOBAL)
> +#define GetProcAddress dlsym
> +#define FreeLibrary dlclose
> +#endif

Not in config.h or something?

- Derek
Martin Storsjö Dec. 31, 2014, 5:03 p.m. | #3
On Wed, 31 Dec 2014, Derek Buitenhuis wrote:

> On 12/31/2014 3:21 PM, Martin Storsjö wrote:
>> I'm not sure if this is worthwhile doing though. It does make the
>> code (and configure) quite a bit more messy.
>
> I am sort of inclined to agree.
>
> It's not that this patch is bad per se, but it kinda sets the
> precedent for much worse things like dynamically loading 8/10-bit
> libx264 or libx265 (which just came up again like 2 days ago
> too).
>
> Nonetheless, comments follow.
>
>> +enabled libopenh264_dyn   && { { check_header wels/codec_ver.h && enabled_any dlopen LoadLibrary; } ||
>> +                                 die "ERROR: OpenH264 1.3 header not found, or dlopen/LoadLibrary not found"; }
>
> I assume this means we already have a working dlopen check that
> takes into account linking differences between platforms?

We do have checks for dlopen with and without -ldl already.

>> +#if HAVE_LOADLIBRARY
>> +#include <windows.h>
>> +#endif
>> +
>> +#if HAVE_DLOPEN && !HAVE_LOADLIBRARY
>> +#include <dlfcn.h>
>> +#define LoadLibrary(x) dlopen(x, RTLD_NOW | RTLD_GLOBAL)
>> +#define GetProcAddress dlsym
>> +#define FreeLibrary dlclose
>> +#endif
>
> Not in config.h or something?

It would probably indeed make sense to add a separate enable-item (so you 
don't need to check individually for dlopen and LoadLibrary, but just for 
the generic dynamic-loading feature) and a separate header that does this. 
This would simplify some code in libavformat/avisynth.c as well. That is, 
if the general concept seems useful enough.

// Martin
Timothy Gu Dec. 31, 2014, 5:09 p.m. | #4
On Wed, Dec 31, 2014 at 9:03 AM, Martin Storsjö <martin@martin.st> wrote:

> It would probably indeed make sense to add a separate enable-item (so you
> don't need to check individually for dlopen and LoadLibrary, but just for
> the generic dynamic-loading feature) and a separate header that does this.

+1. Wanted to do this some time ago but forgot.

Timothy
Luca Barbato Dec. 31, 2014, 5:25 p.m. | #5
On 31/12/14 17:51, Derek Buitenhuis wrote:
> On 12/31/2014 3:21 PM, Martin Storsjö wrote:
>> I'm not sure if this is worthwhile doing though. It does make the
>> code (and configure) quite a bit more messy.
>
> I am sort of inclined to agree.
>
> It's not that this patch is bad per se, but it kinda sets the
> precedent for much worse things like dynamically loading 8/10-bit
> libx264 or libx265 (which just came up again like 2 days ago
> too).

I had those in my wishlist since a while (actually something a little 
more gory) since sometimes I feel the need of it =p

lu
Derek Buitenhuis Dec. 31, 2014, 5:34 p.m. | #6
On 12/31/2014 5:25 PM, Luca Barbato wrote:
> I had those in my wishlist since a while (actually something a little 
> more gory) since sometimes I feel the need of it =p

I am liekly never going to accept to a patch to libx265.c to dynamically load
arbitrary libs, and *especially* not to load both at once (oh my the symbol
nightmares etc).

LD_LIBRARY_PATH works 100% fine with them, and this is indeed what distros do:

    http://sources.debian.net/src/x265/1.4-5/debian/x265-10bit.in/


In odorem suavitatis! Tu autem effugare, diabole; appropinquabit enim judicium Dei!

- Derek
Rémi Denis-Courmont Dec. 31, 2014, 5:51 p.m. | #7
Le 2014-12-31 20:34, Derek Buitenhuis a écrit :
> On 12/31/2014 5:25 PM, Luca Barbato wrote:
>> I had those in my wishlist since a while (actually something a 
>> little
>> more gory) since sometimes I feel the need of it =p
>
> I am liekly never going to accept to a patch to libx265.c to 
> dynamically load
> arbitrary libs, and *especially* not to load both at once (oh my the 
> symbol
> nightmares etc).

On the one hand, symbol nightmares only occur because of RTLD_GLOBAL. 
Yet I am not aware of any good reason to use RTLD_GLOBAL within 
libavcodec.

On the other hand, binary compatibility is peremptory. That pretty much 
excludes x264 and x265, and according to the comments in the previous 
patch, Cisco's OpenH264 as well.
Luca Barbato Dec. 31, 2014, 6:03 p.m. | #8
On 31/12/14 18:34, Derek Buitenhuis wrote:
> On 12/31/2014 5:25 PM, Luca Barbato wrote:
>> I had those in my wishlist since a while (actually something a little
>> more gory) since sometimes I feel the need of it =p
>
> I am liekly never going to accept to a patch to libx265.c to dynamically load
> arbitrary libs, and *especially* not to load both at once (oh my the symbol
> nightmares etc).

thus why I said something a little more gory ^^;

Patch

diff --git a/configure b/configure
index 5e163f9..581aea1 100755
--- a/configure
+++ b/configure
@@ -194,6 +194,7 @@  External library support:
   --enable-libopencore-amrwb enable AMR-WB decoding via libopencore-amrwb [no]
   --enable-libopencv       enable video filtering via libopencv [no]
   --enable-libopenh264     enable H264 encoding via OpenH264 [no]
+  --enable-libopenh264-dyn enable H264 encoding via a dynamically loaded OpenH264 [no]
   --enable-libopenjpeg     enable JPEG 2000 de/encoding via OpenJPEG [no]
   --enable-libopus         enable Opus de/encoding via libopus [no]
   --enable-libpulse        enable Pulseaudio input via libpulse [no]
@@ -1158,6 +1159,7 @@  EXTERNAL_LIBRARY_LIST="
     libopencore_amrwb
     libopencv
     libopenh264
+    libopenh264_dyn
     libopenjpeg
     libopus
     libpulse
@@ -1465,6 +1467,7 @@  SYSTEM_FUNCS="
     inet_aton
     isatty
     jack_port_get_latency_range
+    LoadLibrary
     localtime_r
     mach_absolute_time
     MapViewOfFile
@@ -2004,7 +2007,7 @@  libopencore_amrnb_decoder_deps="libopencore_amrnb"
 libopencore_amrnb_encoder_deps="libopencore_amrnb"
 libopencore_amrnb_encoder_select="audio_frame_queue"
 libopencore_amrwb_decoder_deps="libopencore_amrwb"
-libopenh264_encoder_deps="libopenh264"
+libopenh264_encoder_deps_any="libopenh264 libopenh264_dyn"
 libopenjpeg_decoder_deps="libopenjpeg"
 libopenjpeg_encoder_deps="libopenjpeg"
 libopus_decoder_deps="libopus"
@@ -4101,6 +4104,7 @@  check_func_headers windows.h CoTaskMemFree -lole32
 check_func_headers windows.h GetProcessAffinityMask
 check_func_headers windows.h GetProcessTimes
 check_func_headers windows.h GetSystemTimeAsFileTime
+check_func_headers windows.h LoadLibrary
 check_func_headers windows.h MapViewOfFile
 check_func_headers windows.h SetConsoleTextAttribute
 check_func_headers windows.h Sleep
@@ -4192,6 +4196,8 @@  enabled libopencore_amrnb && require libopencore_amrnb opencore-amrnb/interf_dec
 enabled libopencore_amrwb && require libopencore_amrwb opencore-amrwb/dec_if.h D_IF_init -lopencore-amrwb
 enabled libopencv         && require_pkg_config opencv opencv/cv.h cvCreateImageHeader
 enabled libopenh264       && require libopenh264 wels/codec_api.h WelsGetCodecVersion -lopenh264 -lstdc++
+enabled libopenh264_dyn   && { { check_header wels/codec_ver.h && enabled_any dlopen LoadLibrary; } ||
+                                 die "ERROR: OpenH264 1.3 header not found, or dlopen/LoadLibrary not found"; }
 enabled libopenjpeg       && { { check_header openjpeg.h && check_lib2 openjpeg.h opj_version -lopenjpeg -DOPJ_STATIC; } ||
                                { require_pkg_config libopenjpeg1 openjpeg.h opj_version -DOPJ_STATIC; } }
 enabled libopus           && require_pkg_config opus opus_multistream.h opus_multistream_decoder_create
diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
index 17aa75d..53726fb 100644
--- a/libavcodec/libopenh264enc.c
+++ b/libavcodec/libopenh264enc.c
@@ -30,8 +30,24 @@ 
 #include "avcodec.h"
 #include "internal.h"
 
+#if HAVE_LOADLIBRARY
+#include <windows.h>
+#endif
+
+#if HAVE_DLOPEN && !HAVE_LOADLIBRARY
+#include <dlfcn.h>
+#define LoadLibrary(x) dlopen(x, RTLD_NOW | RTLD_GLOBAL)
+#define GetProcAddress dlsym
+#define FreeLibrary dlclose
+#endif
+
 typedef struct SVCContext {
     const AVClass *av_class;
+#if CONFIG_LIBOPENH264_DYN
+    const char *libname;
+    void *lib;
+#endif
+    void (*destroy_func)(ISVCEncoder *ppEncoder);
     ISVCEncoder *encoder;
     int slice_mode;
     int loopfilter;
@@ -41,6 +57,9 @@  typedef struct SVCContext {
 #define OFFSET(x) offsetof(SVCContext, x)
 #define VE AV_OPT_FLAG_VIDEO_PARAM | AV_OPT_FLAG_ENCODING_PARAM
 static const AVOption options[] = {
+#if CONFIG_LIBOPENH264_DYN
+    { "openh264lib", "OpenH264 library name", OFFSET(libname), AV_OPT_TYPE_STRING, { 0 }, 0, 0, VE },
+#endif
     { "slice_mode", "Slice mode", OFFSET(slice_mode), AV_OPT_TYPE_INT, { .i64 = SM_AUTO_SLICE }, SM_SINGLE_SLICE, SM_RESERVED, VE, "slice_mode" },
     { "fixed", "", 0, AV_OPT_TYPE_CONST, { .i64 = SM_FIXEDSLCNUM_SLICE }, 0, 0, VE, "slice_mode" },
     { "rowmb", "", 0, AV_OPT_TYPE_CONST, { .i64 = SM_ROWMB_SLICE }, 0, 0, VE, "slice_mode" },
@@ -58,8 +77,12 @@  static av_cold int svc_encode_close(AVCodecContext *avctx)
 {
     SVCContext *s = avctx->priv_data;
 
-    if (s->encoder)
-        WelsDestroySVCEncoder(s->encoder);
+    if (s->encoder && s->destroy_func)
+        s->destroy_func(s->encoder);
+#if CONFIG_LIBOPENH264_DYN
+    if (s->lib)
+        FreeLibrary(s->lib);
+#endif
     return 0;
 }
 
@@ -68,14 +91,42 @@  static av_cold int svc_encode_init(AVCodecContext *avctx)
     SVCContext *s = avctx->priv_data;
     SEncParamExt param = { 0 };
     int err = AVERROR_UNKNOWN;
-    OpenH264Version libver = WelsGetCodecVersion();
+    OpenH264Version libver;
+    int (*create_func)(ISVCEncoder **ppEncoder);
+    OpenH264Version (*get_version_func)(void);
+
+#if CONFIG_LIBOPENH264_DYN
+    if (!s->libname) {
+        av_log(avctx, AV_LOG_ERROR, "No library name provided\n");
+        return AVERROR(EINVAL);
+    }
+    s->lib = LoadLibrary(s->libname);
+    if (!s->lib) {
+        av_log(avctx, AV_LOG_ERROR, "Unable to load %s\n", s->libname);
+        return AVERROR(EINVAL);
+    }
+    create_func      = GetProcAddress(s->lib, "WelsCreateSVCEncoder");
+    s->destroy_func  = GetProcAddress(s->lib, "WelsDestroySVCEncoder");
+    get_version_func = GetProcAddress(s->lib, "WelsGetCodecVersion");
+
+    if (!create_func || !s->destroy_func || !get_version_func) {
+        av_log(avctx, AV_LOG_ERROR, "%s doesn't contain the necessary functions\n", s->libname);
+        FreeLibrary(s->lib);
+        return AVERROR(EINVAL);
+    }
+#else
+    create_func = WelsCreateSVCEncoder;
+    get_version_func = WelsGetCodecVersion;
+    s->destroy_func = WelsDestroySVCEncoder;
+#endif
 
+    libver = get_version_func();
     if (memcmp(&libver, &g_stCodecVersion, sizeof(libver))) {
         av_log(avctx, AV_LOG_ERROR, "Incorrect library version loaded\n");
         return AVERROR(EINVAL);
     }
 
-    if (WelsCreateSVCEncoder(&s->encoder)) {
+    if (create_func(&s->encoder)) {
         av_log(avctx, AV_LOG_ERROR, "Unable to create encoder\n");
         return AVERROR_UNKNOWN;
     }