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

content/browser/renderer_host/media/audio_sync_reader.cc

Index: content/browser/renderer_host/media/audio_sync_reader.cc
diff --git a/content/browser/renderer_host/media/audio_sync_reader.cc b/content/browser/renderer_host/media/audio_sync_reader.cc
index dea8ae2a2075a8b5f7cbfcd0efca8bad86dd1672..4f855c599427c2ce3016282b465c7ef1af27fe70 100644
--- a/content/browser/renderer_host/media/audio_sync_reader.cc
+++ b/content/browser/renderer_host/media/audio_sync_reader.cc
@@ -26,7 +26,10 @@ AudioSyncReader::AudioSyncReader(base::SharedMemory* shared_memory,
       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);
@@ -68,16 +71,15 @@ void AudioSyncReader::UpdatePendingBytes(uint32 bytes) {
 
   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
@@ -173,28 +175,36 @@ bool AudioSyncReader::PrepareForeignSocketHandle(
 }
 #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