Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(statistics) remove AudioOutputProblemDetector #2551

Merged
merged 1 commit into from
Aug 8, 2024

Conversation

saghul
Copy link
Member

@saghul saghul commented Aug 8, 2024

It has been broken for over 3 years now, since ca325f5#diff-9e19da30f465ca5665ac3d7ca1aa03d0498aed1be0cb2d7eeb27684a2636da77

Ever since that change, the "audioLevelReportHistory" property is not populated, so it justs acts on nothing an generates bogus log lines such as:

[modules/statistics/AudioOutputProblemDetector.js] A potential problem is detected with the audio output for participant b5fb30bc, local audio levels: [null,null], remote audio levels: undefined

Since nobody seems to have noticed in 3 years it's safe to assume we don't need this at all, so it gets the axe treatment.

It has been broken for over 3 years now, since jitsi@ca325f5#diff-9e19da30f465ca5665ac3d7ca1aa03d0498aed1be0cb2d7eeb27684a2636da77

Ever since that change, the "audioLevelReportHistory" property is not
populated, so it justs acts on nothing an generates bogus log lines such
as:

```
[modules/statistics/AudioOutputProblemDetector.js] A potential problem is detected with the audio output for participant b5fb30bc, local audio levels: [null,null], remote audio levels: undefined
```

Since nobody seems to have noticed in 3 years it's safe to assume we
don't need this at all, so it gets the axe treatment.
@jallamsetty1
Copy link
Member

Good catch @saghul! We have moved away from getting audio levels from getStats to using getSynchronizationSources on the receiver since getStats is very expensive and we do that for the only the top 5 active speakers. We can fix this by populating audioLevelReportHistory for the top 5 active speakers but since its going to be only for a subset of remote SSRCs it doesn't help much.

@saghul saghul merged commit da1d068 into jitsi:master Aug 8, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants