Switch OnMoreData() to use AudioBus.

media/audio/mac/audio_output_mac.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 "media/audio/mac/audio_output_mac.h"
 
 #include <CoreServices/CoreServices.h>
 
 #include "base/basictypes.h"
 #include "base/debug/trace_event.h"
@@ skipping 35 matching lines @@
       packet_size_(params.GetBytesPerBuffer()),
       silence_bytes_(0),
       volume_(1),
       pending_bytes_(0),
       num_source_channels_(params.channels()),
       source_layout_(params.channel_layout()),
       num_core_channels_(0),
       should_swizzle_(false),
       should_down_mix_(false),
       stopped_event_(true /* manual reset */, false /* initial state */),
-      num_buffers_left_(kNumBuffers) {
+      num_buffers_left_(kNumBuffers),
+      audio_bus_(AudioBus::Create(params)) {
   // We must have a manager.
   DCHECK(manager_);
   // A frame is one sample across all channels. In interleaved audio the per
   // frame fields identify the set of n |channels|. In uncompressed audio, a
   // packet is always one frame.
   format_.mSampleRate = params.sample_rate();
   format_.mFormatID = kAudioFormatLinearPCM;
   format_.mFormatFlags = kLinearPCMFormatFlagIsPacked;
   format_.mBitsPerChannel = params.bits_per_sample();
   format_.mChannelsPerFrame = params.channels();
@@ skipping 335 matching lines @@
       // Should never touch audio_stream after signaling as it
       // can be deleted at any moment.
       audio_stream->stopped_event_.Signal();
     }
     return;
   }
 
   // Adjust the number of pending bytes by subtracting the amount played.
   if (!static_cast<AudioQueueUserData*>(buffer->mUserData)->empty_buffer)
     audio_stream->pending_bytes_ -= buffer->mAudioDataByteSize;
+  uint32 filled = 0;
   uint32 capacity = buffer->mAudioDataBytesCapacity;
-  // TODO(sergeyu): Specify correct hardware delay for AudioBuffersState.
-  uint32 filled = source->OnMoreData(
-      reinterpret_cast<uint8*>(buffer->mAudioData), capacity,
-      AudioBuffersState(audio_stream->pending_bytes_, 0));
+  {
+    // Since RenderCallback may be reentrant, we need to ensure the AudioBus
+    // we're sharing is safe for this instance.  OnMoreData() offers no
+    // guarantee of thread safety as well.
+    base::AutoLock lock(audio_stream->audio_bus_lock_);

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

Are you sure this is right?  If this function is re-entrant then we don't want a
lock since we'll get dead-lock (see comment on line 374).  I'm also pretty sure
that this method won't be called from multiple threads in the same time window
so the lock shouldn't be needed.

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

That indicates that a lock shouldn't be held across AudioQueue / OS API calls,
within the calling range of OnMoreData() I believe this is safe (otherwise no
function all the way down OnMoreData could hold a lock).

Without the lock if multiple OnMoreData() calls occur the shared audio_bus may
be trampled as locks further down the OnMoreData chain cycle to allow the next
pass through.

+

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

You use "audio_stream->audio_bus_" three times below.  It would be nice to use a
local AudioBus*

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

Done. Presumably PCMQueueOutAudioOutputStream won't be destroyed while we're in
the middle of this call?

+    DCHECK_EQ(audio_stream->audio_bus_->frames() *
+              audio_stream->format_.mBytesPerFrame, capacity);
+    // TODO(sergeyu): Specify correct hardware delay for AudioBuffersState.
+    int frames_filled = source->OnMoreData(
+        audio_stream->audio_bus_.get(),
+        AudioBuffersState(audio_stream->pending_bytes_, 0));
+    filled = frames_filled * audio_stream->format_.mBytesPerFrame;
+    audio_stream->audio_bus_->ToInterleaved(
+        frames_filled, audio_stream->format_.mBitsPerChannel / 8,
+        buffer->mAudioData);
+  }
 
   // In order to keep the callback running, we need to provide a positive amount
   // of data to the audio queue. To simulate the behavior of Windows, we write
   // a buffer of silence.
   if (!filled) {
     CHECK(audio_stream->silence_bytes_ <= static_cast<int>(capacity));
     filled = audio_stream->silence_bytes_;
 
     // Assume unsigned audio.
     int silence_value = 128;
@@ skipping 104 matching lines @@
   source_ = source;
 }
 
 AudioOutputStream::AudioSourceCallback*
 PCMQueueOutAudioOutputStream::GetSource() {
   base::AutoLock lock(source_lock_);
   return source_;
 }
 
 }  // namespace media