Closed Bug 1341238 Opened 3 years ago Closed 3 years ago

Patch to allow users to force a particular libcubeb audio backend

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: damien, Assigned: jesup)

References

()

Details

Attachments

(3 files, 12 obsolete files)

59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
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 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.
Component: Untriaged → Audio/Video: cubeb
Product: Firefox → Core
Status: UNCONFIRMED → NEW
Ever confirmed: true
Rank: 35
Priority: -- → P3
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.
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]
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.
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 update.sh from media/libcubeb?
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.
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]
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
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: 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
MozReview-Commit-ID: AVTx5hVjNzW
Attachment #8847409 - Flags: review?(kinetik)
Assignee: damien → rjesup
Status: NEW → ASSIGNED
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...
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:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0138ac26d5b88d4735052fd00ea1cbf3307b5a84
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:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bfa1607e35a9bae34e9112949eaffdaaf4f4a030
Pushed by mgregan@mozilla.com:
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
Blocks: 1345147
https://hg.mozilla.org/mozilla-central/rev/5e31fb58a1d3
https://hg.mozilla.org/mozilla-central/rev/f62c98850fd1
https://hg.mozilla.org/mozilla-central/rev/bafa69c3080e
Status: ASSIGNED → RESOLVED
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.