Improve error logging for AUAudioInputStream [chromium/src : main]
Olga Sharonova (Gerrit)
Olga Sharonova added 10 comments![]( "Open in Gerrit")
const char* func_name,We don't really need a function name: we have a unique error message which points to the exact place in the source code.
"Device ID is unknown; Returning OpenOutcome::kFailed");not needed: we have the source code we can look at.
base::StringPrintf("%s: [this=%p] Stop() called", __FUNCTION__, this));We know it's Stop(): we have FUNCTION
base::StringPrintf("%s: [this=%p] Close() called", __FUNCTION__, this));Same - could you ensure throughout that we are not unnecessarily verbose?
std::string error_string = base::StringPrintf(
"%s: [this=%p] voice processing disabled", __FUNCTION__, this);
DVLOG(1) << error_string;
log_callback_.Run(error_string);not needed, it will be noise/spam
std::string error_string = base::StringPrintf(
"%s: [this=%p] audio_device_id is same as output_device_id_for_aec_",
__FUNCTION__, this);
log_callback_.Run(error_string);
DVLOG(1) << error_string;not needed
std::string error_string = "AudioUnitRender() failed ";We don't want to use std::string here: it uses heap to store the string, we end up with dynamic memory allocation. And this code runs on a real-time trhead.
error_string += base::StringPrintf("Too long sequence of %s errors!", err);I don't see it being useful for the log.
std::string error_string =here and in other places - can we just use const char[] where it's a constant string?
log_callback.Run(base::StrCat({__func__, ": ", error_string}));AudioManager should have its own log callback, it may be sufficient to use it. We don't want to overcomplicate APIs for the sake of logging. If we really want to use the input stream logging callback, we can return an error code here.
Related details
- Palak Agarwal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Palak Agarwal (Gerrit)
Palak Agarwal added 11 comments![]( "Open in Gerrit")
We don't really need a function name: we have a unique error message which points to the exact place in the source code.
Done
"Device ID is unknown; Returning OpenOutcome::kFailed");not needed: we have the source code we can look at.
Done
base::StringPrintf("%s: [this=%p] Stop() called", __FUNCTION__, this));We know it's Stop(): we have FUNCTION
Done
base::StringPrintf("%s: [this=%p] Close() called", __FUNCTION__, this));Same - could you ensure throughout that we are not unnecessarily verbose?
Done
std::string error_string = base::StringPrintf(
"%s: [this=%p] voice processing disabled", __FUNCTION__, this);
DVLOG(1) << error_string;
log_callback_.Run(error_string);not needed, it will be noise/spam
Done
std::string error_string = base::StringPrintf(
"%s: [this=%p] audio_device_id is same as output_device_id_for_aec_",
__FUNCTION__, this);
log_callback_.Run(error_string);
DVLOG(1) << error_string;Palak Agarwalnot needed
Done
LogMessageEverywhere(
__FUNCTION__,
base::StringPrintf("Unable to resolve output device id for AEC: %s",
output_device_id));
return;Is this needed? Because LogMessageEverywhere calls log_callback_ inside it.
std::string error_string = "AudioUnitRender() failed ";We don't want to use std::string here: it uses heap to store the string, we end up with dynamic memory allocation. And this code runs on a real-time trhead.
Done
error_string += base::StringPrintf("Too long sequence of %s errors!", err);I don't see it being useful for the log.
Done
here and in other places - can we just use const char[] where it's a constant string?
Done
log_callback.Run(base::StrCat({__func__, ": ", error_string}));AudioManager should have its own log callback, it may be sufficient to use it. We don't want to overcomplicate APIs for the sake of logging. If we really want to use the input stream logging callback, we can return an error code here.
- Olga Sharonova
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Palak Agarwal (Gerrit)
Palak Agarwal voted and added 1 comment![]( "Open in Gerrit")
Votes added by Palak Agarwal
| Commit-Queue | +1 |
1 comment
log_callback_.Run("AU in" + base::StringPrintf(" [this=%p] ", this) +Should we keep this and add it as a prefix in the other logs in this file to identify that the logs are from the input stream.
Related details
- Olga Sharonova
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olga Sharonova (Gerrit)
Olga Sharonova added 4 comments![]( "Open in Gerrit")
std::string FormatOsStatusError(const void* ptr,
const char* message,
OSStatus err) {
return base::StringPrintf("[this=%p] %s (OSStatus error %d: %s)", ptr,
message, static_cast<int>(err),
logging::DescriptionFromOSStatus(err).c_str());
}Keep ducking functions together, move this one a bit?
log_callback_.Run("AU in" + base::StringPrintf(" [this=%p] ", this) +Should we keep this and add it as a prefix in the other logs in this file to identify that the logs are from the input stream.
Yes, most definitely. We need it to be able to filter logs.
return kAudioUnitErr_InvalidElement;Why this error?
const AudioManager::LogCallback& log_callback = GetEnumerationLogCallback();Will this be helpful without knowing which device this is about?
Related details
- Palak Agarwal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Palak Agarwal (Gerrit)
Palak Agarwal voted and added 5 comments![]( "Open in Gerrit")
Votes added by Palak Agarwal
| Commit-Queue | +1 |
5 comments
std::string FormatOsStatusError(const void* ptr,
const char* message,
OSStatus err) {
return base::StringPrintf("[this=%p] %s (OSStatus error %d: %s)", ptr,
message, static_cast<int>(err),
logging::DescriptionFromOSStatus(err).c_str());
}Keep ducking functions together, move this one a bit?
Done, removed this function altogether and moved its functionality in the SendLog function
LogMessageEverywhere(
__FUNCTION__,
base::StringPrintf("Unable to resolve output device id for AEC: %s",
output_device_id));
return;Is this needed? Because LogMessageEverywhere calls log_callback_ inside it.
Acknowledged
log_callback_.Run("AU in" + base::StringPrintf(" [this=%p] ", this) +Olga SharonovaShould we keep this and add it as a prefix in the other logs in this file to identify that the logs are from the input stream.
Yes, most definitely. We need it to be able to filter logs.
Done
return kAudioUnitErr_InvalidElement;Palak AgarwalWhy this error?
Ack but not relevant anymore since i've changed the function signature back to bool
const AudioManager::LogCallback& log_callback = GetEnumerationLogCallback();Will this be helpful without knowing which device this is about?
Since we cant expose device ids, now i'm trying to get device names and use them here. WDYT?
Related details
- Olga Sharonova
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |