Switch OnMoreData() to use AudioBus.

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 2f06f556fb2fbaceab94759a3b33ec614cd4e023..74a0a14b882a932c3fb8613986ae0656f0b56bb9 100644
--- a/content/browser/renderer_host/media/audio_sync_reader.cc
+++ b/content/browser/renderer_host/media/audio_sync_reader.cc
@@ -10,23 +10,25 @@
 #include "base/shared_memory.h"
 #include "base/threading/platform_thread.h"
 #include "media/audio/audio_buffers_state.h"
+#include "media/audio/audio_parameters.h"
 #include "media/audio/shared_memory_util.h"
 
 #if defined(OS_WIN)
 const int kMinIntervalBetweenReadCallsInMs = 10;
 #endif
 
-AudioSyncReader::AudioSyncReader(base::SharedMemory* shared_memory)
+AudioSyncReader::AudioSyncReader(base::SharedMemory* shared_memory,
+                                 const media::AudioParameters& params)
     : shared_memory_(shared_memory) {
+  audio_bus_ = media::AudioBus::WrapMemory(params, shared_memory->memory());

[Chris Rogers, 2012/08/24 20:20:26]

I think we need to have a sanity check here that the |params| size matches the
actual |shared_memory| size.  It *should*, but it's very important to not make a
mistake here.

[DaleCurtis, 2012/08/24 23:53:12]

Done.

+  packet_size_ = media::PacketSizeSizeInBytes(shared_memory_->created_size());
 }
 
 AudioSyncReader::~AudioSyncReader() {
 }
 
 bool AudioSyncReader::DataReady() {
-  return !media::IsUnknownDataSize(
-      shared_memory_,
-      media::PacketSizeSizeInBytes(shared_memory_->created_size()));
+  return !media::IsUnknownDataSize(shared_memory_, packet_size_);
 }
 
 // media::AudioOutputController::SyncReader implementations.
@@ -34,9 +36,7 @@ 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_,
-        media::PacketSizeSizeInBytes(shared_memory_->created_size()));
+    media::SetUnknownDataSize(shared_memory_, packet_size_);
   }
 
   if (socket_.get()) {
@@ -44,10 +44,7 @@ void AudioSyncReader::UpdatePendingBytes(uint32 bytes) {
   }
 }
 
-uint32 AudioSyncReader::Read(void* data, uint32 size) {
-  uint32 max_size = media::PacketSizeSizeInBytes(
-      shared_memory_->created_size());
-
+int AudioSyncReader::Read(media::AudioBus* audio_bus) {
 #if defined(OS_WIN)
   // HACK: yield if reader is called too often.
   // Problem is lack of synchronization between host and renderer. We cannot be
@@ -64,25 +61,27 @@ uint32 AudioSyncReader::Read(void* data, uint32 size) {
   previous_call_time_ = base::Time::Now();
 #endif
 
-  uint32 read_size = std::min(media::GetActualDataSizeInBytes(shared_memory_,
-                                                              max_size),
-                              size);
+  // Retrieve the actual number of bytes available from the shared memory.
+  int size = media::GetActualDataSizeInBytes(shared_memory_, packet_size_);
+  DCHECK_EQ(static_cast<size_t>(size) % sizeof(*audio_bus_->channel(0)), 0U);
 
-  // Get the data from the buffer.
-  memcpy(data, shared_memory_->memory(), read_size);
+  // Copy data from the shared memory into the caller's AudioBus.
+  // TODO(dalecurtis): See if it's sane to avoid the copy by modifying the
+  // caller's AudioBus to point to the shared memory.
+  audio_bus_->CopyTo(audio_bus);
 
-  // If amount read was less than requested, then zero out the remainder.
-  if (read_size < size)
-    memset(static_cast<char*>(data) + read_size, 0, size - read_size);
+  // TODO(dalecurtis): Do we need to zero out remaining frames?

[Chris Rogers, 2012/08/24 20:20:26]

Please do, I remember specifically that we had a bug where this was causing
problems.

[DaleCurtis, 2012/08/24 23:53:12]

Done.

 
   // Zero out the entire buffer.
-  memset(shared_memory_->memory(), 0, max_size);
+  // TODO(dalecurtis): This is probably unnecessary.

[Chris Rogers, 2012/08/24 20:20:26]

It seems like it should be unnecessary, and in normal running conditions it is
not needed.  But, it handles the degenerate case when audio starts glitching
because the renderer can't render fast enough (for example in Web Audio API if
the audio rendering becomes too heavy).  We're already in catastrophic failure
mode at this point because we're glitching, but if we fail to zero out the
memory here, then the glitching anomaly becomes even worse since it causes
repeated buffers (potentially quite loud ones!) to be played back end-end which
sounds a lot worse.

[DaleCurtis, 2012/08/24 23:53:12]

Thanks for the information, left alone.

+  memset(shared_memory_->memory(), 0, packet_size_);
 
   // Store unknown length of data into buffer, in case renderer does not store
   // the length itself. It also helps in decision if we need to yield.
-  media::SetUnknownDataSize(shared_memory_, max_size);
+  media::SetUnknownDataSize(shared_memory_, packet_size_);
 
-  return read_size;
+  // Return the actual number of frames read.
+  return size / (sizeof(*audio_bus_->channel(0)) * audio_bus_->channels());
 }
 
 void AudioSyncReader::Close() {