[PATCH v2 0/5] MR9408: dmloader: Reduce memory usage part 3
-- v2: dmloader: Don't use ReleaseObject() in loader_ClearCache(). dmloader: Mark cached objects as loaded. dmloader: Don't remove the default collection from the cache. dmloader: Free the cache entries manually in loader_Release(). dmloader/tests: Add some ClearCache() tests. https://gitlab.winehq.org/wine/wine/-/merge_requests/9408
From: Anton Baskanov <baskanov(a)gmail.com> --- dlls/dmloader/tests/loader.c | 57 ++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/dlls/dmloader/tests/loader.c b/dlls/dmloader/tests/loader.c index 3d68a19c331..ac6878690c3 100644 --- a/dlls/dmloader/tests/loader.c +++ b/dlls/dmloader/tests/loader.c @@ -162,6 +162,62 @@ static void test_release_object(void) IDirectMusicLoader_Release(loader); } +static void test_clear_cache(void) +{ + IDirectMusicCollection *collection; + IDirectMusicSegment *segment; + IDirectMusicLoader8 *loader; + DMUS_OBJECTDESC desc; + ULONG refcount; + HRESULT hr; + + hr = CoCreateInstance(&CLSID_DirectMusicLoader, NULL, CLSCTX_INPROC, &IID_IDirectMusicLoader8, + (void**)&loader); + ok(hr == S_OK, "Couldn't create Loader %#lx\n", hr); + + memset(&desc, 0, sizeof(desc)); + desc.dwSize = sizeof(desc); + desc.dwValidData = DMUS_OBJ_OBJECT | DMUS_OBJ_CLASS; + desc.guidObject = GUID_DefaultGMCollection; + desc.guidClass = CLSID_DirectMusicCollection; + hr = IDirectMusicLoader_GetObject(loader, &desc, &IID_IDirectMusicCollection, (void **)&collection); + ok(hr == S_OK, "GetObject failed with %#lx\n", hr); + + memset(&desc, 0, sizeof(desc)); + desc.dwSize = sizeof(desc); + desc.dwValidData = DMUS_OBJ_CLASS | DMUS_OBJ_MEMORY; + desc.guidClass = CLSID_DirectMusicSegment; + desc.llMemLength = sizeof(rifffile); + desc.pbMemData = rifffile; + segment = NULL; + hr = IDirectMusicLoader_GetObject(loader, &desc, &IID_IDirectMusicSegment, (void **)&segment); + if (FAILED(hr)) + win_skip("GetObject failed with %#lx, skipping segment tests\n", hr); + + hr = IDirectMusicLoader_ClearCache(loader, &GUID_DirectMusicAllTypes); + ok(hr == S_OK, "ClearCache failed with %#lx\n", hr); + + /* ClearCache releases the loaded objects */ + refcount = IDirectMusicCollection_Release(collection); + todo_wine ok(!refcount, "refcount == %lu, expected 0\n", refcount); + if (segment) + { + refcount = IDirectMusicSegment_Release(segment); + todo_wine ok(!refcount, "refcount == %lu, expected 0\n", refcount); + } + + /* ClearCache doesn't remove the default collection */ + memset(&desc, 0, sizeof(desc)); + desc.dwSize = sizeof(desc); + hr = IDirectMusicLoader_EnumObject(loader, &CLSID_DirectMusicCollection, 0, &desc); + ok(hr == S_OK, "EnumObject failed with %#lx\n", hr); + ok(IsEqualGUID(&desc.guidObject, &GUID_DefaultGMCollection), + "Got object guid %s, expected GUID_DefaultGMCollection\n", + wine_dbgstr_guid(&desc.guidObject)); + + IDirectMusicLoader_Release(loader); +} + static void test_simple_playing(void) { IDirectMusic *music = NULL; @@ -615,6 +671,7 @@ START_TEST(loader) test_directory(); test_caching(); test_release_object(); + test_clear_cache(); test_simple_playing(); test_COM(); test_COM_container(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9408
From: Anton Baskanov <baskanov(a)gmail.com> --- dlls/dmloader/loader.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/dlls/dmloader/loader.c b/dlls/dmloader/loader.c index 3798a0851a0..966d6fa1b80 100644 --- a/dlls/dmloader/loader.c +++ b/dlls/dmloader/loader.c @@ -169,9 +169,15 @@ static ULONG WINAPI loader_Release(IDirectMusicLoader8 *iface) TRACE("(%p)->(): new ref = %lu\n", iface, ref); if (!ref) { + struct cache_entry *obj, *obj2; unsigned int i; - IDirectMusicLoader8_ClearCache(iface, &GUID_DirectMusicAllTypes); + LIST_FOR_EACH_ENTRY_SAFE(obj, obj2, &This->cache, struct cache_entry, entry) { + if ((obj->Desc.dwValidData & DMUS_OBJ_LOADED) && obj->pObject) + IDirectMusicObject_Release(obj->pObject); + list_remove(&obj->entry); + free(obj); + } for (i = 0; i < ARRAY_SIZE(classes); i++) free(This->search_paths[i]); free(This); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9408
From: Anton Baskanov <baskanov(a)gmail.com> --- dlls/dmloader/loader.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/dlls/dmloader/loader.c b/dlls/dmloader/loader.c index 966d6fa1b80..bfcc32da49c 100644 --- a/dlls/dmloader/loader.c +++ b/dlls/dmloader/loader.c @@ -749,8 +749,10 @@ static HRESULT WINAPI loader_ClearCache(IDirectMusicLoader8 *iface, REFGUID clas (obj->Desc.dwValidData & DMUS_OBJ_LOADED)) { /* basically, wrap to ReleaseObject for each object found */ IDirectMusicLoader8_ReleaseObject(iface, obj->pObject); - list_remove(&obj->entry); - free(obj); + if (!IsEqualGUID(&GUID_DefaultGMCollection, &obj->Desc.guidObject)) { + list_remove(&obj->entry); + free(obj); + } } } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9408
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 ++ dlls/dmloader/tests/loader.c | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/dlls/dmloader/loader.c b/dlls/dmloader/loader.c index bfcc32da49c..d8bad56c7f2 100644 --- a/dlls/dmloader/loader.c +++ b/dlls/dmloader/loader.c @@ -445,12 +445,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"); diff --git a/dlls/dmloader/tests/loader.c b/dlls/dmloader/tests/loader.c index ac6878690c3..3b6d9a159a8 100644 --- a/dlls/dmloader/tests/loader.c +++ b/dlls/dmloader/tests/loader.c @@ -199,7 +199,7 @@ static void test_clear_cache(void) /* ClearCache releases the loaded objects */ refcount = IDirectMusicCollection_Release(collection); - todo_wine ok(!refcount, "refcount == %lu, expected 0\n", refcount); + ok(!refcount, "refcount == %lu, expected 0\n", refcount); if (segment) { refcount = IDirectMusicSegment_Release(segment); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9408
From: Anton Baskanov <baskanov(a)gmail.com> Some objects have no GUID or name (e.g. segments created from MIDI files), so ReleaseObject() won't find them in the cache. --- dlls/dmloader/loader.c | 7 +++++-- dlls/dmloader/tests/loader.c | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/dlls/dmloader/loader.c b/dlls/dmloader/loader.c index d8bad56c7f2..0291fffc06d 100644 --- a/dlls/dmloader/loader.c +++ b/dlls/dmloader/loader.c @@ -749,8 +749,11 @@ static HRESULT WINAPI loader_ClearCache(IDirectMusicLoader8 *iface, REFGUID clas LIST_FOR_EACH_ENTRY_SAFE(obj, obj2, &This->cache, struct cache_entry, entry) { if ((IsEqualGUID(class, &GUID_DirectMusicAllTypes) || IsEqualGUID(class, &obj->Desc.guidClass)) && (obj->Desc.dwValidData & DMUS_OBJ_LOADED)) { - /* basically, wrap to ReleaseObject for each object found */ - IDirectMusicLoader8_ReleaseObject(iface, obj->pObject); + if (obj->pObject) { + IDirectMusicObject_Release(obj->pObject); + obj->pObject = NULL; + } + obj->Desc.dwValidData &= ~DMUS_OBJ_LOADED; if (!IsEqualGUID(&GUID_DefaultGMCollection, &obj->Desc.guidObject)) { list_remove(&obj->entry); free(obj); diff --git a/dlls/dmloader/tests/loader.c b/dlls/dmloader/tests/loader.c index 3b6d9a159a8..4f7d52e566f 100644 --- a/dlls/dmloader/tests/loader.c +++ b/dlls/dmloader/tests/loader.c @@ -203,7 +203,7 @@ static void test_clear_cache(void) if (segment) { refcount = IDirectMusicSegment_Release(segment); - todo_wine ok(!refcount, "refcount == %lu, expected 0\n", refcount); + ok(!refcount, "refcount == %lu, expected 0\n", refcount); } /* ClearCache doesn't remove the default collection */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9408
On Mon Nov 10 18:36:35 2025 +0000, Anton Baskanov wrote:
changed this line in [version 2 of the diff](/wine/wine/-/merge_requests/9408/diffs?diff_id=222556&start_sha=1be1225057659793a0f34ed19f07e62daacb114c#83f8e68860a62bd956d733ce05dce14badb41232_170_194) Thanks for pointing out, fixed in v2.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9408#note_121391
v2: - Skip the segment tests if `GetObject` fails to load the segment. - Put `ClearCache` tests in a separate function. - Simplify reference count checks. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9408#note_121390
On Mon Nov 10 20:32:32 2025 +0000, Anton Baskanov wrote:
Thanks for pointing out, fixed in v2. Looks good, thanks.
Waiting on @rbernon in the other comment before approving. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9408#note_121405
On Wed Nov 12 20:54:01 2025 +0000, Anton Baskanov wrote:
@rbernon In [!4339 (comment 51683)](https://gitlab.winehq.org/wine/wine/-/merge_requests/4339#note_51683) you mentioned that d20f430c breaks Final Fantasy VIII in-game music. Do you remember what was the issue exactly? I tried to enter and exit locations with different music, and it seems to play fine (tested the Stream version and the demo). Sorry, I don't remember but if you tested and it worked then I think maybe it's been resolved by some other change.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9408#note_121713
This merge request was approved by Michael Stefaniuc. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9408
The test failures are unrelated to this changes. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9408#note_121717
participants (4)
-
Anton Baskanov -
Anton Baskanov (＠baskanov)
-
Michael Stefaniuc (＠mstefani)
-
Rémi Bernon