Fix loading of modplug files #4736

snue · 2022-04-30T16:44:53Z

The settings for Modplug decoding are not restored/enforced on Mixxx startup since #3446. With the "default" maximum buffer size initialized to zero, all loading attempts of tracker files are doomed to fail. A round-trip to the settings dialog and hitting the "apply" button on the "Modplug Decoder" page works around the issue currently.

This Pull Request:

Loads and applies modplug configuration when the preferences are created.
Marks SoundSourceModPlug as final to silence a warning about virtual dispatch not working in the destructor.

ronso0 · 2022-04-30T17:36:12Z

I never used modplug, though I wonder how such a bug could have slipped through during review.

The settings for Modplug decoding are currently neither persisted nor restored/enforced on Mixxx startup

The settings are persistent, though they are appearantly not reloaded [Edit in the preferences] until the preferences are opened (calling slotUpdate for each pref page).
The fix is as easy as adding

loadSettings();
applySettings();

below this line

mixxx/src/preferences/dialog/dlgprefmodplug.cpp

Line 42 in 3805948

"http://wiki.openmpt.org/Manual:_Setup/Player")));

so they are restored when the preferences are constructed.

If someone else can confirm the bug, this small fix should go to the 2.3 branch so it goes to 2.3.3
Even though it's just the buffer size here, defaults that are needed in various places could go to src/sources/modplug_defs.h for example?

snue · 2022-04-30T18:02:42Z

The settings are persistent, though they are appearantly not reloaded until the preferences are opened (calling slotUpdate for each pref page).
The fix is as easy as adding

Ah cool! I totally forgot how this works :)
I verified that fixes the issue for me. Will update the PR and try to change the target branch to 2.3.

snue · 2022-05-01T17:54:37Z

If someone else can confirm the bug

@ronso0 if you are just short on tracker files you could replicate e.g. with the one released here.

ronso0 · 2022-05-01T22:13:49Z

I can not confirm the bug, that file is loaded and played fine.
mixxx.cpp takes care of restoring the configuration

mixxx/src/mixxx.cpp

Lines 393 to 399 in 3805948

    
           #ifdef __MODPLUG__ 
        
               // restore the configuration for the modplug library before trying to load a module 
        
               DlgPrefModplug* pModplugPrefs = new DlgPrefModplug(nullptr, pConfig); 
        
               pModplugPrefs->loadSettings(); 
        
               pModplugPrefs->applySettings(); 
        
               delete pModplugPrefs; // not needed anymore 
        
           #endif

How come this is not working for you?

snue · 2022-05-02T09:27:21Z

How come this is not working for you?

Interesting. I could reproduce on ~~2.3 and~~ main and the proposed patch fixed it. I'll check again and report back.

snue · 2022-05-02T10:53:08Z

I retried this and indeed 2.3 is actually good. I can still reproduce the issue on main though. The mixxx.cpp is gone/moved to mixxxmainwindow.cpp there and initialization changed. Looks like that initialization code was dropped accidentally when introducing CoreServices in #3446.

Please let me know how you'd like to proceed. I'll switch the target branch back to main as 2.3 doesn't need fixing. But maybe we want to do initialization like before again instead of utilizing the constructor?

ronso0 · 2022-05-02T21:38:46Z

But maybe we want to do initialization like before again instead of utilizing the constructor?

Sure. I'm not familiar with coreservices, though. Just give it a try!

After this line I guess

mixxx/src/coreservices.cpp

Line 314 in 6f86760

#ifdef __MODPLUG__
    // restore the configuration for the modplug library before trying to load a module
    DlgPrefModplug* pModplugPrefs = new DlgPrefModplug(0,
            m_pCoreServices->getSettingsManager().get());
    pModplugPrefs->loadSettings();
    pModplugPrefs->applySettings();
    delete pModplugPrefs; // not needed anymore
#endif

ronso0 · 2022-06-04T21:23:32Z

ping @snue
Do you still intend to implement the proposed coreservices solution?

snue · 2022-06-05T17:17:09Z

Sure. I didn't find the time recently and figured it's not that pressing as 2.3 isn't affected. I'll try this out and update in the coming days hopefully. Stay tuned.

ronso0 · 2022-06-05T18:24:57Z

sure, I didn't mean to push you ; )

snue · 2022-06-23T15:20:05Z

@ronso0 I finally got around to fix this. This is the "just make it work like before" approach. CoreServices should ideally not rely on any UI widget. But I think for now this temporary preferences widget is better than broken Modplug file support.

ronso0 · 2022-06-23T20:06:55Z

Thank you!

Regarding the 2nd commit, I'm not sufficiently familiar with the soundsource stuff.
@uklotzde or @Swiftb0y Do you mind taking a look?

Swiftb0y · 2022-06-23T20:09:49Z

Making things final should not do any harm if it still builds.

Swiftb0y · 2022-06-23T20:11:23Z

src/coreservices.cpp

+    DlgPrefModplug* pModplugPrefs = new DlgPrefModplug(nullptr, pConfig);
+    pModplugPrefs->loadSettings();
+    pModplugPrefs->applySettings();
+    delete pModplugPrefs; // not needed anymore


does it work if you don't allocate?

Suggested change

DlgPrefModplug* pModplugPrefs = new DlgPrefModplug(nullptr, pConfig);

pModplugPrefs->loadSettings();

pModplugPrefs->applySettings();

delete pModplugPrefs; // not needed anymore

DlgPrefModplug modplugPrefs{nullptr, pConfig};

modplugPrefs.loadSettings();

modplugPrefs.applySettings();

Yes that works as well. I switched to just allocate the temporary widget on the stack.

The settings restoration is currently done in the preferences dialog. This was dropped accidentally with the introduction of coreservices in mixxxdj#3446. Strictly speaking it would be cleaner to not rely on a temporary widget here. For now this enures modplug tracker files can be loaded and play correctly after application startup.

This silences a compiler warning about the virtual close() function call from the destructor bypassing the virtual dispatch.

Swiftb0y · 2022-06-23T22:24:44Z

Please fix the code-style issue. then LGTM

snue · 2022-06-23T22:27:30Z

Please fix the code-style issue. then LGTM

Just spotted and fixed 😉. Thanks.

Swiftb0y

Thanks. LGTM

ronso0 · 2022-06-24T23:16:59Z

Thank you!

Unfortunately I can't test atm with the file you shared due to https://bugs.launchpad.net/mixxx/+bug/1979864

snue · 2022-06-25T19:43:30Z

Thank you!

Unfortunately I can't test atm with the file you shared due to https://bugs.launchpad.net/mixxx/+bug/1979864

Oh, that's an interesting bug. But it's not modplug specific, right? Could happen with other sound files as well?

Regarding modplug files, https://modarchive.org has a wide collection of compatible tracks for download. Of course lots of cheesy music there as well. This one should not kill your ears 😜

ronso0 · 2022-06-25T20:46:15Z

Alright, I worked around the bug and the mod track plays fine.
Thank you for the fix!

ronso0 · 2022-06-25T21:39:06Z

@snue Do you have interest and time to look into an alternative way to initialize the decoder, independet from DlgPrefModplug?

snue · 2022-06-26T12:06:45Z

@snue Do you have interest and time to look into an alternative way to initialize the decoder, independet from DlgPrefModplug?

At least not immediately. I might come up with something in the future but won't complain if someone else does it first.

snue force-pushed the fixup/modplug branch from 8d64d81 to 35c580b Compare April 30, 2022 16:48

snue force-pushed the fixup/modplug branch from 35c580b to cb1a05d Compare April 30, 2022 19:15

github-actions bot added the ui label Apr 30, 2022

snue changed the base branch from main to 2.3 April 30, 2022 19:15

snue changed the base branch from 2.3 to main May 2, 2022 10:55

ronso0 added regression sources and removed ui labels May 2, 2022

snue force-pushed the fixup/modplug branch from cb1a05d to 759f8c1 Compare June 23, 2022 15:16

Swiftb0y reviewed Jun 23, 2022

View reviewed changes

snue force-pushed the fixup/modplug branch from 759f8c1 to ca5f5ff Compare June 23, 2022 22:05

snue added 2 commits June 24, 2022 00:24

modplug: mark soundsource final

911f72c

This silences a compiler warning about the virtual close() function call from the destructor bypassing the virtual dispatch.

snue force-pushed the fixup/modplug branch from ca5f5ff to 911f72c Compare June 23, 2022 22:24

Swiftb0y approved these changes Jun 23, 2022

View reviewed changes

ronso0 merged commit 3d1f82b into mixxxdj:main Jun 25, 2022

mixxxbot mentioned this pull request Aug 24, 2022

utc DEBUG_ASSERT when scanning re-added track file #10758

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix loading of modplug files #4736

Fix loading of modplug files #4736

snue commented Apr 30, 2022 •

edited

Loading

ronso0 commented Apr 30, 2022 •

edited

Loading

snue commented Apr 30, 2022

snue commented May 1, 2022

ronso0 commented May 1, 2022

snue commented May 2, 2022 •

edited

Loading

snue commented May 2, 2022

ronso0 commented May 2, 2022

ronso0 commented Jun 4, 2022

snue commented Jun 5, 2022

ronso0 commented Jun 5, 2022

snue commented Jun 23, 2022

ronso0 commented Jun 23, 2022

Swiftb0y commented Jun 23, 2022

Swiftb0y Jun 23, 2022 •

edited

Loading

snue Jun 23, 2022

Swiftb0y commented Jun 23, 2022

snue commented Jun 23, 2022

Swiftb0y left a comment

ronso0 commented Jun 24, 2022

snue commented Jun 25, 2022

ronso0 commented Jun 25, 2022

ronso0 commented Jun 25, 2022

snue commented Jun 26, 2022

Fix loading of modplug files #4736

Fix loading of modplug files #4736

Conversation

snue commented Apr 30, 2022 • edited Loading

ronso0 commented Apr 30, 2022 • edited Loading

snue commented Apr 30, 2022

snue commented May 1, 2022

ronso0 commented May 1, 2022

snue commented May 2, 2022 • edited Loading

snue commented May 2, 2022

ronso0 commented May 2, 2022

ronso0 commented Jun 4, 2022

snue commented Jun 5, 2022

ronso0 commented Jun 5, 2022

snue commented Jun 23, 2022

ronso0 commented Jun 23, 2022

Swiftb0y commented Jun 23, 2022

Swiftb0y Jun 23, 2022 • edited Loading

Choose a reason for hiding this comment

snue Jun 23, 2022

Choose a reason for hiding this comment

Swiftb0y commented Jun 23, 2022

snue commented Jun 23, 2022

Swiftb0y left a comment

Choose a reason for hiding this comment

ronso0 commented Jun 24, 2022

snue commented Jun 25, 2022

ronso0 commented Jun 25, 2022

ronso0 commented Jun 25, 2022

snue commented Jun 26, 2022

snue commented Apr 30, 2022 •

edited

Loading

ronso0 commented Apr 30, 2022 •

edited

Loading

snue commented May 2, 2022 •

edited

Loading

Swiftb0y Jun 23, 2022 •

edited

Loading