[PATCH v5 0/4] MR4852: Add support from AudioPathConfig

List overview All Threads

newer

older

[PATCH v6 0/4] MR4852: Add support...

[PATCH v5 0/7] MR568:...

Yuxuan Shui (＠yshui)

17 Jan 2024 17 Jan '24

5:05 p.m.

-- v5: dmime: Semi-support creating an audio path from config. dmime: IDirectMusicPerformance::CreateAudioPath should fail when config is NULL. dmime: Parse AudioPathConfig

https://gitlab.winehq.org/wine/wine/-/merge_requests/4852

Show replies by date

Yuxuan Shui

17 Jan 17 Jan

5:05 p.m.

New subject: [PATCH v5 1/4] dmime: AudioPathConfig is not AudioPath.

From: Yuxuan Shui yshui@codeweavers.com

AudioPaths are created from IDirectMusicPerformance::CreateAudioPath and CreateStandardAudioPath, and don't have an IDirectMusicObject or an IPersistStream interface. On the other hand AudioPathConfigs are loaded from files, and do have IDirectMusicObject and IPersistStream.

They were somehow confused with each other and implemented in the same struct, this commit fixes that. --- dlls/dmime/audiopath.c | 101 +++++++++++++++++++++++++++++++------ dlls/dmime/dmime_main.c | 4 +- dlls/dmime/dmime_private.h | 1 + dlls/dmime/tests/dmime.c | 12 ++--- 4 files changed, 93 insertions(+), 25 deletions(-)

diff --git a/dlls/dmime/audiopath.c b/dlls/dmime/audiopath.c index 65691f4705f..aa6babd0c9b 100644 --- a/dlls/dmime/audiopath.c +++ b/dlls/dmime/audiopath.c @@ -23,7 +23,6 @@ WINE_DEFAULT_DEBUG_CHANNEL(dmime);

struct IDirectMusicAudioPathImpl { IDirectMusicAudioPath IDirectMusicAudioPath_iface; - struct dmobject dmobj; LONG ref; IDirectMusicPerformance8* pPerf; IDirectMusicGraph* pToolGraph; @@ -33,14 +32,21 @@ struct IDirectMusicAudioPathImpl { BOOL fActive; };

+struct audio_path_config +{ + IUnknown IUnknown_iface; + struct dmobject dmobj; + LONG ref; +}; + static inline struct IDirectMusicAudioPathImpl *impl_from_IDirectMusicAudioPath(IDirectMusicAudioPath *iface) { return CONTAINING_RECORD(iface, struct IDirectMusicAudioPathImpl, IDirectMusicAudioPath_iface); }

-static inline struct IDirectMusicAudioPathImpl *impl_from_IPersistStream(IPersistStream *iface) +static inline struct audio_path_config *impl_from_IPersistStream(IPersistStream *iface) { - return CONTAINING_RECORD(iface, struct IDirectMusicAudioPathImpl, dmobj.IPersistStream_iface); + return CONTAINING_RECORD(iface, struct audio_path_config, dmobj.IPersistStream_iface); }

void set_audiopath_perf_pointer(IDirectMusicAudioPath *iface, IDirectMusicPerformance8 *performance) @@ -74,10 +80,6 @@ static HRESULT WINAPI IDirectMusicAudioPathImpl_QueryInterface (IDirectMusicAudi

if (IsEqualIID (riid, &IID_IDirectMusicAudioPath) || IsEqualIID (riid, &IID_IUnknown)) *ppobj = &This->IDirectMusicAudioPath_iface; - else if (IsEqualIID (riid, &IID_IDirectMusicObject)) - *ppobj = &This->dmobj.IDirectMusicObject_iface; - else if (IsEqualIID (riid, &IID_IPersistStream)) - *ppobj = &This->dmobj.IPersistStream_iface;

if (*ppobj) { IUnknown_AddRef((IUnknown*)*ppobj); @@ -255,7 +257,7 @@ static const IDirectMusicAudioPathVtbl DirectMusicAudioPathVtbl = { };

/* IDirectMusicObject */ -static HRESULT WINAPI path_IDirectMusicObject_ParseDescriptor(IDirectMusicObject *iface, +static HRESULT WINAPI path_config_IDirectMusicObject_ParseDescriptor(IDirectMusicObject *iface, IStream *stream, DMUS_OBJECTDESC *desc) { struct chunk_entry riff = {0}; @@ -294,13 +296,13 @@ static const IDirectMusicObjectVtbl dmobject_vtbl = { dmobj_IDirectMusicObject_Release, dmobj_IDirectMusicObject_GetDescriptor, dmobj_IDirectMusicObject_SetDescriptor, - path_IDirectMusicObject_ParseDescriptor + path_config_IDirectMusicObject_ParseDescriptor };

/* IPersistStream */ -static HRESULT WINAPI path_IPersistStream_Load(IPersistStream *iface, IStream *stream) +static HRESULT WINAPI path_config_IPersistStream_Load(IPersistStream *iface, IStream *stream) { - struct IDirectMusicAudioPathImpl *This = impl_from_IPersistStream(iface); + struct audio_path_config *This = impl_from_IPersistStream(iface);

FIXME("(%p, %p): Loading not implemented yet\n", This, stream);

@@ -314,11 +316,64 @@ static const IPersistStreamVtbl persiststream_vtbl = { dmobj_IPersistStream_Release, dmobj_IPersistStream_GetClassID, unimpl_IPersistStream_IsDirty, - path_IPersistStream_Load, + path_config_IPersistStream_Load, unimpl_IPersistStream_Save, unimpl_IPersistStream_GetSizeMax };

+static struct audio_path_config *impl_from_IUnknown(IUnknown *iface) +{ + return CONTAINING_RECORD(iface, struct audio_path_config, IUnknown_iface); +} + +static HRESULT WINAPI path_config_IUnknown_QueryInterface(IUnknown *iface, REFIID riid, void **ppobj) +{ + struct audio_path_config *This = impl_from_IUnknown(iface); + + TRACE("(%p, %s, %p)\n", This, debugstr_dmguid(riid), ppobj); + + *ppobj = NULL; + + if (IsEqualIID (riid, &IID_IUnknown)) + *ppobj = &This->IUnknown_iface; + else if (IsEqualIID (riid, &IID_IDirectMusicObject)) + *ppobj = &This->dmobj.IDirectMusicObject_iface; + else if (IsEqualIID (riid, &IID_IPersistStream)) + *ppobj = &This->dmobj.IPersistStream_iface; + + if (*ppobj) + { + IUnknown_AddRef((IUnknown*)*ppobj); + return S_OK; + } + + WARN("(%p, %s, %p): not found\n", This, debugstr_dmguid(riid), ppobj); + return E_NOINTERFACE; +} + +static ULONG WINAPI path_config_IUnknown_AddRef(IUnknown *unk) +{ + struct audio_path_config *This = impl_from_IUnknown(unk); + return InterlockedIncrement(&This->ref); +} + +static ULONG WINAPI path_config_IUnknown_Release(IUnknown *unk) +{ + struct audio_path_config *This = impl_from_IUnknown(unk); + ULONG ref = InterlockedDecrement(&This->ref); + + if (!ref) + free(This); + return ref; +} + +static const IUnknownVtbl path_config_unk_vtbl = +{ + path_config_IUnknown_QueryInterface, + path_config_IUnknown_AddRef, + path_config_IUnknown_Release, +}; + /* for ClassFactory */ HRESULT create_dmaudiopath(REFIID riid, void **ppobj) { @@ -329,12 +384,26 @@ HRESULT create_dmaudiopath(REFIID riid, void **ppobj) if (!(obj = calloc(1, sizeof(*obj)))) return E_OUTOFMEMORY; obj->IDirectMusicAudioPath_iface.lpVtbl = &DirectMusicAudioPathVtbl; obj->ref = 1; - dmobject_init(&obj->dmobj, &CLSID_DirectMusicAudioPathConfig, - (IUnknown *)&obj->IDirectMusicAudioPath_iface); - obj->dmobj.IDirectMusicObject_iface.lpVtbl = &dmobject_vtbl; - obj->dmobj.IPersistStream_iface.lpVtbl = &persiststream_vtbl;

hr = IDirectMusicAudioPath_QueryInterface(&obj->IDirectMusicAudioPath_iface, riid, ppobj); IDirectMusicAudioPath_Release(&obj->IDirectMusicAudioPath_iface); return hr; } + +HRESULT create_dmaudiopath_config(REFIID riid, void **ppobj) +{ + struct audio_path_config *obj; + HRESULT hr; + + *ppobj = NULL; + if (!(obj = calloc(1, sizeof(*obj)))) return E_OUTOFMEMORY; + dmobject_init(&obj->dmobj, &CLSID_DirectMusicAudioPathConfig, &obj->IUnknown_iface); + obj->IUnknown_iface.lpVtbl = &path_config_unk_vtbl; + obj->dmobj.IDirectMusicObject_iface.lpVtbl = &dmobject_vtbl; + obj->dmobj.IPersistStream_iface.lpVtbl = &persiststream_vtbl; + obj->ref = 1; + + hr = IUnknown_QueryInterface(&obj->IUnknown_iface, riid, ppobj); + IUnknown_Release(&obj->IUnknown_iface); + return hr; +} diff --git a/dlls/dmime/dmime_main.c b/dlls/dmime/dmime_main.c index 89e70bcd1bd..05d094fc2a2 100644 --- a/dlls/dmime/dmime_main.c +++ b/dlls/dmime/dmime_main.c @@ -120,7 +120,7 @@ static IClassFactoryImpl ParamControlTrack_CF = {{&classfactory_vtbl}, create_dm static IClassFactoryImpl MarkerTrack_CF = {{&classfactory_vtbl}, create_dmmarkertrack}; static IClassFactoryImpl LyricsTrack_CF = {{&classfactory_vtbl}, create_dmlyricstrack}; static IClassFactoryImpl SegTriggerTrack_CF = {{&classfactory_vtbl}, create_dmsegtriggertrack}; -static IClassFactoryImpl AudioPath_CF = {{&classfactory_vtbl}, create_dmaudiopath}; +static IClassFactoryImpl AudioPathConfig_CF = {{&classfactory_vtbl}, create_dmaudiopath_config}; static IClassFactoryImpl WaveTrack_CF = {{&classfactory_vtbl}, create_dmwavetrack};

@@ -182,7 +182,7 @@ HRESULT WINAPI DllGetClassObject(REFCLSID rclsid, REFIID riid, LPVOID *ppv) return S_OK; } else if (IsEqualCLSID (rclsid, &CLSID_DirectMusicAudioPathConfig) && IsEqualIID (riid, &IID_IClassFactory)) { - *ppv = &AudioPath_CF; + *ppv = &AudioPathConfig_CF; IClassFactory_AddRef((IClassFactory*)*ppv); return S_OK; } else if (IsEqualCLSID (rclsid, &CLSID_DirectMusicWaveTrack) && IsEqualIID (riid, &IID_IClassFactory)) { diff --git a/dlls/dmime/dmime_private.h b/dlls/dmime/dmime_private.h index ed1589ece1a..7b744419c38 100644 --- a/dlls/dmime/dmime_private.h +++ b/dlls/dmime/dmime_private.h @@ -57,6 +57,7 @@ extern HRESULT create_dmsegment(REFIID riid, void **ret_iface); extern HRESULT create_dmsegmentstate(REFIID riid, void **ret_iface); extern HRESULT create_dmgraph(REFIID riid, void **ret_iface); extern HRESULT create_dmaudiopath(REFIID riid, void **ret_iface); +extern HRESULT create_dmaudiopath_config(REFIID riid, void **ret_iface);

extern HRESULT create_dmlyricstrack(REFIID riid, void **ret_iface); extern HRESULT create_dmmarkertrack(REFIID riid, void **ret_iface); diff --git a/dlls/dmime/tests/dmime.c b/dlls/dmime/tests/dmime.c index f5ae845cf97..2b7c744acdb 100644 --- a/dlls/dmime/tests/dmime.c +++ b/dlls/dmime/tests/dmime.c @@ -908,12 +908,10 @@ static void test_COM_audiopath(void)

/* IDirectMusicObject and IPersistStream are not supported */ hr = IDirectMusicAudioPath_QueryInterface(dmap, &IID_IDirectMusicObject, (void**)&unk); - todo_wine ok(FAILED(hr) && !unk, "Unexpected IDirectMusicObject interface: hr=%#lx, iface=%p\n", - hr, unk); + ok(FAILED(hr) && !unk, "Unexpected IDirectMusicObject interface: hr=%#lx, iface=%p\n", hr, unk); if (unk) IUnknown_Release(unk); hr = IDirectMusicAudioPath_QueryInterface(dmap, &IID_IPersistStream, (void**)&unk); - todo_wine ok(FAILED(hr) && !unk, "Unexpected IPersistStream interface: hr=%#lx, iface=%p\n", - hr, unk); + ok(FAILED(hr) && !unk, "Unexpected IPersistStream interface: hr=%#lx, iface=%p\n", hr, unk); if (unk) IUnknown_Release(unk);

/* Same refcount for all DirectMusicAudioPath interfaces */ @@ -988,7 +986,7 @@ static void test_COM_audiopathconfig(void) /* IDirectMusicAudioPath not supported */ hr = CoCreateInstance(&CLSID_DirectMusicAudioPathConfig, NULL, CLSCTX_INPROC_SERVER, &IID_IDirectMusicAudioPath, (void**)&dmap); - todo_wine ok(FAILED(hr) && !dmap, + ok(FAILED(hr) && !dmap, "Unexpected IDirectMusicAudioPath interface: hr=%#lx, iface=%p\n", hr, dmap);

/* IDirectMusicObject and IPersistStream supported */ @@ -1018,8 +1016,8 @@ static void test_COM_audiopathconfig(void)

/* IDirectMusicAudioPath still not supported */ hr = IDirectMusicObject_QueryInterface(dmo, &IID_IDirectMusicAudioPath, (void**)&dmap); - todo_wine ok(FAILED(hr) && !dmap, - "Unexpected IDirectMusicAudioPath interface: hr=%#lx, iface=%p\n", hr, dmap); + ok(FAILED(hr) && !dmap, + "Unexpected IDirectMusicAudioPath interface: hr=%#lx, iface=%p\n", hr, dmap);

while (IDirectMusicObject_Release(dmo)); }

-- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4852

Yuxuan Shui

5:05 p.m.

New subject: [PATCH v5 2/4] dmime: Parse AudioPathConfig

From: Yuxuan Shui yshui@codeweavers.com

--- dlls/dmime/audiopath.c | 279 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 274 insertions(+), 5 deletions(-)

diff --git a/dlls/dmime/audiopath.c b/dlls/dmime/audiopath.c index aa6babd0c9b..235db292b83 100644 --- a/dlls/dmime/audiopath.c +++ b/dlls/dmime/audiopath.c @@ -32,11 +32,31 @@ struct IDirectMusicAudioPathImpl { BOOL fActive; };

static inline struct IDirectMusicAudioPathImpl *impl_from_IDirectMusicAudioPath(IDirectMusicAudioPath *iface) @@ -299,15 +319,249 @@ static const IDirectMusicObjectVtbl dmobject_vtbl = { path_config_IDirectMusicObject_ParseDescriptor };

+static HRESULT parse_pchannel_to_buffers_list(struct audio_path_port_config *This, IStream *stream, const struct chunk_entry *pchl) +{ + struct chunk_entry chunk = { .parent = pchl }; + struct audio_path_pchannel_to_buffer *pchannel_to_buffer = NULL; + DMUS_IO_PCHANNELTOBUFFER_HEADER header; + ULONG bytes, i; + HRESULT hr; + + while ((hr = stream_next_chunk(stream, &chunk)) == S_OK) { + switch (chunk.id) + { + case DMUS_FOURCC_PCHANNELS_ITEM: + pchannel_to_buffer = calloc(1, sizeof(*pchannel_to_buffer)); + if (!pchannel_to_buffer) + return E_OUTOFMEMORY; + + hr = stream_read(stream, &header, sizeof(header)); + if (FAILED(hr)) + goto done; + + TRACE("Got PChannelToBuffer header:\n"); + TRACE("\tdwPChannelCount: %lu\n", header.dwPChannelCount); + TRACE("\tdwPChannelBase: %lu\n", header.dwPChannelBase); + TRACE("\tdwBufferCount: %lu\n", header.dwBufferCount); + TRACE("\tdwFlags: %lx\n", header.dwFlags); + + bytes = chunk.size - sizeof(header); + + if (bytes % sizeof(GUID)) + { + WARN("Invalid size of GUIDs array\n"); + hr = E_FAIL; + goto done; + } + + pchannel_to_buffer = calloc(1, sizeof(*pchannel_to_buffer) + bytes); + if (!pchannel_to_buffer) + { + hr = E_OUTOFMEMORY; + goto done; + } + + pchannel_to_buffer->num_of_guids = bytes / sizeof(GUID); + pchannel_to_buffer->header = header; + TRACE("number of GUIDs: %lu\n", pchannel_to_buffer->num_of_guids); + + hr = stream_read(stream, pchannel_to_buffer->guids, bytes); + if (FAILED(hr)) + goto done; + + for (i = 0; i < pchannel_to_buffer->num_of_guids; i++) + TRACE("\tguids[%lu]: %s\n", i, debugstr_dmguid(&pchannel_to_buffer->guids[i])); + + list_add_tail(&This->pchannel_to_buffer_entries, &pchannel_to_buffer->entry); + pchannel_to_buffer = NULL; + break; + case FOURCC_LIST: + if (chunk.type == DMUS_FOURCC_UNFO_LIST) + TRACE("Unfo list in PChannelToBuffer is ignored\n"); + else + WARN("Unknown %s\n", debugstr_chunk(&chunk)); + break; + case MAKEFOURCC('p','b','g','d'): + FIXME("Undocumented %s\n", debugstr_chunk(&chunk)); + break; + default: + WARN("Unknown %s\n", debugstr_chunk(&chunk)); + break; + } + } +done: + if (FAILED(hr)) { + if (pchannel_to_buffer) + free(pchannel_to_buffer); + return hr; + } + return S_OK; +} + +static void port_config_destroy(struct audio_path_port_config *port_config) +{ + struct audio_path_pchannel_to_buffer *pchannel_to_buffer, *next_pchannel_to_buffer; + LIST_FOR_EACH_ENTRY_SAFE(pchannel_to_buffer, next_pchannel_to_buffer, &port_config->pchannel_to_buffer_entries, + struct audio_path_pchannel_to_buffer, entry) + { + list_remove(&pchannel_to_buffer->entry); + free(pchannel_to_buffer); + } + free(port_config); +} + +static HRESULT parse_port_config_list(struct audio_path_config *This, IStream *stream, const struct chunk_entry *pcfl) +{ + struct chunk_entry chunk = { .parent = pcfl }; + struct audio_path_port_config *port_config = calloc(1, sizeof(*port_config)); + HRESULT hr; + + if (!port_config) + return E_OUTOFMEMORY; + + list_init(&port_config->pchannel_to_buffer_entries); + + while ((hr = stream_next_chunk(stream, &chunk)) == S_OK) { + switch (chunk.id) + { + case DMUS_FOURCC_PORTCONFIG_ITEM: + hr = stream_chunk_get_data(stream, &chunk, &port_config->header, sizeof(port_config->header)); + if (FAILED(hr)) + break; + TRACE("Got PortConfig header:\n"); + TRACE("\tdwPChannelBase: %lu\n", port_config->header.dwPChannelBase); + TRACE("\tdwPChannelCount: %lu\n", port_config->header.dwPChannelCount); + TRACE("\tdwFlags: %lx\n", port_config->header.dwFlags); + TRACE("\tguidPort: %s\n", debugstr_dmguid(&port_config->header.guidPort)); + break; + case DMUS_FOURCC_PORTPARAMS_ITEM: + hr = stream_chunk_get_data(stream, &chunk, &port_config->params, sizeof(port_config->params)); + if (FAILED(hr)) + break; + TRACE("Got PortConfig params:\n"); + TRACE("\tdwSize: %lu\n", port_config->params.dwSize); + TRACE("\tdwValidParams: %lx\n", port_config->params.dwValidParams); + if (port_config->params.dwValidParams & DMUS_PORTPARAMS_VOICES) + TRACE("\tvoices: %lu\n", port_config->params.dwVoices); + if (port_config->params.dwValidParams & DMUS_PORTPARAMS_CHANNELGROUPS) + TRACE("\tchannel groups: %lu\n", port_config->params.dwChannelGroups); + if (port_config->params.dwValidParams & DMUS_PORTPARAMS_AUDIOCHANNELS) + TRACE("\taudio channels: %lu\n", port_config->params.dwAudioChannels); + if (port_config->params.dwValidParams & DMUS_PORTPARAMS_SAMPLERATE) + TRACE("\tsample rate: %lu\n", port_config->params.dwSampleRate); + if (port_config->params.dwValidParams & DMUS_PORTPARAMS_EFFECTS) + TRACE("\teffects: %lx\n", port_config->params.dwEffectFlags); + if (port_config->params.dwValidParams & DMUS_PORTPARAMS_SHARE) + TRACE("\tshare: %d\n", port_config->params.fShare); + if (port_config->params.dwValidParams & DMUS_PORTPARAMS_FEATURES) + TRACE("\tfeatures: %lx\n", port_config->params.dwFeatures); + break; + case FOURCC_LIST: + if (chunk.type == DMUS_FOURCC_DSBUFFER_LIST) + FIXME("DirectSound buffer descriptors is not supported\n"); + else if (chunk.type == DMUS_FOURCC_PCHANNELS_LIST) + hr = parse_pchannel_to_buffers_list(port_config, stream, &chunk); + else if (chunk.type == DMUS_FOURCC_UNFO_LIST) + TRACE("Unfo list in PortConfig is ignored\n"); + else + WARN("Unknown %s\n", debugstr_chunk(&chunk)); + break; + default: + WARN("Unknown %s\n", debugstr_chunk(&chunk)); + } + + if (FAILED(hr)) break; + } + + if (FAILED(hr)) { + port_config_destroy(port_config); + return hr; + } + list_add_tail(&This->port_config_entries, &port_config->entry); + return S_OK; +} + +static HRESULT parse_port_configs_list(struct audio_path_config *This, IStream *stream, const struct chunk_entry *pcsl) +{ + struct chunk_entry chunk = { .parent = pcsl }; + HRESULT hr; + + while ((hr = stream_next_chunk(stream, &chunk)) == S_OK) { + switch (MAKE_IDTYPE(chunk.id, chunk.type)) + { + case MAKE_IDTYPE(FOURCC_LIST, DMUS_FOURCC_PORTCONFIG_LIST): + hr = parse_port_config_list(This, stream, &chunk); + if (FAILED(hr)) + return hr; + break; + default: + WARN("Unknown %s\n", debugstr_chunk(&chunk)); + break; + } + } + return SUCCEEDED(hr) ? S_OK : hr; +} + /* IPersistStream */ static HRESULT WINAPI path_config_IPersistStream_Load(IPersistStream *iface, IStream *stream) { struct audio_path_config *This = impl_from_IPersistStream(iface); + struct chunk_entry riff = {0}, chunk = {0}; + HRESULT hr = S_OK; + + FIXME("(%p, %p): Loading\n", This, stream); + + if (!stream) + return E_POINTER; + + hr = stream_get_chunk(stream, &riff); + if (FAILED(hr)) { + WARN("Failed to get chunk %#lx.\n", hr); + return hr; + }

- FIXME("(%p, %p): Loading not implemented yet\n", This, stream); + if (MAKE_IDTYPE(riff.id, riff.type) != MAKE_IDTYPE(FOURCC_RIFF, DMUS_FOURCC_AUDIOPATH_FORM)) + { + WARN("loading failed: unexpected %s\n", debugstr_chunk(&riff)); + return E_UNEXPECTED; + }

- return IDirectMusicObject_ParseDescriptor(&This->dmobj.IDirectMusicObject_iface, stream, - &This->dmobj.desc); + if (FAILED(hr = dmobj_parsedescriptor(stream, &riff, &This->dmobj.desc, + DMUS_OBJ_OBJECT | DMUS_OBJ_VERSION | DMUS_OBJ_NAME | DMUS_OBJ_CATEGORY)) + || FAILED(hr = stream_reset_chunk_data(stream, &riff))) { + WARN("Failed to parse descriptor %#lx.\n", hr); + return hr; + } + This->dmobj.desc.guidClass = CLSID_DirectMusicAudioPathConfig; + This->dmobj.desc.dwValidData |= DMUS_OBJ_CLASS; + + chunk.parent = &riff; + while ((hr = stream_next_chunk(stream, &chunk)) == S_OK) { + switch (chunk.id) { + case FOURCC_LIST: + if (chunk.type == DMUS_FOURCC_PORTCONFIGS_LIST) { + hr = parse_port_configs_list(This, stream, &chunk); + if (FAILED(hr)) + return hr; + } + else if (chunk.type == DMUS_FOURCC_DSBUFFER_LIST) + FIXME("buffer attributes are not supported\n"); + else if (chunk.type == MAKEFOURCC('p','a','p','d')) + FIXME("Undocumented %s\n", debugstr_chunk(&chunk)); + else if (chunk.type != DMUS_FOURCC_UNFO_LIST) + WARN("Unknown %s\n", debugstr_chunk(&chunk)); + break; + case DMUS_FOURCC_GUID_CHUNK: + case DMUS_FOURCC_VERSION_CHUNK: + /* Already parsed in dmobj_parsedescriptor. */ + break; + default: + WARN("Unknown %s\n", debugstr_chunk(&chunk)); + break; + } + } + TRACE("Finished parsing %#lx\n", hr); + return SUCCEEDED(hr) ? S_OK : hr; }

static const IPersistStreamVtbl persiststream_vtbl = { @@ -354,7 +608,11 @@ static HRESULT WINAPI path_config_IUnknown_QueryInterface(IUnknown *iface, REFII static ULONG WINAPI path_config_IUnknown_AddRef(IUnknown *unk) { struct audio_path_config *This = impl_from_IUnknown(unk); - return InterlockedIncrement(&This->ref); + ULONG ref = InterlockedIncrement(&This->ref); + + TRACE("(%p): ref=%ld\n", This, ref); + + return ref; }

static ULONG WINAPI path_config_IUnknown_Release(IUnknown *unk) @@ -362,8 +620,17 @@ static ULONG WINAPI path_config_IUnknown_Release(IUnknown *unk) struct audio_path_config *This = impl_from_IUnknown(unk); ULONG ref = InterlockedDecrement(&This->ref);

- if (!ref) + TRACE("(%p): ref=%ld\n", This, ref); + + if (!ref) { + struct audio_path_port_config *config, *next_config; + LIST_FOR_EACH_ENTRY_SAFE(config, next_config, &This->port_config_entries, struct audio_path_port_config, entry) + { + list_remove(&config->entry); + port_config_destroy(config); + } free(This); + } return ref; }

@@ -403,6 +670,8 @@ HRESULT create_dmaudiopath_config(REFIID riid, void **ppobj) obj->dmobj.IPersistStream_iface.lpVtbl = &persiststream_vtbl; obj->ref = 1;

+ list_init(&obj->port_config_entries); + hr = IUnknown_QueryInterface(&obj->IUnknown_iface, riid, ppobj); IUnknown_Release(&obj->IUnknown_iface); return hr;

-- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4852

Yuxuan Shui

5:05 p.m.

New subject: [PATCH v5 3/4] dmime: IDirectMusicPerformance::CreateAudioPath should fail when config is NULL.

From: Yuxuan Shui yshui@codeweavers.com

--- dlls/dmime/performance.c | 2 +- dlls/dmime/tests/dmime.c | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/dlls/dmime/performance.c b/dlls/dmime/performance.c index bd596750f34..bce5f96352b 100644 --- a/dlls/dmime/performance.c +++ b/dlls/dmime/performance.c @@ -1654,7 +1654,7 @@ static HRESULT WINAPI performance_CreateAudioPath(IDirectMusicPerformance8 *ifac

FIXME("(%p, %p, %d, %p): stub\n", This, pSourceConfig, fActivate, ret_iface);

- if (!ret_iface) return E_POINTER; + if (!ret_iface || !pSourceConfig) return E_POINTER; if (!This->audio_paths_enabled) return DMUS_E_AUDIOPATH_INACTIVE;

create_dmaudiopath(&IID_IDirectMusicAudioPath, (void **)&pPath); diff --git a/dlls/dmime/tests/dmime.c b/dlls/dmime/tests/dmime.c index 2b7c744acdb..e9036511e9d 100644 --- a/dlls/dmime/tests/dmime.c +++ b/dlls/dmime/tests/dmime.c @@ -1256,6 +1256,7 @@ static void test_COM_performance(void) IDirectMusicPerformance *dmp = (IDirectMusicPerformance*)0xdeadbeef; IDirectMusicPerformance *dmp2; IDirectMusicPerformance8 *dmp8; + IDirectMusicAudioPath *dmap = NULL; ULONG refcount; HRESULT hr;

@@ -1284,6 +1285,9 @@ static void test_COM_performance(void) ok (refcount == 3, "refcount == %lu, expected 3\n", refcount); hr = IDirectMusicPerformance_QueryInterface(dmp, &IID_IDirectMusicPerformance8, (void**)&dmp8); ok(hr == S_OK, "QueryInterface for IID_IDirectMusicPerformance8 failed: %#lx\n", hr); + hr = IDirectMusicPerformance8_CreateAudioPath(dmp8, NULL, TRUE, &dmap); + ok(hr == E_POINTER, "Unexpected result from CreateAudioPath: %#lx\n", hr); + ok(dmap == NULL, "Unexpected dmap pointer\n"); refcount = IDirectMusicPerformance_Release(dmp); ok (refcount == 3, "refcount == %lu, expected 3\n", refcount); refcount = IDirectMusicPerformance8_Release(dmp8);

-- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4852

Yuxuan Shui

5:05 p.m.

New subject: [PATCH v5 4/4] dmime: Semi-support creating an audio path from config.

From: Yuxuan Shui yshui@codeweavers.com

--- dlls/dmime/audiopath.c | 38 +---------- dlls/dmime/dmime_private.h | 32 ++++++++++ dlls/dmime/performance.c | 125 ++++++++++++++++++++++++++++++++++++- 3 files changed, 158 insertions(+), 37 deletions(-)

diff --git a/dlls/dmime/audiopath.c b/dlls/dmime/audiopath.c index 235db292b83..eae31f00ea3 100644 --- a/dlls/dmime/audiopath.c +++ b/dlls/dmime/audiopath.c @@ -32,33 +32,6 @@ struct IDirectMusicAudioPathImpl { BOOL fActive; };

-struct audio_path_pchannel_to_buffer -{ - struct list entry; - DMUS_IO_PCHANNELTOBUFFER_HEADER header; - - ULONG num_of_guids; - GUID guids[]; -}; - -struct audio_path_port_config -{ - struct list entry; - DMUS_IO_PORTCONFIG_HEADER header; - DMUS_PORTPARAMS8 params; - - struct list pchannel_to_buffer_entries; -}; - -struct audio_path_config -{ - IUnknown IUnknown_iface; - struct dmobject dmobj; - LONG ref; - - struct list port_config_entries; -}; - static inline struct IDirectMusicAudioPathImpl *impl_from_IDirectMusicAudioPath(IDirectMusicAudioPath *iface) { return CONTAINING_RECORD(iface, struct IDirectMusicAudioPathImpl, IDirectMusicAudioPath_iface); @@ -575,14 +548,9 @@ static const IPersistStreamVtbl persiststream_vtbl = { unimpl_IPersistStream_GetSizeMax };

-static struct audio_path_config *impl_from_IUnknown(IUnknown *iface) -{ - return CONTAINING_RECORD(iface, struct audio_path_config, IUnknown_iface); -} - static HRESULT WINAPI path_config_IUnknown_QueryInterface(IUnknown *iface, REFIID riid, void **ppobj) { - struct audio_path_config *This = impl_from_IUnknown(iface); + struct audio_path_config *This = path_config_from_IUnknown(iface);

TRACE("(%p, %s, %p)\n", This, debugstr_dmguid(riid), ppobj);

@@ -607,7 +575,7 @@ static HRESULT WINAPI path_config_IUnknown_QueryInterface(IUnknown *iface, REFII

static ULONG WINAPI path_config_IUnknown_AddRef(IUnknown *unk) { - struct audio_path_config *This = impl_from_IUnknown(unk); + struct audio_path_config *This = path_config_from_IUnknown(unk); ULONG ref = InterlockedIncrement(&This->ref);

TRACE("(%p): ref=%ld\n", This, ref); @@ -617,7 +585,7 @@ static ULONG WINAPI path_config_IUnknown_AddRef(IUnknown *unk)

static ULONG WINAPI path_config_IUnknown_Release(IUnknown *unk) { - struct audio_path_config *This = impl_from_IUnknown(unk); + struct audio_path_config *This = path_config_from_IUnknown(unk); ULONG ref = InterlockedDecrement(&This->ref);

TRACE("(%p): ref=%ld\n", This, ref); diff --git a/dlls/dmime/dmime_private.h b/dlls/dmime/dmime_private.h index 7b744419c38..b78c0d87939 100644 --- a/dlls/dmime/dmime_private.h +++ b/dlls/dmime/dmime_private.h @@ -105,6 +105,38 @@ typedef struct _DMUS_PRIVATE_TEMPO_PLAY_STATE { DWORD dummy; } DMUS_PRIVATE_TEMPO_PLAY_STATE, *LPDMUS_PRIVATE_TEMPO_PLAY_STATE;

+struct audio_path_pchannel_to_buffer +{ + struct list entry; + DMUS_IO_PCHANNELTOBUFFER_HEADER header; + + ULONG num_of_guids; + GUID guids[]; +}; + +struct audio_path_port_config +{ + struct list entry; + DMUS_IO_PORTCONFIG_HEADER header; + DMUS_PORTPARAMS8 params; + + struct list pchannel_to_buffer_entries; +}; + +struct audio_path_config +{ + IUnknown IUnknown_iface; + struct dmobject dmobj; + LONG ref; + + struct list port_config_entries; +}; + +static inline struct audio_path_config *path_config_from_IUnknown(IUnknown *iface) +{ + return CONTAINING_RECORD(iface, struct audio_path_config, IUnknown_iface); +} + /***************************************************************************** * Misc. */ diff --git a/dlls/dmime/performance.c b/dlls/dmime/performance.c index bce5f96352b..646d3a08013 100644 --- a/dlls/dmime/performance.c +++ b/dlls/dmime/performance.c @@ -1650,17 +1650,138 @@ static HRESULT WINAPI performance_CreateAudioPath(IDirectMusicPerformance8 *ifac IUnknown *pSourceConfig, BOOL fActivate, IDirectMusicAudioPath **ret_iface) { struct performance *This = impl_from_IDirectMusicPerformance8(iface); + struct audio_path_config *audio_path_config; + struct list *first_port_config, *first_pchannel_to_buffer; + struct audio_path_port_config *port_config; + struct audio_path_pchannel_to_buffer *pchannel_to_buffer; IDirectMusicAudioPath *pPath; + IDirectMusicObject *dmo; + IDirectSoundBuffer *buffer, *primary_buffer; + DMUS_OBJECTDESC objDesc; + DMUS_PORTPARAMS8 port_params; + HRESULT hr; + WAVEFORMATEX format; + DSBUFFERDESC desc; + GUID *buffer_guids;

- FIXME("(%p, %p, %d, %p): stub\n", This, pSourceConfig, fActivate, ret_iface); + FIXME("(%p, %p, %d, %p): semi-stub\n", This, pSourceConfig, fActivate, ret_iface);

if (!ret_iface || !pSourceConfig) return E_POINTER; if (!This->audio_paths_enabled) return DMUS_E_AUDIOPATH_INACTIVE;

+ hr = IUnknown_QueryInterface(pSourceConfig, &IID_IDirectMusicObject, (void **)&dmo); + if (FAILED(hr)) + return hr; + + hr = IDirectMusicObject_GetDescriptor(dmo, &objDesc); + IDirectMusicObject_Release(dmo); + if (FAILED(hr)) + return hr; + + if (!IsEqualCLSID(&objDesc.guidClass, &CLSID_DirectMusicAudioPathConfig)) + { + ERR("Unexpected object class %s for source config.\n", debugstr_dmguid(&objDesc.guidClass)); + return E_INVALIDARG; + } + + audio_path_config = path_config_from_IUnknown(pSourceConfig); + first_port_config = list_head(&audio_path_config->port_config_entries); + if (list_next(&audio_path_config->port_config_entries, first_port_config)) + FIXME("Only one port config supported. %p -> %p\n", first_port_config, list_next(&audio_path_config->port_config_entries, first_port_config)); + port_config = LIST_ENTRY(first_port_config, struct audio_path_port_config, entry); + first_pchannel_to_buffer = list_head(&port_config->pchannel_to_buffer_entries); + if (list_next(&port_config->pchannel_to_buffer_entries, first_pchannel_to_buffer)) + FIXME("Only one pchannel to buffer entry supported.\n"); + pchannel_to_buffer = LIST_ENTRY(first_pchannel_to_buffer, struct audio_path_pchannel_to_buffer, entry); + + /* Secondary buffer description */ + memset(&format, 0, sizeof(format)); + format.wFormatTag = WAVE_FORMAT_PCM; + format.nChannels = 1; + format.nSamplesPerSec = 44000; + format.nAvgBytesPerSec = 44000*2; + format.nBlockAlign = 2; + format.wBitsPerSample = 16; + format.cbSize = 0; + + memset(&desc, 0, sizeof(desc)); + desc.dwSize = sizeof(desc); + desc.dwFlags = DSBCAPS_CTRLFX | DSBCAPS_CTRLVOLUME | DSBCAPS_GLOBALFOCUS; + desc.dwBufferBytes = DSBSIZE_MIN; + desc.dwReserved = 0; + desc.lpwfxFormat = &format; + desc.guid3DAlgorithm = GUID_NULL; + + buffer_guids = pchannel_to_buffer->guids; + if (pchannel_to_buffer->num_of_guids == 2) + { + if ((!IsEqualGUID(&buffer_guids[0], &GUID_Buffer_Reverb) && !IsEqualGUID(&buffer_guids[0], &GUID_Buffer_Stereo)) || + (!IsEqualGUID(&buffer_guids[1], &GUID_Buffer_Reverb) && !IsEqualGUID(&buffer_guids[1], &GUID_Buffer_Stereo))) + FIXME("Only a stereo plus reverb buffer is supported\n"); + else + { + desc.dwFlags |= DSBCAPS_CTRLPAN | DSBCAPS_CTRLFREQUENCY; + format.nChannels = 2; + format.nBlockAlign *= 2; + format.nAvgBytesPerSec *=2; + } + } + else if (pchannel_to_buffer->num_of_guids == 1) + { + if (IsEqualGUID(buffer_guids, &GUID_Buffer_Stereo)) + { + desc.dwFlags |= DSBCAPS_CTRLPAN | DSBCAPS_CTRLFREQUENCY; + format.nChannels = 2; + format.nBlockAlign *= 2; + format.nAvgBytesPerSec *=2; + } + else if (IsEqualGUID(buffer_guids, &GUID_Buffer_3D_Dry)) + desc.dwFlags |= DSBCAPS_CTRL3D | DSBCAPS_CTRLFREQUENCY | DSBCAPS_MUTE3DATMAXDISTANCE; + else if (IsEqualGUID(buffer_guids, &GUID_Buffer_Mono)) + desc.dwFlags |= DSBCAPS_CTRLPAN | DSBCAPS_CTRLFREQUENCY; + else + FIXME("Unsupported buffer guid %s\n", debugstr_dmguid(buffer_guids)); + } + else + FIXME("Multiple buffers not supported\n"); + + port_params = port_config->params; + if (!(port_params.dwValidParams & DMUS_PORTPARAMS_CHANNELGROUPS)) + { + port_params.dwValidParams |= DMUS_PORTPARAMS_CHANNELGROUPS; + port_params.dwChannelGroups = (port_config->header.dwPChannelCount + 15) / 16; + } + if (!(port_params.dwValidParams & DMUS_PORTPARAMS_AUDIOCHANNELS)) + { + port_params.dwValidParams |= DMUS_PORTPARAMS_AUDIOCHANNELS; + port_params.dwAudioChannels = format.nChannels; + } + hr = perf_dmport_create(This, &port_config->params); + if (FAILED(hr)) + return hr; + + hr = IDirectSound_CreateSoundBuffer(This->dsound, &desc, &buffer, NULL); + if (FAILED(hr)) + return DSERR_BUFFERLOST; + + /* Update description for creating primary buffer */ + desc.dwFlags |= DSBCAPS_PRIMARYBUFFER; + desc.dwFlags &= ~DSBCAPS_CTRLFX; + desc.dwBufferBytes = 0; + desc.lpwfxFormat = NULL; + + hr = IDirectSound_CreateSoundBuffer(This->dsound, &desc, &primary_buffer, NULL); + if (FAILED(hr)) { + IDirectSoundBuffer_Release(buffer); + return DSERR_BUFFERLOST; + } + create_dmaudiopath(&IID_IDirectMusicAudioPath, (void **)&pPath); set_audiopath_perf_pointer(pPath, iface); + set_audiopath_dsound_buffer(pPath, buffer); + set_audiopath_primary_dsound_buffer(pPath, primary_buffer); + TRACE(" returning IDirectMusicAudioPath interface at %p.\n", *ret_iface);

- /** TODO */ *ret_iface = pPath; return IDirectMusicAudioPath_Activate(*ret_iface, fActivate); }

-- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4852

Yuxuan Shui (＠yshui)

5:08 p.m.

On Wed Jan 17 08:16:29 2024 +0000, Rémi Bernon wrote:

...

There was a lot of struct sharing like that before, I don't think it's a good idea for refactorability. Exposing a few helper functions and manipulating object through their COM interface pointers instead provides better isolation between components.

I considered it when I wrote this, however:

AudioPathConfig doesn't have any COM interfaces defined for it though (besides IDirectMusicObject). Should I add a wine private interface?

Other thing is adding getters is annoying especially when there are linked lists involved. And I feel doing all this for purely internal types doesn't really add much value.

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4852#note_57728

Yuxuan Shui (＠yshui)

5:09 p.m.

I addressed some of the comments. Sorry for the many git pushes, they help me keep track of what changes I've made.

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4852#note_57729

Rémi Bernon (＠rbernon)

10:57 p.m.

Rémi Bernon (@rbernon) commented about dlls/dmime/audiopath.c:

...

 BOOL fActive;
};

+struct audio_path_pchannel_to_buffer +{

struct list entry;

DMUS_IO_PCHANNELTOBUFFER_HEADER header;

ULONG num_of_guids;

GUID guids[];

+};

For flexible array members and to avoid some possible undefined behavior due to struct alignment I usually also add something like that:

``` C_ASSERT(sizeof(struct pool) == offsetof(struct pool, cues[0])); ```

(And if it breaks because of alignment, layout the struct differently or add some explicit padding, but it's rarely needed).

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4852#note_57825

Rémi Bernon (＠rbernon)

10:57 p.m.

Rémi Bernon (@rbernon) commented about dlls/dmime/audiopath.c:

...

 path_config_IDirectMusicObject_ParseDescriptor
};

+static HRESULT parse_pchannel_to_buffers_list(struct audio_path_port_config *This, IStream *stream, const struct chunk_entry *pchl) +{

struct chunk_entry chunk = { .parent = pchl };

struct audio_path_pchannel_to_buffer *pchannel_to_buffer = NULL;

DMUS_IO_PCHANNELTOBUFFER_HEADER header;

ULONG bytes, i;

HRESULT hr;

while ((hr = stream_next_chunk(stream, &chunk)) == S_OK) {

```suggestion:-0+0 while ((hr = stream_next_chunk(stream, &chunk)) == S_OK) { ```

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4852#note_57826

Rémi Bernon (＠rbernon)

10:57 p.m.

Rémi Bernon (@rbernon) commented about dlls/dmime/audiopath.c:

...

}

free(port_config);

+}

+static HRESULT parse_port_config_list(struct audio_path_config *This, IStream *stream, const struct chunk_entry *pcfl) +{
struct chunk_entry chunk = { .parent = pcfl };

struct audio_path_port_config *port_config = calloc(1, sizeof(*port_config));

HRESULT hr;

if (!port_config)
   return E_OUTOFMEMORY;
list_init(&port_config->pchannel_to_buffer_entries);

while ((hr = stream_next_chunk(stream, &chunk)) == S_OK) {

```suggestion:-0+0 while ((hr = stream_next_chunk(stream, &chunk)) == S_OK) { ```

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4852#note_57827

Rémi Bernon (＠rbernon)

10:57 p.m.

Rémi Bernon (@rbernon) commented about dlls/dmime/audiopath.c:

...

}

if (FAILED(hr)) {
   port_config_destroy(port_config);
   return hr;
}

list_add_tail(&This->port_config_entries, &port_config->entry);

return S_OK;
+}

+static HRESULT parse_port_configs_list(struct audio_path_config *This, IStream *stream, const struct chunk_entry *pcsl) +{

struct chunk_entry chunk = { .parent = pcsl };

HRESULT hr;

while ((hr = stream_next_chunk(stream, &chunk)) == S_OK) {

```suggestion:-0+0 while ((hr = stream_next_chunk(stream, &chunk)) == S_OK) { ```

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4852#note_57828

Rémi Bernon (＠rbernon)

10:57 p.m.

Rémi Bernon (@rbernon) commented about dlls/dmime/audiopath.c:

...

/* IPersistStream */ static HRESULT WINAPI path_config_IPersistStream_Load(IPersistStream *iface, IStream *stream) { struct audio_path_config *This = impl_from_IPersistStream(iface);
struct chunk_entry riff = {0}, chunk = {0};

HRESULT hr = S_OK;

FIXME("(%p, %p): Loading\n", This, stream);

if (!stream)
   return E_POINTER;
FIXME("(%p, %p): Loading not implemented yet\n", This, stream);

hr = stream_get_chunk(stream, &riff);

if (FAILED(hr)) {

```suggestion:-0+0 if (FAILED(hr)) { ```

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4852#note_57829

Rémi Bernon (＠rbernon)

10:57 p.m.

Rémi Bernon (@rbernon) commented about dlls/dmime/audiopath.c:

...

FIXME("(%p, %p): Loading not implemented yet\n", This, stream);
hr = stream_get_chunk(stream, &riff);

if (FAILED(hr)) {
   WARN("Failed to get chunk %#lx.\n", hr);
   return hr;
}

if (MAKE_IDTYPE(riff.id, riff.type) != MAKE_IDTYPE(FOURCC_RIFF, DMUS_FOURCC_AUDIOPATH_FORM))

{
   WARN("loading failed: unexpected %s\n", debugstr_chunk(&riff));
   return E_UNEXPECTED;
}

if (FAILED(hr = dmobj_parsedescriptor(stream, &riff, &This->dmobj.desc,
       DMUS_OBJ_OBJECT | DMUS_OBJ_VERSION | DMUS_OBJ_NAME | DMUS_OBJ_CATEGORY))
           || FAILED(hr = stream_reset_chunk_data(stream, &riff))) {

```suggestion:-0+0 || FAILED(hr = stream_reset_chunk_data(stream, &riff))) { ```

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4852#note_57830

Rémi Bernon (＠rbernon)

10:57 p.m.

Rémi Bernon (@rbernon) commented about dlls/dmime/audiopath.c:

...

}

if (FAILED(hr = dmobj_parsedescriptor(stream, &riff, &This->dmobj.desc,
       DMUS_OBJ_OBJECT | DMUS_OBJ_VERSION | DMUS_OBJ_NAME | DMUS_OBJ_CATEGORY))
           || FAILED(hr = stream_reset_chunk_data(stream, &riff))) {
   WARN("Failed to parse descriptor %#lx.\n", hr);
   return hr;
}

This->dmobj.desc.guidClass = CLSID_DirectMusicAudioPathConfig;

This->dmobj.desc.dwValidData |= DMUS_OBJ_CLASS;
return IDirectMusicObject_ParseDescriptor(&This->dmobj.IDirectMusicObject_iface, stream,
       &This->dmobj.desc);
chunk.parent = &riff;

while ((hr = stream_next_chunk(stream, &chunk)) == S_OK) {
   switch (chunk.id) {

```suggestion:-1+0 while ((hr = stream_next_chunk(stream, &chunk)) == S_OK) { switch (chunk.id) { ```

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4852#note_57831

Rémi Bernon (＠rbernon)

10:58 p.m.

Rémi Bernon (@rbernon) commented about dlls/dmime/audiopath.c:

...

if (FAILED(hr = dmobj_parsedescriptor(stream, &riff, &This->dmobj.desc,
       DMUS_OBJ_OBJECT | DMUS_OBJ_VERSION | DMUS_OBJ_NAME | DMUS_OBJ_CATEGORY))
           || FAILED(hr = stream_reset_chunk_data(stream, &riff))) {
   WARN("Failed to parse descriptor %#lx.\n", hr);
   return hr;
}

This->dmobj.desc.guidClass = CLSID_DirectMusicAudioPathConfig;

This->dmobj.desc.dwValidData |= DMUS_OBJ_CLASS;
return IDirectMusicObject_ParseDescriptor(&This->dmobj.IDirectMusicObject_iface, stream,
       &This->dmobj.desc);
chunk.parent = &riff;

while ((hr = stream_next_chunk(stream, &chunk)) == S_OK) {
   switch (chunk.id) {
   case FOURCC_LIST:
       if (chunk.type == DMUS_FOURCC_PORTCONFIGS_LIST) {

```suggestion:-0+0 if (chunk.type == DMUS_FOURCC_PORTCONFIGS_LIST) { ```

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4852#note_57832

Rémi Bernon (＠rbernon)

10:58 p.m.

Rémi Bernon (@rbernon) commented about dlls/dmime/audiopath.c:

...

 struct audio_path_config *This = impl_from_IUnknown(unk);
 ULONG ref = InterlockedDecrement(&This->ref);
if (!ref)

TRACE("(%p): ref=%ld\n", This, ref);

if (!ref) {

```suggestion:-0+0 if (!ref) { ```

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4852#note_57833

Rémi Bernon (＠rbernon)

10:58 p.m.

On Wed Jan 17 16:35:33 2024 +0000, Yuxuan Shui wrote:

...

What about chunks without a type (non-list chunks)?

Their `type` will be 0, so you can have the same case as there is already look at `parse_dls_chunk` for instance.

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4852#note_57834

Rémi Bernon (＠rbernon)

10:58 p.m.

On Wed Jan 17 16:33:09 2024 +0000, Yuxuan Shui wrote:

...

I can't, `!FAILED(hr)` doesn't mean `hr` is `S_OK`. `stream_get_chunk` returns `S_FALSE` for EOF.

I would do something like that, moving the list_add_tail call to the same location where the object ownership is either freed or moved to the list.

``` if (FAILED(hr)) free(pchannel_to_buffer); else list_add_tail(&This->pchannel_to_buffer_entries, &pchannel_to_buffer->entry);

return hr; ```

The called can handle the returned hr the same way up to the topmost level where any FAILED status returned and other success status changed to S_OK.

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4852#note_57835

Rémi Bernon (＠rbernon)

10:58 p.m.

On Wed Jan 17 16:31:24 2024 +0000, Yuxuan Shui wrote:

...

What's your suggestion here?

Elsewhere I used `if (FAILED(hr)) break;` after the switches and `break;` in the error cases. See the recent dmusic/dmime/dmband parsing changes.

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4852#note_57836

Rémi Bernon (＠rbernon)

10:58 p.m.

On Wed Jan 17 16:30:33 2024 +0000, Yuxuan Shui wrote:

...

`pcfl` is referring to the chunk type, it's not hungarian.

Ok, well it definitely looked like it was :) Anyway, I don't think the name really matters here and `parent` seems more consistent with the other similar parsing functions.

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4852#note_57837

Rémi Bernon (＠rbernon)

10:58 p.m.

On Wed Jan 17 17:08:20 2024 +0000, Yuxuan Shui wrote:

...

I considered it when I wrote this, however: AudioPathConfig doesn't have any COM interfaces defined for it though (besides IDirectMusicObject). Should I add a wine private interface? Other thing is adding getters is annoying especially when there are linked lists involved. And I feel doing all this for purely internal types doesn't really add much value.

It could be a couple of private helpers to help performance and audio path config work together, and keep the list iteration an audio path config implementation detail.

For instance, you could have a `audio_path_create_from_config(IUnknown *config, IDirectMusicPerformance8 *performance, IDirectMusicAudioPath **out)` implemented in `audiopath.c` that would iterate the config, create the DirectSound buffers (there's already a `performance_get_dsound` to get the needed dsound pointer)

Then maybe either a `audio_path_get_port_params` that would be called after the creation to initialize the corresponding performance channels, or something the other way that would be called by the audio path creation like `performance_add_audio_path_port(IDirectMusicPerformance8 *iface, DMUS_PORTPARAMS8 *params, ...)`, I'm not completely sure how it is supposed to work.

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4852#note_57838

Rémi Bernon (＠rbernon)

10:58 p.m.

Rémi Bernon (@rbernon) commented about dlls/dmime/performance.c:

...

memset(&format, 0, sizeof(format));

format.wFormatTag = WAVE_FORMAT_PCM;

format.nChannels = 1;

format.nSamplesPerSec = 44000;

format.nAvgBytesPerSec = 44000*2;

format.nBlockAlign = 2;

format.wBitsPerSample = 16;

format.cbSize = 0;

memset(&desc, 0, sizeof(desc));

desc.dwSize = sizeof(desc);

desc.dwFlags = DSBCAPS_CTRLFX | DSBCAPS_CTRLVOLUME | DSBCAPS_GLOBALFOCUS;

desc.dwBufferBytes = DSBSIZE_MIN;

desc.dwReserved = 0;

desc.lpwfxFormat = &format;

desc.guid3DAlgorithm = GUID_NULL;

Imo these static initializations are better done in struct initializers, with designated initialization to make it readable. Anything that's not set will be zero-initialized and can be omitted.

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4852#note_57839

Rémi Bernon (＠rbernon)

10:58 p.m.

Rémi Bernon (@rbernon) commented about dlls/dmime/performance.c:

...

}

else
   FIXME("Multiple buffers not supported\n");
port_params = port_config->params;

if (!(port_params.dwValidParams & DMUS_PORTPARAMS_CHANNELGROUPS))

{
   port_params.dwValidParams |= DMUS_PORTPARAMS_CHANNELGROUPS;
   port_params.dwChannelGroups = (port_config->header.dwPChannelCount + 15) / 16;
}

if (!(port_params.dwValidParams & DMUS_PORTPARAMS_AUDIOCHANNELS))

{
   port_params.dwValidParams |= DMUS_PORTPARAMS_AUDIOCHANNELS;
   port_params.dwAudioChannels = format.nChannels;
}

hr = perf_dmport_create(This, &port_config->params);

Shouldn't this be using `&port_params`?

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4852#note_57840

Yuxuan Shui (＠yshui)

18 Jan 18 Jan

11:54 a.m.

On Wed Jan 17 22:58:15 2024 +0000, Rémi Bernon wrote:

...

Imo these static initializations are better done in struct initializers, with designated initialization to make it readable. Anything that's not set will be zero-initialized and can be omitted.

Yeah, I will do that. These were just copied from `CreateStandardAudioPath`

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4852#note_57892

Yuxuan Shui (＠yshui)

11:59 a.m.

On Wed Jan 17 22:57:55 2024 +0000, Rémi Bernon wrote:

...

For flexible array members and to avoid some possible undefined behavior due to struct alignment I usually also add something like that:
C_ASSERT(sizeof(struct pool) == offsetof(struct pool, cues[0]));
(And if it breaks because of alignment, layout the struct differently or add some explicit padding, but it's rarely needed).

IIRC this is guaranteed by C standard to never fail? Do you have an example where alignment breaks things?

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4852#note_57893

Yuxuan Shui (＠yshui)

12:04 p.m.

On Wed Jan 17 22:58:05 2024 +0000, Rémi Bernon wrote:

...

Ok, well it definitely looked like it was :) Anyway, I don't think the name really matters here and `parent` seems more consistent with the other similar parsing functions.

I think using the chunk type is more common, for example:

```c static HRESULT parse_track_list(struct segment *This, IStream *stream, const struct chunk_entry *trkl) ```

in segment.c

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4852#note_57894