Re: [PATCH v7 2/6] wmp: Add OPEN/PLAY state change notifications

[Jacek Caban]

Hi Anton,

On 03/28/2018 05:31 PM, Anton Romanov wrote:
> diff --git a/dlls/wmp/player.c b/dlls/wmp/player.c
> index 3d0df96bd8..b3da663637 100644
> --- a/dlls/wmp/player.c
> +++ b/dlls/wmp/player.c
> @@ -20,9 +20,12 @@
>
> #include "wine/debug.h"
>  #include <nserror.h>
> +#include "wmpids.h"
>
> WINE_DEFAULT_DEBUG_CHANNEL(wmp);
>
> +static void update_state(WindowsMediaPlayer *wmp, LONG type, LONG state);

How about moving implementation here to avoid forward declaration?
> -
>
> static inline WMPMedia *impl_from_IWMPMedia(IWMPMedia *iface)
>  {
>      return CONTAINING_RECORD(iface, WMPMedia, IWMPMedia_iface);
> @@ -125,14 +128,20 @@ static HRESULT WINAPI WMPPlayer4_put_URL(IWMPPlayer4 *iface, BSTR url)
>      if(url == NULL) {
>          return E_POINTER;
>      }
> - hres = create_media_from_url(url, &media);
> - if (SUCCEEDED(hres)) {
> -
>    update_state(This, DISPID_WMPCOREEVENT_PLAYSTATECHANGE, WMP_PLAY_STATE_TRANSITIONING);
>    hres = IWMPPlayer4_put_currentMedia(iface, media);
>    IWMPMedia_Release(media); /* put will addref */
> -
> -
>    update_state(This, DISPID_WMPCOREEVENT_PLAYSTATECHANGE, WMP_PLAY_STATE_READY);

Should we report ready state if put_currentMedia fails?
> @@ -191,9 +200,13 @@ static HRESULT WINAPI WMPPlayer4_put_currentMedia(IWMPPlayer4 *iface, IWMPMedia
>      if(pMedia == NULL) {
>          return E_POINTER;
>      }
> - update_state(This, DISPID_WMPCOREEVENT_OPENSTATECHANGE, WMP_OPEN_STATE_PLAYLIST_CHANGING);
> if(This->wmpmedia != NULL) {
>     IWMPMedia_Release(This->wmpmedia);
> }
> - update_state(This, DISPID_WMPCOREEVENT_OPENSTATECHANGE, WMP_OPEN_STATE_PLAYLIST_CHANGED);
> - update_state(This, DISPID_WMPCOREEVENT_OPENSTATECHANGE, WMP_OPEN_STATE_PLAYLIST_OPEN_NO_MEDIA);
> - This->wmpmedia = pMedia;
>    IWMPMedia_AddRef(This->wmpmedia);
>    return S_OK;
>
> @@ -1388,19 +1401,31 @@ static HRESULT WINAPI WMPControls_play(IWMPControls *iface)
>                  CLSCTX_INPROC_SERVER,
>                  &IID_IGraphBuilder,
>                  (void **)&This->filter_graph);
> -
>    update_state(This, DISPID_WMPCOREEVENT_OPENSTATECHANGE, WMP_OPEN_STATE_OPENING_UNKNOWN_URL);
> -
>    if (SUCCEEDED(hres))
>        hres = IGraphBuilder_RenderFile(This->filter_graph, media->url, NULL);
>    if (SUCCEEDED(hres))
>        hres = IGraphBuilder_QueryInterface(This->filter_graph, &IID_IMediaControl,
>                (void**)&This->media_control);
> -
>    update_state(This, DISPID_WMPCOREEVENT_OPENSTATECHANGE, WMP_OPEN_STATE_MEDIA_OPEN);

It's similar, is it right in case of loading error?
> diff --git a/dlls/wmp/tests/media.c b/dlls/wmp/tests/media.c
> index 6e4a0c170f..f98a60fbfd 100644
> --- a/dlls/wmp/tests/media.c
> +++ b/dlls/wmp/tests/media.c
> @@ -19,9 +19,42 @@
>  #include <wmp.h>
>  #include <olectl.h>
>  #include <nserror.h>
> +#include <wmpids.h>
> +#include <math.h>
>
> #include "wine/test.h"
>
> +#define DEFINE_EXPECT(func) \
> - static BOOL expect_ ## func = FALSE, called_ ## func = FALSE
> -
>
> +#define SET_EXPECT(func) \
> - expect_ ## func = TRUE
> -
>
> +#define CHECK_EXPECT(func) \
> - do { \
> -
>    ok(expect_ ##func, "unexpected call " #func "\n"); \
> -
>    called_ ## func = TRUE; \
> - }while(0)
> -
>
> +#define CHECK_CALLED(func) \
> - do { \
> -
>    ok(called_ ## func, "expected " #func "\n"); \
> -
>    expect_ ## func = called_ ## func = FALSE; \
> - }while(0)
> -
>
> +#define CHECK_CALLED_OR_BROKEN(func) \
> - do { \
> -
>    ok(called_ ## func || broken(TRUE), "expected " #func "\n"); \
> -
>    expect_ ## func = called_ ## func = FALSE; \
> - }while(0)
> -
>
> +DEFINE_EXPECT(PLAYSTATE_CHANGE);
> +DEFINE_EXPECT(OPENSTATE_CHANGE);
> -
>
> +static BOOL open_state;
> +static BOOL play_state;
> +static HANDLE playing_event;
> -
>
> static const WCHAR mp3file[] = {'t','e','s','t','.','m','p','3',0};
>  static inline WCHAR *load_resource(const WCHAR *name)
>  {
> @@ -48,14 +81,114 @@ static inline WCHAR *load_resource(const WCHAR *name)
>      return pathW;
>  }
>
> +static ULONG WINAPI Dispatch_AddRef(IDispatch *iface)
> +{
> - return 2;
>
> +}
> -
>
> +static ULONG WINAPI Dispatch_Release(IDispatch *iface)
> +{
> - return 1;
>
> +}
> -
>
> +static HRESULT WINAPI Dispatch_GetTypeInfoCount(IDispatch *iface, UINT *pctinfo)
> +{
> - ok(0, "unexpected call\n");
> - return E_NOTIMPL;
>
> +}
> -
>
> +static HRESULT WINAPI Dispatch_GetTypeInfo(IDispatch *iface, UINT iTInfo, LCID lcid,
> -
>    ITypeInfo **ppTInfo)
>
> +{
> - ok(0, "unexpected call\n");
> - return E_NOTIMPL;
>
> +}
> -
>
> +static HRESULT WINAPI Dispatch_GetIDsOfNames(IDispatch *iface, REFIID riid, LPOLESTR *rgszNames,
> -
>    UINT cNames, LCID lcid, DISPID *rgDispId)
>
> +{
> - ok(0, "unexpected call\n");
> - return E_NOTIMPL;
>
> +}
> -
>
> +static HRESULT WINAPI WMPOCXEvents_QueryInterface(IDispatch *iface, REFIID riid, void **ppv)
> +{
> - *ppv = NULL;
> -
> - if(IsEqualGUID(&IID__WMPOCXEvents, riid) || IsEqualGUID(&IID_IDispatch, riid)) {
> -
>    *ppv = iface;
> -
>    return S_OK;
> - }
> -
> - ok(0, "unexpected riid %s\n", wine_dbgstr_guid(riid));
> - return E_NOINTERFACE;
>
> +}
> -
>
> +static HRESULT WINAPI WMPOCXEvents_Invoke(IDispatch *iface, DISPID dispIdMember, REFIID riid,
> -
>    LCID lcid, WORD wFlags, DISPPARAMS *pDispParams, VARIANT *pVarResult,
> -
>    EXCEPINFO *pExcepInfo, UINT *puArgErr)
>
> +{
> - switch(dispIdMember) {
> -
>    /* Uncomment below traces to debug wmp events */
> -
>    case DISPID_WMPCOREEVENT_OPENSTATECHANGE:
> -
>        CHECK_EXPECT(OPENSTATE_CHANGE);
> -
>        open_state = V_UI4(pDispParams->rgvarg);
> -
>        if (winetest_debug > 1)
> -
>            trace("DISPID_WMPCOREEVENT_OPENSTATECHANGE, %d\n", V_UI4(pDispParams->rgvarg));
> -
>        break;
> -
>    case DISPID_WMPCOREEVENT_PLAYSTATECHANGE:
> -
>        CHECK_EXPECT(PLAYSTATE_CHANGE);
> -
>        play_state = V_UI4(pDispParams->rgvarg);
> -
>        if (play_state == WMP_PLAY_STATE_PLAYING) {
> -
>            SetEvent(playing_event);
> -
>        }
> -
>        if (winetest_debug > 1)
> -
>            trace("DISPID_WMPCOREEVENT_PLAYSTATECHANGE, %d\n", V_UI4(pDispParams->rgvarg));
> -
>        break;

How about adding more specific tests? You could split CHECK_EXPECT into
actual states, so it would be like
CHECK_EXPECT(OpenStateChange_MediaChanging),
CHECK_EXPECT(PlatStateChange_Waiting) and alike.
> @@ -87,22 +232,70 @@ static void test_wmp(void)
>  filename = SysAllocString(load_resource(mp3file));
>
> -
>
> SET_EXPECT(OPENSTATE_CHANGE);
> -
>
> SET_EXPECT(PLAYSTATE_CHANGE);
> hres = IWMPPlayer4_put_URL(player4, filename);
> ok(hres == S_OK, "IWMPPlayer4_put_URL failed: %08x\n", hres);
> -
>
> CHECK_CALLED(OPENSTATE_CHANGE);
> -
>
> CHECK_CALLED(PLAYSTATE_CHANGE);
> -
>
> SET_EXPECT(PLAYSTATE_CHANGE);
> -
>
> SET_EXPECT(OPENSTATE_CHANGE);
> hres = IWMPControls_play(controls);
> -
>
> ok(hres == S_OK, "IWMPControls_play failed: %08x\n", hres);
> -
>
> {
> -
>    MSG msg;
> -
>    DWORD start_time = GetTickCount();
> -
>    DWORD dwTimeout = 5000;
> -
>    HANDLE handles[1];
> -
>    handles[0] = playing_event;
> -
>    do {
> -
>        DWORD now = GetTickCount();
> -
>        res = MsgWaitForMultipleObjectsEx(1, handles, start_time + dwTimeout - now,
> -
>                QS_ALLINPUT ,MWMO_ALERTABLE | MWMO_INPUTAVAILABLE);
> -
>        if (res == WAIT_OBJECT_0 + 1) {
> -
>            GetMessageW(&msg, 0, 0, 0);
> -
>            if (winetest_debug > 1)
> -
>                trace("Dispatching %d\n", msg.message);
> -
>            TranslateMessage(&msg);
> -
>            DispatchMessageW(&msg);
> -
>        }
> -
>    }
> -
>    while (res == WAIT_OBJECT_0 + 1);
> -
>    ok(res == WAIT_OBJECT_0 || broken(res == WAIT_TIMEOUT), "Timed out while waiting for media to become ready\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 transition media to playing state.\n");
> -
>    goto playback_skip;
> -
>
> }
> -
>
> CHECK_CALLED(OPENSTATE_CHANGE);
> -
>
> CHECK_CALLED(PLAYSTATE_CHANGE);
> -
>
> SET_EXPECT(OPENSTATE_CHANGE); todo_wine ok(FALSE || broken(TRUE), "WMP in wine changes open state when stopping\n");

This looks bad. If you change tests as I mentioned above, you can just
add todo_wine where appropriate.
> -
>
> SET_EXPECT(PLAYSTATE_CHANGE);
> hres = IWMPControls_stop(controls);
> ok(hres == S_OK, "IWMPControls_stop failed: %08x\n", hres);
> -
>
> CHECK_CALLED(PLAYSTATE_CHANGE);
>
> /* Already Stopped */
> hres = IWMPControls_stop(controls);
> ok(hres == NS_S_WMPCORE_COMMAND_NOT_AVAILABLE, "IWMPControls_stop is available: %08x\n", hres);
> -
>
> SET_EXPECT(OPENSTATE_CHANGE);
> -
>
> SET_EXPECT(PLAYSTATE_CHANGE);
> hres = IWMPControls_play(controls);
> ok(hres == S_OK, "IWMPControls_play failed: %08x\n", hres);
> -
>
> CHECK_CALLED_OR_BROKEN(OPENSTATE_CHANGE);
> -
>
> CHECK_CALLED_OR_BROKEN(PLAYSTATE_CHANGE);
>
> +playback_skip:
> - hres = IConnectionPoint_Unadvise(point, dw);
> - ok(hres == S_OK, "Unadvise failed: %08x\n", hres);
> -
> - IConnectionPoint_Release(point);
> IWMPControls_Release(controls);
> IWMPPlayer4_Release(player4);
> IOleObject_Release(oleobj);
>
> @@ -114,7 +307,10 @@ START_TEST(media)
>  {
>      CoInitialize(NULL);
> -
>
> playing_event = CreateEventW(NULL, FALSE, FALSE, NULL);
> test_wmp();
> -
>
> CloseHandle(playing_event);
> -
>
> CoUninitialize();
>
> }
> diff --git a/dlls/wmp/wmp_main.c b/dlls/wmp/wmp_main.c
> index 29b096f7fd..9a33b2762b 100644
> --- a/dlls/wmp/wmp_main.c
> +++ b/dlls/wmp/wmp_main.c
> @@ -25,6 +25,7 @@
>  WINE_DEFAULT_DEBUG_CHANNEL(wmp);
>
> HINSTANCE wmp_instance;
> +DEFINE_GUID(GUID_NULL,0,0,0,0,0,0,0,0,0,0,0);
>
> static HRESULT WINAPI ClassFactory_QueryInterface(IClassFactory *iface, REFIID riid, void **ppv)
>  {
> diff --git a/dlls/wmp/wmp_private.h b/dlls/wmp/wmp_private.h
> index 05ad5889e2..9e84d56ea8 100644
> --- a/dlls/wmp/wmp_private.h
> +++ b/dlls/wmp/wmp_private.h
> @@ -83,6 +83,7 @@ WMPMedia *unsafe_impl_from_IWMPMedia(IWMPMedia *iface) DECLSPEC_HIDDEN;
>  HRESULT create_media_from_url(BSTR url, IWMPMedia **ppMedia) DECLSPEC_HIDDEN;
>  void ConnectionPointContainer_Init(WindowsMediaPlayer *wmp) DECLSPEC_HIDDEN;
>  void ConnectionPointContainer_Destroy(WindowsMediaPlayer *wmp) DECLSPEC_HIDDEN;
> +void call_sink(ConnectionPoint *This, DISPID dispid, DISPPARAMS *dispparams) DECLSPEC_HIDDEN;
>
> HRESULT WINAPI WMPFactory_CreateInstance(IClassFactory*,IUnknown*,REFIID,void**) DECLSPEC_HIDDEN;
>
> diff --git a/include/wmpids.h b/include/wmpids.h
> new file mode 100644
> index 0000000000..2792f850fa
> --- /dev/null
> +++ b/include/wmpids.h
> @@ -0,0 +1,63 @@
> +/*
> -
> - This library is free software; you can redistribute it and/or
> -
> - modify it under the terms of the GNU Lesser General Public
> -
> - License as published by the Free Software Foundation; either
> -
> - version 2.1 of the License, or (at your option) any later version.
> -
> -
> -
> - This library is distributed in the hope that it will be useful,
> -
> - but WITHOUT ANY WARRANTY; without even the implied warranty of
> -
> - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> -
> - Lesser General Public License for more details.
> -
> -
> -
> - You should have received a copy of the GNU Lesser General Public
> -
> - License along with this library; if not, write to the Free Software
> -
> - Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
> - */
> -
> -
>
> +/* play  state */
> +#define WMP_PLAY_STATE_UNDEFINED     0
> +#define WMP_PLAY_STATE_STOPPED       1
> +#define WMP_PLAY_STATE_PAUSED        2
> +#define WMP_PLAY_STATE_PLAYING       3
> +#define WMP_PLAY_STATE_SCAN_FORWARD  4
> +#define WMP_PLAY_STATE_SCAN_REVERSE  5
> +#define WMP_PLAY_STATE_BUFFERING     6
> +#define WMP_PLAY_STATE_WAITING       7
> +#define WMP_PLAY_STATE_MEDIA_ENDED   8
> +#define WMP_PLAY_STATE_TRANSITIONING 9
> +#define WMP_PLAY_STATE_READY         10
> +#define WMP_PLAY_STATE_RECONNECTING  11
> -
>
> +/* open state */
> +#define WMP_OPEN_STATE_UNDEFINED                 0
> +#define WMP_OPEN_STATE_PLAYLIST_CHANGING         1
> +#define WMP_OPEN_STATE_PLAYLIST_LOCATING         2
> +#define WMP_OPEN_STATE_PLAYLIST_CONNECTING       3
> +#define WMP_OPEN_STATE_PLAYLIST_LOADING          4
> +#define WMP_OPEN_STATE_PLAYLIST_OPENING          5
> +#define WMP_OPEN_STATE_PLAYLIST_OPEN_NO_MEDIA    6
> +#define WMP_OPEN_STATE_PLAYLIST_CHANGED          7
> +#define WMP_OPEN_STATE_MEDIA_CHANGING            8
> +#define WMP_OPEN_STATE_MEDIA_LOCATING            9
> +#define WMP_OPEN_STATE_MEDIA_CONNECTING          10
> +#define WMP_OPEN_STATE_MEDIA_LOADING             11
> +#define WMP_OPEN_STATE_MEDIA_OPENING             12
> +#define WMP_OPEN_STATE_MEDIA_OPEN                13
> +#define WMP_OPEN_STATE_BEGIN_CODEC_ACQUISITION   14
> +#define WMP_OPEN_STATE_END_CODEC_ACQUISITION     15
> +#define WMP_OPEN_STATE_BEGIN_LICENSE_ACQUISITION 16
> +#define WMP_OPEN_STATE_END_LICENSE_ACQUISITION   17
> +#define WMP_OPEN_STATE_BEGIN_INDIVIDUALIZATION   18
> +#define WMP_OPEN_STATE_END_INDIVIDUALIZATION     19
> +#define WMP_OPEN_STATE_MEDIA_WAITING             20
> +#define WMP_OPEN_STATE_OPENING_UNKNOWN_URL       21

Where do those come from? Shouldn't you use WMPOpenState and
WMPPlayState instead?

Thanks,
Jacek