Re: [PATCH] mfmediaengine: Add IMFMediaEngine stub.

[Nikolay Sivov]

On 9/3/19 4:38 PM, Jactry Zeng wrote:
> Signed-off-by: Jactry Zeng
>
> dlls/mfmediaengine/main.c                | 482 ++++++++++++++++++++++-
>   dlls/mfmediaengine/tests/Makefile.in     |   2 +-
>   dlls/mfmediaengine/tests/mfmediaengine.c | 152 +++++++
>   include/mfmediaengine.idl                |  51 +++
>   4 files changed, 684 insertions(+), 3 deletions(-)
>
> diff --git a/dlls/mfmediaengine/main.c b/dlls/mfmediaengine/main.c
> index 4bca0fe0ba..9e33b973d4 100644
> --- a/dlls/mfmediaengine/main.c
> +++ b/dlls/mfmediaengine/main.c
> @@ -24,8 +24,11 @@
>   #include "winbase.h"
>
> #include "mfmediaengine.h"
> +#include "mferror.h"
> +#include "dxgi.h"
>
> #include "wine/debug.h"
> +#include "wine/heap.h"
>
> WINE_DEFAULT_DEBUG_CHANNEL(mfplat);
>
> @@ -43,6 +46,432 @@ BOOL WINAPI DllMain(HINSTANCE instance, DWORD reason, LPVOID reserved)
>       return TRUE;
>   }
>
> +enum media_engine_mode
> +{
> - MEDIA_ENGINE_INVIAL,
> - MEDIA_ENGINE_AUDIO_MODE,
> - MEDIA_ENGINE_RENDERING_MODE,
> - MEDIA_ENGINE_FRAME_SERVER_MODE,
>
> +};

Where does this come from? Is _INVIAL a typo?
> -
>
> +struct media_engine_attributes
> +{
> - IMFMediaEngineNotify *callback;
> - HWND playback_hwnd;
> - DXGI_FORMAT output_format;
> - IMFDXGIDeviceManager *dxgi_manager;
> - enum media_engine_mode mode;
>
> +};
> -
>
> +struct media_engine
> +{
> - IMFMediaEngine IMFMediaEngine_iface;
> - LONG refcount;
> - DWORD flags;
> - struct media_engine_attributes attributes;
>
> +};
> -

Do you need these structures separate? You can still have 
init_media_engine() + heap_alloc_zero() in CreateInstance().
> -
>
> +static ULONG WINAPI media_engine_Release(IMFMediaEngine *iface)
> +{
> - struct media_engine *engine = impl_from_IMFMediaEngine(iface);
> - ULONG refcount = InterlockedDecrement(&engine->refcount);
> -
> - TRACE("(%p) ref=%u.\n", iface, refcount);
> -
> - if (!refcount)
> - {
> -
>    free_media_engine_attributes(engine->attributes);
> -
>    HeapFree(GetProcessHeap(), 0, engine);
> - }
> -
> - return refcount;
>
> +}

heap_free(), but that's not very important.
> -
>
> +static double WINAPI media_engine_GetCurrentTime(IMFMediaEngine *iface)
> +{
> - FIXME("(%p): stub.\n", iface);
> -
> - return 0;
>
> +}

This should use proper float constant.
> -
>
> +static BOOL WINAPI media_engine_IsPaused(IMFMediaEngine *iface)
> +{
> - FIXME("(%p): stub.\n", iface);
> -
> - return 0;
>
> +}

0 -> FALSE.
> +static BOOL WINAPI media_engine_GetAutoPlay(IMFMediaEngine *iface)
> +{
> - FIXME("(%p): stub.\n", iface);
> -
> - return E_NOTIMPL;
>
> +}

Wrong return value.
> static HRESULT WINAPI media_engine_factory_QueryInterface(IMFMediaEngineClassFactory *iface, REFIID riid, void **obj)
>   {
>       if (IsEqualIID(riid, &IID_IMFMediaEngineClassFactory) ||
> @@ -68,12 +497,61 @@ static ULONG WINAPI media_engine_factory_Release(IMFMediaEngineClassFactory *ifa
>       return 1;
>   }
>
> +static void init_media_engine_attributes(IMFAttributes *attributes, struct media_engine_attributes *engine_attr)
> +{
> - HRESULT hr;
> -
> - memset(engine_attr, 0, sizeof(*engine_attr));
> - hr = IMFAttributes_GetUnknown(attributes, &MF_MEDIA_ENGINE_CALLBACK, &IID_IMFMediaEngineNotify,
> -
>                              (void **)&engine_attr->callback);
> - if (FAILED(hr))
> -
>    return;
> -
> - IMFAttributes_GetUINT64(attributes, &MF_MEDIA_ENGINE_PLAYBACK_HWND, (UINT64 *)&engine_attr->playback_hwnd);

This will be writing 8 bytes to 4 bytes destination, for 32 bit build.
> - IMFAttributes_GetUnknown(attributes, &MF_MEDIA_ENGINE_DXGI_MANAGER, &IID_IMFDXGIDeviceManager,
> -
>                         (void **)&engine_attr->dxgi_manager);
> - hr = IMFAttributes_GetUINT32(attributes, &MF_MEDIA_ENGINE_VIDEO_OUTPUT_FORMAT, &engine_attr->output_format);
> - if (engine_attr->playback_hwnd) /* FIXME: handle MF_MEDIA_ENGINE_PLAYBACK_VISUAL */
> -
>    engine_attr->mode = MEDIA_ENGINE_RENDERING_MODE;
> - else
> - {
> -
>    if (SUCCEEDED(hr))
> -
>        engine_attr->mode = MEDIA_ENGINE_FRAME_SERVER_MODE;
> -
>    else
> -
>        engine_attr->mode = MEDIA_ENGINE_AUDIO_MODE;
> - }
>
> +}
> - static HRESULT WINAPI media_engine_factory_CreateInstance(IMFMediaEngineClassFactory *iface, DWORD flags,
>                                                           IMFAttributes *attributes, IMFMediaEngine **engine)
> {
> - FIXME("(%p, %#x, %p, %p): stub.\n", iface, flags, attributes, engine);
> - struct media_engine_attributes engine_attr;
> - struct media_engine *object;
> - return E_NOTIMPL;
> - FIXME("(%p, %#x, %p, %p): semi-stub.\n", iface, flags, attributes, engine);

This one could as well be promoted to TRACE().
> static HRESULT WINAPI media_engine_factory_CreateTimeRange(IMFMediaEngineClassFactory *iface,
> diff --git a/dlls/mfmediaengine/tests/Makefile.in b/dlls/mfmediaengine/tests/Makefile.in
> index 3f2bb5a094..d508db9f6f 100644
> --- a/dlls/mfmediaengine/tests/Makefile.in
> +++ b/dlls/mfmediaengine/tests/Makefile.in
> @@ -1,5 +1,5 @@
>   TESTDLL   = mfmediaengine.dll
> -IMPORTS   = ole32 mfplat mfmediaengine mfuuid
> +IMPORTS   = ole32 mfplat mfmediaengine mfuuid uuid
>
> C_SRCS = \
>    mfmediaengine.c
> diff --git a/dlls/mfmediaengine/tests/mfmediaengine.c b/dlls/mfmediaengine/tests/mfmediaengine.c
> index c7fc89f2b1..a264d0d033 100644
> --- a/dlls/mfmediaengine/tests/mfmediaengine.c
> +++ b/dlls/mfmediaengine/tests/mfmediaengine.c
> @@ -24,11 +24,85 @@
>   #include "winbase.h"
>
> #include "mfapi.h"
> +#include "mfidl.h"
>   #include "mfmediaengine.h"
> +#include "mferror.h"
> +#include "dxgi.h"
> +#include "initguid.h"
>
> #include "wine/heap.h"
>   #include "wine/test.h"
>
> +static HRESULT (WINAPI *pMFCreateDXGIDeviceManager)(UINT *token, IMFDXGIDeviceManager **manager);
> -
>
> +#define EXPECT_REF(obj,ref) _expect_ref((IUnknown*)obj, ref, __LINE__)
> +static void _expect_ref(IUnknown *obj, ULONG ref, int line)
> +{
> - ULONG rc;
> - IUnknown_AddRef(obj);
> - rc = IUnknown_Release(obj);
> - ok_(__FILE__,line)(rc == ref, "Unexpected refcount %d, expected %d.\n", rc, ref);
>
> +}
> -
>
> +static void init_functions(void)
> +{
> - HMODULE mod = GetModuleHandleA("mfplat.dll");
> -
>
> +#define X(f) p##f = (void*)GetProcAddress(mod, #f)
> - X(MFCreateDXGIDeviceManager);
>
> +#undef X
> +}
> -
>
> +struct media_engine_notify
> +{
> - IMFMediaEngineNotify IMFMediaEngineNotify_iface;
> - LONG refcount;
>
> +};
> -
>
> +static inline struct media_engine_notify *impl_from_IMFMediaEngineNotify(IMFMediaEngineNotify *iface)
> +{
> - return CONTAINING_RECORD(iface, struct media_engine_notify, IMFMediaEngineNotify_iface);
>
> +}
> -
>
> +static HRESULT WINAPI media_engine_notify_QueryInterface(IMFMediaEngineNotify *iface, REFIID riid, void **obj)
> +{
> - if (IsEqualIID(riid, &IID_IMFMediaEngineNotify) ||
> -
>    IsEqualIID(riid, &IID_IUnknown))
> - {
> -
>    *obj = iface;
> -
>    IMFMediaEngineNotify_AddRef(iface);
> -
>    return S_OK;
> - }
> -
> - *obj = NULL;
> - return E_NOINTERFACE;
>
> +}
> -
>
> +static ULONG WINAPI media_engine_notify_AddRef(IMFMediaEngineNotify *iface)
> +{
> - struct media_engine_notify *notify = impl_from_IMFMediaEngineNotify(iface);
> - return InterlockedIncrement(&notify->refcount);
>
> +}
> -
>
> +static ULONG WINAPI media_engine_notify_Release(IMFMediaEngineNotify *iface)
> +{
> - struct media_engine_notify *notify = impl_from_IMFMediaEngineNotify(iface);
> - return InterlockedDecrement(&notify->refcount);
>
> +}
> -
>
> +static HRESULT WINAPI media_engine_notify_EventNotify(IMFMediaEngineNotify *iface, DWORD event, DWORD_PTR param1, DWORD param2)
> +{
> - return S_OK;
>
> +}
> -
>
> +static IMFMediaEngineNotifyVtbl media_engine_notify_vtbl =
> +{
> - media_engine_notify_QueryInterface,
> - media_engine_notify_AddRef,
> - media_engine_notify_Release,
> - media_engine_notify_EventNotify,
>
> +};
> - static void test_factory(void)
> {
>     IMFMediaEngineClassFactory *factory, *factory2;
>
> @@ -47,7 +121,83 @@ static void test_factory(void)
>          "Unexpected hr %#x.\n", hr);
>   if (factory)
> - {

Let's return early on !(factory) and indent everything that follows back 
one step.
> -
>    struct media_engine_notify notify_impl = {{&media_engine_notify_vtbl}, 1};
> -
>    IMFMediaEngineNotify *notify = &notify_impl.IMFMediaEngineNotify_iface;
> -
>    IMFMediaEngine *media_engine, *media_engine2;
> -
>    IMFDXGIDeviceManager *manager;
> -
>    IMFAttributes *attributes;
> -
>    UINT token;
> -
> -
>    hr = pMFCreateDXGIDeviceManager(&token, &manager);
> -
>    ok(hr == S_OK, "MFCreateDXGIDeviceManager failed: %#x.\n", hr);
> -
>    hr = MFCreateAttributes(&attributes, 3);
> -
>    ok(hr == S_OK, "MFCreateAttributes failed: %#x.\n", hr);
> -
> -
>    if (0)
> -
>    {
> -
>        /* Crashed on Windows 8 */
> -
>        hr = IMFMediaEngineClassFactory_CreateInstance(factory, MF_MEDIA_ENGINE_WAITFORSTABLE_STATE,
> -
>                                                       NULL, &media_engine);
> -
>        ok(hr == E_POINTER, "IMFMediaEngineClassFactory_CreateInstance got %#x.\n", hr);
> -
> -
>        hr = IMFMediaEngineClassFactory_CreateInstance(factory, MF_MEDIA_ENGINE_WAITFORSTABLE_STATE,
> -
>                                                       NULL, NULL);
> -
>        ok(hr == E_POINTER, "IMFMediaEngineClassFactory_CreateInstance got %#x.\n", hr);
> -
>    }

I think you can remove this crashing test.
> -
> -
>    hr = IMFMediaEngineClassFactory_CreateInstance(factory, MF_MEDIA_ENGINE_WAITFORSTABLE_STATE,
> -
>                                                   attributes, &media_engine);
> -
>    ok(hr == MF_E_ATTRIBUTENOTFOUND, "IMFMediaEngineClassFactory_CreateInstance got %#x.\n", hr);
> -
> -
>    hr = IMFAttributes_SetUnknown(attributes, &MF_MEDIA_ENGINE_OPM_HWND, NULL);
> -
>    ok(hr == S_OK, "IMFAttributes_SetUnknown failed: %#x.\n", hr);
> -
>    hr = IMFMediaEngineClassFactory_CreateInstance(factory, MF_MEDIA_ENGINE_WAITFORSTABLE_STATE,
> -
>                                                   attributes, &media_engine);
> -
>    ok(hr == MF_E_ATTRIBUTENOTFOUND, "IMFMediaEngineClassFactory_CreateInstance got %#x.\n", hr);
> -
> -
>    hr = IMFAttributes_SetUnknown(attributes, &MF_MEDIA_ENGINE_CALLBACK, (IUnknown *)notify);
> -
>    ok(hr == S_OK, "IMFAttributes_SetUnknown failed: %#x.\n", hr);
> -
>    hr = IMFMediaEngineClassFactory_CreateInstance(factory, MF_MEDIA_ENGINE_WAITFORSTABLE_STATE,
> -
>                                                   attributes, &media_engine);
> -
>    ok(hr == S_OK || broken(hr == MF_E_ATTRIBUTENOTFOUND) /* pre win10v1809 */,
> -
>       "IMFMediaEngineClassFactory_CreateInstance got %#x.\n", hr);
> -
>    if (SUCCEEDED(hr))
> -
>        IMFMediaEngine_Release(media_engine);

Let's remove this one too and test only with format attribute.
> -
> -
>    EXPECT_REF(factory, 1);
> -
>    hr = IMFAttributes_SetUINT32(attributes, &MF_MEDIA_ENGINE_VIDEO_OUTPUT_FORMAT, DXGI_FORMAT_UNKNOWN);
> -
>    ok(hr == S_OK, "IMFAttributes_SetUINT32 failed: %#x.\n", hr);
> -
>    hr = IMFMediaEngineClassFactory_CreateInstance(factory, MF_MEDIA_ENGINE_WAITFORSTABLE_STATE,
> -
>                                                   attributes, &media_engine);
> -
>    ok(hr == S_OK, "IMFMediaEngineClassFactory_CreateInstance failed: %#x.\n", hr);
> -
>    if (0)
> -
>    {
> -
>        /* Different version of Windows has different reference count behaviour, and I didn't
> -
>           see their logics. So skipping these tests on Windows. */
> -
>        EXPECT_REF(media_engine, 1);
> -
>        EXPECT_REF(factory, 1);
> -
>    }

If it's inconsistent there is no point in having such dead statements.
> -
> -
>    hr = IMFMediaEngineClassFactory_CreateInstance(factory, MF_MEDIA_ENGINE_WAITFORSTABLE_STATE,
> -
>                                                   attributes, &media_engine2);
> -
>    ok(hr == S_OK, "IMFMediaEngineClassFactory_CreateInstance failed: %#x.\n", hr);
> -
>    if (0)
> -
>    {
> -
>        EXPECT_REF(media_engine, 1);
> -
>        EXPECT_REF(media_engine2, 1);
> -
>        EXPECT_REF(factory, 1);
> -
>    }

Same here.
> -
> -
>    IMFMediaEngine_Release(media_engine);
> -
>    IMFMediaEngine_Release(media_engine2);
> -
>    IMFAttributes_DeleteAllItems(attributes);
> -
>    IMFAttributes_Release(attributes);

You don't have to delete items by hand.