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

Support loading SoundFonts >2GiB on Windows #629

Merged
merged 10 commits into from May 26, 2020
Merged

Support loading SoundFonts >2GiB on Windows #629

merged 10 commits into from May 26, 2020

Conversation

derselbst
Copy link
Member

@derselbst derselbst commented Mar 21, 2020

Since sizeof(long)==4 even on 64 bit Windows, big files cannot be loaded natively via the ANSI C file API. This change makes fluidsynth's file callback API use long long, which is guaranteed to be at least 64 bit wide. This change breaks ABI and therefore requires an SOVERSION bump. Resolves #628.

Although the WindowsXP CI build pipeline reports success, I don't trust that it really can be built on WinXP. @jjceresa Could you pls. compile the 2gb-win branch on WinXP, and:

  • if successful, execute the unit tests as well, or
  • if the build is not successful, could you pls. download the x86 binary from here and test whether that is able to load a soundfont?

Also, the CI pipeline that uses MinGW x86 currently fails executing the unit tests:

https://dev.azure.com/tommbrt/tommbrt/_build/results?buildId=1413&view=results

For some reason, the _fseeki64() call in fluid_is_soundfont()

if(FLUID_FSEEK(fp, 4, SEEK_CUR))

does not advance the file pointer by 4 anymore, it only advances by 3. This causes the next fread() to read 0x62667300 rather than 0x6266736B (=="sfbk" identifier). I'll deal with this after feedback from @jjceresa.

Since sizeof(long) == 4 even on 64 bit Windose, big files cannot be
loaded natively via the ANSI C file API. This change makes fluidsynth's
file callback API use long long, which is guaranteed to be at least 64
bit wide.
@derselbst derselbst added this to the 2.2 milestone Mar 21, 2020
@jjceresa
Copy link
Collaborator

jjceresa commented Mar 21, 2020

1)Here, 2gb-win branch compile successfully using VS2010 on Windows XP (32 bits CPU).
2)Running unit test test_sfont_loading read the file successfully but isn't enough pertinent to validate the change because test file VintageDreamsWaves-v2.sf2 is < 4 Gbytes.
3) Unfortunately my machine has only 2 GBytes of RAM. So I cannot go further.
4) Ofc, running fluidsynth console and loading my usual files (< 2 GBytes) works as before.

May be a simple test trying to seek above the 32bit integer limit and reading some small SF2 chunk
should be sufficient on machine with low amount of RAM ?. But ofc it would be preferable to test this with fluidsynth API on machine with sufficient RAM. May be @trolley813 could do that ?

Please, if you think I can help ask me.

@derselbst
Copy link
Member Author

Thanks @jjceresa . I wasn't sure about XP, because I read that fseeki64 might not be available.

@trolley813 If you could test the 2gb-win branch with your huge soundfont that would be great.

I'll keep thinking about how to set up a unit test for this.

@jjceresa
Copy link
Collaborator

@trolley813 in case you prefer test it without building, I can send you directly the win32 binary dll. It should work even on recent Windows.

@carlo-bramini
Copy link
Contributor

Thanks @jjceresa . I wasn't sure about XP, because I read that fseeki64 might not be available.

The presence of fseeki64() and ftelli64() depends on the version of the msvcrt you are using it seems. But this code:

_lseeki64(_fileno(fp), 0, SEEK_END);
len = _telli64(_fileno(fp));

is working fine even on Windows 98, just tried.

@jjceresa
Copy link
Collaborator

The presence of fseeki64() and ftelli64() depends on the version of the msvcrt you are using it seems. But this code: is working fine even on Windows 98, just tried.

@carlo-bramini , Thanks for the try. Please have you enough RAM to try loading a File > 2 Gbytes ?

@carlo-bramini
Copy link
Contributor

@carlo-bramini , Thanks for the try. Please have you enough RAM to try loading a File > 2 Gbytes ?

On Windows 9x/ME, this test is not possible or it is not really a good idea to do because the OS cannot handle more than 512MB of RAM by design. On NT-based Windows, such test could be done, but where to find so huge soundfonts? Or can I create it myself with some tools?

@derselbst
Copy link
Member Author

The presence of fseeki64() and ftelli64() depends on the version of the msvcrt you are using it seems.

I tried to switch to _lseeki64() on the 2gbtest branch:

https://github.com/FluidSynth/fluidsynth/blob/2gbtest/src/sfloader/fluid_sfont.c#L114-L117

However, this breaks the unit tests even more:

https://dev.azure.com/tommbrt/tommbrt/_build/results?buildId=1434&view=results

fluid_is_soundfont() fails, because the _lseeki64() doesn't do anything. I.e. the next fread() reads 0x0004cd08, just as if the seek was never executed. I read a bit and MinGW solves this problem by executing an fseek() which seeks to the current position in order to invalidate the internal buffer of the FILE structure. But it seems that this doesn't work anymore. I tried fflush(), fseek(fd, 0, SEEK_CUR) and fgetpos()/fsetpos(). Didn't make any difference. Executing it before and after the _lseeki64() call doesn't make any difference either.

I have no idea what's wrong.

@derselbst
Copy link
Member Author

Forgot to mention that I can also reproduce this locally:

Win10 Build 1803
PlatformToolset: v142
WinSDK: 10.0.17763.0

@jjceresa
Copy link
Collaborator

jjceresa commented Mar 22, 2020

@carlo-bramini , Thanks for the try. Please have you enough RAM to try loading a File > 2 Gbytes ?

On NT-based Windows, such test could be done, but where to find so huge soundfonts? Or can I create it myself with some tools?

Here, I tried building manually a huge SF2 file using Polyphone and Swami.I did that by importing/loading a file.wav (100kbytes) in the GUI Samples multiple time. (note that doing this using copy/paste menu with Swami is far more easy and faster than using Polyphone).
When saving file, both tools are limited when the size reach 2 GBytes (still on WinXP). In this case:

  • Swami produces silently a permanent .tmp file (above 2 GBytes) and doesn't change the current file under edition.

  • Polyphone crash.

Swami uses the libinstpatch library for i/o files. I will see later, how we can update libinstpatch for seeking file the same way this PR does for fluidsynth. For now I don't know where to look in the source.

@derselbst
Copy link
Member Author

Just noticed that _telli64() returns something else than _ftelli64():

  Name Value Type
  ftell(sf->sffd) 0x0003dc52 long
  _telli64(_fileno(sf->sffd)) 0x000000000003de46 __int64
  _ftelli64(sf->sffd) 0x000000000003dc52 __int64

Complete utter garbage... So, what to do instead?

  1. Use _ftelli64() and drop support for Windows98 et. al.
  2. Use @carlo-bramini 's method as presented on the mailing list (will need to ensure that the FILE's position stays the same after the function returns)
  3. Use fgetpos() and hope that it will behave like _ftelli64() (it seems to do)
  Name Value Type
  fgetpos( handle, &pos ) 0x000000000003dc52 __int64

Opinions? I sympathize with 3., but for that I need to know how fpos_t is typedefed on WinXP, Win98, etc.

And don't worry about the 2GB soundfont test. I have an idea how to set up a unit test for that. But at first we need to find a FILE API usage that does not break existing tests...

@jjceresa
Copy link
Collaborator

Opinions? I sympathize with 3., but for that I need to know how fpos_t is typedefed on WinXP, Win98, etc.

Here still using VS2010 on WinXP fpos_t is typedefed as: typedef long long fpos_t

@trolley813
Copy link
Contributor

! If you could test the 2gb-win branch with your huge soundfont that would be great.

Thank you very much! I'll try to test it.

@derselbst
Copy link
Member Author

Here still using VS2010 on WinXP fpos_t is typedefed as: typedef long long fpos_t

Really? That's good, but how can this be? According to the web, support for long long was introduced in VS2013. Could you please double check it with sizeof(fpos_t)? Just to be absolutely sure that VS2010 doesn't use some fancy preprocessor override under the hood.

@jjceresa
Copy link
Collaborator

jjceresa commented Mar 22, 2020

Really? That's good, but how can this be? According to the web, support for long long was introduced in VS2013. Could you please double check it with sizeof(fpos_t)?

Right, I was confused by VS2010's "intellisense" that displays that fpos_t is typedefed as long long but a close look in stdio.h shows this:

#if     !__STDC__
/* At this point we could switch both to long long, but we won't do that till next version to avoid any potential compat issues */
typedef __int64 fpos_t;
#define _FPOSOFF(fp) ((long)(fp))
#else
typedef long long fpos_t;
#define _FPOSOFF(fp) ((long)(fp))
#endif

As __STDC__ is not defined this leads to: typedef __int64 fpos_t;

Using printf(), sizeof(fpos_t) is 8 and sizeof(long long) is 8 also.

It seems that fgetpos should be a good candidate.

@trolley813
Copy link
Contributor

Thanks! I've tested it with this free (GPLv3) soundfont (part 1 is 3.99GiB), and it seems to work.

@derselbst derselbst marked this pull request as ready for review April 11, 2020 12:10
@derselbst derselbst merged commit 9995fd8 into master May 26, 2020
@derselbst derselbst deleted the 2gb-win branch May 26, 2020 14:54
derselbst added a commit that referenced this pull request May 12, 2021
The unit tests keep failing when compiling with MinGW x86 on x64 Windows10. This is related to the change introduced in #629. The exact reason for the failure is unknown. I assume it's a broken MinGW implementation of the function `fgetpos()` or `_fseeki64()`, as the tests run fine for WindowsXP x86. However, I have little interest in further investigation, as I don't consider using MinGWx86 on x64 to be a valid use-case.
@stgiga
Copy link

stgiga commented Mar 4, 2023

It's really interesting that my theory about why certain widespread SoundFont implementations are limited to 2GiB was proved right in the case of FluidSynth. My theory was that the code handling size treated the value as a 32-bit signed integer despite the fact that negative file size isn't possible. Thus, by using signed 32bit integers, half of the available potential for sample size was erased. This hurt 24-bit banks dramatically due to the samples being 1.5 times larger. With stereo and a non-conservative recording time, plus a larger sample rate, you can easily exceed 2GiB, thus the signed integer bug artificially limited bank quality by up to a factor of 2. My hope is that this type of fix (treating SF2 size as a uint32 or long long or other type of value able to go from zero all the way up to 2^32 rather than as a signed 32bit integer or God forbid a 32bit float) ends up in other SoundFont environment besides FluidSynth, such as Timidity and such. To me, it's a bug that became relevant whenever people started to have the disk space to store banks at sizes that were previously considered outlandish back in the olden days of soundfonts. Even in 2005 during the X-Fi and SoundFont 2.04 days there were plenty of systems that didn't have enough drive space to justify using a 4GiB bank, a size that was supported natively on Audigy, Audigy 2, and X-Fi cards without requiring software synths like the kind you would use on machines without those cards. To me, plenty of devs of various software SoundFont synths didn't really consider that people might actually hit 2^32 in file size in the future, given how drive space even in 2005 was precious. 2005 was before Macintosh machines switched over to Intel and was before Vista happened, as well as the year before extended support for Windows 9x ended. There were plenty of older PowerPC Macs around and plenty of low quality early Windows XP machines originally meant to run Windows 2000 or 98 floating around, plus other systems with drive space low enough to make 4GiB SoundFonts unrealistic to have on your machine then. So even at the time of the X-Fi there were devs who didn't think that people would need to use the full 2^32 bytes file size supported by the Audigy, Audigy 2, and X-Fi, and their code grew around that. Thus it became a popular bug, and sometimes it deterred people from even attempting banks that large, and people became used to that artificial limit, which may have accidentally kept the bug around longer because people had good banks that weren't anywhere near the limit that they thought were hood enough. Except that people, such as myself, made larger banks, and then this happened. I thank you for fixing this terrible bug!

@trolley813
Copy link
Contributor

My theory was that the code handling size treated the value as a 32-bit signed integer despite the fact that negative file size isn't possible.

However, there are some additional complications with using functions like fseek() (set the position indicator in the file), which accept an offset as a signed integer (since you can seek both forwards and backwards). If you support plenty platforms and architectures (including some long-obsolete ones), maintaining compatibility with all of them is a large pain and a major development hinderer. In particular, FluidSynth is still using C90, a nearly 35-year-old standard.

Actually, I believe that SF2 should be superseded with a new community-based "standard" (not to be confused with SF3, an unofficial extension) more suitable for modern systems, in particular it should allow 64-bit file sizes (a vast majority of modern desktop of even mobile systems are 64-bit), since the 4GiB (2^32) limit is as of now, well, not so large either.

@stgiga
Copy link

stgiga commented Mar 4, 2023

My theory was that the code handling size treated the value as a 32-bit signed integer despite the fact that negative file size isn't possible.

However, there are some additional complications with using functions like fseek() (set the position indicator in the file), which accept an offset as a signed integer (since you can seek both forwards and backwards). If you support plenty platforms and architectures (including some long-obsolete ones), maintaining compatibility with all of them is a large pain and a major development hinderer. In particular, FluidSynth is still using C90, a nearly 35-year-old standard.

Actually, I believe that SF2 should be superseded with a new community-based "standard" (not to be confused with SF3, an unofficial extension) more suitable for modern systems, in particular it should allow 64-bit file sizes (a vast majority of modern desktop of even mobile systems are 64-bit), since the 4GiB (2^32) limit is as of now, well, not so large either.

Me and a fellow SF2 maker have actually been working on such a replacement! We have used 64-bit RIFF, and we have gotten LSB Bank Select, as well as more features out of SF2. We call what we are proposing SFe. I can send a draft soon, but let me show you what an LSB Bank Select SF2 does https://drive.google.com/file/d/1cerpko6kL4SctyHG9ozYFtwePmbHX7CW/view
lsb_on_soundfont_3

The SF2 used is a modified excerpt of my FOSS JummBox SoundFont, available at http://stgiga.itch.io/jummboxsoundfont or http://musical-artifacts.com/artifacts/2722

@stgiga
Copy link

stgiga commented Mar 4, 2023

My theory was that the code handling size treated the value as a 32-bit signed integer despite the fact that negative file size isn't possible.

However, there are some additional complications with using functions like fseek() (set the position indicator in the file), which accept an offset as a signed integer (since you can seek both forwards and backwards). If you support plenty platforms and architectures (including some long-obsolete ones), maintaining compatibility with all of them is a large pain and a major development hinderer. In particular, FluidSynth is still using C90, a nearly 35-year-old standard.
Actually, I believe that SF2 should be superseded with a new community-based "standard" (not to be confused with SF3, an unofficial extension) more suitable for modern systems, in particular it should allow 64-bit file sizes (a vast majority of modern desktop of even mobile systems are 64-bit), since the 4GiB (2^32) limit is as of now, well, not so large either.

Me and a fellow SF2 maker have actually been working on such a replacement! We have used 64-bit RIFF, and we have gotten LSB Bank Select, as well as more features out of SF2. We call what we are proposing SFe. I can send a draft soon, but let me show you what an LSB Bank Select SF2 does https://drive.google.com/file/d/1cerpko6kL4SctyHG9ozYFtwePmbHX7CW/view lsb_on_soundfont_3

The SF2 used is a modified excerpt of my FOSS JummBox SoundFont, available at http://stgiga.itch.io/jummboxsoundfont or http://musical-artifacts.com/artifacts/2722

We also came up with an entirely new standard too, but this was before we found hidden features in SF2 and found ways to extend it. NGMSS

https://docs.google.com/presentation/d/1KZCuhZ7N4BP6SAI33GqUZgB8R3X58jeyf9fdJOQ9Q54/edit?usp=share_link

@trolley813
Copy link
Contributor

Thank you! I'm also started working on a new standard (something between SF2 and a (zipped) SFZ). But this is probably not a proper place to discuss it (since it's off-topic and not (directly) related to FluidSynth).

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.

Cannot load large (2GiB+) SF2 file on Windows
5 participants