Message ID | CAADho6PErOoY8=jTmt7Ybrn434R3P0Y2dmecbh20K_9OP-fygQ@mail.gmail.com |
---|---|
State | New |
Headers | show |
On Thu, Dec 15, 2016 at 12:31 AM, Matthew Wolenetz <wolenetz@chromium.org> wrote: > Some toolchains failed to link a dynamic library containing wavdec.c, > but with either CONFIG_SPDIF_DEMUXER or CONFIG_W64_DEMUXER disabled. > This change adds #if's to explicitly exclude code rather than depend on > toolchain code elision of same condition using "if". > Reference https://crbug.com/591845. Adapted from 2 Chromium ffmpeg > patches for code style: > This seems like a rather odd issue to be toolchain specific. Can you please provide a configure command line that would result in a broken build? - Hendrik
2016-12-15 0:31 GMT+01:00 Matthew Wolenetz <wolenetz@chromium.org>: > Some toolchains failed to link a dynamic library containing wavdec.c, > but with either CONFIG_SPDIF_DEMUXER or CONFIG_W64_DEMUXER disabled. DCE is needed for FFmpeg compilation, this should get fixed by Steve Lhomme's configure patch. I find it hard to understand why wavdec.c would be the only file showing this issue... Carl Eugen
> > This seems like a rather odd issue to be toolchain specific. Can you > please provide a configure command line that would result in a broken > build? I believe the link-time failures occurred with configs like these: IIRC, on Mac, Chromium build hit this with: - https://cs.chromium.org/chromium/src/third_party/ffmpeg/chromium/config/Chromium/mac/x64/config.h?rcl=0&l=4 On Windows, one or both of the following hit this: - https://cs.chromium.org/chromium/src/third_party/ffmpeg/chromium/config/Chromium/win/ia32/config.h?rcl=0&l=4 - https://cs.chromium.org/chromium/src/third_party/ffmpeg/chromium/config/Chromium/win/x64/config.h?rcl=0&l=4 DCE is needed for FFmpeg compilation, this should get fixed by Steve > Lhomme's > configure patch. > I find it hard to understand why wavdec.c would be the only file > showing this issue... Notably, windows still hit this (VS2015 Update 3 19.00.24213.1) *even with -O2.* On Thu, Dec 15, 2016 at 1:06 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2016-12-15 0:31 GMT+01:00 Matthew Wolenetz <wolenetz@chromium.org>: > > Some toolchains failed to link a dynamic library containing wavdec.c, > > but with either CONFIG_SPDIF_DEMUXER or CONFIG_W64_DEMUXER disabled. > > DCE is needed for FFmpeg compilation, this should get fixed by Steve > Lhomme's > configure patch. > > I find it hard to understand why wavdec.c would be the only file > showing this issue... > > Carl Eugen > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
2016-12-15 20:43 GMT+01:00 Matthew Wolenetz <wolenetz@chromium.org>: >> >> This seems like a rather odd issue to be toolchain specific. >> Can you please provide a configure command line that >> would result in a broken build? > > I believe the link-time failures occurred with configs like these: > > IIRC, on Mac, Chromium build hit this with: > > - > https://cs.chromium.org/chromium/src/third_party/ffmpeg/chromium/config/Chromium/mac/x64/config.h?rcl=0&l=4 > > On Windows, one or both of the following hit this: > > - > https://cs.chromium.org/chromium/src/third_party/ffmpeg/chromium/config/Chromium/win/ia32/config.h?rcl=0&l=4 > - > https://cs.chromium.org/chromium/src/third_party/ffmpeg/chromium/config/Chromium/win/x64/config.h?rcl=0&l=4 These do not look like configure lines (only configure is supported)... > > DCE is needed for FFmpeg compilation, this should get fixed >> by Steve Lhomme's configure patch. >> I find it hard to understand why wavdec.c would be the only file >> showing this issue... > > Notably, windows still hit this (VS2015 Update 3 19.00.24213.1) > *even with -O2.* I believe this indicates a compiler issue that you should report upstream and for which we may not accept a workaround. Carl Eugen
On Thu, Dec 15, 2016 at 8:43 PM, Matthew Wolenetz <wolenetz@chromium.org> wrote: >> >> This seems like a rather odd issue to be toolchain specific. Can you >> please provide a configure command line that would result in a broken >> build? > > > I believe the link-time failures occurred with configs like these: > > IIRC, on Mac, Chromium build hit this with: > > - > https://cs.chromium.org/chromium/src/third_party/ffmpeg/chromium/config/Chromium/mac/x64/config.h?rcl=0&l=4 > > On Windows, one or both of the following hit this: > > - > https://cs.chromium.org/chromium/src/third_party/ffmpeg/chromium/config/Chromium/win/ia32/config.h?rcl=0&l=4 > - > https://cs.chromium.org/chromium/src/third_party/ffmpeg/chromium/config/Chromium/win/x64/config.h?rcl=0&l=4 > > DCE is needed for FFmpeg compilation, this should get fixed by Steve >> Lhomme's >> configure patch. >> I find it hard to understand why wavdec.c would be the only file >> showing this issue... > > > Notably, windows still hit this (VS2015 Update 3 19.00.24213.1) *even with > -O2.* > > You are using a custom build system, there is no way to verify any issues for us unless you can provide a configure commandline that fails on our own build-system. The patches do look like they are trying to avoid the use of DCE - and as Carl already pointed out, quite a lot of our code relies on DCE being performed by the compiler/linker, so fixing it in one or two isolated spots wouldn't really fix anything but your own customized specific build, and make our code-base less consistent within itself. DCE generally works fine with Visual Studio, and I do not know of any configure setup that makes it fail - of course with the exception of using --disable-optimizations, as that turns everything of for MSVC right now, including DCE, which makes the build impossible. - Hendrik
On 16 December 2016 at 09:54, Hendrik Leppkes <h.leppkes@gmail.com> wrote: > On Thu, Dec 15, 2016 at 8:43 PM, Matthew Wolenetz <wolenetz@chromium.org> > wrote: > >> > >> This seems like a rather odd issue to be toolchain specific. Can you > >> please provide a configure command line that would result in a broken > >> build? > > > > > > I believe the link-time failures occurred with configs like these: > > > > IIRC, on Mac, Chromium build hit this with: > > > > - > > https://cs.chromium.org/chromium/src/third_party/ > ffmpeg/chromium/config/Chromium/mac/x64/config.h?rcl=0&l=4 > > > > On Windows, one or both of the following hit this: > > > > - > > https://cs.chromium.org/chromium/src/third_party/ > ffmpeg/chromium/config/Chromium/win/ia32/config.h?rcl=0&l=4 > > - > > https://cs.chromium.org/chromium/src/third_party/ > ffmpeg/chromium/config/Chromium/win/x64/config.h?rcl=0&l=4 > > > > DCE is needed for FFmpeg compilation, this should get fixed by Steve > >> Lhomme's > >> configure patch. > >> I find it hard to understand why wavdec.c would be the only file > >> showing this issue... > > > > > > Notably, windows still hit this (VS2015 Update 3 19.00.24213.1) *even > with > > -O2.* > > > > > > You are using a custom build system, there is no way to verify any > issues for us unless you can provide a configure commandline that > fails on our own build-system. > > The patches do look like they are trying to avoid the use of DCE - and > as Carl already pointed out, quite a lot of our code relies on DCE > being performed by the compiler/linker, so fixing it in one or two > isolated spots wouldn't really fix anything but your own customized > specific build, and make our code-base less consistent within itself. > DCE generally works fine with Visual Studio, and I do not know of any > configure setup that makes it fail - of course with the exception of > using --disable-optimizations, as that turns everything of for MSVC > right now, including DCE, which makes the build impossible. Debug builds and lto still have issues with DCE with msvc and icl. Also in an original email that mentioned a similar problem https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2016-April/193454.html people also mentioned issues with clang and gold compilers due to DCE. As per the linked email thread though I dont think adding patches to fix 1 or 2 cases of DCE issues should be excepted, instead we need to work out a global solution to the issues with DCE.
From 77add30b68373c2862520513cb7992b03352b9af Mon Sep 17 00:00:00 2001 From: Matt Wolenetz <wolenetz@google.com> Date: Wed, 14 Dec 2016 15:01:37 -0800 Subject: [PATCH] lavf/wavdec.c: Fix unresolved symbols on Mac and VS2015 Update 3 Some toolchains failed to link a dynamic library containing wavdec.c, but with either CONFIG_SPDIF_DEMUXER or CONFIG_W64_DEMUXER disabled. This change adds #if's to explicitly exclude code rather than depend on toolchain code elision of same condition using "if". Reference https://crbug.com/591845. Adapted from 2 Chromium ffmpeg patches for code style: b281073a7b1ccff67b2cd8ec636facceeeb82327 5d76f94a515900260f185d5949f72ed6fa4bdd94 --- libavformat/wavdec.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/libavformat/wavdec.c b/libavformat/wavdec.c index 7176cd6..bb250ac 100644 --- a/libavformat/wavdec.c +++ b/libavformat/wavdec.c @@ -62,7 +62,8 @@ typedef struct WAVDemuxContext { static void set_spdif(AVFormatContext *s, WAVDemuxContext *wav) { - if (CONFIG_SPDIF_DEMUXER && s->streams[0]->codecpar->codec_tag == 1) { +#if CONFIG_SPDIF_DEMUXER + if (s->streams[0]->codecpar->codec_tag == 1) { enum AVCodecID codec; uint8_t *buf = NULL; int len = 1<<16; @@ -93,6 +94,7 @@ end: av_log(s, AV_LOG_WARNING, "Cannot check for SPDIF\n"); av_free(buf); } +#endif /* CONFIG_SPDIF_DEMUXER */ } #if CONFIG_WAV_DEMUXER @@ -598,8 +600,10 @@ static int wav_read_packet(AVFormatContext *s, AVPacket *pkt) AVStream *st; WAVDemuxContext *wav = s->priv_data; - if (CONFIG_SPDIF_DEMUXER && wav->spdif == 1) +#if CONFIG_SPDIF_DEMUXER + if (wav->spdif == 1) return ff_spdif_read_packet(s, pkt); +#endif /* CONFIG_SPDIF_DEMUXER */ if (wav->smv_data_ofs > 0) { int64_t audio_dts, video_dts; @@ -654,10 +658,14 @@ smv_out: if (wav->ignore_length) left = INT_MAX; if (left <= 0) { - if (CONFIG_W64_DEMUXER && wav->w64) +#if CONFIG_W64_DEMUXER + if (wav->w64) left = find_guid(s->pb, ff_w64_guid_data) - 24; else left = find_tag(wav, s->pb, MKTAG('d', 'a', 't', 'a')); +#else + left = find_tag(wav, s->pb, MKTAG('d', 'a', 't', 'a')); +#endif /* CONFIG_W64_DEMUXER */ if (left < 0) { wav->audio_eof = 1; if (wav->smv_data_ofs > 0 && !wav->smv_eof) -- 2.8.0.rc3.226.g39d4020