Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 0abdf53e authored by Zhihai Xu's avatar Zhihai Xu
Browse files

Failure to start playback on A2DP sink after connection 

This is what happen:
after Headset is connected, we call start_audio_datapath which will send AVDTP_Start command to Headset,
Headset reject it with bad state. Bluedroid stack will ack failure to start_audio_datapath.
The next time we write audio data to bluetooth, we will call start_audio_datapath again to send AVDTP_Start command to Headset
Headset reject it with bad state again. Bluedroid stack will ack failure to start_audio_datapath.
When the third time we call start_audio_datapath, right at that time we receive  AVDTP_Start command from Headset.
Handle AVDTP_Start command and Handle start_audio_datapath are in two different threads.
Handle AVDTP_Start command is in btu_task thread.
Handle start_audio_datapath() is in btif_task thread.
We have race condition in this case
Because when  btif_task processed BTIF_AV_START_STREAM_REQ_EVT(triggered by start_audio_datapath),
it don't know we receive  AVDTP_Start command which is processed in  btu_task.
btif_task will send a message BTA_AV_API_START_EVT to btu_task, which will be handled by bta_av_do_start.
AVDTP_start command from headset is handled by bta_av_start_ok.
bta_av_start_ok will send BTA_AV_START_EVT with suspending true to btif_task and send AVDTP_Suspend command
to headset to suspend the AVDTP for reconfiguration purpose.

in bta_av_do_start, we will check whether the AVDTP is already started, we will know the AVDTP is already start at this time because
bta_av_do_start is also running in btu_task. We will send BTA_AV_START_EVT with success to btif_task.

In the btif_task, BTA_AV_START_EVT will be processed by btif_av_state_opened_handler:
For the first BTA_AV_START_EVT with suspending true sent by bta_av_start_ok, it will ignore it:
if ((p_av->start.status == BTA_SUCCESS) && (p_av->start.suspending == TRUE))
               return TRUE;
For the second BTA_AV_START_EVT with success sent by bta_av_do_start , it will ack success to start_audio_datapath, and change to
BTIF_AV_STATE_STARTED/BTAV_AUDIO_STATE_STARTED, after receive success ack from bluedroid stack, we will start send Audio data to bluetooth.

At last we received  AVDTP_Suspend response accept from Headset, we will send BTA_AV_SUSPEND_EVT to btif_task, which will be
handled by btif_av_state_started_handler. It will call btif_a2dp_on_suspended and call audio_state_cb  with new audio state
BTAV_AUDIO_STATE_STOPPED.
so The state between bluedroid stack and audio data path is out of sync.
The fix is to send failure message if we know we suspend AVDTP in bta_av_do_start,
also make sure we won't miss acknowledgement for pending start if we exit opened state,
to avoid audio data path dead lock.

bug:10953908
Change-Id: I1704839977324b7c4e234eb843cddf3719e10d2c
parent 222a2ddd

system/bta/av/bta_av_aact.c

+30 −12
Original line number Diff line number Diff line
@@ -253,6 +253,28 @@ static void bta_av_save_addr(tBTA_AV_SCB *p_scb, const BD_ADDR b) 
    bdcpy(p_scb->peer_addr, b);

}



/*******************************************************************************

**

** Function         notify_start_failed

**

** Description      notify up-layer AV start failed

**

**

** Returns          void

**

*******************************************************************************/

static void notify_start_failed(tBTA_AV_SCB *p_scb)

{

    tBTA_AV_START   start;

    /* if start failed, clear role */

    p_scb->role &= ~BTA_AV_ROLE_START_INT;

    start.chnl   = p_scb->chnl;

    start.status = BTA_AV_FAIL;

    start.initiator = TRUE;

    start.hndl   = p_scb->hndl;

    (*bta_av_cb.p_cback)(BTA_AV_START_EVT, (tBTA_AV *) &start);

}



/*******************************************************************************

**

** Function         bta_av_st_rc_timer
@@ -1782,9 +1804,14 @@ void bta_av_do_start (tBTA_AV_SCB *p_scb, tBTA_AV_DATA *p_data) 
    else if (p_scb->started)

    {

        p_scb->role |= BTA_AV_ROLE_START_INT;

        if ( p_scb->wait == 0 )

        if ( p_scb->wait == 0 ) {

            if (p_scb->role & BTA_AV_ROLE_SUSPEND) {

                notify_start_failed(p_scb);

            } else {

                bta_av_start_ok(p_scb, NULL);

            }

        }

    }

    APPL_TRACE_DEBUG2("started %d role:x%x", p_scb->started, p_scb->role);

}
@@ -2233,19 +2260,10 @@ void bta_av_start_ok (tBTA_AV_SCB *p_scb, tBTA_AV_DATA *p_data) 
*******************************************************************************/

void bta_av_start_failed (tBTA_AV_SCB *p_scb, tBTA_AV_DATA *p_data)

{

    tBTA_AV_START   start;



    if(p_scb->started == FALSE && p_scb->co_started == FALSE)

    {

        /* if start failed, clear role */

        p_scb->role &= ~BTA_AV_ROLE_START_INT;



        bta_sys_idle(BTA_ID_AV, bta_av_cb.audio_open_cnt, p_scb->peer_addr);

        start.chnl   = p_scb->chnl;

        start.status = BTA_AV_FAIL;

        start.initiator = TRUE;

        start.hndl   = p_scb->hndl;

        (*bta_av_cb.p_cback)(BTA_AV_START_EVT, (tBTA_AV *) &start);

        notify_start_failed(p_scb);

    }



    bta_sys_set_policy(BTA_ID_AV, (HCI_ENABLE_SNIFF_MODE|HCI_ENABLE_MASTER_SLAVE_SWITCH), p_scb->peer_addr);

system/btif/include/btif_media.h

+1 −1
Original line number Diff line number Diff line
@@ -236,7 +236,7 @@ void btif_a2dp_on_init(void); 
void btif_a2dp_setup_codec(void);

void btif_a2dp_on_idle(void);

void btif_a2dp_on_open(void);

void btif_a2dp_on_started(tBTA_AV_START *p_av);

BOOLEAN btif_a2dp_on_started(tBTA_AV_START *p_av, BOOLEAN pending_start);

void btif_a2dp_ack_fail(void);

void btif_a2dp_on_stop_req(void);

void btif_a2dp_on_stopped(tBTA_AV_SUSPEND *p_av);

system/btif/src/btif_av.c

+16 −4
Original line number Diff line number Diff line
@@ -491,14 +491,21 @@ static BOOLEAN btif_av_state_opened_handler(btif_sm_event_t event, void *p_data) 
            if ((p_av->start.status == BTA_SUCCESS) && (p_av->start.suspending == TRUE))

                return TRUE;



            if (btif_a2dp_on_started(&p_av->start,

                ((btif_av_cb.flags & BTIF_AV_FLAG_PENDING_START) != 0))) {

                /* only clear pending flag after acknowledgement */

                btif_av_cb.flags &= ~BTIF_AV_FLAG_PENDING_START;

            btif_a2dp_on_started(&p_av->start);

            }



            /* remain in open state if status failed */

            if (p_av->start.status != BTA_AV_SUCCESS)

                return FALSE;



            /* change state to started */

            /* change state to started, send acknowledgement if start is pending */

            if (btif_av_cb.flags & BTIF_AV_FLAG_PENDING_START) {

                btif_a2dp_on_started(NULL, TRUE);

                /* pending start flag will be cleared when exit current state */

            }

            btif_sm_change_state(btif_av_cb.sm_handle, BTIF_AV_STATE_STARTED);



        } break;
@@ -520,6 +527,11 @@ static BOOLEAN btif_av_state_opened_handler(btif_sm_event_t event, void *p_data) 
            HAL_CBACK(bt_av_callbacks, connection_state_cb,

                BTAV_CONNECTION_STATE_DISCONNECTED, &(btif_av_cb.peer_bda));



            /* change state to idle, send acknowledgement if start is pending */

            if (btif_av_cb.flags & BTIF_AV_FLAG_PENDING_START) {

                btif_a2dp_ack_fail();

                /* pending start flag will be cleared when exit current state */

            }

            btif_sm_change_state(btif_av_cb.sm_handle, BTIF_AV_STATE_IDLE);

            break;
@@ -581,7 +593,7 @@ static BOOLEAN btif_av_state_started_handler(btif_sm_event_t event, void *p_data 


        case BTIF_AV_START_STREAM_REQ_EVT:

            /* we were remotely started, just ack back the local request */

            btif_a2dp_on_started(NULL);

            btif_a2dp_on_started(NULL, TRUE);

            break;



        /* fixme -- use suspend = true always to work around issue with BTA AV */

system/btif/src/btif_media_task.c

+10 −4
Original line number Diff line number Diff line
@@ -844,9 +844,10 @@ void btif_a2dp_on_open(void) 
**

*******************************************************************************/



void btif_a2dp_on_started(tBTA_AV_START *p_av)

BOOLEAN btif_a2dp_on_started(tBTA_AV_START *p_av, BOOLEAN pending_start)

{

    tBTIF_STATUS status;

    BOOLEAN ack = FALSE;



    APPL_TRACE_EVENT0("## ON A2DP STARTED ##");
@@ -854,7 +855,7 @@ void btif_a2dp_on_started(tBTA_AV_START *p_av) 
    {

        /* ack back a local start request */

        a2dp_cmd_acknowledge(A2DP_CTRL_ACK_SUCCESS);

        return;

        return TRUE;

    }



    if (p_av->status == BTA_AV_SUCCESS)
@@ -863,7 +864,10 @@ void btif_a2dp_on_started(tBTA_AV_START *p_av) 
        {

            if (p_av->initiator)

            {

                if (pending_start) {

                    a2dp_cmd_acknowledge(A2DP_CTRL_ACK_SUCCESS);

                    ack = TRUE;

                }

            }

            else

            {
@@ -875,10 +879,12 @@ void btif_a2dp_on_started(tBTA_AV_START *p_av) 
            /* media task is autostarted upon a2dp audiopath connection */

        }

    }

    else

    else if (pending_start)

    {

        a2dp_cmd_acknowledge(A2DP_CTRL_ACK_FAILURE);

        ack = TRUE;

    }

    return ack;

}