webrtc-internals: sort stats by encodingIndex and type [chromium/src : main]
Philipp Hancke (Gerrit)
Philipp Hancke added 1 comment![]( "Open in Gerrit")
Related details
- Henrik Boström
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Henrik Boström (Gerrit)
Henrik Boström added 1 comment![]( "Open in Gerrit")
}));Omg, so little code... TL;DR you need to avoid NaN otherwise I think this will be good.
But `stats1.encodingIndex - stats2.encodingIndex` can be `-2` or `NaN` while `localeCompare` in the range `-1` to `1`. I feel like things could get wrong here?
For example if we want the order:
- All outbound-rtps, in encodingIndex order
- All inbound-rtps
- Everyhing else
It's a problem if the inbound-rtp and foo stats produced NaN because the encodingIndex part of the expression is `NaN` and `NaN + localeCompare` is `NaN` rather than -1 or 1.
It's also a problem if one outbound-rtp that doesn't have encodingIndex (e.g. audio) and one outbound-rtp that does have an encodingIndex is, again, NaN. This comparison needs to be either -1 or 1 to avoid us "mixing" singlecast and simulcast streams e.g.
- encodingIndex 0
- random audio outbound RTP
- encodingIndex 1
- etc
I think this comparison needs to be robust enough that the first part of the expression is never NaN, and in this case the expression should be entirely evaluated based on whether or not the types are matching.
Related details
- Harald Alvestrand
- Philipp Hancke
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philipp Hancke (Gerrit)
Philipp Hancke added 1 comment![]( "Open in Gerrit")
Omg, so little code... TL;DR you need to avoid NaN otherwise I think this will be good.
But `stats1.encodingIndex - stats2.encodingIndex` can be `-2` or `NaN` while `localeCompare` in the range `-1` to `1`. I feel like things could get wrong here?
For example if we want the order:
- All outbound-rtps, in encodingIndex order
- All inbound-rtps
- Everyhing else
It's a problem if the inbound-rtp and foo stats produced NaN because the encodingIndex part of the expression is `NaN` and `NaN + localeCompare` is `NaN` rather than -1 or 1.
It's also a problem if one outbound-rtp that doesn't have encodingIndex (e.g. audio) and one outbound-rtp that does have an encodingIndex is, again, NaN. This comparison needs to be either -1 or 1 to avoid us "mixing" singlecast and simulcast streams e.g.
- encodingIndex 0
- random audio outbound RTP
- encodingIndex 1
- etc
I think this comparison needs to be robust enough that the first part of the expression is never NaN, and in this case the expression should be entirely evaluated based on whether or not the types are matching.
the order very much depends on your personal preferences. I care a lot more about ICE than RTP, why would you get the front row?
Lets sort by encodingIndex if available, type otherwise. Quoting MDN, remember that "(a, b) => a - b sorts numbers in ascending order"
Related details
- Harald Alvestrand
- Henrik Boström
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Henrik Boström (Gerrit)
Henrik Boström voted and added 1 comment![]( "Open in Gerrit")
Votes added by Henrik Boström
| Code-Review | +1 |
1 comment
Philipp HanckeOmg, so little code... TL;DR you need to avoid NaN otherwise I think this will be good.
But `stats1.encodingIndex - stats2.encodingIndex` can be `-2` or `NaN` while `localeCompare` in the range `-1` to `1`. I feel like things could get wrong here?
For example if we want the order:
- All outbound-rtps, in encodingIndex order
- All inbound-rtps
- Everyhing else
It's a problem if the inbound-rtp and foo stats produced NaN because the encodingIndex part of the expression is `NaN` and `NaN + localeCompare` is `NaN` rather than -1 or 1.
It's also a problem if one outbound-rtp that doesn't have encodingIndex (e.g. audio) and one outbound-rtp that does have an encodingIndex is, again, NaN. This comparison needs to be either -1 or 1 to avoid us "mixing" singlecast and simulcast streams e.g.
- encodingIndex 0
- random audio outbound RTP
- encodingIndex 1
- etc
I think this comparison needs to be robust enough that the first part of the expression is never NaN, and in this case the expression should be entirely evaluated based on whether or not the types are matching.
the order very much depends on your personal preferences. I care a lot more about ICE than RTP, why would you get the front row?
Lets sort by encodingIndex if available, type otherwise. Quoting MDN, remember that "(a, b) => a - b sorts numbers in ascending order"
That's good enough for me.
Then the type sorting will result in:
```
"candidate-pair",
"certificate",
"codec",
"data-channel",
"inbound-rtp",
"local-candidate",
"media-playout",
"media-source",
"outbound-rtp",
"peer-connection",
"remote-candidate",
"remote-inbound-rtp",
"remote-outbound-rtp",
"transport"
```
I don't think it's the best sorting but it's also not the worst, as long as they're not spread out all over the place and tend to show up in roughly the same place I'm happy. Ship it.
Related details
- Harald Alvestrand
- Philipp Hancke
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Harald Alvestrand (Gerrit)
Harald Alvestrand added 2 comments![]( "Open in Gerrit")
I don't think you should undo hbos' sort.....
// increasing order.suggest keeping sortStatsReport() as a function, since I suspect that we'll want to tweak this many more times. And don't undo hbos' sort (outbound < inbound < other).
Related details
- Philipp Hancke
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Henrik Boström (Gerrit)
Henrik Boström voted and added 1 comment![]( "Open in Gerrit")
Votes added by Henrik Boström
| Code-Review | +0 |
1 comment
// increasing order.suggest keeping sortStatsReport() as a function, since I suspect that we'll want to tweak this many more times. And don't undo hbos' sort (outbound < inbound < other).
Instead of type localeCompare we can do statsTypeToSortPriority (returning an int) and use that here such that this remains compact
Related details
- Philipp Hancke
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philipp Hancke (Gerrit)
Philipp Hancke added 1 comment![]( "Open in Gerrit")
// increasing order.Henrik Boströmsuggest keeping sortStatsReport() as a function, since I suspect that we'll want to tweak this many more times. And don't undo hbos' sort (outbound < inbound < other).
Instead of type localeCompare we can do statsTypeToSortPriority (returning an int) and use that here such that this remains compact
Moved to a function for easier changes. Problem is we are never going to agree on the "right" order... most logical would actually be by creation timestamp but we don't have that.
Related details
- Harald Alvestrand
- Henrik Boström
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Henrik Boström (Gerrit)
Henrik Boström added 1 comment![]( "Open in Gerrit")
// increasing order.Henrik Boströmsuggest keeping sortStatsReport() as a function, since I suspect that we'll want to tweak this many more times. And don't undo hbos' sort (outbound < inbound < other).
Philipp HanckeInstead of type localeCompare we can do statsTypeToSortPriority (returning an int) and use that here such that this remains compact
Moved to a function for easier changes. Problem is we are never going to agree on the "right" order... most logical would actually be by creation timestamp but we don't have that.
Order is subjective but I don't understand why you're so resisting to the idea of putting the RTP objects first, what's the harm in doing that?
Related details
- Harald Alvestrand
- Philipp Hancke
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philipp Hancke (Gerrit)
Philipp Hancke added 1 comment![]( "Open in Gerrit")
// increasing order.Henrik Boströmsuggest keeping sortStatsReport() as a function, since I suspect that we'll want to tweak this many more times. And don't undo hbos' sort (outbound < inbound < other).
Philipp HanckeInstead of type localeCompare we can do statsTypeToSortPriority (returning an int) and use that here such that this remains compact
Henrik BoströmMoved to a function for easier changes. Problem is we are never going to agree on the "right" order... most logical would actually be by creation timestamp but we don't have that.
Order is subjective but I don't understand why you're so resisting to the idea of putting the RTP objects first, what's the harm in doing that?
if you only want to look at RTP filter for -rtp using the filter box?
There are other nice stats too!
Related details
- Henrik Boström
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |