Fixes Unicode paths on Windows

[itsmattkc]

Fixes #406.

Unlike most other platforms, Windows' Unicode is standardized around UTF-16, an encoding not compatible with char * arrays common in C/C++. As such, to support Unicode correctly when using Win32 APIs, strings must be converted to and from UTF-16 and the Unicode versions of the APIs must be used over the ANSI versions.

This commit introduces the following:

• Utility functions for converting between UTF-8 and UTF-16LE on all platforms:

  • Platform::Utf8ToUtf16
  • Platform::Utf16ToUtf8
  • NOTE: These functions use std::codecvt_utf8_utf16 on macOS and Linux but MultiByteToWideChar and WideCharToMultiByte on Windows because MSVC gave deprecation warnings recommending the Win32 functions with the former.

• Test for these conversion functions to ensure the conversion to and from UTF-8 and UTF-16LE is correct.

• Utility wrappers for "std::ifstream" that automatically convert to and from UTF-16 on Windows so that filenames requiring Unicode encoding function correctly:

  • Platform::CreateInputFileStream
  • Platform::OpenInputFileStream

• Moves the file default compute hash function to the Platform class (Platform::CreateFileContentHash) and switches to the Win32 UTF-16 variant on Windows.

• Adds the UNICODE macro before including "Windows.h" on Windows which ensures all functions called are the Unicode variants instead of the default ANSI variants.

  • Implicitly changes functions such as GetEnvironmentVariable and SetEnvironmentVariable to their Unicode variants.

• Changes the following environment variable related functions to their Win32 Unicode variants on Windows:

  • environ -> _wenviron
  • _putenv_s -> _wputenv_s

• Updates tests using SetEnvironmentVariable to use wide string literals since that function has been switched to the Unicode variant.

In my testing, this appears to function correctly, loading configs, LUTs, and looks with non-ASCII paths, however I have not tested exhaustively. There may be more file-related functions I haven't found/wrapped.

Happy to take any suggestions or make any changes to the implementation upon request.

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ itsmattkc (459bc74)

[hodoulp]

Thanks @itsmattkc for this pull request to tackle the challenging file path character encoding difference between Windows and Linux platforms.

Even If I made several comments/suggestions, I think that you are on the right track.

Any changes in the library must now provide a unit test (a new one or updating an existing one) to demonstrate that the change works on all the supported platforms. Even if it could end-up to be a no-op for Linux platform, a unit test must validate it also.

Note: if the method DefaultComputeHash can be moved in platform then the methods Utf16ToUtf8() and Utf8ToUtf16 could then be completely private i.e. no more define in an header file.

[hodoulp]

Thanks @itsmattkc for all the efforts to fix the file path on the Windows platform.

I do not understand the rational for the latest commit. Could you explain it?

[itsmattkc]

Hey, yes I've made a few more changes the PR overall and am still in the process of finalizing it. I implemented all of your suggestions but then thought it might be good to support Unicode environment variables too (such as the OCIO variable) which has led to having to change some other things too (notably the Windows-based environment variable tests).

As for the latest commit, I assume you're referring to enforcing UTF-16LE over UTF-16BE? That's just because Windows specifically uses UTF-16LE as its Unicode encoding. When I implemented a test of the UTF-8/UTF-16 conversion functions (using pre-existing literals as "control variables"), I found it would fail in some environments because some platforms (notably the CentOS 7 CI set up here) defaulted to UTF-16BE which would lead to the strings failing to compare. Since these functions are specifically intended for Windows anyway, it seemed appropriate to just enforce UTF-16LE in the conversion.

[itsmattkc]

Okay, sorry for the delay, this PR is now ready to be re-reviewed. I've squashed my commits for neatness and updated the OP to better reflect the changes made.

[Simran-B]

• The PR covers std::ifstream, but std::ofstream is also used in a few places (not in the core code however as far as I see). I suppose output file streams should also be handled?
  • apps (ociobakelut, ociocheck, ...)
  • bindings (Python)
  • tests (CPU, GPU)
  • vendor (Photoshop)
• I wonder if there is any Big Endian Windows where UTF-16BE might be used, such as on ARM, but it appears to be Little Endian even on AArch: https://reverseengineering.stackexchange.com/questions/17922/determining-endianness-of-pe-files-windows-on-arm
• Not sure if it's worth mentioning in the commit message, but Microsoft calls UTF-8 "multi-byte" and UTF-16 "wide-character" in their APIs. Some functions have a suffix "A" for ANSI and "W" for wide-character. This isn't really visible in the PR because the UNICODE macro takes care of using the right functions.

[itsmattkc]

• Ah yeah I missed ofstream, I can add support for that too for completeness.
• Yeah, I believe for consistency, Windows is always UTF-16LE.
• I kind of allude in the commit message to the UNICODE macro switching the functions to "Unicode variants", but I can clarify a little further that functions like SetEnvironmentVariable, which by default resolves to the ANSI SetEnvironmentVariableA, now resolves to the Unicode/wide char SetEnvironmentVariableW thanks to the UNICODE macro.

[hodoulp]

Great work.
I only made comments to improve the readability of the code.

[itsmattkc]

Thanks for the review, I believe I have resolved all of your suggestions. Let me know if anything else should be changed.

[fnordware]

Windows projects usually define UNICODE in the build process, usually a Visual Studio project. By defining it in a header like we have here, we may force UNICODE on a Windows project that doesn't want it.

As a library, I think we should respond to UNICODE, not set it. If we have something that explicitly needs the Unicode version, just call it directly, i.e. use SetWindowTextW() instead of SetWindowText().

UNICODE might need to be added to the CMake stuff somewhere. For Windows it might be on by default, but there should be a way to switch it off and everything should still work.

[hodoulp]

Good catch @fnordware the UNICODE define is in a private header so it cannot propagate to any other libraries using OpenColorIO. Did you face a problem?

[fnordware]

I still think defining UNICODE in our own header is a mistake. It should be defined by the build system. It would be like defining __GNUC__ in a header.

[itsmattkc]

Hmm you might be right, I can take a look and see if it can be made agnostic without much difficulty

EDIT: Never mind, I understand what you're saying. So it would make sense to define it in CMake rather than a header?

[hodoulp]

I googled on that UNICODE topic and almost all Windows UNICODE examples define UNICODE in the build system (i.e. not in the source code).

[hodoulp]

@itsmattkc There is a missing ifdef somewhere for Linux and macOS platforms:

2: [ 902/1000] [Platform / create_temp_filename                             ] - PASSED
2: 
2: FAILED: Only supported by the Windows platform..
2: [ 903/1000] [Platform / utf8_utf16_convert                               ] - FAILED

[hodoulp]

Concerning the Windows cmake break, it could be the same fixes than the ones in the pull request #1516.

[itsmattkc]

Looks like I made a mistake in the CMake files, it should compile now

[hodoulp]

Last questions I think by approving.

[hodoulp]

Great Job.

[doug-walker]

Thanks for these improvements! I made one minor suggestion, but approving now.

Some issues were found

[hodoulp]

Great work.

[hodoulp]

@itsmattkc please, update your pull request so I can merge it.