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

Fix af-tests/soundfont-test on bionic #42

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@swesterfeld
Collaborator

swesterfeld commented Jun 3, 2018

Soundfont audio tests are currently failing on bionic (18.04). As suggested here:

https://mail.gnome.org/archives/beast/2018-May/msg00001.html

I propose a fix that

  • regenerates .ref files to match the 18.04 output
  • uses a lower threshold for old fluidsynth versions as to not break the tests on 16.04,...

I was able to find the corresponding fluidsynth changelog entry, which is (fluidsynth 1.1.7):

which suggests that the new behaviour is caused by a bugfix.

swesterfeld added some commits Jun 3, 2018

BUILD: determine soundfont test threshold from fluidsynth version
Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
AF-TESTS: fix soundfont test for newer (>= 1.1.7) fluidsynth versions
 - regenerate .ref file to match the output of new fluidsynth
 - use lower threshold for old fluidsynth versions

Fluidsynth behaviour changed in version 1.1.7 due to this change:

 - fix calculations for modulators (#194)

Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
@tim-janik

This comment has been minimized.

Owner

tim-janik commented Jun 6, 2018

Hi Stefan, thanks for the PR.

Overall it looks good, there's one thing that can be improved though.
I'd like to keep all thresholds in one place, in the Makefile.sub. I see two ways to accomplish that:

a) Change configure.ac's FLUID_THRESHOLD to a FLUID_RELAXED_THRESHOLD boolean and pick the threshold inside the Makefile based on the boolean.

b) Change configure.ac's FLUID_THRESHOLD to FLUID_VERSION and pick a threshold inside the Makefile based on FLUID_VERSION>=1.1.7.

I guess (a) suffices for now, but (b) would be more future proof in case we have to adjust the threshold ever again. Come to think of it, we could run into future problems with other libraries as well. Scattering the threshold and version checks across Makefile.sub and configure.ac for each should be avoided. I.e. I'd merge either (a) or (b), but have a slight preference for (b) in terms of setting a precedent for future patches.

swesterfeld added some commits Jun 11, 2018

AF-TESTS: add version comparision helper script
Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
BUILD: provide fluidsynth version instead of test threshold to af-tests
Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
AF-TESTS: compute soundfont test threshold based on fluidsynth version
Versions >= 1.1.7 use strict threshold.

Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
@swesterfeld

This comment has been minimized.

Collaborator

swesterfeld commented Jun 11, 2018

I see that it would be nicer to have the threshold next to the actual test. I implemented (b), so the version check is now in af-tests/Makefile.sub, using a new helper script based on sort -V - but I doubt we will have this problem often.

@tim-janik

This comment has been minimized.

Owner

tim-janik commented Jul 25, 2018

Thanks, merged, plus I re-added your comments.

@tim-janik tim-janik closed this Jul 25, 2018

@swesterfeld swesterfeld deleted the swesterfeld:fix-fluid-test branch Aug 28, 2018

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