[FFmpeg-devel] [PATCH v5] avcodec/mfenc: Dynamically load MFPlat.DLL

[Martin Storsjö]

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