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

Replaces the command line interface from ANSI to UNICODE on Windows. #1411

Closed
wants to merge 8 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Oct 21, 2024

This pull request replaces the command line interface from ANSI to UNICODE on Windows.

Closes #1410

All additional changes are between #ifdef _WIN32 and #endif.
So this only affects Windows-specific code, especially code that relies on command line strings.

include/fluidsynth/misc.h :
Added export of fluid_alloc().
This fix is for use the macro FLUID_ARRAY.
Without this fix, when linking import library for the libfluidsynth, an error would occur that the symbol "fluid_alloc" is not found.

src/bindings/fluid_filerenderer.c :

  • Converts the utf-8 filename to utf-16, and calls sf_wchar_open() which is the utf-16 version of sf_open().
    sf_open() cannot be used, because it converts the filename to an ANSI string.
  • Exceptionally calls sf_open() if the filename is "-".
    This is because sf_open() does support pipes, but not sf_wchar_open().

src/fluidsynth.c :

This procedure is inspired by FFmpeg code.
#include <shellapi.h>
/* Will be leaked on exit */
static char** win32_argv_utf8 = NULL;
static int win32_argc = 0;

/**
 * Prepare command line arguments for executable.
 * For Windows - perform wide-char to UTF-8 conversion.
 * Input arguments should be main() function arguments.
 * @param argc_ptr Arguments number (including executable)
 * @param argv_ptr Arguments list.
 */
static void prepare_app_arguments(int *argc_ptr, char ***argv_ptr)
{
    char *argstr_flat;
    wchar_t **argv_w;
    int i, buffsize = 0, offset = 0;

    if (win32_argv_utf8) {
        *argc_ptr = win32_argc;
        *argv_ptr = win32_argv_utf8;
        return;
    }

    win32_argc = 0;
    argv_w = CommandLineToArgvW(GetCommandLineW(), &win32_argc);
    if (win32_argc <= 0 || !argv_w)
        return;

    /* determine the UTF-8 buffer size (including NULL-termination symbols) */
    for (i = 0; i < win32_argc; i++)
        buffsize += WideCharToMultiByte(CP_UTF8, 0, argv_w[i], -1,
                                        NULL, 0, NULL, NULL);

    win32_argv_utf8 = av_mallocz(sizeof(char *) * (win32_argc + 1) + buffsize);
    argstr_flat     = (char *)win32_argv_utf8 + sizeof(char *) * (win32_argc + 1);
    if (!win32_argv_utf8) {
        LocalFree(argv_w);
        return;
    }

    for (i = 0; i < win32_argc; i++) {
        win32_argv_utf8[i] = &argstr_flat[offset];
        offset += WideCharToMultiByte(CP_UTF8, 0, argv_w[i], -1,
                                      &argstr_flat[offset],
                                      buffsize - offset, NULL, NULL);
    }
    win32_argv_utf8[i] = NULL;
    LocalFree(argv_w);

    *argc_ptr = win32_argc;
    *argv_ptr = win32_argv_utf8;
}

https://github.com/FFmpeg/FFmpeg/blob/8d940a07d19023a98689f353e4425a14688547e9/fftools/cmdutils.c#L171C1-L223C2

  • Regenerates the utf-8 command arguments from the utf-16 command line string, and overwrites argc and argv at main() startup.
    The new utf-8 arguments are created on the heap, and released at the end of main().
  • Code that relies on ANSI are deleted or changed.
  • Initialize the local variable "settings" in main() to NULL.
    This is because, there is a possibility that delete_fluid_settings() will be called without calling new_fluid_settings().

The operation of these codes was tested using binaries built with Microsoft Visual studio and MSYS2.
Calls from Win32API, and Windows command prompt were confirmed to work as expected.
It has not been confirmed whether the codes will operate normally in other build and execution environments.

Tested with non-ANSI characters.
for example:

  • ♬.sf2
  • ♬.mid
  • ♬.wav

In this case, an error will occur when running with Fluidsynth 2.3.7.
And the file names are not displayed correctly.

Parameter '?.sf2' not a SoundFont or MIDI file or error occurred identifying it.
Parameter '?.mid' not a SoundFont or MIDI file or error occurred identifying it.
Rendering audio to file '?.wav'..
fluidsynth: error: Failed to open audio file '?.wav' for writing

The code in the pull request fixes these issues.

Copy link
Member

@derselbst derselbst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, one comment below. @pedrolcl You also committed encoding related changes in the past, would you like to have a look at this as well?

@@ -67,6 +67,7 @@ extern "C" {

FLUIDSYNTH_API int fluid_is_soundfont(const char *filename);
FLUIDSYNTH_API int fluid_is_midifile(const char *filename);
FLUIDSYNTH_API void* fluid_alloc(size_t len);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm hesitant to make fluid_alloc public for everyone. It is only used inside libfluidsynth. I'd prefer that you use plain malloc() and free() in fluidsynth.c and do not make it public.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, and thank you for checking.
I'll replace the code with malloc and free, test it, and then make a pull request again.
Please wait a while.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link

sonarcloud bot commented Oct 26, 2024

@pedrolcl
Copy link
Contributor

pedrolcl commented Oct 26, 2024

Thanks for the PR, one comment below. @pedrolcl You also committed encoding related changes in the past, would you like to have a look at this as well?

@derselbst : This is just another method of fixing the same problems that were already reported in #128 #983 #1322 #1388 and then supposedly fixed by #718 #1325 and #1390. If the reported problems were already fixed, I don't see the need to re-fix it again. But this is more a comment for the issue report instead of the pull request.

About the PR itself: it is quite intrusive: +99 −87! and I agree with your comment about fluid_alloc(): this change requires bumping the library minor version number. We know that fluid_free() needs to be exposed, because client applications sometimes need to use it to ensure the use of the same C runtime library used to allocate memory, but I don't see the need of exposing fluid_alloc(). Please replace that.

The core of this method is CommandLineToArgvW(GetCommandLineW(),&argc) which I dislike a lot, so I would never have used it.

dev->sndfile = sf_open(filename, SFM_WRITE, &info);
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in http://libsndfile.github.io/libsndfile/api.html#open :

The sf_open() function opens the sound file at the specified path. The filename is byte encoded, but may be utf-8 on Linux, while on Mac OS X it will use the filesystem character set. On Windows, there is also a Windows specific sf_wchar_open() that takes a UTF16_BE encoded filename.

It is quite absurd that libsndfile does not allow UTF-8 file names in Windows like it does on Linux. It may be acceptable this workaround in Fluidsynth, but this should be fixed upstream (libsndfile) as well.

Even more funny: in 1.1.0 you may find this change:

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 notchange.

@ghost
Copy link
Author

ghost commented Oct 28, 2024

I'm closing this pull request due to the comment that the code is not good.
Thanks for checking, maintainer.
I wish Fluidsynth success.

@ghost ghost closed this Oct 28, 2024
@ghost ghost deleted the fluidsynth_update branch October 28, 2024 04:10
This pull request was closed.
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.

Unicode command line string support on Windows.
2 participants