1341238 - Patch to allow users to force a particular libcubeb audio backend

Status: RESOLVED FIXED

(Core :: Audio/Video: cubeb, defect, P3)

Description

[Damien Zammit]

Attachment: 0002-gecko.patch

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20160607223741

Steps to reproduce:

I am attempting to get a new patch into libcubeb upstream, ( see https://github.com/kinetiknz/cubeb/pull/239 )  but it needs to partner with this patch in gecko because my patch slightly changes the cubeb_init API.


Actual results:

The attached patch adds an about:config preference string to gecko that allows the user to set their desired audio backend in libcubeb.  For example, a user who wants to use jack over pulseaudio can specify to use "jack", therefore, the audio backend is forced to the user's choice, *if* the backend was compiled in.  If not, it will default to whatever order was chosen before by cubeb devs.


Expected results:

Hopefully, with the introduction of this backend selection functionality, one day Firefox with JACK support will be compiled into default builds, even if it is switched off or hidden by default at run time.

Comment 1

[Matthew Gregan [:kinetik]]

Comment on attachment 8839412 [details] [diff] [review]
0002-gecko.patch

Review of attachment 8839412 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/CubebUtils.cpp
@@ +330,5 @@
>    }
>  
> +  int rv;
> +  const char* bPref = PREF_CUBEB_BACKEND;
> +  nsAdoptingString value = Preferences::GetString(bPref);

The preferences API is main thread only but GetCubebContextUnlocked is called from other threads.  You need to follow the existing pattern InitLibrary/PrefChanged/etc.

Comment 2

[Damien Zammit]

Attachment: 0002-gecko2.patch

Here's version 2 of the gecko patch that goes with the second version of the github pull request on cubeb.

Comment 3

[Matthew Gregan [:kinetik]]

Comment on attachment 8845823 [details] [diff] [review]
0002-gecko2.patch

Review of attachment 8845823 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM with one nit

::: dom/media/CubebUtils.cpp
@@ +341,5 @@
>        sBrandName, "Did not initialize sbrandName, and not on the main thread?");
>    }
>  
> +  int rv;
> +  rv = cubeb_init(&sCubebContext, sBrandName, sCubebBackendName.get());

This change isn't necessary, please remove before checking in.

Comment 4

[Damien Zammit]

To be clear, do you mean that the above line should read:

`int rv = cubeb_init(&sCubebContext, sBrandName, sCubebBackendName.get());`

(isn't the whole point of the patch to call cubeb_init with new params?)

Comment 5

[Matthew Gregan [:kinetik]]

Yeah, the comment is supposed to be attached to the "int rv;" line.  I just mean don't break it into two lines.

Comment 6

[u480271]

What's the process for updating libcubeb required to get this patch to compile?  Check out libcubeb locally and run update.sh from media/libcubeb?

Comment 7

[Matthew Gregan [:kinetik]]

That's right, along with adding any new upstream files to update.sh so they're picked up during subsequent imports.

In this case I've already prepared an import, it just needs a try run before landing.

Comment 8

[Matthew Gregan [:kinetik]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=13b78bf4e95f9af3cd24a5a8512f19d0b8ab2bd2

Comment 9

[Matthew Gregan [:kinetik]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=894d5950d5425dea1ccd65021b28004fe50c8078

Comment 10

[Matthew Gregan [:kinetik]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=191580b55d3544c2e9f006a5ed74f4e90478adc8

Comment 11

[Matthew Gregan [:kinetik]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d171014270d113368878c897c2c6645b655d0a00

Comment 12

[Matthew Gregan [:kinetik]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3637e615aa565e6fd031f9a150375d856b20288b

Comment 13

[Matthew Gregan [:kinetik]]

Attachment: 0001-Bug-1341238-Update-libcubeb-to-4c39aae8.patch

Comment 14

[Matthew Gregan [:kinetik]]

Attachment: 0002-Bug-1341238-Introduce-hidden-pref-to-force-a-particu.patch

Comment 15

[Matthew Gregan [:kinetik]]

Attachment: 0003-Bug-1341238-Don-t-hang-on-to-pointer-from-temporary-.patch

Comment 16

[Matthew Gregan [:kinetik]]

Attachment: 0004-Bug-1341238-Add-media.cubeb.backend-to-ContentPrefs..patch

Comment 17

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Why are you adding this pref here?

Comment 18

[Matthew Gregan [:kinetik]]

(In reply to Bill McCloskey (:billm) from comment #17)
> Why are you adding this pref here?

Because it's needed early to initialize the audio context with the correct backend (i.e. the same reason the existing media.cubeb.* prefs are in that list).

Comment 19

[Paul Adenot (:padenot)]

Comment on attachment 8846886 [details] [diff] [review]
0004-Bug-1341238-Add-media.cubeb.backend-to-ContentPrefs..patch

Bill, we need to land this asap, so I asked ehsan and bz to have a look, and explained why we need this in content prefs, and they r+ed, but I'll re-explain here.

`cubeb_init` is called very very early in the lifetime of a content process, because it's doing some system calls and IO, etc. (can't be too specific because it's very platform dependent, cubeb is our multi-platform audio IO library), and so we're doing it before locking down the process [0]. When we're done remoting audio properly, this will go away. I adjusted the commit message to provide this info.

[0]: http://searchfox.org/mozilla-central/source/dom/ipc/ContentChild.cpp#1341

Comment 20

[Paul Adenot (:padenot)]

Attachment: Bug 1341238 - Update cubeb to revision d974ad.

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

Comment 21

[Paul Adenot (:padenot)]

Attachment: Bug 1341238 - Introduce (hidden) pref to force a particular libcubeb backend.

This pref is going to be mostly static, but we need to access it very early so
we're using an observer.

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

Comment 22

[Paul Adenot (:padenot)]

Attachment: Bug 1341238 - Don't hang on to pointer from temporary NS_LossyConvertUTF16toASCII.

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

Comment 23

[Paul Adenot (:padenot)]

Attachment: Bug 1341238 - Add media.cubeb.backend to ContentPrefs.

This is necessary because `cubeb_init` is called _very_ early in the lifetime of a
content process, because it needs to be called before enabling seccomp.

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

Comment 24

[Paul Adenot (:padenot)]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f13f5c3c7956e3a6d01e7af1cbe44c681f93372

This is equivalent to Matthew's push above, but with one more commit [0] added to the first patch that uplifts upstream cubeb changes into gecko, and with commit messages that explain the non-obvious reason why those prefs are needed very early (see also comment 19).

Ideally, we'd need this in tomorrow's nightly, we need this async logger to debug a nasty drift problem on OSX, that we can only repro when we're six people talking in a appear.in room, and tomorrow is our weekly standup.

[0]: https://github.com/kinetiknz/cubeb/commit/d974ad6b5fb71bae0c0089cbeb54645dfe4ecbc9

Comment 25

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

New try with test_overload_callback disabled per kinetik (IRC)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3313baef24b3211b9498b241580c5e63e7962cab

Comment 26

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

Attachment: replace std::this_thread::sleep_for for posix with nanosleep

MozReview-Commit-ID: AVTx5hVjNzW

Comment 27

[Matthew Gregan [:kinetik]]

Comment on attachment 8847409 [details] [diff] [review]
replace std::this_thread::sleep_for for posix with nanosleep

Review of attachment 8847409 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/libcubeb/src/cubeb_log.cpp
@@ +18,5 @@
>  /** The maximum number of log messages that can be queued before dropping
>   * messages. */
>  const size_t CUBEB_LOG_MESSAGE_QUEUE_DEPTH = 40;
>  /** Number of milliseconds to wait before dequeuing log messages. */
> +#ifdef SUPPORTS_STD_SLEEP_FOR

We might as well just remove this entirely.

@@ +84,1 @@
>          std::this_thread::sleep_for(CUBEB_LOG_BATCH_PRINT_INTERVAL_MS);

This can just be |Sleep(CUBEB_LOG_BATCH_PRINT_INTERVAL_MS);|, include windows.h behind the same define and then the Windows side is done.  I can update the patch when my build finishes...

Comment 28

[Matthew Gregan [:kinetik]]

Attachment: replace this_thread::sleep_for on Win32

Comment 29

[Matthew Gregan [:kinetik]]

Update libcubeb import again to pick up latest changes (this obsoletes the patches from jesup and I to fix the sleep_for issue).  Also squash the ContentPrefs change into the pref patch, since it doesn't work without it.  This also tries to enable the overloaded callback gtest since the compile error is fixed in the latest import:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0138ac26d5b88d4735052fd00ea1cbf3307b5a84

Comment 30

[Matthew Gregan [:kinetik]]

Attachment: Bug 1341238 - Update cubeb to revision 4ab45776.

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

Comment 31

[Matthew Gregan [:kinetik]]

Attachment: Bug 1341238 - Introduce (hidden) pref to force a particular libcubeb backend.

This adds 'media.cubeb.backend' to ContentPrefs, which is necessary because
`cubeb_init` is called _very_ early in the lifetime of a content process,
because it needs to be called before enabling seccomp.

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

Comment 32

[Matthew Gregan [:kinetik]]

Attachment: Bug 1341238 - Don't hang on to pointer from temporary NS_LossyConvertUTF16toASCII.

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

Comment 33

[Matthew Gregan [:kinetik]]

Sure enough, test_overload_callback causes the GTests to timeout in automation.  It'll need more work before it can be enabled in Gecko.

Push with test excluded:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bfa1607e35a9bae34e9112949eaffdaaf4f4a030

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e31fb58a1d3
Update cubeb to revision 4ab45776.  r=kinetik
https://hg.mozilla.org/integration/mozilla-inbound/rev/f62c98850fd1
Introduce (hidden) pref to force a particular libcubeb backend.  r=kinetik,ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/bafa69c3080e
Don't hang on to pointer from temporary NS_LossyConvertUTF16toASCII.  r=padenot

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5e31fb58a1d3
https://hg.mozilla.org/mozilla-central/rev/f62c98850fd1
https://hg.mozilla.org/mozilla-central/rev/bafa69c3080e