Allow AudioOutputDevice objects to be initialized with a specific hardware output device and store …

media/audio/audio_output_device.cc

Index: media/audio/audio_output_device.cc
diff --git a/media/audio/audio_output_device.cc b/media/audio/audio_output_device.cc
index f65e2aa19be980a4f5de845b1b937cd9e482fbe3..219e8d19e32729be887d2ea95bb8e8c7742e2a9e 100644
--- a/media/audio/audio_output_device.cc
+++ b/media/audio/audio_output_device.cc
@@ -4,8 +4,6 @@
 
 #include "media/audio/audio_output_device.h"
 
-#include <string>
-
 #include "base/callback_helpers.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/time/time.h"
@@ -42,37 +40,48 @@ class AudioOutputDevice::AudioThreadCallback
 AudioOutputDevice::AudioOutputDevice(
     scoped_ptr<AudioOutputIPC> ipc,
     const scoped_refptr<base::SingleThreadTaskRunner>& io_task_runner)
+    : AudioOutputDevice(ipc.Pass(),
+                        io_task_runner,
+                        0,
+                        std::string(),
+                        GURL::EmptyGURL()) {}
+
+AudioOutputDevice::AudioOutputDevice(
+    scoped_ptr<AudioOutputIPC> ipc,
+    const scoped_refptr<base::SingleThreadTaskRunner>& io_task_runner,
+    int session_id,
+    const std::string& device_id,
+    const GURL& security_origin)
     : ScopedTaskRunnerObserver(io_task_runner),
       callback_(NULL),
       ipc_(ipc.Pass()),
       state_(IDLE),
       play_on_start_(true),
-      session_id_(-1),
+      session_id_(session_id),
+      device_id_(device_id),
+      security_origin_(security_origin),
       stopping_hack_(false),
-      current_switch_request_id_(0) {
+      switch_output_device_on_start_(false),
+      did_set_output_params_(true, false),
+      authorization_failed_(false) {

[DaleCurtis, 2015/09/09 01:31:57]

Should this be another state? That avoid mixing two types of state machines,
bool + fsm.

[Guido Urdaneta, 2015/09/09 16:22:23]

Done. Now we have NOT_AUTHORIZED and AUTHORIZED as states.

   CHECK(ipc_);
 
   // The correctness of the code depends on the relative values assigned in the
   // State enum.
   static_assert(IPC_CLOSED < IDLE, "invalid enum value assignment 0");
-  static_assert(IDLE < CREATING_STREAM, "invalid enum value assignment 1");
-  static_assert(CREATING_STREAM < PAUSED, "invalid enum value assignment 2");
-  static_assert(PAUSED < PLAYING, "invalid enum value assignment 3");
+  static_assert(IDLE < AUTHORIZING, "invalid enum value assignment 1");
+  static_assert(AUTHORIZING < CREATING_STREAM,

[DaleCurtis, 2015/09/09 01:31:57]

Seems there should be an authorized state?

[Guido Urdaneta, 2015/09/09 16:22:23]

Done.

+                "invalid enum value assignment 2");
+  static_assert(CREATING_STREAM < PAUSED, "invalid enum value assignment 3");
+  static_assert(PAUSED < PLAYING, "invalid enum value assignment 4");
 }
 
-void AudioOutputDevice::InitializeWithSessionId(const AudioParameters& params,
-                                                RenderCallback* callback,
-                                                int session_id) {
+void AudioOutputDevice::Initialize(const AudioParameters& params,
+                                   RenderCallback* callback) {
   DCHECK(!callback_) << "Calling InitializeWithSessionId() twice?";
   DCHECK(params.IsValid());
   audio_parameters_ = params;
   callback_ = callback;
-  session_id_ = session_id;
-}
-
-void AudioOutputDevice::Initialize(const AudioParameters& params,
-                                   RenderCallback* callback) {
-  InitializeWithSessionId(params, callback, 0);
 }
 
 AudioOutputDevice::~AudioOutputDevice() {
@@ -84,11 +93,18 @@ AudioOutputDevice::~AudioOutputDevice() {
   // its bound parameters in the correct thread instead of implicitly releasing
   // them in the thread where this destructor runs.
   if (!current_switch_callback_.is_null()) {
-    base::ResetAndReturn(&current_switch_callback_).Run(
-        SWITCH_OUTPUT_DEVICE_RESULT_ERROR_OBSOLETE);
+    base::ResetAndReturn(&current_switch_callback_)
+        .Run(SWITCH_OUTPUT_DEVICE_RESULT_ERROR_INTERNAL);
   }
 }
 
+void AudioOutputDevice::RequestDeviceAuthorization() {
+  task_runner()->PostTask(
+      FROM_HERE,
+      base::Bind(&AudioOutputDevice::RequestDeviceAuthorizationOnIOThread,
+                 this));
+}
+
 void AudioOutputDevice::Start() {
   DCHECK(callback_) << "Initialize hasn't been called";
   task_runner()->PostTask(FROM_HERE,
@@ -137,17 +153,40 @@ void AudioOutputDevice::SwitchOutputDevice(
     const std::string& device_id,
     const GURL& security_origin,
     const SwitchOutputDeviceCB& callback) {
-  DVLOG(1) << __FUNCTION__ << "(" << device_id << ")";
   task_runner()->PostTask(
       FROM_HERE, base::Bind(&AudioOutputDevice::SwitchOutputDeviceOnIOThread,
                             this, device_id, security_origin, callback));
 }
 
-void AudioOutputDevice::CreateStreamOnIOThread(const AudioParameters& params) {
+AudioParameters AudioOutputDevice::GetOutputParameters() {
+  CHECK(!task_runner()->BelongsToCurrentThread());
+  did_set_output_params_.Wait();
+  return output_params_;
+}
+
+void AudioOutputDevice::RequestDeviceAuthorizationOnIOThread() {
   DCHECK(task_runner()->BelongsToCurrentThread());
+  // Request authorization only if the stream is stopped
   if (state_ == IDLE) {

[DaleCurtis, 2015/09/09 01:31:57]

DCHECK_EQ? Nothing should be calling this before the factory right?

[Guido Urdaneta, 2015/09/09 16:22:23]

Done.

+    state_ = AUTHORIZING;
+    ipc_->RequestDeviceAuthorization(this, session_id_, device_id_,
+                                     security_origin_);
+  }
+}
+
+void AudioOutputDevice::CreateStreamOnIOThread(const AudioParameters& params) {
+  DCHECK(task_runner()->BelongsToCurrentThread());
+  if (authorization_failed_) {
+    callback_->OnRenderError();
+    return;
+  }
+
+  // Request device authorization again if the stream was stopped

[DaleCurtis, 2015/09/09 01:31:57]

Why when stopped? Shouldn't authorization persist until switch device is called?

[Guido Urdaneta, 2015/09/09 16:22:23]

For practical reasons, authorization also ends at Stop().
The reason is that the browser (ARH) keeps track of authorizations per stream.
Once a stream is closed, ARH forgets everything about the stream, including
authorizations.
An improvement I made is that AOD does not send this IPC if it's the default
device and the parameters are already known.

Alternative approaches would be to keep track of authorizations per AOD or per
renderer frame, but I think that would add a lot of complexity.
Doing it per AOD would require some sort of AOD ID, which currently does not
exist. Doing it per renderer frame does not require a new ID, but in this case
the state in ARH would be more complex, as authorizations would be shared across
all streams/AODs from the same renderer frame.

My take is that Start()/Stop()/Start()/Stop() is not so frequent, and if it
happens I hope it will be mostly with the default device, which does not require
an extra IPC; and if it's not the default device, it will be with a session ID,
which requires an IPC, but not a translation and actual permission check.

+  RequestDeviceAuthorizationOnIOThread();

[DaleCurtis, 2015/09/09 01:31:57]

Why is this necessary? If it's a non-default device the authorization must have
completed on the browser side at least before the CreateStream IPC below will
run. For default, the browser side doesn't care.

Are you doing this to ensure the browser side always gets a auth request first?
Seems this should only be called if !AUTHORIZING or !AUTHORIZED.

[Guido Urdaneta, 2015/09/09 16:22:23]

I guess my comment above explains why it is necessary to request authorization
again for nondefault devices in a Start() after a Stop(), or if parameters are
not known.

+

[DaleCurtis, 2015/09/09 01:31:57]

DCHECK(state_ == AUTHORIZING || state_ == AUTHORIZED); ?

[Guido Urdaneta, 2015/09/09 16:22:23]

Can also be NOT_AUTHORIZED, or IDLE after a Stop() (see explanation above). 

+  if (state_ == AUTHORIZING) {
     state_ = CREATING_STREAM;
-    ipc_->CreateStream(this, params, session_id_);
+    ipc_->CreateStream(this, params);
   }
 }
 
@@ -179,7 +218,7 @@ void AudioOutputDevice::ShutDownOnIOThread() {
   DCHECK(task_runner()->BelongsToCurrentThread());
 
   // Close the stream, if we haven't already.
-  if (state_ >= CREATING_STREAM) {
+  if (state_ > IDLE) {
     ipc_->CloseStream();
     state_ = IDLE;
   }
@@ -210,13 +249,21 @@ void AudioOutputDevice::SwitchOutputDeviceOnIOThread(
     const GURL& security_origin,
     const SwitchOutputDeviceCB& callback) {
   DCHECK(task_runner()->BelongsToCurrentThread());
-  DVLOG(1) << __FUNCTION__ << "(" << device_id << "," << security_origin << ")";
+
+  // Do not allow concurrent SwitchOutputDevice requests
+  if (!current_switch_callback_.is_null()) {
+    callback.Run(SWITCH_OUTPUT_DEVICE_RESULT_ERROR_INTERNAL);
+    return;
+  }
+
+  current_switch_callback_ = callback;
+  current_switch_device_id_ = device_id;
+  current_switch_security_origin_ = security_origin;
   if (state_ >= CREATING_STREAM) {

[DaleCurtis, 2015/09/09 01:31:57]

I think this code should instead store the current state, shutdown the shared
memory and restore the state once the authorization comes through.  Otherwise we
have the shared memory size issue, no?

[Guido Urdaneta, 2015/09/09 16:22:23]

We still have the shared memory size issue.
The original idea was to reject switch requests in ARH if the parameters for the
new device differ from the current device.
That's easy to do and solves the shared memory size issue. I did not include it
in this CL to make it less complex than it already is.

Also, the plan is to remove SwitchOutputDevice() from AOD by making ARMI and
WebRTC to create new AODs when the device needs to be switched. For ARMI it's
easy. I haven't checked WebRTC yet, but it should be doable.

-    SetCurrentSwitchRequest(callback);
-    ipc_->SwitchOutputDevice(device_id, security_origin,
-                             current_switch_request_id_);
+    ipc_->SwitchOutputDevice(current_switch_device_id_,
+                             current_switch_security_origin_);
   } else {
-    callback.Run(SWITCH_OUTPUT_DEVICE_RESULT_ERROR_NOT_SUPPORTED);
+    switch_output_device_on_start_ = true;

[DaleCurtis, 2015/09/09 01:31:57]

Instead of having this, can we set the state back to AUTHORIZING and let
CreateStreamOnIO handle resending the authorization?

[Guido Urdaneta, 2015/09/09 16:22:23]

This isn't intended to address authorization problems, but a  practical problem
that Web developers have reported. Sometimes they call setSinkId() in JavaScript
too quickly and they get an error because the stream has not been created yet.
This fixes that in a way similar to how Play()/Pause() are handled before the
stream is created.
I think this is better than having Web developers implement workarounds due to
an internal implementation detail.

   }
 }
 
@@ -250,6 +297,20 @@ void AudioOutputDevice::OnStateChanged(AudioOutputIPCDelegateState state) {
   }
 }
 
+void AudioOutputDevice::OnDeviceAuthorized(
+    bool success,
+    const media::AudioParameters& output_params) {
+  DCHECK(task_runner()->BelongsToCurrentThread());
+  if (success) {
+    SetOutputParams(output_params);

[DaleCurtis, 2015/09/09 01:31:57]

Seems small to be its own function, just inline?

[Guido Urdaneta, 2015/09/09 16:22:23]

Done.

+    return;
+  }
+
+  authorization_failed_ = true;
+  if (callback_)
+    callback_->OnRenderError();
+}
+
 void AudioOutputDevice::OnStreamCreated(
     base::SharedMemoryHandle handle,
     base::SyncSocket::Handle socket_handle,
@@ -278,46 +339,40 @@ void AudioOutputDevice::OnStreamCreated(
   // delete as they see fit. AudioOutputDevice should internally use WeakPtr
   // to handle teardown and thread hopping. See http://crbug.com/151051 for
   // details.
-  base::AutoLock auto_lock(audio_thread_lock_);
-  if (stopping_hack_)
-    return;
+  {
+    base::AutoLock auto_lock(audio_thread_lock_);
+    if (stopping_hack_)
+      return;
+
+    DCHECK(audio_thread_.IsStopped());
+    audio_callback_.reset(new AudioOutputDevice::AudioThreadCallback(
+        audio_parameters_, handle, length, callback_));
+    audio_thread_.Start(audio_callback_.get(), socket_handle,
+                        "AudioOutputDevice", true);
+    state_ = PAUSED;
 
-  DCHECK(audio_thread_.IsStopped());
-  audio_callback_.reset(new AudioOutputDevice::AudioThreadCallback(
-      audio_parameters_, handle, length, callback_));
-  audio_thread_.Start(
-      audio_callback_.get(), socket_handle, "AudioOutputDevice", true);
-  state_ = PAUSED;
-
-  // We handle the case where Play() and/or Pause() may have been called
-  // multiple times before OnStreamCreated() gets called.
-  if (play_on_start_)
-    PlayOnIOThread();
-}
+    // We handle the case where Play() and/or Pause() may have been called
+    // multiple times before OnStreamCreated() gets called.
+    if (play_on_start_)
+      PlayOnIOThread();
+  }
 
-void AudioOutputDevice::SetCurrentSwitchRequest(
-    const SwitchOutputDeviceCB& callback) {
-  DCHECK(task_runner()->BelongsToCurrentThread());
-  DVLOG(1) << __FUNCTION__;
-  // If there is a previous unresolved request, resolve it as obsolete
-  if (!current_switch_callback_.is_null()) {
-    base::ResetAndReturn(&current_switch_callback_).Run(
-        SWITCH_OUTPUT_DEVICE_RESULT_ERROR_OBSOLETE);
+  if (switch_output_device_on_start_) {

[DaleCurtis, 2015/09/09 01:31:57]

Hmm, at this point the stream needs to be shutdown and restarted, so perhaps
this should just invoke SwitchOutputDeviceOnIOThread after changing the state to
PAUSED?

[Guido Urdaneta, 2015/09/09 16:22:23]

Why does the stream need to be shutdown and restarted?
The intent here is just to send a SwitchOutputDevice IPC after the stream is
created to avoid sending an error to JavaScript when setSinkId() (javascript's
name for SwitchOutputDevice) is called too quickly (e.g., when the stream is
initially muted).

+    ipc_->SwitchOutputDevice(current_switch_device_id_,
+                             current_switch_security_origin_);
   }
-  current_switch_callback_ = callback;
-  current_switch_request_id_++;
 }
 
 void AudioOutputDevice::OnOutputDeviceSwitched(
-    int request_id,
-    SwitchOutputDeviceResult result) {
+    SwitchOutputDeviceResult result,
+    const media::AudioParameters& output_params) {
   DCHECK(task_runner()->BelongsToCurrentThread());
-  DCHECK(request_id <= current_switch_request_id_);
-  DVLOG(1) << __FUNCTION__
-           << "(" << request_id << ", " << result << ")";
-  if (request_id != current_switch_request_id_) {
-    return;
+  if (result == SWITCH_OUTPUT_DEVICE_RESULT_SUCCESS) {
+    session_id_ = 0;  // Output device is no longer attached to an input device
+    device_id_ = current_switch_device_id_;

[DaleCurtis, 2015/09/09 01:31:57]

Clear switch ids?

[Guido Urdaneta, 2015/09/09 16:22:23]

We can clear them, but would there be an advantage?
Once |current_switch_callback_| is null, whatever value the switch ids have is
meaningless.

+    security_origin_ = current_switch_security_origin_;
   }
+  DCHECK(!current_switch_callback_.is_null());
   base::ResetAndReturn(&current_switch_callback_).Run(result);
 }
 
@@ -332,6 +387,15 @@ void AudioOutputDevice::WillDestroyCurrentMessageLoop() {
   ShutDownOnIOThread();
 }
 
+void AudioOutputDevice::SetOutputParams(
+    const media::AudioParameters& output_params) {
+  DCHECK(task_runner()->BelongsToCurrentThread());
+  if (!did_set_output_params_.IsSignaled()) {
+    did_set_output_params_.Signal();
+    output_params_ = output_params;
+  }
+}
+
 // AudioOutputDevice::AudioThreadCallback
 
 AudioOutputDevice::AudioThreadCallback::AudioThreadCallback(