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

RFC: Dynamic sample loading #366

Merged
merged 11 commits into from Apr 22, 2018
Merged

RFC: Dynamic sample loading #366

merged 11 commits into from Apr 22, 2018

Conversation

mawe42
Copy link
Member

@mawe42 mawe42 commented Apr 15, 2018

This pull requests implements the dynamic (or lazy or smart) sample loading feature that we have discussed in http://lists.gnu.org/archive/html/fluid-dev/2018-04/msg00000.html

When enabled via the synth.dynamic-sample-loading setting, samples are loaded to and unloaded from memory on demand. As soon as a preset is selected for a channel, all the samples used in that preset are loaded from the Soundfont. If a preset is deselected, all samples are unloaded again (assuming they are not used by any other selected preset).

It also makes a change to the way Soundfonts are loaded without dynamic sample loading. Previously, Ogg Vorbis compressed samples from SF3 got their compressed data loaded into memory and only then was it decompressed. Further, the decompressed data wasn't stored in the sample cache. So multiple synth instances or multiple loaded Soundfonts always had to decompress the data again. With this change, decompression has moved to the lowest level in fluid_sffile and the decompressed data is stored in the sample cache.

To test this, I've converted the VintageDreams test soundfont to SF3 using the sf3convert utility. It's available for tests via the TEST_SOUNDFONT_SF3 define.

As the title says: this is a request for comments. And it's missing one feature we discussed on the mailing list: analysing a MIDI file to preload all presets that are used in the file. I'll do this as soon as the general idea and implementation of the dynamic loading feature has been tested and validated by a few people.

@jjceresa
Copy link
Collaborator

jjceresa commented Apr 17, 2018

Hi Marcus,
Not yet get a try (i am currently busy on other stuff). Anyway i just wanted to keep my understanding clear (from the MIDI side point of view). Thanks to correct my understanding.

As soon as a preset is selected for a channel, all the samples used in that preset are loaded from the Soundfont. If a preset is deselected, all samples are unloaded again.

I understand that a preset is selected upon CC program change. On this CC receive, the previous preset (if any) is deselected (i.e unloaded) and the new preset is selected (i.e loaded).

With this change, decompression has moved to the lowest level in fluid_sffile and the decompressed data is stored in the sample cache.

In others words this change introduces a new logic faster than the one before.


As the title says: this is a request for comments. And it's missing one feature we discussed on the mailing list: analysing a MIDI file to preload all presets that are used in the file.

Effectively this is another function complementary and useful.


Just a question.
In the actual state of the "sample loading" implementation. Upon receiving a CC program change on a MIDI channel that is currently loading the samples, then if a note on is received on the same channel, the expected behaviour should be to have this noteon waiting until CC program change have finished.
So this mutual exclusion is ensured when calling the MIDI side API (via the well know unique mutex).
Do you see any possible existing soundfont loading API function (calling the new "sample loading" logic) that are missing using this mutex ?

@mawe42
Copy link
Member Author

mawe42 commented Apr 17, 2018

I understand that a preset is selected upon CC program change. On this CC receive, the previous preset (if any) is deselected (i.e unloaded) and the new preset is selected (i.e loaded).

Correct. Although obviously not only on a CC program change, but also if you choose a preset with the select command in the shell.

With this change, decompression has moved to the lowest level in fluid_sffile and the decompressed data is stored in the sample cache.

In others words this change introduces a new logic logic faster than the one before.

Potentially, yes. Not really faster for loding a single SF3 file, but definitely faster if the same SF3 file is used in multiple synth instances or if the sample file is loaded multiple times into the same instance.

Do you see any possible existing soundfont loading API function (calling the new "sample loading" logic) that are missing using this mutex?

I haven't actually checked for that, because that would be a bug in its own right. If there are API functions that do anything with Soundfonts, samples or other Fluidsynth internals that do not lock the API mutex, then that would be a problem with the "normal" Soundfont loading as well. And I think we would have heard about it by now, as it would most likely lead to Segfaults.

This change moves the Ogg Vorbis decompression to fluid_sffile, so that
this is the only place where we have to deal with compressed audio.

It also changes the way the samples are loaded for SF3 files: previously,
the compressed data was copied into memory, then the individual samples
were decompressed (resulting in both compressed and decompressed data to
stay in memory). Also, decompressed data wasn't cached, so previous loads
of the same file ran the decompressed again for each sample.

After this change, the vorbis decompression is changed so that it reads the
compressed data directly from the Soundfont file. And the resulting WAV
data is stored in the sample cache.
This change adds a new feature that enables loading and unloading of
sample data on demand. As soon as a preset is selected for a channel, all
of it's samples are loaded into memory. When a preset is unselected, all
of it's samples are unloaded from memory (unless they are being used by
another selected preset).

This feature is disabled by default and can be switched on with the
"synth.dynamic-sample-loading" setting.
Copy link
Member

@derselbst derselbst left a comment

Choose a reason for hiding this comment

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

Works charming. Soundfont loading overhead basically reduced to zero. Would like to retest it after #369 is fixed.

<setting>
<name>dynamic-sample-loading</name>
<type>bool</type>
<def>0 (TRUE)</def>
Copy link
Member

Choose a reason for hiding this comment

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

0 (FALSE)

@mawe42
Copy link
Member Author

mawe42 commented Apr 21, 2018

Wouldn't the fix for #369 be better suited for master anyway? dynamic-sample-loading might still take a while to be fully tested. And it's not quite complete yet...

@derselbst
Copy link
Member

Wouldn't the fix for #369 be better suited for master anyway?

Technically yes. The truth is: I committed to the wrong branch :/

As far as I can tell, this feature works absolutely fine. I would take it as it is. If you're thinking to add that MIDI file preset preloading feature, I think it should be done in another PR.

So, I currently dont have anything to add to this branch. If you want you can force push to delete 244510e from here. In this case I'll cherry-pick it to master.

@mawe42
Copy link
Member Author

mawe42 commented Apr 21, 2018

As far as I can tell, this feature works absolutely fine. I would take it as it is. If you're thinking to add that MIDI file preset preloading feature, I think it should be done in another PR.

Ok, I'll open separate PRs for preloading and other stuff. But I'd really like to test it a bit more. I might have noticed a problem with the oss midi driver the other day.

So, I currently dont have anything to add to this branch. If you want you can force push to delete 244510e from here. In this case I'll cherry-pick it to master.

Yes, I think that would be cleaner. I would like rewrite
d4e8e94 (wrong email) and remove 244510e, then force push. Ok?

@derselbst
Copy link
Member

Ok. Will cherry pick it to master.

@derselbst
Copy link
Member

I think fluid_voice_optimize_sample() is never called for lazy loaded samples. You can test it by upgrading the test_sf3_defsfont_preset_iteration test to some dynamic_sample_loading test:

  • Notifying each preset to load its samples while iterating
        TEST_ASSERT(preset->notify != NULL);
        preset->notify(preset, FLUID_PRESET_SELECTED, 0);
  • TEST_ASSERT(sample->amplitude_that_reaches_noise_floor_is_valid); during sample iteration

@mawe42
Copy link
Member Author

mawe42 commented Apr 22, 2018

I think fluid_voice_optimize_sample() is never called for lazy loaded samples.

Yes, good point! And fluid_sample_sanitize_loop is also never called. Will fix that.

@mawe42
Copy link
Member Author

mawe42 commented Apr 22, 2018

I think for SF2 samples, we need to include the 46 word area following each sample in the loaded data. Otherwise we might get problems with samples whose loopend is after sample end.

@mawe42
Copy link
Member Author

mawe42 commented Apr 22, 2018

I've done a quick test on Windows 10 with the build artifact from Appveyor. Dynamic sample loading seems to work fine on Windows as well, at least with a simple MIDI playback.

@mawe42
Copy link
Member Author

mawe42 commented Apr 22, 2018

So I think this feature is finished and could be merged. I will follow up with two other PRs:

  • one is a smaller PR that will reduce the memory consumption of the structural information significantly. Currently every instrument in the soundfont gets its own copies of the used instrument zones. In the standard FluidR3_GM soundfont, this means that we create ~18.000 instrument zones instead of ~3000, wasting about 30MB of RAM and quite a bit of processing time.
  • the other one will bring the preload shell command and scanning of MIDI files to preload presets

Copy link
Member

@derselbst derselbst left a comment

Choose a reason for hiding this comment

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

Ok. Also works fine for me. Thanks!

@mawe42 mawe42 merged commit a95a486 into master Apr 22, 2018
@mawe42 mawe42 deleted the dynamic-sample-loading branch April 22, 2018 17:06
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.

None yet

3 participants