Closed Bug 836655 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:tef+, firefox20 wontfix, firefox21 wontfix, firefox22 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 verified)

VERIFIED FIXED
B2G C4 (2jan on)
blocking-b2g tef+
Tracking Status
firefox20 --- wontfix
firefox21 --- wontfix
firefox22 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- verified

People

(Reporter: mchen, Assigned: mchen)

References

Details

Attachments

(2 files, 10 obsolete files)

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.
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #708476 - Flags: review?(amarchesini)
Hi Andrea,

May I know any feedback of this bug?
Thanks.
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) ...
(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.
Yes, it is workable. Will submit next version tomorrow.
(In reply to Marco Chen [:mchen] from comment #5)
> Yes, it is workable. Will submit next version tomorrow.

Great! Thank you.
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #708476 - Attachment is obsolete: true
Attachment #708476 - Flags: review?(amarchesini)
Attachment #711704 - Flags: review?(amarchesini)
Attachment #711704 - Flags: review?(amarchesini) → review+
Attached patch Patch Checkin-Version (obsolete) — Splinter Review
1. All green on try server.
2. Add reviewer.
Attachment #711704 - Attachment is obsolete: true
Attachment #715029 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b093ba2c7ff9
https://hg.mozilla.org/mozilla-central/rev/0acbd06d48a9
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Attached patch unit test modifcation v1 (obsolete) — Splinter Review
1. Fix some error wording.
2. Test that an app can't be allowed to initially play on content channel from background state.
Attachment #715090 - Flags: review?(amarchesini)
Fix the warnings which will be treated as error on some compile option.
Attachment #715029 - Attachment is obsolete: true
Attachment #715091 - Flags: review+
Attachment #715090 - Flags: review?(amarchesini) → review+
https://tbpl.mozilla.org/?tree=Try&rev=c5bfac0ab33a

Try for all platform and with modified unit test.
Attached patch Patch v3 (obsolete) — Splinter Review
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)
Attachment #715091 - Attachment is obsolete: true
Attachment #716367 - Flags: review?(amarchesini)
Attached patch unit test modifcation v3 (obsolete) — Splinter Review
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.
Attachment #715090 - Attachment is obsolete: true
Attachment #716370 - Flags: review?(amarchesini)
Attachment #716370 - Flags: review?(amarchesini) → review+
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?
Attachment #716367 - Flags: review?(amarchesini) → review+
Attached patch Patch v4 (obsolete) — Splinter Review
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.
Attachment #716367 - Attachment is obsolete: true
Attachment #716404 - Flags: review?(amarchesini)
Attached patch Patch v5 (obsolete) — Splinter Review
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])
Attachment #716404 - Attachment is obsolete: true
Attachment #716404 - Flags: review?(amarchesini)
Attachment #716446 - Flags: review?(amarchesini)
Attached patch patch v6 (obsolete) — Splinter Review
Remove unnecessary file change in this patch.
Attachment #716446 - Attachment is obsolete: true
Attachment #716446 - Flags: review?(amarchesini)
Attachment #717014 - Flags: review?(amarchesini)
Comment on attachment 717014 [details] [diff] [review]
patch v6

Thanks for answering. Looks good.
Attachment #717014 - Flags: review?(amarchesini) → review+
Pass the try server and add reviewer name.
Attachment #717014 - Attachment is obsolete: true
Attachment #717768 - Flags: review+
Pass the try server and add reviewer name.
Attachment #716370 - Attachment is obsolete: true
Attachment #717769 - Flags: review+
Attachment #717769 - Attachment is patch: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1c8ec52deaf4
https://hg.mozilla.org/mozilla-central/rev/ec88008d4498
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Attachment #717768 - Flags: approval-mozilla-b2g18?
Attachment #717769 - Flags: approval-mozilla-b2g18?
Blocks: 828283
tef? because this patch is needed for 828283.
blocking-b2g: --- → tef?
blocking-b2g: tef? → tef+
Attachment #717768 - Flags: approval-mozilla-b2g18?
Comment on attachment 717769 [details] [diff] [review]
uni test Checkin-Version

marked tef+ for uplift, no need for approval.
Attachment #717769 - Flags: approval-mozilla-b2g18?
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.