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 cd6db16cd74b23e5dbc15a6a9a7fe022bfd0ec5d..61919b859633c3f1e1ce5b168dda1676eb6f1d69 100644
--- a/content/browser/renderer_host/media/audio_sync_reader.cc
+++ b/content/browser/renderer_host/media/audio_sync_reader.cc
@@ -10,23 +10,26 @@
 #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) {
+  packet_size_ = media::PacketSizeInBytes(shared_memory_->created_size());
+  DCHECK_EQ(packet_size_, media::AudioBus::CalculateMemorySize(params));
+  audio_bus_ = media::AudioBus::WrapMemory(params, shared_memory->memory());
 }
 
 AudioSyncReader::~AudioSyncReader() {
 }
 
 bool AudioSyncReader::DataReady() {
-  return !media::IsUnknownDataSize(
-      shared_memory_,
-      media::PacketSizeInBytes(shared_memory_->created_size()));
+  return !media::IsUnknownDataSize(shared_memory_, packet_size_);
 }
 
 // media::AudioOutputController::SyncReader implementations.
@@ -34,9 +37,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::PacketSizeInBytes(shared_memory_->created_size()));
+    media::SetUnknownDataSize(shared_memory_, packet_size_);
   }
 
   if (socket_.get()) {
@@ -44,10 +45,7 @@ void AudioSyncReader::UpdatePendingBytes(uint32 bytes) {
   }
 }
 
-uint32 AudioSyncReader::Read(void* data, uint32 size) {
-  uint32 max_size = media::PacketSizeInBytes(
-      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
@@ -62,27 +60,44 @@ uint32 AudioSyncReader::Read(void* data, uint32 size) {
     base::PlatformThread::YieldCurrentThread();
   }
   previous_call_time_ = base::Time::Now();
+#else
+  if (!DataReady()) {

[DaleCurtis, 2012/08/29 04:34:43]

I ran into this issue while testing debug builds. OnMoreData() happens too
quickly so we end up with a junk GetActualDataSizeInBytes value.

I'm not sure if this is the right solution or if we should use the Windows
solution on all platforms. Thoughts?

[Chris Rogers, 2012/08/29 04:59:17]

Please don't add this.  At one time, when this code was first added it was
enabled for all platforms and caused problems on Mac OS X.  We know that
OnMoreData() happens too quickly on the waveOut back-end.  The correct solution
is not using this technique.

Let's try to keep this CL targeted to just switching the shared-memory format
over to AudioBus, so we can stage changes in small pieces.  This change is
unrelated.

On 2012/08/29 04:34:43, DaleCurtis wrote:

[DaleCurtis, 2012/08/29 05:17:57]

This is not unrelated as since we're wrapping the shared memory on the renderer
side you can't access the shared memory until DataReady() is true or you'll get
partial / in processing audio.  Also, without this the CHECK() on frames below
fails too since GetActualDataSizeInBytes == UnknownDataSize == UINT32_MAX - 1.

I don't see any harm in the change I've made, otherwise we're reading
uninitialized data (or data in the process of being initialized) and returning
it to the output device which seems much worse.

[DaleCurtis, 2012/08/29 17:03:42]

Chris explained his concerns offline. I'll switch the CHECKs to instead clip the
size, frame values as necessary.

+    audio_bus->Zero();
+    return 0;
+  }
 #endif
 
-  uint32 read_size = std::min(media::GetActualDataSizeInBytes(shared_memory_,
-                                                              max_size),
-                              size);
+  // Retrieve the actual number of bytes available from the shared memory.
+  // TODO(dalecurtis): Technically this is not the exact size.  Due to channel
+  // padding for alignment, there may be more data available than this; AudioBus
+  // will automatically do the right thing during CopyTo().  Rename this method
+  // to GetActualFrameCount().
+  int size = media::GetActualDataSizeInBytes(shared_memory_, packet_size_);
+  // Sanity check the actual size by ensuring it's a multiple of sizeof(float).
+  CHECK_EQ(static_cast<size_t>(size) % sizeof(*audio_bus_->channel(0)), 0U);
+
+  // Compute the actual number of frames read.  It's important to check this
+  // value since the shared memory comes from a potentially untrusted source.
+  int frames =
+      size / (sizeof(*audio_bus_->channel(0)) * audio_bus_->channels());
+  CHECK_GE(frames, 0);
+  CHECK_LE(frames, audio_bus_->frames());
 
-  // Get the data from the buffer.
-  memcpy(data, shared_memory_->memory(), read_size);
+  // Copy data from the shared memory into the caller's AudioBus.
+  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);
+  // Zero out any unfilled frames.
+  audio_bus_->ZeroFramesPartial(frames, audio_bus_->frames() - frames);
 
   // Zero out the entire buffer.
-  memset(shared_memory_->memory(), 0, max_size);
+  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 frames;
 }
 
 void AudioSyncReader::Close() {