User Agent: Mozilla/5.0 (X11; Linux ppc; rv:17.0) Gecko/17.0 Firefox/17.0 Iceweasel/17.0 Build ID: 20121122175550 Actual results: Firefox fails to build on Debian powerpc, see https://buildd.debian.org/status/fetch.php?pkg=iceweasel&arch=powerpc&ver=17.0-1&stamp=1353519787 Expected results: The attached patch allows Firefox to build and run on this platform.

Comment on attachment 684694 [details] [diff] [review] Fix Review of attachment 684694 [details] [diff] [review]: ----------------------------------------------------------------- ::: iceweasel-17.0.orig/media/webrtc/trunk/src/modules/audio_coding/codecs/pcm16b/pcm16b.gypi @@ +12,5 @@ > 'target_name': 'PCM16B', > 'type': '<(library)', > + 'dependencies': [ > + '<(webrtc_root)/common_audio/common_audio.gyp:signal_processing', > + ], What error requires this part?

Comment on attachment 684694 [details] [diff] [review] Fix Review of attachment 684694 [details] [diff] [review]: ----------------------------------------------------------------- SSE2 tests need to be aligned. ::: iceweasel-17.0.orig/media/webrtc/shared_libs.mk @@ +41,5 @@ > $(call EXPAND_LIBNAME_PATH,yuv,$(DEPTH)/media/webrtc/trunk/third_party/libyuv/libyuv_libyuv) \ > $(call EXPAND_LIBNAME_PATH,webrtc_jpeg,$(DEPTH)/media/webrtc/trunk/src/common_video/common_video_webrtc_jpeg) \ > $(NULL) > + > +ifneq (,$(findstring 86,$(CPU_ARCH))) Is this really the right test for sse2 support? I wouldn't think so. (It could be we don't have a good test, in which case this is better than nothing.) Actually, the test should match the test used to build it in video_processing.gyp (or that test should also be changed): it looks for a target_arch of ia32 or x64. Note that target_arch != CPU_ARCH; see configure.in.

(In reply to Randell Jesup [:jesup] from comment #2) > > Is this really the right test for sse2 support? I wouldn't think so. (It > could be we don't have a good test, in which case this is better than > nothing.) I don't know what the right test is in the place(s) that include media/webrtc/shared_libs.mk . I'll gladly leave that to someone who's familiar with this. :) But it seems clear that the build is currently broken for anything that can't handle SSE2.

(In reply to Mike Hommey [:glandium] from comment #1) > What error requires this part? make[1]: Entering directory `/home/daenzer/build/iceweasel-17.0/build-xulrunner/media/webrtc/trunk/src/modules/modules_PCM16B' gcc -o audio_coding/codecs/pcm16b/pcm16b.o -c -fvisibility=hidden -DMOZ_GLUE_IN_PROGRAM -D_FILE_OFFSET_BITS=64 -DCHROMIUM_BUILD -DUSE_NSS=1 -DTOOLKIT_USES_GTK=1 -DGTK_DISABLE_SINGLE_INCLUDES=1 -D_ISOC99_SOURCE=1 -DENABLE_REMOTING=1 -DENABLE_P2P_APIS=1 -DENABLE_CONFIGURATION_POLICY -DENABLE_INPUT_SPEECH -DENABLE_NOTIFICATIONS -DENABLE_GPU=1 -DENABLE_EGLIMAGE=1 -DUSE_SKIA=1 -DENABLE_REGISTER_PROTOCOL_HANDLER=1 -DENABLE_WEB_INTENTS=1 -DENABLE_PLUGIN_INSTALLATION=1 -DWEBRTC_LINUX -DWEBRTC_THREAD_RR -D__STDC_FORMAT_MACROS -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -I. -I/home/daenzer/build/iceweasel-17.0/media/webrtc/trunk/src/modules/.. -I/home/daenzer/build/iceweasel-17.0/media/webrtc/trunk/src/modules/../.. -I/home/daenzer/build/iceweasel-17.0/media/webrtc/trunk/src/modules/audio_coding/codecs/pcm16b/include -fPIC -D_FORTIFY_SOURCE=2 -Wall -Wpointer-arith -Wdeclaration-after-statement -Werror=return-type -Wtype-limits -Wempty-body -Wno-unused -Wno-overlength-strings -Wcast-align -fstack-protector --param=ssp-buffer-size=4 -Wformat -Werror=format-security -fno-strict-aliasing -ffunction-sections -fdata-sections -pthread -pipe -DNDEBUG -DTRIMMED -g -Os -freorder-blocks -fomit-frame-pointer -D_FORTIFY_SOURCE=2 -include ../../../../../../mozilla-config.h -DMOZILLA_CLIENT -MD -MF .deps/pcm16b.o.pp /home/daenzer/build/iceweasel-17.0/media/webrtc/trunk/src/modules/audio_coding/codecs/pcm16b/pcm16b.c /home/daenzer/build/iceweasel-17.0/media/webrtc/trunk/src/modules/audio_coding/codecs/pcm16b/pcm16b.c:19:39: fatal error: signal_processing_library.h: No such file or directory compilation terminated. make[1]: *** [audio_coding/codecs/pcm16b/pcm16b.o] Error 1 Not sure why this doesn't happen on x86...

(In reply to Michel Dänzer from comment #4) > Not sure why this doesn't happen on x86... Ah: #ifdef WEBRTC_BIG_ENDIAN #include "signal_processing_library.h" #endif

(In reply to Michel Dänzer from comment #3) > (In reply to Randell Jesup [:jesup] from comment #2) > > > > Is this really the right test for sse2 support? I wouldn't think so. (It > > could be we don't have a good test, in which case this is better than > > nothing.) > > I don't know what the right test is in the place(s) that include > media/webrtc/shared_libs.mk . I'll gladly leave that to someone who's > familiar with this. :) But it seems clear that the build is currently broken > for anything that can't handle SSE2. First cut (and a reasonable one) would be to make it match the code that builds or doesn't build the SSE2 code - see my original review. Do that and I'll r+ it.

(In reply to Randell Jesup [:jesup] from comment #6) > First cut (and a reasonable one) would be to make it match the code that > builds or doesn't build the SSE2 code - see my original review. The thing is, the SSE2 code build is defined in media/webrtc/trunk/src/modules/video_processing/main/source/video_processing.gypi, but shared_libs.mk is included from layout/media/Makefile(.in). I have no idea if/how variable from the former are available in the latter.

(In reply to Michel Dänzer from comment #7) > The thing is, the SSE2 code build is defined in > media/webrtc/trunk/src/modules/video_processing/main/source/video_processing. > gypi, but shared_libs.mk is included from layout/media/Makefile(.in). I have > no idea if/how variable from the former are available in the latter. The normal test we use elsewhere in the tree is ifneq (,$(INTEL_ARCHITECTURE))

(In reply to Timothy B. Terriberry (:derf) from comment #9) > The normal test we use elsewhere in the tree is > ifneq (,$(INTEL_ARCHITECTURE)) Thanks! Hadn't seen that.

(In reply to Michel Dänzer from comment #5) > (In reply to Michel Dänzer from comment #4) > > Not sure why this doesn't happen on x86... > > Ah: > > #ifdef WEBRTC_BIG_ENDIAN > #include "signal_processing_library.h" > #endif Why does it need that?

Comment on attachment 685102 [details] [diff] [review] Fix for pcm16b.gypi with test for some big endian architectures. Review of attachment 685102 [details] [diff] [review]: ----------------------------------------------------------------- ::: iceweasel-17.0.orig/media/webrtc/trunk/src/modules/audio_coding/codecs/pcm16b/pcm16b.gypi @@ +11,5 @@ > { > 'target_name': 'PCM16B', > 'type': '<(library)', > + 'conditions': [ > + ['target_arch=="ppc32" or target_arch=="ppc64" or target_arch=="sparc"', other big endian architectures: avr32, hppa, m68k, mips (not mipsel), s390 and s390x.

(In reply to Mike Hommey [:glandium] from comment #12) > iceweasel-17.0.orig/media/webrtc/trunk/src/modules/audio_coding/codecs/ > pcm16b/pcm16b.gypi > @@ +11,5 @@ > > { > > 'target_name': 'PCM16B', > > 'type': '<(library)', > > + 'conditions': [ > > + ['target_arch=="ppc32" or target_arch=="ppc64" or target_arch=="sparc"', > > other big endian architectures: avr32, hppa, m68k, mips (not mipsel), s390 > and s390x. I'm waiting with bated breath for WebRTC on the s390. Gotta find whomever ended up with the old campus 390 from college and add some USB ports to it. ;-)

#define WEBRTC_SPL_MEMCPY_W8(v1, v2, length) \ memcpy(v1, v2, (length) * sizeof(char)) #define WEBRTC_SPL_MEMCPY_W16(v1, v2, length) \ memcpy(v1, v2, (length) * sizeof(WebRtc_Word16)) These are the macros pcm16b.c uses from signal_processing_library.h. Pretty ridiculous.

I'm going to test this on Debian.

(In reply to Randell Jesup [:jesup] from comment #13) > I'm waiting with bated breath for WebRTC on the s390. Gotta find whomever > ended up with the old campus 390 from college and add some USB ports to it. > ;-) Well, with this patch, it builds. I haven't tested if it actually works.

Comment on attachment 687403 [details] [diff] [review] Allow webrtc to build on more architectures Review of attachment 687403 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/trunk/src/modules/audio_coding/codecs/pcm16b/pcm16b.c @@ +25,5 @@ > WebRtc_Word16 len, > WebRtc_Word16 *speechOut16b) > { > #ifdef WEBRTC_BIG_ENDIAN > + memcpy(speechOut16b, speechIn16b, len * sizeof(WebRtc_Word16)); I assume there's some reason we can't include signal_processing_library.h here? Should we fix that instead? ::: media/webrtc/trunk/src/typedefs.h @@ +119,5 @@ > +#elif defined(__avr32__) > +#define WEBRTC_ARCH_AVR32 1 > +#define WEBRTC_ARCH_32_BITS 1 > +#define WEBRTC_ARCH_BIG_ENDIAN > +#define WEBRTC_BIG_ENDIAN Wow, very comprehensive.

(In reply to Randell Jesup [:jesup] from comment #17) > Comment on attachment 687403 [details] [diff] [review] > Allow webrtc to build on more architectures > > Review of attachment 687403 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: media/webrtc/trunk/src/modules/audio_coding/codecs/pcm16b/pcm16b.c > @@ +25,5 @@ > > WebRtc_Word16 len, > > WebRtc_Word16 *speechOut16b) > > { > > #ifdef WEBRTC_BIG_ENDIAN > > + memcpy(speechOut16b, speechIn16b, len * sizeof(WebRtc_Word16)); > > I assume there's some reason we can't include signal_processing_library.h > here? Should we fix that instead? The alternative is to add a *lot* of conditions in the gyp file (one for each big endian architecture, see comment 12). Using memcpy, which WEBRTC_SPL_MEMCPY_W* is anyways (see comment 14) is way simpler.

Erf, the shared_libs.mk part landed in bug 750869.

Thanks for fixing this!

I failed to notice there was a __MIPSEL__ block already (I made the original patch for 17, which doesn't have this block). It conflicts with the __mips__ block I added which also handles the big endian variant and the 64-bits variant.

I used some of the above patches but took a different approach and just enabled the signal_processing stuff to build as it appears was intended (which eliminates the issue in comment #18 etc). With the the attached three patches and some portage eclass support (append-ldflags "-logg -lvorbis") in the ebuild everything seems to build and link fine on ppc, and *should* Just Work (TM) on other big endian platforms. The library flags could obviously be added to the right place(s) by someone more familiar with the build setup than I am...

That should've been "With the three attached patches" in comment #27. Sheesh...

When I try to build mozilla-central (117348:4e18ac9b51e2) that includes this fix on my 32 bit ppc (debian) machine with webrtc I get the following error make[5]: Leaving directory `/media/nfs/usb_drive_src/firefox/mozilla-central-hg/src/build-output/security/build' make -C media/webrtc export make[5]: Entering directory `/media/nfs/usb_drive_src/firefox/mozilla-central-hg/src/build-output/media/webrtc' make[5]: *** No rule to make target `trunk/Makefile', needed by `export'. Stop. Everything builds okay with the attach patch. Are similar rules needed for the other platforms? I'm not sure why it built for others on ppc but isn't for me.

Comment on attachment 698508 [details] [diff] [review] fixes 'no rule to make trunk/Makefile' errors on ppc Review of attachment 698508 [details] [diff] [review]: ----------------------------------------------------------------- r+ but you should have a build peer

Comment on attachment 698508 [details] [diff] [review] fixes 'no rule to make trunk/Makefile' errors on ppc https://hg.mozilla.org/integration/mozilla-inbound/rev/efd019bee0ff

Comment on attachment 698508 [details] [diff] [review] fixes 'no rule to make trunk/Makefile' errors on ppc [Approval Request Comment] User impact if declined: Build failure on Linux/OpenBSD powerpc Testing completed (on m-c, etc.): It's on aurora, currently, and has been tested on powerpc by various people already. Risk to taking this patch (and alternatives if risky): NPOTB String or IDL/UUID changes made by this patch: None

Comment on attachment 698508 [details] [diff] [review] fixes 'no rule to make trunk/Makefile' errors on ppc low risk, well tested patch (NPOTB). Avoid's Build failure on Linux/OpenBSD powerpc, has been verified by a couple of users

This issue (or the part that's in the webrtc.org area - media/webrtc/trunk) needs to be filed as an upstream issue at webrtc.org, and preferably the patch put up for codereview there as well.