[PATCH v2 0/2] MR7230: dmime: Fix loading and default soundfont collection.
* * * 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
From: Yuxuan Shui <yshui(a)codeweavers.com> 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) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7230
From: Yuxuan Shui <yshui(a)codeweavers.com> --- 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); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7230
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) 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"`. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7230#note_93303
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:`. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7230#note_93304
On Wed Feb 5 15:41:56 2025 +0000, Michael Stefaniuc wrote:
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:`. Hi, this was specifically done this way because Windows implementation doesn't valid NumTracks. This was raised also back when the midi patch series was first proposed.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7230#note_93708
On Thu Jan 30 20:36:01 2025 +0000, Michael Stefaniuc wrote:
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"`. `!read` was changed to `read < sizeof(magic)` because of @rbernon's suggestion: https://gitlab.winehq.org/wine/wine/-/merge_requests/5081#note_61500 . Think malformed MIDI files with trailing bytes, etc.
Also, because short read leave `magic` uninitialized, you can't then compare it to `"MTrk"`. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7230#note_93710
On Wed Feb 5 15:45:03 2025 +0000, Yuxuan Shui wrote:
`!read` was changed to `read < sizeof(magic)` because of @rbernon's suggestion: https://gitlab.winehq.org/wine/wine/-/merge_requests/5081#note_61500 . Think malformed MIDI files with trailing bytes, etc. Also, because short read leave `magic` uninitialized, you can't then compare it to `"MTrk"`. If there's any ambiguity on whether this is right or not, probably adding a couple of tests with a truncated MIDI track will answer the question.
Other than that the brace needs to be on its own line. The other change LGTM. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7230#note_93742
On Wed Feb 5 17:14:28 2025 +0000, Rémi Bernon wrote:
If there's any ambiguity on whether this is right or not, probably adding a couple of tests with a truncated MIDI track will answer the question. Other than that the brace needs to be on its own line. The other change LGTM. No `magic` is always initialized to all 0 on each itteration of the for loop:
for (i = 0;; i++)
{
BYTE magic[4] = {0}, last_status = 0;
A short read won't change that. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7230#note_93750
On Wed Feb 5 15:45:59 2025 +0000, Yuxuan Shui wrote:
Hi, this was specifically done this way because Windows implementation doesn't validate NumTracks. This was raised also back when the midi patch series was first proposed. OK, no surprises there...
With that my question from the other thread remains important: Also what should happen for an E_FAIL during the `i == 0` case? Aka no track read? Should that be still ignored?\ Just trying to avoid a crash later on. If the code later on can deal with the "0 tracks read" case then all is good. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7230#note_93756
On Wed Feb 5 18:41:20 2025 +0000, Michael Stefaniuc wrote:
No `magic` is always initialized to all 0 on each itteration of the for loop: ``` for (i = 0;; i++) { BYTE magic[4] = {0}, last_status = 0; ``` A short read won't change that. Ah, right Rémi said to remove this initialization and I forgot... So either remove this init and keep the `read < sizeof(magic)` check, or remove that and keep this init. I don't have a preference here.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7230#note_93886
On Wed Feb 5 18:59:08 2025 +0000, Michael Stefaniuc wrote:
OK, no surprises there... With that my question from the other thread remains important: Also what should happen for an E_FAIL during the `i == 0` case? Aka no track read? Should that be still ignored?\ Just trying to avoid a crash later on. If the code later on can deal with the "0 tracks read" case then all is good. There is a test case for a midi file with 0 tracks.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7230#note_93938
participants (5)
-
Marvin -
Michael Stefaniuc (＠mstefani) -
Rémi Bernon
-
Yuxuan Shui
-
Yuxuan Shui (＠yshui)