OSDN Git Service

Using std::promise and std::future to block till A2DP device activated
authorCheney Ni <cheneyni@google.com>
Wed, 19 Dec 2018 10:46:18 +0000 (18:46 +0800)
committerJack He <siyuanh@google.com>
Tue, 29 Jan 2019 23:17:41 +0000 (23:17 +0000)
There is a new interface to replace UIPC with Blueototh Audio Hal v2 and
synchronization issues between BT Stack and Audio Hal was found when
activating a new A2DP device. Because the API to activate an A2DP device
was non-blocking, it was possilbe that there was a race condition when
BT Stack starting A2DP session and Audio Hal was opening A2DP output.
There was a chance that the output was opened before session started and
causing A2DP to have no sound.

This CL uses std::promise and std::future are able to achieve the
serialize of starting session and opening output for A2DP.

Bug: 111519504
Bug: 122505783
Test: A2DP reconnection and switching

Change-Id: I88c42ea1eb5f8def2345dbfaab26c6d1a91c54cc

btif/include/btif_a2dp_sink.h
btif/include/btif_a2dp_source.h
btif/src/btif_a2dp_sink.cc
btif/src/btif_a2dp_source.cc
btif/src/btif_av.cc

index efb3b26..0a9038c 100644 (file)
@@ -22,6 +22,7 @@
 
 #include <inttypes.h>
 #include <stdbool.h>
+#include <future>
 
 #include "bt_types.h"
 #include "bta_av_api.h"
@@ -52,7 +53,8 @@ bool btif_a2dp_sink_startup(void);
 // Start the A2DP Sink session.
 // This function should be called by the BTIF state machine after
 // btif_a2dp_sink_startup() to start the streaming session for |peer_address|.
-bool btif_a2dp_sink_start_session(const RawAddress& peer_address);
+bool btif_a2dp_sink_start_session(const RawAddress& peer_address,
+                                  std::promise<void> peer_ready_promise);
 
 // Restart the A2DP Sink session.
 // This function should be called by the BTIF state machine after
@@ -62,7 +64,8 @@ bool btif_a2dp_sink_start_session(const RawAddress& peer_address);
 // |new_peer_address| is the peer address of the new session. This address
 // cannot be empty.
 bool btif_a2dp_sink_restart_session(const RawAddress& old_peer_address,
-                                    const RawAddress& new_peer_address);
+                                    const RawAddress& new_peer_address,
+                                    std::promise<void> peer_ready_promise);
 
 // End the A2DP Sink session.
 // This function should be called by the BTIF state machine to end the
index bb96e1a..0649012 100644 (file)
@@ -21,6 +21,7 @@
 #define BTIF_A2DP_SOURCE_H
 
 #include <stdbool.h>
+#include <future>
 
 #include "bta_av_api.h"
 
@@ -37,7 +38,8 @@ bool btif_a2dp_source_startup(void);
 // Start the A2DP Source session.
 // This function should be called by the BTIF state machine after
 // btif_a2dp_source_startup() to start the streaming session for |peer_address|.
-bool btif_a2dp_source_start_session(const RawAddress& peer_address);
+bool btif_a2dp_source_start_session(const RawAddress& peer_address,
+                                    std::promise<void> peer_ready_promise);
 
 // Restart the A2DP Source session.
 // This function should be called by the BTIF state machine after
@@ -47,7 +49,8 @@ bool btif_a2dp_source_start_session(const RawAddress& peer_address);
 // |new_peer_address| is the peer address of the new session. This address
 // cannot be empty.
 bool btif_a2dp_source_restart_session(const RawAddress& old_peer_address,
-                                      const RawAddress& new_peer_address);
+                                      const RawAddress& new_peer_address,
+                                      std::promise<void> peer_ready_promise);
 
 // End the A2DP Source session.
 // This function should be called by the BTIF state machine to end the
index 16d43e3..bb1bc49 100644 (file)
@@ -129,7 +129,8 @@ static std::atomic<int> btif_a2dp_sink_state{BTIF_A2DP_SINK_STATE_OFF};
 
 static void btif_a2dp_sink_init_delayed();
 static void btif_a2dp_sink_startup_delayed();
-static void btif_a2dp_sink_start_session_delayed();
+static void btif_a2dp_sink_start_session_delayed(
+    std::promise<void> peer_ready_promise);
 static void btif_a2dp_sink_end_session_delayed();
 static void btif_a2dp_sink_shutdown_delayed();
 static void btif_a2dp_sink_cleanup_delayed();
@@ -211,25 +212,34 @@ static void btif_a2dp_sink_startup_delayed() {
   // Nothing to do
 }
 
-bool btif_a2dp_sink_start_session(const RawAddress& peer_address) {
-  LOG_INFO(LOG_TAG, "%s: peer_address=%s", __func__,
-           peer_address.ToString().c_str());
-  btif_a2dp_sink_cb.worker_thread.DoInThread(
-      FROM_HERE, base::BindOnce(btif_a2dp_sink_start_session_delayed));
-  return true;
+bool btif_a2dp_sink_start_session(const RawAddress& peer_address,
+                                  std::promise<void> peer_ready_promise) {
+  LOG(INFO) << __func__ << ": peer_address=" << peer_address;
+  if (btif_a2dp_sink_cb.worker_thread.DoInThread(
+          FROM_HERE, base::BindOnce(btif_a2dp_sink_start_session_delayed,
+                                    std::move(peer_ready_promise)))) {
+    return true;
+  } else {
+    // cannot set promise but triggers crash
+    LOG(FATAL) << __func__ << ": peer_address=" << peer_address
+               << " fails to context switch";
+    return false;
+  }
 }
 
-static void btif_a2dp_sink_start_session_delayed() {
-  LOG_INFO(LOG_TAG, "%s", __func__);
+static void btif_a2dp_sink_start_session_delayed(
+    std::promise<void> peer_ready_promise) {
+  LOG(INFO) << __func__;
   LockGuard lock(g_mutex);
+  peer_ready_promise.set_value();
   // Nothing to do
 }
 
 bool btif_a2dp_sink_restart_session(const RawAddress& old_peer_address,
-                                    const RawAddress& new_peer_address) {
-  LOG_INFO(LOG_TAG, "%s: old_peer_address=%s new_peer_address=%s", __func__,
-           old_peer_address.ToString().c_str(),
-           new_peer_address.ToString().c_str());
+                                    const RawAddress& new_peer_address,
+                                    std::promise<void> peer_ready_promise) {
+  LOG(INFO) << __func__ << ": old_peer_address=" << old_peer_address
+            << " new_peer_address=" << new_peer_address;
 
   CHECK(!new_peer_address.IsEmpty());
 
@@ -238,15 +248,17 @@ bool btif_a2dp_sink_restart_session(const RawAddress& old_peer_address,
   }
 
   if (!bta_av_co_set_active_peer(new_peer_address)) {
-    LOG_ERROR(LOG_TAG, "%s: Cannot stream audio: cannot set active peer to %s",
-              __func__, new_peer_address.ToString().c_str());
+    LOG(ERROR) << __func__
+               << ": Cannot stream audio: cannot set active peer to "
+               << new_peer_address;
+    peer_ready_promise.set_value();
     return false;
   }
 
   if (old_peer_address.IsEmpty()) {
     btif_a2dp_sink_startup();
   }
-  btif_a2dp_sink_start_session(new_peer_address);
+  btif_a2dp_sink_start_session(new_peer_address, std::move(peer_ready_promise));
 
   return true;
 }
index 47fb1a1..6260a39 100644 (file)
@@ -229,7 +229,7 @@ static BtifA2dpSource btif_a2dp_source_cb;
 static void btif_a2dp_source_init_delayed(void);
 static void btif_a2dp_source_startup_delayed(void);
 static void btif_a2dp_source_start_session_delayed(
-    const RawAddress& peer_address);
+    const RawAddress& peer_address, std::promise<void> start_session_promise);
 static void btif_a2dp_source_end_session_delayed(
     const RawAddress& peer_address);
 static void btif_a2dp_source_shutdown_delayed(void);
@@ -359,24 +359,32 @@ static void btif_a2dp_source_startup_delayed() {
   btif_a2dp_source_cb.SetState(BtifA2dpSource::kStateRunning);
 }
 
-bool btif_a2dp_source_start_session(const RawAddress& peer_address) {
-  LOG_INFO(LOG_TAG, "%s: peer_address=%s state=%s", __func__,
-           peer_address.ToString().c_str(),
-           btif_a2dp_source_cb.StateStr().c_str());
+bool btif_a2dp_source_start_session(const RawAddress& peer_address,
+                                    std::promise<void> peer_ready_promise) {
+  LOG(INFO) << __func__ << ": peer_address=" << peer_address
+            << " state=" << btif_a2dp_source_cb.StateStr();
   btif_a2dp_source_setup_codec(peer_address);
-  btif_a2dp_source_thread.DoInThread(
-      FROM_HERE,
-      base::Bind(&btif_a2dp_source_start_session_delayed, peer_address));
-  return true;
+  if (btif_a2dp_source_thread.DoInThread(
+          FROM_HERE,
+          base::BindOnce(&btif_a2dp_source_start_session_delayed, peer_address,
+                         std::move(peer_ready_promise)))) {
+    return true;
+  } else {
+    // cannot set promise but triggers crash
+    LOG(FATAL) << __func__ << ": peer_address=" << peer_address
+               << " state=" << btif_a2dp_source_cb.StateStr()
+               << " fails to context switch";
+    return false;
+  }
 }
 
 static void btif_a2dp_source_start_session_delayed(
-    const RawAddress& peer_address) {
-  LOG_INFO(LOG_TAG, "%s: peer_address=%s state=%s", __func__,
-           peer_address.ToString().c_str(),
-           btif_a2dp_source_cb.StateStr().c_str());
+    const RawAddress& peer_address, std::promise<void> peer_ready_promise) {
+  LOG(INFO) << __func__ << ": peer_address=" << peer_address
+            << " state=" << btif_a2dp_source_cb.StateStr();
   if (btif_a2dp_source_cb.State() != BtifA2dpSource::kStateRunning) {
-    LOG_ERROR(LOG_TAG, "%s: A2DP Source media task is not running", __func__);
+    LOG(ERROR) << __func__ << ": A2DP Source media task is not running";
+    peer_ready_promise.set_value();
     return;
   }
   if (btif_av_is_a2dp_offload_enabled()) {
@@ -385,17 +393,17 @@ static void btif_a2dp_source_start_session_delayed(
     BluetoothMetricsLogger::GetInstance()->LogBluetoothSessionStart(
         bluetooth::common::CONNECTION_TECHNOLOGY_TYPE_BREDR, 0);
   }
+  peer_ready_promise.set_value();
 }
 
 bool btif_a2dp_source_restart_session(const RawAddress& old_peer_address,
-                                      const RawAddress& new_peer_address) {
+                                      const RawAddress& new_peer_address,
+                                      std::promise<void> peer_ready_promise) {
   bool is_streaming = btif_a2dp_source_cb.media_alarm.IsScheduled();
-  LOG_INFO(LOG_TAG,
-           "%s: old_peer_address=%s new_peer_address=%s is_streaming=%s "
-           "state=%s",
-           __func__, old_peer_address.ToString().c_str(),
-           new_peer_address.ToString().c_str(), logbool(is_streaming).c_str(),
-           btif_a2dp_source_cb.StateStr().c_str());
+  LOG(INFO) << __func__ << ": old_peer_address=" << old_peer_address
+            << " new_peer_address=" << new_peer_address
+            << " is_streaming=" << logbool(is_streaming)
+            << " state=" << btif_a2dp_source_cb.StateStr();
 
   CHECK(!new_peer_address.IsEmpty());
 
@@ -414,7 +422,8 @@ bool btif_a2dp_source_restart_session(const RawAddress& old_peer_address,
 
   // Start the session.
   // If audio was streaming before, start audio streaming as well.
-  btif_a2dp_source_start_session(new_peer_address);
+  btif_a2dp_source_start_session(new_peer_address,
+                                 std::move(peer_ready_promise));
   if (is_streaming) {
     btif_a2dp_source_start_audio_req();
   }
index b004f44..3dcda50 100644 (file)
@@ -443,32 +443,39 @@ class BtifAvSource {
    * reset the active peer
    * @return true on success, otherwise false
    */
-  bool SetActivePeer(const RawAddress& peer_address) {
-    LOG_INFO(LOG_TAG, "%s: peer: %s", __PRETTY_FUNCTION__,
-             peer_address.ToString().c_str());
+  bool SetActivePeer(const RawAddress& peer_address,
+                     std::promise<void> peer_ready_promise) {
+    LOG(INFO) << __PRETTY_FUNCTION__ << ": peer: " << peer_address;
 
-    if (active_peer_ == peer_address) return true;  // Nothing has changed
+    if (active_peer_ == peer_address) {
+      peer_ready_promise.set_value();
+      return true;  // Nothing has changed
+    }
     if (peer_address.IsEmpty()) {
       BTIF_TRACE_EVENT("%s: peer address is empty, shutdown the Audio source",
                        __func__);
       if (!bta_av_co_set_active_peer(peer_address)) {
-        BTIF_TRACE_WARNING("%s: unable to set active peer to empty in BtaAvCo",
-                           __func__);
+        LOG(WARNING) << __func__
+                     << ": unable to set active peer to empty in BtaAvCo";
       }
       btif_a2dp_source_end_session(active_peer_);
       btif_a2dp_source_shutdown();
       active_peer_ = peer_address;
+      peer_ready_promise.set_value();
       return true;
     }
 
     BtifAvPeer* peer = FindPeer(peer_address);
     if (peer != nullptr && !peer->IsConnected()) {
-      BTIF_TRACE_ERROR("%s: Error setting %s as active Source peer", __func__,
-                       peer->PeerAddress().ToString().c_str());
+      LOG(ERROR) << __func__ << ": Error setting " << peer->PeerAddress()
+                 << " as active Source peer";
+      peer_ready_promise.set_value();
       return false;
     }
 
-    if (!btif_a2dp_source_restart_session(active_peer_, peer_address)) {
+    if (!btif_a2dp_source_restart_session(active_peer_, peer_address,
+                                          std::move(peer_ready_promise))) {
+      // cannot set promise but need to be handled within restart_session
       return false;
     }
     active_peer_ = peer_address;
@@ -483,7 +490,8 @@ class BtifAvSource {
    */
   void UpdateCodecConfig(
       const RawAddress& peer_address,
-      const std::vector<btav_a2dp_codec_config_t>& codec_preferences) {
+      const std::vector<btav_a2dp_codec_config_t>& codec_preferences,
+      std::promise<void> peer_ready_promise) {
     // Restart the session if the codec for the active peer is updated
     bool restart_session =
         ((active_peer_ == peer_address) && !active_peer_.IsEmpty());
@@ -497,7 +505,10 @@ class BtifAvSource {
       btif_a2dp_source_encoder_user_config_update_req(peer_address, cp);
     }
     if (restart_session) {
-      btif_a2dp_source_start_session(active_peer_);
+      btif_a2dp_source_start_session(active_peer_,
+                                     std::move(peer_ready_promise));
+    } else {
+      peer_ready_promise.set_value();
     }
   }
 
@@ -581,32 +592,39 @@ class BtifAvSink {
    * reset the active peer
    * @return true on success, otherwise false
    */
-  bool SetActivePeer(const RawAddress& peer_address) {
-    LOG_INFO(LOG_TAG, "%s: peer: %s", __PRETTY_FUNCTION__,
-             peer_address.ToString().c_str());
+  bool SetActivePeer(const RawAddress& peer_address,
+                     std::promise<void> peer_ready_promise) {
+    LOG(INFO) << __PRETTY_FUNCTION__ << ": peer: " << peer_address;
 
-    if (active_peer_ == peer_address) return true;  // Nothing has changed
+    if (active_peer_ == peer_address) {
+      peer_ready_promise.set_value();
+      return true;  // Nothing has changed
+    }
     if (peer_address.IsEmpty()) {
       BTIF_TRACE_EVENT("%s: peer address is empty, shutdown the Audio sink",
                        __func__);
       if (!bta_av_co_set_active_peer(peer_address)) {
-        BTIF_TRACE_WARNING("%s: unable to set active peer to empty in BtaAvCo",
-                           __func__);
+        LOG(WARNING) << __func__
+                     << ": unable to set active peer to empty in BtaAvCo";
       }
       btif_a2dp_sink_end_session(active_peer_);
       btif_a2dp_sink_shutdown();
       active_peer_ = peer_address;
+      peer_ready_promise.set_value();
       return true;
     }
 
     BtifAvPeer* peer = FindPeer(peer_address);
     if (peer != nullptr && !peer->IsConnected()) {
-      BTIF_TRACE_ERROR("%s: Error setting %s as active Sink peer", __func__,
-                       peer->PeerAddress().ToString().c_str());
+      LOG(ERROR) << __func__ << ": Error setting " << peer->PeerAddress()
+                 << " as active Sink peer";
+      peer_ready_promise.set_value();
       return false;
     }
 
-    if (!btif_a2dp_sink_restart_session(active_peer_, peer_address)) {
+    if (!btif_a2dp_sink_restart_session(active_peer_, peer_address,
+                                        std::move(peer_ready_promise))) {
+      // cannot set promise but need to be handled within restart_session
       return false;
     }
     active_peer_ = peer_address;
@@ -961,10 +979,12 @@ void BtifAvSource::Cleanup() {
 
   btif_queue_cleanup(UUID_SERVCLASS_AUDIO_SOURCE);
 
+  std::promise<void> peer_ready_promise;
   do_in_main_thread(
       FROM_HERE,
-      base::Bind(base::IgnoreResult(&BtifAvSource::SetActivePeer),
-                 base::Unretained(&btif_av_source), RawAddress::kEmpty));
+      base::BindOnce(base::IgnoreResult(&BtifAvSource::SetActivePeer),
+                     base::Unretained(&btif_av_source), RawAddress::kEmpty,
+                     std::move(peer_ready_promise)));
   do_in_main_thread(FROM_HERE, base::Bind(&btif_a2dp_source_cleanup));
 
   btif_disable_service(BTA_A2DP_SOURCE_SERVICE_ID);
@@ -1146,10 +1166,12 @@ void BtifAvSink::Cleanup() {
 
   btif_queue_cleanup(UUID_SERVCLASS_AUDIO_SINK);
 
+  std::promise<void> peer_ready_promise;
   do_in_main_thread(
       FROM_HERE,
-      base::Bind(base::IgnoreResult(&BtifAvSink::SetActivePeer),
-                 base::Unretained(&btif_av_sink), RawAddress::kEmpty));
+      base::BindOnce(base::IgnoreResult(&BtifAvSink::SetActivePeer),
+                     base::Unretained(&btif_av_sink), RawAddress::kEmpty,
+                     std::move(peer_ready_promise)));
   do_in_main_thread(FROM_HERE, base::Bind(&btif_a2dp_sink_cleanup));
 
   btif_disable_service(BTA_A2DP_SINK_SERVICE_ID);
@@ -1319,10 +1341,13 @@ void BtifAvStateMachine::StateIdle::OnEnter() {
   // Reset the active peer if this was the active peer and
   // the Idle state was reentered
   if (peer_.IsActivePeer() && peer_.CanBeDeleted()) {
+    std::promise<void> peer_ready_promise;
     if (peer_.IsSink()) {
-      btif_av_source.SetActivePeer(RawAddress::kEmpty);
+      btif_av_source.SetActivePeer(RawAddress::kEmpty,
+                                   std::move(peer_ready_promise));
     } else if (peer_.IsSource()) {
-      btif_av_sink.SetActivePeer(RawAddress::kEmpty);
+      btif_av_sink.SetActivePeer(RawAddress::kEmpty,
+                                 std::move(peer_ready_promise));
     }
   }
 
@@ -1747,7 +1772,9 @@ void BtifAvStateMachine::StateOpened::OnEnter() {
   // For A2DP Source, the setting of the Active device is done by the
   // ActiveDeviceManager in Java.
   if (peer_.IsSource() && btif_av_sink.ActivePeer().IsEmpty()) {
-    if (!btif_av_sink.SetActivePeer(peer_.PeerAddress())) {
+    std::promise<void> peer_ready_promise;
+    if (!btif_av_sink.SetActivePeer(peer_.PeerAddress(),
+                                    std::move(peer_ready_promise))) {
       BTIF_TRACE_ERROR("%s: Error setting %s as active Source peer", __func__,
                        peer_.PeerAddress().ToString().c_str());
     }
@@ -2652,20 +2679,23 @@ static void set_source_silence_peer_int(const RawAddress& peer_address,
 
 // Set the active peer
 static void set_active_peer_int(uint8_t peer_sep,
-                                const RawAddress& peer_address) {
+                                const RawAddress& peer_address,
+                                std::promise<void> peer_ready_promise) {
   BTIF_TRACE_EVENT("%s: peer_sep=%s (%d) peer_address=%s", __func__,
                    (peer_sep == AVDT_TSEP_SRC) ? "Source" : "Sink", peer_sep,
                    peer_address.ToString().c_str());
   BtifAvPeer* peer = nullptr;
   if (peer_sep == AVDT_TSEP_SNK) {
-    if (!btif_av_source.SetActivePeer(peer_address)) {
+    if (!btif_av_source.SetActivePeer(peer_address,
+                                      std::move(peer_ready_promise))) {
       BTIF_TRACE_ERROR("%s: Error setting %s as active Sink peer", __func__,
                        peer_address.ToString().c_str());
     }
     return;
   }
   if (peer_sep == AVDT_TSEP_SRC) {
-    if (!btif_av_sink.SetActivePeer(peer_address)) {
+    if (!btif_av_sink.SetActivePeer(peer_address,
+                                    std::move(peer_ready_promise))) {
       BTIF_TRACE_ERROR("%s: Error setting %s as active Source peer", __func__,
                        peer_address.ToString().c_str());
     }
@@ -2676,6 +2706,7 @@ static void set_active_peer_int(uint8_t peer_sep,
                    (peer_sep == AVDT_TSEP_SRC) ? "Source" : "Sink",
                    peer_address.ToString().c_str(),
                    (peer == nullptr) ? "found" : "connected");
+  peer_ready_promise.set_value();
 }
 
 static bt_status_t src_connect_sink(const RawAddress& peer_address) {
@@ -2752,13 +2783,22 @@ static bt_status_t src_set_active_sink(const RawAddress& peer_address) {
   BTIF_TRACE_EVENT("%s: Peer %s", __func__, peer_address.ToString().c_str());
 
   if (!btif_av_source.Enabled()) {
-    BTIF_TRACE_WARNING("%s: BTIF AV Source is not enabled", __func__);
+    LOG(WARNING) << __func__ << ": BTIF AV Source is not enabled";
     return BT_STATUS_NOT_READY;
   }
 
-  return do_in_main_thread(FROM_HERE, base::Bind(&set_active_peer_int,
-                                                 AVDT_TSEP_SNK,  // peer_sep
-                                                 peer_address));
+  std::promise<void> peer_ready_promise;
+  std::future<void> peer_ready_future = peer_ready_promise.get_future();
+  bt_status_t status = do_in_main_thread(
+      FROM_HERE, base::BindOnce(&set_active_peer_int,
+                                AVDT_TSEP_SNK,  // peer_sep
+                                peer_address, std::move(peer_ready_promise)));
+  if (status == BT_STATUS_SUCCESS) {
+    peer_ready_future.wait();
+  } else {
+    LOG(WARNING) << __func__ << ": BTIF AV Source fails to change peer";
+  }
+  return status;
 }
 
 static bt_status_t codec_config_src(
@@ -2767,14 +2807,23 @@ static bt_status_t codec_config_src(
   BTIF_TRACE_EVENT("%s", __func__);
 
   if (!btif_av_source.Enabled()) {
-    BTIF_TRACE_WARNING("%s: BTIF AV Source is not enabled", __func__);
+    LOG(WARNING) << __func__ << ": BTIF AV Source is not enabled";
     return BT_STATUS_NOT_READY;
   }
 
-  return do_in_main_thread(
-      FROM_HERE, base::Bind(&BtifAvSource::UpdateCodecConfig,
-                            base::Unretained(&btif_av_source), peer_address,
-                            codec_preferences));
+  std::promise<void> peer_ready_promise;
+  std::future<void> peer_ready_future = peer_ready_promise.get_future();
+  bt_status_t status = do_in_main_thread(
+      FROM_HERE,
+      base::BindOnce(&BtifAvSource::UpdateCodecConfig,
+                     base::Unretained(&btif_av_source), peer_address,
+                     codec_preferences, std::move(peer_ready_promise)));
+  if (status == BT_STATUS_SUCCESS) {
+    peer_ready_future.wait();
+  } else {
+    LOG(WARNING) << __func__ << ": BTIF AV Source fails to config codec";
+  }
+  return status;
 }
 
 static void cleanup_src(void) {