845648 - Supply more useful strings for cubeb context and stream names

Status: RESOLVED FIXED

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

Description

[Matthew Gregan [:kinetik]]

Right now we pass "AudioStream" to cubeb_init as the context name and "BufferedAudioStream" to cubeb_stream_init as the stream name.  Neither of these are particularly useful.

It will be a regression on Linux when bug 837563 happens because the old ALSA plugin contained the process name, so an audio stream would show up in the OS audio mixer UI as "ALSA plug-in [firefox]", which will become "AudioStream".

It seems like using brandShortName from the branding bundle is a more useful string for the context name.

The stream name could be derived from the owning document, but choosing the right thing for this may require more thought.

Comment 2

[fincer89]

Attachment: CubebUtils issue

I'm bit frustrated with this issue, since I can't really tell which Firefox audio stream is which: Can't really make the difference between multiple streams because they all have same titles. Please see the attached screenshot for more information. 

It would be really helpful if I was able to sort Cubeb audio streams, without playing additional & daily trial-and-error audio puzzle game on my Linux system.

All audio streams seen in the picture are Youtube video streams (with audio).

Comment 3

[Paul Adenot (:padenot)]

Attachment: Bug 845648 - Use shortBrand for the name of cubeb stream.

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

Comment 4

[Matthew Gregan [:kinetik]]

Comment on attachment 8759623 [details]
Bug 845648 - Use shortBrand for the name of cubeb stream.

https://reviewboard.mozilla.org/r/57572/#review54580

r+ assuming it's safe to use this code in whatever random thread GetCubebContext() ends up called from.

::: dom/media/CubebUtils.cpp:123
(Diff revision 1)
> +    mozilla::services::GetStringBundleService();
> +  if (stringBundleService) {
> +    nsCOMPtr<nsIStringBundle> brandBundle;
> +    nsresult rv = stringBundleService->CreateBundle(kBrandBundleURL,
> +                                           getter_AddRefs(brandBundle));
> +    if (rv == NS_OK) {

Use NS_SUCCEEDED()?

::: dom/media/CubebUtils.cpp:124
(Diff revision 1)
> +  if (stringBundleService) {
> +    nsCOMPtr<nsIStringBundle> brandBundle;
> +    nsresult rv = stringBundleService->CreateBundle(kBrandBundleURL,
> +                                           getter_AddRefs(brandBundle));
> +    if (rv == NS_OK) {
> +      rv = brandBundle->GetStringFromName(MOZ_UTF16("brandShortName"),

Check result here?

Comment 5

[Paul Adenot (:padenot)]

Hrm you're right, it's not threadsafe. It work fine in e10s because `GetCubebContext()` is called on the main thread of the child process at the beginning. This initializes the name at the same time we set the preference handlers, but in the next event loop tick because we can't use XPCOM yet when we get the prefs, it's too early, and we get an error about recursive Layout module initialization.

Comment 6

[Paul Adenot (:padenot)]

Comment on attachment 8759623 [details]
Bug 845648 - Use shortBrand for the name of cubeb stream.

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

Comment 7

[Paul Adenot (:padenot)]

Comment on attachment 8759623 [details]
Bug 845648 - Use shortBrand for the name of cubeb stream.

https://reviewboard.mozilla.org/r/57572/#review55040

Comment 8

[Matthew Gregan [:kinetik]]

Comment on attachment 8759623 [details]
Bug 845648 - Use shortBrand for the name of cubeb stream.

https://reviewboard.mozilla.org/r/57572/#review55234

::: dom/media/CubebUtils.cpp:172
(Diff revision 2)
>  {
>    PrefChanged(PREF_VOLUME_SCALE, nullptr);
>    Preferences::RegisterCallback(PrefChanged, PREF_VOLUME_SCALE);
>    PrefChanged(PREF_CUBEB_LATENCY, nullptr);
>    Preferences::RegisterCallback(PrefChanged, PREF_CUBEB_LATENCY);
> +  NS_DispatchToMainThread(NS_NewRunnableFunction(&InitBrandName));

Is this needed to wait for something to initialize? InitLibrary is already called on the main thread (and we could assert that), so I assume the dispatch is necessary to let some inititalization complete?

Comment 9

[Paul Adenot (:padenot)]

Yes, see the end of comment 5, we can't use XPCOM at the moment we're initing the layout module, we need to wait a bit.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b9212818ced
Use shortBrand for the name of cubeb streams. r=kinetik

Comment 11

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc2a9daf2bb6
Backed out changeset 2b9212818ced for test failures in browser_audioCompeting.js

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/658c25a84057
Use shortBrand for the name of cubeb streams. r=kinetik

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e0258f175c1
Don't try to get the local on Android.

Comment 14

[Paul Adenot (:padenot)]

This last patch ^ stops executing the code on Android. It's failing for some reason: I can't get the shortBrand on android, and I'm not sure why. The context name is unused on Android anyways.

snorp, what would be the right way to get the `shortBrandName` on Android? It's failing with here: https://dxr.mozilla.org/mozilla-central/source/chrome/nsChromeRegistryChrome.cpp#290

Comment 15

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Paul Adenot (:padenot) from comment #14)
> This last patch ^ stops executing the code on Android. It's failing for some
> reason: I can't get the shortBrand on android, and I'm not sure why. The
> context name is unused on Android anyways.
> 
> snorp, what would be the right way to get the `shortBrandName` on Android?
> It's failing with here:
> https://dxr.mozilla.org/mozilla-central/source/chrome/nsChromeRegistryChrome.
> cpp#290

There shouldn't be anything special about Android here. On Nightly, the MATCH_OS_LOCALE_PREF is set and defaults to 'true', so we should be hitting that first branch. I would guess that getUILangCountry() is what ends up failing, and I have no idea why that would be true.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/658c25a84057
https://hg.mozilla.org/mozilla-central/rev/1e0258f175c1