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

media/audio/audio_output_device.cc

 // Copyright (c) 2012 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.
 
 #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"
 #include "base/trace_event/trace_event.h"
 #include "media/audio/audio_output_controller.h"
 #include "media/base/limits.h"
 
 namespace media {
 
 // Takes care of invoking the render callback on the audio thread.
@@ skipping 16 matching lines @@
  private:
   AudioRendererSink::RenderCallback* render_callback_;
   scoped_ptr<AudioBus> output_bus_;
   uint64 callback_num_;
   DISALLOW_COPY_AND_ASSIGN(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() {
   // The current design requires that the user calls Stop() before deleting
   // this class.
   DCHECK(audio_thread_.IsStopped());
 
   // The following makes it possible for |current_switch_callback_| to release
   // 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,
       base::Bind(&AudioOutputDevice::CreateStreamOnIOThread, this,
                  audio_parameters_));
 }
 
 void AudioOutputDevice::Stop() {
   {
     base::AutoLock auto_lock(audio_thread_lock_);
@@ skipping 28 matching lines @@
 }
 
 OutputDevice* AudioOutputDevice::GetOutputDevice() {
   return this;
 }
 
 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_ = CREATING_STREAM;
-    ipc_->CreateStream(this, params, session_id_);
+    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);
+  }
+}
+
 void AudioOutputDevice::PlayOnIOThread() {
   DCHECK(task_runner()->BelongsToCurrentThread());
   if (state_ == PAUSED) {
     TRACE_EVENT_ASYNC_BEGIN0(
         "audio", "StartingPlayback", audio_callback_.get());
     ipc_->PlayStream();
     state_ = PLAYING;
     play_on_start_ = false;
   } else {
     play_on_start_ = true;
   }
 }
 
 void AudioOutputDevice::PauseOnIOThread() {
   DCHECK(task_runner()->BelongsToCurrentThread());
   if (state_ == PLAYING) {
     TRACE_EVENT_ASYNC_END0(
         "audio", "StartingPlayback", audio_callback_.get());
     ipc_->PauseStream();
     state_ = PAUSED;
   }
   play_on_start_ = false;
 }
 
 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;
   }
 
   // We can run into an issue where ShutDownOnIOThread is called right after
   // OnStreamCreated is called in cases where Start/Stop are called before we
   // get the OnStreamCreated callback.  To handle that corner case, we call
   // Stop(). In most cases, the thread will already be stopped.
   //
   // Another situation is when the IO thread goes away before Stop() is called
@@ skipping 10 matching lines @@
   DCHECK(task_runner()->BelongsToCurrentThread());
   if (state_ >= CREATING_STREAM)
     ipc_->SetVolume(volume);
 }
 
 void AudioOutputDevice::SwitchOutputDeviceOnIOThread(
     const std::string& device_id,
     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.

   }
 }
 
 void AudioOutputDevice::OnStateChanged(AudioOutputIPCDelegateState state) {
   DCHECK(task_runner()->BelongsToCurrentThread());
 
   // Do nothing if the stream has been closed.
   if (state_ < CREATING_STREAM)
     return;
 
@@ skipping 13 matching lines @@
       // a callback object via Start() and clear it in Stop().
       if (!audio_thread_.IsStopped())
         callback_->OnRenderError();
       break;
     default:
       NOTREACHED();
       break;
   }
 }
 
+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,
     int length) {
   DCHECK(task_runner()->BelongsToCurrentThread());
   DCHECK(base::SharedMemory::IsHandleValid(handle));
 #if defined(OS_WIN)
   DCHECK(socket_handle);
 #else
   DCHECK_GE(socket_handle, 0);
 #endif
   DCHECK_GT(length, 0);
 
   if (state_ != CREATING_STREAM)
     return;
 
   // We can receive OnStreamCreated() on the IO thread after the client has
   // called Stop() but before ShutDownOnIOThread() is processed. In such a
   // situation |callback_| might point to freed memory. Instead of starting
   // |audio_thread_| do nothing and wait for ShutDownOnIOThread() to get called.
   //
   // TODO(scherkus): The real fix is to have sane ownership semantics. The fact
   // that |callback_| (which should own and outlive this object!) can point to
   // freed memory is a mess. AudioRendererSink should be non-refcounted so that
   // owners (WebRtcAudioDeviceImpl, AudioRendererImpl, etc...) can Stop() and
   // 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);
 }
 
 void AudioOutputDevice::OnIPCClosed() {
   DCHECK(task_runner()->BelongsToCurrentThread());
   state_ = IPC_CLOSED;
   ipc_.reset();
 }
 
 void AudioOutputDevice::WillDestroyCurrentMessageLoop() {
   LOG(ERROR) << "IO loop going away before the audio device has been stopped";
   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(
     const AudioParameters& audio_parameters,
     base::SharedMemoryHandle memory,
     int memory_length,
     AudioRendererSink::RenderCallback* render_callback)
     : AudioDeviceThread::Callback(audio_parameters, memory, memory_length, 1),
       render_callback_(render_callback),
       callback_num_(0) {}
@@ skipping 26 matching lines @@
     TRACE_EVENT_ASYNC_END0("audio", "StartingPlayback", this);
   }
 
   // Update the audio-delay measurement then ask client to render audio.  Since
   // |output_bus_| is wrapping the shared memory the Render() call is writing
   // directly into the shared memory.
   render_callback_->Render(output_bus_.get(), audio_delay_milliseconds);
 }
 
 }  // namespace media.