Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10909185:
Add Mac OS X synchronized audio I/O back-end (Closed)
|
Created:
8 years, 3 months ago by Chris Rogers Modified:
8 years, 3 months ago Reviewers:
Ken Russell (switch to Gerrit), no longer working on chromium, DaleCurtis, jbauman, scherkus (not reviewing) CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionAdd Mac OS X synchronized audio I/O back-end
AudioSynchronizedStream is an implementation of AudioOuputStream for Mac OS X
when using an input and output which are *not* unified in the same driver.
This requires managing a separate input and output thread for each of the drivers
and synchronizing them with a FIFO and varispeed.
This synchronization involves two threads, and requires that the FIFO be made
thread-safe in the sense that one thread may call AudioFifo::Push() while
a second thread calls AudioFifo::Consume(). It is not acceptable to simply
require the client to put locks around these calls because they can (and will) be
contended (causing glitches) since both threads are real-time audio threads.
BUG=none
TEST=extensive manual testing on various machines, audio hardware, and input/output sample-rates
R=scherkus
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=157251
Patch Set 1 #
Total comments: 98
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 25
Patch Set 5 : #
Total comments: 36
Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #
Total comments: 10
Patch Set 9 : #Patch Set 10 : #Patch Set 11 : #Patch Set 12 : #
Total comments: 7
Messages
Total messages: 22 (0 generated)
Please take a look. I'm still working on the details of calling OnMoreIOData(), etc. But the large pieces are here, and this code works in a "play-through" mode when tested in a stand-alone application. Thanks!
my first run. https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... File media/audio/mac/audio_synchronized_mac.cc (right): https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:19: const int kHardwareBufferSize = 128; put them into an anonymous space or static https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:46: is_fifo_initialized_(false), Is this a special user case for fifo? why not ask having a query function like filo.Initialized() in fifo? https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:103: remove this empty line. https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:137: AudioUnitUninitialize(input_unit_); Is it all right to call AudioUnitUninitialize if input_unit_ is 0? or AudioUnitInitialize has not been called? https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:155: if (!good) early return if (!good || is_running_) https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:176: is_running_ = true; I think we should set is_running_ to true here regardless of result. So that we can stop the audio in case AudioOutputUnitStart(output_unit_) fails. https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:190: first_input_time_ = -1; we are setting the values to -1 in both Start() and Stop(), I think it is fine to skip it here. https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:349: OpenAComponent(input_comp, &input_unit_); why don't we care about the return value here? https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:362: OpenAComponent(output_comp, &output_unit_); ditto https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:388: OSStatus AudioSynchronizedStream::EnableIO() { can we change the name to something else like SetupAUHAL, or something else more suitable? https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:450: 0, shouldn't the AudioUnitElement to be 1 here? https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:495: // Get the size of the IO buffer(s) add a period at the end of the comments. Please fix it in other places too. https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:531: 1, Could you please explain which side of format it is getting? input or output? https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:553: ////////////////////////////////////// move /////////////// https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:560: asbd_dev1_in.mChannelsPerFrame : asbd_dev2_out.mChannelsPerFrame); use std::min() https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:589: kAudioUnitScope_Output, I don't understand this part, are we setting the format to the output element of the input unit? https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:684: static int count = 0; How this is used in production code? https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:714: // printf("pushing %d : n = %d\n", remove https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:730: AudioSynchronizedStream* This = use stream as the name? https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... File media/audio/mac/audio_synchronized_mac.h (right): https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.h:33: // handles mismatches between input and output sample-rate and also clock drift So the FIFO is thread safe? https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.h:54: #if CHROME what is this? https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.h:142: struct AudioDeviceInfo { it contains member functions in this struct, we should use class instead. https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.h:149: UInt32 buffer_size_frames_; initialize these values in the constructor.
cool stuff -- here's my initial pass don't forget the .gyp changes! https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... File media/audio/mac/audio_synchronized_mac.cc (right): https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:5: #include "audio_synchronized_mac.h" should be full path https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:19: const int kHardwareBufferSize = 128; On 2012/09/12 09:11:56, xians1 wrote: > put them into an anonymous space or static static, please! :) https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:20: const int kFIFOSize = 16384; s/FIFO/Fifo here + below https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:140: input_unit_ = 0; s/0/NULL/? https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:152: DCHECK(good); a more informative DCHECK would be to DCHECK on each unit (input, output, varispeed) -- then at that point I would simple remove the "good" variable and have your if statement check all three OR simply CHECK() if this is a heinous programming error https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:212: UInt32 size = sizeof(AudioDeviceID); if possible sizeof(variable) instead of sizeof(type) https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:270: UInt32 size = sizeof(AudioDeviceID); if possible sizeof(variable) instead of sizeof(type) https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:432: 0, these zeros and ones aren't very telling -- is there a better way to describe what this parameter means? i.e., comments or perhaps declaring a constant or something like that? https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:509: buffer_size_bytes = buffer_size_frames * sizeof(Float32); if possible sizeof(variable) instead of sizeof(type) https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:563: // and set it to the stream format of AUHAL nit: part of this comment can go on previous line don't forget your periods! https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:564: property_size = sizeof(Float64); if possible sizeof(variable) instead of sizeof(type) https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:612: property_size = sizeof(Float64); if possible sizeof(variable) instead of sizeof(type) https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:661: (sizeof(AudioBuffer) * asbd.mChannelsPerFrame); if possible sizeof(variable) instead of sizeof(type) https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:693: AudioSynchronizedStream* This = hrmmm... can you follow the pattern you've done in other mac code using procs whereby you call a private method? the "This" style isn't very prevalent https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:695: if (This->first_input_time_ < 0.) 0.0? 0.0f? or just 0? https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:730: AudioSynchronizedStream* This = ditto for This https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:754: // printf("n = %d : x = %f\n", (int)n, x); ?? https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:768: AudioSynchronizedStream* This = ditto for This https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:771: if (This->first_input_time_ < 0.) { 0.0? 0.0f? or just 0? https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:819: if (This->first_output_time_ < 0.) { 0.0? 0.0f? or just 0? https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.cc:854: UInt32 property_size = sizeof(UInt32); if possible sizeof(variable) instead of sizeof(type) https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... File media/audio/mac/audio_synchronized_mac.h (right): https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.h:39: // varispeed will slow down slightly. can you move comment to be next to class? https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.h:86: // implement - or remove SetVolume()/GetVolume() from AudioOutputStream. agreed :) https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.h:87: virtual void SetVolume(double volume) OVERRIDE {} de-inline virtual methods https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.h:161: // AudioUnits period https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.h:167: Float64 first_input_time_; are we required to use these types vs. double? https://chromiumcodereview.appspot.com/10909185/diff/1/media/audio/mac/audio_... media/audio/mac/audio_synchronized_mac.h:173: }; DISALLOW etc
PTAL, this still isn't perfect, but I've addressed nearly all the comments. And it works! (at least in my testing so far) I still need to do a better job explaining the SetupStreamFormats() method. And I also need to get mono inputs properly working. I've added a new constructor (Mac-only) in AudioBus which wraps an AudioBufferList. It turns out to be extremely useful. http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... File media/audio/mac/audio_synchronized_mac.cc (right): http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.cc:5: #include "audio_synchronized_mac.h" On 2012/09/12 14:05:29, scherkus wrote: > should be full path Done. http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.cc:19: const int kHardwareBufferSize = 128; On 2012/09/12 14:05:29, scherkus wrote: > On 2012/09/12 09:11:56, xians1 wrote: > > put them into an anonymous space or static > > static, please! :) I think that "const int" is static automatically unless it's exported with the "extern" keyword, but changing to static to be paranoid :) http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.cc:20: const int kFIFOSize = 16384; On 2012/09/12 14:05:29, scherkus wrote: > s/FIFO/Fifo here + below Done. http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.cc:46: is_fifo_initialized_(false), On 2012/09/12 09:11:56, xians1 wrote: > Is this a special user case for fifo? why not ask having a query function like > filo.Initialized() in fifo? I can still clean this part up a little - not yet in this patchset... http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.cc:103: On 2012/09/12 09:11:56, xians1 wrote: > remove this empty line. Done. http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.cc:137: AudioUnitUninitialize(input_unit_); On 2012/09/12 09:11:56, xians1 wrote: > Is it all right to call AudioUnitUninitialize if input_unit_ is 0? or > AudioUnitInitialize has not been called? I've added extra checking here. It *is* ok to call AudioUnitInitialize() if it has not yet been initialized. It will return an error, but is safe. http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.cc:140: input_unit_ = 0; On 2012/09/12 14:05:29, scherkus wrote: > s/0/NULL/? Done. http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.cc:152: DCHECK(good); On 2012/09/12 14:05:29, scherkus wrote: > a more informative DCHECK would be to DCHECK on each unit (input, output, > varispeed) -- then at that point I would simple remove the "good" variable and > have your if statement check all three > > OR simply CHECK() if this is a heinous programming error Done. http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.cc:155: if (!good) On 2012/09/12 09:11:56, xians1 wrote: > early return if (!good || is_running_) Done. http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.cc:176: is_running_ = true; On 2012/09/12 09:11:56, xians1 wrote: > I think we should set is_running_ to true here regardless of result. So that we > can stop the audio in case AudioOutputUnitStart(output_unit_) fails. Done. http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.cc:190: first_input_time_ = -1; On 2012/09/12 09:11:56, xians1 wrote: > we are setting the values to -1 in both Start() and Stop(), I think it is fine > to skip it here. Done. http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.cc:212: UInt32 size = sizeof(AudioDeviceID); On 2012/09/12 14:05:29, scherkus wrote: > if possible sizeof(variable) instead of sizeof(type) Done. http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.cc:270: UInt32 size = sizeof(AudioDeviceID); On 2012/09/12 14:05:29, scherkus wrote: > if possible sizeof(variable) instead of sizeof(type) Done. http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.cc:349: OpenAComponent(input_comp, &input_unit_); On 2012/09/12 09:11:56, xians1 wrote: > why don't we care about the return value here? We do ;) added checks and early return for these cases http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.cc:349: OpenAComponent(input_comp, &input_unit_); On 2012/09/12 09:11:56, xians1 wrote: > why don't we care about the return value here? Done. http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.cc:362: OpenAComponent(output_comp, &output_unit_); On 2012/09/12 09:11:56, xians1 wrote: > ditto Done. http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.cc:388: OSStatus AudioSynchronizedStream::EnableIO() { On 2012/09/12 09:11:56, xians1 wrote: > can we change the name to something else like SetupAUHAL, or something else more > suitable? Leaving name for now, since the property name we're setting is actually called "kAudioOutputUnitProperty_EnableIO" http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.cc:432: 0, On 2012/09/12 14:05:29, scherkus wrote: > these zeros and ones aren't very telling -- is there a better way to describe > what this parameter means? > > i.e., comments or perhaps declaring a constant or something like that? For now I haven't done anything about this. I could either add a comment to all of these lines, or define a constant called "kElementIgnored", or something like that? http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.cc:450: 0, On 2012/09/12 09:11:56, xians1 wrote: > shouldn't the AudioUnitElement to be 1 here? No, since it's "global" scope there isn't any element http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.cc:495: // Get the size of the IO buffer(s) On 2012/09/12 09:11:56, xians1 wrote: > add a period at the end of the comments. Please fix it in other places too. Done. http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.cc:509: buffer_size_bytes = buffer_size_frames * sizeof(Float32); On 2012/09/12 14:05:29, scherkus wrote: > if possible sizeof(variable) instead of sizeof(type) I've moved |buffer_size_bytes| down closer to usage, but can't think of an obvious way to use sizeof(variable) -- actually this is now in a new method called AllocateInputData() http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.cc:531: 1, On 2012/09/12 09:11:56, xians1 wrote: > Could you please explain which side of format it is getting? input or output? Sorry, I still need to address this (better commenting, etc.) in this particular method ---- not yet addressed http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.cc:553: ////////////////////////////////////// On 2012/09/12 09:11:56, xians1 wrote: > move /////////////// Done. http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.cc:560: asbd_dev1_in.mChannelsPerFrame : asbd_dev2_out.mChannelsPerFrame); On 2012/09/12 09:11:56, xians1 wrote: > use std::min() Done. http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.cc:563: // and set it to the stream format of AUHAL On 2012/09/12 14:05:29, scherkus wrote: > nit: part of this comment can go on previous line > > don't forget your periods! Done. http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.cc:564: property_size = sizeof(Float64); On 2012/09/12 14:05:29, scherkus wrote: > if possible sizeof(variable) instead of sizeof(type) Done. http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.cc:589: kAudioUnitScope_Output, On 2012/09/12 09:11:56, xians1 wrote: > I don't understand this part, are we setting the format to the output element of > the input unit? I still need to do a better job explaining this.... http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.cc:612: property_size = sizeof(Float64); On 2012/09/12 14:05:29, scherkus wrote: > if possible sizeof(variable) instead of sizeof(type) Done. http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.cc:661: (sizeof(AudioBuffer) * asbd.mChannelsPerFrame); On 2012/09/12 14:05:29, scherkus wrote: > if possible sizeof(variable) instead of sizeof(type) Here I'm not sure that's easily possible http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.cc:684: static int count = 0; On 2012/09/12 09:11:56, xians1 wrote: > How this is used in production code? Removed! http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.cc:693: AudioSynchronizedStream* This = On 2012/09/12 14:05:29, scherkus wrote: > hrmmm... can you follow the pattern you've done in other mac code using procs > whereby you call a private method? > > the "This" style isn't very prevalent Yes, much better this way :) Done http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.cc:695: if (This->first_input_time_ < 0.) On 2012/09/12 14:05:29, scherkus wrote: > 0.0? 0.0f? or just 0? Done. http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.cc:730: AudioSynchronizedStream* This = On 2012/09/12 09:11:56, xians1 wrote: > use stream as the name? Done. http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.cc:754: // printf("n = %d : x = %f\n", (int)n, x); On 2012/09/12 14:05:29, scherkus wrote: > ?? Done. http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.cc:768: AudioSynchronizedStream* This = On 2012/09/12 14:05:29, scherkus wrote: > ditto for This Done. http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.cc:771: if (This->first_input_time_ < 0.) { On 2012/09/12 14:05:29, scherkus wrote: > 0.0? 0.0f? or just 0? Done. http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.cc:819: if (This->first_output_time_ < 0.) { On 2012/09/12 14:05:29, scherkus wrote: > 0.0? 0.0f? or just 0? Done. http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.cc:854: UInt32 property_size = sizeof(UInt32); On 2012/09/12 14:05:29, scherkus wrote: > if possible sizeof(variable) instead of sizeof(type) Done. http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... File media/audio/mac/audio_synchronized_mac.h (right): http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.h:33: // handles mismatches between input and output sample-rate and also clock drift On 2012/09/12 09:11:56, xians1 wrote: > So the FIFO is thread safe? Good catch! The AudioFifo is currently *not* thread-safe because of the |frames_| variable. However, we can make it thread-safe by getting rid of this variable and calculating it based off read_pos_ and write_pos_ (or similar techniques) --- in other words turn it into a classic lock-free circular buffer. In the meantime, as a very temporary stop-gap fix, I've added |fifo_lock_| and try-lock around the use of the FIFO. This isn't good, since it can cause glitches, but at least will not be dangerous in terms of crashes. http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.h:39: // varispeed will slow down slightly. On 2012/09/12 14:05:29, scherkus wrote: > can you move comment to be next to class? Done. http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.h:54: #if CHROME On 2012/09/12 09:11:56, xians1 wrote: > what is this? Sorry, this is temporary which I need for testing for the next day or so, so I don't expect an LGTM until it's removed :) http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.h:87: virtual void SetVolume(double volume) OVERRIDE {} On 2012/09/12 14:05:29, scherkus wrote: > de-inline virtual methods Done. http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.h:142: struct AudioDeviceInfo { On 2012/09/12 09:11:56, xians1 wrote: > it contains member functions in this struct, we should use class instead. Done. http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.h:149: UInt32 buffer_size_frames_; On 2012/09/12 09:11:56, xians1 wrote: > initialize these values in the constructor. Done. http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.h:161: // AudioUnits On 2012/09/12 14:05:29, scherkus wrote: > period Done. http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.h:167: Float64 first_input_time_; On 2012/09/12 14:05:29, scherkus wrote: > are we required to use these types vs. double? You're right - not in this case -- switched to double http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.h:173: }; On 2012/09/12 14:05:29, scherkus wrote: > DISALLOW etc Done.
http://codereview.chromium.org/10909185/diff/1005/media/audio/mac/audio_synch... File media/audio/mac/audio_synchronized_mac.cc (right): http://codereview.chromium.org/10909185/diff/1005/media/audio/mac/audio_synch... media/audio/mac/audio_synchronized_mac.cc:211: // implement - or remove SetVolume()/GetVolume() from AudioOutputStream. nit: some of this comment can go on previous line http://codereview.chromium.org/10909185/diff/1005/media/audio/mac/audio_synch... media/audio/mac/audio_synchronized_mac.cc:666: UInt32 malloc_size = offsetof(AudioBufferList, mBuffers[0]) + for the sake of other chromium developers can you attempt to use stddint types (int, size_t, uint32, et al) as much as possible? http://codereview.chromium.org/10909185/diff/1005/media/audio/mac/audio_synch... media/audio/mac/audio_synchronized_mac.cc:669: input_data_ = static_cast<AudioBufferList*>(malloc(malloc_size)); any reason to prefer malloc/free over new/delete here + below? http://codereview.chromium.org/10909185/diff/1005/media/audio/mac/audio_synch... media/audio/mac/audio_synchronized_mac.cc:732: if (fifo_.frames() < static_cast<int>(number_of_frames)) { FYI: fifo_ not locked See my comment on line 817 regarding which threads these various procs are running on http://codereview.chromium.org/10909185/diff/1005/media/audio/mac/audio_synch... media/audio/mac/audio_synchronized_mac.cc:749: size_t n = fifo_.frames(); not locked http://codereview.chromium.org/10909185/diff/1005/media/audio/mac/audio_synch... media/audio/mac/audio_synchronized_mac.cc:809: if (first_output_time_ < 0.) { 0.0 http://codereview.chromium.org/10909185/diff/1005/media/audio/mac/audio_synch... media/audio/mac/audio_synchronized_mac.cc:814: unsigned n = static_cast<unsigned>(in_to_out_sample_offset_); s/unsigned/int http://codereview.chromium.org/10909185/diff/1005/media/audio/mac/audio_synch... media/audio/mac/audio_synchronized_mac.cc:817: fifo_.Push(&silence); FYI: not locked to confirm.. the output+varispeed proc run together on one thread and the input proc run on a separate thread? for example it appears you set is_fifo_initialized_ in the output proc but read it from the input proc -- should it also be protected? do we want to be protecting every access to fifo_ or only certain calls? http://codereview.chromium.org/10909185/diff/1005/media/audio/mac/audio_synch... File media/audio/mac/audio_synchronized_mac.h (right): http://codereview.chromium.org/10909185/diff/1005/media/audio/mac/audio_synch... media/audio/mac/audio_synchronized_mac.h:157: : id_(kAudioDeviceUnknown), should be indented by two more spaces http://codereview.chromium.org/10909185/diff/1005/media/audio/mac/audio_synch... media/audio/mac/audio_synchronized_mac.h:195: UInt32 hardware_buffer_size_; any chance we can swap UInt32 for int, size_t, uint32, etc... ? http://codereview.chromium.org/10909185/diff/1005/media/base/audio_bus.h File media/base/audio_bus.h (right): http://codereview.chromium.org/10909185/diff/1005/media/base/audio_bus.h#newc... media/base/audio_bus.h:40: ~AudioBus(); we'll likely have to chat about this change a bit more -- one of the motivations behind having private ctors/dtors w/ static methods that return scoped_ptr<> was to enforce that these objects have single owner and deletion/transfer of ownership is forbidden: http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.h#newc... We can revisit that decision but I'm curious whether it's possible to pre-allocate the AudioBus objects you need inside of audio_synchronized_mac.cc For example, there are snippets of code like the following: AudioBus bus(channels_, number_of_frames, input_data_); ...both of those are member variable data and it's only the # of frames that might change but maybe that can be handled via some other mechanism.
http://codereview.chromium.org/10909185/diff/1005/media/base/audio_bus.h File media/base/audio_bus.h (right): http://codereview.chromium.org/10909185/diff/1005/media/base/audio_bus.h#newc... media/base/audio_bus.h:40: ~AudioBus(); On 2012/09/13 13:06:03, scherkus wrote: > we'll likely have to chat about this change a bit more -- one of the motivations > behind having private ctors/dtors w/ static methods that return scoped_ptr<> was > to enforce that these objects have single owner and deletion/transfer of > ownership is forbidden: > http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.h#newc... > > We can revisit that decision but I'm curious whether it's possible to > pre-allocate the AudioBus objects you need inside of audio_synchronized_mac.cc > > For example, there are snippets of code like the following: > AudioBus bus(channels_, number_of_frames, input_data_); > > ...both of those are member variable data and it's only the # of frames that > might change but maybe that can be handled via some other mechanism. +1, I don't see why you made this change. You should just add a new OS_MAC specific Create method. That said, I don't think this is the place for the AudioBufferList change, this class should remain platform independent. I don't think any other class will use the AudioBufferList method. Which means this is probably better done inside a helper method inside that class. Something like: scoped_ptr<AudioBus> AudioSynchronizedStream::GetAudioBus(int frames, int channels, AudioBufferList* buffer_list) { std::vector temp_vector() <convert buffer_list into temp_vector> return AudioBus::WrapVector(frames, channels, temp_vector); }
PTAL kbr, jbauman: Please review AudioFifo changes for thread safety scherkus, dalecurtis: I still haven't addressed the AudioBus changes (please see my comments). Let me know how strongly you feel, and I'll change appropriately. xians: I still need to add more comments in SetupStreamFormats() I've tuned this code and done lots of cleanup since the last CL. I've spent a lot of time testing different input/output device combinations at different sample-rates. It's generally working quite well with pretty respectable latency (considering everything that's going on here), but might need some additional tweaking over time. Thanks for your reviews! http://codereview.chromium.org/10909185/diff/1005/media/audio/mac/audio_synch... File media/audio/mac/audio_synchronized_mac.cc (right): http://codereview.chromium.org/10909185/diff/1005/media/audio/mac/audio_synch... media/audio/mac/audio_synchronized_mac.cc:211: // implement - or remove SetVolume()/GetVolume() from AudioOutputStream. On 2012/09/13 13:06:03, scherkus wrote: > nit: some of this comment can go on previous line Done. http://codereview.chromium.org/10909185/diff/1005/media/audio/mac/audio_synch... media/audio/mac/audio_synchronized_mac.cc:666: UInt32 malloc_size = offsetof(AudioBufferList, mBuffers[0]) + On 2012/09/13 13:06:03, scherkus wrote: > for the sake of other chromium developers can you attempt to use stddint types > (int, size_t, uint32, et al) as much as possible? Done. http://codereview.chromium.org/10909185/diff/1005/media/audio/mac/audio_synch... media/audio/mac/audio_synchronized_mac.cc:669: input_data_ = static_cast<AudioBufferList*>(malloc(malloc_size)); On 2012/09/13 13:06:03, scherkus wrote: > any reason to prefer malloc/free over new/delete here + below? For the AudioBufferList allocation, I'm using malloc since otherwise (I think) I'd have to create two member variables (input_data_ and "input_data_storage_" of type uint8*) http://codereview.chromium.org/10909185/diff/1005/media/audio/mac/audio_synch... media/audio/mac/audio_synchronized_mac.cc:677: input_data_->mBuffers[i].mData = malloc(buffer_size_bytes); I've removed the malloc() here and am now using an AudioBus allocation |input_bus_| http://codereview.chromium.org/10909185/diff/1005/media/audio/mac/audio_synch... media/audio/mac/audio_synchronized_mac.cc:732: if (fifo_.frames() < static_cast<int>(number_of_frames)) { On 2012/09/13 13:06:03, scherkus wrote: > FYI: fifo_ not locked > > See my comment on line 817 regarding which threads these various procs are > running on I've completely removed this fifo_lock_ and have actually made AudioFifo lock-free / thread-safe. As I mentioned, having to lock in a real-time thread can and does cause glitching. I was hitting it quite a bit in my tuning on Thursday, so realized that we really need to do this properly. In other words, no need to lock anymore :) http://codereview.chromium.org/10909185/diff/1005/media/audio/mac/audio_synch... media/audio/mac/audio_synchronized_mac.cc:749: size_t n = fifo_.frames(); On 2012/09/13 13:06:03, scherkus wrote: > not locked Fixed in AudioFifo thread-safety http://codereview.chromium.org/10909185/diff/1005/media/audio/mac/audio_synch... media/audio/mac/audio_synchronized_mac.cc:809: if (first_output_time_ < 0.) { On 2012/09/13 13:06:03, scherkus wrote: > 0.0 This code has now been removed. http://codereview.chromium.org/10909185/diff/1005/media/audio/mac/audio_synch... media/audio/mac/audio_synchronized_mac.cc:814: unsigned n = static_cast<unsigned>(in_to_out_sample_offset_); On 2012/09/13 13:06:03, scherkus wrote: > s/unsigned/int Removed/simplified http://codereview.chromium.org/10909185/diff/1005/media/audio/mac/audio_synch... media/audio/mac/audio_synchronized_mac.cc:817: fifo_.Push(&silence); On 2012/09/13 13:06:03, scherkus wrote: > FYI: not locked > > to confirm.. the output+varispeed proc run together on one thread and the input > proc run on a separate thread? > > for example it appears you set is_fifo_initialized_ in the output proc but read > it from the input proc -- should it also be protected? > > do we want to be protecting every access to fifo_ or only certain calls? The AudioFifo now has changes to make it lock-free thread-safe in a limited sense where one thread calls Push() and a second thread calls Consume() The AudioFifo::frames() method will always return an amount which is or was very recently valid. The "very recently" valid may sound worrisome, but it turns out that it will either be a value larger than it really is for the Push() thread, or a value smaller than it really is for the Consume() thread, so is always safe. http://codereview.chromium.org/10909185/diff/1005/media/audio/mac/audio_synch... File media/audio/mac/audio_synchronized_mac.h (right): http://codereview.chromium.org/10909185/diff/1005/media/audio/mac/audio_synch... media/audio/mac/audio_synchronized_mac.h:157: : id_(kAudioDeviceUnknown), On 2012/09/13 13:06:03, scherkus wrote: > should be indented by two more spaces Done. http://codereview.chromium.org/10909185/diff/1005/media/audio/mac/audio_synch... media/audio/mac/audio_synchronized_mac.h:195: UInt32 hardware_buffer_size_; On 2012/09/13 13:06:03, scherkus wrote: > any chance we can swap UInt32 for int, size_t, uint32, etc... ? Removed here and in a few other places. But there are many places where we're making direct use of CoreAudio API's where it's best to keep these. http://codereview.chromium.org/10909185/diff/1005/media/base/audio_bus.h File media/base/audio_bus.h (right): http://codereview.chromium.org/10909185/diff/1005/media/base/audio_bus.h#newc... media/base/audio_bus.h:40: ~AudioBus(); On 2012/09/14 09:14:00, DaleCurtis wrote: > On 2012/09/13 13:06:03, scherkus wrote: > > we'll likely have to chat about this change a bit more -- one of the > motivations > > behind having private ctors/dtors w/ static methods that return scoped_ptr<> > was > > to enforce that these objects have single owner and deletion/transfer of > > ownership is forbidden: > > > http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.h#newc... > > > > We can revisit that decision but I'm curious whether it's possible to > > pre-allocate the AudioBus objects you need inside of audio_synchronized_mac.cc > > > > For example, there are snippets of code like the following: > > AudioBus bus(channels_, number_of_frames, input_data_); > > > > ...both of those are member variable data and it's only the # of frames that > > might change but maybe that can be handled via some other mechanism. > > +1, I don't see why you made this change. You should just add a new OS_MAC > specific Create method. > > That said, I don't think this is the place for the AudioBufferList change, this > class should remain platform independent. I don't think any other class will use > the AudioBufferList method. Which means this is probably better done inside a > helper method inside that class. Something like: > > scoped_ptr<AudioBus> AudioSynchronizedStream::GetAudioBus(int frames, int > channels, AudioBufferList* buffer_list) { > std::vector temp_vector() > <convert buffer_list into temp_vector> > return AudioBus::WrapVector(frames, channels, temp_vector); > } Ok, I can change this part if necessary - not that big of a deal. But two advantages I see are: 1) Client code is much easier to read when using stack-based objects as compared with scoped_ptr<> style. We *do* actually have some classes which are allowed to be stack-based. 2) It's less efficient to create objects on the heap than on the stack. It's (occasionally) possible to block on memory allocations which is bad for real-time threads. Ideally, memory allocations shouldn't happen at all on real-time audio threads. Unfortunately, we *do* allocate in several other places already, but I don't like to pile on more.
http://codereview.chromium.org/10909185/diff/10004/media/base/audio_fifo.cc File media/base/audio_fifo.cc (right): http://codereview.chromium.org/10909185/diff/10004/media/base/audio_fifo.cc#n... media/base/audio_fifo.cc:85: NoBarrier_AtomicExchange(&frames_pushed_, new_frames_pushed); This (and the next one) should be NoBarrier_Store, because you're not doing anything with the return value. http://codereview.chromium.org/10909185/diff/10004/media/base/audio_fifo.h File media/base/audio_fifo.h (right): http://codereview.chromium.org/10909185/diff/10004/media/base/audio_fifo.h#ne... media/base/audio_fifo.h:41: int frames() const { return frames_pushed_ - frames_consumed_; } You should put a MemoryBarrier after these loads. I'd also recommend using NoBarrier_Load for consistency.
http://codereview.chromium.org/10909185/diff/1005/media/base/audio_bus.h File media/base/audio_bus.h (right): http://codereview.chromium.org/10909185/diff/1005/media/base/audio_bus.h#newc... media/base/audio_bus.h:40: ~AudioBus(); On 2012/09/15 00:06:08, Chris Rogers wrote: > On 2012/09/14 09:14:00, DaleCurtis wrote: > > On 2012/09/13 13:06:03, scherkus wrote: > > > we'll likely have to chat about this change a bit more -- one of the > > motivations > > > behind having private ctors/dtors w/ static methods that return scoped_ptr<> > > was > > > to enforce that these objects have single owner and deletion/transfer of > > > ownership is forbidden: > > > > > > http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.h#newc... > > > > > > We can revisit that decision but I'm curious whether it's possible to > > > pre-allocate the AudioBus objects you need inside of > audio_synchronized_mac.cc > > > > > > For example, there are snippets of code like the following: > > > AudioBus bus(channels_, number_of_frames, input_data_); > > > > > > ...both of those are member variable data and it's only the # of frames that > > > might change but maybe that can be handled via some other mechanism. > > > > +1, I don't see why you made this change. You should just add a new OS_MAC > > specific Create method. > > > > That said, I don't think this is the place for the AudioBufferList change, > this > > class should remain platform independent. I don't think any other class will > use > > the AudioBufferList method. Which means this is probably better done inside a > > helper method inside that class. Something like: > > > > scoped_ptr<AudioBus> AudioSynchronizedStream::GetAudioBus(int frames, int > > channels, AudioBufferList* buffer_list) { > > std::vector temp_vector() > > <convert buffer_list into temp_vector> > > return AudioBus::WrapVector(frames, channels, temp_vector); > > } > > Ok, I can change this part if necessary - not that big of a deal. But two > advantages I see are: > > 1) Client code is much easier to read when using stack-based objects as compared > with scoped_ptr<> style. We *do* actually have some classes which are allowed > to be stack-based. > > 2) It's less efficient to create objects on the heap than on the stack. It's > (occasionally) possible to block on memory allocations which is bad for > real-time threads. Ideally, memory allocations shouldn't happen at all on > real-time audio threads. Unfortunately, we *do* allocate in several other > places already, but I don't like to pile on more. > > This is going to result in real time allocation either way right? The AudioBus internally will call AlignedAlloc on construction. The only case where this might help is the wrapped methods, and even then I'm not so sure. If you're worried about allocations we can add a resize method to audio bus which intelligently figures out if it needs to reallocate or just adjust the frame cap. Then you can do your allocation once on initialize with a large size once and resize elsewhere. What do you think? I'd prefer to keep the existing methods unless I'm missing something.
I am going to defer my parts to Andrew and Dale, hope it is fine. http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... File media/audio/mac/audio_synchronized_mac.cc (right): http://codereview.chromium.org/10909185/diff/1/media/audio/mac/audio_synchron... media/audio/mac/audio_synchronized_mac.cc:137: AudioUnitUninitialize(input_unit_); On 2012/09/13 01:03:05, Chris Rogers wrote: > On 2012/09/12 09:11:56, xians1 wrote: > > Is it all right to call AudioUnitUninitialize if input_unit_ is 0? or > > AudioUnitInitialize has not been called? > > I've added extra checking here. It *is* ok to call AudioUnitInitialize() if it > has not yet been initialized. It will return an error, but is safe. good, thanks.
http://codereview.chromium.org/10909185/diff/10004/media/audio/mac/audio_mana... File media/audio/mac/audio_manager_mac.cc (right): http://codereview.chromium.org/10909185/diff/10004/media/audio/mac/audio_mana... media/audio/mac/audio_manager_mac.cc:265: } else { nit: else not needed http://codereview.chromium.org/10909185/diff/10004/media/audio/mac/audio_sync... File media/audio/mac/audio_synchronized_mac.cc (right): http://codereview.chromium.org/10909185/diff/10004/media/audio/mac/audio_sync... media/audio/mac/audio_synchronized_mac.cc:35: for (unsigned i = 0; i < io_data->mNumberBuffers; ++i) s/unsigned/int/ or size_t etc... http://codereview.chromium.org/10909185/diff/10004/media/audio/mac/audio_sync... media/audio/mac/audio_synchronized_mac.cc:701: for (unsigned i = 0; i < input_buffer_list_->mNumberBuffers; ++i) { ditto for unsigned http://codereview.chromium.org/10909185/diff/10004/media/audio/mac/audio_sync... File media/audio/mac/audio_synchronized_mac.h (right): http://codereview.chromium.org/10909185/diff/10004/media/audio/mac/audio_sync... media/audio/mac/audio_synchronized_mac.h:61: // remove extra // line http://codereview.chromium.org/10909185/diff/10004/media/audio/mac/audio_sync... media/audio/mac/audio_synchronized_mac.h:159: // Holds the actual data for |input_buffer_list_| add . http://codereview.chromium.org/10909185/diff/10004/media/audio/mac/audio_sync... media/audio/mac/audio_synchronized_mac.h:183: double ave_delta_; what's ave_delta_? if it's average can you use average_delta_ instead? http://codereview.chromium.org/10909185/diff/10004/media/base/audio_bus.cc File media/base/audio_bus.cc (right): http://codereview.chromium.org/10909185/diff/10004/media/base/audio_bus.cc#ne... media/base/audio_bus.cc:157: static_cast<float*>(buffer_list->mBuffers[source_idx].mData)); it does seem that we could have this bit of code as a helper method for mac w/o introducing OS-specific bits into an OS-independent class http://codereview.chromium.org/10909185/diff/10004/media/base/audio_bus.h File media/base/audio_bus.h (right): http://codereview.chromium.org/10909185/diff/10004/media/base/audio_bus.h#new... media/base/audio_bus.h:37: AudioBus(int channels, int frames, AudioBufferList* buffer_list); let's try to close out this issue sooner rather than later -- perhaps using a resize() method as dale suggests http://codereview.chromium.org/10909185/diff/10004/media/base/audio_fifo.cc File media/base/audio_fifo.cc (right): http://codereview.chromium.org/10909185/diff/10004/media/base/audio_fifo.cc#n... media/base/audio_fifo.cc:84: Atomic32 new_frames_pushed = frames_pushed_ + source_size; is the intent of this change to make this class lock-free? I don't see any mention of it in the CL description
PTAL: I've added in the correct logic to AudioManagerMac, and reduced the changes to AudioBus so the constructors are no longer exposed, etc. http://codereview.chromium.org/10909185/diff/10004/media/audio/mac/audio_mana... File media/audio/mac/audio_manager_mac.cc (right): http://codereview.chromium.org/10909185/diff/10004/media/audio/mac/audio_mana... media/audio/mac/audio_manager_mac.cc:265: } else { On 2012/09/17 14:51:17, scherkus wrote: > nit: else not needed Fixed: Also, please note new logic with HasUnifiedDefaultIO() since last patch-set http://codereview.chromium.org/10909185/diff/10004/media/audio/mac/audio_sync... File media/audio/mac/audio_synchronized_mac.cc (right): http://codereview.chromium.org/10909185/diff/10004/media/audio/mac/audio_sync... media/audio/mac/audio_synchronized_mac.cc:35: for (unsigned i = 0; i < io_data->mNumberBuffers; ++i) On 2012/09/17 14:51:17, scherkus wrote: > s/unsigned/int/ or size_t etc... Done. http://codereview.chromium.org/10909185/diff/10004/media/audio/mac/audio_sync... media/audio/mac/audio_synchronized_mac.cc:701: for (unsigned i = 0; i < input_buffer_list_->mNumberBuffers; ++i) { On 2012/09/17 14:51:17, scherkus wrote: > ditto for unsigned Done. http://codereview.chromium.org/10909185/diff/10004/media/audio/mac/audio_sync... File media/audio/mac/audio_synchronized_mac.h (right): http://codereview.chromium.org/10909185/diff/10004/media/audio/mac/audio_sync... media/audio/mac/audio_synchronized_mac.h:61: // On 2012/09/17 14:51:17, scherkus wrote: > remove extra // line Done. http://codereview.chromium.org/10909185/diff/10004/media/audio/mac/audio_sync... media/audio/mac/audio_synchronized_mac.h:159: // Holds the actual data for |input_buffer_list_| On 2012/09/17 14:51:17, scherkus wrote: > add . Done. http://codereview.chromium.org/10909185/diff/10004/media/audio/mac/audio_sync... media/audio/mac/audio_synchronized_mac.h:183: double ave_delta_; On 2012/09/17 14:51:17, scherkus wrote: > what's ave_delta_? > > if it's average can you use average_delta_ instead? Added comment and changed name as you suggested. http://codereview.chromium.org/10909185/diff/10004/media/base/audio_fifo.cc File media/base/audio_fifo.cc (right): http://codereview.chromium.org/10909185/diff/10004/media/base/audio_fifo.cc#n... media/base/audio_fifo.cc:84: Atomic32 new_frames_pushed = frames_pushed_ + source_size; On 2012/09/17 14:51:17, scherkus wrote: > is the intent of this change to make this class lock-free? > > I don't see any mention of it in the CL description Added section in description about thread-safety. http://codereview.chromium.org/10909185/diff/10004/media/base/audio_fifo.cc#n... media/base/audio_fifo.cc:85: NoBarrier_AtomicExchange(&frames_pushed_, new_frames_pushed); On 2012/09/15 00:33:52, jbauman wrote: > This (and the next one) should be NoBarrier_Store, because you're not doing > anything with the return value. Done. http://codereview.chromium.org/10909185/diff/10004/media/base/audio_fifo.h File media/base/audio_fifo.h (right): http://codereview.chromium.org/10909185/diff/10004/media/base/audio_fifo.h#ne... media/base/audio_fifo.h:41: int frames() const { return frames_pushed_ - frames_consumed_; } On 2012/09/15 00:33:52, jbauman wrote: > You should put a MemoryBarrier after these loads. I'd also recommend using > NoBarrier_Load for consistency. Actually, shouldn't it be before we read the values?
Please address my comments. In case you are able to make it for M23, you have my lgtm for it :) http://codereview.chromium.org/10909185/diff/10004/media/audio/mac/audio_sync... File media/audio/mac/audio_synchronized_mac.cc (right): http://codereview.chromium.org/10909185/diff/10004/media/audio/mac/audio_sync... media/audio/mac/audio_synchronized_mac.cc:8: #include <algorithm> nit, add an empty line? http://codereview.chromium.org/10909185/diff/10004/media/audio/mac/audio_sync... media/audio/mac/audio_synchronized_mac.cc:734: if (input_bus_->frames() < available_frames) <=? http://codereview.chromium.org/10909185/diff/10004/media/audio/mac/audio_sync... media/audio/mac/audio_synchronized_mac.cc:749: if (fifo_.frames() < static_cast<int>(number_of_frames)) { nit, do this before creating the bus. http://codereview.chromium.org/10909185/diff/10004/media/audio/mac/audio_sync... media/audio/mac/audio_synchronized_mac.cc:793: const double kCorrectionTimeSeconds = 0.100; 0.1? http://codereview.chromium.org/10909185/diff/10004/media/audio/mac/audio_sync... media/audio/mac/audio_synchronized_mac.cc:795: fifo_rate_compensation_ = why this needs to be a member? can it be a local variable if it is used only in this function? http://codereview.chromium.org/10909185/diff/10004/media/audio/mac/audio_sync... File media/audio/mac/audio_synchronized_mac.h (right): http://codereview.chromium.org/10909185/diff/10004/media/audio/mac/audio_sync... media/audio/mac/audio_synchronized_mac.h:84: AudioDeviceID GetInputDeviceID() { return input_info_.id_; } for the inline accessors use styles like get_input_device_id() const http://codereview.chromium.org/10909185/diff/10004/media/audio/mac/audio_sync... media/audio/mac/audio_synchronized_mac.h:85: AudioDeviceID GetOutputDeviceID() { return output_info_.id_; } ditto http://codereview.chromium.org/10909185/diff/10004/media/base/audio_bus.cc File media/base/audio_bus.cc (right): http://codereview.chromium.org/10909185/diff/10004/media/base/audio_bus.cc#ne... media/base/audio_bus.cc:13: #if defined(OS_MACOSX) move this above #include <limits> http://codereview.chromium.org/10909185/diff/10004/media/base/audio_bus.cc#ne... media/base/audio_bus.cc:155: for (int i = 0; i < channels; ++i) { we should be able to use only either source_idx or i http://codereview.chromium.org/10909185/diff/10004/media/base/audio_fifo.cc File media/base/audio_fifo.cc (right): http://codereview.chromium.org/10909185/diff/10004/media/base/audio_fifo.cc#n... media/base/audio_fifo.cc:132: void AudioFifo::Clear() { when this Clear() is called? is there any potential race with Consume and Push()?
http://codereview.chromium.org/10909185/diff/6011/media/audio/audio_output_re... File media/audio/audio_output_resampler.cc (right): http://codereview.chromium.org/10909185/diff/6011/media/audio/audio_output_re... media/audio/audio_output_resampler.cc:289: SourceIOCallback_Locked(NULL, dest); As mentioned on chat, I feel like this is just asking for an accidental NULL ptr dereference later when something hits the FIFO/resampler. http://codereview.chromium.org/10909185/diff/6011/media/base/audio_bus.cc File media/base/audio_bus.cc (right): http://codereview.chromium.org/10909185/diff/6011/media/base/audio_bus.cc#new... media/base/audio_bus.cc:184: void AudioBus::SetChannelData(int channel, float* data) { This can lead to some pretty confusing situations: 1. WrapMemory(<some contiguous block>) 2. SetChannelData(0, <some channel data>) Now the wrapped bus isn't writing all of its data to the wrapped memory region. Maybe we should prevent usage of this method with WrapMemory and WrapVector objects? If so the constructor above should set <some flag> and DCHECK(data_.get() || <some flag>); http://codereview.chromium.org/10909185/diff/6011/media/base/audio_bus.h File media/base/audio_bus.h (right): http://codereview.chromium.org/10909185/diff/6011/media/base/audio_bus.h#newc... media/base/audio_bus.h:36: static scoped_ptr<AudioBus> Create(int channels); Since this technically isn't a normal Create() how about: CreateWrapper() http://codereview.chromium.org/10909185/diff/6011/media/base/audio_bus.h#newc... media/base/audio_bus.h:86: void set_frames(int frames) { frames_ = frames; } Hmm, I was thinking more along the lines of: void CapFrames(int frames) { CHECK_LE(frames, original_frames); frames_ = frames; } This would allow us to use this method inside AUAudioOutputStream, per last week's discussions, provided we give a large initial frame size. http://codereview.chromium.org/10909185/diff/6011/media/base/audio_fifo.cc File media/base/audio_fifo.cc (right): http://codereview.chromium.org/10909185/diff/6011/media/base/audio_fifo.cc#ne... media/base/audio_fifo.cc:56: return frames_pushed_ - frames_consumed_; Seems like this could overflow on 32-bit systems pretty easily?
PTAL xians: I've addressed your comments, except where I've made a note http://codereview.chromium.org/10909185/diff/10004/media/audio/mac/audio_sync... File media/audio/mac/audio_synchronized_mac.cc (right): http://codereview.chromium.org/10909185/diff/10004/media/audio/mac/audio_sync... media/audio/mac/audio_synchronized_mac.cc:734: if (input_bus_->frames() < available_frames) On 2012/09/17 20:47:46, xians1 wrote: > <=? Done. http://codereview.chromium.org/10909185/diff/10004/media/audio/mac/audio_sync... media/audio/mac/audio_synchronized_mac.cc:749: if (fifo_.frames() < static_cast<int>(number_of_frames)) { On 2012/09/17 20:47:46, xians1 wrote: > nit, do this before creating the bus. Can't do that because we also need access to the bus in the Consume() method below, and we return early inside of the if() http://codereview.chromium.org/10909185/diff/10004/media/audio/mac/audio_sync... media/audio/mac/audio_synchronized_mac.cc:793: const double kCorrectionTimeSeconds = 0.100; On 2012/09/17 20:47:46, xians1 wrote: > 0.1? Done. http://codereview.chromium.org/10909185/diff/10004/media/audio/mac/audio_sync... media/audio/mac/audio_synchronized_mac.cc:795: fifo_rate_compensation_ = On 2012/09/17 20:47:46, xians1 wrote: > why this needs to be a member? can it be a local variable if it is used only in > this function? Good point, but actually I intend to maintain this as a state variable with some further tweaks to the averaging very soon -- so keeping this http://codereview.chromium.org/10909185/diff/10004/media/audio/mac/audio_sync... File media/audio/mac/audio_synchronized_mac.h (right): http://codereview.chromium.org/10909185/diff/10004/media/audio/mac/audio_sync... media/audio/mac/audio_synchronized_mac.h:84: AudioDeviceID GetInputDeviceID() { return input_info_.id_; } On 2012/09/17 20:47:46, xians1 wrote: > for the inline accessors use styles like get_input_device_id() const This is not a strict style rule, just a recommendation. http://codereview.chromium.org/10909185/diff/10004/media/base/audio_fifo.cc File media/base/audio_fifo.cc (right): http://codereview.chromium.org/10909185/diff/10004/media/base/audio_fifo.cc#n... media/base/audio_fifo.cc:132: void AudioFifo::Clear() { On 2012/09/17 20:47:46, xians1 wrote: > when this Clear() is called? is there any potential race with Consume and > Push()? Yes there is, but I already mention in the comments that this class is thread-safe in a "limited sense" for Push() and Consume(), this thread-safety does not extend to Clear(). http://codereview.chromium.org/10909185/diff/6011/media/audio/audio_output_re... File media/audio/audio_output_resampler.cc (right): http://codereview.chromium.org/10909185/diff/6011/media/audio/audio_output_re... media/audio/audio_output_resampler.cc:289: SourceIOCallback_Locked(NULL, dest); On 2012/09/17 21:17:36, DaleCurtis wrote: > As mentioned on chat, I feel like this is just asking for an accidental NULL ptr > dereference later when something hits the FIFO/resampler. How? http://codereview.chromium.org/10909185/diff/6011/media/base/audio_bus.cc File media/base/audio_bus.cc (right): http://codereview.chromium.org/10909185/diff/6011/media/base/audio_bus.cc#new... media/base/audio_bus.cc:184: void AudioBus::SetChannelData(int channel, float* data) { On 2012/09/17 21:17:36, DaleCurtis wrote: > This can lead to some pretty confusing situations: > 1. WrapMemory(<some contiguous block>) > 2. SetChannelData(0, <some channel data>) > > Now the wrapped bus isn't writing all of its data to the wrapped memory region. > > Maybe we should prevent usage of this method with WrapMemory and WrapVector > objects? If so the constructor above should set <some flag> and > DCHECK(data_.get() || <some flag>); We can discuss offline... http://codereview.chromium.org/10909185/diff/6011/media/base/audio_bus.h File media/base/audio_bus.h (right): http://codereview.chromium.org/10909185/diff/6011/media/base/audio_bus.h#newc... media/base/audio_bus.h:36: static scoped_ptr<AudioBus> Create(int channels); On 2012/09/17 21:17:36, DaleCurtis wrote: > Since this technically isn't a normal Create() how about: CreateWrapper() Ok, SGTM http://codereview.chromium.org/10909185/diff/6011/media/base/audio_bus.h#newc... media/base/audio_bus.h:86: void set_frames(int frames) { frames_ = frames; } On 2012/09/17 21:17:36, DaleCurtis wrote: > Hmm, I was thinking more along the lines of: > > void CapFrames(int frames) { > CHECK_LE(frames, original_frames); > frames_ = frames; > } > > This would allow us to use this method inside AUAudioOutputStream, per last > week's discussions, provided we give a large initial frame size. Hmmm, I'm pretty sure we're going to want to be able to *precisely* set the exact number of frames. As an aside, I think that what we're doing in AUAudioOutputStream is a mistake, but that's another story... http://codereview.chromium.org/10909185/diff/6011/media/base/audio_fifo.cc File media/base/audio_fifo.cc (right): http://codereview.chromium.org/10909185/diff/6011/media/base/audio_fifo.cc#ne... media/base/audio_fifo.cc:56: return frames_pushed_ - frames_consumed_; On 2012/09/17 21:17:36, DaleCurtis wrote: > Seems like this could overflow on 32-bit systems pretty easily? It can, but the math still works out properly if either one or both overflows in either direction.
On 2012/09/17 20:44:23, Chris Rogers wrote: > http://codereview.chromium.org/10909185/diff/10004/media/base/audio_fifo.h > File media/base/audio_fifo.h (right): > > http://codereview.chromium.org/10909185/diff/10004/media/base/audio_fifo.h#ne... > media/base/audio_fifo.h:41: int frames() const { return frames_pushed_ - > frames_consumed_; } > On 2012/09/15 00:33:52, jbauman wrote: > > You should put a MemoryBarrier after these loads. I'd also recommend using > > NoBarrier_Load for consistency. > > Actually, shouldn't it be before we read the values? No, it should be afterwards. This variable will generally be used before accessing the buffer (to determine how much space is left in it), so we want a memory barrier to separate reading this variable from modifying the buffer. This is symmetrical with the use of the memory barriers elsewhere.
PTAL jbauman: I've moved the MemoryBarrier to after computing the delta. dalecurtis: I've added |can_set_channel_data_| and CHECKs in SetChannelData()
Fixed minor Windows compile error relating to namespace of MemoryBarrier()
lgtm w/ nits http://codereview.chromium.org/10909185/diff/11018/media/audio/mac/audio_mana... File media/audio/mac/audio_manager_mac.cc (right): http://codereview.chromium.org/10909185/diff/11018/media/audio/mac/audio_mana... media/audio/mac/audio_manager_mac.cc:46: // Returns if the default input device is the same as the default output. Returns... true? :) http://codereview.chromium.org/10909185/diff/11018/media/audio/mac/audio_mana... media/audio/mac/audio_manager_mac.cc:301: } else { nit: no need for else due to early return http://codereview.chromium.org/10909185/diff/11018/media/audio/mac/audio_sync... File media/audio/mac/audio_synchronized_mac.cc (right): http://codereview.chromium.org/10909185/diff/11018/media/audio/mac/audio_sync... media/audio/mac/audio_synchronized_mac.cc:50: // It's ok to pass in a |buffer_list| with fewer channels, in which update comment? I'm not seeing us pass stuff in
audio_bus parts lgtm.
Thanks! Andrew, I addressed your last comments on commit. https://chromiumcodereview.appspot.com/10909185/diff/11018/media/audio/mac/au... File media/audio/mac/audio_manager_mac.cc (right): https://chromiumcodereview.appspot.com/10909185/diff/11018/media/audio/mac/au... media/audio/mac/audio_manager_mac.cc:46: // Returns if the default input device is the same as the default output. On 2012/09/17 23:07:17, scherkus wrote: > Returns... true? :) Done. https://chromiumcodereview.appspot.com/10909185/diff/11018/media/audio/mac/au... media/audio/mac/audio_manager_mac.cc:301: } else { On 2012/09/17 23:07:17, scherkus wrote: > nit: no need for else due to early return Done. https://chromiumcodereview.appspot.com/10909185/diff/11018/media/audio/mac/au... File media/audio/mac/audio_synchronized_mac.cc (right): https://chromiumcodereview.appspot.com/10909185/diff/11018/media/audio/mac/au... media/audio/mac/audio_synchronized_mac.cc:50: // It's ok to pass in a |buffer_list| with fewer channels, in which On 2012/09/17 23:07:17, scherkus wrote: > update comment? > > I'm not seeing us pass stuff in I've moved part of this comment down to line 57 where it really applies.
http://codereview.chromium.org/10909185/diff/11018/media/base/audio_fifo.cc File media/base/audio_fifo.cc (right): http://codereview.chromium.org/10909185/diff/11018/media/base/audio_fifo.cc#n... media/base/audio_fifo.cc:129: } Could you make sure to put another memory barrier here? It'll ensure the producer won't overwrite data in the buffer that hasn't quite been consumed yet. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
