823273 - [B2G][Music] Music volume does not change when a user receives a notification.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: AudioChannel, defect)

Description

[Cody Roesch [:croesch]]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/20100101 Firefox/17.0
Build ID: 20121128204232

Steps to reproduce:

1. Launch build 20121217070202 Version 18.0
2. Launch the Music app and play any song.
3. While the song is playing, send that phone a notification such as a text message.
4. When the phone receives the notification, notice that the volume level does not change so that the user can hear the notification coming in.

According to test case 3703 the volume level should fade temporarily so the user can hear the notification coming in.

Repro: 100%


Actual results:

When receiving a notification on the phone while music was playing, the user does not hear the volume fade so the notification audio is heard.


Expected results:

Music volume level should fade so that the user can hear the notification come in while listening to music.

Comment 1

[Marco Chen [:mchen]]

This is the result after landing of Bug 828200 - Incall video playback doesn't work.
In that bug, we decided to let app in foreground can always play the audio and assume that another audio source will be made by an app which takes the foreground.

An possible way is to make a new Web API for notifying the foreground app there is an channel of higher priority fired by another app. So foreground app can fade it's volume down then back. (Or do it in the Gecko.)

Comment 2

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

Maybe we should make an exception for the notification channel since that's expected to be short-lived?

An ideal solution might be to do something crazy like "turn down volume by 75% for all other channels in foreground apps when alarm or notification channel is playing. And don't adjust it at all when telephony channel is playing". But that seems too complicated for now.

Comment 3

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

Nominating in any case since we're failing a user story.

Comment 4

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

We need to get an answer from someone on which user story to ignore and/or tweak here.

Comment 5

[Lucas Adamski [:ladamski]]

Peter, which user story is the correct one here?

Comment 6

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

So I think the two realistic options we have here is:

Keep current behavior. This means changing the user story which requires the current app to be silenced while the notification is playing.

Change behavior so that we silence the audio of the current app when the "notification" or "alarm" channels are used, but not when the "telephony" channel is used. I.e. use the old behavior for the "notification" and "alarm" channels, but use the new behavior for the "telephony" channel.

I don't know how much work this would be. Andrea, mchen, what do you think?

Comment 7

[Casey Yee]

(In reply to Jonas Sicking (:sicking) from comment #2)
> An ideal solution might be to do something crazy like "turn down volume by
> 75% for all other channels in foreground apps when alarm or notification
> channel is playing. And don't adjust it at all when telephony channel is
> playing". But that seems too complicated for now.

This was the intended UX that I had in mind when writing the user story for sound.

Comment 8

[Marco Chen [:mchen]]

Attachment: Patch v1 - Faded Version

This is a version for fading the volume of content/normal channels in foreground when alarm/notification channels are fired in the background.

I think fallback to comment 6 is more easy so check the faded version first.
Thanks.

Comment 9

[Marco Chen [:mchen]]

The rough idea is

  1. AudioChannelService/Agent:
    a. To change getMuted() to return three states - normal, faded and mute.
    b. In service, it return faded state in getMuted() when
      -> checking content is normal/content in the foreground.
      -> There are notification/alarm channels in playing.

  2. nsHTMLMediaElement:
    When faded stat is returned, reducing the volume by 75% and hook setVolume/setMuted related functions for checking faded state.

  3. Camera/Phone:
    Just apply the API change from AudioChannelAgent.

  4. FM Radio:
    Still mute the FM in faded state.

Comment 10

[Marco Chen [:mchen]]

Attachment: Part 1: Audio Channel

Comment 11

[Marco Chen [:mchen]]

Attachment: Part 2: IPC

Comment 12

[Marco Chen [:mchen]]

Attachment: Part 3: media elment

Comment 13

[Marco Chen [:mchen]]

Attachment: Part 4: FM Radio, Camera, Phone Clients

Provide separated patches on different domain for easing the review process.

Comment 14

[Marco Chen [:mchen]]

Comment on attachment 704484 [details] [diff] [review]
Part 3: media elment

Hi Matthew,

Could you help to give a feedback on this part of patch?
The target here is want to reduce the volume when AudioChannelService asks a channel to fade it sound temporarily.

Thanks.

Comment 15

[Peter Dolanjski [:pdol]]

Fading the volume to hear the incoming notification was the original user story, however it was deemed out of scope at some point.

I don't think we should block on this.

Comment 16

[Andrea Marchesini [:baku]]

Comment on attachment 704482 [details] [diff] [review]
Part 1: Audio Channel

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

::: dom/audiochannel/AudioChannelService.cpp
@@ +88,5 @@
>                                                 AudioChannelType aType)
>  {
>    AudioChannelAgentData* data = new AudioChannelAgentData(aType,
>                                                            true /* mElementHidden */,
> +                                                          AUDIO_CHANNEL_STATE_MUTED /* mMuted */);

mElementHidden => aElementHidden
mMuted => aState

@@ +141,1 @@
>  AudioChannelService::GetMuted(AudioChannelAgent* aAgent, bool aElementHidden)

I think we should rename GetMuted() in GetState()

@@ +146,3 @@
>    }
>  
> +  AudioChannelState muted = GetMutedInternal(data->mType, CONTENT_PARENT_UNKNOWN_CHILD_ID,

GetStateInternal()

@@ +188,3 @@
>    }
>  
> +  if (muted == AUDIO_CHANNEL_STATE_NORMAL) {

if (muted != AUDIO_CHANNEL_STATE_MUTED)

@@ +191,3 @@
>      MOZ_ASSERT(newType != AUDIO_CHANNEL_INT_NORMAL_HIDDEN);
> +    if (ChannelsActiveWithHigherPriorityThan(newType)) {
> +      muted = AUDIO_CHANNEL_STATE_MUTED;

Does it make sense to use CheckVolumeFededCondition() here too?

@@ +207,5 @@
> +
> +  // Consider that audio from notification & alarm channels are with short duration
> +  // so just faded the volume not pause it
> +  if (mChannelCounters[AUDIO_CHANNEL_INT_NOTIFICATION].IsEmpty() &&
> +      mChannelCounters[AUDIO_CHANNEL_INT_ALARM].IsEmpty()) {

What about Public Notification?

Comment 17

[Andrea Marchesini [:baku]]

Comment on attachment 704483 [details] [diff] [review]
Part 2: IPC

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

::: dom/audiochannel/AudioChannelServiceChild.h
@@ +35,5 @@
>  
>    /**
>     * Return true if this type + this mozHidden should be muted.
>     */
> +  virtual AudioChannelState GetMuted(AudioChannelAgent* aAgent, bool aMozHidden);

GetMuted => GetState

Comment 18

[Cody Roesch [:croesch]]

In Unagi build 20130130070201 version 18.0, I'm finding that I only get audio to lower or mute if the headphones are in. If the headphones are NOT in the device, then the volume will not change if i get a text message or a phone call.

Build ID: 20130130070201
Kernel: Dec 5
Gecko  http://hg.mozilla.org/releases/mozilla-b2g18/rev/4593f3e765eb
Gaia   f7f5a0cd17e3d04308cc5850b254947e127122b9

In testrun 4 Fails case
https://moztrap.mozilla.org/runtests/run/712/env/296/?pagenumber=1&pagesize=100&sortfield=order&sortdirection=asc&filter-id=3703&filter-suite=124

UCID music-026

Comment 19

[Cody Roesch [:croesch]]

Unagi Build: 20130304070201
Gecko  http://hg.mozilla.org/releases/mozilla-b2g18/rev/bccf111e9131
Gaia   77fb74ebf3bea340be9f8f4b4c9ac9dd7c12ab0b
Kernel: Dec 5th

In this latest build, the functionality happens exactly as comment 18 describes. When no headphones are present, the music does not become quiet so that the user can hear the notification. Headphones work fine when the music decreases for a second so the incoming notification can be heard.

Comment 20

[Cody Roesch [:croesch]]

Inari Build: 20130418070206
Gecko  http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/0c76ef5f8677
Gaia   64d5096e1746bd4b08bc1bf69844d164ac961970
Kernel:  Feb 21st


For the Inari device build above, the same exact behavior occurs as described in comment 19 occurs.

Comment 21

[Jeni]

Issue repros on 

Leo Build ID: 20130610070206
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/8e3f39363c54
Gaia: ce3b99781d182ad550a325206990c249b0dbcf0e
Platform Version: 18.0
 
Description of issue is the same as comment 19

Comment 22

[John Hammink]

Not blocking on this.

Comment 23

[Sarah Parsons]

Attachment: logcat

Comment 24

[Jeni]

Issue repros on 

Leo Build ID: 20130625070217
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/29933d1937db
Gaia: 1436e2778b90bd74635b0b94d1cf8ccb0d71b60c
Platform Version: 18.1

When headphones are being used music will pause and play notification and then resume music.  Meanwhile if no head phones are used music will continue to play at same volume with no pauses or fading out.

Comment 25

[John Hammink]

Nothing has changed here.

Gecko  http://hg.mozilla.org/releases/mozilla-b2g18/rev/282b5c37cf8d
Gaia   e2ef782119b7e79fc62c48d36f0c36909d982988
BuildID 20130712070210
Version 18.0

What is the correct behavior?  Can you Peter, please clarify what is supposed the correct behavior in this case?

Comment 26

[John Hammink]

Nomming for Koi as this would be a good feature to have - if in fact there is actually a feature here.

Comment 27

[Peter Dolanjski [:pdol]]

(In reply to John Hammink from comment #25)
> What is the correct behavior?  Can you Peter, please clarify what is
> supposed the correct behavior in this case?

I believe it currently works as expected given the original user story was deemed out of scope for 1.0.1 and never re-raised for 1.1.  It is a good suggestion to add it to the backlog.  I have added it into the Systems team backlog.

Comment 28

[Ivan Tsay (:ITsay)]

Nominate it to koi+ and remove the dependency on 976631 to keep this feature moving on for v1.2

Comment 29

[Ivan Tsay (:ITsay)]

Correct the comment 28 about dependency. It should be 876631

Comment 30

[Marco Chen [:mchen]]

Today I rebase the patches here to newest master version and fix some bugs.
Now I am modifying the test case.

Comment 31

[Marco Chen [:mchen]]

Attachment: v2 Part 1 : AudioChannel

Hi Baku,

Thanks for your review first and in this patch I did

  1. Change getMuted to getState.
  2. And states will be normal, muted and faded.
  3. Only consider the faded state when notification channel is playing.
  4. CheckVolumeFededCondition() will be checked when agent is on fg or bg state.

Comment 32

[Marco Chen [:mchen]]

Attachment: v2 Part 2 : IPC

Hi Baku,

1. GetMuted => GetState

Comment 33

[Marco Chen [:mchen]]

Attachment: v2 Part 3 : Media Element

Hi kinetik,

Please help to review this version. Thanks.

Comment 34

[Marco Chen [:mchen]]

Attachment: v2 Part 4 : FM Radio, Camera, Phone Clients

Hi Randy,

Please help to review this part. Thanks.

Comment 35

[Marco Chen [:mchen]]

Attachment: v2 Part 5: Update Test Case

Comment 36

[Andrea Marchesini [:baku]]

Comment on attachment 798454 [details] [diff] [review]
v2 Part 1 : AudioChannel

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

lgtm

::: dom/audiochannel/nsIAudioChannelAgent.idl
@@ +44,5 @@
>    const long AUDIO_AGENT_CHANNEL_PUBLICNOTIFICATION = 6;
>  
>    const long AUDIO_AGENT_CHANNEL_ERROR              = 1000;
>  
> +  const long AUDIO_AGENT_STATE_NORMAL = 0;

indent :)

Comment 37

[Marco Chen [:mchen]]

Attachment: v3 Part 5: Update Test Case

Fixing the compile error on OS X 10.7 Opt build.

Comment 38

[Marco Chen [:mchen]]

Attachment: v2 Part 1 : AudioChannel - Checkin Version

Carry the reviewer's name.

Comment 39

[Marco Chen [:mchen]]

Attachment: v2 Part 2 : IPC - Checkin Version

Carry reviewer's name.

Comment 40

[Marco Chen [:mchen]]

Attachment: v2 Part 3 : Media Element - Checkin Version

Carry reviewer's name.

Comment 41

[Marco Chen [:mchen]]

Attachment: v2 Part 4 : FM Radio, Camera, Phone Clients - Checkin Version

Carry reviewer's name.

Comment 42

[Marco Chen [:mchen]]

Attachment: v3 Part 5: Update Test Case - Checkin Version

Carry reviewer’s name.

Comment 43

[Marco Chen [:mchen]]

Attachment: v2 Part 1 : AudioChannel - Checkin Version

Fix nit in comment 36

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/fd8ca2b2cbdb
https://hg.mozilla.org/integration/b2g-inbound/rev/7b4046fcaf53
https://hg.mozilla.org/integration/b2g-inbound/rev/583edf914fd7
https://hg.mozilla.org/integration/b2g-inbound/rev/39408bd9b813
https://hg.mozilla.org/integration/b2g-inbound/rev/813902236d3a

A reminder - *please* use commit messages that describe what each individual patch is doing rather than repeating the bug summary 5x.

Comment 45

[Marco Chen [:mchen]]

Right. Thanks Ryan.
I didn't my patch name into commit messages. Will take care next time.

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fd8ca2b2cbdb
https://hg.mozilla.org/mozilla-central/rev/7b4046fcaf53
https://hg.mozilla.org/mozilla-central/rev/583edf914fd7
https://hg.mozilla.org/mozilla-central/rev/39408bd9b813
https://hg.mozilla.org/mozilla-central/rev/813902236d3a

Comment 47

[sarsenyev]

The issue still reproduces on the latest Buri 1.2 Aurora MOZ RIL & Leo 1.1 Moz RIL
When receiving a notification while music is playing the music doesn't fade out when receiving the notification.
On Buri 1.2 Aurora build, the music getting a little quieter for a second, after the notification is received, but doesn't fade out completely.

But meanwhile, the issue doesn't reproduce when the device is connected to headphones, the music fades out when receiving notifications

Device:  Buri v1.2 Mozilla RIL
BuildID: 20131015004002
Gaia: 94e3088c56c7348655ae76613defd126426cc722
Gecko: f80fd27d367f
Version: 26.0a2
Firmware Version: US_20130912

Device: Leo V1.1 Mozilla RIL)
BuildID: 20131015041203
Gaia: 680f3b86b1e4ff1411ece6ba397b8b0e56b4b31c
Gecko: 4bfd6a51cd05
Version: 18.0

Comment 48

[Jason Smith [:jsmith]]

Marco - Can you clarify if what we're seeing in comment 47 is a followup bug from this patch or not?

Comment 49

[Marco Chen [:mchen]]

(In reply to sarsenyev from comment #47)
> The issue still reproduces on the latest Buri 1.2 Aurora MOZ RIL & Leo 1.1
> Moz RIL

This is a V1.2 feature so Leo 1.1 didn't have this effect.

> On Buri 1.2 Aurora build, the music getting a little quieter for a second,
> after the notification is received, but doesn't fade out completely.
> 

The effect here is just "reduce the volume level to 0.25 of current one" not a completely fade out to no sound.
So I think "getting a little quieter for a second" is a expected behavior.

> But meanwhile, the issue doesn't reproduce when the device is connected to
> headphones, the music fades out when receiving notifications

This is not done by Gecko implementation but did by vendor's audio HAL.
In order to notify user about notification event, audio will be output to both of speaker and headset.
And in order to avoid other content leak to speaker, the channel small then notification will be "muted".

The conclusion is that that are all under expectation.

Comment 50

[Jason Smith [:jsmith]]

Marking as verified per the above comment then.