Closed Bug 807492 Opened 12 years ago Closed 11 years ago

Support BSDs for WebRTC

Categories

(Core :: WebRTC, defect, P3)

x86_64
FreeBSD
defect

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: jbeich, Assigned: gaston)

References

Details

(Whiteboard: [WebRTC][blocking-webrtc-])

Attachments

(18 files, 8 obsolete files)

3.77 KB, patch
jesup
: review+
ted
: review+
gaston
: checkin+
Details | Diff | Splinter Review
3.55 KB, patch
jesup
: review+
gaston
: checkin+
Details | Diff | Splinter Review
5.36 KB, patch
jesup
: review+
gaston
: checkin+
Details | Diff | Splinter Review
1.57 KB, patch
gaston
: checkin+
Details | Diff | Splinter Review
3.01 KB, patch
jesup
: review+
jbeich
: feedback-
gaston
: checkin+
Details | Diff | Splinter Review
6.56 KB, patch
jesup
: review+
gaston
: checkin+
Details | Diff | Splinter Review
6.59 KB, patch
jesup
: review+
gaston
: checkin+
Details | Diff | Splinter Review
952 bytes, patch
jesup
: review+
gaston
: checkin+
Details | Diff | Splinter Review
4.43 KB, patch
jesup
: review+
gaston
: checkin+
Details | Diff | Splinter Review
16.39 KB, patch
jesup
: review+
gaston
: checkin+
Details | Diff | Splinter Review
780 bytes, patch
ted
: review+
gaston
: checkin+
Details | Diff | Splinter Review
10.37 KB, patch
jesup
: review+
gaston
: checkin+
Details | Diff | Splinter Review
849 bytes, patch
jesup
: review+
gaston
: checkin+
Details | Diff | Splinter Review
1007 bytes, patch
Details | Diff | Splinter Review
7.38 KB, patch
abr
: review+
gaston
: checkin+
Details | Diff | Splinter Review
4.02 KB, patch
ehugg
: review+
gaston
: checkin+
Details | Diff | Splinter Review
4.14 KB, patch
jesup
: review+
gaston
: checkin+
Details | Diff | Splinter Review
1.98 KB, patch
ted
: review+
gaston
: checkin+
Details | Diff | Splinter Review
      No description provided.
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.
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
Attachment #677431 - Flags: review?(rjesup)
(In reply to Jan Beich from comment #2)
> bug 788955

Typo. It should have been bug 798921.
Attachment #677433 - Flags: review?(rjesup)
Depends on: 807683
(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.
NetBSD situation is very similar to OpenBSD: no native alsa on NetBSD either, v4l via video(4)
I'm switching to use os_bsd to avoid typos and reduce porting burden for other forks.
Attachment #677734 - Flags: review?(rjesup)
Attachment #677734 - Flags: review?(rjesup) → review+
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
Attachment #677431 - Flags: review?(ted)
Attachment #677431 - Flags: review?(rjesup)
Attachment #677431 - Flags: review+
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
Attachment #677433 - Flags: review?(rjesup) → review?(ted)
(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."
v2 based on gyp from svn + a random discussion
http://groups.google.com/group/gyp-developer/browse_thread/thread/1bf12be2cc4630d5
Attachment #677433 - Attachment is obsolete: true
Attachment #677433 - Flags: review?(ted)
Attachment #680313 - Flags: review?(ted)
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.
Attachment #680313 - Flags: review?(ted) → review-
Priority: -- → P3
Whiteboard: [WebRTC][blocking-webrtc-]
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.
Attachment #677431 - Flags: review?(ted) → review+
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.
Attachment #680313 - Attachment is obsolete: true
Attachment #684281 - Flags: review?(ted)
(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 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.
Attachment #684281 - Flags: review?(ted) → review-
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 ?
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.
(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.
Depends on: 844430
(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 ?
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bb5fb2ab811
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7ae0dbd36a7

leave open for the remaining wip..
Whiteboard: [WebRTC][blocking-webrtc-] → [WebRTC][blocking-webrtc-][leave open]
Attachment #677431 - Flags: checkin+
Attachment #677734 - Flags: checkin+
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!
Doh, landed with a damn typo adding an erroneous + to chunk 1 of media/webrtc/trunk/third_party/libyuv/libyuv.gyp patch. Fixin'.
Depends on: 844818
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.
Attached patch FreeBSD's patchset for webrtc (obsolete) — Splinter Review
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.
Attachment #723077 - Flags: feedback?(rjesup)
Attachment #723077 - Flags: feedback?(jbeich)
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.
Attachment #723079 - Flags: feedback?(ted)
Attachment #723079 - Flags: feedback?(rjesup)
Attachment #723079 - Flags: feedback?(jbeich)
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].
Attachment #723077 - Flags: feedback?(jbeich)
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.
Attachment #723079 - Flags: feedback?(jbeich) → feedback+
Except for include_pulse_audio and <tr1/tuple> workaround I've merged
Landry's fixes into a patch for mozilla20 and pinged ryoon@netbsd.org.
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 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.
Attachment #723079 - Flags: feedback?(ted) → feedback+
(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...
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.
Sidenote on dlerror() : it was wrong on our side and got fixed in http://marc.info/?l=openbsd-cvs&m=136408915614138&w=2
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.
I got to the point where video works \o/
Let's split that huuuuuge patch into smaller independently reviewable/commitable bits
Attachment #757638 - Flags: review?(rjesup)
Attachment #757639 - Flags: review?(rjesup)
Attached patch Part 2 webrtc/signaling (obsolete) — Splinter Review
Attachment #757641 - Flags: review?(rjesup)
Attachment #757643 - Flags: review?(rjesup)
Attachment #757645 - Flags: review?(rjesup)
Attachment #757646 - Flags: review?(rjesup)
Attachment #757647 - Flags: review?(rjesup)
Attachment #757648 - Flags: review?(rjesup)
Attachment #757649 - Flags: review?(rjesup)
Attachment #757650 - Flags: review?(rjesup)
Attachment #757652 - Flags: review?(rjesup)
Attachment #757654 - Flags: review?(rjesup)
Attached patch Part X gyp bsd flavoring (obsolete) — Splinter Review
Should be more or less the same as att #684281, just rebased. No opinion on this one
Attachment #684281 - Attachment is obsolete: true
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)
Attachment #723077 - Attachment is obsolete: true
Attachment #723079 - Attachment is obsolete: true
Attachment #723077 - Flags: feedback?(rjesup)
Attachment #723079 - Flags: feedback?(rjesup)
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.
Attachment #757638 - Flags: review?(rjesup) → review+
Attachment #757639 - Flags: review?(rjesup) → review+
ekr/abr: The patches for nicer/nrappkit to support BSDs likely should be uplifted
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
Attachment #757641 - Flags: review?(rjesup)
Attachment #757641 - Flags: review?(ethanhugg)
Attachment #757641 - Flags: review+
Attachment #757643 - Flags: review?(rjesup) → review?(ted)
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
Attachment #757645 - Flags: review?(rjesup) → review+
Attachment #757646 - Flags: review?(rjesup) → review+
Attachment #757647 - Flags: review?(rjesup) → review+
Attachment #757648 - Flags: review?(rjesup) → review+
Attachment #757649 - Flags: review?(rjesup) → review+
Attachment #757650 - Flags: review?(rjesup) → review+
Attachment #757652 - Flags: review?(rjesup) → review?(ted)
Attachment #757654 - Flags: review?(rjesup) → review+
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 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.
Attachment #757641 - Flags: review?(ethanhugg) → review-
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.
Attachment #757643 - Flags: review?(ted)
Attachment #757652 - Flags: review?(ted) → review+
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 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.
Attachment #757639 - Flags: review?(adam)
Attached patch missed stuffSplinter Review
- 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 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.
Attachment #757645 - Flags: feedback-
(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 ?
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 ?
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.
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.
(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.
(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
(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.
(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 ?
(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 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)
Attachment #757645 - Flags: checkin+
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)
Attachment #757650 - Flags: checkin+
Argh, my bad. Istr seeing those 4<->7 mixups and i was pretty sure i had fixed them before pushing. Damn.
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.
Attachment #757639 - Flags: review?(adam) → review-
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.
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+..
Attachment #757639 - Attachment is obsolete: true
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.
Attachment #757641 - Attachment is obsolete: true
Attachment #788581 - Flags: review?(ethanhugg)
Attachment #788580 - Flags: review?(adam)
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.
Attachment #788581 - Flags: review?(ethanhugg) → review+
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
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 ?
Flags: needinfo?(rjesup)
 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.
Flags: needinfo?(rjesup)
_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.
Attachment #788684 - Flags: review?(rjesup)
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.
Attachment #757656 - Attachment is obsolete: true
Attachment #788685 - Flags: review?(ted)
(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=
(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 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)
Attachment #788684 - Flags: review?(rjesup) → review+
(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.
(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 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.
Attachment #788580 - Flags: review?(adam) → review+
Attachment #757638 - Flags: checkin+
Attachment #788581 - Flags: checkin+
Attachment #788684 - Flags: checkin+
Attachment #788580 - Flags: checkin+
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 :)
Attachment #757662 - Flags: review?(rjesup)
Attachment #757662 - Flags: review?(rjesup) → review+
Attachment #788685 - Flags: review?(ted) → review+
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...
Attachment #788685 - Flags: checkin+
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.
Whiteboard: [WebRTC][blocking-webrtc-][leave open] → [WebRTC][blocking-webrtc-]
Attachment #757662 - Flags: checkin+
(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)
All pending patches landed, closing (and celebrating!)
Assignee: nobody → landry
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Depends on: 910875
Depends on: 916216
Depends on: 916589
Depends on: 918177
Blocks: 939532
Depends on: 952828
Depends on: 985848
No longer depends on: webrtc_upstream_bugs
Blocks: 1285501
No longer blocks: webrtc_upstream_bugs
You need to log in before you can comment on or make changes to this bug.