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) {
+  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.
 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()) {
     socket_->Send(&bytes, sizeof(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
   // 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();
 #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() {
   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