Supply more useful strings for cubeb context and stream names

RESOLVED FIXED in Firefox 50

Status

()

Core
Audio/Video: cubeb
P2
normal
Rank:
29
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: kinetik, Assigned: padenot)

Tracking

Trunk
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
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.
Component: Audio/Video → Audio/Video: MSG/cubeb/GMP
Component: Audio/Video: MediaStreamGraph → Audio/Video: cubeb

Updated

2 years ago
Rank: 29
Priority: -- → P2
(Reporter)

Updated

2 years ago
Duplicate of this bug: 1140953

Comment 2

2 years ago
Created attachment 8738513 [details]
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).
(Assignee)

Comment 3

2 years ago
Created attachment 8759623 [details]
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/
Attachment #8759623 - Flags: review?(kinetik)
(Assignee)

Updated

2 years ago
Assignee: nobody → padenot
(Reporter)

Comment 4

2 years ago
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?
Attachment #8759623 - Flags: review?(kinetik) → review+
(Assignee)

Comment 5

2 years ago
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.
(Assignee)

Comment 6

2 years ago
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/
(Assignee)

Comment 7

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

https://reviewboard.mozilla.org/r/57572/#review55040
Attachment #8759623 - Flags: review?(padenot)
(Assignee)

Updated

2 years ago
Attachment #8759623 - Flags: review?(padenot)
Attachment #8759623 - Flags: review?(kinetik)
Attachment #8759623 - Flags: review+
(Reporter)

Comment 8

2 years ago
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?
Attachment #8759623 - Flags: review?(kinetik) → review+
(Assignee)

Comment 9

2 years ago
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

2 years ago
Pushed by paul@paul.cx:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b9212818ced
Use shortBrand for the name of cubeb streams. r=kinetik

Comment 11

2 years ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc2a9daf2bb6
Backed out changeset 2b9212818ced for test failures in browser_audioCompeting.js

Comment 12

2 years ago
Pushed by paul@paul.cx:
https://hg.mozilla.org/integration/mozilla-inbound/rev/658c25a84057
Use shortBrand for the name of cubeb streams. r=kinetik

Comment 13

2 years ago
Pushed by paul@paul.cx:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e0258f175c1
Don't try to get the local on Android.
(Assignee)

Comment 14

2 years ago
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
Flags: needinfo?(snorp)
(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.
Flags: needinfo?(snorp)

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/658c25a84057
https://hg.mozilla.org/mozilla-central/rev/1e0258f175c1
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1286341
You need to log in before you can comment on or make changes to this bug.