[FFmpeg-devel] [PATCH v9 1/6] libavutil/wchar_filename.h: Add whcartoutf8, wchartoansi and utf8toansi

[Martin Storsjö]

On Fri, 15 Apr 2022, Nil Admirari wrote:

> These functions are going to be used in libavformat/avisynth.c
> and fftools/cmdutils.c remove MAX_PATH limit.
> ---
> libavutil/wchar_filename.h | 51 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 51 insertions(+)

I looked through this patchset now, and I think it overall looks 
acceptable - with a couple minor things I'd like touched up and clarified.

However while reviewing this, I noticed a preexisting issue regarding the 
av_fopen_utf8 function. This patchset extends the use of that function 
into fftools, which isn't great given the issue... The use is fairly 
marginal (the preset files) so I guess it can be tolerated, as it's a 
preexisting orthogonal issue.

The other question is whether it's tolerable to use more non-installed 
headers (like libavutil/wchar_filename.h) in fftools. (I'd like to have 
this point confirmed with Anton before landing the patchset.)

A couple comments on the patch below:

> diff --git a/libavutil/wchar_filename.h b/libavutil/wchar_filename.h
> index 90f08245..e22ffa8a 100644
> --- a/libavutil/wchar_filename.h
> +++ b/libavutil/wchar_filename.h
> @@ -40,6 +40,57 @@ static inline int utf8towchar(const char *filename_utf8, wchar_t **filename_w)
>     MultiByteToWideChar(CP_UTF8, 0, filename_utf8, -1, *filename_w, num_chars);
>     return 0;
> }
> +
> +av_warn_unused_result
> +static inline int wchartocp(const unsigned int code_page, const wchar_t *filename_w,
> +                            char **filename)
> +{
> +    const DWORD flags = code_page == CP_UTF8 ? MB_ERR_INVALID_CHARS : 0;
> +    const int num_chars = WideCharToMultiByte(code_page, flags, filename_w, -1,
> +                                              NULL, 0, NULL, NULL);

We don't generally use 'const' like this in ffmpeg; we use 'const' where 
it makes a functional difference (i.e. on the type that pointers point 
at), but not for plain scalar values (neither parameters nor local 
variables).


> +    if (num_chars <= 0) {
> +        *filename = NULL;
> +        return 0;
> +    }
> +    *filename = av_calloc(num_chars, sizeof(char));
> +    if (!*filename) {
> +        errno = ENOMEM;
> +        return -1;
> +    }
> +    WideCharToMultiByte(code_page, flags, filename_w, -1,
> +                        *filename, num_chars, NULL, NULL);
> +    return 0;
> +}
> +
> +av_warn_unused_result
> +static inline int wchartoutf8(const wchar_t *filename_w, char **filename)
> +{
> +    return wchartocp(CP_UTF8, filename_w, filename);
> +}
> +
> +av_warn_unused_result
> +static inline int wchartoansi(const wchar_t *filename_w, char **filename)
> +{
> +    return wchartocp(CP_THREAD_ACP, filename_w, filename);
> +}

I think this actually would be more correct to use CP_ACP, not 
CP_THREAD_ACP. A bit of googling led me to 
https://github.com/ocaml/ocaml/issues/7854 which indicates that CP_ACP is 
used for ansi file functions, regardless of the current thread specific 
codepage. (But I see that this already is used in libavformat/avisynth.c, 
so it's in one sense a preexisting issue, but I think it'd be more correct 
to use CP_ACP here.)

// Martin