1360060 - Enable use of rust version of cubeb PulseAudio backend.

Status: RESOLVED FIXED

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

Description

[u480271]

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.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 2

[u480271]

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

Comment 3

[u480271]

Attachment: Bug 1360060 - P1: Enable selection of cubeb-pulse-rust over cubeb-pulse.

Review commit: https://reviewboard.mozilla.org/r/142076/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/142076/

Comment 4

[u480271]

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

Comment 5

[u480271]

Attachment: Bug 1360060 - P2: Add patch to libcubeb/update.sh

Because this change is outside of the cubeb repo, add a patch that is
applied during the process of updating cubeb version.

Review commit: https://reviewboard.mozilla.org/r/142082/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/142082/

Comment 6

[u480271]

Comment on attachment 8870628 [details]
Bug 1360060 - P1: Enable selection of cubeb-pulse-rust over cubeb-pulse.

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

Comment 7

[Matthew Gregan [: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.

Comment 8

[Matthew Gregan [:kinetik]]

Comment on attachment 8870631 [details]
Bug 1360060 - P2: Add patch to libcubeb/update.sh

https://reviewboard.mozilla.org/r/142082/#review145750

Comment 9

[Matthew Gregan [:kinetik]]

Comment on attachment 8870628 [details]
Bug 1360060 - P1: Enable selection of cubeb-pulse-rust over cubeb-pulse.

https://reviewboard.mozilla.org/r/142076/#review145748

Comment 10

[u480271]

Attachment: Bug 1360060 - P3: device_collection_destroy for cubeb-pulse-rs.

Review commit: https://reviewboard.mozilla.org/r/142248/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/142248/

Comment 11

[u480271]

Comment on attachment 8870628 [details]
Bug 1360060 - P1: Enable selection of cubeb-pulse-rust over cubeb-pulse.

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

Comment 12

[u480271]

Comment on attachment 8870631 [details]
Bug 1360060 - P2: Add patch to libcubeb/update.sh

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

Comment 13

[Matthew Gregan [:kinetik]]

Comment on attachment 8870628 [details]
Bug 1360060 - P1: Enable selection of cubeb-pulse-rust over cubeb-pulse.

https://reviewboard.mozilla.org/r/142076/#review146172

Comment 14

[Matthew Gregan [:kinetik]]

Comment on attachment 8870631 [details]
Bug 1360060 - P2: Add patch to libcubeb/update.sh

https://reviewboard.mozilla.org/r/142082/#review146174

Comment 15

[Matthew Gregan [:kinetik]]

Comment on attachment 8870757 [details]
Bug 1360060 - P3: device_collection_destroy for cubeb-pulse-rs.

https://reviewboard.mozilla.org/r/142248/#review146176

Comment 16

[u480271]

Comment on attachment 8870628 [details]
Bug 1360060 - P1: Enable selection of cubeb-pulse-rust over cubeb-pulse.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142076/diff/3-4/

Comment 17

[u480271]

Comment on attachment 8870631 [details]
Bug 1360060 - P2: Add patch to libcubeb/update.sh

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

Comment 18

[u480271]

Comment on attachment 8870757 [details]
Bug 1360060 - P3: device_collection_destroy for cubeb-pulse-rs.

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

Comment 19

[u480271]

Comment on attachment 8870628 [details]
Bug 1360060 - P1: Enable selection of cubeb-pulse-rust over cubeb-pulse.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142076/diff/4-5/

Comment 20

[u480271]

Comment on attachment 8870631 [details]
Bug 1360060 - P2: Add patch to libcubeb/update.sh

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142082/diff/3-4/

Comment 21

[u480271]

Comment on attachment 8870757 [details]
Bug 1360060 - P3: device_collection_destroy for cubeb-pulse-rs.

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

Comment 22

[u480271]

Carry r+ from :kinetik.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9b03743d866bed1764435d8a5daf8cf691a49df&selectedJob=105113788

Looks good.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
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

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/131f4f059729
https://hg.mozilla.org/mozilla-central/rev/372f1eece4d4
https://hg.mozilla.org/mozilla-central/rev/1ba8f05e12f3

Comment 25

[Andreas Tolfsen ❲:ato❳]

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 <[e-mail]>
> 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

Comment 26

[Andreas Tolfsen ❲:ato❳]

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

Comment 27

[Andreas Tolfsen ❲:ato❳]

Correct link to the crash report earlier is
https://crash-stats.mozilla.com/report/index/ac5e48e5-3c13-4553-8b00-b6fbc0170608.

Comment 28

[Andreas Tolfsen ❲:ato❳]

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1371319 for the crash fallout.

Comment 29

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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!

Comment 30

[Sylvestre Ledru [:Sylvestre]]

Ritu, where is the patch or bug for the backout? This one?
Thanks

Comment 31

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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!

Comment 32

[Sylvestre Ledru [:Sylvestre]]

Maire, wdyt? merci!

Comment 33

[Maire Reavy [: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.