Closed Bug 818843 Opened 7 years ago Closed 7 years ago

Enable the makefile system to build WebRTC on B2G.

Categories

(Core :: WebRTC, defect, P1)

All
Gonk (Firefox OS)
defect

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: slee, Assigned: slee)

References

Details

(Whiteboard: [WebRTC] [blocking-webrtc-][getusermedia][blocking-gum-][qa-])

Attachments

(4 files, 18 obsolete files)

3.07 KB, patch
ted
: review+
Details | Diff | Splinter Review
1.74 KB, patch
slee
: review+
Details | Diff | Splinter Review
13.73 KB, patch
slee
: review+
Details | Diff | Splinter Review
4.60 KB, patch
slee
: review+
Details | Diff | Splinter Review
Enable the makefile system to build WebRTC on B2G.
Attached patch part 1, configure.in - WIP (obsolete) — Splinter Review
Attached patch part 2, network (obsolete) — Splinter Review
Attached patch part 3, media - WIP (obsolete) — Splinter Review
I could build WebRTC on B2G and I could run the media, {fake:true, video:true}.
Enable MOZ_MEDIA_NAVIGATOR and turn on preference on B2G
Summary: makefile system for B2G → Enable the makefile system to build WebRTC on B2G.
Wow!  Great work Steven!  I'll look over these patches today.
Priority: -- → P1
Whiteboard: [WebRTC] [blocking-webrtc-][getusermedia][blocking-gum-]
After updating to the latest code, build fails. Bug 750869 defined variable, target_arch. If the target_arch is "arm" and is not armv7, there are building problems. That's because "arm_neon" is only meaningful on armv7, [1]. But in the build system checks "arm_neon" without caring about the value of armv7, [2]. I will file a new bug to solve this problem first.

[1] https://mxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/build/common.gypi#192
[2] https://mxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/src/common_audio/signal_processing/signal_processing.gypi#78
Depends on: 819246
(In reply to StevenLee from comment #7)
> After updating to the latest code, build fails. Bug 750869 defined variable,
> target_arch. If the target_arch is "arm" and is not armv7, there are
> building problems. That's because "arm_neon" is only meaningful on armv7,
> [1]. But in the build system checks "arm_neon" without caring about the
> value of armv7, [2]. I will file a new bug to solve this problem first.

I suspect this is related to, or even fixed by, the patch in bug 815883.
(In reply to Dan Mosedale (:dmose) from comment #8)
> I suspect this is related to, or even fixed by, the patch in bug 815883.
Hi dmose,

I think maybe we are fixing different problems. The problem I encountered is that the checking logic has problem. In [1], the build system will include some unnecessary files and they cause symbols conflicts when linking. The patch of bug 815883 tries to fix some dependencies and flags for arm_neon but it does not set the value of "arm_neon". The default value of "arm_neon" is true. So that if we don't fix the checking logic(that is fixed in 819246) and build WebRTC on ARM, we still get symbol conflict problem, it that right?

[1] https://mxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/src/common_audio/signal_processing/signal_processing.gypi#78
Sounds right to me, though symbol conflicts isn't exactly what I would expect to be the failure mode for trying to use ARM NEON instructions when targeting ARMv6.
Attached patch part 1, configure.in (obsolete) — Splinter Review
Hi jesup,

Would you please review my patches? Thanks.

In this patch, I 
1. Enable WEBRTC on B2G
2. Add new definition, moz_widget_toolkit_gonk, that will be enabled on gonk.
3. It does not build gtest.gyp
4. Include new paths for alsa and rpc
Attachment #689148 - Attachment is obsolete: true
Attachment #691231 - Flags: review?(rjesup)
Attached patch Part 2, network (obsolete) — Splinter Review
1. Add new definition, __Userspace_os_Gonk.
2. Add 2 new functions, get_interface getifaddrs and freeifaddrs, these functions are borrowed from android. I am not sure if it is suitable to do this or we can have better solution.
3. Fix some platform dependency include.
Attachment #689149 - Attachment is obsolete: true
Attachment #691234 - Flags: review?(rjesup)
Hi jesup,

I tried B2G unagi and Linux. They are both fine. But when I upload my patches to try server, there are building errors. I will fix these errors first then update new patches. I will cancel the reviews.
Attachment #691231 - Flags: review?(rjesup)
Attachment #691234 - Flags: review?(rjesup)
Attached patch part 1, configure.in (obsolete) — Splinter Review
1. Enable WebRTC on B2G
2. Don't link OpenSLES. B2G process crashes when linking this library.
3. Add moz_widget_toolkit_gonk
4. Disable testing code.
Attachment #691231 - Attachment is obsolete: true
Attachment #692740 - Flags: review?(rjesup)
Attached patch Part 2, network (obsolete) — Splinter Review
1. Add __Userspace_os_Gonk
Attachment #691234 - Attachment is obsolete: true
Attachment #692742 - Flags: review?(rjesup)
Attached patch part 3, media (obsolete) — Splinter Review
1. disable testing code in mtransport
2. add WEBRTC_GONK 
3. fix some errors of include headers.
4. add some wrapper functions of msg
Attachment #689150 - Attachment is obsolete: true
Attachment #692744 - Flags: review?(rjesup)
Hi jesup,

Above are my updated patches. I fixed the compiling error of emulator and pandaboards. 
Here is the try server log. Please check it.
* try: -b do -p all -u none -t none
** https://tbpl.mozilla.org/?tree=Try&rev=33a1c396e97b
Comment on attachment 692740 [details] [diff] [review]
part 1, configure.in

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

r+ with nits fixed as needed

::: configure.in
@@ +5255,5 @@
> +            MOZ_WEBRTC=1
> +        else
> +            dnl Make sure doesn't get matched by *-linux*
> +            MOZ_WEBRTC=
> +        fi

Likely will get a merge conflict with the android code.  I suggest rebasing your patches on top of the current set of Android patches from gcp/dmose to minimize bitrot and merge issues.

@@ +9052,5 @@
>  dnl so that regeneration via dependencies works correctly
> +   if test -n HAVE_CLOCK_MONOTONIC; then
> +      WEBRTC_CONFIG="-D build_with_mozilla=1 -D moz_have_clock_monotonic=1 --include ${srcdir}/media/webrtc/webrtc_config.gypi -D FORCED_INCLUDE_FILE=${srcdir}/media/webrtc/webrtc_config.gypi"
> +   else
> +      WEBRTC_CONFIG="-D build_with_mozilla=1 -D moz_have_clock_monotonic=0 --include ${srcdir}/media/webrtc/webrtc_config.gypi -D FORCED_INCLUDE_FILE=${srcdir}/media/webrtc/webrtc_config.gypi"

I'd prefer to append "-D moz_have_clock_monotonic=X" to the WEBRTC_CONFIG var after it's set, or do it in a single line in some fashion.  It's painful and error-inducing to have two copies of the same code/defines.

@@ +9086,5 @@
> +         --generator-output=${_objdir}/media/webrtc/signalingtest \
> +         ${srcdir}/media/webrtc/signaling/signaling.gyp
> +       if test "$?" != 0; then
> +         AC_MSG_ERROR([failed to generate WebRTC/SignalingTest Makefiles])
> +       fi

question: do we need to disable creating the makefiles, or just disable building them?  If it's just building, disable building the tests from the Makefile.in in (IIRC) toolkit.

@@ +9099,5 @@
> +       --generator-output=${_objdir}/media/webrtc/trunk/testing/ \
> +       ${srcdir}/media/webrtc/trunk/testing/gtest.gyp
> +     if test "$?" != 0; then
> +       AC_MSG_ERROR([failed to generate gtest Makefiles])
> +     fi

Ditto
Attachment #692740 - Flags: review?(rjesup) → review+
(In reply to Randell Jesup [:jesup] from comment #18)

> ::: configure.in
> @@ +5255,5 @@
> > +            MOZ_WEBRTC=1
> > +        else
> > +            dnl Make sure doesn't get matched by *-linux*
> > +            MOZ_WEBRTC=
> > +        fi
> 
> Likely will get a merge conflict with the android code.  I suggest rebasing
> your patches on top of the current set of Android patches from gcp/dmose to
> minimize bitrot and merge issues.

The Android build patches have all landed, so as long as this patch is based on mozilla-central or mozilla-inbound from yesterday or newer, that shouldn't be a problem.  Of course, trying an Android build with --enable-webrtc to be sure that these changes don't actually cause that Fennec configuration not to build would be nice.
Comment on attachment 692742 [details] [diff] [review]
Part 2, network

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

r+, but really this likely would be better using os_Linux and then using a few #if GONK (or whatever) like Android is doing.  See dmose/gcp's patch for guidance.  I'd prefer to follow their lead in this module (and the diffs will likely be smaller too)

::: netwerk/sctp/src/netinet/sctp.h
@@ +37,5 @@
>  
>  #ifndef _NETINET_SCTP_H_
>  #define _NETINET_SCTP_H_
>  
> +#if (defined(__APPLE__) || defined(__Userspace_os_Linux) || defined(__Userspace_os_Darwin)) || defined(__Userspace_os_Gonk)

Probably better would be !__Userspace_os_Windows  - I presume FreeBSD has stdint too

::: netwerk/sctp/src/user_ip6_var.h
@@ +88,5 @@
>  #define s6_addr16 u.Word
>  #endif
>  #if !defined(__Userspace_os_Windows)
>  #if !defined(__Userspace_os_Linux)
> +#if !defined(__Userspace_os_Gonk)

Probably combine into one #if
Attachment #692742 - Flags: review?(rjesup) → review+
Comment on attachment 692744 [details] [diff] [review]
part 3, media

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

Overall good, but I'm mildly concerned that there are many android optimizations in media/webrtc/trunk that should also apply to B2G (some of which relate to the processor, but others relate to the likely horsepower of the CPU, etc).  However, there are also a bunch of userspace-related Android code bits in there (video renedering, etc), so without reviewing all the code that references ANDROID in that dir, I can't say which way is preferred.

Please also compare to changes in this area currently being used by the Android team

::: media/webrtc/Makefile.in
@@ +20,3 @@
>    trunk/testing \
> +	$(NULL)
> +endif

Correct indentation

::: media/webrtc/signaling/signaling.gyp
@@ +20,5 @@
> +          'WEBRTC_GONK',
> +       ],
> +      }],
> +    ],
> +  }, 

remove trailing spaces

::: media/webrtc/trunk/src/build/common.gypi
@@ +124,5 @@
> +        'defines' : [
> +          'WEBRTC_GONK',
> +        ],
> +      }],
> +  

trailing whitespace
Attachment #692744 - Flags: review?(rjesup) → review+
Comment on attachment 692742 [details] [diff] [review]
Part 2, network

On second thought - r- for now; I'd really prefer (if it works) to use the approach that our Android team is taking.  Please ping dmose if you have any questions.
Attachment #692742 - Flags: review+ → review-
Blocks: 825112
Blocks: 825110
Attached patch part 1, configure.in - v2 (obsolete) — Splinter Review
1. Address comment 18. It creates the makefiles for test cases and disable building them from toolkit/toolkit-makefiles.sh.
2. The network patch is no need when I update to the latest code.
Attachment #692740 - Attachment is obsolete: true
Attachment #692742 - Attachment is obsolete: true
Attachment #697760 - Flags: review+
Attached patch part 2 - media - v2 (obsolete) — Splinter Review
Address comment 21. 

Here is the try server log. 
* try: -b do -p all -u all -t none
** https://tbpl.mozilla.org/?tree=Try&rev=25607b1471b1
Attachment #692744 - Attachment is obsolete: true
Attachment #697764 - Flags: review+
Keywords: checkin-needed
Comment on attachment 697760 [details] [diff] [review]
part 1, configure.in - v2

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

::: configure.in
@@ +5261,5 @@
>  if test -n "$MOZ_WEBRTC"; then
>      case "$target" in
>      *-android*|*-linuxandroid*)
> +        if test -n "$MOZ_B2G"; then
> +            MOZ_WEBRTC=1

You really want to turn WebRTC on for all gonk builds, even in it's current state?  (Not that that's necessarily wrong, I just wanted to be sure that it's intentional).
(In reply to Dan Mosedale (:dmose) from comment #25)
> You really want to turn WebRTC on for all gonk builds, even in it's current
> state?  (Not that that's necessarily wrong, I just wanted to be sure that
> it's intentional).
Hi Dan,

Thanks for your reminding. I think that the patch just enables building webrtc code. The code is not accessible except that we turn on MOZ_MEDIA_NAVIGATOR and media.navigator.enabled. So that I think it should not effect the stability of B2G.
https://hg.mozilla.org/mozilla-central/rev/6f5b253ab7b7
https://hg.mozilla.org/mozilla-central/rev/c9831bed6bb7
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
This broke building B2G on the mac.

/Users/dougt/builds/mozilla-central/media/webrtc/trunk/src/video_engine/../system_wrappers/interface/tick_util.h:180: error: 'timebase' was not declared in this scope/Users/dougt/builds/mozilla-central/me\
dia/webrtc/trunk/src/video_engine/../system_wrappers/interface/tick_util.h:184: error: 'KERN_SUCCESS' was not declared in this scope/Users/dougt/builds/mozilla-central/media/webrtc/trunk/src/video_engine/\
../system_wrappers/interface/tick_util.h:180: error: 'timebase' was not declared in this scope


/Users/dougt/builds/mozilla-central/media/webrtc/trunk/src/video_engine/../system_wrappers/interface/tick_util.h:183: error: 'kern_return_t' was not declared in this scope/Users/dougt/builds/mozilla-centr\
al/media/webrtc/trunk/src/video_engine/../system_wrappers/interface/tick_util.h:183: error: 'kern_return_t' was not declared in this scope

/Users/dougt/builds/mozilla-central/media/webrtc/trunk/src/video_engine/../system_wrappers/interface/tick_util.h:183: error: expected ';' before 'retval'/Users/dougt/builds/mozilla-central/media/webrtc/tr\
unk/src/video_engine/../system_wrappers/interface/tick_util.h:183: error: expected ';' before 'retval'

/Users/dougt/builds/m/Users/dougt/builds/mozilla-central/media/webrtc/trunk/src/video_engine/../system_wrappers/interface/tick_util.h:191: error: 'mach_absolute_time' was not declared in this scopemake[5]\
: 
ozilla-central/media/webrtc/trunk/src/video_engine/../system_wrappers/interface/tick_util.h:184: error: 'retval' was not declared in this scope*** [vie_base_impl.o] Error 1

/Users/dougt/builds/mozilla-central/media/webrtc/trunk/src/video_engine/../system_wrappers/interface/tick_util.h:184: error: 'KERN_SUCCESS' was not declared in this scope/Users/dougt/builds/mozilla-centra\
l/media/webrtc/trunk/src/video_engine/../system_wrappers/interface/tick_util.h:191: error: 'timebase' was not declared in this scope

make[5]: *** Waiting for unfinished jobs....
/Users/dougt/builds/mozilla-central/media/webrtc/trunk/src/video_engine/../system_wrappers/interface/tick_util.h:184: error: 'retval' was not declared in this scope
/Users/dougt/builds/mozilla-central/media/webrtc/trunk/src/video_engine/../system_wrappers/interface/tick_util.h:184
: error: 'KERN_SUCCESS' was not declared in this scope/Users/dougt/builds/mozilla-central/media/webrtc/trunk/src/video_engine/../system_wrappers/interface/tick_util.h:191: error: 'mach_absolute_time' was \
not declared in this scope

/Users/dougt/builds/mozilla-central/media/webrtc/trunk/src/video_engine/../system_wrappers/interface/tick_util.h:191: error: 'timebase' was not declared in this scope
/Users/dougt/builds/mozilla-central/media/webrtc/trunk/src/video_engine/../system_wrappers/interface/tick_util.h:191: error: 'mach_absolute_time' was not declared in this scope
/Users/dougt/builds/mozilla-central/media/webrtc/trunk/src/video_engine/../system_wrappers/interface/tick_util.h:191: error: 'timebase' was not declared in this scope



Sorry - Backed out:

  https://hg.mozilla.org/mozilla-central/rev/a2c05e82dd89
  https://hg.mozilla.org/mozilla-central/rev/8155dd7632bb
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Doug Turner (:dougt) from comment #29)
> This broke building B2G on the mac.
Hi Doug,

Did you build B2G desktop for MAC? Could you give your os version?
[1] uses the similar code and it can be built. Do I miss something?
Thanks.

[1] https://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/time_mac.cc#90
Attached patch part 2 - media - v3 (obsolete) — Splinter Review
rebase
Attachment #697764 - Attachment is obsolete: true
Attachment #704387 - Flags: review+
(In reply to StevenLee from comment #30)
> [1]
> https://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/
> time_mac.cc#90
This code could be compile on mac. 
Here is the try server log. https://tbpl.mozilla.org/?tree=Try&rev=c028a0bcdd95
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e8aa49207d1c
https://hg.mozilla.org/mozilla-central/rev/9b803c2821b9
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Were the configure.in changes reviewed by a build peer?
Steven, this is still broken on mac.  I filed bug 834859.

The tryserver log you point to does not show the build working on mac.  What is broken is building B2G for a device (e.g. Otoro) on Mac.  Tryserver does not test this configuration.  I filed bug 834890 on this.
I'm testing that my backout actually works.  If so, I'll push it.
Also, before pushing again please get review from ted (:ted).  All significant makesystem changes (especially configure.in) need build peer review, including changes to .gyp files.  Most of these are straightforward, but it's still required (and can catch problems like this backout).
Backed out, sorry.

Please note that the error in comment 29 was not the only one I saw.  It also appeared that we were trying to build Mac-specific components in this cross-compiled build:

> src/media/webrtc/trunk/src/modules/audio_device/main/source/mac/audio_mixer_manager_mac.h:25: error: 'AudioDeviceID' has not been declared

https://hg.mozilla.org/integration/mozilla-inbound/rev/38c61e9826b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2a2c71ce437
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [WebRTC] [blocking-webrtc-][getusermedia][blocking-gum-] → [do not land if checkin-needed but no build peer review][WebRTC] [blocking-webrtc-][getusermedia][blocking-gum-]
https://hg.mozilla.org/mozilla-central/rev/38c61e9826b2
https://hg.mozilla.org/mozilla-central/rev/e8aa49207d1c
Whiteboard: [do not land if checkin-needed but no build peer review][WebRTC] [blocking-webrtc-][getusermedia][blocking-gum-] → [don't request checkin until this has build peer review][WebRTC] [blocking-webrtc-][getusermedia][blocking-gum-]
Target Milestone: mozilla20 → ---
Whiteboard: [don't request checkin until this has build peer review][WebRTC] [blocking-webrtc-][getusermedia][blocking-gum-] → [don't request checkin until this has build peer review and actually builds on mac][WebRTC] [blocking-webrtc-][getusermedia][blocking-gum-]
Hi Justin,

Sorry for the inconvenience. 
I will borrow a Mac and fix the compiling error.

Hi Ms2ger,
I will ask ted for review my patch when I work out a new version.
Attached patch part 1, configure.in - v3 (obsolete) — Splinter Review
Attachment #697760 - Attachment is obsolete: true
Attachment #721013 - Flags: review?(ted)
Attached patch part 2 - media - v4 (obsolete) — Splinter Review
Attachment #704387 - Attachment is obsolete: true
Attachment #721017 - Flags: review?(ted)
Attachment #721017 - Flags: review?(dmose)
Attached patch part 3, do not build gtest - v1 (obsolete) — Splinter Review
Here is the try server log. https://tbpl.mozilla.org/?tree=Try&rev=e0ae24cf8aa4
I try to build the opt version of all platforms and test Android-Only Unittest Suites. There is one fault, [uploadsymbols] Error 1. It's related to bug 847683.
Attachment #721020 - Flags: review?(ted)
Depends on: 847683
No longer depends on: 819246
BTW, I could also build Fennec, FireFox, and FFOS on Mac with these patches applied.
wonder if Ted got some time to review this?
Flags: needinfo?(ted)
Attachment #724240 - Flags: review?(ted)
Comment on attachment 721013 [details] [diff] [review]
part 1, configure.in - v3

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

::: configure.in
@@ +9042,5 @@
> +   if test "${MOZ_WIDGET_TOOLKIT}" != "gonk"; then
> +      EXTRA_GYP_DEFINES="-D gtest_target_type=executable -D android_toolchain=${android_toolchain} -G os=android "
> +   else
> +      EXTRA_GYP_DEFINES="-G os=linux "
> +   fi

I would swap these two branches around so that your test is MOZ_WIDGET_TOOLKIT = gonk.

@@ +9062,5 @@
>  
> +   if test "${MOZ_WIDGET_TOOLKIT}" = "gonk"; then
> +      EXTRA_GYP_DEFINES="${EXTRA_GYP_DEFINES} -D moz_widget_toolkit_gonk=1"
> +   else
> +      EXTRA_GYP_DEFINES="${EXTRA_GYP_DEFINES} -D moz_widget_toolkit_gonk=0"

This is a little kludgy. Could we add moz_widget_toolkit_gonk=0 as a default in a gypfile somewhere?
Attachment #721013 - Flags: review?(ted) → review+
Flags: needinfo?(ted)
Comment on attachment 721017 [details] [diff] [review]
part 2 - media - v4

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

#media suggested that ehugg review the cpr changes. I'm only reviewing the gyp changes.

::: media/webrtc/trunk/build/common.gypi
@@ +401,5 @@
>  
>        'conditions': [
> +         ['moz_widget_toolkit_gonk==1', {
> +           'variables': {
> +             'disable_sse2': 1,

Shouldn't this be covered by the
 ['target_arch=="arm" or target_arch=="mipsel"', {
block already in this file?

::: media/webrtc/trunk/webrtc/build/common.gypi
@@ +224,5 @@
>          ],
>        }],
>        ['OS=="linux"', {
> +        'conditions': [
> +          ['moz_have_clock_monotonic==1', {

Seems like this doesn't really need a moz prefix, it's a pretty generic thing.
Attachment #721017 - Flags: review?(ted)
Attachment #721017 - Flags: review?(ethanhugg)
Attachment #721017 - Flags: review?(dmose)
Attachment #721017 - Flags: review+
Comment on attachment 721020 [details] [diff] [review]
part 3, do not build gtest - v1

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

r- for the toolkit.mozbuild piece, but otherwise looks ok with a few nits.

::: media/webrtc/moz.build
@@ +9,3 @@
>  ]
>  
> +if not CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk':

You want to say CONFIG['MOZ_WIDGET_TOOLKIT'] != 'gonk':
(protip: moz.build files are just Python)

::: toolkit/toolkit.mozbuild
@@ +136,5 @@
>      add_tier_dir('platform', 'testing/specialpowers')
>  
>  if CONFIG['MOZ_ENABLE_GTEST']:
> +    if CONFIG['MOZ_WIDGET_TOOLKIT'] != 'gonk':
> +        add_tier_dir('platform', 'testing/gtest')

This is not the right way to do this. You should fix configure to not enable MOZ_ENABLE_GTEST for gonk until we can figure out how to make it build. (and I'd appreciate a followup on making it build)

@@ +249,5 @@
>          'testing/modules',
>      ])
>  
>      if CONFIG['MOZ_WEBRTC']:
> +        if CONFIG['MOZ_WIDGET_TOOLKIT'] != 'gonk':

You can just and this into the if CONFIG['MOZ_WEBRTC'] instead of making a nested if.
Attachment #721020 - Flags: review?(ted) → review-
Attachment #724240 - Flags: review?(ted) → review+
Comment on attachment 721017 [details] [diff] [review]
part 2 - media - v4

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

Good to see you were able to re-use the Linux version of CPR instead of duping another one.

::: media/mtransport/third_party/nICEr/nicer.gyp
@@ +206,5 @@
>  		 ],
> +              }],
> +              ['moz_widget_toolkit_gonk==1', {
> +                'defines' : [
> +                  'WEBRTC_GONK',

I didn't see an instance of this define being used in mtransport, is there another patch that uses it?

::: media/webrtc/trunk/webrtc/system_wrappers/source/thread_posix.cc
@@ +315,5 @@
>      CriticalSectionScoped cs(crit_state_);
>      alive_ = true;
>      dead_  = false;
>    }
> +#if (defined(WEBRTC_LINUX) || defined(WEBRTC_ANDROID) || defined(WEBRTC_GONK)) 

We generally avoid adding trailing whitespace to this code.
Attachment #721017 - Flags: review?(ethanhugg) → review+
Attached patch part 1, configure.in - v4 (obsolete) — Splinter Review
1. Fixed nits mentioned in comment 48.
2. change moz_have_clock_monotonic to have_clock_monotonic
Attachment #721013 - Attachment is obsolete: true
Attachment #725329 - Flags: review+
Attached patch part 2 - media - v5 (obsolete) — Splinter Review
1. Fix nits in comment 49 and comment 51
2. Add definition, MOZ_MEDIA_NAVIGATOR=1, in b2g/confvars.sh. Sorry for that I did not put it in the previous patch.
3. moz_widget_toolkit_gonk has default value 0
4. set "include_internal_audio_device" in configure.in. In the previous version, it is set in supplement.gypi. This flag should be removed when we are porting audio devices. 

I have new changes. Could you review it again? Thanks.
Attachment #721017 - Attachment is obsolete: true
Attachment #725334 - Flags: review?(ted)
Fix the nits in comment 50.
Attachment #721020 - Attachment is obsolete: true
Attachment #725335 - Flags: review?(ted)
Add reviewer information
Attachment #724240 - Attachment is obsolete: true
Attachment #725336 - Flags: review+
Here is the result on try server:

https://tbpl.mozilla.org/?tree=Try&rev=80c39b966814
Comment on attachment 725334 [details] [diff] [review]
part 2 - media - v5

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

::: media/webrtc/trunk/build/common.gypi
@@ +398,5 @@
>  
>        'sas_dll_path%': '<(DEPTH)/third_party/platformsdk_win7/files/redist/x86',
>        'wix_path%': '<(DEPTH)/third_party/wix',
>  
> +       'conditions': [

Did you accidentally indent this by a space?
Attachment #725334 - Flags: review?(ted) → review+
Attachment #725335 - Flags: review?(ted) → review+
addressed comment 57
Attachment #725334 - Attachment is obsolete: true
Attachment #725780 - Flags: review+
Keywords: checkin-needed
Attachment #689151 - Attachment is obsolete: true
Patch 1 doesn't apply cleanly. Please rebase.
Keywords: checkin-needed
Whiteboard: [don't request checkin until this has build peer review and actually builds on mac][WebRTC] [blocking-webrtc-][getusermedia][blocking-gum-] → [WebRTC] [blocking-webrtc-][getusermedia][blocking-gum-]
rebase
Attachment #725329 - Attachment is obsolete: true
Attachment #726712 - Flags: review+
Keywords: checkin-needed
Whiteboard: [WebRTC] [blocking-webrtc-][getusermedia][blocking-gum-] → [WebRTC] [blocking-webrtc-][getusermedia][blocking-gum-][qa-]
Blocks: b2g-webrtc
No longer blocks: 862026
You need to log in before you can comment on or make changes to this bug.