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

RESOLVED FIXED

Status

Firefox OS
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: shawnjohnjr, Assigned: mchen)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-birch])

Attachments

(1 attachment, 3 obsolete attachments)

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.
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Marco,
We have some discussion before, do you have any patch can help this bug? Can you provide it for review?
Blocks: 807758
(Assignee)

Comment 2

5 years ago
Created attachment 721161 [details] [diff] [review]
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.
Attachment #721161 - Flags: feedback?(amarchesini)
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) {
  ...
Attachment #721161 - Flags: feedback?(amarchesini) → feedback+
Marco, is 1.5 sec acceptable?
(Assignee)

Comment 5

5 years ago
Created attachment 728820 [details] [diff] [review]
Patch v2

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

Thanks for the review and comment.
Attachment #721161 - Attachment is obsolete: true
Attachment #728820 - Flags: review?(amarchesini)
(Assignee)

Updated

5 years ago
Summary: [Bluetooth]Sometimes A2DP Start streaming failed due to music resume too fast → [Bluetooth/AudioChannel] A2DP failed to start session due to music resume too fast during SCO is disconnecting.
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
Attachment #728820 - Flags: review?(amarchesini) → review-
(Assignee)

Comment 7

5 years ago
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.
(Assignee)

Comment 8

5 years ago
Created attachment 736202 [details] [diff] [review]
Patch v4

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

Thanks for the review.
Assignee: nobody → mchen
Attachment #728820 - Attachment is obsolete: true
Attachment #736202 - Flags: review?(amarchesini)
(Assignee)

Updated

5 years ago
Attachment #736202 - Attachment is patch: true
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...
}
Attachment #736202 - Flags: review?(amarchesini) → review-
(Assignee)

Comment 10

5 years ago
(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.
(Assignee)

Updated

5 years ago
Flags: needinfo?(amarchesini)
> 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.
Flags: needinfo?(amarchesini)
Ignore my last 2 comments :)
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.
Attachment #736202 - Flags: review- → review+
(Assignee)

Comment 15

5 years ago
Created attachment 742909 [details] [diff] [review]
Patch Checkin-Version

1. Rebase.
2. Add reviewer name.
Attachment #736202 - Attachment is obsolete: true
Attachment #742909 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Whiteboard: [fixed-in-birch]
https://hg.mozilla.org/mozilla-central/rev/1be592ac3c8d
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
blocking-b2g: --- → koi?
Unclear why this was nommed for koi, this landed long back based on comment #17 and should be there in 1.2.
blocking-b2g: koi? → ---
You need to log in before you can comment on or make changes to this bug.