[PATCH 0/1] MR8937: windows.media.playback.backgroundmediaplayer: Implement IBackgroundMediaPlayerStatics::get_Current().
From: Mohamad Al-Jaf <mohamadaljaf(a)gmail.com> --- .../main.c | 40 ++++++++++++++++++- .../private.h | 1 + .../tests/playback.c | 19 +++++++++ 3 files changed, 58 insertions(+), 2 deletions(-) diff --git a/dlls/windows.media.playback.backgroundmediaplayer/main.c b/dlls/windows.media.playback.backgroundmediaplayer/main.c index 66382520957..a22b6272d74 100644 --- a/dlls/windows.media.playback.backgroundmediaplayer/main.c +++ b/dlls/windows.media.playback.backgroundmediaplayer/main.c @@ -29,6 +29,8 @@ struct background_media_player_statics IActivationFactory IActivationFactory_iface; IBackgroundMediaPlayerStatics IBackgroundMediaPlayerStatics_iface; LONG ref; + + IMediaPlayer *media_player; }; static inline struct background_media_player_statics *impl_from_IActivationFactory( IActivationFactory *iface ) @@ -76,7 +78,11 @@ static ULONG WINAPI factory_Release( IActivationFactory *iface ) { struct background_media_player_statics *impl = impl_from_IActivationFactory( iface ); ULONG ref = InterlockedDecrement( &impl->ref ); + TRACE( "iface %p decreasing refcount to %lu.\n", iface, ref ); + + if (!ref) + if (impl->media_player) IMediaPlayer_Release( impl->media_player ); return ref; } @@ -120,10 +126,40 @@ static const struct IActivationFactoryVtbl factory_vtbl = DEFINE_IINSPECTABLE( background_media_player_statics, IBackgroundMediaPlayerStatics, struct background_media_player_statics, IActivationFactory_iface ) +static HRESULT get_media_player( IMediaPlayer **media_player ) +{ + static const WCHAR *media_player_name = L"Windows.Media.Playback.MediaPlayer"; + IInspectable *inspectable = NULL; + HSTRING str = NULL; + HRESULT hr = WindowsCreateString( media_player_name, wcslen( media_player_name ), &str ); + + if (SUCCEEDED(hr)) hr = RoActivateInstance( str, &inspectable ); + if (SUCCEEDED(hr)) + { + hr = IInspectable_QueryInterface( inspectable, &IID_IMediaPlayer, (void **)media_player ); + IInspectable_Release( inspectable ); + } + + WindowsDeleteString( str ); + return hr; +} + static HRESULT WINAPI background_media_player_statics_get_Current( IBackgroundMediaPlayerStatics *iface, IMediaPlayer **player ) { - FIXME( "iface %p, player %p stub!\n", iface, player ); - return E_NOTIMPL; + struct background_media_player_statics *impl = impl_from_IBackgroundMediaPlayerStatics( iface ); + HRESULT hr; + + TRACE( "iface %p, player %p\n", iface, player ); + + if (!impl->media_player && FAILED(hr = get_media_player( &impl->media_player ))) + { + *player = NULL; + return hr; + } + + *player = impl->media_player; + IMediaPlayer_AddRef( *player ); + return S_OK; } static HRESULT WINAPI background_media_player_statics_add_MessageReceivedFromBackground( IBackgroundMediaPlayerStatics *iface, diff --git a/dlls/windows.media.playback.backgroundmediaplayer/private.h b/dlls/windows.media.playback.backgroundmediaplayer/private.h index 63115ff2a6f..345937f80ee 100644 --- a/dlls/windows.media.playback.backgroundmediaplayer/private.h +++ b/dlls/windows.media.playback.backgroundmediaplayer/private.h @@ -28,6 +28,7 @@ #include "winstring.h" #include "activation.h" +#include "roapi.h" #define WIDL_using_Windows_Foundation #define WIDL_using_Windows_Foundation_Collections diff --git a/dlls/windows.media.playback.backgroundmediaplayer/tests/playback.c b/dlls/windows.media.playback.backgroundmediaplayer/tests/playback.c index a99c4080e1b..3f323238b08 100644 --- a/dlls/windows.media.playback.backgroundmediaplayer/tests/playback.c +++ b/dlls/windows.media.playback.backgroundmediaplayer/tests/playback.c @@ -50,6 +50,8 @@ static void test_BackgroundMediaPlayer_Statics(void) static const WCHAR *background_media_player_statics_name = L"Windows.Media.Playback.BackgroundMediaPlayer"; IBackgroundMediaPlayerStatics *background_media_player_statics = (void *)0xdeadbeef; IActivationFactory *factory = (void *)0xdeadbeef; + IMediaPlayer *media_player2 = (void *)0xdeadbeef; + IMediaPlayer *media_player = (void *)0xdeadbeef; HSTRING str; HRESULT hr; LONG ref; @@ -73,6 +75,23 @@ static void test_BackgroundMediaPlayer_Statics(void) hr = IActivationFactory_QueryInterface( factory, &IID_IBackgroundMediaPlayerStatics, (void **)&background_media_player_statics ); ok( hr == S_OK, "got hr %#lx.\n", hr ); + hr = IBackgroundMediaPlayerStatics_get_Current( background_media_player_statics, &media_player ); + ok( hr == S_OK, "got hr %#lx.\n", hr ); + hr = IBackgroundMediaPlayerStatics_get_Current( background_media_player_statics, &media_player2 ); + ok( hr == S_OK, "got hr %#lx.\n", hr ); + ok( media_player == media_player2, "got media_player %p, media_player2 %p.\n", media_player, media_player2 ); + ref = IBackgroundMediaPlayerStatics_Release( background_media_player_statics ); + ok( ref == 2, "got ref %ld.\n", ref ); + ref = IMediaPlayer_Release( media_player2 ); + ok( ref == 2 || broken(ref == 4) /* w1064v1507 */, "got ref %ld.\n", ref ); + ref = IMediaPlayer_Release( media_player ); + ok( ref == 1 || broken(ref == 3) /* w1064v1507 */, "got ref %ld.\n", ref ); + + hr = IActivationFactory_QueryInterface( factory, &IID_IBackgroundMediaPlayerStatics, (void **)&background_media_player_statics ); + ok( hr == S_OK, "got hr %#lx.\n", hr ); + hr = IBackgroundMediaPlayerStatics_get_Current( background_media_player_statics, &media_player ); + ok( hr == S_OK, "got hr %#lx.\n", hr ); + ref = IBackgroundMediaPlayerStatics_Release( background_media_player_statics ); ok( ref == 2, "got ref %ld.\n", ref ); ref = IActivationFactory_Release( factory ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8937
Rémi Bernon (@rbernon) commented about dlls/windows.media.playback.backgroundmediaplayer/tests/playback.c:
hr = IActivationFactory_QueryInterface( factory, &IID_IBackgroundMediaPlayerStatics, (void **)&background_media_player_statics ); ok( hr == S_OK, "got hr %#lx.\n", hr );
+ hr = IBackgroundMediaPlayerStatics_get_Current( background_media_player_statics, &media_player ); + ok( hr == S_OK, "got hr %#lx.\n", hr ); + hr = IBackgroundMediaPlayerStatics_get_Current( background_media_player_statics, &media_player2 ); + ok( hr == S_OK, "got hr %#lx.\n", hr ); + ok( media_player == media_player2, "got media_player %p, media_player2 %p.\n", media_player, media_player2 ); + ref = IBackgroundMediaPlayerStatics_Release( background_media_player_statics ); + ok( ref == 2, "got ref %ld.\n", ref ); + ref = IMediaPlayer_Release( media_player2 ); + ok( ref == 2 || broken(ref == 4) /* w1064v1507 */, "got ref %ld.\n", ref ); + ref = IMediaPlayer_Release( media_player ); + ok( ref == 1 || broken(ref == 3) /* w1064v1507 */, "got ref %ld.\n", ref );
Fwiw ref count checks can be useful in some cases, and they are useful at the end of a test when releasing the last reference to check for leaks, but in general we don't care about matching windows refcounting exactly, and if the test needs to include broken results because some windows versions don't match it's probably worth removing the reference count checks entirely. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8937#note_115420
Rémi Bernon (@rbernon) commented about dlls/windows.media.playback.backgroundmediaplayer/tests/playback.c:
+ hr = IBackgroundMediaPlayerStatics_get_Current( background_media_player_statics, &media_player ); + ok( hr == S_OK, "got hr %#lx.\n", hr ); + hr = IBackgroundMediaPlayerStatics_get_Current( background_media_player_statics, &media_player2 ); + ok( hr == S_OK, "got hr %#lx.\n", hr ); + ok( media_player == media_player2, "got media_player %p, media_player2 %p.\n", media_player, media_player2 ); + ref = IBackgroundMediaPlayerStatics_Release( background_media_player_statics ); + ok( ref == 2, "got ref %ld.\n", ref ); + ref = IMediaPlayer_Release( media_player2 ); + ok( ref == 2 || broken(ref == 4) /* w1064v1507 */, "got ref %ld.\n", ref ); + ref = IMediaPlayer_Release( media_player ); + ok( ref == 1 || broken(ref == 3) /* w1064v1507 */, "got ref %ld.\n", ref ); + + hr = IActivationFactory_QueryInterface( factory, &IID_IBackgroundMediaPlayerStatics, (void **)&background_media_player_statics ); + ok( hr == S_OK, "got hr %#lx.\n", hr ); + hr = IBackgroundMediaPlayerStatics_get_Current( background_media_player_statics, &media_player ); + ok( hr == S_OK, "got hr %#lx.\n", hr ); You're leaking media_player after this.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8937#note_115421
Rémi Bernon (@rbernon) commented about dlls/windows.media.playback.backgroundmediaplayer/main.c:
{ struct background_media_player_statics *impl = impl_from_IActivationFactory( iface ); ULONG ref = InterlockedDecrement( &impl->ref ); + TRACE( "iface %p decreasing refcount to %lu.\n", iface, ref ); + + if (!ref) + if (impl->media_player) IMediaPlayer_Release( impl->media_player );
if (!ref)
{
if (impl->media_player) IMediaPlayer_Release( impl->media_player );
impl->media_player = NULL;
}
We need to reset the factory to its initial state if we want to keep it statically allocated. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8937#note_115422
Rémi Bernon (@rbernon) commented about dlls/windows.media.playback.backgroundmediaplayer/main.c:
{ - FIXME( "iface %p, player %p stub!\n", iface, player ); - return E_NOTIMPL; + struct background_media_player_statics *impl = impl_from_IBackgroundMediaPlayerStatics( iface ); + HRESULT hr; + + TRACE( "iface %p, player %p\n", iface, player ); + + if (!impl->media_player && FAILED(hr = get_media_player( &impl->media_player ))) + { + *player = NULL; + return hr; + } + + *player = impl->media_player; + IMediaPlayer_AddRef( *player ); I suspect this will need to be guarded with a critical section as multiple threads could be calling get_Current concurrently. The same would then be needed for the release, in case some thread is releasing the last factory reference while another just acquired a new reference and called get_Current.
I would say that this is maybe a bit unnecessarily complicated for a stub. For simplicity you could allocate a new IMediaPlayer stub each time get_Current is called, and keep the media_player == media_player2 test as todo_wine. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8937#note_115423
participants (3)
-
Mohamad Al-Jaf -
Mohamad Al-Jaf (＠maljaf)
-
Rémi Bernon