Avoid dropping glitch info on IPC playout glitches [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-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 5 comments![]( "Open in Gerrit")
uint64_t prev_cumulative_glitch_count = 0;prev_cumulative_glitch_count_
int64_t prev_cumulative_glitch_duration_us = 0;prev_cumulative_glitch_duration_us_
.duration = base::Microseconds(cur_cumulative_glitch_duration_us -
prev_cumulative_glitch_duration_us),How do we ensure it's monotonic?
.count = static_cast<uint32_t>(cur_cumulative_glitch_count -saturated_cast?
int64_t cumulative_glitch_duration_us; // base::TimeDelta in microseconds.
uint64_t cumulative_glitch_count;Document more?
Related details
- Fredrik Hernqvist
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. |
Fredrik Hernqvist (Gerrit)
Fredrik Hernqvist added 6 comments![]( "Open in Gerrit")
uint64_t prev_cumulative_glitch_count = 0;Fredrik Hernqvistprev_cumulative_glitch_count_
Done
int64_t prev_cumulative_glitch_duration_us = 0;Fredrik Hernqvistprev_cumulative_glitch_duration_us_
Done
.duration = base::Microseconds(cur_cumulative_glitch_duration_us -
prev_cumulative_glitch_duration_us),How do we ensure it's monotonic?
Added DCHECKs. I am reluctant to use CHECKS here, because they will crash if the counters overflow when incrementing (we have had such a bug before), and that is much worse for the user experience than getting wrong numbers in the glitch stats.
.count = static_cast<uint32_t>(cur_cumulative_glitch_count -Fredrik Hernqvistsaturated_cast?
Good idea, done.
int64_t cumulative_glitch_duration_us; // base::TimeDelta in microseconds.
uint64_t cumulative_glitch_count;Document more?
- 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")
.duration = base::Microseconds(cur_cumulative_glitch_duration_us -
prev_cumulative_glitch_duration_us),Fredrik HernqvistHow do we ensure it's monotonic?
Added DCHECKs. I am reluctant to use CHECKS here, because they will crash if the counters overflow when incrementing (we have had such a bug before), and that is much worse for the user experience than getting wrong numbers in the glitch stats.
How do we actually know it will be monotonic and logic like start/stop or default device change won't mess it up?
Related details
- Fredrik Hernqvist
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. |
Fredrik Hernqvist (Gerrit)
Fredrik Hernqvist added 1 comment![]( "Open in Gerrit")
.duration = base::Microseconds(cur_cumulative_glitch_duration_us -
prev_cumulative_glitch_duration_us),Fredrik HernqvistHow do we ensure it's monotonic?
Olga SharonovaAdded DCHECKs. I am reluctant to use CHECKS here, because they will crash if the counters overflow when incrementing (we have had such a bug before), and that is much worse for the user experience than getting wrong numbers in the glitch stats.
How do we actually know it will be monotonic and logic like start/stop or default device change won't mess it up?
- This shared memory will only be used by a single AudioOutputDeviceThreadCallback, and a single SyncReader.
- Both the values in shared memory and the prev_* values are initialized to 0 upon creation.
- SyncReader is the only one that change the values in shared memory, and it only increases them, thus the values in shared memory are monotonically increasing
- AudioOutputDeviceThreadCallback reads the monotonically increasing values from the shared memory and stores them in the prev_* variables.
- Thus both the variables in shared memory and the prev_* variables are monotonically increasing, and the shared memory variables are always at least as large as the prev_* variables.
- This monotonic logic is constrained to AudioOutputDeviceThreadCallback and SyncReader, so this concern does not exist in the rest of the stack. This means that start/stop or default device change cannot affect it.
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 3 comments![]( "Open in Gerrit")
int64_t cur_cumulative_glitch_duration_us =On a second thought, should we convert the values to TimeDelta when reading them from memory, and use TimeDelta throughout? (This is a general recommendation when dealing with time.)
.duration = base::Microseconds(cur_cumulative_glitch_duration_us -
prev_cumulative_glitch_duration_us),Fredrik HernqvistHow do we ensure it's monotonic?
Olga SharonovaAdded DCHECKs. I am reluctant to use CHECKS here, because they will crash if the counters overflow when incrementing (we have had such a bug before), and that is much worse for the user experience than getting wrong numbers in the glitch stats.
Fredrik HernqvistHow do we actually know it will be monotonic and logic like start/stop or default device change won't mess it up?
- This shared memory will only be used by a single AudioOutputDeviceThreadCallback, and a single SyncReader.
- Both the values in shared memory and the prev_* values are initialized to 0 upon creation.
- SyncReader is the only one that change the values in shared memory, and it only increases them, thus the values in shared memory are monotonically increasing
- AudioOutputDeviceThreadCallback reads the monotonically increasing values from the shared memory and stores them in the prev_* variables.
- Thus both the variables in shared memory and the prev_* variables are monotonically increasing, and the shared memory variables are always at least as large as the prev_* variables.
- This monotonic logic is constrained to AudioOutputDeviceThreadCallback and SyncReader, so this concern does not exist in the rest of the stack. This means that start/stop or default device change cannot affect it.
Thanks. That actually highlights another aspect: we have non-trivial reads and writes of atomics split between two very distant code locations, which makes it hard to maintain the logic. Should we organize some helpers for that and put them together?
int64_t cumulative_glitch_duration_us; // base::TimeDelta in microseconds.
uint64_t cumulative_glitch_count;Fredrik HernqvistDocument more?
Done
- Fredrik Hernqvist
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. |
Fredrik Hernqvist (Gerrit)
Fredrik Hernqvist added 2 comments![]( "Open in Gerrit")
On a second thought, should we convert the values to TimeDelta when reading them from memory, and use TimeDelta throughout? (This is a general recommendation when dealing with time.)
Done
.duration = base::Microseconds(cur_cumulative_glitch_duration_us -
prev_cumulative_glitch_duration_us),Fredrik HernqvistHow do we ensure it's monotonic?
Olga SharonovaAdded DCHECKs. I am reluctant to use CHECKS here, because they will crash if the counters overflow when incrementing (we have had such a bug before), and that is much worse for the user experience than getting wrong numbers in the glitch stats.
Fredrik HernqvistHow do we actually know it will be monotonic and logic like start/stop or default device change won't mess it up?
Olga Sharonova
- This shared memory will only be used by a single AudioOutputDeviceThreadCallback, and a single SyncReader.
- Both the values in shared memory and the prev_* values are initialized to 0 upon creation.
- SyncReader is the only one that change the values in shared memory, and it only increases them, thus the values in shared memory are monotonically increasing
- AudioOutputDeviceThreadCallback reads the monotonically increasing values from the shared memory and stores them in the prev_* variables.
- Thus both the variables in shared memory and the prev_* variables are monotonically increasing, and the shared memory variables are always at least as large as the prev_* variables.
- This monotonic logic is constrained to AudioOutputDeviceThreadCallback and SyncReader, so this concern does not exist in the rest of the stack. This means that start/stop or default device change cannot affect it.
Thanks. That actually highlights another aspect: we have non-trivial reads and writes of atomics split between two very distant code locations, which makes it hard to maintain the logic. Should we organize some helpers for that and put them together?
Good idea, I put the logic in a helper class. How about this?
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 3 comments![]( "Open in Gerrit")
.duration = base::Microseconds(cur_cumulative_glitch_duration_us -
prev_cumulative_glitch_duration_us),Fredrik HernqvistHow do we ensure it's monotonic?
Olga SharonovaAdded DCHECKs. I am reluctant to use CHECKS here, because they will crash if the counters overflow when incrementing (we have had such a bug before), and that is much worse for the user experience than getting wrong numbers in the glitch stats.
Fredrik HernqvistHow do we actually know it will be monotonic and logic like start/stop or default device change won't mess it up?
Olga Sharonova
- This shared memory will only be used by a single AudioOutputDeviceThreadCallback, and a single SyncReader.
- Both the values in shared memory and the prev_* values are initialized to 0 upon creation.
- SyncReader is the only one that change the values in shared memory, and it only increases them, thus the values in shared memory are monotonically increasing
- AudioOutputDeviceThreadCallback reads the monotonically increasing values from the shared memory and stores them in the prev_* variables.
- Thus both the variables in shared memory and the prev_* variables are monotonically increasing, and the shared memory variables are always at least as large as the prev_* variables.
- This monotonic logic is constrained to AudioOutputDeviceThreadCallback and SyncReader, so this concern does not exist in the rest of the stack. This means that start/stop or default device change cannot affect it.
Fredrik HernqvistThanks. That actually highlights another aspect: we have non-trivial reads and writes of atomics split between two very distant code locations, which makes it hard to maintain the logic. Should we organize some helpers for that and put them together?
Good idea, I put the logic in a helper class. How about this?
Nice!
AudioGlitchInfo LoadGlitchInfoFromBuffer(AudioOutputBufferParameters& params);GetGlitchIncrementSinceLastCall, or something like that?
Also: do we always use both methods with the same params throughout the helper lifetime?
// living in shared memory.It's reading them in a very specific way (diff from the previous call). -could you document that?
Related details
- Fredrik Hernqvist
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. |