832665 - Switching back and forth between the music app and FM radio (using home key) causes some unexpected behavior.

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Music, defect, P1)

Description

[Nivi]

These are the steps to reproduce the issue seen -
1. Insert wired headset to the phone.
2. Play Music. 
3. Press Home Key. Music plays on the homescreen.
4. Open FM. FM music plays.
5. Press home key and go back to music and play. Music resumes playing.
6. Again press home key in music screen.
7. Now the music doesn't play in the homescreen (as expected similar to step 3)

Expected beahvior: Music should play even after pressing home key.

Also, killing the FM app seems to fix the problem. Music starts playing in the homescreen. Seems like FM app is causing some issues here.

Comment 1

[Johnny Stenback (:jst)]

Andrea, can you have a look here?

Comment 2

[Marco Chen [:mchen]]

The current design will mute all of content channels if there are more then two apps with content channels are playing in the background. If the behavior is want to kept last one (fall to background) be continue to play, then we need to modify AudioChannelService.

Comment 3

[Michael Vines [:m1] [:evilmachines]]

If this is WAD then closing as wontfix seems fine.  This bug was simply raised because the observed behavour was unexpected and seems buggy

Comment 4

[Marco Chen [:mchen]]

Hi Casey,

Could you help to clarify the UX behavior in this case?

Thanks.

Comment 5

[Andrea Marchesini [:baku]]

(In reply to Michael Vines [:m1] from comment #3)
> If this is WAD then closing as wontfix seems fine.  This bug was simply
> raised because the observed behavour was unexpected and seems buggy

That Marco said is right. The current implementation mutes all the content channels when in background if there are more then two apps playing.

I would suggest this new behaviour:

1. Play Music. 
2. Press Home Key. Music plays on the homescreen.
3. Open FM. FM music plays.
4. Press home key. FM plays on the homescreen.
5. I stop/close/kill FM
6. No music is played
7. Open Music. Music resumes playing.

So, the idea is: a content channel plays in background but it's muted if some other app using a content channel starts playing and it doesn't resume, until it becomes visible again.

Comment 6

[Casey Yee]

(In reply to Andrea Marchesini (:baku) from comment #5)
> So, the idea is: a content channel plays in background but it's muted if
> some other app using a content channel starts playing and it doesn't resume,
> until it becomes visible again.

This sounds like a good solution and exactly what I would recommend :)


In this scenario:
> 1. Play Music. 
> 2. Press Home Key. Music plays on the homescreen.
> 3. Open FM. FM music plays.
> 4. Press home key. FM plays on the homescreen.
> 5. I stop/close/kill FM
> 6. No music is played
> 7. Open Music. Music resumes playing.

I would make it so that the user in Step 7 have to manually resume music so that it doesn't surprise the user when arriving back with music.

Comment 7

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 8

[Marco Chen [:mchen]]

Comment on attachment 704962 [details] [diff] [review]
patch

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

Thanks for taking the consideration in advance.

::: dom/audiochannel/AudioChannelService.cpp
@@ +174,5 @@
> +  // If the audio content channel is visible, let's remember this ChildID.
> +  if (newType == AUDIO_CHANNEL_INT_CONTENT &&
> +      oldType == AUDIO_CHANNEL_INT_CONTENT_HIDDEN &&
> +      !mActiveContentChildIDs.Contains(aChildID)) {
> +

As I knew that there will be only one app (also means one childID) in the foreground but it can have more then one media elements (agents). Since child id will be checked here then there is only one will be added into mActiveContentChildIDs. If this assumption is correct, do we need to use nsTArray or just an uint64_t mActiveContentChildID?

@@ +418,5 @@
>        }
> +
> +      while ((index = mActiveContentChildIDs.IndexOf(childID)) != -1) {
> +        mActiveContentChildIDs.RemoveElementAt(index);
> +      }

Even there are more then one child id in the foreground, line 205 still limits only one will be in mActiveContentChildIDs for each child id. So do we need to use while loop here?

Comment 9

[Andrea Marchesini [:baku]]

> As I knew that there will be only one app (also means one childID) in the
> foreground but it can have more then one media elements (agents). Since
> child id will be checked here then there is only one will be added into
> mActiveContentChildIDs. If this assumption is correct, do we need to use
> nsTArray or just an uint64_t mActiveContentChildID?

I think more than 1 app can be visible. The keyboard is an example.

> Even there are more then one child id in the foreground, line 205 still
> limits only one will be in mActiveContentChildIDs for each child id. So do
> we need to use while loop here?

Yes, because we add the childID just once.

BTW: I would like to use the appID instead childID, but this is a separated bug.

Comment 11

[Marco Chen [:mchen]]

(In reply to Andrea Marchesini (:baku) from comment #9)
> 
> I think more than 1 app can be visible. The keyboard is an example.
> 

Oh, right. Thanks for the reminding.

> > Even there are more then one child id in the foreground, line 205 still
> > limits only one will be in mActiveContentChildIDs for each child id. So do
> > we need to use while loop here?
> 
> Yes, because we add the childID just once.

Since childID is added just once, could we remove the while loop?

Comment 12

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 13

[Marco Chen [:mchen]]

Comment on attachment 705802 [details] [diff] [review]
patch

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

Looks great.

Comment 14

[Johnny Stenback (:jst)]

I tried landing this, but stuff has changed enough for this not to apply cleanly any more :(

Andrea, can you merge this and land on inbound and b2g18 asap?

Comment 15

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a0c4559f5d2e

For inbound I need to have 830648 829561 830672 landed.

Comment 16

[Ed Morley [:emorley]]

Backed out for cppunitest orange:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=a0c4559f5d2e

https://hg.mozilla.org/integration/mozilla-inbound/rev/6007dc902385

Comment 17

[Andrea Marchesini [:baku]]

Attachment: patch

The tests were not updated.

Comment 18

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5197fd796303

Comment 19

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/9ba470b353d2

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5197fd796303

Comment 21

[Sam Shipley]

Issue no longer reproduces on the Unagi device

Build ID: 20130214070203
December 5th Kernel
Gaia: 6544fdb8dddc56f1aefe94482402488c89eeec49
Gecko http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/d1288313218e

Switching between the music app and the FM radio app is flawless