device/bluetooth: Add I/O watcher for audio data retrieval triggered by state change.

device/bluetooth/bluetooth_audio_sink_chromeos.cc

Index: device/bluetooth/bluetooth_audio_sink_chromeos.cc
diff --git a/device/bluetooth/bluetooth_audio_sink_chromeos.cc b/device/bluetooth/bluetooth_audio_sink_chromeos.cc
index fb1ddf7d6ec09565442cde1013ddaeed52f93150..29b0a068dcf6b840402f790ec67106d46294360c 100644
--- a/device/bluetooth/bluetooth_audio_sink_chromeos.cc
+++ b/device/bluetooth/bluetooth_audio_sink_chromeos.cc
@@ -4,12 +4,15 @@
 
 #include "device/bluetooth/bluetooth_audio_sink_chromeos.h"
 
+#include <unistd.h>
+
 #include <algorithm>
 #include <sstream>
 #include <string>
 #include <vector>
 
 #include "base/debug/stack_trace.h"
+#include "base/files/file_util.h"
 #include "base/logging.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "dbus/message.h"
@@ -23,6 +26,10 @@ namespace {
 // TODO(mcchou): Add the constant to dbus/service_constants.h.
 const char kBluetoothAudioSinkServicePath[] = "/org/chromium/AudioSink";
 
+const int kInvalidFd = -1;
+const uint16_t kInvalidReadMtu = 0;
+const uint16_t kInvalidWriteMtu = 0;
+
 ObjectPath GenerateEndpointPath() {
   static unsigned int sequence_number = 0;
   ++sequence_number;
@@ -78,24 +85,26 @@ BluetoothAudioSinkChromeOS::BluetoothAudioSinkChromeOS(
     scoped_refptr<device::BluetoothAdapter> adapter)
     : state_(BluetoothAudioSink::STATE_INVALID),
       volume_(BluetoothAudioSink::kInvalidVolume),
-      read_mtu_(nullptr),
-      write_mtu_(nullptr),
+      read_mtu_(kInvalidReadMtu),
+      write_mtu_(kInvalidWriteMtu),
+      read_failed_logged_(false),
       adapter_(adapter),
       weak_ptr_factory_(this) {
   VLOG(1) << "BluetoothAudioSinkChromeOS created";
 
   DCHECK(adapter_.get());
   DCHECK(adapter_->IsPresent());
+  DCHECK(DBusThreadManager::IsInitialized());
 
   adapter_->AddObserver(this);
 
-  chromeos::BluetoothMediaClient* media =
+  BluetoothMediaClient* media =
       DBusThreadManager::Get()->GetBluetoothMediaClient();
   DCHECK(media);
   media->AddObserver(this);
 
-  chromeos::BluetoothMediaTransportClient *transport =
-      chromeos::DBusThreadManager::Get()->GetBluetoothMediaTransportClient();
+  BluetoothMediaTransportClient* transport =
+      DBusThreadManager::Get()->GetBluetoothMediaTransportClient();
   DCHECK(transport);
   transport->AddObserver(this);
 
@@ -114,13 +123,13 @@ BluetoothAudioSinkChromeOS::~BluetoothAudioSinkChromeOS() {
 
   adapter_->RemoveObserver(this);
 
-  chromeos::BluetoothMediaClient* media =
+  BluetoothMediaClient* media =
       DBusThreadManager::Get()->GetBluetoothMediaClient();
   DCHECK(media);
   media->RemoveObserver(this);
 
-  chromeos::BluetoothMediaTransportClient *transport =
-      chromeos::DBusThreadManager::Get()->GetBluetoothMediaTransportClient();
+  BluetoothMediaTransportClient* transport =
+      DBusThreadManager::Get()->GetBluetoothMediaTransportClient();
   DCHECK(transport);
   transport->RemoveObserver(this);
 }
@@ -133,7 +142,7 @@ void BluetoothAudioSinkChromeOS::Unregister(
   if (!DBusThreadManager::IsInitialized())
     error_callback.Run(BluetoothAudioSink::ERROR_NOT_UNREGISTERED);
 
-  chromeos::BluetoothMediaClient* media =
+  BluetoothMediaClient* media =
       DBusThreadManager::Get()->GetBluetoothMediaClient();
   DCHECK(media);
 
@@ -217,7 +226,7 @@ void BluetoothAudioSinkChromeOS::MediaTransportPropertyChanged(
   VLOG(1) << "MediaTransportPropertyChanged: " << property_name;
 
   // Retrieves the property set of the transport object with |object_path|.
-  chromeos::BluetoothMediaTransportClient::Properties* properties =
+  BluetoothMediaTransportClient::Properties* properties =
       DBusThreadManager::Get()
           ->GetBluetoothMediaTransportClient()
           ->GetProperties(object_path);
@@ -270,6 +279,7 @@ void BluetoothAudioSinkChromeOS::ClearConfiguration(
     const ObjectPath& transport_path) {
   if (transport_path != transport_path_)
     return;
+
   VLOG(1) << "ClearConfiguration";
   StateChanged(BluetoothAudioSink::STATE_DISCONNECTED);
 }
@@ -309,7 +319,7 @@ void BluetoothAudioSinkChromeOS::Register(
   media_path_ = static_cast<BluetoothAdapterChromeOS*>(
       adapter_.get())->object_path();
 
-  chromeos::BluetoothMediaClient* media =
+  BluetoothMediaClient* media =
       DBusThreadManager::Get()->GetBluetoothMediaClient();
   DCHECK(media);
   media->RegisterEndpoint(
@@ -322,17 +332,84 @@ void BluetoothAudioSinkChromeOS::Register(
                  weak_ptr_factory_.GetWeakPtr(), error_callback));
 }
 
+void BluetoothAudioSinkChromeOS::OnFileCanReadWithoutBlocking(int fd) {
+  ReadFromFile();
+}
+
+void BluetoothAudioSinkChromeOS::OnFileCanWriteWithoutBlocking(int fd) {
+  // Do nothing for now.
+}
+
 BluetoothMediaEndpointServiceProvider*
     BluetoothAudioSinkChromeOS::GetEndpointServiceProvider() {
   return media_endpoint_.get();
 }
 
+void BluetoothAudioSinkChromeOS::AcquireFD() {
+  VLOG(1) << "AcquireFD - transport path: " << transport_path_.value();
+
+  read_failed_logged_ = false;

[armansito, 2015/03/12 03:42:55]

I would rename this variable to something like |read_has_failed_|, so that it's
a bit more generic and you could reuse it for things other than logging in the
future.

[Miao, 2015/03/12 22:33:32]

Done.

+
+  DBusThreadManager::Get()->GetBluetoothMediaTransportClient()->Acquire(
+      transport_path_,
+      base::Bind(&BluetoothAudioSinkChromeOS::OnAcquireSucceeded,
+                 weak_ptr_factory_.GetWeakPtr()),
+      base::Bind(&BluetoothAudioSinkChromeOS::OnAcquireFailed,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
+void BluetoothAudioSinkChromeOS::WatchFD() {
+  DCHECK(file_.get() && file_->IsValid());
+
+  VLOG(1) << "WatchFD - file: " << file_->GetPlatformFile()
+          << ", file validity: " << file_->IsValid();
+
+  base::MessageLoopForIO::current()->WatchFileDescriptor(
+      file_->GetPlatformFile(), true, base::MessageLoopForIO::WATCH_READ,
+      &fd_read_watcher_, this);
+}
+
+void BluetoothAudioSinkChromeOS::StopWatchingFD() {
+  if (!file_.get()) {
+    VLOG(1) << "StopWatchingFD - skip";
+    return;
+  }
+
+  bool stopped = fd_read_watcher_.StopWatchingFileDescriptor();
+  VLOG(1) << "StopWatchingFD - watch stopped: " << stopped;
+  CHECK(stopped);
+
+  read_mtu_ = kInvalidReadMtu;
+  write_mtu_ = kInvalidWriteMtu;
+  file_.reset();

[armansito, 2015/03/12 03:42:55]

Add a comment here saying that this will close the fd.

[Miao, 2015/03/12 22:33:32]

Done.

+}
+
+void BluetoothAudioSinkChromeOS::ReadFromFile() {
+  DCHECK(file_.get() && file_->IsValid());
+  DCHECK(data_.get());
+
+  int size = file_->ReadAtCurrentPosNoBestEffort(data_.get(), read_mtu_);
+
+  if (size == -1) {
+    // To reduce the number of logs, log only once for multiple failures.
+    if (!read_failed_logged_) {
+      VLOG(1) << "ReadFromFile - failed";
+      read_failed_logged_ = true;
+    }
+    return;
+  }
+
+  VLOG(1) << "ReadFromFile - read " << size << " bytes";
+  FOR_EACH_OBSERVER(BluetoothAudioSink::Observer, observers_,
+                    BluetoothAudioSinkDataAvailable(this, data_.get(), size));
+}
+
 void BluetoothAudioSinkChromeOS::StateChanged(
     BluetoothAudioSink::State state) {
   if (state == state_)
     return;
 
-  VLOG(1) << "StateChnaged: " << StateToString(state);
+  VLOG(1) << "StateChanged - state: " << StateToString(state);
 
   switch (state) {
     case BluetoothAudioSink::STATE_INVALID:
@@ -342,18 +419,13 @@ void BluetoothAudioSinkChromeOS::StateChanged(
       ResetTransport();
       break;
     case BluetoothAudioSink::STATE_IDLE:
-      // TODO(mcchou): BUG=441581
-      // Triggered by MediaTransportPropertyChanged and SetConfiguration.
-      // Stop watching on file descriptor if there is one.
+      StopWatchingFD();
       break;
     case BluetoothAudioSink::STATE_PENDING:
-      // TODO(mcchou): BUG=441581
-      // Call BluetoothMediaTransportClient::Acquire() to get fd and mtus.
+      AcquireFD();
       break;
     case BluetoothAudioSink::STATE_ACTIVE:
-      // TODO(mcchou): BUG=441581
-      // Read from fd and call DataAvailable.
-      ReadFromFD();
+      WatchFD();
       break;
     default:
       break;
@@ -397,7 +469,7 @@ void BluetoothAudioSinkChromeOS::OnRegisterFailed(
 
 void BluetoothAudioSinkChromeOS::OnUnregisterSucceeded(
     const base::Closure& callback) {
-  VLOG(1) << "Unregisterd";
+  VLOG(1) << "Unregisterd - endpoint: " << endpoint_path_.value();

[armansito, 2015/03/12 03:42:55]

nit: s/Unregisterd/Unregistered/

[Miao, 2015/03/12 22:33:32]

Done.

 
   // Once the state becomes STATE_INVALID, media, media transport and media
   // endpoint will be reset.
@@ -415,12 +487,51 @@ void BluetoothAudioSinkChromeOS::OnUnregisterFailed(
   error_callback.Run(BluetoothAudioSink::ERROR_NOT_UNREGISTERED);
 }
 
-void BluetoothAudioSinkChromeOS::ReadFromFD() {
-  DCHECK_GE(fd_.value(), 0);
+void BluetoothAudioSinkChromeOS::OnAcquireSucceeded(
+    const int fd,
+    const uint16_t read_mtu,
+    const uint16_t write_mtu) {
+  DCHECK_GT(fd, kInvalidFd);
+  DCHECK_GT(read_mtu, kInvalidReadMtu);
+  DCHECK_GT(write_mtu, kInvalidWriteMtu);
+
+  // Avoids unnecessary memory reallocation if read MTU doesn't change.

[armansito, 2015/03/12 03:42:55]

Nice!

[Miao, 2015/03/12 22:33:32]

:)

+  if (read_mtu != read_mtu_) {
+    read_mtu_ = read_mtu;
+    data_.reset(new char[read_mtu_]);
+    VLOG(1) << "OnAcquireSucceeded - allocate " << read_mtu_
+            << " bytes of memory";
+  }
+
+  write_mtu_ = write_mtu;
+
+  // Avoids closing the same file descriptor caused by reassignment.
+  if (!file_.get() || file_->GetPlatformFile() != fd) {
+    file_.reset(new base::File(fd));
+    DCHECK(file_->IsValid());
+    VLOG(1) << "OnAcquireSucceeded - update file";
+  }
+
+  VLOG(1) << "OnAcquireSucceeded - file: " << file_->GetPlatformFile()
+          << ", read MTU: " << read_mtu_ << ", write MTU: " << write_mtu_;
+}
+
+void BluetoothAudioSinkChromeOS::OnAcquireFailed(
+    const std::string& error_name,
+    const std::string& error_message) {
+  VLOG(1) << "OnAcquireFailed - error name: " << error_name
+          << ", error message: " << error_message;
+}
+
+void BluetoothAudioSinkChromeOS::OnReleaseFDSucceeded() {
+  VLOG(1) << "OnReleaseFDSucceeded";
+}
 
-  // TODO(mcchou): BUG=441581
-  // Read from file descriptor using watcher and create a buffer to contain the
-  // data. Notify |Observers_| while there is audio data available.
+void BluetoothAudioSinkChromeOS::OnReleaseFDFailed(
+    const std::string& error_name,
+    const std::string& error_message) {
+  VLOG(1) << "OnReleaseFDFailed - error name: " << error_name
+          << ", error message: " << error_message;
 }
 
 void BluetoothAudioSinkChromeOS::ResetMedia() {
@@ -430,15 +541,18 @@ void BluetoothAudioSinkChromeOS::ResetMedia() {
 }
 
 void BluetoothAudioSinkChromeOS::ResetTransport() {
-  VLOG(1) << "ResetTransport";
-
-  if (transport_path_.value() == "")
+  if (!transport_path_.IsValid()) {
+    VLOG(1) << "ResetTransport - skip";
     return;
-  transport_path_ = dbus::ObjectPath("");
+  }
+
+  VLOG(1) << "ResetTransport - clean-up";
+
   VolumeChanged(BluetoothAudioSink::kInvalidVolume);
-  read_mtu_ = 0;
-  write_mtu_ = 0;
-  fd_.PutValue(-1);
+  transport_path_ = dbus::ObjectPath("");
+  read_mtu_ = kInvalidReadMtu;
+  write_mtu_ = kInvalidWriteMtu;
+  file_.reset();
 }
 
 void BluetoothAudioSinkChromeOS::ResetEndpoint() {