Last Comment Bug 709721 - Slice out media libraries from libxul
: Slice out media libraries from libxul
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Mike Hommey [:glandium]
:
Mentors:
Depends on: 718485
Blocks: 709193 709914
  Show dependency treegraph
 
Reported: 2011-12-12 00:41 PST by Mike Hommey [:glandium]
Modified: 2012-05-01 04:26 PDT (History)
19 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Move video and audio libraries in a gkmedias library on Windows (14.71 KB, patch)
2011-12-12 06:45 PST, Mike Hommey [:glandium]
khuey: review+
Details | Diff | Splinter Review
Move video and audio libraries in a gkmedias library on Windows (14.07 KB, patch)
2011-12-12 11:55 PST, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
part 3 - Only try to export gkmedias symbols that are defined wrt configuration (3.94 KB, patch)
2011-12-15 12:09 PST, Mike Hommey [:glandium]
khuey: review+
Details | Diff | Splinter Review
part 3 - Only try to export gkmedias symbols that are defined wrt configuration (3.94 KB, patch)
2011-12-15 12:17 PST, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
part 3 - Only try to export gkmedias symbols that are defined wrt configuration (4.19 KB, patch)
2011-12-15 13:51 PST, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review

Description Mike Hommey [:glandium] 2011-12-12 00:41:49 PST
In order to make libxul lighter and help move out of bug 709193, we could separate out the media libraries (vpx, tremor, vorbis, etc.) in a separate library.
Comment 1 Mike Hommey [:glandium] 2011-12-12 06:45:19 PST
Created attachment 580897 [details] [diff] [review]
Move video and audio libraries in a gkmedias library on Windows

I successfully built locally with this patch (without PGO). I don't think we need to generalize to all platforms. The gkmedias library is about 300KB.

Trying to build the changeset that was all red, with this patch, and nothing else (that is, no backout of spdy, etc.):
https://tbpl.mozilla.org/?noignore=1&tree=Try&rev=af4700fc9c59
Comment 2 Timothy B. Terriberry (:derf) 2011-12-12 06:55:31 PST
Comment on attachment 580897 [details] [diff] [review]
Move video and audio libraries in a gkmedias library on Windows

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

::: layout/media/symbols
@@ +68,5 @@
> +vorbis_synthesis_init
> +vorbis_synthesis_pcmout
> +vorbis_synthesis_read
> +vorbis_synthesis_restart
> +vpx_codec_dec_init_ver

This list of symbols doesn't cover the MOZ_VP8_ERROR_CONCEALMENT and MOZ_VP8_ENCODER configure flags (which we need for WebRTC).
Comment 3 Mike Hommey [:glandium] 2011-12-12 07:11:57 PST
(In reply to Timothy B. Terriberry (:derf) from comment #2)
> This list of symbols doesn't cover the MOZ_VP8_ERROR_CONCEALMENT and
> MOZ_VP8_ENCODER configure flags (which we need for WebRTC).

Well, this list of symbols purposefully doesn't export symbols we don't use at the moment, so as to eliminate parts of the libraries that are effectively unused. WebRTC is not in the tree at the moment, so I don't know what symbols are going to be used by it, but you can surely add these symbols to the list when webrtc lands or on the webrtc branch.
Comment 4 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-12-12 10:09:27 PST
Comment on attachment 580897 [details] [diff] [review]
Move video and audio libraries in a gkmedias library on Windows

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

r=me

::: layout/media/Makefile.in
@@ +93,5 @@
> +
> +syms.def: symbols Makefile
> +	echo 'LIBRARY $(DLL_PREFIX)gkmedias$(DLL_SUFFIX)' > $@
> +	echo 'EXPORTS' >> $@
> +	cat $< >> $@

Is there a reason we need to do this at build time?

::: media/libtremor/lib/Makefile.in
@@ +44,5 @@
>  MODULE  = tremor
>  LIBRARY_NAME = tremor
>  FORCE_STATIC_LIB= 1
>  LOCAL_INCLUDES += -I$(topsrcdir)/media/libtremor/include/tremor
> +VISIBILITY_FLAGS =

WINNT only?
Comment 5 Benjamin Smedberg [:bsmedberg] 2011-12-12 10:15:54 PST
Given the risk of symbol conflicts on ELF systems, I think we should try to make this Windows-only.
Comment 6 Mike Hommey [:glandium] 2011-12-12 10:18:38 PST
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #5)
> Given the risk of symbol conflicts on ELF systems, I think we should try to
> make this Windows-only.

It is windows only.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #4)
> Comment on attachment 580897 [details] [diff] [review]
> Move video and audio libraries in a gkmedias library on Windows
> 
> Review of attachment 580897 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me
> 
> ::: layout/media/Makefile.in
> @@ +93,5 @@
> > +
> > +syms.def: symbols Makefile
> > +	echo 'LIBRARY $(DLL_PREFIX)gkmedias$(DLL_SUFFIX)' > $@
> > +	echo 'EXPORTS' >> $@
> > +	cat $< >> $@
> 
> Is there a reason we need to do this at build time?

Not anymore. I initially did it on linux, which requires a different format.

> ::: media/libtremor/lib/Makefile.in
> @@ +44,5 @@
> >  MODULE  = tremor
> >  LIBRARY_NAME = tremor
> >  FORCE_STATIC_LIB= 1
> >  LOCAL_INCLUDES += -I$(topsrcdir)/media/libtremor/include/tremor
> > +VISIBILITY_FLAGS =
> 
> WINNT only?

Yep, overlooked that one.
Comment 7 Mike Hommey [:glandium] 2011-12-12 10:24:42 PST
(In reply to Mike Hommey [:glandium] from comment #6)
> > ::: media/libtremor/lib/Makefile.in
> > @@ +44,5 @@
> > >  MODULE  = tremor
> > >  LIBRARY_NAME = tremor
> > >  FORCE_STATIC_LIB= 1
> > >  LOCAL_INCLUDES += -I$(topsrcdir)/media/libtremor/include/tremor
> > > +VISIBILITY_FLAGS =
> > 
> > WINNT only?
> 
> Yep, overlooked that one.

It was meant not to be patched (tremor is for arm)
Comment 8 Mike Hommey [:glandium] 2011-12-12 11:55:09 PST
Created attachment 580995 [details] [diff] [review]
Move video and audio libraries in a gkmedias library on Windows

Refreshed
Comment 9 Mike Hommey [:glandium] 2011-12-13 04:43:26 PST
Apparently, this breaks some video reftests on windows :(
Comment 10 Chris Pearce (:cpearce) 2011-12-13 12:45:03 PST
Can you post a link to you Try push? We've got a number of random orange in the media tests, you could just be hitting them...
Comment 11 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-12-13 12:46:49 PST
Fwiw, given that splitting out uconv failed to help at all (and that's 600+KB of code) idk that this is worth pursuing.
Comment 12 Mike Hommey [:glandium] 2011-12-13 14:17:56 PST
(In reply to Chris Pearce (:cpearce) (Mozilla Corporation) from comment #10)
> Can you post a link to you Try push? We've got a number of random orange in
> the media tests, you could just be hitting them...

https://tbpl.mozilla.org/?noignore=1&tree=Try&rev=683e92eb1df0

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #11)
> Fwiw, given that splitting out uconv failed to help at all (and that's
> 600+KB of code) idk that this is worth pursuing.

The first try I did with the patch from this bug was with SPDY and graphite in and enabled, with PGO, and succeeded 6 times out of 6.
Comment 13 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-12-13 16:06:01 PST
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #11)
> Fwiw, given that splitting out uconv failed to help at all (and that's
> 600+KB of code) idk that this is worth pursuing.

Isn't uconv almost entirely data tables though? If so, then I bet it only reduced the memory required by the compiler by about 1MB, because the compiler probably requires O(n) memory for copying data into the resulting binary, but it probably requires O(n^2) or more for optimizing code.
Comment 14 :Ehsan Akhgari 2011-12-14 15:52:01 PST
See bug 709914 comment 16 onwards for a fix to the test failures here...
Comment 15 Mike Hommey [:glandium] 2011-12-14 22:15:41 PST
http://hg.mozilla.org/mozilla-central/rev/9123d35faa6c
http://hg.mozilla.org/mozilla-central/rev/9a591b290431

Thanks Ehsan for that last part.
Comment 16 Ed Morley [:emorley] 2011-12-15 06:05:40 PST
This appears to have broken win mobile nightlies:
https://tbpl.mozilla.org/php/getParsedLog.php?id=7951089&tree=Mozilla-Inbound

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=3f569b3fbc22
and
https://tbpl.mozilla.org/?rev=beac16509534

(Haven't backed out for now, since it's only the win mobile builds)
Comment 17 Mike Hommey [:glandium] 2011-12-15 06:33:43 PST
Looks like the win mobile builds don't have nestegg and vpx enabled. Is that expected? Sure, we should have some ifdefs anyways, but that sounds strange.
Comment 18 Mike Hommey [:glandium] 2011-12-15 12:09:46 PST
Created attachment 582063 [details] [diff] [review]
part 3 - Only try to export gkmedias symbols that are defined wrt configuration

This should solve the windows mobile issues.
Comment 19 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-12-15 12:12:07 PST
Comment on attachment 582063 [details] [diff] [review]
part 3 - Only try to export gkmedias symbols that are defined wrt configuration

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

/me mumbles something about not caring about windows mobile
Comment 20 Mike Hommey [:glandium] 2011-12-15 12:17:27 PST
Created attachment 582064 [details] [diff] [review]
part 3 - Only try to export gkmedias symbols that are defined wrt configuration

Moved symbols.def generation rule under the #ifdef
Comment 21 Mike Hommey [:glandium] 2011-12-15 13:51:49 PST
Created attachment 582100 [details] [diff] [review]
part 3 - Only try to export gkmedias symbols that are defined wrt configuration

With a little fixup on the AC_DEFINE
Comment 23 Matt Brubeck (:mbrubeck) 2011-12-17 09:04:42 PST
https://hg.mozilla.org/mozilla-central/rev/30f9367dfa68
Comment 24 :aceman 2011-12-18 07:39:30 PST
Could this have broken --disable-ogg and --disable-wave configure options?
I get errors like this:
(00:19:36) aceman: /usr/local/TB-hg/mozilla/content/xml/document/src/nsXMLContentSink.cpp: In member function 'nsresult nsXMLContentSink::HandleStartElement(const PRUnichar*, const PRUnichar**, PRUint32, PRInt
(00:19:36) aceman: /usr/local/TB-hg/mozilla/content/xml/document/src/nsXMLContentSink.cpp:1081:33: error: 'audio' is not a member of 'nsGkAtoms'
(00:19:36) aceman: /usr/local/TB-hg/mozilla/content/xml/document/src/nsXMLContentSink.cpp:1082:33: error: 'video' is not a member of 'nsGkAtoms'
(00:19:36) aceman: gmake[8]: *** [nsXMLContentSink.o] Error 1

Compile runs fine when I do not use those options.
Comment 25 Josh Matthews [:jdm] 2011-12-18 07:49:32 PST
Nope, that's the fault of bug 689384. File a new bug about fixing that and block that one, please?
Comment 26 :aceman 2011-12-18 08:24:02 PST
Thanks. But that bug is secret, that is probably why I didn't find it.
I have filed the new bug, but I can't add the dependency.
Comment 27 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-12-18 08:31:40 PST
(In reply to Josh Matthews [:jdm] from comment #25)
> Nope, that's the fault of bug 689384. File a new bug about fixing that and
> block that one, please?

I think that's the wrong bug #.
Comment 28 Josh Matthews [:jdm] 2011-12-18 08:32:26 PST
Whoops, bug 689834.

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