Fix connection quality warning shown due to stalled stats#5885
Merged
Fix connection quality warning shown due to stalled stats#5885
Conversation
danxuliu
added
3. to review
bug
feature: frontend 🖌️
Member
Author
|
/backport to stable22 |
Member
Author
|
/backport to stable21 |
Member
|
the base PR was merged, time to rebase ? |
The stats were reset whenever "setAnalysisEnabledXXX(true)" was called. Due to this, if the analysis for audio or video was already enabled and the method was called again the stats were wrongly reset. The stats should be reset only when the analysis is really started again. Signed-off-by: Daniel Calviño Sánchez <[email protected]>
This makes the attribute consistent with the stats, and will also make
possible to directly get the quality value given a string identifying
its kind ("audio" or "video").
Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Signed-off-by: Daniel Calviño Sánchez <[email protected]>
When no packets are transmitted the packet lost ratio is set to a value higher than 1 to move towards a "no transmitted data" state faster, although not immediately. Both the "no transmitted data" and "very bad quality" states are based on the packet lost ratio, so this causes the "very bad quality" state to be triggered as soon as no packets are transmitted. However, the stats reported by the browser can sometimes stall for a second (it is unclear whether the browser does not update the stats or Janus does not send them, though). When that happens the stats are still reported, but with the same values as in the previous report. As there were no transmitted packets the "very bad quality" state was (wrongly) triggered. To solve this now if there were no transmitted packets in the last report the analysis is kept on hold. If in the next report the number of packets has changed then the previous report is considered to have stalled and the new values are distributed between the previous and current report. If the number of packets is still zero then the previous report is considered to have been legit and the previous and current values are added as normal. Signed-off-by: Daniel Calviño Sánchez <[email protected]>
danxuliu
force-pushed
the
fix-connection-quality-warning-shown-due-to-stalled-stats
branch
from
76293d8 to
aadfd0e
Compare
Member
Author
Done. |
PVince81
approved these changes
Jul 1, 2021
Member
PVince81
left a comment
PVince81
left a comment
There was a problem hiding this comment.
👍 code looks fine.
This is becoming quite complex, feels like adding some JS tests is overdue...
Member
Author
Totally. Feel free to add them. |
PVince81
deleted the
fix-connection-quality-warning-shown-due-to-stalled-stats
branch
|
The backport to stable22 failed. Please do this backport manually. |
|
The backport to stable21 failed. Please do this backport manually. |
Member
|
need to wait for #5911 before backporting |
Member
|
/backport to stable22 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Requires #5884
(kept as draft until that is merged, but ready for review nevertheless)The stats reported by the browser can sometimes stall for a second (it is unclear whether the browser does not update the stats or Janus does not send them, though). When that happens the stats are still reported, but with the same values as in the previous report. As there were no transmitted packets the "very bad quality" state was (wrongly) triggered.
To solve this now if there were no transmitted packets in the last report the analysis is kept on hold. If in the next report the number of packets has changed then the previous report is considered to have stalled and the new values are distributed between the previous and current report. If the number of packets is still zero then the previous report is considered to have been legit and the previous and current values are added as normal.
Besides the main fix this pull request also fixes the stats being wrongly reset when calling "setAnalysisEnabledXXX(true)" twice. This happens whenever the audio or video is enabled while being already enabled. This happens, for example, when another participant joins and the current media state is sent; whether that should happen or not is a different matter, but in any case, calling "setAnalysisEnabledXXX(true)" twice should not reset the stats.
How to test
_addStats())Result with this pull request
Eventually the logs will show that the stats were stalled (zero transmitted packets in a single report), but there will be no logs about the connection quality warning
Result with this pull request
Eventually the logs will show that the stats were stalled (zero transmitted packets in a single report) and there will be logs about the connection quality warning (high lost packet ratio, ~0.45 and then ~0.375, the lost packet ratio values will be similar to [0, 0, 0, 0, 1.5] and then [0, 0, 0, 1.5, 0])