Enable system app to get currently used audio channel

RESOLVED FIXED in Firefox 18

Status

P1
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: sicking, Assigned: baku)

Tracking

unspecified
B2G C2 (20nov-10dec)
x86
Mac OS X
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed, firefox20 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

We need a way for the system app to get the currently use audio channel. Whenever either of the volume buttons are pressed, the system app needs to know:

* If any audio is currently being played, which is the highest-priority channel
  currently being used? I.e. which channel is currently outputting sound to the
  user.
* If no audio is currently playing.

Originally I had envisioned using a mozChromeEvent every time the currently active channel changes. Essentially that would cause us to fire an extra event to the system app any time audio is started or stopped from any app.

If performance is a problem with such an approach we could introduce an API which would let the system app get the currently playing channel. This could then be called only whenever a volume button is pressed.

This API should only be exposed to the system app, so we don't need to spend time on creating something clean. We should just do whatever makes sense. I don't know if the API needs to be async or if it could be sync. Either should be fine for gaia.
blocking-basecamp: --- → ?
blocking-basecamp: ? → +
Andrea or Marco, who of you should take this? :)
(Assignee)

Comment 2

6 years ago
I wrote a patch for this. Is en event enough? I'm still testing this patch but it seems working fine. Probably today or tomorrow I can attach it here.

The new event is 'audio-channel-changed' and the event contains the name of the "higher" channel used.

Updated

6 years ago
Assignee: nobody → amarchesini
Assignee: amarchesini → nobody
Flags: needinfo?(jonas)
Firing a mozChromeEvent on the system app should be enough.
Flags: needinfo?(jonas)
Assignee: nobody → amarchesini
(Assignee)

Comment 4

6 years ago
Created attachment 684400 [details] [diff] [review]
patch

This patch is based on the patches for 805333
Attachment #684400 - Flags: review?(jonas)
Hi folks, I have some questions here:
* Case#1
** If alarms sounds, and the phone is incoming.
** When the voice call is ended, what channel type I will get?

* Case#2
** If I am listening to music, and then alarms sounds, and then I close the alarm, will I get the channel change event when alarm stops to sound?

Thanks!
(Assignee)

Comment 6

6 years ago
> * Case#1
> ** If alarms sounds, and the phone is incoming.
> ** When the voice call is ended, what channel type I will get?

When the call is ended the audiochannelmanager will dispatch an event saying the current audio channel type is 'alarm' if the alarm is still playing. Otherwise, 'normal'.
Right now this is not true, the audio channel manager is not tracking audio call, but this will be implemented for this week.

> * Case#2
> ** If I am listening to music, and then alarms sounds, and then I close the
> alarm, will I get the channel change event when alarm stops to sound?

yes.
Comment on attachment 684400 [details] [diff] [review]
patch

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

::: dom/audiochannel/AudioChannelService.cpp
@@ +246,5 @@
> +AudioChannelService::ChannelName(AudioChannelType aType)
> +{
> +  switch (aType) {
> +    case AUDIO_CHANNEL_NORMAL:
> +      return NS_LITERAL_STRING("normal").get();

This isn't actually safe on all platforms. On some platforms NS_LITERAL_STRING expands to a runtime constructed objects which holds a short-lived string object. So this would return a pointer to a deallocated buffer.

Your best bet might be to create static globals which are lazily initialized and destroyed on shutdown :( Note that you're not allowed to create static objects with ctors.

@@ +248,5 @@
> +  switch (aType) {
> +    case AUDIO_CHANNEL_NORMAL:
> +      return NS_LITERAL_STRING("normal").get();
> +    case AUDIO_CHANNEL_CONTENT:
> +      return NS_LITERAL_STRING("content").get();

We should never end up sending "content" as the "audio-channel-changed" channel, right? Since "normal" and "content" mix together and share the same volume setting.

Still makes sense to have this API be able to return any channel name of course, but I wanted to check.
Attachment #684400 - Flags: review?(jonas) → review-
Hello, 
is there another WIP patch for this bug? And I notice that there's no ringer type now, is it grouped into notification already? AFAIW, Marco is having another bug for adding ringer type. Thanks!
Setting priority based on triage discussions.  Feel free to decrease priority if you disagree.
Priority: -- → P1
(Assignee)

Comment 10

6 years ago
(In reply to Alive Kuo [:alive] from comment #8)
> Hello, 
> is there another WIP patch for this bug? And I notice that there's no ringer
> type now, is it grouped into notification already? AFAIW, Marco is having
> another bug for adding ringer type. Thanks!

Can you add a dependence to this bug?
(Assignee)

Comment 11

6 years ago
Created attachment 687210 [details] [diff] [review]
patch b
Attachment #684400 - Attachment is obsolete: true
Attachment #687210 - Flags: review?(jonas)

Updated

6 years ago
Target Milestone: --- → B2G C2 (20nov-10dec)
Comment on attachment 687210 [details] [diff] [review]
patch b

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

The review comments from the previous review doesn't appear to be addressed.
Attachment #687210 - Flags: review?(jonas) → review-
(Assignee)

Comment 13

6 years ago
> The review comments from the previous review doesn't appear to be addressed.

Yeah... I forgot to save the changes in mq.
(Assignee)

Comment 14

6 years ago
Created attachment 687525 [details] [diff] [review]
patch c
Attachment #687210 - Attachment is obsolete: true
Attachment #687525 - Flags: review?(jonas)
Comment on attachment 687525 [details] [diff] [review]
patch c

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

::: dom/audiochannel/AudioChannelService.cpp
@@ +242,5 @@
> +    { AUDIO_CHANNEL_NOTIFICATION,       NS_LITERAL_STRING("notification") },
> +    { AUDIO_CHANNEL_ALARM,              NS_LITERAL_STRING("alarm") },
> +    { AUDIO_CHANNEL_TELEPHONY,          NS_LITERAL_STRING("telephony") },
> +    { AUDIO_CHANNEL_PUBLICNOTIFICATION, NS_LITERAL_STRING("publicnotification") },
> +    { -1,                               NS_LITERAL_STRING("unknown") }

This unfortunately will show up in our leak stats as leaked strings. So this isn't ok either.

You can create |nsString*| global variables which you manually delete on XPCOM shutdown (you can hook into [1]).

But the simpler way is likely to just return a char* from this function and do a runtime expansion in AudioChannelService::GetMuted. It's unfortunate to have to take additional runtime cost, but the extra expansion is very unlikely to matter given how much else we do on that codepath.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/build/nsLayoutStatics.cpp#269
Attachment #687525 - Flags: review?(jonas) → review-
(Assignee)

Comment 16

6 years ago
Created attachment 688217 [details] [diff] [review]
patch d
Attachment #687525 - Attachment is obsolete: true
Attachment #688217 - Flags: review?(jonas)
(Assignee)

Comment 17

6 years ago
Created attachment 688260 [details] [diff] [review]
patch d

merge problem
Attachment #688217 - Attachment is obsolete: true
Attachment #688217 - Flags: review?(jonas)
Attachment #688260 - Flags: review?(jonas)
Comment on attachment 688260 [details] [diff] [review]
patch d

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

r=me

::: dom/audiochannel/AudioChannelService.cpp
@@ +189,5 @@
> +      nsString channelName;
> +      channelName.AssignASCII(ChannelName(mCurrentHigherChannel));
> +
> +      nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> +      obs->NotifyObservers(nullptr, "audio-channel-changed", channelName.get());

It should also work to do:

ConvertASCIItoUTF16(ChannelName(...)).get();
Attachment #688260 - Flags: review?(jonas) → review+
Landed on mozilla-central and mozilla-beta:

https://hg.mozilla.org/mozilla-central/rev/137d80a07e64
https://hg.mozilla.org/releases/mozilla-beta/rev/d8192e31580a
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Jonas, can you please land this on aurora too?
status-firefox18: --- → fixed
status-firefox19: --- → affected
status-firefox20: --- → fixed
Jonas, we need to discuss this bug. The implementation now doesn't make sense.
STR:
1. Listen to Music app
2. Set an alarm
3. When alarm goes off

Expected:
* audio-channel-changed changes only one time from content to alarm

Actual:
* audio-channel-changed switched between content and alarm rapidly. Too bad...

I think it's because the alarm sound is just one voice and when the voice ends, the focus is changed back to normal but quickly when the alarm rings again the focus is changed again.
(In reply to Alive Kuo [:alive] from comment #21)
> Jonas, we need to discuss this bug. The implementation now doesn't make
> sense.
> STR:
> 1. Listen to Music app
> 2. Set an alarm
> 3. When alarm goes off
> 
> Expected:
> * audio-channel-changed changes only one time from content to alarm
> 
> Actual:
> * audio-channel-changed switched between content and alarm rapidly. Too
> bad...
> 
> I think it's because the alarm sound is just one voice and when the voice
> ends, the focus is changed back to normal but quickly when the alarm rings
> again the focus is changed again.

I don't think this is a problem to you unless the user continuously pressing the volume key when alarm rings...
Anyway I don't get ringer channel and telephony channel event too. Only alarm, content, notification. Is this because telephony not hooked to audio manager yet?
Another, I don't get an event after content playing is ended. The channel should change to 'default'.
(In reply to Alive Kuo [:alive] from comment #23)
> Anyway I don't get ringer channel and telephony channel event too. Only
> alarm, content, notification. Is this because telephony not hooked to audio
> manager yet?

About ringer, it depends on bug 815322 (marked checkin-needed).
About telephony, it depends on bug 815445. (review?)
(In reply to Alive Kuo [:alive] from comment #24)
> Another, I don't get an event after content playing is ended. The channel
> should change to 'default'.

Please refer to https://bugzilla.mozilla.org/show_bug.cgi?id=811222#c7 .
The normal and content channels are all sent as audio-channel-changed:normal because they are all corresponding to audio.volume.content setting.
(In reply to Marco Chen [:mchen] from comment #26)
> (In reply to Alive Kuo [:alive] from comment #24)
> > Another, I don't get an event after content playing is ended. The channel
> > should change to 'default'.
> 
> Please refer to https://bugzilla.mozilla.org/show_bug.cgi?id=811222#c7 .
> The normal and content channels are all sent as audio-channel-changed:normal
> because they are all corresponding to audio.volume.content setting.

This is type change, IMO irrelevant to what setting they listen to coz we may change the setting in the future. and UX has determined the default channel is notification...
(In reply to Alive Kuo [:alive] from comment #27)
> This is type change, IMO irrelevant to what setting they listen to coz we
> may change the setting in the future. and UX has determined the default
> channel is notification...

If you can make sure what Firefox OS want, maybe you can fire another bug for correcting the behavior to spec.
(In reply to Alive Kuo [:alive] from comment #21)
> I think it's because the alarm sound is just one voice and when the voice
> ends, the focus is changed back to normal but quickly when the alarm rings
> again the focus is changed again.

Maybe a workaround would be to use a auto-looping <audio loop src="..."> element in the dialer app for playing the ringer signal. Hopefully that wouldn't cause the starting/stopping.
(In reply to Alive Kuo [:alive] from comment #27)
> This is type change, IMO irrelevant to what setting they listen to coz we
> may change the setting in the future. and UX has determined the default
> channel is notification...

I'm not sure what you mean by this. The default audio channel should definitely be "normal" and not "notification".

However it's quite likely that we need to add mozaudiochannel="notification" to a few audio elements in gaia.
1eb86723 (Andrea Marchesini         2012-12-05 17:20:59 -0800  957) }, "audio-channel-changed", false);
f90fb59d (Ed Morley                 2012-12-06 11:02:50 +0000  966) }, "audio-channel-changed", false);
 is it normal?
You need to log in before you can comment on or make changes to this bug.