Re: [PATCH] dmime: Implemnet IDirectMusicPerformance8 Add/Remove NotificationType

[Michael Stefaniuc]

Hello Alistair,

is there an application that this patch fixes? Well more like stopping
from crashing...

According to the documentation the Add/Remove NotificationType should be
passed down to the segment and tracks:
http://doc.51windows.net/directx9_sdk/htm/idirectmusicperformance8addnotific...
http://doc.51windows.net/directx9_sdk/htm/idirectmusicperformance8removenoti...
And of course native returns E_NOTIMPL for those methods in the
implementation of some tracks...

So I don't mind adding a band aid in performance, but that should be
still a FIXME with semi-stub or so.
There is also quite some code duplication between the two functions, it
might make sense to factor out the guid to type code.

Please see some other comments inline.

On 10/4/19 3:07 AM, Alistair Leslie-Hughes wrote:
> Signed-off-by: Alistair Leslie-Hughes
>
> dlls/dmime/performance.c       | 79 +++++++++++++++++++++++++++++++---
>  dlls/dmime/tests/performance.c | 50 +++++++++++++++++++++
>  2 files changed, 123 insertions(+), 6 deletions(-)
>
> diff --git a/dlls/dmime/performance.c b/dlls/dmime/performance.c
> index 399b9b9919..bffc8ff872 100644
> --- a/dlls/dmime/performance.c
> +++ b/dlls/dmime/performance.c
> @@ -22,6 +22,16 @@
>
> WINE_DEFAULT_DEBUG_CHANNEL(dmime);
>
> +enum {

If you use an enum please name it and used it as a type for "type".
> - notify_chord,
> - notify_command,
> - notify_measurement,
> - notify_perforance,

^^ typo
> - notify_recompose,
> - notify_segment,
> - notify_custom
>
> +};
> -
>
> typedef struct IDirectMusicPerformance8Impl {
>      IDirectMusicPerformance8 IDirectMusicPerformance8_iface;
>      LONG ref;
> @@ -50,6 +60,9 @@ typedef struct IDirectMusicPerformance8Impl {
>      CRITICAL_SECTION safe;
>      struct DMUS_PMSGItem *head;
>      struct DMUS_PMSGItem *imm_head;
> -
> - BOOL notifications[7];

It seems excessive to use an array of BOOLs. I'd rather prefer a bitfield.
> - GUID custom_guid;
>
> } IDirectMusicPerformance8Impl;
>
> typedef struct DMUS_PMSGItem DMUS_PMSGItem;
> @@ -550,19 +563,72 @@ static HRESULT WINAPI IDirectMusicPerformance8Impl_GetNotificationPMsg(IDirectMu
>  static HRESULT WINAPI IDirectMusicPerformance8Impl_AddNotificationType(IDirectMusicPerformance8 *iface,
>          REFGUID rguidNotificationType)
>  {
> -
>    IDirectMusicPerformance8Impl *This = impl_from_IDirectMusicPerformance8(iface);
> - IDirectMusicPerformance8Impl *This = impl_from_IDirectMusicPerformance8(iface);
> - int type;

Please use the enum as type.
> -
> - TRACE("(%p, %s)\n", This, debugstr_dmguid(rguidNotificationType));
> -
> - if (IsEqualGUID (rguidNotificationType, &GUID_NOTIFICATION_CHORD))
> -
>    type = notify_chord;
> - else if (IsEqualGUID (rguidNotificationType, &GUID_NOTIFICATION_COMMAND))
> -
>    type = notify_command;
> - else if (IsEqualGUID (rguidNotificationType, &GUID_NOTIFICATION_MEASUREANDBEAT))
> -
>    type = notify_measurement;
> - else if (IsEqualGUID (rguidNotificationType, &GUID_NOTIFICATION_PERFORMANCE))
> -
>    type = notify_perforance;
> - else if (IsEqualGUID (rguidNotificationType, &GUID_NOTIFICATION_RECOMPOSE))
> -
>    type = notify_recompose;
> - else if (IsEqualGUID (rguidNotificationType, &GUID_NOTIFICATION_SEGMENT))
> -
>    type = notify_segment;
> - else {
> -
>    type = notify_custom;
> -
>    if (This->notifications[notify_custom])
> -
>        WARN("Only one custom notification currently supported\n");
> -
>    This->custom_guid = *rguidNotificationType;
> - }
> - FIXME("(%p, %s): stub\n", This, debugstr_dmguid(rguidNotificationType));
> - return S_OK;
> -
> if (This->notifications[type])
> -
>    return S_FALSE;
> -
> - This->notifications[type] = TRUE;
> -
> - return S_OK;
>
> }
>
> static HRESULT WINAPI IDirectMusicPerformance8Impl_RemoveNotificationType(IDirectMusicPerformance8 *iface,
>          REFGUID rguidNotificationType)
>  {
> -
>    IDirectMusicPerformance8Impl *This = impl_from_IDirectMusicPerformance8(iface);
> - IDirectMusicPerformance8Impl *This = impl_from_IDirectMusicPerformance8(iface);
> - int type;
> -
> - TRACE("(%p, %s)\n", This, debugstr_dmguid(rguidNotificationType));
> -
> - if (IsEqualGUID (rguidNotificationType, &GUID_NOTIFICATION_CHORD))
> -
>    type = notify_chord;
> - else if (IsEqualGUID (rguidNotificationType, &GUID_NOTIFICATION_COMMAND))
> -
>    type = notify_command;
> - else if (IsEqualGUID (rguidNotificationType, &GUID_NOTIFICATION_MEASUREANDBEAT))
> -
>    type = notify_measurement;
> - else if (IsEqualGUID (rguidNotificationType, &GUID_NOTIFICATION_PERFORMANCE))
> -
>    type = notify_perforance;
> - else if (IsEqualGUID (rguidNotificationType, &GUID_NOTIFICATION_RECOMPOSE))
> -
>    type = notify_recompose;
> - else if (IsEqualGUID (rguidNotificationType, &GUID_NOTIFICATION_SEGMENT))
> -
>    type = notify_segment;
> - else {
> -
>    type = notify_custom;
> -
>    if (IsEqualGUID (&This->custom_guid, rguidNotificationType))
> -
>        This->custom_guid = GUID_NULL;
> -
>    else
> -
>        WARN("Trying to remove an unknown custom notification\n");
> - }
> - FIXME("(%p, %s): stub\n", This, debugstr_dmguid(rguidNotificationType));
> - return S_OK;
> - if (!This->notifications[type])
> -
>    return S_FALSE;
> -
> - This->notifications[type] = FALSE;
> -
> - return S_OK;
>
> }
>
> static HRESULT WINAPI IDirectMusicPerformance8Impl_AddPort(IDirectMusicPerformance8 *iface,
> @@ -1214,6 +1280,7 @@ HRESULT WINAPI create_dmperformance(REFIID lpcGUID, void **ppobj)
>          obj->rtLatencyTime  = 100;  /* 100 ms TO FIX */
>          obj->dwBumperLength =   50; /* 50 ms default */
>          obj->dwPrepareTime  = 1000; /* 1000 ms default */
> -
>    obj->custom_guid = GUID_NULL;
>    return IDirectMusicPerformance8Impl_QueryInterface(&obj->IDirectMusicPerformance8_iface,
>                                                       lpcGUID, ppobj);
>
> }
> diff --git a/dlls/dmime/tests/performance.c b/dlls/dmime/tests/performance.c
> index 4433846f9b..e4e5dc1b58 100644
> --- a/dlls/dmime/tests/performance.c
> +++ b/dlls/dmime/tests/performance.c
> @@ -482,6 +482,55 @@ static void test_notification_type(void)
>      IDirectMusicPerformance8_Release(perf);
>  }
>
> +static void test_add_remove_notification_types(void)
> +{
> - IDirectMusicPerformance8 *perf;
> - HRESULT hr;
> -
> - hr = CoCreateInstance(&CLSID_DirectMusicPerformance, NULL,
> -
>        CLSCTX_INPROC_SERVER, &IID_IDirectMusicPerformance8, (void**)&perf);
> - ok(hr == S_OK, "CoCreateInstance failed: %08x\n", hr);
> -

Please use an array and loop for all the guid pointers.
That way you can check the double add / double remove for all guids.
Btw. any reason you don't check the chord change notification?
Also what happens if you pass in GUID_NULL as "custom" notification?
> - hr = IDirectMusicPerformance8_AddNotificationType(perf, &GUID_NOTIFICATION_SEGMENT);
> - ok(hr == S_OK, "Failed: %08x\n", hr);
> -
> - hr = IDirectMusicPerformance8_AddNotificationType(perf, &GUID_NOTIFICATION_SEGMENT);
> - ok(hr == S_FALSE, "Failed: %08x\n", hr);
> -
> - hr = IDirectMusicPerformance8_AddNotificationType(perf, &GUID_NOTIFICATION_COMMAND);
> - ok(hr == S_OK, "Failed: %08x\n", hr);
> -
> - hr = IDirectMusicPerformance8_AddNotificationType(perf, &GUID_NOTIFICATION_MEASUREANDBEAT);
> - ok(hr == S_OK, "Failed: %08x\n", hr);
> -
> - hr = IDirectMusicPerformance8_AddNotificationType(perf, &GUID_NOTIFICATION_PERFORMANCE);
> - ok(hr == S_OK, "Failed: %08x\n", hr);
> -
> - hr = IDirectMusicPerformance8_AddNotificationType(perf, &GUID_NOTIFICATION_RECOMPOSE);
> - ok(hr == S_OK, "Failed: %08x\n", hr);
> -
> - /* RemoveNotificationType */
> - hr = IDirectMusicPerformance8_RemoveNotificationType(perf, &GUID_NOTIFICATION_SEGMENT);
> - ok(hr == S_OK, "Failed: %08x\n", hr);
> -
> - hr = IDirectMusicPerformance8_RemoveNotificationType(perf, &GUID_NOTIFICATION_SEGMENT);
> - ok(hr == S_FALSE, "Failed: %08x\n", hr);
> -
> - hr = IDirectMusicPerformance8_RemoveNotificationType(perf, &GUID_NOTIFICATION_COMMAND);
> - ok(hr == S_OK, "Failed: %08x\n", hr);
> -
> - hr = IDirectMusicPerformance8_RemoveNotificationType(perf, &GUID_NOTIFICATION_MEASUREANDBEAT);
> - ok(hr == S_OK, "Failed: %08x\n", hr);
> -
> - hr = IDirectMusicPerformance8_RemoveNotificationType(perf, &GUID_NOTIFICATION_PERFORMANCE);
> - ok(hr == S_OK, "Failed: %08x\n", hr);
> -
> - hr = IDirectMusicPerformance8_RemoveNotificationType(perf, &GUID_NOTIFICATION_RECOMPOSE);
> - ok(hr == S_OK, "Failed: %08x\n", hr);
> -
> - IDirectMusicPerformance8_Release(perf);
>
> +}
> -
>
> START_TEST( performance )
>  {
>      HRESULT hr;
> @@ -501,6 +550,7 @@ START_TEST( performance )
>      test_COM();
>      test_createport();
>      test_notification_type();
> -
>
> test_add_remove_notification_types();
>
> CoUninitialize();
>
> }

thanks
bye
    michael