Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WiP] SoundTouch 2.1.1 #1907

Closed
wants to merge 3 commits into from
Closed

[WiP] SoundTouch 2.1.1 #1907

wants to merge 3 commits into from

Conversation

uklotzde
Copy link
Contributor

  • Delete a forgotten .tar.bz2 file
  • Update the bundled version from 2.1.0 to 2.1.1

Please note that we are still bundling SoundTouch 2.0.0 for Mixxx 2.2. That doesn't matter as long as the version provided by the (Linux) distribution is newer. Do the Windows and macOS builds use the bundled version? In this case I would propose a backport of this upgrade to both 2.1 and 2.2.

@daschuer
Copy link
Member

This version also introduces run time exceptions. Do we need to add try catch blocks in Mixxx code?
We also need to consider the new Beat detector already merged previously by SoundTouch 2.1.

@uklotzde
Copy link
Contributor Author

The runtime exceptions already affect the Fedora builds, even if we don't update the bundled version. I received an update of the system library just today. This affects all distributions that receive an upgrade.

@daschuer
Copy link
Member

That does not necessarily mean that all users should "suffer" from it.
So we have a urgent "bug" to consider this for all fedora users. Which version do you use by the way?

@daschuer
Copy link
Member

Reading the soundsource code, we do not really have a problem if the library is updated in the distro. Before: assert crash, now unhanded exception crash. Which not happens because we use it correct.

@uklotzde
Copy link
Contributor Author

Runtime exceptions have been introduced with version 2.1. We could disable the runtime exceptions for SoundTouch entirely and thereby restore the legacy behavior. But then exceptions will be substituted by assert() and code from a release build happily continues as if nothing happened. With maybe unknown side effects any time during the following session.

Undefined behavior is even worse than a crash! A crash might stop the music, hopefully early while practicing and not while performing. You should always pre-listen to your tracks before you actually use them. Undefined behavior might torture your sound system and your listeners, at any possible time.

I prefer an early and sudden crash.

@daschuer
Copy link
Member

LGTM. Waiting for CI

@uklotzde
Copy link
Contributor Author

Exceptions are evil. Luckily we get rid of them in Rust ;)

@uklotzde
Copy link
Contributor Author

A successful CI build is crucial, because I haven't tested this locally!

@rryan
Copy link
Member

rryan commented Nov 19, 2018

A successful CI build is crucial, because I haven't tested this locally!

Do our tests even exercise SoundTouch? I don't think so...

@rryan rryan closed this Nov 19, 2018
@rryan
Copy link
Member

rryan commented Nov 19, 2018

Argh, wrong button.

@rryan rryan reopened this Nov 19, 2018
@uklotzde
Copy link
Contributor Author

uklotzde commented Nov 19, 2018

The CI build should mainly very successful compilation. I have not tried to build the sources locally after adding the exception handler.

@rryan
Copy link
Member

rryan commented Nov 19, 2018

On this topic, I've wanted to enable -fno-exceptions for a while, because we don't use them but we don't get the performance benefit of not using them.

@Be-ing
Copy link
Contributor

Be-ing commented Nov 23, 2018

Why are we bundling this library? Would it be better to remove it from our source tree?

@uklotzde
Copy link
Contributor Author

libmixxxminimal.so contains the bundled sources even though a recent system version (2.1.1) is available:

nm libmixxxminimal.so --demangle
...
0000000000074180 r soundtouch::AAFilter::calculateCoeffs()::__PRETTY_FUNCTION__
0000000000074400 r soundtouch::BPMDetect::updateXCorr(int)::__PRETTY_FUNCTION__
...

On the other hand it libmixxxminimal.so depends on the system library:

ldd libmixxxminimal.so | grep -i soundtouch
	libSoundTouch.so.2 => /usr/lib64/libSoundTouch.so.2 (0x00007f1710b29000)

Any ideas?

@uklotzde uklotzde changed the title SoundTouch 2.1.1 [WiP] SoundTouch 2.1.1 Dec 24, 2018
@uklotzde
Copy link
Contributor Author

I'm trying to remove the bundled libSoundTouch. This PR will then be replaced by a new one, if that works out.

@uklotzde
Copy link
Contributor Author

Superseded by #1960.

@uklotzde uklotzde closed this Dec 24, 2018
@uklotzde uklotzde mentioned this pull request Dec 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants