Fix QFile close bug with libsndfile

[Veratil]

While testing loading Krem Kaakkuja in a MSVC LMMS build, when libsndfile was unable to detect the format of the file it would cause QFile to throw an exception that the file handle was already closed. This might be a bug in libsndfile, but we can prevent it altogether by just not using QFile.

[Veratil]

Per the libsndfile API:

Every call to sf_open_fd() should be matched with a call to sf_close() to free up memory allocated during the call to sf_open_fd().

We missed this one.

[PhysSong]

On Windows, sf_wchar_open should be used instead for handling filename encoding properly. #define ENABLE_SNDFILE_WINDOWS_PROTOTYPES 1 is required for libsndfile < 1.1.0 too.

[Rossmaxx]

This might be the reason sometimes my msvc build crashes when loading any Audio file. The issue was so random that when i clean rebuild, it gets fixed on its own and i couldn't find a consistent reason.

For me, when i build lmms, sometimes it crashes when i load any audio file with the build. But when i nuke it and rebuild, there's a chance this issue might disappear.

[Veratil]

On Windows, sf_wchar_open should be used instead for handling filename encoding properly. #define ENABLE_SNDFILE_WINDOWS_PROTOTYPES 1 is required for libsndfile < 1.1.0 too.

I didn't see in the docs that sf_wchar_open should be used on Windows specifically, just that there's one for UTF16_BE encoded filenames. We're still using a QString which converts to a std::string for the file path. Should that need to be converted for Windows to a LPCWSTR?

[PhysSong]

On Windows, sf_open uses local narrow character encoding, which is lossy and can cause issues with filenames that can't be represented with the encoding.

[Veratil]

Part of the libsndfile 1.1.0 changelog:

Use UTF-8 as internal path encoding on Windows platform.

This is an internal change to unify and simplify the handling of file paths.

On the Windows platform, the file path is always converted to UTF-8 and
converted to UTF-16 only for calls to WinAPI functions.

The behavior of the functions for opening files on other platforms does not
change.

I think this means we can use the normal sf_open correct?

[Veratil]

But with libsndfile < 1.1.0 we'll need to handle it appropriately.

[PhysSong]

Looking into the source code, sndfile still internally converts the path as narrow encoding -> UTF-16 -> UTF-8, meaning we can't pass UTF-8 strings directly on Windows.

[Veratil]

QString uses 16-bit QChar which is UTF-16. With the current code then we do UTF-16 -> std::string (as byte string), that should work right?

[PhysSong]

I mean, the input to sf_open uses (possibly) lossy narrow encoding from local code page on Windows, which itself is problematic

[Rossmaxx]

Don't we have a fix for this in sndfile version 1.2.2?

[Veratil]

We're at a "it's complicated" point here. Apparently my linked version wasn't updated, so I was using a 1.2.0 or .1 DLL that was never actually updated with 1.2.2 one. Once I deleted it, it was replaced with the 1.2.2 DLL I cannot reproduce.

On one hand we should get rid of the QFile usage here, but the bug was because CMake didn't replace the DLL as expected.

[sakertooth]

On one hand we should get rid of the QFile usage here, but the bug was because CMake didn't replace the DLL as expected.

So I believe the use of QFile was done here to seamlessly handle differences between the encoding of file paths between Windows and Unix systems. However, after libsndfile 1.1.0, they changed how they handle file paths internally:

On the Windows platform, the file path is always converted to UTF-8 and
converted to UTF-16 only for calls to WinAPI functions.

I think this allows us to pass in the path using only sf_open as it should be converted to UTF-16 appropriately anyways.

[sakertooth]

Oh, I just realized @Veratil mentioned the same thing above me previously, lol.