[WebAudio] Implement 2-second hysteresis for audibility IPCs [chromium/src : main]
Hongchan Choi (Gerrit)
Hongchan Choi added 1 comment![]( "Open in Gerrit")
Related details
- Michael Wilson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Wilson (Gerrit)
Michael Wilson added 9 comments![]( "Open in Gerrit")
PTAL
I gave some initial comments. I would like to take at least one more review pass to make sure I understand the control flow once the comments are addressed.
std::atomic<int> audibility_sequence_id_{0};We should use an unsigned datatype here, since over a high number of transitions this could overflow and signed overflow is undefined in C++.
```suggestion
std::atomic<unsigned> audibility_sequence_id_{0};
```
void NotifyAudibleAudioStopped(int sequence_id, double silence_time)```suggestion
void NotifyAudibleAudioStopped(unsigned sequence_id, double silence_time)
```
void NotifyAudibleAudioStarted(int sequence_id)```suggestion
void NotifyAudibleAudioStarted(unsigned sequence_id)
```
void AudioContext::NotifyAudibleAudioStarted(int sequence_id) {```suggestion
void AudioContext::NotifyAudibleAudioStarted(unsigned sequence_id) {
```
accumulated_silence_time_.store(0.0, std::memory_order_relaxed);Should we report or cache any accumulated silence time before setting back to 0? I'm thinking of a scenario where this is the first audible buffer after several silent buffers; in that case will we properly report the silence or will it be lost?
silence_time = accumulated_silence_time_.load(std::memory_order_relaxed) +
destination_bus->length() / static_cast<double>(sampleRate());
accumulated_silence_time_.store(silence_time, std::memory_order_relaxed);Maybe using `fetch_add` here would be slightly safer, since we wouldn't have any gap between the load and store:
```suggestion
const double increment = destination_bus->length() / static_cast<double>(sampleRate());
silence_time = accumulated_silence_time_.fetch_add(increment, std::memory_order_relaxed) + increment;
```
DeferredTaskHandler::GraphAutoLocker locker(GetDeferredTaskHandler());What is holding the graph lock here guarding against?
It seems like all the variables being modified in this block are atomics, which are not guarded in other usages by the graph lock. Would it make sense to not hold the lock here at all, and just rely on the atomic variables?
media_player_receiver_.reset();Is resetting the media player receiver related to the rest of the change?
Related details
- Hongchan Choi
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. |
Hongchan Choi (Gerrit)
Hongchan Choi voted and added 8 comments![]( "Open in Gerrit")
Votes added by Hongchan Choi
| Commit-Queue | +1 |
8 comments
We should use an unsigned datatype here, since over a high number of transitions this could overflow and signed overflow is undefined in C++.
```suggestion
std::atomic<unsigned> audibility_sequence_id_{0};
```
Done
void NotifyAudibleAudioStopped(int sequence_id, double silence_time)```suggestion
void NotifyAudibleAudioStopped(unsigned sequence_id, double silence_time)
```
Done
void NotifyAudibleAudioStarted(int sequence_id)Hongchan Choi```suggestion
void NotifyAudibleAudioStarted(unsigned sequence_id)
```
Done
void AudioContext::NotifyAudibleAudioStarted(int sequence_id) {```suggestion
void AudioContext::NotifyAudibleAudioStarted(unsigned sequence_id) {
```
Done
accumulated_silence_time_.store(0.0, std::memory_order_relaxed);Should we report or cache any accumulated silence time before setting back to 0? I'm thinking of a scenario where this is the first audible buffer after several silent buffers; in that case will we properly report the silence or will it be lost?
By design, this hysteresis is strictly for tracking audibility metrics (unlike dynamic range processors like gates or expanders that manipulate the audio signal). I propose keeping it simple by resetting it back to 0.
silence_time = accumulated_silence_time_.load(std::memory_order_relaxed) +
destination_bus->length() / static_cast<double>(sampleRate());
accumulated_silence_time_.store(silence_time, std::memory_order_relaxed);Maybe using `fetch_add` here would be slightly safer, since we wouldn't have any gap between the load and store:
```suggestion
const double increment = destination_bus->length() / static_cast<double>(sampleRate());
silence_time = accumulated_silence_time_.fetch_add(increment, std::memory_order_relaxed) + increment;
```
There are two reasons we did not use fetch_add here:
1. `accumulated_silence_time_` is a `std::atomic<double>` (see below)
2. There is no race condition here. The main thread only writes to it in `ClearAudibilityState()`, but `ClearAudibilityState()` is only called after the audio thread is synchronously stopped (via `StopRendering()`)
---
Re: std::atomic<double> in C++20
If a CPU architecture does not natively support atomic floating-point instructions (which is common for 64-bit doubles on 32-bit platforms, or certain ARM processors), the compiler/standard library implements fetch_add by wrapping the operation in an internal spinlock or mutex (falling back to libatomic).
DeferredTaskHandler::GraphAutoLocker locker(GetDeferredTaskHandler());What is holding the graph lock here guarding against?
It seems like all the variables being modified in this block are atomics, which are not guarded in other usages by the graph lock. Would it make sense to not hold the lock here at all, and just rely on the atomic variables?
Done. Good catch!
media_player_receiver_.reset();Is resetting the media player receiver related to the rest of the change?
It is related to the overall Mojo interaction cleanup we are doing for the AudioContext lifecycle, but I can take this out of this CL. Let me know.
Related details
- Michael Wilson
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. |
Michael Wilson (Gerrit)
Michael Wilson added 3 comments![]( "Open in Gerrit")
accumulated_silence_time_.store(0.0, std::memory_order_relaxed);Hongchan ChoiShould we report or cache any accumulated silence time before setting back to 0? I'm thinking of a scenario where this is the first audible buffer after several silent buffers; in that case will we properly report the silence or will it be lost?
By design, this hysteresis is strictly for tracking audibility metrics (unlike dynamic range processors like gates or expanders that manipulate the audio signal). I propose keeping it simple by resetting it back to 0.
Sorry, I don't understand this response. Are you saying it's fine if the metrics are not accurate? Can we quantify how much inaccuracy is acceptable?
One specific concern I have is that it seems like this metric is used for MEI calculations. If we are under-reporting silence could it possibly artificially inflate the MEI, and thus allow sites to autoplay media that otherwise would not be able to?
silence_time = accumulated_silence_time_.load(std::memory_order_relaxed) +
destination_bus->length() / static_cast<double>(sampleRate());
accumulated_silence_time_.store(silence_time, std::memory_order_relaxed);Hongchan ChoiMaybe using `fetch_add` here would be slightly safer, since we wouldn't have any gap between the load and store:
```suggestion
const double increment = destination_bus->length() / static_cast<double>(sampleRate());
silence_time = accumulated_silence_time_.fetch_add(increment, std::memory_order_relaxed) + increment;
```
There are two reasons we did not use fetch_add here:
1. `accumulated_silence_time_` is a `std::atomic<double>` (see below)
2. There is no race condition here. The main thread only writes to it in `ClearAudibilityState()`, but `ClearAudibilityState()` is only called after the audio thread is synchronously stopped (via `StopRendering()`)---
Re: std::atomic<double> in C++20If a CPU architecture does not natively support atomic floating-point instructions (which is common for 64-bit doubles on 32-bit platforms, or certain ARM processors), the compiler/standard library implements fetch_add by wrapping the operation in an internal spinlock or mutex (falling back to libatomic).
Looks reasonable to me, thanks.
media_player_receiver_.reset();Hongchan ChoiIs resetting the media player receiver related to the rest of the change?
It is related to the overall Mojo interaction cleanup we are doing for the AudioContext lifecycle, but I can take this out of this CL. Let me know.
I don't feel strongly about this. If it's related to the change we can keep it.
Related details
- Hongchan Choi
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. |
Hongchan Choi (Gerrit)
Hongchan Choi added 1 comment![]( "Open in Gerrit")
accumulated_silence_time_.store(0.0, std::memory_order_relaxed);Hongchan ChoiShould we report or cache any accumulated silence time before setting back to 0? I'm thinking of a scenario where this is the first audible buffer after several silent buffers; in that case will we properly report the silence or will it be lost?
Michael WilsonBy design, this hysteresis is strictly for tracking audibility metrics (unlike dynamic range processors like gates or expanders that manipulate the audio signal). I propose keeping it simple by resetting it back to 0.
Sorry, I don't understand this response. Are you saying it's fine if the metrics are not accurate? Can we quantify how much inaccuracy is acceptable?
One specific concern I have is that it seems like this metric is used for MEI calculations. If we are under-reporting silence could it possibly artificially inflate the MEI, and thus allow sites to autoplay media that otherwise would not be able to?
I think that the current hysteresis design is the right trade-off for the following reasons:
1. The `IsAudible()` helper is a simple energy detector (energy > 0) to keep the real-time audio thread fast and lock-free. It does not perform psychoacoustic human-audibility checks. As a result, any page can already trivially fake audibility and inflate its MEI score by streaming a continuous inaudible signal (e.g., a constant 0.5 DC offset or a sub-audible 5 Hz sine wave) for 7 seconds. The proposed hysteresis does not introduce any new exploit vectors that do not already exist.
2. The MEI score and database are maintained **locally** on the user's profile to make local autoplay decisions. They are not uploaded to a global telemetry server or aggregated to modify global autoplay permissions, so any potential local inaccuracy remains isolated to that specific client session.
3. In cases of rapid audibility micro-toggling, this hysteresis solves a severe thread contention issue by eliminating up to a few thousands Mojo IPCs/sec. The CPU overhead and threat to audio thread real-time safety are significantly reduced, which I believe outweighs the theoretical risk of minor engagement index inflation.
Related details
- Michael Wilson
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. |
Michael Wilson (Gerrit)
Michael Wilson voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michael Wilson
| Code-Review | +1 |
1 comment
accumulated_silence_time_.store(0.0, std::memory_order_relaxed);Hongchan ChoiShould we report or cache any accumulated silence time before setting back to 0? I'm thinking of a scenario where this is the first audible buffer after several silent buffers; in that case will we properly report the silence or will it be lost?
Michael WilsonBy design, this hysteresis is strictly for tracking audibility metrics (unlike dynamic range processors like gates or expanders that manipulate the audio signal). I propose keeping it simple by resetting it back to 0.
Hongchan ChoiSorry, I don't understand this response. Are you saying it's fine if the metrics are not accurate? Can we quantify how much inaccuracy is acceptable?
One specific concern I have is that it seems like this metric is used for MEI calculations. If we are under-reporting silence could it possibly artificially inflate the MEI, and thus allow sites to autoplay media that otherwise would not be able to?
I think that the current hysteresis design is the right trade-off for the following reasons:
1. The `IsAudible()` helper is a simple energy detector (energy > 0) to keep the real-time audio thread fast and lock-free. It does not perform psychoacoustic human-audibility checks. As a result, any page can already trivially fake audibility and inflate its MEI score by streaming a continuous inaudible signal (e.g., a constant 0.5 DC offset or a sub-audible 5 Hz sine wave) for 7 seconds. The proposed hysteresis does not introduce any new exploit vectors that do not already exist.
2. The MEI score and database are maintained **locally** on the user's profile to make local autoplay decisions. They are not uploaded to a global telemetry server or aggregated to modify global autoplay permissions, so any potential local inaccuracy remains isolated to that specific client session.
3. In cases of rapid audibility micro-toggling, this hysteresis solves a severe thread contention issue by eliminating up to a few thousands Mojo IPCs/sec. The CPU overhead and threat to audio thread real-time safety are significantly reduced, which I believe outweighs the theoretical risk of minor engagement index inflation.
I don't want to block the CL on this. We can discuss offline and land a follow-up if necessary.
Related details
- Hongchan Choi
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Wilson (Gerrit)
Michael Wilson voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Hongchan Choi
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongchan Choi (Gerrit)
Hongchan Choi added 1 comment![]( "Open in Gerrit")
- Nate Chapin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Chapin (Gerrit)
Nate Chapin voted Code-Review+1![]( "Open in Gerrit")
- Hongchan Choi
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongchan Choi (Gerrit)
Hongchan Choi voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[WebAudio] Implement 2-second hysteresis for audibility IPCs
This CL implements a 2.0-second silence hysteresis for the
AudioContext audibility notifications to the browser process.
Previously, rapid transitions between audible and silent states
could result in excessive Mojo IPC traffic (up to ~2,600 IPCs/sec),
leading to thread contention and high CPU usage.
Changes:
- Added a 2.0-second silence threshold (`kSilenceThresholdSeconds`).
- Delayed the "stopped rendering audible audio" IPC by the threshold
duration. If audio becomes audible again before the threshold
expires, the silence timer is reset and no Mojo IPC is dispatched.
- Passed the silence duration as a parameter to
`NotifyAudibleAudioStopped` to allow the browser to accurately
subtract the silence window from the total audible duration.
- Declared audibility tracking member variables (`was_audible_` and
`accumulated_silence_time_`) as `std::atomic` using relaxed memory
ordering to prevent C++ concurrency problems between the real-time
audio thread and main thread.
- Added comprehensive unit tests in `audio_context_test.cc`.
TAG=agy
CONV=6ca652b0-350c-465c-bcef-c928a2dab54a
- M third_party/blink/common/features.cc
- M third_party/blink/public/common/features.h
- M third_party/blink/renderer/modules/webaudio/audio_context.cc
- M third_party/blink/renderer/modules/webaudio/audio_context.h
- M third_party/blink/renderer/modules/webaudio/audio_context_test.cc
Code-Review: +1 by Michael Wilson, +1 by Nate Chapin
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |