Closed Bug 1360060 Opened 2 years ago Closed 2 years ago

Enable use of rust version of cubeb PulseAudio backend.

Categories

(Core :: Audio/Video: cubeb, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: kamidphish, Assigned: kamidphish)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Now that we have ported the PulseAudio backend to rust and have landed it in the tree, we should enable it in preference over the C backend.
Assignee: nobody → dglastonbury
Status: NEW → ASSIGNED
We should consider having a pref or other easy switchover, unless we're quite sure of the stability and functionality are equivalent, especially given no Aurora
Rank: 15
Priority: -- → P1
(In reply to Randell Jesup [:jesup] from comment #1)
> We should consider having a pref or other easy switchover, unless we're
> quite sure of the stability and functionality are equivalent, especially
> given no Aurora

There is a pref that can be used to select which backend is used by cubeb: media.cubeb.backend
Current setting this pref to "pulse-rust" will select the rust version of the backend.  If I was to make the rust backend take preference, media.cubeb.backend = "pulse" can be used to select the C backend.
Matthew, does this change need a patch so we don't lose the change when cubeb is updated?

I see disable-assert.patch in media/libcubeb so I assume you'll need to see something similar in the patch before granting an r+.
Flags: needinfo?(kinetik)
I think it'd be better to add a USE_PULSE_RUST and insert that above the existing USE_PULSE in cubeb_init.  That way we can take it upstream and not require a patch in Gecko.
Flags: needinfo?(kinetik)
Comment on attachment 8870631 [details]
Bug 1360060 - P2: Add patch to libcubeb/update.sh

https://reviewboard.mozilla.org/r/142082/#review145750
Attachment #8870631 - Flags: review?(kinetik) → review-
Comment on attachment 8870628 [details]
Bug 1360060 - P1: Enable selection of cubeb-pulse-rust over cubeb-pulse.

https://reviewboard.mozilla.org/r/142076/#review145748
Attachment #8870628 - Flags: review?(kinetik) → review-
Comment on attachment 8870628 [details]
Bug 1360060 - P1: Enable selection of cubeb-pulse-rust over cubeb-pulse.

https://reviewboard.mozilla.org/r/142076/#review146172
Attachment #8870628 - Flags: review?(kinetik) → review+
Comment on attachment 8870631 [details]
Bug 1360060 - P2: Add patch to libcubeb/update.sh

https://reviewboard.mozilla.org/r/142082/#review146174
Attachment #8870631 - Flags: review?(kinetik) → review+
Comment on attachment 8870757 [details]
Bug 1360060 - P3: device_collection_destroy for cubeb-pulse-rs.

https://reviewboard.mozilla.org/r/142248/#review146176
Attachment #8870757 - Flags: review?(kinetik) → review+
Depends on: 1357646
Depends on: 1367646
No longer depends on: 1357646
Pushed by dglastonbury@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/131f4f059729
P1: Enable selection of cubeb-pulse-rust over cubeb-pulse. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/372f1eece4d4
P2: Add patch to libcubeb/update.sh r=kinetik
https://hg.mozilla.org/integration/autoland/rev/1ba8f05e12f3
P3: device_collection_destroy for cubeb-pulse-rs. r=kinetik
https://hg.mozilla.org/mozilla-central/rev/131f4f059729
https://hg.mozilla.org/mozilla-central/rev/372f1eece4d4
https://hg.mozilla.org/mozilla-central/rev/1ba8f05e12f3
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
The final commit in this series causes browser
crashes on startup on Linux x86_64.  According to
https://crash-stats.mozilla.com/report/index/ac5e48e5-3c13-4553-8b00-b6f
bc0170608, the crash occurs in libpulsecommon, although I was not able
to enable debug symbols and have the crash reporter pick up on them.

After bisecting m-c from yesterday’s merge, I found this to be the bad
commit:

> % git bisect good
> 2f8dff0a968ddc48b48cd849bc8b51f90ae58295 is the first bad commit
> commit 2f8dff0a968ddc48b48cd849bc8b51f90ae58295
> Author: Dan Glastonbury <dglastonbury@mozilla.com>
> Date:   Wed May 24 19:33:19 2017 +1000
> 
>     Bug 1360060 - P3: device_collection_destroy for cubeb-pulse-rs. r=kinetik
>     
>     MozReview-Commit-ID: Hvn12h4O4FE
> 
> :040000 040000 9aae99f4fc78c7f810c1e8831c6d402fd06d8a7e daa1d9d1b4411a68d28c0eba03c83d339797b82a M	media
Flags: needinfo?(dglastonbury)
For reasons I cannot comprehend, gdb won’t associate the addresses in
the stack with symbols from libxul.so.  But with help from padenot, I
was able to recreate the symbolised stack using

	% addr2line -e libxul.so <addr>

Bear in mind that this is a manually crafted stack, and as a consequence
some of the lines in in the external libraries are missing because I
didn’t bother finding the right .so file:

> #01: /home/ato/src/gecko/js/src/ds/MemoryProtectionExceptionHandler.cpp:267
> #02: /home/ato/src/gecko/js/src/wasm/WasmSignalHandlers.cpp:1313
> #03: ??:?
> #04: ??:?
> #05: ??:0
> #06: /home/ato/src/gecko/media/libcubeb/cubeb-pulse-rs/pulse-ffi/src/ffi_funcs.rs:1235
> #07: /home/ato/src/gecko/media/libcubeb/cubeb-pulse-rs/src/backend/context.rs:161
> #08: /home/ato/src/gecko/media/libcubeb/cubeb-pulse-rs/src/backend/context.rs:80
> #09: cubeb_pulse.cgu-1.rs:?
> #10: cubeb_pulse.cgu-1.rs:?
> #11: /home/ato/src/gecko/media/libcubeb/cubeb-pulse-rs/src/backend/context.rs:131
> #12: /home/ato/src/gecko/media/libcubeb/cubeb-pulse-rs/src/capi.rs:12
> #13: /home/ato/src/gecko/media/libcubeb/cubeb-pulse-rs/src/capi.rs:249
> #14: /home/ato/src/gecko/media/libcubeb/src/cubeb.c:211
> #15: /home/ato/src/gecko/dom/media/CubebUtils.cpp:342
> #16: /home/ato/src/gecko/dom/media/CubebUtils.cpp:205
> #17: /home/ato/src/gecko/dom/ipc/ContentChild.cpp:1470
> #18: /home/ato/src/gecko/obj-x86_64-pc-linux-gnu/ipc/ipdl/PContentChild.cpp:5868 (discriminator 1)
> #19: /home/ato/src/gecko/ipc/glue/MessageChannel.cpp:2075
> #20: /home/ato/src/gecko/ipc/glue/MessageChannel.cpp:2001
> #21: /home/ato/src/gecko/ipc/glue/MessageChannel.cpp:1871
> #22: /home/ato/src/gecko/ipc/glue/MessageChannel.cpp:1904
> #23: /home/ato/src/gecko/xpcom/threads/nsThread.cpp:1369 (discriminator 1)
> #24: /home/ato/src/gecko/xpcom/threads/nsThreadUtils.cpp:472 (discriminator 3)
> #25: /home/ato/src/gecko/ipc/glue/MessagePump.cpp:96
> #26: /home/ato/src/gecko/ipc/glue/MessagePump.cpp:302 (discriminator 1)
> #27: /home/ato/src/gecko/ipc/chromium/src/base/message_loop.cc:239
> #28: /home/ato/src/gecko/ipc/chromium/src/base/message_loop.cc:232
> #29: /home/ato/src/gecko/ipc/chromium/src/base/message_loop.cc:211
> #30: /home/ato/src/gecko/widget/nsBaseAppShell.cpp:156 (discriminator 1)
> #31: /home/ato/src/gecko/toolkit/xre/nsEmbedFunctions.cpp:896
> #32: /home/ato/src/gecko/ipc/glue/MessagePump.cpp:269 (discriminator 1)
> #33: /home/ato/src/gecko/ipc/chromium/src/base/message_loop.cc:239
> #34: /home/ato/src/gecko/ipc/chromium/src/base/message_loop.cc:232
> #35: /home/ato/src/gecko/ipc/chromium/src/base/message_loop.cc:211
> #36: /home/ato/src/gecko/toolkit/xre/nsEmbedFunctions.cpp:712
> #37: /home/ato/src/gecko/toolkit/xre/Bootstrap.cpp:65
> #38: ??:0
> #39: ??:0
> #40: ??:0
> #41: ??:0
Depends on: 1371319
Flags: needinfo?(dglastonbury)
This was reviewed by relmgmt team as a risky uplift to land a few days before Nightly 55 merge to Beta. Maire and Anthony mentioned that this wasn't intended to ride beta55. I am told that there will be a backout Pacific Sunday night. NI Sylvestre, as we may need to wait for this backout to happen before we request m-c -> m-b merge. Thanks!
Flags: needinfo?(sledru)
Ritu, where is the patch or bug for the backout? This one?
Thanks
Flags: needinfo?(sledru) → needinfo?(rkothari)
Anthony, is this change riding to 55 beta post-merge? Or are we planning to disable it? We need the patch to disable asap so that the merge day and 54 go-live remains unaffected. Thanks!
Flags: needinfo?(rkothari) → needinfo?(ajones)
Maire, wdyt? merci!
Flags: needinfo?(mreavy)
The disable (effective backout for beta-and-later) merged to m-c on bug 1372057 about 2 hours ago, so we should be good for beta 55.
Flags: needinfo?(mreavy)
Flags: needinfo?(ajones)
You need to log in before you can comment on or make changes to this bug.