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.
-- v2: dmime: Synthesize a band track for MIDI segments. dmband: Implement setting GUID_BandParam on band tracks. dmime/test: Add test for getting band track from midi file. dmime/test: add MIDI loading test dmime: Implement getting tempo param for MIDI tracks. dmime: Parse MIDI files. dmime: Add stubs for MIDI tracks
From: Yuxuan Shui yshui@codeweavers.com
--- dlls/dmime/Makefile.in | 1 + dlls/dmime/miditracks.c | 499 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 500 insertions(+) create mode 100644 dlls/dmime/miditracks.c
diff --git a/dlls/dmime/Makefile.in b/dlls/dmime/Makefile.in index cf041ef2b27..fe7aa7df272 100644 --- a/dlls/dmime/Makefile.in +++ b/dlls/dmime/Makefile.in @@ -10,6 +10,7 @@ SOURCES = \ graph.c \ lyricstrack.c \ markertrack.c \ + miditracks.c \ paramcontroltrack.c \ performance.c \ segment.c \ diff --git a/dlls/dmime/miditracks.c b/dlls/dmime/miditracks.c new file mode 100644 index 00000000000..8dc0a645247 --- /dev/null +++ b/dlls/dmime/miditracks.c @@ -0,0 +1,499 @@ +/* + * 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); + +union event_data +{ + struct + { + BYTE byte1; + BYTE byte2; + BYTE byte3; + BYTE byte4; + BYTE byte5; + } 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 +{ + UINT 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 event events[]; +}; + +C_ASSERT(sizeof(struct midi_sequence_track) == + offsetof(struct midi_sequence_track, events[0])); + +static inline struct midi_sequence_track * +impl_from_IDirectMusicTrack8(IDirectMusicTrack8 *iface) +{ + return CONTAINING_RECORD(iface, struct midi_sequence_track, + IDirectMusicTrack8_iface); +} + +static void free_event(struct event *event) +{ + if (event->status == 0xff) + { + switch (event->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: + break; + + 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: + free(event->data.variable.data); + break; + default: + ERR("Unknown meta event type %02x\n", event->type); + } + } +} + +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), i; + + TRACE("(%p): %lu\n", This, ref); + + if (!ref) + { + for (i = 0; i < This->event_count; i++) + free_event(This->events + i); + 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); + IDirectMusicGraph *graph; + HRESULT hr; + UINT i; + /* # of quarter notes is the most reasonable common units of time between + * MIDI and other parts of DirectMusic. */ + double number_of_quarter_notes = 0, usec_per_quarter_note = 500000; + /* reference time is in 100 nanoseconds units, or 10 million ticks per + * second. */ + double ref_time = 0; + + 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); + + 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; + + for (i = 0; SUCCEEDED(hr) && i < This->event_count; i++) + { + struct event *item = This->events + i; + DMUS_MIDI_PMSG *msg; + + if (This->ticks_per_quarter_note) + { + ref_time += item->deltaTime * usec_per_quarter_note * 10 / + This->ticks_per_quarter_note; + number_of_quarter_notes += + (double)item->deltaTime / This->ticks_per_quarter_note; + } + else + { + double elapsed = + item->deltaTime * 1000000.0 / This->ticks_per_second; + ref_time += elapsed; + number_of_quarter_notes += elapsed / 10 / usec_per_quarter_note; + } + + if (item->status == 0xff) + { + hr = handle_meta_events(This, item, &usec_per_quarter_note); + if (hr != S_OK) + break; + continue; + } + + if ((MUSIC_TIME)number_of_quarter_notes < + (start_time + DMUS_PPQ - 1) / DMUS_PPQ) + continue; + if ((MUSIC_TIME)number_of_quarter_notes >= end_time / DMUS_PPQ) + continue; + + if (FAILED(hr = IDirectMusicPerformance_AllocPMsg( + performance, sizeof(*msg), (DMUS_PMSG **)&msg))) + break; + + msg->dwFlags = DMUS_PMSGF_REFTIME; + msg->rtTime = (REFERENCE_TIME)ref_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.byte1; + msg->bByte2 = item->data.fixed.byte2; + + 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, + 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 | 12 ++ dlls/dmime/miditracks.c | 307 +++++++++++++++++++++++++++++++------ dlls/dmime/segment.c | 125 ++++++++++++++- 3 files changed, 392 insertions(+), 52 deletions(-)
diff --git a/dlls/dmime/dmime_private.h b/dlls/dmime/dmime_private.h index 96ccd5daf8b..873659a58ce 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" @@ -68,6 +69,9 @@ 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); +extern HRESULT create_midiseqtrack(REFIID riid, void **ppobj, DWORD division); + +MUSIC_TIME get_midiseqtrack_length(IDirectMusicTrack8 *iface);
extern void set_audiopath_perf_pointer(IDirectMusicAudioPath*,IDirectMusicPerformance8*); extern void set_audiopath_dsound_buffer(IDirectMusicAudioPath*,IDirectSoundBuffer*); @@ -111,4 +115,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/miditracks.c b/dlls/dmime/miditracks.c index 8dc0a645247..84c59928e14 100644 --- a/dlls/dmime/miditracks.c +++ b/dlls/dmime/miditracks.c @@ -24,11 +24,7 @@ union event_data { struct { - BYTE byte1; - BYTE byte2; - BYTE byte3; - BYTE byte4; - BYTE byte5; + BYTE byte[5]; } fixed; UINT integer; struct @@ -59,7 +55,8 @@ enum meta_event_type
struct event { - UINT deltaTime; + struct list entry; + DWORD deltaTime; BYTE status; /* type of meta events. */ enum meta_event_type type; @@ -89,12 +86,9 @@ struct midi_sequence_track UINT ticks_per_second; UINT event_count;
- struct event events[]; + struct list events; };
-C_ASSERT(sizeof(struct midi_sequence_track) == - offsetof(struct midi_sequence_track, events[0])); - static inline struct midi_sequence_track * impl_from_IDirectMusicTrack8(IDirectMusicTrack8 *iface) { @@ -102,37 +96,48 @@ impl_from_IDirectMusicTrack8(IDirectMusicTrack8 *iface) IDirectMusicTrack8_iface); }
-static void free_event(struct event *event) +static BOOL is_event_fixed_length(BYTE status, enum meta_event_type type) { - if (event->status == 0xff) - { - switch (event->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: - break; + if (status == 0xf0 || status == 0xf7) + return FALSE;
- 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: - free(event->data.variable.data); - break; - default: - ERR("Unknown meta event type %02x\n", event->type); - } + 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) { @@ -171,14 +176,19 @@ 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); - ULONG ref = InterlockedDecrement(&This->ref), i; + struct event *event, *next_event; + ULONG ref = InterlockedDecrement(&This->ref);
TRACE("(%p): %lu\n", This, ref);
if (!ref) { - for (i = 0; i < This->event_count; i++) - free_event(This->events + i); + LIST_FOR_EACH_ENTRY_SAFE(event, next_event, &This->events, struct event, + entry) + { + list_remove(&event->entry); + free_event(event); + } free(This); }
@@ -219,6 +229,7 @@ static HRESULT WINAPI midi_sequence_track_Play( IDirectMusicSegmentState *segment_state, DWORD track_id) { struct midi_sequence_track *This = impl_from_IDirectMusicTrack8(iface); + struct list *cursor; IDirectMusicGraph *graph; HRESULT hr; UINT i; @@ -242,9 +253,11 @@ static HRESULT WINAPI midi_sequence_track_Play( performance, &IID_IDirectMusicGraph, (void **)&graph))) return hr;
- for (i = 0; SUCCEEDED(hr) && i < This->event_count; i++) + cursor = list_head(&This->events); + for (i = 0; SUCCEEDED(hr) && i < This->event_count; + i++, cursor = list_next(&This->events, cursor)) { - struct event *item = This->events + i; + struct event *item = LIST_ENTRY(cursor, struct event, entry); DMUS_MIDI_PMSG *msg;
if (This->ticks_per_quarter_note) @@ -287,8 +300,8 @@ static HRESULT WINAPI midi_sequence_track_Play( msg->dwType = DMUS_PMSGT_MIDI; msg->dwGroupID = 1; msg->bStatus = item->status; - msg->bByte1 = item->data.fixed.byte1; - msg->bByte2 = item->data.fixed.byte2; + 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, @@ -455,13 +468,195 @@ impl_from_IPersistStream(IPersistStream *iface) 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 = @@ -471,12 +666,21 @@ 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 -}; + unimpl_IPersistStream_GetSizeMax};
-HRESULT create_midiseqtrack(REFIID riid, void **ppobj) +MUSIC_TIME get_midiseqtrack_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; +} + +HRESULT create_midiseqtrack(REFIID riid, void **ppobj, DWORD divison) { struct midi_sequence_track *track; HRESULT hr; @@ -490,6 +694,9 @@ HRESULT create_midiseqtrack(REFIID riid, void **ppobj) (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); diff --git a/dlls/dmime/segment.c b/dlls/dmime/segment.c index b208f89148c..beba1bbd34d 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 @@ -783,6 +790,120 @@ static HRESULT parse_dmsg_chunk(struct segment *This, IStream *stream, const str return SUCCEEDED(hr) ? S_OK : hr; }
+static HRESULT parse_midi_track(struct segment *This, IStream *stream, DWORD division) +{ + IDirectMusicTrack *track; + IPersistStream *pstream; + HRESULT hr; + struct track_entry *entry; + MUSIC_TIME length; + + hr = create_midiseqtrack(&IID_IPersistStream, (void **)&pstream, division); + if (FAILED(hr)) + return hr; + + hr = IPersistStream_Load(pstream, 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 **)&track); + IPersistStream_Release(pstream); + if (FAILED(hr)) + return hr; + + entry = calloc(1, sizeof(*entry)); + if (!entry) + { + IDirectMusicTrack_Release(track); + return E_OUTOFMEMORY; + } + entry->pTrack = track; + list_add_tail(&This->tracks, &entry->entry); + length = get_midiseqtrack_length((IDirectMusicTrack8 *)track); + if (length > This->header.mtLength) + This->header.mtLength = length; + return S_OK; +} + +static HRESULT parse_midi(struct segment *This, IStream *stream) +{ + LARGE_INTEGER offset; + DWORD length; + ULONG i; + HRESULT hr; + WORD format, number_of_tracks, division; + + /* Skip over the 'MThd' magic number. */ + offset.QuadPart = 4; + hr = IStream_Seek(stream, offset, STREAM_SEEK_SET, NULL); + if (FAILED(hr)) + return hr; + + hr = IStream_Read(stream, &length, sizeof(length), NULL); + if (FAILED(hr)) + return hr; + length = GET_BE_DWORD(length); + if (length != 6) + { + WARN("Invalid MIDI header length %lu\n", length); + return DMUS_E_UNSUPPORTED_STREAM; + } + + hr = IStream_Read(stream, &format, sizeof(format), NULL); + if (FAILED(hr)) + 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; + } + + hr = + IStream_Read(stream, &number_of_tracks, sizeof(number_of_tracks), NULL); + if (FAILED(hr)) + return hr; + + number_of_tracks = GET_BE_WORD(number_of_tracks); + hr = IStream_Read(stream, &division, sizeof(division), NULL); + if (FAILED(hr)) + 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); + + This->header.mtLength = 0; + for (i = 0; i < number_of_tracks; i++) + { + hr = parse_midi_track(This, stream, division); + if (FAILED(hr)) + break; + } + + TRACE("Overall MIDI file length %lu\n", This->header.mtLength); + + return hr; +} + static inline struct segment *impl_from_IPersistStream(IPersistStream *iface) { return CONTAINING_RECORD(iface, struct segment, dmobj.IPersistStream_iface); @@ -807,7 +928,7 @@ 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 = parse_midi(This, stream); break;
case MAKE_IDTYPE(FOURCC_RIFF, mmioFOURCC('W','A','V','E')):
From: Yuxuan Shui yshui@codeweavers.com
And use the tempo information for time keeping. --- dlls/dmime/miditracks.c | 145 ++++++++++++++++++++++++++++------------ 1 file changed, 103 insertions(+), 42 deletions(-)
diff --git a/dlls/dmime/miditracks.c b/dlls/dmime/miditracks.c index 84c59928e14..d556b20a186 100644 --- a/dlls/dmime/miditracks.c +++ b/dlls/dmime/miditracks.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; }; @@ -233,12 +234,8 @@ static HRESULT WINAPI midi_sequence_track_Play( IDirectMusicGraph *graph; HRESULT hr; UINT i; - /* # of quarter notes is the most reasonable common units of time between - * MIDI and other parts of DirectMusic. */ - double number_of_quarter_notes = 0, usec_per_quarter_note = 500000; - /* reference time is in 100 nanoseconds units, or 10 million ticks per - * second. */ - double ref_time = 0; + ULONG ticks = 0; + MUSIC_TIME music_time;
TRACE("(%p, %p, %ld, %ld, %ld, %#lx, %p, %p, %ld)\n", This, state_data, start_time, end_time, time_offset, segment_flags, performance, @@ -260,41 +257,28 @@ static HRESULT WINAPI midi_sequence_track_Play( struct event *item = LIST_ENTRY(cursor, struct event, entry); DMUS_MIDI_PMSG *msg;
+ ticks += item->deltaTime; if (This->ticks_per_quarter_note) - { - ref_time += item->deltaTime * usec_per_quarter_note * 10 / - This->ticks_per_quarter_note; - number_of_quarter_notes += - (double)item->deltaTime / This->ticks_per_quarter_note; - } + music_time = + (ULONGLONG)ticks * DMUS_PPQ / This->ticks_per_quarter_note; else - { - double elapsed = - item->deltaTime * 1000000.0 / This->ticks_per_second; - ref_time += elapsed; - number_of_quarter_notes += elapsed / 10 / usec_per_quarter_note; - } + /* This is the "fake" tempo case, ticks and MUSIC_TIME should mean + * the same thing. */ + music_time = ticks;
- if (item->status == 0xff) - { - hr = handle_meta_events(This, item, &usec_per_quarter_note); - if (hr != S_OK) - break; + if (music_time < start_time || music_time >= end_time) continue; - }
- if ((MUSIC_TIME)number_of_quarter_notes < - (start_time + DMUS_PPQ - 1) / DMUS_PPQ) - continue; - if ((MUSIC_TIME)number_of_quarter_notes >= end_time / DMUS_PPQ) + 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_REFTIME; - msg->rtTime = (REFERENCE_TIME)ref_time; + msg->dwFlags = DMUS_PMSGF_MUSICTIME; + msg->mtTime = music_time; msg->dwPChannel = item->status & 0xf; msg->dwVirtualTrackID = track_id; msg->dwType = DMUS_PMSGT_MIDI; @@ -322,10 +306,51 @@ static HRESULT WINAPI midi_sequence_track_GetParam(IDirectMusicTrack8 *iface, 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, @@ -336,7 +361,32 @@ static HRESULT WINAPI midi_sequence_track_SetParam(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 @@ -344,8 +394,16 @@ 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( @@ -552,11 +610,11 @@ static HRESULT read_midi_event(IStream *stream, struct event *event, 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; - } + if (event->data.integer == 0) + { + WARN("Invalid tempo value 0\n"); + hr = E_FAIL; + } } } } @@ -617,6 +675,7 @@ midi_sequence_track_IPersistStream_Load(IPersistStream *iface, IStream *stream)
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; @@ -645,6 +704,8 @@ midi_sequence_track_IPersistStream_Load(IPersistStream *iface, IStream *stream) 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; }
@@ -676,7 +737,7 @@ MUSIC_TIME get_midiseqtrack_length(IDirectMusicTrack8 *iface) struct event *event; MUSIC_TIME length = 0; LIST_FOR_EACH_ENTRY(event, &This->events, struct event, entry) - length += event->deltaTime; + length += event->deltaTime; return length; }
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..7adc1204b0e 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; + 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); + ok(hr == S_OK, "got %#lx\n", hr); + IDirectMusicLoader8_Release(loader); + 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
The assumption is 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 7adc1204b0e..145c6824154 100644 --- a/dlls/dmime/tests/dmime.c +++ b/dlls/dmime/tests/dmime.c @@ -1556,6 +1556,7 @@ static void test_segment(void)
static void test_midi(void) { + IDirectMusicTrack *track = NULL; IDirectMusicSegment8 *segment; IDirectMusicLoader8 *loader; WCHAR test_mid[MAX_PATH]; @@ -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); IDirectMusicSegment8_Release(segment); }
From: Yuxuan Shui yshui@codeweavers.com
--- dlls/dmband/bandtrack.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/dlls/dmband/bandtrack.c b/dlls/dmband/bandtrack.c index 4756ed194ef..f1e22e96b16 100644 --- a/dlls/dmband/bandtrack.c +++ b/dlls/dmband/bandtrack.c @@ -227,7 +227,17 @@ 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 (!entry) + return E_OUTOFMEMORY; + entry->band = band_param->pBand; + entry->head.lBandTimeLogical = band_param->mtTimePhysical; + 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))
From: Yuxuan Shui yshui@codeweavers.com
So that instrument loading can work. --- dlls/dmime/segment.c | 158 +++++++++++++++++++++++++++++++++++++++ dlls/dmime/tests/dmime.c | 2 +- 2 files changed, 159 insertions(+), 1 deletion(-)
diff --git a/dlls/dmime/segment.c b/dlls/dmime/segment.c index beba1bbd34d..aa39dc1c02f 100644 --- a/dlls/dmime/segment.c +++ b/dlls/dmime/segment.c @@ -60,6 +60,117 @@ struct segment
static struct segment *segment_create(void);
+/* Create a trivial DirectMusicBand. + * + * There is no obvious way to create a DirectMusicBand from outside dmband, + * either we add a wine private interface, or generate a RIFF stream and load + * it. + */ +static HRESULT gen_trivial_band_stream(IStream **out_stream) +{ + const LARGE_INTEGER zero = {.QuadPart = 0}; + const DWORD lbil_size = (sizeof(DMUS_IO_INSTRUMENT) + 8 + 8 + 4) * 128 + 4; + const DWORD dmbd_size = 4 + sizeof(GUID) + 8 + lbil_size + 8; + HRESULT hr; + char buffer[128]; + char *p = buffer; + DWORD data; + IStream *stream; + int i; + + TRACE("(%p)\n", out_stream); + + if (!out_stream) + return E_POINTER; + *out_stream = NULL; + + hr = CreateStreamOnHGlobal(NULL, TRUE, &stream); + if (FAILED(hr)) + return hr; + + /* is the endianness going to be ok? */ + /* Top-level RIFF chunk */ + data = FOURCC_RIFF; + memcpy(p, &data, 4); + p += 4; + memcpy(p, &dmbd_size, 4); + p += 4; + data = DMUS_FOURCC_BAND_FORM; + memcpy(p, &data, 4); + p += 4; + + /* GUID chunk */ + data = DMUS_FOURCC_GUID_CHUNK; + memcpy(p, &data, 4); + p += 4; + data = sizeof(GUID); + memcpy(p, &data, 4); + p += 4; + memcpy(p, &CLSID_DirectMusicSegment, sizeof(GUID)); + p += sizeof(GUID); + + /* Band instruments list */ + data = FOURCC_LIST; + memcpy(p, &data, 4); + p += 4; + memcpy(p, &lbil_size, 4); + p += 4; + data = DMUS_FOURCC_INSTRUMENTS_LIST; + memcpy(p, &data, 4); + p += 4; + + hr = IStream_Write(stream, buffer, p - buffer, NULL); + if (FAILED(hr)) + goto out; + + for (i = 0; i < 128; i++) + { + DMUS_IO_INSTRUMENT instrument = { + .dwPatch = i, + .dwPChannel = 0, + .dwFlags = DMUS_IO_INST_PATCH, + }; + + p = buffer; + + data = FOURCC_LIST; + memcpy(p, &data, 4); + p += 4; + data = sizeof(DMUS_IO_INSTRUMENT) + 8 + 4; + memcpy(p, &data, 4); + p += 4; + data = DMUS_FOURCC_INSTRUMENT_LIST; + memcpy(p, &data, 4); + p += 4; + + data = DMUS_FOURCC_INSTRUMENT_CHUNK; + memcpy(p, &data, 4); + p += 4; + data = sizeof(DMUS_IO_INSTRUMENT); + memcpy(p, &data, 4); + p += 4; + memcpy(p, &instrument, sizeof(DMUS_IO_INSTRUMENT)); + p += sizeof(DMUS_IO_INSTRUMENT); + + hr = IStream_Write(stream, buffer, p - buffer, NULL); + if (FAILED(hr)) + goto out; + } + + hr = IStream_Seek(stream, zero, STREAM_SEEK_SET, NULL); + if (FAILED(hr)) + goto out; + *out_stream = stream; + IStream_AddRef(stream); + +out: + if (FAILED(hr)) + WARN("Failed to create trivial band stream, hr %#lx.\n", hr); + IStream_Release(stream); + return hr; +} + + static inline struct segment *impl_from_IDirectMusicSegment8(IDirectMusicSegment8 *iface) { return CONTAINING_RECORD(iface, struct segment, IDirectMusicSegment8_iface); @@ -839,7 +950,54 @@ static HRESULT parse_midi(struct segment *This, IStream *stream) DWORD length; ULONG i; HRESULT hr; + IStream *band_stream; + IDirectMusicBand *band; + IDirectMusicTrack *bandtrack; + IPersistStream *persist_stream; WORD format, number_of_tracks, division; + DMUS_BAND_PARAM bandparam; + + hr = gen_trivial_band_stream(&band_stream); + if (FAILED(hr)) + return hr; + hr = CoCreateInstance(&CLSID_DirectMusicBand, NULL, CLSCTX_INPROC_SERVER, &IID_IDirectMusicBand, (void **)&band); + if (FAILED(hr)) + { + IStream_Release(band_stream); + return hr; + } + + IDirectMusicBand_QueryInterface(band, &IID_IPersistStream, (void **)&persist_stream); + hr = IPersistStream_Load(persist_stream, band_stream); + IPersistStream_Release(persist_stream); + IStream_Release(band_stream); + if (FAILED(hr)) + { + IDirectMusicBand_Release(band); + return hr; + } + + hr = CoCreateInstance(&CLSID_DirectMusicBandTrack, NULL, CLSCTX_INPROC_SERVER, &IID_IDirectMusicTrack, (void **)&bandtrack); + if (FAILED(hr)) + { + IDirectMusicBand_Release(band); + return hr; + } + + bandparam.mtTimePhysical = 0; + bandparam.pBand = band; + hr = IDirectMusicTrack_SetParam(bandtrack, &GUID_BandParam, 0, &bandparam); + IDirectMusicBand_Release(band); + if (FAILED(hr)) + { + IDirectMusicTrack_Release(bandtrack); + return hr; + } + + segment_append_track(This, bandtrack, 0, 0); + IDirectMusicTrack_Release(bandtrack); + if (FAILED(hr)) + return hr;
/* Skip over the 'MThd' magic number. */ offset.QuadPart = 4; diff --git a/dlls/dmime/tests/dmime.c b/dlls/dmime/tests/dmime.c index 145c6824154..ad7dc05ba2c 100644 --- a/dlls/dmime/tests/dmime.c +++ b/dlls/dmime/tests/dmime.c @@ -1572,7 +1572,7 @@ static void test_midi(void) 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); + ok(hr == S_OK, "unable to get band track from midi file: %#lx\n", hr); if (track) IDirectMusicTrack_Release(track);
And there is also the question of how we are going to smuggle a default soundfont into Proton.
On Wed Jan 31 14:52:43 2024 +0000, Yuxuan Shui wrote:
(@rbernon) I was actually almost done writing the bandtrack generation code when I asked the question about how to do this :sweat_smile: If this is too ugly or dumb I can still change it.
ah, turns out if I give a `HGLOBAL` `IStream` to band track `IPersistStream_Load`, it won't be able to get a dmload from it...
Guess this doesn't work after all.
Rémi Bernon (@rbernon) commented about dlls/dmime/miditracks.c:
+{
- 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
Please add trailing commas, this allow future additions to not modify unrelated lines.
Rémi Bernon (@rbernon) commented about dlls/dmime/miditracks.c:
- 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
Same here and in every other vtables.
Rémi Bernon (@rbernon) commented about dlls/dmime/miditracks.c:
+}
+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)
I'm going to nitpick again but let's start with it so that we can get it out of the way:
1) I find the wrapping used here unusual and not very readable. We have some kind of flexible line length limit, not a very hard 80 char limit like seems to be used here.
2) Long arguments lines are more often wrapped instead of having each arguments on their line.
2) Don't use hungarian notation even if the documented interface method signature does, use snake case and prefer separating words with underscores.
Because of how it looks I'm assuming this was formatted with clang-format, which is okay if that helps, but it's still not going to save you from manually checking for readability and consistency with the rest of the module code.
Tweaking the config will help getting nicer results I think. Here's the configuration I've used for dmusic, which is more or less a WineD3D-like code style module (no space in parentheses, double indent continuation):
``` --- DisableFormat: false SortIncludes: false Language: Cpp IndentWidth: 4 ColumnLimit: 100 BreakBeforeBraces: Allman AlignAfterOpenBracket: DontAlign AllowAllArgumentsOnNextLine: true AllowShortIfStatementsOnASingleLine: AllIfsAndElse AllowShortCaseLabelsOnASingleLine: true AllowShortFunctionsOnASingleLine: None AllowShortLoopsOnASingleLine: true ContinuationIndentWidth: 8 BinPackArguments: true BinPackParameters: true PenaltyBreakString: 1000 PenaltyExcessCharacter: 1 PenaltyIndentedWhitespace: 1 PenaltyBreakAssignment: 10000 PenaltyBreakOpenParenthesis: 10000 PenaltyReturnTypeOnItsOwnLine: 10000 PenaltyBreakBeforeFirstCallParameter: 10000 ... ```
Note that even with this configuration I don't believe clang-format does a super good job, and there's still several places where reformatting the whole file will need manual adjustments.
Rémi Bernon (@rbernon) commented about dlls/dmime/miditracks.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)
In this first change, that function is never called, so all the code you add is essentially dead code.
You should reorder and split the commits to make sure the code is reachable (ideally reached, and tested).
It's usually done by having the tests first in the commit order, then implementing a completed stubbed class, and then implementing the class methods one by one.
Rémi Bernon (@rbernon) commented about dlls/dmime/Makefile.in:
graph.c \ lyricstrack.c \ markertrack.c \
- miditracks.c \
Other files are named *track.c, lets name this `miditrack.c`.
Rémi Bernon (@rbernon) commented about dlls/dmime/miditracks.c:
{ struct {
BYTE byte1;BYTE byte2;BYTE byte3;BYTE byte4;BYTE byte5;
BYTE byte[5];
Please do that as soon as the structure is introduced, no need to change it later again then.
Rémi Bernon (@rbernon) commented about dlls/dmime/miditracks.c:
UINT ticks_per_second; UINT event_count;
- struct event events[];
- struct list events;
Same, make it a list upfront if you decide it's better.
Rémi Bernon (@rbernon) commented about dlls/dmime/segment.c:
- entry = calloc(1, sizeof(*entry));
- if (!entry)
- {
IDirectMusicTrack_Release(track);return E_OUTOFMEMORY;- }
- entry->pTrack = track;
- list_add_tail(&This->tracks, &entry->entry);
- length = get_midiseqtrack_length((IDirectMusicTrack8 *)track);
- if (length > This->header.mtLength)
This->header.mtLength = length;- return S_OK;
+}
+static HRESULT parse_midi(struct segment *This, IStream *stream) +{
I'd suggest to move this helper to the miditrack.c file so that everything MIDI-file related lives there.
This could be a `create_midi_tracks_from_stream` helper that the segment would call, getting an array of track pointers, or `enum_midi_tracks_in_stream` with a callback and user pointer that would call back into a segment helper to append the tracks, whatever is simpler.
Rémi Bernon (@rbernon) commented about dlls/dmime/segment.c:
+}
+static HRESULT parse_midi(struct segment *This, IStream *stream) +{
- LARGE_INTEGER offset;
- DWORD length;
- ULONG i;
- HRESULT hr;
- WORD format, number_of_tracks, division;
- /* Skip over the 'MThd' magic number. */
- offset.QuadPart = 4;
- hr = IStream_Seek(stream, offset, STREAM_SEEK_SET, NULL);
- if (FAILED(hr))
return hr;
I'd suggest adopting the very common Wine pattern for such error handling, which will save a lot of vertical space for otherwise very uninteresting and very unlikely error cases:
```suggestion:-3+0 if (FAILED(hr = IStream_Seek(stream, offset, STREAM_SEEK_SET, NULL))) return hr;
```
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); +extern HRESULT create_midiseqtrack(REFIID riid, void **ppobj, DWORD division);
+MUSIC_TIME get_midiseqtrack_length(IDirectMusicTrack8 *iface);
Fwiw the `create_*` functions are mostly historically named. I don't mind if we keep them with these ugly names (until a cleanup commit to change them all) but I'd prefer that new codes and new helpers to be more readable.
Something like `midi_sequence_track_get_length` as the class is named `midi_sequence_track` would be better IMO.
Rémi Bernon (@rbernon) commented about dlls/dmime/miditracks.c:
{double elapsed =item->deltaTime * 1000000.0 / This->ticks_per_second;ref_time += elapsed;number_of_quarter_notes += elapsed / 10 / usec_per_quarter_note;}
/* This is the "fake" tempo case, ticks and MUSIC_TIME should mean* the same thing. */music_time = ticks;
if (item->status == 0xff){hr = handle_meta_events(This, item, &usec_per_quarter_note);if (hr != S_OK)break;
if (music_time < start_time || music_time >= end_time)
There again, you had something and you rewrite it entirely. This should probably be squashed, and the implementation split in a separate change.
Rémi Bernon (@rbernon) commented about dlls/dmime/tests/dmime.c:
+{
- IDirectMusicSegment8 *segment;
- 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);- ok(hr == S_OK, "got %#lx\n", hr);
- IDirectMusicLoader8_Release(loader);
- IDirectMusicSegment8_Release(segment);
So yeah, IMO this test should come first, as well as the next commit which checks that a band track is created.
What I would perhaps also do, to make it is easy to follow and verify the progression of the implementation, is to add end-to-end tests with the mock performance tool (or synth that you can find in dmsynth tests, the midi messages aren't passed to the tool as I suspect they might), and check that playing that track produces the expected MIDI message sequence.
Then, as the parsing implementation progresses you can remove the todo_wine on the messages that are received.
In order to make it simpler you can do that in lock step with the implementation: use an empty MIDI file at first in the test, implement basic parsing, add a few messages in the MIDI file, implement the parsing for them, etc... until you have the parsing of all the MIDI feature you want.
Rémi Bernon (@rbernon) commented about dlls/dmband/bandtrack.c:
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 (!entry)return E_OUTOFMEMORY;entry->band = band_param->pBand;entry->head.lBandTimeLogical = band_param->mtTimePhysical;entry->head.lBandTimePhysical = band_param->mtTimePhysical;IDirectMusicBand_AddRef(entry->band);list_add_tail(&This->bands, &entry->entry);- }
Is this really what this is supposed to be doing? This would need some tests.
Rémi Bernon (@rbernon) commented about dlls/dmime/segment.c:
static struct segment *segment_create(void);
+/* Create a trivial DirectMusicBand.
- There is no obvious way to create a DirectMusicBand from outside dmband,
- either we add a wine private interface, or generate a RIFF stream and load
- it.
- */
+static HRESULT gen_trivial_band_stream(IStream **out_stream)
Yeah, like we discussed this is one way to do it but IMO it might be cleaner to simply move the dmband/band.c / dmband/bandtrack. files into dmusic so they are usable with the PARENTSRC and add them to the dmime Makefile.in, with helpers to create them declared in a `dmusic_band.h` similar to `dmusic_wave.h`. @mstefani might prefer something else, idk.
On Wed Jan 31 16:16:10 2024 +0000, Rémi Bernon wrote:
Yeah, like we discussed this is one way to do it but IMO it might be cleaner to simply move the dmband/band.c / dmband/bandtrack. files into dmusic so they are usable with the PARENTSRC and add them to the dmime Makefile.in, with helpers to create them declared in a `dmusic_band.h` similar to `dmusic_wave.h`. @mstefani might prefer something else, idk.
Yeah turns out generating a band RIFF isn't doable (see my other comment).
On Wed Jan 31 16:15:58 2024 +0000, Rémi Bernon wrote:
I'm going to nitpick again but let's start with it so that we can get it out of the way:
- I find the wrapping used here unusual and not very readable. We have
some kind of flexible line length limit, not a very hard 80 char limit like seems to be used here. 2) Long arguments lines are more often wrapped instead of having each arguments on their line. 2) Don't use hungarian notation even if the documented interface method signature does, use snake case and prefer separating words with underscores. Because of how it looks I'm assuming this was formatted with clang-format, which is okay if that helps, but it's still not going to save you from manually checking for readability and consistency with the rest of the module code. Tweaking the config will help getting nicer results I think. Here's the configuration I've used for dmusic, which is more or less a WineD3D-like code style module (no space in parentheses, double indent continuation):
--- DisableFormat: false SortIncludes: false Language: Cpp IndentWidth: 4 ColumnLimit: 100 BreakBeforeBraces: Allman AlignAfterOpenBracket: DontAlign AllowAllArgumentsOnNextLine: true AllowShortIfStatementsOnASingleLine: AllIfsAndElse AllowShortCaseLabelsOnASingleLine: true AllowShortFunctionsOnASingleLine: None AllowShortLoopsOnASingleLine: true ContinuationIndentWidth: 8 BinPackArguments: true BinPackParameters: true PenaltyBreakString: 1000 PenaltyExcessCharacter: 1 PenaltyIndentedWhitespace: 1 PenaltyBreakAssignment: 10000 PenaltyBreakOpenParenthesis: 10000 PenaltyReturnTypeOnItsOwnLine: 10000 PenaltyBreakBeforeFirstCallParameter: 10000 ...Note that even with this configuration I don't believe clang-format does a super good job, and there's still several places where reformatting the whole file will need manual adjustments.
Yes, this is indeed formatted by clang-format, thanks for the config.
One annoying thing about clang-format is that it cannot format:
```c static Vtbl vtable = { ... }; ```
it insists on formatting it like this:
```c static Vtbl vtable = { ...}; ```
On Wed Jan 31 16:16:00 2024 +0000, Rémi Bernon wrote:
Please do that as soon as the structure is introduced, no need to change it later again then.
I meant to squash this. I think I just missed it.
-
Rémi Bernon (＠rbernon) -
Yuxuan Shui
-
Yuxuan Shui (＠yshui)