Refactor of sffile and defsfont code, including some bugfixes #823

mawe42 · 2021-03-27T21:51:52Z

This PR makes a few refactoring changes to the the fluid_sffile and fluid_defsfont code and also includes fixes for #821 and #813.

The most important refactorings are:

avoid redefining an enum for the generator IDs and use the enum from gen.h instead
get rid of the hack around the SFZone->inststamp pointer by treating the GEN_INSTRUMENT and GEN_SAMPLEID generators like any other generators and resolving the indices in the defsfont import functions instead

I developed it on top of the defsfont-integration-test branch to make sure it doesn't introduce new behaviour. So to test this PR, I would suggest to merge it locally with defsfont-integration-test. I could also rebase it onto that branch, if it would help with review.

With the integration test, I'm fairly certain that this PR doesn't introduce any unwanted effects or new behaviour, but as it's touching such a core part of FluidSynth I would really welcome a very thorough review from both of you. Both about the code changes themselves, but also about the refactorings as a whole. As they touch very old and often used code code paths, the risk of breaking stuff is obviously quite high, even with the integration test as a safety net.

Those fields are not used anywhere, so let's simply remove them.

Not really necessary, checking against Gen_Last is just as understandable and removes a macro.

The previous implementation used 0 as end-of-list sentinel value. But 0 is also the id of the first generator in the invalid_preset_gen list. This effectively prevented checks for invalid preset generators. Also contains some code cleanup to make the check for invalid instrument generators similar to invalid preset generators. Fixes #821

We already have an enum for all generator values, so there is no need to define another list in the loader code.

Presets should not contain sampleid generators, instruments should not contain instrument generators.

This change removes the need for the instsamp hack in instrument and preset zones. The sampleid and instrument generators are treated as any other generator and simply passed to the defsfont import functions. Those read the two generators and use the index to look up valid samples and instruments. Both generators are then reset to GEN_UNUSED again, just to make sure that the rest of FluidSynth doesn't get confused. But that might not be necessary.

Cleanup of the code structure and fix of the ineffective check for global zones that are not first in list. Fixes #813

lgtm-com · 2021-03-27T22:01:39Z

This pull request fixes 1 alert when merging 0f8e2c7 into 8413c35 - view on LGTM.com

fixed alerts:

1 for FIXME comment

jjceresa · 2021-03-28T12:13:56Z

Looking after Fix preset generator validity checks c4d38a7: look good.
Note: We have duplicate code in valid_inst_genid() and valid_preset_genid().

jjceresa · 2021-03-28T12:20:46Z

Suggestion: in 16d2f43 struct _SFPhdr, perhaps could we add a little comment to indicate that library, genre and morphology aren't not used ?.

jjceresa · 2021-03-28T12:29:49Z

About change in Remove instsamp hack dfbef11, I guess this will not augment loading time ?

mawe42 · 2021-03-28T12:52:47Z

Thanks for the review JJC!

Note: We have duplicate code in valid_inst_genid() and valid_preset_genid().

I don't see duplicate code in those two functions. What do you mean exactly?

Suggestion: in 16d2f43 struct _SFPhdr, perhaps could we add a little comment to indicate that library, genre and morphology aren't not used ?

We could do. I'm just wondering what the purpose would be. There is a comment in sffile where we skip those thields, but why would we (or somebody working on FS in the future) need an additional comment in SFPhdr? i think the statement in the SF2 specs about "preserving" those three values means that software that loads and then writes SF2 files should keep the content of those fields untouched. But we will never write SF2 files...

About change in Remove instsamp hack dfbef11, I guess this will not augment loading time ?

It does increase loading time a tiny bit. Here are the timings for loading 10 instances of the FluidR3 soundfont on my machine:

master branch:
real	0m0,740s
user	0m1,064s
sys	0m0,144s

refactor branch:
real	0m0,761s
user	0m1,088s
sys	0m0,134s

If we think that slowdown is too much, then I think we could easily find some places for optimiziation. Like using fluid_list_prepend instead of fluid_list_append for the sample lists in sffile and defsfont. With that change, the timings are:

real	0m0,668s
user	0m0,978s
sys	0m0,154s

Or, if we didn't want to change the order or samples in the list, we could simply keep track of the added last_list element when appending to lists in sffile, reducing the O(n) complexity everywhere to O(1). That would probably give an even greater speedup.

Using prepend both in sffile and defsfont reduces insert complexity to O(1) and does not affect the final order of the sample list, as the reversed order of the samples in sffile is reversed again in defsfont.

mawe42 · 2021-03-28T13:02:09Z

Or, if we didn't want to change the order or samples in the list

Wrong thinking on my part... using fluid_list_prepend in both places preserves the original order, of course. So that is an easy optimisation without side-effects. I've added the change to this PR.

lgtm-com · 2021-03-28T13:10:33Z

This pull request fixes 1 alert when merging 5ebd4d3 into 8413c35 - view on LGTM.com

fixed alerts:

1 for FIXME comment

jjceresa · 2021-03-29T09:41:34Z

I don't see duplicate code in those two functions.

Please don't mind about that. Ignore my comment.

We could do. I'm just wondering what the purpose would be. There is a comment in sffile where we skip those fields

Ah yes. That is sufficient. Please don't mind.

It does increase loading time a tiny bit.

This augmentation seems negligible. Thanks for all the cleanup.

derselbst

Looks pretty good to me as well. I am getting a bit cold feet when looking at the changes in load_pgen and load_igen though. So I would suggest the following procedure:

Release 2.2.0 on easter
Get Marcus' integration test ready
Get my zone-validation-test ready
Test this PR against both tests
Merge this PR

mawe42 · 2021-03-29T17:31:25Z

Sounds good to me. But I would add another point to the list:

Run fuzzing tests with AFL on this pR (which I have done already, but better be safe than sorry)

In case you haven't written the zone unit tests yet: I do have some small (1.5k) soundfonts created during the fuzzing that trigger the invalid zone handling. So we could simply use those as test cases and check them against the expected defsfont state with the dump_sfont tool.

derselbst · 2021-03-30T19:16:39Z

In case you haven't written the zone unit tests yet

I have a few tests, also the broken soundfont from 808 has a test case. It's quite a lot copy pasted. Unfortunately I don't see how to make it much better without loosing too much flexibility while putting in too much effort. (it's checked in)

lgtm-com · 2021-04-03T15:47:31Z

This pull request fixes 1 alert when merging 28890df into ddb13e3 - view on LGTM.com

fixed alerts:

1 for FIXME comment

derselbst · 2021-04-10T13:37:33Z

When the integration test is ready, I would merge master back in and resolve conflicts.

derselbst

Great! As expected, the only detected behaviour change is that GEN_INSTRUMENT is no longer allowed in load_igen. Once CI is green, we can merge this as well.

mawe42 · 2021-04-10T14:31:27Z

As expected, the only detected behaviour change is that GEN_INSTRUMENT is no longer allowed in load_igen.

Yeah... I wondered if that is actually a good thing to add. Yet another SoundFont validity check that FluidSynth doesn't need (because it can cope with this defect). But maybe it will help to improve other SoundFont implementations out there... ;-)

sonarcloud · 2021-04-10T14:34:05Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

81.2% Coverage
9.4% Duplication

lgtm-com · 2021-04-10T14:39:42Z

This pull request fixes 1 alert when merging d275402 into 487156c - view on LGTM.com

fixed alerts:

1 for FIXME comment

derselbst · 2021-04-10T14:45:48Z

As expected, the only detected behaviour change is that GEN_INSTRUMENT is no longer allowed in load_igen.

Yeah... I wondered if that is actually a good thing to add.

I'm very confident that it won't hurt. Whereas fluidsynth previously added a useless instrument generator it now simply skips that generator. Loading is not affected. The user doesn't even see a message, that it had been skipped. That's just fine. It's the correct behaviour.

mawe42 · 2021-04-10T14:52:13Z

The user doesn't even see a message, that it had been skipped.

If a generator is skipped, then we warn that "Some invalid generators were discarded". But of course you are right... it's only a warning, not a hard failure.

derselbst · 2021-04-10T14:57:44Z

Ah, sry, I was only looking at the unit test before and afterwards and didn't recognize it.

mawe42 · 2021-04-10T18:07:03Z

Once CI is green, we can merge this as well.

Looks good, so I would merge this in the next hour (unless you shout stop before :-)

mawe42 added 10 commits March 27, 2021 22:42

Remove unused library, genre and morphology fields

16d2f43

Those fields are not used anywhere, so let's simply remove them.

Remove unused structs in sffile code

b61abdd

Rename Gen_Dummy to Gen_last to clarify the purpose

359af77

Remove Gen_MaxValid macro

84704ea

Not really necessary, checking against Gen_Last is just as understandable and removes a macro.

Remove unused macros

259aece

Remove code duplication by re-using enum from fluid_gen.h

6b63c80

We already have an enum for all generator values, so there is no need to define another list in the loader code.

Add more invalid generators for presets and instruments

6ca9b21

Presets should not contain sampleid generators, instruments should not contain instrument generators.

Refactor load_igen and load_pgen

0f8e2c7

Cleanup of the code structure and fix of the ineffective check for global zones that are not first in list. Fixes #813

mawe42 requested review from derselbst and jjceresa March 27, 2021 21:51

Speedup SF2 loading by using prepend instead of append on samples

5ebd4d3

Using prepend both in sffile and defsfont reduces insert complexity to O(1) and does not affect the final order of the sample list, as the reversed order of the samples in sffile is reversed again in defsfont.

derselbst reviewed Mar 29, 2021

View reviewed changes

This was referenced Apr 7, 2021

Debug output for discarded generators #829

Open

Zone Validation Test #826

Merged

derselbst force-pushed the refactor-and-fix-sffile-defsfont branch from 28890df to aa937cc Compare April 10, 2021 14:21

derselbst approved these changes Apr 10, 2021

View reviewed changes

Merge branch 'master' into refactor-and-fix-sffile-defsfont

d275402

derselbst force-pushed the refactor-and-fix-sffile-defsfont branch from aa937cc to d275402 Compare April 10, 2021 14:30

mawe42 merged commit aad6288 into master Apr 10, 2021

mawe42 deleted the refactor-and-fix-sffile-defsfont branch April 10, 2021 22:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor of sffile and defsfont code, including some bugfixes #823

Refactor of sffile and defsfont code, including some bugfixes #823

mawe42 commented Mar 27, 2021 •

edited

Loading

lgtm-com bot commented Mar 27, 2021

jjceresa commented Mar 28, 2021

jjceresa commented Mar 28, 2021

jjceresa commented Mar 28, 2021

mawe42 commented Mar 28, 2021 •

edited

Loading

mawe42 commented Mar 28, 2021

lgtm-com bot commented Mar 28, 2021

jjceresa commented Mar 29, 2021

derselbst left a comment

mawe42 commented Mar 29, 2021

derselbst commented Mar 30, 2021 •

edited

Loading

lgtm-com bot commented Apr 3, 2021

derselbst commented Apr 10, 2021

derselbst left a comment

mawe42 commented Apr 10, 2021

sonarcloud bot commented Apr 10, 2021

lgtm-com bot commented Apr 10, 2021

derselbst commented Apr 10, 2021

mawe42 commented Apr 10, 2021

derselbst commented Apr 10, 2021

mawe42 commented Apr 10, 2021

Refactor of sffile and defsfont code, including some bugfixes #823

Refactor of sffile and defsfont code, including some bugfixes #823

Conversation

mawe42 commented Mar 27, 2021 • edited Loading

lgtm-com bot commented Mar 27, 2021

jjceresa commented Mar 28, 2021

jjceresa commented Mar 28, 2021

jjceresa commented Mar 28, 2021

mawe42 commented Mar 28, 2021 • edited Loading

mawe42 commented Mar 28, 2021

lgtm-com bot commented Mar 28, 2021

jjceresa commented Mar 29, 2021

derselbst left a comment

Choose a reason for hiding this comment

mawe42 commented Mar 29, 2021

derselbst commented Mar 30, 2021 • edited Loading

lgtm-com bot commented Apr 3, 2021

derselbst commented Apr 10, 2021

derselbst left a comment

Choose a reason for hiding this comment

mawe42 commented Apr 10, 2021

sonarcloud bot commented Apr 10, 2021

lgtm-com bot commented Apr 10, 2021

derselbst commented Apr 10, 2021

mawe42 commented Apr 10, 2021

derselbst commented Apr 10, 2021

mawe42 commented Apr 10, 2021

mawe42 commented Mar 27, 2021 •

edited

Loading

mawe42 commented Mar 28, 2021 •

edited

Loading

derselbst commented Mar 30, 2021 •

edited

Loading