517422 - Allow to build against system libogg*, libvorbis, etc.

Status: NEW

(Firefox Build System :: General, enhancement, P5)

Description

[Mike Hommey [:glandium]]

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]]

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]]

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]]

(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]]

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)]

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]

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)]

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]

(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)]

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.]

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]

Attachment: 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]]

(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]

Attachment: 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]

Oops, forgot to salvage README_MOZILLA hunk from attachment 8382396 [details] [diff] [review].

Comment 15

[Jan Beich]

Attachment: v0 (ogg/vorbis/opus)

Comment 16

[Jan Beich]

Attachment: 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]

Attachment: v1.1 (ogg/vorbis/opus/tremor/theora/speex/soundtouch)

a few minor fixups

Comment 18

[Jan Beich]

Attachment: v1.2 (ogg/vorbis/opus/tremor/theora/speex/soundtouch)

replaced moved includes in favor of MOZ_IMPORT_API

Comment 19

[Jan Beich]

Attachment: 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]

Attachment: v1.3 (ogg/vorbis/opus/tremor/theora/speex/soundtouch)

rebased, https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5a9b1f3f18c6

Comment 21

[Jan Beich]

Attachment: 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]

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]

(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]

Attachment: 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]

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]

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/

Comment 27

[Mitchell Hentges [:mhentges] 🦀]

The patch is obsolete, so I've removed the review request.
I don't believe it's possible to use system opus or vorbis yet, so I'll leave this open.