[PATCH v3 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. -- 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
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 | 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) -- 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 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); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7230
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` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7230#note_93944
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.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7230#note_93945
This merge request was approved by Michael Stefaniuc. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7230
Test failures are unrelated in two D3D device.c -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7230#note_93953
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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7230#note_94005
participants (4)
-
Michael Stefaniuc (＠mstefani) -
Rémi Bernon
-
Yuxuan Shui
-
Yuxuan Shui (＠yshui)