There are still some issues I need to fix, mainly around timing conversion between MIDI and dmusic. Right now MIDI files seem to be cut off before the end is reached.
Please have a look at the general approach in the meantime, I need to know if this is the right way to do this or not.
-- v9: dmusic: Don't stop instrument downloading early on failure. dmime: Create a band track for MIDI segments. dmband: Move band.c to dmusic dmband: Implement getting/setting GUID_BandParam on band tracks. dmime/test: Add test for getting band track from midi file. dmime: Implement IDirectMusicTrack_Play for MIDI tracks. dmime: Implement getting/setting TempoParam for MIDI tracks. dmime: Parse MIDI files.
From: Yuxuan Shui yshui@codeweavers.com
--- dlls/dmime/tests/dmime.c | 20 ++++++++++++++++++++ dlls/dmime/tests/resource.rc | 2 ++ dlls/dmime/tests/test.mid | Bin 0 -> 163 bytes 3 files changed, 22 insertions(+) create mode 100644 dlls/dmime/tests/test.mid
diff --git a/dlls/dmime/tests/dmime.c b/dlls/dmime/tests/dmime.c index 26fdf3ef2a5..d23f7b66087 100644 --- a/dlls/dmime/tests/dmime.c +++ b/dlls/dmime/tests/dmime.c @@ -1554,6 +1554,25 @@ static void test_segment(void) while (IDirectMusicSegment_Release(dms)); }
+static void test_midi(void) +{ + IDirectMusicSegment8 *segment = NULL; + IDirectMusicLoader8 *loader; + WCHAR test_mid[MAX_PATH]; + HRESULT hr; + + load_resource(L"test.mid", test_mid); + + hr = CoCreateInstance(&CLSID_DirectMusicLoader, NULL, CLSCTX_INPROC_SERVER, + &IID_IDirectMusicLoader8, (void **)&loader); + ok(hr == S_OK, "got %#lx\n", hr); + hr = IDirectMusicLoader8_LoadObjectFromFile(loader, &CLSID_DirectMusicSegment, + &IID_IDirectMusicSegment, test_mid, (void **)&segment); + todo_wine ok(hr == S_OK, "got %#lx\n", hr); + IDirectMusicLoader8_Release(loader); + if (segment) IDirectMusicSegment8_Release(segment); +} + static void _add_track(IDirectMusicSegment8 *seg, REFCLSID class, const char *name, DWORD group) { IDirectMusicTrack *track; @@ -4706,6 +4725,7 @@ START_TEST(dmime) test_audiopathconfig(); test_graph(); test_segment(); + test_midi(); test_gettrack(); test_segment_param(); test_track(); diff --git a/dlls/dmime/tests/resource.rc b/dlls/dmime/tests/resource.rc index d49b647b934..e6a06ae105b 100644 --- a/dlls/dmime/tests/resource.rc +++ b/dlls/dmime/tests/resource.rc @@ -21,3 +21,5 @@ /* ffmpeg -f lavfi -i "sine=frequency=600" -t 0.1 -ar 44100 -f wav -acodec pcm_u8 test.wav */ /* @makedep: test.wav */ test.wav RCDATA test.wav +/* @makedep: test.mid */ +test.mid RCDATA test.mid diff --git a/dlls/dmime/tests/test.mid b/dlls/dmime/tests/test.mid new file mode 100644 index 0000000000000000000000000000000000000000..7a46eb85d11d61ba23528912e5ce9c6fd1a0e533 GIT binary patch literal 163 zcmeYb$w*;fU|?flWME=^;2Tnu4dmrA{AXqj$V|-3XZRn%!onoM!SFwl2`KSDkePj< z0>g$%1_q$6RG>Z{hLn1S)Ov;k3=A9CQy91$7@`>%Vu30Z85tNR*aS4n+b}SqQw+`l b?e;bd49*OU|MeN5mP#O53b71m1V|nLiNz$G
literal 0 HcmV?d00001
From: Yuxuan Shui yshui@codeweavers.com
--- dlls/dmime/Makefile.in | 1 + dlls/dmime/midi.c | 275 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 276 insertions(+) create mode 100644 dlls/dmime/midi.c
diff --git a/dlls/dmime/Makefile.in b/dlls/dmime/Makefile.in index cf041ef2b27..f8f622f8f3d 100644 --- a/dlls/dmime/Makefile.in +++ b/dlls/dmime/Makefile.in @@ -10,6 +10,7 @@ SOURCES = \ graph.c \ lyricstrack.c \ markertrack.c \ + midi.c \ paramcontroltrack.c \ performance.c \ segment.c \ diff --git a/dlls/dmime/midi.c b/dlls/dmime/midi.c new file mode 100644 index 00000000000..e515ab12568 --- /dev/null +++ b/dlls/dmime/midi.c @@ -0,0 +1,275 @@ +/* + * Copyright (C) 2024 Yuxuan Shui for CodeWeavers + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA + */ + +#include "dmime_private.h" + +WINE_DEFAULT_DEBUG_CHANNEL(dmime); + +struct midi_sequence_track +{ + IDirectMusicTrack8 IDirectMusicTrack8_iface; + struct dmobject dmobj; /* IPersistStream only */ + LONG ref; +}; + +static inline struct midi_sequence_track *impl_from_IDirectMusicTrack8(IDirectMusicTrack8 *iface) +{ + return CONTAINING_RECORD(iface, struct midi_sequence_track, IDirectMusicTrack8_iface); +} + +static HRESULT WINAPI midi_sequence_track_QueryInterface(IDirectMusicTrack8 *iface, REFIID riid, void **ret_iface) +{ + struct midi_sequence_track *This = impl_from_IDirectMusicTrack8(iface); + + TRACE("(%p, %s, %p)\n", This, debugstr_dmguid(riid), ret_iface); + + if (IsEqualIID(riid, &IID_IUnknown) || IsEqualIID(riid, &IID_IDirectMusicTrack) || + IsEqualIID(riid, &IID_IDirectMusicTrack8)) + *ret_iface = &This->IDirectMusicTrack8_iface; + else if (IsEqualIID(riid, &IID_IPersistStream)) *ret_iface = &This->dmobj.IPersistStream_iface; + else + { + *ret_iface = NULL; + WARN("(%p, %s, %p): not found\n", This, debugstr_dmguid(riid), ret_iface); + return E_NOINTERFACE; + } + + IDirectMusicTrack8_AddRef(iface); + return S_OK; +} + +static ULONG WINAPI midi_sequence_track_AddRef(IDirectMusicTrack8 *iface) +{ + struct midi_sequence_track *This = impl_from_IDirectMusicTrack8(iface); + ULONG ref = InterlockedIncrement(&This->ref); + + TRACE("(%p): %lu\n", This, ref); + + return ref; +} + +static ULONG WINAPI midi_sequence_track_Release(IDirectMusicTrack8 *iface) +{ + struct midi_sequence_track *This = impl_from_IDirectMusicTrack8(iface); + ULONG ref = InterlockedDecrement(&This->ref); + + TRACE("(%p): %lu\n", This, ref); + + if (!ref) free(This); + + return ref; +} + +static HRESULT WINAPI midi_sequence_track_Init(IDirectMusicTrack8 *iface, IDirectMusicSegment *pSegment) +{ + struct midi_sequence_track *This = impl_from_IDirectMusicTrack8(iface); + FIXME("(%p, %p): stub\n", This, pSegment); + return S_OK; +} + +static HRESULT WINAPI midi_sequence_track_InitPlay(IDirectMusicTrack8 *iface, + IDirectMusicSegmentState *pSegmentState, IDirectMusicPerformance *pPerformance, + void **ppStateData, DWORD dwVirtualTrack8ID, DWORD dwFlags) +{ + struct midi_sequence_track *This = impl_from_IDirectMusicTrack8(iface); + FIXME("(%p, %p, %p, %p, %ld, %ld): stub\n", This, pSegmentState, pPerformance, ppStateData, + dwVirtualTrack8ID, dwFlags); + return S_OK; +} + +static HRESULT WINAPI midi_sequence_track_EndPlay(IDirectMusicTrack8 *iface, void *pStateData) +{ + struct midi_sequence_track *This = impl_from_IDirectMusicTrack8(iface); + FIXME("(%p, %p): stub\n", This, pStateData); + return S_OK; +} + +static HRESULT WINAPI midi_sequence_track_Play(IDirectMusicTrack8 *iface, void *state_data, + MUSIC_TIME start_time, MUSIC_TIME end_time, MUSIC_TIME time_offset, DWORD segment_flags, + IDirectMusicPerformance *performance, IDirectMusicSegmentState *segment_state, DWORD track_id) +{ + struct midi_sequence_track *This = impl_from_IDirectMusicTrack8(iface); + + TRACE("(%p, %p, %ld, %ld, %ld, %#lx, %p, %p, %ld)\n", This, state_data, start_time, end_time, + time_offset, segment_flags, performance, segment_state, track_id); + + return E_NOTIMPL; +} + +static HRESULT WINAPI midi_sequence_track_GetParam(IDirectMusicTrack8 *iface, REFGUID type, + MUSIC_TIME time, MUSIC_TIME *next, void *param) +{ + struct midi_sequence_track *This = impl_from_IDirectMusicTrack8(iface); + + TRACE("(%p, %s, %ld, %p, %p): method not implemented\n", This, debugstr_dmguid(type), time, next, param); + return E_NOTIMPL; +} + +static HRESULT WINAPI midi_sequence_track_SetParam(IDirectMusicTrack8 *iface, REFGUID type, + MUSIC_TIME time, void *param) +{ + struct midi_sequence_track *This = impl_from_IDirectMusicTrack8(iface); + + TRACE("(%p, %s, %ld, %p): method not implemented\n", This, debugstr_dmguid(type), time, param); + return E_NOTIMPL; +} + +static HRESULT WINAPI midi_sequence_track_IsParamSupported(IDirectMusicTrack8 *iface, REFGUID type) +{ + struct midi_sequence_track *This = impl_from_IDirectMusicTrack8(iface); + + TRACE("(%p, %s): method not implemented\n", This, debugstr_dmguid(type)); + return E_NOTIMPL; +} + +static HRESULT WINAPI midi_sequence_track_AddNotificationType(IDirectMusicTrack8 *iface, REFGUID notiftype) +{ + struct midi_sequence_track *This = impl_from_IDirectMusicTrack8(iface); + + TRACE("(%p, %s): method not implemented\n", This, debugstr_dmguid(notiftype)); + return E_NOTIMPL; +} + +static HRESULT WINAPI midi_sequence_track_RemoveNotificationType(IDirectMusicTrack8 *iface, REFGUID notiftype) +{ + struct midi_sequence_track *This = impl_from_IDirectMusicTrack8(iface); + + TRACE("(%p, %s): method not implemented\n", This, debugstr_dmguid(notiftype)); + return E_NOTIMPL; +} + +static HRESULT WINAPI midi_sequence_track_Clone(IDirectMusicTrack8 *iface, MUSIC_TIME mtStart, + MUSIC_TIME mtEnd, IDirectMusicTrack **ppTrack) +{ + struct midi_sequence_track *This = impl_from_IDirectMusicTrack8(iface); + FIXME("(%p, %ld, %ld, %p): stub\n", This, mtStart, mtEnd, ppTrack); + return S_OK; +} + +static HRESULT WINAPI midi_sequence_track_PlayEx(IDirectMusicTrack8 *iface, void *pStateData, + REFERENCE_TIME rtStart, REFERENCE_TIME rtEnd, REFERENCE_TIME rtOffset, DWORD dwFlags, + IDirectMusicPerformance *pPerf, IDirectMusicSegmentState *pSegSt, DWORD dwVirtualID) +{ + struct midi_sequence_track *This = impl_from_IDirectMusicTrack8(iface); + FIXME("(%p, %p, 0x%s, 0x%s, 0x%s, %ld, %p, %p, %ld): stub\n", This, pStateData, + wine_dbgstr_longlong(rtStart), wine_dbgstr_longlong(rtEnd), + wine_dbgstr_longlong(rtOffset), dwFlags, pPerf, pSegSt, dwVirtualID); + return S_OK; +} + +static HRESULT WINAPI midi_sequence_track_GetParamEx(IDirectMusicTrack8 *iface, REFGUID type, + REFERENCE_TIME time, REFERENCE_TIME *next, void *param, void *state, DWORD flags) +{ + struct midi_sequence_track *This = impl_from_IDirectMusicTrack8(iface); + + TRACE("(%p, %s, %s, %p, %p, %p, %lx): method not implemented\n", This, debugstr_dmguid(type), + wine_dbgstr_longlong(time), next, param, state, flags); + return E_NOTIMPL; +} + +static HRESULT WINAPI midi_sequence_track_SetParamEx(IDirectMusicTrack8 *iface, REFGUID type, + REFERENCE_TIME time, void *param, void *state, DWORD flags) +{ + struct midi_sequence_track *This = impl_from_IDirectMusicTrack8(iface); + + TRACE("(%p, %s, %s, %p, %p, %lx): method not implemented\n", This, debugstr_dmguid(type), + wine_dbgstr_longlong(time), param, state, flags); + return E_NOTIMPL; +} + +static HRESULT WINAPI midi_sequence_track_Compose(IDirectMusicTrack8 *iface, IUnknown *context, + DWORD trackgroup, IDirectMusicTrack **track) +{ + struct midi_sequence_track *This = impl_from_IDirectMusicTrack8(iface); + + TRACE("(%p, %p, %ld, %p): method not implemented\n", This, context, trackgroup, track); + return E_NOTIMPL; +} + +static HRESULT WINAPI midi_sequence_track_Join(IDirectMusicTrack8 *iface, IDirectMusicTrack *newtrack, + MUSIC_TIME join, IUnknown *context, DWORD trackgroup, IDirectMusicTrack **resulttrack) +{ + struct midi_sequence_track *This = impl_from_IDirectMusicTrack8(iface); + TRACE("(%p, %p, %ld, %p, %ld, %p): method not implemented\n", This, newtrack, join, context, + trackgroup, resulttrack); + return E_NOTIMPL; +} +struct IDirectMusicTrack8Vtbl midi_sequence_track_vtbl = +{ + midi_sequence_track_QueryInterface, + midi_sequence_track_AddRef, + midi_sequence_track_Release, + midi_sequence_track_Init, + midi_sequence_track_InitPlay, + midi_sequence_track_EndPlay, + midi_sequence_track_Play, + midi_sequence_track_GetParam, + midi_sequence_track_SetParam, + midi_sequence_track_IsParamSupported, + midi_sequence_track_AddNotificationType, + midi_sequence_track_RemoveNotificationType, + midi_sequence_track_Clone, + midi_sequence_track_PlayEx, + midi_sequence_track_GetParamEx, + midi_sequence_track_SetParamEx, + midi_sequence_track_Compose, + midi_sequence_track_Join, +}; + +static inline struct midi_sequence_track *impl_from_IPersistStream(IPersistStream *iface) +{ + return CONTAINING_RECORD(iface, struct midi_sequence_track, dmobj.IPersistStream_iface); +} + +static HRESULT WINAPI track_IPersistStream_Load(IPersistStream *iface, IStream *stream) +{ + struct midi_sequence_track *This = impl_from_IPersistStream(iface); + + TRACE("(%p, %p): stub\n", This, stream); + return S_OK; +} + +static const IPersistStreamVtbl midi_sequence_track_persiststream_vtbl = +{ + dmobj_IPersistStream_QueryInterface, + dmobj_IPersistStream_AddRef, + dmobj_IPersistStream_Release, + dmobj_IPersistStream_GetClassID, + unimpl_IPersistStream_IsDirty, + track_IPersistStream_Load, + unimpl_IPersistStream_Save, + unimpl_IPersistStream_GetSizeMax, +}; + +HRESULT create_midiseqtrack(REFIID riid, void **ppobj) +{ + struct midi_sequence_track *track; + HRESULT hr; + + *ppobj = NULL; + if (!(track = calloc(1, sizeof(*track)))) return E_OUTOFMEMORY; + track->IDirectMusicTrack8_iface.lpVtbl = &midi_sequence_track_vtbl; + track->ref = 1; + dmobject_init(&track->dmobj, &CLSID_DirectMusicSeqTrack, (IUnknown *)&track->IDirectMusicTrack8_iface); + track->dmobj.IPersistStream_iface.lpVtbl = &midi_sequence_track_persiststream_vtbl; + + hr = IDirectMusicTrack8_QueryInterface(&track->IDirectMusicTrack8_iface, riid, ppobj); + IDirectMusicTrack8_Release(&track->IDirectMusicTrack8_iface); + + return hr; +}
From: Yuxuan Shui yshui@codeweavers.com
--- dlls/dmime/dmime_private.h | 15 ++ dlls/dmime/midi.c | 390 ++++++++++++++++++++++++++++++++++++- dlls/dmime/segment.c | 25 ++- dlls/dmime/tests/dmime.c | 2 +- 4 files changed, 422 insertions(+), 10 deletions(-)
diff --git a/dlls/dmime/dmime_private.h b/dlls/dmime/dmime_private.h index 96ccd5daf8b..e7624456abd 100644 --- a/dlls/dmime/dmime_private.h +++ b/dlls/dmime/dmime_private.h @@ -31,6 +31,7 @@ #include "winnt.h" #include "wingdi.h" #include "winuser.h" +#include "winternl.h"
#include "wine/debug.h" #include "wine/list.h" @@ -48,6 +49,7 @@ * Interfaces */ typedef struct IDirectMusicAudioPathImpl IDirectMusicAudioPathImpl; +struct midi_parser;
/***************************************************************************** * ClassFactory @@ -68,6 +70,11 @@ extern HRESULT create_dmsysextrack(REFIID riid, void **ret_iface); extern HRESULT create_dmtempotrack(REFIID riid, void **ret_iface); extern HRESULT create_dmtimesigtrack(REFIID riid, void **ret_iface); 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 void midi_parser_destroy(struct midi_parser *parser);
extern void set_audiopath_perf_pointer(IDirectMusicAudioPath*,IDirectMusicPerformance8*); extern void set_audiopath_dsound_buffer(IDirectMusicAudioPath*,IDirectSoundBuffer*); @@ -111,4 +118,12 @@ typedef struct _DMUS_PRIVATE_TEMPO_PLAY_STATE { * Misc. */
+#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 + #endif /* __WINE_DMIME_PRIVATE_H */ diff --git a/dlls/dmime/midi.c b/dlls/dmime/midi.c index e515ab12568..7d6eed3dabc 100644 --- a/dlls/dmime/midi.c +++ b/dlls/dmime/midi.c @@ -20,11 +20,73 @@
WINE_DEFAULT_DEBUG_CHANNEL(dmime);
+union event_data +{ + struct + { + BYTE byte[5]; + } fixed; + UINT integer; + struct + { + SIZE_T size; + BYTE *data; + } variable; +}; + +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, +}; + +struct event +{ + struct list entry; + DWORD deltaTime; + BYTE status; + /* type of meta events. */ + enum meta_event_type type; + union event_data data; +}; + struct midi_sequence_track { IDirectMusicTrack8 IDirectMusicTrack8_iface; struct dmobject dmobj; /* IPersistStream only */ LONG ref; + + /* MIDI deltaTime can be either tempo dependent or not. If + * ticks_per_quarter_note is 0, then ticks_per_second should be used. + * Converting MIDI time to DirectMusic MUSIC_TIME is complicated. MUSIC_TIME + * is number of quarter notes times DMUS_PPQ, this is tempo dependent. If + * MIDI time is tempo dependent as well, then we just need to multiply it by + * a factor (because MIDI can have a different division of quarter notes). + * + * If MIDI time is not tempo dependent however, the conversion would be + * difficult because tempo information can be stored in another track. And + * we need the tempo information to convert MIDI time to MUSIC_TIME. The + * only reasonable way to do this is to report a "fake", fixed tempo, which + * makes MUSIC_TIME and MIDI time the same. + */ + UINT ticks_per_quarter_note; + UINT ticks_per_second; + UINT event_count; + + struct list events; };
static inline struct midi_sequence_track *impl_from_IDirectMusicTrack8(IDirectMusicTrack8 *iface) @@ -32,6 +94,41 @@ static inline struct midi_sequence_track *impl_from_IDirectMusicTrack8(IDirectMu return CONTAINING_RECORD(iface, struct midi_sequence_track, IDirectMusicTrack8_iface); }
+static BOOL is_event_fixed_length(BYTE status, enum meta_event_type type) +{ + if (status == 0xf0 || status == 0xf7) return FALSE; + + if (status != 0xff) return TRUE; + + switch (type) + { + case MIDI_META_SEQUENCE_NUMBER: + case MIDI_META_CHANNEL_PREFIX_ASSIGNMENT: + case MIDI_META_END_OF_TRACK: + case MIDI_META_SET_TEMPO: + case MIDI_META_SMPTE_OFFSET: + case MIDI_META_TIME_SIGNATURE: + case MIDI_META_KEY_SIGNATURE: return TRUE; + + case MIDI_META_TEXT_EVENT: + case MIDI_META_COPYRIGHT_NOTICE: + case MIDI_META_TRACK_NAME: + case MIDI_META_INSTRUMENT_NAME: + case MIDI_META_LYRIC: + case MIDI_META_MARKER: + case MIDI_META_CUE_POINT: + case MIDI_META_SEQUENCER_SPECIFIC: return FALSE; + default: ERR("Unknown meta event type %02x\n", type); return TRUE; + } +} + +static void free_event(struct event *event) +{ + if (event->status == 0xff && !is_event_fixed_length(event->status, event->type)) + free(event->data.variable.data); + free(event); +} + static HRESULT WINAPI midi_sequence_track_QueryInterface(IDirectMusicTrack8 *iface, REFIID riid, void **ret_iface) { struct midi_sequence_track *This = impl_from_IDirectMusicTrack8(iface); @@ -66,11 +163,20 @@ static ULONG WINAPI midi_sequence_track_AddRef(IDirectMusicTrack8 *iface) static ULONG WINAPI midi_sequence_track_Release(IDirectMusicTrack8 *iface) { struct midi_sequence_track *This = impl_from_IDirectMusicTrack8(iface); + struct event *event, *next_event; ULONG ref = InterlockedDecrement(&This->ref);
TRACE("(%p): %lu\n", This, ref);
- if (!ref) free(This); + if (!ref) + { + LIST_FOR_EACH_ENTRY_SAFE(event, next_event, &This->events, struct event, entry) + { + list_remove(&event->entry); + free_event(event); + } + free(This); + }
return ref; } @@ -105,8 +211,8 @@ static HRESULT WINAPI midi_sequence_track_Play(IDirectMusicTrack8 *iface, void * { struct midi_sequence_track *This = impl_from_IDirectMusicTrack8(iface);
- TRACE("(%p, %p, %ld, %ld, %ld, %#lx, %p, %p, %ld)\n", This, state_data, start_time, end_time, - time_offset, segment_flags, performance, segment_state, track_id); + TRACE("(%p, %p, %ld, %ld, %ld, %#lx, %p, %p, %ld): stub\n", This, state_data, start_time, + end_time, time_offset, segment_flags, performance, segment_state, track_id);
return E_NOTIMPL; } @@ -236,12 +342,163 @@ static inline struct midi_sequence_track *impl_from_IPersistStream(IPersistStrea return CONTAINING_RECORD(iface, struct midi_sequence_track, dmobj.IPersistStream_iface); }
-static HRESULT WINAPI track_IPersistStream_Load(IPersistStream *iface, IStream *stream) +/* Like IStream_Read, but accumulates the number of bytes read into + * `total_bytes_read`, instead of storing the bytes read in this + * operation. */ +static inline HRESULT stream_read_accumulate(IStream *stream, void *buffer, ULONG size, ULONG *total_bytes_read) +{ + HRESULT hr; + ULONG bytes_read = 0; + + hr = IStream_Read(stream, (BYTE *)buffer, size, &bytes_read); + if (SUCCEEDED(hr)) *total_bytes_read += bytes_read; + return hr; +} + +static HRESULT read_variable_length_number(IStream *stream, DWORD *out, ULONG *total_bytes_read) +{ + BYTE byte; + HRESULT hr = S_OK; + + *out = 0; + do + { + hr = stream_read_accumulate(stream, &byte, 1, total_bytes_read); + if (hr != S_OK) break; + + *out = (*out << 7) | (byte & 0x7f); + } while (byte & 0x80); + return hr; +} + +static HRESULT read_midi_event(IStream *stream, struct event *event, BYTE last_status, ULONG *total_bytes_read) +{ + BYTE byte; + BYTE status_type; + DWORD length; + HRESULT hr = S_OK; + + hr = read_variable_length_number(stream, &event->deltaTime, total_bytes_read); + if (hr != S_OK) return hr; + + hr = stream_read_accumulate(stream, &byte, 1, total_bytes_read); + if (hr != S_OK) return hr; + + if (byte & 0x80) event->status = byte; + else event->status = last_status; /* MIDI running status */ + + if (event->status == 0xff) + { + hr = stream_read_accumulate(stream, &byte, 1, total_bytes_read); + if (hr != S_OK) return hr; + event->type = byte; + + hr = read_variable_length_number(stream, &length, total_bytes_read); + if (hr != S_OK) return hr; + + if (is_event_fixed_length(event->status, event->type)) + { + if (length > sizeof(event->data.fixed.byte)) + { + WARN("Invalid MIDI meta event length %lu for type %#02x\n", length, event->type); + return E_FAIL; + } + hr = stream_read_accumulate(stream, &event->data.fixed.byte[0], length, total_bytes_read); + if (hr == S_OK) + { + if (event->type == MIDI_META_SET_TEMPO) + { + event->data.integer = (event->data.fixed.byte[0] << 16) | + (event->data.fixed.byte[1] << 8) | event->data.fixed.byte[2]; + if (event->data.integer == 0) + { + WARN("Invalid tempo value 0\n"); + hr = E_FAIL; + } + } + } + } + else + { + event->data.variable.data = malloc(length); + if (!event->data.variable.data) return E_OUTOFMEMORY; + event->data.variable.size = length; + hr = stream_read_accumulate(stream, event->data.variable.data, length, total_bytes_read); + } + TRACE("MIDI meta event type %#02x, length %lu, time +%lu\n", event->type, length, event->deltaTime); + } + else if (event->status == 0xf0 || event->status == 0xf7) + { + hr = read_variable_length_number(stream, &length, total_bytes_read); + if (hr != S_OK) return hr; + + event->data.variable.data = malloc(length); + if (!event->data.variable.data) return E_OUTOFMEMORY; + event->data.variable.size = length; + hr = stream_read_accumulate(stream, event->data.variable.data, length, total_bytes_read); + FIXME("MIDI sysex event type %#02x, length %lu, time +%lu. not " + "supported\n", + event->status, length, event->deltaTime); + } + else + { + status_type = event->status & 0xf0; + hr = stream_read_accumulate(stream, &event->data.fixed.byte[0], 1, total_bytes_read); + if (hr == S_OK && status_type != 0xc0 && status_type != 0xd0) + hr = stream_read_accumulate(stream, &event->data.fixed.byte[1], 1, total_bytes_read); + TRACE("MIDI event status %#02x, data1 %#02x, data2 %#02x, time +%lu\n", event->status, + event->data.fixed.byte[0], event->data.fixed.byte[1], event->deltaTime); + } + + return hr; +} + +static HRESULT WINAPI midi_sequence_track_IPersistStream_Load(IPersistStream *iface, IStream *stream) { struct midi_sequence_track *This = impl_from_IPersistStream(iface); + struct event *event = NULL; + DWORD magic, length; + BYTE last_status = 0; + ULONG bytes_read = 0; + BOOL is_end_of_track = FALSE; + HRESULT hr;
TRACE("(%p, %p): stub\n", This, stream); - return S_OK; + + hr = IStream_Read(stream, &magic, sizeof(magic), NULL); + if (FAILED(hr)) return hr; + if (magic != MAKEFOURCC('M', 'T', 'r', 'k')) + { + WARN("Invalid MIDI track magic number %s\n", debugstr_fourcc(magic)); + return DMUS_E_UNSUPPORTED_STREAM; + } + + hr = IStream_Read(stream, &length, sizeof(length), NULL); + if (FAILED(hr)) return hr; + length = GET_BE_DWORD(length); + TRACE("MIDI track length %lu\n", length); + + while (!is_end_of_track) + { + if (!(event = calloc(1, sizeof(*event)))) return E_OUTOFMEMORY; + + hr = read_midi_event(stream, event, last_status, &bytes_read); + if (hr != S_OK) break; + list_add_tail(&This->events, &event->entry); + This->event_count++; + last_status = event->status; + is_end_of_track = event->status == 0xff && event->type == MIDI_META_END_OF_TRACK; + event = NULL; + } + + if (hr == S_OK && bytes_read != length) + { + WARN("Read %lu bytes, expected %lu\n", bytes_read, length); + hr = E_FAIL; + } + + if (hr != S_OK && event) free_event(event); + return hr; }
static const IPersistStreamVtbl midi_sequence_track_persiststream_vtbl = @@ -251,12 +508,22 @@ static const IPersistStreamVtbl midi_sequence_track_persiststream_vtbl = dmobj_IPersistStream_Release, dmobj_IPersistStream_GetClassID, unimpl_IPersistStream_IsDirty, - track_IPersistStream_Load, + midi_sequence_track_IPersistStream_Load, unimpl_IPersistStream_Save, unimpl_IPersistStream_GetSizeMax, };
-HRESULT create_midiseqtrack(REFIID riid, void **ppobj) +static MUSIC_TIME midi_sequence_track_get_length(IDirectMusicTrack8 *iface) +{ + struct midi_sequence_track *This = impl_from_IDirectMusicTrack8(iface); + struct event *event; + MUSIC_TIME length = 0; + LIST_FOR_EACH_ENTRY(event, &This->events, struct event, entry) + length += event->deltaTime; + return length; +} + +static HRESULT create_midiseqtrack(REFIID riid, void **ppobj, DWORD divison) { struct midi_sequence_track *track; HRESULT hr; @@ -267,9 +534,118 @@ HRESULT create_midiseqtrack(REFIID riid, void **ppobj) track->ref = 1; dmobject_init(&track->dmobj, &CLSID_DirectMusicSeqTrack, (IUnknown *)&track->IDirectMusicTrack8_iface); track->dmobj.IPersistStream_iface.lpVtbl = &midi_sequence_track_persiststream_vtbl; + track->ticks_per_quarter_note = divison; + + list_init(&track->events);
hr = IDirectMusicTrack8_QueryInterface(&track->IDirectMusicTrack8_iface, riid, ppobj); IDirectMusicTrack8_Release(&track->IDirectMusicTrack8_iface);
return hr; } + +struct midi_parser +{ + IStream *stream; + DWORD division; + WORD number_of_tracks, current_track; +}; + +HRESULT midi_parser_next_track(struct midi_parser *parser, IDirectMusicTrack **out_track, MUSIC_TIME *out_length) +{ + IPersistStream *pstream; + HRESULT hr; + MUSIC_TIME midi_length; + + if (parser->current_track >= parser->number_of_tracks) return S_FALSE; + + hr = create_midiseqtrack(&IID_IPersistStream, (void **)&pstream, parser->division); + if (FAILED(hr)) return hr; + + hr = IPersistStream_Load(pstream, parser->stream); + if (hr == S_FALSE) + { + WARN("Early end of stream while read MIDI track\n"); + hr = DMUS_E_INVALIDFILE; + } + if (FAILED(hr)) + { + IPersistStream_Release(pstream); + return hr; + } + + hr = IPersistStream_QueryInterface(pstream, &IID_IDirectMusicTrack, (void **)out_track); + IPersistStream_Release(pstream); + if (FAILED(hr)) return hr; + + midi_length = midi_sequence_track_get_length((IDirectMusicTrack8 *)*out_track); + *out_length = (LONGLONG)midi_length * DMUS_PPQ / parser->division; + parser->current_track++; + return S_OK; +} + +HRESULT midi_parser_new(IStream *stream, struct midi_parser **out_parser) +{ + LARGE_INTEGER offset; + DWORD length; + HRESULT hr; + WORD format, number_of_tracks, division; + struct midi_parser *parser; + + *out_parser = NULL; + + /* Skip over the 'MThd' magic number. */ + offset.QuadPart = 4; + if (FAILED(hr = IStream_Seek(stream, offset, STREAM_SEEK_SET, NULL))) return hr; + + 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; + + number_of_tracks = GET_BE_WORD(number_of_tracks); + if (FAILED(hr = IStream_Read(stream, &division, sizeof(division), NULL))) return hr; + division = GET_BE_WORD(division); + if (division & 0x8000) + { + WARN("SMPTE time division not implemented yet\n"); + return DMUS_E_UNSUPPORTED_STREAM; + } + + TRACE("MIDI format %u, %u tracks, division %u\n", format, number_of_tracks, division); + + parser = calloc(1, sizeof(struct midi_parser)); + if (!parser) return E_OUTOFMEMORY; + parser->stream = stream; + parser->division = division; + parser->number_of_tracks = number_of_tracks; + IStream_AddRef(stream); + *out_parser = parser; + + return hr; +} + +void midi_parser_destroy(struct midi_parser *parser) +{ + IStream_Release(parser->stream); + free(parser); +} diff --git a/dlls/dmime/segment.c b/dlls/dmime/segment.c index b208f89148c..c37e76b91cb 100644 --- a/dlls/dmime/segment.c +++ b/dlls/dmime/segment.c @@ -17,9 +17,16 @@ * License along with this program; if not, write to the Free Software * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA */ - #include "dmime_private.h"
+#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 + WINE_DEFAULT_DEBUG_CHANNEL(dmime);
struct track_entry @@ -791,7 +798,10 @@ 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;
TRACE("(%p, %p): Loading\n", This, stream); @@ -807,7 +817,18 @@ static HRESULT WINAPI segment_persist_stream_Load(IPersistStream *iface, IStream break;
case mmioFOURCC('M','T','h','d'): - FIXME("MIDI file loading not supported\n"); + 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; + } + midi_parser_destroy(midi_parser); break;
case MAKE_IDTYPE(FOURCC_RIFF, mmioFOURCC('W','A','V','E')): diff --git a/dlls/dmime/tests/dmime.c b/dlls/dmime/tests/dmime.c index d23f7b66087..d4be6130ed9 100644 --- a/dlls/dmime/tests/dmime.c +++ b/dlls/dmime/tests/dmime.c @@ -1568,7 +1568,7 @@ static void test_midi(void) ok(hr == S_OK, "got %#lx\n", hr); hr = IDirectMusicLoader8_LoadObjectFromFile(loader, &CLSID_DirectMusicSegment, &IID_IDirectMusicSegment, test_mid, (void **)&segment); - todo_wine ok(hr == S_OK, "got %#lx\n", hr); + ok(hr == S_OK, "got %#lx\n", hr); IDirectMusicLoader8_Release(loader); if (segment) IDirectMusicSegment8_Release(segment); }
From: Yuxuan Shui yshui@codeweavers.com
--- dlls/dmime/midi.c | 73 +++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 68 insertions(+), 5 deletions(-)
diff --git a/dlls/dmime/midi.c b/dlls/dmime/midi.c index 7d6eed3dabc..bb7419f54e4 100644 --- a/dlls/dmime/midi.c +++ b/dlls/dmime/midi.c @@ -85,6 +85,7 @@ struct midi_sequence_track UINT ticks_per_quarter_note; UINT ticks_per_second; UINT event_count; + BOOL has_tempo_events;
struct list events; }; @@ -221,9 +222,41 @@ static HRESULT WINAPI midi_sequence_track_GetParam(IDirectMusicTrack8 *iface, RE MUSIC_TIME time, MUSIC_TIME *next, void *param) { struct midi_sequence_track *This = impl_from_IDirectMusicTrack8(iface); + struct event *event; + ULONG ticks = 0; + MUSIC_TIME music_time; + DMUS_TEMPO_PARAM *tempo = param; + + TRACE("(%p, %s, %ld, %p, %p): semi-stub\n", This, debugstr_dmguid(type), time, next, param); + if (!param) return E_POINTER; + if (!IsEqualGUID(type, &GUID_TempoParam)) return DMUS_E_GET_UNSUPPORTED; + if (!This->has_tempo_events) return DMUS_E_NOT_FOUND; + if (This->ticks_per_quarter_note == 0) + { + /* Return a "fake" tempo to make MIDI time and MUSIC_TIME equivalent. */ + tempo->mtTime = -time; /* set since the start of MUSIC_TIME. */ + if (next) *next = 0; /* always valid. */ + tempo->dblTempo = (double)This->ticks_per_second / DMUS_PPQ; + return S_OK; + }
- TRACE("(%p, %s, %ld, %p, %p): method not implemented\n", This, debugstr_dmguid(type), time, next, param); - return E_NOTIMPL; + if (next) *next = 0; + tempo->dblTempo = 0; + LIST_FOR_EACH_ENTRY(event, &This->events, struct event, entry) + { + ticks += event->deltaTime; + music_time = (ULONGLONG)ticks * DMUS_PPQ / This->ticks_per_quarter_note; + if (event->status == 0xff && event->type == MIDI_META_SET_TEMPO) + { + if (next) *next = music_time - time; + if (music_time > time) break; + tempo->mtTime = music_time - time; + tempo->dblTempo = 6e7 / event->data.integer; + } + } + if (tempo->dblTempo == 0) return DMUS_E_NOT_FOUND; + TRACE("Found tempo %f at time offset %ld\n", tempo->dblTempo, tempo->mtTime); + return S_OK; }
static HRESULT WINAPI midi_sequence_track_SetParam(IDirectMusicTrack8 *iface, REFGUID type, @@ -232,15 +265,42 @@ static HRESULT WINAPI midi_sequence_track_SetParam(IDirectMusicTrack8 *iface, RE struct midi_sequence_track *This = impl_from_IDirectMusicTrack8(iface);
TRACE("(%p, %s, %ld, %p): method not implemented\n", This, debugstr_dmguid(type), time, param); - return E_NOTIMPL; + if (!This->has_tempo_events) return DMUS_E_SET_UNSUPPORTED; + + if (IsEqualGUID(type, &GUID_DisableTempo)) + { + if (!param) return DMUS_E_TYPE_DISABLED; + FIXME("GUID_DisableTempo not handled yet\n"); + return S_OK; + } + if (IsEqualGUID(type, &GUID_EnableTempo)) + { + if (!param) return DMUS_E_TYPE_DISABLED; + FIXME("GUID_EnableTempo not handled yet\n"); + return S_OK; + } + if (IsEqualGUID(type, &GUID_TempoParam)) + { + if (!param) return E_POINTER; + FIXME("GUID_TempoParam not handled yet\n"); + return S_OK; + } + + return DMUS_E_SET_UNSUPPORTED; }
static HRESULT WINAPI midi_sequence_track_IsParamSupported(IDirectMusicTrack8 *iface, REFGUID type) { struct midi_sequence_track *This = impl_from_IDirectMusicTrack8(iface);
- TRACE("(%p, %s): method not implemented\n", This, debugstr_dmguid(type)); - return E_NOTIMPL; + TRACE("(%p, %s): semi-sub\n", This, debugstr_dmguid(type)); + + if (!This->has_tempo_events) return DMUS_E_GET_UNSUPPORTED; + + if (IsEqualGUID(type, &GUID_DisableTempo) || IsEqualGUID(type, &GUID_EnableTempo) || + IsEqualGUID(type, &GUID_TempoParam)) + return S_OK; + return DMUS_E_TYPE_UNSUPPORTED; }
static HRESULT WINAPI midi_sequence_track_AddNotificationType(IDirectMusicTrack8 *iface, REFGUID notiftype) @@ -465,6 +525,7 @@ static HRESULT WINAPI midi_sequence_track_IPersistStream_Load(IPersistStream *if
TRACE("(%p, %p): stub\n", This, stream);
+ This->has_tempo_events = FALSE; hr = IStream_Read(stream, &magic, sizeof(magic), NULL); if (FAILED(hr)) return hr; if (magic != MAKEFOURCC('M', 'T', 'r', 'k')) @@ -488,6 +549,8 @@ static HRESULT WINAPI midi_sequence_track_IPersistStream_Load(IPersistStream *if This->event_count++; last_status = event->status; is_end_of_track = event->status == 0xff && event->type == MIDI_META_END_OF_TRACK; + if (event->status == 0xff && event->type == MIDI_META_SET_TEMPO) + This->has_tempo_events = TRUE; event = NULL; }
From: Yuxuan Shui yshui@codeweavers.com
--- dlls/dmime/midi.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 52 insertions(+), 1 deletion(-)
diff --git a/dlls/dmime/midi.c b/dlls/dmime/midi.c index bb7419f54e4..1df1b9b15fd 100644 --- a/dlls/dmime/midi.c +++ b/dlls/dmime/midi.c @@ -211,11 +211,62 @@ static HRESULT WINAPI midi_sequence_track_Play(IDirectMusicTrack8 *iface, void * IDirectMusicPerformance *performance, IDirectMusicSegmentState *segment_state, DWORD track_id) { struct midi_sequence_track *This = impl_from_IDirectMusicTrack8(iface); + struct list *cursor; + IDirectMusicGraph *graph; + HRESULT hr; + UINT i; + ULONG ticks = 0; + MUSIC_TIME music_time;
TRACE("(%p, %p, %ld, %ld, %ld, %#lx, %p, %p, %ld): stub\n", This, state_data, start_time, end_time, time_offset, segment_flags, performance, segment_state, track_id);
- return E_NOTIMPL; + if (segment_flags) FIXME("segment_flags %#lx not implemented\n", segment_flags); + if (segment_state) FIXME("segment_state %p not implemented\n", segment_state); + + if (FAILED(hr = IDirectMusicPerformance_QueryInterface(performance, &IID_IDirectMusicGraph, (void **)&graph))) + return hr; + + cursor = list_head(&This->events); + for (i = 0; SUCCEEDED(hr) && i < This->event_count; i++, cursor = list_next(&This->events, cursor)) + { + struct event *item = LIST_ENTRY(cursor, struct event, entry); + DMUS_MIDI_PMSG *msg; + + ticks += item->deltaTime; + if (This->ticks_per_quarter_note) + music_time = (ULONGLONG)ticks * DMUS_PPQ / This->ticks_per_quarter_note; + else + /* This is the "fake" tempo case, ticks and MUSIC_TIME should mean + * the same thing. */ + music_time = ticks; + + if (music_time < start_time || music_time >= end_time) continue; + + if (item->status == 0xff || item->status == 0xf0 || item->status == 0xf7) continue; + + if (FAILED(hr = IDirectMusicPerformance_AllocPMsg(performance, sizeof(*msg), (DMUS_PMSG **)&msg))) + break; + + msg->dwFlags = DMUS_PMSGF_MUSICTIME; + msg->mtTime = music_time; + msg->dwPChannel = item->status & 0xf; + msg->dwVirtualTrackID = track_id; + msg->dwType = DMUS_PMSGT_MIDI; + msg->dwGroupID = 1; + msg->bStatus = item->status; + msg->bByte1 = item->data.fixed.byte[0]; + msg->bByte2 = item->data.fixed.byte[1]; + + if (FAILED(hr = IDirectMusicGraph_StampPMsg(graph, (DMUS_PMSG *)msg)) || + FAILED(hr = IDirectMusicPerformance_SendPMsg(performance, (DMUS_PMSG *)msg))) + { + IDirectMusicPerformance_FreePMsg(performance, (DMUS_PMSG *)msg); + break; + } + } + + return SUCCEEDED(hr) ? S_OK : hr; }
static HRESULT WINAPI midi_sequence_track_GetParam(IDirectMusicTrack8 *iface, REFGUID type,
From: Yuxuan Shui yshui@codeweavers.com
Windows adds a "fake" band track to segments loaded from MIDI files, which handles soundfont loading etc. --- dlls/dmime/tests/dmime.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/dlls/dmime/tests/dmime.c b/dlls/dmime/tests/dmime.c index d4be6130ed9..7ff934ce139 100644 --- a/dlls/dmime/tests/dmime.c +++ b/dlls/dmime/tests/dmime.c @@ -1557,6 +1557,7 @@ static void test_segment(void) static void test_midi(void) { IDirectMusicSegment8 *segment = NULL; + IDirectMusicTrack *track = NULL; IDirectMusicLoader8 *loader; WCHAR test_mid[MAX_PATH]; HRESULT hr; @@ -1569,6 +1570,12 @@ static void test_midi(void) hr = IDirectMusicLoader8_LoadObjectFromFile(loader, &CLSID_DirectMusicSegment, &IID_IDirectMusicSegment, test_mid, (void **)&segment); ok(hr == S_OK, "got %#lx\n", hr); + + 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); + IDirectMusicLoader8_Release(loader); if (segment) IDirectMusicSegment8_Release(segment); }
From: Yuxuan Shui yshui@codeweavers.com
--- dlls/dmband/bandtrack.c | 40 ++++++++++++++++++++++++++++++++++---- dlls/dmband/tests/dmband.c | 27 +++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 4 deletions(-)
diff --git a/dlls/dmband/bandtrack.c b/dlls/dmband/bandtrack.c index 4756ed194ef..dbd75a4a299 100644 --- a/dlls/dmband/bandtrack.c +++ b/dlls/dmband/bandtrack.c @@ -198,19 +198,39 @@ static HRESULT WINAPI band_track_Play(IDirectMusicTrack8 *iface, void *state_dat }
static HRESULT WINAPI band_track_GetParam(IDirectMusicTrack8 *iface, REFGUID type, MUSIC_TIME time, - MUSIC_TIME *next, void *param) + MUSIC_TIME *out_next, void *param) { struct band_track *This = impl_from_IDirectMusicTrack8(iface); + struct band_entry *band; + DMUS_BAND_PARAM *bandparam; + MUSIC_TIME prev = -1, next = time;
- TRACE("(%p, %s, %ld, %p, %p)\n", This, debugstr_dmguid(type), time, next, param); + TRACE("(%p, %s, %ld, %p, %p)\n", This, debugstr_dmguid(type), time, out_next, param);
if (!type) return E_POINTER; if (!IsEqualGUID(type, &GUID_BandParam)) return DMUS_E_GET_UNSUPPORTED;
- FIXME("GUID_BandParam not handled yet\n"); + bandparam = param;
+ LIST_FOR_EACH_ENTRY(band, &This->bands, struct band_entry, entry) + { + if (band->head.lBandTimeLogical <= time && band->head.lBandTimeLogical >= prev) + { + bandparam->pBand = band->band; + bandparam->mtTimePhysical = band->head.lBandTimePhysical; + prev = band->head.lBandTimeLogical; + } + else if (band->head.lBandTimeLogical > time && band->head.lBandTimeLogical < next) + next = band->head.lBandTimeLogical; + } + + if (prev == -1) + return DMUS_E_NOT_FOUND; + + IDirectMusicBand_AddRef(bandparam->pBand); + if (out_next) *out_next = next - time; return S_OK; }
@@ -227,7 +247,19 @@ static HRESULT WINAPI band_track_SetParam(IDirectMusicTrack8 *iface, REFGUID typ return DMUS_E_TYPE_UNSUPPORTED;
if (IsEqualGUID(type, &GUID_BandParam)) - FIXME("GUID_BandParam not handled yet\n"); + { + struct band_entry *entry = calloc(1, sizeof(*entry)); + DMUS_BAND_PARAM *band_param = param; + if (!band_param || !band_param->pBand) + return E_POINTER; + if (!entry) + return E_OUTOFMEMORY; + entry->band = band_param->pBand; + entry->head.lBandTimeLogical = time; + entry->head.lBandTimePhysical = band_param->mtTimePhysical; + IDirectMusicBand_AddRef(entry->band); + list_add_tail(&This->bands, &entry->entry); + } else if (IsEqualGUID(type, &GUID_Clear_All_Bands)) FIXME("GUID_Clear_All_Bands not handled yet\n"); else if (IsEqualGUID(type, &GUID_ConnectToDLSCollection)) diff --git a/dlls/dmband/tests/dmband.c b/dlls/dmband/tests/dmband.c index d88606976e4..36f213b0bfd 100644 --- a/dlls/dmband/tests/dmband.c +++ b/dlls/dmband/tests/dmband.c @@ -176,9 +176,12 @@ static void test_dmband(void)
static void test_bandtrack(void) { + DMUS_BAND_PARAM bandparam = { 0 }; IDirectMusicTrack8 *dmt8; + IDirectMusicBand *dmb; IPersistStream *ps; CLSID class; + MUSIC_TIME mt; ULARGE_INTEGER size; char buf[64] = { 0 }; HRESULT hr; @@ -266,6 +269,30 @@ static void test_bandtrack(void) } }
+ hr = CoCreateInstance(&CLSID_DirectMusicBand, NULL, CLSCTX_INPROC_SERVER, + &IID_IDirectMusicBand, (void**)&dmb); + ok(hr == S_OK, "DirectMusicBand create failed: %#lx, expected S_OK\n", hr); + + hr = IDirectMusicTrack8_GetParam(dmt8, &GUID_BandParam, 0, &mt, &bandparam); + ok(hr == DMUS_E_NOT_FOUND, "hr %#lx, expected not found\n", hr); + ok(bandparam.pBand == NULL, "Got band %p, expected NULL\n", bandparam.pBand); + + bandparam.mtTimePhysical = 0; + bandparam.pBand = dmb; + hr = IDirectMusicTrack8_SetParam(dmt8, &GUID_BandParam, 0, &bandparam); + ok(hr == S_OK, "SetParam failed: %#lx, expected S_OK\n", hr); + + hr = IDirectMusicTrack8_GetParam(dmt8, &GUID_BandParam, 0, &mt, &bandparam); + ok(hr == S_OK, "GetParam failed: %#lx, expected S_OK\n", hr); + ok(mt == 0, "Got time %lu, expected 0\n", mt); + ok(bandparam.mtTimePhysical == 0, "Got physical time %lu, expected 0\n", bandparam.mtTimePhysical); + IDirectMusicBand_Release(bandparam.pBand); + IDirectMusicBand_Release(dmb); + + bandparam.pBand = NULL; + hr = IDirectMusicTrack8_SetParam(dmt8, &GUID_BandParam, 0, &bandparam); + ok(hr == E_POINTER, "hr %#lx, expected E_POINTER\n", hr); + hr = IDirectMusicTrack8_AddNotificationType(dmt8, NULL); ok(hr == E_NOTIMPL, "IDirectMusicTrack8_AddNotificationType failed: %#lx\n", hr); hr = IDirectMusicTrack8_RemoveNotificationType(dmt8, NULL);
From: Yuxuan Shui yshui@codeweavers.com
--- dlls/dmband/bandtrack.c | 1 + dlls/dmband/dmband_main.c | 1 + dlls/dmband/dmband_private.h | 10 ---------- dlls/dmusic/Makefile.in | 1 + dlls/{dmband => dmusic}/band.c | 11 ++++++++++- dlls/dmusic/band.h | 30 ++++++++++++++++++++++++++++++ 6 files changed, 43 insertions(+), 11 deletions(-) rename dlls/{dmband => dmusic}/band.c (99%) create mode 100644 dlls/dmusic/band.h
diff --git a/dlls/dmband/bandtrack.c b/dlls/dmband/bandtrack.c index dbd75a4a299..f168ccd9f8b 100644 --- a/dlls/dmband/bandtrack.c +++ b/dlls/dmband/bandtrack.c @@ -17,6 +17,7 @@ */
#include "dmband_private.h" +#include "band.h" #include "dmobject.h"
WINE_DEFAULT_DEBUG_CHANNEL(dmband); diff --git a/dlls/dmband/dmband_main.c b/dlls/dmband/dmband_main.c index 55f979e3fbc..2b470ba4d30 100644 --- a/dlls/dmband/dmband_main.c +++ b/dlls/dmband/dmband_main.c @@ -20,6 +20,7 @@ #include "initguid.h" #include "dmband_private.h" #include "rpcproxy.h" +#include "band.h" #include "dmobject.h"
WINE_DEFAULT_DEBUG_CHANNEL(dmband); diff --git a/dlls/dmband/dmband_private.h b/dlls/dmband/dmband_private.h index 48c5ec02aeb..3b2617322fe 100644 --- a/dlls/dmband/dmband_private.h +++ b/dlls/dmband/dmband_private.h @@ -20,21 +20,16 @@ #ifndef __WINE_DMBAND_PRIVATE_H #define __WINE_DMBAND_PRIVATE_H
-#include <stdio.h> #include <stdarg.h> -#include <string.h>
#define COBJMACROS
#include "windef.h" #include "winbase.h" #include "winnt.h" -#include "wingdi.h" -#include "winuser.h"
#include "wine/debug.h" #include "wine/list.h" -#include "winreg.h" #include "objbase.h"
#include "dmusici.h" @@ -44,11 +39,6 @@ /***************************************************************************** * ClassFactory */ -extern HRESULT create_dmband(REFIID riid, void **ret_iface); extern HRESULT create_dmbandtrack(REFIID riid, void **ret_iface);
-extern HRESULT band_connect_to_collection(IDirectMusicBand *iface, IDirectMusicCollection *collection); -extern HRESULT band_send_messages(IDirectMusicBand *iface, IDirectMusicPerformance *performance, - IDirectMusicGraph *graph, MUSIC_TIME time, DWORD track_id); - #endif /* __WINE_DMBAND_PRIVATE_H */ diff --git a/dlls/dmusic/Makefile.in b/dlls/dmusic/Makefile.in index 8e1c07e90a7..446eca15543 100644 --- a/dlls/dmusic/Makefile.in +++ b/dlls/dmusic/Makefile.in @@ -2,6 +2,7 @@ MODULE = dmusic.dll IMPORTS = dxguid uuid ole32 advapi32 dsound user32 winmm
SOURCES = \ + band.c \ buffer.c \ clock.c \ collection.c \ diff --git a/dlls/dmband/band.c b/dlls/dmusic/band.c similarity index 99% rename from dlls/dmband/band.c rename to dlls/dmusic/band.c index 78ec68ae035..2fdee5701b4 100644 --- a/dlls/dmband/band.c +++ b/dlls/dmusic/band.c @@ -17,7 +17,16 @@ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA */
-#include "dmband_private.h" + +#define COBJMACROS + +#include "wine/debug.h" +#include "wine/list.h" +#include "objbase.h" + +#include "dmusici.h" +#include "dmusicf.h" + #include "dmobject.h"
WINE_DEFAULT_DEBUG_CHANNEL(dmband); diff --git a/dlls/dmusic/band.h b/dlls/dmusic/band.h new file mode 100644 index 00000000000..660f208f34a --- /dev/null +++ b/dlls/dmusic/band.h @@ -0,0 +1,30 @@ +/* DirectMusicBand Private Include + * + * Copyright (C) 2024 Yuxuan Shui for CodeWeavers + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA + */ + +#ifndef __WINE_BAND_H +#define __WINE_BAND_H + +#include "dmusici.h" + +extern HRESULT create_dmband(REFIID riid, void **ret_iface); +extern HRESULT band_connect_to_collection(IDirectMusicBand *iface, IDirectMusicCollection *collection); +extern HRESULT band_send_messages(IDirectMusicBand *iface, IDirectMusicPerformance *performance, + IDirectMusicGraph *graph, MUSIC_TIME time, DWORD track_id); + +#endif
From: Yuxuan Shui yshui@codeweavers.com
So that instrument downloading etc. could work. --- dlls/dmime/Makefile.in | 1 + dlls/dmime/midi.c | 63 ++++++++++++++++++++++++++++++++++++++++++ dlls/dmime/segment.c | 2 ++ dlls/dmusic/band.c | 16 +++++++++++ dlls/dmusic/band.h | 1 + 5 files changed, 83 insertions(+)
diff --git a/dlls/dmime/Makefile.in b/dlls/dmime/Makefile.in index f8f622f8f3d..0f7b7db609e 100644 --- a/dlls/dmime/Makefile.in +++ b/dlls/dmime/Makefile.in @@ -4,6 +4,7 @@ PARENTSRC = ../dmusic
SOURCES = \ audiopath.c \ + band.c \ dmime.idl \ dmime_main.c \ dmobject.c \ diff --git a/dlls/dmime/midi.c b/dlls/dmime/midi.c index 1df1b9b15fd..334f6ecc1ae 100644 --- a/dlls/dmime/midi.c +++ b/dlls/dmime/midi.c @@ -17,6 +17,7 @@ */
#include "dmime_private.h" +#include "band.h"
WINE_DEFAULT_DEBUG_CHANNEL(dmime);
@@ -662,15 +663,77 @@ struct midi_parser { IStream *stream; DWORD division; + BOOL bandtrack_created; WORD number_of_tracks, current_track; };
+static HRESULT create_midi_bandtrack(IStream *stream, IDirectMusicTrack **track) +{ + DMUS_OBJECTDESC default_desc = + { + .dwSize = sizeof(DMUS_OBJECTDESC), + .dwValidData = DMUS_OBJ_OBJECT | DMUS_OBJ_CLASS, + .guidClass = CLSID_DirectMusicCollection, + .guidObject = GUID_DefaultGMCollection, + }; + HRESULT hr; + int i; + IDirectMusicTrack *bandtrack = NULL; + IDirectMusicBand *band = NULL; + IDirectMusicCollection *collection = NULL; + DMUS_BAND_PARAM bandparam; + + hr = stream_get_object(stream, &default_desc, &IID_IDirectMusicCollection, (void **)&collection); + if (FAILED(hr)) WARN("Failed to load default collection from loader, hr %#lx\n", hr); + + hr = CoCreateInstance(&CLSID_DirectMusicBandTrack, NULL, CLSCTX_INPROC_SERVER, + &IID_IDirectMusicTrack, (void **)&bandtrack); + if (FAILED(hr)) return hr; + + hr = create_dmband(&IID_IDirectMusicBand, (void **)&band); + if (FAILED(hr)) goto out; + for (i = 0; i < 128; i++) + { + hr = band_add_instrument(band, i, 0); + if (FAILED(hr)) break; + } + if (FAILED(hr)) goto out; + if (collection) + { + hr = band_connect_to_collection(band, collection); + if (FAILED(hr)) WARN("Failed to connect band to default collection, hr %#lx\n", hr); + } + + bandparam.mtTimePhysical = 0; + bandparam.pBand = band; + hr = IDirectMusicTrack_SetParam(bandtrack, &GUID_BandParam, 0, &bandparam); + if (FAILED(hr)) goto out; + + *track = bandtrack; + IDirectMusicTrack_AddRef(bandtrack); + +out: + IDirectMusicTrack_Release(bandtrack); + if (collection) IDirectMusicCollection_Release(collection); + if (band) IDirectMusicBand_Release(band); + return hr; +} + HRESULT midi_parser_next_track(struct midi_parser *parser, IDirectMusicTrack **out_track, MUSIC_TIME *out_length) { IPersistStream *pstream; HRESULT hr; MUSIC_TIME midi_length;
+ if (!parser->bandtrack_created) + { + hr = create_midi_bandtrack(parser->stream, out_track); + if (FAILED(hr)) return hr; + parser->bandtrack_created = TRUE; + *out_length = 0; + return S_OK; + } + if (parser->current_track >= parser->number_of_tracks) return S_FALSE;
hr = create_midiseqtrack(&IID_IPersistStream, (void **)&pstream, parser->division); diff --git a/dlls/dmime/segment.c b/dlls/dmime/segment.c index c37e76b91cb..49e11cc77e5 100644 --- a/dlls/dmime/segment.c +++ b/dlls/dmime/segment.c @@ -17,7 +17,9 @@ * License along with this program; if not, write to the Free Software * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA */ + #include "dmime_private.h" +#include "band.h"
#ifdef WORDS_BIGENDIAN #define GET_BE_WORD(x) (x) diff --git a/dlls/dmusic/band.c b/dlls/dmusic/band.c index 2fdee5701b4..e5aebc0e46b 100644 --- a/dlls/dmusic/band.c +++ b/dlls/dmusic/band.c @@ -496,6 +496,22 @@ HRESULT band_connect_to_collection(IDirectMusicBand *iface, IDirectMusicCollecti return S_OK; }
+HRESULT band_add_instrument(IDirectMusicBand *iface, DWORD patch, DWORD pchannel) +{ + struct band *This = impl_from_IDirectMusicBand(iface); + struct instrument_entry *entry; + + TRACE("%p, %lu, %lu\n", iface, patch, pchannel); + if (!(entry = calloc(1, sizeof(*entry)))) + return E_OUTOFMEMORY; + + entry->instrument.dwPatch = patch; + entry->instrument.dwPChannel = pchannel; + + list_add_tail(&This->instruments, &entry->entry); + return S_OK; +} + HRESULT band_send_messages(IDirectMusicBand *iface, IDirectMusicPerformance *performance, IDirectMusicGraph *graph, MUSIC_TIME time, DWORD track_id) { diff --git a/dlls/dmusic/band.h b/dlls/dmusic/band.h index 660f208f34a..91c8fcb9c23 100644 --- a/dlls/dmusic/band.h +++ b/dlls/dmusic/band.h @@ -26,5 +26,6 @@ extern HRESULT create_dmband(REFIID riid, void **ret_iface); extern HRESULT band_connect_to_collection(IDirectMusicBand *iface, IDirectMusicCollection *collection); extern HRESULT band_send_messages(IDirectMusicBand *iface, IDirectMusicPerformance *performance, IDirectMusicGraph *graph, MUSIC_TIME time, DWORD track_id); +HRESULT band_add_instrument(IDirectMusicBand *iface, DWORD patch, DWORD pchannel);
#endif
From: Yuxuan Shui yshui@codeweavers.com
Try to download all instruments in a band, even if some of them fails. --- dlls/dmusic/band.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/dlls/dmusic/band.c b/dlls/dmusic/band.c index e5aebc0e46b..08a9c5bd6b3 100644 --- a/dlls/dmusic/band.c +++ b/dlls/dmusic/band.c @@ -202,10 +202,9 @@ static HRESULT WINAPI band_Download(IDirectMusicBand *iface, IDirectMusicInstrument_Release(instrument); }
- if (FAILED(hr)) break; + if (FAILED(hr)) WARN("Failed to download instruments %lu, hr %#lx\n", entry->instrument.dwPatch, hr); }
- if (FAILED(hr)) WARN("Failed to download instruments, hr %#lx\n", hr); return hr; }
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=142833
Your paranoid android.
=== debian11 (32 bit report) ===
dmime: dmime.c:1575: Test succeeded inside todo block: unable to get band track from midi file: 0
=== debian11 (32 bit ar:MA report) ===
dmime: dmime.c:1575: Test succeeded inside todo block: unable to get band track from midi file: 0
=== debian11 (32 bit de report) ===
dmime: dmime.c:1575: Test succeeded inside todo block: unable to get band track from midi file: 0
=== debian11 (32 bit fr report) ===
dmime: dmime.c:1575: Test succeeded inside todo block: unable to get band track from midi file: 0
=== debian11 (32 bit he:IL report) ===
dmime: dmime.c:1575: Test succeeded inside todo block: unable to get band track from midi file: 0
=== debian11 (32 bit hi:IN report) ===
dmime: dmime.c:1575: Test succeeded inside todo block: unable to get band track from midi file: 0
=== debian11 (32 bit ja:JP report) ===
dmime: dmime.c:1575: Test succeeded inside todo block: unable to get band track from midi file: 0
=== debian11 (32 bit zh:CN report) ===
dmime: dmime.c:1575: Test succeeded inside todo block: unable to get band track from midi file: 0
=== debian11b (32 bit WoW report) ===
dmime: dmime.c:1575: Test succeeded inside todo block: unable to get band track from midi file: 0
=== debian11b (64 bit WoW report) ===
dmime: dmime.c:1575: Test succeeded inside todo block: unable to get band track from midi file: 0
ah, i think midi does need a different dmband implementation...
Rémi Bernon (@rbernon) commented about dlls/dmime/tests/dmime.c:
+{
- IDirectMusicSegment8 *segment = NULL;
- IDirectMusicLoader8 *loader;
- WCHAR test_mid[MAX_PATH];
- HRESULT hr;
- load_resource(L"test.mid", test_mid);
- hr = CoCreateInstance(&CLSID_DirectMusicLoader, NULL, CLSCTX_INPROC_SERVER,
&IID_IDirectMusicLoader8, (void **)&loader);- ok(hr == S_OK, "got %#lx\n", hr);
- hr = IDirectMusicLoader8_LoadObjectFromFile(loader, &CLSID_DirectMusicSegment,
&IID_IDirectMusicSegment, test_mid, (void **)&segment);- todo_wine ok(hr == S_OK, "got %#lx\n", hr);
- IDirectMusicLoader8_Release(loader);
- if (segment) IDirectMusicSegment8_Release(segment);
With this commit alone we return S_OK and the test doesn't seem to need a todo_wine (or the `if (segment)` below).
I guess this is probably because we eat the error somewhere in the call chain, but you should adjust the test results so that there's no failure (or spurious success) regardless of which commit we run them at.
You can keep the test as it is, or you can also start by fixing wherever the error is swallowed, so that the todo_wine is indeed needed.
Rémi Bernon (@rbernon) commented about dlls/dmime/midi.c:
+static inline struct midi_sequence_track *impl_from_IDirectMusicTrack8(IDirectMusicTrack8 *iface) +{
- return CONTAINING_RECORD(iface, struct midi_sequence_track, IDirectMusicTrack8_iface);
+}
+static HRESULT WINAPI midi_sequence_track_QueryInterface(IDirectMusicTrack8 *iface, REFIID riid, void **ret_iface) +{
- struct midi_sequence_track *This = impl_from_IDirectMusicTrack8(iface);
- TRACE("(%p, %s, %p)\n", This, debugstr_dmguid(riid), ret_iface);
- if (IsEqualIID(riid, &IID_IUnknown) || IsEqualIID(riid, &IID_IDirectMusicTrack) ||
IsEqualIID(riid, &IID_IDirectMusicTrack8))*ret_iface = &This->IDirectMusicTrack8_iface;- else if (IsEqualIID(riid, &IID_IPersistStream)) *ret_iface = &This->dmobj.IPersistStream_iface;
- else
```suggestion:-4+0 if (IsEqualIID(riid, &IID_IUnknown) || IsEqualIID(riid, &IID_IDirectMusicTrack) || IsEqualIID(riid, &IID_IDirectMusicTrack8)) *ret_iface = &This->IDirectMusicTrack8_iface; else if (IsEqualIID(riid, &IID_IPersistStream)) *ret_iface = &This->dmobj.IPersistStream_iface; else ```
Rémi Bernon (@rbernon) commented about dlls/dmime/midi.c:
- if (!ref) free(This);
- return ref;
+}
+static HRESULT WINAPI midi_sequence_track_Init(IDirectMusicTrack8 *iface, IDirectMusicSegment *pSegment) +{
- struct midi_sequence_track *This = impl_from_IDirectMusicTrack8(iface);
- FIXME("(%p, %p): stub\n", This, pSegment);
- return S_OK;
+}
+static HRESULT WINAPI midi_sequence_track_InitPlay(IDirectMusicTrack8 *iface,
IDirectMusicSegmentState *pSegmentState, IDirectMusicPerformance *pPerformance,void **ppStateData, DWORD dwVirtualTrack8ID, DWORD dwFlags)
Please get rid of the hungarian notation. Same comment for the other functions here.
Rémi Bernon (@rbernon) commented about dlls/dmime/midi.c:
+{
- struct midi_sequence_track *This = impl_from_IDirectMusicTrack8(iface);
- FIXME("(%p, %p): stub\n", This, pStateData);
- return S_OK;
+}
+static HRESULT WINAPI midi_sequence_track_Play(IDirectMusicTrack8 *iface, void *state_data,
MUSIC_TIME start_time, MUSIC_TIME end_time, MUSIC_TIME time_offset, DWORD segment_flags,IDirectMusicPerformance *performance, IDirectMusicSegmentState *segment_state, DWORD track_id)+{
- struct midi_sequence_track *This = impl_from_IDirectMusicTrack8(iface);
- TRACE("(%p, %p, %ld, %ld, %ld, %#lx, %p, %p, %ld)\n", This, state_data, start_time, end_time,
time_offset, segment_flags, performance, segment_state, track_id);- return E_NOTIMPL;
Unless they are tested and supposed to return E_NOTIMPL, functions which are unimplemented and return E_NOTIMPL should generally have a FIXME message instead of a TRACE. Same thing below.
For functions which are called too often, a `static int once` flag can be used to only print the message once.
Same comment for several functions below here.
(And I see where this comes from, but dmusic has a lot of old code, and doesn't follow recent Wine code best practices.)
Rémi Bernon (@rbernon) commented about dlls/dmime/midi.c:
- return S_OK;
+}
+static const IPersistStreamVtbl midi_sequence_track_persiststream_vtbl = +{
- dmobj_IPersistStream_QueryInterface,
- dmobj_IPersistStream_AddRef,
- dmobj_IPersistStream_Release,
- dmobj_IPersistStream_GetClassID,
- unimpl_IPersistStream_IsDirty,
- track_IPersistStream_Load,
- unimpl_IPersistStream_Save,
- unimpl_IPersistStream_GetSizeMax,
+};
+HRESULT create_midiseqtrack(REFIID riid, void **ppobj)
When considering this commit alone, this is still not called anywhere so all of this is dead code. You need to introduce something that uses these stubs at the same time or before they are added.
In this particular case, you could probably split the MIDI parsing commit, introduce the file parser helper first, add these stubs in a separate commit (instantiating the tracks from the parser), and implementing track parsing in a third one for instance.
Rémi Bernon (@rbernon) commented about dlls/dmime/midi.c:
+{
- struct midi_sequence_track *This = impl_from_IDirectMusicTrack8(iface);
- TRACE("(%p, %p, %ld, %p): method not implemented\n", This, context, trackgroup, track);
- return E_NOTIMPL;
+}
+static HRESULT WINAPI midi_sequence_track_Join(IDirectMusicTrack8 *iface, IDirectMusicTrack *newtrack,
MUSIC_TIME join, IUnknown *context, DWORD trackgroup, IDirectMusicTrack **resulttrack)+{
- struct midi_sequence_track *This = impl_from_IDirectMusicTrack8(iface);
- TRACE("(%p, %p, %ld, %p, %ld, %p): method not implemented\n", This, newtrack, join, context,
trackgroup, resulttrack);- return E_NOTIMPL;
+} +struct IDirectMusicTrack8Vtbl midi_sequence_track_vtbl =
Missing newline.
On Wed Jan 31 18:34:08 2024 +0000, Yuxuan Shui wrote:
Is this necessary? What's wrong with implementing all necessary parsing in one commit? I don't think it's big enough to warrant splitting.
It makes reviewing, following the implementation progress, checking whether it works, etc easier. This is pretty much how everything is done in Wine and you will be asked to split things more often than to squash them.
Rémi Bernon (@rbernon) commented about dlls/dmime/dmime_private.h:
extern HRESULT create_dmtempotrack(REFIID riid, void **ret_iface); extern HRESULT create_dmtimesigtrack(REFIID riid, void **ret_iface); extern HRESULT create_dmwavetrack(REFIID riid, void **ret_iface); +/* Create a new MIDI file parser. Note the stream might still be modified even
Let's add a newline above, grouping related and similar functions together.
Rémi Bernon (@rbernon) commented about dlls/dmime/midi.c:
- case MIDI_META_SEQUENCE_NUMBER:
- case MIDI_META_CHANNEL_PREFIX_ASSIGNMENT:
- case MIDI_META_END_OF_TRACK:
- case MIDI_META_SET_TEMPO:
- case MIDI_META_SMPTE_OFFSET:
- case MIDI_META_TIME_SIGNATURE:
- case MIDI_META_KEY_SIGNATURE: return TRUE;
- case MIDI_META_TEXT_EVENT:
- case MIDI_META_COPYRIGHT_NOTICE:
- case MIDI_META_TRACK_NAME:
- case MIDI_META_INSTRUMENT_NAME:
- case MIDI_META_LYRIC:
- case MIDI_META_MARKER:
- case MIDI_META_CUE_POINT:
- case MIDI_META_SEQUENCER_SPECIFIC: return FALSE;
Are we actually going to use all these events? As far as I can see they are not, so I would suggest to simplify the code and event union, only supporting fixed length events, skipping over variable length ones (with a FIXME).
Rémi Bernon (@rbernon) commented about dlls/dmime/midi.c:
if (!(event = calloc(1, sizeof(*event)))) return E_OUTOFMEMORY;hr = read_midi_event(stream, event, last_status, &bytes_read);if (hr != S_OK) break;list_add_tail(&This->events, &event->entry);This->event_count++;last_status = event->status;is_end_of_track = event->status == 0xff && event->type == MIDI_META_END_OF_TRACK;event = NULL;- }
- if (hr == S_OK && bytes_read != length)
- {
WARN("Read %lu bytes, expected %lu\n", bytes_read, length);hr = E_FAIL;- }
Do we really need this check (and the additional read_accumulate helper it requires)?
I understand how it might seem important for correctness, but checks need to be preferably either trivial (like parameter checks), or something that Windows does and that applications would need.
Ideally, any check should have a corresponding test to make sure it can be added without making the behavior different (let's imagine applications with bogus MIDI files and Windows maybe just parses whatever it can from them).
Rémi Bernon (@rbernon) commented about dlls/dmime/midi.c:
hr = stream_read_accumulate(stream, &byte, 1, total_bytes_read);if (hr != S_OK) return hr;event->type = byte;hr = read_variable_length_number(stream, &length, total_bytes_read);if (hr != S_OK) return hr;if (is_event_fixed_length(event->status, event->type)){if (length > sizeof(event->data.fixed.byte)){WARN("Invalid MIDI meta event length %lu for type %#02x\n", length, event->type);return E_FAIL;}hr = stream_read_accumulate(stream, &event->data.fixed.byte[0], length, total_bytes_read);if (hr == S_OK)
```suggestion:-1+0 hr = stream_read_accumulate(stream, &event->data.fixed.byte[0], length, total_bytes_read); if (hr != S_OK) return hr; ```
Rémi Bernon (@rbernon) commented about dlls/dmime/midi.c:
- hr = IStream_Read(stream, &length, sizeof(length), NULL);
- if (FAILED(hr)) return hr;
- length = GET_BE_DWORD(length);
- TRACE("MIDI track length %lu\n", length);
- while (!is_end_of_track)
- {
if (!(event = calloc(1, sizeof(*event)))) return E_OUTOFMEMORY;hr = read_midi_event(stream, event, last_status, &bytes_read);if (hr != S_OK) break;list_add_tail(&This->events, &event->entry);This->event_count++;last_status = event->status;is_end_of_track = event->status == 0xff && event->type == MIDI_META_END_OF_TRACK;event = NULL;
I would find it easier to reason about, and later easier to refactor (think splitting the loop body to a separate helper), if the event variable was scoped here, and either freed or inserted in the list within the loop.
Rémi Bernon (@rbernon) commented about dlls/dmime/midi.c:
hr = stream_read_accumulate(stream, event->data.variable.data, length, total_bytes_read);}TRACE("MIDI meta event type %#02x, length %lu, time +%lu\n", event->type, length, event->deltaTime);- }
- else if (event->status == 0xf0 || event->status == 0xf7)
- {
hr = read_variable_length_number(stream, &length, total_bytes_read);if (hr != S_OK) return hr;event->data.variable.data = malloc(length);if (!event->data.variable.data) return E_OUTOFMEMORY;event->data.variable.size = length;hr = stream_read_accumulate(stream, event->data.variable.data, length, total_bytes_read);FIXME("MIDI sysex event type %#02x, length %lu, time +%lu. not ""supported\n",event->status, length, event->deltaTime);
```suggestion:-2+0 FIXME("MIDI sysex event type %#02x, length %lu, time +%lu. not " "supported\n", event->status, length, event->deltaTime); ```
Rémi Bernon (@rbernon) commented about dlls/dmime/midi.c:
if (is_event_fixed_length(event->status, event->type)){if (length > sizeof(event->data.fixed.byte)){WARN("Invalid MIDI meta event length %lu for type %#02x\n", length, event->type);return E_FAIL;}hr = stream_read_accumulate(stream, &event->data.fixed.byte[0], length, total_bytes_read);if (hr == S_OK){if (event->type == MIDI_META_SET_TEMPO){event->data.integer = (event->data.fixed.byte[0] << 16) |(event->data.fixed.byte[1] << 8) | event->data.fixed.byte[2];if (event->data.integer == 0)
```suggestion:-2+0 event->data.integer = (event->data.fixed.byte[0] << 16) | (event->data.fixed.byte[1] << 8) | event->data.fixed.byte[2]; if (event->data.integer == 0) ```
Rémi Bernon (@rbernon) commented about dlls/dmime/midi.c:
if (length > sizeof(event->data.fixed.byte)){WARN("Invalid MIDI meta event length %lu for type %#02x\n", length, event->type);return E_FAIL;}hr = stream_read_accumulate(stream, &event->data.fixed.byte[0], length, total_bytes_read);if (hr == S_OK){if (event->type == MIDI_META_SET_TEMPO){event->data.integer = (event->data.fixed.byte[0] << 16) |(event->data.fixed.byte[1] << 8) | event->data.fixed.byte[2];if (event->data.integer == 0){WARN("Invalid tempo value 0\n");hr = E_FAIL;
```suggestion:-0+0 return E_FAIL; ```
Rémi Bernon (@rbernon) commented about dlls/dmime/midi.c:
if (event->data.integer == 0){WARN("Invalid tempo value 0\n");hr = E_FAIL;}}}}else{event->data.variable.data = malloc(length);if (!event->data.variable.data) return E_OUTOFMEMORY;event->data.variable.size = length;hr = stream_read_accumulate(stream, event->data.variable.data, length, total_bytes_read);}TRACE("MIDI meta event type %#02x, length %lu, time +%lu\n", event->type, length, event->deltaTime);
Here and below you might print the event parsing message after a possibly a failed read, I think this could be confusing.
Rémi Bernon (@rbernon) commented about dlls/dmime/midi.c:
dmobj_IPersistStream_GetClassID, unimpl_IPersistStream_IsDirty,
- track_IPersistStream_Load,
- midi_sequence_track_IPersistStream_Load, unimpl_IPersistStream_Save, unimpl_IPersistStream_GetSizeMax,
};
-HRESULT create_midiseqtrack(REFIID riid, void **ppobj) +static MUSIC_TIME midi_sequence_track_get_length(IDirectMusicTrack8 *iface) +{
- struct midi_sequence_track *This = impl_from_IDirectMusicTrack8(iface);
- struct event *event;
- MUSIC_TIME length = 0;
- LIST_FOR_EACH_ENTRY(event, &This->events, struct event, entry)
- length += event->deltaTime;
```suggestion:-1+0 LIST_FOR_EACH_ENTRY(event, &This->events, struct event, entry) length += event->deltaTime; ```
Rémi Bernon (@rbernon) commented about dlls/dmime/midi.c:
- /* MIDI deltaTime can be either tempo dependent or not. If
* ticks_per_quarter_note is 0, then ticks_per_second should be used.* Converting MIDI time to DirectMusic MUSIC_TIME is complicated. MUSIC_TIME* is number of quarter notes times DMUS_PPQ, this is tempo dependent. If* MIDI time is tempo dependent as well, then we just need to multiply it by* a factor (because MIDI can have a different division of quarter notes).** If MIDI time is not tempo dependent however, the conversion would be* difficult because tempo information can be stored in another track. And* we need the tempo information to convert MIDI time to MUSIC_TIME. The* only reasonable way to do this is to report a "fake", fixed tempo, which* makes MUSIC_TIME and MIDI time the same.*/- UINT ticks_per_quarter_note;
- UINT ticks_per_second;
The tempo event thing looks complicated, and I would suggest writing tests for them, and implementing their parsing separately.
I haven't looked at everything yet, but it looks better now. I've made some more comments but I will also suggest to split this MR with only the fixed size, non-tempo event parsing for now.
-
Marvin -
Rémi Bernon (＠rbernon) -
Yuxuan Shui
-
Yuxuan Shui (＠yshui)