Last Comment Bug 517422 - Allow to build against system libogg*, libvorbis, etc.
: Allow to build against system libogg*, libvorbis, etc.
Status: NEW
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- enhancement with 1 vote (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-09-18 00:58 PDT by Mike Hommey [:glandium]
Modified: 2016-05-12 11:10 PDT (History)
21 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

MozReview Requests
Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:
Show discarded requests

Attachments
v0 (ogg/vorbis/opus) (11.60 KB, patch)
2014-02-26 10:56 PST, Jan Beich
no flags Details | Diff | Splinter Review
v0 (ogg/vorbis/opus) (11.31 KB, patch)
2014-03-20 00:41 PDT, Jan Beich
no flags Details | Diff | Splinter Review
v0 (ogg/vorbis/opus) (13.29 KB, patch)
2014-03-20 01:34 PDT, Jan Beich
no flags Details | Diff | Splinter Review
v1 (ogg/vorbis/opus/tremor/theora/speex/soundtouch) (20.66 KB, patch)
2014-04-09 23:22 PDT, Jan Beich
no flags Details | Diff | Splinter Review
v1.1 (ogg/vorbis/opus/tremor/theora/speex/soundtouch) (21.44 KB, patch)
2014-04-10 15:29 PDT, Jan Beich
no flags Details | Diff | Splinter Review
v1.2 (ogg/vorbis/opus/tremor/theora/speex/soundtouch) (20.94 KB, patch)
2014-04-25 12:13 PDT, Jan Beich
no flags Details | Diff | Splinter Review
v1.3 (ogg/vorbis/opus/tremor/theora/speex/soundtouch) (22.05 KB, patch)
2014-05-19 19:10 PDT, Jan Beich
no flags Details | Diff | Splinter Review
v1.3 (ogg/vorbis/opus/tremor/theora/speex/soundtouch) (22.33 KB, patch)
2014-08-01 04:47 PDT, Jan Beich
no flags Details | Diff | Splinter Review
v1.3 (ogg/vorbis/opus/tremor/theora/speex/soundtouch) (22.63 KB, patch)
2014-08-06 10:37 PDT, Jan Beich
no flags Details | Diff | Splinter Review
MozReview Request: Bug 517422 - Allow more media/ libs built against their system-wide versions. r?glandium (58 bytes, text/x-review-board-request)
2016-03-13 05:45 PDT, Jan Beich
jbeich: review? (mh+mozilla)
Details | Review

Description Mike Hommey [:glandium] 2009-09-18 00:58:57 PDT
All libraries in /media are also available at system level on linux systems, to name only them, and it would be preferrable for linux distributions to be able to link against them.

As I'm not willing to put time into a clean implementation if it's not going to be considered to be applied, could you tell me if you'd apply a patch doing so ?
Comment 1 Reed Loden [:reed] (use needinfo?) 2009-09-18 07:23:48 PDT
Considering we're cherry-picking security patches lately for these media libs, I think this is probably a bad idea for now until we have more stable libs. Stuff is just in too large of a flux right now to trust system libs that might not be patched against vulnerabilities.
Comment 2 Mike Hommey [:glandium] 2009-09-18 07:28:25 PDT
The point is not to make it mandatory, but make it *possible*. Like sqlite, png, jpeg, bzip2, nss and nspr.
Comment 3 Reed Loden [:reed] (use needinfo?) 2009-09-18 07:33:17 PDT
(In reply to comment #2)
> The point is not to make it mandatory, but make it *possible*. Like sqlite,
> png, jpeg, bzip2, nss and nspr.

Except most of those libs have version numbers we can use as minimum requirements... we don't have the same thing currently for the media libs due to our cherry-picking (which I hope will eventually stabilize as the libs themselves improve over time). Once the libs are in a more stable form to the point where we aren't just cherry-picking, then I think this is fine to do. Until then, not so much, and I don't think we should even offer the option until then.

Does that make sense?
Comment 4 Mike Hommey [:glandium] 2009-09-18 07:36:13 PDT
It makes it clear that I won't have to go through hoops to do it in Debian, then. Hacking Makefile.ins will make my day.
Comment 5 cajbir (:cajbir) 2009-09-18 07:41:26 PDT
The other problem is we have custom patches to work around issues that haven't been, and in some cases won't be, applied upstream. Until these are resolved it's actually not possible to use the system libraries. This mainly affects liboggz/liboggplay.
Comment 6 Hicham 2010-09-29 08:45:50 PDT
I hope that you will merge gstreamer patches so that this media handling issue can be over ( OFF TOPIC ).

Seriously, isn't bundling libraries creating more issues that it solves ? in case of security vulnerabilities, the amount of work to be done by the distribution would be huge compared to fixing only the system's copy.

Please reconsider this decision and don't let Firefox become another Chromiumish product bundling everything.
Comment 7 cajbir (:cajbir) 2010-09-29 16:55:57 PDT
We had to bundle the Ogg libraries as we had custom patches running against bleeding edge SVN revisions to get things to work. Now that we are implemented on top of libogg, libvorbis and libtheora directly it would be easier to use system libraries. I'm not sure if we have any custom patches against those and if not I can't think of any issue with providing an option to link against those.
Comment 8 Takanori MATSUURA 2011-09-24 00:08:38 PDT
(In reply to Chris Double (:doublec) from comment #7)

Have your modification been sent and applied to upstream?

Your modification for libogg and libvorbis is only for OpenSolaris and don't need for other OS. So it would be good to allow to build against system libogg, libtheora, and libvorbis for all OS and block it for OpenSolaris in configure script.
Comment 9 Ralph Giles (:rillian) needinfo me 2011-09-26 15:31:04 PDT
The Solaris modifications were only necessary to build the libraries inside our tree. Those libraries should build from upstream source on OpenSolaris.
Comment 10 Antonio T. 2014-02-10 08:25:34 PST
Hi all.

(In reply to Reed Loden [:reed] from comment #1)
> Considering we're cherry-picking security patches lately for these media
> libs, I think this is probably a bad idea for now until we have more stable
> libs. Stuff is just in too large of a flux right now to trust system libs
> that might not be patched against vulnerabilities.

Does exist now any chance to use system libraries in Firefox ?
Comment 11 Jan Beich 2014-02-26 10:56:21 PST
Created attachment 8382396 [details] [diff] [review]
v0 (ogg/vorbis/opus)

I'll probably add these later:

- libtheora patches don't seem to be in any of releases
- libsoundtouch needs a check against bug 847827 comment 16

Do you have more libs in mind?
Comment 12 Nathan Froyd [:froydnj] 2014-02-26 10:59:04 PST
(In reply to Jan Beich from comment #11)
> Created attachment 8382396 [details] [diff] [review]
> v0 (ogg/vorbis/opus)

This is going to conflict with the patches in bug 677653 for tracking memory allocated from libogg &co.  (Not necessarily a problem, just something to be worked out.)
Comment 13 Jan Beich 2014-03-20 00:41:30 PDT
Created attachment 8393990 [details] [diff] [review]
v0 (ogg/vorbis/opus)

Rebased after bug 677653. Apply after bug 847568 and bug 983953 or fix merge conflicts yourself. esr24 versions of this one and bug 847568 are already in FreeBSD ports.

http://svnweb.freebsd.org/ports/head/www/firefox-esr/files/patch-zz-bug517422
http://svnweb.freebsd.org/ports/head/www/firefox-esr/files/patch-z-bug847568

(In reply to Nathan Froyd (:froydnj) from comment #12)
> (In reply to Jan Beich from comment #11)
> > Created attachment 8382396 [details] [diff] [review]
> > v0 (ogg/vorbis/opus)
> 
> This is going to conflict with the patches in bug 677653 for tracking memory
> allocated from libogg &co.  (Not necessarily a problem, just something to be
> worked out.)

ifdef out like bug 983953 until upstream exposes necessary symbols/headers. Their memory would likely end up in heap-unclassified while libogg &co report zero.
Comment 14 Jan Beich 2014-03-20 01:22:25 PDT
Oops, forgot to salvage README_MOZILLA hunk from attachment 8382396 [details] [diff] [review].
Comment 15 Jan Beich 2014-03-20 01:34:41 PDT
Created attachment 8394000 [details] [diff] [review]
v0 (ogg/vorbis/opus)
Comment 16 Jan Beich 2014-04-09 23:22:35 PDT
Created attachment 8404461 [details] [diff] [review]
v1 (ogg/vorbis/opus/tremor/theora/speex/soundtouch)

More libs added. SoundTouch required moving its include to avoid gcc_hidden.h-induced error:

content/media/Unified_cpp_content_media0.cpp:(.text._ZN7mozilla11AudioStream38EnsureTimeStretcherInitializedUnlockedEv+0x3d): undefined reference to `soundtouch::SoundTouch::SoundTouch()'
Comment 17 Jan Beich 2014-04-10 15:29:06 PDT
Created attachment 8405024 [details] [diff] [review]
v1.1 (ogg/vorbis/opus/tremor/theora/speex/soundtouch)

a few minor fixups
Comment 18 Jan Beich 2014-04-25 12:13:14 PDT
Created attachment 8412833 [details] [diff] [review]
v1.2 (ogg/vorbis/opus/tremor/theora/speex/soundtouch)

replaced moved includes in favor of MOZ_IMPORT_API
Comment 19 Jan Beich 2014-05-19 19:10:09 PDT
Created attachment 8425179 [details] [diff] [review]
v1.3 (ogg/vorbis/opus/tremor/theora/speex/soundtouch)

Abort early, during configure, if system soundtouch is built with --enable-integer-samples on Desktop. A few users stumbled upon

In file included from content/media/Unified_cpp_content_media0.cpp:80:
content/media/AudioStream.cpp:832:37: error:
      no matching member function for call to 'receiveSamples'
    flushedFrames = mTimeStretcher->receiveSamples(reinterpret_cast<AudioDataValue*>(wpos), aFrames);
                    ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~
/usr/local/include/soundtouch/FIFOSamplePipe.h:190:18: note: candidate function not viable: no known
      conversion from 'AudioDataValue *' (aka 'float *') to 'SAMPLETYPE *' (aka 'short *') for 1st
      argument
    virtual uint receiveSamples(SAMPLETYPE *outBuffer, ///< Buffer where to copy output samples.
                 ^
/usr/local/include/soundtouch/FIFOSamplePipe.h:203:18: note: candidate function not viable: requires
      single argument 'maxSamples', but 2 arguments were provided
    virtual uint receiveSamples(uint maxSamples   ///< Remove this many samples from the beginnin...
                 ^
In file included from content/media/Unified_cpp_content_media0.cpp:80:
content/media/AudioStream.cpp:906:36: error:
      cannot initialize a parameter of type 'const SAMPLETYPE *' (aka 'const short *') with an rvalue of
      type 'AudioDataValue *' (aka 'float *')
        mTimeStretcher->putSamples(reinterpret_cast<AudioDataValue*>(input[i]), BytesToFrames(inpu...
                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/local/include/soundtouch/SoundTouch.h:237:31: note: passing argument to parameter 'samples' here
            const SAMPLETYPE *samples,  ///< Pointer to sample buffer.
                              ^
In file included from content/media/Unified_cpp_content_media0.cpp:80:
content/media/AudioStream.cpp:909:47: error:
      no matching member function for call to 'receiveSamples'
    uint32_t receivedFrames = mTimeStretcher->receiveSamples(reinterpret_cast<AudioDataValue*>(wpo...
                              ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~
/usr/local/include/soundtouch/FIFOSamplePipe.h:190:18: note: candidate function not viable: no known
      conversion from 'AudioDataValue *' (aka 'float *') to 'SAMPLETYPE *' (aka 'short *') for 1st
      argument
    virtual uint receiveSamples(SAMPLETYPE *outBuffer, ///< Buffer where to copy output samples.
                 ^
/usr/local/include/soundtouch/FIFOSamplePipe.h:203:18: note: candidate function not viable: requires
      single argument 'maxSamples', but 2 arguments were provided
    virtual uint receiveSamples(uint maxSamples   ///< Remove this many samples from the beginnin...
                 ^
3 errors generated.
Comment 20 Jan Beich 2014-08-01 04:47:55 PDT
Created attachment 8466141 [details] [diff] [review]
v1.3 (ogg/vorbis/opus/tremor/theora/speex/soundtouch)

rebased, https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5a9b1f3f18c6
Comment 21 Jan Beich 2014-08-06 10:37:44 PDT
Created attachment 8468593 [details] [diff] [review]
v1.3 (ogg/vorbis/opus/tremor/theora/speex/soundtouch)

rebased after bug 1045783, https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c2ec4ee42985
Comment 22 Hussam Al-Tayeb 2016-02-20 09:46:30 PST
I found an updated patch for this https://svnweb.freebsd.org/ports/head/www/firefox/files/patch-z-bug517422?view=co but it won't apply to latest trunk.
Comment 23 Jan Beich 2016-03-13 05:45:26 PDT
(In reply to Hussam Al-Tayeb from comment #22)
> I found an updated patch for this
> https://svnweb.freebsd.org/ports/head/www/firefox/files/patch-z-
> bug517422?view=co but it won't apply to latest trunk.

Per comment 13 this bug should be applied after patch-bug847568. However, in ports there's one more accidental dependency - patch-bug1021761. I'm going te re-upload newer version via mozreview while:
- dropping --with-system-opus as it's broken by WebRTC updates
- dropping --with-system-speex because it's only used on Android and WINNT
Comment 24 Jan Beich 2016-03-13 05:45:55 PDT
Created attachment 8729961 [details]
MozReview Request: Bug 517422 - Allow more media/ libs built against their system-wide versions. r?glandium

Review commit: https://reviewboard.mozilla.org/r/39625/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39625/
Comment 25 Jan Beich 2016-03-25 03:32:57 PDT
https://reviewboard.mozilla.org/r/39625/#review38927

::: config/system-headers:1261
(Diff revision 1)
>  X11/Xlibint.h
>  X11/Xlocale.h
>  X11/Xos.h
>  X11/Xutil.h
>  zmouse.h
> +#if MOZ_SYSTEM_SOUNDTOUCH==1

This breaks bundled build i.e., when --with-system-soundtouch is not specified.

../../dom/media/Unified_cpp_dom_media0.o: In function `mozilla::AudioStream::~AudioStream()':
/obj/dom/media/Unified_cpp_dom_media0.cpp:(.text+0x44f0): undefined reference to `soundtouch::destroySoundTouchObj(soundtouch::SoundTouch*)'
/usr/bin/ld: ../../dom/media/Unified_cpp_dom_media0.o: relocation R_X86_64_PC32 against `_ZN10soundtouch20destroySoundTouchObjEPNS_10SoundTouchE' can not be used when making a shared object; recompile with -fPIC
/usr/bin/ld: final link failed: Bad value
Comment 26 Jan Beich 2016-03-25 03:36:28 PDT
Comment on attachment 8729961 [details]
MozReview Request: Bug 517422 - Allow more media/ libs built against their system-wide versions. r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39625/diff/1-2/

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