| Message ID | 1420039277-53673-3-git-send-email-martin@martin.st |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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
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.
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 ^^;
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; }