New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 878764 link

Starred by 2 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 3
Type: Bug


Participants' hotlists:
AudioService-FixIt


Sign in to add a comment

Enable GetStats to work async with the APM

Project Member Reported by maxmorin@chromium.org, Aug 29

Issue description

The GetStats API is async in the JS layer, but (to my understanding) when the call reaches third_party/webrtc code, webrtc asks the APM for stats synchronously. This doesn't play nice with the APM being in the audio process, so we are making an ugly polling mechanism in the renderer so we can serve cached stats. We should instead change webrtc to allow the APM to respond asynchronously.

This is probably complicated?
 
getStats() is already an async API with callback and everything, but a lot of things in third_party/webrtc uses sync invokes.

rtcstatscollector.cc (new getStats) has ProducePartialResultsOnNetworkThread() and ProducePartialResultsOnSignalingThread() and awaits both operations to finish before it merges the reports into a shared report and returns the result. That means some stats object come from one thread and some stats objects come from a different thread, but all stats reports are returned in the result.
We could have "ProducePartialResultsOnAudioProcess()". What's different now is some individual stats members of a stats object may come come from different threads, but that should be OK.

statscollector.cc (old getStats) has "UpdateStats()" which is synchronously implemented. But the stats collector is an implementation detail, the actual PeerConnection[Interface]::GetStats() API (https://cs.chromium.org/chromium/src/third_party/webrtc/pc/peerconnection.cc?sq=package:chromium&dr&g=0&l=1567) is async with a callback. And statscollector.cc's stats objects (called StatsReport for the sake of confusion) are just containers where stats are added. It should be possible to add to them from an async APM callback.

It requires some thinking and refactoring but I don't see any big blockers since Chrome is already treating the API as async.

What could be a problem is large delays are introduced due to waiting for the APM that bottlenecks and applications are assuming that there are no massive delays.
To clarify: rtcstatscollector.cc does not "await" in any blocking sense, I meant the getStats-callback is not invoked until both "ProducePartialResultsOn*" have completed, which in practice means when the network thread has run its task.

statscollector.cc's UpdateStats() is blocking on the signaling thread but its used in an API that already does PostTask to invoke the callback.
Project Member

Comment 3 by bugdroid1@chromium.org, Aug 30

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/86d4f131d29c49e761ee20d9b1c5026b1172ef50

commit 86d4f131d29c49e761ee20d9b1c5026b1172ef50
Author: Max Morin <maxmorin@chromium.org>
Date: Thu Aug 30 21:51:13 2018

APM move: Configure and use APM in Audio Service

ProcessedLocalAudioSource is changed to either create a
MediaStreamAudioProcessor or an AudioServiceAudioProcessorProxy depending
on whether the WebRtcApmInAudioService flag is set.

AudioServiceAudioProcessorProxy proxies GetStats and AECDump calls to
the remote audio processor. Although the JavaScript getStats call is
asynchronous, we currently collect stats synchronously inside Chrome.
Ideally, this would be changed. For now, the proxy overcomes this
mismatch by polling the remote audio processor for stats at regular
intervals. It uses a heuristic to determine the rate at which the user
is calling getStats and tries to match that, within some reasonable
limits. 878764 has been filed to fix this.

For AECDumps, we already get a file-handle from the browser, so it can
just be sent along to the audio service. So the audio service does not
need to be able to create files for this to work.

For an outline of the project this CL is part of, see: https://docs.google.com/document/d/1u4POff_ts_1LE3WDLA_wDDFnUswdlsuHL5DsiTE0a3U/edit?usp=sharing
It's accessible to everyone @chromium.org.

No-try since the test timing out is unrelated.

No-Try: true
Bug: 851959, 878764, 879133,  879243 , 879296
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: I7473c068aa691d69f6ba90dce2550534c9cb3d8a
Reviewed-on: https://chromium-review.googlesource.com/1169471
Commit-Queue: Max Morin <maxmorin@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Olga Sharonova <olka@chromium.org>
Reviewed-by: Max Morin <maxmorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587796}
[modify] https://crrev.com/86d4f131d29c49e761ee20d9b1c5026b1172ef50/content/browser/media/media_internals.cc
[modify] https://crrev.com/86d4f131d29c49e761ee20d9b1c5026b1172ef50/content/browser/renderer_host/media/audio_output_delegate_impl_unittest.cc
[modify] https://crrev.com/86d4f131d29c49e761ee20d9b1c5026b1172ef50/content/browser/webrtc/webrtc_audio_browsertest.cc
[modify] https://crrev.com/86d4f131d29c49e761ee20d9b1c5026b1172ef50/content/renderer/BUILD.gn
[modify] https://crrev.com/86d4f131d29c49e761ee20d9b1c5026b1172ef50/content/renderer/media/audio/mojo_audio_input_ipc.cc
[add] https://crrev.com/86d4f131d29c49e761ee20d9b1c5026b1172ef50/content/renderer/media/stream/audio_service_audio_processor_proxy.cc
[add] https://crrev.com/86d4f131d29c49e761ee20d9b1c5026b1172ef50/content/renderer/media/stream/audio_service_audio_processor_proxy.h
[modify] https://crrev.com/86d4f131d29c49e761ee20d9b1c5026b1172ef50/content/renderer/media/stream/media_stream_audio_processor_options.cc
[modify] https://crrev.com/86d4f131d29c49e761ee20d9b1c5026b1172ef50/content/renderer/media/stream/media_stream_audio_processor_options.h
[modify] https://crrev.com/86d4f131d29c49e761ee20d9b1c5026b1172ef50/content/renderer/media/stream/processed_local_audio_source.cc
[modify] https://crrev.com/86d4f131d29c49e761ee20d9b1c5026b1172ef50/content/renderer/media/stream/processed_local_audio_source.h
[modify] https://crrev.com/86d4f131d29c49e761ee20d9b1c5026b1172ef50/content/renderer/media/webrtc/webrtc_audio_device_impl.cc
[modify] https://crrev.com/86d4f131d29c49e761ee20d9b1c5026b1172ef50/content/renderer/media/webrtc/webrtc_audio_device_impl.h
[modify] https://crrev.com/86d4f131d29c49e761ee20d9b1c5026b1172ef50/content/renderer/media/webrtc/webrtc_audio_renderer.cc
[modify] https://crrev.com/86d4f131d29c49e761ee20d9b1c5026b1172ef50/content/renderer/media/webrtc/webrtc_audio_renderer_unittest.cc
[modify] https://crrev.com/86d4f131d29c49e761ee20d9b1c5026b1172ef50/content/renderer/media/webrtc/webrtc_audio_sink.cc
[modify] https://crrev.com/86d4f131d29c49e761ee20d9b1c5026b1172ef50/content/renderer/media/webrtc/webrtc_audio_sink.h
[modify] https://crrev.com/86d4f131d29c49e761ee20d9b1c5026b1172ef50/content/renderer/media/webrtc/webrtc_media_stream_track_adapter.cc
[modify] https://crrev.com/86d4f131d29c49e761ee20d9b1c5026b1172ef50/content/test/BUILD.gn
[modify] https://crrev.com/86d4f131d29c49e761ee20d9b1c5026b1172ef50/media/audio/BUILD.gn
[modify] https://crrev.com/86d4f131d29c49e761ee20d9b1c5026b1172ef50/media/audio/audio_logging.h
[add] https://crrev.com/86d4f131d29c49e761ee20d9b1c5026b1172ef50/media/audio/audio_processing.cc
[modify] https://crrev.com/86d4f131d29c49e761ee20d9b1c5026b1172ef50/media/audio/audio_processing.h
[modify] https://crrev.com/86d4f131d29c49e761ee20d9b1c5026b1172ef50/media/audio/audio_source_parameters.h
[modify] https://crrev.com/86d4f131d29c49e761ee20d9b1c5026b1172ef50/media/audio/fake_audio_log_factory.cc
[modify] https://crrev.com/86d4f131d29c49e761ee20d9b1c5026b1172ef50/media/mojo/interfaces/audio_logging.mojom
[modify] https://crrev.com/86d4f131d29c49e761ee20d9b1c5026b1172ef50/services/audio/input_controller.cc
[modify] https://crrev.com/86d4f131d29c49e761ee20d9b1c5026b1172ef50/services/audio/input_stream.cc
[modify] https://crrev.com/86d4f131d29c49e761ee20d9b1c5026b1172ef50/services/audio/log_adapter.cc
[modify] https://crrev.com/86d4f131d29c49e761ee20d9b1c5026b1172ef50/services/audio/log_adapter.h
[modify] https://crrev.com/86d4f131d29c49e761ee20d9b1c5026b1172ef50/services/audio/log_factory_manager_unittest.cc
[modify] https://crrev.com/86d4f131d29c49e761ee20d9b1c5026b1172ef50/services/audio/test/mock_log.h

What is the impact of it? Should we raise the priority now?

Sign in to add a comment