Closed
Bug 709721
Opened 12 years ago
Closed 12 years ago
Slice out media libraries from libxul
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla11
People
(Reporter: glandium, Assigned: glandium)
References
Details
Attachments
(2 files, 3 obsolete files)
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.
Assignee | ||
Comment 1•12 years ago
|
||
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
Attachment #580897 -
Flags: review?(khuey)
Comment 2•12 years ago
|
||
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).
Assignee | ||
Comment 3•12 years ago
|
||
(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 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?
Attachment #580897 -
Flags: review?(khuey) → review+
Comment 5•12 years ago
|
||
Given the risk of symbol conflicts on ELF systems, I think we should try to make this Windows-only.
Assignee | ||
Comment 6•12 years ago
|
||
(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.
Assignee | ||
Comment 7•12 years ago
|
||
(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)
Assignee | ||
Comment 8•12 years ago
|
||
Refreshed
Assignee | ||
Updated•12 years ago
|
Attachment #580897 -
Attachment is obsolete: true
Assignee | ||
Comment 9•12 years ago
|
||
Apparently, this breaks some video reftests on windows :(
Comment 10•12 years ago
|
||
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...
Fwiw, given that splitting out uconv failed to help at all (and that's 600+KB of code) idk that this is worth pursuing.
Assignee | ||
Comment 12•12 years ago
|
||
(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•12 years ago
|
||
(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•12 years ago
|
||
See bug 709914 comment 16 onwards for a fix to the test failures here...
Assignee | ||
Comment 15•12 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/9123d35faa6c http://hg.mozilla.org/mozilla-central/rev/9a591b290431 Thanks Ehsan for that last part.
Target Milestone: --- → mozilla11
Assignee | ||
Updated•12 years ago
|
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 16•12 years ago
|
||
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)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 17•12 years ago
|
||
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.
Assignee | ||
Comment 18•12 years ago
|
||
This should solve the windows mobile issues.
Attachment #582063 -
Flags: review?(khuey)
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
Attachment #582063 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 20•12 years ago
|
||
Moved symbols.def generation rule under the #ifdef
Attachment #582064 -
Flags: review?(khuey)
Assignee | ||
Comment 21•12 years ago
|
||
With a little fixup on the AC_DEFINE
Assignee | ||
Updated•12 years ago
|
Attachment #582063 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #582064 -
Attachment is obsolete: true
Attachment #582064 -
Flags: review?(khuey)
Assignee | ||
Comment 22•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/30f9367dfa68
Whiteboard: [inbound]
Comment 23•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/30f9367dfa68
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Comment 24•12 years ago
|
||
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•12 years ago
|
||
Nope, that's the fault of bug 689384. File a new bug about fixing that and block that one, please?
Comment 26•12 years ago
|
||
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.
(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•12 years ago
|
||
Whoops, bug 689834.
You need to log in
before you can comment on or make changes to this bug.
Description
•