Closed Bug 825674 Opened 7 years ago Closed 7 years ago

audio-channel-changed mozChrome event not dispatched correctly in all cases.

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-basecamp:+, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed)

RESOLVED FIXED
B2G C4 (2jan on)
blocking-basecamp +
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed

People

(Reporter: nhirata, Assigned: baku)

References

Details

(Whiteboard: [feedback])

Attachments

(1 file, 6 obsolete files)

## Environment :
Unagi phone, build 2012-12-31

## Repro :
1. install Solitaire from the Marketapp
2. turn the sound on in the option after launching the game (select menu -> option -> settings -> Package - Wood )
3. using the hardware control turn the sound all the way down

## Expected :
1. no sound occurs

## Actual :
1. you can still hear the sound and it's unaffected by the volume control.

## Note :
1. music app and video app work correctly.
2. from feedback
Detailed : work correctly meaning works with head phones as well as phone speakers.

Something to check also is probably browser, e.me app.

Radio is also affected by this bug.  ie Radio is not corresponding to hardware control volume.
Same with the Soundcloud app.
And the dialer, fwiw.
This depends on apps/system/js/sound_manager.js so this is likely a Gaia bug.
Component: General → Gaia::System
(In reply to Vivien Nicolas (:vingtetun) from comment #4)
> This depends on apps/system/js/sound_manager.js so this is likely a Gaia bug.

After investigation it seems like Gaia receive mozChromeEvent of type 'audio-channel-changed' which is good. But the value of those messages is always 'default' while it should be 'normal' when the sounds start and 'default' when it stops.

I've been told that Baiku is a good guy for that.

Also I'm cloning this bug for the second issue that lives here. The fact that once this bug is fixed it will be hard to change the volume of the Solitaire application since it is very short (but this one is a Gaia bug).
Component: Gaia::System → General
Andrea, can you look into this. See comment 0 and comment 5.

Can you check that we are firing the right audio-channel-changed when the audio starts and stops. Seems like we are not.
Assignee: nobody → amarchesini
The browser app is also affected; songs at html5.grooveshark.com play at full volume.
Attached patch patch for gaia (obsolete) — Splinter Review
Attachment #697384 - Flags: review?(alive)
Attached patch patch for AudioChannelService (obsolete) — Splinter Review
Here the patch for the AudioChannelService component.
I removed the 'default' audiochannel because this doesn't seem right. We don't have that audio channel type and the 'default' is called 'normal'.
Attachment #697385 - Flags: review?(jonas)
Comment on attachment 697384 [details] [diff] [review]
patch for gaia

r-.
Default is meaning 'no content playing' AFAIK.
We redefine it in gaia according to UX define to 'notification' channel.

When an audio tag isn't specified with mozAudioChannelType, it's `normal`.
The browser app doesn't have the permission to join competing so apparently any web page's audio element is in content channel.

What should we do to this is: Listening to audio.volume.content in gecko AudioManager when channel=normal? I think this is already done....

Or the root cause is we don't have audio-channel-changed event for channel=normal playing? I haven't investigated on this before...lemme try and reply again.
Attachment #697384 - Flags: review?(alive) → review-
Nope, this is by design...we need to know the channel is changed to 'nothing playing' now.
We have an event when something is start playing, why shouldn't we have another event when it stops?

(In reply to Andrea Marchesini (:baku) from comment #9)
> Created attachment 697385 [details] [diff] [review]
> patch for AudioChannelService
> 
> Here the patch for the AudioChannelService component.
> I removed the 'default' audiochannel because this doesn't seem right. We
> don't have that audio channel type and the 'default' is called 'normal'.
> We have an event when something is start playing, why shouldn't we have
> another event when it stops?

Ok. This can be a good solution.
I prefer to have an event like this and remove the 'default' audio channel.
Is it fine for you?
(In reply to Vivien Nicolas (:vingtetun) from comment #4)
> This depends on apps/system/js/sound_manager.js so this is likely a Gaia bug.

I don't think so...

Just as bug 825970 said, now it's hard to detect a sound which is playing very shortly.
Since we would like to adjust the volume when nothing is playing now.

The similar bug is that when an audio tag is changing src, the background audio is intermittently playing.
We may need some buffer in firing events in platform...but this is also not very smart solution I know.

No good idea on this :(
(In reply to Andrea Marchesini (:baku) from comment #12)
> > We have an event when something is start playing, why shouldn't we have
> > another event when it stops?
> 
> Ok. This can be a good solution.
> I prefer to have an event like this and remove the 'default' audio channel.
> Is it fine for you?

I am fine.
But this seems not resolve this bug. If I understand it correctly, this is what bug 825970 said..the audio is playing too short to let the system app know something is playing?
Confirmed. It's a very short audio when the card is flipped.
Hmmm....what could we do? Could we make the audio-channel-changed event firing on app iframe?
Or Could we pass the frame origin within the event?

(In reply to Alive Kuo [:alive] from comment #14)
> (In reply to Andrea Marchesini (:baku) from comment #12)
> > > We have an event when something is start playing, why shouldn't we have
> > > another event when it stops?
> > 
> > Ok. This can be a good solution.
> > I prefer to have an event like this and remove the 'default' audio channel.
> > Is it fine for you?
> 
> I am fine.
> But this seems not resolve this bug. If I understand it correctly, this is
> what bug 825970 said..the audio is playing too short to let the system app
> know something is playing?
Actually I would say that the current implementation of the volume control looks broken...
Think about this scenario:

1. no app running and the volume is set to 0 (muted). I see the muted icon on the system bar.

2. I open the solitario and I start playing

3. I move up the volume, the icon is still muted but I can hear sound...

I think this is wrong. what do you think?
Then, why the default audio channel is 'notification'? Should it be 'normal'?
Personally I think that the main volume should be the 'normal/content' channel.
If we do this we fix this bug and the bug 825970.

Is there a reason why the default audio channel for the volume manager is 'notification'?
(In reply to Andrea Marchesini (:baku) from comment #17)
> Personally I think that the main volume should be the 'normal/content'
> channel.
> If we do this we fix this bug and the bug 825970.
> 
> Is there a reason why the default audio channel for the volume manager is
> 'notification'?

It's by spec.
http://people.mozilla.com/~lco/FFOS/Sound/

It's reasonable that as an user I'd like to mute/control the volume of notification the phone before any notification/ringer comes up to me.
I am a heavy smartphone user I think. I use the volume rocker every time when I run into the meeting room or movie theater, by pressing the down button several times. I don't want to go to settings to find a settings then.

But this bug is exactly a bug from the spec...need info from UX.
Also let's see if Jonas has some thought for this?

Senario: A short playing audio wouldn't be controlled by hardware buttons.
Flags: needinfo?(lco)
The worst case is let's change both notification+content settings whenever content is playing or not..... (Except for voicecall or alarm)
(In reply to Alive Kuo [:alive] from comment #19)
> The worst case is let's change both notification+content settings whenever
> content is playing or not..... (Except for voicecall or alarm)

Right. I'm not a heavy smartphone user but I think this is the right approach.
Alive> We checked on Android and it's exactly like that: the "default" audio channel is for the "notification".
Also, on Android, there is an icon "muted" when the "notification" channel is muted.

I believe this is the source of inspiration for the spec.
(In reply to Julien Wajsberg [:julienw] from comment #22)
> Also, on Android, there is an icon "muted" when the "notification" channel
> is muted.
> 
> I believe this is the source of inspiration for the spec.

Do you know how Android deals with a short-time audio volume adjust?

I bet it has some buffer mechanism...but I am not sure.

I have a Google Nexus 7 which is android 4.2.1
It seems like this device's default channel is content. Because when I press volume buttons, the overlay tells me it is adjusting normal audio instead of notification volume.
However, when I get a mail or IM notification with a short sound, I could press the button even after the sound disappears to adjust the volume of notification.
I have a Galaxy Nexus with Android 4.2.1 as well. The default channel is definitely "notification".

It seems they have different behaviour on different devices.. which makes sense :-)

Here what I tried :
[a] - open Play Music, no audio is playing -> the volume buttons adjust the content audio
[a'] - open Play Music, audio is playing -> the volume buttons adjust the content audio
[b] - close Play Music when no audio is playing -> the volume buttons adjust the notification audio
[c] - close Play Music when audio is playing -> there is a notification item, and the volume buttons adjust the content audio
[d] - pause the audio from the notification -> the volume buttons adjust the content audio
[e] - remove the notification (there is a cross) -> the volume buttons adjust the notification audio

I noticed no delay at all in these use cases.

(note that notification and ringer seems bound)

The [a] case is what could make the short-time audio volume work: it seems logical to adjust the volume of the application currently in front, I'm not sure we're doing that now.

In your case with the Nexus 7, how do you adjust the notification volume then ? Is it only in the Settings ?
(In reply to Julien Wajsberg [:julienw] from comment #24)
> I have a Galaxy Nexus with Android 4.2.1 as well. The default channel is
> definitely "notification".
> 
> It seems they have different behaviour on different devices.. which makes
> sense :-)
> 
> Here what I tried :
> [a] - open Play Music, no audio is playing -> the volume buttons adjust the
> content audio
> [a'] - open Play Music, audio is playing -> the volume buttons adjust the
> content audio
> [b] - close Play Music when no audio is playing -> the volume buttons adjust
> the notification audio
> [c] - close Play Music when audio is playing -> there is a notification
> item, and the volume buttons adjust the content audio
> [d] - pause the audio from the notification -> the volume buttons adjust the
> content audio
> [e] - remove the notification (there is a cross) -> the volume buttons
> adjust the notification audio
> 
> I noticed no delay at all in these use cases.
> 
> (note that notification and ringer seems bound)
> 
> The [a] case is what could make the short-time audio volume work: it seems
> logical to adjust the volume of the application currently in front, I'm not
> sure we're doing that now.

Kind of..
We only know what channel of audio is playing, but don't know what app is using a channel.
So it's according to 'the highest priority' playing channel now.

> 
> In your case with the Nexus 7, how do you adjust the notification volume
> then ? Is it only in the Settings ?

You could interact with the volume overlay.
1. Press any hardware volume key.
2. A slide with a icon and a config button displays on center of screen.
3. Click the config button to change another type of volume if you want
(In reply to Alive Kuo [:alive] from comment #25)
> (In reply to Julien Wajsberg [:julienw] from comment #24)
> > I have a Galaxy Nexus with Android 4.2.1 as well. The default channel is
> > definitely "notification".
> > 
> > It seems they have different behaviour on different devices.. which makes
> > sense :-)
> > 
> > Here what I tried :
> > [a] - open Play Music, no audio is playing -> the volume buttons adjust the
> > content audio
> > [a'] - open Play Music, audio is playing -> the volume buttons adjust the
> > content audio
> > [b] - close Play Music when no audio is playing -> the volume buttons adjust
> > the notification audio
> > [c] - close Play Music when audio is playing -> there is a notification
> > item, and the volume buttons adjust the content audio
> > [d] - pause the audio from the notification -> the volume buttons adjust the
> > content audio
> > [e] - remove the notification (there is a cross) -> the volume buttons
> > adjust the notification audio
> > 
> > I noticed no delay at all in these use cases.
> > 
> > (note that notification and ringer seems bound)
> > 
> > The [a] case is what could make the short-time audio volume work: it seems
> > logical to adjust the volume of the application currently in front, I'm not
> > sure we're doing that now.
> 
> Kind of..
> We only know what channel of audio is playing, but don't know what app is
> using a channel.
> So it's according to 'the highest priority' playing channel now.

In the [a] case, it seems that if an app is in front (not the homescreen, not the dialer) then the content channel is used.

To know the exact algorithm we should check their source ;)


> 
> > 
> > In your case with the Nexus 7, how do you adjust the notification volume
> > then ? Is it only in the Settings ?
> 
> You could interact with the volume overlay.
> 1. Press any hardware volume key.
> 2. A slide with a icon and a config button displays on center of screen.
> 3. Click the config button to change another type of volume if you want


ok, this is nice. This does not happen on Galaxy Nexus.

-
(In reply to Alive Kuo [:alive] from comment #18)
> 
> But this bug is exactly a bug from the spec...need info from UX.
> Also let's see if Jonas has some thought for this?
> 
> Senario: A short playing audio wouldn't be controlled by hardware buttons.

How common is this scenario, exactly? And what do you mean by a "short" clip? (a 1-second one, or something longer?)

If it's just a single occurrence (i.e. a sound clips plays and then it's gone), then controlling volume hardly matters because the sound has already gone away anyway.

If the problem is a series of short sound clips in sequence, then I can't really provide a better solution than some kind of time buffer. But if this isn't really a common scenario, I don't think it's worth fixing, honestly.

Btw, this use case doesn't seem to be the main concern of the title of this bug. If installed apps in general aren't affected by the hardware volume control, that's a different issue than just a problem with short clips. 

I still think the current design of prioritizing ringer/notification volume over content volume in most cases is the right one. Here it is again with a couple of clarifications:

* If content is NOT playing, which is the most common case, toggling the hardware rocker controls notification+ringer volume. 

* When content is playing, toggling the hardware rocker controls content volume
** I'm not sure if we've implemented this, but there are some cases in which the user will exit the app but content audio continues to play. For example, the user exits the music player app but music continues to play in the background (this is described by case C in comment 24). In this case, toggling the hardware rocker should still control content volume.

Let me know if this matches your expectations and the current implementation.
Flags: needinfo?(lco)
The short-playing-audio is Bug 825970.

I disagree with one thing: I think that if we are eg in the music app with no music playing, toggling the hardware volume button should change the content volume... However I'd be ok with not having this for v1, but it would be best.
(In reply to Julien Wajsberg [:julienw] from comment #28)
> The short-playing-audio is Bug 825970.
> 

I installed and played the game in bug description.
The game plays a ~1 second audio when flipping the poker.

I think bug 825970 is just dupe of this bug.

> I disagree with one thing: I think that if we are eg in the music app with
> no music playing, toggling the hardware volume button should change the
> content volume... However I'd be ok with not having this for v1, but it
> would be best.
(In reply to Larissa Co from comment #27)
> (In reply to Alive Kuo [:alive] from comment #18)
> > 
> > But this bug is exactly a bug from the spec...need info from UX.
> > Also let's see if Jonas has some thought for this?
> > 
> > Senario: A short playing audio wouldn't be controlled by hardware buttons.
> 
> How common is this scenario, exactly? And what do you mean by a "short"
> clip? (a 1-second one, or something longer?)

Ya like you said.
I don't know how common it is..DTMF tone is one of the cases however.
https://bugzilla.mozilla.org/show_bug.cgi?id=784184

We are not able to adjust the DTMF volume(which is not a notification tone now) because it plays too shortly.

> 
> If it's just a single occurrence (i.e. a sound clips plays and then it's
> gone), then controlling volume hardly matters because the sound has already
> gone away anyway.
> 
> If the problem is a series of short sound clips in sequence, then I can't
> really provide a better solution than some kind of time buffer. But if this
> isn't really a common scenario, I don't think it's worth fixing, honestly.
> 
> Btw, this use case doesn't seem to be the main concern of the title of this
> bug. If installed apps in general aren't affected by the hardware volume
> control, that's a different issue than just a problem with short clips. 
> 
> I still think the current design of prioritizing ringer/notification volume
> over content volume in most cases is the right one. Here it is again with a
> couple of clarifications:
> 
> * If content is NOT playing, which is the most common case, toggling the
> hardware rocker controls notification+ringer volume. 
> 
> * When content is playing, toggling the hardware rocker controls content
> volume
> ** I'm not sure if we've implemented this, but there are some cases in which
> the user will exit the app but content audio continues to play. For example,
> the user exits the music player app but music continues to play in the
> background (this is described by case C in comment 24). In this case,
> toggling the hardware rocker should still control content volume.
> 
> Let me know if this matches your expectations and the current implementation.

These all are already done:)

Personally I'd like to implement a buffer in gaia:
* If the channel is changed to 'XXX' once but changed back to 'nothing-playing' state,
  "remember" the channel for 1~2 seconds. For volume rockers usage only.
* If any new channel happens to play in the time buffer, clear the buffered channel.

Andrea, I think we still need to confirm audio-channel-changed event is fired correctly on 'normal' channel case.
Could you make sure this? And tell me the event name that stands for 'nothing is playing now'.
(In reply to Julien Wajsberg [:julienw] from comment #28)
> The short-playing-audio is Bug 825970.
> 
> I disagree with one thing: I think that if we are eg in the music app with
> no music playing, toggling the hardware volume button should change the
> content volume... However I'd be ok with not having this for v1, but it
> would be best.

The problem is, how do we distinguish which apps get content volume toggled vs. the ringer volume? For example, if I'm in a PDF viewer app that has no tones whatsoever, why would toggling the hardware button in the app change content volume?
lco> just checked on my Android 4.2 phone; it does what you're describing. On any app without music, the hardware buttons toggle the notification channel. In the browser, they toggle the notification channel as well, unless it outputs some music (eg on http://html5.grooveshark.com), and then they toggle the content channel.

But in the Play Music app, they always toggle the content channel. That's not the same in their Play Mobie app.

My guess here is that the Play Music app opens the channel when starting. Or there is a special case. I'll try to find an app with short sounds to test that.
Alive, Larissa: Please take the discussion that you are currently having over to bug 825970. This bug is about a platform level problem. Bug 825970 is about the UI level problem of how to make it easier to adjust the volume for the content/normal channel.

Julien: Please file separate bugs if you think that the current policy for what channel the volume buttons affect is wrong.

Andrea: I might be misunderstanding what you are trying to do, but it doesn't seem to me like it'll fix the problem here.

The reason we have the current audio-channel-changed: "default" notification is to let gaia know that *no* audio channel is currently used. I think it would be nice to make that more clear by changing "default" to "none", but that's a separate issue as there is no ambiguity right now.

The reason we need to let gaia know that *no* audio channel is playing is so that gaia can choose which audio channel to adjust the volume for if no audio is currently playing. I.e. we want it to be a UI decision which channel to adjust the volume for if no audio is currently playing.


The problem here seems to be that when the solitaire app plays short audio clips, we do fire a few audio-channel-changed as expected. However all of those events are saying that the channel is "default" (or "none" as it should be named).

The expected behavior is that we first fire an event which says that we're now playing on the "normal" channel, and then another event should be fired when we stop, which indicates that we're now not playing any audio.

I.e. first we should fire a "audio-channel-changed" event for "normal" and then one for "audio-channel-changed" event for "default".
Summary: Installed apps do not get affected by the hardware volume control → audio-channel-changed mozChrome event not dispatched correctly in all cases.
Attached patch wip patch (obsolete) — Splinter Review
Can we can add a stream type like NONE?
blocking-basecamp: ? → +
Comment on attachment 697385 [details] [diff] [review]
patch for AudioChannelService

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

clearing request while waiting for response to comment 33
Attachment #697385 - Flags: review?(jonas)
Attached patch patch b (obsolete) — Splinter Review
This patch changes these things:

1. getMuted received the Agent
2. internally there is a new enum just to have NORMAL, NORMAL_HIDDEN, CONTENT and CONTENT_HIDDEN as different states
3. GetMuted (the internal one) receives the visibility of the element and the previous visibility of the same element. This is useful in order to know if the visibility has changed
4. visibility is stored in the hash table.

I think 'default' vs 'none' should be moved to a separated bug.
Attachment #697384 - Attachment is obsolete: true
Attachment #697385 - Attachment is obsolete: true
Attachment #698606 - Flags: review?(jonas)
Since the audio clip is short, is it possible that?
  1. solitaire app tried to registerType(normal).
   1.a registerType() called notify() to ask solitaire app to do getMuted().

  2. solitaire app unregisterType(normal), due to short audio clip.
  
  3. solitaire app call getMuted() which will check highest channel then send mozChrome event.
   3.a Due to step 2, here can not get normal as highest one. 

So sound manager can't know the short exist of normal channel.

Maybe we can send mozChrome event in registerType() just like in unregisterType() for avoiding this short transition.
>   3. solitaire app call getMuted() which will check highest channel then
> send mozChrome event.
>    3.a Due to step 2, here can not get normal as highest one. 

With the current patch, GetMuted() receives an AudioChannelAgent as first argument, so it's not possible to call GetMuted() after unregisterAgent (unregisterType).

But yes, it makes sense to send an event in registerType() as well. I'm uploading a new patch.
Thank you.
Attached patch patch c (obsolete) — Splinter Review
Attachment #698606 - Attachment is obsolete: true
Attachment #698606 - Flags: review?(jonas)
Attachment #698616 - Flags: review?(jonas)
(In reply to Alive Kuo [:alive] from comment #13)
> (In reply to Vivien Nicolas (:vingtetun) from comment #4)
> > This depends on apps/system/js/sound_manager.js so this is likely a Gaia bug.
> 
> I don't think so...

I change it back in comment 5 ;)
Comment on attachment 698616 [details] [diff] [review]
patch c

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

::: dom/audiochannel/AudioChannelService.cpp
@@ +160,5 @@
> +  return muted;
> +}
> +
> +bool
> +AudioChannelService::GetMuted(AudioChannelType aType, uint64_t aChildID,

Rename this to GetMutedInternal or some such. Otherwise it's easy to get confused about which function is getting called.

@@ +177,5 @@
>  
> +  // We are not visible, maybe we have to mute:
> +  switch (newType) {
> +    case AUDIO_CHANNEL_INT_NORMAL_HIDDEN:
> +      muted = true;

Rather than doing this as a switch-statement, I'd say it's cleaner to do:

if (newType == AUDIO_CHANNEL_INT_NORMAL_HIDDEN) {
  muted = true;
} else if (newType ==  AUDIO_CHANNEL_INT_CONTENT_HIDDEN) {
  ...
}

::: dom/audiochannel/AudioChannelService.h
@@ +66,5 @@
> +  void UnregisterType(AudioChannelType aType, bool aElementHidden,
> +                      uint64_t aChildID);
> +
> +  bool GetMuted(AudioChannelType aType, uint64_t aChildID,
> +                bool aElementHidden, bool aElementHasHidden);

aElementWasHidden. Or aOldElementHidden.
Attachment #698616 - Flags: review?(jonas) → review+
Attached patch patch (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=c5d74b81b2fa
the patch has been sent to try.
Marco can you take a quick look of this patch and give a feedback?
Attachment #697776 - Attachment is obsolete: true
Attachment #698616 - Attachment is obsolete: true
Attachment #699117 - Flags: review+
Attachment #699117 - Flags: feedback?(mchen)
Attached patch patchSplinter Review
https://tbpl.mozilla.org/?tree=Try&rev=607c05527d26
green on try.
Attachment #699117 - Attachment is obsolete: true
Attachment #699117 - Flags: feedback?(mchen)
Attachment #699217 - Flags: review+
Although it is a late feedback response, this patch looks great. I think the only know issue now is about the last app with content channels falling to background can play while no one in the foreground.
https://hg.mozilla.org/mozilla-central/rev/b3110cc62552
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → B2G C4 (2jan on)
You need to log in before you can comment on or make changes to this bug.