[PATCH v3 0/7] MR4339: dmsynth, dmloader: Fix memory leaks.
-- v3: dmloader: Mark cached objects as loaded. dmsynth: Don't leak modulators. dmsynth: Free the allocated presets manually. https://gitlab.winehq.org/wine/wine/-/merge_requests/4339
From: Anton Baskanov <baskanov(a)gmail.com> --- dlls/dmsynth/synth.c | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/dlls/dmsynth/synth.c b/dlls/dmsynth/synth.c index a4196f29a8a..d8acc76d3f0 100644 --- a/dlls/dmsynth/synth.c +++ b/dlls/dmsynth/synth.c @@ -320,6 +320,13 @@ struct event BYTE midi[3]; }; +struct voice +{ + struct list entry; + fluid_voice_t *fluid_voice; + struct wave *wave; +}; + struct synth { IDirectMusicSynth8 IDirectMusicSynth8_iface; @@ -336,6 +343,7 @@ struct synth struct list instruments; struct list waves; struct list events; + struct list voices; fluid_settings_t *fluid_settings; fluid_sfont_t *fluid_sfont; @@ -397,6 +405,7 @@ static ULONG WINAPI synth_Release(IDirectMusicSynth8 *iface) { struct instrument *instrument; struct event *event; + struct voice *voice; struct wave *wave; void *next; @@ -418,6 +427,14 @@ static ULONG WINAPI synth_Release(IDirectMusicSynth8 *iface) free(event); } + LIST_FOR_EACH_ENTRY_SAFE(voice, next, &This->voices, struct voice, entry) + { + list_remove(&voice->entry); + if (voice->wave) + wave_release(voice->wave); + free(voice); + } + fluid_sfont_set_data(This->fluid_sfont, NULL); delete_fluid_sfont(This->fluid_sfont); This->fluid_sfont = NULL; @@ -1742,6 +1759,7 @@ static int synth_preset_noteon(fluid_preset_t *fluid_preset, fluid_synth_t *flui { struct articulation *articulation; struct wave *wave = region->wave; + struct voice *voice; if (key < region->key_range.usLow || key > region->key_range.usHigh) continue; if (vel < region->vel_range.usLow || vel > region->vel_range.usHigh) continue; @@ -1763,6 +1781,28 @@ static int synth_preset_noteon(fluid_preset_t *fluid_preset, fluid_synth_t *flui return FLUID_FAILED; } + LIST_FOR_EACH_ENTRY(voice, &synth->voices, struct voice, entry) + { + if (voice->fluid_voice == fluid_voice) + break; + } + + if (&voice->entry == &synth->voices) + { + if (!(voice = calloc(1, sizeof(struct voice)))) + { + delete_fluid_sample(fluid_sample); + return FLUID_FAILED; + } + voice->fluid_voice = fluid_voice; + list_add_tail(&synth->voices, &voice->entry); + } + + if (voice->wave) + wave_release(voice->wave); + voice->wave = wave; + wave_addref(voice->wave); + set_default_voice_connections(fluid_voice); if (region->wave_sample.cSampleLoops) { @@ -1885,6 +1925,7 @@ HRESULT synth_create(IUnknown **ret_iface) list_init(&obj->instruments); list_init(&obj->waves); list_init(&obj->events); + list_init(&obj->voices); if (!(obj->fluid_settings = new_fluid_settings())) goto failed; if (!(obj->fluid_sfont = new_fluid_sfont(synth_sfont_get_name, synth_sfont_get_preset, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4339
From: Anton Baskanov <baskanov(a)gmail.com> --- dlls/dmsynth/synth.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dlls/dmsynth/synth.c b/dlls/dmsynth/synth.c index d8acc76d3f0..4bb3900998d 100644 --- a/dlls/dmsynth/synth.c +++ b/dlls/dmsynth/synth.c @@ -1738,7 +1738,6 @@ static void set_default_voice_connections(fluid_voice_t *fluid_voice) fluid_voice_gen_set(fluid_voice, GEN_KEYNUM, -1.); fluid_voice_gen_set(fluid_voice, GEN_VELOCITY, -1.); fluid_voice_gen_set(fluid_voice, GEN_SCALETUNE, 100.0); - fluid_voice_gen_set(fluid_voice, GEN_OVERRIDEROOTKEY, -1.); add_voice_connections(fluid_voice, &list, connections); } @@ -1772,7 +1771,6 @@ static int synth_preset_noteon(fluid_preset_t *fluid_preset, fluid_synth_t *flui fluid_sample_set_sound_data(fluid_sample, wave->samples, NULL, wave->sample_count, wave->format.nSamplesPerSec, TRUE); - fluid_sample_set_pitch(fluid_sample, region->wave_sample.usUnityNote, region->wave_sample.sFineTune); if (!(fluid_voice = fluid_synth_alloc_voice(synth->fluid_synth, fluid_sample, chan, key, vel))) { @@ -1821,6 +1819,8 @@ static int synth_preset_noteon(fluid_preset_t *fluid_preset, fluid_synth_t *flui fluid_voice_gen_set(fluid_voice, GEN_STARTLOOPADDROFS, 8 + loop->ulStart); fluid_voice_gen_set(fluid_voice, GEN_ENDLOOPADDROFS, 8 + loop->ulStart + loop->ulLength); } + fluid_voice_gen_set(fluid_voice, GEN_OVERRIDEROOTKEY, region->wave_sample.usUnityNote); + fluid_voice_gen_set(fluid_voice, GEN_FINETUNE, region->wave_sample.sFineTune); LIST_FOR_EACH_ENTRY(articulation, &instrument->articulations, struct articulation, entry) add_voice_connections(fluid_voice, &articulation->list, articulation->connections); LIST_FOR_EACH_ENTRY(articulation, ®ion->articulations, struct articulation, entry) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4339
From: Anton Baskanov <baskanov(a)gmail.com> --- dlls/dmsynth/synth.c | 34 ++++++++++++++++++---------------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/dlls/dmsynth/synth.c b/dlls/dmsynth/synth.c index 4bb3900998d..2aa872c07dc 100644 --- a/dlls/dmsynth/synth.c +++ b/dlls/dmsynth/synth.c @@ -209,6 +209,8 @@ struct wave LONG ref; UINT id; + fluid_sample_t *fluid_sample; + WAVEFORMATEX format; UINT sample_count; short samples[]; @@ -224,7 +226,11 @@ static void wave_addref(struct wave *wave) static void wave_release(struct wave *wave) { ULONG ref = InterlockedDecrement(&wave->ref); - if (!ref) free(wave); + if (!ref) + { + delete_fluid_sample(wave->fluid_sample); + free(wave); + } } struct articulation @@ -824,6 +830,16 @@ static HRESULT synth_download_wave(struct synth *This, DMUS_DOWNLOADINFO *info, } } + if (!(wave->fluid_sample = new_fluid_sample())) + { + WARN("Failed to allocate FluidSynth sample\n"); + free(wave); + return FLUID_FAILED; + } + + fluid_sample_set_sound_data(wave->fluid_sample, wave->samples, NULL, wave->sample_count, + wave->format.nSamplesPerSec, TRUE); + EnterCriticalSection(&This->cs); list_add_tail(&This->waves, &wave->entry); LeaveCriticalSection(&This->cs); @@ -1746,7 +1762,6 @@ static int synth_preset_noteon(fluid_preset_t *fluid_preset, fluid_synth_t *flui { struct instrument *instrument = fluid_preset_get_data(fluid_preset); struct synth *synth = instrument->synth; - fluid_sample_t *fluid_sample; fluid_voice_t *fluid_voice; struct region *region; @@ -1763,19 +1778,9 @@ static int synth_preset_noteon(fluid_preset_t *fluid_preset, fluid_synth_t *flui if (key < region->key_range.usLow || key > region->key_range.usHigh) continue; if (vel < region->vel_range.usLow || vel > region->vel_range.usHigh) continue; - if (!(fluid_sample = new_fluid_sample())) - { - WARN("Failed to allocate FluidSynth sample\n"); - return FLUID_FAILED; - } - - fluid_sample_set_sound_data(fluid_sample, wave->samples, NULL, wave->sample_count, - wave->format.nSamplesPerSec, TRUE); - - if (!(fluid_voice = fluid_synth_alloc_voice(synth->fluid_synth, fluid_sample, chan, key, vel))) + if (!(fluid_voice = fluid_synth_alloc_voice(synth->fluid_synth, wave->fluid_sample, chan, key, vel))) { WARN("Failed to allocate FluidSynth voice\n"); - delete_fluid_sample(fluid_sample); return FLUID_FAILED; } @@ -1788,10 +1793,7 @@ static int synth_preset_noteon(fluid_preset_t *fluid_preset, fluid_synth_t *flui if (&voice->entry == &synth->voices) { if (!(voice = calloc(1, sizeof(struct voice)))) - { - delete_fluid_sample(fluid_sample); return FLUID_FAILED; - } voice->fluid_voice = fluid_voice; list_add_tail(&synth->voices, &voice->entry); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4339
From: Anton Baskanov <baskanov(a)gmail.com> We always set the data. --- dlls/dmsynth/synth.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/dlls/dmsynth/synth.c b/dlls/dmsynth/synth.c index 2aa872c07dc..d7e4a037a57 100644 --- a/dlls/dmsynth/synth.c +++ b/dlls/dmsynth/synth.c @@ -1397,7 +1397,6 @@ static int synth_preset_get_num(fluid_preset_t *fluid_preset) TRACE("(%p)\n", fluid_preset); - if (!instrument) return 0; return instrument->patch; } @@ -1767,8 +1766,6 @@ static int synth_preset_noteon(fluid_preset_t *fluid_preset, fluid_synth_t *flui TRACE("(%p, %p, %u, %u, %u)\n", fluid_preset, fluid_synth, chan, key, vel); - if (!instrument) return FLUID_FAILED; - LIST_FOR_EACH_ENTRY(region, &instrument->regions, struct region, entry) { struct articulation *articulation; @@ -1855,8 +1852,6 @@ static fluid_preset_t *synth_sfont_get_preset(fluid_sfont_t *fluid_sfont, int ba TRACE("(%p, %d, %d)\n", fluid_sfont, bank, patch); - if (!synth) return NULL; - EnterCriticalSection(&synth->cs); LIST_FOR_EACH_ENTRY(instrument, &synth->instruments, struct instrument, entry) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4339
From: Anton Baskanov <baskanov(a)gmail.com> FluidSynth never calls synth_preset_free(), causing preset and instrument leaks. --- dlls/dmsynth/synth.c | 69 +++++++++++++++++++++++++++++++++++++------- 1 file changed, 58 insertions(+), 11 deletions(-) diff --git a/dlls/dmsynth/synth.c b/dlls/dmsynth/synth.c index d7e4a037a57..03e180ef9a3 100644 --- a/dlls/dmsynth/synth.c +++ b/dlls/dmsynth/synth.c @@ -319,6 +319,17 @@ static void instrument_release(struct instrument *instrument) } } +struct preset +{ + struct list entry; + int bank; + int patch; + + fluid_preset_t *fluid_preset; + + struct instrument *instrument; +}; + struct event { struct list entry; @@ -350,6 +361,7 @@ struct synth struct list waves; struct list events; struct list voices; + struct list presets; fluid_settings_t *fluid_settings; fluid_sfont_t *fluid_sfont; @@ -410,6 +422,7 @@ static ULONG WINAPI synth_Release(IDirectMusicSynth8 *iface) if (!ref) { struct instrument *instrument; + struct preset *preset; struct event *event; struct voice *voice; struct wave *wave; @@ -441,6 +454,14 @@ static ULONG WINAPI synth_Release(IDirectMusicSynth8 *iface) free(voice); } + LIST_FOR_EACH_ENTRY_SAFE(preset, next, &This->presets, struct preset, entry) + { + list_remove(&preset->entry); + instrument_release(preset->instrument); + delete_fluid_preset(preset->fluid_preset); + free(preset); + } + fluid_sfont_set_data(This->fluid_sfont, NULL); delete_fluid_sfont(This->fluid_sfont); This->fluid_sfont = NULL; @@ -1393,11 +1414,11 @@ static int synth_preset_get_bank(fluid_preset_t *fluid_preset) static int synth_preset_get_num(fluid_preset_t *fluid_preset) { - struct instrument *instrument = fluid_preset_get_data(fluid_preset); + struct preset *preset = fluid_preset_get_data(fluid_preset); TRACE("(%p)\n", fluid_preset); - return instrument->patch; + return preset->patch; } static BOOL gen_from_connection(const CONNECTION *conn, UINT *gen) @@ -1759,7 +1780,8 @@ static void set_default_voice_connections(fluid_voice_t *fluid_voice) static int synth_preset_noteon(fluid_preset_t *fluid_preset, fluid_synth_t *fluid_synth, int chan, int key, int vel) { - struct instrument *instrument = fluid_preset_get_data(fluid_preset); + struct preset *preset = fluid_preset_get_data(fluid_preset); + struct instrument *instrument = preset->instrument; struct synth *synth = instrument->synth; fluid_voice_t *fluid_voice; struct region *region; @@ -1834,9 +1856,6 @@ static int synth_preset_noteon(fluid_preset_t *fluid_preset, fluid_synth_t *flui static void synth_preset_free(fluid_preset_t *fluid_preset) { - struct instrument *instrument = fluid_preset_get_data(fluid_preset); - fluid_preset_set_data(fluid_preset, NULL); - if (instrument) instrument_release(instrument); } static const char *synth_sfont_get_name(fluid_sfont_t *fluid_sfont) @@ -1849,11 +1868,21 @@ static fluid_preset_t *synth_sfont_get_preset(fluid_sfont_t *fluid_sfont, int ba struct synth *synth = fluid_sfont_get_data(fluid_sfont); struct instrument *instrument; fluid_preset_t *fluid_preset; + struct preset *preset; TRACE("(%p, %d, %d)\n", fluid_sfont, bank, patch); EnterCriticalSection(&synth->cs); + LIST_FOR_EACH_ENTRY(preset, &synth->presets, struct preset, entry) + { + if (preset->bank == bank && preset->patch == patch) + { + LeaveCriticalSection(&synth->cs); + return preset->fluid_preset; + } + } + LIST_FOR_EACH_ENTRY(instrument, &synth->instruments, struct instrument, entry) { if (bank == 128 && instrument->patch == (0x80000000 | patch)) break; @@ -1862,18 +1891,35 @@ static fluid_preset_t *synth_sfont_get_preset(fluid_sfont_t *fluid_sfont, int ba if (&instrument->entry == &synth->instruments) { - fluid_preset = NULL; WARN("Could not find instrument with patch %#x\n", patch); + LeaveCriticalSection(&synth->cs); + return NULL; } - else if ((fluid_preset = new_fluid_preset(fluid_sfont, synth_preset_get_name, synth_preset_get_bank, + + if (!(fluid_preset = new_fluid_preset(fluid_sfont, synth_preset_get_name, synth_preset_get_bank, synth_preset_get_num, synth_preset_noteon, synth_preset_free))) { - fluid_preset_set_data(fluid_preset, instrument); - instrument_addref(instrument); + LeaveCriticalSection(&synth->cs); + return NULL; + } - TRACE("Created fluid_preset %p for instrument %p\n", fluid_preset, instrument); + if (!(preset = calloc(1, sizeof(struct preset)))) + { + delete_fluid_preset(fluid_preset); + LeaveCriticalSection(&synth->cs); + return NULL; } + preset->bank = bank; + preset->patch = patch; + preset->fluid_preset = fluid_preset; + preset->instrument = instrument; + fluid_preset_set_data(fluid_preset, preset); + instrument_addref(instrument); + list_add_tail(&synth->presets, &preset->entry); + + TRACE("Created fluid_preset %p for instrument %p\n", fluid_preset, instrument); + LeaveCriticalSection(&synth->cs); return fluid_preset; @@ -1923,6 +1969,7 @@ HRESULT synth_create(IUnknown **ret_iface) list_init(&obj->waves); list_init(&obj->events); list_init(&obj->voices); + list_init(&obj->presets); if (!(obj->fluid_settings = new_fluid_settings())) goto failed; if (!(obj->fluid_sfont = new_fluid_sfont(synth_sfont_get_name, synth_sfont_get_preset, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4339
From: Anton Baskanov <baskanov(a)gmail.com> --- dlls/dmsynth/synth.c | 1 + 1 file changed, 1 insertion(+) diff --git a/dlls/dmsynth/synth.c b/dlls/dmsynth/synth.c index 03e180ef9a3..3204d3865d6 100644 --- a/dlls/dmsynth/synth.c +++ b/dlls/dmsynth/synth.c @@ -1606,6 +1606,7 @@ static void add_mod_from_connection(fluid_voice_t *fluid_voice, const CONNECTION fluid_mod_set_amount(mod, value); fluid_voice_add_mod(fluid_voice, mod, FLUID_VOICE_OVERWRITE); + delete_fluid_mod(mod); } static void add_voice_connections(fluid_voice_t *fluid_voice, const CONNECTIONLIST *list, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4339
From: Anton Baskanov <baskanov(a)gmail.com> Otherwise, the object won't be found in the cache and will leak on release. --- dlls/dmloader/loader.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dlls/dmloader/loader.c b/dlls/dmloader/loader.c index 78b9790dff3..49fae09cc7f 100644 --- a/dlls/dmloader/loader.c +++ b/dlls/dmloader/loader.c @@ -433,12 +433,14 @@ static HRESULT WINAPI loader_GetObject(IDirectMusicLoader8 *iface, DMUS_OBJECTDE pObjectEntry = calloc(1, sizeof(*pObjectEntry)); DM_STRUCT_INIT(&pObjectEntry->Desc); DMUSIC_CopyDescriptor (&pObjectEntry->Desc, &GotDesc); + pObjectEntry->Desc.dwValidData |= DMUS_OBJ_LOADED; pObjectEntry->pObject = pObject; list_add_head(&This->cache, &pObjectEntry->entry); } else { DMUSIC_CopyDescriptor (&pObjectEntry->Desc, &GotDesc); + pObjectEntry->Desc.dwValidData |= DMUS_OBJ_LOADED; pObjectEntry->pObject = pObject; } TRACE(": filled in cache entry\n"); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4339
v2: Moved `struct preset` allocation to simplify the error path a bit. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4339#note_51474
Rémi Bernon (@rbernon) commented about dlls/dmsynth/synth.c:
free(event); }
+ LIST_FOR_EACH_ENTRY_SAFE(voice, next, &This->voices, struct voice, entry) + { + list_remove(&voice->entry); + if (voice->wave)
You can remove the if there, wave cannot be NULL, same below. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4339#note_51476
Rémi Bernon (@rbernon) commented about dlls/dmsynth/synth.c:
free(event); }
+ LIST_FOR_EACH_ENTRY_SAFE(voice, next, &This->voices, struct voice, entry)
That cleanup should probably go in `Close`, after the synth is destroyed? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4339#note_51477
The preset private data is cleared in `synth_preset_free`, before we release its reference to the instrument. It was not completely obvious to me that `synth_preset_free` is guaranteed to be called last, so I wanted to avoid any trouble. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4339#note_51478
On Wed Nov 8 12:26:23 2023 +0000, Rémi Bernon wrote:
The preset private data is cleared in `synth_preset_free`, before we release its reference to the instrument. It was not completely obvious to me that `synth_preset_free` is guaranteed to be called last, so I wanted to avoid any trouble. Eh, looking at next commit I understand now. This is silly that they have a free callback but never call it... looks like the same for the sfont (although we apparently own it).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4339#note_51479
Rémi Bernon (@rbernon) commented about dlls/dmsynth/synth.c:
free(voice); }
+ LIST_FOR_EACH_ENTRY_SAFE(preset, next, &This->presets, struct preset, entry) + { + list_remove(&preset->entry); + instrument_release(preset->instrument); + delete_fluid_preset(preset->fluid_preset); + free(preset); + } +
I think this should also be done on Close, to avoid keeping instruments and waves alive when the synth is destroyed / re-created. Ideally we should not even keep presets alive more than necessary, but I'm not sure how to do that if FluidSynth doesn't give any info on referenced / released presets (maybe we could try to upstream a better mechanism there). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4339#note_51480
participants (3)
-
Anton Baskanov -
Anton Baskanov (＠baskanov)
-
Rémi Bernon