855208 - [A2DP/AVRCP] Audio disappeared after playing 1 second if Bluetooth AVRCP play command received by Music

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

when issueing Bluetooth AVRCP play command via system message, Music application cannot be launched.

Comment 1

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

STR:
1. Send AVRCP play command without launching Music application previously
2. Music played for 1 second
3. Music audio immediately stopped
(Resume music can resolve it).

Comment 2

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Marco, Randy,
As discussed, it looks like design problem of audio module. Please help to follow up this issue.

Comment 3

[Marco Chen [:mchen]]

Hi all,

The policy of audio channel is
  1. Content Channel playing in the foreground can be set into the active ID list.
  2. Content channel in the background can be allowed to play only if it is in the active ID 
     list.
  3. Content channel playing in the background will be stopped and removed from active ID 
     list when another content channel playing in the foreground.
  4. Content channel paused in the background will never be resumed until it goes to foreground.

This issue in this bug is
  In this bug, music app is lunched by system message and kept on background only. Thus this content channel can't be allowed to play in the background because it doesn't in the active ID list.

Possible solution but have side effect:
  1. Audio channel allows content channel to be played in the background when there is no any other content channel on playing.
  2. Side effect is that content channel paused in the background will be resumed after no others on playing and this obey policy 4 above.

Comment 4

[Andrea Marchesini [:baku]]

> Possible solution but have side effect:

In the new AudioChannel policy we have:

  // When called, if this app is currently attempting to play audio on the "content"
  // channel, but is non-visible and the "content" channel is muted for this app, calling
  // this function will make the system pick this app or tab as the one to provide content audio.
  // If this function is called and there is another visible app or tab which uses the "content"
  // channel, then this function is a no-op.
  void startBackgroundContentAudio();

Can this fix this issue?

Comment 5

[Marco Chen [:mchen]]

(In reply to Andrea Marchesini (:baku) from comment #4)
>   void startBackgroundContentAudio();
> 
> Can this fix this issue?

Thanks for the suggestion first.
Is the scenario of this function with this bug like steps as below?

1. BT headset sends play command via AVRCP.
2. Gecko receives the command then send system message out.
3. Music app is lunached in the background.
4. Music app find out that It is in the background and receives play from AVRCP.
5. Music app plays an audio but that will be paused by audio channel service.
6. Music app call startBackgroundContentAudio() and this will let audio be resumed.

It sounds be an option. Thanks Andrea.

-----------------------------------------------

I just think that if any app who want to start an audio from background need to call additional function call "startBackgroundContentAudio()". It seems to be an extra trouble for app.

Comment 6

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

The startBackgroundContentAudio() function isn't meant for apps themselves to use. The intent is that the system app uses this function. So the system app can detect when only one background app is attempting to play audio on the "content" channel and then calls startBackgroundContentAudio() to let that app play audio.

If multiple background apps are trying to use the "content" channel, and no forground app is, then the system app can choose which app should get to play by calling startBackgroundContentAudio() on that app.

Comment 7

[Marco Chen [:mchen]]

(In reply to Jonas Sicking (:sicking) from comment #6)

I agreed that startBackgroundContentAudio() would be controlled by system app not each app.

So how about this?
  1. AudioChannelAgent::StartPlaying() will call AudioChannelService::RegisterAudioChannelAgent().
  2. In that timing, if the following conditions are all true then AudioChannelService will set this child id into mActiveContentChildIDs.
    a. There is no any app playing in the foreground.
    b. There is no any app allowed to play in the background.
  3. Then this app can be allowed to play in the background.

For these apps in the backgound are paused by AudioChannelService, they still can't play anything because their child ID are not in mActiveContentChildIDs.

Comment 8

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

I would be ok with that.

The downside is that gaia won't be able to fully choose policy since if we automatically turn on the audio gaia likely won't be able to turn it off without there being a short blip of sound.

However if we leave the sound off by default when a background app starts audio, then gaia can immediately detect that and turn it on.

But I don't know if there are any scenarios where gaia wouldn't want to turn on the sound automatically in the scenario you are describing.

Comment 9

[Marco Chen [:mchen]]

(In reply to Jonas Sicking (:sicking) from comment #8)
> I would be ok with that.
> 
> The downside is that gaia won't be able to fully choose policy since if we
> automatically turn on the audio gaia likely won't be able to turn it off
> without there being a short blip of sound.
> 
> However if we leave the sound off by default when a background app starts
> audio, then gaia can immediately detect that and turn it on.

Even gaia detected it's paused by AudioChannelService, currently it has no way to enable audio. Except that we want to create a new Web API for "app" to control policy. Is it neccesary?
(I proposed that if gaia uses AudioChannelManager::occupyChannel() then we can allow it to disable default audio policy in gecko and response to channel status by itself.)

> But I don't know if there are any scenarios where gaia wouldn't want to turn
> on the sound automatically in the scenario you are describing.

I think since gaia app doesn't want to play music "when it is starting from background" then it should not call audio.play() actually not expect playing but paused by AudioChannelService.

Comment 10

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

(In reply to Marco Chen [:mchen] from comment #9)
> Even gaia detected it's paused by AudioChannelService, currently it has no
> way to enable audio.

That's what the startBackgroundContentAudio function is for. It will enable gaia to turn on the audio.

> Except that we want to create a new Web API for "app"
> to control policy. Is it neccesary?

I'm not sure I follow?

> > But I don't know if there are any scenarios where gaia wouldn't want to turn
> > on the sound automatically in the scenario you are describing.
> 
> I think since gaia app doesn't want to play music "when it is starting from
> background" then it should not call audio.play() actually not expect playing
> but paused by AudioChannelService.

What I'm saying is that generally speaking the more of the UX logic that we can put in gaia, including in the system app, the better. And the types of audio policies that we are talking about here is UX.

Right now too much logic is happening in gecko which is why we came up with the API in bug 853101. The API there could allow the system app to implement multiple audio policies. Including gaia could implement the audio policy that you are suggesting in comment 7.

Comment 11

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

Marco: I feel like we are talking past each other here a bit.

I agree that the audio policy in comment 7 is one we should implement. I.e. if a background app A is started and starts playing audio on the "content" channel, and no other foreground or background apps are playing audio on the "content" channel, then we should make the audio from A be the audio that plays.

We have two ways to implement this:

1) Either gecko could implement this by having AudioChannelService detect that only one childID is using the "content" channel and so letting that childID play audio.

2) Let the system app, i.e. gaia, use the audio API from bug 853101 to detect as soon as a background app starts using the audio channel, and then call startBackgroundContentAudio() on that app.

In neither case does the app itself do anything. It's just a question of how much logic we put in gecko and how much logic we put in gaia. This allows for more flexibility if we want to change the policy. So 2 is better from this perspective.

However in this case I don't think that we're going to change the policy very much. So I would be fine with using solution 1.

Comment 12

[Marco Chen [:mchen]]

I also agreed that Gaia can invole more audio policy to let it flexable.

I think if there is an good UX scenario for this issue with startBackgroundContentAudio() called by system app then I would prefer that one too. I just have one concern about that startBackgroundContentAudio() is controll by system app. Thus if there is step needed user to confirm play or not then it will be trouble when device in pocket or bag.

I would expect the UX can allow user triggers play then he/she can enjoy the music right away.

Thanks for your suggestions in this bug. Then maybe we need UX to help here.

Comment 13

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

(In reply to Marco Chen [:mchen] from comment #12)
> I just have one concern about that startBackgroundContentAudio() is
> controll by system app. Thus if there is step needed user to confirm play or
> not then it will be trouble when device in pocket or bag.

I don't understand what you mean by this. The system app can always call startBackgroundContentAudio() without there being a prompt to the user.

Comment 14

[Marco Chen [:mchen]]

(In reply to Jonas Sicking (:sicking) from comment #13)
> I don't understand what you mean by this. The system app can always call
> startBackgroundContentAudio() without there being a prompt to the user.

Hi Jonas, 

I wrote comment 12 when I saw comment 10 only. So I missed what you said on comment 11. (Why don't I get conflict warning when I posted it.) And thanks for your advice and input here. I will try to solve it on solution 1 and I like new APIs can bring audio channel policy more flexibility too.

Comment 15

[Bruce Sun [:brsun]]

Attachment: Allow one background content channel to to be competitive during audio type registeration.

This patch changes the competing behavior of background content channel as below:
1. When one background content channel registers its audio type and there is no foreground content channel at the same time, this background content channel can play its audio.
2. When one foreground content channel changes itself into a background content channel and there is no other foreground content channel at the same time, this background content channel can play its audio.
3. When one foreground content channel registers its audio type, all background content channels should be muted.
4. When one background content channel changes itself into a foreground content channel, all background content channels should be muted.
5. When one content channel unregisters its audio type, muted/unmuted state of any other content channel keeps the same.

Comment 16

[Bruce Sun [:brsun]]

Attachment: bug855208-background-content-channel.v2.patch

This patch is based on the previous one with the following changes:
 - Allow a background content channel to playback if there is a foreground content channel with the same ChildID.
 - Sync the testing program with current behavior.

Comment 17

[Bruce Sun [:brsun]]

One question about UX behavior.

If we already have one content channel playing in background, what would be the proper behavior when another background content channel trying to playback? Let the original one keeps playing or let the new one to playback?

ps. "bug855208-background-content-channel.v2.patch" mutes the original one and let the new one to playback.

Comment 18

[Marco Chen [:mchen]]

Comment on attachment 804412 [details] [diff] [review]
Allow one background content channel to to be competitive during audio type registeration.

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

::: dom/audiochannel/AudioChannelService.h
@@ +157,5 @@
>  
>    AudioChannelType mCurrentHigherChannel;
>    AudioChannelType mCurrentVisibleHigherChannel;
>  
> +  // One hidden content channel will become competitive:

I think the unit of competitive is based on child ID not a content channel.
Maybe we can change the comment to

"A background process contained content channel(s) will become competitive when:"

@@ +162,5 @@
> +  //   1. When this hidden content channel registers in AudioChannelService and
> +  //   there is no other content channel in the foreground.
> +  //   2. When this content channel goes from foreground into background and
> +  //   there is no other content channel in the foreground.
> +  // One competitive hidden content channel will become non-competitive:

"A background process contained content channel(s) will become non-competitive when:"

@@ +164,5 @@
> +  //   2. When this content channel goes from foreground into background and
> +  //   there is no other content channel in the foreground.
> +  // One competitive hidden content channel will become non-competitive:
> +  //   1. When there is a foreground content channel registers in
> +  //   AudioChannelService.

According to the current design, AudioChannelService::RegisterType always registers a agent from background status. So there is no chance for first condition and we can remove this comment. (Except that we modified the AudioChannelAgent to bring in hidden status when calling registerType().)

@@ +166,5 @@
> +  // One competitive hidden content channel will become non-competitive:
> +  //   1. When there is a foreground content channel registers in
> +  //   AudioChannelService.
> +  //   2. When there is a content channel goes from background into foreground.
> +  //   3. When its process unregisters all hidden content channels.

Even background process unregisters all it's content channel, we still reserve it's permission so it can continue to play next song. (This should be removed)

@@ +168,5 @@
> +  //   AudioChannelService.
> +  //   2. When there is a content channel goes from background into foreground.
> +  //   3. When its process unregisters all hidden content channels.
> +  //   4. When its process shuts down.
> +  uint64_t mCompetitiveHiddenContentChildID;

The name is too long. ^_^

Comment 19

[Bruce Sun [:brsun]]

(In reply to Marco Chen [:mchen] (PTO, 09/16, 09/18~09/22) from comment #18)
> Comment on attachment 804412 [details] [diff] [review]
> Allow one background content channel to to be competitive during audio type
> registeration.
> 
> Review of attachment 804412 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/audiochannel/AudioChannelService.h
> @@ +157,5 @@
> >  
> >    AudioChannelType mCurrentHigherChannel;
> >    AudioChannelType mCurrentVisibleHigherChannel;
> >  
> > +  // One hidden content channel will become competitive:
> 
> I think the unit of competitive is based on child ID not a content channel.
> Maybe we can change the comment to
> 
> "A background process contained content channel(s) will become competitive
> when:"
> 
> @@ +162,5 @@
> > +  //   1. When this hidden content channel registers in AudioChannelService and
> > +  //   there is no other content channel in the foreground.
> > +  //   2. When this content channel goes from foreground into background and
> > +  //   there is no other content channel in the foreground.
> > +  // One competitive hidden content channel will become non-competitive:
> 
> "A background process contained content channel(s) will become
> non-competitive when:"

Thanks. The original comment was really not good. I will refine these comments as advised.

> 
> @@ +164,5 @@
> > +  //   2. When this content channel goes from foreground into background and
> > +  //   there is no other content channel in the foreground.
> > +  // One competitive hidden content channel will become non-competitive:
> > +  //   1. When there is a foreground content channel registers in
> > +  //   AudioChannelService.
> 
> According to the current design, AudioChannelService::RegisterType always
> registers a agent from background status. So there is no chance for first
> condition and we can remove this comment. (Except that we modified the
> AudioChannelAgent to bring in hidden status when calling registerType().)

This comment describes the design logic how and when a background process with content channels should be muted. Although an agent is always treated as hidden during AuioChannelService::RegisterType(), this implementation trick might be changed depends on how the real visibility state of the corresponding process can be synchronized to AudioChannelAgent and AudioChannelService. But no matter we treat a process as hidden by default or not, the logic of how to compete content channels between foreground and background process should be self-contained. I would suggest to add more wordings to describe about the current implementation. (ex. 1. When there is a foreground content channel registers in AudioChannelService. (But by current design, any process is treated as hidden during registration, so this condition is never satisfied.))
What do you think?

> 
> @@ +166,5 @@
> > +  // One competitive hidden content channel will become non-competitive:
> > +  //   1. When there is a foreground content channel registers in
> > +  //   AudioChannelService.
> > +  //   2. When there is a content channel goes from background into foreground.
> > +  //   3. When its process unregisters all hidden content channels.
> 
> Even background process unregisters all it's content channel, we still
> reserve it's permission so it can continue to play next song. (This should
> be removed)

If one background process unregisters all its content channels, that would mean it doesn't want to playback any content channel. If this process wants to playback next song in the background, or any other process wants to playback any content channel in the background, they must register itself again and try to grant the permission. And if there is no foreground process which contains content channels, the latest one should be able to play its content channel. So any one background process has its chance to grant the permission already.
I think the open question here is whether we should block others from granting the permission or not even if there is no background content channel playing. And the feedback from UX might affect how we handle it.

> 
> @@ +168,5 @@
> > +  //   AudioChannelService.
> > +  //   2. When there is a content channel goes from background into foreground.
> > +  //   3. When its process unregisters all hidden content channels.
> > +  //   4. When its process shuts down.
> > +  uint64_t mCompetitiveHiddenContentChildID;
> 
> The name is too long. ^_^

I will try to name it with a shorter one. :)

Comment 20

[Marco Chen [:mchen]]

(In reply to Bruce Sun [:brsun] from comment #19)

> What do you think?
> 

Yes, we can comment the logical and real implementation for reader to understand what audiochannelservice did. Thanks.

> I think the open question here is whether we should block others from
> granting the permission or not even if there is no background content
> channel playing. And the feedback from UX might affect how we handle it.
> 
You are right.

Comment 21

[Marco Chen [:mchen]]

Comment on attachment 806391 [details] [diff] [review]
bug855208-background-content-channel.v2.patch

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

Hi Andrea,

This patch tried to remove two member variables from AudioChannelService and I think it makes sense.
According to these two are added by you, it would be better to review by you also.
Could you help this?
(This is not a final patch so I transfer it to you via feedback only.)

thanks,

nsTArray<uint64_t> mActiveContentChildIDs;		// One hidden content channel will become competitive:
bool mActiveContentChildIDsFrozen;

Comment 22

[Carrie Wang [New account: [e-mail]]]

(In reply to Bruce Sun [:brsun] from comment #17)
> One question about UX behavior.
> 
> If we already have one content channel playing in background, what would be
> the proper behavior when another background content channel trying to
> playback? Let the original one keeps playing or let the new one to playback?
> 
> ps. "bug855208-background-content-channel.v2.patch" mutes the original one
> and let the new one to playback.

Hi Bruce, from UX perspective, we'd suggest that the later one could replace the former one. 
(Because the user just took action by AVRCP, they would expect some reaction from the new one.)
Thanks!

Comment 23

[Bruce Sun [:brsun]]

Attachment: bug855208-background-content-channel.v3.patch

This patch is based on bug855208-background-content-channel.v2.patch with the following modification:
 1. Correct comments to describe that the unit of the audio competiting policy is the whole process instead of each signle content channel.
 2. Add comments to describe that each registered audio type is treated as hidden by default.
 3. Name the variable that stores the playable content ChildID a shorter one.

Comment 24

[Marco Chen [:mchen]]

Comment on attachment 812539 [details] [diff] [review]
bug855208-background-content-channel.v3.patch

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

Looks good and I think there is less thing we need to clarify only.
Near to the target. Thanks.

::: dom/audiochannel/AudioChannelService.cpp
@@ +127,5 @@
>        mWithVideoChildIDs.AppendElement(aChildID);
>      }
>  
> +    // One hidden content channel can be playable only when there is no other
> +    // content channels in the foreground.

s/no other content channels/no any content channel/

@@ +292,5 @@
>    if (newType == AUDIO_CHANNEL_INT_NORMAL_HIDDEN ||
>        (newType == AUDIO_CHANNEL_INT_CONTENT_HIDDEN &&
> +       !(mChannelCounters[AUDIO_CHANNEL_INT_CONTENT].Contains(aChildID) ||
> +         (mChannelCounters[AUDIO_CHANNEL_INT_CONTENT].IsEmpty() &&
> +          mPlayableHiddenContentChildID == aChildID)))) {

Could you add the comment for the rules here?
ex" when audio with content channel moving from foreground to background
         1. if there still is any audio in the foreground for this child ID then it should be playable because this process is under transition from foreground to background.

@@ +465,2 @@
>               mChannelCounters[AUDIO_CHANNEL_INT_CONTENT_HIDDEN].Contains(
> +             mPlayableHiddenContentChildID)) {

Do we have any chance that mPlayableHiddenContentChildID has a valid child ID but actually this ID didn't in mChannelCounters[AUDIO_CHANNEL_INT_CONTENT_HIDDEN]?

Or we just need to check whether mPlayableHiddenContentChildID has a valid value or not.

::: dom/audiochannel/AudioChannelService.h
@@ +173,5 @@
> +
> +  // mPlayableHiddenContentChildID stores the ChildID of the process which can
> +  // play content channel(s) in the background.
> +  // A background process contained content channel(s) will become playable:
> +  //   1. When this background process and registers its content channel(s) in

s/and//

Comment 25

[Bruce Sun [:brsun]]

(In reply to Marco Chen [:mchen] (PTO, 09/16, 09/18~09/22) from comment #24)
> Could you add the comment for the rules here?
> ex" when audio with content channel moving from foreground to background
>          1. if there still is any audio in the foreground for this child ID
> then it should be playable because this process is under transition from
> foreground to background.

No problem.

> @@ +465,2 @@
> >               mChannelCounters[AUDIO_CHANNEL_INT_CONTENT_HIDDEN].Contains(
> > +             mPlayableHiddenContentChildID)) {
> 
> Do we have any chance that mPlayableHiddenContentChildID has a valid child
> ID but actually this ID didn't in
> mChannelCounters[AUDIO_CHANNEL_INT_CONTENT_HIDDEN]?
> 
> Or we just need to check whether mPlayableHiddenContentChildID has a valid
> value or not.

You are right. Checking mPlayableHiddenContentChildID is enough. Because as long as mPlayableHiddenContentChildID has valid value, it implies that this child id is in mChannelCounters[AUDIO_CHANNEL_INT_CONTENT_HIDDEN] already.

Comment 26

[Bruce Sun [:brsun]]

Attachment: bug855208-background-content-channel.v4.patch

This patch is based on attachment 812539 [details] [diff] [review] with the following modification:
 1. Correct wordings of comments.
 2. Add comments to describe how AUDIO_CHANNEL_STATE_MUTED is returned while checking content channels of one process the during transition from foreground to background.
 3. Remove redundant conditions while checking whether there is some playable content channels of any background process.

Comment 27

[Bruce Sun [:brsun]]

The try results of attachment 813425 [details] [diff] [review] are all green: https://tbpl.mozilla.org/?tree=Try&rev=485220bf2f7c.

Comment 28

[Marco Chen [:mchen]]

Comment on attachment 813425 [details] [diff] [review]
bug855208-background-content-channel.v4.patch

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

Great Job. Thanks.

Comment 29

[Bruce Sun [:brsun]]

Attachment: bug855208-background-content-channel.check-in.patch

This patch can resolve this bug. Check-in needed.

Comment 30

[Eric Chou [:ericchou] [:echou]]

https://hg.mozilla.org/integration/b2g-inbound/rev/ec2efae11376

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ec2efae11376

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8d2350db842c