Implement native DLS parser#1626
Conversation
spessasus
left a comment
spessasus
left a comment
There was a problem hiding this comment.
Testing only gm.dls is not nearly enough: The parser does not work with all files.
audio.mp4
This was produced by the following sound bank authored by Yamaha:
I've confirmed that with gm.dls it works fine so it's not MIDIs fault.
It would also be easier for native DLS support to be enabled by default in the fork. I don't have a lot of experience with CMake so it took me quite a long time to figure out building with the native parser.
Also the following file does not load:
CrysDLS v1.23.zip
It loads in: Microsoft DirectMusic Producer, Audio Compositor, SpessaFont.
|
@spessasus Thanks for your test! Could you provide the MIDI of audio.mp4? |
|
|
About CrysDLS v1.23.dls: I just found 'crs1' chunks which have wrong size. Probably Crystal just don't want people read it (as it is encrypted originally). Update: also wsmp chunk have wrong cbSize. |
|
see also spessasus/spessasynth_core#5 |
|
I know, but given that DMUS producer (an official DLS authoring tool by microsoft) loads it then I think the fix applied in core should also be applied here. Who knows how many files like these are out there? And given that fluidsynth has way bigger reach than spessasynth, it'll probably receive a lot more reports like these. DLS already lacks tools which aren't 20 years old (DMUSProducer, Audio Compositor) or proprietary and paid (Awave studio) so making it work with as many DLS files out there as possible would be a good idea, I think. |
|
Now CrysDLS should load without problem. But I have no idea about the Yamaha DLS. |
|
You can try opening it in spessafont, converting to SF2 and comparing Fluid's sf structure to the one your parser produces? |
|
aha I just found problem: The code read 8-bit samples as signed. |
src/sfloader/fluid_dls.cpp
Outdated
| // } | ||
| // return inst->banklsb; | ||
|
|
||
| // Yamaha's DLS seems to use the same bank select mode as GS |
There was a problem hiding this comment.
Well, no, actually. This DLS has both XG drums (bank 127) and GS drums (bank 128).
Ideally in XG mode you'd select the XG drums and in GS mode, GS drums, since they have different drum maps, especially kick in XG being metronome in GS.
And it also has bank LSB numbers whereas GS only uses MSB.
There was a problem hiding this comment.
Yeah you're right. I just had some "warning: Instrument not found" and did that. Actually the DLS is not complete.
There was a problem hiding this comment.
Oh no XG uses 127 as drum bank?
There was a problem hiding this comment.
This reverts commit ec1ea9c. thanks @spessasus
|
It seems that more complex files confuse the parser: For example the preset 000:081 Saw Lead is played out of tune. You can hear it in this MIDI at around 1:18 and 1:30. EDIT (derselbst): Github mirror GeneralUser-GS.dls - multi-archive 7z file, remove the |
|
@spessasus I think it is the DLS's problem. I tested it with libsonivox and it produced same as the fork. The Saw Lead instrument contains articulation list ('lar2') under both 'lins' and 'rgn2', which is not allowed in DLS (see DLS-2 2.17 Instrument Object Hierarchy). As it is a melodic instrument, it should contains instrument-level articulations only. The implementation is the fork and Sonivox is both use region-level articulations only when it is present. If the instrument-level is regarded as "global zone", I am not sure which rule applies here (supersedes or adds) |
|
The difference between DLS 1 and DLS 2 is that DLS 2 allows articulators in every region, not just the global one. If the chunk was "lart" instead of "lar2", then it wouldn't be allowed. You can load the file with DirectMusic Producer or Audio Compositor which support DLS level 2. Please see section 3.3 DLS Level 1 Compatibility notes of the DLS 2 specification. It clearly states that melodic instrument restrictions only apply to Level 1, not 2. Edit: section 2.19.3 Generic Level 2 DLS file contains articulators in the region chunks. |
|
So DLS-2 allows region-level articulators nevertheless the instument is drums or melodic. But the problem is:
But the Saw Lead contains both instrument-level and region-level level 2 articulators! |
|
In this case, the vel2att mod in Fury.dls does not override the default one, so they should exists together. |
That's the thing! So velocity of 0 would not produce sound (vel2att evaluates to 960 cB) and velocity of 127 should also be silent (fury.dls articulator evaluates to 960 cB). |
| GEN_VIBLFOTOPITCH, FLUID_MOD_CHANNELPRESSURE, FLUID_MOD_GC, 0, 0, FLUID_MOD_TRANSFORM_LINEAR, 0, nullptr, nullptr, nullptr }); | ||
| // pitch wheel --(rpn 0)-> pitch 12800 cents | ||
| // see also the comment in gen.h about GEN_PITCH | ||
| mods.push_back(fluid_mod_t{ GEN_FINETUNE, |
There was a problem hiding this comment.
While DLS specifies 12800 cents instead of 12700, I think that given fluidsynth uses the SF2 synthesis model we should stick with the SF2's 12700 cents to make sure that everything is in tune.
In fact,
this file for testing RPN Range
Is slightly out of tune when compared to an SF2 conversion of gm.dls. Render the two, invert one in audacity and mix them down (essentially a PCM difference). then you can hear that they are silent when no pitch bend is applied (meaning no difference in PCM), but the more pitch bend is applied, the louder it gets meaning more detune.
There was a problem hiding this comment.
@derselbst, what do you think? Does fluid's internal sound bank structure directly replicate the soundfont model? If so, then changing the default pitch modulator for DLS will result in the DLS files being detuned from their SF2 counterparts.
There was a problem hiding this comment.
My naïve thinking is, that a DLS accurately converted to SF2 should yield the same synthesized audio output. Hence, I would vote to indeed specify the SF2 default of 12700 cents here, i.e. removing the modulator override here.
The same thought could be used to argue against overriding chorus and reverb modulators as well.
If a DLS or "DLS converted to a SoundFont" is serious about its modulator amounts, the DLS / Soundfont is free to specify / override the modulator(s) in the file itself. Otherwise it's subject to the defaults imposed by the synthesis model it will be used in.
Thinking this further, an additional argument comes to my mind: Fluidsynth provides an API for manipulating default modulators. If someone wanted to use this API to e.g. change the amount of the default pitch modulator to any other value, they would only be able to do so in the SF2 world, but not for DLS, since the modulator-override is hard-coded here.
I don't know about @rsp4jack, but I'd vote for not overriding any SF2 default modulators with DLS specific ones. (PS: You can still keep this code, but #if 0ed-out for reference and documentation purposes.)
There was a problem hiding this comment.
The same thought could be used to argue against overriding chorus and reverb modulators as well.
That should stay. The DLS specification clearly says that effects send uses the full range of the articulators (i.e. 127 equals in 100% send). Specifically, table 6, page 30.
My converter inserts these modulators too. You can open gm.dls in spessafont and you'll see the modulators overriden with 1000. An accurate DLS to SF2 conversion should indeed set the default modulators for the effects.
And one important thing to override is the vibLfo modulator for dls level 1 files (more specifically, disabling the mod wheel -> vibLFO modulator).
DLS level 1 only has the mod LFO. gm.dls for examples is a DLS level 1 file. You can see with the Piano 1 instrument that it sets the mod LFO to pitch articulation (with modulation wheel as control). If we left vibrato LFO on, both would influence the pitch and the mod wheel would behave inaccurately.
There was a problem hiding this comment.
The DLS specification clearly says
It's great if the DLS spec says that. But fluidsynth is a SF2 synth, pls. keep that in mind.
If your DLS to SF2 converter inserts the DLS overrides for those modulators, great - that's a decision which is up to you, and it's entirely valid. A user is free to change or remove those modulators again from the SF2 file.
Hardcoding the overrides here in the code might be the right thing to achieve accurate DLS support, but in context of fluidsynth it would limit its API usage and cause potentially unexpected results. If the user wants a complete DLS experience with fluidsynth, they are free to use the API to put proper modulators in place that resemble the DLS behavior. We can document how to do this. We can even add a commandline option which enables this "DLS experience mode". But hardcoding the overrides here just seems wrong, the more I think about it.
There was a problem hiding this comment.
If the user wants SF2's effects send, they can change it via the API
You're thinking it the wrong way, IMO. Fluidsynth is a SF2 synth. No one has to change anything via the API in order to get SF2-compliant output.
Also, it's not possible to manipulate DMOD modulators via the API. DMOD is part of the file. In fact, fluidsynth doesn't provide any API for manipulating modulators in the file. Solely once a voice has been spawned and all the modulators have been added to the voice, then it be manipulated via the voice API.
I truly appreciate all the effort that does into providing the best possible DLS experience, esp. with the recent Fury.dls fix. But whenever it comes to clashes between the DLS and the SF2 world, I will give SF2 precedence. I'll propose a DLS mode. Not sure when I'll be able to do so. And I'll need to think how to implement it (i.e. commandline flag vs. shell command).
There was a problem hiding this comment.
Fluidsynth provides an API for manipulating default modulators. If someone wanted to use this API to e.g. change the amount of the default pitch modulator to any other value, they would only be able to do so in the SF2 world, but not for DLS, since the modulator-override is hard-coded here.
Personally I think having a DLS implementation with good accuracy has more significance than supporting fluid_synth_add_default_mod APIs for DLS. But anyway, it's up to you.
A better solution is, probably, to have default_mod per soundbank, and then DLS can have different default mods with SF2. This could probably be a big change, so it should be implemented later if necessary.
There was a problem hiding this comment.
I definitely agree with rsp4jack, I think that if a user uploads a DLS file, they will probably expect accurate playback and won't mess with fluidsynth API. If they really need that, they can just convert the file to an SF2 file and mess with it. The documentation could explicitly say that DLS doesn't work with the API so the sound bank needs to be converted into an SF2 one. Sacrificing the most likely use-case (just using a DLS file) for something relatively obscure (messing with modulators on an API level, something which most users, probably using something like Qsynth, will not do) is a bad trade IMO.
Though derselbst is the one who has write access to this repo so our opinions don't really matter...
There was a problem hiding this comment.
Though derselbst is the one who has write access to this repo so our opinions don't really matter
Your opinions do matter, I appreciate them and will account for them when proposing an alternative approach to this.
There was a problem hiding this comment.
I have addressed this issue of hard-coded "default" modulator following rsp4jack's proposal from above, pls. have a look: #1652
EDIT: I just reformatted the codebase in that PR, you probably only want to look at the first two commits of that PR, i.e. https://github.com/FluidSynth/fluidsynth/pull/1652/files/de3707519c77cd51bcea091dd6d001677fe67b23)
|
I just found a bug so all modulators different from default modulators are ignored. I will push a fix. Now Fury.dls is audible. |
|
'pgal' chunk in MobileBAE DLS banks is supported now. I tested Nokia LLoyd Bank DLS and it sounds good. |
spessasus/spessasynth_core#14 (comment) This is the complete pgal chunk structure, hope it will be useful to you |
|
@SmithGoll Thanks for your help! I have pushed a fix. I think this PR is complete now. |
derselbst
changed the title
| return std::nullopt; | ||
| } | ||
|
|
||
| inline bool fluid_dls_font::execute_cdls(fluid_long_long_t offset, int size) |
There was a problem hiding this comment.
@rsp4jack
Are there actually any DLS files that make use of conditional chunk? Is there a point in parsing it at all?
To me it sounds like a possibility of some DLS being extracted from some proprietary software, which could declare the entire collection as only playable on some random manufacturer ID even though fluidsynth could play it just fine, but CDL would tell it to ignore it. And from what I can see in the code, if CDL fails, it throws an error. Unless I missed something, that is.
There was a problem hiding this comment.
Are there actually any DLS files that make use of conditional chunk?
AFAIK, no.
Is there a point in parsing it at all?
It is required by DLS spec, so...
if CDL fails, it throws an error. Unless I missed something, that is.
An error will be thrown only when there is an error in the CDLs (e.g. stack underflow). Otherwise it will be evaluated and bypass the chunk if it is evaluated to false.
derselbst
left a comment
derselbst
left a comment
There was a problem hiding this comment.
I gave it one final review. Looks good and works excellently - many thanks again! I'd be ready to merge it. @spessasus How about you?
I have 2 things to say:
RFC @rsp4jack |
I'm not a fan of breaking functionality that some users might be used to from one release to another. Just like glib, libinstpatch will stay in 2.5.x. (Thinking of package dependencies / package maintainers on Linux, etc.) I will announce that accordingly. I have no strong opinion on the CDL topic. I'd tend to say keep it for now and remove it later if it causes problems. I'd be fine if rsp4jack removes it right away. I'm intending to retain the full commit history anyway. |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
While working on the rest of the codebase, I'm getting more and more merge conflicts with this branch. Because of that, I'm merging the changes now. I have a few more days of vacation and would like to get 2.5.0 as ready as possible during that time. Thanks again to both of you @spessasus and @rsp4jack for testing and implementing the changes! |
|
@spessasus It is reasonable to ignore CDLs but I would like to keep it for now. If there are problems reported with it, I will consider create a patch. |
4 participants
Description
This PR introduces a native DLS loader, written in C++17. Using C++17 makes it more concise and have good readability.
It provides almost full support for DLS. Current DLS support is provided by libinstpatch, and it lacks some key DLS functionality. (see #1373)
When this PR is merged, completely getting rid of GLib will be possible! (with C++17 OSAL)
Issues (not the GitHub one)
With this PR, we will have two DLS parser. Should we just say bye-bye and throw instpatch parser away? And if it is kept here, what is it for?
The instpatch parser perhaps supports GIG and SLI but it doesn't seem to work (see Support Loading DLS Files #493, I'm not sure if libinstpatch is still the same).
If the problem above is clear, report.cmake should be changed (I have not add enable-native-dls to it yet)
Another problem I found about CXX_STANDARD:
fluidsynth/CMakeLists.txt
Lines 565 to 572 in 8d26042
fluidsynth/CMakeLists.txt
Lines 708 to 716 in 8d26042
fluidsynth/CMakeLists.txt
Lines 783 to 786 in 8d26042
If OSAL is cpp17 and OBOE is enabled, CMAKE_CXX_STANDARD will become 14, even if the compiler support 17 or 23.
I think target_compile_features should be used here.
Linked issues (this is the GitHub one)
Fixes #1373
Fixes #1426
Fixes #1584 (when libinstpatch is disabled)
#847