Switch OnMoreData() to use AudioBus.

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/process_util.h"
 #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.
 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()) {
     socket_->Send(&bytes, sizeof(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
   // sure if renderer already filled the buffer, and due to all the plugins we
   // cannot change the API, so we yield if previous call was too recent.
   // Optimization: if renderer is "new" one that writes length of data we can
   // stop yielding the moment length is written -- not ideal solution,
   // but better than nothing.
   while (!DataReady() &&
          ((base::Time::Now() - previous_call_time_).InMilliseconds() <
           kMinIntervalBetweenReadCallsInMs)) {
     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);
 
-  // Get the data from the buffer.
-  memcpy(data, shared_memory_->memory(), read_size);
+  // 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());
 
-  // 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);
+  // Copy data from the shared memory into the caller's AudioBus.
+  audio_bus_->CopyTo(audio_bus);
+
+  // 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() {
   if (socket_.get()) {
     socket_->Close();
   }
 }
 
 bool AudioSyncReader::Init() {
   socket_.reset(new base::CancelableSyncSocket());
@@ skipping 17 matching lines @@
 bool AudioSyncReader::PrepareForeignSocketHandle(
     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