Last Comment Bug 757637 - Rollup of make system changes for WebRTC
: Rollup of make system changes for WebRTC
Status: RESOLVED FIXED
[qa-]
:
Product: Core
Classification: Components
Component: WebRTC (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Randell Jesup [:jesup]
:
:
Mentors:
Depends on: 643692
Blocks: webrtc 771981 1269165
  Show dependency treegraph
 
Reported: 2012-05-22 15:31 PDT by Randell Jesup [:jesup]
Modified: 2016-04-30 21:52 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Rollup patch for gyp->Makefile generator, etc (65.97 KB, patch)
2012-05-22 15:31 PDT, Randell Jesup [:jesup]
mh+mozilla: feedback-
Details | Diff | Splinter Review
Rollup media/webrtc/trunk changes from webrtc.org drop (83.78 KB, patch)
2012-05-22 16:05 PDT, Randell Jesup [:jesup]
tterribe: review+
ted: review+
Details | Diff | Splinter Review
Remove hacks for symbol visibility and makefile regeneration from gyp files (26.62 KB, patch)
2012-05-23 12:33 PDT, Randell Jesup [:jesup]
no flags Details | Diff | Splinter Review
Updated rollup makesystem patch (42.00 KB, patch)
2012-05-23 12:51 PDT, Randell Jesup [:jesup]
no flags Details | Diff | Splinter Review
Updated rollup makesystem patch (v2) (41.98 KB, patch)
2012-05-25 01:25 PDT, Randell Jesup [:jesup]
khuey: review-
ted: review+
mh+mozilla: feedback+
Details | Diff | Splinter Review
updates against previous patch per review (13.05 KB, patch)
2012-06-07 12:05 PDT, Randell Jesup [:jesup]
tterribe: review+
Details | Diff | Splinter Review
updates against previous patch per review (25.95 KB, patch)
2012-06-15 12:08 PDT, Randell Jesup [:jesup]
khuey: review+
Details | Diff | Splinter Review

Description Randell Jesup [:jesup] 2012-05-22 15:31:12 PDT
Created attachment 626213 [details] [diff] [review]
Rollup patch for gyp->Makefile generator, etc

This rolls up the majority of the makesystem changes for WebRTC, in particular the gyp->Makefile converter ted wrote in bug 643692.

Note that the current patch uploaded here has the default as webrtc enabled, and before landing I plan to switch it disabled.  Also the patch includes generation of the makefiles for media/webrtc/signaling, which will land in a later traunch, and so will be removed before landing.

This is not a standalone patch, it's being used to partition out the code for individual reviews.  There will be further patches such as the changes to the webrtc gyp files to work with this.

This covers everything outside of media (and includes media/webrtc/Makefile.in and shared_libs.mk)
Comment 1 Randell Jesup [:jesup] 2012-05-22 15:33:51 PDT
Comment on attachment 626213 [details] [diff] [review]
Rollup patch for gyp->Makefile generator, etc

Looking for initial review; I expect to have issues to deal with.  This is the first primary blocker to initial preffed-off landing of webrtc.

(ducks for cover)
Comment 2 Randell Jesup [:jesup] 2012-05-22 16:05:16 PDT
Created attachment 626228 [details] [diff] [review]
Rollup media/webrtc/trunk changes from webrtc.org drop

Does not include windows video capture code or any third_party/* modules
Mostly makesystem code, a few bugfixes, and also libvpx interface-level changes
Comment 3 Randell Jesup [:jesup] 2012-05-22 16:06:41 PDT
The second patch has the coordinating changes to gyp for mozmake (os.sep, etc), along with changes to the gyp files themselves (filtering out cflags except for one or two exceptions, etc).
Comment 4 Mike Hommey [:glandium] 2012-05-22 22:42:20 PDT
Comment on attachment 626213 [details] [diff] [review]
Rollup patch for gyp->Makefile generator, etc

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

::: client.mk
@@ +253,5 @@
>  	$(wildcard $(TOPSRCDIR)/build/autoconf/*.m4) \
>  	$(TOPSRCDIR)/js/src/aclocal.m4 \
> +	$(wildcard $(TOPSRCDIR)/media/webrtc/build/autoconf/*.m4) \
> +	$(TOPSRCDIR)/media/webrtc/signaling/signaling.gyp \
> +	$(TOPSRCDIR)/media/webrtc/trunk/test/metrics.gyp \

EXTRA_CONFIG_DEPS is used as a dependency to rebuild configure and js/src/configure. Why do you want to refresh them when a gyp file changes? It's running configure that would be necessary, but then, it would be better to have an equivalent to make-makefile and remake them individually, like the rules re-making Makefiles.

::: config/rules.mk
@@ +643,5 @@
>  endif # LIBRARY_NAME
>  
> +#ifneq (,$(filter-out %.$(LIB_SUFFIX),$(SHARED_LIBRARY_LIBS)))
> +#$(error SHARED_LIBRARY_LIBS must contain .$(LIB_SUFFIX) files only)
> +#endif

Commenting this out is not a very good idea right now. However, this is likely going to go away with bug 757339.

::: toolkit/library/Makefile.in
@@ +590,5 @@
>  endif
>  
> +ifdef MOZ_WEBRTC
> +ifeq ($(OS_TARGET),Linux)
> +OS_LIBS += -lexpat

Do we really want to use system expat when we have ours?
Comment 5 Randell Jesup [:jesup] 2012-05-22 23:00:32 PDT
(In reply to Mike Hommey [:glandium] from comment #4)
> Comment on attachment 626213 [details] [diff] [review]
> Rollup patch for gyp->Makefile generator, etc
> 
> Review of attachment 626213 [details] [diff] [review]:

Thanks for the quick response

> -----------------------------------------------------------------
> 
> ::: client.mk
> @@ +253,5 @@
> >  	$(wildcard $(TOPSRCDIR)/build/autoconf/*.m4) \
> >  	$(TOPSRCDIR)/js/src/aclocal.m4 \
> > +	$(wildcard $(TOPSRCDIR)/media/webrtc/build/autoconf/*.m4) \
> > +	$(TOPSRCDIR)/media/webrtc/signaling/signaling.gyp \
> > +	$(TOPSRCDIR)/media/webrtc/trunk/test/metrics.gyp \
> 
> EXTRA_CONFIG_DEPS is used as a dependency to rebuild configure and
> js/src/configure. Why do you want to refresh them when a gyp file changes?
> It's running configure that would be necessary, but then, it would be better
> to have an equivalent to make-makefile and remake them individually, like
> the rules re-making Makefiles.

It might, but note that we'd have to track the in-gypfile dependencies as well (they include each other).

While annoying (somewhat) having it rebuild all the makefiles when any of the gyp files changes is safe and guaranteed not to miss any dependencies, instead of spending a lot of time building a dependency-parser for gyp files.  And gyp->makefile is fast, though on Windows configure itself is slow for no good reason.

I'd be good with filing a bug to eventually replace this with something fancier.

> 
> ::: config/rules.mk
> @@ +643,5 @@
> >  endif # LIBRARY_NAME
> >  
> > +#ifneq (,$(filter-out %.$(LIB_SUFFIX),$(SHARED_LIBRARY_LIBS)))
> > +#$(error SHARED_LIBRARY_LIBS must contain .$(LIB_SUFFIX) files only)
> > +#endif
> 
> Commenting this out is not a very good idea right now. However, this is
> likely going to go away with bug 757339.
> 
> ::: toolkit/library/Makefile.in
> @@ +590,5 @@
> >  endif
> >  
> > +ifdef MOZ_WEBRTC
> > +ifeq ($(OS_TARGET),Linux)
> > +OS_LIBS += -lexpat
> 
> Do we really want to use system expat when we have ours?

Ours is PR_UniChar's, we need one for char * (until we eliminate libjingle or at least the major portion of it, which we plan to do).  Right now sippcc (signalling library to replace jingle) uses it too, to be seen if we can eliminate that use.  Initial landing will not include libjingle or sipcc.
Comment 6 Mike Hommey [:glandium] 2012-05-22 23:08:52 PDT
(In reply to Randell Jesup [:jesup] from comment #5)
> > Do we really want to use system expat when we have ours?
> 
> Ours is PR_UniChar's, we need one for char * (until we eliminate libjingle
> or at least the major portion of it, which we plan to do).  Right now sippcc
> (signalling library to replace jingle) uses it too, to be seen if we can
> eliminate that use.  Initial landing will not include libjingle or sipcc.

Why do you need it on linux only, then?
Comment 7 Randell Jesup [:jesup] 2012-05-23 02:34:40 PDT
(In reply to Mike Hommey [:glandium] from comment #6)
> (In reply to Randell Jesup [:jesup] from comment #5)
> > > Do we really want to use system expat when we have ours?
> > 
> > Ours is PR_UniChar's, we need one for char * (until we eliminate libjingle
> > or at least the major portion of it, which we plan to do).  Right now sippcc
> > (signalling library to replace jingle) uses it too, to be seen if we can
> > eliminate that use.  Initial landing will not include libjingle or sipcc.
> 
> Why do you need it on linux only, then?

Windows doesn't have it (we build a copy in third_party), and Mac should use it but for some odd reason it doesn't work (need to dig up the bug on that; caused a lot of pain).  Eventually we should use it on Mac as well.
Comment 8 Ted Mielczarek [:ted.mielczarek] 2012-05-23 05:04:41 PDT
I didn't add that bit to client.mk, that should be unnecessary. The gyp backend writes out proper dependencies to regenerate Makefiles as-needed.

The expat situation is unfortunate, but as jesup described it's due to our expat being UTF-16 and libjingle wanting a UTF-8 one. I believe Windows does build the in-WebRTC-tree copy, and I don't remember what happens on Mac.
Comment 9 Ted Mielczarek [:ted.mielczarek] 2012-05-23 05:06:41 PDT
Comment on attachment 626213 [details] [diff] [review]
Rollup patch for gyp->Makefile generator, etc

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

::: media/webrtc/trunk/tools/gyp/pylib/gyp/generator/mozmake.py
@@ +99,5 @@
> +endif
> +
> +# Rules for regenerating Makefiles from GYP files.
> +Makefile: %(input_gypfiles)s %(generator)s
> +	$(PYTHON) %(commandline)s

Here's the bit with the rules for regenerating Makefiles from GYP files.

@@ +432,5 @@
> +                 "--toplevel-dir=$(topsrcdir)",
> +                 #XXX: handle other generator_flags gracefully?
> +                 "-G OBJDIR=$(DEPTH)"] + \
> +                 ['-D%s' % d for d in options.defines] + \
> +                 [topsrcdir_path(b) for b in params['build_files']]

You can see here where we cobble the commandline back together to stick it in the Makefile.
Comment 10 Mike Hommey [:glandium] 2012-05-23 07:18:33 PDT
(In reply to Mike Hommey [:glandium] from comment #4)
> > +#ifneq (,$(filter-out %.$(LIB_SUFFIX),$(SHARED_LIBRARY_LIBS)))
> > +#$(error SHARED_LIBRARY_LIBS must contain .$(LIB_SUFFIX) files only)
> > +#endif
> 
> Commenting this out is not a very good idea right now. However, this is
> likely going to go away with bug 757339.

I have to retract on this. SHARED_LIBRARY_LIBS is used for static libraries, too, and putting things in there that are not libraries is wrong. Use EXTRA_DSO_LDOPTS instead.
Comment 11 Randell Jesup [:jesup] 2012-05-23 11:52:30 PDT
Aha.  The client.mk thing was a temporary hack until the gypfile dependency was in place; I'd forgotten that and when I saw it I assumed that was how ted had handled it (seemed rather bear-skins and stone-knives, and that's because it is).  I'll simply remove that.
Comment 12 Randell Jesup [:jesup] 2012-05-23 12:33:19 PDT
Created attachment 626550 [details] [diff] [review]
Remove hacks for symbol visibility and makefile regeneration from gyp files
Comment 13 Randell Jesup [:jesup] 2012-05-23 12:51:28 PDT
Created attachment 626560 [details] [diff] [review]
Updated rollup makesystem patch

Incorporates patch 626550 to remove old hacks
Comment 14 Randell Jesup [:jesup] 2012-05-23 12:55:10 PDT
Comment on attachment 626228 [details] [diff] [review]
Rollup media/webrtc/trunk changes from webrtc.org drop

r? to derf for the vp8 parts, khuey for the rest.

The ALSA stuff is only until we import the fix from upstream (they finally fixed it; issue 450 I think at webrtc.org).
Comment 15 Timothy B. Terriberry (:derf) 2012-05-23 14:43:11 PDT
Comment on attachment 626228 [details] [diff] [review]
Rollup media/webrtc/trunk changes from webrtc.org drop

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

I've upgraded m-c to libvpx 1.0.0 now, so we may no longer need all the libvpx build changes, but there isn't actually anything wrong with them, so I'm fine giving an r+.

::: media/webrtc/trunk/src/modules/video_coding/codecs/vp8/main/source/vp8.gypi
@@ +39,5 @@
> +                 '$(DIST)/include',
> +               ],
> +               'defines': [
> +                 # This must be updated to match mozilla's version of libvpx
> +                 'WEBRTC_LIBVPX_VERSION=971',

You should be able to set this to 1000 (v1.0.0) now.

@@ +40,5 @@
> +               ],
> +               'defines': [
> +                 # This must be updated to match mozilla's version of libvpx
> +                 'WEBRTC_LIBVPX_VERSION=971',
> +                 'WEBRTC_LIBVPX_TEMPORAL_LAYERS=0'

You should also be able to set this to 1 now. Perhaps that means you don't actually need this as a separate #define now.
Comment 16 Randell Jesup [:jesup] 2012-05-24 12:15:55 PDT
Comment on attachment 626560 [details] [diff] [review]
Updated rollup makesystem patch

Asking r? to ted for parts not written by ted (or convert to f+/- if you prefer)
Comment 17 Randell Jesup [:jesup] 2012-05-24 12:19:06 PDT
Comment on attachment 626228 [details] [diff] [review]
Rollup media/webrtc/trunk changes from webrtc.org drop

Asking r? to ted for parts not written by ted (or convert to f+/- if you prefer).  Or ted and khuey can have a cage match to see who gets to escape reviewing it.  :-)

This is largely changes to gyp that I did (and the alsa fix waiting for us to pick up the upstream fix).
Comment 18 Randell Jesup [:jesup] 2012-05-25 01:25:10 PDT
Created attachment 627128 [details] [diff] [review]
Updated rollup makesystem patch (v2)

Very minor update to previous patch to remove JPEG_LIBS from gkmedia link now that jpeg is in gkmedia always.  (See bug 758413)

And I noticed that the old license is on the Makefile.in, and there's no license on mozmake.py (but I doubt if it should be, as the file is derived from make.py under gyp's license, even though it's a new file).
Comment 19 Mike Hommey [:glandium] 2012-05-25 01:39:13 PDT
(In reply to Randell Jesup [:jesup] from comment #18)
> And I noticed that the old license is on the Makefile.in, and there's no
> license on mozmake.py (but I doubt if it should be, as the file is derived
> from make.py under gyp's license, even though it's a new file).

There should be a license header in mozmake.py, *especially* if it's not MPL.
Comment 20 Ted Mielczarek [:ted.mielczarek] 2012-05-25 04:58:24 PDT
We should copy the BSD license headers from the other gyp code. The rest of GYP is BSD-licensed, and this is functionally a part of GYP, so it should be too.
Comment 21 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-06-06 14:09:52 PDT
Comment on attachment 626228 [details] [diff] [review]
Rollup media/webrtc/trunk changes from webrtc.org drop

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

I don't think there's anything here that I need to review.
Comment 22 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-06-06 14:13:49 PDT
Comment on attachment 627128 [details] [diff] [review]
Updated rollup makesystem patch (v2)

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

r- because I'd like to see this again.  Also, we need to figure out what to do about expat.

I also haven't looked at the gyp bits in detail, but I'm inclined not to look too hard.

::: configure.in
@@ +716,5 @@
>              AC_DEFINE(_CRT_SECURE_NO_WARNINGS)
>              AC_DEFINE(_CRT_NONSTDC_NO_WARNINGS)
>          elif test "$_CC_MAJOR_VERSION" = "17"; then
>              _CC_SUITE=11
> +            _MSVS_VERSION=2011

Yuck yuck yuck.  Why does gyp care what version of VS this is?

@@ +5522,5 @@
> +    MOZ_WEBM=1
> +    MOZ_RAW=1
> +    MOZ_VP8_ENCODER=1
> +    MOZ_VP8_ERROR_CONCEALMENT=1
> +fi

We need code that makes sure that someone who passes --disable-media also passes --disable-webrtc.  Ditto for the other things in here.

Also, why does WebRTC need MOZ_RAW?

@@ +8978,5 @@
> +   if test "${target_cpu}" = "x86_64"; then
> +      OS_BITS=64
> +   else
> +      OS_BITS=32
> +   fi

Just test HAVE_64BIT_OS.

@@ +8991,5 @@
> +     -D include_internal_video_render=0 \
> +     $EXTRA_GYP_DEFINES \
> +     --generator-output=${_objdir}/media/webrtc/trunk \
> +     --toplevel-dir=${srcdir} \
> +     -G OBJDIR=${_objdir} \

We repeat these defines 3 times.  Lets put them in a single variable and use that.

::: layout/media/symbols.def.in
@@ +154,5 @@
> +?GetInterface@ViEBase@webrtc@@SAPAV12@PAVVideoEngine@2@@Z
> +?Create@VideoEngine@webrtc@@SAPAV12@XZ
> +?Delete@VideoEngine@webrtc@@SA_NAAPAV12@@Z
> +#endif
> +#endif

Unless we absolutely have to do this, we should avoid it.  Can we add declspec dllexport/import to the relevant stuff?

::: media/webrtc/Makefile.in
@@ +33,5 @@
> +# and other provisions required by the GPL or the LGPL. If you do not delete
> +# the provisions above, a recipient may use your version of this file under
> +# the terms of any one of the MPL, the GPL or the LGPL.
> +#
> +# ***** END LICENSE BLOCK *****

MPL 2!

::: media/webrtc/shared_libs.mk
@@ +1,1 @@
> +# shared libs for webrtc

This needs a license header.

::: media/webrtc/trunk/tools/gyp/pylib/gyp/generator/mozmake.py
@@ +1,1 @@
> +# Python 2.5 needs this for the with statement.

This needs a license header.

::: toolkit/library/Makefile.in
@@ +452,5 @@
>  ifeq ($(MOZ_WIDGET_TOOLKIT),cocoa)
>  CXXFLAGS += $(TK_CFLAGS)
>  OS_LIBS += \
>    -framework SystemConfiguration \
> +  -framework QTKit \

I don't know enough about Mac to know if this is kosher.

@@ +592,5 @@
> +ifdef MOZ_WEBRTC
> +ifeq ($(OS_TARGET),Linux)
> +OS_LIBS += -lexpat
> +endif
> +endif

This makes me very sad.  We should avoid this if at all possible.
Comment 23 Randell Jesup [:jesup] 2012-06-06 15:18:38 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #22)
> Comment on attachment 627128 [details] [diff] [review]
> Updated rollup makesystem patch (v2)
> 
> Review of attachment 627128 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- because I'd like to see this again.  Also, we need to figure out what to
> do about expat.

Thanks!  I think I have answers for most of this.

> I also haven't looked at the gyp bits in detail, but I'm inclined not to
> look too hard.

Sure.  And glandium has looked at some of those (compiler flags, etc).  We basically ignore webrtc's compiler flags except for a few rare cases like sse2, and ted can look too.

> 
> ::: configure.in
> @@ +716,5 @@
> >              AC_DEFINE(_CRT_SECURE_NO_WARNINGS)
> >              AC_DEFINE(_CRT_NONSTDC_NO_WARNINGS)
> >          elif test "$_CC_MAJOR_VERSION" = "17"; then
> >              _CC_SUITE=11
> > +            _MSVS_VERSION=2011
> 
> Yuck yuck yuck.  Why does gyp care what version of VS this is?

trunk/build/common.gypi turns on and off compiler options (multi-core, incremental linking on 32-bit, pre-compiled headers)

> 
> @@ +5522,5 @@
> > +    MOZ_WEBM=1
> > +    MOZ_RAW=1
> > +    MOZ_VP8_ENCODER=1
> > +    MOZ_VP8_ERROR_CONCEALMENT=1
> > +fi
> 
> We need code that makes sure that someone who passes --disable-media also
> passes --disable-webrtc.  Ditto for the other things in here.

Gotcha.  Most of these are mandatory for webrtc, so we need to handle configure conflicts - what's the standard; kick out and say "don't do that"?

> Also, why does WebRTC need MOZ_RAW?

I believe the reason is so that video elements can work on raw YUV data.  It's possible this isn't really needed.  Derf?

> @@ +8978,5 @@
> > +   if test "${target_cpu}" = "x86_64"; then
> > +      OS_BITS=64
> > +   else
> > +      OS_BITS=32
> > +   fi
> 
> Just test HAVE_64BIT_OS.

Ok.

> @@ +8991,5 @@
> > +     -D include_internal_video_render=0 \
> > +     $EXTRA_GYP_DEFINES \
> > +     --generator-output=${_objdir}/media/webrtc/trunk \
> > +     --toplevel-dir=${srcdir} \
> > +     -G OBJDIR=${_objdir} \
> 
> We repeat these defines 3 times.  Lets put them in a single variable and use
> that.

Ok.

> ::: layout/media/symbols.def.in
> @@ +154,5 @@
> > +?GetInterface@ViEBase@webrtc@@SAPAV12@PAVVideoEngine@2@@Z
> > +?Create@VideoEngine@webrtc@@SAPAV12@XZ
> > +?Delete@VideoEngine@webrtc@@SA_NAAPAV12@@Z
> > +#endif
> > +#endif
> 
> Unless we absolutely have to do this, we should avoid it.  Can we add
> declspec dllexport/import to the relevant stuff?

I can try, but last time I looked it didn't seem to do what we needed (and I think that's why symbols.def.in exists...)  Most are .cc files; cairo puts everything in extern "C" - and they still all need to be listed there.  extern "C" would make it more readable, and avoid having to have two versions of each.  This will require mods to the main code; that's probably not a problem.

> ::: media/webrtc/Makefile.in
> @@ +33,5 @@
> > +# and other provisions required by the GPL or the LGPL. If you do not delete
> > +# the provisions above, a recipient may use your version of this file under
> > +# the terms of any one of the MPL, the GPL or the LGPL.
> > +#
> > +# ***** END LICENSE BLOCK *****
> 
> MPL 2!

Yup!  (Knew we needed to revise.)

> ::: media/webrtc/shared_libs.mk
> @@ +1,1 @@
> > +# shared libs for webrtc
> 
> This needs a license header.

Ok.

> ::: media/webrtc/trunk/tools/gyp/pylib/gyp/generator/mozmake.py
> @@ +1,1 @@
> > +# Python 2.5 needs this for the with statement.
> 
> This needs a license header.

This one is derived from /make.py so I assume it falls into the BSD-ish license that's under, so I don't know we add an MPL header.

> ::: toolkit/library/Makefile.in
> @@ +452,5 @@
> >  ifeq ($(MOZ_WIDGET_TOOLKIT),cocoa)
> >  CXXFLAGS += $(TK_CFLAGS)
> >  OS_LIBS += \
> >    -framework SystemConfiguration \
> > +  -framework QTKit \
> 
> I don't know enough about Mac to know if this is kosher.

Required to get to the A/V capture code on Mac.

> @@ +592,5 @@
> > +ifdef MOZ_WEBRTC
> > +ifeq ($(OS_TARGET),Linux)
> > +OS_LIBS += -lexpat
> > +endif
> > +endif
> 
> This makes me very sad.  We should avoid this if at all possible.

Already removed it (see comment I made in another bug on expat); the cost is a second copy of expat in our executable (and on built on Linux (Fedora 15), we still also dynamic link to expat as well, but the binaries probably won't).  (We need a char* expat; xul's (which we can't access anyways on Windows) is PR_UniChar*)
Comment 24 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-06-06 15:21:39 PDT
(In reply to Randell Jesup [:jesup] from comment #23)
> (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #22)
> > Comment on attachment 627128 [details] [diff] [review]
> > Updated rollup makesystem patch (v2)
> > 
> > Review of attachment 627128 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > r- because I'd like to see this again.  Also, we need to figure out what to
> > do about expat.
> 
> Thanks!  I think I have answers for most of this.
> 
> > I also haven't looked at the gyp bits in detail, but I'm inclined not to
> > look too hard.
> 
> Sure.  And glandium has looked at some of those (compiler flags, etc).  We
> basically ignore webrtc's compiler flags except for a few rare cases like
> sse2, and ted can look too.

Good.

> > 
> > ::: configure.in
> > @@ +716,5 @@
> > >              AC_DEFINE(_CRT_SECURE_NO_WARNINGS)
> > >              AC_DEFINE(_CRT_NONSTDC_NO_WARNINGS)
> > >          elif test "$_CC_MAJOR_VERSION" = "17"; then
> > >              _CC_SUITE=11
> > > +            _MSVS_VERSION=2011
> > 
> > Yuck yuck yuck.  Why does gyp care what version of VS this is?
> 
> trunk/build/common.gypi turns on and off compiler options (multi-core,
> incremental linking on 32-bit, pre-compiled headers)

Hmm, ok.  I need to look at this harder next time.

> > 
> > @@ +5522,5 @@
> > > +    MOZ_WEBM=1
> > > +    MOZ_RAW=1
> > > +    MOZ_VP8_ENCODER=1
> > > +    MOZ_VP8_ERROR_CONCEALMENT=1
> > > +fi
> > 
> > We need code that makes sure that someone who passes --disable-media also
> > passes --disable-webrtc.  Ditto for the other things in here.
> 
> Gotcha.  Most of these are mandatory for webrtc, so we need to handle
> configure conflicts - what's the standard; kick out and say "don't do that"?



> > Also, why does WebRTC need MOZ_RAW?
> 
> I believe the reason is so that video elements can work on raw YUV data. 
> It's possible this isn't really needed.  Derf?
> 
> > @@ +8978,5 @@
> > > +   if test "${target_cpu}" = "x86_64"; then
> > > +      OS_BITS=64
> > > +   else
> > > +      OS_BITS=32
> > > +   fi
> > 
> > Just test HAVE_64BIT_OS.
> 
> Ok.
> 
> > @@ +8991,5 @@
> > > +     -D include_internal_video_render=0 \
> > > +     $EXTRA_GYP_DEFINES \
> > > +     --generator-output=${_objdir}/media/webrtc/trunk \
> > > +     --toplevel-dir=${srcdir} \
> > > +     -G OBJDIR=${_objdir} \
> > 
> > We repeat these defines 3 times.  Lets put them in a single variable and use
> > that.
> 
> Ok.
> 
> > ::: layout/media/symbols.def.in
> > @@ +154,5 @@
> > > +?GetInterface@ViEBase@webrtc@@SAPAV12@PAVVideoEngine@2@@Z
> > > +?Create@VideoEngine@webrtc@@SAPAV12@XZ
> > > +?Delete@VideoEngine@webrtc@@SA_NAAPAV12@@Z
> > > +#endif
> > > +#endif
> > 
> > Unless we absolutely have to do this, we should avoid it.  Can we add
> > declspec dllexport/import to the relevant stuff?
> 
> I can try, but last time I looked it didn't seem to do what we needed (and I
> think that's why symbols.def.in exists...)  Most are .cc files; cairo puts
> everything in extern "C" - and they still all need to be listed there. 
> extern "C" would make it more readable, and avoid having to have two
> versions of each.  This will require mods to the main code; that's probably
> not a problem.
> 
> > ::: media/webrtc/Makefile.in
> > @@ +33,5 @@
> > > +# and other provisions required by the GPL or the LGPL. If you do not delete
> > > +# the provisions above, a recipient may use your version of this file under
> > > +# the terms of any one of the MPL, the GPL or the LGPL.
> > > +#
> > > +# ***** END LICENSE BLOCK *****
> > 
> > MPL 2!
> 
> Yup!  (Knew we needed to revise.)
> 
> > ::: media/webrtc/shared_libs.mk
> > @@ +1,1 @@
> > > +# shared libs for webrtc
> > 
> > This needs a license header.
> 
> Ok.
> 
> > ::: media/webrtc/trunk/tools/gyp/pylib/gyp/generator/mozmake.py
> > @@ +1,1 @@
> > > +# Python 2.5 needs this for the with statement.
> > 
> > This needs a license header.
> 
> This one is derived from /make.py so I assume it falls into the BSD-ish
> license that's under, so I don't know we add an MPL header.
> 
> > ::: toolkit/library/Makefile.in
> > @@ +452,5 @@
> > >  ifeq ($(MOZ_WIDGET_TOOLKIT),cocoa)
> > >  CXXFLAGS += $(TK_CFLAGS)
> > >  OS_LIBS += \
> > >    -framework SystemConfiguration \
> > > +  -framework QTKit \
> > 
> > I don't know enough about Mac to know if this is kosher.
> 
> Required to get to the A/V capture code on Mac.
> 
> > @@ +592,5 @@
> > > +ifdef MOZ_WEBRTC
> > > +ifeq ($(OS_TARGET),Linux)
> > > +OS_LIBS += -lexpat
> > > +endif
> > > +endif
> > 
> > This makes me very sad.  We should avoid this if at all possible.
> 
> Already removed it (see comment I made in another bug on expat); the cost is
> a second copy of expat in our executable (and on built on Linux (Fedora 15),
> we still also dynamic link to expat as well, but the binaries probably
> won't).  (We need a char* expat; xul's (which we can't access anyways on
> Windows) is PR_UniChar*)

Uh, that's a pain.
Comment 25 Randell Jesup [:jesup] 2012-06-06 16:03:45 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #24)

> > > 
> > > ::: configure.in
> > > @@ +716,5 @@
> > > >              AC_DEFINE(_CRT_SECURE_NO_WARNINGS)
> > > >              AC_DEFINE(_CRT_NONSTDC_NO_WARNINGS)
> > > >          elif test "$_CC_MAJOR_VERSION" = "17"; then
> > > >              _CC_SUITE=11
> > > > +            _MSVS_VERSION=2011
> > > 
> > > Yuck yuck yuck.  Why does gyp care what version of VS this is?
> > 
> > trunk/build/common.gypi turns on and off compiler options (multi-core,
> > incremental linking on 32-bit, pre-compiled headers)
> 
> Hmm, ok.  I need to look at this harder next time.

You may not need to: I believe these only apply to standalone executables generated via gyp I think; mostly test code.  This is the standard chromium common.gypi I think, or at least some snapshot of it.  I think we have another older snapshot (unused) in our tree in the ipc dir tree.

> > > @@ +592,5 @@
> > > > +ifdef MOZ_WEBRTC
> > > > +ifeq ($(OS_TARGET),Linux)
> > > > +OS_LIBS += -lexpat
> > > > +endif
> > > > +endif
> > > 
> > > This makes me very sad.  We should avoid this if at all possible.
> > 
> > Already removed it (see comment I made in another bug on expat); the cost is
> > a second copy of expat in our executable (and on built on Linux (Fedora 15),
> > we still also dynamic link to expat as well, but the binaries probably
> > won't).  (We need a char* expat; xul's (which we can't access anyways on
> > Windows) is PR_UniChar*)
> 
> Uh, that's a pain.

Yeah.  We need expat right now for signaling (libjingle and the replacement for most of it, sipcc (media/webrtc/signaling) both use it).  I'll circle with the sipcc people to see if we can (as we move toward full WebRTC) remove the sipcc parts that need expat.  Otherwise because of the gkmedia thing we're stuck with either two copies, or -lexpat on linux/mac and two copies on Windows.
Comment 26 Randell Jesup [:jesup] 2012-06-07 08:30:42 PDT
> > +    MOZ_WEBM=1
> > +    MOZ_RAW=1
> > +    MOZ_VP8_ENCODER=1
> > +    MOZ_VP8_ERROR_CONCEALMENT=1
> > +fi
> 
> We need code that makes sure that someone who passes --disable-media also
> passes --disable-webrtc.  Ditto for the other things in here.

MOZ_MEDIA, MOZ_VP8_ENCODER and MOZ_VP8_ERROR_CONCEALMENT have no --enable or disable options; they're turned on as needed by other options.

--disable-raw will already turn off MOZ_RAW (it's after MOZ_WEBRTC)
--disable-webm will (soon) turn off MOZ_WEBM without turning off VP8
Comment 27 Randell Jesup [:jesup] 2012-06-07 12:05:47 PDT
Created attachment 631070 [details] [diff] [review]
updates against previous patch per review
Comment 28 Randell Jesup [:jesup] 2012-06-07 12:10:06 PDT
Comment on attachment 631070 [details] [diff] [review]
updates against previous patch per review

Switched default to preffed off (per landing plan)

mozmake.py doesn't seem to actually be derived from make.py, so I licensed that.
I broke out webm from webrtc, which required being able to build libvpx without webm - derf, please look at that. I've done builds with all the various combinations.

And while not shown here, I also removed -lexpat from linux per my comment
Comment 29 Randell Jesup [:jesup] 2012-06-07 12:13:57 PDT
Comment on attachment 631070 [details] [diff] [review]
updates against previous patch per review

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

::: media/webrtc/trunk/src/build/common.gypi
@@ +137,5 @@
>        }],
>        ['OS=="win"', {
>          'defines': [
>            'WEBRTC_WIN',
> +	  'WEBRTC_EXPORT',

This is experimental to try to provoke _declspec(dllexport) on some of the symbols we need to export
Comment 30 Timothy B. Terriberry (:derf) 2012-06-07 12:21:17 PDT
Comment on attachment 631070 [details] [diff] [review]
updates against previous patch per review

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

LTGM.

::: config/autoconf.mk.in
@@ +145,5 @@
>  MOZ_OMX_PLUGIN = @MOZ_OMX_PLUGIN@
>  MOZ_GSTREAMER = @MOZ_GSTREAMER@
>  MOZ_VP8_ERROR_CONCEALMENT = @MOZ_VP8_ERROR_CONCEALMENT@
>  MOZ_VP8_ENCODER = @MOZ_VP8_ENCODER@
> +MOZ_VP8 = @MOZ_VP8@

I'd prefer to have this above the other two VP8 defines here (it's a logical progression: decoder, better decoder, encoder), but that's no big deal.

::: configure.in
@@ +4551,4 @@
>  MOZ_MEDIA_PLUGINS=
>  MOZ_MEDIA_NAVIGATOR=
>  MOZ_OMX_PLUGIN=
> +MOZ_VP8=

Like here.

@@ +8842,5 @@
>  AC_SUBST(MOZ_MEDIA_PLUGINS)
>  AC_SUBST(MOZ_OMX_PLUGIN)
>  AC_SUBST(MOZ_VP8_ERROR_CONCEALMENT)
>  AC_SUBST(MOZ_VP8_ENCODER)
> +AC_SUBST(MOZ_VP8)

Same here.
Comment 31 Ted Mielczarek [:ted.mielczarek] 2012-06-07 12:23:36 PDT
I'd still like to leave mozmake.py as BSD licensed since it runs as part of GYP. Even if we never upstream it, it's still weird for it to have a license different to the rest of GYP.
Comment 32 Randell Jesup [:jesup] 2012-06-07 12:42:59 PDT
(In reply to Ted Mielczarek [:ted] from comment #31)
> I'd still like to leave mozmake.py as BSD licensed since it runs as part of
> GYP. Even if we never upstream it, it's still weird for it to have a license
> different to the rest of GYP.

Sure.  Will do; that was my original assumption, I'll go back to that.  Should I put a note to that effect in there?

CCing gerv for his comments.
Comment 33 Ted Mielczarek [:ted.mielczarek] 2012-06-07 12:45:16 PDT
You can slap whatever license header GYP uses on there. I consider it a patch to upstream software, even if we don't actually upstream it.
Comment 34 Ted Mielczarek [:ted.mielczarek] 2012-06-07 13:12:06 PDT
Comment on attachment 626228 [details] [diff] [review]
Rollup media/webrtc/trunk changes from webrtc.org drop

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

::: media/webrtc/trunk/peerconnection.gyp
@@ +13,5 @@
>    },  
>  
> +  # for mozilla, we want to force stuff to build but we don't want peerconnection_client or server
> +  # for unknown reasons, 'target' must be outside of conditions.  And don't try to build a dummy
> +  # executable...

I can't recall exactly why, but I think they wanted to pull in the protobuf stuff, and we had a hard time building that.

@@ +26,5 @@
> +          'action_name': 'dummy',
> +	  'action': [
> +	     'echo ARGHHHHHHHHHHHHHHHHHHHH',
> +          ],
> +          'message': 'Generating scream',

I LOLed at this a little.

::: media/webrtc/trunk/src/build/common.gypi
@@ +66,5 @@
>          'webrtc_root%': '<(DEPTH)/third_party/webrtc',
>        }, {
>          # Settings for the standalone (not-in-Chromium) build.
>  
> +        'include_pulse_audio%': 0,

What's the plan for merging this with upstream? You have a lot of fiddly changes here.

(Side note: the WebRTC standalone build seems like bullshit, since lots of things were broken.)

::: media/webrtc/trunk/src/build/getsdksamplesdir.py
@@ +13,5 @@
> +    print >>sys.stderr, "Could not locate Windows SDK Samples directory via the registry"
> +    return 1
> +
> +if __name__ == '__main__':
> +    sys.exit(main())

I actually don't think we need this anymore since we rewrote all that code instead of using the stuff from the SDK.

::: media/webrtc/trunk/src/supplement.gypi
@@ +1,2 @@
>  #!/usr/bin/env python
>  # This file is generated by trunk/tools/create_supplement_gypi.py.  Not for check-in.

Does this get generated as part of the build? If so, we should not include it like it says.

::: media/webrtc/trunk/tools/gyp/pylib/gyp/generator/make.py
@@ +213,5 @@
>  
>  # The source directory tree.
>  srcdir := %(srcdir)s
>  abs_srcdir := $(abspath $(srcdir))
> +os_sep := %(os_sep)s

We should drop all these changes to make.py since we're not using it.

::: media/webrtc/trunk/tools/gyp/pylib/gyp/mac_tool.py
@@ +40,5 @@
>  
>    def ExecFlock(self, lockfile, *cmd_list):
>      """Emulates the most basic behavior of Linux's flock(1)."""
>      # Rely on exception handling to report errors.
> +    fd = os.open(lockfile, os.O_RDONLY|os.O_NOCTTY|os.O_CREAT, 0666)

Also this.
Comment 35 Ted Mielczarek [:ted.mielczarek] 2012-06-07 13:16:05 PDT
Comment on attachment 627128 [details] [diff] [review]
Updated rollup makesystem patch (v2)

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

r+ if you address khuey's concerns.

::: media/webrtc/Makefile.in
@@ +44,5 @@
> +include $(DEPTH)/config/autoconf.mk
> +
> +DIRS = trunk \
> +       trunk/testing \
> +       $(NULL)

This should be:
DIRS = \
  trunk \
  trunk/testing \
  $(NULL)
(two spaces, not tabs)

::: media/webrtc/shared_libs.mk
@@ +1,3 @@
> +# shared libs for webrtc
> +SHARED_LIBRARY_LIBS += \
> +	$(call EXPAND_LIBNAME_PATH,jingle,$(DEPTH)/media/webrtc/trunk/third_party/libjingle/libjingle_libjingle) \

Two spaces, not tabs.

::: toolkit/toolkit-tiers.mk
@@ +127,5 @@
>  endif
>  
> +ifdef MOZ_WEBRTC
> +tier_platform_dirs += \
> +	        media/webrtc \

I know you're just copying what's already here, but two-space indent please.
Comment 36 Randell Jesup [:jesup] 2012-06-07 13:34:44 PDT
>::: media/webrtc/trunk/src/supplement.gypi
>@@ +1,2 @@
>>  #!/usr/bin/env python
>>  # This file is generated by trunk/tools/create_supplement_gypi.py.  Not for check-in.

> Does this get generated as part of the build? If so, we should not include it like it says.

It gets generated when we check out the code from svn, so we really need to import it (and we need a modification anyways).  The update procedure should pick up any upstream changes correctly and merge our change into the result.

> -    fd = os.open(lockfile, os.O_RDONLY|os.O_NOCTTY|os.O_CREAT, 0o666)
> +    fd = os.open(lockfile, os.O_RDONLY|os.O_NOCTTY|os.O_CREAT, 0666)

I want to keep this (I'll upstream it as well).  0o666 is simply wrong.
Comment 37 Ted Mielczarek [:ted.mielczarek] 2012-06-08 05:45:52 PDT
That looks like valid Python to me:
>>> 0o666 == 0666
True
Comment 38 Randell Jesup [:jesup] 2012-06-08 06:19:13 PDT
(In reply to Ted Mielczarek [:ted] from comment #37)
> That looks like valid Python to me:
> >>> 0o666 == 0666
> True

All I know is that I was getting errors building for Mac before I made that change (some time ago).  Also: is it something that might be python version dependent?  (I'm no python expert!)

I can remove it and see what happens...
Comment 39 Ted Mielczarek [:ted.mielczarek] 2012-06-08 06:20:33 PDT
Ah, you're right, that's Python 2.6+.
Comment 40 Gervase Markham [:gerv] 2012-06-09 09:41:09 PDT
No problem with mozmake.py being under the same licence as the rest of gyp; but please put a header on it. If a copyright line is part of the header, the copyright holder is the Mozilla Foundation.

Gerv
Comment 41 Ted Mielczarek [:ted.mielczarek] 2012-06-11 10:29:37 PDT
Gerv: with all other Google projects, we assign copyright to Google. (We have a corporate contributor licensing agreement on file with them.) I don't know if it's that important in this case, since I have no immediate plans to upstream this code.
Comment 42 Mike Hommey [:glandium] 2012-06-11 10:48:52 PDT
(In reply to Ted Mielczarek [:ted] from comment #41)
> Gerv: with all other Google projects, we assign copyright to Google. (We
> have a corporate contributor licensing agreement on file with them.) I don't
> know if it's that important in this case, since I have no immediate plans to
> upstream this code.

I don't think we are actually assigning copyright to Google. We grant them a special license, but the copyright is still ours.
Comment 43 Ted Mielczarek [:ted.mielczarek] 2012-06-11 11:16:09 PDT
You may be right, but IANAL:
http://code.google.com/legal/corporate-cla-v1.0.html
"Subject to the terms and conditions of this Agreement, You hereby grant to Google and to recipients of software distributed by Google a perpetual, worldwide, non-exclusive, no-charge, royalty-free, irrevocable copyright license to reproduce, prepare derivative works of, publicly display, publicly perform, sublicense, and distribute Your Contributions and such derivative works."
Comment 44 Gervase Markham [:gerv] 2012-06-12 01:18:30 PDT
A grant of rights is not the same as a copyright assignment, although often the difference is pretty much only this one: whose name appears on the copyright statement!

Please use MoFo as copyright holder if there's a spot in the boilerplate for it.

Gerv
Comment 45 Randell Jesup [:jesup] 2012-06-15 12:08:29 PDT
Created attachment 633620 [details] [diff] [review]
updates against previous patch per review
Comment 46 Randell Jesup [:jesup] 2012-06-15 12:11:22 PDT
Comment on attachment 633620 [details] [diff] [review]
updates against previous patch per review

Replace patch with one with a minor update to the mozmake.py copyright header per Gerv

Already has r+ from derf on the VP8 bits, moving r? for kyle forward.
Comment 47 Randell Jesup [:jesup] 2012-06-15 12:13:11 PDT
(In reply to Randell Jesup [:jesup] from comment #46)
> Comment on attachment 633620 [details] [diff] [review]
> updates against previous patch per review
> 
> Replace patch with one with a minor update to the mozmake.py copyright
> header per Gerv

Re-read gerv's comment, he said MoFo not MoCo; I'll revise before checkin (if it matters).
Comment 48 Gervase Markham [:gerv] 2012-06-18 02:58:55 PDT
Yes, please say MoFo rather than MoCo.

Gerv
Comment 50 Randell Jesup [:jesup] 2012-06-21 06:52:57 PDT
Relanding:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=2ad9ba0e0ccb

Note You need to log in before you can comment on or make changes to this bug.