[PATCH v15 0/3] MR5081: dmime: Read through a MIDI file
Add a dummy track parser, that reads through MIDI tracks and events. To make sure we understand the file structure correctly, no actual tracks or events are generated. Also add test cases to make sure we read through a MIDI file correctly. This is skeleton of the actual MIDI parser. Pending on the merge of !4982 -- v15: dmime/tests: Improve error reporting from expect_track. dmime: Read through a MIDI file. dmime: Don't skip chunk for MIDI files. https://gitlab.winehq.org/wine/wine/-/merge_requests/5081
From: Yuxuan Shui <yshui(a)codeweavers.com> MIDI files aren't RIFF, calling stream_skip_chunk on it is invalid. --- dlls/dmime/segment.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/dlls/dmime/segment.c b/dlls/dmime/segment.c index 82e06f4537d..b0485aaedd9 100644 --- a/dlls/dmime/segment.c +++ b/dlls/dmime/segment.c @@ -844,7 +844,9 @@ static HRESULT WINAPI segment_persist_stream_Load(IPersistStream *iface, IStream } } - stream_skip_chunk(stream, &chunk); + if (chunk.id != mmioFOURCC('M', 'T', 'h', 'd')) + stream_skip_chunk(stream, &chunk); + if (FAILED(hr)) { WARN("Failed to load segment from stream %p, hr %#lx\n", stream, hr); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5081
From: Yuxuan Shui <yshui(a)codeweavers.com> Add a dummy track parser, that reads through MIDI tracks and events. To make sure we understand the file structure correctly, no actual tracks or events are generated. Also add test cases to make sure we read through a MIDI file correctly. --- dlls/dmime/midi.c | 153 ++++++++++++++++++++++--- dlls/dmime/tests/dmime.c | 236 +++++++++++++++++++++++++++++++-------- 2 files changed, 325 insertions(+), 64 deletions(-) diff --git a/dlls/dmime/midi.c b/dlls/dmime/midi.c index a7e57947c6f..f8d87ee5d4f 100644 --- a/dlls/dmime/midi.c +++ b/dlls/dmime/midi.c @@ -34,9 +34,145 @@ struct midi_parser IStream *stream; }; +enum meta_event_type +{ + MIDI_META_SEQUENCE_NUMBER = 0x00, + MIDI_META_TEXT_EVENT = 0x01, + MIDI_META_COPYRIGHT_NOTICE = 0x02, + MIDI_META_TRACK_NAME = 0x03, + MIDI_META_INSTRUMENT_NAME = 0x04, + MIDI_META_LYRIC = 0x05, + MIDI_META_MARKER = 0x06, + MIDI_META_CUE_POINT = 0x07, + MIDI_META_CHANNEL_PREFIX_ASSIGNMENT = 0x20, + MIDI_META_END_OF_TRACK = 0x2f, + MIDI_META_SET_TEMPO = 0x51, + MIDI_META_SMPTE_OFFSET = 0x54, + MIDI_META_TIME_SIGNATURE = 0x58, + MIDI_META_KEY_SIGNATURE = 0x59, + MIDI_META_SEQUENCER_SPECIFIC = 0x7f, +}; + +static HRESULT stream_read_at_most(IStream *stream, void *buffer, ULONG size, ULONG *bytes_left) +{ + HRESULT hr; + ULONG read = 0; + if (size > *bytes_left) hr = IStream_Read(stream, buffer, *bytes_left, &read); + else hr = IStream_Read(stream, buffer, size, &read); + if (hr != S_OK) return hr; + *bytes_left -= read; + if (read < size) return S_FALSE; + return S_OK; +} + +static HRESULT read_variable_length_number(IStream *stream, DWORD *out, ULONG *bytes_left) +{ + BYTE byte; + HRESULT hr = S_OK; + + *out = 0; + do + { + hr = stream_read_at_most(stream, &byte, 1, bytes_left); + if (hr != S_OK) return hr; + *out = (*out << 7) | (byte & 0x7f); + } while (byte & 0x80); + return S_OK; +} + +static HRESULT read_midi_event(IStream *stream, BYTE *last_status, ULONG *bytes_left) +{ + BYTE byte; + BYTE status, meta_event_type; + DWORD length; + LARGE_INTEGER offset; + HRESULT hr = S_OK; + DWORD delta_time; + + if ((hr = read_variable_length_number(stream, &delta_time, bytes_left)) != S_OK) return hr; + + if ((hr = stream_read_at_most(stream, &byte, 1, bytes_left)) != S_OK) return hr; + + if (byte & 0x80) + { + status = byte; + *last_status = byte; + if ((hr = stream_read_at_most(stream, &byte, 1, bytes_left)) != S_OK) return hr; + } + else status = *last_status; + + if (status == 0xff) + { + meta_event_type = byte; + + if ((hr = read_variable_length_number(stream, &length, bytes_left)) != S_OK) return hr; + + switch (meta_event_type) + { + default: + if (*bytes_left < length) return S_FALSE; + offset.QuadPart = length; + if (FAILED(hr = IStream_Seek(stream, offset, STREAM_SEEK_CUR, NULL))) return hr; + FIXME("MIDI meta event type %#02x, length %lu, time +%lu. not supported\n", + meta_event_type, length, delta_time); + *bytes_left -= length; + } + TRACE("MIDI meta event type %#02x, length %lu, time +%lu\n", meta_event_type, length, delta_time); + } + else if (status == 0xf0 || status == 0xf7) + { + if (byte & 0x80) + { + if ((hr = read_variable_length_number(stream, &length, bytes_left)) != S_OK) return hr; + length = length << 8 | (byte & 0x7f); + } + else length = byte; + + if (*bytes_left < length) return S_FALSE; + offset.QuadPart = length; + if (FAILED(hr = IStream_Seek(stream, offset, STREAM_SEEK_CUR, NULL))) return hr; + *bytes_left -= length; + FIXME("MIDI sysex event type %#02x, length %lu, time +%lu. not supported\n", status, length, delta_time); + } + else + { + if ((status & 0xf0) != 0xc0 && (status & 0xf0) != 0xd0) + if ((hr = stream_read_at_most(stream, &byte, 1, bytes_left)) != S_OK) return hr; + FIXME("MIDI event status %#02x, time +%lu, not supported\n", status, delta_time); + } + + return S_OK; +} + HRESULT midi_parser_next_track(struct midi_parser *parser, IDirectMusicTrack **out_track, MUSIC_TIME *out_length) { - TRACE("(%p, %p, %p): stub\n", parser, out_track, out_length); + WORD i = 0; + TRACE("(%p, %p): stub\n", parser, out_length); + while (TRUE) + { + HRESULT hr; + BYTE magic[4] = {0}, last_status = 0; + DWORD length_be; + ULONG length; + ULONG read = 0; + + TRACE("Start parsing track %u\n", i); + if ((hr = IStream_Read(parser->stream, magic, sizeof(magic), &read)) != S_OK) return hr; + if (read == 0) break; + if (memcmp(magic, "MTrk", 4) != 0) break; + + if ((hr = IStream_Read(parser->stream, &length_be, sizeof(length_be), NULL)) != S_OK) break; + length = GET_BE_DWORD(length_be); + TRACE("Track %u, length %lu bytes\n", i, length); + + while ((hr = read_midi_event(parser->stream, &last_status, &length)) == S_OK) + ; + + if (FAILED(hr)) return hr; + TRACE("End of track %u\n", i); + i += 1; + } + TRACE("End of file\n"); return S_FALSE; } @@ -56,24 +192,9 @@ HRESULT midi_parser_new(IStream *stream, struct midi_parser **out_parser) if (FAILED(hr = IStream_Read(stream, &length, sizeof(length), NULL))) return hr; length = GET_BE_DWORD(length); - if (length != 6) - { - WARN("Invalid MIDI header length %lu\n", length); - return DMUS_E_UNSUPPORTED_STREAM; - } if (FAILED(hr = IStream_Read(stream, &format, sizeof(format), NULL))) return hr; format = GET_BE_WORD(format); - if (format > 2) - { - WARN("Invalid MIDI format %u\n", format); - return DMUS_E_UNSUPPORTED_STREAM; - } - if (format == 2) - { - FIXME("MIDI format 2 not implemented yet\n"); - return DMUS_E_UNSUPPORTED_STREAM; - } if (FAILED(hr = IStream_Read(stream, &number_of_tracks, sizeof(number_of_tracks), NULL))) return hr; diff --git a/dlls/dmime/tests/dmime.c b/dlls/dmime/tests/dmime.c index eaeff2e86bc..6ccb944375c 100644 --- a/dlls/dmime/tests/dmime.c +++ b/dlls/dmime/tests/dmime.c @@ -21,6 +21,7 @@ #include <stdarg.h> #include <math.h> #include <windef.h> +#include <winternl.h> #include <wine/test.h> #include <initguid.h> #include <ole2.h> @@ -32,6 +33,14 @@ DEFINE_GUID(GUID_NULL,0,0,0,0,0,0,0,0,0,0,0); DEFINE_GUID(GUID_Bunk,0xFFFFFFFF,0xFFFF,0xFFFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF); +#ifdef WORDS_BIGENDIAN +#define GET_BE_WORD(x) (x) +#define GET_BE_DWORD(x) (x) +#else +#define GET_BE_WORD(x) RtlUshortByteSwap(x) +#define GET_BE_DWORD(x) RtlUlongByteSwap(x) +#endif + static ULONG get_refcount(void *iface) { IUnknown *unknown = iface; @@ -1554,14 +1563,81 @@ static void test_segment(void) while (IDirectMusicSegment_Release(dms)); } +static void _expect_track(IDirectMusicSegment8 *seg, REFCLSID expect, const char *name, DWORD group, + DWORD index, BOOL ignore_guid) +{ + IDirectMusicTrack *track; + IPersistStream *ps; + CLSID class; + HRESULT hr; + + if (ignore_guid) + hr = IDirectMusicSegment8_GetTrack(seg, &GUID_NULL, group, index, &track); + else + hr = IDirectMusicSegment8_GetTrack(seg, expect, group, index, &track); + if (!expect) { + ok(hr == DMUS_E_NOT_FOUND, "GetTrack failed: %#lx, expected DMUS_E_NOT_FOUND\n", hr); + return; + } + + ok(hr == S_OK, "GetTrack failed: %#lx, expected S_OK\n", hr); + if (FAILED(hr)) return; + hr = IDirectMusicTrack_QueryInterface(track, &IID_IPersistStream, (void**)&ps); + ok(hr == S_OK, "QueryInterface for IID_IPersistStream failed: %#lx\n", hr); + hr = IPersistStream_GetClassID(ps, &class); + ok(hr == S_OK, "IPersistStream_GetClassID failed: %#lx\n", hr); + ok(IsEqualGUID(&class, expect), "For group %#lx index %lu: Expected class %s got %s\n", + group, index, name, wine_dbgstr_guid(&class)); + + IPersistStream_Release(ps); + IDirectMusicTrack_Release(track); +} + +#define expect_track(seg, class, group, index) \ + _expect_track(seg, &CLSID_DirectMusic ## class, #class, group, index, TRUE) +#define expect_guid_track(seg, class, group, index) \ + _expect_track(seg, &CLSID_DirectMusic ## class, #class, group, index, FALSE) + static void test_midi(void) { + static const char midi_meta_set_tempo[] = + { + 0x04, /* delta time = 4 */ + 0xff, /* event type, MIDI meta event */ + 0x51, /* meta event type, Set Tempo */ + 0x03, /* event data lenght, 3 bytes */ + 0x03,0x0d,0x40 /* tempo, 200000 us per quarter-note, i.e. 300 bpm */ + }; IDirectMusicSegment8 *segment = NULL; IDirectMusicTrack *track = NULL; IDirectMusicLoader8 *loader; + IPersistStream *persist; + IStream *stream; + LARGE_INTEGER zero = { .QuadPart = 0 }; + ULARGE_INTEGER position = { .QuadPart = 0 }; WCHAR test_mid[MAX_PATH], bogus_mid[MAX_PATH]; HRESULT hr; - +#include <pshpack1.h> + struct + { + char magic[4]; + UINT32 length; + WORD format; + WORD count; + WORD ppqn; + } header = + { + .magic = "MThd", + }; + struct + { + char magic[4]; + UINT32 length; + } track_header = + { + .magic = "MTrk", + }; +#include <poppack.h> load_resource(L"test.mid", test_mid); /* This is a MIDI file with wrong track length. */ load_resource(L"bogus.mid", bogus_mid); @@ -1573,19 +1649,14 @@ static void test_midi(void) &IID_IDirectMusicSegment, test_mid, (void **)&segment); ok(hr == S_OK, "got %#lx\n", hr); - /* test.mid has 1 seq track, 1 tempo track, and 1 band track */ - hr = IDirectMusicSegment8_GetTrack(segment, &CLSID_DirectMusicBandTrack, 0xffffffff, 0, &track); - todo_wine ok(hr == S_OK, "unable to get band track from midi file: %#lx\n", hr); - if (track)IDirectMusicTrack_Release(track); - track = NULL; - hr = IDirectMusicSegment8_GetTrack(segment, &CLSID_DirectMusicSeqTrack, 0xffffffff, 0, &track); - todo_wine ok(hr == S_OK, "got %#lx\n", hr); - if (track) IDirectMusicTrack_Release(track); - track = NULL; - hr = IDirectMusicSegment8_GetTrack(segment, &CLSID_DirectMusicTempoTrack, 0xffffffff, 0, &track); - todo_wine ok(hr == S_OK, "got %#lx\n", hr); - if (track) IDirectMusicTrack_Release(track); - track = NULL; + todo_wine expect_track(segment, BandTrack, -1, 0); + todo_wine expect_track(segment, ChordTrack, -1, 1); + todo_wine expect_track(segment, TempoTrack, -1, 2); + todo_wine expect_track(segment, TimeSigTrack, -1, 3); + todo_wine expect_track(segment, SeqTrack, -1, 4); + /* no more tracks */ + hr = IDirectMusicSegment8_GetTrack(segment, &GUID_NULL, -1, 5, &track); + ok(hr == DMUS_E_NOT_FOUND, "unexpected extra track\n"); if (segment) IDirectMusicSegment8_Release(segment); segment = NULL; @@ -1594,6 +1665,109 @@ static void test_midi(void) ok(hr == S_OK, "got %#lx\n", hr); if (segment) IDirectMusicSegment8_Release(segment); + /* parse MIDI file without any track */ + + hr = CoCreateInstance(&CLSID_DirectMusicSegment, NULL, CLSCTX_INPROC_SERVER, + &IID_IDirectMusicSegment, (void **)&segment); + ok(hr == S_OK, "got %#lx\n", hr); + + hr = IDirectMusicSegment_QueryInterface(segment, &IID_IPersistStream, (void **)&persist); + ok(hr == S_OK, "got %#lx\n", hr); + hr = CreateStreamOnHGlobal(0, TRUE, &stream); + ok(hr == S_OK, "got %#lx\n", hr); + header.format = GET_BE_WORD(123); + header.count = GET_BE_WORD(123); + header.ppqn = GET_BE_WORD(123); + header.length = GET_BE_DWORD(sizeof(header) - 8); + hr = IStream_Write(stream, &header, sizeof(header), NULL); + ok(hr == S_OK, "got %#lx\n", hr); + hr = IStream_Seek(stream, zero, STREAM_SEEK_SET, NULL); + ok(hr == S_OK, "got %#lx\n", hr); + hr = IPersistStream_Load(persist, stream); + ok(hr == S_OK, "got %#lx\n", hr); + hr = IStream_Seek(stream, zero, STREAM_SEEK_CUR, &position); + ok(hr == S_OK, "got %#lx\n", hr); + ok(position.QuadPart == sizeof(header), "got %lld\n", position.QuadPart); + IPersistStream_Release(persist); + IStream_Release(stream); + /* TempoTrack and TimeSigTrack seems to be optional. */ + todo_wine expect_track(segment, BandTrack, -1, 0); + todo_wine expect_track(segment, ChordTrack, -1, 1); + todo_wine expect_track(segment, SeqTrack, -1, 2); + IDirectMusicSegment_Release(segment); + + /* parse MIDI file with 1 track that has 1 event. */ + + hr = CoCreateInstance(&CLSID_DirectMusicSegment, NULL, CLSCTX_INPROC_SERVER, + &IID_IDirectMusicSegment, (void **)&segment); + ok(hr == S_OK, "got %#lx\n", hr); + + hr = IDirectMusicSegment_QueryInterface(segment, &IID_IPersistStream, (void **)&persist); + ok(hr == S_OK, "got %#lx\n", hr); + hr = CreateStreamOnHGlobal(0, TRUE, &stream); + ok(hr == S_OK, "got %#lx\n", hr); + header.format = GET_BE_WORD(123); + header.count = GET_BE_WORD(123); + header.ppqn = GET_BE_WORD(123); + header.length = GET_BE_DWORD(sizeof(header) - 8); + hr = IStream_Write(stream, &header, sizeof(header), NULL); + ok(hr == S_OK, "got %#lx\n", hr); + track_header.length = RtlUlongByteSwap(sizeof(track_header) - 8 + sizeof(midi_meta_set_tempo)); + hr = IStream_Write(stream, &track_header, sizeof(track_header), NULL); + ok(hr == S_OK, "got %#lx\n", hr); + hr = IStream_Write(stream, midi_meta_set_tempo, sizeof(midi_meta_set_tempo), NULL); + ok(hr == S_OK, "got %#lx\n", hr); + hr = IStream_Seek(stream, zero, 0, NULL); + ok(hr == S_OK, "got %#lx\n", hr); + hr = IPersistStream_Load(persist, stream); + ok(hr == S_OK, "got %#lx\n", hr); + hr = IStream_Seek(stream, zero, STREAM_SEEK_CUR, &position); + ok(hr == S_OK, "got %#lx\n", hr); + ok(position.QuadPart == sizeof(header) + sizeof(track_header) + sizeof(midi_meta_set_tempo), + "got %lld\n", position.QuadPart); + IPersistStream_Release(persist); + IStream_Release(stream); + todo_wine expect_track(segment, BandTrack, -1, 0); + todo_wine expect_track(segment, ChordTrack, -1, 1); + todo_wine expect_track(segment, TempoTrack, -1, 2); + todo_wine expect_track(segment, SeqTrack, -1, 3); + IDirectMusicSegment_Release(segment); + + /* parse MIDI file with a track with 0 length, but has an event. */ + + hr = CoCreateInstance(&CLSID_DirectMusicSegment, NULL, CLSCTX_INPROC_SERVER, + &IID_IDirectMusicSegment, (void **)&segment); + ok(hr == S_OK, "got %#lx\n", hr); + + hr = IDirectMusicSegment_QueryInterface(segment, &IID_IPersistStream, (void **)&persist); + ok(hr == S_OK, "got %#lx\n", hr); + hr = CreateStreamOnHGlobal(0, TRUE, &stream); + ok(hr == S_OK, "got %#lx\n", hr); + header.format = GET_BE_WORD(123); + header.count = GET_BE_WORD(123); + header.ppqn = GET_BE_WORD(123); + header.length = 0; + hr = IStream_Write(stream, &header, sizeof(header), NULL); + ok(hr == S_OK, "got %#lx\n", hr); + track_header.length = 0; + hr = IStream_Write(stream, &track_header, sizeof(track_header), NULL); + ok(hr == S_OK, "got %#lx\n", hr); + hr = IStream_Write(stream, midi_meta_set_tempo, sizeof(midi_meta_set_tempo), NULL); + ok(hr == S_OK, "got %#lx\n", hr); + hr = IStream_Seek(stream, zero, 0, NULL); + ok(hr == S_OK, "got %#lx\n", hr); + hr = IPersistStream_Load(persist, stream); + ok(hr == S_OK, "got %#lx\n", hr); + hr = IStream_Seek(stream, zero, STREAM_SEEK_CUR, &position); + ok(hr == S_OK, "got %#lx\n", hr); + ok(position.QuadPart == sizeof(header) + sizeof(track_header) + 4, "got %lld\n", position.QuadPart); + IPersistStream_Release(persist); + IStream_Release(stream); + todo_wine expect_track(segment, BandTrack, -1, 0); + todo_wine expect_track(segment, ChordTrack, -1, 1); + /* there is no tempo track. */ + todo_wine expect_track(segment, SeqTrack, -1, 2); + IDirectMusicSegment_Release(segment); IDirectMusicLoader8_Release(loader); } @@ -1615,40 +1789,6 @@ static void _add_track(IDirectMusicSegment8 *seg, REFCLSID class, const char *na #define add_track(seg, class, group) _add_track(seg, &CLSID_DirectMusic ## class, #class, group) -static void _expect_track(IDirectMusicSegment8 *seg, REFCLSID expect, const char *name, DWORD group, - DWORD index, BOOL ignore_guid) -{ - IDirectMusicTrack *track; - IPersistStream *ps; - CLSID class; - HRESULT hr; - - if (ignore_guid) - hr = IDirectMusicSegment8_GetTrack(seg, &GUID_NULL, group, index, &track); - else - hr = IDirectMusicSegment8_GetTrack(seg, expect, group, index, &track); - if (!expect) { - ok(hr == DMUS_E_NOT_FOUND, "GetTrack failed: %#lx, expected DMUS_E_NOT_FOUND\n", hr); - return; - } - - ok(hr == S_OK, "GetTrack failed: %#lx, expected S_OK\n", hr); - hr = IDirectMusicTrack_QueryInterface(track, &IID_IPersistStream, (void**)&ps); - ok(hr == S_OK, "QueryInterface for IID_IPersistStream failed: %#lx\n", hr); - hr = IPersistStream_GetClassID(ps, &class); - ok(hr == S_OK, "IPersistStream_GetClassID failed: %#lx\n", hr); - ok(IsEqualGUID(&class, expect), "For group %#lx index %lu: Expected class %s got %s\n", - group, index, name, wine_dbgstr_guid(&class)); - - IPersistStream_Release(ps); - IDirectMusicTrack_Release(track); -} - -#define expect_track(seg, class, group, index) \ - _expect_track(seg, &CLSID_DirectMusic ## class, #class, group, index, TRUE) -#define expect_guid_track(seg, class, group, index) \ - _expect_track(seg, &CLSID_DirectMusic ## class, #class, group, index, FALSE) - static void test_gettrack(void) { IDirectMusicSegment8 *seg; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5081
From: Yuxuan Shui <yshui(a)codeweavers.com> Include a line number of the call site. --- dlls/dmime/tests/dmime.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/dlls/dmime/tests/dmime.c b/dlls/dmime/tests/dmime.c index 6ccb944375c..7e1b419251e 100644 --- a/dlls/dmime/tests/dmime.c +++ b/dlls/dmime/tests/dmime.c @@ -1564,7 +1564,7 @@ static void test_segment(void) } static void _expect_track(IDirectMusicSegment8 *seg, REFCLSID expect, const char *name, DWORD group, - DWORD index, BOOL ignore_guid) + DWORD index, BOOL ignore_guid, const char *file, UINT line) { IDirectMusicTrack *track; IPersistStream *ps; @@ -1576,17 +1576,17 @@ static void _expect_track(IDirectMusicSegment8 *seg, REFCLSID expect, const char else hr = IDirectMusicSegment8_GetTrack(seg, expect, group, index, &track); if (!expect) { - ok(hr == DMUS_E_NOT_FOUND, "GetTrack failed: %#lx, expected DMUS_E_NOT_FOUND\n", hr); + ok_(file, line)(hr == DMUS_E_NOT_FOUND, "GetTrack failed: %#lx, expected DMUS_E_NOT_FOUND\n", hr); return; } - ok(hr == S_OK, "GetTrack failed: %#lx, expected S_OK\n", hr); + ok_(file, line)(hr == S_OK, "GetTrack failed: %#lx, expected S_OK\n", hr); if (FAILED(hr)) return; hr = IDirectMusicTrack_QueryInterface(track, &IID_IPersistStream, (void**)&ps); - ok(hr == S_OK, "QueryInterface for IID_IPersistStream failed: %#lx\n", hr); + ok_(file, line)(hr == S_OK, "QueryInterface for IID_IPersistStream failed: %#lx\n", hr); hr = IPersistStream_GetClassID(ps, &class); - ok(hr == S_OK, "IPersistStream_GetClassID failed: %#lx\n", hr); - ok(IsEqualGUID(&class, expect), "For group %#lx index %lu: Expected class %s got %s\n", + ok_(file, line)(hr == S_OK, "IPersistStream_GetClassID failed: %#lx\n", hr); + ok_(file, line)(IsEqualGUID(&class, expect), "For group %#lx index %lu: Expected class %s got %s\n", group, index, name, wine_dbgstr_guid(&class)); IPersistStream_Release(ps); @@ -1594,9 +1594,9 @@ static void _expect_track(IDirectMusicSegment8 *seg, REFCLSID expect, const char } #define expect_track(seg, class, group, index) \ - _expect_track(seg, &CLSID_DirectMusic ## class, #class, group, index, TRUE) + _expect_track(seg, &CLSID_DirectMusic ## class, #class, group, index, TRUE, __FILE__, __LINE__) #define expect_guid_track(seg, class, group, index) \ - _expect_track(seg, &CLSID_DirectMusic ## class, #class, group, index, FALSE) + _expect_track(seg, &CLSID_DirectMusic ## class, #class, group, index, FALSE, __FILE__, __LINE__) static void test_midi(void) { @@ -1816,15 +1816,15 @@ static void test_gettrack(void) expect_track(seg, SeqTrack, 0x1, 2); expect_track(seg, TempoTrack, 0x1, 3); expect_track(seg, WaveTrack, 0x1, 4); - _expect_track(seg, NULL, "", 0x1, 5, TRUE); - _expect_track(seg, NULL, "", 0x1, DMUS_SEG_ANYTRACK, TRUE); + _expect_track(seg, NULL, "", 0x1, 5, TRUE, __FILE__, __LINE__); + _expect_track(seg, NULL, "", 0x1, DMUS_SEG_ANYTRACK, TRUE, __FILE__, __LINE__); expect_track(seg, ParamControlTrack, 0x2, 0); expect_track(seg, WaveTrack, 0x80000000, 0); expect_track(seg, SegmentTriggerTrack, 0x3, 2); /* groups 1+2 combined index */ expect_track(seg, SeqTrack, 0x3, 3); /* groups 1+2 combined index */ expect_track(seg, TempoTrack, 0x7, 4); /* groups 1+2+3 combined index */ expect_track(seg, TempoTrack, 0xffffffff, 4); /* all groups combined index */ - _expect_track(seg, NULL, "", 0xffffffff, DMUS_SEG_ANYTRACK, TRUE); + _expect_track(seg, NULL, "", 0xffffffff, DMUS_SEG_ANYTRACK, TRUE, __FILE__, __LINE__); /* Use the GUID in GetTrack */ hr = IDirectMusicSegment8_GetTrack(seg, &CLSID_DirectMusicLyricsTrack, 0, 0, &track); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5081
update: rebased and reorder commits. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5081#note_61489
Rémi Bernon (@rbernon) commented about dlls/dmime/midi.c:
+ else + { + if ((status & 0xf0) != 0xc0 && (status & 0xf0) != 0xd0) + if ((hr = stream_read_at_most(stream, &byte, 1, bytes_left)) != S_OK) return hr; + FIXME("MIDI event status %#02x, time +%lu, not supported\n", status, delta_time); + } + + return S_OK; +} + HRESULT midi_parser_next_track(struct midi_parser *parser, IDirectMusicTrack **out_track, MUSIC_TIME *out_length) { - TRACE("(%p, %p, %p): stub\n", parser, out_track, out_length); + WORD i = 0; + TRACE("(%p, %p): stub\n", parser, out_length); + while (TRUE)
TRACE("(%p, %p): stub\n", parser, out_length);
for (;;)
I believe wine more often uses `for (;;)` for unbounded loops. A bit of spacing around traces would make it more readable too. Or maybe alternatively it could be: ```suggestion:-1+0 TRACE("(%p, %p): stub\n", parser, out_length); for (i = 0;; i++) ``` Saving you the increment in the end. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5081#note_61499
Rémi Bernon (@rbernon) commented about dlls/dmime/midi.c:
HRESULT midi_parser_next_track(struct midi_parser *parser, IDirectMusicTrack **out_track, MUSIC_TIME *out_length) { - TRACE("(%p, %p, %p): stub\n", parser, out_track, out_length); + WORD i = 0; + TRACE("(%p, %p): stub\n", parser, out_length); + while (TRUE) + { + HRESULT hr; + BYTE magic[4] = {0}, last_status = 0; + DWORD length_be; + ULONG length; + ULONG read = 0; + + TRACE("Start parsing track %u\n", i); + if ((hr = IStream_Read(parser->stream, magic, sizeof(magic), &read)) != S_OK) return hr; + if (read == 0) break; That check seems unnecessary as magic is zero-initialized? Or maybe better leave it uninitialized and do:
```suggestion:-0+0 if (read < sizeof(magic)) break; ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5081#note_61500
Rémi Bernon (@rbernon) commented about dlls/dmime/midi.c:
+ TRACE("(%p, %p): stub\n", parser, out_length); + while (TRUE) + { + HRESULT hr; + BYTE magic[4] = {0}, last_status = 0; + DWORD length_be; + ULONG length; + ULONG read = 0; + + TRACE("Start parsing track %u\n", i); + if ((hr = IStream_Read(parser->stream, magic, sizeof(magic), &read)) != S_OK) return hr; + if (read == 0) break; + if (memcmp(magic, "MTrk", 4) != 0) break; + + if ((hr = IStream_Read(parser->stream, &length_be, sizeof(length_be), NULL)) != S_OK) break; + length = GET_BE_DWORD(length_be); Should check the read size there maybe?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5081#note_61501
Rémi Bernon (@rbernon) commented about dlls/dmime/midi.c:
+ if ((hr = IStream_Read(parser->stream, magic, sizeof(magic), &read)) != S_OK) return hr; + if (read == 0) break; + if (memcmp(magic, "MTrk", 4) != 0) break; + + if ((hr = IStream_Read(parser->stream, &length_be, sizeof(length_be), NULL)) != S_OK) break; + length = GET_BE_DWORD(length_be); + TRACE("Track %u, length %lu bytes\n", i, length); + + while ((hr = read_midi_event(parser->stream, &last_status, &length)) == S_OK) + ; + + if (FAILED(hr)) return hr; + TRACE("End of track %u\n", i); + i += 1; + } + TRACE("End of file\n");
TRACE("End of file\n");
Nit: let it breathe a bit :) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5081#note_61502
Rémi Bernon (@rbernon) commented about dlls/dmime/midi.c:
+ HRESULT hr = S_OK; + + *out = 0; + do + { + hr = stream_read_at_most(stream, &byte, 1, bytes_left); + if (hr != S_OK) return hr; + *out = (*out << 7) | (byte & 0x7f); + } while (byte & 0x80); + return S_OK; +} + +static HRESULT read_midi_event(IStream *stream, BYTE *last_status, ULONG *bytes_left) +{ + BYTE byte; + BYTE status, meta_event_type; Nit: Fwiw Wine usually prefers grouping variable declarations of the same type.
```suggestion:-1+0 BYTE byte, status, meta_event_type; ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5081#note_61503
Rémi Bernon (@rbernon) commented about dlls/dmime/midi.c:
+{ + BYTE byte; + BYTE status, meta_event_type; + DWORD length; + LARGE_INTEGER offset; + HRESULT hr = S_OK; + DWORD delta_time; + + if ((hr = read_variable_length_number(stream, &delta_time, bytes_left)) != S_OK) return hr; + + if ((hr = stream_read_at_most(stream, &byte, 1, bytes_left)) != S_OK) return hr; + + if (byte & 0x80) + { + status = byte; + *last_status = byte; Nit: in this case as well, it's very common to use combined assignment for variables of the same type.
```suggestion:-1+0 status = *last_status = byte; ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5081#note_61504
Rémi Bernon (@rbernon) commented about dlls/dmime/midi.c:
+ { + if ((hr = read_variable_length_number(stream, &length, bytes_left)) != S_OK) return hr; + length = length << 8 | (byte & 0x7f); + } + else length = byte; + + if (*bytes_left < length) return S_FALSE; + offset.QuadPart = length; + if (FAILED(hr = IStream_Seek(stream, offset, STREAM_SEEK_CUR, NULL))) return hr; + *bytes_left -= length; + FIXME("MIDI sysex event type %#02x, length %lu, time +%lu. not supported\n", status, length, delta_time); + } + else + { + if ((status & 0xf0) != 0xc0 && (status & 0xf0) != 0xd0) + if ((hr = stream_read_at_most(stream, &byte, 1, bytes_left)) != S_OK) return hr; Nit: What about combining the ifs with &&, or bracing the outermost one? I think this looks a bit wobbly.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5081#note_61505
participants (3)
-
Rémi Bernon -
Yuxuan Shui
-
Yuxuan Shui (＠yshui)