Closed Bug 811222 Opened 9 years ago Closed 9 years ago

Enable system app to get currently used audio channel

Categories

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

x86
macOS
defect

Tracking

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

RESOLVED FIXED
B2G C2 (20nov-10dec)
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed
firefox20 --- fixed

People

(Reporter: sicking, Assigned: baku)

References

Details

Attachments

(1 file, 4 obsolete files)

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? :)
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.
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
Attached patch patch (obsolete) — Splinter Review
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!
> * 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
(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?
Attached patch patch b (obsolete) — Splinter Review
Attachment #684400 - Attachment is obsolete: true
Attachment #687210 - Flags: review?(jonas)
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-
> The review comments from the previous review doesn't appear to be addressed.

Yeah... I forgot to save the changes in mq.
Attached patch patch c (obsolete) — Splinter Review
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-
Attached patch patch d (obsolete) — Splinter Review
Attachment #687525 - Attachment is obsolete: true
Attachment #688217 - Flags: review?(jonas)
Attached patch patch dSplinter Review
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
Closed: 9 years ago
Resolution: --- → FIXED
Jonas, can you please land this on aurora too?
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.