Add public API to pin and unpin presets to the sample cache #698
Conversation
Attempts to keep the changes for this feature mostly contained in the default loader, as this is the only loader that currently supports dynamic sample loading anyway.
|
This new functions integrates naturally in the existing
This Perhaps this could be changed but it doesn't worth (imo) because the user will got an audible feedback that something is wrong in case of failure. Also we have the DEBUG log that should help, but it seems incomplete: |
Good point! And I agree, it could be cleaned up, but it's not really that important at the moment.
Also a good point. I will add that to this PR if we decide to go ahead with this. Thanks for your review! |
|
I've added some tests for the pinning and unpinning. Seems to work fine, although I haven't tested in an actual application yet. To be able to test this properly, I added a new helper function to count the number of entries in the samplecache ( |
|
That is strange, the WindowsCI debug_prof build fails on one of the pinning tests: I have no idea what could cause this... do you? |
There was a problem hiding this comment.
First of all, the code looks really nice and clean. Good job!
The downside is that calls to the new fluid_synth_pin_preset and fluid_synth_unpin_presets can not return any information about if the pinning or unpinning has actually succeeded.
This
absence of informationis not particular to the 2 new pin/unpin functions. Thisabsencewas already the case withinfluid_channel_set_preset(). This is due to the fact that we use the macrofluid_preset_notify()that ignores the code returned by the callbackdynamic_samples_preset_notify(). Similarlydynamic_samples_preset_notify()function ignores the code returned byload_preset_samples()andunload_preset_samples().
load_preset_samples()ignores the code returned byfluid_defsfont_load_sampledata().
unload_preset_samples()cannot return a code when callingunload_sample()which return void.
We must make sure that fluid_synth_pin_preset() returns the correct result. Relying on audible feedback is not sufficient. And apart from what JJC already said, I currently see no bigger problem, why returning this information should not be possible or hard to implement.
I added a new helper function to count the number of entries in the samplecache. Do you think test helper functions like that are acceptable?
I think it's fine.
So, all I can say is, give it test :) And don't worry too much about the failing test, I can assist with that upcoming weekend.
Thanks :-)
Ok, I'll add all the missing return paths.
I will make the return value changes first, adjust the tests and then try to integrate it into my application.
Luckily I can reproduce it on my machine. With build type Debug, the test fails. Really strange! |
|
Fixed the test failure on Debug builds, it was a missing initialization of the new I must say, I really like our CI infrastructure! :-) |
I am not sure what you mean by "test fails", but here D:\WinProjet\Git\GitHub\fluidsynth-VS10\test\Release>test_preset_pinning.exe fluidsynth: Loaded samples on sfont 1: 123 fluidsynth: Loaded samples on sfont 1: 123 fluidsynth: error: There is no preset with bank number 42 and preset num fluidsynth: error: There is no preset with bank number 42 and preset num fluidsynth: warning: Sample 'SineWave': ROM sample ignored fluidsynth: debug: Pinning preset 'Lead Synth 2' fluidsynth: debug: Unpinning preset 'Lead Synth 2' fluidsynth: debug: Pinning preset 'Lead Synth 2' fluidsynth: debug: Selected preset 'Lead Synth 2' on channel 0 fluidsynth: debug: Selected preset 'Aluminium Plate' on channel 1 fluidsynth: debug: Deselected preset 'Lead Synth 2' from channel 0 fluidsynth: debug: Deselected preset 'Aluminium Plate' from channel 1 fluidsynth: debug: Unpinning preset 'Lead Synth 2' fluidsynth: debug: Selected preset 'Lead Synth 2' on channel 0 fluidsynth: Loaded samples on sfont 1: 4 fluidsynth: debug: Deselected preset 'Lead Synth 2' from channel 0 fluidsynth: debug: Pinning preset 'Lead Synth 2' fluidsynth: debug: Unpinning preset 'Lead Synth 2' fluidsynth: debug: Unpinning preset 'Lead Synth 2' |
Thanks for testing it! The error I meant was this: But it's fixed now, missing initialization of |
|
From your recent changes test_preset_pinning.c fail to compile, so I fix that in next commit. |
Great, thanks! I sometimes forget that we need to be C89 compatible here. |
mawe42
changed the title
|
One open point is left: The DLS preset should return FLUID_FAILED when notified via PIN / UNPIN. I could look into this on the weekend. |
Ah, yes. I could add a new macro that checks if |
|
Hm, but one could also argue that as DLS presets are never loaded with dynamic-sample-loading, it's very similar to the case when calling If we apply the same logic to DLS samples, then it should also return FLUID_OK. After all, all the samples are already in memory. |
Exactly my thought, after having written my comment above... I'm just 3 hours too late. Ok, let's keep the FLUID_OK logic, I'll amend the API comment. |
|
Thanks for rewriting the comments and adding the devdoc entries! I've tested the feature in my application now: it seems to work as intended and helps me to remove the ugly hack I had used previously. So from my perspective, this is finished. The question remains if it is a feature we actually want to include, adding yet more public API functions. So far I seem to be the only one with a use-case for it... |
There was a problem hiding this comment.
The question remains if it is a feature we actually want to include, adding yet more public API functions.
Yes, we want. It's a feature that didn't exist before and that nicely integrates into the existing architecture. (Unlike PR 672, for which I'm uncertain if it will be already included in 2.2.0, as we need to talk about it again).
So, allow me to squash it once the build is green.
|
Kudos, SonarCloud Quality Gate passed!
|
Ah, yes. Reading the thread in #669 again, you suggested that I start a discussion about that on the mailing list. I will try to get back into the topic and then make a post so we can discuss it further, ok?
Sounds good to me, thanks! |
Perfect. |
* Properly handle overlapping notes when using fluid_event_note() (FluidSynth#637) * Fix regression introduced in a893994 Mentioned commit broke fluid_synth_start() when using a DLS soundfont. * Fix an uninitialized memory access that could possibly trigger an FPE trap for instruments that use the exclusive class generator * Update API docs * Bump to 2.1.4 * Update Doxyfile * Turn incompatible-pointer-types warning into error * Fix passing arguments from incompatible pointer type * Fix a NULL deref in jack driver * Fix a possible race condition during midi autoconnect * Fix printf format warnings * Update Android Asset loader to new callback API * Update Travis CI (FluidSynth#658) * update to Ubuntu Focal * use clang10 * avoid unintentional fallbacks to default `/usr/bin/c++` compiler * fix related compiler warnings * fix NULL permitted for out and fx pointer buffer Closes FluidSynth#659 * CMakeLists.txt: fix build with gcc 4.8 (FluidSynth#661) -Werror=incompatible-pointer-types is unconditionally used since version 2.1.4 and 137a14e. This will raise a build failure when checking for threads on gcc 4.8: /home/buildroot/autobuild/run/instance-3/output-1/host/bin/arm-none-linux-gnueabi-gcc --sysroot=/home/buildroot/autobuild/run/instance-3/output-1/host/arm-buildroot-linux-gnueabi/sysroot -DTESTKEYWORD=inline -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 -Os -Wall -W -Wpointer-arith -Wcast-qual -Wstrict-prototypes -Wno-unused-parameter -Wdeclaration-after-statement -Werror=implicit-function-declaration -Werror=incompatible-pointer-types -Wbad-function-cast -Wcast-align -DNDEBUG -fPIE -o CMakeFiles/cmTC_98946.dir/CheckIncludeFile.c.o -c /home/buildroot/autobuild/run/instance-3/output-1/build/fluidsynth-2.1.4/CMakeFiles/CMakeTmp/CheckIncludeFile.c cc1: error: -Werror=incompatible-pointer-types: no option -Wincompatible-pointer-types Fixes: - http://autobuild.buildroot.org/results/13cbba871db56ef8657a3d13c6ac8e1b4da0d244 Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com> * TravisCI: add a build for GCC 4.8 (FluidSynth#662) * Remove unused member variable * Limiting audio-channels to audio-groups (FluidSynth#663) * Use a runtime check to detect version of libinstpatch (FluidSynth#666) It could be that during runtime an older version of libinstpatch is used than the one fluidsynth was compiled against. In this case, libinstpatch will fail to load DLS fonts, because libinstpatch's initialization semantics don't match those compiled into fluidsynth. * Add a chart about voice mixing and rendering * Mapping of fx unit output to dry buffers in mix mode. (FluidSynth#668) Currently, all fx unit output (in mix mode) are mapped to the `first buffer`. This is not appropriate for synth.audio-groups > 1 This PR allows the mapping of fx output based on `fx unit index` and `synth.audio-groups` value. This allows us to get the `fx output `mixed to the respective `buffer` on which a `MIDI channel` is mapped. For example: with `synth.audio-groups = 3` and `synth.effect-groups = 3`: - MIDI chan 0 (dry + fx0) is mapped to buf 0 - MIDI chan 1 (dry + fx1) is mapped to buf 1 - MIDI chan 2 (dry + fx2) is mapped to buf 2 * Add multi channels support for audio driver. (FluidSynth#667) This PR addresses FluidSynth#665. 1) Add new functions for multi channels support: `fluid_synth_write_float_channels()`, `fluid_synth_write_s16_channels()` 2) `dsound` and `waveout` driver make use of this support. tested on 2 audio devices: - creative SB Live! (6 channels). - Realtek: ALC889A (8 channels). * Bump to 2.1.5 * Add SonarQube static code analysis (FluidSynth#671) * Add SonarQube and LGTM badges to README * Remove fluid_event_any_control_change() from public API (FluidSynth#674) Originally, I have only marked it deprecated. But since we have an SOVERSION bump next release and because this function was only meant for internal usage, I think it's safe to remove it right now. * Remove dead code * Fix an impossible NULL deref * Fix a NULL dereference Access to field 'zone' results in a dereference of a null pointer (loaded from variable 'prev_preset'), if `size` is negative. Problem is: Parameter `size` is `chunk.size` and should be unsigned. * Fix another NULL dereference Access to field 'zone' results in a dereference of a null pointer (loaded from variable 'pr'), if size is negative. However, size should be unsigned. * Remove a FIXME I don't see any problem calling fluid_channel_init() from within synth context * Remove a FIXME I don't see any 'allocation' of preset. And ALL public synth functions have a mutex lock which might potentially block when called from synth context, but only then if the client app pessimizes this situation by extensively calling the synth from outside the synth context. * Remove a FIXME I have no clue what it refers to or what it's meant by that. * Add comment into empty block * Remove a FIXME Not aware of any problems caused by the old glib thread API. It will be removed sooner or later anyway. * Remove a FIXME * Set the systemd unit target to default.target fluidsynth.service.in: The [Install] section [1] in systemd unit declares in which target the service will be started. The `multi-user.target` [2] - managed by the systemd _system_ service manager - is used in the `fluidsynth.service`. However, as it is a _user_ unit it needs to be pulled in by the `default.target` [3] instead, which is the main target for the user session (as started by `user@.service` [4]). [1] https://www.freedesktop.org/software/systemd/man/systemd.unit.html#%5BInstall%5D%20Section%20Options [2] https://www.freedesktop.org/software/systemd/man/systemd.special.html#multi-user.target [3] https://www.freedesktop.org/software/systemd/man/systemd.special.html#default.target1 [4] https://www.freedesktop.org/software/systemd/man/user@.service.html * Define FLUIDSYNTH_API on OS/2 Previously, CMake on OS/2 exported all the symbols unconditionally. Now it exports necessary symbols only. As a result, it's necessary to define FLUIDSYNTH_API correctly. Addresses FluidSynth#678 * Make winmidi driver multi devices capable. (FluidSynth#677) * Fix minor bug in windows audio driver (FluidSynth#680) * Improve error reporting in Waveout and DSound drivers * Fix Windows build * Add proper unicode support to Windows error reporting * Fix build on Windows 9x/ME Addresses FluidSynth#679 * Promote Controller/Pressure/Bend event functions to 32bits (FluidSynth#670) * Elaborate on synth.cpu-cores * Add FluidMixer chart to API docs * Ensure WaveOut compatibility with Win9X/NT (FluidSynth#687) * Update and rename README.Android.md to README.md * Update Android CircleCI build to use latest orb, Android API, Oboe and Cerbero (FluidSynth#690) This fixes the currently-broken CircleCI build for Android-useable .so files. Currently the Cerbero build is based off https://github.com/falrm/cerbero until https://gitlab.freedesktop.org/gstreamer/cerbero/-/merge_requests/641 is merged and deployed to the GitHub cerbero mirror. Here is a successful build with the updated CircleCI workflow: https://app.circleci.com/pipelines/github/falrm/fluidsynth-android/31/workflows/0ad3186a-394c-4736-984b-96496b608053/jobs/32 Fixes FluidSynth#688 * Replace FreeBSD 13.0 with 11.4 (FluidSynth#692) 13.0 hasn't been released yet and the CI build keeps failing for long. * Remove unused variable * Fix possible uninitialized use of dry_idx variable * avoid an unlikely race condition * Add hint message when compiled without getopt support (FluidSynth#697) * Add getopt support to CMake summary * Add public API to pin and unpin presets to the sample cache (FluidSynth#698) Following the discussion about an API to pin and unpin preset samples in the sample cache here: https://lists.nongnu.org/archive/html/fluid-dev/2020-10/msg00016.html Short explanation of the change: Only the default loader currently supports dynamic sample loading, so I thought it might be a good idea to keep the changes for this feature mostly contained in the default loader as well. I've added two new preset notify flags (FLUID_PRESET_PIN and FLUID_PRESET_UNPIN) that are handled by the preset->notify callback and trigger the loading and possibly unloading of the samples. * Revert "remove VintageDreamsWaves-v2.sf3" This reverts commit a36c06c. We've got explicit permission from Ian Wilson to convert it to SF3. Addresses FluidSynth#701. * Updated XSL / styling for fluidsettings.xml * Cleanup section label markup and rendering * Use (empty string) for empty default values of str settings * shell.port is an int setting, not num * Update periods and period-size with current values from source * Consistently format all floats * Better explain currently unused effects-channels * Update effects-groups description to avoid the word "unit" * Update ladspa.active description Use 1 (TRUE) for consistency and mention LADSPA documentation * As gs is default for midi-bank-select, list it as first option for clarity * Options seems to be more widely used, so use that instead of Choices * Remove FLUIDSYNTH_API and FLUID_DEPRECATED macros from documentation * Remove "References" and "Referenced by" links from doc They auto generated links are quite long on some functions, making the documentation harder to read. * Enable navigation sidebar * Make larger enums easier to read * Move doxygen customizations into separate directory * Restructure devdocs into separate pages * Change files into groups / modules * Some additional subgrouping * Use xsltproc to include settings in API documentation * Replace all links to fluidsettings.xml with proper \ref's * Command Shell group for all shell related commands With subgroups for command handler, shell and server. * Audio output group With subgroups Audio Driver and File Renderer * Logging interface * MIDI input group Contains MIDI Driver, MIDI Router, MIDI Player and MIDI Events * MIDI Seqencer documentation * Settings documentation * Miscellaneous group * SoundFont API Includes Generators, Modulators, Loader etc * Add version defines and functions to misc group * Rename setting reference page name to lowercase, for consistency * Structure the large synth header into subgroups Also include version.h and ladspa.h in the Synthesizer group. * Consistent capitalization of usage guide section names * Some more brief message abbreviation hints * Custom doxygen layout to rename modules to API Reference * Sort groups/modules, briefs and members * Updated documentation styling * Remove footer, as it takes away valuable vertical space * Make sure libxslt is only searched if doxygen is available as well * Also update the styling of the deprecated list * Mark settings with callbacks as realtime and output this in the generated docs * Separate new_* and delete_* functions from the rest * Sync the static Doxyfile with Doxyfile.cmake Still missing is the integration of the generated fluidsettings.txt, as that requires a build script currently not available on the server generating the public API docs. * Split doxygen INPUT into separate lines, for easier readability * Move recent changes into separate file * Move usage guide pages into separate files in doc/usage * Move examples into doc/examples directory * Split HTML_EXTRA_FILEs into separate lines * Use \image for images and improve quality of FluidMixer image * Use custom \setting{} alias to link to fluid settings * Smaller cleanup and reformatting of long lines. * Add generated fluidsettings.txt for fluidsynth.org API doc build Probably not the final solution, but works for now. * Hide nav sync toggle button * Style improvements for small screens - hide side nav - hide search box - make content full height * Improve styling of field tables (enum values) * Document how to revert the styling and layout changes * Add documentation hints to style guide * Make top links black on hover, not white * Add missing group brief descriptions * Remove debug leftover * Remove obsolete doxygen config options * Add intro text to deprecated list * Use SVG for fluid mixer image * Workaround for doxygen bug with linebreaks in ALIASES Using \_linebr is not ideal, as it's an internal command. But that seems to be the most compatible way to specify line breaks in ALIASES accross different doxygen versions at the moment. * GitHub Action to build the API docs from master branch (FluidSynth#704) Uploads the complete HTML API docs as an artifact called api_docs.zip * Remove unused command alias and sync Doxyfile.cmake and Doxyfile * Settings reference style more consistent with rest of reference pages * Update generated fluidsettings.txt for API doc build on fluidsynth.org * Fx unit api (FluidSynth#673) This PR addresses FluidSynth#669 point 2.1. It proposes set/get API functions to change/read fx unit parameters. The deprecated shell commands are updated. Now the commands line have 2 parameters: - first parameter is the fx unit index. - second parameter is the value to apply to the fx unit. * Update owner of the SoundFont registered trademark. (FluidSynth#706) As of the time of this PR, the SoundFont registered trademark is owned by Creative Technology Ltd. http://tmsearch.uspto.gov/bin/showfield?f=doc&state=4803:rj74xq.2.1 http://assignments.uspto.gov/assignments/q?db=tm&qt=sno&reel=&frame=&sno=74325965 * Handle GS SysEx messages for setting whether a channel is used for rhythm part. (FluidSynth#708) Some MIDI files that uses the GS standard uses channels other than channel 10 as percussion channel. Currently FluidSynth ignores the messages setting that up, causing notes meant to be played with a drum instrument played with a melodic instrument or vice versa. This patch will partially fix the issue. Currently the implementation in this patch doesn't cover a specific "quirk" in Roland GS Modules: they seem to remember the last used instrument in the selected mode. This patch simply sets the instrument number to 0 instead. A test file is attached. If played correctly (with `-o synth.device-id=16`) no out of place drum or piano sounds should be heard. [wikipedia_MIDI_sample_gstest.mid.gz](https://github.com/FluidSynth/fluidsynth/files/5610727/wikipedia_MIDI_sample_gstest.mid.gz) * Fix Windows CI Remove fake pkg-config * Re-enable unit tests with mingw and allow them to fail to ensure build artifacts are being published * Update API doc build to upload to GH pages * Fix build path in API doc publish step * Clean existing files in API doc on GH pages * Fix commit message for deploying API doc * Also set commit name and email for api doc build commits * Commit to test API doc build Will be removed with next commit again. * Revert "Commit to test API doc build" This reverts commit fd39f6e. * Make some strings const (FluidSynth#716) * Replace g_ascii_strtoll() with FLUID_STRTOL() (FluidSynth#717) * Elaborate on synth.device-id * Breaking unit tests for WindowsXP should be fatal * Update Issue templates to point to GitHub discussion Co-authored-by: Tom M <tom.mbrt@googlemail.com> Co-authored-by: jjceresa <jjc_fluid@orange.fr> Co-authored-by: Fabrice Fontaine <fontaine.fabrice@gmail.com> Co-authored-by: jjceresa <32781294+jjceresa@users.noreply.github.com> Co-authored-by: David Runge <dave@sleepmap.de> Co-authored-by: KO Myung-Hun <komh@chollian.net> Co-authored-by: Jon Latané <jonlatane@gmail.com> Co-authored-by: Marcus Weseloh <marcus@weseloh.cc> Co-authored-by: Nathan Umali <some1namednate@gmail.com> Co-authored-by: Chris Xiong <chirs241097@gmail.com> Co-authored-by: Carlo Bramini <30959007+carlo-bramini@users.noreply.github.com>
Following the discussion about an API to pin and unpin preset samples in the sample cache here:
https://lists.nongnu.org/archive/html/fluid-dev/2020-10/msg00016.html
This is a request for comment pull-request, the code has not been tested yet. I only checked if it would compile and am presenting it here to get feedback on the approach.
Short explanation of the change:
Only the default loader currently supports dynamic sample loading, so I thought it might be a good idea to keep the changes for this feature mostly contained in the default loader as well. I've added two new preset notify flags (
FLUID_PRESET_PINandFLUID_PRESET_UNPIN) that are handled by thepreset->notifycallback and trigger the loading and possibly unloading of the samples.The advantage of this approach is that changes to existing datastructures are contained to the internal default loader. The downside is that calls to the new
fluid_synth_pin_presetandfluid_synth_unpin_presetscan not return any information about if the pinning or unpinning has actually succeeded.Looking forward to your thoughts!