807492 - Support BSDs for WebRTC

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P3)

Comment 1

[Jan Beich]

After playing Tetris with the build I have a few random comments:
- (signaling, audio_device, etc) mimicking OS X where no difference
- (system_wrappers/cpu_bsd) not implemented yet; kern.cp_times for per-cpu stats?
- (audio_device) alsa with oss plugin may be easier to reuse than portaudio
- (video_capture) freebsd provides linux v4l2 devices via webcamd port
- alsa/v4l2 can be installed from pkgsrc

http://trillian.chruetertee.ch/freebsd-gecko/browser/trunk/www/firefox-nightly/files/patch-bug807492

Pretty much any test shows issues with audio

  $ firefox -P test -no-remote http://people.mozilla.com/~anarayanan/webrtc/
  ALSA lib control.c:953:(snd_ctl_open_noupdate) Invalid CTL
  ALSA lib control.c:953:(snd_ctl_open_noupdate) Invalid CTL
  ALSA lib control.c:953:(snd_ctl_open_noupdate) Invalid CTL
  ALSA lib control.c:953:(snd_ctl_open_noupdate) Invalid CTL
  ALSA lib control.c:953:(snd_ctl_open_noupdate) Invalid CTL
  ALSA lib pcm.c:2219:(snd_pcm_open_noupdate) Unknown PCM
  ALSA lib control.c:953:(snd_ctl_open_noupdate) Invalid CTL
  ALSA lib pcm.c:2219:(snd_pcm_open_noupdate) Unknown PCM
  [...]
  WARNING: no real random source present!

The last warning is probably related to bug 798206.

Simple WebRTC Data Channel Test works

  pc2 got remote stream from pc1 audio
  pc1 got remote stream from pc2 audio
  HIP HIP HOORAY
  connect for data channel called
  pc1 onConnection
  pc1 created channel [object DataChannel] binarytype = blob
  pc1 new binarytype = blob
  pc1 state:Open
  pc1 onopen fired for [object DataChannel]
  pc1 state: undefined
  pc2 onConnection
  pc2 onDataChannel [0] = [object DataChannel], label='This is pc1'
  pc2 created channel [object DataChannel] binarytype = blob
  pc2 new binarytype = blob
  *** pc2 state:Connecting
  *** pc2 no onopen??! possible race
  *** pc2 onopen fired, sending to [object DataChannel]
  pc2 said: pc2 says Hi there!
  *** pc1 said: pc1 says Hello..., length=17
  *** pc1 said: blah, length=4
  pc2 said: huh?
  *** pc1 sent Blob: [object Blob], length=105
  *** pc2 sent Blob: [object Blob], length=804

Other demos seem to require a cam/mic that I don't have.

Comment 2

[Jan Beich]

Attachment: Bug 807492 - Disable SSE4.1 code in libyuv if binutils is too old.

libyuv fails to build with binutils 2.17.50, the version shipped with FreeBSD. Not clue why this wasn't fixed as part of bug 788955 or bug 800798.

{standard input}: Assembler messages:
{standard input}:51: Error: no such instruction: `pmulld %xmm6,%xmm0'
{standard input}:57: Error: no such instruction: `pmulld %xmm5,%xmm3'
{standard input}:61: Error: no such instruction: `pmulld %xmm5,%xmm4'
{standard input}:66: Error: no such instruction: `pmulld %xmm5,%xmm2'
{standard input}:69: Error: no such instruction: `pmulld %xmm5,%xmm1'
gmake[5]: *** [source/compare.o] Error 1

Comment 3

[Jan Beich]

(In reply to Jan Beich from comment #2)
> bug 788955

Typo. It should have been bug 798921.

Comment 4

[Jan Beich]

Attachment: Bug 807492 - Don't treat unsupported platforms in GYP as Linux.

Comment 5

[Landry Breuil (:gaston)]

(In reply to Jan Beich from comment #1)
> After playing Tetris with the build I have a few random comments:
> - (signaling, audio_device, etc) mimicking OS X where no difference
> - (system_wrappers/cpu_bsd) not implemented yet; kern.cp_times for per-cpu
> stats?
> - (audio_device) alsa with oss plugin may be easier to reuse than portaudio
> - (video_capture) freebsd provides linux v4l2 devices via webcamd port
> - alsa/v4l2 can be installed from pkgsrc

For OpenBSD :
- No alsa, direct oss might be desirable as a fallback, otherwise we need an sndio backend (like it was done for cubeb) : http://www.openbsd.org/cgi-bin/man.cgi?query=sndio
- v4l2 support via video(4) http://www.openbsd.org/cgi-bin/man.cgi?query=video&sektion=4

Btw, insane job on your (free)bsd patch, but shouldnt that work be done directly against upstream webrtc.org, then backported here ? randall, what do you think about it, since iirc you're part of upstream ?

When i'm done unfu**** m-c on ppc (bug 792085) i'll take your patch as a start to see what pieces are missing for OpenBSD.

Comment 6

[Martin Husemann]

NetBSD situation is very similar to OpenBSD: no native alsa on NetBSD either, v4l via video(4)

Comment 7

[Jan Beich]

Attachment: Bug 807492 - Define os_bsd early so it can be used later in the file.

I'm switching to use os_bsd to avoid typos and reduce porting burden for other forks.

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 677431 [details] [diff] [review]
Bug 807492 - Disable SSE4.1 code in libyuv if binutils is too old.

Review of attachment 677431 [details] [diff] [review]:
-----------------------------------------------------------------

This may help Seamonkey as well.

r? to a build peer

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 677433 [details] [diff] [review]
Bug 807492 - Don't treat unsupported platforms in GYP as Linux.

Review of attachment 677433 [details] [diff] [review]:
-----------------------------------------------------------------

Can we do this in a way that doesn't touch common.py?  And then submit an Issue at webrtc.org (or wherever gyp's issue tracker is) to correct that?  If we need to take in our tree, we can, but I try to limit patches to upstream

Revector to Ted

Comment 10

[Jan Beich]

(In reply to Randell Jesup [:jesup] from comment #9)
> Can we do this in a way that doesn't touch common.py?

Sure, common.py is unused with mozilla build. But OS=="dragonfly" and OS=="netbsd" wouldn't make sense for upstream then. While chromium may not build for those systems firefox certainly does, at least the version in pkgsrc.

> And then submit an Issue at webrtc.org (or wherever gyp's issue tracker is) to correct that?

Mailing our chromium team in case they can help.
http://lists.freebsd.org/pipermail/freebsd-chromium/2012-November/000548.html

Doing upstream work myself needs investing lots of time:
- cannot build even www/chromium port (local?)
- google account signup doesn't work with tor:
  "The page you requested is invalid."

Comment 11

[Jan Beich]

Attachment: Bug 807492 - Don't treat unsupported platforms in GYP as Linux.

v2 based on gyp from svn + a random discussion
http://groups.google.com/group/gyp-developer/browse_thread/thread/1bf12be2cc4630d5

Comment 12

[(not currently active) Ted Mielczarek]

Comment on attachment 680313 [details] [diff] [review]
Bug 807492 - Don't treat unsupported platforms in GYP as Linux.

Review of attachment 680313 [details] [diff] [review]:
-----------------------------------------------------------------

r- for the core GYP changes. Changes to mozmake.py are fine.

::: media/webrtc/trunk/tools/gyp/pylib/gyp/common.py
@@ +369,5 @@
> +    return params['flavor']
> +  if platform in flavors:
> +    return flavors[platform]
> +
> +  return platform

I don't want to take local GYP patches, that's going to make merging complicated. (If jesup wants to deal with that that's up to him.)

::: media/webrtc/trunk/tools/gyp/pylib/gyp/generator/mozmake.py
@@ +128,5 @@
> +    return params['flavor']
> +  if platform in flavors:
> +    return flavors[platform]
> +
> +  return platform

I don't think we actually use flavor for anything substantial in mozmake, so this should be fine.

Comment 13

[(not currently active) Ted Mielczarek]

Comment on attachment 677431 [details] [diff] [review]
Bug 807492 - Disable SSE4.1 code in libyuv if binutils is too old.

Review of attachment 677431 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +1437,5 @@
> +    AC_TRY_COMPILE([asm ("pmulld %xmm6,%xmm0");],,AC_MSG_RESULT([yes])
> +                     [HAVE_TOOLCHAIN_SUPPORT_MSSE4_1=1],
> +                     AC_MSG_RESULT([no]))
> +    CFLAGS=$_SAVE_CFLAGS
> +    AC_SUBST(HAVE_TOOLCHAIN_SUPPORT_MSSE4_1)

If you're not using this in a Makefile.in then you don't actually need to AC_SUBST here.

Comment 14

[Jan Beich]

Attachment: Bug 807492 - Add BSD flavoring to GYP.

removed common.py hunk, v2 for mozmake.py changes. 'bsd' flavor is needed by make.py (for flock command) while chromium already uses OS==freebsd and OS==openbsd.

Comment 15

[Jan Beich]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #13)
> If you're not using this in a Makefile.in then you don't actually need to
> AC_SUBST here.

I want to leave AC_SUBST there for consistency with -mssse3 check immediately before.

Comment 16

[(not currently active) Ted Mielczarek]

Comment on attachment 684281 [details] [diff] [review]
Bug 807492 - Add BSD flavoring to GYP.

Review of attachment 684281 [details] [diff] [review]:
-----------------------------------------------------------------

Looks mostly good, but I have one issue.

::: media/webrtc/trunk/tools/gyp/pylib/gyp/generator/mozmake.py
@@ +125,5 @@
> +    'sunos'    : 'solaris',
> +    'dragonfly': 'bsd',
> +    'freebsd'  : 'bsd',
> +    'netbsd'   : 'bsd',
> +    'openbsd'  : 'bsd',

If chromium is using "freebsd" and "openbsd", do you want to go for consistency with them? (I don't really have an opinion.)

@@ +141,5 @@
>  
>  def CalculateVariables(default_variables, params):
> +  flavor = GetFlavor(params)
> +  if flavor == 'bsd':
> +    flavor = platform.system().lower()

I don't understand why you're special-casing this here when you could just handle it in GetFlavor above.

Comment 17

[Landry Breuil (:gaston)]

Trying to move forward on this - given that attachments #677431 and #677734 have r+, can i assume the can be landed safely (maybe after unbitrotting them) ? 

As for att #684281 if i understand it right it needs more work ?

Comment 18

[Landry Breuil (:gaston)]

Sidenote : after taking the patch from http://trillian.chruetertee.ch/freebsd-gecko/browser/trunk/www/firefox-nightly/files/patch-bug807492 and massaging it a bit for non-alsa OSes - there's still a long way before having it in a commitable state, since webrtc seems to rely/depend on netwerk/sctp and this part is not ported at all to the other bsds.

Comment 19

[Jan Beich]

(In reply to Landry Breuil (:gaston) from comment #18)
> after taking the patch from
> http://trillian.chruetertee.ch/freebsd-gecko/browser/trunk/www/firefox-
> nightly/files/patch-bug807492 and massaging it a bit for non-alsa OSes

Only Linux has native ALSA. PulseAudio may be a better choice given it
works on a lot more systems and already has both audio_device and
libcubeb backends.

> there's still a long way before having it in a commitable state

Ignore gyp changes for now, they're too hard to do right without
upstream coordination.

> webrtc seems to rely/depend on netwerk/sctp and this part is not ported at
> all to the other bsds.

It didn't initially compile on FreeBSD, see bug 790343.

Comment 20

[Landry Breuil (:gaston)]

(In reply to Jan Beich from comment #19)
> (In reply to Landry Breuil (:gaston) from comment #18)
> > after taking the patch from
> > http://trillian.chruetertee.ch/freebsd-gecko/browser/trunk/www/firefox-
> > nightly/files/patch-bug807492 and massaging it a bit for non-alsa OSes
> 
> Only Linux has native ALSA. PulseAudio may be a better choice given it
> works on a lot more systems and already has both audio_device and
> libcubeb backends.

Ah, from your patch i understood you were planning to use alsa on freebsd - i'll see what needs to be done to enable pulse on bsd, but i'll also investigate how to write support for our sndio rather than depending on pulse..


> > webrtc seems to rely/depend on netwerk/sctp and this part is not ported at
> > all to the other bsds.
> 
> It didn't initially compile on FreeBSD, see bug 790343.

Tracked in #844430 now.

randall, ted, can/should i unbitrot/push the two r+ed patches ?

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

yes, please

Comment 22

[Landry Breuil (:gaston)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bb5fb2ab811
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7ae0dbd36a7

leave open for the remaining wip..

Comment 23

[Landry Breuil (:gaston)]

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/8500e710ece7 for configure bustage on all platforms but android :

TypeError: bad operand type for unary +: 'str' while reading /builds/slave/m-in-l64-d-0000000000000000000/build/media/webrtc/trunk/third_party/libyuv/libyuv.gyp 

Nice!

Comment 24

[Landry Breuil (:gaston)]

Doh, landed with a damn typo adding an erroneous + to chunk 1 of media/webrtc/trunk/third_party/libyuv/libyuv.gyp patch. Fixin'.

Comment 25

[Landry Breuil (:gaston)]

Relanded without the typo: https://hg.mozilla.org/integration/mozilla-inbound/rev/818d4459953d
All green on try: https://tbpl.mozilla.org/?tree=Try&rev=08f556dfe80a

Comment 26

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/818d4459953d

Comment 27

[Landry Breuil (:gaston)]

Fwiw about the gyp changes, the following got commited in upstream gyp : https://codereview.chromium.org/12389082/

gyp's common.py now properly recognizes freebsd and openbsd flavors.

Comment 28

[Landry Breuil (:gaston)]

Attachment: FreeBSD's patchset for webrtc

Trying to move forward on this, here's the patch from http://trillian.chruetertee.ch/freebsd-gecko/browser/trunk/www/firefox-nightly/files/patch-bug807492 massaged to apply against m-c.

It still contains the mozmake.py changes from attachment #648281 [details], maybe those could be separated from the whole patchset. I dont really know in which direction to move with it - what is GetFlavor() besides a huge dictionary anyway ? ted ?

Setting feedback? to see if it's in the right direction.

Comment 29

[Landry Breuil (:gaston)]

Attachment: OpenBSD fixes on top of FreeBSD patchset

And here's the patch to apply on top of att #723077, that allows me to build m-c with webrtc enabled on OpenBSD (it also needs the patches from bug #844818 & bug #844430 which are pending review)

Some details on the errors i encountered and fixed in this patch :

media/webrtc/trunk/webrtc/system_wrappers/source/trace_posix.cc:58:17: error: static_cast from 'long *' to 'time_t *' (aka 'int *') is not allowed
    localtime_r(static_cast<time_t *>(&system_time_high_res.tv_sec), &buffer);

i took the same fix as seen in  netwerk/sctp/user_socket.c -> (const time_t *) cast


media/mtransport/third_party/nrappkit/src/port/generic/include/sys/queue.h
__offsetof is not defined on OpenBSD. Apparently only FreeBSD has __offsetof as visible to userland ('offsetof' is with _KERNEL), netbsd and openbsd have plain offsetof, hence change

-#if !defined(BSD) && !defined(DARWIN)
+#if !defined(__FreeBSD__) && !defined(DARWIN)


/src/mozilla-central/media/mtransport/third_party/nrappkit/src/util/util.c
:361:72: error: use of undeclared identifier 'errno'
      r_log(LOG_GENERIC,LOG_CRIT,"Couldn't open PID file: %s",strerror(errno));

-> include <errno.h> inconditionally, cant harm

- net/if_var.h doesnt exist on NetBSD and OpenBSD

- OpenBSD doesnt have plain <tuple> header, only <tr1/tuple>. I could really find a simpler way than defining GTEST_USE_OWN_TR1_TUPLE=1 in all the gyp places where gtest header is included. Otherwise builds blows with missing include path to <tuple>. Apparently darwin has the same issue - better fix welcome.

media/webrtc/trunk/webrtc/modules/audio_device/linux/latebindingsymboltable_linux.cc
uses dlerror() which returns a const char* per the spec, so add the const. I hope it doesnt break on others, but given that GetDllError() returns a static const char*, that should be more correct.

provide a proper implem with for GetThreadId() casting pthread_self() to <uintptr_t> in media/webrtc/trunk/webrtc/system_wrappers/source/thread_posix.cc - apparently it seems we have an undocumented getthrid() on OpenBSD returning a pid_t

media/mtransport/third_party/nrappkit/src/util/util.c:568:1: warning: no previous prototype for function 'inet_ntop' [-Wmissing-prototypes]
inet_ntop(int af, const void *src, char *dst, size_t size)

Maybe only a warning, but triggered by USE_OWN_INET_NTOP added for all BSDs, why ? Removing that define builds without errors on OpenBSD, and i'm pretty sure FreeBSD has a correct inet_ntop() implem. Jan ?

Pushed the 4 patches to https://tbpl.mozilla.org/?tree=Try&rev=ea09e15928ac - only built-tested on amd64 with clang, and did no runtime-testing yet besides starting nightly via ssh X forwarding and ensuring it didnt crash right away.

Feedback and nits welcome.

Comment 30

[Jan Beich]

Comment on attachment 723077 [details] [diff] [review]
FreeBSD's patchset for webrtc

Review of attachment 723077 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Landry Breuil (:gaston) from comment #28)
> Trying to move forward on this, here's the patch from
> http://trillian.chruetertee.ch/freebsd-gecko/browser/trunk/www/firefox-
> nightly/files/patch-bug807492 massaged to apply against m-c.

Massaged for hg qimport bz:807492 or git bz apply 807492 ? ;)
It should apply just fine with patch -p0.

::: media/webrtc/trunk/webrtc/build/common.gypi
@@ +132,5 @@
> +      ['OS=="openbsd" or OS=="netbsd"', {
> +        'include_pulse_audio%': 1,
> +      }, {
> +        'include_pulse_audio%': 0,
> +      }],

On one hand build_with_chromium==0 no longer uses pulseaudio,
on the other bug 844818 overrides the above unless you specify --enable-pulseaudio.

The hunk should have been a part of attachment 723079 [details] [diff] [review].

Comment 31

[Jan Beich]

Comment on attachment 723079 [details] [diff] [review]
OpenBSD fixes on top of FreeBSD patchset

f+ for builds fine here, on FreeBSD

(In reply to Landry Breuil (:gaston) from comment #29)
> - OpenBSD doesnt have plain <tuple> header, only <tr1/tuple>. I could really
> find a simpler way than defining GTEST_USE_OWN_TR1_TUPLE=1 in all the gyp
> places where gtest header is included. Otherwise builds blows with missing
> include path to <tuple>. Apparently darwin has the same issue - better fix
> welcome.

Try newer libstdc++ or upstream fix at
https://code.google.com/p/googletest/source/detail?r=629

> media/webrtc/trunk/webrtc/modules/audio_device/linux/latebindingsymboltable_linux.cc
> uses dlerror() which returns a const char* per the spec, so add the const. I
> hope it doesnt break on others, but given that GetDllError() returns a static
> const char*, that should be more correct.

What spec?

http://pubs.opengroup.org/onlinepubs/9699919799/functions/dlerror.html

> media/mtransport/third_party/nrappkit/src/util/util.c:568:1: warning: no previous prototype for function 'inet_ntop' [-Wmissing-prototypes]
> inet_ntop(int af, const void *src, char *dst, size_t size)
>
> Maybe only a warning, but triggered by USE_OWN_INET_NTOP added for all BSDs,
> why ? Removing that define builds without errors on OpenBSD, and i'm pretty
> sure FreeBSD has a correct inet_ntop() implem. Jan ?

Doh, I've got the ifdef logic backwards.

Comment 32

[Jan Beich]

Except for include_pulse_audio and <tr1/tuple> workaround I've merged
Landry's fixes into a patch for mozilla20 and pinged [e-mail].
It's a dogfooding attempt to catch a few more bugs in the patchset.

http://trillian.chruetertee.ch/freebsd-gecko/browser/trunk/www/firefox/files/patch-bug807492

Note, bug 844818 doesn't work with --enable-pulseaudio on mozilla20.
I couldn't track which GYP change on mozilla-central fixed it but
something in bug 830247 could be the one.

Comment 33

[(not currently active) Ted Mielczarek]

Comment on attachment 723079 [details] [diff] [review]
OpenBSD fixes on top of FreeBSD patchset

Review of attachment 723079 [details] [diff] [review]:
-----------------------------------------------------------------

The build bits look fine to me.

Comment 34

[Landry Breuil (:gaston)]

(In reply to Jan Beich from comment #31)
> Comment on attachment 723079 [details] [diff] [review]
> OpenBSD fixes on top of FreeBSD patchset
> 
> f+ for builds fine here, on FreeBSD
> 
> (In reply to Landry Breuil (:gaston) from comment #29)
> > - OpenBSD doesnt have plain <tuple> header, only <tr1/tuple>. I could really
> > find a simpler way than defining GTEST_USE_OWN_TR1_TUPLE=1 in all the gyp
> > places where gtest header is included. Otherwise builds blows with missing
> > include path to <tuple>. Apparently darwin has the same issue - better fix
> > welcome.
> 
> Try newer libstdc++ or upstream fix at
> https://code.google.com/p/googletest/source/detail?r=629

Oh, this is indeed much better. That allows me to get rid of all the -DGTEST_USE_OWN_TR1_TUPLE=1 lines for OpenBSD in the gyp files. I'll post a new patchset with that.

> > media/webrtc/trunk/webrtc/modules/audio_device/linux/latebindingsymboltable_linux.cc
> > uses dlerror() which returns a const char* per the spec, so add the const. I
> > hope it doesnt break on others, but given that GetDllError() returns a static
> > const char*, that should be more correct.
> 
> What spec?
> 
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/dlerror.html

Right, i was mistaken. Anyway, GetDllError() returns a const char * so.. it's more correct this way.

We're approaching a reviewable patch imo, now to do some real-world testing...

Comment 35

[Landry Breuil (:gaston)]

I've done some real-time testing on OpenBSD/amd64, didnt get crashes but no positive results.

All tests from http://mozilla.github.com/webrtc-landing/ or https://apprtc.appspot.com/ or http://simpl.info/getusermedia/ or http://srdemo.alanta.com/ do nothing here, and i didnt find the way (yet) to make webrtc more verbose.

Comment 36

[Landry Breuil (:gaston)]

Sidenote on dlerror() : it was wrong on our side and got fixed in http://marc.info/?l=openbsd-cvs&m=136408915614138&w=2

Comment 37

[Landry Breuil (:gaston)]

I resumed working on this - but with some changes in upstream libyuv or webrtc, it seems the yuv_disable_asm chunk (which was supposed to do the trick for say, x86) doesnt work anymore, see bug 878446.

Comment 38

[Landry Breuil (:gaston)]

I got to the point where video works \o/

Comment 39

[Landry Breuil (:gaston)]

Attachment: Part 1.1 media/mtransport build bits

Let's split that huuuuuge patch into smaller independently reviewable/commitable bits

Comment 40

[Landry Breuil (:gaston)]

Attachment: Part 1.2 media/mtransport header & defines

Comment 41

[Landry Breuil (:gaston)]

Attachment: Part 2 webrtc/signaling

Comment 42

[Landry Breuil (:gaston)]

Attachment: Part 3 fix gtest's detection of tr1/tuple

Comment 43

[Landry Breuil (:gaston)]

Attachment: Part 4 common webrtc build bits

Comment 44

[Landry Breuil (:gaston)]

Attachment: Part 5 audio_device

Comment 45

[Landry Breuil (:gaston)]

Attachment: Part 6 video_capture

Comment 46

[Landry Breuil (:gaston)]

Attachment: Part 7 video_engine

Comment 47

[Landry Breuil (:gaston)]

Attachment: Part 8 voice_engine

Comment 48

[Landry Breuil (:gaston)]

Attachment: Part 9 system_wrappers

Comment 49

[Landry Breuil (:gaston)]

Attachment: Part 10 system-headers

Comment 50

[Landry Breuil (:gaston)]

Attachment: Part 11 networking modules rtp/udp

Comment 51

[Landry Breuil (:gaston)]

Attachment: Part X gyp bsd flavoring

Should be more or less the same as att #684281, just rebased. No opinion on this one

Comment 52

[Landry Breuil (:gaston)]

Attachment: Part Z enable webrtc on *bsd by default

Maybe for later once this stuff is reliable, but let's keep it out for the moment.

Feel free to comment/review/redirect review separately on all those bits. So far runtime testing here showed the webcam working locally quite reliably , speaker not detected yet, mic detected but not working (but pulseaudio on openbsd doesnt have recording working) and some datachannel/p2p thing crashing the browser (the test on http://mozilla.github.io/webrtc-landing/data_test.html). Remote video between 2 instances not tested yet.

Tested with various success (at least webcam working fine, and gupshup showing sending/receiving offers without crashing) : 
http://nightly-gupshup.herokuapp.com
http://mozilla.github.io/webrtc-landing/gum_test.html
https://apprtc.appspot.com/


All in all, it goes in the right direction, so let's try to get this moving forward. I'd really like to target 24 for all this. Along with the depending bugs of course (844430, 844818 & 878446 for i386/libyuv asm)

Comment 53

[Landry Breuil (:gaston)]

Dunno what i tested wrong but http://mozilla.github.io/webrtc-landing/data_test.html seems ok now, starting & sending text over the two inputs - was maybe due to enabling all the debugging possible. will do more tests ofc.

Comment 54

[Randell Jesup [:jesup] (needinfo me)]

ekr/abr: The patches for nicer/nrappkit to support BSDs likely should be uplifted

Comment 55

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 757641 [details] [diff] [review]
Part 2 webrtc/signaling

Review of attachment 757641 [details] [diff] [review]:
-----------------------------------------------------------------

Ethan should check the signaling.gyp changes

Comment 56

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 757645 [details] [diff] [review]
Part 4 common webrtc build bits

Review of attachment 757645 [details] [diff] [review]:
-----------------------------------------------------------------

Please open an issue at webrtc.org for BSD support, and if possible submit a patch (or more likely patches) to codereview there.  This would cover any non-mozilla-specific changes to media/webrtc/trunk/*  

Note the issue # here

Comment 57

[Randell Jesup [:jesup] (needinfo me)]

I suggest submitting a number of small code patches (virtual clones of the ones here) applied against the head of the webrtc.org codebase, tied to one Issue for BSD support

Comment 58

[Ethan Hugg [:ehugg]]

Comment on attachment 757641 [details] [diff] [review]
Part 2 webrtc/signaling

Review of attachment 757641 [details] [diff] [review]:
-----------------------------------------------------------------

r- because it looks like you're changing the build params for Mac build in signaling.gyp

::: media/webrtc/signaling/signaling.gyp
@@ +227,5 @@
>  
>            'cflags_mozilla': [
>            ],
>          }],
> +        ['os_bsd==1', {

I assume that 'OS="bsd"' doesnt' work out of the box here, but OS is set to something and it's unclear that it's not one of the other values.  We need to either set OS=bsd explicitly on the gyp line - "gyp -DOS=bsd" or find out what it's already set to and put the 'if os_bsd==1' bit in there to set a different define list

@@ +814,5 @@
>  
>  
>            'defines' : [
>              'SIP_OS_OSX',
> +            # using BSD extensions, leave _POSIX_SOURCE undefined

It looks like you're changing values here for the OSX case.  I think changing the Mac build should not be part of this bug.

@@ -808,5 @@
>              'USE_TIMER_SELECT_BASED',
>              'FULL_BUILD',
>              'STUBBED_OUT',
>              'USE_PRINTF',
> -            '_DARWIN_C_SOURCE',

Same here, I assume we need to have an if os_bsd here to have it set it's own defines list.

Comment 59

[(not currently active) Ted Mielczarek]

Comment on attachment 757643 [details] [diff] [review]
Part 3 fix gtest's detection of tr1/tuple

Review of attachment 757643 [details] [diff] [review]:
-----------------------------------------------------------------

This is upstream gtest stuff, I don't think we ought to be patching it locally.

Comment 60

[Eric Rescorla (:ekr)]

In other environments we override this type of thing in the Makefile.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #59)
> Comment on attachment 757643 [details] [diff] [review]
> Part 3 fix gtest's detection of tr1/tuple
> 
> Review of attachment 757643 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is upstream gtest stuff, I don't think we ought to be patching it
> locally.

Comment 61

[Eric Rescorla (:ekr)]

Comment on attachment 757639 [details] [diff] [review]
Part 1.2 media/mtransport header & defines

Review of attachment 757639 [details] [diff] [review]:
-----------------------------------------------------------------

Anything that goes into nrappkit or nICEr should be reviewed by abr or myself.

Comment 62

[Jan Beich]

Attachment: missed stuff

- including malloc.h on freebsd is harmful, it'd break because
  of the #error in the header

- building on an unknown platform may fail due to undeclared NULL,
  noticed before adding WEBRTC_BSD

The hunks were part of attachment 723077 [details] [diff] [review] but no longer present
in attachment 757650 [details] [diff] [review]. Landry?

Comment 63

[Jan Beich]

Comment on attachment 757645 [details] [diff] [review]
Part 4 common webrtc build bits

Review of attachment 757645 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/trunk/webrtc/build/common.gypi
@@ +123,5 @@
>          # Switch between Android audio device OpenSL ES implementation
>          # and Java Implementation
>          'enable_android_opensl%': 0,
>        }],
> +      ['OS=="linux" or OS=="solaris" or OS=="freebsd" or OS=="dragonflybsd"', {

s/dragonflybsd/dragonfly/, look at how it's spelled later in the file or in configure.in.

Have you ignored comment 19? alsa does not work out of the box on
anywhere else but linux. pkgsrc and freebsd ports patches should be
upstreamed first (unlikely). That would also make alsa work on openbsd.

For now use pulse by default everywhere except win32/osx/linux.
It may benefit other platforms but not as much as sdl or gstreamer
backend would do.

Comment 64

[Landry Breuil (:gaston)]

(In reply to Eric Rescorla (:ekr) from comment #60)
> In other environments we override this type of thing in the Makefile.
> 
> (In reply to Ted Mielczarek [:ted.mielczarek] from comment #59)
> > Comment on attachment 757643 [details] [diff] [review]
> > Part 3 fix gtest's detection of tr1/tuple
> > 
> > Review of attachment 757643 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This is upstream gtest stuff, I don't think we ought to be patching it
> > locally.

This comes straight from https://code.google.com/p/googletest/source/detail?r=629 which hasnt been commited upstream it seems.

The other option is to do it as shown in comment 34, defining GTEST_USE_OWN_TR1_TUPLE=1 for OpenBSD in all the gyp places where gtest is used. Was that what you meant by 'In other environments we override this type of thing in the Makefile.', or should it be elsewhere in the build chain ?

Comment 65

[Landry Breuil (:gaston)]

Note that _i think_ all places using os_bsd==1 tests (ie part1, part2 & part4) in .gyp files might depend on Att #757656 which sets it on bsd platforms, so it might need to be reviewed/pushed first for the others to properly work on top of it ?

Comment 66

[Landry Breuil (:gaston)]

disregard comment 65, os_bsd is defined in common.gypi since https://hg.mozilla.org/integration/mozilla-inbound/rev/3bb5fb2ab811 - too much stuff piling on top.

Comment 67

[Jan Beich]

os_bsd should never exist. I've added it because upstream doesn't
even know what GetFlavor is supposed to do: a few flavors shared by
more than one platform or just an alias for sys.platform.

Looking again, I think, many uses of os_bsd and WEBRTC_BSD can be
eliminated with os_posix and WEBRTC_POSIX, helping 'solaris', too.

Comment 68

[Landry Breuil (:gaston)]

(In reply to Jan Beich from comment #62)
> Created attachment 760097 [details] [diff] [review]
> missed stuff
> 
> - including malloc.h on freebsd is harmful, it'd break because
>   of the #error in the header

malloc.h might be here for a reason, of if anything i'd put the #include <malloc.h> within #ifndef (__FreeBSD__) to avoir having unwanted effects on other platforms. Or send that to try.

> - building on an unknown platform may fail due to undeclared NULL,
>   noticed before adding WEBRTC_BSD

What does this actually fix ? What unknown platform ?

(In reply to Jan Beich from comment #67)
> os_bsd should never exist. I've added it because upstream doesn't
> even know what GetFlavor is supposed to do: a few flavors shared by
> more than one platform or just an alias for sys.platform.

Well, now it's in so at least let's try to use it.

> Looking again, I think, many uses of os_bsd and WEBRTC_BSD can be
> eliminated with os_posix and WEBRTC_POSIX, helping 'solaris', too.

Are you willing to do the work, including the try runs ? This is already quite painful, not speaking of the upstreaming to webrtc.org which hasnt even started.

Comment 69

[Landry Breuil (:gaston)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #59)
> Comment on attachment 757643 [details] [diff] [review]
> Part 3 fix gtest's detection of tr1/tuple
> 
> Review of attachment 757643 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is upstream gtest stuff, I don't think we ought to be patching it
> locally.

Looking again, this is gtest's upstream r629, so what would harm in patching it locally ? Next time webrtc updates its gtest copy, the fix will be merged automatically. And the copy of gtest in testing/gtest/gtest/ already got two local patches ... the copy in ipc/chromium/src/testing/gtest/ got two patches too (one against gtest-port.h common with the copy in testing and the one in webrtc/trunk/testing/gtest/) - the copy in webrtc already got some patches too.

So, for consistency, would it be okay to make Part 3 a backport or upstream r629 to the three copies of gtest-port.h we have in the tree ? ted ?

I've pushed the r+'ed patches (ie part 4 to 11) to try in https://tbpl.mozilla.org/?tree=Try&rev=c089f6f6642c - this includes a fixed part 4 to use pulse on OS="solaris" or os_bsd=1 (leaving alsa only to linux) - and added a cset to see what happens if one removes #include <malloc.h> from media/webrtc/trunk/webrtc/system_wrappers/source/atomic32_posix.cc.

If everything goes well, i'd like to push that first batch of patches to inbound... unless randall plans to update webrtc to 3.30 first in 880879 ? Should i make this bug depend on the latter ?

(In reply to Jan Beich from comment #67)
> Looking again, I think, many uses of os_bsd and WEBRTC_BSD can be
> eliminated with os_posix and WEBRTC_POSIX, helping 'solaris', too.

WEBRTC_ANDROID defines WEBRTC_POSIX too... so i'm not sure changing all the WEBRTC_BSD|WEBRTC_LINUX|WEBRTC_MAC to WEBRTC_POSIX wouldnt break android :) here be dragons

Comment 70

[(not currently active) Ted Mielczarek]

(In reply to Landry Breuil (:gaston) from comment #69)
> Looking again, this is gtest's upstream r629, so what would harm in patching
> it locally ?

If this has already landed upstream then I think it's fine to land it locally. I wouldn't bother getting additional review, I'd just say r=upstream.

Comment 71

[Landry Breuil (:gaston)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #70)
> (In reply to Landry Breuil (:gaston) from comment #69)
> > Looking again, this is gtest's upstream r629, so what would harm in patching
> > it locally ?
> 
> If this has already landed upstream then I think it's fine to land it
> locally. I wouldn't bother getting additional review, I'd just say
> r=upstream.

But then land the full r629 revision, not just the chunk that fixes GTEST_ENV_HAS_STD_TUPLE_ definition ?

Comment 72

[Landry Breuil (:gaston)]

(In reply to Landry Breuil (:gaston) from comment #69)

> I've pushed the r+'ed patches (ie part 4 to 11) to try in
> https://tbpl.mozilla.org/?tree=Try&rev=c089f6f6642c - this includes a fixed
> part 4 to use pulse on OS="solaris" or os_bsd=1 (leaving alsa only to linux)
> - and added a cset to see what happens if one removes #include <malloc.h>
> from media/webrtc/trunk/webrtc/system_wrappers/source/atomic32_posix.cc.

Everything went well on try, so i'll merge the #include <malloc.h> removal in Part 9, and i'll land only the chunk of Part 3 that comes from upstream. Just waiting for a window where i have time to watch, and make sure i dont conflict with bug 880879

Comment 73

[Landry Breuil (:gaston)]

Since it seems to be a quiet period now, pushed part 3 to 11 :
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/502d77755cf0
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1a1a7cab4e24
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5b90393fadf6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2b9c1abcf86f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/55fd62dfb8f6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/deb4e701df4c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3c3d76ae7541
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9dd829a085d3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/59181cb244fd

Watching the tree to see if it sticks before marking checkin+

Comment 74

[Landry Breuil (:gaston)]

Comment on attachment 757643 [details] [diff] [review]
Part 3 fix gtest's detection of tr1/tuple

https://hg.mozilla.org/integration/mozilla-inbound/rev/502d77755cf0

Comment 75

[Landry Breuil (:gaston)]

Comment on attachment 757645 [details] [diff] [review]
Part 4 common webrtc build bits

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a1a7cab4e24

(landed with only linux using alsa, the others (solaris+bsd) using pulse)

Comment 76

[Landry Breuil (:gaston)]

Comment on attachment 757646 [details] [diff] [review]
Part 5 audio_device

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b90393fadf6

Comment 77

[Landry Breuil (:gaston)]

Comment on attachment 757647 [details] [diff] [review]
Part 6 video_capture

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b9c1abcf86f

Comment 78

[Landry Breuil (:gaston)]

Comment on attachment 757648 [details] [diff] [review]
Part 7 video_engine

https://hg.mozilla.org/integration/mozilla-inbound/rev/55fd62dfb8f6

Comment 79

[Landry Breuil (:gaston)]

Comment on attachment 757649 [details] [diff] [review]
Part 8 voice_engine

https://hg.mozilla.org/integration/mozilla-inbound/rev/deb4e701df4c

Comment 80

[Landry Breuil (:gaston)]

Comment on attachment 757650 [details] [diff] [review]
Part 9 system_wrappers

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c3d76ae7541

(landed with #include <malloc.h> removed for FreeBSD)

Comment 81

[Landry Breuil (:gaston)]

Comment on attachment 757652 [details] [diff] [review]
Part 10 system-headers

https://hg.mozilla.org/integration/mozilla-inbound/rev/9dd829a085d3

Comment 82

[Landry Breuil (:gaston)]

Comment on attachment 757654 [details] [diff] [review]
Part 11 networking modules rtp/udp

https://hg.mozilla.org/integration/mozilla-inbound/rev/59181cb244fd

Comment 83

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/502d77755cf0
https://hg.mozilla.org/mozilla-central/rev/1a1a7cab4e24
https://hg.mozilla.org/mozilla-central/rev/5b90393fadf6
https://hg.mozilla.org/mozilla-central/rev/2b9c1abcf86f
https://hg.mozilla.org/mozilla-central/rev/55fd62dfb8f6
https://hg.mozilla.org/mozilla-central/rev/deb4e701df4c
https://hg.mozilla.org/mozilla-central/rev/3c3d76ae7541
https://hg.mozilla.org/mozilla-central/rev/9dd829a085d3
https://hg.mozilla.org/mozilla-central/rev/59181cb244fd

Note that a few of these had the wrong bug # in the commit message...

Comment 84

[Landry Breuil (:gaston)]

Argh, my bad. Istr seeing those 4<->7 mixups and i was pretty sure i had fixed them before pushing. Damn.

Comment 85

[Adam Roach [:abr]]

Comment on attachment 757639 [details] [diff] [review]
Part 1.2 media/mtransport header & defines

Review of attachment 757639 [details] [diff] [review]:
-----------------------------------------------------------------

r- for changing Darwin/MacOS behavior

::: media/mtransport/third_party/nICEr/src/util/mbslen.c
@@ +50,5 @@
> +#  define HAVE_XLOCALE
> +# endif
> +#endif
> +
> +#ifdef HAVE_XLOCALE

HAVE_XLOCALE does not apparently get set for Darwin systems, which means the patch changes Mac behavior.

Comment 86

[Adam Roach [:abr]]

Comment on attachment 757639 [details] [diff] [review]
Part 1.2 media/mtransport header & defines

Review of attachment 757639 [details] [diff] [review]:
-----------------------------------------------------------------

Ah, I found it. It's in the "Part 1.1" patch. The issue here is that nICEr is a third-party product, and we upstream our changes to the parent project periodically. The .gyp file is mozilla-specific; the upstream project uses normal Makefiles. Rather than asking you to patch the upstream Makefiles, I think it's best if you simply change the mbslen.c file along the lines of:

#ifdef DARWIN
#define HAVE_XLOCALE
#endif

This will work for both the Mozilla tree as well as for the upstream project.

Comment 87

[Landry Breuil (:gaston)]

Attachment: bug-807492-part1.2-mtransport-header-defines-bits

Added to media/mtransport/third_party/nICEr/src/util/mbslen.c as per comment 85 & 86

#ifdef DARWIN
#define HAVE_XLOCALE
#endif

carrying over rjesup's r+..

Comment 88

[Landry Breuil (:gaston)]

Attachment: Part 2 webrtc/signaling

Per comment 58, add a conditional for OS=mac or os_bsd=1 to set the defines without changing macosX. Indentation not changed on purpose for readability, will be fixed for commit of course. Testing it locally.

Comment 89

[Ethan Hugg [:ehugg]]

Comment on attachment 788581 [details] [diff] [review]
Part 2 webrtc/signaling

Review of attachment 788581 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is OK now.  I ran a OSX build and it appears the compile params are unaffected from this patch so if it works for you on BSD then it should be fine.

Comment 90

[Landry Breuil (:gaston)]

Thanks.. there's a new failure (probably caused by the webrtc update) which i'm also going to deal with in a separate patch :

/src/mozilla-central/media/webrtc/trunk/webrtc/modules/../../webrtc/modules/video_coding/main/source/timestamp_extrapolator.h:4
2:27: error: expected member name or ';' after declaration specifiers
    double                _P[2][2];
    ~~~~~~                ^
/usr/include/ctype.h:49:12: note: expanded from macro '_P'
#define _P      0x10

Comment 91

[Landry Breuil (:gaston)]

And for a reason i dont understand, now WEBRTC_LINUX gets defined in webrtc's Makefiles everywhere even on BSD, which leads to build failures in the codepaths where we want to use MacOS code (ie patch from part 8) :

34:39.17 vie_channel_manager.o
34:39.29 In file included from /src/mozilla-central/media/webrtc/trunk/webrtc/voice_engine/channel.cc:11:
34:39.29 In file included from /src/mozilla-central/media/webrtc/trunk/webrtc/voice_engine/../../webrtc/voice_engine/channel.h:22:
34:39.29 In file included from /src/mozilla-central/media/webrtc/trunk/webrtc/voice_engine/../../webrtc/voice_engine/dtmf_inband.h:19:
34:39.29 /src/mozilla-central/media/webrtc/trunk/webrtc/voice_engine/voice_engine_defines.h:264:12: fatal error: 'linux/net.h' file not found
34:39.29   #include <linux/net.h>

media/webrtc/trunk/webrtc/build/common.gypi only adds WEBRTC_LINUX on OS=linux & OS=android cases, and should define WEBRTC_BSD for us, so i'm totally puzzled.

randall, any idea ? a change in the update to webrtc 3.30 ?

Comment 92

[Landry Breuil (:gaston)]

 Disregard the latter, this was probably due to the fact that i was trying to build without  'Part X gyp bsd flavoring' - which is still required.

Comment 93

[Landry Breuil (:gaston)]

Attachment: Part 12 rename _P to PP in timestamp_extrapolator

_P is a reserved historical macro on the BSDs, defined in ctype.h.. a solution is to rename _P to PP, or #undef _P before using it.

Comment 94

[Landry Breuil (:gaston)]

Attachment: Part X gyp bsd flavoring (simplified)

Per comment 16, simplify flavor handling only in mozmake.py generator. 'flavor' doesnt seem present in params when being called from the build system, so it shouldnt change anything. Will push to try for extra safety.

Comment 95

[Landry Breuil (:gaston)]

 https://tbpl.mozilla.org/?tree=Try&rev=26775eb1ce18

Comment 96

[Jan Beich]

(In reply to Landry Breuil (:gaston) from comment #93)
> Created attachment 788684 [details] [diff] [review]
> Part 12 rename _P to PP in timestamp_extrapolator
> 
> _P is a reserved historical macro on the BSDs, defined in ctype.h.. a
> solution is to rename _P to PP, or #undef _P before using it.

Only on OpenBSD.

http://bxr.su/search?q=&defs=_P&refs=&path=

Comment 97

[Landry Breuil (:gaston)]

(In reply to Jan Beich from comment #96)
> (In reply to Landry Breuil (:gaston) from comment #93)
> > Created attachment 788684 [details] [diff] [review]
> > Part 12 rename _P to PP in timestamp_extrapolator
> > 
> > _P is a reserved historical macro on the BSDs, defined in ctype.h.. a
> > solution is to rename _P to PP, or #undef _P before using it.
> 
> Only on OpenBSD.
> 
> http://bxr.su/search?q=&defs=_P&refs=&path=

FreeBSD and NetBSD also had it in the past but it was moved around/removed over time. That said, renaming the variable fixes it.

Comment 98

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 788684 [details] [diff] [review]
Part 12 rename _P to PP in timestamp_extrapolator

Review of attachment 788684 [details] [diff] [review]:
-----------------------------------------------------------------

If you change PP to _PP -> r+
(Google is using _foo for member vars)

We need to make a plan to file all these individual items as upstream webrtc.org bugs once all the issues are worked out here.  I do NOT want to be carrying the merge pain on every update of changes scattered all over the tree forevermore.  :-)

(You can start now if you want with ones you feel are finished).  File as an Issue (BSD support), and then build and upload CLs against the main webrtc.org source base.  See http://www.webrtc.org/reference/contributing -- yes, it's impossible to find on webrtc.org)

Comment 99

[Landry Breuil (:gaston)]

(In reply to Randell Jesup [:jesup] from comment #98)

> We need to make a plan to file all these individual items as upstream
> webrtc.org bugs once all the issues are worked out here.  I do NOT want to
> be carrying the merge pain on every update of changes scattered all over the
> tree forevermore.  :-)

I totally understand that :)
 
> (You can start now if you want with ones you feel are finished).  File as an
> Issue (BSD support), and then build and upload CLs against the main
> webrtc.org source base.  See http://www.webrtc.org/reference/contributing --
> yes, it's impossible to find on webrtc.org)

I've already filed a tracking issue (see also on this bug, http://code.google.com/p/webrtc/issues/detail?id=1911) but i've had an insane pain just trying to get standalone webrtc to build on OpenBSD. Blindly back/forwardporting patches against their svn and submitting them via CL will be "doable", but making sure it's tested within their infrastructure is just crazy.

Comment 100

[Landry Breuil (:gaston)]

(In reply to Landry Breuil (:gaston) from comment #97)
> (In reply to Jan Beich from comment #96)
> > (In reply to Landry Breuil (:gaston) from comment #93)
> > > Created attachment 788684 [details] [diff] [review]
> > > Part 12 rename _P to PP in timestamp_extrapolator
> > > 
> > > _P is a reserved historical macro on the BSDs, defined in ctype.h.. a
> > > solution is to rename _P to PP, or #undef _P before using it.
> > 
> > Only on OpenBSD.
> > 
> > http://bxr.su/search?q=&defs=_P&refs=&path=
> 
> FreeBSD and NetBSD also had it in the past but it was moved around/removed
> over time. That said, renaming the variable fixes it.

And just for the record after doing some research and asking knowledgeable people :

C99 standard, section 7.1.3 "Reserved identifiers":
                                                                                            -- All identifiers that begin with an underscore and either
   an uppercase letter or another underscore are always
   reserved for any use.
  
https://www.securecoding.cert.org/confluence/display/seccode/DCL37-C.+Do+not+declare+or+define+a+reserved+identifier

Comment 101

[Adam Roach [:abr]]

Comment on attachment 788580 [details] [diff] [review]
bug-807492-part1.2-mtransport-header-defines-bits

Review of attachment 788580 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

Comment 102

[Landry Breuil (:gaston)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d3779a4ca8f
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f08cd76c8c0
https://hg.mozilla.org/integration/mozilla-inbound/rev/3db54c98078c
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d06025ea63d

Left open for parts X and Z, pending review & more testing.

As for part12, jesup agreed to rename from _P to _pp over irc, which was done when pushing.

Comment 103

[Landry Breuil (:gaston)]

Comment on attachment 757662 [details] [diff] [review]
Part Z enable webrtc on *bsd by default

Once enough testing on BSD would have be done... it'd be time to flip the switch :)

Comment 104

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/0d3779a4ca8f
https://hg.mozilla.org/mozilla-central/rev/2f08cd76c8c0
https://hg.mozilla.org/mozilla-central/rev/3db54c98078c
https://hg.mozilla.org/mozilla-central/rev/2d06025ea63d

Comment 105

[Landry Breuil (:gaston)]

Comment on attachment 788685 [details] [diff] [review]
Part X gyp bsd flavoring (simplified)

https://hg.mozilla.org/integration/mozilla-inbound/rev/08949167dd55

waiting for the dust to settle from bug 905920 and my builders to go green before enabling webrtc by default on BSD when landing part Z...

Comment 106

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/08949167dd55

Comment 107

[Landry Breuil (:gaston)]

Since this still builds with the upcoming webrtc 3.34 update (bug 901583) let's be brave and pull the trigger (ie enable WebRTC by default on *BSD)
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b68f15672e3

This way it'll be simpler to have builds available for testing/debugging, and we can finally close this giant bug (thus removing [leave open]) - we'll file followups for runtime issues.

Comment 108

[Ed Morley [:emorley]]

(In reply to Landry Breuil (:gaston) from comment #107)
> Since this still builds with the upcoming webrtc 3.34 update (bug 901583)
> let's be brave and pull the trigger (ie enable WebRTC by default on *BSD)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/9b68f15672e3
> 
> This way it'll be simpler to have builds available for testing/debugging,
> and we can finally close this giant bug (thus removing [leave open]) - we'll
> file followups for runtime issues.

https://hg.mozilla.org/mozilla-central/rev/9b68f15672e3

(Landed with wrong bug #, have commented in the other bug)

Comment 109

[Landry Breuil (:gaston)]

All pending patches landed, closing (and celebrating!)