Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1613073004:
Changes error handling for AudioUnitRender on Mac OSX (Closed)
|
Created:
4 years, 11 months ago by henrika (OOO until Aug 14) Modified:
4 years, 11 months ago Reviewers:
tommi (sloooow) - chröme CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChanges error handling for AudioUnitRender on Mac OSX
BUG=549021
Committed: https://crrev.com/435741872d39afd2ed06142d4bbdeef2bac6a41e
Cr-Commit-Position: refs/heads/master@{#370681}
Patch Set 1 #
Total comments: 8
Patch Set 2 : nit #Patch Set 3 : Feedback from Tommi #Messages
Total messages: 16 (6 generated)
Description was changed from ========== Changes error handling for AudioUnitRender on Mac OSX BUG= ========== to ========== Changes error handling for AudioUnitRender on Mac OSX BUG=NONE ==========
henrika@chromium.org changed reviewers: + tommi@chromium.org
As discussed.
https://codereview.chromium.org/1613073004/diff/1/media/audio/mac/audio_low_l... File media/audio/mac/audio_low_latency_input_mac.cc (right): https://codereview.chromium.org/1613073004/diff/1/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:609: DLOG(ERROR) << "kAudioUnitErr_CannotDoInCurrentContext"; maybe LOG(ERROR) for now? In case this could be useful for in-the-field troubleshooting. https://codereview.chromium.org/1613073004/diff/1/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:614: DLOG(ERROR) << "kAudioUnitErr_NoConnection"; I think we should treat this one like a general case. If the device isn't connected, it makes sense to report an error and close the stream. Logging the error seems useful though.
https://codereview.chromium.org/1613073004/diff/1/media/audio/mac/audio_low_l... File media/audio/mac/audio_low_latency_input_mac.cc (right): https://codereview.chromium.org/1613073004/diff/1/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:609: DLOG(ERROR) << "kAudioUnitErr_CannotDoInCurrentContext"; On 2016/01/21 12:36:12, tommi-chromium wrote: > maybe LOG(ERROR) for now? In case this could be useful for in-the-field > troubleshooting. what about this comment? https://codereview.chromium.org/1613073004/diff/1/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:614: DLOG(ERROR) << "kAudioUnitErr_NoConnection"; On 2016/01/21 12:36:12, tommi-chromium wrote: > I think we should treat this one like a general case. If the device isn't > connected, it makes sense to report an error and close the stream. Logging the > error seems useful though. LOG(ERROR) could be useful for this error as well (or any other error actually). What about adding a LOG(ERROR) just before we call HandleError()?
https://codereview.chromium.org/1613073004/diff/1/media/audio/mac/audio_low_l... File media/audio/mac/audio_low_latency_input_mac.cc (right): https://codereview.chromium.org/1613073004/diff/1/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:609: DLOG(ERROR) << "kAudioUnitErr_CannotDoInCurrentContext"; Done. Changed some more to LOG as well. To ensure that we can see these logs in Canary as well. https://codereview.chromium.org/1613073004/diff/1/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:614: DLOG(ERROR) << "kAudioUnitErr_NoConnection"; On 2016/01/21 12:36:12, tommi-chromium wrote: > I think we should treat this one like a general case. If the device isn't > connected, it makes sense to report an error and close the stream. Logging the > error seems useful though. Done.
PTAL
lgtm
https://codereview.chromium.org/1613073004/diff/1/media/audio/mac/audio_low_l... File media/audio/mac/audio_low_latency_input_mac.cc (right): https://codereview.chromium.org/1613073004/diff/1/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:609: DLOG(ERROR) << "kAudioUnitErr_CannotDoInCurrentContext"; On 2016/01/21 12:42:42, tommi-chromium wrote: > On 2016/01/21 12:36:12, tommi-chromium wrote: > > maybe LOG(ERROR) for now? In case this could be useful for in-the-field > > troubleshooting. > > what about this comment? Done. https://codereview.chromium.org/1613073004/diff/1/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:614: DLOG(ERROR) << "kAudioUnitErr_NoConnection"; It is covered above by OSSTATUS_DLOG(ERROR, result) << "AudioUnitRender() failed ";
Description was changed from ========== Changes error handling for AudioUnitRender on Mac OSX BUG=NONE ========== to ========== Changes error handling for AudioUnitRender on Mac OSX BUG=549021 ==========
The CQ bit was checked by henrika@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1613073004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1613073004/40001
Message was sent while issue was closed.
Description was changed from ========== Changes error handling for AudioUnitRender on Mac OSX BUG=549021 ========== to ========== Changes error handling for AudioUnitRender on Mac OSX BUG=549021 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Changes error handling for AudioUnitRender on Mac OSX BUG=549021 ========== to ========== Changes error handling for AudioUnitRender on Mac OSX BUG=549021 Committed: https://crrev.com/435741872d39afd2ed06142d4bbdeef2bac6a41e Cr-Commit-Position: refs/heads/master@{#370681} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/435741872d39afd2ed06142d4bbdeef2bac6a41e Cr-Commit-Position: refs/heads/master@{#370681} |
