Re: [PATCH v2 0/1] MR8937: windows.media.playback.backgroundmediaplayer: Implement IBackgroundMediaPlayerStatics::get_Current().

[Rémi Bernon (＠rbernon)]

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 );
```