[PATCH v10 2/5] wmp: Add media completion notifications

[Jacek Caban]

Hi Anton,

On 04/06/2018 09:22 PM, Anton Romanov wrote:
> +BOOL init_player(WindowsMediaPlayer *wmp)
>  {
> +    InitOnceExecuteOnce(&class_init_once, register_player_msg_class, NULL, NULL);
> +    wmp->msg_window = CreateWindowW( MAKEINTRESOURCEW(player_msg_class), NULL, 0, 0,
> +            0, 0, 0, HWND_MESSAGE, 0, wmp_instance, wmp );
> +    if (!wmp->msg_window) {
> +        ERR("Failed to create message window, GetLastError: %d\n", GetLastError());
> +        return FALSE;
> +    }
> +    if (!WM_WMPEVENT) {
> +        WM_WMPEVENT= RegisterWindowMessageW(WMPmessageW);
> +    }

This could be moved to register_player_msg_class since we already use
INIT_ONCE for that.

> +    if (!WM_WMPEVENT) {
> +        ERR("Failed to register window message, GetLastError: %d\n", GetLastError());
> +        return FALSE;
> +    }
> +
>      wmp->IWMPPlayer4_iface.lpVtbl = &WMPPlayer4Vtbl;
>      wmp->IWMPPlayer_iface.lpVtbl = &WMPPlayerVtbl;
>      wmp->IWMPSettings_iface.lpVtbl = &WMPSettingsVtbl;
> @@ -1834,6 +1922,7 @@ void init_player(WindowsMediaPlayer *wmp)
>  
>      wmp->invoke_urls = VARIANT_TRUE;
>      wmp->auto_start = VARIANT_TRUE;
> +    return TRUE;
>  }
>  
>  void destroy_player(WindowsMediaPlayer *wmp)
> @@ -1841,6 +1930,7 @@ void destroy_player(WindowsMediaPlayer *wmp)
>      IWMPControls_stop(&wmp->IWMPControls_iface);
>      if(wmp->wmpmedia)
>          IWMPMedia_Release(wmp->wmpmedia);
> +    DestroyWindow(wmp->msg_window);
>  }
>  
>  WMPMedia *unsafe_impl_from_IWMPMedia(IWMPMedia *iface)
> diff --git a/dlls/wmp/tests/media.c b/dlls/wmp/tests/media.c
> index b33be4fa20..3774d475fb 100644
> --- a/dlls/wmp/tests/media.c
> +++ b/dlls/wmp/tests/media.c
> @@ -65,9 +65,11 @@ DEFINE_EXPECT(PLAYSTATE);
>  DEFINE_EXPECT(OPENSTATE);
>  
>  static HANDLE playing_event;
> +static HANDLE completed_event;
>  static DWORD main_thread_id;
>  
>  static const WCHAR mp3file[] = {'t','e','s','t','.','m','p','3',0};
> +static const WCHAR mp3file1s[] = {'t','e','s','t','1','s','.','m','p','3',0};
>  static inline WCHAR *load_resource(const WCHAR *name)
>  {
>      static WCHAR pathW[MAX_PATH];
> @@ -151,6 +153,8 @@ static HRESULT WINAPI WMPOCXEvents_Invoke(IDispatch *iface, DISPID dispIdMember,
>              CHECK_EXPECT(PLAYSTATE, V_UI4(pDispParams->rgvarg));
>              if (V_UI4(pDispParams->rgvarg) == wmppsPlaying) {
>                  SetEvent(playing_event);
> +            } else if (V_UI4(pDispParams->rgvarg) == wmppsMediaEnded) {
> +                SetEvent(completed_event);
>              }
>              if (winetest_debug > 1)
>                  trace("DISPID_WMPCOREEVENT_PLAYSTATECHANGE, %d\n", V_UI4(pDispParams->rgvarg));
> @@ -208,6 +212,88 @@ static HRESULT pump_messages(DWORD dwTimeout, DWORD nCount, const HANDLE *pHandl
>      return res;
>  }
>  
> +static void test_completion_event(void)
> +{
> +    DWORD res = 0;
> +    IWMPPlayer4 *player4;
> +    HRESULT hres;
> +    BSTR filename;
> +    IConnectionPointContainer *container;
> +    IConnectionPoint *point;
> +    IOleObject *oleobj;
> +    static DWORD dw = 100;
> +
> +    hres = CoCreateInstance(&CLSID_WindowsMediaPlayer, NULL, CLSCTX_INPROC_SERVER, &IID_IOleObject, (void**)&oleobj);
> +    if(hres == REGDB_E_CLASSNOTREG) {
> +        win_skip("CLSID_WindowsMediaPlayer not registered\n");
> +        return;
> +    }
> +    ok(hres == S_OK, "Could not create CLSID_WindowsMediaPlayer instance: %08x\n", hres);
> +
> +    hres = IOleObject_QueryInterface(oleobj, &IID_IConnectionPointContainer, (void**)&container);
> +    ok(hres == S_OK, "QueryInterface(IID_IConnectionPointContainer) failed: %08x\n", hres);
> +    if(FAILED(hres))
> +        return;
> +
> +    hres = IConnectionPointContainer_FindConnectionPoint(container, &IID__WMPOCXEvents, &point);
> +    IConnectionPointContainer_Release(container);
> +    ok(hres == S_OK, "FindConnectionPoint failed: %08x\n", hres);
> +
> +    hres = IConnectionPoint_Advise(point, (IUnknown*)&WMPOCXEvents, &dw);
> +    ok(hres == S_OK, "Advise failed: %08x\n", hres);
> +
> +    hres = IOleObject_QueryInterface(oleobj, &IID_IWMPPlayer4, (void**)&player4);
> +    ok(hres == S_OK, "Could not get IWMPPlayer4 iface: %08x\n", hres);
> +
> +    filename = SysAllocString(load_resource(mp3file1s));
> +
> +    SET_EXPECT(OPENSTATE, wmposPlaylistChanging);
> +    SET_EXPECT(OPENSTATE, wmposPlaylistOpenNoMedia);
> +    SET_EXPECT(OPENSTATE, wmposPlaylistChanged);
> +    SET_EXPECT(OPENSTATE, wmposOpeningUnknownURL);
> +    SET_EXPECT(OPENSTATE, wmposMediaOpen);
> +    SET_EXPECT(OPENSTATE, wmposMediaOpening);
> +    SET_EXPECT(PLAYSTATE, wmppsPlaying);
> +    SET_EXPECT(PLAYSTATE, wmppsMediaEnded);
> +    SET_EXPECT(PLAYSTATE, wmppsStopped);
> +    SET_EXPECT(PLAYSTATE, wmppsTransitioning);
> +    /* following two are sent on vistau64 vms only */
> +    SET_EXPECT(OPENSTATE, wmposMediaChanging);
> +    SET_EXPECT(PLAYSTATE, wmppsReady);

You don't have CHECK_EXPECT (or alike) after the code, so the test
leaves those in an invalid state. You may want to introduce something
like CLEAR_CALLED.

> +    hres = IWMPPlayer4_put_URL(player4, filename);
> +    ok(hres == S_OK, "IWMPPlayer4_put_URL failed: %08x\n", hres);
> +    res = pump_messages(3000, 1, &completed_event);
> +    ok(res == WAIT_OBJECT_0 || broken(res == WAIT_TIMEOUT), "Timed out while waiting for media to complete\n");
> +    
> +    if (res == WAIT_TIMEOUT) {
> +        /* This happens on Vista Ultimate 64 vms
> +         * I have been unable to find out source of this behaviour */
> +        win_skip("Failed to play media\n");
> +        goto playback_skip;

test_wmp() already has that issue, but note that you will leave expected
states in a wrong state here. You need to ensure they are right (by
adding CLEAR_CALLED) or ensure that we don't run more tests depending on
them. You could for example return FALSE from test_cmp() if you detect
and then not call test_completion_event(). This way
test_completion_event wouldn't even need the code to cope with broken setup.

> +    }
> +    CHECK_CALLED(OPENSTATE, wmposPlaylistChanging);
> +    CHECK_CALLED(OPENSTATE, wmposPlaylistChanged);
> +    CHECK_CALLED(OPENSTATE, wmposPlaylistOpenNoMedia);
> +    CHECK_CALLED(PLAYSTATE, wmppsTransitioning);
> +    CHECK_CALLED(OPENSTATE, wmposOpeningUnknownURL);
> +    CHECK_CALLED(OPENSTATE, wmposMediaOpen);
> +    /* MediaOpening happens only on xp, 2003 */
> +    todo_wine CHECK_CALLED_OR_BROKEN(OPENSTATE, wmposMediaOpening);

It's not clear to me that lack of this notification should be considered
as broken. This should probably use something like CLEAR_CALLED.

Thanks,
Jacek