Closed Bug 709721 Opened 12 years ago Closed 12 years ago

Slice out media libraries from libxul

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

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.
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 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).
(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+
Given the risk of symbol conflicts on ELF systems, I think we should try to make this Windows-only.
(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.
(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)
Attachment #580897 - Attachment is obsolete: true
Blocks: 709914
Apparently, this breaks some video reftests on windows :(
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.
(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.
(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.
See bug 709914 comment 16 onwards for a fix to the test failures here...
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
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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 → ---
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.
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+
Moved symbols.def generation rule under the #ifdef
Attachment #582064 - Flags: review?(khuey)
Attachment #582063 - Attachment is obsolete: true
Attachment #582064 - Attachment is obsolete: true
Attachment #582064 - Flags: review?(khuey)
https://hg.mozilla.org/mozilla-central/rev/30f9367dfa68
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
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.
Nope, that's the fault of bug 689384. File a new bug about fixing that and block that one, please?
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 #.
Whoops, bug 689834.
Depends on: 718485
Blocks: 750661
No longer blocks: 750661
You need to log in before you can comment on or make changes to this bug.