Introduce CoreAudioUtil class with LogCallback [chromium/src : main]
Olga Sharonova (Gerrit)
Olga Sharonova added 4 comments![]( "Open in Gerrit")
I took a look because was looking for the context for previous CLs; some comments which I think makes sense to share early
std::unique_ptr<core_audio_mac::CoreAudioUtil> core_audio_util_;Everywhere: does not need to be a unique_ptr, just CoreAudioUtilHelper which takes a logging callback in the constructor? (Why do we need a lazy initialization?)
class MEDIA_EXPORT CoreAudioUtil {This does not need to sit in core_audo_util, it's just a helper class.
#include "media/audio/audio_manager.h"Please don't introduce this dependancy. We can just use a generic log callback.
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 3 comments![]( "Open in Gerrit")
std::unique_ptr<core_audio_mac::CoreAudioUtil> core_audio_util_;Everywhere: does not need to be a unique_ptr, just CoreAudioUtilHelper which takes a logging callback in the constructor? (Why do we need a lazy initialization?)
I've removed the lazy construction but still kept the unique_ptr as we can't initialize it in the constructor (several tests crash) as it needs to be initialized on the Audio thread.
This does not need to sit in core_audo_util, it's just a helper class.
Done
Please don't introduce this dependancy. We can just use a generic log callback.
- 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 abandoned this change.
Palak Agarwal abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
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 Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Palak Agarwal (Gerrit)
Palak Agarwal abandoned this change.
Palak Agarwal abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Palak Agarwal (Gerrit)
Palak Agarwal voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Olga Sharonova
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olga Sharonova (Gerrit)
Olga Sharonova added 6 comments![]( "Open in Gerrit")
#include "media/audio/mac/core_audio_util_mac.h"not needed?
// TODO(crbug.com/484894482): Provide non-empty callback to `core_audio_util_`What is the problem with providing the callback right away? If it's complicated or if we want to do it in a follow-up - let's not touch the class at all.
std::unique_ptr<core_audio_mac::CoreAudioUtil> core_audio_util_;forward-declare the class
std::optional<std::string> GetDeviceUniqueID(Why some of them are moved to the class but not the others? That is confusing.
class CoreAudioUtil {Helper. It's not all CoreAudio utils there are.
std::optional<std::string> GetDeviceUniqueID(AudioObjectID device_id,
const LogCallback& log_callback) {
return GetDeviceStringProperty(device_id, kAudioDevicePropertyDeviceUID,
log_callback);
}
I understand that you did it to minimize the diff, but it makes the file a mess: public functions mixed up with class methods.
How about we just ADD CoreAudioUtilHelper class (a helper wrapper than provide logging capabilities, not meant to replace all access to CoreAudio) and KEEP all the public function as they are?
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. |
Olga Sharonova (Gerrit)
Olga Sharonova added 3 comments![]( "Open in Gerrit")
// TODO(crbug.com/484894482): Merge this with CoreAudioUtil::GetDefaultDevice.I don't understand all these TODOs. What is the plan for them?
// GetEnumerationCallback to core_audio_mac::GetDefaultDevice.What's up here?
// TODO(crbug.com/484894482): Add logging here through enumeration callback.Why can't we do it now? Is it complicated?
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. |