Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8234009:
Adding input and output delay estimation for mac. (Closed)
|
Created:
9 years, 2 months ago by no longer working on chromium Modified:
9 years, 1 month ago CC:
chromium-reviews, hclam+watch_chromium.org, ddorwin+watch_chromium.org, fischman+watch_chromium.org, acolwell+watch_chromium.org, annacc+watch_chromium.org, ajwong+watch_chromium.org, vrk (LEFT CHROMIUM), scherkus (not reviewing) Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionAdding input and output delay estimation for mac.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107642
Patch Set 1 : '' #
Total comments: 50
Patch Set 2 : changed the code based on comments && fixed unittest #Patch Set 3 : increase the delay precision #
Total comments: 9
Patch Set 4 : update #
Total comments: 14
Patch Set 5 : remove the deprecated AudioHardwareGetProperty and update comments #
Total comments: 32
Patch Set 6 : update #
Total comments: 4
Patch Set 7 : change the code based on comment from Chris #Patch Set 8 : replace deprecated AudioDevice* with AudioObject* & round the latency before converting to bytes #
Messages
Total messages: 30 (0 generated)
Hello, This patch adds the delay estimation to low-latency input/output audio on Mac. Could you please review it? Thanks, /SX
Did initial review of the input part. Main comments applies to output side as well. Please fix both sides and I'll review again. Looks good so far. Thanks. http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... File media/audio/mac/audio_low_latency_input_mac.cc (right): http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.cc:142: UInt32 size = sizeof(input_device_id_); I would use a local here and only assign if we are all OK. http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.cc:218: // Update the capture device hardware latency. Can be a static method instead. http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.cc:306: UpdateCaptureLatency(time_stamp); Have you checked if we need to call it each callback? http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.cc:310: uint32 capture_delay_bytes = (1.0e-3 * capture_latency_ms_ * I would divide by 1000, and clean up casting here. http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.cc:364: Float64 audio_unit_latency_s = 0; sec and 0.0. http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.cc:366: OSStatus result = AudioUnitGetProperty( Use same style as in rest of the code. http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.cc:371: audio_unit_latency_s = 0; not needed http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.cc:377: result = AudioDeviceGetProperty(input_device_id_, 0, true, ditto http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.cc:382: device_latency_frames = 0; ditto http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.cc:388: result = AudioDeviceGetPropertyInfo(input_device_id_, 0, true, ditto http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.cc:394: result = AudioDeviceGetProperty( ditto http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.cc:399: return; Wrong! http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.cc:403: stream_ids[0], 0, kAudioStreamPropertyLatency, ditto http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.cc:413: hardware_latency_ms_ = static_cast<uint32>(1.0e3 * audio_unit_latency_s + Are you sure? http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.cc:420: // Get the delay between now and when the data was hitting the hardware. aooch ;-) http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.cc:424: uint32 delay_ms = static_cast<uint32>(1e-6 * (now_ns - input_time_ns) + 0.5); Are you sure? http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.cc:426: capture_latency_ms_ = delay_ms + hardware_latency_ms_; I would add a DCHECK here just in case (santity?) http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... File media/audio/mac/audio_low_latency_input_mac.h (right): http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.h:30: // - The latency consists of two parts: 1, hardware latency, which includes Can be cleaned up. Thanks. http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.h:74: const AudioTimeStamp* output_time_stamp, remove output http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.h:83: // Get capture hardware latency, this is called only when initiating "Gets the fixed capture hardware latency and stores it during initialization". http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.h:84: // the input Audio Unit. I would remove 'input' and call the method StoreHardwareLatency() instead. http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.h:87: // Update capture delay value. Updates the current capture delay value. http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.h:111: // The UID refers to the input audio device. Which device? http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.h:124: // Capture hardware latency in millisecond, the value is updated when Remove last part (, the value). Add 'fixed'. http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.h:128: // Capture latency in millisecond. Current ..
Changed the code based on the comments from Henrik, and fixed the unittests. http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... File media/audio/mac/audio_low_latency_input_mac.cc (right): http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.cc:142: UInt32 size = sizeof(input_device_id_); On 2011/10/12 12:10:11, henrika1 wrote: > I would use a local here and only assign if we are all OK. Done. http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.cc:218: // Update the capture device hardware latency. On 2011/10/12 12:10:11, henrika1 wrote: > Can be a static method instead. Can't make it since the function needs some member variables. Hope it is fine. http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.cc:306: UpdateCaptureLatency(time_stamp); The value should be quite stable in the normal cases, but it'd better the check it for each callback since we never know if the delay is pending up, for example, soundcard can become less responsive for some reasons. And calling the function does not cost much since it only does some calculation. On 2011/10/12 12:10:11, henrika1 wrote: > Have you checked if we need to call it each callback? http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.cc:310: uint32 capture_delay_bytes = (1.0e-3 * capture_latency_ms_ * On 2011/10/12 12:10:11, henrika1 wrote: > I would divide by 1000, and clean up casting here. Done. http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.cc:364: Float64 audio_unit_latency_s = 0; On 2011/10/12 12:10:11, henrika1 wrote: > sec and 0.0. Done. http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.cc:366: OSStatus result = AudioUnitGetProperty( On 2011/10/12 12:10:11, henrika1 wrote: > Use same style as in rest of the code. Done. http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.cc:371: audio_unit_latency_s = 0; On 2011/10/12 12:10:11, henrika1 wrote: > not needed Done. http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.cc:377: result = AudioDeviceGetProperty(input_device_id_, 0, true, On 2011/10/12 12:10:11, henrika1 wrote: > ditto Done. http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.cc:382: device_latency_frames = 0; On 2011/10/12 12:10:11, henrika1 wrote: > ditto Done. http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.cc:388: result = AudioDeviceGetPropertyInfo(input_device_id_, 0, true, On 2011/10/12 12:10:11, henrika1 wrote: > ditto Done. http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.cc:394: result = AudioDeviceGetProperty( On 2011/10/12 12:10:11, henrika1 wrote: > ditto Done. http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.cc:399: return; On 2011/10/12 12:10:11, henrika1 wrote: > Wrong! Done. http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.cc:403: stream_ids[0], 0, kAudioStreamPropertyLatency, On 2011/10/12 12:10:11, henrika1 wrote: > ditto Done. http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.cc:413: hardware_latency_ms_ = static_cast<uint32>(1.0e3 * audio_unit_latency_s + On 2011/10/12 12:10:11, henrika1 wrote: > Are you sure? Done. http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.cc:420: // Get the delay between now and when the data was hitting the hardware. On 2011/10/12 12:10:11, henrika1 wrote: > aooch ;-) Done. http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.cc:424: uint32 delay_ms = static_cast<uint32>(1e-6 * (now_ns - input_time_ns) + 0.5); On 2011/10/12 12:10:11, henrika1 wrote: > Are you sure? I would like to keep it, since it is a more readable comparing with /1000000 http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.cc:426: capture_latency_ms_ = delay_ms + hardware_latency_ms_; On 2011/10/12 12:10:11, henrika1 wrote: > I would add a DCHECK here just in case (santity?) I need to think a bit more here. For other platforms like linux, the delay value can be quite high in some special scenarios, like switching devices or when the CPU spikes. but it will recover soon after the problems are gone. I am not sure if it will happen on Mac. If it does, having a DCHECK here may break the call. http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... File media/audio/mac/audio_low_latency_input_mac.h (right): http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.h:30: // - The latency consists of two parts: 1, hardware latency, which includes On 2011/10/12 12:10:11, henrika1 wrote: > Can be cleaned up. Thanks. Done. http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.h:74: const AudioTimeStamp* output_time_stamp, On 2011/10/12 12:10:11, henrika1 wrote: > remove output Done. http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.h:83: // Get capture hardware latency, this is called only when initiating On 2011/10/12 12:10:11, henrika1 wrote: > "Gets the fixed capture hardware latency and stores it during initialization". Done. http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.h:84: // the input Audio Unit. On 2011/10/12 12:10:11, henrika1 wrote: > I would remove 'input' and call the method StoreHardwareLatency() instead. Done. http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.h:87: // Update capture delay value. On 2011/10/12 12:10:11, henrika1 wrote: > Updates the current capture delay value. Done. http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.h:111: // The UID refers to the input audio device. On 2011/10/12 12:10:11, henrika1 wrote: > Which device? Done. http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.h:124: // Capture hardware latency in millisecond, the value is updated when On 2011/10/12 12:10:11, henrika1 wrote: > Remove last part (, the value). Add 'fixed'. Done. http://codereview.chromium.org/8234009/diff/8001/media/audio/mac/audio_low_la... media/audio/mac/audio_low_latency_input_mac.h:128: // Capture latency in millisecond. On 2011/10/12 12:10:11, henrika1 wrote: > Current .. Done.
Hi Henrik, I haven't had a chance to go through in detail, but since the CoreAudio API returns more precision, could you please store the latency (in ms) as a float instead of integer value. I realize that WebRTC does not require this high of accuracy, but other clients could benefit from it. And it does not appear to be more difficult to maintain the precision.
Chris, it sounds like a good idea to maintain a higher precision all the way up to the actual "byte callback". I'm sure that Shijing will be able to make these modifications.
Done with increasing the precision of the delay value.
http://codereview.chromium.org/8234009/diff/13005/media/audio/mac/audio_low_l... File media/audio/mac/audio_low_latency_input_mac.cc (right): http://codereview.chromium.org/8234009/diff/13005/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:223: StoreHardwareLatency(); functions that have side effects (i.e., setting the value of hardware_latency_frames_) this can be unintuitive instead we could either: 1) inline the function here along with all the other AudioUnit{Get/Set}Property functions 2) change the function to return the value and we can assign it here I also noticed that the code here calls HandleError() and returns false for Open() during an error where your code only logs the error. Do we consider it a failure to Open() the device if we can't get the hardware latency? http://codereview.chromium.org/8234009/diff/13005/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:310: UpdateCaptureLatency(time_stamp); functions that have side effects like this can be confusing it looks like you don't need capture_latency_frames_ anywhere outside this function. What if we changed UpdateCaptureLatency() to GetCaptureLatency() so it didn't have to store the capture_latency_frames_ value? http://codereview.chromium.org/8234009/diff/13005/media/audio/mac/audio_low_l... File media/audio/mac/audio_low_latency_input_mac.h (right): http://codereview.chromium.org/8234009/diff/13005/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.h:125: double hardware_latency_frames_; frames are typically integers -- any reason why we're using double? http://codereview.chromium.org/8234009/diff/13005/media/audio/mac/audio_low_l... File media/audio/mac/audio_low_latency_output_mac.cc (right): http://codereview.chromium.org/8234009/diff/13005/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_output_mac.cc:112: StoreHardwareLatency(); ditto http://codereview.chromium.org/8234009/diff/13005/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_output_mac.cc:207: UpdatePlayoutLatency(output_time_stamp); ditto
Please take a new round. http://codereview.chromium.org/8234009/diff/13005/media/audio/mac/audio_low_l... File media/audio/mac/audio_low_latency_input_mac.cc (right): http://codereview.chromium.org/8234009/diff/13005/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:223: StoreHardwareLatency(); On 2011/10/19 16:35:15, scherkus wrote: > functions that have side effects (i.e., setting the value of > hardware_latency_frames_) this can be unintuitive > > instead we could either: > 1) inline the function here along with all the other > AudioUnit{Get/Set}Property functions > 2) change the function to return the value and we can assign it here > > > I also noticed that the code here calls HandleError() and returns false for > Open() during an error where your code only logs the error. Do we consider it a > failure to Open() the device if we can't get the hardware latency? I took the 2nd solution. It is more like a warning if we fail to get the hardware latency, it is not critical. The hardware latency only plays a small part on the overall latency, the value is usually 1-2ms, but it may differ from device to device. http://codereview.chromium.org/8234009/diff/13005/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:310: UpdateCaptureLatency(time_stamp); On 2011/10/19 16:35:15, scherkus wrote: > functions that have side effects like this can be confusing > > it looks like you don't need capture_latency_frames_ anywhere outside this > function. What if we changed UpdateCaptureLatency() to GetCaptureLatency() so > it didn't have to store the capture_latency_frames_ value? Good idea, done. http://codereview.chromium.org/8234009/diff/13005/media/audio/mac/audio_low_l... File media/audio/mac/audio_low_latency_output_mac.cc (right): http://codereview.chromium.org/8234009/diff/13005/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_output_mac.cc:112: StoreHardwareLatency(); On 2011/10/19 16:35:15, scherkus wrote: > ditto Done. http://codereview.chromium.org/8234009/diff/13005/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_output_mac.cc:207: UpdatePlayoutLatency(output_time_stamp); On 2011/10/19 16:35:15, scherkus wrote: > ditto Done.
http://codereview.chromium.org/8234009/diff/24001/media/audio/mac/audio_low_l... File media/audio/mac/audio_low_latency_input_mac.cc (right): http://codereview.chromium.org/8234009/diff/24001/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:221: // Gets the capture device hardware latency and stores the value. comment isn't needed http://codereview.chromium.org/8234009/diff/24001/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:375: if (result) nit: add {} when we have a macro as the only statement also why not return 0 in error cases? http://codereview.chromium.org/8234009/diff/24001/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:386: if (result) ditto + for rest of result checks http://codereview.chromium.org/8234009/diff/24001/media/audio/mac/audio_low_l... File media/audio/mac/audio_low_latency_input_mac.h (right): http://codereview.chromium.org/8234009/diff/24001/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.h:84: // Gets the fixed capture hardware latency and store it during initialization. would add "returns 0 if not available" http://codereview.chromium.org/8234009/diff/24001/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.h:125: double hardware_latency_frames_; still curious about using double for frames instead of int or int64 http://codereview.chromium.org/8234009/diff/24001/media/audio/mac/audio_low_l... File media/audio/mac/audio_low_latency_output_mac.cc (right): http://codereview.chromium.org/8234009/diff/24001/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_output_mac.cc:297: if (result) ditto on using {} and returning 0 http://codereview.chromium.org/8234009/diff/24001/media/audio/mac/audio_low_l... File media/audio/mac/audio_low_latency_output_mac.h (right): http://codereview.chromium.org/8234009/diff/24001/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_output_mac.h:67: double GetHardwareLatency(); ditto on returning zero
http://codereview.chromium.org/8234009/diff/24001/media/audio/mac/audio_low_l... File media/audio/mac/audio_low_latency_input_mac.cc (right): http://codereview.chromium.org/8234009/diff/24001/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:221: // Gets the capture device hardware latency and stores the value. On 2011/10/19 18:23:57, scherkus wrote: > comment isn't needed Done. http://codereview.chromium.org/8234009/diff/24001/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:375: if (result) On 2011/10/19 18:23:57, scherkus wrote: > nit: add {} when we have a macro as the only statement > Done. > also why not return 0 in error cases? There are totally 3 different latencies we are trying to get here: 1, audio unit latency; 2, audio device latency; 3, audio stream latency. They are quite independent to each other. So failing in one does not mean we will fail getting the rest. http://codereview.chromium.org/8234009/diff/24001/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:386: if (result) On 2011/10/19 18:23:57, scherkus wrote: > ditto + for rest of result checks Done. http://codereview.chromium.org/8234009/diff/24001/media/audio/mac/audio_low_l... File media/audio/mac/audio_low_latency_input_mac.h (right): http://codereview.chromium.org/8234009/diff/24001/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.h:84: // Gets the fixed capture hardware latency and store it during initialization. On 2011/10/19 18:23:57, scherkus wrote: > would add "returns 0 if not available" Done. http://codereview.chromium.org/8234009/diff/24001/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.h:125: double hardware_latency_frames_; On 2011/10/19 18:23:57, scherkus wrote: > still curious about using double for frames instead of int or int64 Because frame is the general unit for the audio IO implementations, it is very easy to be converted into bytes or ms, so I decided to use it as the unit for the latency as well. Hope it is fine here. And on Mac, the latency values include a float value in second, uint64 value in ns, and I need to convert those values to frames. In order to keep the precision, I need to use at least float here. Then I just chose double. If we want to use int64 as the type, we may need to change the unit to be ns, for example. It is more readable but a bit more difficult on using it. http://codereview.chromium.org/8234009/diff/24001/media/audio/mac/audio_low_l... File media/audio/mac/audio_low_latency_output_mac.cc (right): http://codereview.chromium.org/8234009/diff/24001/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_output_mac.cc:297: if (result) On 2011/10/19 18:23:57, scherkus wrote: > ditto on using {} and returning 0 Done. http://codereview.chromium.org/8234009/diff/24001/media/audio/mac/audio_low_l... File media/audio/mac/audio_low_latency_output_mac.h (right): http://codereview.chromium.org/8234009/diff/24001/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_output_mac.h:67: double GetHardwareLatency(); On 2011/10/19 18:23:57, scherkus wrote: > ditto on returning zero Done.
LGTM w/ nits thanks for the explanations! http://codereview.chromium.org/8234009/diff/19009/media/audio/mac/audio_low_l... File media/audio/mac/audio_low_latency_input_mac.cc (right): http://codereview.chromium.org/8234009/diff/19009/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:426: // Logs the warning if it fails to get the stream latency. nit: comment adds no value -- remove http://codereview.chromium.org/8234009/diff/19009/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:431: // Store the hardware latency value in frames. ditto http://codereview.chromium.org/8234009/diff/19009/media/audio/mac/audio_low_l... File media/audio/mac/audio_low_latency_output_mac.cc (right): http://codereview.chromium.org/8234009/diff/19009/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_output_mac.cc:349: // Log the warning if it fails to get stream latency. ditto http://codereview.chromium.org/8234009/diff/19009/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_output_mac.cc:354: // Returns the hardware latency value in freams. ditto
LGTM with nits. http://codereview.chromium.org/8234009/diff/19009/media/audio/mac/audio_low_l... File media/audio/mac/audio_low_latency_input_mac.cc (right): http://codereview.chromium.org/8234009/diff/19009/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:141: // First, obtain the current input device selected by the user. May I ask why you decided to use another API here? http://codereview.chromium.org/8234009/diff/19009/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:160: // Store the input device id. Don't think this comment adds much. http://codereview.chromium.org/8234009/diff/19009/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:228: hardware_latency_frames_ = GetHardwareLatency(); But a comment here could emphasize that parts of the delay estimate is fixed and can be stored here. http://codereview.chromium.org/8234009/diff/19009/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:372: if (!audio_unit_ || input_device_id_ == kAudioObjectUnknown) Is this a valid state? Guess it is since this part will be very low. Perhaps a DLOG()? http://codereview.chromium.org/8234009/diff/19009/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:385: DLOG(WARNING) << "GetHardwareLatency: Could not get audio unit latency."; Don't need GetHardwareLatency:, the log message is rich as is. Function name is added to output log. Also, DLOG_IF(WARNING, result != noErr) << http://codereview.chromium.org/8234009/diff/19009/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:397: DLOG(WARNING) << "GetHardwareLatency: Could not get device latency."; ditto http://codereview.chromium.org/8234009/diff/19009/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:432: return static_cast<double>(audio_unit_latency_sec * I would write it as ((x*a) + b + c) http://codereview.chromium.org/8234009/diff/19009/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:438: // Get the delay between now and when the data was reaching the hardware. This comment is not clear. See my proposal in the header file. Basically, you are measuring the time difference from the timestamp of the first recorded frame in a packet and the timestamp of the callback. http://codereview.chromium.org/8234009/diff/19009/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:439: UInt64 input_time_ns = AudioConvertHostTimeToNanos( capture_time_ns http://codereview.chromium.org/8234009/diff/19009/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:445: return (delay_frames + hardware_latency_frames_); Comment about sum of fixed parts and dynamic part perhaps. http://codereview.chromium.org/8234009/diff/19009/media/audio/mac/audio_low_l... File media/audio/mac/audio_low_latency_input_mac.h (right): http://codereview.chromium.org/8234009/diff/19009/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.h:31: // 1, Hardware latency, which includes Audio Unit latency, audio device I would use 1) and 2) http://codereview.chromium.org/8234009/diff/19009/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.h:33: // 2, The delay between now and the scheduled time stamp that tells when the Please rewrite this comment from scratch since it is not clear. I would say something like: "The time difference between the actual recording instant and the time where the data packet is provided to us as a callback".
Done with the comments from Andrew and Henrik. Chris, any input from you? Thanks, BR, -SX http://codereview.chromium.org/8234009/diff/19009/media/audio/mac/audio_low_l... File media/audio/mac/audio_low_latency_input_mac.cc (right): http://codereview.chromium.org/8234009/diff/19009/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:141: // First, obtain the current input device selected by the user. On 2011/10/21 07:27:23, henrika1 wrote: > May I ask why you decided to use another API here? Andrew pointed out that the AudioHardwareGetProperty is deprecated in deprecated in 10.6 in favour of AudioObjectGetPropertyData(). See CL: http://codereview.chromium.org/210009 http://codereview.chromium.org/8234009/diff/19009/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:160: // Store the input device id. On 2011/10/21 07:27:23, henrika1 wrote: > Don't think this comment adds much. Done. http://codereview.chromium.org/8234009/diff/19009/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:228: hardware_latency_frames_ = GetHardwareLatency(); On 2011/10/21 07:27:23, henrika1 wrote: > But a comment here could emphasize that parts of the delay estimate is fixed and > can be stored here. Done. http://codereview.chromium.org/8234009/diff/19009/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:372: if (!audio_unit_ || input_device_id_ == kAudioObjectUnknown) On 2011/10/21 07:27:23, henrika1 wrote: > Is this a valid state? Guess it is since this part will be very low. Perhaps a > DLOG()? It is safer to add a check here, since this function is a member to the class. Done. http://codereview.chromium.org/8234009/diff/19009/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:385: DLOG(WARNING) << "GetHardwareLatency: Could not get audio unit latency."; On 2011/10/21 07:27:23, henrika1 wrote: > Don't need GetHardwareLatency:, the log message is rich as is. Function name is > added to output log. > > Also, DLOG_IF(WARNING, result != noErr) << Done. http://codereview.chromium.org/8234009/diff/19009/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:397: DLOG(WARNING) << "GetHardwareLatency: Could not get device latency."; On 2011/10/21 07:27:23, henrika1 wrote: > ditto Done. http://codereview.chromium.org/8234009/diff/19009/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:426: // Logs the warning if it fails to get the stream latency. On 2011/10/21 00:20:58, scherkus wrote: > nit: comment adds no value -- remove Done. http://codereview.chromium.org/8234009/diff/19009/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:431: // Store the hardware latency value in frames. On 2011/10/21 00:20:58, scherkus wrote: > ditto Done. http://codereview.chromium.org/8234009/diff/19009/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:432: return static_cast<double>(audio_unit_latency_sec * On 2011/10/21 07:27:23, henrika1 wrote: > I would write it as ((x*a) + b + c) Done. http://codereview.chromium.org/8234009/diff/19009/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:438: // Get the delay between now and when the data was reaching the hardware. On 2011/10/21 07:27:23, henrika1 wrote: > This comment is not clear. See my proposal in the header file. Basically, you > are measuring the time difference from the timestamp of the first recorded frame > in a packet and the timestamp of the callback. Done. http://codereview.chromium.org/8234009/diff/19009/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:439: UInt64 input_time_ns = AudioConvertHostTimeToNanos( On 2011/10/21 07:27:23, henrika1 wrote: > capture_time_ns Done. http://codereview.chromium.org/8234009/diff/19009/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:445: return (delay_frames + hardware_latency_frames_); On 2011/10/21 07:27:23, henrika1 wrote: > Comment about sum of fixed parts and dynamic part perhaps. Done. http://codereview.chromium.org/8234009/diff/19009/media/audio/mac/audio_low_l... File media/audio/mac/audio_low_latency_input_mac.h (right): http://codereview.chromium.org/8234009/diff/19009/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.h:31: // 1, Hardware latency, which includes Audio Unit latency, audio device On 2011/10/21 07:27:23, henrika1 wrote: > I would use 1) and 2) Done. http://codereview.chromium.org/8234009/diff/19009/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.h:33: // 2, The delay between now and the scheduled time stamp that tells when the On 2011/10/21 07:27:23, henrika1 wrote: > Please rewrite this comment from scratch since it is not clear. I would say > something like: > > "The time difference between the actual recording instant and the time where the > data packet is provided to us as a callback". Done. http://codereview.chromium.org/8234009/diff/19009/media/audio/mac/audio_low_l... File media/audio/mac/audio_low_latency_output_mac.cc (right): http://codereview.chromium.org/8234009/diff/19009/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_output_mac.cc:349: // Log the warning if it fails to get stream latency. On 2011/10/21 00:20:58, scherkus wrote: > ditto Done. http://codereview.chromium.org/8234009/diff/19009/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_output_mac.cc:354: // Returns the hardware latency value in freams. On 2011/10/21 00:20:58, scherkus wrote: > ditto Done.
http://codereview.chromium.org/8234009/diff/29001/media/audio/mac/audio_low_l... File media/audio/mac/audio_low_latency_input_mac.cc (right): http://codereview.chromium.org/8234009/diff/29001/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:320: (capture_latency_frames * format_.mBytesPerFrame + 0.5); Don't we want to convert "capture_latency_frames" to an integer *before* converting to bytes so that the number of bytes is an even multiple of mBytesPerFrame? Something like this? uint32 capture_delay_bytes = static_cast<uint32> (capture_latency_frames + 0.5) * format_.mBytesPerFrame; http://codereview.chromium.org/8234009/diff/29001/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:433: // when the data packet is provided as a callback nit: period at end of sentence
http://codereview.chromium.org/8234009/diff/29001/media/audio/mac/audio_low_l... File media/audio/mac/audio_low_latency_input_mac.cc (right): http://codereview.chromium.org/8234009/diff/29001/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:320: (capture_latency_frames * format_.mBytesPerFrame + 0.5); On 2011/10/26 19:03:53, Chris Rogers wrote: > Don't we want to convert "capture_latency_frames" to an integer *before* > converting to bytes so that the number of bytes is an even multiple of > mBytesPerFrame? > > Something like this? > > uint32 capture_delay_bytes = static_cast<uint32> > (capture_latency_frames + 0.5) * format_.mBytesPerFrame; Good question. I did the rounding after the conversion because we are telling the clients on how many bytes of data are pending in the soundcard. For example, we have 0.7 frames latency, 2 bytes per frames, here are are telling the client that we have 1 bytes pending. But if we have 0.8 frames latency, then it will become 2 bytes. So the precision is a bit higher here. In WebRTC we always round it after the conversion, but I believe it is just a legacy. Our AEC is designed to handle some tolerance. Let me know if you think we should do it in the other way. http://codereview.chromium.org/8234009/diff/29001/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:433: // when the data packet is provided as a callback On 2011/10/26 19:03:53, Chris Rogers wrote: > nit: period at end of sentence Done.
On 2011/10/26 22:07:23, xians1 wrote: > http://codereview.chromium.org/8234009/diff/29001/media/audio/mac/audio_low_l... > File media/audio/mac/audio_low_latency_input_mac.cc (right): > > http://codereview.chromium.org/8234009/diff/29001/media/audio/mac/audio_low_l... > media/audio/mac/audio_low_latency_input_mac.cc:320: (capture_latency_frames * > format_.mBytesPerFrame + 0.5); > On 2011/10/26 19:03:53, Chris Rogers wrote: > > Don't we want to convert "capture_latency_frames" to an integer *before* > > converting to bytes so that the number of bytes is an even multiple of > > mBytesPerFrame? > > > > Something like this? > > > > uint32 capture_delay_bytes = static_cast<uint32> > > (capture_latency_frames + 0.5) * format_.mBytesPerFrame; > > Good question. I did the rounding after the conversion because we are telling > the clients on how many bytes of data are pending in the soundcard. > For example, we have 0.7 frames latency, 2 bytes per frames, here are are > telling the client that we have 1 bytes pending. But if we have 0.8 frames > latency, then it will become 2 bytes. So the precision is a bit higher here. > In WebRTC we always round it after the conversion, but I believe it is just a > legacy. Our AEC is designed to handle some tolerance. > Let me know if you think we should do it in the other way. This isn't your fault, but I think it's kind of strange that we're even dealing in "byte" quantities at all in the OnData() and OnMoreData() methods. This is just the way the API was created a while back. A more natural unit would be "sample-frame" since this is the smallest unit which makes sense when talking with the audio hardware (unless we're writing encoded audio - which we're not). Actually, looking closer I see we deal in byte quantities in two places in OnData(): void OnData(AudioInputStream* stream, const uint8* src, uint32 size, uint32 hardware_delay_bytes); in "size" and "hardware_delay_bytes" It seems like "size" should be in units of sample-frames and "hardware_delay_bytes" should be called "hardware_delay_seconds" and be a double-precision number in units of seconds. Then we wouldn't have to worry about all of this :) Longer term, it would be good to clean some of these interfaces up. But, given what we have now it *does* seem odd to pass a number of bytes which represents a fractional part of a sample, since if you're dealing with PCM data on a byte-level, it will always be a multiple of the bytes-per-sample-frame.
On Wed, Oct 26, 2011 at 3:30 PM, <crogers@google.com> wrote: > On 2011/10/26 22:07:23, xians1 wrote: > > http://codereview.chromium.**org/8234009/diff/29001/media/** > audio/mac/audio_low_latency_**input_mac.cc<http://codereview.chromium.org/8234009/diff/29001/media/audio/mac/audio_low_latency_input_mac.cc> > >> File media/audio/mac/audio_low_**latency_input_mac.cc (right): >> > > > http://codereview.chromium.**org/8234009/diff/29001/media/** > audio/mac/audio_low_latency_**input_mac.cc#newcode320<http://codereview.chromium.org/8234009/diff/29001/media/audio/mac/audio_low_latency_input_mac.cc#newcode320> > >> media/audio/mac/audio_low_**latency_input_mac.cc:320: >> (capture_latency_frames * >> format_.mBytesPerFrame + 0.5); >> On 2011/10/26 19:03:53, Chris Rogers wrote: >> > Don't we want to convert "capture_latency_frames" to an integer *before* >> > converting to bytes so that the number of bytes is an even multiple of >> > mBytesPerFrame? >> > >> > Something like this? >> > >> > uint32 capture_delay_bytes = static_cast<uint32> >> > (capture_latency_frames + 0.5) * format_.mBytesPerFrame; >> > > Good question. I did the rounding after the conversion because we are >> telling >> the clients on how many bytes of data are pending in the soundcard. >> For example, we have 0.7 frames latency, 2 bytes per frames, here are are >> telling the client that we have 1 bytes pending. But if we have 0.8 frames >> latency, then it will become 2 bytes. So the precision is a bit higher >> here. >> In WebRTC we always round it after the conversion, but I believe it is >> just a >> legacy. Our AEC is designed to handle some tolerance. >> Let me know if you think we should do it in the other way. >> > > This isn't your fault, but I think it's kind of strange that we're even > dealing > in "byte" quantities at all in the OnData() and OnMoreData() methods. This > is > just the way the API was created a while back. A more natural unit would > be > "sample-frame" since this is the smallest unit which makes sense when > talking > with the audio hardware (unless we're writing encoded audio - which we're > not). > > Actually, looking closer I see we deal in byte quantities in two places in > OnData(): > > void OnData(AudioInputStream* stream, const uint8* src, > uint32 size, uint32 hardware_delay_bytes); > > in "size" and "hardware_delay_bytes" > > It seems like "size" should be in units of sample-frames and > "hardware_delay_bytes" should be called "hardware_delay_seconds" and be a > double-precision number in units of seconds. > > Then we wouldn't have to worry about all of this :) > > I can't agree more. In WebRTC we always deal the audio in units of frames and delay in the units of ms. > Longer term, it would be good to clean some of these interfaces up. But, > given > what we have now it *does* seem odd to pass a number of bytes which > represents a > fractional part of a sample, since if you're dealing with PCM data on a > byte-level, it will always be a multiple of the bytes-per-sample-frame. > > I think either way works for WebRTC, so I will change it to round the value before converting to bytes. > http://codereview.chromium.**org/8234009/<http://codereview.chromium.org/8234... >
Rounded the latency in units of frames before converting to bytes. Replaced all the deprecated AudioDevice* API with AudioObject* API.
Chris, you stated earlier that "could you please store the latency (in ms) as a float instead of integer value", and now you propose a lower resolution in the actual callback. May I ask why?
On 2011/10/27 06:48:15, henrika1 wrote: > Chris, > > you stated earlier that "could you please store the latency (in ms) > as a float instead of integer value", and now you propose a lower resolution in > the actual callback. May I ask why? Hi Henrik, if I get it right, Chris' idea is to deliver the latency in units of frames, so the rounding should happen before converting to bytes, in favor of using "sample-frame" as unit for general audio stuff. Chris, correct me if I am wrong here.
One millisecond is around 44 sample-frames @44.1KHz, so dealing with ms is only accurate to a chunk of 44 sample-frames. I'm talking about rounding to the nearest sample-frame, which isn't as course. As I mentioned in my last comment, I think it would be best if OnData() used a double representing seconds (or ms) for the delay, then we could be as accurate as we like. I think it's somewhat strange that we should be dealing in bytes at all. On 2011/10/27 06:48:15, henrika1 wrote: > Chris, > > you stated earlier that "could you please store the latency (in ms) > as a float instead of integer value", and now you propose a lower resolution in > the actual callback. May I ask why?
Shijing, yes that's right. On 2011/10/27 17:48:31, xians1 wrote: > On 2011/10/27 06:48:15, henrika1 wrote: > > Chris, > > > > you stated earlier that "could you please store the latency (in ms) > > as a float instead of integer value", and now you propose a lower resolution > in > > the actual callback. May I ask why? > > Hi Henrik, if I get it right, Chris' idea is to deliver the latency in units of > frames, so the rounding should happen before converting to bytes, in favor of > using "sample-frame" as unit for general audio stuff. > > Chris, correct me if I am wrong here.
I am going to commit the CL if no more input from you guys. Thanks, /SX
Did you change the rounding? It didn't seem like the change has been made in the latest upload. On 2011/10/27 18:36:19, xians1 wrote: > I am going to commit the CL if no more input from you guys. > > Thanks, > /SX
On 2011/10/27 18:38:40, Chris Rogers wrote: > Did you change the rounding? It didn't seem like the change has been made in > the latest upload. > Yes, in the latest patch set 8, it has been changed for both the input and output. BR, /SX
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/8234009/36004
Change committed as 107642
Shijing, I don't know the full story of why we use bytes as unit for the delay estimate; guess it's related to the existing status on the output side. In any case, using milliseconds feels more natural and it is most likely an operation which the client must perform anyhow. Could you investigate the amount of changes needed to go for ms instead? I am personally fine with an integer representation.
Henrik, do you have any particular argument for providing considerably less precision than the OS-specific APIs provide? As I mentioned before, 1ms is a pretty course granularity. It may be fine for WebRTC, but in the future there may be other clients for this information. There's no reason to artificially quantize it when a higher precision value is available. On 2011/10/28 07:09:42, henrika1 wrote: > Shijing, > > I don't know the full story of why we use bytes as unit for the delay estimate; > guess it's related to the existing status on the output side. > > In any case, using milliseconds feels more natural and it is most likely an > operation which the client must perform anyhow. > > Could you investigate the amount of changes needed to go for ms instead? I am > personally fine with an integer representation.
Let's finalize this discussion off-line. /Henrik On Fri, Oct 28, 2011 at 7:35 PM, <crogers@google.com> wrote: > Henrik, do you have any particular argument for providing considerably less > precision than the OS-specific APIs provide? As I mentioned before, 1ms > is a > pretty course granularity. It may be fine for WebRTC, but in the future > there > may be other clients for this information. There's no reason to > artificially > quantize it when a higher precision value is available. > > > On 2011/10/28 07:09:42, henrika1 wrote: > >> Shijing, >> > > I don't know the full story of why we use bytes as unit for the delay >> > estimate; > >> guess it's related to the existing status on the output side. >> > > In any case, using milliseconds feels more natural and it is most likely >> an >> operation which the client must perform anyhow. >> > > Could you investigate the amount of changes needed to go for ms instead? >> I am >> personally fine with an integer representation. >> > > > > http://codereview.chromium.**org/8234009/<http://codereview.chromium.org/8234... > -- Henrik Andreasson | Software Engineer | henrika@google.com | +46 70 376 91 74 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
