[PATCH v3 0/3] MR9373: dmusic,dmsynth: Reduce memory usage part 2
-- v3: dmsynth: Remove format and sample_count from struct wave. dmsynth: Allow zero-copy access to the sample data. dmusic: Defer releasing IDirectMusicDownload when can_free is FALSE. https://gitlab.winehq.org/wine/wine/-/merge_requests/9373
From: Anton Baskanov <baskanov(a)gmail.com> --- dlls/dmusic/port.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/dlls/dmusic/port.c b/dlls/dmusic/port.c index 84a9e759ab2..1945bf82c4f 100644 --- a/dlls/dmusic/port.c +++ b/dlls/dmusic/port.c @@ -29,6 +29,7 @@ struct download_entry struct list entry; IDirectMusicDownload *download; HANDLE handle; + BOOL can_free; DWORD id; }; @@ -522,6 +523,7 @@ static HRESULT WINAPI synth_port_download_Download(IDirectMusicPortDownload *ifa IDirectMusicDownload_AddRef(download); entry->id = info->dwDLId; entry->handle = handle; + entry->can_free = can_free; list_add_tail(&This->downloads, &entry->entry); } @@ -529,11 +531,19 @@ static HRESULT WINAPI synth_port_download_Download(IDirectMusicPortDownload *ifa return hr; } +static HRESULT CALLBACK unload_callback(HANDLE handle, HANDLE user_data) +{ + IDirectMusicDownload *download = (IDirectMusicDownload *)user_data; + IDirectMusicDownload_Release(download); + return S_OK; +} + static HRESULT WINAPI synth_port_download_Unload(IDirectMusicPortDownload *iface, IDirectMusicDownload *download) { struct synth_port *This = synth_from_IDirectMusicPortDownload(iface); struct download_entry *entry; HANDLE handle = 0; + BOOL can_free; TRACE("(%p/%p)->(%p)\n", iface, This, download); @@ -544,15 +554,21 @@ static HRESULT WINAPI synth_port_download_Unload(IDirectMusicPortDownload *iface if (entry->download == download) { list_remove(&entry->entry); - IDirectMusicDownload_Release(entry->download); handle = entry->handle; + can_free = entry->can_free; free(entry); break; } } if (!handle) return S_OK; - return IDirectMusicSynth_Unload(This->synth, handle, NULL, NULL); + + if (can_free) + { + IDirectMusicDownload_Release(download); + return IDirectMusicSynth_Unload(This->synth, handle, NULL, NULL); + } + return IDirectMusicSynth_Unload(This->synth, handle, unload_callback, download); } static const IDirectMusicPortDownloadVtbl synth_port_download_vtbl = { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9373
From: Anton Baskanov <baskanov(a)gmail.com> --- dlls/dmsynth/synth.c | 30 +++++++++++++++++++----------- dlls/dmsynth/tests/dmsynth.c | 2 +- 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/dlls/dmsynth/synth.c b/dlls/dmsynth/synth.c index cc095a5e89d..5b93ff5da6f 100644 --- a/dlls/dmsynth/synth.c +++ b/dlls/dmsynth/synth.c @@ -219,11 +219,9 @@ struct wave WAVEFORMATEX format; UINT sample_count; - short samples[]; + short *samples; }; -C_ASSERT(sizeof(struct wave) == offsetof(struct wave, samples[0])); - static void wave_addref(struct wave *wave) { InterlockedIncrement(&wave->ref); @@ -237,6 +235,7 @@ static void wave_release(struct wave *wave) if (wave->callback) wave->callback(wave, wave->user_data); delete_fluid_sample(wave->fluid_sample); + free(wave->samples); free(wave); } } @@ -823,6 +822,7 @@ static HRESULT synth_download_wave(struct synth *This, DMUS_DOWNLOADINFO *info, DMUS_WAVEDATA *wave_data = (DMUS_WAVEDATA *)(data + offsets[wave_info->ulWaveDataIdx]); struct wave *wave; UINT sample_count; + short *samples; if (TRACE_ON(dmsynth)) { @@ -841,25 +841,32 @@ static HRESULT synth_download_wave(struct synth *This, DMUS_DOWNLOADINFO *info, if (wave_info->WaveformatEx.wFormatTag != WAVE_FORMAT_PCM) return DMUS_E_NOTPCM; sample_count = wave_data->cbSize / wave_info->WaveformatEx.nBlockAlign; - if (!(wave = calloc(1, offsetof(struct wave, samples[sample_count])))) return E_OUTOFMEMORY; + if (!(wave = calloc(1, sizeof(struct wave)))) return E_OUTOFMEMORY; wave->ref = 1; wave->id = info->dwDLId; wave->format = wave_info->WaveformatEx; wave->sample_count = sample_count; - if (wave_info->WaveformatEx.nBlockAlign == 1) + if (wave_info->WaveformatEx.nBlockAlign == 2) { - while (sample_count--) + samples = (short *)wave_data->byData; + } + else + { + if (!(wave->samples = malloc(sample_count * sizeof(short)))) { - short sample = (wave_data->byData[sample_count] - 0x80) << 8; - wave->samples[sample_count] = sample; + WARN("Failed to allocate FluidSynth sample data\n"); + free(wave); + return FLUID_FAILED; } + samples = wave->samples; } - else if (wave_info->WaveformatEx.nBlockAlign == 2) + + if (wave_info->WaveformatEx.nBlockAlign == 1) { while (sample_count--) { - short sample = ((short *)wave_data->byData)[sample_count]; + short sample = (wave_data->byData[sample_count] - 0x80) << 8; wave->samples[sample_count] = sample; } } @@ -881,7 +888,7 @@ static HRESULT synth_download_wave(struct synth *This, DMUS_DOWNLOADINFO *info, /* Although the doc says there should be 8-frame padding around the data, * FluidSynth doesn't actually require this since version 1.0.8. */ - fluid_sample_set_sound_data(wave->fluid_sample, wave->samples, NULL, wave->sample_count, + fluid_sample_set_sound_data(wave->fluid_sample, samples, NULL, wave->sample_count, wave->format.nSamplesPerSec, FALSE); EnterCriticalSection(&This->cs); @@ -922,6 +929,7 @@ static HRESULT WINAPI synth_Download(IDirectMusicSynth8 *iface, HANDLE *ret_hand case DMUS_DOWNLOADINFO_INSTRUMENT2: return synth_download_instrument(This, info, offsets, data, ret_handle); case DMUS_DOWNLOADINFO_WAVE: + *ret_free = FALSE; return synth_download_wave(This, info, offsets, data, ret_handle); case DMUS_DOWNLOADINFO_WAVEARTICULATION: FIXME("Download type DMUS_DOWNLOADINFO_WAVEARTICULATION not yet supported\n"); diff --git a/dlls/dmsynth/tests/dmsynth.c b/dlls/dmsynth/tests/dmsynth.c index b0dd4d8838b..696eceda612 100644 --- a/dlls/dmsynth/tests/dmsynth.c +++ b/dlls/dmsynth/tests/dmsynth.c @@ -1200,7 +1200,7 @@ static void test_IDirectMusicSynth(void) hr = IDirectMusicSynth_Download(synth, &wave_handle, &wave_download, &can_free); ok(hr == S_OK, "got %#lx\n", hr); ok(!!wave_handle, "got %p\n", wave_handle); - todo_wine ok(can_free == FALSE, "got %u\n", can_free); + ok(can_free == FALSE, "got %u\n", can_free); can_free = 0xdeadbeef; instrument_handle = NULL; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9373
From: Anton Baskanov <baskanov(a)gmail.com> --- dlls/dmsynth/synth.c | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/dlls/dmsynth/synth.c b/dlls/dmsynth/synth.c index 5b93ff5da6f..5a8ae226616 100644 --- a/dlls/dmsynth/synth.c +++ b/dlls/dmsynth/synth.c @@ -216,9 +216,6 @@ struct wave HANDLE user_data; fluid_sample_t *fluid_sample; - - WAVEFORMATEX format; - UINT sample_count; short *samples; }; @@ -823,6 +820,7 @@ static HRESULT synth_download_wave(struct synth *This, DMUS_DOWNLOADINFO *info, struct wave *wave; UINT sample_count; short *samples; + int i; if (TRACE_ON(dmsynth)) { @@ -844,8 +842,6 @@ static HRESULT synth_download_wave(struct synth *This, DMUS_DOWNLOADINFO *info, if (!(wave = calloc(1, sizeof(struct wave)))) return E_OUTOFMEMORY; wave->ref = 1; wave->id = info->dwDLId; - wave->format = wave_info->WaveformatEx; - wave->sample_count = sample_count; if (wave_info->WaveformatEx.nBlockAlign == 2) { @@ -864,18 +860,18 @@ static HRESULT synth_download_wave(struct synth *This, DMUS_DOWNLOADINFO *info, if (wave_info->WaveformatEx.nBlockAlign == 1) { - while (sample_count--) + for (i = 0; i < sample_count; ++i) { - short sample = (wave_data->byData[sample_count] - 0x80) << 8; - wave->samples[sample_count] = sample; + short sample = (wave_data->byData[i] - 0x80) << 8; + wave->samples[i] = sample; } } else if (wave_info->WaveformatEx.nBlockAlign == 4) { - while (sample_count--) + for (i = 0; i < sample_count; ++i) { - short sample = ((UINT *)wave_data->byData)[sample_count] >> 16; - wave->samples[sample_count] = sample; + short sample = ((UINT *)wave_data->byData)[i] >> 16; + wave->samples[i] = sample; } } @@ -888,8 +884,8 @@ static HRESULT synth_download_wave(struct synth *This, DMUS_DOWNLOADINFO *info, /* Although the doc says there should be 8-frame padding around the data, * FluidSynth doesn't actually require this since version 1.0.8. */ - fluid_sample_set_sound_data(wave->fluid_sample, samples, NULL, wave->sample_count, - wave->format.nSamplesPerSec, FALSE); + fluid_sample_set_sound_data(wave->fluid_sample, samples, NULL, sample_count, + wave_info->WaveformatEx.nSamplesPerSec, FALSE); EnterCriticalSection(&This->cs); list_add_tail(&This->waves, &wave->entry); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9373
On Thu Nov 6 04:43:31 2025 +0000, Anton Baskanov wrote:
That's weird, the patch should be a on-op on it's own, as `can_free` is always TRUE at that point. I can't reproduce the failure locally, could you please attach the full test log? Nevermind, I think I figured it out: unload callback was causing a double-free.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9373#note_120987
v3: - Don't pass the unload callback to `IDirectMusicSynth_Unload()` when `can_free` is TRUE. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9373#note_120988
On Thu Nov 6 05:11:42 2025 +0000, Anton Baskanov wrote:
v3: - Don't pass the unload callback to `IDirectMusicSynth_Unload()` when `can_free` is TRUE. Yes, that seems to have been the issue. I can no longer reproduce the failure.
It was 100% reproducible for me with the first patch. But for good measures I've let the tests run in a loop. I'll let @rbernon chime in on the patches before signing off myself. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9373#note_121065
Rémi Bernon (@rbernon) commented about dlls/dmsynth/synth.c:
if (wave_info->WaveformatEx.wFormatTag != WAVE_FORMAT_PCM) return DMUS_E_NOTPCM;
sample_count = wave_data->cbSize / wave_info->WaveformatEx.nBlockAlign; - if (!(wave = calloc(1, offsetof(struct wave, samples[sample_count])))) return E_OUTOFMEMORY; + if (!(wave = calloc(1, sizeof(struct wave)))) return E_OUTOFMEMORY;
You could keep the single allocation, while still using a pointer for the samples (with 0 extra allocation size if the samples don't need to be copied). It would save some error handling and you could only replace the `wave_info->WaveformatEx.nBlockAlign == 2` conditional block below, without having to split it in two (which makes it slightly less obvious that the sample conversion is exclusive with the zero-copy). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9373#note_121072
This merge request was approved by Rémi Bernon. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9373
participants (4)
-
Anton Baskanov -
Anton Baskanov (＠baskanov)
-
Michael Stefaniuc (＠mstefani)
-
Rémi Bernon