[PATCH v7 2/6] wmp: Add OPEN/PLAY state change notifications
Anton Romanov
theli.ua at gmail.com
Thu Mar 29 10:38:04 CDT 2018
On Thu, Mar 29, 2018 at 7:11 AM, Jacek Caban <jacek at codeweavers.com> wrote:
> 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?
Sure.
>> +
>> 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?
Probably not.
>>
>> @@ -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?
Probably not.
>> 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.
Well, one call to something like put_url produces about 10 state
changes, some are playlist related that is functionality we are
missing.
Do we need / want to test for them all?
If we want to be as specific as possible - do we need to test for
order of those changes as well?
>> @@ -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.
this todo_wine is just TODO message, its not strictly required here. I
am not sure what would change with tracking each individual state.
I can just remove this todo.
>> + 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?
I made those up, for w/e reason I was blind and didn't see those
enums. I will change them to enums from msdn.
More information about the wine-devel
mailing list