Closed Bug 1341238 Opened 3 years ago Closed 3 years ago

Patch to allow users to force a particular libcubeb audio backend


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




Tracking Status
firefox55 --- fixed


(Reporter: damien, Assigned: jesup)





(3 files, 12 obsolete files)

59 bytes, text/x-review-board-request
59 bytes, text/x-review-board-request
59 bytes, text/x-review-board-request
Attached patch 0002-gecko.patch (obsolete) — Splinter Review
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 )  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.
Component: Untriaged → Audio/Video: cubeb
Product: Firefox → Core
Ever confirmed: true
Rank: 35
Priority: -- → P3
Comment on attachment 8839412 [details] [diff] [review]

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.
Attachment #8839412 - Flags: review-
Attached patch 0002-gecko2.patch (obsolete) — Splinter Review
Here's version 2 of the gecko patch that goes with the second version of the github pull request on cubeb.
Attachment #8839412 - Attachment is obsolete: true
Comment on attachment 8845823 [details] [diff] [review]

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.
Attachment #8845823 - Flags: review+
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?)
Yeah, the comment is supposed to be attached to the "int rv;" line.  I just mean don't break it into two lines.
What's the process for updating libcubeb required to get this patch to compile?  Check out libcubeb locally and run from media/libcubeb?
That's right, along with adding any new upstream files to 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.
Assignee: nobody → damien
Why are you adding this pref here?
(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).
Attachment #8846884 - Flags: review?(padenot) → review+
Comment on attachment 8846886 [details] [diff] [review]

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.

Attachment #8846886 - Flags: review?(wmccloskey) → review+
Attachment #8847214 - Flags: review?(ehsan)
Attachment #8847213 - Flags: review?(padenot)
Attachment #8847212 - Flags: review?(kinetik)
Attachment #8846882 - Attachment is obsolete: true
Attachment #8846883 - Attachment is obsolete: true
Attachment #8846884 - Attachment is obsolete: true
Attachment #8846886 - Attachment is obsolete: true
Try push:

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 room, and tomorrow is our weekly standup.

MozReview-Commit-ID: AVTx5hVjNzW
Attachment #8847409 - Flags: review?(kinetik)
Assignee: damien → rjesup
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. */

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...
Attachment #8847409 - Flags: review?(kinetik) → review+
Attachment #8847433 - Flags: review?(rjesup)
Attachment #8847433 - Attachment is patch: true
Attachment #8847433 - Attachment mime type: text/x-patch → text/plain
Attachment #8847433 - Flags: review?(rjesup) → review+
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:
Attachment #8847211 - Attachment is obsolete: true
Attachment #8847212 - Attachment is obsolete: true
Attachment #8847213 - Attachment is obsolete: true
Attachment #8847214 - Attachment is obsolete: true
Attachment #8847409 - Attachment is obsolete: true
Attachment #8847433 - Attachment is obsolete: true
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:
Pushed by
Update cubeb to revision 4ab45776.  r=kinetik
Introduce (hidden) pref to force a particular libcubeb backend.  r=kinetik,ehsan
Don't hang on to pointer from temporary NS_LossyConvertUTF16toASCII.  r=padenot
Blocks: 1345147
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1348021
You need to log in before you can comment on or make changes to this bug.