[PATCH 0/1] MR5040: dmband: Implement getting/setting GUID_BandParam on band tracks.

[Yuxuan Shui (＠yshui)]

split from !4982

that MR doesn't use this anymore but might still be good to have.

[PATCH 1/1] dmband: Implement getting/setting GUID_BandParam on band tracks.

[Yuxuan Shui]

From: Yuxuan Shui

---
 dlls/dmband/bandtrack.c    | 40 ++++++++++++++++++++++++++++++++++----
 dlls/dmband/tests/dmband.c | 27 +++++++++++++++++++++++++
 2 files changed, 63 insertions(+), 4 deletions(-)

diff --git a/dlls/dmband/bandtrack.c b/dlls/dmband/bandtrack.c
index 4756ed194ef..dbd75a4a299 100644
--- a/dlls/dmband/bandtrack.c
+++ b/dlls/dmband/bandtrack.c
@@ -198,19 +198,39 @@ static HRESULT WINAPI band_track_Play(IDirectMusicTrack8 *iface, void *state_dat
 }

static HRESULT WINAPI band_track_GetParam(IDirectMusicTrack8 *iface, REFGUID type, MUSIC_TIME time,
-        MUSIC_TIME *next, void *param)
+        MUSIC_TIME *out_next, void *param)
 {
     struct band_track *This = impl_from_IDirectMusicTrack8(iface);
+    struct band_entry *band;
+    DMUS_BAND_PARAM *bandparam;
+    MUSIC_TIME prev = -1, next = time;

-    TRACE("(%p, %s, %ld, %p, %p)\n", This, debugstr_dmguid(type), time, next, param);
+    TRACE("(%p, %s, %ld, %p, %p)\n", This, debugstr_dmguid(type), time, out_next, param);

if (!type)
         return E_POINTER;
     if (!IsEqualGUID(type, &GUID_BandParam))
         return DMUS_E_GET_UNSUPPORTED;

-    FIXME("GUID_BandParam not handled yet\n");
+    bandparam = param;

+    LIST_FOR_EACH_ENTRY(band, &This->bands, struct band_entry, entry)
+    {
+        if (band->head.lBandTimeLogical <= time && band->head.lBandTimeLogical >= prev)
+        {
+            bandparam->pBand = band->band;
+            bandparam->mtTimePhysical = band->head.lBandTimePhysical;
+            prev = band->head.lBandTimeLogical;
+        }
+        else if (band->head.lBandTimeLogical > time && band->head.lBandTimeLogical < next)
+            next = band->head.lBandTimeLogical;
+    }
+
+    if (prev == -1)
+        return DMUS_E_NOT_FOUND;
+
+    IDirectMusicBand_AddRef(bandparam->pBand);
+    if (out_next) *out_next = next - time;
     return S_OK;
 }

@@ -227,7 +247,19 @@ static HRESULT WINAPI band_track_SetParam(IDirectMusicTrack8 *iface, REFGUID typ
         return DMUS_E_TYPE_UNSUPPORTED;

if (IsEqualGUID(type, &GUID_BandParam))
-        FIXME("GUID_BandParam not handled yet\n");
+    {
+        struct band_entry *entry = calloc(1, sizeof(*entry));
+        DMUS_BAND_PARAM *band_param = param;
+        if (!band_param || !band_param->pBand)
+            return E_POINTER;
+        if (!entry)
+            return E_OUTOFMEMORY;
+        entry->band = band_param->pBand;
+        entry->head.lBandTimeLogical = time;
+        entry->head.lBandTimePhysical = band_param->mtTimePhysical;
+        IDirectMusicBand_AddRef(entry->band);
+        list_add_tail(&This->bands, &entry->entry);
+    }
     else if (IsEqualGUID(type, &GUID_Clear_All_Bands))
         FIXME("GUID_Clear_All_Bands not handled yet\n");
     else if (IsEqualGUID(type, &GUID_ConnectToDLSCollection))
diff --git a/dlls/dmband/tests/dmband.c b/dlls/dmband/tests/dmband.c
index d88606976e4..36f213b0bfd 100644
--- a/dlls/dmband/tests/dmband.c
+++ b/dlls/dmband/tests/dmband.c
@@ -176,9 +176,12 @@ static void test_dmband(void)

static void test_bandtrack(void)
 {
+    DMUS_BAND_PARAM bandparam = { 0 };
     IDirectMusicTrack8 *dmt8;
+    IDirectMusicBand *dmb;
     IPersistStream *ps;
     CLSID class;
+    MUSIC_TIME mt;
     ULARGE_INTEGER size;
     char buf[64] = { 0 };
     HRESULT hr;
@@ -266,6 +269,30 @@ static void test_bandtrack(void)
         }
     }

+    hr = CoCreateInstance(&CLSID_DirectMusicBand, NULL, CLSCTX_INPROC_SERVER,
+            &IID_IDirectMusicBand, (void**)&dmb);
+    ok(hr == S_OK, "DirectMusicBand create failed: %#lx, expected S_OK\n", hr);
+
+    hr = IDirectMusicTrack8_GetParam(dmt8, &GUID_BandParam, 0, &mt, &bandparam);
+    ok(hr == DMUS_E_NOT_FOUND, "hr %#lx, expected not found\n", hr);
+    ok(bandparam.pBand == NULL, "Got band %p, expected NULL\n", bandparam.pBand);
+
+    bandparam.mtTimePhysical = 0;
+    bandparam.pBand = dmb;
+    hr = IDirectMusicTrack8_SetParam(dmt8, &GUID_BandParam, 0, &bandparam);
+    ok(hr == S_OK, "SetParam failed: %#lx, expected S_OK\n", hr);
+
+    hr = IDirectMusicTrack8_GetParam(dmt8, &GUID_BandParam, 0, &mt, &bandparam);
+    ok(hr == S_OK, "GetParam failed: %#lx, expected S_OK\n", hr);
+    ok(mt == 0, "Got time %lu, expected 0\n", mt);
+    ok(bandparam.mtTimePhysical == 0, "Got physical time %lu, expected 0\n", bandparam.mtTimePhysical);
+    IDirectMusicBand_Release(bandparam.pBand);
+    IDirectMusicBand_Release(dmb);
+
+    bandparam.pBand = NULL;
+    hr = IDirectMusicTrack8_SetParam(dmt8, &GUID_BandParam, 0, &bandparam);
+    ok(hr == E_POINTER, "hr %#lx, expected E_POINTER\n", hr);
+
     hr = IDirectMusicTrack8_AddNotificationType(dmt8, NULL);
     ok(hr == E_NOTIMPL, "IDirectMusicTrack8_AddNotificationType failed: %#lx\n", hr);
     hr = IDirectMusicTrack8_RemoveNotificationType(dmt8, NULL);

[Rémi Bernon (＠rbernon)]

Rémi Bernon (@rbernon) commented about dlls/dmband/bandtrack.c:
>      return DMUS_E_TYPE_UNSUPPORTED;
>
>  if (IsEqualGUID(type, &GUID_BandParam))
> -
>    FIXME("GUID_BandParam not handled yet\n");
> - {
> -
>    struct band_entry *entry = calloc(1, sizeof(*entry));
> -
>    DMUS_BAND_PARAM *band_param = param;
> -
>    if (!band_param || !band_param->pBand)
> -
>        return E_POINTER;
> -
>    if (!entry)
> -
>        return E_OUTOFMEMORY;
> -
>    entry->band = band_param->pBand;

```suggestion:-6+0
        struct band_entry *entry;
        DMUS_BAND_PARAM *band_param = param;
        if (!band_param || !band_param->pBand)
            return E_POINTER;
        if (!(entry = calloc(1, sizeof(*entry))))
            return E_OUTOFMEMORY;
        entry->band = band_param->pBand;
```

To avoid a potential leak on error.

[Rémi Bernon (＠rbernon)]

Rémi Bernon (@rbernon) commented about dlls/dmband/bandtrack.c:
> - MUSIC_TIME prev = -1, next = time;
> - TRACE("(%p, %s, %ld, %p, %p)\n", This, debugstr_dmguid(type), time, next, param);
> -
>
> TRACE("(%p, %s, %ld, %p, %p)\n", This, debugstr_dmguid(type), time, out_next, param);
>
> if (!type)
>     return E_POINTER;
> if (!IsEqualGUID(type, &GUID_BandParam))
>     return DMUS_E_GET_UNSUPPORTED;
> - FIXME("GUID_BandParam not handled yet\n");
> -
>
> bandparam = param;
> -
>
> LIST_FOR_EACH_ENTRY(band, &This->bands, struct band_entry, entry)
> -
>
> {
> -
>    if (band->head.lBandTimeLogical <= time && band->head.lBandTimeLogical >= prev)

Adding a few more tests you'll see that this should return the first band even if `lBandTimeLogical >= time`.

[Rémi Bernon (＠rbernon)]

Rémi Bernon (@rbernon) commented about dlls/dmband/bandtrack.c:
> - {
> -
>    if (band->head.lBandTimeLogical <= time && band->head.lBandTimeLogical >= prev)
> -
>    {
> -
>        bandparam->pBand = band->band;
> -
>        bandparam->mtTimePhysical = band->head.lBandTimePhysical;
> -
>        prev = band->head.lBandTimeLogical;
> -
>    }
> -
>    else if (band->head.lBandTimeLogical > time && band->head.lBandTimeLogical < next)
> -
>        next = band->head.lBandTimeLogical;
> - }
> -
> - if (prev == -1)
> -
>    return DMUS_E_NOT_FOUND;
> -
> - IDirectMusicBand_AddRef(bandparam->pBand);
> - if (out_next) *out_next = next - time;

Also, with a couple more tests you'll see that this is not supposed to be a difference but simply the next band logical time.

[Yuxuan Shui (＠yshui)]

On Tue Feb  6 18:39:17 2024 +0000, Rémi Bernon wrote:
> Adding a few more tests you'll see that this should return the first
> band even if `lBandTimeLogical >= time`.

sorry, it's somewhat unclear what you meant. are you saying when there's multiple band with the same logical time, the first one is returned?

[Yuxuan Shui (＠yshui)]

On Tue Feb  6 18:39:18 2024 +0000, Rémi Bernon wrote:
> Also, with a couple more tests you'll see that this is not supposed to
> be a difference but simply the next band logical time.

guess i shouldn't have trusted the documentation :/

[Rémi Bernon (＠rbernon)]

On Fri Feb  9 11:23:56 2024 +0000, Yuxuan Shui wrote:
> sorry, it's somewhat unclear what you meant. are you saying when there's
> multiple band with the same logical time, the first one is returned?

I meant that if you add a single band with a time that's > 0, it still returns that band from GetParam with time == 0.

Anyway, how that works with multiple bands will also require tests, so just write more tests :).

[Yuxuan Shui (＠yshui)]

On Fri Feb  9 11:36:51 2024 +0000, Rémi Bernon wrote:
> I meant that if you add a single band with a time that's > 0, it still
> returns that band from GetParam with time == 0.
> Anyway, how that works with multiple bands will also require tests, so
> just write more tests :).

so how do i tell bands apart? the pointers returned by GetParam is different from what i gave it, so i can't compare them (i think it clones the band internally somehow? do we need to do that too?). and the DMUS_OBJ_NAME i set on it disappears too.