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

Add multi channels support for audio driver. #667

Merged
merged 8 commits into from
Sep 2, 2020
Merged

Conversation

jjceresa
Copy link
Collaborator

This PR addresses #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).

jjceresa added 4 commits August 31, 2020 16:42
-fluid_synth_write_float_channels()
-fluid_synth_write_s16_channels().

-fluid_synth_write_float() call fluid_synth_write_float_channels().
-fluid_synth_write_s16() call fluid_synth_write_s16_channels().
1)Add multi channels support
- update format structure.
- call fluid_synth_write_float_channels() instead of fluid_synth_write_float().
- call fluid_synth_write_s16_channels() instead of fluid_synth_write_s16().

2)Fix a minor memory leak: free device GUID
- update format structure.
- call fluid_synth_write_float_channels() instead of fluid_synth_write_float().
- call fluid_synth_write_s16_channels() instead of fluid_synth_write_s16().
@derselbst
Copy link
Member

Before going down into a detailed review, I keep thinking about whether fluid_synth_write_float_channels and fluid_synth_write_s16_channels is the right way to go. Perhaps we should better use fluid_synth_process() and manually translate its planar buffers into interleaved PCM. The things I have in mind:

  1. fluid_synth_process is ready for surround audio.
  2. It should be possible to remove the extra mixing currently done in fluid_synth_process by passing the user-provided buffers directly into rvoice_mixer.
  3. The effects are always written to the first stereo buffer.
  4. 200 new lines of code in fluid_synth.c vs. a functionality that (I think) we already have.
  5. Having a possible "planar to interleaved PCM translation with optional dithering" might turn out as a functionality that we could add to our public API if requested; whereas I'd be cautious adding fluid_synth_write_float_channels or fluid_synth_write_s16_channels adding to the public API. (More rendering functions make it more confusing, IMO.)

For point 2. it would be helpful if we don't have more "generic" audio rendering interfaces that we have to care about.

For point 3. I've seen that you #668 would address this. However, I have always thought of this fluid_rvoice_mixer_set_mix_fx() more as a hack that we better should get rid of in the future. And IMO fluid_synth_process with its flexible buffer setup allows us to do this.

So, I'm not strongly against these two functions, I'm just thinking of alternatives. And my proposal would be to use fluid_synth_process and while performing the planar to interleaved PCM conversion, also perform the optional dithering. If performance turns out to be a bottleneck, we can still fall back to your provided solution.

What do you think?

@derselbst derselbst added this to the 2.2 milestone Aug 31, 2020
@jjceresa
Copy link
Collaborator Author

jjceresa commented Sep 1, 2020

Before going down into a detailed review, I keep thinking about whether fluid_synth_write_float_channels and fluid_synth_write_s16_channels is the right way to go. Perhaps we should better use fluid_synth_process() and manually translate its planar buffers into interleaved PCM.

Actually fluid_synth_write_float(), fluid_synth_write_float() or similar fluid_synth_write_xxxx_channels() can be called by all audio drivers. These function accept a flexible buffer organisation (interleaved/non interleaved). This is required by the host audio driver that dictates how the buffers must be written (interleaved/not interleaved) by these functions. (for example alsa driver require interleaved for PCM and non interleaved for float sample). So, I see these functions as the simpler appropriate direct interface between the mixer and any host device driver to final load speakers.

point 1,2

I see fluid_synth_process() as an appropriate generic interface between the mixer and any application that intends to do more processing. This interface exposes the main buffer and fx buffer. So the application can do what it wants with these buffers (map, process, mix).
fluid_synth_process() is probably the only one rendering API that an application should keep in mind.

whereas I'd be cautious adding fluid_synth_write_float_channels or fluid_synth_write_s16_channels adding to the public API. (More rendering functions make it more confusing, IMO.)

Yes, and for now I don't think that fluid_synth_write_xxxx_channels() should be public API (IMO they are only useful for driver and should stay private).

Point 3 and PR 668

The intends was to propose an API that allows to

  • map any MIDI channel to any dry buffer.
  • map any MIDI channel to any fx unit input.
  • map any fx output to any dry buffer.

The motivation is to get (for example):

  • distinct MIDI instrument played on distinct stereo speakers.
  • distinct MIDI instrument having its own fx unit.
  • instrument fx output not necessarily mixed on the same speakers that the one used for this instrument dry audio.

All these feature are possible if the mixer keeps its fx mix mode capability.
Please see attached figure below.

200 new lines of code in fluid_synth.c vs. a functionality that (I think) we already have.

fluid_synth_write_xxxx_channels() each have 20 lines of required new code.
fluid_synth_write_s16(), fluid_synth_write_float() each require 4 lines of new code because
they call respective fluid_synth_write_xxxx_channels() .
Total: 50 lines of new code. 150 lines of comments !. ;)

@jjceresa
Copy link
Collaborator Author

jjceresa commented Sep 1, 2020

I have added the missing document in branch #668

@jjceresa
Copy link
Collaborator Author

jjceresa commented Sep 1, 2020

Thinking about points 2 and 5

2 It should be possible to remove the extra mixing currently done in fluid_synth_process by passing the user-provided buffers directly into rvoice_mixer

Ok, that means that:

  • the mixer (fluid_rvoice_buffers_mix()) should be able to put the dry audio directly in the caller-provided dry buffers.
  • similarly fluid_rvoice_mixer_process_fx() should be able to put fx audio directly in the caller-provided fx buffers.
    That means that before calling fluid_synth_render_blocks(), the mixer should have knowledge of these caller-provided buffers.
    2.1) So, from inside fluid_synth_process() an "internal API" is required to set the mixer with theses buffers. Up to the caller of 'fluid_synth_process()' to decide if the fx buffers should be mixed in the same buffer than dry buffer.

5-Having a possible "planar to interleaved PCM translation with optional dithering" might turn out as a functionality that we could add to our public API if requested.

5.1) assuming fluid_synth_process() is called by an audio driver, it seems that this API functionality is required because before calling fluid_synth_process(), the mixer must know how the buffer (possibly provided by the host device driver) should be written:

  • (interleaved/not interleaved),
  • float or PCM (with/without dithering).
  • another option could be to request the mixer to force the mixing of fx audio into the dry buffer. This is useful for audio driver that intends fx mixed with dry audio. This avoid to provide and prepare fx buffer each time fluid_synth_process() is called.

That sounds as a mixer enhancement as skipping actual internal mixer buffers should be a bit faster.
Also this should simplify adding support for audio driver with multiple output.

All this means also that fluid_synth_write_float() and fluid_synth_write_s16() should be updated and may be
part of functionalities inside should be moved into the mixer.

Both these "mixer API" functionalities (2.1) and (5.1) need to be designed carefully.
Now I have talked to much, I need to drink something.

@derselbst
Copy link
Member

Ok, I understand your points.

It means that internally, we are not using our existing rendering interfaces, we rather make old-existing ones more generic. At the same time, there seems to be a need for an audio rendering interface that supports interleaved rendering. Actually, I do have the same need in my app, currently with manual planar to interleaved translation though.

On the other hand and without major changes to rvoice_mixer, my point 2. is only possible with an interface like fluid_synth_process ...It's a dilemma...

I think that for now, I'll take your change. And if I ever get to implement my point 2. I'll also consolidate the rendering interfaces.

I'll give you a more detailed review soon. And we need to find out why the unit tests are failing.

src/drivers/fluid_dsound.c Outdated Show resolved Hide resolved
src/drivers/fluid_waveout.c Outdated Show resolved Hide resolved
src/drivers/fluid_waveout.c Outdated Show resolved Hide resolved
src/drivers/fluid_dsound.c Outdated Show resolved Hide resolved
src/drivers/fluid_waveout.c Outdated Show resolved Hide resolved
@jjceresa
Copy link
Collaborator Author

jjceresa commented Sep 2, 2020

At the same time, there seems to be a need for an audio rendering interface that supports interleaved rendering.
Yes this audio rendering interface should support both interleaved (or non-interleaved) rendering.

And we need to find out why the unit tests are failing.

Hope that commit e9a9d35 will fix that ?

@derselbst
Copy link
Member

derselbst commented Sep 2, 2020

Ok, seems to work now. Thanks!

@derselbst derselbst merged commit f94cee0 into master Sep 2, 2020
@derselbst derselbst deleted the drv-muti-chan branch September 2, 2020 19:31
CartoonFan pushed a commit to CartoonFan/fluidsynth that referenced this pull request Dec 20, 2020
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants