Integrate ReferenceSignalProvider error handling in the AudioService [chromium/src : main]
Fredrik Hernqvist (Gerrit)
Fredrik Hernqvist voted and added 1 comment![]( "Open in Gerrit")
Votes added by Fredrik Hernqvist
| Commit-Queue | +1 |
1 comment
Related details
- Olga Sharonova
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olga Sharonova (Gerrit)
Olga Sharonova added 5 comments![]( "Open in Gerrit")
reference_stream_error_callback_.Run();Do we have any threading expectations?
base::BindPostTask(task_runner_,So what do we have with error posting? Could you describe the full picture - how many times we post them in each context?
Also, what happens if the error signaling is scheduled after some other operations?
// InputStream will be responsible for closing the stream after callingWhich stream? Itself? Or InputController? Or?
event_handler_->OnError(What about this error being signaled in the context of Record(), especially given that InputStream is responsible for closing, according to the comment?
}We need to do a better cleanup/state maintenance.
1) audio_callback_ being not null does not mean Record() started successfully any more
2) processing_fifo_ will run the dedicated thread even if we have not started.
Related details
- Fredrik Hernqvist
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fredrik Hernqvist (Gerrit)
Fredrik Hernqvist added 6 comments![]( "Open in Gerrit")
static int error_test;Ignore the changes in the catap files until the rest of the code is ready, I use this for testing.
Do we have any threading expectations?
Added a check, done.
base::BindPostTask(task_runner_,So what do we have with error posting? Could you describe the full picture - how many times we post them in each context?
Also, what happens if the error signaling is scheduled after some other operations?
event_handler_->OnError() can be called in these situations:
1) In DoReportError posted from the AudioCallback when the input stream has an error (through the callback bound here). This can happen an arbitrary number of times depending on the OS implementation of the input stream, but probably once.
2) In DoReportError posted from the AudioProcessorHandler when the reference stream has had an error. This can happen once.
3) In DoCreate() (once) if we fail to create/open the stream.
4) In Record() (once per record) if we fail to start the reference signal.
I have not found a case where it matters when the error signaling is scheduled, the result is always that the stream is eventually destroyed.
// InputStream will be responsible for closing the stream after callingWhich stream? Itself? Or InputController? Or?
Both, clarified.
event_handler_->OnError(What about this error being signaled in the context of Record(), especially given that InputStream is responsible for closing, according to the comment?
It can be called at any time (it's also called in OnCreate, for example). It will schedule the deletion of the InputStream and InputController, but that won't happen until Record() finishes.
We need to do a better cleanup/state maintenance.
1) audio_callback_ being not null does not mean Record() started successfully any more
2) processing_fifo_ will run the dedicated thread even if we have not started.
I reordered it so that audio_callback_ will be null and the fifo will not start if the reference stream failed to start.
Related details
- Olga Sharonova
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fredrik Hernqvist (Gerrit)
Fredrik Hernqvist added 1 comment![]( "Open in Gerrit")
event_handler_->OnError(Fredrik HernqvistWhat about this error being signaled in the context of Record(), especially given that InputStream is responsible for closing, according to the comment?
It can be called at any time (it's also called in OnCreate, for example). It will schedule the deletion of the InputStream and InputController, but that won't happen until Record() finishes.
The JS-observable effect is that the stream will end (with the ended-event being dispatched) shortly after the getUserMedia-promise resolves successfully.
Related details
- Olga Sharonova
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olga Sharonova (Gerrit)
Olga Sharonova added 11 comments![]( "Open in Gerrit")
const ReferenceStreamErrorCallback reference_stream_error_callback_;GUARDED_BY_CONTEXT(owning_sequence_)?
// Called on the main thread.On owning_sequence_?
// Failed to open aec reference stream due to device in use by another app.
// Failed to open aec reference stream due to lack of system permissions.
// Failed to create aec reference stream.
// The AEC reference stream failed to start. By calling OnError() we let
// the owning InputStream know about the error. This will schedule
// the deletion of the InputStream and the InputController on the main
// thread. As the InputController is now about to be deleted, we exit
// early and do not start the stream or the processing thread.Is this true for all DoReportError() calls? Why do we need to state it here specifically?
base::BindPostTask(task_runner_,Fredrik HernqvistSo what do we have with error posting? Could you describe the full picture - how many times we post them in each context?
Also, what happens if the error signaling is scheduled after some other operations?
event_handler_->OnError() can be called in these situations:
1) In DoReportError posted from the AudioCallback when the input stream has an error (through the callback bound here). This can happen an arbitrary number of times depending on the OS implementation of the input stream, but probably once.
2) In DoReportError posted from the AudioProcessorHandler when the reference stream has had an error. This can happen once.
3) In DoCreate() (once) if we fail to create/open the stream.
4) In Record() (once per record) if we fail to start the reference signal.I have not found a case where it matters when the error signaling is scheduled, the result is always that the stream is eventually destroyed.
1) In DoReportError posted from the AudioCallback
From which threads?
2) In DoReportError posted from the AudioProcessorHandle
From which threads?
4) In Record() (once per record) if we fail to start the reference signal.
once per record? the comment you added in the new PS says InputController will be deleted.
event_handler_->OnError(Fredrik HernqvistWhat about this error being signaled in the context of Record(), especially given that InputStream is responsible for closing, according to the comment?
Fredrik HernqvistIt can be called at any time (it's also called in OnCreate, for example). It will schedule the deletion of the InputStream and InputController, but that won't happen until Record() finishes.
The JS-observable effect is that the stream will end (with the ended-event being dispatched) shortly after the getUserMedia-promise resolves successfully.
" It will schedule the deletion of the InputStream"
could you point to the code?
Fredrik HernqvistWe need to do a better cleanup/state maintenance.
1) audio_callback_ being not null does not mean Record() started successfully any more
2) processing_fifo_ will run the dedicated thread even if we have not started.
I reordered it so that audio_callback_ will be null and the fifo will not start if the reference stream failed to start.
- Fredrik Hernqvist
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fredrik Hernqvist (Gerrit)
Fredrik Hernqvist added 10 comments![]( "Open in Gerrit")
const ReferenceStreamErrorCallback reference_stream_error_callback_;Fredrik HernqvistGUARDED_BY_CONTEXT(owning_sequence_)?
Done
// Called on the main thread.Fredrik HernqvistOn owning_sequence_?
Done
// Failed to open aec reference stream due to device in use by another appFredrik Hernqvist.
Done
// Failed to open aec reference stream due to lack of system permissionsFredrik Hernqvist.
Done
// Failed to open aec reference streamFredrik Hernqvist.
Done
// Failed to create aec reference streamFredrik Hernqvist.
Done
// The AEC reference stream failed to start. By calling OnError() we let
// the owning InputStream know about the error. This will schedule
// the deletion of the InputStream and the InputController on the main
// thread. As the InputController is now about to be deleted, we exit
// early and do not start the stream or the processing thread.Is this true for all DoReportError() calls? Why do we need to state it here specifically?
Yes it's always true. Removed the comment.
event_handler_->OnError(Fredrik HernqvistDoReportError()
Done
base::BindPostTask(task_runner_,Fredrik HernqvistSo what do we have with error posting? Could you describe the full picture - how many times we post them in each context?
Also, what happens if the error signaling is scheduled after some other operations?
Olga Sharonovaevent_handler_->OnError() can be called in these situations:
1) In DoReportError posted from the AudioCallback when the input stream has an error (through the callback bound here). This can happen an arbitrary number of times depending on the OS implementation of the input stream, but probably once.
2) In DoReportError posted from the AudioProcessorHandler when the reference stream has had an error. This can happen once.
3) In DoCreate() (once) if we fail to create/open the stream.
4) In Record() (once per record) if we fail to start the reference signal.I have not found a case where it matters when the error signaling is scheduled, the result is always that the stream is eventually destroyed.
1) In DoReportError posted from the AudioCallback
From which threads?
2) In DoReportError posted from the AudioProcessorHandle
From which threads?
4) In Record() (once per record) if we fail to start the reference signal.
once per record? the comment you added in the new PS says InputController will be deleted.
1) Posted from either realtime thread or main thread (as usual with AudioInputCallback::OnError)
2) Always posted from main thread
4) As far as I understand the code, there will only ever be one call to Record() regardless of errors. I stated it as "once per record" in case that ever changes, but it may just have confused things.
event_handler_->OnError(Fredrik HernqvistWhat about this error being signaled in the context of Record(), especially given that InputStream is responsible for closing, according to the comment?
Fredrik HernqvistIt can be called at any time (it's also called in OnCreate, for example). It will schedule the deletion of the InputStream and InputController, but that won't happen until Record() finishes.
Olga SharonovaThe JS-observable effect is that the stream will end (with the ended-event being dispatched) shortly after the getUserMedia-promise resolves successfully.
" It will schedule the deletion of the InputStream"
could you point to the code?
OnError() implementation: https://source.chromium.org/chromium/chromium/src/+/main:services/audio/input_stream.cc;l=255;drc=2a9c070f15d3fc47eeee29d0c1982f82f0d11fb6
Then the call chain goes
OnError() ->
OnStreamError() ->
PostTask(CallDeleter) to main thread ->
...wait for the task to run... ->
CallDeleter() ->
delete_callback_.Run() ->
StreamFactory::DestroyInputStream() ->
~InputStream()
Related details
- Olga Sharonova
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olga Sharonova (Gerrit)
Olga Sharonova voted and added 4 comments![]( "Open in Gerrit")
Votes added by Olga Sharonova
| Code-Review | +1 |
4 comments
// Called on owning_sequence_.`owning_sequence_`. (backticks)
base::BindPostTask(task_runner_,Fredrik HernqvistSo what do we have with error posting? Could you describe the full picture - how many times we post them in each context?
Also, what happens if the error signaling is scheduled after some other operations?
Olga Sharonovaevent_handler_->OnError() can be called in these situations:
1) In DoReportError posted from the AudioCallback when the input stream has an error (through the callback bound here). This can happen an arbitrary number of times depending on the OS implementation of the input stream, but probably once.
2) In DoReportError posted from the AudioProcessorHandler when the reference stream has had an error. This can happen once.
3) In DoCreate() (once) if we fail to create/open the stream.
4) In Record() (once per record) if we fail to start the reference signal.I have not found a case where it matters when the error signaling is scheduled, the result is always that the stream is eventually destroyed.
Fredrik Hernqvist1) In DoReportError posted from the AudioCallback
From which threads?
2) In DoReportError posted from the AudioProcessorHandle
From which threads?
4) In Record() (once per record) if we fail to start the reference signal.
once per record? the comment you added in the new PS says InputController will be deleted.
1) Posted from either realtime thread or main thread (as usual with AudioInputCallback::OnError)
2) Always posted from main thread
4) As far as I understand the code, there will only ever be one call to Record() regardless of errors. I stated it as "once per record" in case that ever changes, but it may just have confused things.
Acknowledged
event_handler_->OnError(Fredrik HernqvistWhat about this error being signaled in the context of Record(), especially given that InputStream is responsible for closing, according to the comment?
Fredrik HernqvistIt can be called at any time (it's also called in OnCreate, for example). It will schedule the deletion of the InputStream and InputController, but that won't happen until Record() finishes.
Olga SharonovaThe JS-observable effect is that the stream will end (with the ended-event being dispatched) shortly after the getUserMedia-promise resolves successfully.
Fredrik Hernqvist" It will schedule the deletion of the InputStream"
could you point to the code?
OnError() implementation: https://source.chromium.org/chromium/chromium/src/+/main:services/audio/input_stream.cc;l=255;drc=2a9c070f15d3fc47eeee29d0c1982f82f0d11fb6
Then the call chain goes
OnError() ->
OnStreamError() ->
PostTask(CallDeleter) to main thread ->
...wait for the task to run... ->
CallDeleter() ->
delete_callback_.Run() ->
StreamFactory::DestroyInputStream() ->
~InputStream()
- Fredrik Hernqvist
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fredrik Hernqvist (Gerrit)
Fredrik Hernqvist added 3 comments![]( "Open in Gerrit")
static int error_test;Ignore the changes in the catap files until the rest of the code is ready, I use this for testing.
resolving
// Called on owning_sequence_.Fredrik Hernqvist`owning_sequence_`. (backticks)
`Done`
// The AEC reference stream failed to start. By calling OnError() we let
// the owning InputStream know about the error. This will schedule
// the deletion of the InputStream and the InputController on the main
// thread. As the InputController is now about to be deleted, we exit
// early and do not start the stream or the processing thread.Fredrik HernqvistIs this true for all DoReportError() calls? Why do we need to state it here specifically?
Yes it's always true. Removed the comment.
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fredrik Hernqvist (Gerrit)
Fredrik Hernqvist voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
15 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: services/audio/audio_processor_handler.h
Insertions: 1, Deletions: 1.
@@ -110,7 +110,7 @@
void OnPlayoutData(const media::AudioBus& audio_bus,
int sample_rate,
base::TimeDelta delay) final;
- // Called on owning_sequence_.
+ // Called on `owning_sequence_`.
void OnReferenceStreamError() final;
// mojom::AudioProcessorControls implementation.
```
Change information
Integrate ReferenceSignalProvider error handling in the AudioService
This plumbs the errors into InputController, which calls OnError on its
EventHandler. This means that when an AEC reference loopback stream has
an error, all subscribing capture streams will stop with an error.
- M services/audio/audio_processor_handler.cc
- M services/audio/audio_processor_handler.h
- M services/audio/input_controller.cc
- M services/audio/input_controller.h
- M services/audio/input_controller_unittest.cc
- M services/audio/input_stream.cc
- M services/audio/output_tapper.cc
- M services/audio/output_tapper.h
Code-Review: +1 by Olga Sharonova
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |