1802513 - MP3s are decoded using the system's ffmpeg on desktop Linux, instead of the vendored ffmpeg

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect)

Description

[Paul Adenot (:padenot)]

Found in https://bugzilla.mozilla.org/show_bug.cgi?id=1796532#c13.

I verified that, e.g. on macOS, the mp3 decoder vendored in Firefox is used (instead of e.g. the OS's decoder, that we also have code for).

Comment 1

[Paul Adenot (:padenot)]

It tooks a while to get to the bottom of this, but it's the utility process sandbox that prevents the runtime linker for our ffvpx library to work.

It's failing here: https://searchfox.org/mozilla-central/source/dom/media/platforms/ffmpeg/ffvpx/FFVPXRuntimeLinker.cpp#67, attempting to read /proc/self/exe to get the current binary path.

Alex, do you mind having a look at this?

Comment 2

[Paul Adenot (:padenot)]

When running Firefox with MOZ_DISABLE_UTILITY_SANDBOX set to 1, ffvpx is used, otherwise, the ffmpeg libraries from the system are used.

An easy way to check this is to profile Firefox with the Firefox Profiler and check where the actual mp3 decoding function are coming from, mozavcodec.so if ffvpx is used, some other path if the system's library are in use.

Comment 3

[:gerard-majax]

How much urgent is this?

Comment 4

[:gerard-majax]

Jed, what is your take on allowing readlink in Utility sandbox? That seems to be enough. Maybe we should limit it to allowing /proc/self/exe ?

Comment 5

[:gerard-majax]

GetDescriptionName() should probably reflect audio decoder being ffmpeg/ffvpx like video does.

Comment 6

[:gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #5)

GetDescriptionName() should probably reflect audio decoder being ffmpeg/ffvpx like video does.

I dont really get how it works, but https://searchfox.org/mozilla-central/rev/d2fa2b9615dd8460b079804d0075180f689d2ba6/dom/media/platforms/ffmpeg/ffvpx/moz.build#38 doing the same on audio and playing with sandbox rules shows we have either ffmpeg or ffvpx, so we can test that as well

Comment 7

[:gerard-majax]

Attachment: Bug 1802513 - Expose ffmpeg/ffvpx audio r?alwu!

Comment 8

[:gerard-majax]

Attachment: Bug 1802513 - Allow readlink(/proc/self/exe) in Utility sandbox for FFVPX r?jld!

Depends on D163226

Comment 9

[:gerard-majax]

Attachment: Bug 1802513 - Augment utility audio tests with ffmpeg/ffvpx r?alwu!

Depends on D163227

Comment 10

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Alexandre LISSY :gerard-majax from comment #4)

Jed, what is your take on allowing readlink in Utility sandbox? That seems to be enough. Maybe we should limit it to allowing /proc/self/exe ?

readlink is already allowed (partially) via the file broker, so it should work to just add a rule for reading /proc/self/exe to the broker policy. (That will also allow opening and reading the executable file, but of course most of it is already mapped into the process.)

Comment 11

[:gerard-majax]

Comment on attachment 9305601 [details]
Bug 1802513 - Allow readlink(/proc/self/exe) in Utility sandbox for FFVPX r?jld!

Beta/Release Uplift Approval Request

• User impact if declined: Wrong version of ffmpeg used
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Actual fix is a one-line change to allow readlink(/proc/self/exe), and it comes with improved testing. It is easy to backout in case of issues.
• String changes made/needed: N/A
• Is Android affected?: No

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/177018bf4443
Expose ffmpeg/ffvpx audio r=alwu
https://hg.mozilla.org/integration/autoland/rev/c068d136d7a3
Allow readlink(/proc/self/exe) in Utility sandbox for FFVPX r=gcp
https://hg.mozilla.org/integration/autoland/rev/e6598d87dcf2
Augment utility audio tests with ffmpeg/ffvpx r=alwu

Comment 13

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/177018bf4443
https://hg.mozilla.org/mozilla-central/rev/c068d136d7a3
https://hg.mozilla.org/mozilla-central/rev/e6598d87dcf2

Comment 14

[Dianna Smith [:diannaS]]

Comment on attachment 9305600 [details]
Bug 1802513 - Expose ffmpeg/ffvpx audio r?alwu!

Approved for 108.0b9

Comment 15

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9e1547f9f535
https://hg.mozilla.org/releases/mozilla-beta/rev/d4006d5d789c
https://hg.mozilla.org/releases/mozilla-beta/rev/f77dc3c04876