811222 - Enable system app to get currently used audio channel

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P1)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Andrea or Marco, who of you should take this? :)

Comment 2

[Andrea Marchesini [:baku]]

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.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Firing a mozChromeEvent on the system app should be enough.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: patch

This patch is based on the patches for 805333

Comment 5

[(inactive after 6/18) Alive Kuo [:alive]]

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!

Comment 6

[Andrea Marchesini [:baku]]

> * 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 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 8

[(inactive after 6/18) Alive Kuo [:alive]]

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!

Comment 9

[Andrew Overholt [:overholt]]

Setting priority based on triage discussions.  Feel free to decrease priority if you disagree.

Comment 10

[Andrea Marchesini [:baku]]

(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?

Comment 11

[Andrea Marchesini [:baku]]

Attachment: patch b

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 13

[Andrea Marchesini [:baku]]

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

Yeah... I forgot to save the changes in mq.

Comment 14

[Andrea Marchesini [:baku]]

Attachment: patch c

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 16

[Andrea Marchesini [:baku]]

Attachment: patch d

Comment 17

[Andrea Marchesini [:baku]]

Attachment: patch d

merge problem

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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();

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Landed on mozilla-central and mozilla-beta:

https://hg.mozilla.org/mozilla-central/rev/137d80a07e64
https://hg.mozilla.org/releases/mozilla-beta/rev/d8192e31580a

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Jonas, can you please land this on aurora too?

Comment 21

[(inactive after 6/18) Alive Kuo [:alive]]

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.

Comment 22

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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...

Comment 23

[(inactive after 6/18) Alive Kuo [:alive]]

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?

Comment 24

[(inactive after 6/18) Alive Kuo [:alive]]

Another, I don't get an event after content playing is ended. The channel should change to 'default'.

Comment 25

[Marco Chen [:mchen]]

(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?)

Comment 26

[Marco Chen [:mchen]]

(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.

Comment 27

[(inactive after 6/18) Alive Kuo [:alive]]

(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...

Comment 28

[Marco Chen [:mchen]]

(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.

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c07a72d3f235

Comment 30

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(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.

Comment 31

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(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.

Comment 32

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

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?