Skip to content

Conversation

@mxmilkiib
Copy link
Contributor

implements issue #9862 by replacing libmodplug with libopenmpt as the preferred tracker module decoder, while maintaining full backward compatibility with existing DSP effects and user preferences.

changes:

  • add SoundSourceOpenMPT class for libopenmpt decoding
  • create TrackerDSP class with ported libmodplug DSP algorithms (reverb, megabass, surround, noise reduction)
  • add FindOpenMPT.cmake for build system integration
  • update CMakeLists.txt to support both libraries (openmpt preferred)
  • modify preferences dialog to configure both decoders
  • register openmpt provider with modplug as fallback

features:

  • superior playback accuracy and format support from libopenmpt
  • all existing DSP effects work identically to libmodplug
  • seamless migration: existing user preferences apply automatically
  • active development and security updates from openmpt project
  • libmodplug remains available as fallback option

technical notes:

  • DSP code ported from libmodplug's public domain implementation
  • both decoders pre-render entire module to RAM for seeking
  • DSP effects applied post-decode for consistency
  • same preferences UI works for both implementations

if(QT_VERSION VERSION_GREATER_EQUAL 6.10)
# from Qt 6.10 GuiPrivate required for QShader/rendergraph (rhi/qshader.h)
list(APPEND QT_EXTRA_COMPONENTS "GuiPrivate")
endif()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already merged. Can you rebase your PR on current main?

git fetch upstream 
git rebase upstream/main 
git push -f 

This way we get rid of these criss cross merges.

CMakeLists.txt Outdated
find_package(Modplug)
default_option(MODPLUG "Modplug module decoder support" "Modplug_FOUND")
default_option(OPENMPT "OpenMPT module decoder support" "OpenMPT_FOUND")
default_option(MODPLUG "Modplug module decoder support (legacy)" "Modplug_FOUND")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should default to the new OpenMPT. Legacy should only be used if explicit requested. I suggest to remove to use OFF for MODPLUG.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clean up the configure log, you should only call find_package(Modplug) when OpenMPT is not found.
This should be come with a warning like "checking for fall back library ... ". Goal is a consistent output of the configure stage.

message(STATUS "libmodplug available as fallback (libopenmpt preferred)")
else()
message(STATUS "using libmodplug for tracker module support")
endif()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. For my understanding libopenmpt fully covers the libmodplug features. Do we know a single file that is not opened by libopenmpt and where libmodplug succeeds?
I suggest to use only one soundsource but keep this option for experimental purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

afaicr, they didn't carry over the DSP because they a) were not good, b) not what the artist intended, and c) a responsibility for the player

} else if (fileType == "mptm") {
return "OpenMPT";
} else {
return "Module";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tree needs to be extended for the new file types.


} // anonymous namespace

// MARK: STATIC MEMBERS
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this comment for?

Copy link
Contributor Author

@mxmilkiib mxmilkiib Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm. its a VS Code minimap marker. I thought I set my rules to not use those for projects that have an official repo that isn't mine, but I guess that phraseology is not clear enough

there are 5 files with "// vim "* formatting strings, I guess they were accidentally included

it's not like they have no worth, especially given how may folk use VS Code derived editors these days, but there was a request to get rid of them

image

Comment on lines +78 to +77
if (m_nXBassDepth > 8)
m_nXBassDepth = 8;
if (m_nXBassDepth < 2)
m_nXBassDepth = 2;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (m_nXBassDepth > 8)
m_nXBassDepth = 8;
if (m_nXBassDepth < 2)
m_nXBassDepth = 2;
if (m_nXBassDepth > 8) {
m_nXBassDepth = 8;
}
if (m_nXBassDepth < 2) {
m_nXBassDepth = 2;
}

Comment on lines +257 to +258
if (++m_nReverbBufferPos >= m_nReverbSize)
m_nReverbBufferPos = 0;
if (++m_nReverbBufferPos2 >= m_nReverbSize2)
m_nReverbBufferPos2 = 0;
if (++m_nReverbBufferPos3 >= m_nReverbSize3)
m_nReverbBufferPos3 = 0;
if (++m_nReverbBufferPos4 >= m_nReverbSize4)
m_nReverbBufferPos4 = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (++m_nReverbBufferPos >= m_nReverbSize)
m_nReverbBufferPos = 0;
if (++m_nReverbBufferPos2 >= m_nReverbSize2)
m_nReverbBufferPos2 = 0;
if (++m_nReverbBufferPos3 >= m_nReverbSize3)
m_nReverbBufferPos3 = 0;
if (++m_nReverbBufferPos4 >= m_nReverbSize4)
m_nReverbBufferPos4 = 0;
if (++m_nReverbBufferPos >= m_nReverbSize) {
m_nReverbBufferPos = 0;
}
if (++m_nReverbBufferPos2 >= m_nReverbSize2) {
m_nReverbBufferPos2 = 0;
}
if (++m_nReverbBufferPos3 >= m_nReverbSize3) {
m_nReverbBufferPos3 = 0;
}
if (++m_nReverbBufferPos4 >= m_nReverbSize4) {
m_nReverbBufferPos4 = 0;
}

Comment on lines +58 to +64
static constexpr int XBASSBUFFERSIZE = 128;
static constexpr int FILTERBUFFERSIZE = 256;
static constexpr int SURROUNDBUFFERSIZE = 8192;
static constexpr int REVERBBUFFERSIZE = 8192;
static constexpr int REVERBBUFFERSIZE2 = 6128;
static constexpr int REVERBBUFFERSIZE3 = 4432;
static constexpr int REVERBBUFFERSIZE4 = 2964;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these constants shoule be renamed to names like kXBassBufferSize

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am afraid having a optional DSP in the SoundSource is foreign to Mixxx.
I would prefer to decode the files bit perfect to be 1:1 comparable with libmodplug.
The DSP part can be added to the effect rack and is then accessable during mixing.

@mxmilkiib
Copy link
Contributor Author

DSP part can be added to the effect rack

as in, add a "Module DSP" effect that has all the same options in it?

implements issue mixxxdj#9862 by replacing libmodplug with libopenmpt as the
preferred tracker module decoder, while maintaining full backward
compatibility with existing DSP effects and user preferences.

changes:
- add SoundSourceOpenMPT class for libopenmpt decoding
- create TrackerDSP class with ported libmodplug DSP algorithms
  (reverb, megabass, surround, noise reduction)
- add FindOpenMPT.cmake for build system integration
- update CMakeLists.txt to support both libraries (openmpt preferred)
- modify preferences dialog to configure both decoders
- register openmpt provider with modplug as fallback

features:
- superior playback accuracy and format support from libopenmpt
- all existing DSP effects work identically to libmodplug
- seamless migration: existing user preferences apply automatically
- active development and security updates from openmpt project
- libmodplug remains available as fallback option

technical notes:
- DSP code ported from libmodplug's public domain implementation
- both decoders pre-render entire module to RAM for seeking
- DSP effects applied post-decode for consistency
- same preferences UI works for both implementations
@mxmilkiib mxmilkiib force-pushed the feature/replace-libmodplug-with-libopenmpt branch from 8380a19 to da80f04 Compare October 25, 2025 02:46
@mxmilkiib
Copy link
Contributor Author

mxmilkiib commented Oct 25, 2025

test fail on Windows only, hmm

https://gist.github.com/mxmilkiib/78e10285885c5bff3289e4042d1de5bc

long story short, the AI isn't certain but proposes a small change
the diagnosis is not conclusive or certain, so I wouldn't trust anything after it gets ambiguous
though I should probably let it run its course to see what happens

The test creates an empty QImage dummyFrame; and expects it to produce an empty QByteArray(), but on Windows, an empty QImage may have different behavior for constBits() and sizeInBytes() compared to other platforms.
...
Let me propose a fix: The test should use testing::_ (any argument) instead of QByteArray() for the second parameter, OR it should create a proper QImage with actual dimensions so the byte array isn't ambiguous.
...
the screenWillSentRawDataIfConfigured test was timing out on windows due to a strict mock expectation requiring an exact empty QByteArray(). an empty QImage may have platform-specific behavior for constBits() and sizeInBytes(), causing the created QByteArray to not match the expected empty one on windows.

changed EXPECT_CALL to use wildcard matcher (_) for the frame data parameter, since the test is verifying that the method is called, not the exact content of the byte array for an empty/null image."`*

my bet would be on a one-off test fail, or something else, even though this should be deterministic, but I am ignorant

edit: gonna see if it reproduces, then try that "fix"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants