[PATCH v2 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.

--
  v2: 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 v2 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 | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/dlls/dmime/midi.c b/dlls/dmime/midi.c
index 57b6a3adf90..ba6d16e73e2 100644
--- a/dlls/dmime/midi.c
+++ b/dlls/dmime/midi.c
@@ -339,8 +339,12 @@ static HRESULT midi_parser_parse(struct midi_parser *parser, IDirectMusicSegment
         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 (hr == E_FAIL || read < sizeof(magic)) {
+            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 v2 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 ba6d16e73e2..bc8319feccb 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);

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

[Marvin]

Hi,

It looks like your patch introduced the new failures shown below.
Please investigate and fix them before resubmitting your patch.
If they are not new, fixing them anyway would help a lot. Otherwise
please ask for the known failures list to be updated.

The tests also ran into some preexisting test failures. If you know how
to fix them that would be helpful. See the TestBot job for the details:

The full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=151162

Your paranoid android.

=== debian11b (64 bit WoW report) ===

user32:
input.c:4306: Test succeeded inside todo block: button_down_hwnd_todo 1: got MSG_TEST_WIN hwnd 00000000016500E0, msg WM_LBUTTONDOWN, wparam 0x1, lparam 0x320032

Report validation errors:
mshtml:script crashed (c0000005)

[Michael Stefaniuc (＠mstefani)]

Michael Stefaniuc (@mstefani) commented about dlls/dmime/midi.c:
>      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 (hr == E_FAIL || read < sizeof(magic)) {

I'm confused about the EOF detection here.

`IStream_Read()` should give you the `E_FAIL` if the stream is already at EOF. But then the read is `0`.\
So the check should be more of a `hr == E_FAIL && !read`. Assuming that any other `E_FAIL` should be still passed on.

Also what should happen for an `E_FAIL` during the `i == 0` case? Aka no track read? Should that be still ignored?

The short read case `read < sizeof(magic)` is already handled by the comparison of `magic` with `"MTrk"`.

[Michael Stefaniuc (＠mstefani)]

In addition to the EOF detection comment:\
The number of midi tracks should come from the `NumTracks` field of the `MThd` chunk, no?\
The infinite for loop would change to
```
   for (i = 0; i < NumTracks; i++)
```
and every E_FAIL would be an error. Even if that's due to EOF as that would be a premature EOF (corrupt file).

Tests are not a problem for patch 2. But can you please specify which application relies on that change?\

Just a minor nitpick on patch 2: The commit message contains a typo `dmimi:`.