Implementing AudioOutputDevice authorization timeout

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 <stddef.h>
 #include <stdint.h>
 
 #include <cmath>
 #include <utility>
 
 #include "base/callback_helpers.h"
 #include "base/macros.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/time/time.h"
 #include "base/trace_event/trace_event.h"
 #include "build/build_config.h"
 #include "media/audio/audio_device_description.h"
 #include "media/audio/audio_output_controller.h"
 #include "media/base/limits.h"
 
+constexpr int kAuthorizationTimeoutMs = 1000;

[o1ka, 2016/06/08 15:46:06]

Not sure about the value

[o1ka, 2016/06/09 12:02:04]

Looks like renderer hang timeout is defined by content::kHungRendererDelayMs
(https://cs-staging.chromium.org/chromium/src/content/common/content_constants...),

which is (for now) defined as 5000 for android / 30000 otherwise
(https://cs-staging.chromium.org/chromium/src/content/common/content_constants...).

We can probably use something like content::kHungRendererDelayMs/2 as
authorization timeout, or stick with min(3000, kHungRendererDelayMs). Not sure
which strategy is more reasonable: is it normal to wait for 15 seconds?

[DaleCurtis, 2016/06/09 18:25:34]

I don't think we want to block for anymore than 5 seconds, but we should
definitely have a UMA to figure out how often this is ticking as well as one
which includes when the response actually comes in if possible.

[o1ka, 2016/06/10 14:21:08]

Ok. I'm not sure if we should wait for 5 seconds on Android - I think we should
be a little below the threshold.

+
 namespace media {
 
 // Takes care of invoking the render callback on the audio thread.
 // An instance of this class is created for each capture stream in
 // OnStreamCreated().
 class AudioOutputDevice::AudioThreadCallback
     : public AudioDeviceThread::Callback {
  public:
   AudioThreadCallback(const AudioParameters& audio_parameters,
                       base::SharedMemoryHandle memory,
@@ skipping 114 matching lines @@
                               session_id_, device_id_)
                               ? matched_device_id_
                               : device_id_,
                           device_status_, output_params_);
 }
 
 void AudioOutputDevice::RequestDeviceAuthorizationOnIOThread() {
   DCHECK(task_runner()->BelongsToCurrentThread());
   DCHECK_EQ(state_, IDLE);
   state_ = AUTHORIZING;
+
+  if (auth_timeout_action_)

[DaleCurtis, 2016/06/09 18:25:34]

Instead, if you use a base::OneShotTimer here you can replace all of this with 

auth_timeout_action_.Start(FROM_HERE, delay, base::Bind(...));

on l.173. It'll take care of posting the task, doing nothing when the timer
isn't running, and canceling it if it is running (as well as cleaning up the
timer on destruction).

[o1ka, 2016/06/10 14:21:08]

Thanks! Done.

+    auth_timeout_action_->Cancel();
+
+  // Create the closure on the thread it's used on. It's guaranteed to be
+  // deleted on the same thread since users must call Stop() before deleting
+  // AudioOutputDevice.
+  auth_timeout_action_.reset(new base::CancelableClosure(
+      base::Bind(&AudioOutputDevice::ProcessDeviceAuthorizationOnIOThread, this,
+                 OUTPUT_DEVICE_STATUS_ERROR_INTERNAL, media::AudioParameters(),
+                 std::string(), false)));
+
   ipc_->RequestDeviceAuthorization(this, session_id_, device_id_,
                                    security_origin_);
+  task_runner()->PostDelayedTask(
+      FROM_HERE, auth_timeout_action_->callback(),
+      base::TimeDelta::FromMilliseconds(kAuthorizationTimeoutMs));
 }
 
 void AudioOutputDevice::CreateStreamOnIOThread(const AudioParameters& params) {
   DCHECK(task_runner()->BelongsToCurrentThread());
   switch (state_) {
     case IPC_CLOSED:
       if (callback_)
         callback_->OnRenderError();
       break;
 
@@ skipping 53 matching lines @@
 void AudioOutputDevice::ShutDownOnIOThread() {
   DCHECK(task_runner()->BelongsToCurrentThread());
 
   // Close the stream, if we haven't already.
   if (state_ >= AUTHORIZING) {
     ipc_->CloseStream();
     state_ = IDLE;
   }
   start_on_authorized_ = false;
 
+  if (auth_timeout_action_) {

[DaleCurtis, 2016/06/09 18:25:34]

No need for this with the timer (or cancellable really), it'll auto drop on
destruction.

[o1ka, 2016/06/10 14:21:08]

I guess we do need this, because  both cancellable and timer are not thread-safe
and should be destroyed on the thread they are used on.
For example:
https://cs-staging.chromium.org/chromium/src/base/timer/timer.h?sq=package:ch...

+    auth_timeout_action_->Cancel();
+    auth_timeout_action_.reset(nullptr);
+  }
+
   // 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
   // in which case, we cannot use the message loop to close the thread handle
   // and can't rely on the main thread existing either.
   base::AutoLock auto_lock_(audio_thread_lock_);
   base::ThreadRestrictions::ScopedAllowIO allow_io;
@@ skipping 35 matching lines @@
     default:
       NOTREACHED();
       break;
   }
 }
 
 void AudioOutputDevice::OnDeviceAuthorized(
     OutputDeviceStatus device_status,
     const media::AudioParameters& output_params,
     const std::string& matched_device_id) {
+  ProcessDeviceAuthorizationOnIOThread(device_status, output_params,
+                                       matched_device_id, true);
+}
+
+void AudioOutputDevice::ProcessDeviceAuthorizationOnIOThread(

[o1ka, 2016/06/08 15:46:06]

Will move it to an appropriate place, now it's here to make diff simpler.

+    OutputDeviceStatus device_status,
+    const media::AudioParameters& output_params,
+    const std::string& matched_device_id,
+    bool responce_received) {

[Guido Urdaneta, 2016/06/09 12:09:49]

s/responce/response

or maybe use a more explicit name like |did_time_out|.

[o1ka, 2016/06/10 14:21:08]

Done.

   DCHECK(task_runner()->BelongsToCurrentThread());
+
+  if (responce_received) {
+    if (auth_timeout_action_)

[DaleCurtis, 2016/06/09 18:25:34]

Would just be .Stop() with timer. You can always call it outside of the nest
here and simplifyy these nested conditionals to a single line w/ return.

[o1ka, 2016/06/10 14:21:08]

Done.

+      auth_timeout_action_->Cancel();
+
+    if (state_ == IPC_CLOSED) {
+      // ProcessDeviceAuthorizationOnIOThread has already been called with
+      // |responce_received| set to false, i.e. authorization timed out, but now
+      // we received the authorization responce. Do nothing, since we can't
+      // upgrade device information after |did_receive_auth_| is signalled.
+      // TODO(olka): log UMA stat.
+      return;
+    }
+  }
+
+  if (!responce_received) {

[Guido Urdaneta, 2016/06/09 12:09:49]

else?

[o1ka, 2016/06/09 12:38:28]

Yes, I'll need "else" for UMA stats (those will be in the next patch if all the
reviewers approve the approach).

[o1ka, 2016/06/10 14:21:08]

Done.

+    DCHECK_EQ(OUTPUT_DEVICE_STATUS_ERROR_INTERNAL, device_status);
+    // TODO(olka): log UMA stat.
+  }
+
   DCHECK_EQ(state_, AUTHORIZING);
 
   // It may happen that a second authorization is received as a result to a
   // call to Start() after Stop(). If the status for the second authorization
   // differs from the first, it will not be reflected in |device_status_|
   // to avoid a race.
   // This scenario is unlikely. If it occurs, the new value will be
   // different from OUTPUT_DEVICE_STATUS_OK, so the AudioOutputDevice
   // will enter the IPC_CLOSED state anyway, which is the safe thing to do.
   // This is preferable to holding a lock.
@@ skipping 146 matching lines @@
 
   // Update the audio-delay measurement, inform about the number of skipped
   // frames, and 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(), std::round(frames_delayed),
                            frames_skipped);
 }
 
 }  // namespace media