[MPlayer-dev-eng] [PATCH]Win32: correct the encoding of filename in console message

[Reimar Döffinger]

On Sun, Aug 09, 2015 at 02:20:57AM +0800, 朱海 wrote:
> +    mb2wc = (PMB2WC)GetProcAddress(kernel32, "MultiByteToWideChar");
> +    wc2mb = (PWC2MB)GetProcAddress(kernel32, "WideCharToMultiByte");

FFmpeg already uses these unconditionally, so I think we can just
remove the address lookup and use them directly.

> +//   if have iconv and msg_charset, ensure the filename is 'msg_charset' encoding
> +//   if no iconv nor msg_charset, keep it as system default encoding
> +const char* filename_recode(const char* filename)
> +{
> +#if !defined(CONFIG_ICONV) || !defined(MSG_CHARSET)
> +
> +#if defined(__MINGW32__) || defined(__CYGWIN__)

I think we use #ifdef WIN32 or so for this usually.
But as mentioned later I am not sure it should apply
to cygwin, I think it's not unlikely if using cygwin
a console with UTF-8 support will be used, too.

> +    if (win32_use_utf8)
> +        return win_default_charset(filename);
> +#endif
> +    return filename;
> +
> +#else // CONFIG_ICONV && MSG_CHARSET
> +
> +#if defined(__MINGW32__) || defined(__CYGWIN__)
> +    if (win32_use_utf8) {
> +        if (!mp_msg_charset || !strcasecmp(mp_msg_charset, "noconv")) {
> +            return win_default_charset(filename);
> +        }

Why is this case inside the CONFIG_ICONV ifdefs, and does it really
make sense for the two cases to be different?
Why should "noconv" result in re-encoding if iconv is not available?
I think the intent is for it to ensure no charset conversion
is ever done for messages.

> +        if (!strcasecmp(MSG_CHARSET, "UTF-8") || !strcasecmp(MSG_CHARSET, "UTF8"))
> +            return filename;

As far as I can tell this is already checked inside convert_charset.
And as this is a define we should only ever allow one variant,
i.e. MSG_CHARSET should never ever be UTF8, utf8 or utf-8 but always
UTF-8.

> +        return convert_charset(filename, MSG_CHARSET, "UTF-8");
> +    }
> +#endif // fallback if not using utf8 filename for win32
> +
> +    return convert_charset(filename, MSG_CHARSET, mp_msg_charset);
> +#endif
> +}
> +
>  void mp_msg_init(void){
>      int i;
>      char *env = getenv("MPLAYER_VERBOSE");
> @@ -198,7 +262,8 @@
>      tmp[MSGSIZE_MAX-1] = 0;
> 
>  #if defined(CONFIG_ICONV) && defined(MSG_CHARSET)
> -    if (mp_msg_charset && strcasecmp(mp_msg_charset, "noconv")) {
> +    if (mp_msg_charset && strcasecmp(mp_msg_charset, "noconv")
> +            && strcasecmp(mp_msg_charset, MSG_CHARSET)) {


This seems unrelated and should be in a separate patch.

> Index: stream/stream_file.c
> ===================================================================
> diff --git a/trunk/stream/stream_file.c b/trunk/stream/stream_file.c
> --- a/trunk/stream/stream_file.c        (revision 37444)
> +++ b/trunk/stream/stream_file.c        (working copy)
> @@ -26,9 +26,10 @@
>  #if HAVE_SETMODE
>  #include <io.h>
>  #endif
> -#ifdef __MINGW32__
> +#if defined(__MINGW32__) || defined(__CYGWIN__)
>  #include <windows.h>
>  #include <share.h>
> +#include "mpcommon.h"
>  #endif
> 
>  #include "mp_msg.h"
> @@ -120,7 +121,7 @@
>    return STREAM_UNSUPPORTED;
>  }
> 
> -#ifdef __MINGW32__
> +#if defined(__MINGW32__) || defined(__CYGWIN__)
>  static int win32_open(const char *fname, int m, int omode)
>  {
>      int cnt;


I suspect cygwin was left out on purpose,
as it has its own hacks.
Either way I really think this belongs at
least in a separate patch.

> @@ -127,7 +128,11 @@
>      int fd = -1;
>      wchar_t fname_w[MAX_PATH];
>      int WINAPI (*mb2wc)(UINT, DWORD, LPCSTR, int, LPWSTR, int) = NULL;
> -    HMODULE kernel32 = GetModuleHandle("Kernel32.dll");
> +    HMODULE kernel32;
> +
> +    if (!win32_use_utf8) goto fallback;
> +
> +    kernel32 = GetModuleHandle("Kernel32.dll");
>      if (!kernel32) goto fallback;
>      mb2wc = GetProcAddress(kernel32, "MultiByteToWideChar");
>      if (!mb2wc) goto fallback;

I believe this breaks the intended behaviour.
It should be possible to pass file names as
UTF-8 to MPlayer and have them opened properly.
Coupling it to whether the arguments were passed as
wide args break that.