device/bluetooth: Add Bluetooth Audio Sink base class.

device/bluetooth/bluetooth_audio_sink.h

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef DEVICE_BLUETOOTH_BLUETOOTH_AUDIO_SINK_H_
+#define DEVICE_BLUETOOTH_BLUETOOTH_AUDIO_SINK_H_
+
+#include <string>
+#include <vector>
+
+#include "base/callback.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "dbus/file_descriptor.h"

[armansito, 2014/12/02 19:20:20]

We can't have dbus includes here. The code wouldn't compile on windows/mac.

+
+namespace device {
+
+class BluetoothAdapter;
+
+// BluetoothAudioSink represents a local audio sink where a remote device can

[armansito, 2014/12/02 19:20:19]

In general you shouldn't refer to any BlueZ concepts here. What your class
represents is really a "Bluetooth A2DP Sink implementation". You should document
this class with that high-level description and perhaps go into a little more
detail but refrain from mentioning endpoints, media transports, etc that are
BlueZ API concepts.

Though it's OK to use A2DP terminology in general. Basically present this class
to your users as a way to act as an A2DP sink. How it internally does it is not
something that your users should need to know.

[Miao, 2014/12/03 20:06:38]

Done.

+// stream audio data. BluetoothAudioSink manages the creation/deletion of a BT
+// Media Endpoint and delivers the file descriptor from the BT Media Transport
+// which attaches to that endpoint.

[armansito, 2014/12/02 19:20:19]

I wouldn't go into BlueZ API details here. This is meant to be a cross-platform
API, so these concepts won't apply to Windows/Mac (even if we don't ever provide
an implementation for those platforms, we should keep this generic).

[Miao, 2014/12/03 20:06:38]

Done.

+// An user applications can obtain a pointer to a BluetoothAudioSink object via

[armansito, 2014/12/02 19:20:19]

"An user applications" -> "User applications" ?

[Miao, 2014/12/03 20:06:38]

Done.

+// the interface provided by BluetoothAdapter. The validity of
+// BluetoothAudioSink's endopint object depends on whether BluetoothAdapter is

[armansito, 2014/12/02 19:20:20]

s/endopint/endpoint/

Though I wouldn't mention endpoints here. Just say "The validity of a
BluetoothAudioSink depends on ..."

[Miao, 2014/12/03 20:06:38]

Done.

+// present and whether it is powered. Therefore, BlueotoothAudioSink has to keep
+// track of the changes on both BluetoothAdapter's state and the connection to
+// the remote device.

[armansito, 2014/12/02 19:20:19]

Get rid of the last sentence. These are implementation details that aren't
relevant to the user.

[Miao, 2014/12/03 20:06:38]

Done.

+class BluetoothAudioSink : public base::RefCounted<BluetoothAudioSink> {
+ public:
+  // Possible values indicating the connection states between the
+  // BluetoothAudioSink and the remote device. Since a BluetoothAudioSink object
+  // depends on the BluetoothAdapter object's state, the BluetoothAudioSink has
+  // to unregister its endpoint if either present or power of the
+  // BluetoothAdapter object's changes.

[armansito, 2014/12/02 19:20:19]

You don't need the second sentence. Just saying "Possible Audio Sink states" is
enough for this enum.

[Miao, 2014/12/03 20:06:38]

Done.

+  enum AudioSinkState {
+    STATE_INVALID,  // BluetoothAdapter not presented or not powered.
+    STATE_DISCONNECTED,  // Not connected.
+    STATE_IDLE,  // Connected but not streaming.
+    STATE_PENDING,  // Connected, streaming but not acquired.
+    STATE_ACTIVE,  // Connected, streaming and acquired.
+  };
+
+  // Used to configure an BT Media Endpoint.

[armansito, 2014/12/02 19:20:20]

Don't mention anything about BlueZ APIs. Just say "Options to configure an A2DP
audio sink".

[Miao, 2014/12/03 20:06:38]

Done.

+  struct AudioSinkOptions {

[armansito, 2014/12/02 19:20:20]

I'd just call this "struct Options". BluetoothAudioSink::AudioSinkOptions is a
bit wordy.

[Miao, 2014/12/03 20:06:38]

Done.

+    AudioSinkOptions();
+    ~AudioSinkOptions();
+
+    scoped_ptr<uint8_t> codec;
+    scoped_ptr<std::vector<uint8_t>> capabilities;

[armansito, 2014/12/02 19:20:20]

Aren't codec and capabilities mandatory? I would remove the scoped_ptr's here
and set them to a meaningful default value in the constructor.

[Miao, 2014/12/03 20:06:38]

Done.
Codec and capabilities are mandatory. In the case where applications may use
codecs other than SBC, these options allow applications to provide their own
configurations. I was thinking about making codec and capabilities NULL, and
while registering an audio sink, we need to check whether codec and capabilities
are NULL or not. But what you suggest is better, it is obvious that the default
values should be in the constructor of Options:)

+  };
+
+  // Interface for observing changes from Bluetooth Audio Sinks.

[armansito, 2014/12/02 19:20:20]

from a BluetoothAudioSink

[Miao, 2014/12/03 20:06:38]

Done.

+  class Observer {
+   public:
+    virtual ~Observer() {}
+
+    // Called when the state of the BluetoothAudioSink object is changed.
+    // |audio_sink| indicates the object being changed, and |audio_sink_state|
+    // indicates the new state of that object.
+    virtual void BluetoothAudioSinkStateChanged(
+        BluetoothAudioSink* audio_sink,
+        const BluetoothAudioSink::AudioSinkState& audio_sink_state) = 0;

[armansito, 2014/12/02 19:20:20]

Pass enum by value. No need for a const reference since enum is really just an
int.

[Miao, 2014/12/03 20:06:38]

Done.

+
+    // Called when the volume of the BluetoothAudioSink object is changed.
+    // |audio_sink| indicates the object being changed, and |audio_sink_volume|
+    // indicates the new volume level of that object.
+    virtual void BluetoothAudioSinkVolumeChanged(
+        BluetoothAudioSink* audio_sink,
+        uint16_t audio_sink_volume) = 0;
+
+    // Called when the file descriptor of the BluetoothAudioSink object is
+    // ready to be read. |fd| is the file descriptor, where |read_mtu| and
+    // |write_mtu| are the maximum transport units for reading and writing.
+    virtual void BluetoothAudioSinkFdAvailable(BluetoothAudioSink* audio_sink,
+                                               const dbus::FileDescriptor& fd,

[armansito, 2014/12/02 19:20:20]

Don't expose dbus objects here since the whole point of this base class is to
abstract away platform details. Find a more suitable type to represent the file
descriptor, probably your own socket abstraction that works with base::IOBuffer.
You should probably sort this out with Ben. 

If you have your own socket abstraction then you could also encapsulate the MTUs
as part of its public interface. Just a thought.

+                                               uint16_t read_mtu,
+                                               uint16_t write_mtu) = 0;
+  };
+
+  // The EndpointAcuiredCallback is used to pass the scoped_refptr of

[armansito, 2014/12/02 19:20:19]

Acuired -> Acquired

Also you called it EndpointRegisteredCallback. So you should probably correct
the comment.

[Miao, 2014/12/03 20:06:38]

Done.

+  // a BluetoothAudioSink object while a BT Media Endpoint is successfully

[armansito, 2014/12/02 19:20:19]

Don't mention BlueZ concepts.

[Miao, 2014/12/03 20:06:38]

Done.

+  // created and registered.
+  typedef base::Callback<void(
+      scoped_refptr<BluetoothAudioSink>)> EndpointRegisteredCallback;

[armansito, 2014/12/02 19:20:20]

I would call this "AudioSinkRegisteredCallback" since you're returning a
BluetoothAudioSink.

[Miao, 2014/12/03 20:06:38]

Done.

+
+  // The EndpointDeletedCallback is used to handle the clean-up while a BT Media

[armansito, 2014/12/02 19:20:20]

EndpointUnregisteredCallback?

[Miao, 2014/12/03 20:06:38]

Done.

+  // Endpoint is unregistered.
+  typedef base::Callback<void()> EndpointUnregisteredCallback;

[armansito, 2014/12/02 19:20:20]

This typedef is unnecessary since you're method can just accept a base::Closure
instead.

[Miao, 2014/12/03 20:06:38]

Done.

+
+  // The ErrorCallback is used for the methods that can fail in which case it
+  // can fail.

[armansito, 2014/12/02 19:20:19]

"in which case it can fail" -> "in which case it is called"

[Miao, 2014/12/03 20:06:38]

Done.

[Miao, 2014/12/03 20:06:38]

Done.

+  typedef base::Callback<void(const std::string& error_message)> ErrorCallback;
+
+  virtual ~BluetoothAudioSink();
+
+  // Adds and removes a observer for events on the BluetoothAudioSink object. If
+  // monitoring multiple audio sinks, check the |audio_sink| parameter of
+  // observer methods to determine which audio sink is issuing the event.
+  virtual void AddObserver(Observer* observer) = 0;
+  virtual void RemoveObserver(Observer* observer) = 0;
+
+  // Registers a BT Media Endpoint. User applications can use |options| to
+  // configure a new BT Media Endpoint which will bound to the audio sink.
+  // |callback| will be executed if the endpoint is successfully created,
+  // otherwise |error_callback| will be called.
+  virtual void RegisterEndpoint(const AudioSinkOptions& options,
+                                const EndpointRegisteredCallback& callback,
+                                const ErrorCallback& error_callback) = 0;

[armansito, 2014/12/02 19:20:19]

I thought we were going to have this method in BluetoothAdapter (which is what
you say in the top-level comment above). It also doesn't make sense to have a
RegisterEndpoint instance method in BluetoothAudioSink that returns a new
instance of a BluetoothAudioSink...

[Miao, 2014/12/03 20:06:38]

Sorry about the confusion, there is no doubt that
RegisterEndpoint/UnregisterEndpoint will be placed in BluetoothAdapter.
RegisterEndpoint and UnregisterEndpoint should be renamed, and they handle the
actual works to register/unregister an audio sink.

+
+  // Unregister the BT Media Endpoint if there is one. |callback| should handle
+  // the clean-up after the endpoint is deleted successfully, otherwise
+  // |error_callback| will be called.
+  virtual void UnregisterEndpoint(const EndpointUnregisteredCallback& callback,

[armansito, 2014/12/02 19:20:19]

I think just base::Closure is sufficient.

[armansito, 2014/12/02 19:20:20]

Should we call this method just "Unregister"? I don't think we should mention
BlueZ concepts here.

[Miao, 2014/12/03 20:06:38]

Done.

+                                  const ErrorCallback& error_callback) = 0;
+
+ protected:
+  BluetoothAudioSink();
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(BluetoothAudioSink);
+};
+
+}  // namespace device
+
+#endif  // DEVICE_BLUETOOTH_BLUETOOTH_AUDIO_SINK_H_