RFC: SoundFont Loading Refactor #360

mawe42 · 2018-04-01T22:35:50Z

Important: This isn't a finished pull request, but a work in progress. So please do not merge this in it's current form.

I've started this large refactor because I wanted to add a new feature to the soundfont loader: lazy-loading of presets and samples. The idea is that in certain real-time performance situations, small delays in switching presets for loading the required samples are absolutely acceptable. So on loading a new soundfont and selecting a preset, only the absolute minimum of information is actually read from the file and loaded to RAM. This would greatly reduce loading time and save huge amounts of memory for larger soundfonts that contain many instruments and presets. Please note: this feature hasn't been implemented in this PR. I'm just explaining this here to give some background to some of the changes I've made to the loading infrastructure.

Just a quick summary of what has already been done:

more the low-level soundfont loading code (the code taken over from Smurf) into a separate file
cleanup the low-level code, bringing the style more in line with the rest of FluidSynth's codebase
refactor the low-level code to prepare it for selective loading of preset zones, instruments and samples
move the ogg vorbis compression to fluid_sfont
refactor the sample start/end and loopstart/loopend checking, making it possible to use the same logic
and code for both compressed and uncompressed samples
move the samplecache code from defsfont to own file, move sample loading code to low-level code in fluid_sffile
refactor and cleanup the samplecache code

Next I am going to work in the preset loading functions. But before I carry on and add more hours and work, I would really like to get some feedback on the direction I'm taking. Do the changes I've made so far look good, or do you think I'm going into the wrong direction?

Also, I've got one specific question: currently, whenever FluidSynth core requests a preset from a defsfont loader, a new fluid_preset_t struct is allocated... every time. To me that seems totally pointless, because that fluid_preset_t struct only contains pointers to the accessor functions and uses the (singleton) fluid_defpreset_t to do any real work anyway. So I'm thinking of changing the defsfont loader to pre-create those fluid_preset_t structs and only return a pointer to them when asked. That would mean we could get rid of the preset_stack dance in defspreset.

derselbst · 2018-04-02T08:02:00Z

src/sfloader/fluid_sfont.c

+        FLUID_LOG(FLUID_DBG, "Sample '%s': invalid loop start '%d', setting to sample start '%d'",
+                sample->name, sample->loopstart, sample->start);
+        sample->loopstart = sample->start;
+        modified = TRUE;


This (and below) actually introduces a regression for #171. As the comment points out we should not modify this in case of (sample->loopstart < sample->start) || (sample->loopend < sample->start), at least for SF2 files. This handling would be probably ok for SF3, as it does not share a big sample buffer like SF2.

I don't think it introduces a regression. It's not quite what the comment says, but that's how it is implemented in master:

invalid_loopstart = (sam->loopstart < sam->start) || (sam->loopstart >= sam->loopend);

And when invalid_loopstart is set, loopstart is set to sample->start.

Sry, my bad. I was convinced it sounded wrong during a first rehearsal. But you're right, it's fine.

derselbst · 2018-04-02T08:03:16Z

src/sfloader/fluid_sfont.h

@@ -24,6 +24,10 @@

 #include "fluidsynth.h"

+FLUIDSYNTH_API int fluid_sample_validate(fluid_sample_t *sample, unsigned int max_end);
+FLUIDSYNTH_API int fluid_sample_sanitize_loop(fluid_sample_t *sample, unsigned int max_end);
+FLUIDSYNTH_API int fluid_sample_decompress_vorbis(fluid_sample_t *sample);


Why are you making these public?

Ah, good point. That's a mistake.

derselbst

Do the changes I've made so far look good, or do you think I'm going into the wrong direction?

Overall this looks good to me. It splits the huge overloaded fluid_defsfont.c into more handy parts. It might be worth bringing the lazy-loading feature to the mailing list for discussion, but that's a different story.

Also, I've got one specific question: currently, whenever FluidSynth core requests a preset from a defsfont loader, a new fluid_preset_t struct is allocated... every time. To me that seems totally pointless, because that fluid_preset_t struct only contains pointers to the accessor functions and uses the (singleton) fluid_defpreset_t to do any real work anyway. So I'm thinking of changing the defsfont loader to pre-create those fluid_preset_t structs and only return a pointer to them when asked. That would mean we could get rid of the preset_stack dance in defspreset.

Yes, that TODO has been on the list for long. Your proposal sounds good to me. Not quite sure about the schedule of your works. Would it be #360 -> Re-Formatting -> Preset-Stack-Refactor -> Lazy-Loading ? I'm open to postpone the Reformatting even further, just let me know.

Some notes below.

derselbst · 2018-04-02T11:57:49Z

src/sfloader/fluid_samplecache.c

+        entry = new_samplecache_entry(sf, sample_start, sample_count);
+        if (entry == NULL)
+        {
+            return FLUID_FAILED;


return without mutex unlock

Well spotted, thanks.

derselbst · 2018-04-02T12:06:14Z

src/sfloader/fluid_samplecache.c

+        entry->mlocked = (fluid_mlock(entry->sample_data, entry->sample_count * 2) == 0);
+        if (entry->sample_data24 != NULL)
+        {
+            entry->mlocked &= (fluid_mlock(entry->sample_data24, entry->sample_count) == 0);


Suppose: The first mlock succeeds, the second one fails, so that entry->mlocked is false, causing the memory not to be unlocked in fluid_samplecache_unload().

Good point! Will change it so that it only tries to the mlock 24-bit data if the first mlock succeeds.

derselbst · 2018-04-02T12:21:35Z

src/sfloader/fluid_samplecache.c

+{
+#if defined(WIN32) || defined(__OS2__)
+    *modification_time = 0;
+    return FLUID_OK;


// FIXME: use g_file_info_get_modification_time()

Here's a quickly smashed together C99 version that should retrieve the mod time of a file. Uncompiled and untested, as I'm currently not at home.

int res = FLUID_FAILED; #if GLIB_CHECK_VERSION(2,26,0) // g_date_time_new_from_timeval_utc() requires at least 2.26 GFile* gfile = g_file_new_for_path (filename); if(gfile != NULL) { GFileInfo* info = g_file_query_info (gfile, G_FILE_ATTRIBUTE_STANDARD_SIZE "," G_FILE_ATTRIBUTE_TIME_MODIFIED, G_FILE_QUERY_INFO_NONE, NULL, NULL); if (info != NULL) { GTimeVal tv; g_file_info_get_modification_time (info, &tv); GDateTime* dt = g_date_time_new_from_timeval_utc(&tv); if(dt != NULL) { // TODO: converting int64 to time_t, does it compile? is it safe?? *modification_time = g_date_time_to_unix(dt); g_date_time_unref(dt); res = FLUID_OK; } g_object_unref(info); } g_object_unref(gfile); } #else #warning "Consider upgrading glib to at least version 2.26" #endif return res;

Great, thanks! Regarding the g_date_time_new_from_timeval_utc: we could switch to using a uint64 for the modification_time in fluid_samplecache_t. Then we could just to the following:

*modification_time = tv.tv_sec * 1000000 + tv.tv_usec;

What do you think?

Ok sure. But unless you really need microsecond precision for a file mod date, *modification_time = tv.tv_sec should be sufficient (which should result in quite the same behaviour as g_date_time_to_unix()).

Hm, microsecond precision is probably not required. So lets go with seconds and see how it goes. Precision can always be easily increased if it poses a problem...

It seems that the functions you used here are from GIO, not GLIB. I don't think adding another dependency for the modification time is worth it. But we could simply use g_stat:

--- fluid_sys.h: #include <glib/gstdio.h> typedef GStatBuf fluid_stat_buf_t; #define fluid_stat(_filename, _statbuf) g_stat((_filename), (_statbuf)) --- fluid_samplecache.c: static int fluid_get_file_modification_time(char *filename, time_t *modification_time) { fluid_stat_buf_t buf; if (fluid_stat(filename, &buf)) { return FLUID_FAILED; } *modification_time = buf.st_mtime; return FLUID_OK; }

Oh, I didnt notice I was digging in GIO. You're right, and g_stat is the much more elegant way. Thx!

derselbst · 2018-04-02T12:39:28Z

src/sfloader/fluid_sffile.c

+    /* Load 16-bit sample data */
+    if (sf->fcbs->fseek(sf->sffd, sf->samplepos + (start * 2), SEEK_SET) == FLUID_FAILED)
+    {
+        perror("error");


perror seems redundant

derselbst · 2018-04-02T12:40:54Z

src/sfloader/fluid_sffile.c

+        goto error_exit;
+    }
+
+    loaded_data = (short *)FLUID_MALLOC(count * 2);


I'm not a friend of casting malloc() to the destination pointer type.

Ok, I'll use FLUID_ARRAY.

Hm... seeing that FLUID_ARRAY does exactly that (just hidden behind a macro)... what exactly do you propose?

Ah, now I understand. You just want to remove the explicit cast?

You just want to remove the explicit cast?

Yes. But FLUID_ARRAY really seems to be the better solution for this purpose (just forgot about it).

seeing that FLUID_ARRAY does exactly that (just hidden behind a macro)...

At least the cast is "abstracted" through the macro, so I dont further care.

derselbst · 2018-04-02T12:47:34Z

src/sfloader/fluid_sffile.c

+        goto error_exit;
+    }
+
+    if (sf->sample24pos && ((start > sf->sample24size) || (end > sf->sample24size)))


I would postpone this check to the if (sf->sample24pos){...} below. And instead of hard failing in case there is an error related to SM24, I would prefer to just ignore the SM24 then instead of fatally failing to load the whole sample.

Yes, good idea. And more in line with how we treat 24-bit samples everywhere else.

derselbst · 2018-04-02T12:49:18Z

src/sfloader/fluid_sffile.c

+        if (loaded_data24 == NULL)
+        {
+            FLUID_LOG(FLUID_ERR, "Out of memory");
+            return FLUID_FAILED;


goto error_exit; instead of direct return to avoid memory leak of loaded_data.

derselbst · 2018-04-02T13:02:20Z

src/sfloader/fluid_sffile.h

+
+struct _SFInst
+{ /* Instrument structure */
+    unsigned short idx;


Perhaps better use unsigned int for this index?

Yes... no idea why I chose short here...

derselbst · 2018-04-02T13:02:57Z

src/sfloader/fluid_sfont.h

@@ -170,6 +174,7 @@ struct _fluid_preset_t {
 */
 struct _fluid_sample_t
 {
+  unsigned short idx;


uint idx; ?

mawe42 · 2018-04-02T20:44:03Z

Overall this looks good to me. It splits the huge overloaded fluid_defsfont.c into more handy parts.

Great, thanks for your feedback and the initial review! I'll carry on then :-) There are a few more refactorings I'd like to do to the remaining defsfont code before attempting to add the new feature.

It might be worth bringing the lazy-loading feature to the mailing list for discussion, but that's a different story.

Ok, I will do that.

Not quite sure about the schedule of your works. Would it be #360 -> Re-Formatting -> Preset-Stack-Refactor -> Lazy-Loading ? I'm open to postpone the Reformatting even further, just let me know.

It would be great if we could postpone the reformatting until after the additional refactorings I'd like to do. So #360 -> Preset-Stack + a few other refactorings -> Re-Formatting -> Lazy Loading.

mawe42 · 2018-04-03T20:38:07Z

I like your idea of only lazy loading sample data first. So I'm going to rebase this branch again, trying to remove most of the functional changes I made to the low-level loading in fluid_sffile that were in preparation for the lazy-loading of structural information. Should have kept this as a clean refactor in the first place and not mix it with functional changes... :-/

Then I will add the new modification time code and get rid of the preset stack and close this branch. Lazy loading will come in a new branch (and possibly after reformatting). Does that sound good to you, or would you rather have the preset stack stuff in a separate PR?

derselbst · 2018-04-04T08:34:13Z

So I'm going to rebase this branch again, trying to remove most of the functional changes I made to the low-level loading in fluid_sffile

Ok. But generally I like the idea of having this fluid_sffile to handle the low-level loading.

If it's not too much trouble for you I would choose to have the preset stack separately. Apart from that this seems fine to me.

# Conflicts: # src/sfloader/fluid_sf2.c

# Conflicts: # src/sfloader/fluid_sf2.h

Makes the whole file easier to read and gives the implementation a little more "object-oriented" feel.

They were only used in soundfont loading code, so are probably a remnant from Smurf. The rest of the FluidSynth code doesn't use underscore functions, so remove them here as well for consistency. Also use single quotes in double quoted string, to remove the need for escaping chars.

…efsfont loader

As fluid_defsfont contains code that deals with fluid_sfont_t and fluid_defsfont_t, fluid_preset_t and fluid_defpreset_t, it sometimes gets very confusion to know which type is currently being accessed by "sfont" or "preset". Also clarify some preset zone / inst zone ambiguities.

mawe42 · 2018-04-04T09:20:52Z

Ok, I've taken out all functional changes to the fluid_sffile code and rebased the branch. Apart from any bugfixes, I would call this refactor finished. The preset_stack removal will come in a new PR. And I don't really mind if you want to do the reformat before or after.

derselbst · 2018-04-04T09:28:17Z

Ok will have a look at it. And just forget about the reformat at least for now, currently too much activity. Will see when it fits.

mawe42 · 2018-04-04T09:31:50Z

Ah, g_stat requires at least glib 2.6. Will add a check and warning, just like in your suggested implementation.

derselbst · 2018-04-04T09:34:21Z

No, fluidsynth requires 2.6.5 anyway:

fluidsynth/CMakeLists.txt

Line 384 in 9196636

pkg_check_modules ( GLIB REQUIRED glib-2.0>=2.6.5 gthread-2.0>=2.6.5 )

mawe42 · 2018-04-04T10:03:49Z

No, fluidsynth requires 2.6.5 anyway

Perfect.

derselbst · 2018-04-04T15:05:54Z

src/sfloader/fluid_samplecache.c

+                    }
+                }
+
+                fluid_list_remove(samplecache_list, entry);


This should be samplecache_list = fluid_list_remove(samplecache_list, entry);. Otherwise samplecache_list is not set to NULL when the last element is removed resulting in a NULL deref in line 248.

Of course, well spotted.

derselbst · 2018-04-04T19:29:35Z

src/sfloader/fluid_sfont.c

+        return FLUID_FAILED;
+    }
+
+    return TRUE;


Because you're using FLUID_FAILED, this should be FLUID_OK.

Good point.

…lues

derselbst · 2018-04-06T19:44:30Z

So far testing worked as expected, no problems nor crashes. I also think it's time to add some unittest(s). I added one for the sample cache (#361), mainly aiming to test the ref count mechanism, i.e. to avoid using sample data from a soundfont that has already been unloaded by its parent synth.

There is a struct _fluid_sfont_info_t in fluid_synth.h which seems to be a bit redundant as it adds another indirection layer. I think it's members could just be integrated in struct _fluid_sfont_t as it's not a public struct anymore. And that fluid_synth_t::sfont_hash seems to be completely redundant? (just by passing the sfont_info directly

- fluid_synth_sfont_unref (synth, sfont_info->sfont);
+ fluid_synth_sfont_unref (synth, sfont_info);

in fluid_synth.c???)

Long story short: This PR is fine. If you want you can take care of those two minor issues right here or I'll have a try.

mawe42 · 2018-04-06T21:21:25Z

Yes, that _fluid_sfont_info_t does look like it could be merged into fluid_sfont_t.

And the hash does look redundant. But I wonder what Element intended here... why implement this indirection? Maybe he didn't want to change fluid_sfont_t because it was public. And the hashtable to avoid having to iterate a list when searching for the fluid_sfont_info_t for a sfont?

If you want to have a go a those two, I would be glad. I'll do the preset_stack removal first, then prepare a lazy loading branch, just to flesh out a few ideas.

So I'll merge this PR, ok?

derselbst

Ok

derselbst · 2018-04-07T07:55:46Z

Maybe he didn't want to change fluid_sfont_t because it was public. And the hashtable to avoid having to iterate a list when searching for the fluid_sfont_info_t for a sfont?

Just my thought.

derselbst · 2018-04-07T16:29:05Z

Just found that the unit test segfaulted with the vintagedreams sfont using your changes. Fixed a misplaced return FLUID_OK and modified the sample import code to not exit early, when it sees a broken sample:

32961c4...6ab1c7f

mawe42 · 2018-04-07T20:41:06Z

Ah, thanks for catching that!

mawe42 · 2018-04-07T22:09:38Z

I've just removed the noise debug messages for samples with loopstart = loopend: fa0d103

derselbst reviewed Apr 2, 2018

View reviewed changes

mawe42 added 24 commits April 4, 2018 11:03

Move low-level soundfont loading functions to separate files

879a141

Remove reference to defsfont from low-level function

167048f

Remove unused defines and declarations

12902f7

Move declarations to top of file

5c58c30

Move private defines from fluid_sf2.h to fluid_sf2.c

333e8a1

Source formatting (clang-format)

79c3beb

Cleanup error handling, remove gerr function

d538d7f

Make all private functions static

4dbc312

Remove sfont_zone_delete, replace with direct invocations instead

f83c49d

Cleanup return values

af3e6bb

Clarify some function names and comments

6fc816c

# Conflicts: # src/sfloader/fluid_sf2.c

Rename public and static methods to naming conventions

e7109a9

# Conflicts: # src/sfloader/fluid_sf2.h

Separate typedefs and struct definitons

500931b

Use FLUID_FREE instead of free

78be6f7

Move public functions to top of file and document them properly

fb37481

Cleanup error handling in fluid_sf2_load

181b972

Store file_callbacks in SFData structure

9c31e96

More expressive variable names in load_phdr

74e2a4b

Cleanup function signatures, passing SFData to every function

5dc64d1

Makes the whole file easier to read and gives the implementation a little more "object-oriented" feel.

Rename fluid_sf2 to fluid_sffile

10b87e9

Make size macro names easier to read

7f09321

Cleanup delete_* functions

b6bdb6b

Rename fluid_sf2_* functions to fluid_sffile_*

1bc69be

mawe42 added 9 commits April 4, 2018 11:11

Remove redundant error messages

c7e579a

Remove explicit cast of FLUID_MALLOC return value

e8717e5

Properly free memory on error

352bdef

Treat failures while loading 24-bit sample data as not fatal

312459e

Use FLUID_ARRAY instead of FLUID_MALLOC for sample buffers

cf04a4f

Use FLUID_STRDUP instead of manual allocate and copy for strings in d…

109e899

…efsfont loader

Use fluid_list_get to access list entry data

d1c620f

Use glib's g_stat to get file modification time

f8bc376

mawe42 force-pushed the sfont-loader-refactor branch from 8d0a5d2 to f8bc376 Compare April 4, 2018 09:14

derselbst reviewed Apr 4, 2018

View reviewed changes

mawe42 added 2 commits April 5, 2018 19:20

Fix bug in removing element from samplecache_list

362a3f2

Return FLUID_OK instead if TRUE to be consistent with other return va…

9c1f3bd

…lues

derselbst approved these changes Apr 7, 2018

View reviewed changes

derselbst added this to the 2.0 milestone Apr 7, 2018

mawe42 merged commit 32961c4 into master Apr 7, 2018

mawe42 deleted the sfont-loader-refactor branch April 7, 2018 10:05

mawe42 mentioned this pull request Apr 8, 2018

Preset stack removal #365

Merged

RFC: SoundFont Loading Refactor #360

RFC: SoundFont Loading Refactor #360

Conversation

mawe42 commented Apr 1, 2018 • edited

derselbst Apr 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derselbst left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mawe42 commented Apr 2, 2018

mawe42 commented Apr 3, 2018

derselbst commented Apr 4, 2018

mawe42 commented Apr 4, 2018 • edited

derselbst commented Apr 4, 2018

mawe42 commented Apr 4, 2018

derselbst commented Apr 4, 2018

mawe42 commented Apr 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derselbst commented Apr 6, 2018

mawe42 commented Apr 6, 2018

derselbst left a comment

Choose a reason for hiding this comment

derselbst commented Apr 7, 2018

derselbst commented Apr 7, 2018

mawe42 commented Apr 7, 2018

mawe42 commented Apr 7, 2018

mawe42 commented Apr 1, 2018 •

edited

derselbst Apr 2, 2018 •

edited

derselbst left a comment •

edited

mawe42 commented Apr 4, 2018 •

edited