Message ID | 20220415080748.18517-1-nil-admirari@mailo.com |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel,v9,1/6] libavutil/wchar_filename.h: Add whcartoutf8, wchartoansi and utf8toansi | expand |
Context | Check | Description |
---|---|---|
yinshiyou/make_loongarch64 | success | Make finished |
yinshiyou/make_fate_loongarch64 | success | Make fate finished |
andriy/make_x86 | success | Make finished |
andriy/make_fate_x86 | success | Make fate finished |
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
> 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... Reverted back to fopen(). > 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.) Currently the header consist entirely of static inline functions. If it's not OK to use it here, please suggest a better place for these functions. > 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). Removed const from everywhere except pointer arguments. > I think this actually would be more correct to use CP_ACP, not > CP_THREAD_ACP. Changed to CP_ACP. New version of the patch: https://ffmpeg.org/pipermail/ffmpeg-devel/2022-April/295569.html.
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); + 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); +} + +av_warn_unused_result +static inline int utf8toansi(const char *filename_utf8, char **filename) +{ + wchar_t *filename_w = NULL; + int ret = -1; + if (utf8towchar(filename_utf8, &filename_w)) + return -1; + + if (!filename_w) { + *filename = NULL; + return 0; + } + + ret = wchartoansi(filename_w, filename); + av_free(filename_w); + return ret; +} #endif #endif /* AVUTIL_WCHAR_FILENAME_H */