[PATCH v9 0/2] 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 -- v9: dmime/tests: Improve error reporting from expect_track. 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/dmime_private.h | 2 +- dlls/dmime/midi.c | 159 ++++++++++++++++++++++++++--- dlls/dmime/segment.c | 16 +-- dlls/dmime/tests/dmime.c | 200 ++++++++++++++++++++++++++++--------- 4 files changed, 303 insertions(+), 74 deletions(-) diff --git a/dlls/dmime/dmime_private.h b/dlls/dmime/dmime_private.h index cdf0810381d..3c6c6fd5d99 100644 --- a/dlls/dmime/dmime_private.h +++ b/dlls/dmime/dmime_private.h @@ -73,7 +73,7 @@ extern HRESULT create_dmwavetrack(REFIID riid, void **ret_iface); /* Create a new MIDI file parser. Note the stream might still be modified even * when this function fails. */ extern HRESULT midi_parser_new(IStream *stream, struct midi_parser **out_parser); -extern HRESULT midi_parser_next_track(struct midi_parser *parser, IDirectMusicTrack **out_track, MUSIC_TIME *out_length); +extern HRESULT midi_parser_parse(struct midi_parser *parser, MUSIC_TIME *out_length); extern void midi_parser_destroy(struct midi_parser *parser); extern void set_audiopath_perf_pointer(IDirectMusicAudioPath*,IDirectMusicPerformance8*); diff --git a/dlls/dmime/midi.c b/dlls/dmime/midi.c index a7e57947c6f..d4ee82f724f 100644 --- a/dlls/dmime/midi.c +++ b/dlls/dmime/midi.c @@ -34,10 +34,153 @@ struct midi_parser IStream *stream; }; -HRESULT midi_parser_next_track(struct midi_parser *parser, IDirectMusicTrack **out_track, MUSIC_TIME *out_length) +enum meta_event_type { - TRACE("(%p, %p, %p): stub\n", parser, out_track, out_length); - return S_FALSE; + 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 read_variable_length_number(IStream *stream, DWORD *out) +{ + BYTE byte; + HRESULT hr = S_OK; + + *out = 0; + do + { + hr = IStream_Read(stream, &byte, 1, NULL); + if (hr != S_OK) break; + + *out = (*out << 7) | (byte & 0x7f); + } while (byte & 0x80); + return hr; +} + +static HRESULT read_midi_event(IStream *stream, BYTE *last_status) +{ + BYTE byte; + BYTE status, meta_event_type; + DWORD length, read; + LARGE_INTEGER offset; + HRESULT hr = S_OK; + DWORD delta_time; + + hr = read_variable_length_number(stream, &delta_time); + if (hr != S_OK) return hr; + + hr = IStream_Read(stream, &byte, 1, &read); + if (hr != S_OK) return hr; + if (read == 0) return S_FALSE; + + if (byte & 0x80) + { + status = byte; + *last_status = byte; + } + else + { + /* MIDI running status, the byte we just read is not the + * status byte, so move back 1 byte. */ + status = *last_status; + offset.QuadPart = -1; + hr = IStream_Seek(stream, offset, STREAM_SEEK_CUR, NULL); + if (FAILED(hr)) return hr; + } + + if (status == 0xff) + { + hr = IStream_Read(stream, &byte, 1, NULL); + if (hr != S_OK) return hr; + meta_event_type = byte; + + hr = read_variable_length_number(stream, &length); + if (hr != S_OK) return hr; + + switch (meta_event_type) + { + case MIDI_META_END_OF_TRACK: + if (length != 0) + { + ERR("Invalid MIDI meta event length %lu for end of track event.\n", length); + return E_FAIL; + } + return S_FALSE; + default: + 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); + } + TRACE("MIDI meta event type %#02x, length %lu, time +%lu\n", meta_event_type, length, delta_time); + } + else if (status == 0xf0 || status == 0xf7) + { + if ((hr = read_variable_length_number(stream, &length)) != S_OK) return hr; + + offset.QuadPart = length; + if (FAILED(hr = IStream_Seek(stream, offset, STREAM_SEEK_CUR, NULL))) return hr; + 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) offset.QuadPart = 1; + else offset.QuadPart = 2; + if (FAILED(hr = IStream_Seek(stream, offset, STREAM_SEEK_CUR, NULL))) return hr; + FIXME("MIDI event status %#02x, length: %lld, time +%lu, not supported\n", status, offset.QuadPart, delta_time); + } + + return S_OK; +} + +HRESULT midi_parser_parse(struct midi_parser *parser, MUSIC_TIME *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; + 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) + { + TRACE("End of file\n"); + break; + } + if (memcmp(magic, "MTrk", 4) != 0) + { + WARN("Invalid track magic number\n"); + return DMUS_E_UNSUPPORTED_STREAM; + } + + if ((hr = IStream_Read(parser->stream, &length, sizeof(length), NULL)) != S_OK) return E_FAIL; + length = GET_BE_DWORD(length); + TRACE("Track %u, length %lu bytes\n", i, length); + + while ((hr = read_midi_event(parser->stream, &last_status)) == S_OK) + ; + + if (FAILED(hr)) return hr; + TRACE("End of track %u\n", i); + i += 1; + } + return S_OK; } HRESULT midi_parser_new(IStream *stream, struct midi_parser **out_parser) @@ -64,16 +207,6 @@ HRESULT midi_parser_new(IStream *stream, struct midi_parser **out_parser) 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/segment.c b/dlls/dmime/segment.c index 82e06f4537d..2573bfec767 100644 --- a/dlls/dmime/segment.c +++ b/dlls/dmime/segment.c @@ -791,8 +791,6 @@ static inline struct segment *impl_from_IPersistStream(IPersistStream *iface) static HRESULT WINAPI segment_persist_stream_Load(IPersistStream *iface, IStream *stream) { struct segment *This = impl_from_IPersistStream(iface); - IDirectMusicTrack *track; - MUSIC_TIME length; struct chunk_entry chunk = {0}; struct midi_parser *midi_parser; HRESULT hr; @@ -812,15 +810,7 @@ static HRESULT WINAPI segment_persist_stream_Load(IPersistStream *iface, IStream case mmioFOURCC('M','T','h','d'): hr = midi_parser_new(stream, &midi_parser); if (FAILED(hr)) break; - This->header.mtLength = 0; - while ((hr = midi_parser_next_track(midi_parser, &track, &length)) == S_OK) - { - hr = segment_append_track(This, track, 1, 0); - IDirectMusicTrack_Release(track); - if (FAILED(hr)) break; - if (length > This->header.mtLength) - This->header.mtLength = length; - } + hr = midi_parser_parse(midi_parser, &This->header.mtLength); midi_parser_destroy(midi_parser); break; @@ -844,7 +834,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); diff --git a/dlls/dmime/tests/dmime.c b/dlls/dmime/tests/dmime.c index eaeff2e86bc..7d5dcfc481f 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,73 @@ 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); IDirectMusicLoader8_Release(loader); } @@ -1615,40 +1753,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 7d5dcfc481f..f1f52606802 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) { @@ -1780,15 +1780,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
Rémi Bernon (@rbernon) commented about dlls/dmime/segment.c:
case mmioFOURCC('M','T','h','d'): hr = midi_parser_new(stream, &midi_parser); if (FAILED(hr)) break; - This->header.mtLength = 0; - while ((hr = midi_parser_next_track(midi_parser, &track, &length)) == S_OK) - { - hr = segment_append_track(This, track, 1, 0); - IDirectMusicTrack_Release(track); - if (FAILED(hr)) break; - if (length > This->header.mtLength) - This->header.mtLength = length; - } + hr = midi_parser_parse(midi_parser, &This->header.mtLength); midi_parser_destroy(midi_parser);
I don't understand why you're removing this now. Yes, probably it was introduced too early because nothing creates tracks, but now it's here and I expect that you will need a way to pass the created tracks to the segment? So let's keep it so we don't have to reintroduce it later. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5081#note_61337
Rémi Bernon (@rbernon) commented about dlls/dmime/midi.c:
+ TRACE("Start parsing track %u\n", i); + if ((hr = IStream_Read(parser->stream, magic, sizeof(magic), &read)) != S_OK) return hr; + if (read == 0) + { + TRACE("End of file\n"); + break; + } + if (memcmp(magic, "MTrk", 4) != 0) + { + WARN("Invalid track magic number\n"); + return DMUS_E_UNSUPPORTED_STREAM; + } + + if ((hr = IStream_Read(parser->stream, &length, sizeof(length), NULL)) != S_OK) return E_FAIL; + length = GET_BE_DWORD(length); + TRACE("Track %u, length %lu bytes\n", i, length); What about allocating a `length` sized buffer and read everything at once? Would probably make the parsing a bit simpler wrt backtracking.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5081#note_61339
Rémi Bernon (@rbernon) commented about dlls/dmime/midi.c:
+ if (hr != S_OK) return hr; + if (read == 0) return S_FALSE; + + if (byte & 0x80) + { + status = byte; + *last_status = byte; + } + else + { + /* MIDI running status, the byte we just read is not the + * status byte, so move back 1 byte. */ + status = *last_status; + offset.QuadPart = -1; + hr = IStream_Seek(stream, offset, STREAM_SEEK_CUR, NULL); + if (FAILED(hr)) return hr; I don't think backtracking is a good pattern, especially if you consider that you don't know what the stream will have to do for that.
You're going to read or skip that byte in every case anyway, can we just keep it? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5081#note_61338
Rémi Bernon (@rbernon) commented about dlls/dmime/midi.c:
+ hr = IStream_Read(stream, &byte, 1, NULL); + if (hr != S_OK) return hr; + meta_event_type = byte; + + hr = read_variable_length_number(stream, &length); + if (hr != S_OK) return hr; + + switch (meta_event_type) + { + case MIDI_META_END_OF_TRACK: + if (length != 0) + { + ERR("Invalid MIDI meta event length %lu for end of track event.\n", length); + return E_FAIL; + } return S_FALSE; I don't see any evidence in the tests that the EOT event makes it stop parsing.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5081#note_61340
On Thu Feb 15 16:51:38 2024 +0000, Rémi Bernon wrote:
I don't understand why you're removing this now. Yes, probably it was introduced too early because nothing creates tracks, but now it's here and I expect that you will need a way to pass the created tracks to the segment? So let's keep it so we don't have to reintroduce it later. The interface for getting tracks will be different down the line.
The reason for the old interface is I thought we will be getting 1 seqtrack for each track in MIDI, and it turns out this isn't the case. Instead there is one track of each kind, and keep using the `next_track` interface becomes really awkward. So I removed it here. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5081#note_61343
On Thu Feb 15 16:51:39 2024 +0000, Rémi Bernon wrote:
I don't think backtracking is a good pattern, especially if you consider that you don't know what the stream will have to do for that. You're going to read or skip that byte in every case anyway, can we just keep it? Doing this makes the logic simpler, I will change it.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5081#note_61345
On Thu Feb 15 16:51:39 2024 +0000, Rémi Bernon wrote:
What about allocating a `length` sized buffer and read everything at once? Would probably make the parsing a bit simpler wrt backtracking. This is an easily avoidable allocation. I will change the backtracking.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5081#note_61346
On Thu Feb 15 17:16:41 2024 +0000, Yuxuan Shui wrote:
The interface for getting tracks will be different down the line. The reason for the old interface is I thought we will be getting 1 seqtrack for each track in MIDI, and it turns out this isn't the case. Instead there is one track of each kind, and keep using the `next_track` interface becomes really awkward. So I removed it here. Okay, then let's change that interface only when it'll be needed.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5081#note_61349
participants (3)
-
Rémi Bernon -
Yuxuan Shui
-
Yuxuan Shui (＠yshui)