[PATCH v3 0/2] MR7230: dmime: Fix loading and default soundfont collection.

[Yuxuan Shui (＠yshui)]

* * *

BTW I did try to create tests to make sure we connect collection the same way Windows does it. Problem is `GUID_ConnectToDLSCollection` is not gettable. Then I tried to create a `IDirectMusicPerformance` implementation and intercept `DownloadInstrument`. Interestingly Windows `IDirectMusicSegment::Download` doesn't call `DownloadInstrument` at all, instead it downloads instruments through a private, undocumented interface. So there seems to be no way to test this.

--
  v3: dmimi: Connect default collection to MIDI bandtrack.
      dmime: Handle IStream EOF correctly in MIDI parser.

https://gitlab.winehq.org/wine/wine/-/merge_requests/7230

[PATCH v3 1/2] dmime: Handle IStream EOF correctly in MIDI parser.

[Yuxuan Shui]

From: Yuxuan Shui

Read returns E_FAIL when the EOF is encountered, which was treated as a failure.
---
 dlls/dmime/midi.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/dlls/dmime/midi.c b/dlls/dmime/midi.c
index 57b6a3adf90..74a5f928a17 100644
--- a/dlls/dmime/midi.c
+++ b/dlls/dmime/midi.c
@@ -332,15 +332,19 @@ static HRESULT midi_parser_parse(struct midi_parser *parser, IDirectMusicSegment

for (i = 0;; i++)
     {
-        BYTE magic[4] = {0}, last_status = 0;
+        BYTE magic[4], last_status = 0;
         DWORD length_be;
         ULONG length;
         ULONG read = 0;
         struct midi_event event = {0};

TRACE("Start parsing track %u\n", i);
-        if ((hr = IStream_Read(parser->stream, magic, sizeof(magic), &read)) != S_OK) break;
-        if (read < sizeof(magic)) break;
+        hr = IStream_Read(parser->stream, magic, sizeof(magic), &read);
+        if (read < sizeof(magic)) {
+            if (hr == E_FAIL) hr = S_OK;
+            break;
+        }
+        if (FAILED(hr)) break;
         if (memcmp(magic, "MTrk", 4) != 0) break;

if ((hr = IStream_Read(parser->stream, &length_be, sizeof(length_be), &read)) != S_OK)

[PATCH v3 2/2] dmimi: Connect default collection to MIDI bandtrack.

[Yuxuan Shui]

From: Yuxuan Shui

---
 dlls/dmime/midi.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/dlls/dmime/midi.c b/dlls/dmime/midi.c
index 74a5f928a17..3928976c1e0 100644
--- a/dlls/dmime/midi.c
+++ b/dlls/dmime/midi.c
@@ -327,8 +327,19 @@ static HRESULT midi_parser_parse(struct midi_parser *parser, IDirectMusicSegment
     MUSIC_TIME music_length = 0;
     DMUS_IO_SEQ_ITEM *seq_items = NULL;
     struct midi_seqtrack_item *item;
+    DMUS_OBJECTDESC default_desc =
+    {
+        .dwSize = sizeof(DMUS_OBJECTDESC),
+        .dwValidData = DMUS_OBJ_OBJECT | DMUS_OBJ_CLASS,
+        .guidClass = CLSID_DirectMusicCollection,
+        .guidObject = GUID_DefaultGMCollection,
+    };
+    IDirectMusicObject *collection = NULL;

TRACE("(%p, %p): semi-stub\n", parser, segment);
+    if (FAILED(hr = stream_get_object(parser->stream, &default_desc, &IID_IDirectMusicCollection,
+            (void **)&collection)))
+        WARN("Failed to load default collection from loader, hr %#lx\n", hr);

for (i = 0;; i++)
     {
@@ -403,6 +414,7 @@ static HRESULT midi_parser_parse(struct midi_parser *parser, IDirectMusicSegment
     qsort(seq_items, parser->seqtrack_items_count, sizeof(DMUS_IO_SEQ_ITEM), midi_seqtrack_item_compare);

music_length = (ULONGLONG)music_length * DMUS_PPQ / parser->division + 1;
+    if (collection) IDirectMusicTrack_SetParam(parser->bandtrack, &GUID_ConnectToDLSCollection, 0, collection);
     if (SUCCEEDED(hr)) hr = IDirectMusicSegment8_SetLength(segment, music_length);
     if (SUCCEEDED(hr)) hr = IDirectMusicSegment8_InsertTrack(segment, parser->bandtrack, 0xffff);
     if (SUCCEEDED(hr)) hr = IDirectMusicSegment8_InsertTrack(segment, parser->chordtrack, 0xffff);

[Yuxuan Shui (＠yshui)]

On Thu Feb  6 18:18:40 2025 +0000, Yuxuan Shui wrote:
> changed this line in [version 3 of the diff](/wine/wine/-/merge_requests/7230/diffs?diff_id=156069&start_sha=4ae60118edcb26d00d13838edc038aa624478ba5#d6cec31b6bad63c7ac3cfdf7adde34022c856065_354_354)

I remove the init, I think keeping the short read check fits the definition of `E_FAIL` better:
> In particular, if the read attempt was successful but fewer than cb bytes were read, the function returns E_FAIL.

Which implies short read is sufficient but not necessary for `E_FAIL`

[Michael Stefaniuc (＠mstefani)]

On Thu Feb  6 18:20:15 2025 +0000, Yuxuan Shui wrote:
> I remove the init, I think keeping the short read check fits the
> definition of `E_FAIL` better:
> > In particular, if the read attempt was successful but fewer than cb
>
> bytes were read, the function returns E_FAIL.
> Which implies short read is sufficient but not necessary for `E_FAIL`

I don't mind either. Both work.\
But short read check is more obvious solution.

[PATCH v3 0/2] MR7230: dmime: Fix loading and default soundfont collection. - approved

[Michael Stefaniuc (＠mstefani)]

This merge request was approved by Michael Stefaniuc.

[Michael Stefaniuc (＠mstefani)]

Test failures are unrelated in two D3D device.c

[Rémi Bernon (＠rbernon)]

Rémi Bernon (@rbernon) commented about dlls/dmime/midi.c:
>  for (i = 0;; i++)
>  {
> -
>    BYTE magic[4] = {0}, last_status = 0;
> -
>    BYTE magic[4], last_status = 0;
>    DWORD length_be;
>    ULONG length;
>    ULONG read = 0;
>    struct midi_event event = {0};
>
>    TRACE("Start parsing track %u\n", i);
> -
>    if ((hr = IStream_Read(parser->stream, magic, sizeof(magic), &read)) != S_OK) break;
> -
>    if (read < sizeof(magic)) break;
> -
>    hr = IStream_Read(parser->stream, magic, sizeof(magic), &read);
> -
>    if (read < sizeof(magic)) {

You're missing a line break before the brace.