Merge 286955 "Delay back to back PulseAudio write request callba..."

> Delay back to back PulseAudio write request callbacks.
> 
> Avoids trampling shared memory when Pulse issues back to back
> callbacks for whatever reason.  Locally I hit the sleep many
> times per stream without any noticeable glitching.
> 
> I've also adjusted the |minreq| value to reduce the number of
> callbacks, in practice this shouldn't hurt buffering since we
> always write out a full buffer regardless of the amount requested.
> 
> BUG=366433
> TEST=pulse output is glitch free on WebAudio, HTML5, WebRTC.
> 
> Review URL: https://codereview.chromium.org/414313005

TBR=dalecurtis@chromium.org

Review URL: https://codereview.chromium.org/473563002

git-svn-id: svn://svn.chromium.org/chrome/branches/2062/src@289331 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 40 insertions, 15 deletions

diff --git a/media/audio/pulse/pulse_output.cc b/media/audio/pulse/pulse_output.cc
index 19fc47b..e0a0b42 100644
--- a/media/audio/pulse/pulse_output.cc
+++ b/media/audio/pulse/pulse_output.cc
@@ -124,26 +124,31 @@ void PulseAudioOutputStream::FulfillWriteRequest(size_t requested_bytes) {
     CHECK_GE(pa_stream_begin_write(pa_stream_, &buffer, &bytes_to_fill), 0);
     CHECK_EQ(bytes_to_fill, static_cast<size_t>(params_.GetBytesPerBuffer()));
 
+    // NOTE: |bytes_to_fill| may be larger than |requested_bytes| now, this is
+    // okay since pa_stream_begin_write() is the authoritative source on how
+    // much can be written.
+
     int frames_filled = 0;
     if (source_callback_) {
-      uint32 hardware_delay = pulse::GetHardwareLatencyInBytes(
-          pa_stream_, params_.sample_rate(),
-          params_.GetBytesPerFrame());
+      const uint32 hardware_delay = pulse::GetHardwareLatencyInBytes(
+          pa_stream_, params_.sample_rate(), params_.GetBytesPerFrame());
       frames_filled = source_callback_->OnMoreData(
           audio_bus_.get(), AudioBuffersState(0, hardware_delay));
-    }
 
-    // Zero any unfilled data so it plays back as silence.
-    if (frames_filled < audio_bus_->frames()) {
-      audio_bus_->ZeroFramesPartial(
-          frames_filled, audio_bus_->frames() - frames_filled);
-    }
+      // Zero any unfilled data so it plays back as silence.
+      if (frames_filled < audio_bus_->frames()) {
+        audio_bus_->ZeroFramesPartial(
+            frames_filled, audio_bus_->frames() - frames_filled);
+      }
 
-    // Note: If this ever changes to output raw float the data must be clipped
-    // and sanitized since it may come from an untrusted source such as NaCl.
-    audio_bus_->Scale(volume_);
-    audio_bus_->ToInterleaved(
-        audio_bus_->frames(), params_.bits_per_sample() / 8, buffer);
+      // Note: If this ever changes to output raw float the data must be clipped
+      // and sanitized since it may come from an untrusted source such as NaCl.
+      audio_bus_->Scale(volume_);
+      audio_bus_->ToInterleaved(
+          audio_bus_->frames(), params_.bits_per_sample() / 8, buffer);
+    } else {
+      memset(buffer, 0, bytes_to_fill);
+    }
 
     if (pa_stream_write(pa_stream_, buffer, bytes_to_fill, NULL, 0LL,
                         PA_SEEK_RELATIVE) < 0) {
@@ -152,7 +157,23 @@ void PulseAudioOutputStream::FulfillWriteRequest(size_t requested_bytes) {
       }
     }
 
+    // NOTE: As mentioned above, |bytes_remaining| may be negative after this.
     bytes_remaining -= bytes_to_fill;
+
+    // Despite telling Pulse to only request certain buffer sizes, it will not
+    // always obey.  In these cases we need to avoid back to back reads from
+    // the renderer as it won't have time to complete the request.
+    //
+    // We can't defer the callback as Pulse will never call us again until we've
+    // satisfied writing the requested number of bytes.
+    //
+    // TODO(dalecurtis): It might be worth choosing the sleep duration based on
+    // the hardware latency return above.  Watch http://crbug.com/366433 to see
+    // if a more complicated wait process is necessary.  We may also need to see
+    // if a PostDelayedTask should be used here to avoid blocking the PulseAudio
+    // command thread.
+    if (source_callback_ && bytes_remaining > 0)
+      base::PlatformThread::Sleep(params_.GetBufferDuration() / 4);
   }
 }
 
diff --git a/media/audio/pulse/pulse_util.cc b/media/audio/pulse/pulse_util.cc
index 66f52c2..c06195e 100644
--- a/media/audio/pulse/pulse_util.cc
+++ b/media/audio/pulse/pulse_util.cc
@@ -274,9 +274,13 @@ bool CreateOutputStream(pa_threaded_mainloop** mainloop,
   // |minreq| bytes.  |tlength| should be a multiple of |minreq|; too low and
   // Pulse will issue callbacks way too fast, too high and we don't get
   // callbacks frequently enough.
+  //
+  // Setting |minreq| to the exact buffer size leads to more callbacks than
+  // necessary, so we've clipped it to half the buffer size.  Regardless of the
+  // requested amount, we'll always fill |params.GetBytesPerBuffer()| though.
   pa_buffer_attr pa_buffer_attributes;
   pa_buffer_attributes.maxlength = static_cast<uint32_t>(-1);
-  pa_buffer_attributes.minreq = params.GetBytesPerBuffer();
+  pa_buffer_attributes.minreq = params.GetBytesPerBuffer() / 2;
   pa_buffer_attributes.prebuf = static_cast<uint32_t>(-1);
   pa_buffer_attributes.tlength = params.GetBytesPerBuffer() * 3;
   pa_buffer_attributes.fragsize = static_cast<uint32_t>(-1);