FFMS / ffms2

An FFmpeg based source library and Avisynth/VapourSynth plugin for easy frame accurate access
Other
576 stars 104 forks source link

ffmsindex build fails with GCC 7.2/MinGW-w64 5.0.2 #304

Closed qyot27 closed 6 years ago

qyot27 commented 6 years ago

See title. GCC 7.2 built against MinGW-w64 5.0.2, ffms2 git HEAD as of 2017-11-23, happens on master branch (and locally on the triage branches I use for updating the C plugin, where I actually noticed this first).

[ffms2:$] make
 CXX    src/index/ffmsindex.o
src/index/ffmsindex.cpp: In function 'int wmain(int, const wchar_t**)':
src/index/ffmsindex.cpp:241:10: error: 'wstring_convert' is not a member of 'std'
     std::wstring_convert<std::codecvt_utf8_utf16<wchar_t>, wchar_t> Conversion;
          ^~~~~~~~~~~~~~~
src/index/ffmsindex.cpp:241:10: note: suggested alternative: 'wstringstream'
     std::wstring_convert<std::codecvt_utf8_utf16<wchar_t>, wchar_t> Conversion;
          ^~~~~~~~~~~~~~~
          wstringstream
src/index/ffmsindex.cpp:241:58: error: expected primary-expression before ',' token
     std::wstring_convert<std::codecvt_utf8_utf16<wchar_t>, wchar_t> Conversion;
                                                          ^
src/index/ffmsindex.cpp:241:60: error: expected primary-expression before 'wchar_t'
     std::wstring_convert<std::codecvt_utf8_utf16<wchar_t>, wchar_t> Conversion;
                                                            ^~~~~~~
src/index/ffmsindex.cpp:244:28: error: 'Conversion' was not declared in this scope
         StringStorage[i] = Conversion.to_bytes(_argv[i]);
                            ^~~~~~~~~~
src/index/ffmsindex.cpp:244:28: note: suggested alternative: '_environ'
         StringStorage[i] = Conversion.to_bytes(_argv[i]);
                            ^~~~~~~~~~
                            _environ
<builtin>: recipe for target 'src/index/ffmsindex.o' failed
make: *** [src/index/ffmsindex.o] Error 1
[ffms2:$] 

GCC (rather, libstdc++) supposedly supports codecvt as part of the rest of C++11, but I'm not sure if there's a big 'but not for Windows' disclaimer on that or not.

myrsloik commented 6 years ago

I suspect the 'but not for Windows' disclaimer is still valid. On the positive side I wouldn't mind a patch to simply use the windows native conversion functions instead since codecvt got deprecated.

nak5124 commented 6 years ago

This is not Windows specific issue but GCC. To use wstring_convert on GCC, you should include not only \ but also \.

On MSVC, locale is included in codecvt.

BlohoJo commented 6 years ago

This issue is blocking media-autobuild_suite from working.

jb-alvarado/media-autobuild_suite#646

Could someone please add the #include <locale> line to ffmsindex.cpp?

CCnut commented 6 years ago

@BlohoJo If you do not necessarily need ffms2, you can change x2642 to 1 and wait ffms2 to fix it.

myrsloik commented 6 years ago

I'm going to remove all codecvt use later tonight and solve it that way. It's deprecated anyway nowadays.

qyot27 commented 6 years ago

Could someone please add the #include line to ffmsindex.cpp?

Just adding #include <locale> doesn't actually fix it, anyway. It just shuffles the problem away to the fact that wmain isn't supported by MinGW and causes link errors related to WinMain. The only reason I hadn't responded with that yet was that real life intervened.

BlohoJo commented 6 years ago

OK, thanks very much you guys. :)

myrsloik commented 6 years ago

What does it fail on now?

qyot27 commented 6 years ago

It now fails on the wmain invocation, as mentioned above.

 CXX    ffmsindex.exe
/usr/bin/../lib/gcc/i686-w64-mingw32/7.2.0/../../../../i686-w64-mingw32/lib/../lib/libmingw32.a(lib32_libmingw32_a-crt0_c.o): In function `main':
/home/qyot27/mingw-packages/mingw-w64-v5.0.2/mingw-w64-crt/crt/crt0_c.c:18: undefined reference to `WinMain@16'
collect2: error: ld returned 1 exit status
GNUmakefile:85: recipe for target 'ffmsindex.exe' failed
make: *** [ffmsindex.exe] Error 1
[ffms2:$] 

Whether it's the 'correct' thing to do or not, but that can be averted by restoring the MinGW ifdef from an older version of ffmsindex:

diff --git a/src/index/ffmsindex.cpp b/src/index/ffmsindex.cpp
index abda2a1..e86c8a8 100644
--- a/src/index/ffmsindex.cpp
+++ b/src/index/ffmsindex.cpp
@@ -235,7 +235,19 @@ void DoIndexing() {
 } // namespace {

 #ifdef _WIN32
+#ifdef __MINGW32__
+// mingw doesn't support wmain
+extern int _CRT_glob;
+extern "C" void __wgetmainargs(int*, wchar_t***, wchar_t***, int, int*);
+int main() {
+    int argc;
+    wchar_t **_argv;
+    wchar_t **env;
+    int si = 0;
+    __wgetmainargs(&argc, &_argv, &env, _CRT_glob, &si);
+#else
 int wmain(int argc, const wchar_t *_argv[]) {
+#endif // defined(_WIN32) && !defined(__MINGW32__)
     std::vector<const char *> StringPtrs(argc);
     std::vector<std::string> StringStorage(argc);

-- 
2.14.1

Other than that, no other errors, and the compiled binary works as expected (tested with filenames 'test.mkv' and 'あの夏で待ってる.mkv').

jtanx commented 6 years ago

It should compile fine with wmain if you pass -municode to gcc.

qyot27 commented 6 years ago

From what I remember when I tried investigating the -municode option a couple weeks ago, just adding -municode to the linker flags was not enough (#defines and extern "C" have to be added, IIRC). And even if it did compile, the binary crashed immediately.

jtanx commented 6 years ago

Seems to work for me with no modifications. Although I'm not cross-compiling (msys2/mingw-w64).

./autogen.sh
./configure
make LDFLAGS=-municode
qyot27 commented 6 years ago

A) That's polluting the entire build with -municode, when the only piece that would require it is ffmsindex. B) Those instructions are for the static library, so ffmsindex is almost completely unneeded.

So let's try it anyway, using the configure-provided method for appending linker flags:

PKG_CONFIG_PATH=$HOME/ffmpeg_build_for_ffms2/32bit/lib/pkgconfig ./configure \
>     --prefix=$HOME/ffms2-avs --cross-prefix=i686-w64-mingw32- \
>     --host=i686-pc-mingw32 --enable-shared --enable-avisynth \
>     --enable-vapoursynth --enable-debug --extra-cppflags="-mfpmath=sse \
>     -march=pentium3 -msse -mtune=pentium3" --extra-ldflags="-municode"
no working c compiler found
! cxx_check && die no working c++ compiler found

config.log:

checking whether i686-w64-mingw32-gcc works... no
Failed commandline was:
--------------------------------------------------
i686-w64-mingw32-gcc conftest.c  -Wall  -municode 
/usr/bin/../lib/gcc/i686-w64-mingw32/7.2.0/../../../../i686-w64-mingw32/lib/../lib/libmingw32.a(lib32_libmingw32_a-crt0_w.o): In function `wmain':
/home/qyot27/mingw-packages/mingw-w64-v5.0.2/mingw-w64-crt/crt/crt0_w.c:23: undefined reference to `wWinMain@16'
collect2: error: ld returned 1 exit status
--------------------------------------------------
DIED: no working c compiler found
! cxx_check && die no

Omitting the --extra-ldflags and forcing -municode through make LDFLAGS+="-municode" results in a flood of linker errors because it loses the FFmpeg libs.

myrsloik commented 6 years ago

Would another way to "solve" it without resorting to hack and ifdefs simply be to add a configure switch to skip ffmsindex?

jtanx commented 6 years ago

@qyot27 the errors you're seeing during the configure stage while having -municode on is because the configure tests use main instead of wmain (e.g. the inverse condition to ffmsindex). That's why I manually specified it on the make step.

If you wanted to integrate this into the build script, something like this should work:

diff --git a/Makefile.am b/Makefile.am
index 6e17d62..f065ed8 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -49,3 +49,4 @@ include_HEADERS = $(top_srcdir)/include/ffms.h $(top_srcdir)/include/ffmscompat.
 bin_PROGRAMS = src/index/ffmsindex
 src_index_ffmsindex_SOURCES = src/index/ffmsindex.cpp
 src_index_ffmsindex_LDADD = src/core/libffms2.la
+src_index_ffmsindex_LDFLAGS = @src_index_ffmsindex_LDFLAGS@
diff --git a/configure.ac b/configure.ac
index 00b3429..114c7ea 100644
--- a/configure.ac
+++ b/configure.ac
@@ -149,8 +149,10 @@ AC_MSG_RESULT($bsymbolic)

 if echo "$host" | $GREP "mingw" >/dev/null 2>&1; then
     LTUNDEF="-no-undefined"
+    src_index_ffmsindex_LDFLAGS="$src_index_ffmsindex_LDFLAGS -municode"
 fi
 AC_SUBST([LTUNDEF])
+AC_SUBST([src_index_ffmsindex_LDFLAGS])

 AC_CONFIG_FILES([
 Makefile

Although your comments about:

Omitting the --extra-ldflags and forcing -municode through make LDFLAGS+="-municode" results in a flood of linker errors because it loses the FFmpeg libs.

Are interesting; I never saw that during the build process when municode was specified everywhere. Would you mind sharing what those linker errors are?

qyot27 commented 6 years ago

After some more tinkering, municode is passing correctly and the binary works as expected. No more errors on the triage branch.

BlohoJo commented 6 years ago

Is this going to get fixed any time soon? I know things take time and I don't mean to sound like I'm saying "hurry up" or anything... I'm just trying to figure out if it's worth waiting for a fix or not and just go ahead and omit ffms2 from the build. Which depends if this is going to take days, weeks, months, or just not get fixed...

Yes I know it's Christmas. Merry Christmas! :)

myrsloik commented 6 years ago

As soon as someone makes a sane pull request. I don't use mingw so I'm not going to do it myself.

BlohoJo commented 6 years ago

media-autobuild_suite has been patched so that's now working with ffms2. :P