[PATCH v2 0/1] MR8937: windows.media.playback.backgroundmediaplayer: Implement IBackgroundMediaPlayerStatics::get_Current().
-- v2: windows.media.playback.backgroundmediaplayer: Implement IBackgroundMediaPlayerStatics::get_Current().
From: Mohamad Al-Jaf mohamadaljaf@gmail.com
--- .../main.c | 66 ++++++++++++++++++- .../private.h | 1 + .../tests/playback.c | 10 +++ 3 files changed, 75 insertions(+), 2 deletions(-)
diff --git a/dlls/windows.media.playback.backgroundmediaplayer/main.c b/dlls/windows.media.playback.backgroundmediaplayer/main.c index 66382520957..e6ef2ca4f82 100644 --- a/dlls/windows.media.playback.backgroundmediaplayer/main.c +++ b/dlls/windows.media.playback.backgroundmediaplayer/main.c @@ -24,11 +24,22 @@
WINE_DEFAULT_DEBUG_CHANNEL(playback);
+static CRITICAL_SECTION media_player_cs; +static CRITICAL_SECTION_DEBUG media_player_cs_debug = +{ + 0, 0, &media_player_cs, + { &media_player_cs_debug.ProcessLocksList, &media_player_cs_debug.ProcessLocksList }, + 0, 0, { (DWORD_PTR)(__FILE__ ": media_player_cs") } +}; +static CRITICAL_SECTION media_player_cs = { &media_player_cs_debug, -1, 0, 0, 0, 0 }; + 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 +87,20 @@ 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) + { + EnterCriticalSection( &media_player_cs ); + if (impl->media_player) + { + IMediaPlayer_Release( impl->media_player ); + impl->media_player = NULL; + } + LeaveCriticalSection( &media_player_cs ); + } + return ref; }
@@ -120,10 +144,44 @@ 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 ); + + EnterCriticalSection( &media_player_cs ); + + if (!impl->media_player && FAILED(hr = get_media_player( &impl->media_player ))) + { + *player = NULL; + LeaveCriticalSection( &media_player_cs ); + return hr; + } + + *player = impl->media_player; + IMediaPlayer_AddRef( *player ); + LeaveCriticalSection( &media_player_cs ); + return S_OK; }
static HRESULT WINAPI background_media_player_statics_add_MessageReceivedFromBackground( IBackgroundMediaPlayerStatics *iface, @@ -221,11 +279,15 @@ HRESULT WINAPI DllGetActivationFactory( HSTRING classid, IActivationFactory **fa
TRACE( "class %s, factory %p.\n", debugstr_hstring( classid ), factory );
+ EnterCriticalSection( &media_player_cs ); + *factory = NULL;
if (!wcscmp( buffer, RuntimeClass_Windows_Media_Playback_BackgroundMediaPlayer )) IActivationFactory_QueryInterface( background_media_player_factory, &IID_IActivationFactory, (void **)factory );
+ LeaveCriticalSection( &media_player_cs ); + if (*factory) return S_OK; return CLASS_E_CLASSNOTAVAILABLE; } 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..593355d67f3 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,14 @@ 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 ); + IMediaPlayer_Release( media_player2 ); + IMediaPlayer_Release( media_player ); + ref = IBackgroundMediaPlayerStatics_Release( background_media_player_statics ); ok( ref == 2, "got ref %ld.\n", ref ); ref = IActivationFactory_Release( factory );
On Wed Sep 10 18:21:23 2025 +0000, Mohamad Al-Jaf wrote:
changed this line in [version 2 of the diff](/wine/wine/-/merge_requests/8937/diffs?diff_id=207946&start_sha=497c94a37d22bf454583a86b9698dc49d2ce7f0d#394fa049ad0a1d80896480e1d02f6a5662bd4c15_88_83)
Yeah, I didn't think it was worth adding originally.
On Tue Sep 9 07:42:22 2025 +0000, Rémi Bernon wrote:
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.
How's this?
Rémi Bernon (@rbernon) commented about dlls/windows.media.playback.backgroundmediaplayer/main.c:
+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;
```suggestion:-10+0 HRESULT hr;
if (FAILED(hr = WindowsCreateString( media_player_name, wcslen( media_player_name ), &str ))) return hr; if (SUCCEEDED(hr = RoActivateInstance( str, &inspectable ))) { hr = IInspectable_QueryInterface( inspectable, &IID_IMediaPlayer, (void **)media_player ); IInspectable_Release( inspectable ); }
WindowsDeleteString( str ); return hr; ```
Otherwise you'd call WindowsDeleteString even if WindowsCreateString failed (no big deal but still).
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)
- {
EnterCriticalSection( &media_player_cs );if (impl->media_player){IMediaPlayer_Release( impl->media_player );impl->media_player = NULL;}LeaveCriticalSection( &media_player_cs );
This works because you also guard the factory QueryInterface, but I would say it's not the canonical way to do it. Guarding the factory AddRef shouldn't be necessary, and instead you should probably check the ref count again within the critical section:
```suggestion:-6+0 EnterCriticalSection( &media_player_cs ); if (!impl->ref && impl->media_player) { IMediaPlayer_Release( impl->media_player ); impl->media_player = NULL; } LeaveCriticalSection( &media_player_cs ); ```
Rémi Bernon (@rbernon) commented about dlls/windows.media.playback.backgroundmediaplayer/main.c:
TRACE( "class %s, factory %p.\n", debugstr_hstring( classid ), factory );
EnterCriticalSection( &media_player_cs );
*factory = NULL;
if (!wcscmp( buffer, RuntimeClass_Windows_Media_Playback_BackgroundMediaPlayer )) IActivationFactory_QueryInterface( background_media_player_factory, &IID_IActivationFactory, (void **)factory );
LeaveCriticalSection( &media_player_cs );
And remove these.
```suggestion:-7+0 *factory = NULL;
if (!wcscmp( buffer, RuntimeClass_Windows_Media_Playback_BackgroundMediaPlayer )) IActivationFactory_QueryInterface( background_media_player_factory, &IID_IActivationFactory, (void **)factory ); ```
-
Mohamad Al-Jaf -
Mohamad Al-Jaf (＠maljaf)
-
Rémi Bernon (＠rbernon)