InputController initialization refactoring [chromium/src : main]
Pablo Barrera González (Gerrit)
Pablo Barrera González has uploaded the change for review![]( "Open in Gerrit")
Commit message
InputController initialization refactoring
Moving MaybeSetUpAudioProcessing out of the constructor because it might fail.
After this change InputController::Create returns a valid pointer even if
VoiceIsolation can't be created. Any error during creation is communicated
through event_handler_.
Change diff
Change information
- M services/audio/input_controller.cc
- M services/audio/input_controller.h
- M services/audio/input_controller_unittest.cc
Related details
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 3 comments![]( "Open in Gerrit")
media::AecdumpRecordingManager* aecdump_recording_manager,Not needed?
std::make_unique<StatsReporter>(reference_signal_provider, this)) {I'd very much like to avoid passing reference_signal_provider twice in some elegant way, but if it's too much work we can be sloppy here.
event_handler_->OnError(STREAM_CREATE_ERROR);This needs to be moved into DoCreate, to keep the separation of concerns. (so I suggest we return kOk, kVoiceIsolationFailure, kUnknown - or something like that)
Related details
- Pablo Barrera González
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. |
Pablo Barrera González (Gerrit)
Pablo Barrera González added 3 comments![]( "Open in Gerrit")
media::AecdumpRecordingManager* aecdump_recording_manager,Pablo Barrera GonzálezNot needed?
Acknowledged
std::make_unique<StatsReporter>(reference_signal_provider, this)) {I'd very much like to avoid passing reference_signal_provider twice in some elegant way, but if it's too much work we can be sloppy here.
Not clear.
StatsReperter is already used in the constructor, so we need to have it as soon as possible. And I can't now build the StatsReporte in Create() to pass it to the constructor because it does need to know the controller it's connected to.
event_handler_->OnError(STREAM_CREATE_ERROR);This needs to be moved into DoCreate, to keep the separation of concerns. (so I suggest we return kOk, kVoiceIsolationFailure, kUnknown - or something like that)
I can move the code, no problem. I put it here to initialize the VoiceIsolation only once it was clear nothing else was failing, but likely it's not the best place.
I don't understand the part about returning kVoiceIsolationFailure, etc. What is returning these error codes? DoCreate?
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 2 comments![]( "Open in Gerrit")
std::make_unique<StatsReporter>(reference_signal_provider, this)) {Pablo Barrera GonzálezI'd very much like to avoid passing reference_signal_provider twice in some elegant way, but if it's too much work we can be sloppy here.
Not clear.
StatsReperter is already used in the constructor, so we need to have it as soon as possible. And I can't now build the StatsReporte in Create() to pass it to the constructor because it does need to know the controller it's connected to.
"Used" is a very high word. We just log AEC type We can do it in MaybeSetUpAudioProcessing
event_handler_->OnError(STREAM_CREATE_ERROR);Pablo Barrera GonzálezThis needs to be moved into DoCreate, to keep the separation of concerns. (so I suggest we return kOk, kVoiceIsolationFailure, kUnknown - or something like that)
I can move the code, no problem. I put it here to initialize the VoiceIsolation only once it was clear nothing else was failing, but likely it's not the best place.
I don't understand the part about returning kVoiceIsolationFailure, etc. What is returning these error codes? DoCreate?
No, this function will return them, and DoCreate will make a decision continue or not (separation of concerns: it's not a business of this function to make this decision).
Related details
- Olga Sharonova
- Pablo Barrera González
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. |
Pablo Barrera González (Gerrit)
Pablo Barrera González added 1 comment![]( "Open in Gerrit")
std::make_unique<StatsReporter>(reference_signal_provider, this)) {Pablo Barrera GonzálezI'd very much like to avoid passing reference_signal_provider twice in some elegant way, but if it's too much work we can be sloppy here.
Olga SharonovaNot clear.
StatsReperter is already used in the constructor, so we need to have it as soon as possible. And I can't now build the StatsReporte in Create() to pass it to the constructor because it does need to know the controller it's connected to.
"Used" is a very high word. We just log AEC type We can do it in MaybeSetUpAudioProcessing
MaybeSetUpAudioProcessing is only used it we are in CHROME_WIDE_ECHO_CANCELLATION. Do we need the log if CHROME_WIDE_ECHO_CANCELLATION is not set?
Related details
- Olga Sharonova
- 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 1 comment![]( "Open in Gerrit")
std::make_unique<StatsReporter>(reference_signal_provider, this)) {Pablo Barrera GonzálezI'd very much like to avoid passing reference_signal_provider twice in some elegant way, but if it's too much work we can be sloppy here.
Olga SharonovaNot clear.
StatsReperter is already used in the constructor, so we need to have it as soon as possible. And I can't now build the StatsReporte in Create() to pass it to the constructor because it does need to know the controller it's connected to.
Pablo Barrera González"Used" is a very high word. We just log AEC type We can do it in MaybeSetUpAudioProcessing
MaybeSetUpAudioProcessing is only used it we are in CHROME_WIDE_ECHO_CANCELLATION. Do we need the log if CHROME_WIDE_ECHO_CANCELLATION is not set?
- Olga Sharonova
- Pablo Barrera González
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. |
Pablo Barrera González (Gerrit)
Pablo Barrera González added 2 comments![]( "Open in Gerrit")
std::make_unique<StatsReporter>(reference_signal_provider, this)) {Pablo Barrera GonzálezI'd very much like to avoid passing reference_signal_provider twice in some elegant way, but if it's too much work we can be sloppy here.
Olga SharonovaNot clear.
StatsReperter is already used in the constructor, so we need to have it as soon as possible. And I can't now build the StatsReporte in Create() to pass it to the constructor because it does need to know the controller it's connected to.
Pablo Barrera González"Used" is a very high word. We just log AEC type We can do it in MaybeSetUpAudioProcessing
MaybeSetUpAudioProcessing is only used it we are in CHROME_WIDE_ECHO_CANCELLATION. Do we need the log if CHROME_WIDE_ECHO_CANCELLATION is not set?
Ok, let's move it to DoCreate.
Pablo Barrera GonzálezThis needs to be moved into DoCreate, to keep the separation of concerns. (so I suggest we return kOk, kVoiceIsolationFailure, kUnknown - or something like that)
Olga SharonovaI can move the code, no problem. I put it here to initialize the VoiceIsolation only once it was clear nothing else was failing, but likely it's not the best place.
I don't understand the part about returning kVoiceIsolationFailure, etc. What is returning these error codes? DoCreate?
No, this function will return them, and DoCreate will make a decision continue or not (separation of concerns: it's not a business of this function to make this decision).
- Olga Sharonova
- 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 4 comments![]( "Open in Gerrit")
const media::AudioParameters& output_params,
const media::AudioParameters& device_params,Why are we passing these here?
DoCreateStatsReporter(reference_signal_provider.get());We don't need it as a function, just create an object here?
Related details
- Olga Sharonova
- Pablo Barrera González
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 1 comment![]( "Open in Gerrit")
raw_ptr<MlModelManager> ml_model_manager) {Forgot to say: please switch the order of parameters? (usually managers and factories go first, parameters second)
Related details
- Olga Sharonova
- Pablo Barrera González
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. |
Pablo Barrera González (Gerrit)
Pablo Barrera González added 4 comments![]( "Open in Gerrit")
const media::AudioParameters& output_params,
const media::AudioParameters& device_params,Why are we passing these here?
Done
stats_reporter_(nullptr) {Pablo Barrera Gonzáleznot needed
Done
Forgot to say: please switch the order of parameters? (usually managers and factories go first, parameters second)
Done
DoCreateStatsReporter(reference_signal_provider.get());We don't need it as a function, just create an object here?
- Olga Sharonova
- 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 voted and added 6 comments![]( "Open in Gerrit")
Votes added by Olga Sharonova
| Code-Review | +1 |
6 comments
std::unique_ptr<media::VoiceIsolation> MaybeCreateVoiceIsolation(nit: add empty line
// processing components. If returns false if there is an issue that prevents
// DoCreate to continue executing.delete
std::unique_ptr<media::VoiceIsolation> voice_isolation = nullptr;drop
if (model_handle && model_handle->Get()) {if (!model_handle || !model_handle->Get()) {
return nullptr;
} voice_isolation = std::make_unique<media::VoiceIsolation>();return std::make_unique<media::VoiceIsolation>();
Related details
- Olga Sharonova
- Pablo Barrera González
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. |