diff mbox

[FFmpeg-devel] lavf/wavdec.c: Fix unresolved symbols on Mac and VS2015 Update 3

Message ID CAADho6PErOoY8=jTmt7Ybrn434R3P0Y2dmecbh20K_9OP-fygQ@mail.gmail.com
State New
Headers show

Commit Message

Matthew Wolenetz Dec. 14, 2016, 11:31 p.m. UTC 
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

Comments

Hendrik Leppkes Dec. 15, 2016, 8:42 a.m. UTC | #1 
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
Carl Eugen Hoyos Dec. 15, 2016, 9:06 a.m. UTC | #2 
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
Matthew Wolenetz Dec. 15, 2016, 7:43 p.m. UTC | #3 
>
> 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
>
Carl Eugen Hoyos Dec. 15, 2016, 10:16 p.m. UTC | #4 
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
Hendrik Leppkes Dec. 15, 2016, 10:54 p.m. UTC | #5 
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
Matt Oliver Dec. 16, 2016, 12:41 a.m. UTC | #6 
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.
diff mbox

Patch

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