Switch audio synchronization from sleep() based to select() based.

content/browser/renderer_host/media/audio_sync_reader.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 "content/browser/renderer_host/media/audio_sync_reader.h"
 
 #include <algorithm>
 
 #include "base/command_line.h"
 #include "base/memory/shared_memory.h"
 #include "base/metrics/histogram.h"
 #include "content/public/common/content_switches.h"
 #include "media/audio/audio_buffers_state.h"
 #include "media/audio/audio_parameters.h"
 #include "media/audio/shared_memory_util.h"
 
 using media::AudioBus;
 
 namespace content {
 
 AudioSyncReader::AudioSyncReader(base::SharedMemory* shared_memory,
                                  const media::AudioParameters& params,
                                  int input_channels)
     : shared_memory_(shared_memory),
       input_channels_(input_channels),
       mute_audio_(CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kMuteAudio)),
       renderer_callback_count_(0),
-      renderer_missed_callback_count_(0) {
+      renderer_missed_callback_count_(0),
+      // TODO(dalecurtis): Cap it at 20ms?
+      maximum_wait_time_(params.GetBufferDuration() / 2),

[henrika (OOO until Aug 14), 2013/08/20 07:50:30]

Just checking; what is the unit of GetBufferDuration()?

[DaleCurtis, 2013/09/11 01:16:03]

base::TimeDelta

+      buffer_index_(0) {
   packet_size_ = media::PacketSizeInBytes(shared_memory_->requested_size());
   int input_memory_size = 0;
   int output_memory_size = AudioBus::CalculateMemorySize(params);
   if (input_channels_ > 0) {
     // The input storage is after the output storage.
     int frames = params.frames_per_buffer();
     input_memory_size = AudioBus::CalculateMemorySize(input_channels_, frames);
     char* input_data =
         static_cast<char*>(shared_memory_->memory()) + output_memory_size;
     input_bus_ = AudioBus::WrapMemory(input_channels_, frames, input_data);
@@ skipping 21 matching lines @@
 // media::AudioOutputController::SyncReader implementations.
 void AudioSyncReader::UpdatePendingBytes(uint32 bytes) {
   if (bytes != static_cast<uint32>(media::kPauseMark)) {
     // Store unknown length of data into buffer, so we later
     // can find out if data became available.
     media::SetUnknownDataSize(shared_memory_, packet_size_);
   }
 
   if (socket_) {
     socket_->Send(&bytes, sizeof(bytes));
+    buffer_index_++;

[tommi (sloooow) - chröme, 2013/08/20 10:55:57]

nit: ++buffer_index_;

[DaleCurtis, 2013/09/11 01:16:03]

Done.

   }
 }
 
 int AudioSyncReader::Read(bool block, const AudioBus* source, AudioBus* dest) {
   ++renderer_callback_count_;
-  if (!DataReady()) {
+  if (!WaitTillDataReady()) {
     ++renderer_missed_callback_count_;
-
-    if (block)
-      WaitTillDataReady();
+    return 0;
   }
 
   // Copy optional synchronized live audio input for consumption by renderer
   // process.
   if (source && input_bus_) {
     DCHECK_EQ(source->channels(), input_bus_->channels());
     // TODO(crogers): In some cases with device and sample-rate changes
     // it's possible for an AOR to insert a resampler in the path.
     // Because this is used with the Web Audio API, it'd be better
     // to bypass the device change handling in AOR and instead let
@@ skipping 75 matching lines @@
     base::ProcessHandle process_handle,
     base::FileDescriptor* foreign_handle) {
   foreign_handle->fd = foreign_socket_->handle();
   foreign_handle->auto_close = false;
   if (foreign_handle->fd != -1)
     return true;
   return false;
 }
 #endif
 
-void AudioSyncReader::WaitTillDataReady() {
+bool AudioSyncReader::WaitTillDataReady() {
+  if (!socket_)

[tommi (sloooow) - chröme, 2013/08/20 10:55:57]

is this something exected or unexpected (i.e. [D]CHECK)?

[DaleCurtis, 2013/09/11 01:16:03]

Set during Init() and never changed.  Checks removed.

+    return false;
+
+  size_t bytes_received = 0;
+  uint32 renderer_buffer_index = 0;
+
   base::TimeTicks start = base::TimeTicks::Now();
-  const base::TimeDelta kMaxWait = base::TimeDelta::FromMilliseconds(20);
-#if defined(OS_WIN)
-  // Sleep(0) on Windows lets the other threads run.
-  const base::TimeDelta kSleep = base::TimeDelta::FromMilliseconds(0);
-#else
-  // We want to sleep for a bit here, as otherwise a backgrounded renderer won't
-  // get enough cpu to send the data and the high priority thread in the browser
-  // will use up a core causing even more skips.
-  const base::TimeDelta kSleep = base::TimeDelta::FromMilliseconds(2);
-#endif
-  base::TimeDelta time_since_start;
   do {
-    base::PlatformThread::Sleep(kSleep);
-    time_since_start = base::TimeTicks::Now() - start;
-  } while (!DataReady() && time_since_start < kMaxWait);
+    bytes_received = socket_->Receive(

[tommi (sloooow) - chröme, 2013/08/20 10:55:57]

Since we're using sockets, we should be able to check multiple in one go.
I did some work a long time ago that could possibly be used in this case.  Mind
taking a look?

https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/cross_...
There are tests etc there as well.

[DaleCurtis, 2013/08/20 18:27:51]

Not sure I follow which "multiple" items you want to check here?  We only have
one socket.  Can you elaborate on how and what waiting on multiple descriptors
would help with?

Yeah your code was my inspiration here; I wanted something that didn't need to
construct additional sockets though, so I came up with this back and forth model
w/ an index.

[tommi (sloooow) - chröme, 2013/08/21 12:33:26]

Sorry, for some reason I parsed this loop as cycling through sockets.
Great that you're aware of it and even better that you found it useful! ;-)

+        &renderer_buffer_index, sizeof(renderer_buffer_index),
+        maximum_wait_time_);

[DaveMoore, 2013/08/19 22:49:43]

Does it mean anything (skipping) if something other than the right buffer_index
is read before we finally get the right one? Should there be an error or
histogram about it?

[DaleCurtis, 2013/08/19 23:08:22]

It means we timed out on one or more previous calls.  I'd expect this to loop no
more than once in most cases.  Timeouts are now recorded via
renderer_missed_callback_count_ since this change implies we're living in a
world where having to wait is unavoidable.  That variable maps to the
Media.AudioRendererMissedDeadline percentage in UMA.

Previously the miss count was incremented prior to data waiting; which meant the
miss may have been recoverable w/ some waiting.

+  } while (bytes_received > 0 && renderer_buffer_index != buffer_index_);
+
+  base::TimeDelta time_since_start = base::TimeTicks::Now() - start;
   UMA_HISTOGRAM_CUSTOM_TIMES("Media.AudioOutputControllerDataNotReady",
                              time_since_start,
                              base::TimeDelta::FromMilliseconds(1),
                              base::TimeDelta::FromMilliseconds(1000),
                              50);
+
+  // Receive() timed out or another unknown error occurred.

[henrika (OOO until Aug 14), 2013/08/20 07:50:30]

Could you add some more details about what can cause Receive to time out. Given
that is such an essential part of this method.

[DaleCurtis, 2013/09/11 01:16:03]

Done.

+  if (!bytes_received) {
+    DVLOG(2) << "AudioSyncReader::WaitTillDataReady() timed out.";
+    return false;
+  }
+
+  DCHECK_EQ(bytes_received, sizeof(renderer_buffer_index));
+  DCHECK(DataReady());
+  return true;
 }
 
 }  // namespace content