Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10916105:
Add Mac OS X unified audio I/O back-end (Closed)
|
Created:
8 years, 3 months ago by Chris Rogers Modified:
8 years, 3 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionAdd Mac OS X unified audio I/O back-end
Implementation of AudioOuputStream for Mac OS X using the
CoreAudio AudioHardware API suitable for low-latency synchronized audio I/O.
For use with professional audio devices (Firewire/USB/etc.) which support *both* input and output
in the same driver callback.
BUG=none
TEST=none
(manually tested on several different audio hardware devices on Snow Leopard and Mountain Lion)
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156919
Patch Set 1 #Patch Set 2 : #
Total comments: 89
Patch Set 3 : #Patch Set 4 : #
Total comments: 15
Patch Set 5 : #
Messages
Total messages: 14 (0 generated)
PTAL - thanks!
This needs to land after: http://codereview.chromium.org/10830268/ Thus the build-bot error...
Hi Shijing, could you please have a look?
nice! apologies about the delay -- I mistakenly archived this review in my inbox :( https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... File media/audio/mac/audio_unified_mac.cc (right): https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:17: #define DEST_CHANNEL_OFFSET 0 can we remove this or is there some other way to accomplish the same testing w/o checking in test code like this? https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:21: const int kDefaultInputChannels = 2; static https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:37: // We must have a manager. nit: remove comment (not adding any value) https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:39: // A frame is one sample across all channels. In interleaved audio the per nit: blank lines before new // comment blocks https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:56: client_input_channels_ = kDefaultInputChannels; looks like we can move this into ctor initializer list https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:58: input_bus_ = AudioBus::Create(client_input_channels_, move client_input_channels_ to next line w/ 4 space indent OR align next line w/ client_input_channels_ https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:64: Stop(); fix indent (should be 2 spaces) https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:108: LOG(ERROR) << "Requested sample-rate must match the hardware sample-rate."; nit: can you emit sample the two sample rates? https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:118: result = AudioObjectSetPropertyData(device_, &pa, 0, 0, nit: fix indent style -- should either look like the other AudioObjectGetProperyData() calls here OR align next line to match ( https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:122: result = AudioObjectSetPropertyData(device_, &pa, 0, 0, ditto https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:170: AudioObjectGetPropertyDataSize(device_, &pa, 0, 0, &size); do we need to check the return value of these calls + ones below? https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:207: Stop(); // make sure to stop if the client forgot. Can we treat this as a programmer error / DCHECK()? https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:220: << "Open() has not been called successfully"; DCHECK() instead? https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:229: // input_data_.reset(new int16[number_of_frames_ * input_channels_]); ??? https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:261: OSStatus AudioHardwareUnifiedStream::Render( Should we have two Render() methods for interleaved vs non-interleaved? There seems to be a lot of internal branching + TODOs re: memcpy() if source is planar. I feel the minor code duplication will be worth the clarity for handling the interleaved case vs non-interleaved case. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:262: AudioDeviceID inDevice, unix_hacker https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:271: for (unsigned channel_index = 0; channel_index < client_input_channels_; s/unsigned/int/ https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:272: ++channel_index) { indent one more space https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:279: sourceP = static_cast<float*>(inInputData->mBuffers[0].mData) + unix_hacker perhaps _ptr instead of P? https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:308: ++channel_index) { indent one more space https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:334: AudioDeviceID inDevice, unix_hacker https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... File media/audio/mac/audio_unified_mac.h (right): https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.h:6: // It's required to first get the native sample-rate of the default can we merge this into the class docs? https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.h:46: unsigned input_channels() const { return input_channels_; } s/unsigned/int/ here and everywhere else Rest of the audio codebase has settled on int for channel counts. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.h:50: OSStatus Render(AudioDeviceID inDevice, unix_hacker style for all parmas https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.h:57: static OSStatus RenderProc(AudioDeviceID inDevice, unix_hacker style https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.h:78: unsigned client_input_channels_; s/unsigned/int/ here and below https://chromiumcodereview.appspot.com/10916105/diff/10002/media/media.gyp File media/media.gyp (right): https://chromiumcodereview.appspot.com/10916105/diff/10002/media/media.gyp#ne... media/media.gyp:118: remove all these blank lines https://chromiumcodereview.appspot.com/10916105/diff/10002/media/media.gyp#ne... media/media.gyp:120: 'audio/mac/audio_synchronized_mac.h', I don't see these in the CL -- remove? https://chromiumcodereview.appspot.com/10916105/diff/10002/media/media.gyp#ne... media/media.gyp:122: 'audio/mac/CAPlayThrough/AudioChannel.cpp', remove?
https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... File media/audio/mac/audio_unified_mac.cc (right): https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:29: volume_(1), nit, 1.0f? https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:64: Stop(); It looks like a programming error if the clients do not stop/close the stream before deleting it. Remove the Stop() and add a DCHECK on device_. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:68: device_ = kAudioDeviceUnknown; nit, device_ has been initialized in the constructor, use a DCHECK_EQ(device_, kAudioDeviceUnknown) instead https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:120: add a DCHECK here to verify the result? https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:122: result = AudioObjectSetPropertyData(device_, &pa, 0, 0, ditto https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:137: result = AudioObjectGetPropertyDataSize(device_, &pa, 0, 0, &size); We are not sure if the machine has input device or not, check the result and size before allocating memory, https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:142: result = AudioObjectGetPropertyData(device_, &pa, 0, 0, &size, &input_list); DCHECK(result) ? https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:147: if (input_channels_per_frame_ == 1 && input_list.mNumberBuffers > 1) { We don't need to check input_list.mNumberBuffers > 1 here. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:170: AudioObjectGetPropertyDataSize(device_, &pa, 0, 0, &size); DCHECK the size bigger than 0 https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:179: if (output_channels_per_frame_ == 1 && output_list.mNumberBuffers > 1) { no need to check output_list.mNumberBuffers > 1 https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:209: AudioDeviceDestroyIOProcID(device_, io_proc_id_); set device_ to kAudioObjectUnknown https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:230: // output_data_.reset(new int16[number_of_frames_ * output_channels_]); looks like debugging code, remove? https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:269: // TODO(crogers): its better to simply memcpy() if source is already planar. nit, it's or remove its https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:272: ++channel_index) { nit, fix the indentation. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:273: float* sourceP; source, or source_ptr? https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:309: float* destP; dest or dest_ptr https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... File media/audio/mac/audio_unified_mac.h (right): https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.h:84: unsigned input_channels_; I know they are the same, but how about using unsigned int to be more specific? https://chromiumcodereview.appspot.com/10916105/diff/10002/media/media.gyp File media/media.gyp (right): https://chromiumcodereview.appspot.com/10916105/diff/10002/media/media.gyp#ne... media/media.gyp:127: 'audio/mac/CAPlayThrough/AudioFIFO.h', are these files part of this CL?
Great work, Henrik showed us some demos using getUserMedia for web-audio, it is awesome!!!
Thanks for the reviews! I think I've addressed all comments except for Andrew's suggested re-factoring of Render(). I'll look into seeing if Render() can be better factored... https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... File media/audio/mac/audio_unified_mac.cc (right): https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:17: #define DEST_CHANNEL_OFFSET 0 On 2012/09/10 11:26:19, scherkus wrote: > can we remove this or is there some other way to accomplish the same testing w/o > checking in test code like this? Yes, this was only for testing -- removed! https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:21: const int kDefaultInputChannels = 2; On 2012/09/10 11:26:19, scherkus wrote: > static Done. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:29: volume_(1), On 2012/09/10 12:14:59, xians1 wrote: > nit, 1.0f? Done. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:37: // We must have a manager. On 2012/09/10 11:26:19, scherkus wrote: > nit: remove comment (not adding any value) Done. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:39: // A frame is one sample across all channels. In interleaved audio the per On 2012/09/10 11:26:19, scherkus wrote: > nit: blank lines before new // comment blocks Done. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:56: client_input_channels_ = kDefaultInputChannels; On 2012/09/10 11:26:19, scherkus wrote: > looks like we can move this into ctor initializer list Done. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:58: input_bus_ = AudioBus::Create(client_input_channels_, On 2012/09/10 11:26:19, scherkus wrote: > move client_input_channels_ to next line w/ 4 space indent OR align next line w/ > client_input_channels_ Done. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:64: Stop(); On 2012/09/10 11:26:19, scherkus wrote: > fix indent (should be 2 spaces) Removed, and added DCHECK https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:68: device_ = kAudioDeviceUnknown; On 2012/09/10 12:14:59, xians1 wrote: > nit, device_ has been initialized in the constructor, use a DCHECK_EQ(device_, > kAudioDeviceUnknown) instead Done. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:108: LOG(ERROR) << "Requested sample-rate must match the hardware sample-rate."; On 2012/09/10 11:26:19, scherkus wrote: > nit: can you emit sample the two sample rates? Done. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:118: result = AudioObjectSetPropertyData(device_, &pa, 0, 0, On 2012/09/10 11:26:19, scherkus wrote: > nit: fix indent style -- should either look like the other > AudioObjectGetProperyData() calls here OR align next line to match ( Done. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:120: On 2012/09/10 12:14:59, xians1 wrote: > add a DCHECK here to verify the result? Fixed: I'm now logging an error here and returning early. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:122: result = AudioObjectSetPropertyData(device_, &pa, 0, 0, On 2012/09/10 11:26:19, scherkus wrote: > ditto Done. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:122: result = AudioObjectSetPropertyData(device_, &pa, 0, 0, On 2012/09/10 12:14:59, xians1 wrote: > ditto Done. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:137: result = AudioObjectGetPropertyDataSize(device_, &pa, 0, 0, &size); On 2012/09/10 12:14:59, xians1 wrote: > We are not sure if the machine has input device or not, check the result and > size before allocating memory, Done. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:142: result = AudioObjectGetPropertyData(device_, &pa, 0, 0, &size, &input_list); On 2012/09/10 12:14:59, xians1 wrote: > DCHECK(result) ? Done. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:147: if (input_channels_per_frame_ == 1 && input_list.mNumberBuffers > 1) { On 2012/09/10 12:14:59, xians1 wrote: > We don't need to check input_list.mNumberBuffers > 1 here. I think it's better to be more explicit since this condition must be true https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:170: AudioObjectGetPropertyDataSize(device_, &pa, 0, 0, &size); On 2012/09/10 11:26:19, scherkus wrote: > do we need to check the return value of these calls + ones below? Done. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:170: AudioObjectGetPropertyDataSize(device_, &pa, 0, 0, &size); On 2012/09/10 12:14:59, xians1 wrote: > DCHECK the size bigger than 0 Done. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:179: if (output_channels_per_frame_ == 1 && output_list.mNumberBuffers > 1) { On 2012/09/10 12:14:59, xians1 wrote: > no need to check output_list.mNumberBuffers > 1 I just want to be more explicit here. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:207: Stop(); // make sure to stop if the client forgot. On 2012/09/10 11:26:19, scherkus wrote: > Can we treat this as a programmer error / DCHECK()? Done. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:209: AudioDeviceDestroyIOProcID(device_, io_proc_id_); On 2012/09/10 12:14:59, xians1 wrote: > set device_ to kAudioObjectUnknown Done. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:220: << "Open() has not been called successfully"; On 2012/09/10 11:26:19, scherkus wrote: > DCHECK() instead? Done. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:229: // input_data_.reset(new int16[number_of_frames_ * input_channels_]); On 2012/09/10 11:26:19, scherkus wrote: > ??? Done. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:230: // output_data_.reset(new int16[number_of_frames_ * output_channels_]); On 2012/09/10 12:14:59, xians1 wrote: > looks like debugging code, remove? Done. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:261: OSStatus AudioHardwareUnifiedStream::Render( On 2012/09/10 11:26:19, scherkus wrote: > Should we have two Render() methods for interleaved vs non-interleaved? > > There seems to be a lot of internal branching + TODOs re: memcpy() if source is > planar. I feel the minor code duplication will be worth the clarity for handling > the interleaved case vs non-interleaved case. The problem is that the interleave vs. non-interleaved can happen on both the input *and* output side, so there would need to be 2x2 == 4 separate methods. I'll have another look to see if I can clean up more... https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:262: AudioDeviceID inDevice, On 2012/09/10 11:26:19, scherkus wrote: > unix_hacker Done. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:269: // TODO(crogers): its better to simply memcpy() if source is already planar. On 2012/09/10 12:14:59, xians1 wrote: > nit, it's or remove its Done. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:271: for (unsigned channel_index = 0; channel_index < client_input_channels_; On 2012/09/10 11:26:19, scherkus wrote: > s/unsigned/int/ Done. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:272: ++channel_index) { On 2012/09/10 11:26:19, scherkus wrote: > indent one more space Done. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:272: ++channel_index) { On 2012/09/10 12:14:59, xians1 wrote: > nit, fix the indentation. Done. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:273: float* sourceP; On 2012/09/10 12:14:59, xians1 wrote: > source, or source_ptr? Done. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:279: sourceP = static_cast<float*>(inInputData->mBuffers[0].mData) + On 2012/09/10 11:26:19, scherkus wrote: > unix_hacker > > perhaps _ptr instead of P? just simplified to |source| https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:308: ++channel_index) { On 2012/09/10 11:26:19, scherkus wrote: > indent one more space Done. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:309: float* destP; On 2012/09/10 12:14:59, xians1 wrote: > dest or dest_ptr Done. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.cc:334: AudioDeviceID inDevice, On 2012/09/10 11:26:19, scherkus wrote: > unix_hacker Done. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... File media/audio/mac/audio_unified_mac.h (right): https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.h:6: // It's required to first get the native sample-rate of the default On 2012/09/10 11:26:19, scherkus wrote: > can we merge this into the class docs? Done. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.h:46: unsigned input_channels() const { return input_channels_; } On 2012/09/10 11:26:19, scherkus wrote: > s/unsigned/int/ here and everywhere else > > Rest of the audio codebase has settled on int for channel counts. Done. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.h:50: OSStatus Render(AudioDeviceID inDevice, On 2012/09/10 11:26:19, scherkus wrote: > unix_hacker style for all parmas Done. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.h:57: static OSStatus RenderProc(AudioDeviceID inDevice, On 2012/09/10 11:26:19, scherkus wrote: > unix_hacker style Done. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.h:78: unsigned client_input_channels_; On 2012/09/10 11:26:19, scherkus wrote: > s/unsigned/int/ here and below Done. https://chromiumcodereview.appspot.com/10916105/diff/10002/media/audio/mac/au... media/audio/mac/audio_unified_mac.h:84: unsigned input_channels_; On 2012/09/10 12:14:59, xians1 wrote: > I know they are the same, but how about using unsigned int to be more specific? I switched to "int" according to Andrew's comments.
lgtm w/ nits + q http://codereview.chromium.org/10916105/diff/8/media/audio/mac/audio_unified_... File media/audio/mac/audio_unified_mac.cc (right): http://codereview.chromium.org/10916105/diff/8/media/audio/mac/audio_unified_... media/audio/mac/audio_unified_mac.cc:350: for (unsigned channel_index = 0; channel_index < format_.mChannelsPerFrame; s/unsigned/int/ http://codereview.chromium.org/10916105/diff/8/media/audio/mac/audio_unified_... media/audio/mac/audio_unified_mac.cc:354: unsigned dest_channel_index = channel_index; s/unsigned/int/ http://codereview.chromium.org/10916105/diff/8/media/audio/mac/audio_unified_... media/audio/mac/audio_unified_mac.cc:388: return -1; q: is there a better return code to return here or does it not matter? http://codereview.chromium.org/10916105/diff/8/media/audio/mac/audio_unified_... File media/audio/mac/audio_unified_mac.h (right): http://codereview.chromium.org/10916105/diff/8/media/audio/mac/audio_unified_... media/audio/mac/audio_unified_mac.h:26: nit: remove blank line http://codereview.chromium.org/10916105/diff/8/media/audio/mac/audio_unified_... media/audio/mac/audio_unified_mac.h:96: static int kDefaultInputChannels; oh whoops -- I mean this can be compilation-unit static (i.e., not part of this class definition but just a good old fashioned static const int inside the .cc)
lgtm with three nits. http://codereview.chromium.org/10916105/diff/8/media/audio/mac/audio_unified_... File media/audio/mac/audio_unified_mac.cc (right): http://codereview.chromium.org/10916105/diff/8/media/audio/mac/audio_unified_... media/audio/mac/audio_unified_mac.cc:101: LOG(ERROR) << "Requested sample-rate must match the hardware sample-rate."; nit, we can log only one time: << "Requested sample-rate: " << format_.mSampleRate << " must match the hardware sample-rate: " << sample_rate; http://codereview.chromium.org/10916105/diff/8/media/audio/mac/audio_unified_... media/audio/mac/audio_unified_mac.cc:285: OSStatus result = AudioDeviceStop(device_, io_proc_id_); I think you will have a compiling warning about "Unused variable result" in release build here. http://codereview.chromium.org/10916105/diff/8/media/audio/mac/audio_unified_... media/audio/mac/audio_unified_mac.cc:304: OSStatus AudioHardwareUnifiedStream::Render( This function will only return noErr, how do you thinking making it void, and RenderProc return noErr after calling Render(). I am actually fine with the current solution, so ignore this comment if you prefer the current approach.
https://chromiumcodereview.appspot.com/10916105/diff/8/media/audio/mac/audio_... File media/audio/mac/audio_unified_mac.cc (right): https://chromiumcodereview.appspot.com/10916105/diff/8/media/audio/mac/audio_... media/audio/mac/audio_unified_mac.cc:101: LOG(ERROR) << "Requested sample-rate must match the hardware sample-rate."; On 2012/09/11 15:40:28, xians1 wrote: > nit, we can log only one time: > << "Requested sample-rate: " << format_.mSampleRate > << " must match the hardware sample-rate: " << sample_rate; Done. https://chromiumcodereview.appspot.com/10916105/diff/8/media/audio/mac/audio_... media/audio/mac/audio_unified_mac.cc:304: OSStatus AudioHardwareUnifiedStream::Render( On 2012/09/11 15:40:28, xians1 wrote: > This function will only return noErr, how do you thinking making it void, and > RenderProc return noErr after calling Render(). > I am actually fine with the current solution, so ignore this comment if you > prefer the current approach. It's probably fine either way, but leaving it because we may modify the code to return an actual error and to be consistent with similar code in AUAudioOutputStream and AudioSynchronizedStream. https://chromiumcodereview.appspot.com/10916105/diff/8/media/audio/mac/audio_... media/audio/mac/audio_unified_mac.cc:350: for (unsigned channel_index = 0; channel_index < format_.mChannelsPerFrame; On 2012/09/11 12:22:25, scherkus wrote: > s/unsigned/int/ Done. https://chromiumcodereview.appspot.com/10916105/diff/8/media/audio/mac/audio_... media/audio/mac/audio_unified_mac.cc:354: unsigned dest_channel_index = channel_index; On 2012/09/11 12:22:25, scherkus wrote: > s/unsigned/int/ Done. https://chromiumcodereview.appspot.com/10916105/diff/8/media/audio/mac/audio_... media/audio/mac/audio_unified_mac.cc:388: return -1; On 2012/09/11 12:22:25, scherkus wrote: > q: is there a better return code to return here or does it not matter? This is really an anomaly which should never happen in any real run-time situation. I don't think there's any reasonable CoreAudio error code here, and this value will just be ignored. https://chromiumcodereview.appspot.com/10916105/diff/8/media/audio/mac/audio_... File media/audio/mac/audio_unified_mac.h (right): https://chromiumcodereview.appspot.com/10916105/diff/8/media/audio/mac/audio_... media/audio/mac/audio_unified_mac.h:26: On 2012/09/11 12:22:25, scherkus wrote: > nit: remove blank line Done. https://chromiumcodereview.appspot.com/10916105/diff/8/media/audio/mac/audio_... media/audio/mac/audio_unified_mac.h:96: static int kDefaultInputChannels; On 2012/09/11 12:22:25, scherkus wrote: > oh whoops -- I mean this can be compilation-unit static (i.e., not part of this > class definition but just a good old fashioned static const int inside the .cc) Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/crogers@google.com/10916105/11002
Sorry for I got bad news for ya. Compile failed with a clobber build. Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/crogers@google.com/10916105/11002
Change committed as 156919 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
