| Message ID | a97a6596-0921-f4a1-edc4-b8384dbc79f2@tytanium.xyz |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,v5] avcodec/mfenc: Dynamically load MFPlat.DLL | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/configure_loongarch64 | warning | Failed to apply patch |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_armv7_RPi4 | success | Make finished |
| andriy/make_fate_armv7_RPi4 | success | Make fate finished |
On Sat, 21 May 2022, Trystan Mata wrote: > Changes since the v4: > - Avoid having two mf_init function declared (missing #if !HAVE_UWP) > - Better commit message about linking with UWP > > Changes since the v3: > - Library handle and function pointer are no longer in MFContext. > - If UWP is enabled, avcodec will be linked directly against MFPlat.DLL. > - MediaFoundation functions are now called like MFTEnumEx, like Martin > Storsjö suggested in his review of the v3. > > I forgot to mention it on earlier versions, this patch addresses > https://trac.ffmpeg.org/ticket/9788. diff --git a/libavcodec/mf_utils.c b/libavcodec/mf_utils.c index eeabd0ce0b..b8756cccea 100644 --- a/libavcodec/mf_utils.c +++ b/libavcodec/mf_utils.c @@ -24,6 +24,7 @@ #include "mf_utils.h" #include "libavutil/pixdesc.h" +#include "compat/w32dlfcn.h" HRESULT ff_MFGetAttributeSize(IMFAttributes *pattr, REFGUID guid, UINT32 *pw, UINT32 *ph) @@ -47,9 +48,83 @@ HRESULT ff_MFSetAttributeSize(IMFAttributes *pattr, REFGUID guid, #define ff_MFSetAttributeRatio ff_MFSetAttributeSize #define ff_MFGetAttributeRatio ff_MFGetAttributeSize -// MFTEnumEx was missing from mingw-w64's mfplat import library until -// mingw-w64 v6.0.0, thus wrap it and load it using GetProcAddress. -// It's also missing in Windows Vista's mfplat.dll. +// Windows N editions does not provide MediaFoundation by default. +// So to avoid DLL loading error, MediaFoundation is dynamically loaded except +// on UWP build since LoadLibrary is not available on it. +#if !HAVE_UWP +static HMODULE mf_library = NULL; + +int ff_mf_load_library(void *log) +{ + mf_library = win32_dlopen("mfplat.dll"); + + if (!mf_library) { + av_log(log, AV_LOG_ERROR, "DLL mfplat.dll failed to open\n"); + return AVERROR_UNKNOWN; + } + + return 0; +} + +void ff_mf_unload_library(void) +{ + FreeLibrary(mf_library); + mf_library = NULL; +} You shouldn't use win32_dlopen directly (there's no preexisting code that does that). If you want to use the helpers from w32dlfcn.h, call dlopen and dlclose, which then redirect to those functions. But here I don't see any need to use that wrapper when you probably could just call LoadLibrary directly. (There's no need for using wchar APIs and long path handling and all that, when it's just given the hardcoded path "mfplat.dll".) Secondly, this won't probably work as intended if you have two mfenc instances running at the same time - you'd load the library twice but only unload it once. To work around that, you can either add some sort of reference counting around the library, or keep the library reference in a per-encoder struct. In this case I think I would prefer to keep it in a struct. + +#define GET_MF_FUNCTION(ptr, func_name) \ + ptr = (void *)GetProcAddress(mf_library, #func_name ""); \ Why the extra "" here? + if (!ptr) \ + return E_FAIL; +#else +// In UWP (which lacks LoadLibrary), just link directly against +// the functions - this requires building with new/complete enough +// import libraries. +#define GET_MF_FUNCTION(ptr, func_name) \ + ptr = func_name; \ + if (!ptr) \ + return E_FAIL; +#endif + +HRESULT ff_MFStartup(ULONG Version, DWORD dwFlags) +{ + HRESULT (WINAPI *MFStartup_ptr)(ULONG Version, DWORD dwFlags) = NULL; + GET_MF_FUNCTION(MFStartup_ptr, MFStartup); For functions like this, fetching the function pointer every time it's called probably is fine (as it's only called once on startup), but ... + return MFStartup_ptr(Version, dwFlags); +} + +HRESULT ff_MFShutdown(void) +{ + HRESULT (WINAPI *MFShutdown_ptr)(void) = NULL; + GET_MF_FUNCTION(MFShutdown_ptr, MFShutdown); + return MFShutdown_ptr(); +} + +HRESULT ff_MFCreateSample(IMFSample **ppIMFSample) +{ + HRESULT (WINAPI *MFCreateSample_ptr)(IMFSample **ppIMFSample) = NULL; + GET_MF_FUNCTION(MFCreateSample_ptr, MFCreateSample); This is called once per frame at least, right? For such cases I think it would be much better if we'd keep the function pointer in a struct (which then would be owned/contained in MFContext) so we don't need to look it up every time. @@ -1034,7 +1034,11 @@ static int mf_create(void *log, IMFTransform **mft, const AVCodec *codec, int us return 0; } +#if !HAVE_UWP +static int mf_init_encoder(AVCodecContext *avctx) +#else static int mf_init(AVCodecContext *avctx) +#endif { MFContext *c = avctx->priv_data; HRESULT hr; @@ -1134,6 +1138,10 @@ static int mf_close(AVCodecContext *avctx) ff_free_mf(&c->mft); +#if !HAVE_UWP + ff_mf_unload_library(); +#endif + av_frame_free(&c->frame); av_freep(&avctx->extradata); @@ -1142,6 +1150,21 @@ static int mf_close(AVCodecContext *avctx) return 0; } +#if !HAVE_UWP +static int mf_init(AVCodecContext *avctx) +{ + int ret; + + if ((ret = ff_mf_load_library(avctx)) == 0) { + if ((ret = mf_init_encoder(avctx)) == 0) { + return 0; + } + } + mf_close(avctx); + return ret; +} +#endif + This feels a bit messy with the same function defined differently between the desktop/UWP cases. Wouldn't it be more straightforward to just keep all the code for the desktop case, and add ifdefs within e.g. ff_mf_load_library and ff_mf_unload_library, so that those functions are simple no-ops if building for UWP? // Martin
Firstly, thank you for your review. > You shouldn't use win32_dlopen directly (there's no preexisting code that > does that). If you want to use the helpers from w32dlfcn.h, call dlopen > and dlclose, which then redirect to those functions. But here I don't see > any need to use that wrapper when you probably could just call LoadLibrary > directly. (There's no need for using wchar APIs and long path handling and > all that, when it's just given the hardcoded path "mfplat.dll".) Ok, I will switch totally only LoadLibrary calls. > Secondly, this won't probably work as intended if you have two mfenc > instances running at the same time - you'd load the library twice but only > unload it once. To work around that, you can either add some sort of > reference counting around the library, or keep the library reference in a > per-encoder struct. > > In this case I think I would prefer to keep it in a struct. > This is called once per frame at least, right? For such cases I think it > would be much better if we'd keep the function pointer in a struct (which > then would be owned/contained in MFContext) so we don't need to look it up > every time. I completely missed this. I'm not used to dynamic loading. I will fix this by putting back the library handle and function ptrs in the MFContext struct. I think I will put each looked up symbol in the struct. So every MF function is loaded the same way. > This feels a bit messy with the same function defined differently between > the desktop/UWP cases. Wouldn't it be more straightforward to just keep > all the code for the desktop case, and add ifdefs within e.g. > ff_mf_load_library and ff_mf_unload_library, so that those functions are > simple no-ops if building for UWP? I will fix that, thank you again for review. // Trystan
Firstly, thank you for your review. > You shouldn't use win32_dlopen directly (there's no preexisting code that > does that). If you want to use the helpers from w32dlfcn.h, call dlopen > and dlclose, which then redirect to those functions. But here I don't see > any need to use that wrapper when you probably could just call LoadLibrary > directly. (There's no need for using wchar APIs and long path handling and > all that, when it's just given the hardcoded path "mfplat.dll".) Ok, I will switch totally only LoadLibrary calls. > Secondly, this won't probably work as intended if you have two mfenc > instances running at the same time - you'd load the library twice but only > unload it once. To work around that, you can either add some sort of > reference counting around the library, or keep the library reference in a > per-encoder struct. > > In this case I think I would prefer to keep it in a struct. > This is called once per frame at least, right? For such cases I think it > would be much better if we'd keep the function pointer in a struct (which > then would be owned/contained in MFContext) so we don't need to look it up > every time. I completely missed this. I'm not used to dynamic loading. I will fix this by putting back the library handle and function ptrs in the MFContext struct. I think I will put each looked up symbol in the struct. So every MF function is loaded the same way. > This feels a bit messy with the same function defined differently between > the desktop/UWP cases. Wouldn't it be more straightforward to just keep > all the code for the desktop case, and add ifdefs within e.g. > ff_mf_load_library and ff_mf_unload_library, so that those functions are > simple no-ops if building for UWP? I will fix that, thank you again for review. // Trystan
From 9115df2346e23d2e3eff4fb78cbd80419267eb7d Mon Sep 17 00:00:00 2001 From: Trystan Mata <trystan.mata@tytanium.xyz> Date: Sat, 21 May 2022 16:26:56 +0200 Subject: [PATCH] avcodec/mfenc: Dynamically load MediaFoundation library Allows non-UWP builds of FFmpeg with MediaFoundation to work on N editions of Windows which are without MediaFoundation by default. On UWP target, avcodec is link directly against MediaFoundation since LoadLibrary is not available. This commit adresses https://trac.ffmpeg.org/ticket/9788 Signed-off-by: Trystan Mata <trystan.mata@tytanium.xyz> --- configure | 5 +- libavcodec/mf_utils.c | 106 ++++++++++++++++++++++++++++++++++-------- libavcodec/mf_utils.h | 19 ++++++-- libavcodec/mfenc.c | 25 +++++++++- 4 files changed, 131 insertions(+), 24 deletions(-) diff --git a/configure b/configure index f115b21064..432a0d163d 100755 --- a/configure +++ b/configure @@ -3130,7 +3130,6 @@ wmv3_vdpau_hwaccel_select="vc1_vdpau_hwaccel" # hardware-accelerated codecs mediafoundation_deps="mftransform_h MFCreateAlignedMemoryBuffer" -mediafoundation_extralibs="-lmfplat -lmfuuid -lole32 -lstrmiids" omx_deps="libdl pthreads" omx_rpi_select="omx" qsv_deps="libmfx" @@ -6876,6 +6875,10 @@ test_cpp <<EOF && enable uwp && d3d11va_extralibs="-ldxgi -ld3d11" #endif EOF +# mediafoundation requires linking directly to mfplat and mfuuid if building +# for uwp target +enabled uwp && mediafoundation_extralibs="-lmfplat -lmfuuid -lole32 -lstrmiids" || mediafoundation_extralibs="-lole32 -lstrmiids" + enabled libdrm && check_pkg_config libdrm_getfb2 libdrm "xf86drmMode.h" drmModeGetFB2 diff --git a/libavcodec/mf_utils.c b/libavcodec/mf_utils.c index eeabd0ce0b..b8756cccea 100644 --- a/libavcodec/mf_utils.c +++ b/libavcodec/mf_utils.c @@ -24,6 +24,7 @@ #include "mf_utils.h" #include "libavutil/pixdesc.h" +#include "compat/w32dlfcn.h" HRESULT ff_MFGetAttributeSize(IMFAttributes *pattr, REFGUID guid, UINT32 *pw, UINT32 *ph) @@ -47,9 +48,83 @@ HRESULT ff_MFSetAttributeSize(IMFAttributes *pattr, REFGUID guid, #define ff_MFSetAttributeRatio ff_MFSetAttributeSize #define ff_MFGetAttributeRatio ff_MFGetAttributeSize -// MFTEnumEx was missing from mingw-w64's mfplat import library until -// mingw-w64 v6.0.0, thus wrap it and load it using GetProcAddress. -// It's also missing in Windows Vista's mfplat.dll. +// Windows N editions does not provide MediaFoundation by default. +// So to avoid DLL loading error, MediaFoundation is dynamically loaded except +// on UWP build since LoadLibrary is not available on it. +#if !HAVE_UWP +static HMODULE mf_library = NULL; + +int ff_mf_load_library(void *log) +{ + mf_library = win32_dlopen("mfplat.dll"); + + if (!mf_library) { + av_log(log, AV_LOG_ERROR, "DLL mfplat.dll failed to open\n"); + return AVERROR_UNKNOWN; + } + + return 0; +} + +void ff_mf_unload_library(void) +{ + FreeLibrary(mf_library); + mf_library = NULL; +} + +#define GET_MF_FUNCTION(ptr, func_name) \ + ptr = (void *)GetProcAddress(mf_library, #func_name ""); \ + if (!ptr) \ + return E_FAIL; +#else +// In UWP (which lacks LoadLibrary), just link directly against +// the functions - this requires building with new/complete enough +// import libraries. +#define GET_MF_FUNCTION(ptr, func_name) \ + ptr = func_name; \ + if (!ptr) \ + return E_FAIL; +#endif + +HRESULT ff_MFStartup(ULONG Version, DWORD dwFlags) +{ + HRESULT (WINAPI *MFStartup_ptr)(ULONG Version, DWORD dwFlags) = NULL; + GET_MF_FUNCTION(MFStartup_ptr, MFStartup); + return MFStartup_ptr(Version, dwFlags); +} + +HRESULT ff_MFShutdown(void) +{ + HRESULT (WINAPI *MFShutdown_ptr)(void) = NULL; + GET_MF_FUNCTION(MFShutdown_ptr, MFShutdown); + return MFShutdown_ptr(); +} + +HRESULT ff_MFCreateSample(IMFSample **ppIMFSample) +{ + HRESULT (WINAPI *MFCreateSample_ptr)(IMFSample **ppIMFSample) = NULL; + GET_MF_FUNCTION(MFCreateSample_ptr, MFCreateSample); + return MFCreateSample_ptr(ppIMFSample); +} + +HRESULT ff_MFCreateAlignedMemoryBuffer(DWORD cbMaxLength, DWORD cbAligment, + IMFMediaBuffer **ppBuffer) +{ + HRESULT (WINAPI *MFCreateAlignedMemoryBuffer_ptr)(DWORD cbMaxLength, + DWORD cbAligment, + IMFMediaBuffer **ppBuffer) = NULL; + GET_MF_FUNCTION(MFCreateAlignedMemoryBuffer_ptr, MFCreateAlignedMemoryBuffer); + return MFCreateAlignedMemoryBuffer_ptr(cbMaxLength, cbAligment, ppBuffer); +} + +HRESULT ff_MFCreateMediaType(IMFMediaType **ppMFType) +{ + HRESULT (WINAPI *MFCreateMediaType_ptr)(IMFMediaType **ppMFType) = NULL; + GET_MF_FUNCTION(MFCreateMediaType_ptr, MFCreateMediaType); + return MFCreateMediaType_ptr(ppMFType); +} + +// MFTEnumEx is missing in Windows Vista's mfplat.dll. HRESULT ff_MFTEnumEx(GUID guidCategory, UINT32 Flags, const MFT_REGISTER_TYPE_INFO *pInputType, const MFT_REGISTER_TYPE_INFO *pOutputType, @@ -60,18 +135,7 @@ HRESULT ff_MFTEnumEx(GUID guidCategory, UINT32 Flags, const MFT_REGISTER_TYPE_INFO *pOutputType, IMFActivate ***pppMFTActivate, UINT32 *pnumMFTActivate) = NULL; -#if !HAVE_UWP - HANDLE lib = GetModuleHandleW(L"mfplat.dll"); - if (lib) - MFTEnumEx_ptr = (void *)GetProcAddress(lib, "MFTEnumEx"); -#else - // In UWP (which lacks GetModuleHandle), just link directly against - // the functions - this requires building with new/complete enough - // import libraries. - MFTEnumEx_ptr = MFTEnumEx; -#endif - if (!MFTEnumEx_ptr) - return E_FAIL; + GET_MF_FUNCTION(MFTEnumEx_ptr, MFTEnumEx); return MFTEnumEx_ptr(guidCategory, Flags, pInputType, @@ -112,13 +176,13 @@ IMFSample *ff_create_memory_sample(void *fill_data, size_t size, size_t align) IMFSample *sample; IMFMediaBuffer *buffer; - hr = MFCreateSample(&sample); + hr = ff_MFCreateSample(&sample); if (FAILED(hr)) return NULL; align = FFMAX(align, 16); // 16 is "recommended", even if not required - hr = MFCreateAlignedMemoryBuffer(size, align - 1, &buffer); + hr = ff_MFCreateAlignedMemoryBuffer(size, align - 1, &buffer); if (FAILED(hr)) return NULL; @@ -561,7 +625,7 @@ static int init_com_mf(void *log) return AVERROR(ENOSYS); } - hr = MFStartup(MF_VERSION, MFSTARTUP_FULL); + hr = ff_MFStartup(MF_VERSION, MFSTARTUP_FULL); if (FAILED(hr)) { av_log(log, AV_LOG_ERROR, "could not initialize MediaFoundation\n"); CoUninitialize(); @@ -573,7 +637,7 @@ static int init_com_mf(void *log) static void uninit_com_mf(void) { - MFShutdown(); + ff_MFShutdown(); CoUninitialize(); } @@ -673,6 +737,10 @@ error_uninit_mf: void ff_free_mf(IMFTransform **mft) { +#if !HAVE_UWP + if (!mf_library) + return; +#endif if (*mft) IMFTransform_Release(*mft); *mft = NULL; diff --git a/libavcodec/mf_utils.h b/libavcodec/mf_utils.h index d514723c3b..6384bc2133 100644 --- a/libavcodec/mf_utils.h +++ b/libavcodec/mf_utils.h @@ -50,9 +50,22 @@ HRESULT ff_MFSetAttributeSize(IMFAttributes *pattr, REFGUID guid, #define ff_MFSetAttributeRatio ff_MFSetAttributeSize #define ff_MFGetAttributeRatio ff_MFGetAttributeSize -// MFTEnumEx was missing from mingw-w64's mfplat import library until -// mingw-w64 v6.0.0, thus wrap it and load it using GetProcAddress. -// It's also missing in Windows Vista's mfplat.dll. +// Windows N editions does not provide MediaFoundation by default. +// So to avoid DLL loading error, MediaFoundation is dynamically loaded except +// on UWP build since LoadLibrary is not available on it. +#if !HAVE_UWP +int ff_mf_load_library(void *log); +void ff_mf_unload_library(void); +#endif + +HRESULT ff_MFCreateSample(IMFSample **ppIMFSample); +HRESULT ff_MFCreateAlignedMemoryBuffer(DWORD cbMaxLength, DWORD cbAligment, + IMFMediaBuffer **ppBuffer); +HRESULT ff_MFStartup(ULONG Version, DWORD dwFlags); +HRESULT ff_MFShutdown(void); +HRESULT ff_MFCreateMediaType(IMFMediaType **ppMFType); + +// MFTEnumEx is missing in Windows Vista's mfplat.dll. HRESULT ff_MFTEnumEx(GUID guidCategory, UINT32 Flags, const MFT_REGISTER_TYPE_INFO *pInputType, const MFT_REGISTER_TYPE_INFO *pOutputType, diff --git a/libavcodec/mfenc.c b/libavcodec/mfenc.c index 280941cf2e..60815455b7 100644 --- a/libavcodec/mfenc.c +++ b/libavcodec/mfenc.c @@ -777,7 +777,7 @@ static int mf_choose_output_type(AVCodecContext *avctx) if (out_type) { av_log(avctx, AV_LOG_VERBOSE, "picking output type %d.\n", out_type_index); } else { - hr = MFCreateMediaType(&out_type); + hr = ff_MFCreateMediaType(&out_type); if (FAILED(hr)) { ret = AVERROR(ENOMEM); goto done; @@ -1034,7 +1034,11 @@ static int mf_create(void *log, IMFTransform **mft, const AVCodec *codec, int us return 0; } +#if !HAVE_UWP +static int mf_init_encoder(AVCodecContext *avctx) +#else static int mf_init(AVCodecContext *avctx) +#endif { MFContext *c = avctx->priv_data; HRESULT hr; @@ -1134,6 +1138,10 @@ static int mf_close(AVCodecContext *avctx) ff_free_mf(&c->mft); +#if !HAVE_UWP + ff_mf_unload_library(); +#endif + av_frame_free(&c->frame); av_freep(&avctx->extradata); @@ -1142,6 +1150,21 @@ static int mf_close(AVCodecContext *avctx) return 0; } +#if !HAVE_UWP +static int mf_init(AVCodecContext *avctx) +{ + int ret; + + if ((ret = ff_mf_load_library(avctx)) == 0) { + if ((ret = mf_init_encoder(avctx)) == 0) { + return 0; + } + } + mf_close(avctx); + return ret; +} +#endif + #define OFFSET(x) offsetof(MFContext, x) #define MF_ENCODER(MEDIATYPE, NAME, ID, OPTS, EXTRA) \ -- 2.36.1
Changes since the v4: - Avoid having two mf_init function declared (missing #if !HAVE_UWP) - Better commit message about linking with UWP Changes since the v3: - Library handle and function pointer are no longer in MFContext. - If UWP is enabled, avcodec will be linked directly against MFPlat.DLL. - MediaFoundation functions are now called like MFTEnumEx, like Martin Storsjö suggested in his review of the v3. I forgot to mention it on earlier versions, this patch addresses https://trac.ffmpeg.org/ticket/9788. // Trystan