-
Notifications
You must be signed in to change notification settings - Fork 635
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Connection quality #1490
Connection quality #1490
Conversation
With score normalization, the quality indicator showed good under conditions which should have normally showed some badness. So, a few things in this PR - Do not normalize scores - Pick the weakest link as the representative score (moving away from averaging) - For down track direction, when reporting delta stats, take the number of packets sent actually. If there are holes in the feed (upstream packet loss), down tracks should not be penalised for that loss. State of things in connection quality feature - Audio uses rtcscore-go (with a change to accommodate RED codec). This follows the E-model. - Camera uses rtcscore-go. No change here. NOTE: THe rtscore here is purely based on bits per pixel per frame (bpf). This has the following existing issues (no change, these were already there) o Does not take packet loss, jitter, rtt into account o Expected frame rate is not available. So, measured frame rate is used as expected frame rate also. If expected frame rate were available, the score could be reduced for lower frame rates. - Screen share tracks: No change. This uses the very old simple loss based thresholding for scoring. As the bit rate varies a lot based on content and rtcscore video algorithm used for camera relies on bits per pixel per frame, this could produce a very low value (large width/height encoded in a small number of bits because of static content) and hence a low score. So, the old loss based thresholding is used.
DynacastPauseDelay: d.params.DynacastPauseDelay, | ||
Logger: d.params.Logger, | ||
MimeType: mime, | ||
Logger: d.params.Logger, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unrelated clean up. Just roaming the code and noticed.
} | ||
return 0, 0 | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing unused stuff
return | ||
} | ||
|
||
// take median of scores in a longer window to prevent quality reporting oscillations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the hysteresis. I am not too happy about responsiveness of this. Have set it to 25 seconds of wait if quality drops to POOR
, i. e. we will take median quality 25 seconds after POOR
starts. And that wait is 15 seconds for GOOD
. So, if things are transitioning from POOR
-> GOOD
-> EXCELLENT
, this could take 40 seconds. That might be fine, but there should be better ways. Let me know if you can think of something.
Reason for 25 and 15 seconds is that the analysis window is 5 seconds and wanted to have enough windows to take median.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems fine to me especially for first pass. if it feels too slow reacting, we can come back to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this flapped too much the ui updates might be distracting so having a bit of delay might be better user experience
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, that's the goal. Just don't want it to be too slow 馃槃
@@ -264,62 +264,6 @@ func (s *StreamTrackerManager) SetMaxExpectedSpatialLayer(layer int32) int32 { | |||
return prev | |||
} | |||
|
|||
func (s *StreamTrackerManager) DistanceToDesired() int32 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cleaning up unused stuff.
for _, subTrack := range subscribedTracks { | ||
if subTrack.IsMuted() || subTrack.MediaTrack().IsMuted() { | ||
continue | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the muted check from here. The quality scorer is notified of mute transitions and reports quality accordingly (i. e. when muted EXCELLENT
is reported). So, leaving it completely to quality scorer to report based on its current state.
select { | ||
case <-cs.done: | ||
<-tk.C |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is super minor but we should standardize on using selects. i'll fix the place i did this in room the other day. there's no reason to wait for the next tick to exit these loops.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be great to not wait. Please let me know how to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the way you had it before was good
select {
case <-cs.done:
return
case <-ticker.C:
// work...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool, will change it back. Was trying to get rid of one field in the struct 馃槃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, we could use dc's fuse if you want something more ergonomic. i guess this doesn't avoid the struct field though :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm 馃憤
// | ||
// For video: | ||
// o No in-built codec repair available, hence same for all codecs | ||
func getPacketLossWeight(mimeType string, isFecEnabled bool) float64 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how were these constants picked?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empirical. There is the e-model, an ITU-T standard, but not tuned for modern codecs. rtcscore was an attempt at a model, but that is a bit complex and also does not consider packet loss for video codecs. So, this is a result of filtering a bunch of those inputs/ signals into something simple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah i assumed as much i was just asking out of curiosity. red is like 2 or 3x redundant broadcasts? and fec is some sort of error correcting code. do the qualities represent some probability of having undecodable segments of some length within a sample?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- rtcscore: https://github.com/ggarber/rtcscore
- Shishir ported that to Go: https://github.com/livekit/rtcscore-go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah i assumed as much i was just asking out of curiosity. red is like 2 or 3x redundant broadcasts? and fec is some sort of error correcting code. do the qualities represent some probability of having undecodable segments of some length within a sample?
Yes, these are representative. Ideally, we would get data from the edge about the following
- how many packets/samples had to be concealed.
- how many were recovered using FEC.
But, those are harder to do. For example, an audio track could get forwarded to multiple subscribers. Each subscriber could behave differently. To get purely upstream quality, SFU would have to try to decode (including a jitter buffer) to really figure out what is the quality of the upstream track.
RED used to be 3x (every packet includes 2 previous packets), but I thought I saw 2 packets recently. Not sure if something has changed. If it is 3, we can lose two packets out of every three and still be able to decode 100%. But, losses are bursty and quality is not great beyond 10% empirically (from user reports). We can measure loss characteristic (isolated vs bursty) and apply a better weight based on pattern in the analysis window. But, that is more compute. We do measure that for the whole stream, but been avoiding doing that in analysis windows as it is per packet processing.
Opus has in-built FEC. It adds FEC bits based on loss. Based on loss reported in RTCP Receiver Report, Opus will add FEC bits if negotiated. Again, it does not do great against bursty losses, but does quite well when having isolated losses (https://opus-codec.org/examples/).
So, this combination of numbers is a filtering of all the factors and is a balance between available data/available knowledge on this matter (which I am probably privy to only 1% or less)/implementation considerations.
- track no layer expected case - always update transition - always call updateScore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome work!!
return | ||
} | ||
|
||
// take median of scores in a longer window to prevent quality reporting oscillations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems fine to me especially for first pass. if it feels too slow reacting, we can come back to it.
An attempt to redo connection quality so that it is not too optimistic.
@davidzhao @cnderrauber @paulwe There is still one bit missing, but wanted to get this out to get 馃憖 and feedback. I will look at how to wire up the missing bit. Will also leave inline notes.
Also, requires a bunch more testing. And I am guessing there will be some iterations needed 馃槃 .
Overview
qualityScorer
module.Commentary on bit rate based quality evaluation:
The idea behind bit rate based measurement is that the sender is expected to stream one or more layers at some expected bit rate. For example, with up track, the expected bit rate is sum of all layer bit rates till max expected layer. Then the actual bitrate (across all layers till max expected layer) is an indicator of publisher's ability to satisfy that expectation. For example, if a layer cannot be published, actual bit rate will be lower compared to expected bit rate and that will drop quality. So, using aggregate bit rate as an indicator of publisher's ability to publish at expected/advertised bitrate.
The idea behind this is to make it work well for simulcast track/SVC tracks with multiple layers, but also for the down track which forward only certain layers. Basically, by recording expected bitrate, common method can be applied irrespective of track type.
The missing bit:
Have not hooked up bitrate transition for down track. With down track, need to propagate ideal layer bit rate as transitions into the scorer. This is probably quite a few lines of code change (including in test). So, postponing that for now. For now, down tracks will purely use packet loss based quality measurement.