Improve loading a DLS file and searching for instruments in it

[M-HT]

The embedded samples aren't very good, so I wanted to load a DLS file (using function EAS_LoadDLSCollection) to improve the quality, but some of the DLS file that I test weren't loaded and when a DLS file was loaded then the synthesizer was still using the embedded samples instead of samples from the DLS file.

So I made some changes to improve it:

• increase some limits to allow loading some DLS files which couldn't be loaded before
• differentiate between melodic and drum instruments when loading DLS file and when searching for instruments in DLS file
• improve searching for instruments in DLS file, with fallbacks to defaults for missing instruments

[pedrolcl]

Thank you very much for your contribution!. I just want to ask for some additional information.

First of all, I want that every PR have an issue. It may be a request for enhancements or a request for bug fixes, or documentation about something, but the issue ticket should reflect a clear motivation for the changes. The pull request is not the place to explain the motivations, only the solution, or changes applied to achieve the goals stated by the issue. An example just have been added in the issue #5 and the PR #6 . By the way: XMF is a container format embedding SMF and DLS files in a single binary file.

In this case, I would like to know:

• What have you tried? You say: "load a DLS file (using function EAS_LoadDLSCollection)". Please add a sample (minimal) program using this function. Either adding new functionality to the existing example program (preferred), or in a totally new minimal example program.
• You also say: "some of the DLS file that I test weren't loaded and when a DLS file was loaded then the synthesizer was still using the embedded samples instead of samples from the DLS file". What DLS files are failing? Please link or attach samples of failing DLS files.

Second:

This project is a fork of the AOSP repository because I wanted to add a build system that makes it possible to use and distribute the library on any OS, which is something that probably Google is not interested to do. But I don't want to fork it in the sense of adding new functionality or fixing bugs without contributing to the upstream AOSP project. In this case, it would be better to send the pull request directly to the Google AOSP repo, and if they don't want to accept or do not react, then we can apply here the patches. We may apply here the changes anyway, of course.

Finally:

If the library code is changed, like in this case, then it would be nice to have unitary tests, just to confirm that in the future other future library changes are not breaking the functionality. Just imagine that Google changes something, and we want to pick those changes: what if they break something that we fixed here? This is exactly the situation why we want to contribute patches upstream in open source projects.

[M-HT]

I'm not clear what exactly should be in an issue and what in a pull request, so I'll write everything here for now.

I'll address the second point first:

The ability to load external DLS file (function EAS_LoadDLSCollection) was removed in AOSP, so I don't know how to demonstrate the issue there.
I think the proposed changes are usable in the upstream even without it, but what's the actual upstream ?
The github project is a mirror of https://android.googlesource.com/platform/external/sonivox.git.
Are pull requests in the github mirror accepted ? Or should the pull request be made somewhere else ? I can't see how to do anything except viewing at https://android.googlesource.com.
Also, sonivox resides under external/ in AOSP, does that mean that the actual upstream (of sonivox) is somewhere else (outside of AOSP) ?

Back to the first point:

This isn't how I tested it originally, but the example program (example/sonivoxrender.c) can be modified to show the problem like this:

• define DLS_SYNTHESIZER before including eas.h

#define DLS_SYNTHESIZER 1

• add function for reading DLS file

int dlsReadAt(void *handle, void *buf, int offset, int size) {
    int ret;

    ret = fseek((FILE *) handle, offset, SEEK_SET);
    if (ret < 0) return 0;

    return fread(buf, 1, size, (FILE *) handle);
}

int dlsSize(void *handle) {
    int ret;

    ret = fseek((FILE *) handle, 0, SEEK_END);
    if (ret < 0) return ret;

    return ftell((FILE *) handle);
}

• add code to load DLS file after EAS_Init

    EAS_FILE mDLSFile;
    static const char *dlsPath = "path/to/dls_file.dls";
    mDLSFile.handle = fopen(dlsPath, "rb");
    if (mDLSFile.handle == NULL) {
        fprintf(stderr, "Failed to open %s. error: %s\n", dlsPath, strerror(errno));
        ok = EXIT_FAILURE;
        return ok;
    }
    mDLSFile.readAt = dlsReadAt;
    mDLSFile.size = dlsSize;
    result = EAS_LoadDLSCollection(mEASDataHandle, NULL, &mDLSFile);
    if (result != EAS_SUCCESS) {
        fclose(mDLSFile.handle);
        fprintf(stderr, "Failed to load DLS file\n");
        ok = EXIT_FAILURE;
        goto cleanup;
    }
    fclose(mDLSFile.handle);

The actual path to .dls file should of course be used.

I tested following DLS files:

• gm.dls, which is part of Windows - loads succesfully without any changes
• FF8.dls from the game Final Fantasy VIII - loads succesfully without any changes
• Fury.dls from https://www.ixbt.com/multimedia/dlsfury-eng.html or from https://www.ronimusic.com/smp_ios_dls_files.htm - loads only with increased limits
• DLSbyXG.dls from http://www.vgmpf.com/Wiki/index.php?title=Analog_Devices_Sound_Demo_(W32) - loads only with increased limits
• FF8.dls (Samuel Slight's Final Fantasy VIII DLS) from https://ffsf.aaron-kelley.net/download_8.html - loads only with increased limits
• Airfont_340.dls from https://www.ronimusic.com/smp_ios_dls_files.htm - loads only with increased limits

I increased the limits to load 4 of 6 tested DLS files.
But even with DLS loaded, the sample program generates the same output (as without DLS loaded), which is what the other changes are for. Should I explain them ?
Edit: To clarify, it generates the same output for midi files which don't set banks. Changing banks isn't defined in GM1 (General Midi) so midi files conforming to GM1 have no reason to set banks (even to default).

And the last point:

I don't have much experience with unitary tests, but how would you make a test for this ?
I can imagine using some .dls file, some .mid file and expected raw data which the test would compare to the generated raw data, but that's not a very good test because the generated raw data is influenced by many more things than the changes I'm proposing.

[pedrolcl]

The ability to load external DLS file (function EAS_LoadDLSCollection) was removed in AOSP, so I don't know how to demonstrate the issue there.

Yes, it was removed by this commit af41595, which I've reverted.

Are pull requests in the github mirror accepted ? Or should the pull request be made somewhere else ? I can't see how to do anything except viewing at https://android.googlesource.com/.
Also, sonivox resides under external/ in AOSP, does that mean that the actual upstream (of sonivox) is somewhere else (outside of AOSP) ?

I can't answer your questions, because I'm not affiliated with Google or AOSP. We may ask @philburk if he has some additional tips. The only thing I know is the AOSP contributing documentation and the Android tickets: examples. But they also begin the process with an issue ticket, as usual.

This isn't how I tested it originally, but the example program (example/sonivoxrender.c) can be modified to show the problem like this: define DLS_SYNTHESIZER before including eas.h
#define DLS_SYNTHESIZER 1

This is material for the issue ticket: the public header "eas.h" conditionally defines the function EAS_LoadDLSCollection() depending on the macro DLS_SYNTHESIZER. But this definition is made in the CMakeLists.txt as PRIVATE, so any downstream project needs to define the macro again. This issue needs to be fixed:

sonivox/CMakeLists.txt

Lines 115 to 131 in 82ff2ab

	target_compile_definitions( sonivox-objects PRIVATE
	UNIFIED_DEBUG_MESSAGES
	EAS_WT_SYNTH
	NUM_OUTPUT_CHANNELS=2
	_SAMPLE_RATE_22050
	MAX_SYNTH_VOICES=64
	_16_BIT_SAMPLES
	_FILTER_ENABLED
	DLS_SYNTHESIZER
	_REVERB_ENABLED
	_CHORUS_ENABLED
	#_IMELODY_PARSER
	#_RTTTL_PARSER
	#_OTA_PARSER
	#_XMF_PARSER
	#JET_INTERFACE
	)

This is material for an additional commit included in this pull request:

• either define in the CMakeLists.txt the symbol DLS_SYNTHESIZER as PUBLIC, so the downstream projects have this symbol always defined, or (preferred):
• remove this #ifdef DLS_SYNTHESIZER ... #endif from eas.cmake. Since we have DLS support in the library, it has no sense to hide the function EAS_LoadDLSCollection() behind conditional compilation.

add function for reading DLS file in example program (example/sonivoxrender.c) ...

The changed example program should be included in a commit in this PR. Please let the DLS file to be an optional argument:

sonivox/example/sonivoxrender.c

Lines 247 to 251 in 82ff2ab

	while ((c = getopt (argc, argv, "hr:w:")) != -1) {
	switch (c)
	{
	case 'h':
	fprintf (stderr, "Usage: %s [-h] [-r 0..4] [-w 0..32765] file.mid ...\n"\

would become:

while ((c = getopt (argc, argv, "hd:r:w:")) != -1) { 
      switch (c)
      {
      case 'h':
            fprintf (stderr, "Usage: %s [-h] [-d file.dls] [-r 0..4] [-w 0..32765] file.mid ...\n"\
      // ...

I tested following DLS files:

This list of successful and failing DLS files should be explained in the issue ticket.

I increased the limits to load 4 of 6 tested DLS files.
But even with DLS loaded, the sample program generates the same output (as without DLS loaded), which is what the other changes are for. Should I explain them ?

No, the code should be self-explaining. If needed, you may add comments explaining something, and use named constants instead of "magic numbers".

Edit: To clarify, it generates the same output for midi files which don't set banks. Changing banks isn't defined in GM1 (General Midi) so midi files conforming to GM1 have no reason to set banks (even to default).

That is correct. MIDI files do not have the obligation of conforming to GM, GS or XG. If MIDI files aren't prepared to be used with a DLS bank, the library should not make any effort to fix that.

I don't have much experience with unitary tests, but how would you make a test for this ?

Don't worry about it. It is nice to have that feature, but we may live without it.

[M-HT]

This isn't how I tested it originally, but the example program (example/sonivoxrender.c) can be modified to show the problem like this: define DLS_SYNTHESIZER before including eas.h
#define DLS_SYNTHESIZER 1

This is material for the issue ticket: the public header "eas.h" conditionally defines the function EAS_LoadDLSCollection() depending on the macro DLS_SYNTHESIZER. But this definition is made in the CMakeLists.txt as PRIVATE, so any downstream project needs to define the macro again. This issue needs to be fixed:

sonivox/CMakeLists.txt

Lines 115 to 131 in 82ff2ab

	target_compile_definitions( sonivox-objects PRIVATE
	UNIFIED_DEBUG_MESSAGES
	EAS_WT_SYNTH
	NUM_OUTPUT_CHANNELS=2
	_SAMPLE_RATE_22050
	MAX_SYNTH_VOICES=64
	_16_BIT_SAMPLES
	_FILTER_ENABLED
	DLS_SYNTHESIZER
	_REVERB_ENABLED
	_CHORUS_ENABLED
	#_IMELODY_PARSER
	#_RTTTL_PARSER
	#_OTA_PARSER
	#_XMF_PARSER
	#JET_INTERFACE
	)

This is material for an additional commit included in this pull request:

* either define in the CMakeLists.txt the symbol DLS_SYNTHESIZER as PUBLIC, so the downstream projects have this symbol always defined, or  (preferred):

* remove this `#ifdef DLS_SYNTHESIZER ... #endif` from `eas.cmake`. Since we have DLS support in the library, it has no sense to hide the function `EAS_LoadDLSCollection()` behind conditional compilation.

I can make a separate issue about this, but I don't know what's the correct or preferred solution, so I can't make a fix for it.

add function for reading DLS file in example program (example/sonivoxrender.c) ...

The changed example program should be included in a commit in this PR. Please let the DLS file to be an optional argument:

I can include the changed example program in a commit, but it depends on resolving the issue with DLS_SYNTHESIZER, so it will have to wait.

Edit: To clarify, it generates the same output for midi files which don't set banks. Changing banks isn't defined in GM1 (General Midi) so midi files conforming to GM1 have no reason to set banks (even to default).

That is correct. MIDI files do not have the obligation of conforming to GM, GS or XG. If MIDI files aren't prepared to be used with a DLS bank, the library should not make any effort to fix that.

The way I see it, there are two principal uses for DLS files:

1. File formats where a DLS file embedded, file formats which reference external DLS files and midi files which implicitly expect a particular DLS file without explicitly declaring it.
2. DLS as a general soundfont. Something like .sf2 file with fluidsynth. Meaning a midi file is rendered using the DLS even if the midi file isn't prepared to be used with a DLS bank.

As it is now, the library can only work for the first use case. And for this use case your comment is correct - the library doesn't need to fix any problems.
But the changes that I made are about making the library also work in the second use case.
Should these changes be part of the library or should I close this pull request and keep these changes just in my fork ?

[pedrolcl]

This is material for an additional commit included in this pull request:

* either define in the CMakeLists.txt the symbol DLS_SYNTHESIZER as PUBLIC, so the downstream projects have this symbol always defined, or  (preferred):

* remove this `#ifdef DLS_SYNTHESIZER ... #endif` from `eas.cmake`. Since we have DLS support in the library, it has no sense to hide the function `EAS_LoadDLSCollection()` behind conditional compilation.

I can make a separate issue about this, but I don't know what's the correct or preferred solution, so I can't make a fix for it.

I said that the preferred solution is to remove #ifdef DLS_SYNTHESIZER ... #endif from eas.cmake.

[pedrolcl]

The way I see it, there are two principal uses for DLS files:

1. File formats where a DLS file embedded, file formats which reference external DLS files and midi files which implicitly expect a particular DLS file without explicitly declaring it.

2. DLS as a general soundfont. Something like .sf2 file with `fluidsynth`. Meaning a midi file is rendered using the DLS even if the midi file isn't prepared to be used with a DLS bank.

As it is now, the library can only work for the first use case. And for this use case your comment is correct - the library doesn't need to fix any problems. But the changes that I made are about making the library also work in the second use case.

A library doesn't do anything by itself, but needs a program linking the library that performs whatever is necessary to fulfill the use cases. I don't believe that the library should worry about your second use case, when the programs using the library can perform MIDI bank changes.

This is a rule of software design called separation of policy and mechanism.

Should these changes be part of the library or should I close this pull request and keep these changes just in my fork ?

I am not convinced yet to accept the changes you propose. And this PR right now is incomplete, so it can't be merged.

[M-HT]

A library doesn't do anything by itself, but needs a program linking the library that performs whatever is necessary to fulfill the use cases. I don't believe that the library should worry about your second use case, when the programs using the library can perform MIDI bank changes.

The problem with this theory is that the program doesn't know which banks/instruments are in the DLS file, but the library does. So the program can't do bank changes based on this information, but the library can.

So the question remains. Should the library be usable for the second use case or not ?

[pedrolcl]

The problem with this theory is that the program doesn't know which banks/instruments are in the DLS file, but the library does. So the program can't do bank changes based on this information, but the library can.

The program may know that information, if the developer is slightly smart. My VMPK application extracts the list of banks and programs from DLS and SF2 files, and builds INS text files (Cakewalk instruments definitions) using that information. Another program that performs a similar process (for SF2 files) is Qtractor. It is not so difficult.

So the question remains. Should the library be usable for the second use case or not ?

My point remains. The library IS already usable for both use cases.

[M-HT]

The updated commit resolves issue #11.