847255 - [Bluetooth/AudioChannel] A2DP failed to start session due to music resume too fast during SCO is disconnecting.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

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

STR:
1. Playing music via a2dp
2. Make an incoming call
3. Answer that call (SCO connection is established)
4. End the call (SCO connection disconnecting)
5. Music resumes (AVDTP start stream, SCO connection is disconnected)

With many a2dp headsets, we can see this problem.
We notice resuming music is too fast, this leads A2DP starts streaming happened before SCO connection really get disconnected.
There are many A2DP headsets cannot accept start streaming if SCO connection state is connected. This creates start streaming fail. Remote headset might send AVDTP state "BAD STATE" in return.
It would be better if call ended, music player resume can be delayed 1~2 seconds. Otherwise, it would be hard to handle start streaming in bad state. A2DP connection might need to disconnect/reconnect to recover. On Android, we might not see this problem easily because the speed of resuming music is slower.

Comment 1

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

Marco,
We have some discussion before, do you have any patch can help this bug? Can you provide it for review?

Comment 2

[Marco Chen [:mchen]]

Attachment: patch v1

Hi Andrea,

According to that gecko resume the music automatically after end of phone call, in case of A2DP & SCO enabled this resume will trigger A2DP connection during SCO is in de-connection state. That causes the fail of A2DP connection. Since we can't modify bluez stack I proposed to delay 1 second of unregistering telephony channel. Could you give some advice for this? Thanks.

Comment 3

[Andrea Marchesini [:baku]]

Comment on attachment 721161 [details] [diff] [review]
patch v1

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

I think this is fine. Waiting for a complete review request :)

::: dom/audiochannel/AudioChannelService.cpp
@@ +124,5 @@
>  AudioChannelService::UnregisterType(AudioChannelType aType,
>                                      bool aElementHidden,
>                                      uint64_t aChildID)
>  {
> +  if (aType == AUDIO_CHANNEL_TELEPHONY) {

I would do:

if (aType == AUDIO_CHANNEL_TELEPHONY &&
    (mContentChannels[AUDIO_CHANNEL_INT_TELEPHONY_HIDDEN] + mContentChannels[AUDIO_CHANNEL_INT_TELEPHONY]) == 1 &&
    XRE_GetProcessType() == GeckoProcessType_Default) {
  ...

Comment 4

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

Marco, is 1.5 sec acceptable?

Comment 5

[Marco Chen [:mchen]]

Attachment: Patch v2

1. Follow the suggestion from Comment 3.
2. Follow the timeout value from BT folk.

Thanks for the review and comment.

Comment 6

[Andrea Marchesini [:baku]]

Comment on attachment 728820 [details] [diff] [review]
Patch v2

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

Give me a feedback about this new approach.

::: dom/audiochannel/AudioChannelService.cpp
@@ +129,5 @@
> +  // 1. User can have time to remove device from his ear before music resuming.
> +  // 2. Give BT SCO to be disconnected before starting to connect A2DP.
> +  if (aType == AUDIO_CHANNEL_TELEPHONY &&
> +      (mChannelCounters[AUDIO_CHANNEL_INT_TELEPHONY_HIDDEN].Length() +
> +      mChannelCounters[AUDIO_CHANNEL_INT_TELEPHONY].Length()) == 1 &&

indentation should probably be:

if (aType == AUDIO_CHANNEL_TELEPHONY &&
    (mChannelCounters[AUDIO_CHANNEL_INT_TELEPHONY_HIDDEN].Length() +
     mChannelCounters[AUDIO_CHANNEL_INT_TELEPHONY].Length()) == 1 &&
    XRE_GetProcessType() == GeckoProcessType_Default) {

@@ +134,5 @@
> +      XRE_GetProcessType() == GeckoProcessType_Default) {
> +    if (mDeferTelChannelTimer) {
> +      mDeferTelChannelTimer->Cancel();
> +      mDeferTelChannelTimer = nullptr;
> +      UnregisterTypeInternal(aType, mTimerElementHidden, mTimerChildID);

I think we should remove this line.
Think about this workflow:

1. telephony channel A starts
2. telephony channel A stops - timer starts
3. telephony channel B starts
4. telephony channel B stops (no timer, because we still have A registered)
5. timeout channel A stops immediately after.

I think we should do this:

if (XRE_GetProcessType() == GeckoProcessType_Default &&
    aType == AUDIO_CHANNEL_TELEPHONY &&
    (mChannelCounters[AUDIO_CHANNEL_INT_TELEPHONY_HIDDEN].Length() +
     mChannelCounters[AUDIO_CHANNEL_INT_TELEPHONY].Length()) == 1) {
  MOZ_ASSERT(!mDeferTelChannelTimer);

  .. here the timer..
}

Then in RegisterType, after the registration:

if (XRE_GetProcessType() == GeckoProcessType_Default &&
    aType == AUDIO_CHANNEL_TELEPHONY && mDeferTelchannelTimer) {
  mDeferTelChannelTimer->Cancel();
  mDeferTelChannelTimer = nullptr;
  UnregisterTypeInternal(aType, mTimerElementHidden, mTimerChildID);
}

@@ +136,5 @@
> +      mDeferTelChannelTimer->Cancel();
> +      mDeferTelChannelTimer = nullptr;
> +      UnregisterTypeInternal(aType, mTimerElementHidden, mTimerChildID);
> +    }
> +    mTimerElementHidden = aElementHidden;

I would store the aType too.

@@ +148,5 @@
> +}
> +
> +void
> +AudioChannelService::UnregisterTypeInternal(AudioChannelType aType,
> +                                    bool aElementHidden,

Indentation

::: dom/audiochannel/AudioChannelService.h
@@ +76,5 @@
>    void RegisterType(AudioChannelType aType, uint64_t aChildID);
>    void UnregisterType(AudioChannelType aType, bool aElementHidden,
>                        uint64_t aChildID);
> +  void UnregisterTypeInternal(AudioChannelType aType, bool aElementHidden,
> +                      uint64_t aChildID);

Indentation

Comment 7

[Marco Chen [:mchen]]

Good catch, Andrea. Thanks for the suggestion and will update in next patch.

> I would store the aType too.
I think this is no necessary, because we only perform this work on telephony channel only.

Comment 8

[Marco Chen [:mchen]]

Attachment: Patch v4

1. Follow reviewr's comment.
2. Add NS_IMPL_ISUPPORTS2(AudioChannelService, nsIObserver, nsITimerCallback)

Thanks for the review.

Comment 9

[Andrea Marchesini [:baku]]

Comment on attachment 736202 [details] [diff] [review]
Patch v4

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

It looks good but you should add something in updateChannelType() too:

1. if the visibility of the last TELEPHONY element changes, you should track it

if (mDeferTelChannelTimer && aChildID == mDeferChildID && aType == TELEPHONY) {
  mTimerElementHidden = the new visibility...
}

Comment 10

[Marco Chen [:mchen]]

(In reply to Andrea Marchesini (:baku) from comment #9)
> Comment on attachment 736202 [details] [diff] [review]
> Patch v4
> 1. if the visibility of the last TELEPHONY element changes, you should track it

Hi Andrea,

Thanks for the review.
And the assignment of mTimerElementHidden is happened when the last telephony channel is |out of life cycle| already. So even that child ID is still alive, it is no chance to update its visiblity because that AudioChannelAgent already called .StopPlaying(). Or do I miss something? Thanks.

Comment 11

[Andrea Marchesini [:baku]]

> And the assignment of mTimerElementHidden is happened when the last
> telephony channel is |out of life cycle| already. So even that child ID is
> still alive, it is no chance to update its visiblity because that
> AudioChannelAgent already called .StopPlaying(). Or do I miss something?
> Thanks.

My only concern is that, maybe in the future a MediaElement can be set to telephony channel.
If this happens, the visibility will be change the internal type from INT_TELEPHONY to INT_TELEPHONY_HIDDEN.

I prefer to cover this case too. If you can just add that check in the updateState() method, the rest of the patch looks perfect.

In general, the AudioChannelService should not know how the rest of the system works. That object should be quite independent and currently it still is.

Comment 12

[Andrea Marchesini [:baku]]

Ignore my last 2 comments :)

Comment 13

[Andrea Marchesini [:baku]]

Comment on attachment 736202 [details] [diff] [review]
Patch v4

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

you are right, the visibility cannot change at that point.

Comment 14

[Marco Chen [:mchen]]

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

Comment 15

[Marco Chen [:mchen]]

Attachment: Patch Checkin-Version

1. Rebase.
2. Add reviewer name.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/1be592ac3c8d

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1be592ac3c8d

Comment 18

[bhavana bajaj [:bajaj]]

Unclear why this was nommed for koi, this landed long back based on comment #17 and should be there in 1.2.