836655 - [Audio] Volume control from HW keys will be kept to content channel after play a song in Music App.

Status: VERIFIED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Marco Chen [:mchen]]

Reproducing Steps:

1. To adjust audio volume by HW keys and it will be adjusted on ringer/notification.
2. To adjust audio volume by HW keys after playing a song by music app. And it will be on content/normal channel.
2. Volume control will be kept on content/normal channel until killing the music app from card view.

Comment 1

[Marco Chen [:mchen]]

Attachment: Patch v1

The root cause is that AudioChannelService::UnregisterType() doesn't handle the change on mActiveContentChildIDs when type is content channel.

So to add the code for checking whether this childID is in mActiveContentChildIDs then remove it if some conditions are true.

Comment 2

[Marco Chen [:mchen]]

Hi Andrea,

May I know any feedback of this bug?
Thanks.

Comment 3

[Andrea Marchesini [:baku]]

Comment on attachment 708476 [details] [diff] [review]
Patch v1

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

Sorry for the delay. I'm not 100% sure this patch is implementing what we really want.

::: dom/audiochannel/AudioChannelService.cpp
@@ +135,5 @@
> +    // We need to check whether this unregister will affect mActiveContentChildIDs.
> +    // Then the event of channel changed can be fired with correct value.
> +    if (aType == AUDIO_CHANNEL_CONTENT &&
> +      mActiveContentChildIDs.Contains(aChildID)) {
> +      if ((mActiveContentChildIDsFrozen &&

I'm not totally convinced that this is what we want.
Think about this scenario:

1. we have a music player that is playing.
2. we lock the device, and the music player becomes invisible (mActiveContentChildIDs contains the music player ID and the mActiveContentChildIDsFrozen is set to true)
3. the music player changes song and doing that it stops and starts an HTMLMediaElement.

With this patch we removes the ID from the list of Active ContentChilds, so the audio player will be muted if there is a stop and start.

Probably what we want is:

if (aType == AUDIO_CHANNEL_CONTENT && !mActiveContentChildIDsFrozen && mActiveContentChildIDs.Contains(aChildID) ...

Comment 4

[Marco Chen [:mchen]]

(In reply to Andrea Marchesini (:baku) from comment #3)
> 1. we have a music player that is playing.
> 2. we lock the device, and the music player becomes invisible
> (mActiveContentChildIDs contains the music player ID and the
> mActiveContentChildIDsFrozen is set to true)
> 3. the music player changes song and doing that it stops and starts an
> HTMLMediaElement.
> 
> With this patch we removes the ID from the list of Active ContentChilds, so
> the audio player will be muted if there is a stop and start.

Thanks for your finding here. I think you are right for the regression you mentioned.

> Probably what we want is:
> 
> if (aType == AUDIO_CHANNEL_CONTENT && !mActiveContentChildIDsFrozen &&
> mActiveContentChildIDs.Contains(aChildID) ...

And I think you add a condition of |!mActiveContentChildIDsFrozen| is want to avoid the removing of childID in the test case above. So music play can continue to play from song to next song.

But once the music player in the background already finished the last song. In your suggestion, that childID will be exist until killing music player. Then volume control will be kept on content channel too.

So in the current design now, maybe we need to do that
  1. Keep the last childID in mActiveContentChildIDs even there is no any audio played by it.
     (so this can satisfy your test case.)
  2. Add a more condition at link below by checking whether there is any agent exist for active child ID in addition.
     (so we can notify the current audio channel is "none", when music app in the background after finishing last song)
     http://mxr.mozilla.org/mozilla-central/source/dom/audiochannel/AudioChannelService.cpp#294

How about this?
Thanks.

Comment 5

[Marco Chen [:mchen]]

Yes, it is workable. Will submit next version tomorrow.

Comment 6

[Andrea Marchesini [:baku]]

(In reply to Marco Chen [:mchen] from comment #5)
> Yes, it is workable. Will submit next version tomorrow.

Great! Thank you.

Comment 7

[Marco Chen [:mchen]]

Attachment: Patch v2

Two modification here.

1. Removing ChildID from Active Content List if it is in the foreground. If ChildID in the background we kept it for allowing it to play next song.

2. For event of audio-channel-changed, when there is ChildID in the background and Active Content List, we will fire event only when there is any content channel belonged to this ChildID under playing.

Comment 8

[Marco Chen [:mchen]]

Wait for try server.

https://tbpl.mozilla.org/?tree=Try&rev=da07d79734fa

Comment 9

[Marco Chen [:mchen]]

Attachment: Patch Checkin-Version

1. All green on try server.
2. Add reviewer.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Pushed bustage fix as well, since it triggered warnings:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0acbd06d48a9

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/b093ba2c7ff9
https://hg.mozilla.org/mozilla-central/rev/0acbd06d48a9

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And backed out for a failing unit test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/524e7bc67431
https://hg.mozilla.org/mozilla-central/rev/524e7bc67431

Comment 14

[Marco Chen [:mchen]]

Attachment: unit test modifcation v1

1. Fix some error wording.
2. Test that an app can't be allowed to initially play on content channel from background state.

Comment 15

[Marco Chen [:mchen]]

Attachment: Patch Checkin-Version with fixing warning

Fix the warnings which will be treated as error on some compile option.

Comment 16

[Marco Chen [:mchen]]

https://tbpl.mozilla.org/?tree=Try&rev=c5bfac0ab33a

Try for all platform and with modified unit test.

Comment 17

[Marco Chen [:mchen]]

Attachment: Patch v3

The difference between v3 and reviewed is tried to solve the issue happened on unit test with case as below;
  Test Case:
     1. content channel is playing in the foreground.
     2. content channel in playing is moving to background. (mActiveContentChildIDsFrozen = true) 
     3. content channel in playing is moving to foreground. 
        (issue here: Frozen flag didn't be set to false)
     4. content channel stops to play.
        (due to issue on step 3, it's child ID doesn't be removed from list)

Comment 18

[Marco Chen [:mchen]]

Attachment: unit test modifcation v3

Issue Description:
  The current design of Agent class can't be used to test the transition of content channel between foreground & background.

Root Cause:
  When test sets different visibility state to Agent, it will call Agent::StartPlaying() to get PlayState. But actually this API will call StopPlaying() first for avoiding double registration, this causes the agent stops the old one and register a new into AudioChannelService and not notify the transition between fore/background to AudioChannelService.

Comment 19

[Andrea Marchesini [:baku]]

Comment on attachment 716367 [details] [diff] [review]
Patch v3

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

the patch looks good. Can you fix the comment or explain better why mActiveContentChildIDs[0] is good enough? Thanks.

::: dom/audiochannel/AudioChannelService.cpp
@@ +304,5 @@
> +    // And need to check whether there is any content channels under playing
> +    // now.
> +    else if (!mActiveContentChildIDs.IsEmpty() &&
> +             !mChannelCounters[AUDIO_CHANNEL_INT_CONTENT_HIDDEN].Contains(
> +             mActiveContentChildIDs[0])) {

why just the first item of the array? In theory, we should check all of them, is it?

Comment 20

[Marco Chen [:mchen]]

Attachment: Patch v4

Thanks for the question.

First, I think we have conclusion about only one app can play audio in the background. So we can expect that if there is no content channel played in the foreground then there is only one childID can be allowed to in the background. This is why we directly use index - 0 to do check.

And according this question, I add modification on agent doing transition from foreground to background. I removed childID from list if there is another one still playing in the foreground. So finally there is only one can be allowed to play in the background.

Comment 21

[Marco Chen [:mchen]]

Attachment: Patch v5

The difference between v4 & v5 is at SendAudioChannelChangedNotification().

!mChannelCounters[AUDIO_CHANNEL_INT_CONTENT_HIDDEN].Contains(
             mActiveContentChildIDs[0])

It should be 

mChannelCounters[AUDIO_CHANNEL_INT_CONTENT_HIDDEN].Contains(
             mActiveContentChildIDs[0])

Comment 22

[Marco Chen [:mchen]]

Attachment: patch v6

Remove unnecessary file change in this patch.

Comment 23

[Andrea Marchesini [:baku]]

Comment on attachment 717014 [details] [diff] [review]
patch v6

Thanks for answering. Looks good.

Comment 24

[Marco Chen [:mchen]]

Attachment: Patch Checkin-Version

Pass the try server and add reviewer name.

Comment 25

[Marco Chen [:mchen]]

Attachment: uni test Checkin-Version

Pass the try server and add reviewer name.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c8ec52deaf4
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec88008d4498

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1c8ec52deaf4
https://hg.mozilla.org/mozilla-central/rev/ec88008d4498

Comment 28

[Andrea Marchesini [:baku]]

tef? because this patch is needed for 828283.

Comment 29

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 717769 [details] [diff] [review]
uni test Checkin-Version

marked tef+ for uplift, no need for approval.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/6505e0b9632b
https://hg.mozilla.org/releases/mozilla-b2g18/rev/9906f9c093f3

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/ffa0ced5e6ff
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/9829eccdee59

Comment 31

[Angela Hubenya]

This issue is no longer reproducing. Volume bar is displayed properly whether the music app is killed or not.  
Changing status Verified

Unagi
Build ID: 20130313070202
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/e74dafa6b2d9
Gaia: b34e726147f8e671ad8c538b50900ccfbffcb084